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

[Sudeep Holla <sudeep.holla@arm.com>]

On Tue, May 14, 2024 at 12:55:20PM +0000, Sebastian Ene wrote:
> On Fri, May 10, 2024 at 07:38:10PM +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/driver.c | 33 ++++++++++++++++++++++++++++++-
> >  1 file changed, 32 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index 9f89ee0aaa6b..159f2106a541 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -1235,6 +1235,29 @@ void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid)
> >  	kfree(pbuf);
> >  }
> 
> Hello Sudeep,
> 
> > 
> > +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;
> > +
> > +		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;
> > @@ -1243,6 +1266,14 @@ 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) {
> 
> Should we remove the version check inside the ffa_device_match_uuid
> function as this function will only be called from the bus notifier ?
>

Good catch, I didn't notice we already have the check there. I will drop
that version check inside ffa_device_match_uuid().

> > +		ret = bus_register_notifier(&ffa_bus_type, &ffa_bus_nb);
> > +		if (ret) {
> > +			pr_err("Failed to register FF-A bus notifiers\n");
> > +			return ret;
> > +		}
> > +	}
> > +
> >  	count = ffa_partition_probe(&uuid_null, &pbuf);
> >  	if (count <= 0) {
> >  		pr_info("%s: No partitions found, error %d\n", __func__, count);
> > @@ -1254,7 +1285,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
> > 
> 
> Also I wonder if we should have the change from patch 2 (the one which
> removes the ffa_device_match_uuid call from ffa_device_match) inside
> this. Wdyth ?
> 

I think so, again missed to see this as I had it in reverse order until
I was about to push the patches out. I realised the order can be swapped
actually without breaking the bisection but missed this. That's why you
see my note in 2/2 instead of 1/2 😁. I may have to also mov the NULL UUID
check in probe here. I will check that.

Thanks for taking a look at these.

-- 
Regards,
Sudeep

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