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

[Paul Barker]

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.barker at sancloud.com
w: https://sancloud.com/