net: pse-pd: add poll path and LED trigger support

PATCH Superseded Carlo Szelinsky 4 participants 19 messages Apr 10 – May 27

Subsystems (4)

Networking Network drivers LED PSE network driver

lore

Share & tools

lore

Carlo Szelinsky

author

2026-04-10 12:44 UTC #0 lore raw

v4 cover net: pse-pd: add poll path and LED trigger support

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/[email protected]/ open here

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

Signature · 1 line

2.43.0

Carlo Szelinsky

author

2026-04-10 12:44 UTC #1 lore raw

PATCH v4 1/2 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 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 <[email protected]>

---
 drivers/net/pse-pd/pse_core.c | 164 +++++++++++++++++++++++++++-------
 include/linux/pse-pd/pse.h    |  12 +++
 2 files changed, 146 insertions(+), 30 deletions(-)

drivers/net/pse-pd/pse_core.c +134 ~~-30~~

index 3beaaaeec9e1..f641a6fa087f 100644
@@ -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)

include/linux/pse-pd/pse.h +12 -0

index 4e5696cfade7..44d5d10e239d 100644
@@ -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);

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2026-04-13 22:53 UTC #2 lore raw

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

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

Quoted 4 lines

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?

Quoted 2 lines

drivers/net/pse-pd/pse_core.c +0 -0

[ ... ]

Quoted 14 lines

drivers/net/pse-pd/pse_core.c +3 ~~-10~~

+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 */

drivers/net/pse-pd/pse_core.c static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id, · line 1263

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +1 -7

-	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.

[ ... ]

drivers/net/pse-pd/pse_core.c static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id,

Quoted 13 lines

drivers/net/pse-pd/pse_core.c +13 -0

+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.

[ ... ]

drivers/net/pse-pd/pse_core.c static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id, · line 1337

Quoted 13 lines

drivers/net/pse-pd/pse_core.c +13 -0

+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?

Signature · 1 line

pw-bot: cr

Kory Maincent

maintainer

2026-04-14 14:05 UTC #3 lore raw

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

Quoted 62 lines · On Mon, 13 Apr 2026 15:50:21 -0700 Jakub Kicinski <[email protected]> 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.
Quoted 4 lines
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?
Quoted 2 lines
drivers/net/pse-pd/pse_core.c +0 -0
[ ... ]
Quoted 14 lines
drivers/net/pse-pd/pse_core.c +3 ~~-10~~
+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 */
drivers/net/pse-pd/pse_core.c static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id, · line 1263
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +1 -7
-	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?

Quoted 5 lines

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

Signature · 3 lines

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

Kory Maincent

maintainer

2026-04-14 14:11 UTC #4 lore raw

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

Quoted 65 lines · On Tue, 14 Apr 2026 16:05:06 +0200 Kory Maincent <[email protected]>…

Quoted 62 lines · On Mon, 13 Apr 2026 15:50:21 -0700 Jakub Kicinski <[email protected]> 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.
Quoted 4 lines
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?
Quoted 2 lines
drivers/net/pse-pd/pse_core.c +0 -0
[ ... ]
Quoted 14 lines
drivers/net/pse-pd/pse_core.c +3 ~~-10~~
+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 */
drivers/net/pse-pd/pse_core.c static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id, · line 1263
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +1 -7
-	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?

Quoted 8 lines

Quoted 5 lines
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,

Signature · 3 lines

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

Carlo Szelinsky

author

2026-05-23 19:29 UTC #5 lore raw

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

Hi Kory,

Sorry for the late reply. Too many different track and I got lost.

Just to close the loop on the notifs_mask OOB:
v5 1/2 ("net: pse-pd: add devm_pse_poll_helper()", sent Apr 29) extends your bitmap pattern from 5099807f335c to pse_handle_events() and pse_poll_worker(). Storage moved to notifs_mask pointers backed by devm_bitmap_zalloc, with bitmap_zero / bitmap_empty / for_each_set_bit on real pointers.

So the new code in v5 doesn't reintroduce the OOB.

Does this make sense?

Cheers,
Carlo

Kory Maincent

maintainer

2026-05-27 08:32 UTC #6 lore raw

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

Hello Carlo,

Quoted 11 lines · On Sat, 23 May 2026 21:20:27 +0200 Carlo Szelinsky <[email protected]> wrote:

Hi Kory,
Sorry for the late reply. Too many different track and I got lost.
Just to close the loop on the notifs_mask OOB:
v5 1/2 ("net: pse-pd: add devm_pse_poll_helper()", sent Apr 29) extends your bitmap pattern from 5099807f335c to pse_handle_events() and pse_poll_worker(). Storage moved to notifs_mask pointers backed by devm_bitmap_zalloc, with bitmap_zero / bitmap_empty / for_each_set_bit on real pointers.

Quoted 3 lines

So the new code in v5 doesn't reintroduce the OOB.
Does this make sense?

This does.
Sorry I didn't have the time to take a look at you v5.

Regards,

Signature · 3 lines

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

Carlo Szelinsky

author

2026-04-10 12:45 UTC #7 lore raw

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

In reply to 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 <[email protected]>

Closes https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here

Signed-off-by Carlo Szelinsky <[email protected]>

---
 drivers/net/pse-pd/pse_core.c | 134 +++++++++++++++++++++++++++++++++-
 include/linux/pse-pd/pse.h    |  22 ++++++
 2 files changed, 154 insertions(+), 2 deletions(-)

drivers/net/pse-pd/pse_core.c +132 -2

index f641a6fa087f..dfc84340afb9 100644
@@ -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;

include/linux/pse-pd/pse.h +22 -0

index 44d5d10e239d..0058636a6299 100644
@@ -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
 };
 
 /**

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2026-04-13 22:51 UTC #8 lore raw

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

Quoted 12 lines · On Fri, 10 Apr 2026 14:44:28 +0200 Carlo Szelinsky wrote:

include/linux/pse-pd/pse.h +8 -0

@@ -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

include/linux/pse-pd/pse.h struct pse_pi { · line 287

Quoted 10 lines

include/linux/pse-pd/pse.h +9 -0

+ */
+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.

