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

[Chen-Yu Tsai]

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. For the immediate fix I would keep the release function
to free the regulators, and then do the rework for -next.


ChenYu