fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

[PATCH] fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

Posted by Thorsten Blum 2 weeks, 3 days ago

The variables were never clamped because the return value of clamp_val()
was not used. Fix this by assigning the clamped values, and use clamp()
instead of clamp_val().

Cc: stable@vger.kernel.org
Fixes: 3f16ff608a75 ("[ARM] pxafb: cleanup of the timing checking code")
Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
---
 drivers/video/fbdev/pxafb.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
index b96a8a96bce8..e418eee825fb 100644
--- a/drivers/video/fbdev/pxafb.c
+++ b/drivers/video/fbdev/pxafb.c
@@ -419,12 +419,12 @@ static int pxafb_adjust_timing(struct pxafb_info *fbi,
 	var->yres = max_t(int, var->yres, MIN_YRES);
 
 	if (!(fbi->lccr0 & LCCR0_LCDT)) {
-		clamp_val(var->hsync_len, 1, 64);
-		clamp_val(var->vsync_len, 1, 64);
-		clamp_val(var->left_margin,  1, 255);
-		clamp_val(var->right_margin, 1, 255);
-		clamp_val(var->upper_margin, 1, 255);
-		clamp_val(var->lower_margin, 1, 255);
+		var->hsync_len = clamp(var->hsync_len, 1, 64);
+		var->vsync_len = clamp(var->vsync_len, 1, 64);
+		var->left_margin  = clamp(var->left_margin,  1, 255);
+		var->right_margin = clamp(var->right_margin, 1, 255);
+		var->upper_margin = clamp(var->upper_margin, 1, 255);
+		var->lower_margin = clamp(var->lower_margin, 1, 255);
 	}
 
 	/* make sure each line is aligned on word boundary */
-- 
Thorsten Blum <thorsten.blum@linux.dev>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4

Re: [PATCH] fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

Posted by Helge Deller 2 weeks, 3 days ago

On 12/2/25 19:15, Thorsten Blum wrote:
> The variables were never clamped because the return value of clamp_val()
> was not used. Fix this by assigning the clamped values, and use clamp()
> instead of clamp_val().
> 
> Cc: stable@vger.kernel.org
> Fixes: 3f16ff608a75 ("[ARM] pxafb: cleanup of the timing checking code")
> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> ---
>   drivers/video/fbdev/pxafb.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)

Thanks for the patch!
It looks good, so I'll include it in the fbdev tree.
Out of curiosity:
How did you notice? Do you actually have the hardware and tested it?

