| Message ID | 20230424134003.297827-1-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [1/1] net: phy: Fix reading LED reg property | expand |
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
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
> $ 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
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)
'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(-)