| Message ID | 20250218-marvell-88q2xxx-hwmon-enable-at-probe-v1-2-999a304c8a11@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: phy: marvell-88q2xxx: Enable temperature measurement in probe again | expand |
Hi Dimitri, On Tue, Feb 18, 2025 at 07:33:10PM +0100, Dimitri Fedrau wrote: > If the PHYs reset is asserted it returns 0xffff for any read operation. > Prevent reading the temperature in this case and return with an I/O error. > Write operations are ignored by the device. > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > --- > drivers/net/phy/marvell-88q2xxx.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > index 30d71bfc365597d77c34c48f05390db9d63c4af4..c1ae27057ee34feacb31c2e3c40b2b1769596408 100644 > --- a/drivers/net/phy/marvell-88q2xxx.c > +++ b/drivers/net/phy/marvell-88q2xxx.c > @@ -647,6 +647,12 @@ static int mv88q2xxx_hwmon_read(struct device *dev, > struct phy_device *phydev = dev_get_drvdata(dev); > int ret; > > + /* If the PHYs reset is asserted it returns 0xffff for any read > + * operation. Return with an I/O error in this case. > + */ > + if (phydev->mdio.reset_state == 1) > + return -EIO; > + > switch (attr) { > case hwmon_temp_input: > ret = phy_read_mmd(phydev, MDIO_MMD_PCS, > It makes sense to me. However, aren't most phys that allow reading sensors over MDIO affected by this issue? I couldn't find anything similar, are they ignoring that use-case? Regards, Stefan
Hi Stefan, Am Wed, Feb 19, 2025 at 07:29:49AM +0100 schrieb Stefan Eichenberger: > Hi Dimitri, > > On Tue, Feb 18, 2025 at 07:33:10PM +0100, Dimitri Fedrau wrote: > > If the PHYs reset is asserted it returns 0xffff for any read operation. > > Prevent reading the temperature in this case and return with an I/O error. > > Write operations are ignored by the device. > > > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > --- > > drivers/net/phy/marvell-88q2xxx.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > index 30d71bfc365597d77c34c48f05390db9d63c4af4..c1ae27057ee34feacb31c2e3c40b2b1769596408 100644 > > --- a/drivers/net/phy/marvell-88q2xxx.c > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > @@ -647,6 +647,12 @@ static int mv88q2xxx_hwmon_read(struct device *dev, > > struct phy_device *phydev = dev_get_drvdata(dev); > > int ret; > > > > + /* If the PHYs reset is asserted it returns 0xffff for any read > > + * operation. Return with an I/O error in this case. > > + */ > > + if (phydev->mdio.reset_state == 1) > > + return -EIO; > > + > > switch (attr) { > > case hwmon_temp_input: > > ret = phy_read_mmd(phydev, MDIO_MMD_PCS, > > > > It makes sense to me. However, aren't most phys that allow reading > sensors over MDIO affected by this issue? I couldn't find anything > similar, are they ignoring that use-case? > Yes, you are right, but only if the PHYs hard reset is controlled with "reset-gpios" or similar. I didn't find anything about it too. Best regards, Dimitri Fedrau
On Tue, Feb 18, 2025 at 07:33:10PM +0100, Dimitri Fedrau wrote: > If the PHYs reset is asserted it returns 0xffff for any read operation. > Prevent reading the temperature in this case and return with an I/O error. > Write operations are ignored by the device. I think the commit message could be improved. Explain why the PHY reset would be asserted. You are saying it is because the interface is admin down. That is a concept the user is more likely to understand. > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") Is this really a fix? My personal reason for this change was architecture, it seemed odd to probe the hwmon device in one spot and then enable it later. But is it really broken? Stable rules say: It must either fix a real bug that bothers people or just add a device ID > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > --- > drivers/net/phy/marvell-88q2xxx.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > index 30d71bfc365597d77c34c48f05390db9d63c4af4..c1ae27057ee34feacb31c2e3c40b2b1769596408 100644 > --- a/drivers/net/phy/marvell-88q2xxx.c > +++ b/drivers/net/phy/marvell-88q2xxx.c > @@ -647,6 +647,12 @@ static int mv88q2xxx_hwmon_read(struct device *dev, > struct phy_device *phydev = dev_get_drvdata(dev); > int ret; > > + /* If the PHYs reset is asserted it returns 0xffff for any read > + * operation. Return with an I/O error in this case. > + */ > + if (phydev->mdio.reset_state == 1) > + return -EIO; Maybe ENETDOWN is better? Andrew
Hi Dimitri, On Wed, Feb 19, 2025 at 11:54:58AM +0100, Dimitri Fedrau wrote: > Hi Stefan, > > Am Wed, Feb 19, 2025 at 07:29:49AM +0100 schrieb Stefan Eichenberger: > > Hi Dimitri, > > > > On Tue, Feb 18, 2025 at 07:33:10PM +0100, Dimitri Fedrau wrote: > > > If the PHYs reset is asserted it returns 0xffff for any read operation. > > > Prevent reading the temperature in this case and return with an I/O error. > > > Write operations are ignored by the device. > > > > > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > > --- > > > drivers/net/phy/marvell-88q2xxx.c | 6 ++++++ > > > 1 file changed, 6 insertions(+) > > > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > > index 30d71bfc365597d77c34c48f05390db9d63c4af4..c1ae27057ee34feacb31c2e3c40b2b1769596408 100644 > > > --- a/drivers/net/phy/marvell-88q2xxx.c > > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > > @@ -647,6 +647,12 @@ static int mv88q2xxx_hwmon_read(struct device *dev, > > > struct phy_device *phydev = dev_get_drvdata(dev); > > > int ret; > > > > > > + /* If the PHYs reset is asserted it returns 0xffff for any read > > > + * operation. Return with an I/O error in this case. > > > + */ > > > + if (phydev->mdio.reset_state == 1) > > > + return -EIO; > > > + > > > switch (attr) { > > > case hwmon_temp_input: > > > ret = phy_read_mmd(phydev, MDIO_MMD_PCS, > > > > > > > It makes sense to me. However, aren't most phys that allow reading > > sensors over MDIO affected by this issue? I couldn't find anything > > similar, are they ignoring that use-case? > > > Yes, you are right, but only if the PHYs hard reset is controlled with > "reset-gpios" or similar. I didn't find anything about it too. Okay, I see. Maybe at one point it can be generalized, but for now this sounds good to me. Just address the inputs from Andrew. Regards, Stefan
Hi Andrew, Am Wed, Feb 19, 2025 at 02:21:23PM +0100 schrieb Andrew Lunn: > On Tue, Feb 18, 2025 at 07:33:10PM +0100, Dimitri Fedrau wrote: > > If the PHYs reset is asserted it returns 0xffff for any read operation. > > Prevent reading the temperature in this case and return with an I/O error. > > Write operations are ignored by the device. > > I think the commit message could be improved. Explain why the PHY > reset would be asserted. You are saying it is because the interface is > admin down. That is a concept the user is more likely to understand. > Will improve the commit message. > > Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") > > Is this really a fix? My personal reason for this change was > architecture, it seemed odd to probe the hwmon device in one spot and > then enable it later. But is it really broken? Stable rules say: > > It must either fix a real bug that bothers people or just add a device ID > That's fine for me. I don't think it is something that is really bothering people. Will remove the fixes tag and switch to net-next. Thanks for pointing out. > > Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> > > --- > > drivers/net/phy/marvell-88q2xxx.c | 6 ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c > > index 30d71bfc365597d77c34c48f05390db9d63c4af4..c1ae27057ee34feacb31c2e3c40b2b1769596408 100644 > > --- a/drivers/net/phy/marvell-88q2xxx.c > > +++ b/drivers/net/phy/marvell-88q2xxx.c > > @@ -647,6 +647,12 @@ static int mv88q2xxx_hwmon_read(struct device *dev, > > struct phy_device *phydev = dev_get_drvdata(dev); > > int ret; > > > > + /* If the PHYs reset is asserted it returns 0xffff for any read > > + * operation. Return with an I/O error in this case. > > + */ > > + if (phydev->mdio.reset_state == 1) > > + return -EIO; > > Maybe ENETDOWN is better? > That is way better than EIO, so users could actually know why the sensor doesn't return the temperature. Thanks again. Best regards, Dimitri Fedrau
diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c index 30d71bfc365597d77c34c48f05390db9d63c4af4..c1ae27057ee34feacb31c2e3c40b2b1769596408 100644 --- a/drivers/net/phy/marvell-88q2xxx.c +++ b/drivers/net/phy/marvell-88q2xxx.c @@ -647,6 +647,12 @@ static int mv88q2xxx_hwmon_read(struct device *dev, struct phy_device *phydev = dev_get_drvdata(dev); int ret; + /* If the PHYs reset is asserted it returns 0xffff for any read + * operation. Return with an I/O error in this case. + */ + if (phydev->mdio.reset_state == 1) + return -EIO; + switch (attr) { case hwmon_temp_input: ret = phy_read_mmd(phydev, MDIO_MMD_PCS,
If the PHYs reset is asserted it returns 0xffff for any read operation. Prevent reading the temperature in this case and return with an I/O error. Write operations are ignored by the device. Fixes: a197004cf3c2 ("net: phy: marvell-88q2xxx: Fix temperature measurement with reset-gpios") Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com> --- drivers/net/phy/marvell-88q2xxx.c | 6 ++++++ 1 file changed, 6 insertions(+)