Helge 
> diff --git a/drivers/video/fbdev/pxafb.c b/drivers/video/fbdev/pxafb.c
> index b96a8a96bce8..e418eee825fb 100644
> --- a/drivers/video/fbdev/pxafb.c
> +++ b/drivers/video/fbdev/pxafb.c
> @@ -419,12 +419,12 @@ static int pxafb_adjust_timing(struct pxafb_info *fbi,
>   	var->yres = max_t(int, var->yres, MIN_YRES);
>   
>   	if (!(fbi->lccr0 & LCCR0_LCDT)) {
> -		clamp_val(var->hsync_len, 1, 64);
> -		clamp_val(var->vsync_len, 1, 64);
> -		clamp_val(var->left_margin,  1, 255);
> -		clamp_val(var->right_margin, 1, 255);
> -		clamp_val(var->upper_margin, 1, 255);
> -		clamp_val(var->lower_margin, 1, 255);
> +		var->hsync_len = clamp(var->hsync_len, 1, 64);
> +		var->vsync_len = clamp(var->vsync_len, 1, 64);
> +		var->left_margin  = clamp(var->left_margin,  1, 255);
> +		var->right_margin = clamp(var->right_margin, 1, 255);
> +		var->upper_margin = clamp(var->upper_margin, 1, 255);
> +		var->lower_margin = clamp(var->lower_margin, 1, 255);
>   	}
>   
>   	/* make sure each line is aligned on word boundary */

Re: [PATCH] fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

Posted by Thorsten Blum 2 weeks, 3 days ago

On 2. Dec 2025, at 19:28, Helge Deller wrote:
> On 12/2/25 19:15, Thorsten Blum wrote:
>> The variables were never clamped because the return value of clamp_val()
>> was not used. Fix this by assigning the clamped values, and use clamp()
>> instead of clamp_val().
>> Cc: stable@vger.kernel.org
>> Fixes: 3f16ff608a75 ("[ARM] pxafb: cleanup of the timing checking code")
>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
>> ---
>>  drivers/video/fbdev/pxafb.c | 12 ++++++------
>>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> Thanks for the patch!
> It looks good, so I'll include it in the fbdev tree.
> Out of curiosity:
> How did you notice? Do you actually have the hardware and tested it?

I only compile-tested it.

I stumbled upon another driver with the same bug and then used grep to
search for other instances and found about 6 or 7, including this one.

Thanks,
Thorsten

Re: [PATCH] fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

Posted by David Laight 2 weeks, 3 days ago

On Tue, 2 Dec 2025 19:36:17 +0100
Thorsten Blum <thorsten.blum@linux.dev> wrote:

> On 2. Dec 2025, at 19:28, Helge Deller wrote:
> > On 12/2/25 19:15, Thorsten Blum wrote:  
> >> The variables were never clamped because the return value of clamp_val()
> >> was not used. Fix this by assigning the clamped values, and use clamp()
> >> instead of clamp_val().
> >> Cc: stable@vger.kernel.org
> >> Fixes: 3f16ff608a75 ("[ARM] pxafb: cleanup of the timing checking code")
> >> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
> >> ---
> >>  drivers/video/fbdev/pxafb.c | 12 ++++++------
> >>  1 file changed, 6 insertions(+), 6 deletions(-)  
> > 
> > Thanks for the patch!
> > It looks good, so I'll include it in the fbdev tree.
> > Out of curiosity:
> > How did you notice? Do you actually have the hardware and tested it?  
> 
> I only compile-tested it.
> 
> I stumbled upon another driver with the same bug and then used grep to
> search for other instances and found about 6 or 7, including this one.

I've just hacked minmax.h so clamp() and clamp_t() (and thus clamp_val())
are 'static inline u64 __must_check clamp(...)'.

Didn't find any in any other files.
But I think you missed some double error in the same file (nearby).

	David

> 
> Thanks,
> Thorsten
> 
>

Re: [PATCH] fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

Posted by Helge Deller 2 weeks, 3 days ago

On 12/2/25 19:36, Thorsten Blum wrote:
> On 2. Dec 2025, at 19:28, Helge Deller wrote:
>> On 12/2/25 19:15, Thorsten Blum wrote:
>>> The variables were never clamped because the return value of clamp_val()
>>> was not used. Fix this by assigning the clamped values, and use clamp()
>>> instead of clamp_val().
>>> Cc: stable@vger.kernel.org
>>> Fixes: 3f16ff608a75 ("[ARM] pxafb: cleanup of the timing checking code")
>>> Signed-off-by: Thorsten Blum <thorsten.blum@linux.dev>
>>> ---
>>>   drivers/video/fbdev/pxafb.c | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> Thanks for the patch!
>> It looks good, so I'll include it in the fbdev tree.
>> Out of curiosity:
>> How did you notice? Do you actually have the hardware and tested it?
> 
> I only compile-tested it.
> 
> I stumbled upon another driver with the same bug and then used grep to
> search for other instances and found about 6 or 7, including this one.

Ok. But this then means, maybe the clamping isn't needed (since nobody complained),
or that nobody noticed because nobody uses the driver any longer.
Anyway, I believe the patch is correct, so I leave it in.

Thank you!
Helge

Re: [PATCH] fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

Posted by Andy Shevchenko 2 weeks, 2 days ago

On Tue, Dec 02, 2025 at 08:36:08PM +0100, Helge Deller wrote:
> On 12/2/25 19:36, Thorsten Blum wrote:
> > On 2. Dec 2025, at 19:28, Helge Deller wrote:
> > > On 12/2/25 19:15, Thorsten Blum wrote:

...

> > > How did you notice? Do you actually have the hardware and tested it?
> > 
> > I only compile-tested it.
> > 
> > I stumbled upon another driver with the same bug and then used grep to
> > search for other instances and found about 6 or 7, including this one.
> 
> Ok. But this then means, maybe the clamping isn't needed (since nobody complained),
> or that nobody noticed because nobody uses the driver any longer.

I think it's a combination of factors: 1) rarely people have this hardware,
especially nowadays, to run more or less new kernel; 2) there are no conditions
happened that this patch fixes in their environments; 3) something else I
missed.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] fbdev/pxafb: Fix multiple clamped values in pxafb_adjust_timing

Posted by Raag Jadav 2 weeks, 2 days ago

On Wed, Dec 03, 2025 at 11:49:07AM +0200, Andy Shevchenko wrote:
> On Tue, Dec 02, 2025 at 08:36:08PM +0100, Helge Deller wrote:
> > On 12/2/25 19:36, Thorsten Blum wrote:
> > > On 2. Dec 2025, at 19:28, Helge Deller wrote:
> > > > On 12/2/25 19:15, Thorsten Blum wrote:
> 
> ...
> 
> > > > How did you notice? Do you actually have the hardware and tested it?
> > > 
> > > I only compile-tested it.
> > > 
> > > I stumbled upon another driver with the same bug and then used grep to
> > > search for other instances and found about 6 or 7, including this one.
> > 
> > Ok. But this then means, maybe the clamping isn't needed (since nobody complained),
> > or that nobody noticed because nobody uses the driver any longer.
> 
> I think it's a combination of factors: 1) rarely people have this hardware,
> especially nowadays, to run more or less new kernel; 2) there are no conditions
> happened that this patch fixes in their environments; 3) something else I
> missed.

I had a quick look at commit 3f16ff608a75 and seems like there wasn't much
happening either way.

Raag