Re: [PATCH 2/5] efi/libstub: gop: Find GOP handle instead of GOP data

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi Javier,

thanks for reviewing.

Am 24.10.25 um 11:47 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
>> The device handle of the GOP device is required to retrieve the
>> correct EDID data. Find the handle instead of the GOP data. Still
>> return the GOP data in the function arguments, as we already looked
>> it up.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/firmware/efi/libstub/gop.c | 27 +++++++++++++++++----------
>>   1 file changed, 17 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/firmware/efi/libstub/gop.c b/drivers/firmware/efi/libstub/gop.c
>> index 3785fb4986b4..fd32be8dd146 100644
>> --- a/drivers/firmware/efi/libstub/gop.c
>> +++ b/drivers/firmware/efi/libstub/gop.c
>> @@ -402,12 +402,13 @@ setup_pixel_info(struct screen_info *si, u32 pixels_per_scan_line,
>>   	}
>>   }
>>   
>> -static efi_graphics_output_protocol_t *find_gop(unsigned long num,
>> -						const efi_handle_t handles[])
>> +static efi_handle_t find_handle_with_primary_gop(unsigned long num, const efi_handle_t handles[],
>> +						 efi_graphics_output_protocol_t **found_gop)
>>   {
>>   	efi_graphics_output_protocol_t *first_gop;
>> -	efi_handle_t h;
>> +	efi_handle_t h, first_gop_handle;
>>   
>> +	first_gop_handle = NULL;
>>   	first_gop = NULL;
>>
> I think the logic of this function could be simplified if you remove some
> of the variables. For example, I don't think you need a fist_gop variable
> anymore now that you are passing a found_gop variable as a function param.
>
>>   	for_each_efi_handle(h, handles, num) {
>> @@ -442,19 +443,25 @@ static efi_graphics_output_protocol_t *find_gop(unsigned long num,
>>   		 */
>>   		status = efi_bs_call(handle_protocol, h,
>>   				     &EFI_CONSOLE_OUT_DEVICE_GUID, &dummy);
>> -		if (status == EFI_SUCCESS)
>> -			return gop;
>> -
>> -		if (!first_gop)
>> +		if (status == EFI_SUCCESS) {
>> +			if (found_gop)
>> +				*found_gop = gop;
>> +			return h;
>> +		} else if (!first_gop_handle) {
>> +			first_gop_handle = h;
>>   			first_gop = gop;
> You can just assign *found_gop = gop here...
>
>> +		}
>>   	}
>>   
>> -	return first_gop;
>> +	if (found_gop)
>> +		*found_gop = first_gop;
> ...and then this assignment won't be needed anynmore.

TBH I choose that style on purpose. It's easily parseable by the eye. 
found_gop is allowed be NULL and we'd have to test within the loop. I 
found this uneasy to read. And assigning *found_gop early leaks state to 
the outside before it's ready. That's probably not a problem here, but I 
find that questionable.

>
>> +	return first_gop_handle;
> Also, given that you are calling first_gop_handle to the variable to
> store the first gop handle, I would for consistency name the parameter
> fist_gop and just drop the local variable with the same name.

The found_gop is not necessarily the first_gop. We want to return the 
primary device's handle and GOP state. The first one is only returned if 
there's no clear primary one. See [1] as for how the primary is being 
detected.

[1] 
https://elixir.bootlin.com/linux/v6.17.4/source/drivers/firmware/efi/libstub/gop.c#L433

Best regards
Thomas

>
> But I agree with the general logic of the patch, so regardless of what
> you prefer to do:
>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>

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