[PATCH 0/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[PATCH 0/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[Dimitri Fedrau]

Patchset fixes these:
- Enable temperature measurement in probe again
- Prevent reading temperature with asserted reset

Signed-off-by: Dimitri Fedrau <dima.fedrau@gmail.com>
---
Dimitri Fedrau (2):
      net: phy: marvell-88q2xxx: Enable temperature measurement in probe again
      net: phy: marvell-88q2xxx: Prevent reading temperature with asserted reset

 drivers/net/phy/marvell-88q2xxx.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
---
base-commit: 071ed42cff4fcdd89025d966d48eabef59913bf2
change-id: 20250217-marvell-88q2xxx-hwmon-enable-at-probe-2a2666325985

Best regards,
-- 
Dimitri Fedrau <dima.fedrau@gmail.com>

[PATCH 1/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[Dimitri Fedrau]

Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
mv88q222x_config_init with the consequence that the sensor is only
usable when the PHY is configured. Enable the sensor in
mv88q2xxx_hwmon_probe as well to fix this.

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 | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644
--- a/drivers/net/phy/marvell-88q2xxx.c
+++ b/drivers/net/phy/marvell-88q2xxx.c
@@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
 	struct device *dev = &phydev->mdio.dev;
 	struct device *hwmon;
 	char *hwmon_name;
+	int ret;
+
+	/* Enable temperature sense */
+	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
+			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
+	if (ret < 0)
+		return ret;
 
 	priv->enable_temp = true;
 	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));

-- 
2.39.5

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

[Dimitri Fedrau]

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 1/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[Stefan Eichenberger]

Hi Dimitri,

On Tue, Feb 18, 2025 at 07:33:09PM +0100, Dimitri Fedrau wrote:
> Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
> mv88q222x_config_init with the consequence that the sensor is only
> usable when the PHY is configured. Enable the sensor in
> mv88q2xxx_hwmon_probe as well to fix this.
> 
> 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 | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644
> --- a/drivers/net/phy/marvell-88q2xxx.c
> +++ b/drivers/net/phy/marvell-88q2xxx.c
> @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
>  	struct device *dev = &phydev->mdio.dev;
>  	struct device *hwmon;
>  	char *hwmon_name;
> +	int ret;
> +
> +	/* Enable temperature sense */
> +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> +	if (ret < 0)
> +		return ret;
>  
>  	priv->enable_temp = true;
>  	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));

Is it necessary to have it enabled in probe and in config? Is that
because of the soft reset? Can it happen that the phy is reset but
config is not called, then we would end up in the same situation right?

Regards,
Stefan

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

