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.
Thanks,
--
Paul Barker
Principal Software Engineer
SanCloud Ltd
e: paul.bar...@sancloud.com
w: https://sancloud.com/
