Re: [PATCH] PCI/pwrctrl: Do not power off on pwrctrl device removal

[Manivannan Sadhasivam]

On Thu, Feb 26, 2026 at 04:57:59PM +0800, Chen-Yu Tsai wrote:
> On Thu, Feb 26, 2026 at 4:23 PM Manivannan Sadhasivam <mani@xxxxxxxxxx> wrote:
> >
> > On Tue, Feb 24, 2026 at 03:12:56PM +0800, Chen-Yu Tsai wrote:
> > > With the move to explicit pwrctrl power on/off APIs, the caller, i.e.
> > > the PCI driver should manage the power state. The pwrctrl drivers should
> > > not try to clean up or power off when they are removed, as this might
> > > end up disabling an already disabled regulator, causing a big warning.
> > > This can be triggered if a PCI controller driver's .remove() callback
> > > calls pci_pwrctrl_destroy_devices() after pci_pwrctrl_power_off_devices().
> > >
> >
> > Also, we should add the devlink dependency as below so that the pwrctrl driver
> > becomes the supplier of the controller driver. This will prevent the pwrctrl
> > driver from being removed before the controller driver.
> >
> > diff --git a/drivers/pci/pwrctrl/core.c b/drivers/pci/pwrctrl/core.c
> > index 6f7dea6746e0..0c1cc9599b41 100644
> > --- a/drivers/pci/pwrctrl/core.c
> > +++ b/drivers/pci/pwrctrl/core.c
> > @@ -311,6 +311,12 @@ static int pci_pwrctrl_create_device(struct device_node *np,
> >                 return -EINVAL;
> >         }
> >
> > +       if (device_link_add(parent, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
> > +               dev_info(parent, "Failed to add device link\n");
> > +               of_platform_device_destroy(&pdev->dev, NULL);
> > +               return -EINVAL;
> > +       }
> > +
> >         return 0;
> >  }
> >
> >
> > Even so, the pwrctrl drivers should not power off the resources on their own.
> > So this patch is valid on its own. I just have one comment below.
> >
> > > Drop the devm cleanup parts that turn off regulators from the pwrctrl
> > > drivers.
> > >
> > > Fixes: b921aa3f8dec ("PCI/pwrctrl: Switch to pwrctrl create, power on/off, destroy APIs")
> > > Cc: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxxxxxxxx>
> > > Signed-off-by: Chen-Yu Tsai <wenst@xxxxxxxxxxxx>
> > > ---
> > > Hi,
> > >
> > > I ran into this while integrating the new pci_pwrctrl_*() API into the
> > > MediaTek driver. I am sending this separately since this change is
> > > unrelated and does not conflict with or depend on the other changes for
> > > the driver itself.
> > >
> > > I think this should be merged for fixes.
> > >
> > >  drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c | 12 ------------
> > >  drivers/pci/pwrctrl/slot.c               | 12 ------------
> > >  2 files changed, 24 deletions(-)
> > >
> > > diff --git a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
> > > index 0d0377283c37..c7e4beec160a 100644
> > > --- a/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
> > > +++ b/drivers/pci/pwrctrl/pci-pwrctrl-pwrseq.c
> > > @@ -68,13 +68,6 @@ static int pwrseq_pwrctrl_power_off(struct pci_pwrctrl *pwrctrl)
> > >       return pwrseq_power_off(pwrseq->pwrseq);
> > >  }
> > >
> > > -static void devm_pwrseq_pwrctrl_power_off(void *data)
> > > -{
> > > -     struct pwrseq_pwrctrl *pwrseq = data;
> > > -
> > > -     pwrseq_pwrctrl_power_off(&pwrseq->pwrctrl);
> > > -}
> > > -
> > >  static int pwrseq_pwrctrl_probe(struct platform_device *pdev)
> > >  {
> > >       const struct pwrseq_pwrctrl_pdata *pdata;
> > > @@ -101,11 +94,6 @@ static int pwrseq_pwrctrl_probe(struct platform_device *pdev)
> > >               return dev_err_probe(dev, PTR_ERR(pwrseq->pwrseq),
> > >                                    "Failed to get the power sequencer\n");
> > >
> > > -     ret = devm_add_action_or_reset(dev, devm_pwrseq_pwrctrl_power_off,
> > > -                                    pwrseq);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > >       pwrseq->pwrctrl.power_on = pwrseq_pwrctrl_power_on;
> > >       pwrseq->pwrctrl.power_off = pwrseq_pwrctrl_power_off;
> > >
> > > diff --git a/drivers/pci/pwrctrl/slot.c b/drivers/pci/pwrctrl/slot.c
> > > index 082af81efe25..0ad920d8596d 100644
> > > --- a/drivers/pci/pwrctrl/slot.c
> > > +++ b/drivers/pci/pwrctrl/slot.c
> > > @@ -59,14 +59,6 @@ static int slot_pwrctrl_power_off(struct pci_pwrctrl *pwrctrl)
> > >       return 0;
> > >  }
> > >
> > > -static void devm_slot_pwrctrl_release(void *data)
> > > -{
> > > -     struct slot_pwrctrl *slot = data;
> > > -
> > > -     slot_pwrctrl_power_off(&slot->pwrctrl);
> > > -     regulator_bulk_free(slot->num_supplies, slot->supplies);
> >
> > Here you are leaking the regulators as they will never get freed.
> 
> Oops. You're right. I guess we should just add a devres version of
> of_regulator_bulk_get_all(), but that ends up being a cross-tree
> dependency.

I'm not sure if Mark will accept it as he was not happy with bulk_get_all()
version first of all.

> For the immediate fix I would keep the release function
> to free the regulators, and then do the rework for -next.
> 

Sounds good to me.

- Mani

-- 
மணிவண்ணன் சதாசிவம்