On Thu, Nov 5, 2015 at 2:44 PM, Arnd Bergmann <[email protected]> wrote:
> On Thursday 05 November 2015 11:13:30 Amitoj Kaur Chawla wrote:
>> On Thu, Nov 5, 2015 at 5:19 AM, Arnd Bergmann <[email protected]> wrote:
>> > On Wednesday 04 November 2015 23:51:07 Amitoj Kaur Chawla wrote:
>> >> 32 bit systems using 'struct timeval' will break in the year 2038, so
>> >> we modify the code appropriately.
>> >>
>> >> This patch replaces the use of struct timeval and do_gettimeofday()
>> >> with 64 bit ktime_get_seconds() since we only need to find elapsed
>> >> seconds.
>> >>
>> >> stats_reset_time variable has been converted from a u32 to u64 type to
>> >> store the 64 bit value returned by ktime_get_seconds().
>> >>
>> >> Signed-off-by: Amitoj Kaur Chawla <[email protected]>
>> >
>> > The changelog looks good, but you seem to have introduced a new
>> > bug:
>> >
>> >> @@ -3347,9 +3345,8 @@ __bfa_cb_fcport_stats_get(void *cbarg,
>> >> bfa_boolean_t complete)
>> >> union bfa_fcport_stats_u *ret;
>> >>
>> >> if (complete) {
>> >> - struct timeval tv;
>> >> + u64 timestamp = ktime_get_seconds();
>> >> if (fcport->stats_status == BFA_STATUS_OK)
>> >> - do_gettimeofday(&tv);
>> >>
>> >> list_for_each_safe(qe, qen, &fcport->stats_pending_q) {
>> >> bfa_q_deq(&fcport->stats_pending_q, &qe);
>> >
>> > The 'if' condition now applies to the list_for_each_safe(), so the
>> > behavior of the code changes significantly.
>> >
>>
>> I'll remove the if condition totally since it doesn't make sense
>> anymore to keep it.
>
> That works, but why not keep the code as it is here? ktime_get_seconds()
> still adds a little overhead if we call it unconditionally, and the
> author of the original code apparently felt that it was worth optimizing
> for.
>
Oh okay, I sent a v2 thinking that the removal of if condition would
work. But since you're saying that the optimising seems necessary.
I'll change it to
u64 timestamp;
if (fcport->stats_status == BFA_STATUS_OK)
timestamp = ktime_get_seconds();
Is this fine?
>> >> diff --git a/drivers/scsi/bfa/bfa_svc.h b/drivers/scsi/bfa/bfa_svc.h
>> >> index ef07365..0c25ee3 100644
>> >> --- a/drivers/scsi/bfa/bfa_svc.h
>> >> +++ b/drivers/scsi/bfa/bfa_svc.h
>> >> @@ -504,7 +504,7 @@ struct bfa_fcport_s {
>> >> struct list_head stats_pending_q;
>> >> struct list_head statsclr_pending_q;
>> >> bfa_boolean_t stats_qfull;
>> >> - u32 stats_reset_time; /* stats reset time stamp */
>> >> + u64 stats_reset_time; /* stats reset time stamp */
>> >> bfa_boolean_t diag_busy; /* diag busy status */
>> >> bfa_boolean_t beacon; /* port beacon status */
>> >> bfa_boolean_t link_e2e_beacon; /* link beacon status */
>> >
>> > Let's talk about types here: u64 is technically correct, but the
>> > return type of ktime_get_seconds() is 'time64_t', so if you want to
>> > stay with 64 bits just use that.
>> >
>>
>> I'll use time64_t next time.
>>
>> > Leaving the type as u32 is also fine as I explained earlier because
>> > monotonic times do not overflow 32-bit variables in practice.
>> >
>> > Arnd
>>
>>
>> Oh okay, I'll leave stats_reset_time as is.
>
> As with every patch, it doesn't matter so much which one of those two
> (or possibly 'unsigned long' if you like) you pick here, as long as
> it's clear from the changelog that you have thought about it and made
> a decision one way or another. This means if someone disagrees with your
> choice, they just need to reply to your changelog instead of having to
> understand all the details.
>
> Arnd
I'll also mention in the changelog in v3 that the stats_reset_time
variable was not changed to 64 bit type since this is monotonic time.
Works?
Thanks,
--
Amitoj
_______________________________________________
Y2038 mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/y2038
