Re: [PATCH net-next v5 2/2] net: pse-pd: add LED trigger support via notification path

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: pse-pd: add LED trigger support via notification path

This commit adds per-PI LED triggers to the PSE core subsystem to reflect
delivering and enabled states. The LED state is updated during hardware
events and administrative state changes via ethtool.

Link: https://lore.kernel.org/oe-kbuild-all/202603251254.o5PqMBRU-lkp@intel.com/
Link: https://lore.kernel.org/oe-kbuild-all/202603251250.cuMCk5Yv-lkp@intel.com/

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index b7ffec0c942c..bff97211ae22 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> @@ -669,6 +670,168 @@ static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_LEDS_TRIGGERS)

[ ... ]

> +static int pse_led_triggers_register(struct pse_controller_dev *pcdev)
> +{
> +	struct device *dev = pcdev->dev;
> +	const char *dev_id;
> +	int i, ret;
> +
> +	dev_id = dev_name(dev);
> +
> +	pcdev->pi_led_trigs = devm_kcalloc(dev, pcdev->nr_lines,
> +					   sizeof(*pcdev->pi_led_trigs),
> +					   GFP_KERNEL);

Since devm resources are released in strict LIFO order, and pi_led_trigs
is allocated here after the regulators are registered in
pse_controller_register(), will the pi_led_trigs array be freed before
the regulators are unregistered on driver unbind?

When the regulator core later unregisters the regulators and flushes
pending disables, could it invoke pse_pi_disable() and call pse_led_update(),
dereferencing the dangling pcdev->pi_led_trigs pointer?

> +	if (!pcdev->pi_led_trigs)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < pcdev->nr_lines; i++) {
> +		struct pse_pi_led_triggers *trigs = &pcdev->pi_led_trigs[i];
> +
> +		/* Skip PIs not described in device tree */
> +		if (!pcdev->no_of_pse_pi && !pcdev->pi[i].np)
> +			continue;
> +
> +		trigs->delivering.name = devm_kasprintf(dev, GFP_KERNEL,
> +							"pse-%s:port%d:delivering",
> +							dev_id, i);
> +		if (!trigs->delivering.name)
> +			return -ENOMEM;
> +		trigs->delivering.activate = pse_led_delivering_activate;
> +
> +		ret = devm_led_trigger_register(dev, &trigs->delivering);

Regulators are exposed before this code runs in pse_controller_register().
What happens if a consumer calls regulator_enable() concurrently?

If a concurrent pse_pi_enable() calls pse_led_update() right after
trigs->delivering.name is set but before devm_led_trigger_register() executes:

pse_led_update() sees pcdev->pi_led_trigs and trigs->delivering.name are
both non-NULL, and proceeds to call led_trigger_event().

Since devm_led_trigger_register() hasn't initialized the trigger's internal
led_cdevs list head yet, will led_trigger_event() attempt to iterate a
zeroed memory list and cause a NULL pointer dereference?

> +		if (ret) {
> +			trigs->delivering.name = NULL;
> +			return ret;
> +		}

[ ... ]

> @@ -1107,6 +1273,31 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = pse_led_triggers_register(pcdev);
> +	if (ret) {
> +		/* LED triggers are non-essential for power delivery; warn
> +		 * and continue. NULL out the array so pse_led_update()'s
> +		 * early-return guard short-circuits any later calls onto
> +		 * partially-registered triggers.
> +		 */
> +		dev_warn(pcdev->dev, "Failed to register LED triggers: %d\n",
> +			 ret);
> +		pcdev->pi_led_trigs = NULL;
> +	}

If pse_led_triggers_register() fails halfway through, the device probe still
succeeds, which means devm cleanup will not run. Could the successfully
registered LED triggers from earlier loop iterations remain registered
indefinitely with the LED subsystem?

Since pcdev->pi_led_trigs is NULLed, pse_led_update() will short-circuit.
Does this leave these globally visible triggers permanently orphaned without
ever receiving state updates?

> +
> +	/* Query initial LED state for all PIs so already-active ports
> +	 * are reflected immediately without waiting for a hardware event.
> +	 * Hold pcdev->lock: regulators are already exposed and a
> +	 * concurrent regulator_enable() would race on the hw callbacks
> +	 * and on last_delivering / last_enabled.
> +	 */
> +	mutex_lock(&pcdev->lock);

[ ... ]

> @@ -1267,7 +1458,14 @@ static void pse_handle_events(struct pse_controller_dev *pcdev,
>  		struct pse_ntf ntf = {};
>  		int ret;
>  
> -		/* Do nothing PI not described */
> +		/* Update LEDs for described PIs regardless of consumer state.
> +		 * LED triggers are registered at controller init, before any
> +		 * PHY claims a PSE control, so rdev may still be NULL here.
> +		 */
> +		if (pcdev->no_of_pse_pi || pcdev->pi[i].np)
> +			pse_led_update(pcdev, i);

The docstring for pse_led_update() requires it to be called with pcdev->lock
held. Does calling it here locklessly inside the event handler violate
that locking contract?

Could this lack of synchronization cause data races on the cached state
variables, or allow concurrent execution of the hardware callbacks
(pi_get_pw_status and pi_get_admin_state) if they are invoked at the same
time by other paths?

> +
> +		/* Skip regulator/netlink path for PIs without consumers */
>  		if (!pcdev->pi[i].rdev)
>  			continue;