Re: [PATCH net-next 3/3] net: phy: dap8211r: add led hardware control support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] net: phy: dap8211r: link trigger rejected in
  dap8211r_led_hw_is_supported()
- [Medium] net: phy: dap8211r: LED register not cleared in
  dap8211r_led_hw_control_set()
--

commit 7e5f0824bef63e117be29a9ce8d9c67d5c813bfa
Author: Artem Shimko <a.shimko.dev@gmail.com>

net: phy: dap8211r: add led hardware control support

This commit adds hardware LED control support to the DAP8211R PHY driver by
implementing the led_hw_is_supported(), led_hw_control_set(), and
led_hw_control_get() callbacks.

> diff --git a/drivers/net/phy/dap8211r.c b/drivers/net/phy/dap8211r.c
> index 4eb50c26397fb..2e371c231d32c 100644
> --- a/drivers/net/phy/dap8211r.c
> +++ b/drivers/net/phy/dap8211r.c
[ ... ]
> @@ -57,6 +58,33 @@
>   */
>  #define DAP8211R_DEFAULT_DEL_SEL	0xD
>  
> +#define DAP8211R_LED_ID_0		0
> +#define DAP8211R_LED_ID_1		1
> +#define DAP8211R_LED_ID_2		2
> +#define DAP8211R_LED_0			0xA00C
> +#define DAP8211R_LED_1			0xA00D
> +#define DAP8211R_LED_2			0xA00E
> +#define DAP8211R_LED_ACT_BLINK		BIT(13)
> +#define DAP8211R_LED_FULL_DUPLEX	BIT(12)
> +#define DAP8211R_LED_HALF_DUPLEX	BIT(11)
> +#define DAP8211R_LED_TX			BIT(10)
> +#define DAP8211R_LED_RX			BIT(9)
> +#define DAP8211R_LED_LINK_1000		BIT(6)
> +#define DAP8211R_LED_LINK_100		BIT(5)
> +#define DAP8211R_LED_LINK_10		BIT(4)
> +
> +#define DAP8211R_LED_LINK		(DAP8211R_LED_LINK_10	| \
> +					DAP8211R_LED_LINK_100	| \
> +					DAP8211R_LED_LINK_1000)
> +
> +#define DAP8211R_LED_SUPPORTED_TRG	(BIT(TRIGGER_NETDEV_LINK_10)	| \
> +					BIT(TRIGGER_NETDEV_LINK_100)	| \
> +					BIT(TRIGGER_NETDEV_LINK_1000)	| \
> +					BIT(TRIGGER_NETDEV_TX)		| \
> +					BIT(TRIGGER_NETDEV_RX)		| \
> +					BIT(TRIGGER_NETDEV_HALF_DUPLEX)	| \
> +					BIT(TRIGGER_NETDEV_FULL_DUPLEX))

[Severity: Medium]
Should DAP8211R_LED_SUPPORTED_TRG include BIT(TRIGGER_NETDEV_LINK)?

Without it, dap8211r_led_hw_is_supported() returns -EOPNOTSUPP whenever
TRIGGER_NETDEV_LINK is requested, so enabling the netdev link trigger on a
DAP8211R LED fails and the TRIGGER_NETDEV_LINK handling in
dap8211r_led_hw_control_set() is never reached.

[ ... ]

> @@ -142,6 +170,156 @@ static int dap8211r_modify_ext(struct phy_device *phydev, u16 reg, u16 mask, u16
[ ... ]
> +static int dap8211r_led_hw_control_set(struct phy_device *phydev, u8 index, unsigned long rules)
> +{
> +	u16 led_mode = 0, led_reg;
> +
> +	switch (index) {
> +	case DAP8211R_LED_ID_0:
> +		led_reg = DAP8211R_LED_0;
> +		break;
> +	case DAP8211R_LED_ID_1:
> +		led_reg = DAP8211R_LED_1;
> +		break;
> +	case DAP8211R_LED_ID_2:
> +		led_reg = DAP8211R_LED_2;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK, &rules))
> +		led_mode |= DAP8211R_LED_LINK;
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK_10, &rules))
> +		led_mode |= DAP8211R_LED_LINK_10;
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK_100, &rules))
> +		led_mode |= DAP8211R_LED_LINK_100;
> +
> +	if (test_bit(TRIGGER_NETDEV_LINK_1000, &rules))
> +		led_mode |= DAP8211R_LED_LINK_1000;
> +
> +	if (test_bit(TRIGGER_NETDEV_RX, &rules))
> +		led_mode |= DAP8211R_LED_RX | DAP8211R_LED_ACT_BLINK;
> +
> +	if (test_bit(TRIGGER_NETDEV_TX, &rules))
> +		led_mode |= DAP8211R_LED_TX | DAP8211R_LED_ACT_BLINK;
> +
> +	if (test_bit(TRIGGER_NETDEV_HALF_DUPLEX, &rules))
> +		led_mode |= DAP8211R_LED_HALF_DUPLEX;
> +
> +	if (test_bit(TRIGGER_NETDEV_FULL_DUPLEX, &rules))
> +		led_mode |= DAP8211R_LED_FULL_DUPLEX;
> +
> +	if (!led_mode)
> +		return -EOPNOTSUPP;
> +
> +	return dap8211r_write_ext(phydev, led_reg, led_mode);
> +}

[Severity: Medium]
Does returning -EOPNOTSUPP when led_mode is 0 leave the previous trigger bits
enabled in the PHY register when all trigger rules are cleared?

When the last active netdev trigger rule is disabled (rules == 0),
dap8211r_led_hw_is_supported() returns 0, so set_baseline_state() in
drivers/leds/trigger/ledtrig-netdev.c calls hw_control_set() with mode == 0
to turn off the hardware triggers.  Returning -EOPNOTSUPP here skips
dap8211r_write_ext(), leaving the old configuration active in led_reg.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113110546.173624-1-a.shimko.dev@gmail.com?part=3