[PATCH 0/4] firmware: arm_ffa: Few fixes for FF-A notification support

[PATCH 0/4] firmware: arm_ffa: Few fixes for FF-A notification support

[Sudeep Holla]

Hi,

These are set of small fixes around FF-A notification support that are
currently queued in -next. It is mostly to take care of absence of
the notification support in the firmware as well as allowing them to be
optional and continue initialisation even when the notification fails.

Regards,
Sudeep

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
Sudeep Holla (4):
      firmware: arm_ffa: Allow FF-A initialisation even when notification fails
      firmware: arm_ffa: Setup the partitions after the notification initialisation
      firmware: arm_ffa: Add checks for the notification enabled state
      firmware: arm_ffa: Fix FFA notifications cleanup path

 drivers/firmware/arm_ffa/driver.c | 65 ++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 21 deletions(-)
---
base-commit: bcefd1bf63b1ec9bb08067021cf47f0fad96f395
change-id: 20231024-ffa-notification-fixes-9d5cf1e131ef

Best regards,
-- 
Regards,
Sudeep


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

[PATCH 1/4] firmware: arm_ffa: Allow FF-A initialisation even when notification fails

[Sudeep Holla]

FF-A notifications are optional feature in the specification. Currently
we allow to continue if the firmware reports no support for the
notifications. However, we fail to continue and complete the FF-A
driver initialisation if the notification setup fails for any reason.

Let us allow the FF-A driver to complete the initialisation even if the
notification fails to setup. We will just flag the error and continue
to provide other features in the driver.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 07b72c679247..b4ba52d674e5 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1390,20 +1390,20 @@ static void ffa_notifications_cleanup(void)
 	}
 }
 
-static int ffa_notifications_setup(void)
+static void ffa_notifications_setup(void)
 {
 	int ret, irq;
 
 	ret = ffa_features(FFA_NOTIFICATION_BITMAP_CREATE, 0, NULL, NULL);
 	if (ret) {
-		pr_err("Notifications not supported, continuing with it ..\n");
-		return 0;
+		pr_info("Notifications not supported, continuing with it ..\n");
+		return;
 	}
 
 	ret = ffa_notification_bitmap_create();
 	if (ret) {
-		pr_err("notification_bitmap_create error %d\n", ret);
-		return ret;
+		pr_info("Notification bitmap create error %d\n", ret);
+		return;
 	}
 	drv_info->bitmap_created = true;
 
@@ -1426,10 +1426,11 @@ static int ffa_notifications_setup(void)
 	ret = ffa_sched_recv_cb_update(drv_info->vm_id, ffa_self_notif_handle,
 				       drv_info, true);
 	if (!ret)
-		return ret;
+		return;
 cleanup:
+	pr_info("Notification setup failed %d, not enabled\n", ret);
 	ffa_notifications_cleanup();
-	return ret;
+	return;
 }
 
 static int __init ffa_init(void)
@@ -1487,13 +1488,9 @@ static int __init ffa_init(void)
 
 	ffa_set_up_mem_ops_native_flag();
 
-	ret = ffa_notifications_setup();
-	if (ret)
-		goto partitions_cleanup;
+	ffa_notifications_setup();
 
 	return 0;
-partitions_cleanup:
-	ffa_partitions_cleanup();
 free_pages:
 	if (drv_info->tx_buffer)
 		free_pages_exact(drv_info->tx_buffer, RXTX_BUFFER_SIZE);

-- 
2.42.0


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

Re: [PATCH 1/4] firmware: arm_ffa: Allow FF-A initialisation even when notification fails

[Jens Wiklander]

