[PATCH] drm/i915/gt: Ensure sleep calculations in wait_for_freq() do not use negative value of timeout_ms

[Jani Nikula]

On Tue, 05 Aug 2025, Krzysztof Karas <krzysztof.karas at intel.com> wrote:
>> On Tue, 05 Aug 2025, Krzysztof Karas <krzysztof.karas at intel.com> wrote:
>> > wait_for_freq() allows timeout_ms to be negative to pass it down
>> > to msecs_to_jiffies(). If the argument to the latter function is
>> > indeed negative, then MAX_JIFFY_OFFSET is returned from it.
>> >
>> > However, later in the wait_for_freq()'s "do while" loop,
>> > "timeout_ms" is used to calculate a new value for "sleep", which
>> > is a plain integer. While "sleep" being negative does not lead
>> > to erroneous behavior, as the "sleep" is cast to unsigned type
>> > in usleep_range(), it does not seem intentional.
>> >
>> > Change the type of "sleep" variable to unsigned and ensure it
>> > does not use "timeout_ms", when it is a negative value in the
>> > calculations.
>> 
>> That's an elaborate explanation for a case that never
>> occurs. wait_for_freq() is used in three places with fixed timeout
>> values.
> Which means that whoever decides to pass a negative value here
> in the future will be met with an unpleasant surprise when
> looking at sleep values, because we never validate the input to
> this function.

Honestly, if we started doing that for even the simplest stuff like
this, the driver would mostly consist of parameter validation and error
handling and propagation.

Input validation does not mean we'd have to validate all input
originating within the kernel.

It's better to fail early with that unpleasant surprise than to brush
the issue under the carpet.

>> What's the real reason for the change?
> Do you imply something was obscured?

No, just wondering how you stumbled on it and decided it really needed
to be changed.

> This patch is supposed to try and fix mishandling of int type
> variable, which represents time to sleep. Time should not be
> represented by a negative value, unless we are going back in
> time. Hence, the change to unsigned and additional check on
> timeout_ms.

Unfortunately, unsigned doesn't really protect you from mistakes like
this. Stuff just ends up being really, really big instead of negative.

BR,
Jani.

-- 
Jani Nikula, Intel