Message-ID: <64414933-c918-1613-255e-880017bc426a@gmail.com>
Date:   Sun, 19 Jul 2020 22:20:19 +0300
From:   Sergei Shtylyov <sergei.shtylyov@...il.com>
To:     Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
Cc:     "REE dirk.behme@...bosch.com" <dirk.behme@...bosch.com>,
        "Shashikant.Suguni@...bosch.com" <Shashikant.Suguni@...bosch.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-renesas-soc@...r.kernel.org" 
        <linux-renesas-soc@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: Re: [PATCH/RFC] net: ethernet: ravb: Try to wake subqueue instead of
 stop on timeout

Hello!

   Sorry about another late reply, was having h/w issues on the new work...

On 07/06/2020 12:25 PM, Yoshihiro Shimoda wrote:

>>>>>>>>> From: Yoshihiro Shimoda, Sent: Tuesday, May 26, 2020 6:47 PM
>>>>>>>>>
>>>>>>>>> According to the report of [1], this driver is possible to cause
>>>>>>>>> the following error in ravb_tx_timeout_work().
>>>>>>>>>
>>>>>>>>> ravb e6800000.ethernet ethernet: failed to switch device to config mode
>>>>>>>>>
>>>>>>>>> This error means that the hardware could not change the state
>>>>>>>>> from "Operation" to "Configuration" while some tx queue is operating.
>>>>>>>>> After that, ravb_config() in ravb_dmac_init() will fail, and then
>>>>>>>>> any descriptors will be not allocaled anymore so that NULL porinter
>>
>>     Pointer. :-)
> 
> Oops! I should fix it :)
> 
>>>>>>>>> dereference happens after that on ravb_start_xmit().
>>>>>>>>>
>>>>>>>>> Such a case is possible to be caused because this driver supports
>>>>>>>>> two queues (NC and BE) and the ravb_stop_dma() is possible to return
>>>>>>>>> without any stopping process if TCCR or CSR register indicates
>>>>>>>>> the hardware is operating for TX.
>>
>>     Maybe we should just fix those blind assumptions?
> 
> Maybe I should have described some facts instead of assumptions like below?
> If so, I should modify the code too.
> 
> After ravb_stop_dma() was called, the driver assumed any transfers were
> stopped. However, the current ravb_tx_timeout_work() doesn't check whether
> the ravb_stop_dma() is succeeded without any error or not. So, we should
> fix it.

   Yes. Better a stuck TX queue (with a chance to recover) than kernel oops...

>>>>>>>>> To fix the issue, just try to wake the subqueue on
>>>>>>>>> ravb_tx_timeout_work() if the descriptors are not full instead
>>>>>>>>> of stop all transfers (all queues of TX and RX).
>>>>>>>>>
>>>>>>>>> [1]
>>>>>>>>> https://lore.kernel.org/linux-renesas-soc/20200518045452.2390-1-dirk.behme@de.bosch.com/
>>>>>>>>>
>>>>>>>>> Reported-by: Dirk Behme <dirk.behme@...bosch.com>
>>>>>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>
>>>>>>>>> ---
>>>>>>>>>     I'm guessing that this issue is possible to happen if:
>>>>>>>>>     - ravb_start_xmit() calls netif_stop_subqueue(), and
>>>>>>>>>     - ravb_poll() will not be called with some reason, and
>>>>>>>>>     - netif_wake_subqueue() will be not called, and then
>>>>>>>>>     - dev_watchdog() in net/sched/sch_generic.c calls ndo_tx_timeout().
>>>>>>>>>
>>>>>>>>>     However, unfortunately, I didn't reproduce the issue yet.
>>>>>>>>>     To be honest, I'm also guessing other queues (SR) of this hardware
>>>>>>>>>     which out-of tree driver manages are possible to reproduce this issue,
>>>>>>>>>     but I didn't try such environment for now...
>>>>>>>>>
>>>>>>>>>     So, I marked RFC on this patch now.
>>>>>>>>
>>>>>>>> I'm afraid, but do you have any comments about this patch?
>>>>>>>
>>>>>>>       I agree that we should now reset only the stuck queue, not both but I
>>>>>>> doubt your solution is good enough. Let me have another look...
>>>>>>
>>>>>> Thank you for your comment! I hope this solution is good enough...
>>>>>
>>>>> I'm sorry again and again. But, do you have any time to look this patch?
>>>>
>>>>      Yes, in the sense of reviewing -- I don't consider it complete. And no, in
>>>> the sense of looking into the issue myself... Can we do a per-queue tear-down
>>>> and re-init (not necessarily all in 1 patch)?
>>
>>     In fact, it would ensue many changes...
> 
> I think so.
> 
>>> Thank you for your comment! I'm not sure this "re-init" mean. But, we can do
>>
>>     Well, I meant the ring re-allocation and re-formatting... but (looking at
>> sh_eth) the former is not really necessary, it's enough to just stop the TX
>> ring and then re-format it and re-start...
> 
> I got it. I also think the ring re-allocation is not really necessary.
> 
>> Well, unfortunately, the way I
>> structured the code, we can't do *just* that...
> 
> I agree. We need refactoring for it.
> 
>>> a per-queue tear-down if DMAC is still working. And, we can prepare new descriptors
>>> for the queue after tear-down.
>>>
>>> < Tear-down >
>>> 1. Set DT_EOS to the desc_bat[queue].
>>> 2. Set DLR.LBAx to 1.
>>> 3. Check if DLA.LBAx is cleared.
>>
>>     DLR.LBAx, you mean?
> 
> Yes. I heard this procedure from BSP team.
> 
>>     Well, I was thinking of polling TCCR and CSR like the current
>> ravb_stop_dma() does, but if that works...
> 
> I'm not sure whether polling TCCR and CSR is enough or not.
> Instead of polling those registers, maybe we should poll whether
> ravb_stop_dma() is succeeded or not?

   Yes, if by polling you mean just checking the result of it. :-)

> Especially, result of ravb_config() is
> a key point whether the hardware is really stopped or not.
> So, I'm thinking that just polling the ravb_stop_dma() in
> ravb_tx_timeout_work() is better than the per-queue tear-down and
> re-init now. But, what do you think?

   I don't think it's better since we're now supposed to handle a per-queue
TX timeout (still not sure it's possible with this h/w). But of course, it's
better as it's simple enough for a bug fix.

>>> < Prepare new descriptors and start again >
>>> 4. Prepare new descriptors.
>>
>>     That's where the cause for using the workqueue lies -- the descriptors are
>> allocated with GFP_KERNEL, not GFP_ATOMIC...
> 
> IIUC, we can avoid to use the workqueue if re-allocation is not really necessary.
> 
>> if you have time/desire to
>> untangle all this, I'd appreciate it; else I'd have to work on this in my
>> copious free time... :-)
> 
> If we don't need refactoring, I think I can do it :)

   Let's go forward with the simple fix (assuming it fixes the original oops).

[...]

MBR, Sergei

Powered by blists - more mailing lists