linux-kernel - Re: [PATCH v3 2/2] soc: fsl: qbman: Use raw spinlock for cgr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 4 Apr 2023 12:04:57 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     Crystal Wood <oss@...error.net>, Li Yang <leoyang.li@....com>,
        linuxppc-dev@...ts.ozlabs.org, linux-arm-kernel@...ts.infradead.org
Cc:     Vladimir Oltean <vladimir.oltean@....com>,
        Claudiu Manoil <claudiu.manoil@....com>,
        Camelia Groza <camelia.groza@....com>,
        linux-kernel@...r.kernel.org, Roy Pledge <roy.pledge@....com>,
        "David S . Miller" <davem@...emloft.net>,
        Madalin Bucur <madalin.bucur@....com>
Subject: Re: [PATCH v3 2/2] soc: fsl: qbman: Use raw spinlock for cgr_lock

On 4/4/23 11:33, Crystal Wood wrote:
> On Tue, 2023-04-04 at 10:55 -0400, Sean Anderson wrote:
> 
>> @@ -1456,11 +1456,11 @@ static void tqm_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);
> 
> qm_mc_result_timeout() spins with a timeout of 10 ms which is very
> inappropriate for a raw lock.  What is the actual expected upper bound?

Hm, maybe we can move this qm_mc stuff outside cgr_lock? In most other
places they're called without cgr_lock, which implies that its usage
here is meant to synchronize against some other function.

>>                 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);
>>  }
> 
> The callback loop is also a bit concerning...

The callbacks (in .../dpaa/dpaa_eth.c and .../caam/qi.c) look OK. The
only thing which might take a bit is dpaa_eth_refill_bpools, which
allocates memory (from the atomic pool).

--Sean