netdev - Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGVrzcb-di9Q9z8VohmSt7f0tq=BOxJ2=XXiRKRj1FToQo3=fg@mail.gmail.com>
Date:	Sat, 21 Feb 2015 10:56:25 -0800
From:	Florian Fainelli <f.fainelli@...il.com>
To:	Tobias Waldekranz <tobias@...dekranz.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

2015-02-21 2:30 GMT-08:00 Tobias Waldekranz <tobias@...dekranz.com>:
> On Thu, Feb 12, 2015 at 08:13:28AM -0800, Florian Fainelli wrote:
>> 2015-02-12 6:13 GMT-08:00 Tobias Waldekranz <tobias@...dekranz.com>:
>> > Statically setup the PCS Control on the MAC to match the fixed PHY.
>>
>> bcm_sf2 supports both fixed PHYs and regular PHYs, yet we do not need
>> to get access to the fixed PHY status from the adjust_link callback
>> because you could implement a separate fixed_link_update callback for
>> that purpose.
>>
>> Did not that work for you?
>>
>
> That was my first approach and it worked fine. The only issue I saw
> was that the callback was continously called at each poll cycle even
> though the link state had not changed.

Right, we poll using this callback fairly often. Just like the PHY
libraries adjust_link, drivers are responsible for implementing
"caching" and avoiding the callback to be invoked too frequently.

>
> So then I implemented the same check for updates that was in the
> regular adjust_link callback. But before I submitted that version of
> the patch I looked att the sf2 code, and it seemed as though this code
> uses the callback to update the phy status based on the chip state and
> not the other way around. Did I misunderstand the code?

It is a two step process:

- fixed_link_update updates the fixed PHY's status member to reflect
the HW changes (link change mostly), updates the PHY device
speed/duplex/pause parameters

- adjust_link reads the PHY device speed/duplex/pause and applies
these to the HW registers

>
> Not wanting to break your code, I went with this approach instead. But
> if you're fine with it, I'm more than happy to go with that version.

I think it is a little cleaner since the adjust_link callback does not
need to know what kind of PHY device it is dealing with, while the
fixed_link_update() one only takes care of fixed PHYs.

Thanks
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html