[PATCH RFC 0/7] Clause 45 PHY probing cleanups

[PATCH RFC 0/7] Clause 45 PHY probing cleanups

[Russell King - ARM Linux admin]

Hi,

In response to the patch set that Jeremy posted, this is my proposal
to expand our Clause 45 PHY probing.

I've taken a slightly different approach, with the view to avoiding
as much behavioural change as possible.  The biggest difference is
to do with "devices_in_package" - we were using it for two different
purposes, which are now separated.

This is not against net-next nor net trees, but against my own private
tree, but I'm posting it to serve as an illustration of what I think
should be done - I knocked this up this morning.

The only potential regression that I'm expecting is with 88x3310 PHYs
of the later revision, which have the clause 22 registers implemented.
I haven't yet checked whether they set bit 0, but if they do, the
various decision points that we have based on that bit could adversely
affect this PHY - it needs testing, which I'll do when I dig out the
appropriate hardware.  Probably also needs the 2110 PHYs checked as
well.

I haven't tested this series yet beyond compile testing.

Given the proximity of the merge window, this *isn't* code I'd like to
see merged into net-next - it's way too risky at this point.  So, we
have time to consider our options.

 drivers/net/phy/bcm87xx.c    |   2 +-
 drivers/net/phy/cortina.c    |   3 +-
 drivers/net/phy/phy-c45.c    |   4 +-
 drivers/net/phy/phy-core.c   |  11 ++--
 drivers/net/phy/phy.c        |   4 +-
 drivers/net/phy/phy_device.c | 141 +++++++++++++++++++++++++++----------------
 drivers/net/phy/phylink.c    |  19 +++---
 include/linux/mdio.h         |  31 ++++++++++
 include/linux/phy.h          |  14 ++---
 9 files changed, 146 insertions(+), 83 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

[PATCH RFC 1/7] net: mdiobus: add clause 45 mdiobus accessors

[Russell King]

There is a recurring pattern throughout some of the PHY code converting
a devad and regnum to our packed clause 45 representation. Rather than
having this scattered around the code, let's put a common translation
function in mdio.h, and provide some register accessors.

Convert the phylib core, phylink, bcm87xx and cortina to use these.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/bcm87xx.c    |  2 +-
 drivers/net/phy/cortina.c    |  3 +--
 drivers/net/phy/phy-core.c   | 11 ++++-------
 drivers/net/phy/phy.c        |  4 ++--
 drivers/net/phy/phy_device.c | 20 ++++++++------------
 drivers/net/phy/phylink.c    | 11 +++++------
 include/linux/mdio.h         | 31 +++++++++++++++++++++++++++++++
 include/linux/phy.h          |  6 ------
 8 files changed, 52 insertions(+), 36 deletions(-)

diff --git a/drivers/net/phy/bcm87xx.c b/drivers/net/phy/bcm87xx.c
index f6dce6850850..df360e1c5069 100644
--- a/drivers/net/phy/bcm87xx.c
+++ b/drivers/net/phy/bcm87xx.c
@@ -55,7 +55,7 @@ static int bcm87xx_of_reg_init(struct phy_device *phydev)
 		u16 mask	= be32_to_cpup(paddr++);
 		u16 val_bits	= be32_to_cpup(paddr++);
 		int val;
-		u32 regnum = MII_ADDR_C45 | (devid << 16) | reg;
+		u32 regnum = mdiobus_c45_addr(devid, reg);
 		val = 0;
 		if (mask) {
 			val = phy_read(phydev, regnum);
diff --git a/drivers/net/phy/cortina.c b/drivers/net/phy/cortina.c
index 856cdc36aacd..d254f517d4d4 100644
--- a/drivers/net/phy/cortina.c
+++ b/drivers/net/phy/cortina.c
@@ -17,8 +17,7 @@
 
 static int cortina_read_reg(struct phy_device *phydev, u16 regnum)
 {
-	return mdiobus_read(phydev->mdio.bus, phydev->mdio.addr,
-			    MII_ADDR_C45 | regnum);
+	return mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr, 0, regnum);
 }
 
 static int cortina_read_status(struct phy_device *phydev)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 204a6d94535e..7182ed81f3ec 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -390,9 +390,8 @@ int __phy_read_mmd(struct phy_device *phydev, int devad, u32 regnum)
 	if (phydev->drv->read_mmd) {
 		val = phydev->drv->read_mmd(phydev, devad, regnum);
 	} else if (phydev->is_c45) {
-		u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
-
-		val = __mdiobus_read(phydev->mdio.bus, phydev->mdio.addr, addr);
+		val = __mdiobus_c45_read(phydev->mdio.bus, phydev->mdio.addr,
+					 devad, regnum);
 	} else {
 		struct mii_bus *bus = phydev->mdio.bus;
 		int phy_addr = phydev->mdio.addr;
@@ -447,10 +446,8 @@ int __phy_write_mmd(struct phy_device *phydev, int devad, u32 regnum, u16 val)
 	if (phydev->drv && phydev->drv->write_mmd) {
 		ret = phydev->drv->write_mmd(phydev, devad, regnum, val);
 	} else if (phydev->is_c45) {
-		u32 addr = MII_ADDR_C45 | (devad << 16) | (regnum & 0xffff);
-
-		ret = __mdiobus_write(phydev->mdio.bus, phydev->mdio.addr,
-				      addr, val);
+		ret = __mdiobus_c45_write(phydev->mdio.bus, phydev->mdio.addr,
+					  devad, regnum, val);
 	} else {
 		struct mii_bus *bus = phydev->mdio.bus;
 		int phy_addr = phydev->mdio.addr;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 7bded691a5d3..d38de72c60c5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -353,7 +353,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
 			prtad = mdio_phy_id_prtad(mii_data->phy_id);
 			devad = mdio_phy_id_devad(mii_data->phy_id);
-			devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
+			devad = mdiobus_c45_addr(devad, mii_data->reg_num);
 		} else {
 			prtad = mii_data->phy_id;
 			devad = mii_data->reg_num;
@@ -366,7 +366,7 @@ int phy_mii_ioctl(struct phy_device *phydev, struct ifreq *ifr, int cmd)
 		if (mdio_phy_id_is_c45(mii_data->phy_id)) {
 			prtad = mdio_phy_id_prtad(mii_data->phy_id);
 			devad = mdio_phy_id_devad(mii_data->phy_id);
-			devad = MII_ADDR_C45 | devad << 16 | mii_data->reg_num;
+			devad = mdiobus_c45_addr(devad, mii_data->reg_num);
 		} else {
 			prtad = mii_data->phy_id;
 			devad = mii_data->reg_num;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 16728bb73766..d14c4ba24a90 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -673,16 +673,14 @@ EXPORT_SYMBOL(phy_device_create);
 static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
 				   u32 *devices_in_package)
 {
-	int phy_reg, reg_addr;
+	int phy_reg;
 
-	reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS2;
-	phy_reg = mdiobus_read(bus, addr, reg_addr);
+	phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS2);
 	if (phy_reg < 0)
 		return -EIO;
 	*devices_in_package = phy_reg << 16;
 
-	reg_addr = MII_ADDR_C45 | dev_addr << 16 | MDIO_DEVS1;
-	phy_reg = mdiobus_read(bus, addr, reg_addr);
+	phy_reg = mdiobus_c45_read(bus, addr, dev_addr, MDIO_DEVS1);
 	if (phy_reg < 0)
 		return -EIO;
 	*devices_in_package |= phy_reg;
@@ -707,11 +705,11 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
  *
  */
 static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
-			   struct phy_c45_device_ids *c45_ids) {
-	int phy_reg;
-	int i, reg_addr;
+			   struct phy_c45_device_ids *c45_ids)
+{
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
 	u32 *devs = &c45_ids->devices_in_package;
+	int i, phy_reg;
 
 	/* Find first non-zero Devices In package. Device zero is reserved
 	 * for 802.3 c45 complied PHYs, so don't probe it at first.
@@ -745,14 +743,12 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 		if (!(c45_ids->devices_in_package & (1 << i)))
 			continue;
 
-		reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1;
-		phy_reg = mdiobus_read(bus, addr, reg_addr);
+		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] = phy_reg << 16;
 
-		reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2;
-		phy_reg = mdiobus_read(bus, addr, reg_addr);
+		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID2);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] |= phy_reg;
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 425c24ba4bbe..6defd5eddd58 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1702,7 +1702,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
 	if (mdio_phy_id_is_c45(phy_id)) {
 		prtad = mdio_phy_id_prtad(phy_id);
 		devad = mdio_phy_id_devad(phy_id);
-		devad = MII_ADDR_C45 | devad << 16 | reg;
+		devad = mdiobus_c45_addr(devad << 16, reg);
 	} else if (phydev->is_c45) {
 		switch (reg) {
 		case MII_BMCR:
@@ -1725,7 +1725,7 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
 			return -EINVAL;
 		}
 		prtad = phy_id;
-		devad = MII_ADDR_C45 | devad << 16 | reg;
+		devad = mdiobus_c45_addr(devad, reg);
 	} else {
 		prtad = phy_id;
 		devad = reg;
@@ -1742,7 +1742,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
 	if (mdio_phy_id_is_c45(phy_id)) {
 		prtad = mdio_phy_id_prtad(phy_id);
 		devad = mdio_phy_id_devad(phy_id);
-		devad = MII_ADDR_C45 | devad << 16 | reg;
+		devad = mdiobus_c45_addr(devad, reg);
 	} else if (phydev->is_c45) {
 		switch (reg) {
 		case MII_BMCR:
@@ -1765,7 +1765,7 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
 			return -EINVAL;
 		}
 		prtad = phy_id;
-		devad = MII_ADDR_C45 | devad << 16 | reg;
+		devad = mdiobus_c45_addr(devad, reg);
 	} else {
 		prtad = phy_id;
 		devad = reg;
@@ -2525,7 +2525,6 @@ void phylink_mii_c22_pcs_an_restart(struct mdio_device *pcs)
 }
 EXPORT_SYMBOL_GPL(phylink_mii_c22_pcs_an_restart);
 
-#define C45_ADDR(d,a)	(MII_ADDR_C45 | (d) << 16 | (a))
 void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
 				   struct phylink_link_state *state)
 {
@@ -2533,7 +2532,7 @@ void phylink_mii_c45_pcs_get_state(struct mdio_device *pcs,
 	int addr = pcs->addr;
 	int stat;
 
-	stat = mdiobus_read(bus, addr, C45_ADDR(MDIO_MMD_PCS, MDIO_STAT1));
+	stat = mdiobus_c45_read(bus, addr, MDIO_MMD_PCS, MDIO_STAT1);
 	if (stat < 0) {
 		state->link = false;
 		return;
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 917e4bb2ed71..36d2e0673d03 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -9,6 +9,13 @@
 #include <uapi/linux/mdio.h>
 #include <linux/mod_devicetable.h>
 
+/* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
+ * IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips.
+ */
+#define MII_ADDR_C45		(1<<30)
+#define MII_DEVADDR_C45_SHIFT	16
+#define MII_REGADDR_C45_MASK	GENMASK(15, 0)
+
 struct gpio_desc;
 struct mii_bus;
 
@@ -326,6 +333,30 @@ int mdiobus_write_nested(struct mii_bus *bus, int addr, u32 regnum, u16 val);
 int mdiobus_modify(struct mii_bus *bus, int addr, u32 regnum, u16 mask,
 		   u16 set);
 
+static inline u32 mdiobus_c45_addr(int devad, u16 regnum)
+{
+	return MII_ADDR_C45 | devad << MII_DEVADDR_C45_SHIFT | regnum;
+}
+
+static inline int __mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
+				     u16 regnum)
+{
+	return __mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
+}
+
+static inline int __mdiobus_c45_write(struct mii_bus *bus, int prtad, int devad,
+				      u16 regnum, u16 val)
+{
+	return __mdiobus_write(bus, prtad, mdiobus_c45_addr(devad, regnum),
+			       val);
+}
+
+static inline int mdiobus_c45_read(struct mii_bus *bus, int prtad, int devad,
+				   u16 regnum)
+{
+	return mdiobus_read(bus, prtad, mdiobus_c45_addr(devad, regnum));
+}
+
 int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 90f5dfb835cc..9b7c46cf14d3 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -224,12 +224,6 @@ static inline const char *phy_modes(phy_interface_t interface)
 
 #define MII_BUS_ID_SIZE	61
 
-/* Or MII_ADDR_C45 into regnum for read/write on mii_bus to enable the 21 bit
-   IEEE 802.3ae clause 45 addressing mode used by 10GIGE phy chips. */
-#define MII_ADDR_C45 (1<<30)
-#define MII_DEVADDR_C45_SHIFT	16
-#define MII_REGADDR_C45_MASK	GENMASK(15, 0)
-
 struct device;
 struct phylink;
 struct sfp_bus;
-- 
2.20.1

[PATCH RFC 2/7] net: phy: clean up cortina workaround

[Russell King]

Move the Cortina PHY workaround out of the "devices in package" loop;
it doesn't need to be in there as the control flow will terminate the
loop once we enter the workaround irrespective of the workaround's
outcome. The workaround is triggered by the ID being mostly 1's, which
will in any case terminate the loop.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 28 +++++++++++++---------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d14c4ba24a90..e04284c4ebf8 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -718,23 +718,21 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
 		if (phy_reg < 0)
 			return -EIO;
+	}
+
+	if ((*devs & 0x1fffffff) == 0x1fffffff) {
+		/* If mostly Fs, there is no device there, then let's probe
+		 * MMD 0, as some 10G PHYs have zero Devices In package,
+		 * e.g. Cortina CS4315/CS4340 PHY.
+		 */
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
+		if (phy_reg < 0)
+			return -EIO;
 
+		/* no device there, let's get out of here */
 		if ((*devs & 0x1fffffff) == 0x1fffffff) {
-			/*  If mostly Fs, there is no device there,
-			 *  then let's continue to probe more, as some
-			 *  10G PHYs have zero Devices In package,
-			 *  e.g. Cortina CS4315/CS4340 PHY.
-			 */
-			phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
-			if (phy_reg < 0)
-				return -EIO;
-			/* no device there, let's get out of here */
-			if ((*devs & 0x1fffffff) == 0x1fffffff) {
-				*phy_id = 0xffffffff;
-				return 0;
-			} else {
-				break;
-			}
+			*phy_id = 0xffffffff;
+			return 0;
 		}
 	}
 
