Re: [PATCH] pmdomain: core: Wait for device link removals before dropping genpd->dev

On Thu, Aug 6, 2026 at 2:00 PM Saravana Kannan <[email protected]> wrote:
>
> On Tue, Aug 4, 2026 at 7:31 AM Ulf Hansson <[email protected]> wrote:
> >
> > >
> > + Saravana, Rafael, Danilo
> >
> > On Fri, Jul 31, 2026 at 12:21 PM jaseg <[email protected]> wrote:
> > >
> > > From: Jan Sebastian Götte <[email protected]>
> > >
> > > genpd->dev is embedded in struct generic_pm_domain and its release
> > > function is empty, so providers free the containing genpd with a plain
> > > kfree() once of_genpd_remove_last() returns, ignoring its refcount.
> >
> > Yes, there is certainly room for improvements in regards to reference
> > counting and freeing data in genpd. It's moving slowly in the right
> > direction though.
> >
> > Anyway, the release function is actually set to
> > genpd_provider_release(), which ideally should free the data in the
> > long run, but we haven't fully reached that point yet.
> >
> > That said, I guess you actually are referring to the ->remove()
> > callback for the genpd_provider_drv, right?
> >
> > >
> > > Since genpd->dev is registered on the genpd provider bus, fw_devlink
> > > creates device links to it, and those are torn down asynchronously.
> > > Nothing made genpd_remove() wait for those teardowns, so the provider
> > > could free the memory backing genpd->dev while the queued workers still
> > > used it.
> > >
> > > This is reachable at boot on qrb2210, where the firmware rejects PC mode
> > > and psci_cpuidle_domain_probe() removes all the CPU PM domains before
> > > returning -EPROBE_DEFER. The bug is asymptomatic on defconfig, but shows
> > > up when enabling KASAN or INIT_ON_FREE_DEFAULT_ON. In some builds, it
> > > causes the kernel to crash a few hundred ms into the boot.
> > >
> > > Call device_link_wait_removal() before dropping the final reference. All
> > > link removal work is queued from device_del(), via
> > > device_links_driver_cleanup() and device_links_purge(), which precedes
> > > genpd_free_data(), and flush_workqueue() waits for it to complete.
> > >
> > > Note: This patch was LLM-assisted. I reproduced the issue and tested
> > > this patch on hardware, and I did my best to verify it by hand. However,
> > > I'm far from an expert in pmdomain, so YMMV.
> > >
> > > Assisted-by: Claude:claude-5-opus
> > > Fixes: 18a3a510ecfd ("pmdomain: core: Add the genpd->dev to the genpd provider bus")
> > > Cc: [email protected]
> > > Signed-off-by: Jan Sebastian Götte <[email protected]>
> > > ---
> > >  drivers/pmdomain/core.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/pmdomain/core.c b/drivers/pmdomain/core.c
> > > index 842c4169e290..4eeb980e5a40 100644
> > > --- a/drivers/pmdomain/core.c
> > > +++ b/drivers/pmdomain/core.c
> > > @@ -2348,6 +2348,9 @@ static int genpd_alloc_data(struct generic_pm_domain *genpd)
> > >
> > >  static void genpd_free_data(struct generic_pm_domain *genpd)
> > >  {
> > > +       /* Pending device link removals still reference genpd->dev. */
> > > +       device_link_wait_removal();
> > > +
> >
> > Even if this fixes the issue, it looks wrong to me that drivers should
> > have to deal with this themselves at device removal.
> >
> > In other words, I was expecting the driver core to deal with this for
> > everyone, but that seems not to be the case. Hmm.
>
> I agree.
>
>  I'm not even sure if device_link_wait_removal() should have been
> added. The point of refcounting is that we don't have to worry about
> who releases in what order and the last one to release will have a
> resource freed. Needing both ref counting and a say to sync feels
> wrong. Let me take a closer look.
>
> -Saravana
>
> >
> > Note that, the device removal should typically happen when the genpd
> > provider calls of_genpd_del_provider() and it calls device_del().
> >
> > >         put_device(&genpd->dev);
> > >         if (genpd->device_id != -ENXIO)
> > >                 ida_free(&genpd_ida, genpd->device_id);
> > > --
> > > 2.53.0
> > >
> >
> > I will have a closer look at the driver core for this. In the
> > meantime, maybe Saravana, Rafael or Danilo have some ideas already?

I took a closer look.

I think the problem is that the genpd framework's internal structure
(struct generic_pm_domain) is allocated by the provider driver and
each one of them allocates it in a different way. For example:
* Part of a container struct's allocation. Eg: struct scpsys_domain
* Using a kzalloc(). Eg: dt_idle_pd_alloc()

And the genpd framework has the struct device as a member of struct
generic_pm_domain and device_add()s it. But the problem is that the
framework now has no control on when the struct device is freed AND
the provider driver has no way of knowing when it's safe to release
this specific genpd->dev and any containers of it.

I think the right solution is for struct generic_pm_domain to change
the "dev" field to a pointer and then allocate the device inside
genpd_alloc_data() and free it in the device's release function (which
is currently a no-op). There are a few places genpd->dev is used
outside of the framework, but those can be fixed up easily.

$ git grep -c 'genpd->dev' -- drivers/
drivers/pmdomain/arm/scmi_perf_domain.c:1
drivers/pmdomain/core.c:59
drivers/pmdomain/governor.c:2
drivers/soc/tegra/pmc.c:3

$ git grep -c '&genpd->dev' -- drivers/
drivers/pmdomain/arm/scmi_perf_domain.c:1
drivers/pmdomain/core.c:40
drivers/pmdomain/governor.c:2
drivers/soc/tegra/pmc.c:3

Hope this helps.

-Saravana