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

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> ---
>  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.

> +	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.

But I agree with the general logic of the patch, so regardless of what
you prefer to do:

Reviewed-by: Javier Martinez Canillas <javierm@xxxxxxxxxx>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat