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

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

[Sudeep Holla]

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);
 }

+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) {
+		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


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

[PATCH 2/2] firmware: arm_ffa: Split bus and driver into distinct modules

[Sudeep Holla]

Make the FF-A bus on its own as a distinct module initialized at
subsys_initcall level when builtin.

Keep the FF-A driver core stack, together with any configured transport,
in a different module initialized as module_init level.

FF-A drivers initialization is now changed to module_init level.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/Makefile |  6 ++++--
 drivers/firmware/arm_ffa/bus.c    | 22 ++++++++++++++++------
 drivers/firmware/arm_ffa/common.h |  2 --
 drivers/firmware/arm_ffa/driver.c | 12 ++----------
 4 files changed, 22 insertions(+), 20 deletions(-)

Hi Sebastian,

I reworked the patch I shared with you in private to include handling of
the FF-A v1.0 workaround I had removed as a quick hack when I shared the
patch. Please check if these couple of patches help you in the initcall
order issue you were facing.

Regards,
Sudeep

diff --git a/drivers/firmware/arm_ffa/Makefile b/drivers/firmware/arm_ffa/Makefile
index 9d9f37523200..168990a7e792 100644
--- a/drivers/firmware/arm_ffa/Makefile
+++ b/drivers/firmware/arm_ffa/Makefile
@@ -2,5 +2,7 @@
 ffa-bus-y = bus.o
 ffa-driver-y = driver.o
 ffa-transport-$(CONFIG_ARM_FFA_SMCCC) += smccc.o
-ffa-module-objs := $(ffa-bus-y) $(ffa-driver-y) $(ffa-transport-y)
-obj-$(CONFIG_ARM_FFA_TRANSPORT) = ffa-module.o
+ffa-core-objs := $(ffa-bus-y)
+ffa-module-objs := $(ffa-driver-y) $(ffa-transport-y)
+obj-$(CONFIG_ARM_FFA_TRANSPORT)  = ffa-core.o
+obj-$(CONFIG_ARM_FFA_TRANSPORT) += ffa-module.o
diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c
index 2f557e90f2eb..0c83931485f6 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);
 }

@@ -232,14 +235,21 @@ void ffa_device_unregister(struct ffa_device *ffa_dev)
 }
 EXPORT_SYMBOL_GPL(ffa_device_unregister);

-int arm_ffa_bus_init(void)
+static int __init arm_ffa_bus_init(void)
 {
 	return bus_register(&ffa_bus_type);
 }
+subsys_initcall(arm_ffa_bus_init);

-void arm_ffa_bus_exit(void)
+static void __exit arm_ffa_bus_exit(void)
 {
 	ffa_devices_unregister();
 	bus_unregister(&ffa_bus_type);
 	ida_destroy(&ffa_bus_id);
 }
+module_exit(arm_ffa_bus_exit);
+
+MODULE_ALIAS("ffa-core");
+MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
+MODULE_DESCRIPTION("ARM FF-A bus");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h
index d6eccf1fd3f6..9c6425a81d0d 100644
--- a/drivers/firmware/arm_ffa/common.h
+++ b/drivers/firmware/arm_ffa/common.h
@@ -14,8 +14,6 @@ typedef struct arm_smccc_1_2_regs ffa_value_t;

 typedef void (ffa_fn)(ffa_value_t, ffa_value_t *);

-int arm_ffa_bus_init(void);
-void arm_ffa_bus_exit(void);
 bool ffa_device_is_valid(struct ffa_device *ffa_dev);
 void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid);

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 159f2106a541..7cdd425c5fc4 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1603,14 +1603,9 @@ static int __init ffa_init(void)
 	if (ret)
 		return ret;

-	ret = arm_ffa_bus_init();
-	if (ret)
-		return ret;
-
 	drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL);
 	if (!drv_info) {
-		ret = -ENOMEM;
-		goto ffa_bus_exit;
+		return -ENOMEM;
 	}

 	ret = ffa_version_check(&drv_info->version);
@@ -1671,11 +1666,9 @@ static int __init ffa_init(void)
 	free_pages_exact(drv_info->rx_buffer, RXTX_BUFFER_SIZE);
 free_drv_info:
 	kfree(drv_info);
