netdev - Re: [PATCH net-next v2 3/3] net: phylink: add separate pcs operations structure

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] [day] [month] [year] [list]

Message-ID: <20200330100844.GE25745@shell.armlinux.org.uk>
Date:   Mon, 30 Mar 2020 11:08:44 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S. Miller" <davem@...emloft.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/3] net: phylink: add separate pcs
 operations structure

On Sun, Mar 29, 2020 at 01:42:08PM -0700, Florian Fainelli wrote:
> 
> 
> On 3/29/2020 9:01 AM, Russell King wrote:
> > Add a separate set of PCS operations, which MAC drivers can use to
> > couple phylink with their associated MAC PCS layer.  The PCS
> > operations include:
> > 
> > - pcs_get_state() - reads the link up/down, resolved speed, duplex
> >    and pause from the PCS.
> > - pcs_config() - configures the PCS for the specified mode, PHY
> >    interface type, and setting the advertisement.
> > - pcs_an_restart() - restarts 802.3 in-band negotiation with the
> >    link partner
> > - pcs_link_up() - informs the PCS that link has come up, and the
> >    parameters of the link. Link parameters are used to program the
> >    PCS for fixed speed and non-inband modes.
> > 
> > Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> > ---
> 
> Although your kernel documentation is pretty comprehensive, I am fairly
> sure people are going to get confused about whether they need to
> implement pcs_an_restart vs. mac_an_restart and pcs_get_state vs.
> mac_pcs_get_state (with the possibility of a naming confusion for the
> latter). Maybe some guidelines in the comment as to which one to
> implement could save some support.

If one is making use of the PCS operations, then the PCS operations
are used in preference to the MAC operations in all cases except the
pcs_config() and pcs_link_up() methods.

This is already documented with:

 * When present, this overrides mac_pcs_get_state() in &struct
 * phylink_mac_ops.

 * When PCS ops are present, this overrides mac_an_restart() in &struct
 * phylink_mac_ops.

I'm not sure adding more words will help - in my experience, the more
words that are used, the more people nitpick and mess up.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up