-- 
2.20.1

[PATCH RFC 3/7] net: phy: clean up PHY ID reading

[Russell King]

Rearrange the code to read the PHY IDs, so we don't call get_phy_id()
only to immediately call get_phy_c45_ids().  Move that logic into
get_phy_device(), which results in better readability.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index e04284c4ebf8..0d6b6ca66216 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -756,29 +756,18 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 }
 
 /**
- * get_phy_id - reads the specified addr for its ID.
+ * get_phy_c22_id - reads the specified addr for its clause 22 ID.
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
  * @phy_id: where to store the ID retrieved.
- * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
- * @c45_ids: where to store the c45 ID information.
- *
- * Description: In the case of a 802.3-c22 PHY, reads the ID registers
- *   of the PHY at @addr on the @bus, stores it in @phy_id and returns
- *   zero on success.
- *
- *   In the case of a 802.3-c45 PHY, get_phy_c45_ids() is invoked, and
- *   its return value is in turn returned.
  *
+ * Read the 802.3 clause 22 PHY ID from the PHY at @addr on the @bus.
+ * Return the PHY ID read from the PHY in @phy_id on successful access.
  */
-static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
-		      bool is_c45, struct phy_c45_device_ids *c45_ids)
+static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
 {
 	int phy_reg;
 
-	if (is_c45)
-		return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
-
 	/* Grab the bits from PHYIR1, and put them in the upper half */
 	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
 	if (phy_reg < 0) {
@@ -817,7 +806,11 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	c45_ids.devices_in_package = 0;
 	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
 
-	r = get_phy_id(bus, addr, &phy_id, is_c45, &c45_ids);
+	if (is_c45)
+		r = get_phy_c45_ids(bus, addr, &phy_id, &c45_ids);
+	else
+		r = get_phy_c22_id(bus, addr, &phy_id);
+
 	if (r)
 		return ERR_PTR(r);
 
-- 
2.20.1

[PATCH RFC 4/7] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Russell King]

Add support for probing MMDs above 7 for a valid devices-in-package
specifier.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++--
 include/linux/phy.h          |  2 ++
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 0d6b6ca66216..fa9164ac0f3d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 }
 EXPORT_SYMBOL(phy_device_create);
 
+/* phy_c45_probe_present - checks to see if a MMD is present in the package
+ * @bus: the target MII bus
+ * @prtad: PHY package address on the MII bus
+ * @devad: PHY device (MMD) address
+ *
+ * Read the MDIO_STAT2 register, and check whether a device is responding
+ * at this address.
+ *
+ * Returns: negative error number on bus access error, zero if no device
+ * is responding, or positive if a device is present.
+ */
+static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
+{
+	int stat2;
+
+	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
+	if (stat2 < 0)
+		return stat2;
+
+	return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL;
+}
+
 /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
