netdev - Re: [PATCH net-next v2 13/14] net: phy: add Broadcom BCM84881 PHY driver

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: <20191219005545.GY1344@shell.armlinux.org.uk>
Date:   Thu, 19 Dec 2019 00:55:45 +0000
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 13/14] net: phy: add Broadcom BCM84881 PHY
 driver

On Tue, Dec 10, 2019 at 06:46:40PM +0000, Russell King - ARM Linux admin wrote:
> On Tue, Dec 10, 2019 at 05:58:37PM +0000, Russell King - ARM Linux admin wrote:
> > On Tue, Dec 10, 2019 at 09:34:16AM -0800, Florian Fainelli wrote:
> > > On 12/9/19 7:19 AM, Russell King wrote:
> > > > Add a rudimentary Clause 45 driver for the BCM84881 PHY, found on
> > > > Methode DM7052 SFPs.
> > > > 
> > > > Signed-off-by: Russell King <rmk+kernel@...linux.org.uk>
> > > 
> > > Reviewed-by: Florian Fainelli <f.fainelli@...il.com>
> > > 
> > > > ---
> > > >  drivers/net/phy/Kconfig    |   6 +
> > > >  drivers/net/phy/Makefile   |   1 +
> > > >  drivers/net/phy/bcm84881.c | 269 +++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 276 insertions(+)
> > > >  create mode 100644 drivers/net/phy/bcm84881.c
> > > > 
> > > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > > > index fe602648b99f..41272106dea9 100644
> > > > --- a/drivers/net/phy/Kconfig
> > > > +++ b/drivers/net/phy/Kconfig
> > > > @@ -329,6 +329,12 @@ config BROADCOM_PHY
> > > >  	  Currently supports the BCM5411, BCM5421, BCM5461, BCM54616S, BCM5464,
> > > >  	  BCM5481, BCM54810 and BCM5482 PHYs.
> > > >  
> > > > +config BCM84881_PHY
> > > > +	bool "Broadcom BCM84881 PHY"
> > > > +	depends on PHYLIB=y
> > > > +	---help---
> > > > +	  Support the Broadcom BCM84881 PHY.
> > > 
> > > Cannot we make this tristate, I believe we cannot until there are more
> > > fundamental issues (that you just reported) to be fixed, correct?
> > 
> > Indeed.  The problem I saw was that although the bcm84881 has the
> > PHY correctly described, for whatever reason, the module was not
> > loaded.
> > 
> > What I think is going in is that with modern udev userspace,
> > request_module() is not functional, and we do not publish the
> > module IDs for Clause 45 PHYs via uevent.  Consequently, there
> > exists no mechanism to load a Clause 45 PHY driver from the
> > filesystem.
> 
> I just attempted booting with sfp as a module, bcm84881 as a module.
> sfp has to be loaded for the SFP cage to be recognised, so module
> loading is availble prior to the PHY being known to the kernel.
> 
> The SFP is probed, and the PHY identified (via my debug):
> 
> [    7.209549] sfp sfp: phy PMA devid: 0xae02 0x5151
> 
> The PHY is not bound to its driver at this point.
> 
> We then try to connect to the PHY, but the support mask is zero,
> so we know nothing about what modes this PHY supports:
> 
> [    7.215985] mvneta f1034000.ethernet eno2: phylink_sfp_connect_phy: s=00,00000000,00000000 a=00,00000000,00000000
> [    7.215997] mvneta f1034000.ethernet eno2: validation with support 00,00000000,00000000 failed: -22
> [    7.226343] sfp sfp: sfp_add_phy failed: -22
> 
> and we fail - because we are unable to identify what mode we should
> configure the MAC side for, because we have no idea what the
> capabilities of the PHY are at this stage.
> 
> We can't wait until we've called phylink_attach_phy(), because that
> configures the PHY for the phy interface mode that was passed in.
> 
> There is no sign of the bcm84881 module being loaded.

Okay, I see what is going on - I just added debug into __request_module,
and got:

[  234.729163] __request_module: mdio:-10101110000000100101000101010001
[  234.732561] __request_module: mdio:-10101110000000100101000101010001
[  234.735729] __request_module: mdio:00000011011000100000000000000000

on inserting this SFP.  This comes from this:

#define MDIO_ID_FMT "%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d%d"
#define MDIO_ID_ARGS(_id) \
        (_id)>>31, ((_id)>>30) & 1, ((_id)>>29) & 1, ((_id)>>28) & 1,   \
        ((_id)>>27) & 1, ((_id)>>26) & 1, ((_id)>>25) & 1, ((_id)>>24) & 1, \
        ((_id)>>23) & 1, ((_id)>>22) & 1, ((_id)>>21) & 1, ((_id)>>20) & 1, \
        ((_id)>>19) & 1, ((_id)>>18) & 1, ((_id)>>17) & 1, ((_id)>>16) & 1, \
        ((_id)>>15) & 1, ((_id)>>14) & 1, ((_id)>>13) & 1, ((_id)>>12) & 1, \
        ((_id)>>11) & 1, ((_id)>>10) & 1, ((_id)>>9) & 1, ((_id)>>8) & 1, \
        ((_id)>>7) & 1, ((_id)>>6) & 1, ((_id)>>5) & 1, ((_id)>>4) & 1, \
        ((_id)>>3) & 1, ((_id)>>2) & 1, ((_id)>>1) & 1, (_id) & 1

coupled with:

static int phy_request_driver_module(struct phy_device *dev, int phy_id)
{
...
        ret = request_module(MDIO_MODULE_PREFIX MDIO_ID_FMT,
                             MDIO_ID_ARGS(phy_id));

The signed-ness of the parameter passed into MDIO_ID_ARGS() matters.
Hence, (0xae025151)>>31 becomes -1, and %d prints it as -1.

phy_id should be u32, just like it is everywhere else in phylib. Also,
MDIO_ID_ARGS() should probably be adapted to not care about the signed-
ness of its argument.

Thoughts?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up