[PATCH net-next v4 0/2] net: pse-pd: add poll path and LED trigger support

[PATCH net-next v4 0/2] net: pse-pd: add poll path and LED trigger support

[Carlo Szelinsky]

Thanks to Kory, Oleksij, Krzysztof and Andrew for all the helpful
feedback on earlier versions - really appreciate the time you put
into reviewing this.

This series adds poll-based event detection and LED trigger support
to the PSE core subsystem.

Patch 1 introduces the poll path independently of LED support,
so it can be tested in isolation on boards with and without IRQ
configured.

Patch 2 adds LED triggers that hook into the shared event handling
path introduced by patch 1.

Note: pse_handle_events() and the existing pse_isr() pass notifs_mask
as a single unsigned long, which limits the bitmask to BITS_PER_LONG
PI lines. This is a pre-existing constraint in the IRQ path and is
sufficient for all current PSE controllers (max 48 ports vs 64-bit
unsigned long), but may need to be converted to DECLARE_BITMAP() if
future hardware exceeds this limit.

Changes since v3:
- Dropped the dt-bindings poll-interval-ms patch: the poll interval
  is a driver decision, not a hardware property (Krzysztof)
- Removed of_property_read_u32() for poll-interval-ms from
  devm_pse_poll_helper(); the 500ms default is now hardcoded but
  drivers can override pcdev->poll_interval_ms before calling the
  helper
- Rebased on net-next/main

Changes since v2:
- Based on net-next/main, added net-next subject prefix
- Added --base tree information
- Added CC for devicetree list and DT maintainers
- Collected Reviewed-by from Kory Maincent on patch 1/3
- Fixed build error when CONFIG_LEDS_TRIGGERS is disabled:
  moved LED registration before list_add(), removing the
  pcdev->pi_led_trigs = NULL assignment on conditionally
  compiled struct member (reported by kernel test robot)
- Fixed use-after-free on device unbind: poll work is now
  cancelled via devm_add_action_or_reset() to ensure correct
  devres teardown ordering (poll_work cancelled before
  poll_notifs is freed)
- Used system_freezable_wq for poll worker to prevent hardware
  access during system suspend
- Added PoDL power status and admin state checks to LED triggers
  so they work for both C33 and PoDL controller types
- Used dev_name(dev) for LED trigger names to ensure uniqueness
  across multiple PSE controllers (of_node->name can be generic)
- Added initial LED state query at registration so already-active
  ports are reflected immediately
- Added pse_led_update() calls in regulator enable/disable paths
  so ethtool admin state changes are reflected in LEDs
- Moved LED trigger registration before list_add() to prevent
  race where IRQ/poll could invoke pse_led_update() on partially
  initialized triggers

Changes since v1:
- Split single patch into 3 separate patches
- Extracted pse_handle_events() and devm_pse_poll_helper() as a
  standalone poll path (patches 1-2), testable without LED code
- Added DT binding for poll-interval-ms as a separate patch
- Renamed led-poll-interval-ms to poll-interval-ms for generic use
- Fire LED triggers from the notification path rather than a
  separate poll loop

Tested on Realtek RTL9303 with HS104 PoE chip, poll path only
(without IRQ configured). Verified PD connect/disconnect notifications
and LED trigger state changes.

Link: https://lore.kernel.org/all/20260329153124.2823980-1-github@szelinsky.de/
Link: https://lore.kernel.org/all/20260323201225.1836561-1-github@szelinsky.de/
Link: https://lore.kernel.org/all/20260314235916.2391678-1-github@szelinsky.de/

Carlo Szelinsky (2):
  net: pse-pd: add devm_pse_poll_helper()
  net: pse-pd: add LED trigger support via notification path

 drivers/net/pse-pd/pse_core.c | 296 ++++++++++++++++++++++++++++++----
 include/linux/pse-pd/pse.h    |  34 ++++
 2 files changed, 299 insertions(+), 31 deletions(-)


base-commit: b3e69fc3196fc421e26196e7792f17b0463edc6f
-- 
2.43.0

[PATCH net-next v4 1/2] net: pse-pd: add devm_pse_poll_helper()

[Carlo Szelinsky]

Extract the common event handling loop from pse_isr() into a shared
pse_handle_events() function, and add a generic poll-based alternative
to the IRQ path for PSE controllers that lack interrupt support or
have IRQ lines not wired on the board.