@@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 {
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
 	u32 *devs = &c45_ids->devices_in_package;
-	int i, phy_reg;
+	int i, ret, phy_reg;
 
 	/* Find first non-zero Devices In package. Device zero is reserved
 	 * for 802.3 c45 complied PHYs, so don't probe it at first.
 	 */
-	for (i = 1; i < num_ids && *devs == 0; i++) {
+	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
+		if (i >= 8) {
+			/* Only probe for the devices-in-package if there
+			 * is a PHY reporting as present here; this avoids
+			 * picking up on PHYs that implement non-IEEE802.3
+			 * compliant register spaces.
+			 */
+			ret = phy_c45_probe_present(bus, addr, i);
+			if (ret < 0)
+				return -EIO;
+
+			if (!ret)
+				continue;
+		}
 		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
 		if (phy_reg < 0)
 			return -EIO;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 9b7c46cf14d3..41c046545354 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -350,6 +350,8 @@ enum phy_state {
 	PHY_NOLINK,
 };
 
+#define MDIO_MMD_NUM 32
+
 /**
  * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
  * @devices_in_package: Bit vector of devices present.
-- 
2.20.1

[PATCH RFC 5/7] net: phy: set devices_in_package only after validation

[Russell King]

Only set the devices_in_package to a non-zero value if we find a valid
value for this field, so we avoid leaving it set to e.g. 0x1fffffff.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index fa9164ac0f3d..a483d79cfc87 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -730,13 +730,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 			   struct phy_c45_device_ids *c45_ids)
 {
 	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
-	u32 *devs = &c45_ids->devices_in_package;
+	u32 devs_in_pkg = 0;
 	int i, ret, phy_reg;
 
 	/* Find first non-zero Devices In package. Device zero is reserved
 	 * for 802.3 c45 complied PHYs, so don't probe it at first.
 	 */
-	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
+	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0; i++) {
 		if (i >= 8) {
 			/* Only probe for the devices-in-package if there
 			 * is a PHY reporting as present here; this avoids
@@ -750,22 +750,22 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 			if (!ret)
 				continue;
 		}
-		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
 		if (phy_reg < 0)
 			return -EIO;
 	}
 
-	if ((*devs & 0x1fffffff) == 0x1fffffff) {
+	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
 		/* If mostly Fs, there is no device there, then let's probe
 		 * MMD 0, as some 10G PHYs have zero Devices In package,
 		 * e.g. Cortina CS4315/CS4340 PHY.
 		 */
-		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
+		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
 		if (phy_reg < 0)
 			return -EIO;
 
 		/* no device there, let's get out of here */
-		if ((*devs & 0x1fffffff) == 0x1fffffff) {
+		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
 			*phy_id = 0xffffffff;
 			return 0;
 		}
@@ -773,7 +773,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 
 	/* Now probe Device Identifiers for each device present. */
 	for (i = 1; i < num_ids; i++) {
-		if (!(c45_ids->devices_in_package & (1 << i)))
+		if (!(devs_in_pkg & (1 << i)))
 			continue;
 
 		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
@@ -786,6 +786,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 			return -EIO;
 		c45_ids->device_ids[i] |= phy_reg;
 	}
+
+	c45_ids->devices_in_package = devs_in_pkg;
+
 	*phy_id = 0;
 	return 0;
 }
-- 
2.20.1

[PATCH RFC 6/7] net: phy: split devices_in_package

[Russell King]

We have two competing requirements for the devices_in_package field.
We want to use it as a bit array indicating which MMDs are present, but
we also want to know if the Clause 22 registers are present.

Since "devices in package" is a term used in the 802.3 specification,
keep this as the as-specified values read from the PHY, and introduce
a new member "mmds_present" to indicate which MMDs are actually
present in the PHY, derived from the "devices in package" value.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy-c45.c    | 4 ++--
 drivers/net/phy/phy_device.c | 6 +++---
 drivers/net/phy/phylink.c    | 8 ++++----
 include/linux/phy.h          | 4 +++-
 4 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 8cd952950a75..4b5805e2bd0e 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -219,7 +219,7 @@ int genphy_c45_read_link(struct phy_device *phydev)
 	int val, devad;
 	bool link = true;
 
-	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
+	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
 		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
 		if (val < 0)
 			return val;
@@ -397,7 +397,7 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
 	int val;
 
 	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
-	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
+	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
 		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
 		if (val < 0)
 			return val;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a483d79cfc87..1c948bbf4fa0 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -707,9 +707,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
 		return -EIO;
 	*devices_in_package |= phy_reg;
 
-	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
-	*devices_in_package &= ~BIT(0);
-
 	return 0;
 }
 
@@ -788,6 +785,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 	}
 
 	c45_ids->devices_in_package = devs_in_pkg;
+	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
+	c45_ids->mmds_present = devs_in_pkg & ~BIT(0);
 
 	*phy_id = 0;
 	return 0;
@@ -842,6 +841,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	int r;
 
 	c45_ids.devices_in_package = 0;
+	c45_ids.mmds_present = 0;
 	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
 
 	if (is_c45)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 6defd5eddd58..b548e0418694 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1709,11 +1709,11 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
 		case MII_BMSR:
 		case MII_PHYSID1:
 		case MII_PHYSID2:
-			devad = __ffs(phydev->c45_ids.devices_in_package);
+			devad = __ffs(phydev->c45_ids.mmds_present);
 			break;
 		case MII_ADVERTISE:
 		case MII_LPA:
-			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
+			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
 				return -EINVAL;
 			devad = MDIO_MMD_AN;
 			if (reg == MII_ADVERTISE)
@@ -1749,11 +1749,11 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
 		case MII_BMSR:
 		case MII_PHYSID1:
 		case MII_PHYSID2:
-			devad = __ffs(phydev->c45_ids.devices_in_package);
+			devad = __ffs(phydev->c45_ids.mmds_present);
 			break;
 		case MII_ADVERTISE:
 		case MII_LPA:
