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@xxxxxxxxxxxx/ >>>>>>>>> >>>>>>>>> Reported-by: Dirk Behme <dirk.behme@xxxxxxxxxxxx> >>>>>>>>> Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> >>>>>>>>> --- >>>>>>>>> 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