The new devm_pse_poll_helper() function sets up a delayed work that
periodically calls the driver's map_event callback to detect state
changes, feeding events into the existing ntf_fifo / pse_send_ntf_worker
notification pipeline. This reuses the same pse_irq_desc interface as
the IRQ path — the driver provides a map_event callback that populates
per-PI notification arrays.

The poll worker uses system_freezable_wq to avoid running during system
suspend when the underlying hardware (e.g. I2C bus) may be inaccessible.

Work cancellation on teardown is handled via devm_add_action_or_reset()
to ensure the delayed work is cancelled before poll_notifs is freed
by devres, avoiding a use-after-free when devm_pse_poll_helper() is
called after devm_pse_controller_register() (devres LIFO ordering).

The poll interval defaults to 500ms if not set by the driver, balancing
responsiveness against bus load (e.g. I2C).

Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 164 +++++++++++++++++++++++++++-------
 include/linux/pse-pd/pse.h    |  12 +++
 2 files changed, 146 insertions(+), 30 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 3beaaaeec9e1..f641a6fa087f 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -14,10 +14,18 @@
 #include <linux/regulator/driver.h>
 #include <linux/regulator/machine.h>
 #include <linux/rtnetlink.h>
+#include <linux/workqueue.h>
 #include <net/net_trackers.h>
 
 #define PSE_PW_D_LIMIT INT_MAX
 
+/*
+ * Default poll interval for controllers without IRQ support.
+ * 500ms provides a reasonable trade-off between responsiveness
+ * (event detection, PD detection) and I2C bus utilization.
+ */
+#define PSE_DEFAULT_POLL_INTERVAL_MS 500
+
 static DEFINE_MUTEX(pse_list_mutex);
 static LIST_HEAD(pse_controller_list);
 static DEFINE_XARRAY_ALLOC(pse_pw_d_map);
@@ -1238,66 +1246,103 @@ static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id,
 }
 
 /**
- * pse_isr - IRQ handler for PSE
- * @irq: irq number
- * @data: pointer to user interrupt structure
+ * pse_handle_events - Process PSE events for all PIs
+ * @pcdev: a pointer to the PSE controller device
+ * @notifs: per-PI notification array
+ * @notifs_mask: bitmask of PIs with events
  *
- * Return: irqreturn_t - status of IRQ
+ * Common event handling shared between IRQ and poll paths.
+ * Caller must hold pcdev->lock.
  */
-static irqreturn_t pse_isr(int irq, void *data)
+static void pse_handle_events(struct pse_controller_dev *pcdev,
+			      unsigned long *notifs,
+			      unsigned long notifs_mask)
 {
-	struct pse_controller_dev *pcdev;
-	unsigned long notifs_mask = 0;
-	struct pse_irq_desc *desc;
-	struct pse_irq *h = data;
-	int ret, i;
-
-	desc = &h->desc;
-	pcdev = h->pcdev;
-
-	/* Clear notifs mask */
-	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
-	mutex_lock(&pcdev->lock);
-	ret = desc->map_event(irq, pcdev, h->notifs, &notifs_mask);
-	if (ret || !notifs_mask) {
-		mutex_unlock(&pcdev->lock);
-		return IRQ_NONE;
-	}
+	int i;
 
 	for_each_set_bit(i, &notifs_mask, pcdev->nr_lines) {
-		unsigned long notifs, rnotifs;
+		unsigned long pi_notifs, rnotifs;
 		struct pse_ntf ntf = {};
+		int ret;
 
 		/* Do nothing PI not described */
 		if (!pcdev->pi[i].rdev)
 			continue;
 
-		notifs = h->notifs[i];
+		pi_notifs = notifs[i];
 		if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[i].pw_d)) {
-			ret = pse_set_config_isr(pcdev, i, notifs);
+			ret = pse_set_config_isr(pcdev, i, pi_notifs);
 			if (ret)
-				notifs |= ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR;
+				pi_notifs |= ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR;
 		}
 
-		dev_dbg(h->pcdev->dev,
-			"Sending PSE notification EVT 0x%lx\n", notifs);
+		dev_dbg(pcdev->dev,
+			"Sending PSE notification EVT 0x%lx\n", pi_notifs);
 
