[PATCH 1/2] efi: remove error in efi_disk_probe

[Patrick DELAUNAY]

Hi,

On 3/9/23 09:57, Heinrich Schuchardt wrote:
> On 3/8/23 14:26, Patrick Delaunay wrote:
>> EFI has no reason to block the dm core device_probe() in the callback
>> efi_disk_probe() registered with EVT_DM_POST_PROBE.
>>
>> This patch avoids to have error in DM core on device_probe()
>>
>>    ret = device_notify(dev, EVT_DM_POST_PROBE);
>>
>> only because EFI is not able to create its instance/handle.
>
> This should only occur if we are out of memory or if you call
> efi_disk_probe() twice for the same device.


OK


>
>
>>
>> For example on usb start, when the SAME KEY (PID/VID) is present on
>> 2 ports of the USB HUB, the 2nd key have the same EFI device path
>> with the call stack:
>
> We need the HUB device with its USB port in the device path.


ok


struct efi_device_path_usb_class {
     struct efi_device_path dp;
     u16 vendor_id;
     u16 product_id;
     u8 device_class;
     u8 device_subclass;
     u8 device_protocol;
} __packed;


So a correction need to be done in 
./lib/efi_loader/efi_device_path.c:dp_fill()

     case UCLASS_MASS_STORAGE:
     case UCLASS_USB_HUB:

and ./lib/efi_loader/efi_device_path_to_text.c::dp_msging()

     case DEVICE_PATH_SUB_TYPE_MSG_USB_CLASS


to add USB port or other identifier (usb dev number for example) to 
identify each device

and not only use PID/VID as today.


for example use device ID as it is done

UCLASS_NVME => dp->hba_port = desc->devnum;

UCLASS_IDE => dp->logical_unit_number = desc->devnum;


>
> The way we currently create device paths is not good. We should traverse
> the dm tree to the root and create a node for each dm device. The code
> code for creating the individual nodes should be moved to uclasses.


I think that the USB port number can be found in USB DM in usb_device: 
udev->portnr


PS: hub_address can be also found with udev->parent->devnum;


>
> @Simon: is that ok for you?
>
>>
>> efi_disk_probe()
>>     efi_disk_create_raw()
>>         efi_disk_add_dev()
>>             efi_install_multiple_protocol_interfaces()
>>                 EFI_ALREADY_STARTED
>
> If we create the same device path for two USB devices, this is a bug we
> must fix.


OK,


so you can forget my serie


>
>>
>> In case of error in probe, the 2nd key is unbound and deactivated for
>> the next usb commands even if the limitation is only for EFI.
>>
>> This patch removes any return error in probe event callback;
>> if something occurs in EFI registration, the device is still probed.
>>
>> Signed-off-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
>> ---
>>
>>   lib/efi_loader/efi_disk.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c
>> index d2256713a8e7..8d53ba3bd27e 100644
>> --- a/lib/efi_loader/efi_disk.c
>> +++ b/lib/efi_loader/efi_disk.c
>> @@ -677,14 +677,18 @@ int efi_disk_probe(void *ctx, struct event *event)
>>       desc = dev_get_uclass_plat(dev);
>>       if (desc->uclass_id != UCLASS_EFI_LOADER) {
>>           ret = efi_disk_create_raw(dev, agent_handle);
>> -        if (ret)
>> -            return -1;
>> +        if (ret) {
>> +            log_err("efi_disk_create_raw %s failed (%d)\n",
>> +                dev->name, ret);
>
> This isn't a message a non-developer can easily understand.
>
>> +            return 0;
>> +        }
>>       }
>>
>>       device_foreach_child(child, dev) {
>>           ret = efi_disk_create_part(child, agent_handle);
>>           if (ret)
>> -            return -1;
>> +            log_err("efi_disk_create_part %s failed (%d)\n",
>
> ditto.
>
> Best regards
>
> Heinrich
>
>> +                dev->name, ret);
>>       }
>>
>>       return 0;


Patrick