* [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node
2025-05-28 8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
@ 2025-05-28 8:49 ` Sudeep Holla
2025-05-28 15:31 ` Jens Wiklander
2025-05-28 8:49 ` [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking Sudeep Holla
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2025-05-28 8:49 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, Sudeep Holla, Jens Wiklander,
Jérôme Forissier
Commit e0573444edbf ("firmware: arm_ffa: Add interfaces to request
notification callbacks") adds support for notifier callbacks by allocating
and inserting a callback node into a hashtable during registration of
notifiers. However, during unregistration, the code only removes the
node from the hashtable without freeing the associated memory, resulting
in a memory leak.
Resolve the memory leak issue by ensuring the allocated notifier callback
node is properly freed after it is removed from the hashtable entry.
Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/firmware/arm_ffa/driver.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index fe55613a8ea993378474671d3c5756309da34fbc..6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1284,6 +1284,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
} else {
hash_del(&cb_info->hnode);
+ kfree(cb_info);
}
return 0;
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node
2025-05-28 8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
@ 2025-05-28 15:31 ` Jens Wiklander
0 siblings, 0 replies; 8+ messages in thread
From: Jens Wiklander @ 2025-05-28 15:31 UTC (permalink / raw)
To: Sudeep Holla; +Cc: linux-arm-kernel, linux-kernel, Jérôme Forissier
On Wed, May 28, 2025 at 10:50 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> Commit e0573444edbf ("firmware: arm_ffa: Add interfaces to request
> notification callbacks") adds support for notifier callbacks by allocating
> and inserting a callback node into a hashtable during registration of
> notifiers. However, during unregistration, the code only removes the
> node from the hashtable without freeing the associated memory, resulting
> in a memory leak.
>
> Resolve the memory leak issue by ensuring the allocated notifier callback
> node is properly freed after it is removed from the hashtable entry.
>
> Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/firmware/arm_ffa/driver.c | 1 +
> 1 file changed, 1 insertion(+)
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Cheers,
Jens
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index fe55613a8ea993378474671d3c5756309da34fbc..6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1284,6 +1284,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
> hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
> } else {
> hash_del(&cb_info->hnode);
> + kfree(cb_info);
> }
>
> return 0;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking
2025-05-28 8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
2025-05-28 8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
@ 2025-05-28 8:49 ` Sudeep Holla
2025-05-28 15:33 ` Jens Wiklander
2025-05-28 8:49 ` [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context Sudeep Holla
2025-06-09 10:31 ` [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
3 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2025-05-28 8:49 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, Sudeep Holla, Jens Wiklander,
Jérôme Forissier
The notifier callback node allocation is currently done while holding
the notify_lock mutex. While this is safe even if memory allocation may
sleep, we need to move the allocation outside the locked region in
preparation to move from using muxtes to rwlocks.
Move the memory allocation to avoid potential sleeping in atomic context
once the locks are moved from mutex to rwlocks.
Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/firmware/arm_ffa/driver.c | 48 +++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 24 deletions(-)
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de..44eecb786e67b205161e2d48602e1f1b53533360 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -1250,13 +1250,12 @@ notifier_hnode_get_by_type(u16 notify_id, enum notify_type type)
return NULL;
}
-static int
-update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
- void *cb_data, bool is_registration, bool is_framework)
+static int update_notifier_cb(struct ffa_device *dev, int notify_id,
+ struct notifier_cb_info *cb, bool is_framework)
{
struct notifier_cb_info *cb_info = NULL;
enum notify_type type = ffa_notify_type_get(dev->vm_id);
- bool cb_found;
+ bool cb_found, is_registration = !!cb;
if (is_framework)
cb_info = notifier_hnode_get_by_vmid_uuid(notify_id, dev->vm_id,
@@ -1270,18 +1269,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
return -EINVAL;
if (is_registration) {
- cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
- if (!cb_info)
- return -ENOMEM;
-
- cb_info->dev = dev;
- cb_info->cb_data = cb_data;
- if (is_framework)
- cb_info->fwk_cb = cb;
- else
- cb_info->cb = cb;
-
- hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
+ hash_add(drv_info->notifier_hash, &cb->hnode, notify_id);
} else {
hash_del(&cb_info->hnode);
kfree(cb_info);
@@ -1303,8 +1291,7 @@ static int __ffa_notify_relinquish(struct ffa_device *dev, int notify_id,
mutex_lock(&drv_info->notify_lock);
- rc = update_notifier_cb(dev, notify_id, NULL, NULL, false,
- is_framework);
+ rc = update_notifier_cb(dev, notify_id, NULL, is_framework);
if (rc) {
pr_err("Could not unregister notification callback\n");
mutex_unlock(&drv_info->notify_lock);
@@ -1335,6 +1322,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
{
int rc;
u32 flags = 0;
+ struct notifier_cb_info *cb_info = NULL;
if (ffa_notifications_disabled())
return -EOPNOTSUPP;
@@ -1342,6 +1330,17 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
if (notify_id >= FFA_MAX_NOTIFICATIONS)
return -EINVAL;
+ cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
+ if (!cb_info)
+ return -ENOMEM;
+
+ cb_info->dev = dev;
+ cb_info->cb_data = cb_data;
+ if (is_framework)
+ cb_info->fwk_cb = cb;
+ else
+ cb_info->cb = cb;
+
mutex_lock(&drv_info->notify_lock);
if (!is_framework) {
@@ -1349,21 +1348,22 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
flags = PER_VCPU_NOTIFICATION_FLAG;
rc = ffa_notification_bind(dev->vm_id, BIT(notify_id), flags);
- if (rc) {
- mutex_unlock(&drv_info->notify_lock);
- return rc;
- }
+ if (rc)
+ goto out_unlock_free;
}
- rc = update_notifier_cb(dev, notify_id, cb, cb_data, true,
- is_framework);
+ rc = update_notifier_cb(dev, notify_id, cb_info, is_framework);
if (rc) {
pr_err("Failed to register callback for %d - %d\n",
notify_id, rc);
if (!is_framework)
ffa_notification_unbind(dev->vm_id, BIT(notify_id));
}
+
+out_unlock_free:
mutex_unlock(&drv_info->notify_lock);
+ if (rc)
+ kfree(cb_info);
return rc;
}
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking
2025-05-28 8:49 ` [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking Sudeep Holla
@ 2025-05-28 15:33 ` Jens Wiklander
0 siblings, 0 replies; 8+ messages in thread
From: Jens Wiklander @ 2025-05-28 15:33 UTC (permalink / raw)
To: Sudeep Holla; +Cc: linux-arm-kernel, linux-kernel, Jérôme Forissier
On Wed, May 28, 2025 at 10:50 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The notifier callback node allocation is currently done while holding
> the notify_lock mutex. While this is safe even if memory allocation may
> sleep, we need to move the allocation outside the locked region in
> preparation to move from using muxtes to rwlocks.
>
> Move the memory allocation to avoid potential sleeping in atomic context
> once the locks are moved from mutex to rwlocks.
>
> Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/firmware/arm_ffa/driver.c | 48 +++++++++++++++++++--------------------
> 1 file changed, 24 insertions(+), 24 deletions(-)
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Cheers,
Jens
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 6f75cdf29720993b1cd95eb7d3a36d01b0fdd1de..44eecb786e67b205161e2d48602e1f1b53533360 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -1250,13 +1250,12 @@ notifier_hnode_get_by_type(u16 notify_id, enum notify_type type)
> return NULL;
> }
>
> -static int
> -update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
> - void *cb_data, bool is_registration, bool is_framework)
> +static int update_notifier_cb(struct ffa_device *dev, int notify_id,
> + struct notifier_cb_info *cb, bool is_framework)
> {
> struct notifier_cb_info *cb_info = NULL;
> enum notify_type type = ffa_notify_type_get(dev->vm_id);
> - bool cb_found;
> + bool cb_found, is_registration = !!cb;
>
> if (is_framework)
> cb_info = notifier_hnode_get_by_vmid_uuid(notify_id, dev->vm_id,
> @@ -1270,18 +1269,7 @@ update_notifier_cb(struct ffa_device *dev, int notify_id, void *cb,
> return -EINVAL;
>
> if (is_registration) {
> - cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
> - if (!cb_info)
> - return -ENOMEM;
> -
> - cb_info->dev = dev;
> - cb_info->cb_data = cb_data;
> - if (is_framework)
> - cb_info->fwk_cb = cb;
> - else
> - cb_info->cb = cb;
> -
> - hash_add(drv_info->notifier_hash, &cb_info->hnode, notify_id);
> + hash_add(drv_info->notifier_hash, &cb->hnode, notify_id);
> } else {
> hash_del(&cb_info->hnode);
> kfree(cb_info);
> @@ -1303,8 +1291,7 @@ static int __ffa_notify_relinquish(struct ffa_device *dev, int notify_id,
>
> mutex_lock(&drv_info->notify_lock);
>
> - rc = update_notifier_cb(dev, notify_id, NULL, NULL, false,
> - is_framework);
> + rc = update_notifier_cb(dev, notify_id, NULL, is_framework);
> if (rc) {
> pr_err("Could not unregister notification callback\n");
> mutex_unlock(&drv_info->notify_lock);
> @@ -1335,6 +1322,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
> {
> int rc;
> u32 flags = 0;
> + struct notifier_cb_info *cb_info = NULL;
>
> if (ffa_notifications_disabled())
> return -EOPNOTSUPP;
> @@ -1342,6 +1330,17 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
> if (notify_id >= FFA_MAX_NOTIFICATIONS)
> return -EINVAL;
>
> + cb_info = kzalloc(sizeof(*cb_info), GFP_KERNEL);
> + if (!cb_info)
> + return -ENOMEM;
> +
> + cb_info->dev = dev;
> + cb_info->cb_data = cb_data;
> + if (is_framework)
> + cb_info->fwk_cb = cb;
> + else
> + cb_info->cb = cb;
> +
> mutex_lock(&drv_info->notify_lock);
>
> if (!is_framework) {
> @@ -1349,21 +1348,22 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
> flags = PER_VCPU_NOTIFICATION_FLAG;
>
> rc = ffa_notification_bind(dev->vm_id, BIT(notify_id), flags);
> - if (rc) {
> - mutex_unlock(&drv_info->notify_lock);
> - return rc;
> - }
> + if (rc)
> + goto out_unlock_free;
> }
>
> - rc = update_notifier_cb(dev, notify_id, cb, cb_data, true,
> - is_framework);
> + rc = update_notifier_cb(dev, notify_id, cb_info, is_framework);
> if (rc) {
> pr_err("Failed to register callback for %d - %d\n",
> notify_id, rc);
> if (!is_framework)
> ffa_notification_unbind(dev->vm_id, BIT(notify_id));
> }
> +
> +out_unlock_free:
> mutex_unlock(&drv_info->notify_lock);
> + if (rc)
> + kfree(cb_info);
>
> return rc;
> }
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context
2025-05-28 8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
2025-05-28 8:49 ` [PATCH 1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node Sudeep Holla
2025-05-28 8:49 ` [PATCH 2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking Sudeep Holla
@ 2025-05-28 8:49 ` Sudeep Holla
2025-05-28 15:34 ` Jens Wiklander
2025-06-09 10:31 ` [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
3 siblings, 1 reply; 8+ messages in thread
From: Sudeep Holla @ 2025-05-28 8:49 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, Sudeep Holla, Jens Wiklander,
Jérôme Forissier
The current use of a mutex to protect the notifier hashtable accesses
can lead to issues in the atomic context. It results in the below
kernel warnings:
| BUG: sleeping function called from invalid context at kernel/locking/mutex.c:258
| in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 9, name: kworker/0:0
| preempt_count: 1, expected: 0
| RCU nest depth: 0, expected: 0
| CPU: 0 UID: 0 PID: 9 Comm: kworker/0:0 Not tainted 6.14.0 #4
| Workqueue: ffa_pcpu_irq_notification notif_pcpu_irq_work_fn
| Call trace:
| show_stack+0x18/0x24 (C)
| dump_stack_lvl+0x78/0x90
| dump_stack+0x18/0x24
| __might_resched+0x114/0x170
| __might_sleep+0x48/0x98
| mutex_lock+0x24/0x80
| handle_notif_callbacks+0x54/0xe0
| notif_get_and_handle+0x40/0x88
| generic_exec_single+0x80/0xc0
| smp_call_function_single+0xfc/0x1a0
| notif_pcpu_irq_work_fn+0x2c/0x38
| process_one_work+0x14c/0x2b4
| worker_thread+0x2e4/0x3e0
| kthread+0x13c/0x210
| ret_from_fork+0x10/0x20
To address this, replace the mutex with an rwlock to protect the notifier
hashtable accesses. This ensures that read-side locking does not sleep and
multiple readers can acquire the lock concurrently, avoiding unnecessary
contention and potential deadlocks. Writer access remains exclusive,
preserving correctness.
This change resolves warnings from lockdep about potential sleep in
atomic context.
Cc: Jens Wiklander <jens.wiklander@linaro.org>
Reported-by: Jérôme Forissier <jerome.forissier@linaro.org>
Closes: https://github.com/OP-TEE/optee_os/issues/7394
Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
drivers/firmware/arm_ffa/driver.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
index 44eecb786e67b205161e2d48602e1f1b53533360..37eb2e6c2f9f4d30831b7bf6ce5142a39a19f50c 100644
--- a/drivers/firmware/arm_ffa/driver.c
+++ b/drivers/firmware/arm_ffa/driver.c
@@ -110,7 +110,7 @@ struct ffa_drv_info {
struct work_struct sched_recv_irq_work;
struct xarray partition_info;
DECLARE_HASHTABLE(notifier_hash, ilog2(FFA_MAX_NOTIFICATIONS));
- struct mutex notify_lock; /* lock to protect notifier hashtable */
+ rwlock_t notify_lock; /* lock to protect notifier hashtable */
};
static struct ffa_drv_info *drv_info;
@@ -1289,19 +1289,19 @@ static int __ffa_notify_relinquish(struct ffa_device *dev, int notify_id,
if (notify_id >= FFA_MAX_NOTIFICATIONS)
return -EINVAL;
- mutex_lock(&drv_info->notify_lock);
+ write_lock(&drv_info->notify_lock);
rc = update_notifier_cb(dev, notify_id, NULL, is_framework);
if (rc) {
pr_err("Could not unregister notification callback\n");
- mutex_unlock(&drv_info->notify_lock);
+ write_unlock(&drv_info->notify_lock);
return rc;
}
if (!is_framework)
rc = ffa_notification_unbind(dev->vm_id, BIT(notify_id));
- mutex_unlock(&drv_info->notify_lock);
+ write_unlock(&drv_info->notify_lock);
return rc;
}
@@ -1341,7 +1341,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
else
cb_info->cb = cb;
- mutex_lock(&drv_info->notify_lock);
+ write_lock(&drv_info->notify_lock);
if (!is_framework) {
if (is_per_vcpu)
@@ -1361,7 +1361,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
}
out_unlock_free:
- mutex_unlock(&drv_info->notify_lock);
+ write_unlock(&drv_info->notify_lock);
if (rc)
kfree(cb_info);
@@ -1407,9 +1407,9 @@ static void handle_notif_callbacks(u64 bitmap, enum notify_type type)
if (!(bitmap & 1))
continue;
- mutex_lock(&drv_info->notify_lock);
+ read_lock(&drv_info->notify_lock);
cb_info = notifier_hnode_get_by_type(notify_id, type);
- mutex_unlock(&drv_info->notify_lock);
+ read_unlock(&drv_info->notify_lock);
if (cb_info && cb_info->cb)
cb_info->cb(notify_id, cb_info->cb_data);
@@ -1447,9 +1447,9 @@ static void handle_fwk_notif_callbacks(u32 bitmap)
ffa_rx_release();
- mutex_lock(&drv_info->notify_lock);
+ read_lock(&drv_info->notify_lock);
cb_info = notifier_hnode_get_by_vmid_uuid(notify_id, target, &uuid);
- mutex_unlock(&drv_info->notify_lock);
+ read_unlock(&drv_info->notify_lock);
if (cb_info && cb_info->fwk_cb)
cb_info->fwk_cb(notify_id, cb_info->cb_data, buf);
@@ -1974,7 +1974,7 @@ static void ffa_notifications_setup(void)
goto cleanup;
hash_init(drv_info->notifier_hash);
- mutex_init(&drv_info->notify_lock);
+ rwlock_init(&drv_info->notify_lock);
drv_info->notif_enabled = true;
return;
--
2.34.1
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context
2025-05-28 8:49 ` [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context Sudeep Holla
@ 2025-05-28 15:34 ` Jens Wiklander
0 siblings, 0 replies; 8+ messages in thread
From: Jens Wiklander @ 2025-05-28 15:34 UTC (permalink / raw)
To: Sudeep Holla; +Cc: linux-arm-kernel, linux-kernel, Jérôme Forissier
On Wed, May 28, 2025 at 10:50 AM Sudeep Holla <sudeep.holla@arm.com> wrote:
>
> The current use of a mutex to protect the notifier hashtable accesses
> can lead to issues in the atomic context. It results in the below
> kernel warnings:
>
> | BUG: sleeping function called from invalid context at kernel/locking/mutex.c:258
> | in_atomic(): 1, irqs_disabled(): 1, non_block: 0, pid: 9, name: kworker/0:0
> | preempt_count: 1, expected: 0
> | RCU nest depth: 0, expected: 0
> | CPU: 0 UID: 0 PID: 9 Comm: kworker/0:0 Not tainted 6.14.0 #4
> | Workqueue: ffa_pcpu_irq_notification notif_pcpu_irq_work_fn
> | Call trace:
> | show_stack+0x18/0x24 (C)
> | dump_stack_lvl+0x78/0x90
> | dump_stack+0x18/0x24
> | __might_resched+0x114/0x170
> | __might_sleep+0x48/0x98
> | mutex_lock+0x24/0x80
> | handle_notif_callbacks+0x54/0xe0
> | notif_get_and_handle+0x40/0x88
> | generic_exec_single+0x80/0xc0
> | smp_call_function_single+0xfc/0x1a0
> | notif_pcpu_irq_work_fn+0x2c/0x38
> | process_one_work+0x14c/0x2b4
> | worker_thread+0x2e4/0x3e0
> | kthread+0x13c/0x210
> | ret_from_fork+0x10/0x20
>
> To address this, replace the mutex with an rwlock to protect the notifier
> hashtable accesses. This ensures that read-side locking does not sleep and
> multiple readers can acquire the lock concurrently, avoiding unnecessary
> contention and potential deadlocks. Writer access remains exclusive,
> preserving correctness.
>
> This change resolves warnings from lockdep about potential sleep in
> atomic context.
>
> Cc: Jens Wiklander <jens.wiklander@linaro.org>
> Reported-by: Jérôme Forissier <jerome.forissier@linaro.org>
> Closes: https://github.com/OP-TEE/optee_os/issues/7394
> Fixes: e0573444edbf ("firmware: arm_ffa: Add interfaces to request notification callbacks")
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
> drivers/firmware/arm_ffa/driver.c | 22 +++++++++++-----------
> 1 file changed, 11 insertions(+), 11 deletions(-)
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Tested-by: Jens Wiklander <jens.wiklander@linaro.org>
Cheers,
Jens
>
> diff --git a/drivers/firmware/arm_ffa/driver.c b/drivers/firmware/arm_ffa/driver.c
> index 44eecb786e67b205161e2d48602e1f1b53533360..37eb2e6c2f9f4d30831b7bf6ce5142a39a19f50c 100644
> --- a/drivers/firmware/arm_ffa/driver.c
> +++ b/drivers/firmware/arm_ffa/driver.c
> @@ -110,7 +110,7 @@ struct ffa_drv_info {
> struct work_struct sched_recv_irq_work;
> struct xarray partition_info;
> DECLARE_HASHTABLE(notifier_hash, ilog2(FFA_MAX_NOTIFICATIONS));
> - struct mutex notify_lock; /* lock to protect notifier hashtable */
> + rwlock_t notify_lock; /* lock to protect notifier hashtable */
> };
>
> static struct ffa_drv_info *drv_info;
> @@ -1289,19 +1289,19 @@ static int __ffa_notify_relinquish(struct ffa_device *dev, int notify_id,
> if (notify_id >= FFA_MAX_NOTIFICATIONS)
> return -EINVAL;
>
> - mutex_lock(&drv_info->notify_lock);
> + write_lock(&drv_info->notify_lock);
>
> rc = update_notifier_cb(dev, notify_id, NULL, is_framework);
> if (rc) {
> pr_err("Could not unregister notification callback\n");
> - mutex_unlock(&drv_info->notify_lock);
> + write_unlock(&drv_info->notify_lock);
> return rc;
> }
>
> if (!is_framework)
> rc = ffa_notification_unbind(dev->vm_id, BIT(notify_id));
>
> - mutex_unlock(&drv_info->notify_lock);
> + write_unlock(&drv_info->notify_lock);
>
> return rc;
> }
> @@ -1341,7 +1341,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
> else
> cb_info->cb = cb;
>
> - mutex_lock(&drv_info->notify_lock);
> + write_lock(&drv_info->notify_lock);
>
> if (!is_framework) {
> if (is_per_vcpu)
> @@ -1361,7 +1361,7 @@ static int __ffa_notify_request(struct ffa_device *dev, bool is_per_vcpu,
> }
>
> out_unlock_free:
> - mutex_unlock(&drv_info->notify_lock);
> + write_unlock(&drv_info->notify_lock);
> if (rc)
> kfree(cb_info);
>
> @@ -1407,9 +1407,9 @@ static void handle_notif_callbacks(u64 bitmap, enum notify_type type)
> if (!(bitmap & 1))
> continue;
>
> - mutex_lock(&drv_info->notify_lock);
> + read_lock(&drv_info->notify_lock);
> cb_info = notifier_hnode_get_by_type(notify_id, type);
> - mutex_unlock(&drv_info->notify_lock);
> + read_unlock(&drv_info->notify_lock);
>
> if (cb_info && cb_info->cb)
> cb_info->cb(notify_id, cb_info->cb_data);
> @@ -1447,9 +1447,9 @@ static void handle_fwk_notif_callbacks(u32 bitmap)
>
> ffa_rx_release();
>
> - mutex_lock(&drv_info->notify_lock);
> + read_lock(&drv_info->notify_lock);
> cb_info = notifier_hnode_get_by_vmid_uuid(notify_id, target, &uuid);
> - mutex_unlock(&drv_info->notify_lock);
> + read_unlock(&drv_info->notify_lock);
>
> if (cb_info && cb_info->fwk_cb)
> cb_info->fwk_cb(notify_id, cb_info->cb_data, buf);
> @@ -1974,7 +1974,7 @@ static void ffa_notifications_setup(void)
> goto cleanup;
>
> hash_init(drv_info->notifier_hash);
> - mutex_init(&drv_info->notify_lock);
> + rwlock_init(&drv_info->notify_lock);
>
> drv_info->notif_enabled = true;
> return;
>
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling
2025-05-28 8:49 [PATCH 0/3] firmware: arm_ffa: Fixes for FF-A notification handling Sudeep Holla
` (2 preceding siblings ...)
2025-05-28 8:49 ` [PATCH 3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context Sudeep Holla
@ 2025-06-09 10:31 ` Sudeep Holla
3 siblings, 0 replies; 8+ messages in thread
From: Sudeep Holla @ 2025-06-09 10:31 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, Jens Wiklander,
Jérôme Forissier, Sudeep Holla
On Wed, 28 May 2025 09:49:40 +0100, Sudeep Holla wrote:
> This patch series improves the arm_ffa notification callback handling by
> addressing safety and memory issues. It replaces a mutex with an rwlock
> to prevent sleeping in atomic context, resolving kernel warnings. Memory
> allocation is moved outside the lock to support this transition safely.
> Additionally, a memory leak in the notifier unregistration path is fixed
> by properly freeing the callback node.
>
> [...]
Applied to sudeep.holla/linux (for-next/ffa/fixes), thanks!
[1/3] firmware: arm_ffa: Fix memory leak by freeing notifier callback node
https://git.kernel.org/sudeep.holla/c/a833d31ad867
[2/3] firmware: arm_ffa: Move memory allocation outside the mutex locking
https://git.kernel.org/sudeep.holla/c/27e850c88df0
[3/3] firmware: arm_ffa: Replace mutex with rwlock to avoid sleep in atomic context
https://git.kernel.org/sudeep.holla/c/9ca7a421229b
--
Regards,
Sudeep
^ permalink raw reply [flat|nested] 8+ messages in thread