Jakub Kicinski

maintainer

2026-04-13 22:53 UTC #9 lore raw

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

In reply to Carlo Szelinsky

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

Link https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here

Quoted 4 lines

drivers/net/pse-pd/pse_core.c +0 -0
index f641a6fa087fb..dfc84340afb9d 100644

[ ... ]

Quoted 18 lines

drivers/net/pse-pd/pse_core.c +18 -0

+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 */

drivers/net/pse-pd/pse_core.c static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev, · line 754

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +10 -0

+		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.

Quoted 16 lines

drivers/net/pse-pd/pse_core.c +16 -0

+		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;
+}

[ ... ]

drivers/net/pse-pd/pse_core.c int pse_controller_register(struct pse_controller_dev *pcdev) · line 1224

Quoted 9 lines

drivers/net/pse-pd/pse_core.c +5 -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?

drivers/net/pse-pd/pse_core.c int pse_controller_register(struct pse_controller_dev *pcdev) · line 1232

Quoted 8 lines

drivers/net/pse-pd/pse_core.c +8 -0

+
+	/* 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.

Quoted 4 lines

drivers/net/pse-pd/pse_core.c +1 -0

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

Carlo Szelinsky

author

2026-04-29 21:32 UTC #10 lore raw

v5 cover net: pse-pd: add poll path and LED trigger support

In reply to Carlo Szelinsky

Thanks to Kory, Oleksij, Krzysztof, Andrew and Jakub for all the helpful feedback on earlier versions:-) I really appreciate the time you put into reviewing this. I hope this is now the last round ;-)

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: while reworking the poll teardown for v5, I noticed pse_release_pis() (which kfree()s the pi[] array) runs before disable_irq() / cancel_delayed_work_sync() in pse_controller_unregister(). A concurrent IRQ or poll worker that fires in that window would read through a freed pi[]. This is pre-existing for the IRQ path (since fc0e6db30941, "Add support for reporting events"); the v5 poll cancel inherits the same placement for symmetry. Does it make sense that I send a separate fix that disables both async sources before pse_release_pis() or am I wrong here?

Changes since v4:
- Rebased on top of "net: pse-pd: fix out-of-bounds bitmap access in pse_isr() on 32-bit" (5099807f335c, merged via net). Extended the same bitmap-pointer pattern to the new pse_handle_events() and pse_poll_worker() code paths so for_each_set_bit() does not read past a single unsigned long when nr_lines > BITS_PER_LONG (Jakub, Kory)
- Cancel the poll work explicitly in pse_controller_unregister(), next to the existing disable_irq() for the IRQ path, instead of relying on devm_add_action_or_reset() LIFO ordering. Makes the helper safe even when a driver registers it in the standard order (helper before devm_pse_controller_register()) (Jakub)
- struct pse_pi_led_triggers no longer wrapped in
#if IS_ENABLED(CONFIG_LEDS_TRIGGERS); only the function bodies remain ifdef'd, so reference sites no longer need their own ifdefs (Jakub)
- Added .activate callbacks for both LED triggers. Without these, an LED bound to a trigger after pse_controller_register() (e.g. via sysfs) would stay dark until the next hardware event toggled state (Jakub)
- Run the post-registration initial-state pass under pcdev->lock, matching pse_led_update()'s documented locking contract and avoiding races with concurrent regulator_enable() that share last_delivering / last_enabled and the hardware ops (Jakub)
- On partial pse_led_triggers_register() failure, NULL out pcdev->pi_led_trigs so the existing early-return guard in pse_led_update() short-circuits any later calls onto partially-registered triggers (Jakub)

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/[email protected]/ open here

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 | 355 +++++++++++++++++++++++++++++++---
 include/linux/pse-pd/pse.h    |  32 +++
 2 files changed, 356 insertions(+), 31 deletions(-)

base-commit: 09942ddedcb960f9e78fd817ec33f501d1040c5b --
2.43.0

Carlo Szelinsky

author

2026-04-29 21:33 UTC #11 lore raw

PATCH v5 1/2 net: pse-pd: add devm_pse_poll_helper()

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.

Cancel the poll work explicitly in pse_controller_unregister() (next to the existing disable_irq() for the IRQ path) so it cannot outlive the controller it pushes events into. This avoids relying on devres LIFO ordering between devm_pse_controller_register() and devm_pse_poll_helper() when a driver follows the standard pattern of setting up the helper before registering the controller.

The notifs_mask is allocated as a real bitmap via devm_bitmap_zalloc() and passed to pse_handle_events() and the driver's map_event callback as a pointer (mirroring the pattern from commit 5099807f335c ("net: pse-pd: fix out-of-bounds bitmap access in pse_isr() on 32-bit")) so for_each_set_bit() does not read past a single unsigned long when nr_lines > BITS_PER_LONG.

The poll interval defaults to 500ms, balancing responsiveness against bus load (e.g. I2C).

Signed-off-by Carlo Szelinsky <[email protected]>

---
 drivers/net/pse-pd/pse_core.c | 157 +++++++++++++++++++++++++++-------
 include/linux/pse-pd/pse.h    |  14 +++
 2 files changed, 140 insertions(+), 31 deletions(-)

drivers/net/pse-pd/pse_core.c +126 ~~-31~~

index 87aa4f4e9724..b7ffec0c942c 100644
@@ -14,10 +14,17 @@
 #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);

@@ -1118,6 +1125,8 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 	pse_release_pis(pcdev);
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
+	if (pcdev->polling)
+		cancel_delayed_work_sync(&pcdev->poll_work);
 	cancel_work_sync(&pcdev->ntf_work);
 	kfifo_free(&pcdev->ntf_fifo);
 	mutex_lock(&pse_list_mutex);

@@ -1239,66 +1248,104 @@ 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: bitmap of PIs with events (sized for pcdev->nr_lines)
  *
- * 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;
-	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));
-	bitmap_zero(h->notifs_mask, pcdev->nr_lines);
-	mutex_lock(&pcdev->lock);
-	ret = desc->map_event(irq, pcdev, h->notifs, h->notifs_mask);
-	if (ret || bitmap_empty(h->notifs_mask, pcdev->nr_lines)) {
-		mutex_unlock(&pcdev->lock);
-		return IRQ_NONE;
-	}
+	int i;
 
-	for_each_set_bit(i, h->notifs_mask, pcdev->nr_lines) {
-		unsigned long notifs, rnotifs;
+	for_each_set_bit(i, notifs_mask, pcdev->nr_lines) {
+		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;
+	struct pse_irq *h = data;
+	int ret;
+
+	pcdev = h->pcdev;
 
+	/* Clear notifs mask */
+	memset(h->notifs, 0, pcdev->nr_lines * sizeof(*h->notifs));
+	bitmap_zero(h->notifs_mask, pcdev->nr_lines);
+	mutex_lock(&pcdev->lock);
+	ret = h->desc.map_event(irq, pcdev, h->notifs, h->notifs_mask);
+	if (ret || bitmap_empty(h->notifs_mask, pcdev->nr_lines)) {
+		mutex_unlock(&pcdev->lock);
+		return IRQ_NONE;
+	}
+
+	pse_handle_events(pcdev, h->notifs, h->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);
+	int ret;
+
+	memset(pcdev->poll_notifs, 0,
+	       pcdev->nr_lines * sizeof(*pcdev->poll_notifs));
+	bitmap_zero(pcdev->poll_notifs_mask, pcdev->nr_lines);
+	mutex_lock(&pcdev->lock);
+	ret = pcdev->poll_desc.map_event(0, pcdev, pcdev->poll_notifs,
+					 pcdev->poll_notifs_mask);
+	if (!ret && !bitmap_empty(pcdev->poll_notifs_mask, pcdev->nr_lines))
+		pse_handle_events(pcdev, pcdev->poll_notifs,
+				  pcdev->poll_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

@@ -1356,6 +1403,54 @@ int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
 }
 EXPORT_SYMBOL_GPL(devm_pse_irq_helper);
 
