Re: [PATCH net] net: pse-pd: don't enable a PI again while disabling it

[Kory Maincent <kory.maincent@bootlin.com>]

On 10/18/26 20:20, Ross Cawston wrote:
> With software power control, _pse_pi_disable() frees the power budget
> of the PI and then retries the power delivery of the PIs waiting for
> power in its power domain: those that are admin enabled, have a PD
> detected, and are not enabled in hardware. The PI being disabled is one
> of them, as its admin state and PD detection are only cleared after
> _pse_pi_disable() returns, so it is enabled again right away:
> 
> - an admin disable of a port with a PD keeps the port powered, and the
>    next admin enable allocates its budget a second time
> - a disconnection enables the now empty port again, with a new budget
>    allocation that is never freed: when the port reports the
>    disconnection again, its PD detection is already cleared
> - a PI disabled to free budget for a PI of higher priority is enabled
>    again with the budget it just freed, so priorities never take effect
> 
> Clear the admin state and the PD detection before disabling the PI,
> and don't retry the power domain in pse_disable_pi_pol(), whose freed
> budget is meant for the PI being allocated. That PI stays waiting for
> power and is retried when budget is freed later.
> 
> The problem was found, and this fix written, with an AI coding
> assistant, while testing the budget on a switch with two Realtek RTL8239
> PSE controllers, whose driver (not in mainline yet) uses the static
> budget strategy like the TPS23881. Before this change, "ethtool
> --set-pse <port> c33-pse-admin-control disable" left a port with a PD
> powered, and the budget allocated in the power domain grew with every
> disable and every unplug. With it, the port stays off, and the allocated
> budget is back to 0 after a disable and after an unplug. Tested on that
> switch with OpenWrt's 6.18 kernel; the change for mainline is the same
> apart from context and is build-tested.
> 
> Fixes: ffef61d6d273 ("net: pse-pd: Add support for budget evaluation strategies")
> Signed-off-by: Ross Cawston <ross@r-sc.ca>
> ---
>   drivers/net/pse-pd/pse_core.c | 50 ++++++++++++++++++++++++++++-------
>   1 file changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index a5e6d7b26b9..b46e3a13a31 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -448,14 +448,14 @@ static void pse_pi_deallocate_pw_budget(struct pse_pi *pi)
>   }
>   
>   /**
> - * _pse_pi_disable - Call disable operation. Assumes the PSE lock has been
> - *		     acquired.
> + * __pse_pi_disable - Call disable operation and free the power budget of the
> + *		      PI. Assumes the PSE lock has been acquired.
>    * @pcdev: a pointer to the PSE
>    * @id: index of the PSE control
>    *
>    * Return: 0 on success and failure value on error
>    */
> -static int _pse_pi_disable(struct pse_controller_dev *pcdev, int id)
> +static int __pse_pi_disable(struct pse_controller_dev *pcdev, int id)
>   {
>   	const struct pse_controller_ops *ops = pcdev->ops;
>   	int ret;
> @@ -469,6 +469,29 @@ static int _pse_pi_disable(struct pse_controller_dev *pcdev, int id)
>   
>   	pse_pi_deallocate_pw_budget(&pcdev->pi[id]);
>   
> +	return 0;
> +}
> +
> +/**
> + * _pse_pi_disable - Disable a PI, then retry the power delivery of the PIs
> + *		     waiting for power in its power domain. Assumes the PSE
> + *		     lock has been acquired.
> + * @pcdev: a pointer to the PSE
> + * @id: index of the PSE control
> + *
> + * The caller must have cleared the admin state or the PD detection of the
> + * PI first, or the retry enables the PI again.
> + *
> + * Return: 0 on success and failure value on error
> + */
> +static int _pse_pi_disable(struct pse_controller_dev *pcdev, int id)
> +{
> +	int ret;
> +
> +	ret = __pse_pi_disable(pcdev, id);
> +	if (ret)
> +		return ret;
> +
>   	if (pse_pw_d_is_sw_pw_control(pcdev, pcdev->pi[id].pw_d))
>   		pse_pw_d_retry_power_delivery(pcdev, pcdev->pi[id].pw_d);

Thanks for your fix.
I think, I would prefer to have these two lines moved out of 
_pse_pi_disable() calls than to have another disable function.
Indeed this is not strictly related to the disable call.
This will also simplify your change below.

> @@ -490,7 +513,10 @@ static int pse_disable_pi_pol(struct pse_controller_dev *pcdev, int id)
>   
>   	dev_dbg(pcdev->dev, "Disabling PI %d to free power budget\n", id);
>   
> -	ret = _pse_pi_disable(pcdev, id);
> +	/* The power freed is for the PI being allocated, and this PI waits
> +	 * for power again: no retry of the power domain here.
> +	 */
> +	ret = __pse_pi_disable(pcdev, id);
>   	if (ret)
>   		notifs |= ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR;
>   
> @@ -703,14 +729,18 @@ static int pse_pi_disable(struct regulator_dev *rdev)
>   {
>   	struct pse_controller_dev *pcdev = rdev_get_drvdata(rdev);
>   	struct pse_pi *pi;
> +	bool enabled;
>   	int id, ret;
>   
>   	id = rdev_get_id(rdev);
>   	pi = &pcdev->pi[id];
>   	mutex_lock(&pcdev->lock);
> +	/* Disabled before _pse_pi_disable() retries the power domain */
> +	enabled = pi->admin_state_enabled;
> +	pi->admin_state_enabled = 0;
>   	ret = _pse_pi_disable(pcdev, id);
> -	if (!ret)
> -		pi->admin_state_enabled = 0;
> +	if (ret)
> +		pi->admin_state_enabled = enabled;
>   
>   	mutex_unlock(&pcdev->lock);
>   	return 0;
> @@ -1229,10 +1259,12 @@ static int pse_set_config_isr(struct pse_controller_dev *pcdev, int id,
>   				ret = 0;
>   		}
>   	} else if (notifs & ETHTOOL_C33_PSE_EVENT_DISCONNECTION) {
> -		if (pcdev->pi[id].admin_state_enabled &&
> -		    pcdev->pi[id].isr_pd_detected)
> -			ret = _pse_pi_disable(pcdev, id);
> +		bool pd_detected = pcdev->pi[id].isr_pd_detected;
> +
> +		/* Gone before _pse_pi_disable() retries the power domain */
>   		pcdev->pi[id].isr_pd_detected = false;
> +		if (pcdev->pi[id].admin_state_enabled && pd_detected)
> +			ret = _pse_pi_disable(pcdev, id);
>   	}
>   
>   	return ret;

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