| Message ID | 20241111163306.860-1-m.masimov@maxima.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm/mgag200: Apply upper limit for clock variable | expand |
On 11/11/2024 17:33, Murad Masimov wrote: > If the value of the clock variable is higher than 800000, the value of the > variable m, which is used as a divisor, will remain zero, because > (clock * testp) will be higher than vcomax in every loop iteration, which > leads to skipping every iteration and leaving variable m unmodified. > > Clamp value of the clock variable between the lower and the upper limits. > It should be correct, since there is already a similar lower limit check. I don't think it is correct. If the clock asked is > 800000, then delta > premitteddelta, and it will return -EINVAL. With your patch it will instead configure the clock to 800000 which is too low for the mode asked and will result in corrupted output. Best regards,
On Thu, 14. Nov 17:47, Jocelyn Falempe wrote: > On 11/11/2024 17:33, Murad Masimov wrote: > > If the value of the clock variable is higher than 800000, the value of the > > variable m, which is used as a divisor, will remain zero, because > > (clock * testp) will be higher than vcomax in every loop iteration, which > > leads to skipping every iteration and leaving variable m unmodified. > > > > Clamp value of the clock variable between the lower and the upper limits. > > It should be correct, since there is already a similar lower limit check. > > I don't think it is correct. > > If the clock asked is > 800000, then delta > premitteddelta, and it will > return -EINVAL. In many cases when clock is > 800000, the check won't be reached as the division by "m" variable containing a zero value will have occured just before. > With your patch it will instead configure the clock to 800000 which is too > low for the mode asked and will result in corrupted output. Worth moving the check just after the loop or e.g. explicitly denying clocks > 800000 at the beginning of the function?
On 14/11/2024 19:05, Fedor Pchelkin wrote: > On Thu, 14. Nov 17:47, Jocelyn Falempe wrote: >> On 11/11/2024 17:33, Murad Masimov wrote: >>> If the value of the clock variable is higher than 800000, the value of the >>> variable m, which is used as a divisor, will remain zero, because >>> (clock * testp) will be higher than vcomax in every loop iteration, which >>> leads to skipping every iteration and leaving variable m unmodified. >>> >>> Clamp value of the clock variable between the lower and the upper limits. >>> It should be correct, since there is already a similar lower limit check. >> >> I don't think it is correct. >> >> If the clock asked is > 800000, then delta > premitteddelta, and it will >> return -EINVAL. > > In many cases when clock is > 800000, the check won't be reached as the > division by "m" variable containing a zero value will have occured just > before. > >> With your patch it will instead configure the clock to 800000 which is too >> low for the mode asked and will result in corrupted output. > > Worth moving the check just after the loop or e.g. explicitly denying > clocks > 800000 at the beginning of the function? > I think it's better to check just after the for loop, if m is still 0, just return -EINVAL.
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c index 7a32d3b1d226..4934c27b084e 100644 --- a/drivers/gpu/drm/mgag200/mgag200_g200se.c +++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c @@ -216,8 +216,7 @@ static int mgag200_g200se_04_pixpllc_atomic_check(struct drm_crtc *crtc, m = n = p = s = 0; delta = 0xffffffff; - if (clock < 25000) - clock = 25000; + clock = clamp(clock, 25000L, 800000L); clock = clock * 2; /* Permited delta is 0.5% as VESA Specification */
If the value of the clock variable is higher than 800000, the value of the variable m, which is used as a divisor, will remain zero, because (clock * testp) will be higher than vcomax in every loop iteration, which leads to skipping every iteration and leaving variable m unmodified. Clamp value of the clock variable between the lower and the upper limits. It should be correct, since there is already a similar lower limit check. Found by Linux Verification Center (linuxtesting.org) with SVACE. Fixes: e829d7ef9f17 ("drm/mgag200: Add support for a new rev of G200e") Signed-off-by: Murad Masimov <m.masimov@maxima.ru> --- drivers/gpu/drm/mgag200/mgag200_g200se.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) -- 2.39.2