[PATCH v5 1/3] efi_loader: Add SPI I/O protocol support

[Paul Barker]

On 24/12/2022 14:09, Heinrich Schuchardt wrote:
> On 12/24/22 13:25, Paul Barker wrote:
>> On 13/12/2022 07:15, Ilias Apalodimas wrote:
>>> Hi Paul,
>>>
>>> Apologies for the delayed reply.
>>>
>>> [...]
>>>
>>>> +static efi_status_t
>>>> +export_spi_peripheral(struct efi_spi_bus *bus, struct udevice *dev)
>>>> +{
>>>> +    efi_string_t name_utf16, vendor_utf16, part_number_utf16;
>>>> +    struct efi_spi_peripheral_priv *priv;
>>>> +    efi_status_t status;
>>>> +    efi_handle_t handle = NULL;
>>>> +    struct udevice *dev_bus = dev->parent;
>>>> +    struct spi_slave *target;
>>>> +    const char *name = dev_read_name(dev);
>>>> +    const char *vendor = dev_read_string(dev, "u-boot,uefi-spi-vendor");
>>>> +    const char *part_number = dev_read_string(dev,
>>>> +            "u-boot,uefi-spi-part-number");
>>>> +    efi_guid_t *guid = (efi_guid_t *)dev_read_u8_array_ptr(dev,
>>>> +            "u-boot,uefi-spi-io-guid", 16);
>>>> +
>>>> +    if (device_get_uclass_id(dev) == UCLASS_SPI_EMUL) {
>>>> +        debug("Skipping emulated SPI peripheral %s\n", name);
>>>> +        goto fail_1;
>>>> +    }
>>>> +
>>>> +    if (!vendor || !part_number || !guid) {
>>>> +        debug("Skipping SPI peripheral %s\n", name);
>>>> +        status = EFI_UNSUPPORTED;
>>>> +        goto fail_1;
>>>> +    }
>>>> +
>>>> +    if (!device_active(dev)) {
>>>> +        int ret = device_probe(dev);
>>>> +        if (ret) {
>>>> +            debug("Skipping SPI peripheral %s, probe failed\n",
>>>> +                  name);
>>>> +            goto fail_1;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    target = dev_get_parent_priv(dev);
>>>> +    if (!target) {
>>>> +        debug("Skipping uninitialized SPI peripheral %s\n", name);
>>>> +        status = EFI_UNSUPPORTED;
>>>> +        goto fail_1;
>>>> +    }
>>>> +
>>>> +    debug("Registering SPI dev %d:%d, name %s\n",
>>>> +          dev_bus->seq_, spi_chip_select(dev), name);
>>>> +
>>>> +    priv = calloc(1, sizeof(*priv));
>>>> +    if (!priv) {
>>>> +        status = EFI_OUT_OF_RESOURCES;
>>>> +        goto fail_1;
>>>> +    }
>>>> +
>>>> +    vendor_utf16 = efi_convert_string(vendor);
>>>> +    if (!vendor_utf16) {
>>>> +        status = EFI_OUT_OF_RESOURCES;
>>>> +        goto fail_2;
>>>> +    }
>>>> +
>>>> +    part_number_utf16 = efi_convert_string(part_number);
>>>> +    if (!part_number_utf16) {
>>>> +        status = EFI_OUT_OF_RESOURCES;
>>>> +        goto fail_3;
>>>> +    }
>>>> +
>>>> +    name_utf16 = efi_convert_string(name);
>>>> +    if (!name_utf16) {
>>>> +        status = EFI_OUT_OF_RESOURCES;
>>>> +        goto fail_4;
>>>> +    }
>>>> +
>>>> +    priv->target = target;
>>>> +
>>>> +    efi_spi_init_part(&priv->part, target, vendor_utf16, part_number_utf16);
>>>> +
>>>> +    efi_spi_init_peripheral(&priv->peripheral, &priv->part,
>>>> +                bus, target, guid, name_utf16);
>>>> +
>>>> +    efi_spi_append_peripheral(&priv->peripheral, bus);
>>>> +
>>>> +    efi_spi_init_io_protocol(&priv->io_protocol, &priv->peripheral, target);
>>>> +
>>>> +    status = efi_install_multiple_protocol_interfaces(&handle, guid,
>>>> +                              &priv->io_protocol,
>>>> +                              NULL);
>>>
>>> There's a protocols installed here as well as in
>>> efi_spi_protocol_register().  But I don't see those being uninstalled
>>> somewhere.  Shouldn't destroy_efi_spi_bus() call
>>> efi_uninstall_multiple_protocol_interfaces() as well ?
>>
>> Yes, `destroy_efi_spi_bus()` and `destroy_efi_spi_peripheral()`
>> should cleanup everything created by `export_spi_bus()` and
>> `export_spi_peripheral()` respectively.
>>
>> I think we can just call `efi_delete_handle()` on the relevant handle in
>> `destroy_efi_spi_peripheral()` as that will remove all protocols anyway
>> and the call is simpler. I can make that change in v6 of the series.
> 
> This patch does not correctly interface with the driver model.
> 
> What we need is:
> 
> * When a SPI flash device is probed successfully this causes the
> installation of the protocols and thereby the creation of the handle.
> * When trying to remove a SPI flash device this causes the
> uninstallation of the protocols.
> 
> For interfacing with the driver model you should use the events
> EVT_DM_POST_PROBE and EVT_DM_PRE_REMOVE. Cf. efi_bl_init().
> 
> When the driver model tries to remove the SPI device you have to call
> UninstallMultipleProtocolInterfaces().
> 
> efi_delete_handle() does not check if one of the protocols installed on
> the handles has been opened (e.g. with BY_DRIVER) and therefore cannot
> request those parts of the loaded code that still hold references to the
> protocol interfaces to close the protocols. This may lead to crashes.
> 
> If UninstallMultipleProtocolInterfaces() returns an error, the remove
> event handler must return an error to avoid the removal of the device.

Hi Heinrich,

I appreciate your feedback, though I have to say I'm disappointed that
this feedback wasn't provided on an earlier iteration of the patch
series.

I've got some other pressing priorities this week but will try to
re-write the relevant functions next week and re-submit.

Thanks,

-- 
Paul Barker
Principal Software Engineer
SanCloud Ltd

e: paul.barker at sancloud.com
w: https://sancloud.com/