From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Chocron, Jonathan" Subject: RE: [PATCH net-next 4/8] net: ethernet: add the Alpine Ethernet driver Date: Thu, 2 Nov 2017 16:05:59 +0000 Message-ID: <8023db8c8218450fb6c97049db054989@EX13D13UWA001.ant.amazon.com> References: <20170203181216.30214-1-antoine.tenart@free-electrons.com> <20170203181216.30214-5-antoine.tenart@free-electrons.com> <20170203205823.GA22572@lunn.ch> <1503841641816.62526@amazon.com> <20170828180932.GG10418@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Antoine Tenart , "netdev@vger.kernel.org" , "davem@davemloft.net" , "linux-arm-kernel@lists.infradead.org" , "thomas.petazzoni@free-electrons.com" , "arnd@arndb.de" , "BSHARA, Said" To: Andrew Lunn Return-path: Received: from smtp-fw-33001.amazon.com ([207.171.190.10]:31865 "EHLO smtp-fw-33001.amazon.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932280AbdKBQGd (ORCPT ); Thu, 2 Nov 2017 12:06:33 -0400 In-Reply-To: <20170828180932.GG10418@lunn.ch> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Monday, August 28, 2017 9:10 PM > To: Chocron, Jonathan > Cc: Antoine Tenart ; > netdev@vger.kernel.org; davem@davemloft.net; linux-arm- > kernel@lists.infradead.org; thomas.petazzoni@free-electrons.com; > arnd@arndb.de > Subject: Re: [PATCH net-next 4/8] net: ethernet: add the Alpine Ethernet > driver >=20 > On Sun, Aug 27, 2017 at 01:47:19PM +0000, Chocron, Jonathan wrote: > > This is a fixed version of my previous response (using proper indentati= on > and leaving only the specific questions responded to). >=20 > Wow, this is old. 3 Feb 2017. I had to go dig into the archive to refres= h my > memory. >=20 > > > > +/* MDIO */ > > > > +#define AL_ETH_MDIO_C45_DEV_MASK 0x1f0000 > > > > +#define AL_ETH_MDIO_C45_DEV_SHIFT 16 > > > > +#define AL_ETH_MDIO_C45_REG_MASK 0xffff > > > > + > > > > +static int al_mdio_read(struct mii_bus *bp, int mii_id, int reg) > > > > +{ > > > > + struct al_eth_adapter *adapter =3D bp->priv; > > > > + u16 value =3D 0; > > > > + int rc; > > > > + int timeout =3D MDIO_TIMEOUT_MSEC; > > > > + > > > > + while (timeout > 0) { > > > > + if (reg & MII_ADDR_C45) { > > > > + netdev_dbg(adapter->netdev, "[c45]: dev %x re= g %x val > %x\n", > > > > + ((reg & AL_ETH_MDIO_C45_DEV_MASK) = >> > AL_ETH_MDIO_C45_DEV_SHIFT), > > > > + (reg & AL_ETH_MDIO_C45_REG_MASK), = value); > > > > + rc =3D al_eth_mdio_read(&adapter->hw_adapter,= adapter- > >phy_addr, > > > > + ((reg & AL_ETH_MDIO_C45_DEV_MASK) >> > AL_ETH_MDIO_C45_DEV_SHIFT), > > > > + (reg & AL_ETH_MDIO_C45_REG_MASK), &va= lue); > > > > + } else { > > > > + rc =3D al_eth_mdio_read(&adapter->hw_adapter,= adapter- > >phy_addr, > > > > + MDIO_DEVAD_NONE, reg, &= value); > > > > + } > > > > + > > > > + if (rc =3D=3D 0) > > > > + return value; > > > > + > > > > + netdev_dbg(adapter->netdev, > > > > + "mdio read failed. try again in 10 > > > > + msec\n"); > > > > + > > > > + timeout -=3D 10; > > > > + msleep(10); > > > > + } > > > > > > This is rather unusual, retrying MDIO operations. Are you working > > > around a hardware bug? I suspect this also opens up race conditions, > > > in particular with PHY interrupts, which can be clear on read. > > > > The MDIO bus is shared between the ethernet units. There is a HW lock > > used to arbitrate between different interfaces trying to access the > > bus, therefore there is a retry loop. The reg isn't accessed before > > obtaining the lock, so there shouldn't be any clear on read issues. > > > > > > +/* al_eth_mdiobus_setup - initialize mdiobus and register to > > > > +kernel */ static int al_eth_mdiobus_setup(struct al_eth_adapter > > > > +*adapter) { > > > > + struct phy_device *phydev; > > > > + int i; > > > > + int ret =3D 0; > > > > + > > > > + adapter->mdio_bus =3D mdiobus_alloc(); > > > > + if (!adapter->mdio_bus) > > > > + return -ENOMEM; > > > > + > > > > + adapter->mdio_bus->name =3D "al mdio bus"; > > > > + snprintf(adapter->mdio_bus->id, MII_BUS_ID_SIZE, "%x", > > > > + (adapter->pdev->bus->number << 8) | adapter->pdev- > >devfn); > > > > + adapter->mdio_bus->priv =3D adapter; > > > > + adapter->mdio_bus->parent =3D &adapter->pdev->dev; > > > > + adapter->mdio_bus->read =3D &al_mdio_read; > > > > + adapter->mdio_bus->write =3D &al_mdio_write; > > > > + adapter->mdio_bus->phy_mask =3D ~BIT(adapter->phy_addr); > > > > > > Why do this? > > > > Since the MDIO bus is shared, we want each interface to probe only for = the > PHY associated with it. >=20 > So i think this is the core of the problem. You have one physical MDIO bu= s, > yet you register it twice with the MDIO framework. >=20 > How about you only register it once? A lot of the complexity then goes aw= ay. > The mutex in the mdio core per bus means you don't need your hardware > locking. All that code goes away. All the retry code goes away. Life is s= imple. >=20 > Andrew We indeed have one physical MDIO bus, but have multiple masters on it, each "behind" a different internal PCIe device. Since the accesses to the b= us are done "indirectly" through each master, we can't register the bus only o= nce. Think of the scenario that we register it in the driver context of PCIe dev= ice A, and then the driver is unbound from just this device. Device B won't be abl= e to access the bus since it was registered with callbacks that use a PCIe BA= R of device A, which is no longer valid. Is it possible to register the mdio_bus struct as a global instance at driv= er load, and someway pass the offset to the specific device's MDIO master, as part o= f each read/write transaction towards the MDIO bus? Or perhaps you have another suggestion which takes into account the issues = I've described? Jonathan