-ffa_bus_exit:
-	arm_ffa_bus_exit();
 	return ret;
 }
-subsys_initcall(ffa_init);
+module_init(ffa_init);

 static void __exit ffa_exit(void)
 {
@@ -1685,7 +1678,6 @@ static void __exit ffa_exit(void)
 	free_pages_exact(drv_info->tx_buffer, RXTX_BUFFER_SIZE);
 	free_pages_exact(drv_info->rx_buffer, RXTX_BUFFER_SIZE);
 	kfree(drv_info);
-	arm_ffa_bus_exit();
 }
 module_exit(ffa_exit);

--
2.43.2


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

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

[Sebastian Ene]

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 ?

> +		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 ?


Thank you,
Sebastian

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

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

[Sudeep Holla]

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

Re: [PATCH 2/2] firmware: arm_ffa: Split bus and driver into distinct modules

[Sebastian Ene]

On Fri, May 10, 2024 at 07:38:11PM +0100, Sudeep Holla wrote:
> Make the FF-A bus on its own as a distinct module initialized at
> subsys_initcall level when builtin.
> 
> Keep the FF-A driver core stack, together with any configured transport,
> in a different module initialized as module_init level.
> 
> FF-A drivers initialization is now changed to module_init level.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/Makefile |  6 ++++--
>  drivers/firmware/arm_ffa/bus.c    | 22 ++++++++++++++++------
>  drivers/firmware/arm_ffa/common.h |  2 --
>  drivers/firmware/arm_ffa/driver.c | 12 ++----------
>  4 files changed, 22 insertions(+), 20 deletions(-)
> 
> Hi Sebastian,
> 
> I reworked the patch I shared with you in private to include handling of
> the FF-A v1.0 workaround I had removed as a quick hack when I shared the
> patch. Please check if these couple of patches help you in the initcall
> order issue you were facing.
> 
> Regards,
> Sudeep

Hello Sudeep,


Thank for sharing the patch. I gave it a spin with pKVM and OPTee under
Qemu and it works well. Having the bus driver initialize separately is a
good ideea.

> 
> diff --git a/drivers/firmware/arm_ffa/Makefile b/drivers/firmware/arm_ffa/Makefile
> index 9d9f37523200..168990a7e792 100644
> --- a/drivers/firmware/arm_ffa/Makefile
> +++ b/drivers/firmware/arm_ffa/Makefile
> @@ -2,5 +2,7 @@
>  ffa-bus-y = bus.o
>  ffa-driver-y = driver.o
>  ffa-transport-$(CONFIG_ARM_FFA_SMCCC) += smccc.o
> -ffa-module-objs := $(ffa-bus-y) $(ffa-driver-y) $(ffa-transport-y)
> -obj-$(CONFIG_ARM_FFA_TRANSPORT) = ffa-module.o
> +ffa-core-objs := $(ffa-bus-y)
> +ffa-module-objs := $(ffa-driver-y) $(ffa-transport-y)
> +obj-$(CONFIG_ARM_FFA_TRANSPORT)  = ffa-core.o
> +obj-$(CONFIG_ARM_FFA_TRANSPORT) += ffa-module.o
> diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c
> index 2f557e90f2eb..0c83931485f6 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);
>  }
> 
> @@ -232,14 +235,21 @@ void ffa_device_unregister(struct ffa_device *ffa_dev)
>  }
>  EXPORT_SYMBOL_GPL(ffa_device_unregister);
> 
> -int arm_ffa_bus_init(void)
> +static int __init arm_ffa_bus_init(void)
>  {
>  	return bus_register(&ffa_bus_type);
>  }
> +subsys_initcall(arm_ffa_bus_init);
> 
> -void arm_ffa_bus_exit(void)
> +static void __exit arm_ffa_bus_exit(void)
>  {
>  	ffa_devices_unregister();
>  	bus_unregister(&ffa_bus_type);
>  	ida_destroy(&ffa_bus_id);
>  }
> +module_exit(arm_ffa_bus_exit);
> +
> +MODULE_ALIAS("ffa-core");
> +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> +MODULE_DESCRIPTION("ARM FF-A bus");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h
> index d6eccf1fd3f6..9c6425a81d0d 100644
> --- a/drivers/firmware/arm_ffa/common.h
> +++ b/drivers/firmware/arm_ffa/common.h
> @@ -14,8 +14,6 @@ typedef struct arm_smccc_1_2_regs ffa_value_t;
> 
>  typedef void (ffa_fn)(ffa_value_t, ffa_value_t *);
> 
> -int arm_ffa_bus_init(void);
> -void arm_ffa_bus_exit(void);
>  bool ffa_device_is_valid(struct ffa_device *ffa_dev);
>  void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid);
> 
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 159f2106a541..7cdd425c5fc4 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1603,14 +1603,9 @@ static int __init ffa_init(void)
>  	if (ret)
>  		return ret;
> 
> -	ret = arm_ffa_bus_init();
> -	if (ret)
> -		return ret;
> -
>  	drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL);
>  	if (!drv_info) {
> -		ret = -ENOMEM;
> -		goto ffa_bus_exit;
> +		return -ENOMEM;
>  	}
> 
>  	ret = ffa_version_check(&drv_info->version);
> @@ -1671,11 +1666,9 @@ static int __init ffa_init(void)
>  	free_pages_exact(drv_info->rx_buffer, RXTX_BUFFER_SIZE);
>  free_drv_info:
>  	kfree(drv_info);
> -ffa_bus_exit:
> -	arm_ffa_bus_exit();
>  	return ret;
>  }
> -subsys_initcall(ffa_init);
> +module_init(ffa_init);
> 
>  static void __exit ffa_exit(void)
>  {
> @@ -1685,7 +1678,6 @@ static void __exit ffa_exit(void)
>  	free_pages_exact(drv_info->tx_buffer, RXTX_BUFFER_SIZE);
>  	free_pages_exact(drv_info->rx_buffer, RXTX_BUFFER_SIZE);
>  	kfree(drv_info);
> -	arm_ffa_bus_exit();
>  }
>  module_exit(ffa_exit);
> 
> --
> 2.43.2
> 

