Re: [PATCH v3 2/7] drm: Add drm_timeout_rel_to_jiffies()

["Maíra Canal" <mcanal@igalia.com>]

Hi Thomas,

Thank you for your review!

On 28/09/26 13:37, Thomas Zimmermann wrote:

[...]

>> +unsigned long drm_timeout_rel_to_jiffies(u64 timeout_nsec)
>> +{
>> +    u64 secs;
>> +    u32 rem;
>> +
>> +    /* Make 0 timeout means poll, as for the absolute variant. */
>> +    if (timeout_nsec == 0)
>> +        return 0;
>> +
>> +    /*
>> +     * As nsecs_to_jiffies64() does not guard against overflow, split
>> +     * the timeout into whole seconds and nanoseconds. This way
>> +     * nsecs_to_jiffies64() is always handed a value below a second.
>> +     */
>> +    secs = div_u64_rem(timeout_nsec, NSEC_PER_SEC, &rem);
>> +    if (secs >= MAX_JIFFY_OFFSET / HZ)
>> +        return MAX_JIFFY_OFFSET;
>> +
>> +    return min_t(u64, MAX_JIFFY_OFFSET,
>> +             secs_to_jiffies(secs) + nsecs_to_jiffies64(rem) + 1);
> 
> The timeout is controled by user space, right? Can these additions 
> overflow?  I see that secs is tested against MAX_JIFFY_OFFSET, but is 
> that sufficient?

Although the timeout is controlled by user-space, the additions cannot
overflow here. The secs test bounds the multiplication, which is why it
is written as secs >= MAX_JIFFY_OFFSET / HZ rather than secs * HZ >=
MAX_JIFFY_OFFSET (which could overflow).

Past that check, we know that the remaining is smaller than one second
and therefore, nsecs_to_jiffies64(rem) <= HZ - 1. So, the sum + 1 is at
most MAX_JIFFY_OFFSET.

> 
> BTW there was this NSEC % HZ test in the original code? What was it good 
> for? It is no longer useful?
> 

I believe that NSEC_PER_SEC % HZ was only useful to check if it was a
plain division, which wouldn't overflow. With the current approach, I
believe it's no longer needed.

Best regards,
- Maíra
> Best regards
> Thomas
> 
> 
>> +}
>> +EXPORT_SYMBOL(drm_timeout_rel_to_jiffies);
>> diff --git a/include/drm/drm_timeout.h b/include/drm/drm_timeout.h
>> index cd9621c52062..6ee222a3e97c 100644
>> --- a/include/drm/drm_timeout.h
>> +++ b/include/drm/drm_timeout.h
>> @@ -12,5 +12,6 @@
>>   #include <linux/types.h>
>>   signed long drm_timeout_abs_to_jiffies(s64 timeout_nsec);
>> +unsigned long drm_timeout_rel_to_jiffies(u64 timeout_nsec);
>>   #endif
>>
>