| Message ID | E1jdac2-0005sv-W5@rmk-PC.armlinux.org.uk |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | Clause 45 PHY probing cleanups | expand |
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.
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...
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.
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]; };
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(-)