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]

Date:	Mon, 23 Feb 2015 12:13:28 +0100
From:	Tobias Waldekranz <tobias@...dekranz.com>
To:	Florian Fainelli <f.fainelli@...il.com>
Cc:	netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH 2/2] dsa: mv88e6131: support fixed PHYs

On Sat, Feb 21, 2015 at 10:56:25AM -0800, Florian Fainelli wrote:
> 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

Right, in my case I just need to do an initial config according to the
fixed settings which are read from the device-tree.

In the case where there is a real PHY attached to the switch, an
internal machine will poll the PHY and setup the MAC accordingly. So
HW will take care of step 2 for me.

> >
> > 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.

In my case I do need to know, since I want the switch's phy polling
unit to do the work when possible. Maybe I should just rethink the
whole thing and do the setup at probe-time using some other method.

I will get back to you with a better solution :)

> Thanks
> -- 
> Florian

-- 
Thanks
 - wkz
--
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