Re: [PATCH net-next v4 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.
---
Date:   Fri Apr 10 12:44:28 2026 +0000

    net: pse-pd: add LED trigger support via notification path
    
    Add per-PI "delivering" and "enabled" LED triggers to the PSE core
    subsystem. LED state is updated from the shared pse_handle_events()
    function whenever the IRQ or poll path detects a state change, as well
    as from the regulator enable/disable paths so that host-initiated
    admin state changes via ethtool are immediately reflected.

    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 f641a6fa087fb..dfc84340afb9d 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c

[ ... ]

> +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);
> +	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;
> +
> +		ret = devm_led_trigger_register(dev, &trigs->delivering);

The commit message states that the initial LED state is queried at
registration time so already-active ports are reflected immediately.

Since no LED is bound to these triggers at registration time, the initial
led_trigger_event() call will be dropped. If an LED is bound later, will
it remain in the default (off) state because there is no .activate callback
configured on the trigger to sync the current state?

Furthermore, since the state is cached in last_delivering and last_enabled,
subsequent updates might not fire new events until the hardware state
actually toggles.

> +		if (ret)
> +			return ret;
> +
> +		trigs->enabled.name = devm_kasprintf(dev, GFP_KERNEL,
> +						     "pse-%s:port%d:enabled",
> +						     dev_id, i);
> +		if (!trigs->enabled.name)
> +			return -ENOMEM;
> +
> +		ret = devm_led_trigger_register(dev, &trigs->enabled);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}

[ ... ]

> @@ -1108,6 +1217,20 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
>  	if (ret)
>  		return ret;
>  
> +	ret = pse_led_triggers_register(pcdev);
> +	if (ret) {
> +		dev_warn(pcdev->dev, "Failed to register LED triggers: %d\n",
> +			 ret);
> +	}

If devm_led_trigger_register() fails inside pse_led_triggers_register(),
the function returns an error but leaves trigs->delivering.name allocated
and non-NULL.

Because we only log a warning here and continue execution, the cleanup
for pcdev->pi_led_trigs does not occur.

Later, pse_led_update() verifies validity using this check:

	if (!trigs->delivering.name)
		return;

Since the name is non-NULL, it proceeds to call led_trigger_event() on the
unregistered trigger. Could this result in a NULL pointer dereference,
as the led_cdevs list head within the zero-initialized led_trigger
structure is uninitialized?

> +
> +	/* Query initial LED state for all PIs so already-active ports
> +	 * are reflected immediately without waiting for a hardware event.
> +	 */
> +	for (i = 0; i < pcdev->nr_lines; i++) {
> +		if (pcdev->no_of_pse_pi || pcdev->pi[i].np)
> +			pse_led_update(pcdev, i);
> +	}

The docstring for pse_led_update() specifies that it "Must be called with
pcdev->lock held."

Since this loop calls pse_led_update(pcdev, i) without acquiring
pcdev->lock, and the PI regulators have already been registered and exposed
to consumers earlier in this function, can this race with a concurrent
regulator_enable()?

A concurrent regulator_enable() can trigger pse_pi_enable(), which acquires
the lock and also calls pse_led_update(). This could result in unprotected
calls to the pi_get_pw_status and pi_get_admin_state hardware callbacks,
and concurrent writes to the last_delivering and last_enabled variables.

> +
>  	mutex_lock(&pse_list_mutex);
>  	list_add(&pcdev->list, &pse_controller_list);
>  	mutex_unlock(&pse_list_mutex);