[PATCH 1/1] net: phy: Fix reading LED reg property

[PATCH 1/1] net: phy: Fix reading LED reg property

[Alexander Stein]

'reg' is always encoded in 32 bits, thus it has to be read using the
function with the corresponding bit width.

Fixes: 01e5b728e9e4 ("net: phy: Add a binding for PHY LEDs")
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
phy_device::index is u8, so an intermediate step is necessary, without
changing the whole phydev LED API.

 drivers/net/phy/phy_device.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index d373446ab5ac..d5c4c7f86a20 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -3028,6 +3028,7 @@ static int of_phy_led(struct phy_device *phydev,
 	struct led_init_data init_data = {};
 	struct led_classdev *cdev;
 	struct phy_led *phyled;
+	u32 index;
 	int err;
 
 	phyled = devm_kzalloc(dev, sizeof(*phyled), GFP_KERNEL);
@@ -3037,10 +3038,11 @@ static int of_phy_led(struct phy_device *phydev,
 	cdev = &phyled->led_cdev;
 	phyled->phydev = phydev;
 
-	err = of_property_read_u8(led, "reg", &phyled->index);
+	err = of_property_read_u32(led, "reg", &index);
 	if (err)
 		return err;
 
+	phyled->index = index;
 	if (phydev->drv->led_brightness_set)
 		cdev->brightness_set_blocking = phy_led_set_brightness;
 	if (phydev->drv->led_blink_set)
-- 
2.34.1

Re: [PATCH 1/1] net: phy: Fix reading LED reg property

[Andrew Lunn]

On Mon, Apr 24, 2023 at 03:40:02PM +0200, Alexander Stein wrote:
> 'reg' is always encoded in 32 bits, thus it has to be read using the
> function with the corresponding bit width.

Hi Alexander

Is this an endian thing? Does it return the wrong value on big endian
systems?

I deliberately used of_property_read_u8() because it will perform a
range check, and if the value is bigger or smaller than 0-256 it will
return an error. Your change does not include such range checks, which
i don't like.

  Andrew

Re: [PATCH 1/1] net: phy: Fix reading LED reg property

[Alexander Stein]

Hi Andrew,

Am Montag, 24. April 2023, 15:47:14 CEST schrieb Andrew Lunn:
> On Mon, Apr 24, 2023 at 03:40:02PM +0200, Alexander Stein wrote:
> > 'reg' is always encoded in 32 bits, thus it has to be read using the
> > function with the corresponding bit width.
> 
> Hi Alexander
> 
> Is this an endian thing? Does it return the wrong value on big endian
> systems?

It is an endian issue, but the platform's endianess doesn't matter here. The 
encoding for device properties is (always) big-endian, so a 32-bit 'reg' value 
of '2' looks like this:

$ hexdump -C /sys/firmware/devicetree/base/soc@0/bus@30800000/
ethernet@30bf0000/mdio/ethernet-phy@3/leds/led@2/reg
00000000  00 00 00 02                                       |....|
00000004

Using of_property_read_u8 will only read the first byte, thus all values of 
reg result in 0.

> I deliberately used of_property_read_u8() because it will perform a
> range check, and if the value is bigger or smaller than 0-256 it will
> return an error. Your change does not include such range checks, which
> i don't like.

Sure, I can added this check.

Best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 1/1] net: phy: Fix reading LED reg property

[Andrew Lunn]

> $ hexdump -C /sys/firmware/devicetree/base/soc@0/bus@30800000/
> ethernet@30bf0000/mdio/ethernet-phy@3/leds/led@2/reg
> 00000000  00 00 00 02                                       |....|
> 00000004
> 
> Using of_property_read_u8 will only read the first byte, thus all values of 
> reg result in 0.

Ah! Thanks for the explanation. And the board i tested only had one
led, at reg = <0>; so it worked.

> > I deliberately used of_property_read_u8() because it will perform a
> > range check, and if the value is bigger or smaller than 0-256 it will
> > return an error. Your change does not include such range checks, which
> > i don't like.
> 
> Sure, I can added this check.

Great, thanks.

       Andrew