netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Sergei Shtylyov <sergei.shtylyov@gmail.com>
To: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>,
	Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Cc: "REE dirk.behme@de.bosch.com" <dirk.behme@de.bosch.com>,
	"Shashikant.Suguni@in.bosch.com" <Shashikant.Suguni@in.bosch.com>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"linux-renesas-soc@vger.kernel.org" 
	<linux-renesas-soc@vger.kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"kuba@kernel.org" <kuba@kernel.org>
Subject: Re: [PATCH/RFC] net: ethernet: ravb: Try to wake subqueue instead of stop on timeout
Date: Sun, 19 Jul 2020 22:20:19 +0300	[thread overview]
Message-ID: <64414933-c918-1613-255e-880017bc426a@gmail.com> (raw)
In-Reply-To: <TY2PR01MB3692E49BB082DEF855A3ACBAD8690@TY2PR01MB3692.jpnprd01.prod.outlook.com>

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@de.bosch.com>
>>>>>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.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

  reply	other threads:[~2020-07-19 19:20 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-26  9:46 [PATCH/RFC] net: ethernet: ravb: Try to wake subqueue instead of stop on timeout Yoshihiro Shimoda
2020-06-15  5:58 ` Yoshihiro Shimoda
2020-06-15  8:12   ` Sergei Shtylyov
2020-06-19  5:46     ` Yoshihiro Shimoda
2020-06-29  5:24       ` Yoshihiro Shimoda
2020-06-29  8:40         ` Sergei Shtylyov
2020-06-29 10:08           ` Yoshihiro Shimoda
2020-06-30  5:22           ` Yoshihiro Shimoda
2020-07-05 20:28             ` Sergei Shtylyov
2020-07-06  9:25               ` Yoshihiro Shimoda
2020-07-19 19:20                 ` Sergei Shtylyov [this message]
2020-07-20  1:48                   ` Yoshihiro Shimoda

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=64414933-c918-1613-255e-880017bc426a@gmail.com \
    --to=sergei.shtylyov@gmail.com \
    --cc=Shashikant.Suguni@in.bosch.com \
    --cc=davem@davemloft.net \
    --cc=dirk.behme@de.bosch.com \
    --cc=kuba@kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=sergei.shtylyov@cogentembedded.com \
    --cc=yoshihiro.shimoda.uh@renesas.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).