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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