Re: [PATCH 2/2] net: phy: motorcomm: add missing trigger link for YT8521

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] In drivers/net/phy/motorcomm.c, yt8521_led_hw_control_get()
  now sets TRIGGER_NETDEV_LINK when YT8521_LED_10_ON_EN,…
- [Medium] net: phy: motorcomm: bugs in yt8521_led_brightness_set() and
  force mode
- [Low] The patch fixes a user-visible get/set asymmetry: after the
  netdev trigger activates on hardware set up for 'link at any speed',…

--

net: phy: motorcomm: add missing trigger link for YT8521

This patch makes yt8521_led_hw_control_get() report TRIGGER_NETDEV_LINK
when the 10, 100 and 1000 link enable bits are all set in the LED config
register. The goal is to match yt8521_led_hw_control_set(), which already
handles TRIGGER_NETDEV_LINK.

> net: phy: motorcomm: add missing trigger link for YT8521

[Severity: Low]
This isn't a bug, but should this carry a Fixes: tag? The get/set
asymmetry comes from the commit that added the LED callbacks:

Fixes: 355b82c54c12 ("net: phy: motorcomm: Add support for PHY LEDs on YT8521")

Also, the subject only names YT8521. The PHY_ID_YT8531 entry in
motorcomm_phy_drvs[] uses the same callback:

    .led_hw_control_get = yt8521_led_hw_control_get,

so YT8531 behaviour changes too. Could the commit message say so?

> diff --git a/drivers/net/phy/motorcomm.c b/drivers/net/phy/motorcomm.c
> index bcd3afef0d496..5b1d17bd231a7 100644
> --- a/drivers/net/phy/motorcomm.c
> +++ b/drivers/net/phy/motorcomm.c
> @@ -1901,6 +1901,11 @@ static int yt8521_led_hw_control_get(struct phy_device *phydev, u8 index,
>  	if (val & YT8521_LED_10_ON_EN)
>  		__set_bit(TRIGGER_NETDEV_LINK_10, rules);
>  
> +	if ((val & YT8521_LED_10_ON_EN) &&
> +	    (val & YT8521_LED_100_ON_EN) &&
> +	    (val & YT8521_LED_1000_ON_EN))
> +		__set_bit(TRIGGER_NETDEV_LINK, rules);

[Severity: Medium]
This isn't introduced by this patch, but it is on the same hardware
control path. yt8521_led_brightness_set(), added by the previous patch
in this series, seems to have a few problems.

The return value of ytphy_read_ext() is stored straight into u16 regs:

    regs = ytphy_read_ext(phydev, YTPHY_LED_GENERAL_CFG_REG);

If the MDIO access fails, the negative errno is truncated. The force
bits are applied on top of it and the result is written back to
YTPHY_LED_GENERAL_CFG_REG. Wouldn't that clobber the force enable and
force mode bits of every LED, not just the one at index?

phy_led_set_brightness() only holds phydev->lock around the callback,
but ytphy_read_ext() and ytphy_write_ext() require the caller to hold
the MDIO bus lock. Each access is a YTPHY_PAGE_SELECT write followed
by a YTPHY_PAGE_DATA access. Another user of the bus can change the
page select in between, so the wrong extended register gets read or
written. The read-modify-write of YTPHY_LED_GENERAL_CFG_REG is not
atomic either.

Should this use ytphy_write_ext_with_lock() and
ytphy_modify_ext_with_lock() instead? That would also take care of the
unchecked read above. The ytphy_read_ext() and ytphy_write_ext() calls
in yt8521_led_hw_control_get() and yt8521_led_hw_control_set() look
like they have the same locking issue.

Finally, when value != LED_OFF this sets YTPHY_LED_FORCE_EN(index) in
YTPHY_LED_GENERAL_CFG_REG. But yt8521_led_hw_control_set() only writes
YT8521_LED0_CFG_REG + index and never clears the force bits.

The netdev trigger does not turn the LED off before offloading.
set_baseline_state() calls hw_control_set() directly when
trigger_data->hw_control is true. led_trigger_set() only forces
LED_OFF when a previous trigger is being removed.

So take an LED with trigger none. Writing 1 to brightness and then
selecting the netdev trigger with an offloadable mode would leave the
force bit set. The LED then stays lit regardless of link state or
activity.

Should yt8521_led_hw_control_set() clear YTPHY_LED_FORCE_EN(index) and
YTPHY_LED_FORCE_MODE(index) before programming the rules, similar to
what mxl86110_led_hw_control_set() does?

[Severity: Medium]
TRIGGER_NETDEV_LINK_10, LINK_100 and LINK_1000 are still set just above
this, so the reported mode becomes LINK | LINK_10 | LINK_100 | LINK_1000.
Can the netdev trigger work with that combination?

netdev_trig_activate() stores the result without checking it:

drivers/leds/trigger/ledtrig-netdev.c:netdev_trig_activate() {
    ...
			rc = led_cdev->hw_control_get(led_cdev, &mode);
			if (!rc)
				trigger_data->mode = mode;
    ...
}

But netdev_led_attr_store() treats LINK and the per-speed bits as
mutually exclusive:

drivers/leds/trigger/ledtrig-netdev.c:netdev_led_attr_store() {
    ...
	assign_bit(bit, &mode, state);

	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
	    (test_bit(TRIGGER_NETDEV_LINK_10, &mode) ||
	     test_bit(TRIGGER_NETDEV_LINK_100, &mode) ||
	     test_bit(TRIGGER_NETDEV_LINK_1000, &mode) ||
    ...
		return -EINVAL;
    ...
}

Say the trigger activates on an LED whose config register has the
10/100/1000 bits set. That can be the boot default, or the result of an
earlier yt8521_led_hw_control_set() with TRIGGER_NETDEV_LINK, which
writes exactly those three bits.

In that case, wouldn't every sysfs write that leaves LINK set (tx=1,
rx=1, full_duplex=1, link_10=0, even link=1) now fail with -EINVAL?
Only link=0 would get through.

Before this patch, get returned LINK_10 | LINK_100 | LINK_1000, which
is a valid mode, so those writes worked. It also means set(LINK)
followed by get gives back a mode the trigger rejects.

Should get report TRIGGER_NETDEV_LINK instead of the three per-speed
bits when all three are on, rather than in addition to them?

realtek_main.c (rtl8211f, rtl8211e, rtl822x) and air_an8801.c use the
same additive pattern, so they seem to have the same latent conflict.

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261110100001.1873309-1-amadeus%40jmu.edu.cn