+/**
+ * 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;
+
+	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;
+
+	pcdev->poll_notifs_mask = devm_bitmap_zalloc(dev, pcdev->nr_lines,
+						     GFP_KERNEL);
+	if (!pcdev->poll_notifs_mask)
+		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;
+
+	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)

include/linux/pse-pd/pse.h +14 -0

index 4e5696cfade7..4b5d5b11a084 100644
@@ -292,6 +292,12 @@ 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
+ * @poll_notifs_mask: bitmap of PIs with events 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 +318,12 @@ 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 long *poll_notifs_mask;
 	unsigned int pis_prio_max;
 	u32 supp_budget_eval_strategies;
 	struct work_struct ntf_work;

@@ -345,6 +357,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);

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2026-05-05 01:57 UTC #12 lore raw

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

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

This patch extracts the common event handling loop from pse_isr() into a shared function and adds a poll-based alternative for PSE controllers that lack interrupt support.

Quoted 4 lines

drivers/net/pse-pd/pse_core.c +0 -0
index 87aa4f4e9724..b7ffec0c942c 100644

[ ... ]

drivers/net/pse-pd/pse_core.c void pse_controller_unregister(struct pse_controller_dev *pcdev) · line 1129

Quoted 6 lines

drivers/net/pse-pd/pse_core.c +2 -0

@@ -1118,6 +1125,8 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 	pse_release_pis(pcdev);
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
+	if (pcdev->polling)
+		cancel_delayed_work_sync(&pcdev->poll_work);

Because pse_release_pis() executes kfree(pcdev->pi) before the delayed work is cancelled, can pse_poll_worker() run concurrently and access the freed pcdev->pi array inside pse_handle_events() before the cancellation completes?

[ ... ]

Quoted 8 lines

drivers/net/pse-pd/pse_core.c +4 -0

@@ -1356,6 +1403,54 @@ int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
 }
 EXPORT_SYMBOL_GPL(devm_pse_irq_helper);
 
+/**
+ * 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)

[ ... ]

Quoted 18 lines

drivers/net/pse-pd/pse_core.c +18 -0

+int devm_pse_poll_helper(struct pse_controller_dev *pcdev,
+			 const struct pse_irq_desc *d)
+{
+	struct device *dev = pcdev->dev;
+
+	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;
+
+	pcdev->poll_notifs_mask = devm_bitmap_zalloc(dev, pcdev->nr_lines,
+						     GFP_KERNEL);
+	if (!pcdev->poll_notifs_mask)

drivers/net/pse-pd/pse_core.c int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq, · line 1448

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +10 -0

+		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;
+
+	queue_delayed_work(system_freezable_wq, &pcdev->poll_work,
+			   msecs_to_jiffies(pcdev->poll_interval_ms));

Since memory is allocated with devm_ helpers but no devm cleanup action is registered to cancel the delayed work, what happens if a driver's probe function encounters an error after calling this helper? Will devres free the pcdev structure while the system workqueue still has the delayed work armed, leading to a use-after-free when the timer expires?

The commit message mentions that drivers typically set up the helper before registering the controller. If queue_delayed_work() schedules the worker to run before devm_pse_controller_register() completes, will pse_poll_worker() attempt to acquire pcdev->lock and push events into pcdev->ntf_fifo before they are initialized?

Quoted 4 lines

drivers/net/pse-pd/pse_core.c +4 -0
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(devm_pse_poll_helper);

Carlo Szelinsky

author

2026-05-16 10:24 UTC #13 lore raw

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

Hi Jakub,

Thanks for sending the AI review. I checked the code again... both points are real, but one of them looks to me like more theory than practice. Out of curiosity, what prompt do you use for these reviews, so I can use it before sending the next version? Replying inline:

Quoted 10 lines · On Mon, 4 May 2026 18:57:57 -0700 Jakub Kicinski wrote:

drivers/net/pse-pd/pse_core.c void pse_controller_unregister(struct pse_controller_dev *pcdev) · line 1129
Quoted 6 lines
drivers/net/pse-pd/pse_core.c +2 -0
@@ -1118,6 +1125,8 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 	pse_release_pis(pcdev);
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
+	if (pcdev->polling)
+		cancel_delayed_work_sync(&pcdev->poll_work);
Because pse_release_pis() executes kfree(pcdev->pi) before the delayed work is cancelled, can pse_poll_worker() run concurrently and access the freed pcdev->pi array inside pse_handle_events() before the cancellation completes?

Yes, seems like a real bug. The IRQ thread and the poll worker can still run after pse_release_pis() frees pcdev->pi, and both end up in pse_handle_events() touching pcdev->pi[i]. We need to stop them first. v6 will reorder pse_controller_unregister() to:

	if (pcdev->irq)
		disable_irq(pcdev->irq);
	if (pcdev->polling)
		cancel_delayed_work_sync(&pcdev->poll_work);
	cancel_work_sync(&pcdev->ntf_work);
	pse_release_pis(pcdev);

The IRQ side seem to be already broken before this patch. I went through the git log:

* fc0e6db30941 ("net: pse-pd: Add support for reporting events") added pse_isr but never called disable_irq() in unregister at all.
* ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies") added the disable_irq() call, but put it in the wrong place (after pse_release_pis()).

What do you suggest to do, I could imagine: ...

1) Send the fix as a standalone "Fixes:" patch to net first, then rebase v6 on top once it lands? Or fold it into v6 as patch 1/3? My vote is standalone since the fix stands on its own, but up to you.

2) Which commit for the Fixes: tag? I'd pick ffef61d6d273 (that's where the broken order came from), but fc0e6db30941 also works (no protection at all back then). wdyt?

Quoted 11 lines

Quoted 3 lines
drivers/net/pse-pd/pse_core.c +3 -0
+int devm_pse_poll_helper(struct pse_controller_dev *pcdev,
+			 const struct pse_irq_desc *d)
+{
[...]
Review
Quoted 2 lines
(quoted diff) +2 -0
+	queue_delayed_work(system_freezable_wq, &pcdev->poll_work,
+			   msecs_to_jiffies(pcdev->poll_interval_ms));
Since memory is allocated with devm_ helpers but no devm cleanup action is registered to cancel the delayed work, what happens if a driver's probe function encounters an error after calling this helper? [...]

Quoted 5 lines

The commit message mentions that drivers typically set up the helper before registering the controller. If queue_delayed_work() schedules the worker to run before devm_pse_controller_register() completes, will pse_poll_worker() attempt to acquire pcdev->lock and push events into pcdev->ntf_fifo before they are initialized?

The probe-failure case is a real bug. If probe fails between this helper and devm_pse_controller_register(), the work stays queued and fires later on a pcdev whose devres memory is gone. UAF.

The "worker fires before mutex/kfifo are ready" case is real in theory, but I don't think you can actually hit it?! Default poll_interval_ms is 1000 ms, so pse_controller_register() would need to take over a second to lose the race. Worth fixing to keep the code clean, but not urgent or?

Same fix for both: move queue_delayed_work() out of the helper. The helper just allocates the state and does INIT_DELAYED_WORK. pse_controller_register() then arms the work as the very last step on success, gated on pcdev->polling. So the work only ever runs when everything is set up AND register has succeeded.

Should I fix both in v6, or just the probe-failure leak? One change covers both, so I would suggest to do both. wdyt?

So the steps for v6 [1/2] could be:
* standalone "Fixes:" patch reordering pse_controller_unregister() (sent to net first, unless you suggest to fold it in)
* move queue_delayed_work() into pse_controller_register()

Do you have any other feedback, do I miss anything ?

Cheers,
Carlo

Oleksij Rempel

maintainer

2026-05-17 09:57 UTC #14 lore raw

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

Quoted 6 lines · Carlo Szelinsky

Hi Jakub,
Thanks for sending the AI review. I checked the code again... both points are real, but one of them looks to me like more theory than practice. Out of curiosity, what prompt do you use for these reviews, so I can use it before sending the next version?

Sashiko is using https://github.com/masoncl/review-prompts

I personally run pre-review with opencode + review-prompts +
gemini-3.1-pro-preview-customtools / or claude-sonnet-4-5

After installing review-prompts to opencode config the review can be done with "/kreview commit_id"

But sashiko still finds things, i do not know what is the difference.

Quoted 35 lines

Replying inline:
Quoted 10 lines · On Mon, 4 May 2026 18:57:57 -0700 Jakub Kicinski wrote:
drivers/net/pse-pd/pse_core.c void pse_controller_unregister(struct pse_controller_dev *pcdev) · line 1129
Quoted 6 lines
drivers/net/pse-pd/pse_core.c +2 -0
@@ -1118,6 +1125,8 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
 	pse_release_pis(pcdev);
 	if (pcdev->irq)
 		disable_irq(pcdev->irq);
+	if (pcdev->polling)
+		cancel_delayed_work_sync(&pcdev->poll_work);
Because pse_release_pis() executes kfree(pcdev->pi) before the delayed work is cancelled, can pse_poll_worker() run concurrently and access the freed pcdev->pi array inside pse_handle_events() before the cancellation completes?
Yes, seems like a real bug. The IRQ thread and the poll worker can still run after pse_release_pis() frees pcdev->pi, and both end up in pse_handle_events() touching pcdev->pi[i]. We need to stop them first. v6 will reorder pse_controller_unregister() to:
	if (pcdev->irq)
		disable_irq(pcdev->irq);
	if (pcdev->polling)
		cancel_delayed_work_sync(&pcdev->poll_work);
	cancel_work_sync(&pcdev->ntf_work);
	pse_release_pis(pcdev);
The IRQ side seem to be already broken before this patch. I went through the git log:
* fc0e6db30941 ("net: pse-pd: Add support for reporting events") added pse_isr but never called disable_irq() in unregister at all.
* ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies") added the disable_irq() call, but put it in the wrong place (after pse_release_pis()).

Quoted 6 lines

What do you suggest to do, I could imagine: ...
1) Send the fix as a standalone "Fixes:" patch to net first, then rebase v6 on top once it lands? Or fold it into v6 as patch 1/3? My vote is standalone since the fix stands on its own, but up to you.

Yes, please send the fix to stable. In this case you will need to wait until net-next includes stable fixes.

Quoted 3 lines

2) Which commit for the Fixes: tag? I'd pick ffef61d6d273 (that's where the broken order came from), but fc0e6db30941 also works (no protection at all back then). wdyt?

If it is introduce by ffef61d6d273, then use this for Fixes.

Quoted 21 lines

Quoted 11 lines
Quoted 3 lines
drivers/net/pse-pd/pse_core.c +3 -0
+int devm_pse_poll_helper(struct pse_controller_dev *pcdev,
+			 const struct pse_irq_desc *d)
+{
[...]
Review
Quoted 2 lines
(quoted diff) +2 -0
+	queue_delayed_work(system_freezable_wq, &pcdev->poll_work,
+			   msecs_to_jiffies(pcdev->poll_interval_ms));
Since memory is allocated with devm_ helpers but no devm cleanup action is registered to cancel the delayed work, what happens if a driver's probe function encounters an error after calling this helper? [...]
Quoted 5 lines
The commit message mentions that drivers typically set up the helper before registering the controller. If queue_delayed_work() schedules the worker to run before devm_pse_controller_register() completes, will pse_poll_worker() attempt to acquire pcdev->lock and push events into pcdev->ntf_fifo before they are initialized?
The probe-failure case is a real bug. If probe fails between this helper and devm_pse_controller_register(), the work stays queued and fires later on a pcdev whose devres memory is gone. UAF.

Quoted 5 lines

The "worker fires before mutex/kfifo are ready" case is real in theory, but I don't think you can actually hit it?! Default poll_interval_ms is 1000 ms, so pse_controller_register() would need to take over a second to lose the race. Worth fixing to keep the code clean, but not urgent or?

The probe() is a process and can be outscheduled by any high prio process at any time. May be on a single core SoC with pse-pd core build as module and squashfs root it will make the boot load heavy enough to reproduce it?

Quoted 5 lines

Same fix for both: move queue_delayed_work() out of the helper. The helper just allocates the state and does INIT_DELAYED_WORK. pse_controller_register() then arms the work as the very last step on success, gated on pcdev->polling. So the work only ever runs when everything is set up AND register has succeeded.

Sounds good.

Quoted 2 lines

Should I fix both in v6, or just the probe-failure leak? One change covers both, so I would suggest to do both. wdyt?

Fixes for pre-existing bugs to net, new bugs to net-next :)

Best Regards,
Oleksij

Signature · 4 lines

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Carlo Szelinsky

author

2026-04-29 21:33 UTC #15 lore raw

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

In reply to Carlo Szelinsky

Each trigger registers an .activate callback that syncs a freshly-bound LED to the cached state, so an LED bound after pse_controller_register() (e.g. via sysfs) does not stay dark until the next hardware event.

The post-registration initial-state pass is run under pcdev->lock, matching pse_led_update()'s documented locking contract and avoiding races with concurrent regulator_enable() paths that share last_delivering / last_enabled and the hardware ops.

If pse_led_triggers_register() fails partway through, the partially- registered triggers are detached by clearing pcdev->pi_led_trigs, so the existing early-return guard in pse_led_update() is the single point of truth and a stale led_trigger.name cannot fool the dereference path into touching an unregistered trigger.

The per-PI trigger struct lives outside the CONFIG_LEDS_TRIGGERS ifdef to avoid forcing every reference site to be wrapped; the LED-specific code paths remain ifdef'd.

Link https://lore.kernel.org/oe-kbuild-all/[email protected]/ open here

Signed-off-by Carlo Szelinsky <[email protected]>

---
 drivers/net/pse-pd/pse_core.c | 200 +++++++++++++++++++++++++++++++++-
 include/linux/pse-pd/pse.h    |  18 +++
 2 files changed, 217 insertions(+), 1 deletion(-)

drivers/net/pse-pd/pse_core.c +199 -1

index b7ffec0c942c..bff97211ae22 100644
@@ -8,6 +8,7 @@
 #include <linux/device.h>
 #include <linux/ethtool.h>
 #include <linux/ethtool_netlink.h>
+#include <linux/leds.h>
 #include <linux/of.h>
 #include <linux/phy.h>
 #include <linux/pse-pd/pse.h>

@@ -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)
+/**
+ * pse_pi_get_states - Fetch current delivering/enabled state for a PI
+ * @pcdev: PSE controller device
+ * @id: PI index
+ * @delivering: out, set to true if PI is currently delivering power
+ * @enabled: out, set to true if PI is administratively enabled
+ *
+ * Queries hardware via the controller ops. Caller must hold pcdev->lock.
+ *
+ * Return: 0 on success, negative errno on failure.
+ */
+static int pse_pi_get_states(struct pse_controller_dev *pcdev, int id,
+			     bool *delivering, bool *enabled)
+{
+	struct pse_pw_status pw_status = {};
+	struct pse_admin_state admin_state = {};
+	int ret;
+
+	ret = pcdev->ops->pi_get_pw_status(pcdev, id, &pw_status);
+	if (ret)
+		return ret;
+	ret = pcdev->ops->pi_get_admin_state(pcdev, id, &admin_state);
+	if (ret)
+		return ret;
+
+	*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;
+
+	return 0;
+}
+
+/**
+ * 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;
+	bool delivering, enabled;
+
+	if (!pcdev->pi_led_trigs)
+		return;
+
+	trigs = &pcdev->pi_led_trigs[id];
+	if (!trigs->delivering.name)
+		return;
+
+	if (pse_pi_get_states(pcdev, id, &delivering, &enabled))
+		return;
+
+	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);
+	}
+}
+
+/* Sync a freshly-bound LED to the cached trigger state. Without these
+ * .activate callbacks, an LED bound to the trigger after
+ * pse_controller_register() (e.g. via sysfs) would stay dark until the
+ * next hardware event toggles state.
+ */
+static int pse_led_delivering_activate(struct led_classdev *led_cdev)
+{
+	struct pse_pi_led_triggers *trigs =
+		container_of(led_cdev->trigger, struct pse_pi_led_triggers,
+			     delivering);
+
+	led_set_brightness(led_cdev,
+			   trigs->last_delivering ? LED_FULL : LED_OFF);
+	return 0;
+}
+
+static int pse_led_enabled_activate(struct led_classdev *led_cdev)
+{
+	struct pse_pi_led_triggers *trigs =
+		container_of(led_cdev->trigger, struct pse_pi_led_triggers,
+			     enabled);
+
+	led_set_brightness(led_cdev,
+			   trigs->last_enabled ? LED_FULL : LED_OFF);
+	return 0;
+}
+
+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;
+		trigs->delivering.activate = pse_led_delivering_activate;
+
+		ret = devm_led_trigger_register(dev, &trigs->delivering);
+		if (ret) {
+			trigs->delivering.name = NULL;
+			return ret;
+		}
+
+		trigs->enabled.name = devm_kasprintf(dev, GFP_KERNEL,
+						     "pse-%s:port%d:enabled",
+						     dev_id, i);
+		if (!trigs->enabled.name)
+			return -ENOMEM;
+		trigs->enabled.activate = pse_led_enabled_activate;
+
+		ret = devm_led_trigger_register(dev, &trigs->enabled);
+		if (ret) {
+			trigs->enabled.name = NULL;
+			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);

