1. Patchew
  2. linux
  3. [PATCH 0/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again
  4. View patch

[PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset

Dimitri Fedrau posted 2 patches 10 months ago
There is a newer version of this series
[PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset
Posted by Dimitri Fedrau 10 months ago 
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,

-- 
2.39.5
Re: [PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset
Posted by Andrew Lunn 10 months ago 
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
Re: [PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset
Posted by Dimitri Fedrau 10 months ago 
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
Re: [PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset
Posted by Stefan Eichenberger 10 months ago 
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
Re: [PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset
Posted by Dimitri Fedrau 10 months ago 
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
Re: [PATCH 2/2] net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset
Posted by Stefan Eichenberger 10 months ago 
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

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.