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

[Heinrich Schuchardt]

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.

Best regards

Heinrich