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

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
    net: pse-pd: add devm_pse_poll_helper()

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

> diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
> index 87aa4f4e9724..b7ffec0c942c 100644
> --- a/drivers/net/pse-pd/pse_core.c
> +++ b/drivers/net/pse-pd/pse_core.c
[ ... ]
> @@ -1118,6 +1125,8 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev)
>  	pse_release_pis(pcdev);
>  	if (pcdev->irq)
>  		disable_irq(pcdev->irq);
> +	if (pcdev->polling)
> +		cancel_delayed_work_sync(&pcdev->poll_work);

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

[ ... ]
> @@ -1356,6 +1403,54 @@ int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq,
>  }
>  EXPORT_SYMBOL_GPL(devm_pse_irq_helper);
>  
> +/**
> + * devm_pse_poll_helper - Register poll-based PSE event notifier
> + * @pcdev: a pointer to the PSE controller device
> + * @d: PSE event description (uses same pse_irq_desc as IRQ path)
[ ... ]
> +int devm_pse_poll_helper(struct pse_controller_dev *pcdev,
> +			 const struct pse_irq_desc *d)
> +{
> +	struct device *dev = pcdev->dev;
> +
> +	if (!d || !d->map_event || !d->name)
> +		return -EINVAL;
> +
> +	pcdev->poll_desc = *d;
> +	pcdev->poll_notifs = devm_kcalloc(dev, pcdev->nr_lines,
> +					  sizeof(*pcdev->poll_notifs),
> +					  GFP_KERNEL);
> +	if (!pcdev->poll_notifs)
> +		return -ENOMEM;
> +
> +	pcdev->poll_notifs_mask = devm_bitmap_zalloc(dev, pcdev->nr_lines,
> +						     GFP_KERNEL);
> +	if (!pcdev->poll_notifs_mask)
> +		return -ENOMEM;
> +
> +	if (!pcdev->poll_interval_ms)
> +		pcdev->poll_interval_ms = PSE_DEFAULT_POLL_INTERVAL_MS;
> +
> +	INIT_DELAYED_WORK(&pcdev->poll_work, pse_poll_worker);
> +	pcdev->polling = true;
> +
> +	queue_delayed_work(system_freezable_wq, &pcdev->poll_work,
> +			   msecs_to_jiffies(pcdev->poll_interval_ms));

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

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

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(devm_pse_poll_helper);