Ack, Sebastian

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

Re: [PATCH 2/2] firmware: arm_ffa: Split bus and driver into distinct modules

[Sudeep Holla]

On Tue, May 14, 2024 at 02:54:46PM +0000, Sebastian Ene wrote:
> On Fri, May 10, 2024 at 07:38:11PM +0100, Sudeep Holla wrote:
> > Make the FF-A bus on its own as a distinct module initialized at
> > subsys_initcall level when builtin.
> > 
> > Keep the FF-A driver core stack, together with any configured transport,
> > in a different module initialized as module_init level.
> > 
> > FF-A drivers initialization is now changed to module_init level.
> > 
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_ffa/Makefile |  6 ++++--
> >  drivers/firmware/arm_ffa/bus.c    | 22 ++++++++++++++++------
> >  drivers/firmware/arm_ffa/common.h |  2 --
> >  drivers/firmware/arm_ffa/driver.c | 12 ++----------
> >  4 files changed, 22 insertions(+), 20 deletions(-)
> > 
> > Hi Sebastian,
> > 
> > I reworked the patch I shared with you in private to include handling of
> > the FF-A v1.0 workaround I had removed as a quick hack when I shared the
> > patch. Please check if these couple of patches help you in the initcall
> > order issue you were facing.
> > 
> > Regards,
> > Sudeep
> 
> Hello Sudeep,
> 
> 
> Thank for sharing the patch. I gave it a spin with pKVM and OPTee under
> Qemu and it works well. Having the bus driver initialize separately is a
> good ideea.
>

Thanks for the review and testing.

