Re: [PATCH net v3 08/14] net: pse-pd: stop async event sources before freeing PI data in unregister

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] net: pse-pd: spurious error event in pse_isr() on -ENODEV
  during unregister

--

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
> @@ -612,6 +612,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);

[Severity: Medium]
pse_isr() can report a false power control error during unregister.

Say the threaded pse_isr() is already blocked on pcdev->lock when
pse_controller_unregister() sets pcdev->unregistering and drops the
lock. It then takes the lock while disable_irq() is still waiting for
it to finish.

For a classification event on a PI with admin_state_enabled set,
pse_set_config_isr() calls _pse_pi_delivery_power_sw_pw_ctrl(). That
now returns -ENODEV at this new check.

pse_set_config_isr() only clears -ERANGE, so -ENODEV goes back to
pse_isr(). pse_isr() treats any non-zero return as a failure and sets
ETHTOOL_PSE_EVENT_SW_PW_CONTROL_ERROR for that PI. It pushes the event
into pcdev->ntf_fifo and schedules ntf_work before cancel_work_sync()
runs.

If pse_send_ntf_worker() is already running at that point, it can
pick up the entry and send it through ethnl_pse_send_ntf(). Userspace
then sees a power control error for a PI that did not fail; the
controller is only going away.

This is not a memory safety problem, since disable_irq() orders the
queueing before the drain, but the event is misleading.

Should pse_set_config_isr() filter the -ENODEV from the unregistering
check the same way it filters -ERANGE? Or would you rather have
pse_isr() skip the PIs once pcdev->unregistering is set?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261103220217.1131971-1-github%40szelinsky.de