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
next prev parent 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).