[PATCH] Refactor watchdog probing

[Christian Storm]

From: Christian Storm <christi...@siemens.com>

Refactor the watchdog probing in terms of
(1) renaming scan_devices() to probe_watchdogs(), and
(2) replacing the static upper-bounded PCI device enumeration
with a dynamic one based on EFI's LocateHandleBuffer().

Signed-off-by: Christian Storm <christi...@siemens.com>
---
main.c | 105 +++++++++++++++++++++++++++++----------------------------
1 file changed, 54 insertions(+), 51 deletions(-)

diff --git a/main.c b/main.c
index 0bec838..b11c5fb 100644
--- a/main.c
+++ b/main.c
@@ -27,72 +27,75 @@ extern const unsigned long init_array_start[];
extern const unsigned long init_array_end[];
extern CHAR16 *boot_medium_path;

-static EFI_STATUS probe_watchdog(EFI_LOADED_IMAGE *loaded_image,
- EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
- UINT16 pci_device_id, UINTN timeout)
-{
- const unsigned long *entry;
-
- for (entry = init_array_start; entry < init_array_end; entry++) {
- EFI_STATUS (*probe)(EFI_PCI_IO *, UINT16, UINT16, UINTN);
-
- probe = loaded_image->ImageBase + *entry;
- if (probe(pci_io, pci_vendor_id, pci_device_id, timeout) ==
- EFI_SUCCESS) {
- return EFI_SUCCESS;
- }
- }
+#define PCI_GET_VENDOR_ID(id) id
+#define PCI_GET_PRODUCT_ID(id) id >> 16

- return EFI_UNSUPPORTED;
-}
-
-static EFI_STATUS scan_devices(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)
+static EFI_STATUS probe_watchdogs(EFI_LOADED_IMAGE *loaded_image, UINTN timeout)
{
- EFI_HANDLE devices[1000];
- UINTN count, size = sizeof(devices);
- EFI_PCI_IO *pci_io;
- EFI_STATUS status;
- UINT32 value;
-
- status = uefi_call_wrapper(BS->LocateHandle, 5, ByProtocol,
- &PciIoProtocol, NULL, &size, devices);
- if (EFI_ERROR(status)) {
- return status;
+ if (init_array_end - init_array_start == 0) {
+ WARNING(L"No watchdog drivers registered.\n");
+ return EFI_NOT_FOUND;
}

- count = size / sizeof(EFI_HANDLE);
- if (count == 0) {
- return probe_watchdog(loaded_image, NULL, 0, 0, timeout);
+ UINTN handle_count = 0;
+ EFI_HANDLE *handle_buffer = NULL;
+ EFI_STATUS status = uefi_call_wrapper(BS->LocateHandleBuffer, 5,
+ ByProtocol, &PciIoProtocol, NULL,
+ &handle_count, &handle_buffer);
+ if (EFI_ERROR(status) || (handle_count == 0)) {
+ WARNING(L"No PCI I/O Protocol handles found.\n");
+ if (handle_buffer) {
+ FreePool(handle_buffer);
+ }
+ return EFI_UNSUPPORTED;
}

- do {
- EFI_HANDLE device = devices[count - 1];
-
- count--;
-
- status = uefi_call_wrapper(BS->OpenProtocol, 6, device,
- &PciIoProtocol, (VOID **)&pci_io,
- this_image, NULL,
- EFI_OPEN_PROTOCOL_GET_PROTOCOL);
+ EFI_PCI_IO_PROTOCOL *pci_io;
+ UINT32 value;
+ for (UINTN index = 0; index < handle_count; index++) {
+ status = uefi_call_wrapper(BS->OpenProtocol, 6,
+ handle_buffer[index], &PciIoProtocol,
+ (VOID **)&pci_io, this_image, NULL,
+ EFI_OPEN_PROTOCOL_BY_HANDLE_PROTOCOL);
if (EFI_ERROR(status)) {
- error_exit(L"Cannot open PciIoProtocol while probing watchdogs",
- status);
+ ERROR(L"Cannot not open PciIoProtocol: %r\n", status);
+ FreePool(handle_buffer);
+ return status;
}

status = uefi_call_wrapper(pci_io->Pci.Read, 5, pci_io,
EfiPciIoWidthUint32, PCI_VENDOR_ID,
1, &value);
if (EFI_ERROR(status)) {
- error_exit(L"Cannot read from PCI device while probing watchdogs",
- status);
+ WARNING(
+ L"Cannot not read from PCI device, skipping: %r\n",
+ status);
+ (VOID) uefi_call_wrapper(
+ BS->CloseProtocol, 4, handle_buffer[index],
+ &PciIoProtocol, this_image, NULL);
+ continue;
}

- status = probe_watchdog(loaded_image, pci_io, (UINT16)value,
- value >> 16, timeout);
+ EFI_STATUS (*probe)(EFI_PCI_IO *, UINT16, UINT16, UINTN);
+ for (const unsigned long *entry = init_array_start;
+ entry < init_array_end; entry++) {
+ probe = loaded_image->ImageBase + *entry;
+ if ((status = probe(pci_io, PCI_GET_VENDOR_ID(value),
+ PCI_GET_PRODUCT_ID(value),
+ timeout)) == EFI_SUCCESS) {
+ break;
+ }
+ }
+
+ (VOID) uefi_call_wrapper(BS->CloseProtocol, 4,
+ handle_buffer[index], &PciIoProtocol,
+ this_image, NULL);

- uefi_call_wrapper(BS->CloseProtocol, 4, device, &PciIoProtocol,
- this_image, NULL);
- } while (status != EFI_SUCCESS && count > 0);
+ if (status == EFI_SUCCESS) {
+ break;
+ }
+ }
+ FreePool(handle_buffer);

return status;
}
@@ -168,7 +171,7 @@ EFI_STATUS efi_main(EFI_HANDLE image_handle, EFI_SYSTEM_TABLE *system_table)
if (bg_loader_params.timeout == 0) {
WARNING(L"Watchdog is disabled.\n");
} else {
- status = scan_devices(loaded_image, bg_loader_params.timeout);
+ status = probe_watchdogs(loaded_image, bg_loader_params.timeout);
if (EFI_ERROR(status)) {
error_exit(L"Cannot probe watchdog", status);
}
--
2.33.0

[Jan Kiszka]

On 24.08.21 16:07, Christian Storm wrote:
> From: Christian Storm <christi...@siemens.com>
>
> Refactor the watchdog probing in terms of
> (1) renaming scan_devices() to probe_watchdogs(), and
> (2) replacing the static upper-bounded PCI device enumeration
> with a dynamic one based on EFI's LocateHandleBuffer().

Can you leave a word on the impact of change (2)?

Jan

Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Christian Storm]

Hi Jan,

> > Refactor the watchdog probing in terms of
> > (1) renaming scan_devices() to probe_watchdogs(), and
> > (2) replacing the static upper-bounded PCI device enumeration
> > with a dynamic one based on EFI's LocateHandleBuffer().
>
> Can you leave a word on the impact of change (2)?

Sure: Previous to this change you had
EFI_HANDLE devices[1000]
which means the maximum number of handles that can be probed had an
artificial hard upper limit. Now, with this change, LocateHandleBuffer()
allocates a buffer sufficiently sized to hold the handles available. So,
this change removes the artificial restriction in the number handles
that can be probed.

Kind regards,
Christian

--
Dr. Christian Storm
Siemens AG, Technology, T RDA IOT SES-DE
Otto-Hahn-Ring 6, 81739 München, Germany

[Jan Kiszka]

On 24.08.21 18:06, Christian Storm wrote:
> Hi Jan,
>
>>> Refactor the watchdog probing in terms of
>>> (1) renaming scan_devices() to probe_watchdogs(), and
>>> (2) replacing the static upper-bounded PCI device enumeration
>>> with a dynamic one based on EFI's LocateHandleBuffer().
>>
>> Can you leave a word on the impact of change (2)?
>
> Sure: Previous to this change you had
> EFI_HANDLE devices[1000]
> which means the maximum number of handles that can be probed had an
> artificial hard upper limit. Now, with this change, LocateHandleBuffer()
> allocates a buffer sufficiently sized to hold the handles available. So,
> this change removes the artificial restriction in the number handles
> that can be probed.
>

Thanks, applied, adding another sentence to make this clearer.

Jan