Re: [PATCH 1/2] soc: fsl: qbman: Always disable interrupts when taking cgr_lock

2023-03-31 Thread Sean Anderson 
On 3/31/23 06:58, Camelia Alexandra Groza wrote:
>> -Original Message-
>> From: Sean Anderson 
>> Sent: Monday, March 27, 2023 22:29
>> To: Leo Li ; linuxppc-dev@lists.ozlabs.org; linux-arm-
>> ker...@lists.infradead.org
>> Cc: Scott Wood ; linux-ker...@vger.kernel.org; David S .
>> Miller ; Claudiu Manoil ;
>> Roy Pledge ; Vladimir Oltean
>> ; Camelia Alexandra Groza
>> ; Sean Anderson 
>> Subject: [PATCH 1/2] soc: fsl: qbman: Always disable interrupts when taking
>> cgr_lock
>> 
>> 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 
>> ---
>> 
>>  drivers/soc/fsl/qbman/qman.c | 8 
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
>> index 739e4eee6b75..ff870ca07596 100644
>> --- a/drivers/soc/fsl/qbman/qman.c
>> +++ b/drivers/soc/fsl/qbman/qman.c
>> @@ -1456,7 +1456,7 @@ static void qm_congestion_task(struct work_struct
>> *work)
>>  union qm_mc_result *mcr;
>>  struct qman_cgr *cgr;
>> 
>> -spin_lock(>cgr_lock);
>> +spin_lock_irq(>cgr_lock);
>>  qm_mc_start(>p);
>>  qm_mc_commit(>p, QM_MCC_VERB_QUERYCONGESTION);
>>  if (!qm_mc_result_timeout(>p, )) {
>> @@ -1476,7 +1476,7 @@ static void qm_congestion_task(struct work_struct
>> *work)
>>  list_for_each_entry(cgr, >cgr_cbs, node)
>>  if (cgr->cb && qman_cgrs_get(, cgr->cgrid))
>>  cgr->cb(p, cgr, qman_cgrs_get(, cgr->cgrid));
>> -spin_unlock(>cgr_lock);
>> +spin_unlock_irq(>cgr_lock);
>>  qman_p_irqsource_add(p, QM_PIRQ_CSCI);
>>  }
> 
> There is one more spin_unlock call in qm_congestion_task on the error path 
> that needs updating:
> 
> if (!qm_mc_result_timeout(>p, )) {
>   spin_unlock(>cgr_lock);

Will fix. Thanks.

--Sean

> 
>> @@ -2440,7 +2440,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32
>> flags,
>>  preempt_enable();
>> 
>>  cgr->chan = p->config->channel;
>> -spin_lock(>cgr_lock);
>> +spin_lock_irq(>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(>cgrs[1], cgr->cgrid))
>>  cgr->cb(p, cgr, 1);
>>  out:
>> -spin_unlock(>cgr_lock);
>> +spin_unlock_irq(>cgr_lock);
>>  put_affine_portal();
>>  return ret;
>>  }
>> --
>> 2.35.1.1320.gc452695387.dirty
>

RE: [PATCH 1/2] soc: fsl: qbman: Always disable interrupts when taking cgr_lock

2023-03-31 Thread Camelia Alexandra Groza 
> -Original Message-
> From: Sean Anderson 
> Sent: Monday, March 27, 2023 22:29
> To: Leo Li ; linuxppc-dev@lists.ozlabs.org; linux-arm-
> ker...@lists.infradead.org
> Cc: Scott Wood ; linux-ker...@vger.kernel.org; David S .
> Miller ; Claudiu Manoil ;
> Roy Pledge ; Vladimir Oltean
> ; Camelia Alexandra Groza
> ; Sean Anderson 
> Subject: [PATCH 1/2] soc: fsl: qbman: Always disable interrupts when taking
> cgr_lock
> 
> 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 
> ---
> 
>  drivers/soc/fsl/qbman/qman.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
> index 739e4eee6b75..ff870ca07596 100644
> --- a/drivers/soc/fsl/qbman/qman.c
> +++ b/drivers/soc/fsl/qbman/qman.c
> @@ -1456,7 +1456,7 @@ static void qm_congestion_task(struct work_struct
> *work)
>   union qm_mc_result *mcr;
>   struct qman_cgr *cgr;
> 
> - spin_lock(>cgr_lock);
> + spin_lock_irq(>cgr_lock);
>   qm_mc_start(>p);
>   qm_mc_commit(>p, QM_MCC_VERB_QUERYCONGESTION);
>   if (!qm_mc_result_timeout(>p, )) {
> @@ -1476,7 +1476,7 @@ static void qm_congestion_task(struct work_struct
> *work)
>   list_for_each_entry(cgr, >cgr_cbs, node)
>   if (cgr->cb && qman_cgrs_get(, cgr->cgrid))
>   cgr->cb(p, cgr, qman_cgrs_get(, cgr->cgrid));
> - spin_unlock(>cgr_lock);
> + spin_unlock_irq(>cgr_lock);
>   qman_p_irqsource_add(p, QM_PIRQ_CSCI);
>  }

There is one more spin_unlock call in qm_congestion_task on the error path that 
needs updating:

if (!qm_mc_result_timeout(>p, )) {
spin_unlock(>cgr_lock);

Regards,
Camelia

> @@ -2440,7 +2440,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32
> flags,
>   preempt_enable();
> 
>   cgr->chan = p->config->channel;
> - spin_lock(>cgr_lock);
> + spin_lock_irq(>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(>cgrs[1], cgr->cgrid))
>   cgr->cb(p, cgr, 1);
>  out:
> - spin_unlock(>cgr_lock);
> + spin_unlock_irq(>cgr_lock);
>   put_affine_portal();
>   return ret;
>  }
> --
> 2.35.1.1320.gc452695387.dirty

[PATCH 1/2] soc: fsl: qbman: Always disable interrupts when taking cgr_lock

2023-03-27 Thread Sean Anderson 
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 
---

 drivers/soc/fsl/qbman/qman.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 739e4eee6b75..ff870ca07596 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -1456,7 +1456,7 @@ static void qm_congestion_task(struct work_struct *work)
union qm_mc_result *mcr;
struct qman_cgr *cgr;
 
-   spin_lock(>cgr_lock);
+   spin_lock_irq(>cgr_lock);
qm_mc_start(>p);
qm_mc_commit(>p, QM_MCC_VERB_QUERYCONGESTION);
if (!qm_mc_result_timeout(>p, )) {
@@ -1476,7 +1476,7 @@ static void qm_congestion_task(struct work_struct *work)
list_for_each_entry(cgr, >cgr_cbs, node)
if (cgr->cb && qman_cgrs_get(, cgr->cgrid))
cgr->cb(p, cgr, qman_cgrs_get(, cgr->cgrid));
-   spin_unlock(>cgr_lock);
+   spin_unlock_irq(>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(>cgr_lock);
+   spin_lock_irq(>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(>cgrs[1], cgr->cgrid))
cgr->cb(p, cgr, 1);
 out:
-   spin_unlock(>cgr_lock);
+   spin_unlock_irq(>cgr_lock);
put_affine_portal();
return ret;
 }
-- 
2.35.1.1320.gc452695387.dirty