Hi Am 19.04.23 um 22:27 schrieb Pierre Asselin:
Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxxxx/T/#m38e859354329ab9f756da91e99b546e3b140fa91I 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:
OpenPGP_signature
Description: OpenPGP digital signature
