diff mbox series

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

Message ID 20240510183811.3802285-2-sudeep.holla@arm.com (mailing list archive)
State New, archived
Headers show
Series [1/2] firmware: arm_ffa: Move the FF-A v1.0 NULL UUID workaround to bus notifier | expand

Commit Message

Sudeep Holla May 10, 2024, 6:38 p.m. UTC 
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

--
2.43.2

Comments

Sebastian Ene May 14, 2024, 2:54 p.m. UTC | #1 
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
Sudeep Holla May 15, 2024, 9:58 a.m. UTC | #2 
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
diff mbox series

Patch

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