linux-kernel - RE: [PATCH net-next] net: phy: microchip

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: <CO1PR11MB4771395A5D050DC1662E3C08E2BA2@CO1PR11MB4771.namprd11.prod.outlook.com>
Date: Fri, 9 Aug 2024 11:58:47 +0000
From: <Divya.Koppera@...rochip.com>
To: <linux@...linux.org.uk>
CC: <Arun.Ramadoss@...rochip.com>, <UNGLinuxDriver@...rochip.com>,
	<andrew@...n.ch>, <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 Russell,

Thanks for the review, please find my reply inline.

> -----Original Message-----
> From: Russell King <linux@...linux.org.uk>
> Sent: Thursday, August 8, 2024 5:49 PM
> To: Divya Koppera - I30481 <Divya.Koppera@...rochip.com>
> Cc: Arun Ramadoss - I17769 <Arun.Ramadoss@...rochip.com>;
> UNGLinuxDriver <UNGLinuxDriver@...rochip.com>; andrew@...n.ch;
> 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/tree/drivers/net/phy/phy_device.c#n3544

We will re-look into these settings while submitting auto-negotiation patch in future series.


> > +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/tree/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/tree/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.

> 
> Thanks.
> 
> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Thanks,
Divya