smp_call_function_single disables IRQs when executing the callback. To
prevent deadlocks, we must disable IRQs when taking cgr_lock elsewhere.
This is already done by qman_update_cgr and qman_delete_cgr; fix the
other lockers.
Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver")
Signed-off-by: Sean Anderson <[email protected]>
---
Changes in v2:
- Fix one additional call to spin_unlock
drivers/soc/fsl/qbman/qman.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 739e4eee6b75..1bf1f1ea67f0 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -1456,11 +1456,11 @@ static void qm_congestion_task(struct work_struct *work)
union qm_mc_result *mcr;
struct qman_cgr *cgr;
- spin_lock(&p->cgr_lock);
+ spin_lock_irq(&p->cgr_lock);
qm_mc_start(&p->p);
qm_mc_commit(&p->p, QM_MCC_VERB_QUERYCONGESTION);
if (!qm_mc_result_timeout(&p->p, &mcr)) {
- spin_unlock(&p->cgr_lock);
+ spin_unlock_irq(&p->cgr_lock);
dev_crit(p->config->dev, "QUERYCONGESTION timeout\n");
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
return;
@@ -1476,7 +1476,7 @@ static void qm_congestion_task(struct work_struct *work)
list_for_each_entry(cgr, &p->cgr_cbs, node)
if (cgr->cb && qman_cgrs_get(&c, cgr->cgrid))
cgr->cb(p, cgr, qman_cgrs_get(&rr, cgr->cgrid));
- spin_unlock(&p->cgr_lock);
+ spin_unlock_irq(&p->cgr_lock);
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
}
@@ -2440,7 +2440,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
preempt_enable();
cgr->chan = p->config->channel;
- spin_lock(&p->cgr_lock);
+ spin_lock_irq(&p->cgr_lock);
if (opts) {
struct qm_mcc_initcgr local_opts = *opts;
@@ -2477,7 +2477,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
qman_cgrs_get(&p->cgrs[1], cgr->cgrid))
cgr->cb(p, cgr, 1);
out:
- spin_unlock(&p->cgr_lock);
+ spin_unlock_irq(&p->cgr_lock);
put_affine_portal();
return ret;
}
--
2.35.1.1320.gc452695387.dirty
cgr_lock may be locked with interrupts already disabled by
smp_call_function_single. As such, we must use a raw spinlock to avoid
problems on PREEMPT_RT kernels. Although this bug has existed for a
while, it was not apparent until commit ef2a8d5478b9 ("net: dpaa: Adjust
queue depth on rate change") which invokes smp_call_function_single via
qman_update_cgr_safe every time a link goes up or down.
Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver")
Reported-by: Vladimir Oltean <[email protected]>
Link: https://lore.kernel.org/all/20230323153935.nofnjucqjqnz34ej@skbuf/
Signed-off-by: Sean Anderson <[email protected]>
---
(no changes since v1)
drivers/soc/fsl/qbman/qman.c | 22 +++++++++++-----------
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 1bf1f1ea67f0..7a1558aba523 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -991,7 +991,7 @@ struct qman_portal {
/* linked-list of CSCN handlers. */
struct list_head cgr_cbs;
/* list lock */
- spinlock_t cgr_lock;
+ raw_spinlock_t cgr_lock;
struct work_struct congestion_work;
struct work_struct mr_work;
char irqname[MAX_IRQNAME];
@@ -1281,7 +1281,7 @@ static int qman_create_portal(struct qman_portal *portal,
/* if the given mask is NULL, assume all CGRs can be seen */
qman_cgrs_fill(&portal->cgrs[0]);
INIT_LIST_HEAD(&portal->cgr_cbs);
- spin_lock_init(&portal->cgr_lock);
+ raw_spin_lock_init(&portal->cgr_lock);
INIT_WORK(&portal->congestion_work, qm_congestion_task);
INIT_WORK(&portal->mr_work, qm_mr_process_task);
portal->bits = 0;
@@ -1456,11 +1456,11 @@ static void qm_congestion_task(struct work_struct *work)
union qm_mc_result *mcr;
struct qman_cgr *cgr;
- spin_lock_irq(&p->cgr_lock);
+ raw_spin_lock_irq(&p->cgr_lock);
qm_mc_start(&p->p);
qm_mc_commit(&p->p, QM_MCC_VERB_QUERYCONGESTION);
if (!qm_mc_result_timeout(&p->p, &mcr)) {
- spin_unlock_irq(&p->cgr_lock);
+ raw_spin_unlock_irq(&p->cgr_lock);
dev_crit(p->config->dev, "QUERYCONGESTION timeout\n");
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
return;
@@ -1476,7 +1476,7 @@ static void qm_congestion_task(struct work_struct *work)
list_for_each_entry(cgr, &p->cgr_cbs, node)
if (cgr->cb && qman_cgrs_get(&c, cgr->cgrid))
cgr->cb(p, cgr, qman_cgrs_get(&rr, cgr->cgrid));
- spin_unlock_irq(&p->cgr_lock);
+ raw_spin_unlock_irq(&p->cgr_lock);
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
}
@@ -2440,7 +2440,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
preempt_enable();
cgr->chan = p->config->channel;
- spin_lock_irq(&p->cgr_lock);
+ raw_spin_lock_irq(&p->cgr_lock);
if (opts) {
struct qm_mcc_initcgr local_opts = *opts;
@@ -2477,7 +2477,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
qman_cgrs_get(&p->cgrs[1], cgr->cgrid))
cgr->cb(p, cgr, 1);
out:
- spin_unlock_irq(&p->cgr_lock);
+ raw_spin_unlock_irq(&p->cgr_lock);
put_affine_portal();
return ret;
}
@@ -2512,7 +2512,7 @@ int qman_delete_cgr(struct qman_cgr *cgr)
return -EINVAL;
memset(&local_opts, 0, sizeof(struct qm_mcc_initcgr));
- spin_lock_irqsave(&p->cgr_lock, irqflags);
+ raw_spin_lock_irqsave(&p->cgr_lock, irqflags);
list_del(&cgr->node);
/*
* If there are no other CGR objects for this CGRID in the list,
@@ -2537,7 +2537,7 @@ int qman_delete_cgr(struct qman_cgr *cgr)
/* add back to the list */
list_add(&cgr->node, &p->cgr_cbs);
release_lock:
- spin_unlock_irqrestore(&p->cgr_lock, irqflags);
+ raw_spin_unlock_irqrestore(&p->cgr_lock, irqflags);
put_affine_portal();
return ret;
}
@@ -2577,9 +2577,9 @@ static int qman_update_cgr(struct qman_cgr *cgr, struct qm_mcc_initcgr *opts)
if (!p)
return -EINVAL;
- spin_lock_irqsave(&p->cgr_lock, irqflags);
+ raw_spin_lock_irqsave(&p->cgr_lock, irqflags);
ret = qm_modify_cgr(cgr, 0, opts);
- spin_unlock_irqrestore(&p->cgr_lock, irqflags);
+ raw_spin_unlock_irqrestore(&p->cgr_lock, irqflags);
put_affine_portal();
return ret;
}
--
2.35.1.1320.gc452695387.dirty
On Fri, Mar 31, 2023 at 11:14:12AM -0400, Sean Anderson wrote:
> smp_call_function_single disables IRQs when executing the callback. To
> prevent deadlocks, we must disable IRQs when taking cgr_lock elsewhere.
> This is already done by qman_update_cgr and qman_delete_cgr; fix the
> other lockers.
>
> Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver")
If you've identified smp_call_function_single() as the problem, then the
true issue seems to lie in commit 96f413f47677 ("soc/fsl/qbman: fix
issue in qman_delete_cgr_safe()") and not in the initial commit, no?
Anyway,
Tested-by: Vladimir Oltean <[email protected]>
On Fri, Mar 31, 2023 at 11:14:13AM -0400, Sean Anderson wrote:
> cgr_lock may be locked with interrupts already disabled by
> smp_call_function_single. As such, we must use a raw spinlock to avoid
> problems on PREEMPT_RT kernels. Although this bug has existed for a
> while, it was not apparent until commit ef2a8d5478b9 ("net: dpaa: Adjust
> queue depth on rate change") which invokes smp_call_function_single via
> qman_update_cgr_safe every time a link goes up or down.
>
> Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver")
Same comment about the Fixes tag.
git tag --contains c535e923bb97
v4.9
git tag --contains 96f413f47677
v4.16
Looking at https://www.kernel.org/, I see that kernel 4.14 is still
maintained but should not have this patch backported, do you agree?
> Reported-by: Vladimir Oltean <[email protected]>
> Link: https://lore.kernel.org/all/20230323153935.nofnjucqjqnz34ej@skbuf/
> Signed-off-by: Sean Anderson <[email protected]>
> ---
Anyway,
Tested-by: Vladimir Oltean <[email protected]>
On 4/3/23 10:02, Vladimir Oltean wrote:
> On Fri, Mar 31, 2023 at 11:14:12AM -0400, Sean Anderson wrote:
>> smp_call_function_single disables IRQs when executing the callback. To
>> prevent deadlocks, we must disable IRQs when taking cgr_lock elsewhere.
>> This is already done by qman_update_cgr and qman_delete_cgr; fix the
>> other lockers.
>>
>> Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver")
>
> If you've identified smp_call_function_single() as the problem, then the
> true issue seems to lie in commit 96f413f47677 ("soc/fsl/qbman: fix
> issue in qman_delete_cgr_safe()") and not in the initial commit, no?
Yes, that seems better. I did a blame and saw that qman_delete_cgr_safe
had been around since the initial driver, but I didn't realize it worked
in a different way back then.
--Sean
> Anyway,
>
> Tested-by: Vladimir Oltean <[email protected]>
> -----Original Message-----
> From: Sean Anderson <[email protected]>
> Sent: Monday, April 3, 2023 18:22
> To: Vladimir Oltean <[email protected]>
> Cc: Leo Li <[email protected]>; [email protected]; linux-arm-
> [email protected]; Scott Wood <[email protected]>; Camelia
> Alexandra Groza <[email protected]>; [email protected];
> Roy Pledge <[email protected]>; David S . Miller
> <[email protected]>; Claudiu Manoil <[email protected]>
> Subject: Re: [PATCH v2 1/2] soc: fsl: qbman: Always disable interrupts when
> taking cgr_lock
>
> On 4/3/23 10:02, Vladimir Oltean wrote:
> > On Fri, Mar 31, 2023 at 11:14:12AM -0400, Sean Anderson wrote:
> >> smp_call_function_single disables IRQs when executing the callback. To
> >> prevent deadlocks, we must disable IRQs when taking cgr_lock elsewhere.
> >> This is already done by qman_update_cgr and qman_delete_cgr; fix the
> >> other lockers.
> >>
> >> Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver")
> >
> > If you've identified smp_call_function_single() as the problem, then the
> > true issue seems to lie in commit 96f413f47677 ("soc/fsl/qbman: fix
> > issue in qman_delete_cgr_safe()") and not in the initial commit, no?
>
> Yes, that seems better. I did a blame and saw that qman_delete_cgr_safe
> had been around since the initial driver, but I didn't realize it worked
> in a different way back then.
>
> --Sean
>
> > Anyway,
> >
> > Tested-by: Vladimir Oltean <[email protected]>
Apart from Vladimir's comment:
Reviewed-by: Camelia Groza <[email protected]>
> -----Original Message-----
> From: Sean Anderson <[email protected]>
> Sent: Friday, March 31, 2023 18:14
> To: Leo Li <[email protected]>; [email protected]; linux-arm-
> [email protected]
> Cc: Scott Wood <[email protected]>; Camelia Alexandra Groza
> <[email protected]>; [email protected]; Roy Pledge
> <[email protected]>; David S . Miller <[email protected]>; Claudiu
> Manoil <[email protected]>; Vladimir Oltean
> <[email protected]>; Sean Anderson <[email protected]>
> Subject: [PATCH v2 2/2] soc: fsl: qbman: Use raw spinlock for cgr_lock
>
> cgr_lock may be locked with interrupts already disabled by
> smp_call_function_single. As such, we must use a raw spinlock to avoid
> problems on PREEMPT_RT kernels. Although this bug has existed for a
> while, it was not apparent until commit ef2a8d5478b9 ("net: dpaa: Adjust
> queue depth on rate change") which invokes smp_call_function_single via
> qman_update_cgr_safe every time a link goes up or down.
>
> Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver")
> Reported-by: Vladimir Oltean <[email protected]>
> Link: https://lore.kernel.org/all/20230323153935.nofnjucqjqnz34ej@skbuf/
> Signed-off-by: Sean Anderson <[email protected]>
Reviewed-by: Camelia Groza <[email protected]>