Re: [PATCH net 1/2] net: pse-pd: disable IRQ before freeing PI data in unregister

[Simon Horman <horms@kernel.org>]

On Sat, May 30, 2026 at 12:50:31PM +0200, Carlo Szelinsky wrote:
> Hi Simon,
> 
> Thanks for the review.

Hi Carlo,

Likewise, thanks for your response.

> > pse_flush_pw_ds() runs before disable_irq(), so an interrupt could
> > hit a freed regulator.
> 
> Correct, and it's the same bug. I moved disable_irq() above
> pse_release_pis(), but pse_flush_pw_ds() still runs while the IRQ is
> live, and it can free pw_d->supply. The ISR uses that supply via
> pse_pi_deallocate_pw_budget(). So the race stays open.
> 
> Fix: disable the IRQ (and cancel the poll work) before
> pse_flush_pw_ds() too. I'll fold that into patch 1 for v2.

Ack, sounds good.

> > cancel_work_sync() after pse_release_pis() may use freed pcdev->pi.
> 
> I don't think so. The worker only touches the kfifo and the
> pse_control list, not pcdev->pi. The patch 1 message says this.
> Did I miss a path where the worker reaches pcdev->pi?

I am concerned that this can occur if pse_send_ntf_worker()
calls pse_control_put(). In which case __pse_control_release()
may run, which accesses psec->pcdev->pi[psec->id].admin_state_enabled.

> 
> > Regulator ops still reachable after pcdev->pi is freed.
> 
> That is what patch 2 fixes for the disable path. Are you pointing at
> a different path than the regulator_unregister() disable flush?

I agree this relates to patch 2/2, and I wonder if you may have
missed the AI-generated review of it that I forwarded.

* https://lore.kernel.org/netdev/20260527122418.2410341-2-horms@kernel.org/

I think the problem here is that wile patch 2/2
addresses pse_pi_disable by adding a check for !pcdev->pi,
it doesn't address similar problems in pse_pi_is_enabled()
and pse_pi_enable().

I'd also like to draw your attention to the issue around synchronising
access to pcdev->pi in pse_release_pis in the review of 2/2.

> 
> > device still in the list / external consumers / power domain tied to
> > devm lifetime.
> 
> These look pre-existing and not part of this series. Do you agree, or
> do you see one of them as caused by this series?

Thanks, agreed.

> The pre-existing items above (list, consumers, devm lifetime) - would
> you want them fixed inside this net series, or handled separately on
> top? So I know what to do before sending v2.

For these last three pre-existing items I think it's best to handle them
separately. To avoid complicating this series more than necessary.