@@ -694,6 +857,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;
 	}

@@ -701,6 +865,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;

@@ -718,6 +883,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;

@@ -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;
+	}
+
+	/* 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);
+	for (i = 0; i < pcdev->nr_lines; i++) {
+		if (pcdev->no_of_pse_pi || pcdev->pi[i].np)
+			pse_led_update(pcdev, i);
+	}
+	mutex_unlock(&pcdev->lock);
+
 	mutex_lock(&pse_list_mutex);
 	list_add(&pcdev->list, &pse_controller_list);
 	mutex_unlock(&pse_list_mutex);

@@ -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);
+
+		/* Skip regulator/netlink path for PIs without consumers */
 		if (!pcdev->pi[i].rdev)
 			continue;

include/linux/pse-pd/pse.h +18 -0

index 4b5d5b11a084..ba321e669642 100644
@@ -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,21 @@ struct pse_pi {
 	int pw_allocated_mW;
 };
 
+/**
+ * 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;
+};
+
 /**
  * struct pse_ntf - PSE notification element
  *

@@ -304,6 +320,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;

@@ -329,6 +346,7 @@ 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 */
+	struct pse_pi_led_triggers *pi_led_trigs;
 };
 
 /**

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2026-05-05 01:58 UTC #16 lore raw

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

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/[email protected]/ open here

Quoted 4 lines

drivers/net/pse-pd/pse_core.c +0 -0
index b7ffec0c942c..bff97211ae22 100644

[ ... ]

Quoted 5 lines

drivers/net/pse-pd/pse_core.c +1 -0

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

[ ... ]

drivers/net/pse-pd/pse_core.c static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev, · line 787

Quoted 11 lines

drivers/net/pse-pd/pse_core.c +11 -0

+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?

Quoted 8 lines

drivers/net/pse-pd/pse_core.c +8 -0

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

drivers/net/pse-pd/pse_core.c static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev, · line 805

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +10 -0

+			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?

Quoted 4 lines

drivers/net/pse-pd/pse_core.c +4 -0

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

[ ... ]

Quoted 5 lines

drivers/net/pse-pd/pse_core.c +1 -0

@@ -1107,6 +1273,31 @@ int pse_controller_register(struct pse_controller_dev *pcdev)
 	if (ret)
 		return ret;
 
+	ret = pse_led_triggers_register(pcdev);

drivers/net/pse-pd/pse_core.c int pse_controller_register(struct pse_controller_dev *pcdev) · line 1286

Quoted 10 lines

drivers/net/pse-pd/pse_core.c +10 -0

+	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?

Quoted 8 lines

drivers/net/pse-pd/pse_core.c +8 -0

+
+	/* 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);

[ ... ]

drivers/net/pse-pd/pse_core.c static void pse_handle_events(struct pse_controller_dev *pcdev, · line 1466

Quoted 11 lines

drivers/net/pse-pd/pse_core.c +6 -1

@@ -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?

Quoted 4 lines

drivers/net/pse-pd/pse_core.c +2 -0

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

Carlo Szelinsky

author

2026-05-16 10:18 UTC #17 lore raw

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

Hi Jakub,

Thanks for the second pass! I went through all four points and would love to clarify some points before moving on to v6. Replying inline again:

Quoted 9 lines · On Mon, 4 May 2026 18:57:59 -0700 Jakub Kicinski wrote:

drivers/net/pse-pd/pse_core.c static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev, · line 787
Quoted 3 lines
drivers/net/pse-pd/pse_core.c +3 -0
+	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?

Quoted 3 lines

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?

Yes, seems to be real: devm LIFO frees pi_led_trigs before the regulators get unregistered. If a deferred disable fires during regulator_unregister() (via flush_work on rdev->disable_work), pse_pi_disable() runs and pse_led_update() walks freed memory.

But for one piece I got a question: the same path also touches pcdev->pi. pse_pi_disable() calls _pse_pi_disable(), before:
pse_led_update(), and _pse_pi_disable() derefs pcdev->pi[id] (via pse_pi_deallocate_pw_budget(&pcdev->pi[id]) and pcdev->pi[id].pw_d). pcdev->pi is already freed by pse_release_pis() in pse_controller_unregister(). So setting pi_led_trigs NULL only fixes the LED half, or?

Should v6 also handle pcdev->pi here (NULL after kfree, plus a guard in _pse_pi_disable)? Feels like the same class of bug as 1/2's reorder, just on the regulator-cleanup path. Or is a separate fix outside this patch series better?

Quoted 7 lines

Quoted 2 lines

drivers/net/pse-pd/pse_core.c +2 -0

+	for (i = 0; i < pcdev->nr_lines; i++) {
+		struct pse_pi_led_triggers *trigs = &pcdev->pi_led_trigs[i];

[...]

Quoted 3 lines

drivers/net/pse-pd/pse_core.c +3 -0

+		trigs->delivering.name = devm_kasprintf(dev, GFP_KERNEL,
+							"pse-%s:port%d:delivering",
+							dev_id, i);

[...]

Quoted 7 lines

drivers/net/pse-pd/pse_core.c static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev, · line 805
Quoted 1 line
drivers/net/pse-pd/pse_core.c +1 -0
+		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? [...] 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?

Right, but the window is microseconds wide (between setting trigs->delivering.name and devm_led_trigger_register() returning). You'd need a consumer racing during probe to hit it, right?

The fix seems to be simple: move pse_led_triggers_register() before the regulator loop in pse_controller_register(). of_load_pse_pis() runs earlier so pi[]/pi[i].np is already filled in, which is all the trigger loop needs. Regulators only get exposed after triggers are fully registered, no race window left.

Question: Do you agree with that solution for v6?

Quoted 5 lines

drivers/net/pse-pd/pse_core.c +5 -0

+	ret = pse_led_triggers_register(pcdev);
+	if (ret) {
+		/* LED triggers are non-essential for power delivery; warn
+		 * and continue. [...]
+		dev_warn(pcdev->dev, "Failed to register LED triggers: %d\n",

Quoted 8 lines

drivers/net/pse-pd/pse_core.c int pse_controller_register(struct pse_controller_dev *pcdev) · line 1286
Quoted 3 lines
drivers/net/pse-pd/pse_core.c +3 -0
+			 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?

Would love to clarify this one with you as well: devm cleanup does still run on driver unbind, since devm_led_trigger_register() attaches a per-call release action. So the partially-registered triggers stay in sysfs until unbind, but they're not leaked across it. And with pi_led_trigs = NULL, pse_led_update() short-circuits
so the orphans never fire either. So practically they're inert sysfs entries until unbind, not a leak.

That said, I agree the "warn and orphan" middle ground is a bit weird. So which path should I follow:

1) treat LED reg failure as fatal: return ret, fail probe. Smallest change, matches the rest of the function. The "non-essential" framing was mine, happy to drop it.
2) wrap the registration in a devres group so partial failures roll back the triggers we did manage to register.
3) leave as-is, since devm already cleans up on unbind.

I'd go with 1) since OOM during probe is fatal for most things anyway, but happy to do 2) or 3) if you have a preference.

Quoted 8 lines

Quoted 1 line
drivers/net/pse-pd/pse_core.c +1 -0
+		/* Update LEDs for described PIs regardless of consumer state.
[...]
Review
Quoted 2 lines
(quoted diff) +2 -0
+		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?

From what I see, both callers of pse_handle_events() hold pcdev->lock across the call:

* pse_isr() takes mutex_lock(&pcdev->lock) at line 1524, then calls pse_handle_events() at line 1531.
* pse_poll_worker() takes mutex_lock(&pcdev->lock) at line 1547, then calls pse_handle_events() at line 1551.

So pse_led_update() is running with the lock held in both paths.

Tbh I don't get completely what the AI is flagging here, or is this a false positive? If false positive, I'd still add lockdep_assert_held(&pcdev->lock) in pse_led_update() and pse_handle_events() to make the contract explicit and catch any future caller that forgets, but that would be documentation, not a bug fix.

So plan for v6 [2/2], pending your answers:
* NULL pcdev->pi_led_trigs in pse_controller_unregister() (and possibly NULL pcdev->pi too, depending on your answer?)
* move pse_led_triggers_register() before the regulator loop
* add lockdep_assert_held() (assuming is a false positive)
* whichever option you pick for question 3?

Depending on what we do with [1/2], I'll roll v6 with your decisions baked in.

Sorry for the long text...

Cheers,
Carlo

Oleksij Rempel

maintainer

2026-05-17 09:33 UTC #18 lore raw

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

Hi Carlo,

Thank you for your work!

Quoted 33 lines · Carlo Szelinsky

Hi Jakub,
Thanks for the second pass! I went through all four points and would love to clarify some points before moving on to v6. Replying inline again:
Quoted 9 lines · On Mon, 4 May 2026 18:57:59 -0700 Jakub Kicinski wrote:
drivers/net/pse-pd/pse_core.c static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev, · line 787
Quoted 3 lines
drivers/net/pse-pd/pse_core.c +3 -0
+	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?
Quoted 3 lines
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?
Yes, seems to be real: devm LIFO frees pi_led_trigs before the regulators get unregistered. If a deferred disable fires during regulator_unregister() (via flush_work on rdev->disable_work), pse_pi_disable() runs and pse_led_update() walks freed memory.
But for one piece I got a question: the same path also touches pcdev->pi. pse_pi_disable() calls _pse_pi_disable(), before:
pse_led_update(), and _pse_pi_disable() derefs pcdev->pi[id] (via pse_pi_deallocate_pw_budget(&pcdev->pi[id]) and pcdev->pi[id].pw_d). pcdev->pi is already freed by pse_release_pis() in pse_controller_unregister(). So setting pi_led_trigs NULL only fixes the LED half, or?

Quoted 4 lines

Should v6 also handle pcdev->pi here (NULL after kfree, plus a guard in _pse_pi_disable)? Feels like the same class of bug as 1/2's reorder, just on the regulator-cleanup path. Or is a separate fix outside this patch series better?

Please, send fixes for existing bugs separately for stable fixes.

Quoted 19 lines

Quoted 7 lines
Quoted 2 lines
drivers/net/pse-pd/pse_core.c +2 -0
+	for (i = 0; i < pcdev->nr_lines; i++) {
+		struct pse_pi_led_triggers *trigs = &pcdev->pi_led_trigs[i];
[...]
Quoted 3 lines
drivers/net/pse-pd/pse_core.c +3 -0
+		trigs->delivering.name = devm_kasprintf(dev, GFP_KERNEL,
+							"pse-%s:port%d:delivering",
+							dev_id, i);
[...]
Quoted 7 lines
drivers/net/pse-pd/pse_core.c static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev, · line 805
Quoted 1 line
drivers/net/pse-pd/pse_core.c +1 -0
+		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? [...] 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?
Right, but the window is microseconds wide (between setting trigs->delivering.name and devm_led_trigger_register() returning). You'd need a consumer racing during probe to hit it, right?

Quoted 7 lines

The fix seems to be simple: move pse_led_triggers_register() before the regulator loop in pse_controller_register(). of_load_pse_pis() runs earlier so pi[]/pi[i].np is already filled in, which is all the trigger loop needs. Regulators only get exposed after triggers are fully registered, no race window left.
Question: Do you agree with that solution for v6?

Sounds plausible.

Quoted 32 lines

Quoted 5 lines
Quoted 5 lines
drivers/net/pse-pd/pse_core.c +5 -0
+	ret = pse_led_triggers_register(pcdev);
+	if (ret) {
+		/* LED triggers are non-essential for power delivery; warn
+		 * and continue. [...]
+		dev_warn(pcdev->dev, "Failed to register LED triggers: %d\n",
Quoted 8 lines
drivers/net/pse-pd/pse_core.c int pse_controller_register(struct pse_controller_dev *pcdev) · line 1286
Quoted 3 lines
drivers/net/pse-pd/pse_core.c +3 -0
+			 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?
Would love to clarify this one with you as well: devm cleanup does still run on driver unbind, since devm_led_trigger_register() attaches a per-call release action. So the partially-registered triggers stay in sysfs until unbind, but they're not leaked across it. And with pi_led_trigs = NULL, pse_led_update() short-circuits
so the orphans never fire either. So practically they're inert sysfs entries until unbind, not a leak.
That said, I agree the "warn and orphan" middle ground is a bit weird. So which path should I follow:
1) treat LED reg failure as fatal: return ret, fail probe. Smallest change, matches the rest of the function. The "non-essential" framing was mine, happy to drop it.
2) wrap the registration in a devres group so partial failures roll back the triggers we did manage to register.
3) leave as-is, since devm already cleans up on unbind.

Quoted 2 lines

I'd go with 1) since OOM during probe is fatal for most things anyway, but happy to do 2) or 3) if you have a preference.

1 feels for me as most straightforward way. I would expect to see this kind of errors in a pre-production phase not in the end product.

Quoted 17 lines

Quoted 8 lines
Quoted 1 line
drivers/net/pse-pd/pse_core.c +1 -0
+		/* Update LEDs for described PIs regardless of consumer state.
[...]
Review
Quoted 2 lines
(quoted diff) +2 -0
+		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?
From what I see, both callers of pse_handle_events() hold pcdev->lock across the call:
* pse_isr() takes mutex_lock(&pcdev->lock) at line 1524, then calls pse_handle_events() at line 1531.
* pse_poll_worker() takes mutex_lock(&pcdev->lock) at line 1547, then calls pse_handle_events() at line 1551.

Quoted 4 lines

So pse_led_update() is running with the lock held in both paths.
Tbh I don't get completely what the AI is flagging here, or is this a false positive?

Yes.

Quoted 5 lines

If false positive, I'd still add lockdep_assert_held(&pcdev->lock) in pse_led_update() and pse_handle_events() to make the contract explicit and catch any future caller that forgets, but that would be documentation, not a bug fix.

I do not know what here went wrong, too much context?

In the raw log it looks like LLM was able to detect proper locking:
https://sashiko.dev/#/log/35678

"I'm seeing that in pse_controller_register, the code takes the mutex, iterates and calls pse_led_update. Also, in pse_handle_events, this is happening again, and this is called from pse_isr with mutex_lock(&pcdev->lock). And in pse_pi_enable, it's again called within the mutex. So, pse_led_update *always* seems to be called with the lock held. This is consistent and good."

And later:

"However, there's no evidence that pcdev->lock is held inside pse_handle_events. Furthermore, the code uses a separate ntf_fifo_lock."

We can ignore it. In case if you cares, lockdep_assert_held() sounds good, comments can be misread. I would add it to pse_handle_events(), it will make it fully visible for AI and detectable by CI.

Quoted 7 lines

So plan for v6 [2/2], pending your answers:
* NULL pcdev->pi_led_trigs in pse_controller_unregister() (and possibly NULL pcdev->pi too, depending on your answer?)
* move pse_led_triggers_register() before the regulator loop
* add lockdep_assert_held() (assuming is a false positive)
* whichever option you pick for question 3?

Quoted 7 lines

Depending on what we do with [1/2], I'll roll v6 with your decisions baked in.
Sorry for the long text...
Cheers,
Carlo

Thank you!

Best Regards,
Oleksij

Signature · 4 lines

Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |