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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Tue, Oct 24, 2023 at 12:56 PM Sudeep Holla <[email protected]> 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 <[email protected]>
> ---
> 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
>
On Wed, Oct 25, 2023 at 09:10:11AM +0200, Jens Wiklander wrote:
> On Tue, Oct 24, 2023 at 12:56 PM Sudeep Holla <[email protected]> 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 <[email protected]>
> > ---
> > 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
Hi Sudeep,
On Tue, Oct 24, 2023 at 12:56 PM Sudeep Holla <[email protected]> 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 <[email protected]>
> ---
> 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 <[email protected]>
Cheers,
Jens
> ---
> base-commit: bcefd1bf63b1ec9bb08067021cf47f0fad96f395
> change-id: 20231024-ffa-notification-fixes-9d5cf1e131ef
>
> Best regards,
> --
> Regards,
> Sudeep
>
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 <[email protected]> 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 <[email protected]>
> > ---
> > 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 <[email protected]>
Thanks a lot for testing, much appreciated!
--
Regards,
Sudeep