Re: [PATCH v2 1/2] firmware: arm_ffa: Move the FF-A v1.0 NULL UUID workaround to bus notifier

[Sebastian Ene <sebastianene@google.com>]

On Wed, May 15, 2024 at 10:40:27AM +0100, Sudeep Holla wrote:
> Currently FF-A bus ffa_device_match() handles the workaround for the
> FF-A v1.0 devices which are not populated with UUID to match. The FF-A
> bus layer calls into FF-A driver and populates the device UUID if it
> matches with the driver attempting to match.
> 
> But this forces to have both FF-A bus and core driver to be bundled into
> a single module. However, keep it as a single module adds problems for
> the FF-A driver registrations and their initcall levels.
> 
> In preparation to split the FF-A bus and the core driver into distinct
> modules, we need to move the workaround away from the FF-A bus layer.
> We can add it into the FF-A core driver as a bus notifier.
> 
> In order to do so, we need to always match any driver with the device if
> the UUID is NULL and then during the driver binding phase, we can populate
> the UUID if it matches with the driver UUID table using the bus notifiers.
> We also need to add a check for NULL UUID before calling the device/driver
> probe as devices with NULL UUID is possible since we match all for that
> case.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/bus.c    | 11 +++++---
>  drivers/firmware/arm_ffa/driver.c | 45 ++++++++++++++++++++++++-------
>  2 files changed, 43 insertions(+), 13 deletions(-)
> 
> v1->v2:
> 	- Dropped unnecessary version check in ffa_device_match_uuid()
> 	- Moved all the logic associated with ffa_bus_notifier() into this
> 	  patch which previously had incorrectly spilled into second patch
> 	  Thanks to Sebastian Ene for pointing it out.
> 

Hello Sudeep,

Thanks for the v2, this looks good to me.

Acked-by: Sebastian Ene <sebastianene@google.com>

> diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c
> index 2f557e90f2eb..4baaec7f0a09 100644
> --- a/drivers/firmware/arm_ffa/bus.c
> +++ b/drivers/firmware/arm_ffa/bus.c
> @@ -30,12 +30,11 @@ static int ffa_device_match(struct device *dev, struct device_driver *drv)
>  	while (!uuid_is_null(&id_table->uuid)) {
>  		/*
>  		 * FF-A v1.0 doesn't provide discovery of UUIDs, just the
> -		 * partition IDs, so fetch the partitions IDs for this
> -		 * id_table UUID and assign the UUID to the device if the
> -		 * partition ID matches
> +		 * partition IDs, so match it unconditionally here and handle
> +		 * it via the installed bus notifier during driver binding.
>  		 */
>  		if (uuid_is_null(&ffa_dev->uuid))
> -			ffa_device_match_uuid(ffa_dev, &id_table->uuid);
> +			return 1;
> 
>  		if (uuid_equal(&ffa_dev->uuid, &id_table->uuid))
>  			return 1;
> @@ -50,6 +49,10 @@ static int ffa_device_probe(struct device *dev)
>  	struct ffa_driver *ffa_drv = to_ffa_driver(dev->driver);
>  	struct ffa_device *ffa_dev = to_ffa_dev(dev);
> 
> +	/* UUID can be still NULL with FF-A v1.0, so just skip probing them */
> +	if (uuid_is_null(&ffa_dev->uuid))
> +		return -ENODEV;
> +
>  	return ffa_drv->probe(ffa_dev);
>  }
> 
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 1609247cfafc..61d514776e5b 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1224,14 +1224,6 @@ void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
>  	int count, idx;
>  	struct ffa_partition_info *pbuf, *tpbuf;
> 
> -	/*
> -	 * FF-A v1.1 provides UUID for each partition as part of the discovery
> -	 * API, the discovered UUID must be populated in the device's UUID and
> -	 * there is no need to copy the same from the driver table.
> -	 */
> -	if (drv_info->version > FFA_VERSION_1_0)
> -		return;
> -
>  	count = ffa_partition_probe(uuid, &pbuf);
>  	if (count <= 0)
>  		return;
> @@ -1242,6 +1234,35 @@ void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
>  	kfree(pbuf);
>  }
> 
> +static int
> +ffa_bus_notifier(struct notifier_block *nb, unsigned long action, void *data)
> +{
> +	struct device *dev = data;
> +	struct ffa_device *fdev = to_ffa_dev(dev);
> +
> +	if (action == BUS_NOTIFY_BIND_DRIVER) {
> +		struct ffa_driver *ffa_drv = to_ffa_driver(dev->driver);
> +		const struct ffa_device_id *id_table= ffa_drv->id_table;
> +
> +		/*
> +		 * FF-A v1.1 provides UUID for each partition as part of the
> +		 * discovery API, the discovered UUID must be populated in the
> +		 * device's UUID and there is no need to workaround by copying
> +		 * the same from the driver table.
> +		 */
> +		if (uuid_is_null(&fdev->uuid))
> +			ffa_device_match_uuid(fdev, &id_table->uuid);
> +
> +		return NOTIFY_OK;
> +	}
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static struct notifier_block ffa_bus_nb = {
> +	.notifier_call = ffa_bus_notifier,
> +};
> +
>  static int ffa_setup_partitions(void)
>  {
>  	int count, idx, ret;
> @@ -1250,6 +1271,12 @@ static int ffa_setup_partitions(void)
>  	struct ffa_dev_part_info *info;
>  	struct ffa_partition_info *pbuf, *tpbuf;
> 
> +	if (drv_info->version == FFA_VERSION_1_0) {
> +		ret = bus_register_notifier(&ffa_bus_type, &ffa_bus_nb);
> +		if (ret)
> +			pr_err("Failed to register FF-A bus notifiers\n");
> +	}
> +
>  	count = ffa_partition_probe(&uuid_null, &pbuf);
>  	if (count <= 0) {
>  		pr_info("%s: No partitions found, error %d\n", __func__, count);
> @@ -1261,7 +1288,7 @@ static int ffa_setup_partitions(void)
>  		import_uuid(&uuid, (u8 *)tpbuf->uuid);
> 
>  		/* Note that if the UUID will be uuid_null, that will require
> -		 * ffa_device_match() to find the UUID of this partition id
> +		 * ffa_bus_notifier() to find the UUID of this partition id
>  		 * with help of ffa_device_match_uuid(). FF-A v1.1 and above
>  		 * provides UUID here for each partition as part of the
>  		 * discovery API and the same is passed.
> --
> 2.43.2
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel