Hi Javier,
thanks for reviewing.
Am 24.10.25 um 11:47 schrieb Javier Martinez Canillas:
Thomas Zimmermann <[email protected]> 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 <[email protected]>
---
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 <[email protected]>
--
--
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)