> > 
> > diff --git a/drivers/firmware/arm_ffa/Makefile b/drivers/firmware/arm_ffa/Makefile
> > index 9d9f37523200..168990a7e792 100644
> > --- a/drivers/firmware/arm_ffa/Makefile
> > +++ b/drivers/firmware/arm_ffa/Makefile
> > @@ -2,5 +2,7 @@
> >  ffa-bus-y = bus.o
> >  ffa-driver-y = driver.o
> >  ffa-transport-$(CONFIG_ARM_FFA_SMCCC) += smccc.o
> > -ffa-module-objs := $(ffa-bus-y) $(ffa-driver-y) $(ffa-transport-y)
> > -obj-$(CONFIG_ARM_FFA_TRANSPORT) = ffa-module.o
> > +ffa-core-objs := $(ffa-bus-y)
> > +ffa-module-objs := $(ffa-driver-y) $(ffa-transport-y)
> > +obj-$(CONFIG_ARM_FFA_TRANSPORT)  = ffa-core.o
> > +obj-$(CONFIG_ARM_FFA_TRANSPORT) += ffa-module.o
> > diff --git a/drivers/firmware/arm_ffa/bus.c b/drivers/firmware/arm_ffa/bus.c
> > index 2f557e90f2eb..0c83931485f6 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);
> >  }
> > 
> > @@ -232,14 +235,21 @@ void ffa_device_unregister(struct ffa_device *ffa_dev)
> >  }
> >  EXPORT_SYMBOL_GPL(ffa_device_unregister);
> > 
> > -int arm_ffa_bus_init(void)
> > +static int __init arm_ffa_bus_init(void)
> >  {
> >  	return bus_register(&ffa_bus_type);
> >  }
> > +subsys_initcall(arm_ffa_bus_init);
> > 
> > -void arm_ffa_bus_exit(void)
> > +static void __exit arm_ffa_bus_exit(void)
> >  {
> >  	ffa_devices_unregister();
> >  	bus_unregister(&ffa_bus_type);
> >  	ida_destroy(&ffa_bus_id);
> >  }
> > +module_exit(arm_ffa_bus_exit);
> > +
> > +MODULE_ALIAS("ffa-core");
> > +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> > +MODULE_DESCRIPTION("ARM FF-A bus");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/firmware/arm_ffa/common.h b/drivers/firmware/arm_ffa/common.h
> > index d6eccf1fd3f6..9c6425a81d0d 100644
> > --- a/drivers/firmware/arm_ffa/common.h
> > +++ b/drivers/firmware/arm_ffa/common.h
> > @@ -14,8 +14,6 @@ typedef struct arm_smccc_1_2_regs ffa_value_t;
> > 
> >  typedef void (ffa_fn)(ffa_value_t, ffa_value_t *);
> > 
> > -int arm_ffa_bus_init(void);
> > -void arm_ffa_bus_exit(void);
> >  bool ffa_device_is_valid(struct ffa_device *ffa_dev);
> >  void ffa_device_match_uuid(struct ffa_device *ffa_dev, const uuid_t *uuid);
> > 
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index 159f2106a541..7cdd425c5fc4 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
> > @@ -1603,14 +1603,9 @@ static int __init ffa_init(void)
> >  	if (ret)
> >  		return ret;
> > 
> > -	ret = arm_ffa_bus_init();
> > -	if (ret)
> > -		return ret;
> > -
> >  	drv_info = kzalloc(sizeof(*drv_info), GFP_KERNEL);
> >  	if (!drv_info) {
> > -		ret = -ENOMEM;
> > -		goto ffa_bus_exit;
> > +		return -ENOMEM;
> >  	}
> > 
> >  	ret = ffa_version_check(&drv_info->version);
> > @@ -1671,11 +1666,9 @@ static int __init ffa_init(void)
> >  	free_pages_exact(drv_info->rx_buffer, RXTX_BUFFER_SIZE);
> >  free_drv_info:
> >  	kfree(drv_info);
> > -ffa_bus_exit:
> > -	arm_ffa_bus_exit();
> >  	return ret;
> >  }
> > -subsys_initcall(ffa_init);
> > +module_init(ffa_init);
> > 
> >  static void __exit ffa_exit(void)
> >  {
> > @@ -1685,7 +1678,6 @@ static void __exit ffa_exit(void)
> >  	free_pages_exact(drv_info->tx_buffer, RXTX_BUFFER_SIZE);
> >  	free_pages_exact(drv_info->rx_buffer, RXTX_BUFFER_SIZE);
> >  	kfree(drv_info);
> > -	arm_ffa_bus_exit();
> >  }
> >  module_exit(ffa_exit);
> > 
> > --
> > 2.43.2
> > 
> 
> Ack, Sebastian

I wasn't sure if that meant

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

So, I didn't add it intentionally in v2. Please ack/review v2 if you are
happy with that.

--
Regards,
Sudeep

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