On Wed, Nov 19, 2025 at 11:08:01AM +0100, Petr Mladek wrote:
On Thu 2025-11-13 15:32:33, Andy Shevchenko wrote:
Use %ptSp instead of open coded variants to print content of struct timespec64 in human readable format.
I was about to commit the changes into printk/linux.git and found a mistake during the final double check, see below.
diff --git a/drivers/scsi/fnic/fnic_trace.c b/drivers/scsi/fnic/fnic_trace.c index cdc6b12b1ec2..0a849a195a8e 100644 --- a/drivers/scsi/fnic/fnic_trace.c +++ b/drivers/scsi/fnic/fnic_trace.c @@ -215,30 +213,26 @@ int fnic_get_stats_data(struct stats_debug_info *debug, { int len = 0; int buf_size = debug->buf_size;
- struct timespec64 val1, val2;
- struct timespec64 val, val1, val2; int i = 0;
- ktime_get_real_ts64(&val1);
- ktime_get_real_ts64(&val); len = scnprintf(debug->debug_buffer + len, buf_size - len, "------------------------------------------\n" "\t\tTime\n" "------------------------------------------\n");
- val1 = timespec64_sub(val, stats->stats_timestamps.last_reset_time);
- val2 = timespec64_sub(val, stats->stats_timestamps.last_read_time); len += scnprintf(debug->debug_buffer + len, buf_size - len,
"Current time : [%lld:%ld]\n""Last stats reset time: [%lld:%09ld]\n""Last stats read time: [%lld:%ld]\n""delta since last reset: [%lld:%ld]\n""delta since last read: [%lld:%ld]\n",- (s64)val1.tv_sec, val1.tv_nsec,
- (s64)stats->stats_timestamps.last_reset_time.tv_sec,
- stats->stats_timestamps.last_reset_time.tv_nsec,
- (s64)stats->stats_timestamps.last_read_time.tv_sec,
- stats->stats_timestamps.last_read_time.tv_nsec,
- (s64)timespec64_sub(val1, stats->stats_timestamps.last_reset_time).tv_sec,
- timespec64_sub(val1, stats->stats_timestamps.last_reset_time).tv_nsec,
- (s64)timespec64_sub(val1, stats->stats_timestamps.last_read_time).tv_sec,
- timespec64_sub(val1, stats->stats_timestamps.last_read_time).tv_nsec);
"Current time : [%ptSp]\n""Last stats reset time: [%ptSp]\n""Last stats read time: [%ptSp]\n""delta since last reset: [%ptSp]\n""delta since last read: [%ptSp]\n",Both delta times are printed at the end.
&val,&stats->stats_timestamps.last_reset_time, &val1,&stats->stats_timestamps.last_read_time, &val2);I think that this should be:
&stats->stats_timestamps.last_reset_time, &stats->stats_timestamps.last_read_time, &val1, &val2);stats->stats_timestamps.last_read_time = val1;
The original code stored the current time in "val1". This should be:
stats->stats_timestamps.last_read_time = val;
@@ -416,8 +410,8 @@ int fnic_get_stats_data(struct stats_debug_info *debug, jiffies_to_timespec64(stats->misc_stats.last_ack_time, &val2);
Just for record. Another values are stored into @val1 and @val2 at this point.
len += scnprintf(debug->debug_buffer + len, buf_size - len,
"Last ISR time: %llu (%8llu.%09lu)\n""Last ACK time: %llu (%8llu.%09lu)\n"
"Last ISR time: %llu (%ptSp)\n""Last ACK time: %llu (%ptSp)\n"@@ -437,10 +431,8 @@ int fnic_get_stats_data(struct stats_debug_info *debug, "Number of rport not ready: %lld\n" "Number of receive frame errors: %lld\n" "Port speed (in Mbps): %lld\n",
(u64)stats->misc_stats.last_isr_time,(s64)val1.tv_sec, val1.tv_nsec,(u64)stats->misc_stats.last_ack_time,(s64)val2.tv_sec, val2.tv_nsec,
(u64)stats->misc_stats.last_isr_time, &val1,(u64)stats->misc_stats.last_ack_time, &val2,So, this is correct!
(u64)atomic64_read(&stats->misc_stats.max_isr_jiffies), (u64)atomic64_read(&stats->misc_stats.max_isr_time_ms), (u64)atomic64_read(&stats->misc_stats.corr_work_done),Now, I think that there is no need to resend the entire huge patchset.
I could either fix this when comitting or commit the rest and you could send only this patch for review.
Thank you for the thoroughly done review, I changed that patch between the versions and the problem is that for printf() specifiers (extensions) we do not have an automatic type checking. We starve for a GCC plugin for that, yeah...
In any case, if you fold your changes in, I will appreciate that! Otherwise it's also fine with me to send a patch separately later on.
PS: All other patches look good. Well, nobody acked 7th patch yet. But I think that the change is pretty straightforward and we could do it even without an ack.
This is my understanding as well. It changes the output, but that output is debug anyway. So I don't expect breakage of anything we have an obligation to keep working.