Re: Realtek 8139 problem on 486.

[Nikolai Zhubr <zhubr.2@gmail.com>]

Hello Arnd,

03.07.2021 12:10, Arnd Bergmann:
> The simplest workaround would be to just move the
> "spin_lock_irqsave(&tp->lock, flags);" a few lines down, below the rx
> processing. This keeps the locking rules exactly how they were before

Indeed, moving spin_lock_irqsave below rtl8139_rx eliminated the warn_on 
message apparently, here is a new log:

https://pastebin.com/dVFNVEu4

and here is my resulting diff:

https://pastebin.com/CzNzsUPu

My usual tests run fine. However I still see 2 issues:

1. I do not understand all this locking thing enough to do a good 
cleanup myself, and it looks like it needs one;
2. It looks like in case of incorrect (edge) triggering mode, the "poll 
approach" with no loop added in the poll function would still allow a 
race window, as explained in following outline (from some previous mails):

22.06.2021 14:12, David Laight:
 > Typically you need to:
 > 1) stop the chip driving IRQ low.
 > 2) process all the completed RX and TX entries.
 > 3) clear the chip's interrupt pending bits (often write to clear).
 > 4) check for completed RX/TX entries, back to 2 if found.
 > 5) enable driving IRQ.
 >
 > The loop (4) is needed because of the timing window between
 > (2) and (3).
 > You can swap (2) and (3) over - but then you get an additional
 > interrupt if packets arrive during processing - which is common.

So in terms of such outline, the "poll approach" now implements 1, 2, 3, 
5 but still misses 4, and my understanding is that it is therefore still 
not a complete solution for the broken triggering case (Although 
practically, the time window might be too small for the race effect to 
be ever observable) From my previous testing I know that such a loop 
does not affect the perfomance too much anyway, so it seems quite safe 
to add it. Maybe I've missunderstood something though.


Thank you,

Regards,
Nikolai


> your patch, and anything beyond that could be done as a follow-up
> cleanup, if someone wants to think this through more.
>
>          Arnd
>