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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <26277a0c-abda-c13a-80bf-528b9e167c21@suse.de>
Date:   Wed, 19 Apr 2023 09:14:02 +0200
From:   Thomas Zimmermann <tzimmermann@...e.de>
To:     Pierre Asselin <pa@...ix.com>, dri-devel@...ts.freedesktop.org
Cc:     Javier Martinez Canillas <javierm@...hat.com>,
        Daniel Vetter <daniel.vetter@...ll.ch>,
        Ard Biesheuvel <ardb@...nel.org>,
        Hans de Goede <hdegoede@...hat.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] firmware/sysfb: Fix VESA format selection

Hi

Am 19.04.23 um 06:48 schrieb Pierre Asselin:
> Some legacy BIOSes report no reserved bits in their 32-bit rgb mode,
> breaking the calculation of bits_per_pixel in commit f35cd3fa7729
> [firmware/sysfb: Fix EFI/VESA format selection].  However they report
> lfb_depth correctly for those modes.  Keep the computation but
> set bits_per_pixel to lfb_depth if the latter is larger.
> 
> 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?

> 
> Link: https://lore.kernel.org/r/4Psm6B6Lqkz1QXM@panix3.panix.com
> Link: https://lore.kernel.org/r/20230412150225.3757223-1-javierm@redhat.com
> Fixes: f35cd3fa7729 [firmware/sysfb: Fix EFI/VESA format selection]
> Signed-off-by: Pierre Asselin <pa@...ix.com>
> ---
>   drivers/firmware/sysfb_simplefb.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..358b792a8845 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -51,7 +51,8 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   	 *
>   	 * It's not easily possible to fix this in struct screen_info,
>   	 * as this could break UAPI. The best solution is to compute
> -	 * bits_per_pixel here and ignore lfb_depth. In the loop below,
> +	 * bits_per_pixel from the color bits, reserved bits and
> +	 * reported lfb_depth, whichever is highest.  In the loop below,
>   	 * ignore simplefb formats with alpha bits, as EFI and VESA
>   	 * don't specify alpha channels.
>   	 */
> @@ -60,6 +61,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   					  si->green_size + si->green_pos,
>   					  si->blue_size + si->blue_pos),
>   				     si->rsvd_size + si->rsvd_pos);
> +		bits_per_pixel= max(bits_per_pixel, (u32)si->lfb_depth);

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);

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().

Best regards
Thomas


>   	} else {
>   		bits_per_pixel = si->lfb_depth;
>   	}
> 
> base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Download attachment "OpenPGP_signature" of type "application/pgp-signature" (841 bytes)