netdev - RE: [PATCH net-next] net: phy: microchip_t1: Adds support for LAN887x phy

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: <CO1PR11MB47718C1F661DE9CD19D98432E2852@CO1PR11MB4771.namprd11.prod.outlook.com>
Date: Mon, 12 Aug 2024 15:07:53 +0000
From: <Divya.Koppera@...rochip.com>
To: <andrew@...n.ch>
CC: <linux@...linux.org.uk>, <Arun.Ramadoss@...rochip.com>,
	<UNGLinuxDriver@...rochip.com>, <hkallweit1@...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] net: phy: microchip_t1: Adds support for LAN887x
 phy

Hi Andrew,

Thanks for review. My reply is inline.

> -----Original Message-----
> From: Andrew Lunn <andrew@...n.ch>
> Sent: Friday, August 9, 2024 6:51 PM
> To: Divya Koppera - I30481 <Divya.Koppera@...rochip.com>
> Cc: linux@...linux.org.uk; Arun Ramadoss - I17769
> <Arun.Ramadoss@...rochip.com>; UNGLinuxDriver
> <UNGLinuxDriver@...rochip.com>; hkallweit1@...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] net: phy: microchip_t1: Adds support for
> LAN887x phy
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > > On Thu, Aug 08, 2024 at 08:29:16PM +0530, Divya Koppera wrote:
> > > > +static int lan887x_config_init(struct phy_device *phydev) {
> > > > +     /* Disable pause frames */
> > > > +     linkmode_clear_bit(ETHTOOL_LINK_MODE_Pause_BIT, phydev-
> > > >supported);
> > > > +     /* Disable asym pause */
> > > > +     linkmode_clear_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT,
> > > > +phydev->supported);
> > >
> > > Why is this here? Pause frames are just like normal ethernet frames,
> > > they only have meaning to the MAC, not to the PHY.
> > >
> > > In any case, by the time the config_init() method has been called,
> > > the higher levels have already looked at phydev->supported and made
> > > decisions on what's there.
> > >
> >
> > We tried to disable this in get_features.
> > These are set again in phy_probe API.
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tr
> > ee/drivers/net/phy/phy_device.c#n3544
> >
> > We will re-look into these settings while submitting auto-negotiation patch
> in future series.
> 
> Let me see if i understand this correctly. You don't have autoneg at the
> moment. Hence you cannot negotiate pause. PHYLIB is setting pause is
> supported by default. Ethtool then probably suggests pause is supported, if
> the MAC you are using is not masking it out.
> 
> Since pause frames are just regular frames, the PHY should just be passing
> them through. So you should be able to forced pause, rather than autoneg
> pause:
> 
> ethtool --pause eth42 autoneg off] rx on tx on
> 
> assuming the MAC supports pause.
> 
> Does this still work if you clear the PUASE bits from supported as you are
> doing? Ideally we want to offer force paused configuration if the MAC
> supports it.
> 

I will recheck and apply in new version.

> > > > +static int lan887x_config_aneg(struct phy_device *phydev) {
> > > > +     int ret;
> > > > +
> > > > +     /* First patch only supports 100Mbps and 1000Mbps force-mode.
> > > > +      * T1 Auto-Negotiation (Clause 98 of IEEE 802.3) will be added later.
> > > > +      */
> > > > +     if (phydev->autoneg != AUTONEG_DISABLE) {
> > > > +             /* PHY state is inconsistent due to ANEG Enable set
> > > > +              * so we need to assign ANEG Disable for consistent behavior
> > > > +              */
> > > > +             phydev->autoneg = AUTONEG_DISABLE;
> > >
> > > If you clear phydev->supported's autoneg bit, then phylib ought to
> > > enforce this for you. Please check this rather than adding code to drivers.
> >
> > Phylib is checking if advertisement is empty or not, but the feature is not
> verified against supported parameter.
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tr
> > ee/drivers/net/phy/phy.c#n1092
> >
> > But in the following statement phylib is updating advertising parameter.
> > https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/tr
> > ee/drivers/net/phy/phy.c#n1113
> >
> > This is making the feature enabled in driver, the right thing is to fix the
> library.
> > We will fix the phylib in next series.
> 
> I'm not too surprised you are hitting such issues. Not actually supporting
> autoneg is pretty uncommon, and is not well tested. Thanks for offering to fix
> this up.
> 
>     Andrew

/Divya