-		ntf.notifs = notifs;
+		ntf.notifs = pi_notifs;
 		ntf.id = i;
 		kfifo_in_spinlocked(&pcdev->ntf_fifo, &ntf, 1,
 				    &pcdev->ntf_fifo_lock);
 		schedule_work(&pcdev->ntf_work);
 
-		rnotifs = pse_to_regulator_notifs(notifs);
+		rnotifs = pse_to_regulator_notifs(pi_notifs);
 		regulator_notifier_call_chain(pcdev->pi[i].rdev, rnotifs,
 					      NULL);
 	}
+}
+
+/**
+ * pse_isr - IRQ handler for PSE
+ * @irq: irq number
+ * @data: pointer to user interrupt structure
+ *
+ * Return: irqreturn_t - status of IRQ
+ */
+static irqreturn_t pse_isr(int irq, void *data)
+{
+	struct pse_controller_dev *pcdev;
+	unsigned long notifs_mask = 0;
+	struct pse_irq *h = data;
+	int ret;
 
+	pcdev = h->pcdev;
+
+	/* Clear notifs mask */
+	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
+	mutex_lock(&pcdev->lock);
+	ret = h->desc.map_event(irq, pcdev, h->notifs, &notifs_mask);
+	if (ret || !notifs_mask) {
+		mutex_unlock(&pcdev->lock);
+		return IRQ_NONE;
+	}
+
+	pse_handle_events(pcdev, h->notifs, notifs_mask);
 	mutex_unlock(&pcdev->lock);
 
 	return IRQ_HANDLED;
 }
 