[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?

Regards,
Stefan

Re: [PATCH 1/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[Dimitri Fedrau]

Hi Stefan,

thanks for reviewing.

Am Wed, Feb 19, 2025 at 07:16:58AM +0100 schrieb Stefan Eichenberger:
> Hi Dimitri,
> 
> On Tue, Feb 18, 2025 at 07:33:09PM +0100, Dimitri Fedrau wrote:
> > Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
> > mv88q222x_config_init with the consequence that the sensor is only
> > usable when the PHY is configured. Enable the sensor in
> > mv88q2xxx_hwmon_probe as well to fix this.
> > 
> > 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 | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644
> > --- a/drivers/net/phy/marvell-88q2xxx.c
> > +++ b/drivers/net/phy/marvell-88q2xxx.c
> > @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> >  	struct device *dev = &phydev->mdio.dev;
> >  	struct device *hwmon;
> >  	char *hwmon_name;
> > +	int ret;
> > +
> > +	/* Enable temperature sense */
> > +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> > +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> > +	if (ret < 0)
> > +		return ret;
> >  
> >  	priv->enable_temp = true;
> >  	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
> 
> Is it necessary to have it enabled in probe and in config? Is that
> because of the soft reset? Can it happen that the phy is reset but
> config is not called, then we would end up in the same situation right?
>
Even if the phy is not configured yet, it is probed and the PHYs hard reset
is deasserted, so I can read the temperature. I think the situation you
mean is when the PHY is brought up and down again. In this case the hard
reset of the PHY is asserted and I can't read the temperature. That's
the second patch of the series that fixes this issue.

Best regards,
Dimitri

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

[Dimitri Fedrau]

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

[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.

> 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 1/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[Stefan Eichenberger]

Hi Dimitri,

On Wed, Feb 19, 2025 at 11:46:47AM +0100, Dimitri Fedrau wrote:
> Hi Stefan,
> 
> thanks for reviewing.
> 
> Am Wed, Feb 19, 2025 at 07:16:58AM +0100 schrieb Stefan Eichenberger:
> > Hi Dimitri,
> > 
> > On Tue, Feb 18, 2025 at 07:33:09PM +0100, Dimitri Fedrau wrote:
> > > Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
> > > mv88q222x_config_init with the consequence that the sensor is only
> > > usable when the PHY is configured. Enable the sensor in
> > > mv88q2xxx_hwmon_probe as well to fix this.
> > > 
> > > 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 | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> > > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644
> > > --- a/drivers/net/phy/marvell-88q2xxx.c
> > > +++ b/drivers/net/phy/marvell-88q2xxx.c
> > > @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > >  	struct device *dev = &phydev->mdio.dev;
> > >  	struct device *hwmon;
> > >  	char *hwmon_name;
> > > +	int ret;
> > > +
> > > +	/* Enable temperature sense */
> > > +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> > > +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> > > +	if (ret < 0)
> > > +		return ret;
> > >  
> > >  	priv->enable_temp = true;
> > >  	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
> > 
> > Is it necessary to have it enabled in probe and in config? Is that
> > because of the soft reset? Can it happen that the phy is reset but
> > config is not called, then we would end up in the same situation right?
> >
> Even if the phy is not configured yet, it is probed and the PHYs hard reset
> is deasserted, so I can read the temperature. I think the situation you
> mean is when the PHY is brought up and down again. In this case the hard
> reset of the PHY is asserted and I can't read the temperature. That's
> the second patch of the series that fixes this issue.

Okay I see, thanks for the explaination. So the code in
mv88q222x_config_init is required after a hard reset of the phy. I was
just thinking, if we could avoid the duplication but I guess both
enables are required. Could you maybe add a comment why we need to have
it enabled in both places?

Regards,
Stefan

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

[Stefan Eichenberger]

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

Re: [PATCH 1/2] net: phy: marvell-88q2xxx: Enable temperature measurement in probe again

[Dimitri Fedrau]

Am Wed, Feb 19, 2025 at 06:25:47PM +0100 schrieb Stefan Eichenberger:
> Hi Dimitri,
> 
> On Wed, Feb 19, 2025 at 11:46:47AM +0100, Dimitri Fedrau wrote:
> > Hi Stefan,
> > 
> > thanks for reviewing.
> > 
> > Am Wed, Feb 19, 2025 at 07:16:58AM +0100 schrieb Stefan Eichenberger:
> > > Hi Dimitri,
> > > 
> > > On Tue, Feb 18, 2025 at 07:33:09PM +0100, Dimitri Fedrau wrote:
> > > > Enabling of the temperature sensor was moved from mv88q2xxx_hwmon_probe to
> > > > mv88q222x_config_init with the consequence that the sensor is only
> > > > usable when the PHY is configured. Enable the sensor in
> > > > mv88q2xxx_hwmon_probe as well to fix this.
> > > > 
> > > > 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 | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/phy/marvell-88q2xxx.c b/drivers/net/phy/marvell-88q2xxx.c
> > > > index a3996471a1c9a5d4060d5d19ce44aa70e902a83f..30d71bfc365597d77c34c48f05390db9d63c4af4 100644
> > > > --- a/drivers/net/phy/marvell-88q2xxx.c
> > > > +++ b/drivers/net/phy/marvell-88q2xxx.c
> > > > @@ -718,6 +718,13 @@ static int mv88q2xxx_hwmon_probe(struct phy_device *phydev)
> > > >  	struct device *dev = &phydev->mdio.dev;
> > > >  	struct device *hwmon;
> > > >  	char *hwmon_name;
> > > > +	int ret;
> > > > +
> > > > +	/* Enable temperature sense */
> > > > +	ret = phy_modify_mmd(phydev, MDIO_MMD_PCS, MDIO_MMD_PCS_MV_TEMP_SENSOR2,
> > > > +			     MDIO_MMD_PCS_MV_TEMP_SENSOR2_DIS_MASK, 0);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > >  
> > > >  	priv->enable_temp = true;
> > > >  	hwmon_name = devm_hwmon_sanitize_name(dev, dev_name(dev));
> > > 
> > > Is it necessary to have it enabled in probe and in config? Is that
> > > because of the soft reset? Can it happen that the phy is reset but
> > > config is not called, then we would end up in the same situation right?
> > >
> > Even if the phy is not configured yet, it is probed and the PHYs hard reset
> > is deasserted, so I can read the temperature. I think the situation you
> > mean is when the PHY is brought up and down again. In this case the hard
> > reset of the PHY is asserted and I can't read the temperature. That's
> > the second patch of the series that fixes this issue.
> 
> Okay I see, thanks for the explaination. So the code in
> mv88q222x_config_init is required after a hard reset of the phy. I was
> just thinking, if we could avoid the duplication but I guess both
> enables are required. Could you maybe add a comment why we need to have
> it enabled in both places?
>
Will add the comment.

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

[Dimitri Fedrau]

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