Re: [PATCH v2] firmware/sysfb: Fix VESA format selection

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3453 bytes --]

Hi

Am 19.04.23 um 22:27 schrieb Pierre Asselin:
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 19.04.23 um 06:48 schrieb Pierre Asselin:
>>>
>>> v2 fixes the warnings from a max3() macro with arguments of different
>>> types;  split the bits_per_pixel assignment to avoid uglyfing the code
>>> with too many typecasts.
>>
>> What exactly was that warning?
> 
> A friendly note from a robot; make W=1 sysfb_simplefb.o .
> https://lore.kernel.org/dri-devel/20230418183325.2327-1-pa@panix.com/T/#m38e859354329ab9f756da91e99b546e3b140fa91
> 
> 
>> I liked the all-in-one assignment of the original patch. So I'd rather
>> go back to v1 and copy si->lfb_depth to the correct type, like this:
>>
>>     u32 depth = si->lfb_depth;
>>     bits_per_pixel = max3(max3(colors),
>>     		        rsvd,
>>                           depth);
> 
> Would that work?  If I understand correctly max3() checks that all args
> have the same type.  {red,green,blue,rsvd}.{size,pos} are all u8 while
> lfb_depth is u16.  The best I can do is

Maybe make the depth variable a u8 then with a clamp_t()-based cast there:

   u8 depth = clamp_t(u8, si->lfb_depth, 1, 32);

There's currently no way that lfb_depth would be outside the [1, 32] range.

> 
>      bits_per_pixel = max3((u16)max3(si->red_size + si->red_pos,
>                                      si->green_size + si->green_pos,
>                                      si->blue_size + si->blue_pos),
>                            (u16)(si->rsvd_size + si->rsvd_pos),
>                            si->lfb_depth);
> 
> That compiles quietly with W=1 but those two casts are ugly.
> If I do that, would K&R-on-parentheses read better ?
> 
>      bits_per_pixel = max3(
>                            (u16)max3(
>                                    si->red_size + si->red_pos,
>                                    si->green_size + si->green_pos,
>                                    si->blue_size + si->blue_pos
>                            ),
>                            (u16)(si->rsvd_size + si->rsvd_pos),
>                            si->lfb_depth
>                       );
> 
> I think it's clearer, but not kernel style and still ugly.
> 
>> Or, if you want to get fancy, you could add max3_t() to <linux/minmax.h>
>>
>>     #define max3_t(type, x, y, z)   max_t(type, max_t(type, x, y), z)
>>
>> and do
>>
>>     bits_per_pixel = max3_t(u32,
>>                             max3(colors),
>>                             rsvd,
>>                             si->lfb_depth)
>>
>> You could also add a max4_t(type, x, y, z, w) to <linux/minmax.h> and
>> compare all values with max4_t().
> 
> That would be a two-patch series.  I'd rather keep it to the strict
> minimum that fixes the regression.  (You trust me to even *look* at a
> kernel header and not break it ?  Dangerous assumption!)
> 
> I'm new at this.  Two months ago I didn't know what to type a the
> command line after "git".

Welcome to the kernel community. :)

> 
> Incidentally, should I send v3 as a new email or reply to the chain?

As a new mail, please. It's easier for readers and tools.

Best regards
Thomas

> 
> --PA
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]