+static void pse_poll_worker(struct work_struct *work)
+{
+	struct pse_controller_dev *pcdev =
+		container_of(work, struct pse_controller_dev,
+			     poll_work.work);
+	unsigned long notifs_mask = 0;
+	int ret;
+
+	memset(pcdev->poll_notifs, 0,
+	       pcdev->nr_lines * sizeof(*pcdev->poll_notifs));
+	mutex_lock(&pcdev->lock);
+	ret = pcdev->poll_desc.map_event(0, pcdev, pcdev->poll_notifs,
+					 &notifs_mask);
+	if (!ret && notifs_mask)
+		pse_handle_events(pcdev, pcdev->poll_notifs, notifs_mask);
+	mutex_unlock(&pcdev->lock);
+
+	queue_delayed_work(system_freezable_wq, &pcdev->poll_work,
+			   msecs_to_jiffies(pcdev->poll_interval_ms));
+}
+
 /**
  * devm_pse_irq_helper - Register IRQ based PSE event notifier
  * @pcdev: a pointer to the PSE
@@ -1351,6 +1396,65 @@ int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
 }
 EXPORT_SYMBOL_GPL(devm_pse_irq_helper);
 
+static void pse_poll_cancel(void *data)
+{
+	struct pse_controller_dev *pcdev = data;
+
+	cancel_delayed_work_sync(&pcdev->poll_work);
+}
+
+/**
+ * devm_pse_poll_helper - Register poll-based PSE event notifier
+ * @pcdev: a pointer to the PSE controller device
+ * @d: PSE event description (uses same pse_irq_desc as IRQ path)
+ *
+ * For PSE controllers without IRQ support or with IRQ not wired. Sets
+ * up a delayed work that periodically calls the driver's map_event
+ * callback to detect state changes, feeding events into the standard
+ * notification pipeline.
+ *
+ * The poll worker uses system_freezable_wq to ensure it does not run
+ * during system suspend while the hardware may be inaccessible.
+ *
+ * Return: 0 on success and errno on failure
+ */
+int devm_pse_poll_helper(struct pse_controller_dev *pcdev,
+			 const struct pse_irq_desc *d)
+{
+	struct device *dev = pcdev->dev;
+	int ret;
+
+	if (!d || !d->map_event || !d->name)
+		return -EINVAL;
+
+	pcdev->poll_desc = *d;
+	pcdev->poll_notifs = devm_kcalloc(dev, pcdev->nr_lines,
+					  sizeof(*pcdev->poll_notifs),
+					  GFP_KERNEL);
+	if (!pcdev->poll_notifs)
+		return -ENOMEM;
+
+	if (!pcdev->poll_interval_ms)
+		pcdev->poll_interval_ms = PSE_DEFAULT_POLL_INTERVAL_MS;
+
+	INIT_DELAYED_WORK(&pcdev->poll_work, pse_poll_worker);
+	pcdev->polling = true;
+
+	/* Register devm action to cancel poll work before poll_notifs is
+	 * freed by devres. This ensures correct teardown ordering since
+	 * devm_pse_poll_helper() is called after devm_pse_controller_register().
+	 */
+	ret = devm_add_action_or_reset(dev, pse_poll_cancel, pcdev);
+	if (ret)
+		return ret;
+
+	queue_delayed_work(system_freezable_wq, &pcdev->poll_work,
+			   msecs_to_jiffies(pcdev->poll_interval_ms));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_pse_poll_helper);
+
 /* PSE control section */
 
 static void __pse_control_release(struct kref *kref)
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 4e5696cfade7..44d5d10e239d 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -292,6 +292,11 @@ struct pse_ntf {
  * @pi: table of PSE PIs described in this controller device
  * @no_of_pse_pi: flag set if the pse_pis devicetree node is not used
  * @irq: PSE interrupt
+ * @polling: flag indicating poll-based event detection is active
+ * @poll_interval_ms: poll interval in milliseconds
+ * @poll_work: delayed work for poll-based event detection
+ * @poll_desc: copy of the driver's event descriptor for polling
+ * @poll_notifs: per-PI notification scratch space for poll worker
  * @pis_prio_max: Maximum value allowed for the PSE PIs priority
  * @supp_budget_eval_strategies: budget evaluation strategies supported
  *				 by the PSE
@@ -312,6 +317,11 @@ struct pse_controller_dev {
 	struct pse_pi *pi;
 	bool no_of_pse_pi;
 	int irq;
+	bool polling;
+	unsigned int poll_interval_ms;
+	struct delayed_work poll_work;
+	struct pse_irq_desc poll_desc;
+	unsigned long *poll_notifs;
 	unsigned int pis_prio_max;
 	u32 supp_budget_eval_strategies;
 	struct work_struct ntf_work;
@@ -345,6 +355,8 @@ int devm_pse_controller_register(struct device *dev,
 				 struct pse_controller_dev *pcdev);
 int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
 			int irq_flags, const struct pse_irq_desc *d);
+int devm_pse_poll_helper(struct pse_controller_dev *pcdev,
+			 const struct pse_irq_desc *d);
 
 struct pse_control *of_pse_control_get(struct device_node *node,
 				       struct phy_device *phydev);
-- 
2.43.0

Re: [PATCH net-next v4 1/2] net: pse-pd: add devm_pse_poll_helper()

[Jakub Kicinski]

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 devm_pse_poll_helper()

Extract the common event handling loop from pse_isr() into a shared
pse_handle_events() function, and add a generic poll-based alternative
to the IRQ path for PSE controllers that lack interrupt support.

> Work cancellation on teardown is handled via devm_add_action_or_reset()
> to ensure the delayed work is cancelled before poll_notifs is freed
> by devres, avoiding a use-after-free when devm_pse_poll_helper() is
> called after devm_pse_controller_register() (devres LIFO ordering).

Is it safe to rely on this devm registration order to prevent a
use-after-free?

Standard Linux driver initialization typically sets up interrupts and
pollers before device registration to prevent race conditions where an
unready device is exposed to userspace.

If a driver follows this standard ordering by calling the helper before
registration, devres will unregister the controller first. This will
tear down resources in pse_controller_unregister() while the poll work
remains active.

If the worker executes during this race window, could it access the
already-freed pcdev->pi array or push to the destroyed ntf_fifo? Would
it be safer to explicitly cancel asynchronous event sources within
pse_controller_unregister() instead?

> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
[ ... ]
> +static void pse_handle_events(struct pse_controller_dev *pcdev,
> +			      unsigned long *notifs,
> +			      unsigned long notifs_mask)
>  {
> -	struct pse_controller_dev *pcdev;
> -	unsigned long notifs_mask = 0;
> -	struct pse_irq_desc *desc;
> -	struct pse_irq *h = data;
> -	int ret, i;
> -
> -	desc = &h->desc;
> -	pcdev = h->pcdev;
> -
> -	/* Clear notifs mask */
> -	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
> -	mutex_lock(&pcdev->lock);
> -	ret = desc->map_event(irq, pcdev, h->notifs, &notifs_mask);
> -	if (ret || !notifs_mask) {
> -		mutex_unlock(&pcdev->lock);
> -		return IRQ_NONE;
> -	}
> +	int i;
>  
>  	for_each_set_bit(i, &notifs_mask, pcdev->nr_lines) {

Can this lead to an out-of-bounds read on the stack if a controller
supports more than BITS_PER_LONG ports?

The notifs_mask variable is passed by value to pse_handle_events(), but
for_each_set_bit() takes its address. If pcdev->nr_lines is greater than
BITS_PER_LONG (e.g., a 48-port switch on a 32-bit architecture), the
macro will read past the function argument on the stack into uninitialized
memory.

[ ... ]
> +static irqreturn_t pse_isr(int irq, void *data)
> +{
> +	struct pse_controller_dev *pcdev;
> +	unsigned long notifs_mask = 0;
> +	struct pse_irq *h = data;
> +	int ret;
> +
> +	pcdev = h->pcdev;
> +
> +	/* Clear notifs mask */
> +	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
> +	mutex_lock(&pcdev->lock);
> +	ret = h->desc.map_event(irq, pcdev, h->notifs, &notifs_mask);

Could this also result in a stack out-of-bounds write?

If map_event() uses set_bit() to report an event on a port index greater
than or equal to BITS_PER_LONG, it will write past the 4-byte boundary of
the scalar notifs_mask variable, corrupting adjacent stack memory.

[ ... ]
> +static void pse_poll_worker(struct work_struct *work)
> +{
> +	struct pse_controller_dev *pcdev =
> +		container_of(work, struct pse_controller_dev,
> +			     poll_work.work);
> +	unsigned long notifs_mask = 0;
> +	int ret;
> +
> +	memset(pcdev->poll_notifs, 0,
> +	       pcdev->nr_lines * sizeof(*pcdev->poll_notifs));
> +	mutex_lock(&pcdev->lock);
> +	ret = pcdev->poll_desc.map_event(0, pcdev, pcdev->poll_notifs,
> +					 &notifs_mask);

Does this share the same out-of-bounds write risk for notifs_mask as the
IRQ handler above?
-- 
pw-bot: cr

Re: [PATCH net-next v4 1/2] net: pse-pd: add devm_pse_poll_helper()

[Kory Maincent]

On Mon, 13 Apr 2026 15:50:21 -0700
Jakub Kicinski <kuba@kernel.org> wrote:

> 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 devm_pse_poll_helper()
> 
> Extract the common event handling loop from pse_isr() into a shared
> pse_handle_events() function, and add a generic poll-based alternative
> to the IRQ path for PSE controllers that lack interrupt support.
> 
> > Work cancellation on teardown is handled via devm_add_action_or_reset()
> > to ensure the delayed work is cancelled before poll_notifs is freed
> > by devres, avoiding a use-after-free when devm_pse_poll_helper() is
> > called after devm_pse_controller_register() (devres LIFO ordering).  
> 
> Is it safe to rely on this devm registration order to prevent a
> use-after-free?
> 
> Standard Linux driver initialization typically sets up interrupts and
> pollers before device registration to prevent race conditions where an
> unready device is exposed to userspace.
> 
> If a driver follows this standard ordering by calling the helper before
> registration, devres will unregister the controller first. This will
> tear down resources in pse_controller_unregister() while the poll work
> remains active.
> 
> If the worker executes during this race window, could it access the
> already-freed pcdev->pi array or push to the destroyed ntf_fifo? Would
> it be safer to explicitly cancel asynchronous event sources within
> pse_controller_unregister() instead?
> 
> > --- a/drivers/net/pse-pd/pse_core.c
> > +++ b/drivers/net/pse-pd/pse_core.c  
> [ ... ]
> > +static void pse_handle_events(struct pse_controller_dev *pcdev,
> > +			      unsigned long *notifs,
> > +			      unsigned long notifs_mask)
> >  {
> > -	struct pse_controller_dev *pcdev;
> > -	unsigned long notifs_mask = 0;
> > -	struct pse_irq_desc *desc;
> > -	struct pse_irq *h = data;
> > -	int ret, i;
> > -
> > -	desc = &h->desc;
> > -	pcdev = h->pcdev;
> > -
> > -	/* Clear notifs mask */
> > -	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
> > -	mutex_lock(&pcdev->lock);
> > -	ret = desc->map_event(irq, pcdev, h->notifs, &notifs_mask);
> > -	if (ret || !notifs_mask) {
> > -		mutex_unlock(&pcdev->lock);
> > -		return IRQ_NONE;
> > -	}
> > +	int i;
> >  
> >  	for_each_set_bit(i, &notifs_mask, pcdev->nr_lines) {  
> 
> Can this lead to an out-of-bounds read on the stack if a controller
> supports more than BITS_PER_LONG ports?
> 
> The notifs_mask variable is passed by value to pse_handle_events(), but
> for_each_set_bit() takes its address. If pcdev->nr_lines is greater than
> BITS_PER_LONG (e.g., a 48-port switch on a 32-bit architecture), the
> macro will read past the function argument on the stack into uninitialized
> memory.

It's seems there is a possible out-of-bound issue in my code :/ Oops.
Carlo, could you take a look and propose a fix? Otherwise, I'll handle it.

Regards
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net-next v4 1/2] net: pse-pd: add devm_pse_poll_helper()

[Kory Maincent]

On Tue, 14 Apr 2026 16:05:06 +0200
Kory Maincent <kory.maincent@bootlin.com> wrote:

> On Mon, 13 Apr 2026 15:50:21 -0700
> Jakub Kicinski <kuba@kernel.org> wrote:
> 
> > 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 devm_pse_poll_helper()
> > 
> > Extract the common event handling loop from pse_isr() into a shared
> > pse_handle_events() function, and add a generic poll-based alternative
> > to the IRQ path for PSE controllers that lack interrupt support.
> >   
> > > Work cancellation on teardown is handled via devm_add_action_or_reset()
> > > to ensure the delayed work is cancelled before poll_notifs is freed
> > > by devres, avoiding a use-after-free when devm_pse_poll_helper() is
> > > called after devm_pse_controller_register() (devres LIFO ordering).    
> > 
> > Is it safe to rely on this devm registration order to prevent a
> > use-after-free?
> > 
> > Standard Linux driver initialization typically sets up interrupts and
> > pollers before device registration to prevent race conditions where an
> > unready device is exposed to userspace.
> > 
> > If a driver follows this standard ordering by calling the helper before
> > registration, devres will unregister the controller first. This will
> > tear down resources in pse_controller_unregister() while the poll work
> > remains active.
> > 
> > If the worker executes during this race window, could it access the
> > already-freed pcdev->pi array or push to the destroyed ntf_fifo? Would
> > it be safer to explicitly cancel asynchronous event sources within
> > pse_controller_unregister() instead?
> >   
> > > --- a/drivers/net/pse-pd/pse_core.c
> > > +++ b/drivers/net/pse-pd/pse_core.c    
> > [ ... ]  
> > > +static void pse_handle_events(struct pse_controller_dev *pcdev,
> > > +			      unsigned long *notifs,
> > > +			      unsigned long notifs_mask)
> > >  {
> > > -	struct pse_controller_dev *pcdev;
> > > -	unsigned long notifs_mask = 0;
> > > -	struct pse_irq_desc *desc;
> > > -	struct pse_irq *h = data;
> > > -	int ret, i;
> > > -
> > > -	desc = &h->desc;
> > > -	pcdev = h->pcdev;
> > > -
> > > -	/* Clear notifs mask */
> > > -	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
> > > -	mutex_lock(&pcdev->lock);
> > > -	ret = desc->map_event(irq, pcdev, h->notifs, &notifs_mask);
> > > -	if (ret || !notifs_mask) {
> > > -		mutex_unlock(&pcdev->lock);
> > > -		return IRQ_NONE;
> > > -	}
> > > +	int i;
> > >  
> > >  	for_each_set_bit(i, &notifs_mask, pcdev->nr_lines) {    
> > 
> > Can this lead to an out-of-bounds read on the stack if a controller
> > supports more than BITS_PER_LONG ports?
> > 
> > The notifs_mask variable is passed by value to pse_handle_events(), but
> > for_each_set_bit() takes its address. If pcdev->nr_lines is greater than
> > BITS_PER_LONG (e.g., a 48-port switch on a 32-bit architecture), the
> > macro will read past the function argument on the stack into uninitialized
> > memory.  
> 
> It's seems there is a possible out-of-bound issue in my code :/ Oops.
> Carlo, could you take a look and propose a fix? Otherwise, I'll handle it.

But currently it can't be reached as the only driver that support interrupt is
the TPS23881 with 8 ports.

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

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

[Carlo Szelinsky]

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.

Both C33 and PoDL power status and admin state are checked so that LED
triggers work for both controller types.

Trigger names use dev_name(dev) (e.g. "pse-1-003c:port0:delivering")
to ensure uniqueness when multiple PSE controllers are present on the
same system.

Initial LED state is queried at registration time so already-active
ports are reflected immediately without waiting for a hardware event.

LED trigger registration is performed before adding the controller to
the global list, avoiding a race where an IRQ or poll event could
invoke pse_led_update() on a partially initialized trigger.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202603251254.o5PqMBRU-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202603251250.cuMCk5Yv-lkp@intel.com/
Signed-off-by: Carlo Szelinsky <github@szelinsky.de>
---
 drivers/net/pse-pd/pse_core.c | 134 +++++++++++++++++++++++++++++++++-
 include/linux/pse-pd/pse.h    |  22 ++++++
 2 files changed, 154 insertions(+), 2 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index f641a6fa087f..dfc84340afb9 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -12,9 +12,10 @@
 #include <linux/phy.h>
 #include <linux/pse-pd/pse.h>
 #include <linux/regulator/driver.h>
+#include <linux/leds.h>
+#include <linux/workqueue.h>
 #include <linux/regulator/machine.h>
 #include <linux/rtnetlink.h>
-#include <linux/workqueue.h>
 #include <net/net_trackers.h>
 
 #define PSE_PW_D_LIMIT INT_MAX
@@ -670,6 +671,111 @@ static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_LEDS_TRIGGERS)
+/**
+ * pse_led_update - Update LED triggers for a PI based on current state
+ * @pcdev: PSE controller device
+ * @id: PI index
+ *
+ * Queries the current power status and admin state of the PI and
+ * fires LED trigger events on state changes. Called from the
+ * notification path and the regulator enable/disable paths.
+ *
+ * Must be called with pcdev->lock held.
+ */
+static void pse_led_update(struct pse_controller_dev *pcdev, int id)
+{
+	struct pse_pi_led_triggers *trigs;
+	struct pse_pw_status pw_status = {};
+	struct pse_admin_state admin_state = {};
+	bool delivering, enabled;
+
+	if (!pcdev->pi_led_trigs)
+		return;
+
+	trigs = &pcdev->pi_led_trigs[id];
+	if (!trigs->delivering.name)
+		return;
+
+	if (pcdev->ops->pi_get_pw_status(pcdev, id, &pw_status))
+		return;
+	if (pcdev->ops->pi_get_admin_state(pcdev, id, &admin_state))
+		return;
+
+	delivering = pw_status.c33_pw_status ==
+		ETHTOOL_C33_PSE_PW_D_STATUS_DELIVERING ||
+		pw_status.podl_pw_status ==
+		ETHTOOL_PODL_PSE_PW_D_STATUS_DELIVERING;
+	enabled = admin_state.c33_admin_state ==
+		ETHTOOL_C33_PSE_ADMIN_STATE_ENABLED ||
+		admin_state.podl_admin_state ==
+		ETHTOOL_PODL_PSE_ADMIN_STATE_ENABLED;
+
+	if (trigs->last_delivering != delivering) {
+		trigs->last_delivering = delivering;
+		led_trigger_event(&trigs->delivering,
+				  delivering ? LED_FULL : LED_OFF);
+	}
+
+	if (trigs->last_enabled != enabled) {
+		trigs->last_enabled = enabled;
+		led_trigger_event(&trigs->enabled,
+				  enabled ? LED_FULL : LED_OFF);
+	}
+}
+
+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);
+		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;
+}
+#else
+static inline void pse_led_update(struct pse_controller_dev *pcdev, int id) {}
+static int pse_led_triggers_register(struct pse_controller_dev *pcdev)
+{
+	return 0;
+}
+#endif /* CONFIG_LEDS_TRIGGERS */
+
 static int pse_pi_enable(struct regulator_dev *rdev)
 {
 	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);
@@ -695,6 +801,7 @@ static int pse_pi_enable(struct regulator_dev *rdev)
 			pcdev->pi[id].admin_state_enabled = 1;
 			ret = 0;
 		}
+		pse_led_update(pcdev, id);
 		mutex_unlock(&pcdev->lock);
 		return ret;
 	}
@@ -702,6 +809,7 @@ static int pse_pi_enable(struct regulator_dev *rdev)
 	ret = ops->pi_enable(pcdev, id);
 	if (!ret)
 		pcdev->pi[id].admin_state_enabled = 1;
+	pse_led_update(pcdev, id);
 	mutex_unlock(&pcdev->lock);
 
 	return ret;
@@ -719,6 +827,7 @@ static int pse_pi_disable(struct regulator_dev *rdev)
 	ret = _pse_pi_disable(pcdev, id);
 	if (!ret)
 		pi->admin_state_enabled = 0;
+	pse_led_update(pcdev, id);
 
 	mutex_unlock(&pcdev->lock);
 	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);
+	}
+
+	/* 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);
+	}
+
 	mutex_lock(&pse_list_mutex);
 	list_add(&pcdev->list, &pse_controller_list);
 	mutex_unlock(&pse_list_mutex);
@@ -1265,7 +1388,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);
+
+		/* Skip regulator/netlink path for PIs without consumers */
 		if (!pcdev->pi[i].rdev)
 			continue;
 
diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h
index 44d5d10e239d..0058636a6299 100644
--- a/include/linux/pse-pd/pse.h
+++ b/include/linux/pse-pd/pse.h
@@ -10,6 +10,7 @@
 #include <linux/kfifo.h>
 #include <uapi/linux/ethtool.h>
 #include <uapi/linux/ethtool_netlink_generated.h>
+#include <linux/leds.h>
 #include <linux/regulator/driver.h>
 
 /* Maximum current in uA according to IEEE 802.3-2022 Table 145-1 */
@@ -266,6 +267,23 @@ struct pse_pi {
 	int pw_allocated_mW;
 };
 
+#if IS_ENABLED(CONFIG_LEDS_TRIGGERS)
+/**
+ * struct pse_pi_led_triggers - LED trigger state for a PSE PI
+ *
+ * @delivering: LED trigger for power delivering state
+ * @enabled: LED trigger for admin enabled state
+ * @last_delivering: cached delivering state for change detection
+ * @last_enabled: cached enabled state for change detection
+ */
+struct pse_pi_led_triggers {
+	struct led_trigger delivering;
+	struct led_trigger enabled;
+	bool last_delivering;
+	bool last_enabled;
+};
+#endif
+
 /**
  * struct pse_ntf - PSE notification element
  *
@@ -303,6 +321,7 @@ struct pse_ntf {
  * @ntf_work: workqueue for PSE notification management
  * @ntf_fifo: PSE notifications FIFO
  * @ntf_fifo_lock: protect @ntf_fifo writer
+ * @pi_led_trigs: per-PI LED trigger state array
  */
 struct pse_controller_dev {
 	const struct pse_controller_ops *ops;
@@ -327,6 +346,9 @@ struct pse_controller_dev {
 	struct work_struct ntf_work;
 	DECLARE_KFIFO_PTR(ntf_fifo, struct pse_ntf);
 	spinlock_t ntf_fifo_lock; /* Protect @ntf_fifo writer */
+#if IS_ENABLED(CONFIG_LEDS_TRIGGERS)
+	struct pse_pi_led_triggers *pi_led_trigs;
+#endif
 };
 
 /**
-- 
2.43.0

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

[Jakub Kicinski]

On Fri, 10 Apr 2026 14:44:28 +0200 Carlo Szelinsky wrote:
> @@ -266,6 +267,23 @@ struct pse_pi {
>  	int pw_allocated_mW;
>  };
>  
> +#if IS_ENABLED(CONFIG_LEDS_TRIGGERS)
> +/**
> + * struct pse_pi_led_triggers - LED trigger state for a PSE PI
> + *
> + * @delivering: LED trigger for power delivering state
> + * @enabled: LED trigger for admin enabled state
> + * @last_delivering: cached delivering state for change detection
> + * @last_enabled: cached enabled state for change detection
> + */
> +struct pse_pi_led_triggers {
> +	struct led_trigger delivering;
> +	struct led_trigger enabled;
> +	bool last_delivering;
> +	bool last_enabled;
> +};
> +#endif
> +
>  /**

avoid wrapping type definitions in ifdefs.
This only forces other code to also be wrapped in ifdefs.

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

[Jakub Kicinski]

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);