netdev - Re: [PATCH net-next 3/3] macb: support the two tx descriptors on at91rm9200

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]

Message-ID: <061451b2-95b5-e37f-595f-1a93f8c3c2ab@microchip.com>
Date:   Wed, 21 Oct 2020 09:02:33 +0000
From:   <Claudiu.Beznea@...rochip.com>
To:     <w@....eu>
CC:     <Nicolas.Ferre@...rochip.com>, <netdev@...r.kernel.org>,
        <davem@...emloft.net>, <kuba@...nel.org>, <daniel@...f.com>
Subject: Re: [PATCH net-next 3/3] macb: support the two tx descriptors on
 at91rm9200

Hi Willy,

On 14.10.2020 19:27, Willy Tarreau wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Hi Claudiu,
> 
> first, thanks for your feedback!
> 
> On Wed, Oct 14, 2020 at 04:08:00PM +0000, Claudiu.Beznea@...rochip.com wrote:
>>> @@ -3994,11 +3996,10 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
>>>                                         struct net_device *dev)
>>>  {
>>>         struct macb *lp = netdev_priv(dev);
>>> +       unsigned long flags;
>>>
>>> -       if (macb_readl(lp, TSR) & MACB_BIT(RM9200_BNQ)) {
>>> -               int desc = 0;
>>> -
>>> -               netif_stop_queue(dev);
>>> +       if (lp->rm9200_tx_len < 2) {
>>> +               int desc = lp->rm9200_tx_tail;
>>
>> I think you also want to protect these reads with spin_lock() to avoid
>> concurrency with the interrupt handler.
> 
> I don't think it's needed because the condition doesn't change below
> us as the interrupt handler only decrements. However I could use a
> READ_ONCE to make things cleaner. And in practice this test was kept
> to keep some sanity checks but it never fails, as if the queue length
> reaches 2, the queue is stopped (and I never got the device busy message
> either before nor after the patch).
> 
>>>                 /* Store packet information (to free when Tx completed) */
>>>                 lp->rm9200_txq[desc].skb = skb;
>>> @@ -4012,6 +4013,15 @@ static netdev_tx_t at91ether_start_xmit(struct sk_buff *skb,
>>>                         return NETDEV_TX_OK;
>>>                 }
>>>
>>> +               spin_lock_irqsave(&lp->lock, flags);
>>> +
>>> +               lp->rm9200_tx_tail = (desc + 1) & 1;
>>> +               lp->rm9200_tx_len++;
>>> +               if (lp->rm9200_tx_len > 1)
>>> +                       netif_stop_queue(dev);
> 
> This is where we guarantee that we won't call start_xmit() again with
> rm9200_tx_len >= 2.

I see it!

> 
>>> @@ -4088,21 +4100,39 @@ static irqreturn_t at91ether_interrupt(int irq, void *dev_id)
>>>                 at91ether_rx(dev);
>>>
>>>         /* Transmit complete */
>>> -       if (intstatus & MACB_BIT(TCOMP)) {
>>> +       if (intstatus & (MACB_BIT(TCOMP) | MACB_BIT(RM9200_TBRE))) {
>>>                 /* The TCOM bit is set even if the transmission failed */
>>>                 if (intstatus & (MACB_BIT(ISR_TUND) | MACB_BIT(ISR_RLE)))
>>>                         dev->stats.tx_errors++;
>>>
>>> -               desc = 0;
>>> -               if (lp->rm9200_txq[desc].skb) {
>>> +               spin_lock(&lp->lock);
>>
>> Also, this lock could be moved before while, below, as you want to protect
>> the rm9200_tx_len and rm9200_tx_tails members of lp as I understand.
> 
> Sure, but it actually *is* before the while(). I'm sorry if that was not
> visible from the context of the diff. The while is just a few lins below,
> thus rm9200_tx_len and rm9200_tx_tail are properly protected. Do not
> hesitate to tell me if something is not clear or if I'm wrong!

What I was trying to say is that since for this version of IP TSR is only
read once, here, in interrupt context and the idea of the lock is to
protect the lp->rm9200_tx_tail and lp->rm9200_tx_len, the spin_lock() call
could be moved just before while:

+               spin_lock(&lp->lock);
+
+               tsr = macb_readl(lp, TSR);
+
+               /* we have three possibilities here:
+                *   - all pending packets transmitted (TGO, implies BNQ)
+                *   - only first packet transmitted (!TGO && BNQ)
+                *   - two frames pending (!TGO && !BNQ)
+                * Note that TGO ("transmit go") is called "IDLE" on RM9200.
+                */
+               qlen = (tsr & MACB_BIT(TGO)) ? 0 :
+                       (tsr & MACB_BIT(RM9200_BNQ)) ? 1 : 2;
+

here

+               while (lp->rm9200_tx_len > qlen) {

Let me know if I'm missing something.

Thank you,
Claudiu

> 
> Thanks!
> Willy
>