Bug 43021

Created attachment 72780 [details]
Reboot only D3 transition avoidance

The condition may even be completely removed.

Just verified that it's NOT a PCI config space save/restore issue - diffing log files of both pre and post resume lspci -xxx outputs comes up empty - IOW save/restore probably _is_ properly being done by PM layer now, and it does work correctly. Will try the attached patch next, and failing that, try to revert the via-rhine.c commit by commit.

OK, the attached patch did NOT work.


I also just did the link state test - resumed (resulting in broken connection), then pulled the cable, then reinserted - bingo! So that perhaps means it's an issue near MII/PHY parts, right?

Will now proceed with reverting individual commits unless told otherwise. ;)

a384a33bb1c9ec2 broke it (fc3e0f8aec05dd8 failed, 7ab87ff4c770eed worked, a384a33bb1c9ec2 failed, then 7ab87ff4c770eed worked again).

So far, so good (well, at least not too bad...).

And in fact I _am_ getting a
via-rhine 0000:00:12.0: eth-via: high bit wait (71/80) cycle count: 1024

(doh - should have checked dmesg _much_ earlier)

BTW, I also have:
"via_rhine: Broken BIOS detected, avoid_D3 enabled"



Well, the thing is... I just simply don't know what the hell could be breaking here.
I tried to disable the newly introduced udelay(10); in rhine_wait_bit()
(which is about the very only thing in the entire overly sizeable patch
which might be responsible AFAICS), but that didn't help.


BTW, the
                if (high ^ !!(ioread8(ioaddr + reg) & mask))

line is awful.
It should be changed to update a new
bool has_mask_bits = false;
helper variable and then simply check for
if (high ^ has_mask_bits)

Or, IOW,

        bool has_mask_bits = false;
        for (i = 0; i < 1024; i++) {
                has_mask_bits = !!(ioread8(ioaddr + reg) & mask);
                if (high ^ has_mask_bits)
                        break;
                udelay(10);
        }

Oh, and for giggles one could move the repeated ioaddr + reg calc out of the loop,
but this is quite likely such a simple optimization that any semi-modern compiler is likely doing that anyway, thus it might end up as a pessimization (would need to check generated asm).



One thing on my TODO list: perhaps we're being hit by a PCI posting issue?

The dmesg log strongly pinpoints rhine_disable_linkmon()
as the culprit. I'm running out of ideas - I will now reintroduce the macro specifically for that function to see whether that possibly fixes it.


BTW, the patch itself definitely goes in the right direction - it's just a bit unwieldy to analyze.

I'll do some more investigation now.

Perhaps it's also a matter of formerly

if (debug > X)-scoped

accesses of e.g. ioread16(ioaddr + ChipCmd) formerly being skipped and now unconditionally being executed, thereby causing very unwanted chip-side resets or some such? (just some paranoia)

I will have to comment out all those lines to see whether this was the problem.

Disregard my "unwieldy" and "if (debug > X)" comments - I just had a look at an overly broad diff :-P

Well, thus a384a33bb1c9ec2d9 "via-rhine: RHINE_WAIT_FOR macro removal." diff is even smaller, which makes this breakage all the more puzzling (perhaps compiler issue even??).

I'll try to tweak some crucial code and see what happens.

XOR check in rhine_wait_bit() _was_ buggy indeed - with that fixed a384a33bb1c9ec2 now works (haven't verified HEAD yet).
Patch to follow.
It's not a good idea to skip separation of concerns and thereby increase code complexity beyond understandability (has_mask_bits bool helper).
Us humans are stooopid bozos :)

Created attachment 72787 [details]
Fix to make a384a33bb1c9ec not break things

(In reply to comment #8)
> Created an attachment (id=72787) [details]
> Fix to make a384a33bb1c9ec not break things

Ok, I add your Signed-off-by and I submit it now.

Thanks.

-- 
Ueimor