linux-kernel - RE: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK

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: <MWHPR11MB1693565E49EAB189C2084C58EF1B9@MWHPR11MB1693.namprd11.prod.outlook.com>
Date:   Tue, 6 Dec 2022 22:58:25 +0000
From:   <Jerry.Ray@...rochip.com>
To:     <linux@...linux.org.uk>, <olteanv@...il.com>
CC:     <andrew@...n.ch>, <f.fainelli@...il.com>, <davem@...emloft.net>,
        <edumazet@...gle.com>, <kuba@...nel.org>, <pabeni@...hat.com>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH net-next v3 2/2] dsa: lan9303: Move to PHYLINK

> > > This patch replaces the .adjust_link api with the .phylink_get_caps api.
> >
> > Am I supposed to read this commit description and understand what the
> > change does?
> >
> > You can't "replace" adjust_link with phylink_get_caps, since they don't
> > do the same thing. The equivalent set of operations are roughly
> > phylink_mac_config and phylink_mac_link_up, probably just the latter in
> > your case.
> >
> > By deleting adjust_link and not replacing with any of the above, the
> > change is telling me that nothing from adjust_link was needed?
> 
> ...
> 
> > > -static void lan9303_adjust_link(struct dsa_switch *ds, int port,
> > > -                           struct phy_device *phydev)
> > > -{
> > > -   struct lan9303 *chip = ds->priv;
> > > -   int ctl;
> > > -
> > > -   if (!phy_is_pseudo_fixed_link(phydev))
> > > -           return;
> 
> If this is a not a fixed link, adjust_link does nothing.
> 
> > > -
> > > -   ctl = lan9303_phy_read(ds, port, MII_BMCR);
> > > -
> > > -   ctl &= ~BMCR_ANENABLE;
> > > -
> > > -   if (phydev->speed == SPEED_100)
> > > -           ctl |= BMCR_SPEED100;
> > > -   else if (phydev->speed == SPEED_10)
> > > -           ctl &= ~BMCR_SPEED100;
> > > -   else
> > > -           dev_err(ds->dev, "unsupported speed: %d\n", phydev->speed);
> > > -
> > > -   if (phydev->duplex == DUPLEX_FULL)
> > > -           ctl |= BMCR_FULLDPLX;
> > > -   else
> > > -           ctl &= ~BMCR_FULLDPLX;
> > > -
> > > -   lan9303_phy_write(ds, port, MII_BMCR, ctl);
> >
> > Are you going to explain why modifying this register is no longer needed?
> 
> ... otherwise it is a fixed link, so the PHY is configured for the fixed
> link setting - which I think would end up writing to the an emulation of
> the PHY, and would end up writing the same settings back to the PHY as
> the PHY was already configured.
> 
> So, I don't think adjust_link does anything useful, and I think this is
> an entirely appropriate change.
> 
> 

I, too, thought I would need phylink_mac_config and phylink_mac_link_up to
completely migrate away from PHYLIB to PHYLINK.  But it seems as though the
phylink layer is now taking care of the speed / duplex / etc settings of the
phy registers.  There is no DSA driver housecleaning needed at these other
phylink_ api hook functions.  At least, that's my current level of
understanding...