-			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
+			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
 				return -EINVAL;
 			devad = MDIO_MMD_AN;
 			if (reg == MII_ADVERTISE)
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 41c046545354..0d41c710339a 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -354,11 +354,13 @@ enum phy_state {
 
 /**
  * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
- * @devices_in_package: Bit vector of devices present.
+ * @devices_in_package: IEEE 802.3 devices in package register value.
+ * @mmds_present: bit vector of MMDs present.
  * @device_ids: The device identifer for each present device.
  */
 struct phy_c45_device_ids {
 	u32 devices_in_package;
+	u32 mmds_present;
 	u32 device_ids[8];
 };
 
-- 
2.20.1

[PATCH RFC 7/7] net: phy: read MMD ID from all present MMDs

[Russell King]

Expand the device_ids[] array to allow all MMD IDs to be read rather
than just the first 8 MMDs, but only read the ID if the MDIO_STAT2
register reports that a device really is present here for these new
devices to maintain compatibility with our current behaviour.

88X3310 PHY vendor MMDs do are marked as present in the
devices_in_package, but do not contain IEE 802.3 compatible register
sets in their lower space.  This avoids reading incorrect values as MMD
identifiers.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phy_device.c | 14 ++++++++++++++
 include/linux/phy.h          |  2 +-
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1c948bbf4fa0..92742c7be80f 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -773,6 +773,20 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 		if (!(devs_in_pkg & (1 << i)))
 			continue;
 
+		if (i >= 8) {
+			/* Only probe the MMD ID for MMDs >= 8 if they report
+			 * that they are present. We have at least one PHY that
+			 * reports MMD presence in devs_in_pkg, but does not
+			 * contain valid IEEE 802.3 ID registers in some MMDs.
+			 */
+			ret = phy_c45_probe_present(bus, addr, i);
+			if (ret < 0)
+				return ret;
+
+			if (!ret)
+				continue;
+		}
+
 		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
 		if (phy_reg < 0)
 			return -EIO;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 0d41c710339a..3325dd8fb9ac 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -361,7 +361,7 @@ enum phy_state {
 struct phy_c45_device_ids {
 	u32 devices_in_package;
 	u32 mmds_present;
-	u32 device_ids[8];
+	u32 device_ids[MDIO_MMD_NUM];
 };
 
 struct macsec_context;
-- 
2.20.1

Re: [PATCH RFC 1/7] net: mdiobus: add clause 45 mdiobus accessors

[Andrew Lunn]

On Tue, May 26, 2020 at 03:31:01PM +0100, Russell King wrote:
> There is a recurring pattern throughout some of the PHY code converting
> a devad and regnum to our packed clause 45 representation. Rather than
> having this scattered around the code, let's put a common translation
> function in mdio.h, and provide some register accessors.
> 
> Convert the phylib core, phylink, bcm87xx and cortina to use these.

Hi Russell

This is a useful patch whatever we decide about C45 probing. If you
can do some basic testing of it, i say submit it for this merge
window.

	Andrew

Re: [PATCH RFC 1/7] net: mdiobus: add clause 45 mdiobus accessors

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 04:39:06PM +0200, Andrew Lunn wrote:
> On Tue, May 26, 2020 at 03:31:01PM +0100, Russell King wrote:
> > There is a recurring pattern throughout some of the PHY code converting
> > a devad and regnum to our packed clause 45 representation. Rather than
> > having this scattered around the code, let's put a common translation
> > function in mdio.h, and provide some register accessors.
> > 
> > Convert the phylib core, phylink, bcm87xx and cortina to use these.
> 
> Hi Russell
> 
> This is a useful patch whatever we decide about C45 probing. If you
> can do some basic testing of it, i say submit it for this merge
> window.

It's almost fine, except for one << 16 I seem to have left in
phylink.c.

I can also report that the 2nd revision of the 88x3310 PHY does
_not_ have bit 0 set in the devices-in-package (just like the first
revision).  The 2nd revision should respond to clause 22 cycles, but
as it's connected to the XSMI interface on the 8040, clause 22 cycles
can't be generated.

Also, I found this in linux/mdio.h:

#define MDIO_SUPPORTS_C22               1
#define MDIO_SUPPORTS_C45               2
#define MDIO_EMULATE_C22                4

which are for use with struct mdio_if_info which we don't use in
phylib.  That seems relevant to our discussions last night.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 7/7] net: phy: read MMD ID from all present MMDs

[Jeremy Linton]

Hi,

On 5/26/20 9:31 AM, Russell King wrote:
> Expand the device_ids[] array to allow all MMD IDs to be read rather
> than just the first 8 MMDs, but only read the ID if the MDIO_STAT2
> register reports that a device really is present here for these new
> devices to maintain compatibility with our current behaviour.
> 
> 88X3310 PHY vendor MMDs do are marked as present in the
> devices_in_package, but do not contain IEE 802.3 compatible register
> sets in their lower space.  This avoids reading incorrect values as MMD
> identifiers.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/net/phy/phy_device.c | 14 ++++++++++++++
>   include/linux/phy.h          |  2 +-
>   2 files changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 1c948bbf4fa0..92742c7be80f 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -773,6 +773,20 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>   		if (!(devs_in_pkg & (1 << i)))
>   			continue;
>   
> +		if (i >= 8) {
> +			/* Only probe the MMD ID for MMDs >= 8 if they report
> +			 * that they are present. We have at least one PHY that
> +			 * reports MMD presence in devs_in_pkg, but does not
> +			 * contain valid IEEE 802.3 ID registers in some MMDs.
> +			 */
> +			ret = phy_c45_probe_present(bus, addr, i);
> +			if (ret < 0)
> +				return ret;
> +
> +			if (!ret)
> +				continue;
> +		}
> +
>   		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
>   		if (phy_reg < 0)
>   			return -EIO;
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 0d41c710339a..3325dd8fb9ac 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -361,7 +361,7 @@ enum phy_state {
>   struct phy_c45_device_ids {
>   	u32 devices_in_package;
>   	u32 mmds_present;
> -	u32 device_ids[8];
> +	u32 device_ids[MDIO_MMD_NUM];

You have a array overflow/invalid access if you don't do this earlier in 
4/7.


>   };
>   
>   struct macsec_context;
> 

Re: [PATCH RFC 3/7] net: phy: clean up PHY ID reading

[Jeremy Linton]

Hi,

On 5/26/20 9:31 AM, Russell King wrote:
> Rearrange the code to read the PHY IDs, so we don't call get_phy_id()
> only to immediately call get_phy_c45_ids().  Move that logic into
> get_phy_device(), which results in better readability.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/net/phy/phy_device.c | 25 +++++++++----------------
>   1 file changed, 9 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index e04284c4ebf8..0d6b6ca66216 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -756,29 +756,18 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>   }
>   
>   /**
> - * get_phy_id - reads the specified addr for its ID.
> + * get_phy_c22_id - reads the specified addr for its clause 22 ID.
>    * @bus: the target MII bus
>    * @addr: PHY address on the MII bus
>    * @phy_id: where to store the ID retrieved.
> - * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
> - * @c45_ids: where to store the c45 ID information.
> - *
> - * Description: In the case of a 802.3-c22 PHY, reads the ID registers
> - *   of the PHY at @addr on the @bus, stores it in @phy_id and returns
> - *   zero on success.
> - *
> - *   In the case of a 802.3-c45 PHY, get_phy_c45_ids() is invoked, and
> - *   its return value is in turn returned.
>    *
> + * Read the 802.3 clause 22 PHY ID from the PHY at @addr on the @bus.
> + * Return the PHY ID read from the PHY in @phy_id on successful access.
>    */
> -static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
> -		      bool is_c45, struct phy_c45_device_ids *c45_ids)
> +static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
>   {
>   	int phy_reg;
>   
> -	if (is_c45)
> -		return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
> -
>   	/* Grab the bits from PHYIR1, and put them in the upper half */
>   	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
>   	if (phy_reg < 0) {
> @@ -817,7 +806,11 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
>   	c45_ids.devices_in_package = 0;
>   	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
>   
> -	r = get_phy_id(bus, addr, &phy_id, is_c45, &c45_ids);
> +	if (is_c45)
> +		r = get_phy_c45_ids(bus, addr, &phy_id, &c45_ids);
> +	else
> +		r = get_phy_c22_id(bus, addr, &phy_id);
> +
>   	if (r)
>   		return ERR_PTR(r);
>   
> 

I see this, and the c22 regs detection, but I don't see how your 
choosing to use the c22 regs if the 45's aren't responding. Which was 
one of the primary purposes of that other set.

Re: [PATCH RFC 6/7] net: phy: split devices_in_package

[Jeremy Linton]

Hi,

On 5/26/20 9:31 AM, Russell King wrote:
> We have two competing requirements for the devices_in_package field.
> We want to use it as a bit array indicating which MMDs are present, but
> we also want to know if the Clause 22 registers are present.
> 
> Since "devices in package" is a term used in the 802.3 specification,
> keep this as the as-specified values read from the PHY, and introduce
> a new member "mmds_present" to indicate which MMDs are actually
> present in the PHY, derived from the "devices in package" value.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/net/phy/phy-c45.c    | 4 ++--
>   drivers/net/phy/phy_device.c | 6 +++---
>   drivers/net/phy/phylink.c    | 8 ++++----
>   include/linux/phy.h          | 4 +++-
>   4 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 8cd952950a75..4b5805e2bd0e 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -219,7 +219,7 @@ int genphy_c45_read_link(struct phy_device *phydev)
>   	int val, devad;
>   	bool link = true;
>   
> -	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> +	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
>   		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
>   		if (val < 0)
>   			return val;
> @@ -397,7 +397,7 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
>   	int val;
>   
>   	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
> -	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> +	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
>   		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
>   		if (val < 0)
>   			return val;
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index a483d79cfc87..1c948bbf4fa0 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -707,9 +707,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
>   		return -EIO;
>   	*devices_in_package |= phy_reg;
>   
> -	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> -	*devices_in_package &= ~BIT(0);
> -
>   	return 0;
>   }
>   
> @@ -788,6 +785,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>   	}
>   
>   	c45_ids->devices_in_package = devs_in_pkg;
> +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> +	c45_ids->mmds_present = devs_in_pkg & ~BIT(0);
>   
>   	*phy_id = 0;
>   	return 0;
> @@ -842,6 +841,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
>   	int r;
>   
>   	c45_ids.devices_in_package = 0;
> +	c45_ids.mmds_present = 0;
>   	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
>   
>   	if (is_c45)
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 6defd5eddd58..b548e0418694 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1709,11 +1709,11 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
>   		case MII_BMSR:
>   		case MII_PHYSID1:
>   		case MII_PHYSID2:
> -			devad = __ffs(phydev->c45_ids.devices_in_package);
> +			devad = __ffs(phydev->c45_ids.mmds_present);
>   			break;
>   		case MII_ADVERTISE:
>   		case MII_LPA:
> -			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
> +			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
>   				return -EINVAL;
>   			devad = MDIO_MMD_AN;
>   			if (reg == MII_ADVERTISE)
> @@ -1749,11 +1749,11 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
>   		case MII_BMSR:
>   		case MII_PHYSID1:
>   		case MII_PHYSID2:
> -			devad = __ffs(phydev->c45_ids.devices_in_package);
> +			devad = __ffs(phydev->c45_ids.mmds_present);
>   			break;
>   		case MII_ADVERTISE:
>   		case MII_LPA:
> -			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
> +			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
>   				return -EINVAL;
>   			devad = MDIO_MMD_AN;
>   			if (reg == MII_ADVERTISE)
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 41c046545354..0d41c710339a 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -354,11 +354,13 @@ enum phy_state {
>   
>   /**
>    * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
> - * @devices_in_package: Bit vector of devices present.
> + * @devices_in_package: IEEE 802.3 devices in package register value.
> + * @mmds_present: bit vector of MMDs present.
>    * @device_ids: The device identifer for each present device.
>    */
>   struct phy_c45_device_ids {
>   	u32 devices_in_package;
> +	u32 mmds_present;
>   	u32 device_ids[8];
>   };

It seems like the majority of the devices_in_package accessors are just 
bit masking for a given MMD/field. The case that has the problem is the 
__ffs() calls which failed to account for this. So why not just fix 
those two cases instead of creating a duplicate field with exactly the 
same data in it minus a bit.

Re: [PATCH RFC 5/7] net: phy: set devices_in_package only after validation

[Jeremy Linton]

Hi,

On 5/26/20 9:31 AM, Russell King wrote:
> Only set the devices_in_package to a non-zero value if we find a valid
> value for this field, so we avoid leaving it set to e.g. 0x1fffffff.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>   drivers/net/phy/phy_device.c | 17 ++++++++++-------
>   1 file changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index fa9164ac0f3d..a483d79cfc87 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -730,13 +730,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>   			   struct phy_c45_device_ids *c45_ids)
>   {
>   	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> -	u32 *devs = &c45_ids->devices_in_package;
> +	u32 devs_in_pkg = 0;
>   	int i, ret, phy_reg;
>   
>   	/* Find first non-zero Devices In package. Device zero is reserved
>   	 * for 802.3 c45 complied PHYs, so don't probe it at first.
>   	 */
> -	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> +	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0; i++) {
>   		if (i >= 8) {
>   			/* Only probe for the devices-in-package if there
>   			 * is a PHY reporting as present here; this avoids
> @@ -750,22 +750,22 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>   			if (!ret)
>   				continue;
>   		}
> -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
> +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
>   		if (phy_reg < 0)
>   			return -EIO;
>   	}
>   
> -	if ((*devs & 0x1fffffff) == 0x1fffffff) {
> +	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
>   		/* If mostly Fs, there is no device there, then let's probe
>   		 * MMD 0, as some 10G PHYs have zero Devices In package,
>   		 * e.g. Cortina CS4315/CS4340 PHY.
>   		 */
> -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
> +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
>   		if (phy_reg < 0)
>   			return -EIO;
>   
>   		/* no device there, let's get out of here */
> -		if ((*devs & 0x1fffffff) == 0x1fffffff) {
> +		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
>   			*phy_id = 0xffffffff;
>   			return 0;
>   		}
> @@ -773,7 +773,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>   
>   	/* Now probe Device Identifiers for each device present. */
>   	for (i = 1; i < num_ids; i++) {
> -		if (!(c45_ids->devices_in_package & (1 << i)))
> +		if (!(devs_in_pkg & (1 << i)))
>   			continue;
>   
>   		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
> @@ -786,6 +786,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>   			return -EIO;
>   		c45_ids->device_ids[i] |= phy_reg;
>   	}
> +
> +	c45_ids->devices_in_package = devs_in_pkg;
> +
>   	*phy_id = 0;
>   	return 0;
>   }
> 

You have solved the case of 0xFFFFFFFFF devices in package, but It looks 
like the case of devs in package = 0  still gets a successful return 
from this path. Which can still create phys with phy_id=0 and 0 MMDs AFAIK.

Re: [PATCH RFC 0/7] Clause 45 PHY probing cleanups

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 03:29:48PM +0100, Russell King - ARM Linux admin wrote:
> Hi,
> 
> In response to the patch set that Jeremy posted, this is my proposal
> to expand our Clause 45 PHY probing.
> 
> I've taken a slightly different approach, with the view to avoiding
> as much behavioural change as possible.  The biggest difference is
> to do with "devices_in_package" - we were using it for two different
> purposes, which are now separated.
> 
> This is not against net-next nor net trees, but against my own private
> tree, but I'm posting it to serve as an illustration of what I think
> should be done - I knocked this up this morning.
> 
> The only potential regression that I'm expecting is with 88x3310 PHYs
> of the later revision, which have the clause 22 registers implemented.
> I haven't yet checked whether they set bit 0, but if they do, the
> various decision points that we have based on that bit could adversely
> affect this PHY - it needs testing, which I'll do when I dig out the
> appropriate hardware.  Probably also needs the 2110 PHYs checked as
> well.

Tested on the later revision of the 88x3310 PHY with some additional
prints:

orion-mdio f212a600.mdio: scanning prt 0 mmd 1...
orion-mdio f212a600.mdio: prt 0: dip=c000009a
orion-mdio f212a600.mdio: prt 0 mmd 1: id 0x002b09ab
orion-mdio f212a600.mdio: prt 0 mmd 3: id 0x002b09ab
orion-mdio f212a600.mdio: prt 0 mmd 4: id 0x01410dab
orion-mdio f212a600.mdio: prt 0 mmd 7: id 0x002b09ab
orion-mdio f212a600.mdio: prt 0 mmd 30: prs=0
orion-mdio f212a600.mdio: prt 0 mmd 31: prs=0

orion-mdio f212a600.mdio: scanning prt 8 mmd 1...
orion-mdio f212a600.mdio: prt 8: dip=c000009a
orion-mdio f212a600.mdio: prt 8 mmd 1: id 0x002b09ab
orion-mdio f212a600.mdio: prt 8 mmd 3: id 0x002b09ab
orion-mdio f212a600.mdio: prt 8 mmd 4: id 0x01410dab
orion-mdio f212a600.mdio: prt 8 mmd 7: id 0x002b09ab
orion-mdio f212a600.mdio: prt 8 mmd 30: prs=0
orion-mdio f212a600.mdio: prt 8 mmd 31: prs=0

which is what is expected from this PHY.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 7/7] net: phy: read MMD ID from all present MMDs

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 10:35:46AM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/26/20 9:31 AM, Russell King wrote:
> > Expand the device_ids[] array to allow all MMD IDs to be read rather
> > than just the first 8 MMDs, but only read the ID if the MDIO_STAT2
> > register reports that a device really is present here for these new
> > devices to maintain compatibility with our current behaviour.
> > 
> > 88X3310 PHY vendor MMDs do are marked as present in the
> > devices_in_package, but do not contain IEE 802.3 compatible register
> > sets in their lower space.  This avoids reading incorrect values as MMD
> > identifiers.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >   drivers/net/phy/phy_device.c | 14 ++++++++++++++
> >   include/linux/phy.h          |  2 +-
> >   2 files changed, 15 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 1c948bbf4fa0..92742c7be80f 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -773,6 +773,20 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   		if (!(devs_in_pkg & (1 << i)))
> >   			continue;
> > +		if (i >= 8) {
> > +			/* Only probe the MMD ID for MMDs >= 8 if they report
> > +			 * that they are present. We have at least one PHY that
> > +			 * reports MMD presence in devs_in_pkg, but does not
> > +			 * contain valid IEEE 802.3 ID registers in some MMDs.
> > +			 */
> > +			ret = phy_c45_probe_present(bus, addr, i);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			if (!ret)
> > +				continue;
> > +		}
> > +
> >   		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
> >   		if (phy_reg < 0)
> >   			return -EIO;
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 0d41c710339a..3325dd8fb9ac 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -361,7 +361,7 @@ enum phy_state {
> >   struct phy_c45_device_ids {
> >   	u32 devices_in_package;
> >   	u32 mmds_present;
> > -	u32 device_ids[8];
> > +	u32 device_ids[MDIO_MMD_NUM];
> 
> You have a array overflow/invalid access if you don't do this earlier in
> 4/7.

I'm very sorry, but you are mistaken - there is no overflow.

The overflow would happen if I'd changed the _second_ loop in
get_phy_c45_ids(), but that still relies upon the size of this
array.  In fact, everywhere that the device_ids array is indexed
with a for() loop, the maximum bound is defined by the element
size of the array.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 3/7] net: phy: clean up PHY ID reading

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 10:38:31AM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/26/20 9:31 AM, Russell King wrote:
> > Rearrange the code to read the PHY IDs, so we don't call get_phy_id()
> > only to immediately call get_phy_c45_ids().  Move that logic into
> > get_phy_device(), which results in better readability.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >   drivers/net/phy/phy_device.c | 25 +++++++++----------------
> >   1 file changed, 9 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index e04284c4ebf8..0d6b6ca66216 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -756,29 +756,18 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   }
> >   /**
> > - * get_phy_id - reads the specified addr for its ID.
> > + * get_phy_c22_id - reads the specified addr for its clause 22 ID.
> >    * @bus: the target MII bus
> >    * @addr: PHY address on the MII bus
> >    * @phy_id: where to store the ID retrieved.
> > - * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
> > - * @c45_ids: where to store the c45 ID information.
> > - *
> > - * Description: In the case of a 802.3-c22 PHY, reads the ID registers
> > - *   of the PHY at @addr on the @bus, stores it in @phy_id and returns
> > - *   zero on success.
> > - *
> > - *   In the case of a 802.3-c45 PHY, get_phy_c45_ids() is invoked, and
> > - *   its return value is in turn returned.
> >    *
> > + * Read the 802.3 clause 22 PHY ID from the PHY at @addr on the @bus.
> > + * Return the PHY ID read from the PHY in @phy_id on successful access.
> >    */
> > -static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
> > -		      bool is_c45, struct phy_c45_device_ids *c45_ids)
> > +static int get_phy_c22_id(struct mii_bus *bus, int addr, u32 *phy_id)
> >   {
> >   	int phy_reg;
> > -	if (is_c45)
> > -		return get_phy_c45_ids(bus, addr, phy_id, c45_ids);
> > -
> >   	/* Grab the bits from PHYIR1, and put them in the upper half */
> >   	phy_reg = mdiobus_read(bus, addr, MII_PHYSID1);
> >   	if (phy_reg < 0) {
> > @@ -817,7 +806,11 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
> >   	c45_ids.devices_in_package = 0;
> >   	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
> > -	r = get_phy_id(bus, addr, &phy_id, is_c45, &c45_ids);
> > +	if (is_c45)
> > +		r = get_phy_c45_ids(bus, addr, &phy_id, &c45_ids);
> > +	else
> > +		r = get_phy_c22_id(bus, addr, &phy_id);
> > +
> >   	if (r)
> >   		return ERR_PTR(r);
> > 
> 
> I see this, and the c22 regs detection, but I don't see how your choosing to
> use the c22 regs if the 45's aren't responding. Which was one of the primary
> purposes of that other set.

You are entirely correct, but I am not aiming for that in this series.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 6/7] net: phy: split devices_in_package

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 10:39:08AM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/26/20 9:31 AM, Russell King wrote:
> > We have two competing requirements for the devices_in_package field.
> > We want to use it as a bit array indicating which MMDs are present, but
> > we also want to know if the Clause 22 registers are present.
> > 
> > Since "devices in package" is a term used in the 802.3 specification,
> > keep this as the as-specified values read from the PHY, and introduce
> > a new member "mmds_present" to indicate which MMDs are actually
> > present in the PHY, derived from the "devices in package" value.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >   drivers/net/phy/phy-c45.c    | 4 ++--
> >   drivers/net/phy/phy_device.c | 6 +++---
> >   drivers/net/phy/phylink.c    | 8 ++++----
> >   include/linux/phy.h          | 4 +++-
> >   4 files changed, 12 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > index 8cd952950a75..4b5805e2bd0e 100644
> > --- a/drivers/net/phy/phy-c45.c
> > +++ b/drivers/net/phy/phy-c45.c
> > @@ -219,7 +219,7 @@ int genphy_c45_read_link(struct phy_device *phydev)
> >   	int val, devad;
> >   	bool link = true;
> > -	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> > +	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
> >   		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
> >   		if (val < 0)
> >   			return val;
> > @@ -397,7 +397,7 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
> >   	int val;
> >   	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
> > -	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> > +	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
> >   		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> >   		if (val < 0)
> >   			return val;
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index a483d79cfc87..1c948bbf4fa0 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -707,9 +707,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
> >   		return -EIO;
> >   	*devices_in_package |= phy_reg;
> > -	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > -	*devices_in_package &= ~BIT(0);
> > -
> >   	return 0;
> >   }
> > @@ -788,6 +785,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   	}
> >   	c45_ids->devices_in_package = devs_in_pkg;
> > +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > +	c45_ids->mmds_present = devs_in_pkg & ~BIT(0);
> >   	*phy_id = 0;
> >   	return 0;
> > @@ -842,6 +841,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
> >   	int r;
> >   	c45_ids.devices_in_package = 0;
> > +	c45_ids.mmds_present = 0;
> >   	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
> >   	if (is_c45)
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 6defd5eddd58..b548e0418694 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -1709,11 +1709,11 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
> >   		case MII_BMSR:
> >   		case MII_PHYSID1:
> >   		case MII_PHYSID2:
> > -			devad = __ffs(phydev->c45_ids.devices_in_package);
> > +			devad = __ffs(phydev->c45_ids.mmds_present);
> >   			break;
> >   		case MII_ADVERTISE:
> >   		case MII_LPA:
> > -			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
> > +			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
> >   				return -EINVAL;
> >   			devad = MDIO_MMD_AN;
> >   			if (reg == MII_ADVERTISE)
> > @@ -1749,11 +1749,11 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
> >   		case MII_BMSR:
> >   		case MII_PHYSID1:
> >   		case MII_PHYSID2:
> > -			devad = __ffs(phydev->c45_ids.devices_in_package);
> > +			devad = __ffs(phydev->c45_ids.mmds_present);
> >   			break;
> >   		case MII_ADVERTISE:
> >   		case MII_LPA:
> > -			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
> > +			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
> >   				return -EINVAL;
> >   			devad = MDIO_MMD_AN;
> >   			if (reg == MII_ADVERTISE)
> > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > index 41c046545354..0d41c710339a 100644
> > --- a/include/linux/phy.h
> > +++ b/include/linux/phy.h
> > @@ -354,11 +354,13 @@ enum phy_state {
> >   /**
> >    * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
> > - * @devices_in_package: Bit vector of devices present.
> > + * @devices_in_package: IEEE 802.3 devices in package register value.
> > + * @mmds_present: bit vector of MMDs present.
> >    * @device_ids: The device identifer for each present device.
> >    */
> >   struct phy_c45_device_ids {
> >   	u32 devices_in_package;
> > +	u32 mmds_present;
> >   	u32 device_ids[8];
> >   };
> 
> It seems like the majority of the devices_in_package accessors are just bit
> masking for a given MMD/field. The case that has the problem is the __ffs()
> calls which failed to account for this. So why not just fix those two cases
> instead of creating a duplicate field with exactly the same data in it minus
> a bit.

I think I explained that in the commit message...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 5/7] net: phy: set devices_in_package only after validation

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 10:39:49AM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/26/20 9:31 AM, Russell King wrote:
> > Only set the devices_in_package to a non-zero value if we find a valid
> > value for this field, so we avoid leaving it set to e.g. 0x1fffffff.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >   drivers/net/phy/phy_device.c | 17 ++++++++++-------
> >   1 file changed, 10 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index fa9164ac0f3d..a483d79cfc87 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -730,13 +730,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   			   struct phy_c45_device_ids *c45_ids)
> >   {
> >   	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> > -	u32 *devs = &c45_ids->devices_in_package;
> > +	u32 devs_in_pkg = 0;
> >   	int i, ret, phy_reg;
> >   	/* Find first non-zero Devices In package. Device zero is reserved
> >   	 * for 802.3 c45 complied PHYs, so don't probe it at first.
> >   	 */
> > -	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> > +	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0; i++) {
> >   		if (i >= 8) {
> >   			/* Only probe for the devices-in-package if there
> >   			 * is a PHY reporting as present here; this avoids
> > @@ -750,22 +750,22 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   			if (!ret)
> >   				continue;
> >   		}
> > -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
> > +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
> >   		if (phy_reg < 0)
> >   			return -EIO;
> >   	}
> > -	if ((*devs & 0x1fffffff) == 0x1fffffff) {
> > +	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
> >   		/* If mostly Fs, there is no device there, then let's probe
> >   		 * MMD 0, as some 10G PHYs have zero Devices In package,
> >   		 * e.g. Cortina CS4315/CS4340 PHY.
> >   		 */
> > -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
> > +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
> >   		if (phy_reg < 0)
> >   			return -EIO;
> >   		/* no device there, let's get out of here */
> > -		if ((*devs & 0x1fffffff) == 0x1fffffff) {
> > +		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
> >   			*phy_id = 0xffffffff;
> >   			return 0;
> >   		}
> > @@ -773,7 +773,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   	/* Now probe Device Identifiers for each device present. */
> >   	for (i = 1; i < num_ids; i++) {
> > -		if (!(c45_ids->devices_in_package & (1 << i)))
> > +		if (!(devs_in_pkg & (1 << i)))
> >   			continue;
> >   		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
> > @@ -786,6 +786,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> >   			return -EIO;
> >   		c45_ids->device_ids[i] |= phy_reg;
> >   	}
> > +
> > +	c45_ids->devices_in_package = devs_in_pkg;
> > +
> >   	*phy_id = 0;
> >   	return 0;
> >   }
> > 
> 
> You have solved the case of 0xFFFFFFFFF devices in package, but It looks
> like the case of devs in package = 0  still gets a successful return from
> this path. Which can still create phys with phy_id=0 and 0 MMDs AFAIK.

Correct - I'm not looking to change the behaviour in this patch
beyond the intended change of ensuring that c45_ids->devices_in_package
remains untouched until we intend to return successfully.

The zero MMDs issue is an entirely orthogonal problem not addressed
in this series.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 6/7] net: phy: split devices_in_package

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 04:47:54PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 26, 2020 at 10:39:08AM -0500, Jeremy Linton wrote:
> > Hi,
> > 
> > On 5/26/20 9:31 AM, Russell King wrote:
> > > We have two competing requirements for the devices_in_package field.
> > > We want to use it as a bit array indicating which MMDs are present, but
> > > we also want to know if the Clause 22 registers are present.
> > > 
> > > Since "devices in package" is a term used in the 802.3 specification,
> > > keep this as the as-specified values read from the PHY, and introduce
> > > a new member "mmds_present" to indicate which MMDs are actually
> > > present in the PHY, derived from the "devices in package" value.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >   drivers/net/phy/phy-c45.c    | 4 ++--
> > >   drivers/net/phy/phy_device.c | 6 +++---
> > >   drivers/net/phy/phylink.c    | 8 ++++----
> > >   include/linux/phy.h          | 4 +++-
> > >   4 files changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> > > index 8cd952950a75..4b5805e2bd0e 100644
> > > --- a/drivers/net/phy/phy-c45.c
> > > +++ b/drivers/net/phy/phy-c45.c
> > > @@ -219,7 +219,7 @@ int genphy_c45_read_link(struct phy_device *phydev)
> > >   	int val, devad;
> > >   	bool link = true;
> > > -	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> > > +	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
> > >   		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
> > >   		if (val < 0)
> > >   			return val;
> > > @@ -397,7 +397,7 @@ int genphy_c45_pma_read_abilities(struct phy_device *phydev)
> > >   	int val;
> > >   	linkmode_clear_bit(ETHTOOL_LINK_MODE_Autoneg_BIT, phydev->supported);
> > > -	if (phydev->c45_ids.devices_in_package & MDIO_DEVS_AN) {
> > > +	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
> > >   		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_STAT1);
> > >   		if (val < 0)
> > >   			return val;
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index a483d79cfc87..1c948bbf4fa0 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -707,9 +707,6 @@ static int get_phy_c45_devs_in_pkg(struct mii_bus *bus, int addr, int dev_addr,
> > >   		return -EIO;
> > >   	*devices_in_package |= phy_reg;
> > > -	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > > -	*devices_in_package &= ~BIT(0);
> > > -
> > >   	return 0;
> > >   }
> > > @@ -788,6 +785,8 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   	}
> > >   	c45_ids->devices_in_package = devs_in_pkg;
> > > +	/* Bit 0 doesn't represent a device, it indicates c22 regs presence */
> > > +	c45_ids->mmds_present = devs_in_pkg & ~BIT(0);
> > >   	*phy_id = 0;
> > >   	return 0;
> > > @@ -842,6 +841,7 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
> > >   	int r;
> > >   	c45_ids.devices_in_package = 0;
> > > +	c45_ids.mmds_present = 0;
> > >   	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
> > >   	if (is_c45)
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 6defd5eddd58..b548e0418694 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -1709,11 +1709,11 @@ static int phylink_phy_read(struct phylink *pl, unsigned int phy_id,
> > >   		case MII_BMSR:
> > >   		case MII_PHYSID1:
> > >   		case MII_PHYSID2:
> > > -			devad = __ffs(phydev->c45_ids.devices_in_package);
> > > +			devad = __ffs(phydev->c45_ids.mmds_present);
> > >   			break;
> > >   		case MII_ADVERTISE:
> > >   		case MII_LPA:
> > > -			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
> > > +			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
> > >   				return -EINVAL;
> > >   			devad = MDIO_MMD_AN;
> > >   			if (reg == MII_ADVERTISE)
> > > @@ -1749,11 +1749,11 @@ static int phylink_phy_write(struct phylink *pl, unsigned int phy_id,
> > >   		case MII_BMSR:
> > >   		case MII_PHYSID1:
> > >   		case MII_PHYSID2:
> > > -			devad = __ffs(phydev->c45_ids.devices_in_package);
> > > +			devad = __ffs(phydev->c45_ids.mmds_present);
> > >   			break;
> > >   		case MII_ADVERTISE:
> > >   		case MII_LPA:
> > > -			if (!(phydev->c45_ids.devices_in_package & MDIO_DEVS_AN))
> > > +			if (!(phydev->c45_ids.mmds_present & MDIO_DEVS_AN))
> > >   				return -EINVAL;
> > >   			devad = MDIO_MMD_AN;
> > >   			if (reg == MII_ADVERTISE)
> > > diff --git a/include/linux/phy.h b/include/linux/phy.h
> > > index 41c046545354..0d41c710339a 100644
> > > --- a/include/linux/phy.h
> > > +++ b/include/linux/phy.h
> > > @@ -354,11 +354,13 @@ enum phy_state {
> > >   /**
> > >    * struct phy_c45_device_ids - 802.3-c45 Device Identifiers
> > > - * @devices_in_package: Bit vector of devices present.
> > > + * @devices_in_package: IEEE 802.3 devices in package register value.
> > > + * @mmds_present: bit vector of MMDs present.
> > >    * @device_ids: The device identifer for each present device.
> > >    */
> > >   struct phy_c45_device_ids {
> > >   	u32 devices_in_package;
> > > +	u32 mmds_present;
> > >   	u32 device_ids[8];
> > >   };
> > 
> > It seems like the majority of the devices_in_package accessors are just bit
> > masking for a given MMD/field. The case that has the problem is the __ffs()
> > calls which failed to account for this. So why not just fix those two cases
> > instead of creating a duplicate field with exactly the same data in it minus
> > a bit.
> 
> I think I explained that in the commit message...

I should also point out that we may wish to clear bits in mmds_present
when we scan the IDs and discover STAT2 registers that indicate that
the MMD is not implemented, while leaving the devices_in_package
field unaltered.

To do that would be a behavioural change, which is something that
this series is trying to avoid.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 5/7] net: phy: set devices_in_package only after validation

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 04:50:28PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, May 26, 2020 at 10:39:49AM -0500, Jeremy Linton wrote:
> > Hi,
> > 
> > On 5/26/20 9:31 AM, Russell King wrote:
> > > Only set the devices_in_package to a non-zero value if we find a valid
> > > value for this field, so we avoid leaving it set to e.g. 0x1fffffff.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >   drivers/net/phy/phy_device.c | 17 ++++++++++-------
> > >   1 file changed, 10 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index fa9164ac0f3d..a483d79cfc87 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -730,13 +730,13 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   			   struct phy_c45_device_ids *c45_ids)
> > >   {
> > >   	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> > > -	u32 *devs = &c45_ids->devices_in_package;
> > > +	u32 devs_in_pkg = 0;
> > >   	int i, ret, phy_reg;
> > >   	/* Find first non-zero Devices In package. Device zero is reserved
> > >   	 * for 802.3 c45 complied PHYs, so don't probe it at first.
> > >   	 */
> > > -	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> > > +	for (i = 1; i < MDIO_MMD_NUM && devs_in_pkg == 0; i++) {
> > >   		if (i >= 8) {
> > >   			/* Only probe for the devices-in-package if there
> > >   			 * is a PHY reporting as present here; this avoids
> > > @@ -750,22 +750,22 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   			if (!ret)
> > >   				continue;
> > >   		}
> > > -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, devs);
> > > +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, i, &devs_in_pkg);
> > >   		if (phy_reg < 0)
> > >   			return -EIO;
> > >   	}
> > > -	if ((*devs & 0x1fffffff) == 0x1fffffff) {
> > > +	if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
> > >   		/* If mostly Fs, there is no device there, then let's probe
> > >   		 * MMD 0, as some 10G PHYs have zero Devices In package,
> > >   		 * e.g. Cortina CS4315/CS4340 PHY.
> > >   		 */
> > > -		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, devs);
> > > +		phy_reg = get_phy_c45_devs_in_pkg(bus, addr, 0, &devs_in_pkg);
> > >   		if (phy_reg < 0)
> > >   			return -EIO;
> > >   		/* no device there, let's get out of here */
> > > -		if ((*devs & 0x1fffffff) == 0x1fffffff) {
> > > +		if ((devs_in_pkg & 0x1fffffff) == 0x1fffffff) {
> > >   			*phy_id = 0xffffffff;
> > >   			return 0;
> > >   		}
> > > @@ -773,7 +773,7 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   	/* Now probe Device Identifiers for each device present. */
> > >   	for (i = 1; i < num_ids; i++) {
> > > -		if (!(c45_ids->devices_in_package & (1 << i)))
> > > +		if (!(devs_in_pkg & (1 << i)))
> > >   			continue;
> > >   		phy_reg = mdiobus_c45_read(bus, addr, i, MII_PHYSID1);
> > > @@ -786,6 +786,9 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   			return -EIO;
> > >   		c45_ids->device_ids[i] |= phy_reg;
> > >   	}
> > > +
> > > +	c45_ids->devices_in_package = devs_in_pkg;
> > > +
> > >   	*phy_id = 0;
> > >   	return 0;
> > >   }
> > > 
> > 
> > You have solved the case of 0xFFFFFFFFF devices in package, but It looks
> > like the case of devs in package = 0  still gets a successful return from
> > this path. Which can still create phys with phy_id=0 and 0 MMDs AFAIK.
> 
> Correct - I'm not looking to change the behaviour in this patch
> beyond the intended change of ensuring that c45_ids->devices_in_package
> remains untouched until we intend to return successfully.
> 
> The zero MMDs issue is an entirely orthogonal problem not addressed
> in this series.

Please note that this problem has existed back to the original addition
of clause 45 support in this commit:

commit ac28b9f8cd66d6bc54f8063df59e99abd62173a4
Author: David Daney <david.daney@cavium.com>
Date:   Wed Jun 27 07:33:35 2012 +0000

I think it only becomes a problem when we start autoprobing clause 45
PHYs.

The scanning in __mdiobus_register() does not support locating a
clause 45 PHY - it only issues clause 22 cycles.  If you are lucky
enough to have a clause 45 PHY that responds to clause 22 cycles,
then the PHY device will be created using the information in the
clause 22 registers.  get_phy_device() is called with is_c45 false.
Hence, the path above will not be used.

The only way for a clause 45 PHY to be created is explicitly via
some method - either through DT declaring a C45 PHY, or via a driver
calling get_phy_device() with is_c45 true.  There are two ethernet
drivers which do this in drivers/net, using explicit addresses
rather than autoprobing.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 4/7] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 03:31:16PM +0100, Russell King wrote:
> Add support for probing MMDs above 7 for a valid devices-in-package
> specifier.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++--
>  include/linux/phy.h          |  2 ++
>  2 files changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 0d6b6ca66216..fa9164ac0f3d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>  }
>  EXPORT_SYMBOL(phy_device_create);
>  
> +/* phy_c45_probe_present - checks to see if a MMD is present in the package
> + * @bus: the target MII bus
> + * @prtad: PHY package address on the MII bus
> + * @devad: PHY device (MMD) address
> + *
> + * Read the MDIO_STAT2 register, and check whether a device is responding
> + * at this address.
> + *
> + * Returns: negative error number on bus access error, zero if no device
> + * is responding, or positive if a device is present.
> + */
> +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
> +{
> +	int stat2;
> +
> +	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
> +	if (stat2 < 0)
> +		return stat2;
> +
> +	return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL;
> +}
> +
>  /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
>   * @bus: the target MII bus
>   * @addr: PHY address on the MII bus
> @@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>  {
>  	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
>  	u32 *devs = &c45_ids->devices_in_package;
> -	int i, phy_reg;
> +	int i, ret, phy_reg;
>  
>  	/* Find first non-zero Devices In package. Device zero is reserved
>  	 * for 802.3 c45 complied PHYs, so don't probe it at first.
>  	 */
> -	for (i = 1; i < num_ids && *devs == 0; i++) {
> +	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> +		if (i >= 8) {
> +			/* Only probe for the devices-in-package if there
> +			 * is a PHY reporting as present here; this avoids
> +			 * picking up on PHYs that implement non-IEEE802.3
> +			 * compliant register spaces.
> +			 */
> +			ret = phy_c45_probe_present(bus, addr, i);
> +			if (ret < 0)
> +				return -EIO;
> +
> +			if (!ret)
> +				continue;
> +		}

A second look at 802.3, this can't be done for all MMDs (which becomes
visible when I look at the results from the 88x3310.)  Only MMDs 1, 2,
3, 4, 5, 30 and 31 are defined to have this register with the "Device
Present" bit pair.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up

Re: [PATCH RFC 4/7] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Jeremy Linton]

Hi,

On 5/26/20 12:14 PM, Russell King - ARM Linux admin wrote:
> On Tue, May 26, 2020 at 03:31:16PM +0100, Russell King wrote:
>> Add support for probing MMDs above 7 for a valid devices-in-package
>> specifier.
>>
>> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
>> ---
>>   drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++--
>>   include/linux/phy.h          |  2 ++
>>   2 files changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 0d6b6ca66216..fa9164ac0f3d 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>>   }
>>   EXPORT_SYMBOL(phy_device_create);
>>   
>> +/* phy_c45_probe_present - checks to see if a MMD is present in the package
>> + * @bus: the target MII bus
>> + * @prtad: PHY package address on the MII bus
>> + * @devad: PHY device (MMD) address
>> + *
>> + * Read the MDIO_STAT2 register, and check whether a device is responding
>> + * at this address.
>> + *
>> + * Returns: negative error number on bus access error, zero if no device
>> + * is responding, or positive if a device is present.
>> + */
>> +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
>> +{
>> +	int stat2;
>> +
>> +	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
>> +	if (stat2 < 0)
>> +		return stat2;
>> +
>> +	return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL;
>> +}
>> +
>>   /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
>>    * @bus: the target MII bus
>>    * @addr: PHY address on the MII bus
>> @@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
>>   {
>>   	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
>>   	u32 *devs = &c45_ids->devices_in_package;
>> -	int i, phy_reg;
>> +	int i, ret, phy_reg;
>>   
>>   	/* Find first non-zero Devices In package. Device zero is reserved
>>   	 * for 802.3 c45 complied PHYs, so don't probe it at first.
>>   	 */
>> -	for (i = 1; i < num_ids && *devs == 0; i++) {
>> +	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
>> +		if (i >= 8) {
>> +			/* Only probe for the devices-in-package if there
>> +			 * is a PHY reporting as present here; this avoids
>> +			 * picking up on PHYs that implement non-IEEE802.3
>> +			 * compliant register spaces.
>> +			 */
>> +			ret = phy_c45_probe_present(bus, addr, i);
>> +			if (ret < 0)
>> +				return -EIO;
>> +
>> +			if (!ret)
>> +				continue;
>> +		}
> 
> A second look at 802.3, this can't be done for all MMDs (which becomes
> visible when I look at the results from the 88x3310.)  Only MMDs 1, 2,
> 3, 4, 5, 30 and 31 are defined to have this register with the "Device
> Present" bit pair.
> 

I'm not sure it helps, but my thought process following some of the 
discussion last night was:

something to the effect:

	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
+		if (i & RESERVED_MMDS)
+			continue;

where RESERVED_MMDS was a hardcoded bitfield matching the IEEE reserved 
MMDs. or maybe a "IGNORE_MMDS" which also includes BIT0 and other MMDs 
the code doesn't understand.

Re: [PATCH RFC 4/7] net: phy: add support for probing MMDs >= 8 for devices-in-package

[Russell King - ARM Linux admin]

On Tue, May 26, 2020 at 12:20:10PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/26/20 12:14 PM, Russell King - ARM Linux admin wrote:
> > On Tue, May 26, 2020 at 03:31:16PM +0100, Russell King wrote:
> > > Add support for probing MMDs above 7 for a valid devices-in-package
> > > specifier.
> > > 
> > > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > > ---
> > >   drivers/net/phy/phy_device.c | 39 ++++++++++++++++++++++++++++++++++--
> > >   include/linux/phy.h          |  2 ++
> > >   2 files changed, 39 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > > index 0d6b6ca66216..fa9164ac0f3d 100644
> > > --- a/drivers/net/phy/phy_device.c
> > > +++ b/drivers/net/phy/phy_device.c
> > > @@ -659,6 +659,28 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
> > >   }
> > >   EXPORT_SYMBOL(phy_device_create);
> > > +/* phy_c45_probe_present - checks to see if a MMD is present in the package
> > > + * @bus: the target MII bus
> > > + * @prtad: PHY package address on the MII bus
> > > + * @devad: PHY device (MMD) address
> > > + *
> > > + * Read the MDIO_STAT2 register, and check whether a device is responding
> > > + * at this address.
> > > + *
> > > + * Returns: negative error number on bus access error, zero if no device
> > > + * is responding, or positive if a device is present.
> > > + */
> > > +static int phy_c45_probe_present(struct mii_bus *bus, int prtad, int devad)
> > > +{
> > > +	int stat2;
> > > +
> > > +	stat2 = mdiobus_c45_read(bus, prtad, devad, MDIO_STAT2);
> > > +	if (stat2 < 0)
> > > +		return stat2;
> > > +
> > > +	return (stat2 & MDIO_STAT2_DEVPRST) == MDIO_STAT2_DEVPRST_VAL;
> > > +}
> > > +
> > >   /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
> > >    * @bus: the target MII bus
> > >    * @addr: PHY address on the MII bus
> > > @@ -709,12 +731,25 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
> > >   {
> > >   	const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
> > >   	u32 *devs = &c45_ids->devices_in_package;
> > > -	int i, phy_reg;
> > > +	int i, ret, phy_reg;
> > >   	/* Find first non-zero Devices In package. Device zero is reserved
> > >   	 * for 802.3 c45 complied PHYs, so don't probe it at first.
> > >   	 */
> > > -	for (i = 1; i < num_ids && *devs == 0; i++) {
> > > +	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> > > +		if (i >= 8) {
> > > +			/* Only probe for the devices-in-package if there
> > > +			 * is a PHY reporting as present here; this avoids
> > > +			 * picking up on PHYs that implement non-IEEE802.3
> > > +			 * compliant register spaces.
> > > +			 */
> > > +			ret = phy_c45_probe_present(bus, addr, i);
> > > +			if (ret < 0)
> > > +				return -EIO;
> > > +
> > > +			if (!ret)
> > > +				continue;
> > > +		}
> > 
> > A second look at 802.3, this can't be done for all MMDs (which becomes
> > visible when I look at the results from the 88x3310.)  Only MMDs 1, 2,
> > 3, 4, 5, 30 and 31 are defined to have this register with the "Device
> > Present" bit pair.
> > 
> 
> I'm not sure it helps, but my thought process following some of the
> discussion last night was:
> 
> something to the effect:
> 
> 	for (i = 1; i < MDIO_MMD_NUM && *devs == 0; i++) {
> +		if (i & RESERVED_MMDS)
> +			continue;
> 
> where RESERVED_MMDS was a hardcoded bitfield matching the IEEE reserved
> MMDs. or maybe a "IGNORE_MMDS" which also includes BIT0 and other MMDs the
> code doesn't understand.

That seems to be walking into a mine field - which MMDs are reserved
depends on which revision of the IEEE 802.3 specification you look at:

Spec version	Reserved MMDs
2012, 2015	0, 12-28
2018		0, 14-28

and as technology progresses, it's likely more MMDs will no longer be
reserved.

There is another problem: 802.3 explicitly defines the devices in
package registers for MMD 1, 2, 3, 4, 5, 6, 7, and 29, but then goes
on to say:

"Each MMD contains registers 5 and 6, as defined in Table 45-2."

which seems rather contradictory.

There is also the problem that some PHYs have different values
in their devices-in-package register for each MMD:

MMD	devices-in-package
1,3,7	c000009a
4	4000001a
30	00000000 (device present = not present)
31	fffe0000 (device present = not present)

What fun.  Thankfully, MMD1 will be read first, so it doesn't
cause us a problem.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 424kbps up