net: pse-pd: assorted lifetime and error-path fixes
Subsystems (3)
v2 cover net: pse-pd: assorted lifetime and error-path fixes
Fixes for the PSE core and the pd692x0 driver, found while working on the PSE controller teardown. Most of them are about what happens when a controller unbinds or fails to register.
1 The kfifo is too small: podl-pse-regulator and any controller with
0 or 1 lines cannot register.
2 Removes two checks that can never be true. Not a fix; it makes 3
smaller.
3 A module reference leaks when reading the port state fails. 4 A failed IRQ request stays in pcdev->irq, so unregister disables a
line another device may own.
5 The notification worker can read a PHY that was already freed. 6 Setting a power limit checks the PI state without the lock. 7 A power domain shared by two controllers is freed when the one that
created it unbinds.
8 Unregister frees the PI array before it stops the IRQ handler. No
in-tree driver hits this; 10 to 12 build on the new order.
9 Unregister frees the PI array while a lookup can still find the
controller.
10 The PI regulators outlive the PI array, the PSE core can still
power a PI while the controller goes away, and a powered PI keeps its budget.
11 A failed registration leaks the kfifo and leaves power domains with
a stale supply handle.
12 A shared power domain's supply handle goes stale when the controller
that created it unbinds.
13 pd692x0 leaks its manager power budget when probe fails. 14 pd692x0 crashes on a manager supply that is not resolved yet.
Some patches need earlier ones: 7 needs the locked check from 6, 10 to 12 build on 7 to 9, and 14 relies on 13 to give back a budget that was already reserved.
Testing: every patch builds on its own with W=1 and is checkpatch clean. 1, 5, 7, 10, 11 and 12 were tested in QEMU (arm64, KASAN, lockdep, kmemleak) with test-only fake PSE controllers: without the patch, the failure in its commit message shows up; with the whole series, the tests are clean, except for the older lockdep report below. The other patches were found by code inspection and are build-tested only.
Ross Cawston's "net: pse-pd: don't enable a PI again while disabling it" [3] applies on top of this series without conflicts. The tests also hit two older bugs that this series does not fix:
- With the static strategy, turning off a PI to make room for a waiting one retries the waiting PI from inside its own allocation, which recurses until the kernel stack overflows. [3] removes that retry.
- pse_isr(), setting a power limit and unregister hold pcdev->lock and then take the supply's regulator lock, while regulator_enable() on a PI takes them the other way round. Lockdep reports it when the supply lock is contended.
Older problems that reviews of this series found, not fixed here:
- A pse_control held by a PHY can outlive its controller. The PSE notifier series for net-next fixes this.
- Setting a power limit changes the budget and the hardware limit under two separate locks, so pse_isr() can run in between.
- On tps23881, pse_controller_unregister() calls disable_irq() after devres has already freed the IRQ.
- A PI waiting for budget can get budget reserved by a power limit change; that budget is not freed when the PI is powered later.
- The PI regulators are visible before their power domain is set. A PI enabled in that short time skips the budget check.
Changes in v2:
- 2 is new; 3 now only fixes the remaining error path (Kory).
- 5: the worker comment says why a shared PI is safe (Sashiko).
- 6: kernel-doc of the return value fixed (Sashiko, Kory).
- 8: the message no longer claims bugs that 5 and 7 made impossible or that no in-tree driver can hit; Fixes: tag dropped.
- 8 and 9 are patches 1 and 3 of [1]. 9 now takes the controller off the list before the worker drain and the power domain flush, as [2] 1/4 did. Patch 2 of [1] is replaced by 10, which keeps the PI array until the PI regulators are gone, lets the core power no PI after unregister starts, and switches off the PIs that hold budget.
- 11 is patch 2 of the PSE core fixes for net-next [2], moved here.
- 12 is new; v1 listed this bug as not fixed. The domain no longer has a supply handle of its own; each PI uses its regulator's handle.
- The si3474 patch and the power limit overflow fix move to net-next.
- Collected Reviewed-by tags; each patch says if it was triggered.
- v1: https://lore.kernel.org/netdev/[email protected]/
[1] https://lore.kernel.org/netdev/[email protected]/
[2] https://lore.kernel.org/netdev/[email protected]/
[3] https://lore.kernel.org/netdev/[email protected]/
Carlo Szelinsky (14):
net: pse-pd: size the notification kfifo after defaulting nr_lines
net: pse-pd: remove dead pi_get_admin_state() checks
net: pse-pd: fix module refcount leak in pse_control_get_internal()
net: pse-pd: do not keep a failed IRQ in pcdev->irq
net: pse-pd: do not reach a freed PHY from the notification worker
net: pse-pd: check the PI state under the lock when setting a power
limit
net: pse-pd: do not tie a shared power domain to its creator's devres
net: pse-pd: stop async event sources before freeing PI data in
unregister
net: pse-pd: unregister from the controller list before freeing PI
data
net: pse-pd: keep the PI data until the PI regulators are gone
net: pse-pd: unwind allocations when controller registration fails
net: pse-pd: use each PI's own supply handle for the power budget
net: pse-pd: pd692x0: release the manager power budget through devres
net: pse-pd: pd692x0: defer probe while a manager supply is unresolved
drivers/net/pse-pd/pd692x0.c | 31 +++- drivers/net/pse-pd/pse_core.c | 319 ++++++++++++++++++++++++---------- include/linux/pse-pd/pse.h | 5 + 3 files changed, 255 insertions(+), 100 deletions(-)
base-commit: 78731b46967e3e7102b5cd5062bef185f3404b5c
Signature · 1 line
2.43.0
PATCH v2 1/14 net: pse-pd: size the notification kfifo after defaulting nr_lines
pse_controller_register() allocates the notification kfifo before it turns nr_lines == 0 into 1, and kfifo_alloc() fails for a size below 2. So a controller with nr_lines 0 or 1 cannot register. pse_regulator (podl-pse-regulator) never sets nr_lines, so its probe has failed with -EINVAL ever since the kfifo was added.
Apply the default first, and allocate at least two entries.
Triggered in QEMU (arm64) with a test-only PSE controller with nr_lines 0: its probe failed with "failed to allocate kfifo notifications". With this patch it registers.
--- drivers/net/pse-pd/pse_core.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)
drivers/net/pse-pd/pse_core.c +7 -4
@@ -1044,16 +1044,19 @@ int pse_controller_register(struct pse_controller_dev *pcdev)mutex_init (&pcdev->lock);INIT_LIST_HEAD (&pcdev->pse_control_head);spin_lock_init (&pcdev->ntf_fifo_lock); - ret =kfifo_alloc (&pcdev->ntf_fifo, pcdev->nr_lines,GFP_KERNEL ); + +if (!pcdev->nr_lines) + pcdev->nr_lines =1 ; + +/* kfifo_alloc() rejects a size that rounds up to less than 2 */ + ret =kfifo_alloc (&pcdev->ntf_fifo,max (pcdev->nr_lines,2U ), +GFP_KERNEL );if (ret) {dev_err (pcdev->dev,"failed to allocate kfifo notifications\n" );return ret; }INIT_WORK (&pcdev->ntf_work, pse_send_ntf_worker); -if (!pcdev->nr_lines) - pcdev->nr_lines =1 ; -if (!pcdev->ops->pi_get_admin_state || !pcdev->ops->pi_get_pw_status) { dev_err(pcdev->dev,
Signature · 1 line
2.43.0
Re: [PATCH net v2 01/14] net: pse-pd: size the notification kfifo after defaulting nr_lines
Quoted 8 lines · Carlo Szelinsky
pse_controller_register()allocates the notification kfifo before it turnsnr_lines== 0 into 1, andkfifo_alloc()fails for a size below 2. So a controller withnr_lines0 or 1 cannot register.pse_regulator(podl-pse-regulator) never setsnr_lines, so its probe has failed with-EINVALever since the kfifo was added.Apply the default first, and allocate at least two entries.
Quoted 7 lines
Triggered in QEMU (arm64) with a test-only PSE controller with
nr_lines0: its probe failed with "failed to allocate kfifo notifications". With this patch it registers.Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")Reviewed-by Kory Maincent <[email protected]>Signed-off-by Carlo Szelinsky <[email protected]>
Thank you!
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 |
PATCH v2 2/14 net: pse-pd: remove dead pi_get_admin_state() checks
In reply to Carlo Szelinskypse_controller_register() already fails when a driver has no pi_get_admin_state() callback. So the checks for it in pse_pi_is_enabled() and pse_control_get_internal() are never true. Remove them.
No functional change. It makes the next patch, which fixes an error path in pse_control_get_internal(), smaller.
--- drivers/net/pse-pd/pse_core.c | 10 ---------- 1 file changed, 10 deletions(-)
drivers/net/pse-pd/pse_core.c +0 -10
@@ -412,13 +412,8 @@ static bool pse_pw_d_is_sw_pw_control(struct pse_controller_dev *pcdev,static int pse_pi_is_enabled (struct regulator_dev *rdev) {struct pse_controller_dev *pcdev =rdev_get_drvdata (rdev); -const struct pse_controller_ops *ops;int id, ret; - ops = pcdev->ops; -if (!ops->pi_get_admin_state) -return -EOPNOTSUPP ; - id =rdev_get_id (rdev);mutex_lock (&pcdev->lock);if (pse_pw_d_is_sw_pw_control (pcdev, pcdev->pi[id].pw_d)) {
@@ -1425,11 +1420,6 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index, goto free_psec; } -if (!pcdev->ops->pi_get_admin_state) { - ret = -EOPNOTSUPP ; - goto free_psec; - } -/* Initialize admin_state_enabled before the regulator_get. This * aims to have the right value reported in the first is_enabled * call in case of control managed by software.
Signature · 1 line
2.43.0
Re: [PATCH net v2 02/14] net: pse-pd: remove dead pi_get_admin_state() checks
Quoted 9 lines · Carlo Szelinsky
pse_controller_register()already fails when a driver has nopi_get_admin_state()callback. So the checks for it inpse_pi_is_enabled()andpse_control_get_internal()are never true. Remove them.No functional change. It makes the next patch, which fixes an error path in
pse_control_get_internal(), smaller.Signed-off-by Carlo Szelinsky <[email protected]>
Thank you!
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 |
PATCH v2 3/14 net: pse-pd: fix module refcount leak in pse_control_get_internal()
In reply to Carlo Szelinskypse_control_get_internal() takes a module reference, but when pse_pi_is_hw_enabled() fails, it skips module_put(). This happens on any I2C or UART error. Each time, one more reference leaks, and the PSE driver can no longer be unloaded.
Send this error path to put_module, like the regulator_get_exclusive() error path.
Found by code inspection. Not reproduced.
--- drivers/net/pse-pd/pse_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/pse-pd/pse_core.c +1 -1
@@ -1426,7 +1426,7 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,*/ ret =pse_pi_is_hw_enabled (pcdev, index);if (ret <0 ) - goto free_psec; + goto put_module; pcdev->pi[index].admin_state_enabled = ret; psec->ps = regulator_get_exclusive(pcdev->dev,
Signature · 1 line
2.43.0
Re: [PATCH net v2 03/14] net: pse-pd: fix module refcount leak in pse_control_get_internal()
Quoted 5 lines · Carlo Szelinsky
pse_control_get_internal()takes a module reference, but whenpse_pi_is_hw_enabled()fails, it skipsmodule_put(). This happens on any I2C or UART error. Each time, one more reference leaks, and the PSE driver can no longer be unloaded.
Quoted 8 lines
Send this error path to
put_module, like theregulator_get_exclusive()error path.Found by code inspection. Not reproduced.
Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")Reviewed-by Kory Maincent <[email protected]>Signed-off-by Carlo Szelinsky <[email protected]>
Thank you!
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 |
PATCH v2 4/14 net: pse-pd: do not keep a failed IRQ in pcdev->irq
In reply to Carlo Szelinskydevm_pse_irq_helper() keeps the IRQ number in pcdev->irq even when the IRQ request failed. The failed probe then runs pse_controller_unregister(), which calls disable_irq() on that line. If another device owns it, that device stops getting interrupts.
Clear pcdev->irq when the request fails. It is now set before the request, because pse_isr() can run as soon as the IRQ is enabled and pse_pw_d_is_sw_pw_control() reads it.
Found by code inspection. Not reproduced.
--- drivers/net/pse-pd/pse_core.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)
drivers/net/pse-pd/pse_core.c +9 -3
@@ -1343,14 +1343,20 @@ int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,if (!h->notifs_mask)return -ENOMEM ; +/* pse_isr() can run as soon as the IRQ is enabled, and +* pse_pw_d_is_sw_pw_control() reads pcdev->irq. +*/ + pcdev->irq = irq; ret =devm_request_threaded_irq (dev, irq,NULL , pse_isr,IRQF_ONESHOT | irq_flags, irq_name, h); -if (ret) +if (ret) { + pcdev->irq =0 ;dev_err (pcdev->dev,"Failed to request IRQ %d\n" , irq); +return ret; + } - pcdev->irq = irq; -return ret; +return 0 ; }EXPORT_SYMBOL_GPL (devm_pse_irq_helper);
Signature · 1 line
2.43.0
Re: [PATCH net v2 04/14] net: pse-pd: do not keep a failed IRQ in pcdev->irq
Quoted 9 lines · Carlo Szelinsky
devm_pse_irq_helper()keeps the IRQ number inpcdev->irqeven when the IRQ request failed. The failed probe then runspse_controller_unregister(), which callsdisable_irq()on that line. If another device owns it, that device stops getting interrupts.Clear
pcdev->irqwhen the request fails. It is now set before the request, becausepse_isr()can run as soon as the IRQ is enabled andpse_pw_d_is_sw_pw_control()reads it.
Quoted 5 lines
Found by code inspection. Not reproduced.
Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")Reviewed-by Kory Maincent <[email protected]>Signed-off-by Carlo Szelinsky <[email protected]>
Thank you!
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 |
PATCH v2 5/14 net: pse-pd: do not reach a freed PHY from the notification worker
In reply to Carlo SzelinskyThe notification worker holds a reference on the pse_control, but not on the PHY that psec->attached_phydev points to. If the PHY is removed while the worker runs, the pse_control survives and the worker reads the freed PHY. The same happens without a race when two PHYs share one PI:attached_phydev keeps pointing at the first PHY after it is removed.
pse_control_put() unlinks and frees a pse_control under pse_list_mutex, and phy_device_remove() calls it before the PHY is freed. So instead of taking a reference, let the worker look the pse_control up and use attached_phydev under pse_list_mutex. This nests pse_list_mutex inside rtnl; nothing under pse_list_mutex takes rtnl.
For a PI shared by two PHYs there is no single netdev to report to, so clear attached_phydev when a second PHY takes the pse_control, and warn. Events for that PI are then no longer reported, even after one PHY goes away. Failing the second PHY instead would fail its whole MDIO bus.
Triggered in QEMU (arm64, KASAN) with a test-only PSE controller:
removing a PHY while the worker waits for rtnl, and removing one of two PHYs on a shared PI, both give
BUG: KASAN: slab-use-after-free in pse_send_ntf_worker+0x2b0/0x2bc
With this patch both are gone.
--- drivers/net/pse-pd/pse_core.c | 44 ++++++++++++++++++++++++++--------- 1 file changed, 33 insertions(+), 11 deletions(-)
drivers/net/pse-pd/pse_core.c +33 -11
@@ -238,24 +238,22 @@ static int of_load_pse_pis(struct pse_controller_dev *pcdev)* @pcdev: a pointer to the PSE * @id: index of the PSE control * -* Return: pse_control pointer or NULL. The device returned has had a -* reference added and the pointer is safe until the user calls -* pse_control_put() to indicate they have finished with it. +* Must be called with pse_list_mutex held. No reference is taken: the +* pointer is only valid until pse_list_mutex is released. +* +* Return: pse_control pointer or NULL. */ static struct pse_control *pse_control_find_by_id (struct pse_controller_dev *pcdev,int id) {struct pse_control *psec; -mutex_lock (&pse_list_mutex); +lockdep_assert_held (&pse_list_mutex); +list_for_each_entry (psec, &pcdev->pse_control_head, list) { -if (psec->id == id) { -kref_get (&psec->refcnt); -mutex_unlock (&pse_list_mutex); +if (psec->id == id)return psec; - } } -mutex_unlock (&pse_list_mutex);return NULL ; }
@@ -268,6 +266,7 @@ pse_control_find_by_id(struct pse_controller_dev *pcdev, int id)static struct net_device *pse_control_get_netdev (struct pse_control *psec) {ASSERT_RTNL (); +lockdep_assert_held (&pse_list_mutex);if (!psec || !psec->attached_phydev)return NULL ;
@@ -1015,13 +1014,23 @@ static void pse_send_ntf_worker(struct work_struct *work)struct net_device *netdev;struct pse_control *psec; - psec =pse_control_find_by_id (pcdev, ntf.id); +/* If attached_phydev is set, it names the only PHY that +* holds this pse_control: a second PHY on the same PI +* clears it in pse_control_get_internal(). That PHY's +* pse_control_put() unlinks and frees the pse_control under +* pse_list_mutex before the PHY itself can be freed. Holding +* the mutex across the lookup and the attached_phydev +* dereference therefore keeps the PHY alive, which a +* reference on the pse_control alone would not. +*/ rtnl_lock (); +mutex_lock (&pse_list_mutex); + psec =pse_control_find_by_id (pcdev, ntf.id); netdev =pse_control_get_netdev (psec);if (netdev)ethnl_pse_send_ntf (netdev, ntf.notifs); +mutex_unlock (&pse_list_mutex);rtnl_unlock (); -pse_control_put (psec); } }
@@ -1412,6 +1421,19 @@ pse_control_get_internal(struct pse_controller_dev *pcdev, unsigned int index,list_for_each_entry (psec, &pcdev->pse_control_head, list) {if (psec->id == index) { +/* attached_phydev can only name one PHY. If a second +* one shares the PI, either may be removed first and +* leave it pointing at a freed phy_device, so stop +* reporting events for this PI instead. +*/ +if (psec->attached_phydev && + psec->attached_phydev != phydev) { +dev_warn (pcdev->dev, +"PI %u shared by several PHYs, not reporting its events\n" , + index); + psec->attached_phydev =NULL ; + } +kref_get (&psec->refcnt);return psec; }
Signature · 1 line
2.43.0
Re: [PATCH net v2 05/14] net: pse-pd: do not reach a freed PHY from the notification worker
CCing: Maxime Chevallier <[email protected]> for phy_port part.
Quoted 12 lines · Carlo Szelinsky
The notification worker holds a reference on the
pse_control, but not on the PHY thatpsec->attached_phydevpoints to. If the PHY is removed while the worker runs, thepse_controlsurvives and the worker reads the freed PHY. The same happens without a race when two PHYs share one PI:attached_phydevkeeps pointing at the first PHY after it is removed.
pse_control_put()unlinks and frees apse_controlunderpse_list_mutex, andphy_device_remove()calls it before the PHY is freed. So instead of taking a reference, let the worker look thepse_controlup and useattached_phydevunderpse_list_mutex. This nestspse_list_mutexinside rtnl; nothing underpse_list_mutextakes rtnl.
Quoted 4 lines
For a PI shared by two PHYs there is no single netdev to report to, so clear
attached_phydevwhen a second PHY takes thepse_control, and warn. Events for that PI are then no longer reported, even after one PHY goes away. Failing the second PHY instead would fail its whole MDIO bus.
A PI shared by two PHYs on one connector is a rare setup and no in-tree DT has one (prtt1c gives each PHY its own PI), so dropping its events is fine for this fix.
If this is ever lifted, the PI should hang off the port rather than a
PHY: a PI powers a connector, and with phy_port (Documentation/networking/phy-port.rst) net-next already has a port object to attach it to. That is net-next work, not for this series.
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 |
PATCH v2 6/14 net: pse-pd: check the PI state under the lock when setting a power limit
In reply to Carlo Szelinskypse_ethtool_set_pw_limit() checks whether the PI is powered without pcdev->lock. pse_isr() changes that state under the lock, so if the PD disconnects in between, budget is reserved for a port that is no longer powered.
Do the check in pse_pi_update_pw_budget() under the lock, and report whether the budget was changed, so the rollback after a failed regulator_set_current_limit() runs only then. This also rolls back a budget that started from zero, which the old code skipped.
Found by code inspection. Not reproduced.
--- drivers/net/pse-pd/pse_core.c | 39 ++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 14 deletions(-)
drivers/net/pse-pd/pse_core.c +25 -14
@@ -1821,22 +1821,35 @@ EXPORT_SYMBOL_GPL(pse_ethtool_set_config);* @pcdev: a pointer to the PSE controller device * @id: index of the PSE PI * @pw_req: power requested +* @updated: set to true if the budget was changed * @extack: extack for reporting useful error messages * -* Return: Previous power allocated on success and failure value on error +* Return: Power allocated before the update, or a negative error code. +* 0 is also returned when the budget was not changed, so callers +* check @updated before they roll back. */ static int pse_pi_update_pw_budget (struct pse_controller_dev *pcdev,int id, -const unsigned int pw_req, +const unsigned int pw_req,bool *updated,struct netlink_ext_ack *extack) {struct pse_pi *pi = &pcdev->pi[id];int previous_pw_allocated;int pw_diff, ret =0 ; + *updated = false; +/* We don't want pw_allocated_mW value change in the middle of an * power budget update */ mutex_lock (&pcdev->lock); +/* Only a PI under software power control that is enabled and has a +* PD detected gets a budget. Check this here, as pse_isr() changes +* it under the same lock. +*/ +if (!pse_pw_d_is_sw_pw_control (pcdev, pi->pw_d) || + !pi->admin_state_enabled || !pi->isr_pd_detected) + goto out; + previous_pw_allocated = pi->pw_allocated_mW; pw_diff = pw_req - previous_pw_allocated;if (!pw_diff) {
@@ -1854,6 +1867,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,regulator_free_power_budget (pi->pw_d->supply, -pw_diff); } pi->pw_allocated_mW = pw_req; + *updated = true; ret = previous_pw_allocated; out:
@@ -1874,6 +1888,7 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec, const unsigned int pw_limit) {int uV, uA, ret, previous_pw_allocated =0 ; +bool updated;s64 tmp_64;if (pw_limit >MAX_PI_PW )
@@ -1900,21 +1915,17 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec,/* Update power budget only in software power control case and * if a Power Device is powered. */ -if (pse_pw_d_is_sw_pw_control (psec->pcdev, - psec->pcdev->pi[psec->id].pw_d) && - psec->pcdev->pi[psec->id].admin_state_enabled && - psec->pcdev->pi[psec->id].isr_pd_detected) { - ret =pse_pi_update_pw_budget (psec->pcdev, psec->id, - pw_limit, extack); -if (ret <0 ) -return ret; - previous_pw_allocated = ret; - } + ret =pse_pi_update_pw_budget (psec->pcdev, psec->id, pw_limit, + &updated, extack); +if (ret <0 ) +return ret; + previous_pw_allocated = ret; ret =regulator_set_current_limit (psec->ps,0 , uA); -if (ret <0 && previous_pw_allocated) { +if (ret <0 && updated) {pse_pi_update_pw_budget (psec->pcdev, psec->id, - previous_pw_allocated, extack); + previous_pw_allocated, &updated, + extack); }return ret;
Signature · 1 line
2.43.0
Re: [PATCH net v2 06/14] net: pse-pd: check the PI state under the lock when setting a power limit
Quoted 10 lines · Carlo Szelinsky
pse_ethtool_set_pw_limit()checks whether the PI is powered withoutpcdev->lock.pse_isr()changes that state under the lock, so if the PD disconnects in between, budget is reserved for a port that is no longer powered.Do the check in
pse_pi_update_pw_budget()under the lock, and report whether the budget was changed, so the rollback after a failedregulator_set_current_limit()runs only then. This also rolls back a budget that started from zero, which the old code skipped.
Quoted 5 lines
Found by code inspection. Not reproduced.
Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")Reviewed-by Kory Maincent <[email protected]>Signed-off-by Carlo Szelinsky <[email protected]>
Thank you!
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 |
[email protected]
AI reviewRe: [PATCH net v2 06/14] net: pse-pd: check the PI state under the lock when setting a power limit
In reply to Carlo SzelinskyThank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider.
Critical: 0 · High: 1 (1) · Medium: 1 (1) · Low: 1
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [High] The new check in pse_pi_update_pw_budget() (drivers/net/pse- pd/pse_core.c:1849-1851) only tests `pse_pw_d_is_sw_pw_control() &&…
- [Medium] pse_ethtool_set_pw_limit() still races pse_isr() after the change.
- [Low] The new eligibility check in pse_pi_update_pw_budget() also applies to the rollback call, so the rollback can be skipped while the…
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is an AB-BA deadlock between pcdev->lock and the supply regulator's ww_mutex.
- [Medium] This problem wasn't introduced by this patch, but the rollback call to pse_pi_update_pw_budget() in pse_ethtool_set_pw_limit() (lines…
--
net: pse-pd: check the PI state under the lock when setting a power limit
This patch moves the "PI is powered" check from pse_ethtool_set_pw_limit() into pse_pi_update_pw_budget(), where it runs under pcdev->lock. The helper now reports through a new updated argument whether the budget changed. The rollback after a failed regulator_set_current_limit() uses that flag instead of a non-zero previous allocation.
Quoted 5 lines
drivers/net/pse-pd/pse_core.c +0
-0@@ -1821,22 +1821,35 @@ EXPORT_SYMBOL_GPL(pse_ethtool_set_config);
[ ... ]
Quoted 12 lines
static int pse_pi_update_pw_budget(struct
pse_controller_dev*pcdev, int id,drivers/net/pse-pd/pse_core.c +3
-1-const unsigned int pw_req, + const unsigned int pw_req, bool *updated, struct netlink_ext_ack *extack) { struct pse_pi *pi = &pcdev->pi[id];int previous_pw_allocated;int pw_diff, ret =0 ; + *updated = false; +/* We don't want pw_allocated_mW value change in the middle of an
drivers/net/pse-pd/pse_core.c EXPORT_SYMBOL_GPL(pse_ethtool_set_config); · line 1851
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +7
-0* power budget update */ mutex_lock (&pcdev->lock); +/* Only a PI under software power control that is enabled and has a +* PD detected gets a budget. Check this here, as pse_isr() changes +* it under the same lock. +*/ +if (!pse_pw_d_is_sw_pw_control (pcdev, pi->pw_d) || + !pi->admin_state_enabled || !pi->isr_pd_detected) + goto out;
[Severity: High]
Do admin_state_enabled and isr_pd_detected really mean the PI is powered?
Both flags can stay set while the port is off in hardware. pse_set_config_isr() hides -ERANGE from the static budget allocation:
pcdev->pi[id].isr_pd_detected = true;
if (pcdev->pi[id].admin_state_enabled) {
ret = _pse_pi_delivery_power_sw_pw_ctrl(pcdev, id,
&extack);
if (ret == -ERANGE)
ret = 0;
}pse_pi_enable() also sets admin_state_enabled on -ERANGE. In both cases the PI is left pending, with pw_allocated_mW == 0 and pi_enable() never called.
pse_disable_pi_pol() is another case. It turns off a powered PI through _pse_pi_disable() to shed budget, but it leaves both flags set.
The core already treats this as a separate state in pse_pi_is_admin_enable_pending():
if (!pcdev->pi[id].admin_state_enabled || !pcdev->pi[id].isr_pd_detected) return false; ret = pse_pi_is_hw_enabled(pcdev, id); /* PSE PI is already enabled at hardware level */ if (ret == 1) return false;
A pending PI passes this check. regulator_request_power_budget() then reserves pw_limit, and pi->pw_allocated_mW is set to pw_limit, while no power is delivered.
pse_pw_d_retry_power_delivery() can power the port later. It is called from _pse_pi_disable() of another PI and from pse_ethtool_set_prio(). The path is _pse_pi_delivery_power_sw_pw_ctrl() -> pse_pi_allocate_pw_budget_static_prio():
while (regulator_request_power_budget(pi->pw_d->supply, pw_req) == -ERANGE) {
...
pi->pw_allocated_mW = pw_req;This requests the full pw_req again and overwrites pw_allocated_mW. The earlier ethtool reservation is never freed. pse_pi_deallocate_pw_budget() only frees the overwritten value.
Does the pw_limit reserved through ethtool then stay charged to the supply for good?
The commit message says the patch stops budget being reserved "for a port that is no longer powered". That still seems possible, for example right after pse_disable_pi_pol() turns a port off. The same flag test existed before the patch, outside the lock.
Would checking pse_pi_is_hw_enabled() == 1 here, under the lock, close this?
The comment kept in pse_ethtool_set_pw_limit() ("if a Power Device is powered") also doesn't match what this check tests.
[Severity: Low]
The rollback call in pse_ethtool_set_pw_limit() also goes through this check. Can the rollback be skipped while the increased reservation is still held?
pse_ethtool_set_pw_limit()
pse_pi_update_pw_budget() raises P -> L, updated = true, unlock
pse_isr() DISCONNECTION
pse_set_config_isr()
_pse_pi_disable()
ops->pi_disable() fails (e.g. an I2C error), returns before
pse_pi_deallocate_pw_budget()
pcdev->pi[id].isr_pd_detected = false;pse_ethtool_set_pw_limit()
regulator_set_current_limit() fails
pse_pi_update_pw_budget(previous_pw_allocated)
!pi->isr_pd_detected -> goto out
pw_allocated_mW then stays at L. The pre-patch rollback would have restored it to P. Failing this check doesn't always mean the PI's reservation has been released.
drivers/net/pse-pd/pse_core.c static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id, · line 1868
Quoted 7 lines
drivers/net/pse-pd/pse_core.c +1
-0+ previous_pw_allocated = pi->pw_allocated_mW; pw_diff = pw_req - previous_pw_allocated;if (!pw_diff) {@@ -1854,6 +1867,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,regulator_free_power_budget (pi->pw_d->supply, -pw_diff); }
[Severity: High]
This is a pre-existing issue and was not introduced by this patch. Is there an AB-BA deadlock between pcdev->lock and the supply regulator's lock?
Path A, ethtool set_config under rtnl:
pse_ethtool_c33_set_config()
regulator_enable(psec->ps)
regulator_lock_dependent()
regulator_lock_recursive() locks PI rdev, then its supply rdev
_regulator_enable()->_regulator_do_enable()
pse_pi_enable()
mutex_lock(&pcdev->lock)regulator_disable() -> pse_pi_disable() takes the locks in the same order.
Path B, the threaded IRQ:
pse_isr()
mutex_lock(&pcdev->lock)
pse_set_config_isr()
_pse_pi_delivery_power_sw_pw_ctrl()
pse_pi_allocate_pw_budget_static_prio()
regulator_request_power_budget(pi->pw_d->supply)
regulator_lock(supply rdev)On DISCONNECTION, _pse_pi_disable() -> regulator_free_power_budget() takes the same supply lock.
pw_d->supply comes from regulator_get(&rdev->dev, rdev->supply_name). It is the same supply rdev that path A already holds.
If A holds the supply rdev and waits for pcdev->lock, while B holds pcdev->lock and waits for the supply rdev, both tasks hang for good. A also holds rtnl_lock.
pse_pi_update_pw_budget(), including its new rollback use, keeps the same pcdev->lock -> supply order.
Quoted 5 lines
drivers/net/pse-pd/pse_core.c +1
-0pi->pw_allocated_mW = pw_req; + *updated = true; ret = previous_pw_allocated; out:
[ ... ]
Quoted 18 lines
drivers/net/pse-pd/pse_core.c +4
-10@@ -1900,21 +1915,17 @@ int pse_ethtool_set_pw_limit(struct pse_control *psec,/* Update power budget only in software power control case and * if a Power Device is powered. */ -if (pse_pw_d_is_sw_pw_control (psec->pcdev, - psec->pcdev->pi[psec->id].pw_d) && - psec->pcdev->pi[psec->id].admin_state_enabled && - psec->pcdev->pi[psec->id].isr_pd_detected) { - ret =pse_pi_update_pw_budget (psec->pcdev, psec->id, - pw_limit, extack); -if (ret <0 ) -return ret; - previous_pw_allocated = ret; - } + ret =pse_pi_update_pw_budget (psec->pcdev, psec->id, pw_limit, + &updated, extack); +if (ret <0 ) +return ret;
drivers/net/pse-pd/pse_core.c int pse_ethtool_set_pw_limit(struct pse_control *psec, · line 1929
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +4
-2+ previous_pw_allocated = ret; ret =regulator_set_current_limit (psec->ps,0 , uA); -if (ret <0 && previous_pw_allocated) { +if (ret <0 && updated) {pse_pi_update_pw_budget (psec->pcdev, psec->id, - previous_pw_allocated, extack); + previous_pw_allocated, &updated, + extack); }
[Severity: Medium]pcdev->lock is still dropped between three steps: the budget update, regulator_set_current_limit() (which takes the lock again in pse_pi_set_current_limit()), and the rollback.
Can updated and previous_pw_allocated be stale by the time the rollback runs? The rollback also applies an absolute value to whatever PD session exists at that point.
Two interleavings with pse_isr() look possible.
(a) After the budget update, pse_isr() handles DISCONNECTION, which frees the budget, and then CLASSIFICATION. _pse_pi_delivery_power_sw_pw_ctrl() budgets the new PD as min(pi_get_pw_req, pi_get_pw_limit), using the old hardware limit.
If regulator_set_current_limit() then fails, the rollback forces the new session's allocation back to the old session's value.
The patch adds a rollback-from-zero case (previous_pw_allocated == 0 and updated == true, e.g. for a pending PI). In that case the rollback can free the new PD's whole budget while the PD is powered, so the supply is oversubscribed.
(b) A PD is classified between the budget update and the hardware limit write. It is budgeted from the old limit, and then the new, higher limit is programmed. The hardware then allows more power than is reserved.
These windows existed before the patch. The rollback driven by updated is new.
The commit message describes the pse_isr() race. Would it make sense to do the budget update and the hardware limit write in one pcdev->lock section, so that race is fully closed?
[Severity: Medium]
This isn't a bug introduced by this patch, but the return value of the rollback pse_pi_update_pw_budget() call is ignored. What happens if the rollback's re-request fails?
When the limit is lowered from P to L, the first call frees P - L mW and drops pcdev->lock. A concurrent pse_isr() on another PI in the same domain can take that freed budget.
regulator_set_current_limit() can then fail. pse_pi_set_current_limit() returns -ERANGE for max_uA > MAX_PI_CURRENT or a null voltage, and ops->pi_set_pw_limit() can also fail. The rollback's regulator_request_power_budget(P - L) then returns -ERANGE.
pw_allocated_mW stays at L, but the hardware keeps the old limit P.
The rollback also overwrites extack with "not enough power budget available". That hides the real regulator_set_current_limit() error.
Quoted 2 lines
drivers/net/pse-pd/pse_core.c +0
-0return ret;
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020171249.853085-1-github%40szelinsky.de
Re: [PATCH net v2 06/14] net: pse-pd: check the PI state under the lock when setting a power limit
Quoted 2 lines · On Fri, 23 Oct 2026 08:13:34 +0000, [email protected] wrote:
[Severity: High]
Doadmin_state_enabledandisr_pd_detectedreally mean the PI is powered?
Kory, a question about pending PIs. A PI can be admin enabled with a PD detected but without power: when there was not enough budget when it was enabled or when the PD was detected, or when a PI with higher priority took its budget. Such a PI passes the check in pse_pi_update_pw_budget(), so setting a power limit reserves budget for it if the limit fits. When the PI is powered later, pse_pi_allocate_pw_budget_static_prio() sets pw_allocated_mW again, and the budget reserved before is never freed.
This was the same before my patch. Should set_pw_limit skip a pending PI, for example with pse_pi_is_admin_enable_pending()? If yes, I would send that as a separate patch.
Carlo
Re: [PATCH net v2 06/14] net: pse-pd: check the PI state under the lock when setting a power limit
Quoted 3 lines · Carlo Szelinsky
This was the same before my patch. Should
set_pw_limitskip a pending PI, for example withpse_pi_is_admin_enable_pending()? If yes, I would send that as a separate patch.
Hi Kory,
a short ping on this question about pending PIs. It does not block v3. I only need your answer for the separate fix.
Thanks,
Carlo
Kory Maincent
maintainerRe: [PATCH net v2 06/14] net: pse-pd: check the PI state under the lock when setting a power limit
In reply to Carlo SzelinskyQuoted 13 lines · Carlo Szelinsky
Quoted 2 lines · On Fri, 23 Oct 2026 08:13:34 +0000, [email protected] wrote:
[Severity: High]
Doadmin_state_enabledandisr_pd_detectedreally mean the PI is powered?Kory, a question about pending PIs. A PI can be admin enabled with a PD detected but without power: when there was not enough budget when it was enabled or when the PD was detected, or when a PI with higher priority took its budget. Such a PI passes the check in
pse_pi_update_pw_budget(), so setting a power limit reserves budget for it if the limit fits. When the PI is powered later,pse_pi_allocate_pw_budget_static_prio()setspw_allocated_mWagain, and the budget reserved before is never freed.
Quoted 3 lines
This was the same before my patch. Should
set_pw_limitskip a pending PI, for example withpse_pi_is_admin_enable_pending()? If yes, I would send that as a separate patch.
Sorry for the late reply.
No, I don't think we should skip setting power limit for a pending PI.
Lowering the power limit of a PI could allow this pending PI to be powered again with a smaller power budget.
Regards,
Signature · 3 lines
Köry Maincent, Bootlin Embedded Linux and kernel engineering https://bootlin.com
Oleksij Rempel
maintainerRe: [PATCH net v2 06/14] net: pse-pd: check the PI state under the lock when setting a power limit
Quoted 16 lines · Kory Maincent
Quoted 13 lines · Carlo Szelinsky
Quoted 2 lines · On Fri, 23 Oct 2026 08:13:34 +0000, [email protected] wrote:
[Severity: High]
Doadmin_state_enabledandisr_pd_detectedreally mean the PI is powered?Kory, a question about pending PIs. A PI can be admin enabled with a PD detected but without power: when there was not enough budget when it was enabled or when the PD was detected, or when a PI with higher priority took its budget. Such a PI passes the check in
pse_pi_update_pw_budget(), so setting a power limit reserves budget for it if the limit fits. When the PI is powered later,pse_pi_allocate_pw_budget_static_prio()setspw_allocated_mWagain, and the budget reserved before is never freed.
Quoted 8 lines
Quoted 3 lines
This was the same before my patch. Should
set_pw_limitskip a pending PI, for example withpse_pi_is_admin_enable_pending()? If yes, I would send that as a separate patch.Sorry for the late reply.
No, I don't think we should skip setting power limit for a pending PI. Lowering the power limit of a PI could allow this pending PI to be powered again with a smaller power budget.
Ack, makes sense.
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 |
PATCH v2 7/14 net: pse-pd: do not tie a shared power domain to its creator's devres
In reply to Carlo SzelinskyA power domain is shared by all controllers on the same supply and counted with a kref, but it is devm memory of the controller that created it. When that controller unbinds while another one still uses the domain, devres frees it anyway, and the other controller and the global map keep pointing at freed memory. The same happens when pse_register_pw_ds() fails part way.
Allocate the domain with kzalloc() and free it on its last kref_put(). A domain left over from a failed pse_register_pw_ds() is now leaked instead of freed while still in the map.
Because the domain can now be freed at the put, pse_flush_pw_ds() clears pi[].pw_d under pcdev->lock before the put, and pse_ethtool_set_prio() checks it under that lock. The PI regulator ops and pse_isr() use it under the same lock.
Triggered in QEMU (arm64, KASAN) with two test-only PSE controllers on one supply: after the one that created the domain unbinds, binding it again gives
BUG: KASAN: slab-use-after-free in pse_controller_register+0xb98/0x1274
With this patch it is gone.
--- drivers/net/pse-pd/pse_core.c | 43 ++++++++++++++++++++++++----------- 1 file changed, 30 insertions(+), 13 deletions(-)
drivers/net/pse-pd/pse_core.c +30 -13
@@ -881,6 +881,7 @@ static void __pse_pw_d_release(struct kref *kref)regulator_put (pw_d->supply);xa_erase (&pse_pw_d_map, pw_d->id);mutex_unlock (&pse_pw_d_mutex); +kfree (pw_d); }/**
@@ -893,10 +894,18 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)int i;for (i =0 ; i < pcdev->nr_lines; i++) { -if (!pcdev->pi[i].pw_d) +/* Clear it before the put that may free the domain. The PI +* regulator ops and pse_isr() hold pcdev->lock while they +* use it. +*/ +mutex_lock (&pcdev->lock); + pw_d = pcdev->pi[i].pw_d; + pcdev->pi[i].pw_d =NULL ; +mutex_unlock (&pcdev->lock); +if (!pw_d)continue ; - pw_d =xa_load (&pse_pw_d_map, pcdev->pi[i].pw_d->id); + pw_d =xa_load (&pse_pw_d_map, pw_d->id);if (!pw_d)continue ;
@@ -906,24 +915,30 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev) }/** -* devm_pse_alloc_pw_d - allocate a new PSE power domain for a device -* @dev: device that is registering this PSE power domain +* pse_alloc_pw_d - allocate a new PSE power domain +* +* A power domain is shared by every controller whose PIs sit on the same +* supply, so it must not be tied to the lifetime of the controller that +* happened to create it. It is freed by __pse_pw_d_release() on the last +* kref_put(). * * Return: Pointer to the newly allocated PSE power domain or error pointers */ -static struct pse_power_domain *devm_pse_alloc_pw_d (struct device *dev) +static struct pse_power_domain *pse_alloc_pw_d (void ) {struct pse_power_domain *pw_d;int index, ret; - pw_d =devm_kzalloc (dev,sizeof (*pw_d),GFP_KERNEL ); + pw_d =kzalloc_obj (*pw_d);if (!pw_d)return ERR_PTR (-ENOMEM ); ret =xa_alloc (&pse_pw_d_map, &index, pw_d,XA_LIMIT (1 ,PSE_PW_D_LIMIT ),GFP_KERNEL ); -if (ret) +if (ret) { +kfree (pw_d);return ERR_PTR (ret); + }kref_init (&pw_d->refcnt); pw_d->id = index;
@@ -969,7 +984,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)continue ; } - pw_d =devm_pse_alloc_pw_d (pcdev->dev); + pw_d =pse_alloc_pw_d ();if (IS_ERR (pw_d)) { ret =PTR_ERR (pw_d); goto out;
@@ -978,6 +993,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev) supply =regulator_get (&rdev->dev, rdev->supply_name);if (IS_ERR (supply)) {xa_erase (&pse_pw_d_map, pw_d->id); +kfree (pw_d); ret =PTR_ERR (supply); goto out; }
@@ -1949,15 +1965,16 @@ int pse_ethtool_set_prio(struct pse_control *psec,const struct pse_controller_ops *ops;int ret =0 ; -if (!pcdev->pi[psec->id].pw_d) { -NL_SET_ERR_MSG (extack,"no power domain attached" ); -return -EOPNOTSUPP ; - } -/* We don't want priority change in the middle of an * enable/disable call or a priority mode change */ mutex_lock (&pcdev->lock); +if (!pcdev->pi[psec->id].pw_d) { +NL_SET_ERR_MSG (extack,"no power domain attached" ); + ret = -EOPNOTSUPP ; + goto out; + } +switch (pcdev->pi[psec->id].pw_d->budget_eval_strategy) {case PSE_BUDGET_EVAL_STRAT_STATIC :if (prio >= pcdev->nr_lines) {
Signature · 1 line
2.43.0
Re: [PATCH net v2 07/14] net: pse-pd: do not tie a shared power domain to its creator's devres
Quoted 20 lines · Carlo Szelinsky
A power domain is shared by all controllers on the same supply and counted with a kref, but it is devm memory of the controller that created it. When that controller unbinds while another one still uses the domain, devres frees it anyway, and the other controller and the global map keep pointing at freed memory. The same happens when
pse_register_pw_ds()fails part way.Allocate the domain with
kzalloc()and free it on its lastkref_put(). A domain left over from a failedpse_register_pw_ds()is now leaked instead of freed while still in the map.Because the domain can now be freed at the put,
pse_flush_pw_ds()clears pi[].pw_d underpcdev->lockbefore the put, andpse_ethtool_set_prio()checks it under that lock. The PI regulator ops andpse_isr()use it under the same lock.Triggered in QEMU (arm64, KASAN) with two test-only PSE controllers on one supply: after the one that created the domain unbinds, binding it again gives
Quoted 6 lines
BUG: KASAN: slab-use-after-free in pse_controller_register+0xb98/0x1274With this patch it is gone.
Fixes 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")Signed-off-by Carlo Szelinsky <[email protected]>
The power domain lifetime changes in 7 and 10-12 build on each other; if there is another round, folding them into fewer patches would make them easier to review.
Thank you!
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 |
Re: [PATCH net v2 07/14] net: pse-pd: do not tie a shared power domain to its creator's devres
Quoted 3 lines · Oleksij Rempel
The power domain lifetime changes in 7 and 10-12 build on each other; if there is another round, folding them into fewer patches would make them easier to review.
Thanks for the review. For v3 I would merge 7, the power domain part of 11 and 12 into one patch. Then no commit in between still has the supply handle problem or the leak on a failed registration that sashiko found in 7. The kfifo part of 11 stays a small patch. The release function is then pse_pw_d_release() from the start, with the lockdep_assert_held() you asked for.
I would keep 10 on its own. Without pse_pi_power_off() it only moves the free of the PI array to devres and adds the unregistering flag. It does not touch the power domain any more.
One downside: Mark and Liam would then have to review the rdev->supply change inside a bigger patch. If you prefer, I can keep 12 separate for them and only merge 7 and the power domain part of 11.
Is that what you meant?
Carlo
Re: [PATCH net v2 07/14] net: pse-pd: do not tie a shared power domain to its creator's devres
Quoted 5 lines · Carlo Szelinsky
One downside: Mark and Liam would then have to review the
rdev->supplychange inside a bigger patch. If you prefer, I can keep 12 separate for them and only merge 7 and the power domain part of 11.Is that what you meant?
Hi Oleksij,
a short ping on this. The answer decides how I build v3, and I would like to send it soon.
If I hear nothing by Monday, November 2, I will keep 12 separate and merge only 7 and the power domain part of 11.
Mark, Liam: if you find a moment, Oleksij's question on 12/14 about the PSE core using rdev->supply is still open:
https://lore.kernel.org/netdev/[email protected]/
Thanks,
Carlo
Oleksij Rempel
maintainerRe: [PATCH net v2 07/14] net: pse-pd: do not tie a shared power domain to its creator's devres
Hi Carlo,
Quoted 6 lines · Carlo Szelinsky
Quoted 5 lines · Carlo Szelinsky
One downside: Mark and Liam would then have to review the
rdev->supplychange inside a bigger patch. If you prefer, I can keep 12 separate for them and only merge 7 and the power domain part of 11.Is that what you meant?
ack
Quoted 7 lines
Hi Oleksij,
a short ping on this. The answer decides how I build v3, and I would like to send it soon.
If I hear nothing by Monday, November 2, I will keep 12 separate and merge only 7 and the power domain part of 11.
ack.
Quoted 3 lines
Mark, Liam: if you find a moment, Oleksij's question on 12/14 about the PSE core using
rdev->supplyis still open:
https://lore.kernel.org/netdev/[email protected]/
On 8, I agree with Sashiko, please move the unregistering flag from 10 into 8:
https://lore.kernel.org/netdev/[email protected]/
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 |
[email protected]
AI reviewRe: [PATCH net v2 07/14] net: pse-pd: do not tie a shared power domain to its creator's devres
In reply to Carlo SzelinskyThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider.
Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0 (1)
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [High] At this commit, the shared power domain still depends on its creator's lifetime through pw_d->supply.
- [Medium] At this commit, a partial failure in pse_register_pw_ds() leaks the power domains that were already set up.
Pre-existing issues:
- [High] At this commit, pse_controller_unregister() calls pse_flush_pw_ds() and then pse_release_pis(), which kfree()s pcdev->pi…
- [Low] pse_register_pw_ds() publishes pcdev->pi[i].pw_d (line 978 for a shared domain, line 1007 for a new one) holding only pse_pw_d_mutex,…
--
net: pse-pd: do not tie a shared power domain to its creator's devres
This commit allocates PSE power domains with kzalloc() instead of devm_kzalloc(), and frees them from __pse_pw_d_release() on the last kref_put(). pse_flush_pw_ds() now clears pi[].pw_d under pcdev->lock before the put, and pse_ethtool_set_prio() checks pw_d under that lock.
Quoted 4 lines
drivers/net/pse-pd/pse_core.c +0
-0
[ ... ]
drivers/net/pse-pd/pse_core.c static void pse_flush_pw_ds(struct pse_controller_dev *pcdev) · line 904
Quoted 13 lines
drivers/net/pse-pd/pse_core.c +8
-1@@ -893,10 +894,18 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)int i;for (i =0 ; i < pcdev->nr_lines; i++) { -if (!pcdev->pi[i].pw_d) +/* Clear it before the put that may free the domain. The PI +* regulator ops and pse_isr() hold pcdev->lock while they +* use it. +*/ +mutex_lock (&pcdev->lock); + pw_d = pcdev->pi[i].pw_d; + pcdev->pi[i].pw_d =NULL ; +mutex_unlock (&pcdev->lock);
[Severity: High]
This is a pre-existing issue, but pcdev->lock here protects the pw_d pointer, not the pcdev->pi array that holds it. Can the PI regulator ops or pse_isr() still reach a freed pcdev->pi after this?
pse_controller_unregister() does:
pse_flush_pw_ds(pcdev); pse_release_pis(pcdev); if (pcdev->irq) disable_irq(pcdev->irq);
pse_release_pis() ends with kfree(pcdev->pi) and does not take pcdev->lock.
The IRQ is disabled only after the free. The devm-registered PI regulators are unregistered even later by devres, because devm_pse_controller_release() runs before them in reverse devres order.
In that window, a sysfs "state" read or a consumer's regulator_disable() can reach pse_pi_is_enabled() or pse_pi_disable(). They do:
mutex_lock(&pcdev->lock);
if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d)) {and that reads the freed array. If the driver's IRQ outlives unregister, pse_isr() also reads pcdev->pi[i] under the same lock.
Later commits in the series fix this. "net: pse-pd: stop async event sources before freeing PI data in unregister" moves disable_irq() and cancel_work_sync() to the start. "net: pse-pd: keep the PI data until the PI regulators are gone" frees pcdev->pi from a devm action registered before the PI regulators, and sets pcdev->unregistering under pcdev->lock. One of those commit messages shows a KASAN slab-use-after-free in pse_pi_is_enabled().
[Severity: Low]
This is also a pre-existing issue. The comment says pw_d users rely on pcdev->lock, but the writer in pse_register_pw_ds() doesn't take it. It publishes the pointer while holding only pse_pw_d_mutex:
if (pcdev->supp_budget_eval_strategies) pw_d->budget_eval_strategy = pcdev->supp_budget_eval_strategies; else pw_d->budget_eval_strategy = PSE_BUDGET_EVAL_STRAT_DISABLED; kref_init(&pw_d->refcnt); pcdev->pi[i].pw_d = pw_d;
The shared-domain branch does the same with pcdev->pi[i].pw_d = pw_d.
devm_pse_pi_regulator_register() has already registered the PI regulators earlier in pse_controller_register().
So a concurrent sysfs "state" read could run pse_pi_is_enabled() and see a non-NULL pw_d with a stale budget_eval_strategy. Would it then fall back to the pse_pi_is_hw_enabled() path?
The impact looks limited to one wrong is_enabled answer during probe. The publish path is still unlocked at the end of the series.
Quoted 4 lines
drivers/net/pse-pd/pse_core.c +1
-1continue ; - pw_d =xa_load (&pse_pw_d_map, pcdev->pi[i].pw_d->id); + pw_d =xa_load (&pse_pw_d_map, pw_d->id);
[ ... ]
drivers/net/pse-pd/pse_core.c static int pse_register_pw_ds(struct pse_controller_dev *pcdev) · line 993
Quoted 11 lines
drivers/net/pse-pd/pse_core.c +1
-1@@ -969,7 +984,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)continue ; } - pw_d =devm_pse_alloc_pw_d (pcdev->dev); + pw_d =pse_alloc_pw_d ();if (IS_ERR (pw_d)) { ret =PTR_ERR (pw_d); goto out;@@ -978,6 +993,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev) supply =regulator_get (&rdev->dev, rdev->supply_name);
[Severity: High]
Is the shared domain still tied to its creator's lifetime through pw_d->supply?
regulator_get() is called with &rdev->dev, and rdev is this controller's devm-registered PI regulator. The regulator core records rdev->dev as the consumer device and adds the handle to the parent supply's consumer_list. The handle is then stored in pw_d->supply.
With this patch the domain can outlive its creator. Suppose the creating controller unbinds while another controller still holds a reference. Its PI rdev is unregistered and freed, but pw_d->supply->dev still points to it.
Would a read of the debugfs regulator_summary then dereference the freed device through consumer->dev in regulator_summary_show_subtree()? And would the final put also use the stale dev pointer?
__pse_pw_d_release()
regulator_put(pw_d->supply)
destroy_regulator()
This conflicts with the new kernel-doc on pse_alloc_pw_d(), which says the domain "must not be tied to the lifetime of the controller that happened to create it".
The later commit "net: pse-pd: use each PI's own supply handle for the power budget" removes the supply member from struct pse_power_domain. Its commit message shows a KASAN slab-use-after-free in regulator_summary_show_subtree().
drivers/net/pse-pd/pse_core.c static int pse_register_pw_ds(struct pse_controller_dev *pcdev) · line 999
Quoted 6 lines
drivers/net/pse-pd/pse_core.c +1
-0if (IS_ERR (supply)) {xa_erase (&pse_pw_d_map, pw_d->id); +kfree (pw_d); ret =PTR_ERR (supply); goto out; }
[Severity: Medium]
Does a failure partway through this loop leak the domains already set up for earlier PIs?
Each earlier iteration either kref_init()s a new kzalloc'd domain or kref_get()s a shared one, and stores it in pcdev->pi[j].pw_d. If pse_alloc_pw_d() or regulator_get() then fails for a later PI, the code jumps to:
out:
mutex_unlock(&pse_pw_d_mutex); return ret;
That label does no unwinding, and pse_controller_register() returns without calling pse_flush_pw_ds():
ret = pse_register_pw_ds(pcdev); if (ret) return ret;
The domain is no longer devm memory, so nothing frees it. The struct, its supply handle and its xarray slot all leak, and a shared domain keeps one extra reference.
The commit message mentions this ("is now leaked instead of freed while still in the map"). The later commit "net: pse-pd: unwind allocations when controller registration fails" adds an unwind loop using pse_pi_take_pw_d() and pse_pw_d_leave() before pse_pw_d_mutex is dropped.
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020171249.853085-1-github%40szelinsky.de
PATCH v2 8/14 net: pse-pd: stop async event sources before freeing PI data in unregister
In reply to Carlo Szelinskypse_controller_unregister() flushes the power domains and frees the PI array before it disables the IRQ, while pse_isr() uses both. No in-tree driver hits this: tps23881, the only user of devm_pse_irq_helper(), requests its IRQ after it registers the controller, so devres frees the IRQ before unregister runs. A driver that keeps its IRQ across unregister would read freed memory.
Disable the IRQ and drain the notification worker first, and only then flush the power domains and release the PI array. The next patches in this series build on this order.
Found by code inspection. Not reproduced.
--- drivers/net/pse-pd/pse_core.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/net/pse-pd/pse_core.c +6 -2
@@ -1137,11 +1137,15 @@ EXPORT_SYMBOL_GPL(pse_controller_register);*/ void pse_controller_unregister (struct pse_controller_dev *pcdev) { -pse_flush_pw_ds (pcdev); -pse_release_pis (pcdev); +/* Stop the IRQ and the notification worker first: pse_isr() uses +* pcdev->pi and the power domains that pse_flush_pw_ds() drops, +* and the worker reads the kfifo that is freed below. +*/ if (pcdev->irq)disable_irq (pcdev->irq);cancel_work_sync (&pcdev->ntf_work); +pse_flush_pw_ds (pcdev); +pse_release_pis (pcdev);kfifo_free (&pcdev->ntf_fifo);mutex_lock (&pse_list_mutex);list_del (&pcdev->list);
Signature · 1 line
2.43.0
Re: [PATCH net v2 08/14] net: pse-pd: stop async event sources before freeing PI data in unregister
Quoted 7 lines · Carlo Szelinsky
pse_controller_unregister()flushes the power domains and frees the PI array before it disables the IRQ, whilepse_isr()uses both. No in-tree driver hits this: tps23881, the only user ofdevm_pse_irq_helper(), requests its IRQ after it registers the controller, so devres frees the IRQ before unregister runs. A driver that keeps its IRQ across unregister would read freed memory.
Quoted 8 lines
Disable the IRQ and drain the notification worker first, and only then flush the power domains and release the PI array. The next patches in this series build on this order.
Found by code inspection. Not reproduced.
Reviewed-by Kory Maincent <[email protected]>Signed-off-by Carlo Szelinsky <[email protected]>
Thank you!
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 |
[email protected]
AI reviewRe: [PATCH net v2 08/14] net: pse-pd: stop async event sources before freeing PI data in unregister
In reply to Carlo SzelinskyThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider.
Critical: 0 · High: 1 · Medium: 0 · Low: 0
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [High] This patch's reorder lets ntf_work be queued again after cancel_work_sync().
--
net: pse-pd: stop async event sources before freeing PI data in unregister
This commit reorders pse_controller_unregister() so the IRQ is disabled and the notification worker is cancelled before the power domains are flushed and the PI array is released. The aim is to stop pse_isr() and the worker from using freed PI data.
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +0
-2@@ -1137,11 +1137,15 @@ EXPORT_SYMBOL_GPL(pse_controller_register);*/ void pse_controller_unregister (struct pse_controller_dev *pcdev) { -pse_flush_pw_ds (pcdev); -pse_release_pis (pcdev);
drivers/net/pse-pd/pse_core.c EXPORT_SYMBOL_GPL(pse_controller_register); · line 1149
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +6
-0+/* Stop the IRQ and the notification worker first: pse_isr() uses +* pcdev->pi and the power domains that pse_flush_pw_ds() drops, +* and the worker reads the kfifo that is freed below. +*/ if (pcdev->irq)disable_irq (pcdev->irq);cancel_work_sync (&pcdev->ntf_work); +pse_flush_pw_ds (pcdev); +pse_release_pis (pcdev);kfifo_free (&pcdev->ntf_fifo);
[Severity: High]
Can ntf_work be queued again after this cancel_work_sync()?
pse_isr() is not the only producer. pse_disable_pi_pol() also queues the worker, and the IRQ has nothing to do with it:
drivers/net/pse-pd/pse_core.c:pse_disable_pi_pol() {
...
kfifo_in_spinlocked(&pcdev->ntf_fifo, &ntf, 1, &pcdev->ntf_fifo_lock);
schedule_work(&pcdev->ntf_work);
...
}It is reached through this chain:
_pse_pi_delivery_power_sw_pw_ctrl()
pse_pi_allocate_pw_budget()
pse_pi_allocate_pw_budget_static_prio()
pse_disable_pi_prio()
pse_disable_pi_pol()
That chain can start from three places:
- pse_pi_enable(). The PI regulators are devres-managed, so they are still registered when unregister runs.
- _pse_pi_disable()->pse_pw_d_retry_power_delivery().
- pse_ethtool_set_prio().
On these paths the only gate is pse_pw_d_is_sw_pw_control(). For a STATIC budget domain it returns true until pse_flush_pw_ds() sets pw_d to NULL:
if (pw_d->budget_eval_strategy == PSE_BUDGET_EVAL_STRAT_STATIC) return true;
The old order cleared pw_d before the final cancel_work_sync(). Take tps23881 as an example: it uses the STATIC strategy and allocates priv with devm_kzalloc(). With the new order, this sequence looks possible:
CPU0 CPU1pse_controller_unregister()
disable_irq()
cancel_work_sync(&pcdev->ntf_work)
pse_ethtool_set_prio()
mutex_lock(&pcdev->lock)
... goes over budget ...
pse_disable_pi_pol()
schedule_work(&pcdev->ntf_work) pse_flush_pw_ds()
pse_release_pis()
kfifo_free(&pcdev->ntf_fifo)
devres frees priv (embeds pcdev)
pse_send_ntf_worker()
kfifo_out(&pcdev->ntf_fifo, ...)Would pse_send_ntf_worker() then use the freed pcdev and the freed kfifo?
The new comment says the worker is stopped first. Nothing seems to stop it from being queued again between cancel_work_sync() and pse_flush_pw_ds().
A later patch in this series, "net: pse-pd: keep the PI data until the PI regulators are gone", appears to close this window. It sets pcdev->unregistering under pcdev->lock before cancel_work_sync(). While that flag is set, _pse_pi_delivery_power_sw_pw_ctrl() and pse_pi_enable() return -ENODEV.
Could that check be moved into this patch, or the patches reordered, so the intermediate state does not have this window?
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020171249.853085-1-github%40szelinsky.de
PATCH v2 9/14 net: pse-pd: unregister from the controller list before freeing PI data
In reply to Carlo Szelinskypse_controller_unregister() frees the PI array while the controller is still on pse_controller_list, and only removes it from the list afterwards. of_pse_control_get() walks that list under pse_list_mutex and reads pcdev->pi[i].np in of_pse_match_pi(). A consumer that probes at that moment can read the freed PI array.
Remove the controller from the list first, before the worker is drained, the power domains are flushed and the PI array is freed. Both sides take pse_list_mutex, so once the controller is off the list, no new lookup can find it, and a lookup that is already running finishes before the PI array is freed.
Found by code inspection. Not reproduced.
--- drivers/net/pse-pd/pse_core.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)
drivers/net/pse-pd/pse_core.c +7 -3
@@ -1143,13 +1143,17 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)*/ if (pcdev->irq)disable_irq (pcdev->irq); +/* Unlink before tearing down: of_pse_control_get() walks the list +* and reads pcdev->pi[], and must not hand out a new control for a +* controller that is going away. +*/ +mutex_lock (&pse_list_mutex); +list_del (&pcdev->list); +mutex_unlock (&pse_list_mutex);cancel_work_sync (&pcdev->ntf_work);pse_flush_pw_ds (pcdev);pse_release_pis (pcdev);kfifo_free (&pcdev->ntf_fifo); -mutex_lock (&pse_list_mutex); -list_del (&pcdev->list); -mutex_unlock (&pse_list_mutex); }EXPORT_SYMBOL_GPL (pse_controller_unregister);
Signature · 1 line
2.43.0
Re: [PATCH net v2 09/14] net: pse-pd: unregister from the controller list before freeing PI data
Quoted 12 lines · Carlo Szelinsky
pse_controller_unregister()frees the PI array while the controller is still onpse_controller_list, and only removes it from the list afterwards.of_pse_control_get()walks that list underpse_list_mutexand reads pcdev->pi[i].np inof_pse_match_pi(). A consumer that probes at that moment can read the freed PI array.Remove the controller from the list first, before the worker is drained, the power domains are flushed and the PI array is freed. Both sides take
pse_list_mutex, so once the controller is off the list, no new lookup can find it, and a lookup that is already running finishes before the PI array is freed.
Quoted 5 lines
Found by code inspection. Not reproduced.
Fixes 9be9567a7c59 ("net: pse-pd: Add support for PSE PIs")Reviewed-by Kory Maincent <[email protected]>Signed-off-by Carlo Szelinsky <[email protected]>
Thank you!
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 |
PATCH v2 10/14 net: pse-pd: keep the PI data until the PI regulators are gone
In reply to Carlo Szelinskypse_controller_unregister() frees the PI array, but the PI regulators are only unregistered after it, by devres. Until then their ops still run, for example on a read of the regulator's sysfs "state" or when a consumer enables or disables the PI, and they read the freed array.
Free the PI array from a devm action that is added before the PI regulators, so devres frees it after them. This also frees it when registration fails, where it was leaked before.
While the controller goes away, the PSE core must not power a PI. pse_controller_unregister() now first sets pcdev->unregistering under pcdev->lock. After that, enabling a PI fails with -ENODEV, and so does _pse_pi_delivery_power_sw_pw_ctrl(), which pse_isr(), a priority change and the retry after a disable use to power a PI. Disabling a PI still switches it off and frees its budget.
pse_flush_pw_ds() also switches off each PI that holds budget and gives the budget back before it drops the power domain. Nothing controls such a PI once its controller is gone, and before, its budget stayed reserved on the supply. If a PI cannot be switched off, its budget stays reserved, as the PI may still draw it.
Triggered in QEMU (arm64, KASAN) with a test-only PSE controller:
reading the PI regulators' sysfs "state" while the controller unbinds gives
BUG: KASAN: slab-use-after-free in pse_pi_is_enabled+0x294/0x2d4
and unbinding it with budget reserved leaves that budget on the supply. With this patch both are gone, the PIs are off after the unbind, and a priority change during the unbind no longer powers a PI. pse_isr() is not covered by the test, as the test controller has no IRQ.
--- drivers/net/pse-pd/pse_core.c | 54 ++++++++++++++++++++++++++++++++++- include/linux/pse-pd/pse.h | 3 ++ 2 files changed, 56 insertions(+), 1 deletion(-)
drivers/net/pse-pd/pse_core.c +53 -1
@@ -147,6 +147,11 @@ static void pse_release_pis(struct pse_controller_dev *pcdev)kfree (pcdev->pi); } +static void devm_pse_release_pis (void *data) +{ +pse_release_pis (data); +} +/** * of_load_pse_pis - load all the PSE PIs * @pcdev: a pointer to the PSE controller device
@@ -612,6 +617,13 @@ static int _pse_pi_delivery_power_sw_pw_ctrl(struct pse_controller_dev *pcdev,struct pse_pi *pi = &pcdev->pi[id];int ret, pw_req; +/* pse_isr(), a priority change and the retry after a disable all +* power a PI through here, so stop them here once unregistering +* has started. +*/ +if (pcdev->unregistering) +return -ENODEV ; +if (!ops->pi_get_pw_req) {/* No power allocation management */ ret = ops->pi_enable (pcdev, id);
@@ -668,6 +680,11 @@ static int pse_pi_enable(struct regulator_dev *rdev) id =rdev_get_id (rdev);mutex_lock (&pcdev->lock); +if (pcdev->unregistering) { +mutex_unlock (&pcdev->lock); +return -ENODEV ; + } +if (pse_pw_d_is_sw_pw_control (pcdev, pcdev->pi[id].pw_d)) {/* Manage enabled status by software. * Real enable process will happen if a port is connected.
@@ -884,6 +901,27 @@ static void __pse_pw_d_release(struct kref *kref)kfree (pw_d); } +/* A PI that holds budget is under software power control, and nothing +* controls it once its controller is gone: switch it off, then give the +* budget back. If it cannot be switched off, keep the budget reserved, +* as the PI may still draw it. +*/ +static void pse_pi_power_off (struct pse_controller_dev *pcdev,int id) +{ +int ret; + +if (!pcdev->pi[id].pw_allocated_mW) +return ; + + ret = pcdev->ops->pi_disable ? pcdev->ops->pi_disable (pcdev, id) : + -EOPNOTSUPP ; +if (ret) { +dev_warn (pcdev->dev,"PI %d: cannot switch off: %d\n" , id, ret); +return ; + } +pse_pi_deallocate_pw_budget (&pcdev->pi[id]); +} +/** * pse_flush_pw_ds - flush all PSE power domains of a PSE * @pcdev: a pointer to the initialized PSE controller device
@@ -899,6 +937,7 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)* use it. */ mutex_lock (&pcdev->lock); +pse_pi_power_off (pcdev, i); pw_d = pcdev->pi[i].pw_d; pcdev->pi[i].pw_d =NULL ;mutex_unlock (&pcdev->lock);
@@ -1088,6 +1127,13 @@ int pse_controller_register(struct pse_controller_dev *pcdev)if (ret)return ret; +/* The PI regulator ops use pcdev->pi until devres unregisters the +* regulators, so free it from devres too, after them. +*/ + ret =devm_add_action_or_reset (pcdev->dev, devm_pse_release_pis, pcdev); +if (ret) +return ret; +if (pcdev->ops->setup_pi_matrix) { ret = pcdev->ops->setup_pi_matrix (pcdev);if (ret)
@@ -1137,6 +1183,13 @@ EXPORT_SYMBOL_GPL(pse_controller_register);*/ void pse_controller_unregister (struct pse_controller_dev *pcdev) { +/* The PI regulators stay until devres unregisters them: from now +* on the PSE core must not power a PI. +*/ +mutex_lock (&pcdev->lock); + pcdev->unregistering = true; +mutex_unlock (&pcdev->lock); +/* Stop the IRQ and the notification worker first: pse_isr() uses * pcdev->pi and the power domains that pse_flush_pw_ds() drops, * and the worker reads the kfifo that is freed below.
@@ -1152,7 +1205,6 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)mutex_unlock (&pse_list_mutex);cancel_work_sync (&pcdev->ntf_work);pse_flush_pw_ds (pcdev); -pse_release_pis (pcdev);kfifo_free (&pcdev->ntf_fifo); }EXPORT_SYMBOL_GPL (pse_controller_unregister);
include/linux/pse-pd/pse.h +3 -0
@@ -298,6 +298,8 @@ struct pse_ntf {* @ntf_work: workqueue for PSE notification management * @ntf_fifo: PSE notifications FIFO * @ntf_fifo_lock: protect @ntf_fifo writer +* @unregistering: set under @lock when unregistering starts; the PSE +* core switches no PI on after that */ struct pse_controller_dev {const struct pse_controller_ops *ops;
@@ -317,6 +319,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 */ +bool unregistering; };/**
Signature · 1 line
2.43.0
Oleksij Rempel
maintainerRe: [PATCH net v2 10/14] net: pse-pd: keep the PI data until the PI regulators are gone
Quoted 34 lines · Carlo Szelinsky
pse_controller_unregister()frees the PI array, but the PI regulators are only unregistered after it, by devres. Until then their ops still run, for example on a read of the regulator's sysfs "state" or when a consumer enables or disables the PI, and they read the freed array.Free the PI array from a devm action that is added before the PI regulators, so devres frees it after them. This also frees it when registration fails, where it was leaked before.
While the controller goes away, the PSE core must not power a PI.
pse_controller_unregister()now first setspcdev->unregisteringunderpcdev->lock. After that, enabling a PI fails with-ENODEV, and so does_pse_pi_delivery_power_sw_pw_ctrl(), whichpse_isr(), a priority change and the retry after a disable use to power a PI. Disabling a PI still switches it off and frees its budget.
pse_flush_pw_ds()also switches off each PI that holds budget and gives the budget back before it drops the power domain. Nothing controls such a PI once its controller is gone, and before, its budget stayed reserved on the supply. If a PI cannot be switched off, its budget stays reserved, as the PI may still draw it.Triggered in QEMU (arm64, KASAN) with a test-only PSE controller:
reading the PI regulators' sysfs "state" while the controller unbinds givesBUG: KASAN: slab-use-after-free in pse_pi_is_enabled+0x294/0x2d4and unbinding it with budget reserved leaves that budget on the supply. With this patch both are gone, the PIs are off after the unbind, and a priority change during the unbind no longer powers a PI.
pse_isr()is not covered by the test, as the test controller has no IRQ.
Quoted 3 lines
Fixes d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")Signed-off-by Carlo Szelinsky <[email protected]>
Freeing the PI array from devres after the PI regulators, and refusing to power a PI once unregistering has started, look good to me.
Please drop pse_pi_power_off() from this patch, and the text about it in the commit message and the cover letter.
The PI regulators follow the regulator model. A regulator that goes away does not switch its output off; the consumer has to disable it before it calls regulator_put(). For a PI the consumer is the PSE core itself, on pcdev->dev. So on unbind every enabled PI should go off through the normal disable path, before pse_flush_pw_ds() clears pi->pw_d. The disable path also gives the budget back.
pse_pi_power_off() switches off only the PIs that hold budget. That is not enough: a PI under hardware control, or under software control without pi_get_pw_req(), can deliver power with no budget allocated.
Switching the PIs off on unbind belongs to the fix for a pse_control outliving its controller, not to this patch. Patch 11 calls pse_pi_power_off() from pse_pi_take_pw_d(), so patch 11 needs to drop that call, and the comment above it, too.
Here "off" means what IEEE 802.3 calls mr_pse_enable = disable: no power and no detection, as if there was no PSE. pi_disable() does not do this on every driver today. tps23881 turns detection and classification back on after power off, and the core relies on detection staying on, since it also uses pi_disable() to power off a PI that stays admin enabled. Fixing tps23881 is not part of this series.
pw-bot: cr
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 |
[email protected]
AI reviewRe: [PATCH net v2 10/14] net: pse-pd: keep the PI data until the PI regulators are gone
In reply to Carlo SzelinskyThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider.
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider.
Critical: 0 · High: 1 (1) · Medium: 1 · Low: 0 (1)
If you believe none of the issues are legitimate, please reply to each issue of Medium severity or higher and explain why the AI is wrong. If any of them are legitimate and you plan to respin, please let patchwork know by including "pw-bot: cr" as a separate line at the end of your reply (one such reply per series is enough).
- [High] New AB-BA deadlock between controller unbind and a concurrent consumer regulator_enable()/regulator_disable() on a PI.
- [Medium] If pi_disable() fails during unbind, the budget kept on the supply can never be given back.
Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but the patch does not fix it either.
- [Low] pse_controller_register() returns without kfifo_free() on every failure after kfifo_alloc() (missing callbacks, of_load_pse_pis(), the…
--
net: pse-pd: keep the PI data until the PI regulators are gone
This patch moves the free of the PSE PI array into a devm action that is registered before the PI regulators, so the regulator ops no longer read freed memory during unbind. It adds a pcdev->unregistering flag, and once unregistration starts the PI enable paths fail with -ENODEV. It also makes pse_flush_pw_ds() switch off each PI that holds power budget and give that budget back to the supply.
Quoted 4 lines
drivers/net/pse-pd/pse_core.c +0
-0
[ ... ]
Quoted 12 lines
drivers/net/pse-pd/pse_core.c +8
-0@@ -884,6 +901,27 @@ static void __pse_pw_d_release(struct kref *kref)kfree (pw_d); } +/* A PI that holds budget is under software power control, and nothing +* controls it once its controller is gone: switch it off, then give the +* budget back. If it cannot be switched off, keep the budget reserved, +* as the PI may still draw it. +*/ +static void pse_pi_power_off (struct pse_controller_dev *pcdev,int id) +{ +int ret;
drivers/net/pse-pd/pse_core.c static void __pse_pw_d_release(struct kref *kref) · line 921
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +10
-0+ +if (!pcdev->pi[id].pw_allocated_mW) +return ; + + ret = pcdev->ops->pi_disable ? pcdev->ops->pi_disable (pcdev, id) : + -EOPNOTSUPP ; +if (ret) { +dev_warn (pcdev->dev,"PI %d: cannot switch off: %d\n" , id, ret); +return ; + }
[Severity: Medium]
If pi_disable() fails here, can anything release this budget later?
pse_flush_pw_ds() clears pw_d right after this returns, whether or not the switch-off worked:
pse_pi_power_off(pcdev, i); pw_d = pcdev->pi[i].pw_d; pcdev->pi[i].pw_d = NULL;
After that, pse_pi_deallocate_pw_budget() always returns early:
if (!pi->pw_d || !pi->pw_allocated_mW) return;
So neither a later successful _pse_pi_disable() nor a rebind gives the budget back to the supply. regulator_put() does not touch pw_requested_mW either.
Also, is "the PI may still draw it" true for tps23881? tps23881_pi_disable() first writes PWOFF to TPS23881_REG_PW_EN. Only after that does it do several I2C read-modify-writes (DISC_EN, DET_CLA_EN, power policy). If any of those fails, it returns an error even though the port is already unpowered.
pse_pi_take_pw_d() still handles it the same way at the end of the series.
Quoted 7 lines
drivers/net/pse-pd/pse_core.c +3
-0+pse_pi_deallocate_pw_budget (&pcdev->pi[id]); +} +/** * pse_flush_pw_ds - flush all PSE power domains of a PSE * @pcdev: a pointer to the initialized PSE controller device
drivers/net/pse-pd/pse_core.c static void pse_flush_pw_ds(struct pse_controller_dev *pcdev) · line 943
Quoted 7 lines
drivers/net/pse-pd/pse_core.c +1
-0@@ -899,6 +937,7 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)* use it. */ mutex_lock (&pcdev->lock); +pse_pi_power_off (pcdev, i); pw_d = pcdev->pi[i].pw_d; pcdev->pi[i].pw_d =NULL ;mutex_unlock (&pcdev->lock);
[Severity: High]
Can this deadlock with a consumer that calls regulator_disable() on the same PI?
When a PI has pw_allocated_mW set, this takes the supply regulator lock while pcdev->lock is held:
pse_flush_pw_ds()
mutex_lock(&pcdev->lock)
pse_pi_power_off()
pse_pi_deallocate_pw_budget()
regulator_free_power_budget(pi->pw_d->supply, ...)
regulator_lock(rdev)The regulator core takes the same locks in the other order. One example is an ethtool admin disable, which runs with rtnl held:
pse_ethtool_c33_set_config()
regulator_disable(psec->ps)
regulator_lock_dependent()
regulator_lock_recursive() /* PI rdev, then supply rdev */
_regulator_disable()
pse_pi_disable()
mutex_lock(&pcdev->lock)
Suppose the ethtool task holds the PI and supply rdev locks and waits on pcdev->lock, while the unbind holds pcdev->lock and waits in regulator_lock() for the supply rdev. Then neither task can make progress. regulator_lock() passes a NULL ww_acquire_ctx, so there is no -EDEADLK back-off, and the ethtool task keeps holding rtnl.
pse_controller_unregister() does not remove the PHY's pse_control, so this consumer call can run during the unbind. __pse_control_release() on PHY removal also calls regulator_disable().
pse_isr() and pse_pi_update_pw_budget() already take these locks in this order, but this adds a new case on the teardown path. At the end of the series pse_pi_take_pw_d() still calls pse_pi_power_off() under pcdev->lock, and the supply handle it uses is the same supply rdev.
drivers/net/pse-pd/pse_core.c int pse_controller_register(struct pse_controller_dev *pcdev) · line 1135
Quoted 10 lines
drivers/net/pse-pd/pse_core.c +6
-0@@ -1088,6 +1127,13 @@ int pse_controller_register(struct pse_controller_dev *pcdev)if (ret)return ret; +/* The PI regulator ops use pcdev->pi until devres unregisters the +* regulators, so free it from devres too, after them. +*/ + ret =devm_add_action_or_reset (pcdev->dev, devm_pse_release_pis, pcdev); +if (ret) +return ret;
[Severity: Low]
This is a pre-existing issue, but this new return skips kfifo_free(&pcdev->ntf_fifo), as do the other error returns after kfifo_alloc() in pse_controller_register(). devm_pse_controller_register() does not call pse_controller_unregister() when registration fails. Does that leak the ntf_fifo buffer on every probe failure, including -EPROBE_DEFER retries?
This seems to be fixed later in the series by "net: pse-pd: unwind allocations when controller registration fails", where every error path goes to free_kfifo.
[ ... ]
drivers/net/pse-pd/pse_core.c void pse_controller_unregister(struct pse_controller_dev *pcdev) · line 1209
Quoted 7 lines
drivers/net/pse-pd/pse_core.c +0
-1@@ -1152,7 +1205,6 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)mutex_unlock (&pse_list_mutex);cancel_work_sync (&pcdev->ntf_work);pse_flush_pw_ds (pcdev); -pse_release_pis (pcdev);kfifo_free (&pcdev->ntf_fifo); }
[Severity: High]
This isn't a bug introduced by this patch, and the patch doesn't fix it
either: pse_control objects that a PHY still holds after the controller unbinds.
pse_controller_unregister() only takes pcdev off pse_controller_list. It never walks pcdev->pse_control_head, so each psec keeps its psec->pcdev pointer.
After the unbind, devres frees pcdev->pi through devm_pse_release_pis(). It also frees the driver private data that contains pcdev. The psec is only dropped later, in phy_device_remove()->pse_control_put().
Can a later ETHTOOL_MSG_PSE_GET then access freed memory? That command has no admin permission flag, and it reaches:
pse_get_pse_attributes()
pse_ethtool_get_status()
pi = &pcdev->pi[psec->id];
mutex_lock(&pcdev->lock);It then calls ops function pointers loaded from the freed pcdev.
Removing the PHY later has the same problem. __pse_control_release() reads psec->pcdev->pi[psec->id].admin_state_enabled and calls regulator_disable()/regulator_put() on a regulator whose rdev has already been unregistered.
Signature · 1 line
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261020171249.853085-1-github%40szelinsky.de
PATCH v2 11/14 net: pse-pd: unwind allocations when controller registration fails
In reply to Carlo SzelinskyWhen pse_controller_register() fails, it does not free the notification kfifo. Worse, if pse_register_pw_ds() fails part way, the power domains it already set up stay in the global map. Their supply handle belongs to a PI regulator that devres then frees, so the next user of the map reads freed memory.
Free the kfifo on every error path, and let pse_register_pw_ds() undo its own power domains while it still holds pse_pw_d_mutex, so no other controller can pick one up first. The PI array is already freed by devres.
Found by code inspection and AI review. Triggered in QEMU (arm64, KASAN) with a test-only PSE controller and an injected failure in pse_register_pw_ds() at the third PI: reading the regulator summary in debugfs then gives
BUG: KASAN: slab-use-after-free in regulator_summary_show_subtree+0x770/0x9c4
With this patch the failure is undone, and kmemleak reports nothing.
--- drivers/net/pse-pd/pse_core.c | 73 +++++++++++++++++++++++++---------- 1 file changed, 52 insertions(+), 21 deletions(-)
drivers/net/pse-pd/pse_core.c +52 -21
@@ -889,7 +889,8 @@ devm_pse_pi_regulator_register(struct pse_controller_dev *pcdev,return 0 ; } -static void __pse_pw_d_release (struct kref *kref) +/* Release for a put made with pse_pw_d_mutex already held. */ +static void pse_pw_d_release_locked (struct kref *kref) { struct pse_power_domain *pw_d = container_of(kref, struct pse_power_domain,
@@ -897,7 +898,6 @@ static void __pse_pw_d_release(struct kref *kref)regulator_put (pw_d->supply);xa_erase (&pse_pw_d_map, pw_d->id); -mutex_unlock (&pse_pw_d_mutex);kfree (pw_d); }
@@ -922,6 +922,30 @@ static void pse_pi_power_off(struct pse_controller_dev *pcdev, int id)pse_pi_deallocate_pw_budget (&pcdev->pi[id]); } +static void __pse_pw_d_release (struct kref *kref) +{ +pse_pw_d_release_locked (kref); +mutex_unlock (&pse_pw_d_mutex); +} + +/* Switch off the PI, give back its budget and clear pi[id].pw_d before +* the domain's reference is dropped. The PI regulator ops and pse_isr() +* hold pcdev->lock while they use it. +*/ +static struct pse_power_domain * +pse_pi_take_pw_d (struct pse_controller_dev *pcdev,int id) +{ +struct pse_power_domain *pw_d; + +mutex_lock (&pcdev->lock); +pse_pi_power_off (pcdev, id); + pw_d = pcdev->pi[id].pw_d; + pcdev->pi[id].pw_d =NULL ; +mutex_unlock (&pcdev->lock); + +return pw_d; +} +/** * pse_flush_pw_ds - flush all PSE power domains of a PSE * @pcdev: a pointer to the initialized PSE controller device
@@ -932,15 +956,7 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)int i;for (i =0 ; i < pcdev->nr_lines; i++) { -/* Clear it before the put that may free the domain. The PI -* regulator ops and pse_isr() hold pcdev->lock while they -* use it. -*/ -mutex_lock (&pcdev->lock); -pse_pi_power_off (pcdev, i); - pw_d = pcdev->pi[i].pw_d; - pcdev->pi[i].pw_d =NULL ; -mutex_unlock (&pcdev->lock); + pw_d =pse_pi_take_pw_d (pcdev, i);if (!pw_d)continue ;
@@ -958,8 +974,8 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)* * A power domain is shared by every controller whose PIs sit on the same * supply, so it must not be tied to the lifetime of the controller that -* happened to create it. It is freed by __pse_pw_d_release() on the last -* kref_put(). +* happened to create it. It is freed by pse_pw_d_release_locked() on the +* last kref_put(). * * Return: Pointer to the newly allocated PSE power domain or error pointers */
@@ -1047,6 +1063,13 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev) } out: +/* Undo this call's domains before another controller can find them */ +while (ret && i--) { +struct pse_power_domain *pw_d =pse_pi_take_pw_d (pcdev, i); + +if (pw_d) +kref_put (&pw_d->refcnt, pse_pw_d_release_locked); + }mutex_unlock (&pse_pw_d_mutex);return ret; }
@@ -1120,24 +1143,25 @@ int pse_controller_register(struct pse_controller_dev *pcdev) !pcdev->ops->pi_get_pw_status) {dev_err (pcdev->dev,"Mandatory status report callbacks are missing" ); -return -EINVAL ; + ret = -EINVAL ; + goto free_kfifo; } ret =of_load_pse_pis (pcdev);if (ret) -return ret; + goto free_kfifo;/* The PI regulator ops use pcdev->pi until devres unregisters the * regulators, so free it from devres too, after them. */ ret =devm_add_action_or_reset (pcdev->dev, devm_pse_release_pis, pcdev);if (ret) -return ret; + goto free_kfifo;if (pcdev->ops->setup_pi_matrix) { ret = pcdev->ops->setup_pi_matrix (pcdev);if (ret) -return ret; + goto free_kfifo; }/* Each regulator name len is pcdev dev name + 7 char +
@@ -1154,26 +1178,33 @@ int pse_controller_register(struct pse_controller_dev *pcdev)continue ; reg_name =devm_kzalloc (pcdev->dev, reg_name_len,GFP_KERNEL ); -if (!reg_name) -return -ENOMEM ; +if (!reg_name) { + ret = -ENOMEM ; + goto free_kfifo; + }snprintf (reg_name, reg_name_len,"pse-%s_pi%d" ,dev_name (pcdev->dev), i); ret =devm_pse_pi_regulator_register (pcdev, reg_name, i);if (ret) -return ret; + goto free_kfifo; } ret =pse_register_pw_ds (pcdev);if (ret) -return ret; + goto free_kfifo;mutex_lock (&pse_list_mutex);list_add (&pcdev->list, &pse_controller_list);mutex_unlock (&pse_list_mutex);return 0 ; + +free_kfifo: +cancel_work_sync (&pcdev->ntf_work); +kfifo_free (&pcdev->ntf_fifo); +return ret; }EXPORT_SYMBOL_GPL (pse_controller_register);
Signature · 1 line
2.43.0
Oleksij Rempel
maintainerRe: [PATCH net v2 11/14] net: pse-pd: unwind allocations when controller registration fails
Quoted 16 lines · Carlo Szelinsky
When
pse_controller_register()fails, it does not free the notification kfifo. Worse, ifpse_register_pw_ds()fails part way, the power domains it already set up stay in the global map. Their supply handle belongs to a PI regulator that devres then frees, so the next user of the map reads freed memory.Free the kfifo on every error path, and let
pse_register_pw_ds()undo its own power domains while it still holdspse_pw_d_mutex, so no other controller can pick one up first. The PI array is already freed by devres.Found by code inspection and AI review. Triggered in QEMU (arm64, KASAN) with a test-only PSE controller and an injected failure in
pse_register_pw_ds()at the third PI: reading the regulator summary in debugfs then gives
Quoted 7 lines
BUG: KASAN: slab-use-after-free in regulator_summary_show_subtree+0x770/0x9c4With this patch the failure is undone, and kmemleak reports nothing.
Fixes 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")Signed-off-by Carlo Szelinsky <[email protected]>
Besides the pse_pi_power_off() call (see my reply to patch 10), two small things for the next version.
Helpers that expect pse_pw_d_mutex to be held should check it with lockdep_assert_held(), not only say so in a comment: please add it to pse_pw_d_release_locked() here, and to pse_pw_d_leave() in patch 12.
Once patch 12 removes __pse_pw_d_release(), only one release function is left, so the _locked suffix has nothing to tell apart. Please rename pse_pw_d_release_locked() to pse_pw_d_release() in patch 12 and keep the assert.
pw-bot: cr
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 |
Oleksij Rempel
maintainerRe: [PATCH net v2 11/14] net: pse-pd: unwind allocations when controller registration fails
In reply to Carlo SzelinskyQuoted 16 lines · Carlo Szelinsky
When
pse_controller_register()fails, it does not free the notification kfifo. Worse, ifpse_register_pw_ds()fails part way, the power domains it already set up stay in the global map. Their supply handle belongs to a PI regulator that devres then frees, so the next user of the map reads freed memory.Free the kfifo on every error path, and let
pse_register_pw_ds()undo its own power domains while it still holdspse_pw_d_mutex, so no other controller can pick one up first. The PI array is already freed by devres.Found by code inspection and AI review. Triggered in QEMU (arm64, KASAN) with a test-only PSE controller and an injected failure in
pse_register_pw_ds()at the third PI: reading the regulator summary in debugfs then gives
Quoted 7 lines
BUG: KASAN: slab-use-after-free in regulator_summary_show_subtree+0x770/0x9c4With this patch the failure is undone, and kmemleak reports nothing.
Fixes 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")Fixes ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")Signed-off-by Carlo Szelinsky <[email protected]>
Besides the pse_pi_power_off() call (see my reply to patch 10), two small things for the next version.
Helpers that expect pse_pw_d_mutex to be held should check it with lockdep_assert_held(), not only say so in a comment: please add it to pse_pw_d_release_locked() here, and to pse_pw_d_leave() in patch 12.
Once patch 12 removes __pse_pw_d_release(), only one release function is left, so the _locked suffix has nothing to tell apart. Please rename pse_pw_d_release_locked() to pse_pw_d_release() in patch 12 and keep the assert.
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 |
PATCH v2 12/14 net: pse-pd: use each PI's own supply handle for the power budget
In reply to Carlo SzelinskyA power domain shared by several controllers holds one supply handle. Its consumer device is a PI regulator of the controller that created the domain. When that controller unbinds, devres frees the regulator, but the other controllers still use the domain and its handle, which now points at a freed device. Reading the regulator summary in debugfs then reads freed memory.
The domain does not need a handle of its own. The budget is counted in the supply's regulator_dev, and each PI regulator already holds a handle to that supply, which lives as long as the PI regulator. Use that handle for the budget, and drop the domain's handle. pse_flush_pw_ds() clears a PI's pw_d before devres unregisters the PI regulator, so the budget is never used through a handle that is gone.
To find the domain of a new PI, compare its supply with the supply of a PI already in the domain. For this, keep a list of the PIs of each domain under pse_pw_d_mutex, which pse_flush_pw_ds() now holds for its whole loop.
Triggered in QEMU (arm64, KASAN) with two test-only PSE controllers on one supply: after the one that created the domain unbinds, reading /sys/kernel/debug/regulator/regulator_summary gives
BUG: KASAN: slab-use-after-free in regulator_summary_show_subtree+0x770/0x9c4
With this patch the report is gone. A second test, where one controller changes its power limit in a loop while the other one unbinds, is clean too.
--- drivers/net/pse-pd/pse_core.c | 61 ++++++++++++++++------------------- include/linux/pse-pd/pse.h | 2 ++ 2 files changed, 29 insertions(+), 34 deletions(-)
drivers/net/pse-pd/pse_core.c +27 -34
@@ -45,16 +45,17 @@ struct pse_control {/** * struct pse_power_domain - a PSE power domain * @id: ID of the power domain -* @supply: Power supply the Power Domain * @refcnt: Number of gets of this pse_power_domain * @budget_eval_strategy: Current power budget evaluation strategy of the * power domain +* @pis: PIs that use the power domain, under pse_pw_d_mutex. The supply +* handles of their regulators all point at the same supply. */ struct pse_power_domain {int id; -struct regulator *supply;struct kref refcnt;u32 budget_eval_strategy; +struct list_head pis; }; static int of_load_single_pse_pi_pairset(struct device_node *node,
@@ -442,7 +443,7 @@ static void pse_pi_deallocate_pw_budget(struct pse_pi *pi)if (!pi->pw_d || !pi->pw_allocated_mW)return ; -regulator_free_power_budget (pi->pw_d->supply, pi->pw_allocated_mW); +regulator_free_power_budget (pi->rdev->supply, pi->pw_allocated_mW); pi->pw_allocated_mW =0 ; }
@@ -555,7 +556,7 @@ pse_pi_allocate_pw_budget_static_prio(struct pse_controller_dev *pcdev, int id,int ret, _prio; _prio = pcdev->nr_lines; -while (regulator_request_power_budget (pi->pw_d->supply, pw_req) == -ERANGE ) { +while (regulator_request_power_budget (pi->rdev->supply, pw_req) == -ERANGE ) {if (_prio <= pi->prio) { NL_SET_ERR_MSG_FMT(extack, "PI %d: not enough power budget available",
@@ -896,7 +897,6 @@ static void pse_pw_d_release_locked(struct kref *kref) struct pse_power_domain, refcnt); -regulator_put (pw_d->supply);xa_erase (&pse_pw_d_map, pw_d->id);kfree (pw_d); }
@@ -922,12 +922,6 @@ static void pse_pi_power_off(struct pse_controller_dev *pcdev, int id)pse_pi_deallocate_pw_budget (&pcdev->pi[id]); } -static void __pse_pw_d_release (struct kref *kref) -{ -pse_pw_d_release_locked (kref); -mutex_unlock (&pse_pw_d_mutex); -} -/* Switch off the PI, give back its budget and clear pi[id].pw_d before * the domain's reference is dropped. The PI regulator ops and pse_isr() * hold pcdev->lock while they use it.
@@ -946,6 +940,13 @@ pse_pi_take_pw_d(struct pse_controller_dev *pcdev, int id)return pw_d; } +/* Drop @pi from @pw_d, with pse_pw_d_mutex held */ +static void pse_pw_d_leave (struct pse_power_domain *pw_d,struct pse_pi *pi) +{ +list_del (&pi->pw_d_node); +kref_put (&pw_d->refcnt, pse_pw_d_release_locked); +} +/** * pse_flush_pw_ds - flush all PSE power domains of a PSE * @pcdev: a pointer to the initialized PSE controller device
@@ -955,18 +956,13 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)struct pse_power_domain *pw_d;int i; +mutex_lock (&pse_pw_d_mutex);for (i =0 ; i < pcdev->nr_lines; i++) { pw_d =pse_pi_take_pw_d (pcdev, i); -if (!pw_d) -continue ; - - pw_d =xa_load (&pse_pw_d_map, pw_d->id); -if (!pw_d) -continue ; - -kref_put_mutex (&pw_d->refcnt, __pse_pw_d_release, - &pse_pw_d_mutex); +if (pw_d) +pse_pw_d_leave (pw_d, &pcdev->pi[i]); } +mutex_unlock (&pse_pw_d_mutex); }/**
@@ -1014,7 +1010,6 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)for (i =0 ; i < pcdev->nr_lines; i++) {struct regulator_dev *rdev = pcdev->pi[i].rdev;struct pse_power_domain *pw_d; -struct regulator *supply;bool present = false;unsigned long index;
@@ -1025,10 +1020,14 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)continue ;xa_for_each (&pse_pw_d_map, index, pw_d) { +struct pse_pi *pi =list_first_entry (&pw_d->pis, + struct pse_pi, + pw_d_node); +/* Power supply already registered as a PSE power * domain. */ -if (regulator_is_equal (pw_d->supply, rdev->supply)) { +if (regulator_is_equal (pi->rdev->supply, rdev->supply)) { present = true; pcdev->pi[i].pw_d = pw_d;break ;
@@ -1036,6 +1035,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev) }if (present) {kref_get (&pw_d->refcnt); +list_add_tail (&pcdev->pi[i].pw_d_node, &pw_d->pis);continue ; }
@@ -1045,20 +1045,13 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev) goto out; } - supply =regulator_get (&rdev->dev, rdev->supply_name); -if (IS_ERR (supply)) { -xa_erase (&pse_pw_d_map, pw_d->id); -kfree (pw_d); - ret =PTR_ERR (supply); - goto out; - } - - pw_d->supply = supply;if (pcdev->supp_budget_eval_strategies) pw_d->budget_eval_strategy = pcdev->supp_budget_eval_strategies;else pw_d->budget_eval_strategy =PSE_BUDGET_EVAL_STRAT_DISABLED ;kref_init (&pw_d->refcnt); +INIT_LIST_HEAD (&pw_d->pis); +list_add_tail (&pcdev->pi[i].pw_d_node, &pw_d->pis); pcdev->pi[i].pw_d = pw_d; }
@@ -1068,7 +1061,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)struct pse_power_domain *pw_d =pse_pi_take_pw_d (pcdev, i);if (pw_d) -kref_put (&pw_d->refcnt, pse_pw_d_release_locked); +pse_pw_d_leave (pw_d, &pcdev->pi[i]); }mutex_unlock (&pse_pw_d_mutex);return ret;
@@ -1962,7 +1955,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,if (!pw_diff) { goto out; }else if (pw_diff >0 ) { - ret =regulator_request_power_budget (pi->pw_d->supply, pw_diff); + ret =regulator_request_power_budget (pi->rdev->supply, pw_diff);if (ret) { NL_SET_ERR_MSG_FMT(extack, "PI %d: not enough power budget available",
@@ -1971,7 +1964,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id, } }else { -regulator_free_power_budget (pi->pw_d->supply, -pw_diff); +regulator_free_power_budget (pi->rdev->supply, -pw_diff); } pi->pw_allocated_mW = pw_req; *updated = true;
include/linux/pse-pd/pse.h +2 -0
@@ -247,6 +247,7 @@ struct pse_pi_pairset {* @rdev: regulator represented by the PSE PI * @admin_state_enabled: PI enabled state * @pw_d: Power domain of the PSE PI +* @pw_d_node: entry in the list of PIs that use @pw_d * @prio: Priority of the PSE PI. Used in static budget evaluation strategy * @isr_pd_detected: PSE PI detection status managed by the interruption * handler. This variable is relevant when the power enabled
@@ -261,6 +262,7 @@ struct pse_pi {struct regulator_dev *rdev;bool admin_state_enabled;struct pse_power_domain *pw_d; +struct list_head pw_d_node;int prio;bool isr_pd_detected;int pw_allocated_mW;
Signature · 1 line
2.43.0
Oleksij Rempel
maintainerRe: [PATCH net v2 12/14] net: pse-pd: use each PI's own supply handle for the power budget
Quoted 23 lines · Carlo Szelinsky
A power domain shared by several controllers holds one supply handle. Its consumer device is a PI regulator of the controller that created the domain. When that controller unbinds, devres frees the regulator, but the other controllers still use the domain and its handle, which now points at a freed device. Reading the regulator summary in debugfs then reads freed memory.
The domain does not need a handle of its own. The budget is counted in the supply's
regulator_dev, and each PI regulator already holds a handle to that supply, which lives as long as the PI regulator. Use that handle for the budget, and drop the domain's handle.pse_flush_pw_ds()clears a PI'spw_dbefore devres unregisters the PI regulator, so the budget is never used through a handle that is gone.To find the domain of a new PI, compare its supply with the supply of a PI already in the domain. For this, keep a list of the PIs of each domain under
pse_pw_d_mutex, whichpse_flush_pw_ds()now holds for its whole loop.Triggered in QEMU (arm64, KASAN) with two test-only PSE controllers on one supply: after the one that created the domain unbinds, reading
/sys/kernel/debug/regulator/regulator_summarygives
Quoted 8 lines
BUG: KASAN: slab-use-after-free in regulator_summary_show_subtree+0x770/0x9c4With this patch the report is gone. A second test, where one controller changes its power limit in a loop while the other one unbinds, is clean too.
Fixes 50f8b341d268 ("net: pse-pd: Add support for PSE power domains")Signed-off-by Carlo Szelinsky <[email protected]>
Adding Mark and Liam, as this patch uses regulator core internals. Carlo, please keep them on Cc for the next version.
Mark, Liam: the PSE (Power Sourcing Equipment, for PoE and PoDL) core registers each PI (power interface) as a regulator, and the PIs on one supply share a power budget. This patch drops the PSE core's own struct regulator handle to that supply. It now calls regulator_request_power_budget() and regulator_free_power_budget() on pi->rdev->supply instead. The comment above struct regulator_dev in include/linux/regulator/driver.h says it should not be used by anything except the regulator core. pse_core.c already reads pi->rdev->supply to find which PIs share a supply. With this patch it also relies on the regulator core keeping that handle valid for as long as the PI regulator exists.
Is it OK for the PSE core to use rdev->supply directly, or should the regulator core provide an API for it? If you do not object, I am fine with this patch going to net as a fix.
Carlo: apart from rebasing on the patch 10 and 11 changes, the only changes I ask for here are the ones from my reply to patch 11:lockdep_assert_held() in pse_pw_d_leave(), and renaming pse_pw_d_release_locked() to pse_pw_d_release(). Nothing else unless Mark or Liam object.
Mark, Liam, an alternative for later, not for this series: a PI regulator has no budget limit of its own, and regulator_request_power_budget() already passes a request on to the supply. So the PSE core could request the budget through a consumer handle to the PI regulator instead, but it would have the same lock ordering problem the PSE core already has between its own lock and the regulator locks.
Quoted 212 lines
--- drivers/net/pse-pd/pse_core.c | 61 ++++++++++++++++------------------- include/linux/pse-pd/pse.h | 2 ++ 2 files changed, 29 insertions(+), 34 deletions(-)drivers/net/pse-pd/pse_core.c +27
-34@@ -45,16 +45,17 @@ struct pse_control {/** * struct pse_power_domain - a PSE power domain * @id: ID of the power domain -* @supply: Power supply the Power Domain * @refcnt: Number of gets of this pse_power_domain * @budget_eval_strategy: Current power budget evaluation strategy of the * power domain +* @pis: PIs that use the power domain, under pse_pw_d_mutex. The supply +* handles of their regulators all point at the same supply. */ struct pse_power_domain {int id; -struct regulator *supply;struct kref refcnt;u32 budget_eval_strategy; +struct list_head pis; }; static int of_load_single_pse_pi_pairset(struct device_node *node,@@ -442,7 +443,7 @@ static void pse_pi_deallocate_pw_budget(struct pse_pi *pi)if (!pi->pw_d || !pi->pw_allocated_mW)return ; -regulator_free_power_budget (pi->pw_d->supply, pi->pw_allocated_mW); +regulator_free_power_budget (pi->rdev->supply, pi->pw_allocated_mW); pi->pw_allocated_mW =0 ; }@@ -555,7 +556,7 @@ pse_pi_allocate_pw_budget_static_prio(struct pse_controller_dev *pcdev, int id,int ret, _prio; _prio = pcdev->nr_lines; -while (regulator_request_power_budget (pi->pw_d->supply, pw_req) == -ERANGE ) { +while (regulator_request_power_budget (pi->rdev->supply, pw_req) == -ERANGE ) {if (_prio <= pi->prio) { NL_SET_ERR_MSG_FMT(extack, "PI %d: not enough power budget available",@@ -896,7 +897,6 @@ static void pse_pw_d_release_locked(struct kref *kref) struct pse_power_domain, refcnt); -regulator_put (pw_d->supply);xa_erase (&pse_pw_d_map, pw_d->id);kfree (pw_d); }@@ -922,12 +922,6 @@ static void pse_pi_power_off(struct pse_controller_dev *pcdev, int id)pse_pi_deallocate_pw_budget (&pcdev->pi[id]); } -static void __pse_pw_d_release (struct kref *kref) -{ -pse_pw_d_release_locked (kref); -mutex_unlock (&pse_pw_d_mutex); -} -/* Switch off the PI, give back its budget and clear pi[id].pw_d before * the domain's reference is dropped. The PI regulator ops and pse_isr() * hold pcdev->lock while they use it. @@ -946,6 +940,13 @@ pse_pi_take_pw_d(struct pse_controller_dev *pcdev, int id)return pw_d; } +/* Drop @pi from @pw_d, with pse_pw_d_mutex held */ +static void pse_pw_d_leave (struct pse_power_domain *pw_d,struct pse_pi *pi) +{ +list_del (&pi->pw_d_node); +kref_put (&pw_d->refcnt, pse_pw_d_release_locked); +} +/** * pse_flush_pw_ds - flush all PSE power domains of a PSE * @pcdev: a pointer to the initialized PSE controller device @@ -955,18 +956,13 @@ static void pse_flush_pw_ds(struct pse_controller_dev *pcdev)struct pse_power_domain *pw_d;int i; +mutex_lock (&pse_pw_d_mutex);for (i =0 ; i < pcdev->nr_lines; i++) { pw_d =pse_pi_take_pw_d (pcdev, i); -if (!pw_d) -continue ; - - pw_d =xa_load (&pse_pw_d_map, pw_d->id); -if (!pw_d) -continue ; - -kref_put_mutex (&pw_d->refcnt, __pse_pw_d_release, - &pse_pw_d_mutex); +if (pw_d) +pse_pw_d_leave (pw_d, &pcdev->pi[i]); } +mutex_unlock (&pse_pw_d_mutex); }/** @@ -1014,7 +1010,6 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)for (i =0 ; i < pcdev->nr_lines; i++) {struct regulator_dev *rdev = pcdev->pi[i].rdev;struct pse_power_domain *pw_d; -struct regulator *supply;bool present = false;unsigned long index;@@ -1025,10 +1020,14 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)continue ;xa_for_each (&pse_pw_d_map, index, pw_d) { +struct pse_pi *pi =list_first_entry (&pw_d->pis, + struct pse_pi, + pw_d_node); +/* Power supply already registered as a PSE power * domain. */ -if (regulator_is_equal (pw_d->supply, rdev->supply)) { +if (regulator_is_equal (pi->rdev->supply, rdev->supply)) { present = true; pcdev->pi[i].pw_d = pw_d;break ;@@ -1036,6 +1035,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev) }if (present) {kref_get (&pw_d->refcnt); +list_add_tail (&pcdev->pi[i].pw_d_node, &pw_d->pis);continue ; }@@ -1045,20 +1045,13 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev) goto out; } - supply =regulator_get (&rdev->dev, rdev->supply_name); -if (IS_ERR (supply)) { -xa_erase (&pse_pw_d_map, pw_d->id); -kfree (pw_d); - ret =PTR_ERR (supply); - goto out; - } - - pw_d->supply = supply;if (pcdev->supp_budget_eval_strategies) pw_d->budget_eval_strategy = pcdev->supp_budget_eval_strategies;else pw_d->budget_eval_strategy =PSE_BUDGET_EVAL_STRAT_DISABLED ;kref_init (&pw_d->refcnt); +INIT_LIST_HEAD (&pw_d->pis); +list_add_tail (&pcdev->pi[i].pw_d_node, &pw_d->pis); pcdev->pi[i].pw_d = pw_d; }@@ -1068,7 +1061,7 @@ static int pse_register_pw_ds(struct pse_controller_dev *pcdev)struct pse_power_domain *pw_d =pse_pi_take_pw_d (pcdev, i);if (pw_d) -kref_put (&pw_d->refcnt, pse_pw_d_release_locked); +pse_pw_d_leave (pw_d, &pcdev->pi[i]); }mutex_unlock (&pse_pw_d_mutex);return ret;@@ -1962,7 +1955,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id,if (!pw_diff) { goto out; }else if (pw_diff >0 ) { - ret =regulator_request_power_budget (pi->pw_d->supply, pw_diff); + ret =regulator_request_power_budget (pi->rdev->supply, pw_diff);if (ret) { NL_SET_ERR_MSG_FMT(extack, "PI %d: not enough power budget available",@@ -1971,7 +1964,7 @@ static int pse_pi_update_pw_budget(struct pse_controller_dev *pcdev, int id, } }else { -regulator_free_power_budget (pi->pw_d->supply, -pw_diff); +regulator_free_power_budget (pi->rdev->supply, -pw_diff); } pi->pw_allocated_mW = pw_req; *updated = true;include/linux/pse-pd/pse.h +2
-0@@ -247,6 +247,7 @@ struct pse_pi_pairset {* @rdev: regulator represented by the PSE PI * @admin_state_enabled: PI enabled state * @pw_d: Power domain of the PSE PI +* @pw_d_node: entry in the list of PIs that use @pw_d * @prio: Priority of the PSE PI. Used in static budget evaluation strategy * @isr_pd_detected: PSE PI detection status managed by the interruption * handler. This variable is relevant when the power enabled @@ -261,6 +262,7 @@ struct pse_pi {struct regulator_dev *rdev;bool admin_state_enabled;struct pse_power_domain *pw_d; +struct list_head pw_d_node;int prio;bool isr_pd_detected;int pw_allocated_mW;Signature · 1 line
2.43.0
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 |
PATCH v2 13/14 net: pse-pd: pd692x0: release the manager power budget through devres
In reply to Carlo Szelinskypd692x0 reserves a power budget on each manager's supply in setup_pi_matrix(), but gives it back only on that function's own errors and in remove(). If probe fails later, or the request fails for one manager after others succeeded, the budget stays reserved on vmain. Each probe retry, e.g. after -EPROBE_DEFER, reserves it again until nothing is left.
Release the budget from a devm action instead, and drop the explicit release calls.
Found by code inspection. Not reproduced.
--- drivers/net/pse-pd/pd692x0.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)
drivers/net/pse-pd/pd692x0.c +15 -7
@@ -1224,8 +1224,9 @@ static void pd692x0_of_put_managers(struct pd692x0_priv *priv, } } -static void pd692x0_managers_free_pw_budget (struct pd692x0_priv *priv) +static void pd692x0_managers_free_pw_budget (void *data) { +struct pd692x0_priv *priv = data;int i;for (i =0 ; i <PD692X0_MAX_MANAGERS ; i++) {
@@ -1279,31 +1280,39 @@ static int pd692x0_setup_pi_matrix(struct pse_controller_dev *pcdev)if (ret) goto err_of_managers; +/* Frees whatever pd692x0_req_managers_pw_budget() manages to +* request, on any later probe failure and on unbind. Added after +* the manager regulators so that it runs before devres unregisters +* them and their supply goes away. +*/ + ret =devm_add_action_or_reset (&priv->client->dev, + pd692x0_managers_free_pw_budget, priv); +if (ret) + goto err_of_managers; + ret =pd692x0_req_managers_pw_budget (priv);if (ret) goto err_of_managers; ret =pd692x0_set_ports_matrix (priv, manager);if (ret) - goto err_managers_req_pw; + goto err_of_managers;/* Do not init the conf if it is already saved */ if (!priv->cfg_saved) { ret =pd692x0_hw_conf_init (priv);if (ret) - goto err_managers_req_pw; + goto err_of_managers; ret =pd692x0_save_user_byte (priv);if (ret) - goto err_managers_req_pw; + goto err_of_managers; }pd692x0_of_put_managers (priv, manager);kfree (manager);return 0 ; -err_managers_req_pw: -pd692x0_managers_free_pw_budget (priv); err_of_managers:pd692x0_of_put_managers (priv, manager); err_free_manager:
@@ -1855,7 +1864,6 @@ static void pd692x0_i2c_remove(struct i2c_client *client) {struct pd692x0_priv *priv =i2c_get_clientdata (client); -pd692x0_managers_free_pw_budget (priv);firmware_upload_unregister (priv->fwl); }
Signature · 1 line
2.43.0
PATCH v2 14/14 net: pse-pd: pd692x0: defer probe while a manager supply is unresolved
In reply to Carlo Szelinskypd692x0_req_managers_pw_budget() uses each manager regulator's supply without a NULL check. The supply is NULL while the vmain regulator is described but not registered yet, because regulator_register() does not wait for its supply. Probe then crashes.
Return -EPROBE_DEFER in that case.
Found by code inspection. Not reproduced.
--- drivers/net/pse-pd/pd692x0.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/net/pse-pd/pd692x0.c +9 -0
@@ -1056,6 +1056,15 @@ pd692x0_req_managers_pw_budget(struct pd692x0_priv *priv)struct regulator *supply = priv->manager_reg[i]->supply;int pw_budget; +/* The regulator core registers the manager regulator even +* when its vmain supply cannot be resolved yet and retries +* later, leaving ->supply NULL until then. +*/ +if (!supply) +return dev_err_probe (&priv->client->dev, -EPROBE_DEFER , +"manager %d: vmain supply not resolved\n" , + i); + pw_budget =regulator_get_unclaimed_power_budget (supply);if (!pw_budget)/* Do nothing if no power budget */
Signature · 1 line
2.43.0