On Tue, Oct 24, 2023 at 12:56 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> FF-A notifications are optional feature in the specification. Currently
> we allow to continue if the firmware reports no support for the
> notifications. However, we fail to continue and complete the FF-A
> driver initialisation if the notification setup fails for any reason.
>
> Let us allow the FF-A driver to complete the initialisation even if the
> notification fails to setup. We will just flag the error and continue
> to provide other features in the driver.
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/firmware/arm_ffa/driver.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 07b72c679247..b4ba52d674e5 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1390,20 +1390,20 @@ static void ffa_notifications_cleanup(void)
>         }
>  }
>
> -static int ffa_notifications_setup(void)
> +static void ffa_notifications_setup(void)
>  {
>         int ret, irq;
>
>         ret = ffa_features(FFA_NOTIFICATION_BITMAP_CREATE, 0, NULL, NULL);
>         if (ret) {
> -               pr_err("Notifications not supported, continuing with it ..\n");
> -               return 0;
> +               pr_info("Notifications not supported, continuing with it ..\n");
> +               return;
>         }
>
>         ret = ffa_notification_bitmap_create();
>         if (ret) {
> -               pr_err("notification_bitmap_create error %d\n", ret);
> -               return ret;
> +               pr_info("Notification bitmap create error %d\n", ret);
> +               return;
>         }
>         drv_info->bitmap_created = true;
>
> @@ -1426,10 +1426,11 @@ static int ffa_notifications_setup(void)
>         ret = ffa_sched_recv_cb_update(drv_info->vm_id, ffa_self_notif_handle,
>                                        drv_info, true);
>         if (!ret)
> -               return ret;
> +               return;
>  cleanup:
> +       pr_info("Notification setup failed %d, not enabled\n", ret);
>         ffa_notifications_cleanup();
> -       return ret;
> +       return;

This return is redundant.

Thanks,
Jens

>  }
>
>  static int __init ffa_init(void)
> @@ -1487,13 +1488,9 @@ static int __init ffa_init(void)
>
>         ffa_set_up_mem_ops_native_flag();
>
> -       ret = ffa_notifications_setup();
> -       if (ret)
> -               goto partitions_cleanup;
> +       ffa_notifications_setup();
>
>         return 0;
> -partitions_cleanup:
> -       ffa_partitions_cleanup();
>  free_pages:
>         if (drv_info->tx_buffer)
>                 free_pages_exact(drv_info->tx_buffer, RXTX_BUFFER_SIZE);
>
> --
> 2.42.0
>

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

Re: [PATCH 1/4] firmware: arm_ffa: Allow FF-A initialisation even when notification fails

[Sudeep Holla]

On Wed, Oct 25, 2023 at 09:10:11AM +0200, Jens Wiklander wrote:
> On Tue, Oct 24, 2023 at 12:56 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > FF-A notifications are optional feature in the specification. Currently
> > we allow to continue if the firmware reports no support for the
> > notifications. However, we fail to continue and complete the FF-A
> > driver initialisation if the notification setup fails for any reason.
> >
> > Let us allow the FF-A driver to complete the initialisation even if the
> > notification fails to setup. We will just flag the error and continue
> > to provide other features in the driver.
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/firmware/arm_ffa/driver.c | 21 +++++++++------------
> >  1 file changed, 9 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> > index 07b72c679247..b4ba52d674e5 100644
> > --- a/drivers/firmware/arm_ffa/driver.c
> > +++ b/drivers/firmware/arm_ffa/driver.c
[..]
> > @@ -1426,10 +1426,11 @@ static int ffa_notifications_setup(void)
> >         ret = ffa_sched_recv_cb_update(drv_info->vm_id, ffa_self_notif_handle,
> >                                        drv_info, true);
> >         if (!ret)
> > -               return ret;
> > +               return;
> >  cleanup:
> > +       pr_info("Notification setup failed %d, not enabled\n", ret);
> >         ffa_notifications_cleanup();
> > -       return ret;
> > +       return;
>
> This return is redundant.
>

Thanks, will fix it.

--
Regards,
Sudeep

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

[PATCH 2/4] firmware: arm_ffa: Setup the partitions after the notification initialisation

[Sudeep Holla]

Currently the notifications are setup of the partitions are probed and
FF-A devices are added on the FF-A bus. The FF-A driver probe can be
called even before the FF-A notification setup happens which is wrong
and may result in failure or misbehaviour in the FF-A partition device
probe.

In order to ensure the FF-A notifications are setup before the FF-A
devices are probed, let us move the FF-A partition setup after the
completion of FF-A notification setup.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index b4ba52d674e5..0ab30b571a69 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1422,11 +1422,7 @@ static void ffa_notifications_setup(void)
 	hash_init(drv_info->notifier_hash);
 	mutex_init(&drv_info->notify_lock);
 
-	/* Register internal scheduling callback */
-	ret = ffa_sched_recv_cb_update(drv_info->vm_id, ffa_self_notif_handle,
-				       drv_info, true);
-	if (!ret)
-		return;
+	return;
 cleanup:
 	pr_info("Notification setup failed %d, not enabled\n", ret);
 	ffa_notifications_cleanup();
@@ -1484,12 +1480,17 @@ static int __init ffa_init(void)
 	mutex_init(&drv_info->rx_lock);
 	mutex_init(&drv_info->tx_lock);
 
-	ffa_setup_partitions();
-
 	ffa_set_up_mem_ops_native_flag();
 
 	ffa_notifications_setup();
 
+	ffa_setup_partitions();
+
+	ret = ffa_sched_recv_cb_update(drv_info->vm_id, ffa_self_notif_handle,
+				       drv_info, true);
+	if (ret)
+		pr_info("Failed to register driver sched callback %d\n", ret);
+
 	return 0;
 free_pages:
 	if (drv_info->tx_buffer)

-- 
2.42.0


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

[PATCH 3/4] firmware: arm_ffa: Add checks for the notification enabled state

[Sudeep Holla]

We need to check if the FF-A notifications are enabled or not in all
the notification operations that are accessible for the FF-A device
from the FF-A driver. This helps to avoid making calls to the FF-A
firmware even if the notification setup has failed.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 0ab30b571a69..b097452597a2 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -99,6 +99,7 @@ struct ffa_drv_info {
 	void *tx_buffer;
 	bool mem_ops_native;
 	bool bitmap_created;
+	bool notif_enabled;
 	unsigned int sched_recv_irq;
 	unsigned int cpuhp_state;
 	struct ffa_pcpu_irq __percpu *irq_pcpu;
@@ -889,6 +890,8 @@ static int ffa_memory_lend(struct ffa_mem_ops_args *args)
 
 #define FFA_SECURE_PARTITION_ID_FLAG	BIT(15)
 
+#define ffa_notifications_disabled()	(!drv_info->notif_enabled)
+
 enum notify_type {
 	NON_SECURE_VM,
 	SECURE_PARTITION,
@@ -908,6 +911,9 @@ static int ffa_sched_recv_cb_update(u16 part_id, ffa_sched_recv_cb callback,
 	struct ffa_dev_part_info *partition;
 	bool cb_valid;
 
+	if (ffa_notifications_disabled())
+		return -EOPNOTSUPP;
+
 	partition = xa_load(&drv_info->partition_info, part_id);
 	write_lock(&partition->rw_lock);
 
@@ -1001,6 +1007,9 @@ static int ffa_notify_relinquish(struct ffa_device *dev, int notify_id)
 	int rc;
 	enum notify_type type = ffa_notify_type_get(dev->vm_id);
 
+	if (ffa_notifications_disabled())
+		return -EOPNOTSUPP;
+
 	if (notify_id >= FFA_MAX_NOTIFICATIONS)
 		return -EINVAL;
 
@@ -1027,6 +1036,9 @@ static int ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
 	u32 flags = 0;
 	enum notify_type type = ffa_notify_type_get(dev->vm_id);
 
+	if (ffa_notifications_disabled())
+		return -EOPNOTSUPP;
+
 	if (notify_id >= FFA_MAX_NOTIFICATIONS)
 		return -EINVAL;
 
@@ -1057,6 +1069,9 @@ static int ffa_notify_send(struct ffa_device *dev, int notify_id,
 {
 	u32 flags = 0;
 
+	if (ffa_notifications_disabled())
+		return -EOPNOTSUPP;
+
 	if (is_per_vcpu)
 		flags |= (PER_VCPU_NOTIFICATION_FLAG | vcpu << 16);
 
@@ -1388,6 +1403,7 @@ static void ffa_notifications_cleanup(void)
 		ffa_notification_bitmap_destroy();
 		drv_info->bitmap_created = false;
 	}
+	drv_info->notif_enabled = false;
 }
 
 static void ffa_notifications_setup(void)
@@ -1422,6 +1438,7 @@ static void ffa_notifications_setup(void)
 	hash_init(drv_info->notifier_hash);
 	mutex_init(&drv_info->notify_lock);
 
+	drv_info->notif_enabled = true;
 	return;
 cleanup:
 	pr_info("Notification setup failed %d, not enabled\n", ret);

-- 
2.42.0


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

[PATCH 4/4] firmware: arm_ffa: Fix FFA notifications cleanup path

[Sudeep Holla]

We allow the FF-A to be initialised successfully even when notification
fails. When the notification fails, ffa_notifications_cleanup() gets
called on the failure path.

However, the driver information about the notifications like the irq,
workqueues and cpu hotplug state for enabling and disabling percpu IRQ
are not cleared. This may result in unexpected behaviour during CPU
hotplug because of percpu IRQ being enabled and disabled or during the
driver removal when ffa_notifications_cleanup() gets executed again.

Fix the FFA notifications cleanup path by clearing all the notification
related driver information.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/firmware/arm_ffa/driver.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index b097452597a2..e636181694aa 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1326,8 +1326,10 @@ static int ffa_sched_recv_irq_map(void)
 
 static void ffa_sched_recv_irq_unmap(void)
 {
-	if (drv_info->sched_recv_irq)
+	if (drv_info->sched_recv_irq) {
 		irq_dispose_mapping(drv_info->sched_recv_irq);
+		drv_info->sched_recv_irq = 0;
+	}
 }
 
 static int ffa_cpuhp_pcpu_irq_enable(unsigned int cpu)
@@ -1344,17 +1346,23 @@ static int ffa_cpuhp_pcpu_irq_disable(unsigned int cpu)
 
 static void ffa_uninit_pcpu_irq(void)
 {
-	if (drv_info->cpuhp_state)
+	if (drv_info->cpuhp_state) {
 		cpuhp_remove_state(drv_info->cpuhp_state);
+		drv_info->cpuhp_state = 0;
+	}
 
-	if (drv_info->notif_pcpu_wq)
+	if (drv_info->notif_pcpu_wq) {
 		destroy_workqueue(drv_info->notif_pcpu_wq);
+		drv_info->notif_pcpu_wq = NULL;
+	}
 
 	if (drv_info->sched_recv_irq)
 		free_percpu_irq(drv_info->sched_recv_irq, drv_info->irq_pcpu);
 
-	if (drv_info->irq_pcpu)
+	if (drv_info->irq_pcpu) {
 		free_percpu(drv_info->irq_pcpu);
+		drv_info->irq_pcpu = NULL;
+	}
 }
 
 static int ffa_init_pcpu_irq(unsigned int irq)

-- 
2.42.0


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

Re: [PATCH 0/4] firmware: arm_ffa: Few fixes for FF-A notification support

[Jens Wiklander]

Hi Sudeep,

On Tue, Oct 24, 2023 at 12:56 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Hi,
>
> These are set of small fixes around FF-A notification support that are
> currently queued in -next. It is mostly to take care of absence of
> the notification support in the firmware as well as allowing them to be
> optional and continue initialisation even when the notification fails.
>
> Regards,
> Sudeep
>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> Sudeep Holla (4):
>       firmware: arm_ffa: Allow FF-A initialisation even when notification fails
>       firmware: arm_ffa: Setup the partitions after the notification initialisation
>       firmware: arm_ffa: Add checks for the notification enabled state
>       firmware: arm_ffa: Fix FFA notifications cleanup path
>
>  drivers/firmware/arm_ffa/driver.c | 65 ++++++++++++++++++++++++++-------------
>  1 file changed, 44 insertions(+), 21 deletions(-)

Works as expected with and without FF-A notifications enabled in the
secure world.

Tested-by: Jens Wiklander <jens.wiklander@linaro.org>

Cheers,
Jens

> ---
> base-commit: bcefd1bf63b1ec9bb08067021cf47f0fad96f395
> change-id: 20231024-ffa-notification-fixes-9d5cf1e131ef
>
> Best regards,
> --
> Regards,
> Sudeep
>

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

Re: [PATCH 0/4] firmware: arm_ffa: Few fixes for FF-A notification support

[Sudeep Holla]

On Wed, Oct 25, 2023 at 01:55:38PM +0200, Jens Wiklander wrote:
> Hi Sudeep,
>
> On Tue, Oct 24, 2023 at 12:56 PM Sudeep Holla <sudeep.holla@arm.com> wrote:
> >
> > Hi,
> >
> > These are set of small fixes around FF-A notification support that are
> > currently queued in -next. It is mostly to take care of absence of
> > the notification support in the firmware as well as allowing them to be
> > optional and continue initialisation even when the notification fails.
> >
> > Regards,
> > Sudeep
> >
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> > Sudeep Holla (4):
> >       firmware: arm_ffa: Allow FF-A initialisation even when notification fails
> >       firmware: arm_ffa: Setup the partitions after the notification initialisation
> >       firmware: arm_ffa: Add checks for the notification enabled state
> >       firmware: arm_ffa: Fix FFA notifications cleanup path
> >
> >  drivers/firmware/arm_ffa/driver.c | 65 ++++++++++++++++++++++++++-------------
> >  1 file changed, 44 insertions(+), 21 deletions(-)
>
> Works as expected with and without FF-A notifications enabled in the
> secure world.
>
> Tested-by: Jens Wiklander <jens.wiklander@linaro.org>

Thanks a lot for testing, much appreciated!

--
Regards,
Sudeep

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

Re: [PATCH 0/4] firmware: arm_ffa: Few fixes for FF-A notification support

[Sudeep Holla]

On Tue, 24 Oct 2023 11:56:16 +0100, Sudeep Holla wrote:
> These are set of small fixes around FF-A notification support that are
> currently queued in -next. It is mostly to take care of absence of
> the notification support in the firmware as well as allowing them to be
> optional and continue initialisation even when the notification fails.
> 
> Regards,
> Sudeep
>
> [...]

Applied to sudeep.holla/linux (for-next/ffa/fixes), thanks!

[1/4] firmware: arm_ffa: Allow FF-A initialisation even when notification fails
      (Dropped redundant return before applying as suggested by Jens)
      https://git.kernel.org/sudeep.holla/c/95520fc07743
[2/4] firmware: arm_ffa: Setup the partitions after the notification initialisation
      https://git.kernel.org/sudeep.holla/c/6f47023f7a52
[3/4] firmware: arm_ffa: Add checks for the notification enabled state
      https://git.kernel.org/sudeep.holla/c/f4bfcaee34bc
[4/4] firmware: arm_ffa: Fix FFA notifications cleanup path
      https://git.kernel.org/sudeep.holla/c/6d67cbe67a86
--
Regards,
Sudeep


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