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

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

Posted by Chen-Yu Tsai 1 month, 3 weeks ago

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().

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@oss.qualcomm.com>
Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
---
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);
-}
-
 static int slot_pwrctrl_probe(struct platform_device *pdev)
 {
 	struct slot_pwrctrl *slot;
@@ -105,10 +97,6 @@ static int slot_pwrctrl_probe(struct platform_device *pdev)
 	slot->pwrctrl.power_on = slot_pwrctrl_power_on;
 	slot->pwrctrl.power_off = slot_pwrctrl_power_off;
 
-	ret = devm_add_action_or_reset(dev, devm_slot_pwrctrl_release, slot);
-	if (ret)
-		return ret;
-
 	pci_pwrctrl_init(&slot->pwrctrl, dev);
 
 	ret = devm_pci_pwrctrl_device_set_ready(dev, &slot->pwrctrl);
-- 
2.53.0.371.g1d285c8824-goog

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

Posted by Manivannan Sadhasivam 1 month, 2 weeks ago

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@oss.qualcomm.com>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> 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.

- Mani

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

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

Posted by Chen-Yu Tsai 1 month, 2 weeks ago

On Thu, Feb 26, 2026 at 4:23 PM Manivannan Sadhasivam <mani@kernel.org> 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@oss.qualcomm.com>
> > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > ---
> > 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

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

Posted by Manivannan Sadhasivam 1 month, 2 weeks ago

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@kernel.org> 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@oss.qualcomm.com>
> > > Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> > > ---
> > > 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

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

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

Posted by Bartosz Golaszewski 1 month, 3 weeks ago

On Tue, 24 Feb 2026 08:12:56 +0100, Chen-Yu Tsai <wenst@chromium.org> said:
> 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().
>
> 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@oss.qualcomm.com>
> Signed-off-by: Chen-Yu Tsai <wenst@chromium.org>
> ---
> 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.
>

Makes sense.

Reviewed-by: Bartosz Golaszewski <bartosz.golaszewski@oss.qualcomm.com>