[PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Uwe Kleine-König]

Many drivers that use runtime PM call pm_runtime_get_sync() or one of
its variants in their remove callback. So calling pm_runtime_put_sync()
directly before calling the remove callback results (under some
conditions) in the driver's suspend routine being called just to resume
it again afterwards.

So delay the pm_runtime_put_sync() call until after device_remove().

Confirmed on a stm32mp157a that doing

	echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/unbind

(starting with a runtime-pm suspended 4400e000.can) results in one call
less of m_can_runtime_resume() and m_can_runtime_suspend() each after
this change was applied.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

side note: To test I added a dev_info() to m_can_runtime_resume() and
m_can_runtime_suspend(). I was surprised that directly after boot I had:

	# dmesg | grep -E '4400e000.can: m_can_runtime_(resume|suspend)' | wc -l
	15

I didn't go down that rabbit hole to debug this.

Best regards
Uwe

 drivers/base/dd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index 9c09ca5c4ab6..d97f6b1486d1 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -1267,10 +1267,10 @@ static void __device_release_driver(struct device *dev, struct device *parent)
 
 		bus_notify(dev, BUS_NOTIFY_UNBIND_DRIVER);
 
-		pm_runtime_put_sync(dev);
-
 		device_remove(dev);
 
+		pm_runtime_put_sync(dev);
+
 		if (dev->bus && dev->bus->dma_cleanup)
 			dev->bus->dma_cleanup(dev);
 
-- 
2.39.2

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Rafael J. Wysocki]

On Thu, May 11, 2023 at 9:34 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Many drivers that use runtime PM call pm_runtime_get_sync() or one of
> its variants in their remove callback. So calling pm_runtime_put_sync()
> directly before calling the remove callback results (under some
> conditions) in the driver's suspend routine being called just to resume
> it again afterwards.
>
> So delay the pm_runtime_put_sync() call until after device_remove().
>
> Confirmed on a stm32mp157a that doing
>
>         echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/unbind
>
> (starting with a runtime-pm suspended 4400e000.can) results in one call
> less of m_can_runtime_resume() and m_can_runtime_suspend() each after
> this change was applied.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

I'm not against this change, although I kind of expect it to trigger
some fallout that will need to be addressed.  So caveat emtor.

Anyway

Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

> ---
> Hello,
>
> side note: To test I added a dev_info() to m_can_runtime_resume() and
> m_can_runtime_suspend(). I was surprised that directly after boot I had:
>
>         # dmesg | grep -E '4400e000.can: m_can_runtime_(resume|suspend)' | wc -l
>         15
>
> I didn't go down that rabbit hole to debug this.
>
> Best regards
> Uwe
>
>  drivers/base/dd.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 9c09ca5c4ab6..d97f6b1486d1 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -1267,10 +1267,10 @@ static void __device_release_driver(struct device *dev, struct device *parent)
>
>                 bus_notify(dev, BUS_NOTIFY_UNBIND_DRIVER);
>
> -               pm_runtime_put_sync(dev);
> -
>                 device_remove(dev);
>
> +               pm_runtime_put_sync(dev);
> +
>                 if (dev->bus && dev->bus->dma_cleanup)
>                         dev->bus->dma_cleanup(dev);
>
> --
> 2.39.2
>

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1531 bytes --]

On Thu, May 11, 2023 at 12:18:09PM +0200, Rafael J. Wysocki wrote:
> On Thu, May 11, 2023 at 9:34 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Many drivers that use runtime PM call pm_runtime_get_sync() or one of
> > its variants in their remove callback. So calling pm_runtime_put_sync()
> > directly before calling the remove callback results (under some
> > conditions) in the driver's suspend routine being called just to resume
> > it again afterwards.
> >
> > So delay the pm_runtime_put_sync() call until after device_remove().
> >
> > Confirmed on a stm32mp157a that doing
> >
> >         echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/unbind
> >
> > (starting with a runtime-pm suspended 4400e000.can) results in one call
> > less of m_can_runtime_resume() and m_can_runtime_suspend() each after
> > this change was applied.
> >
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> 
> I'm not against this change, although I kind of expect it to trigger
> some fallout that will need to be addressed.  So caveat emtor.
> 
> Anyway
> 
> Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>

Thanks for your review tag. I wondered if there will be some fallout,
and don't know what to expect yet. Sounds like getting it into next soon
is a good idea?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 1830 bytes --]

On Thu, May 11, 2023 at 12:39:23PM +0200, Uwe Kleine-König wrote:
> On Thu, May 11, 2023 at 12:18:09PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 11, 2023 at 9:34 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Many drivers that use runtime PM call pm_runtime_get_sync() or one of
> > > its variants in their remove callback. So calling pm_runtime_put_sync()
> > > directly before calling the remove callback results (under some
> > > conditions) in the driver's suspend routine being called just to resume
> > > it again afterwards.
> > >
> > > So delay the pm_runtime_put_sync() call until after device_remove().
> > >
> > > Confirmed on a stm32mp157a that doing
> > >
> > >         echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/unbind
> > >
> > > (starting with a runtime-pm suspended 4400e000.can) results in one call
> > > less of m_can_runtime_resume() and m_can_runtime_suspend() each after
> > > this change was applied.
> > >
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > 
> > I'm not against this change, although I kind of expect it to trigger
> > some fallout that will need to be addressed.  So caveat emtor.
> > 
> > Anyway
> > 
> > Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
> 
> Thanks for your review tag. I wondered if there will be some fallout,
> and don't know what to expect yet. Sounds like getting it into next soon
> is a good idea?!

No, this seems like very bad idea and even violates the documentation
which clearly states that the usage counter is balanced before calling
remove() so that drivers can use pm_runtime_suspend() to put devices
into suspended state.

There's is really no good reason to even try to change as this is in no
way a fast path.

NAK.

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Rafael J. Wysocki]

On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, May 11, 2023 at 12:39:23PM +0200, Uwe Kleine-König wrote:
> > On Thu, May 11, 2023 at 12:18:09PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, May 11, 2023 at 9:34 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > Many drivers that use runtime PM call pm_runtime_get_sync() or one of
> > > > its variants in their remove callback. So calling pm_runtime_put_sync()
> > > > directly before calling the remove callback results (under some
> > > > conditions) in the driver's suspend routine being called just to resume
> > > > it again afterwards.
> > > >
> > > > So delay the pm_runtime_put_sync() call until after device_remove().
> > > >
> > > > Confirmed on a stm32mp157a that doing
> > > >
> > > >         echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/unbind
> > > >
> > > > (starting with a runtime-pm suspended 4400e000.can) results in one call
> > > > less of m_can_runtime_resume() and m_can_runtime_suspend() each after
> > > > this change was applied.
> > > >
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > >
> > > I'm not against this change, although I kind of expect it to trigger
> > > some fallout that will need to be addressed.  So caveat emtor.
> > >
> > > Anyway
> > >
> > > Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
> >
> > Thanks for your review tag. I wondered if there will be some fallout,
> > and don't know what to expect yet. Sounds like getting it into next soon
> > is a good idea?!
>
> No, this seems like very bad idea and even violates the documentation
> which clearly states that the usage counter is balanced before calling
> remove() so that drivers can use pm_runtime_suspend() to put devices
> into suspended state.

I missed that, sorry.

> There's is really no good reason to even try to change as this is in no
> way a fast path.

Still, I think that while the "put" part needs to be done before
device_remove(), the actual state change can be carried out later.

So something like

    pm_runtime_put_noidle(dev);

    device_remove(dev);

    pm_runtime_suspend(dev);

would generally work, wouldn't it?

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Johan Hovold]

On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:

> > No, this seems like very bad idea and even violates the documentation
> > which clearly states that the usage counter is balanced before calling
> > remove() so that drivers can use pm_runtime_suspend() to put devices
> > into suspended state.
> 
> I missed that, sorry.
> 
> > There's is really no good reason to even try to change as this is in no
> > way a fast path.
> 
> Still, I think that while the "put" part needs to be done before
> device_remove(), the actual state change can be carried out later.
> 
> So something like
> 
>     pm_runtime_put_noidle(dev);
> 
>     device_remove(dev);
> 
>     pm_runtime_suspend(dev);
> 
> would generally work, wouldn't it?

No, as drivers typically disable runtime pm in their remove callbacks,
that pm_runtime_suspend() would amount to a no-op (and calling the
driver pm ops post unbind and the driver having freed its data would
not work either).

Johan

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Rafael J. Wysocki]

On Fri, May 12, 2023 at 9:39 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
>
> > > No, this seems like very bad idea and even violates the documentation
> > > which clearly states that the usage counter is balanced before calling
> > > remove() so that drivers can use pm_runtime_suspend() to put devices
> > > into suspended state.
> >
> > I missed that, sorry.
> >
> > > There's is really no good reason to even try to change as this is in no
> > > way a fast path.
> >
> > Still, I think that while the "put" part needs to be done before
> > device_remove(), the actual state change can be carried out later.
> >
> > So something like
> >
> >     pm_runtime_put_noidle(dev);
> >
> >     device_remove(dev);
> >
> >     pm_runtime_suspend(dev);
> >
> > would generally work, wouldn't it?
>
> No, as drivers typically disable runtime pm in their remove callbacks,

What exactly do you mean by "typically"?  None of the PCI drivers
should do that, for instance.

> that pm_runtime_suspend() would amount to a no-op (and calling the
> driver pm ops post unbind and the driver having freed its data would
> not work either).

Well, not really.

There are drivers and there are bus types/PM domains.  Drivers need
not disable PM-runtime in their "remove" callbacks if they know that
the bus type/PM domain will take care of handling PM-runtime properly
after the driver's remove callback has run and the bus type/PM domain
may very well want its PM-runtime suspend callback to run then (for
example, to remove power from the unused device).  Arguably it can
invoke runtime_suspend() from its "remove" callback, so it's not like
this is a big deal, but IMO it helps if the most general case is
considered.

Anyway, the question here really is: Does it make sense to carry out a
runtime suspend immediately before device_remove()?  Honestly, I'm not
sure about that.

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Johan Hovold]

On Fri, May 12, 2023 at 04:04:59PM +0200, Rafael J. Wysocki wrote:
> On Fri, May 12, 2023 at 9:39 AM Johan Hovold <johan@kernel.org> wrote:
> >
> > On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
> >
> > > > No, this seems like very bad idea and even violates the documentation
> > > > which clearly states that the usage counter is balanced before calling
> > > > remove() so that drivers can use pm_runtime_suspend() to put devices
> > > > into suspended state.
> > >
> > > I missed that, sorry.
> > >
> > > > There's is really no good reason to even try to change as this is in no
> > > > way a fast path.
> > >
> > > Still, I think that while the "put" part needs to be done before
> > > device_remove(), the actual state change can be carried out later.
> > >
> > > So something like
> > >
> > >     pm_runtime_put_noidle(dev);
> > >
> > >     device_remove(dev);
> > >
> > >     pm_runtime_suspend(dev);
> > >
> > > would generally work, wouldn't it?
> >
> > No, as drivers typically disable runtime pm in their remove callbacks,
> 
> What exactly do you mean by "typically"?  None of the PCI drivers
> should do that, for instance.

I had platform drivers in mind, but so do i2c drivers for example.

> > that pm_runtime_suspend() would amount to a no-op (and calling the
> > driver pm ops post unbind and the driver having freed its data would
> > not work either).
> 
> Well, not really.
> 
> There are drivers and there are bus types/PM domains.  Drivers need
> not disable PM-runtime in their "remove" callbacks if they know that
> the bus type/PM domain will take care of handling PM-runtime properly
> after the driver's remove callback has run and the bus type/PM domain
> may very well want its PM-runtime suspend callback to run then (for
> example, to remove power from the unused device).  Arguably it can
> invoke runtime_suspend() from its "remove" callback, so it's not like
> this is a big deal, but IMO it helps if the most general case is
> considered.

My point was that hundreds of drivers do and for these this call becomes
a no-op. Same for buses that disable runtime pm at remove.

> Anyway, the question here really is: Does it make sense to carry out a
> runtime suspend immediately before device_remove()?  Honestly, I'm not
> sure about that.

I'd say it doesn't really matter as driver unbind is not a common
operation and drivers using autosuspend would generally not be affected
either.

You can try to rework this, but clearly it needs more thought than
simply moving the put sync and some drivers may also be relying on the
current behaviour.

A quick grep reveals a few which would be left active if you change
pm_runtime_put_sync() to pm_runtime_put_noidle(), even if that could be
fixed driver by driver of course.

Johan

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Rafael J. Wysocki]

On Fri, May 12, 2023 at 5:00 PM Johan Hovold <johan@kernel.org> wrote:
>
> On Fri, May 12, 2023 at 04:04:59PM +0200, Rafael J. Wysocki wrote:
> > On Fri, May 12, 2023 at 9:39 AM Johan Hovold <johan@kernel.org> wrote:
> > >
> > > On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> > > > On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
> > >
> > > > > No, this seems like very bad idea and even violates the documentation
> > > > > which clearly states that the usage counter is balanced before calling
> > > > > remove() so that drivers can use pm_runtime_suspend() to put devices
> > > > > into suspended state.
> > > >
> > > > I missed that, sorry.
> > > >
> > > > > There's is really no good reason to even try to change as this is in no
> > > > > way a fast path.
> > > >
> > > > Still, I think that while the "put" part needs to be done before
> > > > device_remove(), the actual state change can be carried out later.
> > > >
> > > > So something like
> > > >
> > > >     pm_runtime_put_noidle(dev);
> > > >
> > > >     device_remove(dev);
> > > >
> > > >     pm_runtime_suspend(dev);
> > > >
> > > > would generally work, wouldn't it?
> > >
> > > No, as drivers typically disable runtime pm in their remove callbacks,
> >
> > What exactly do you mean by "typically"?  None of the PCI drivers
> > should do that, for instance.
>
> I had platform drivers in mind, but so do i2c drivers for example.
>
> > > that pm_runtime_suspend() would amount to a no-op (and calling the
> > > driver pm ops post unbind and the driver having freed its data would
> > > not work either).
> >
> > Well, not really.
> >
> > There are drivers and there are bus types/PM domains.  Drivers need
> > not disable PM-runtime in their "remove" callbacks if they know that
> > the bus type/PM domain will take care of handling PM-runtime properly
> > after the driver's remove callback has run and the bus type/PM domain
> > may very well want its PM-runtime suspend callback to run then (for
> > example, to remove power from the unused device).  Arguably it can
> > invoke runtime_suspend() from its "remove" callback, so it's not like
> > this is a big deal, but IMO it helps if the most general case is
> > considered.
>
> My point was that hundreds of drivers do and for these this call becomes
> a no-op. Same for buses that disable runtime pm at remove.
>
> > Anyway, the question here really is: Does it make sense to carry out a
> > runtime suspend immediately before device_remove()?  Honestly, I'm not
> > sure about that.
>
> I'd say it doesn't really matter as driver unbind is not a common
> operation and drivers using autosuspend would generally not be affected
> either.
>
> You can try to rework this, but clearly it needs more thought than
> simply moving the put sync and some drivers may also be relying on the
> current behaviour.
>
> A quick grep reveals a few which would be left active if you change
> pm_runtime_put_sync() to pm_runtime_put_noidle(), even if that could be
> fixed driver by driver of course.

OK, fair enough.

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2045 bytes --]

On Fri, May 12, 2023 at 09:40:01AM +0200, Johan Hovold wrote:
> On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> > On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
> 
> > > No, this seems like very bad idea and even violates the documentation
> > > which clearly states that the usage counter is balanced before calling
> > > remove() so that drivers can use pm_runtime_suspend() to put devices
> > > into suspended state.
> > 
> > I missed that, sorry.
> > 
> > > There's is really no good reason to even try to change as this is in no
> > > way a fast path.
> > 
> > Still, I think that while the "put" part needs to be done before
> > device_remove(), the actual state change can be carried out later.
> > 
> > So something like
> > 
> >     pm_runtime_put_noidle(dev);
> > 
> >     device_remove(dev);
> > 
> >     pm_runtime_suspend(dev);
> > 
> > would generally work, wouldn't it?
> 
> No, as drivers typically disable runtime pm in their remove callbacks,
> that pm_runtime_suspend() would amount to a no-op (and calling the
> driver pm ops post unbind and the driver having freed its data would
> not work either).

However if a driver author also cares for the CONFIG_PM=n case, calling
pm_runtime_suspend() doesn't have the intended effect and so it's
unfortunately complicated to rely on runtime-pm to power down your
device and you have to do it by hand anyhow (unless you let your driver
depend on CONFIG_PM). So I'm not convinced that "A driver can call
pm_runtime_suspend() to power down" is a useful thing to have.

In the end something like 72362dcdf654 ("can: mcp251xfd:
mcp251xfd_unregister(): simplify runtime PM handling") might be an
approach. But IMHO it's more complicated than it should be and honestly
I'm not sure if it's safe and correct this way.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 2616 bytes --]

On Fri, May 12, 2023 at 08:49:25PM +0200, Uwe Kleine-König wrote:
> On Fri, May 12, 2023 at 09:40:01AM +0200, Johan Hovold wrote:
> > On Thu, May 11, 2023 at 04:44:25PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, May 11, 2023 at 1:48 PM Johan Hovold <johan@kernel.org> wrote:
> > 
> > > > No, this seems like very bad idea and even violates the documentation
> > > > which clearly states that the usage counter is balanced before calling
> > > > remove() so that drivers can use pm_runtime_suspend() to put devices
> > > > into suspended state.
> > > 
> > > I missed that, sorry.
> > > 
> > > > There's is really no good reason to even try to change as this is in no
> > > > way a fast path.
> > > 
> > > Still, I think that while the "put" part needs to be done before
> > > device_remove(), the actual state change can be carried out later.
> > > 
> > > So something like
> > > 
> > >     pm_runtime_put_noidle(dev);
> > > 
> > >     device_remove(dev);
> > > 
> > >     pm_runtime_suspend(dev);
> > > 
> > > would generally work, wouldn't it?
> > 
> > No, as drivers typically disable runtime pm in their remove callbacks,
> > that pm_runtime_suspend() would amount to a no-op (and calling the
> > driver pm ops post unbind and the driver having freed its data would
> > not work either).
> 
> However if a driver author also cares for the CONFIG_PM=n case, calling
> pm_runtime_suspend() doesn't have the intended effect and so it's
> unfortunately complicated to rely on runtime-pm to power down your
> device and you have to do it by hand anyhow (unless you let your driver
> depend on CONFIG_PM). So I'm not convinced that "A driver can call
> pm_runtime_suspend() to power down" is a useful thing to have.

Right, but we do have drivers that have CONFIG_PM as an explicit
dependency.

> In the end something like 72362dcdf654 ("can: mcp251xfd:
> mcp251xfd_unregister(): simplify runtime PM handling") might be an
> approach. But IMHO it's more complicated than it should be and honestly
> I'm not sure if it's safe and correct this way.

Yeah, unfortunately runtime PM is fairly underspecified so we end up
with this multitude of implementations, many of which are broken in
various ways. A smaller API with documented best-practices may have
helped, but that's not where we are right now.

Looks like 72362dcdf654 ("can: mcp251xfd: mcp251xfd_unregister():
simplify runtime PM handling") introduces yet another way to do things,
and which will break if anyone enables (or tries to use this pattern in
another driver with) autosuspend...

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1392 bytes --]

On 17.05.2023 10:28:01, Johan Hovold wrote:
> Right, but we do have drivers that have CONFIG_PM as an explicit
> dependency.
> 
> > In the end something like 72362dcdf654 ("can: mcp251xfd:
> > mcp251xfd_unregister(): simplify runtime PM handling") might be an
> > approach. But IMHO it's more complicated than it should be and honestly
> > I'm not sure if it's safe and correct this way.
> 
> Yeah, unfortunately runtime PM is fairly underspecified so we end up
> with this multitude of implementations, many of which are broken in
> various ways. A smaller API with documented best-practices may have
> helped, but that's not where we are right now.
> 
> Looks like 72362dcdf654 ("can: mcp251xfd: mcp251xfd_unregister():
> simplify runtime PM handling") introduces yet another way to do things,
> and which will break if anyone enables (or tries to use this pattern in
> another driver with) autosuspend...

ACK - I think in that driver it works, as the runtime PM is resumed
during interface up and suspended in interface down. IMHO autosuspend
would not bring any benefits here...

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2660 bytes --]

Hello Johan,

On Thu, May 11, 2023 at 01:48:25PM +0200, Johan Hovold wrote:
> On Thu, May 11, 2023 at 12:39:23PM +0200, Uwe Kleine-König wrote:
> > On Thu, May 11, 2023 at 12:18:09PM +0200, Rafael J. Wysocki wrote:
> > > On Thu, May 11, 2023 at 9:34 AM Uwe Kleine-König
> > > <u.kleine-koenig@pengutronix.de> wrote:
> > > >
> > > > Many drivers that use runtime PM call pm_runtime_get_sync() or one of
> > > > its variants in their remove callback. So calling pm_runtime_put_sync()
> > > > directly before calling the remove callback results (under some
> > > > conditions) in the driver's suspend routine being called just to resume
> > > > it again afterwards.
> > > >
> > > > So delay the pm_runtime_put_sync() call until after device_remove().
> > > >
> > > > Confirmed on a stm32mp157a that doing
> > > >
> > > >         echo 4400e000.can > /sys/bus/platform/drivers/m_can_platform/unbind
> > > >
> > > > (starting with a runtime-pm suspended 4400e000.can) results in one call
> > > > less of m_can_runtime_resume() and m_can_runtime_suspend() each after
> > > > this change was applied.
> > > >
> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > 
> > > I'm not against this change, although I kind of expect it to trigger
> > > some fallout that will need to be addressed.  So caveat emtor.
> > > 
> > > Anyway
> > > 
> > > Reviewed-by: Rafael J. Wysocki <rafael@kernel.org>
> > 
> > Thanks for your review tag. I wondered if there will be some fallout,
> > and don't know what to expect yet. Sounds like getting it into next soon
> > is a good idea?!
> 
> No, this seems like very bad idea and even violates the documentation
> which clearly states that the usage counter is balanced before calling
> remove() so that drivers can use pm_runtime_suspend() to put devices
> into suspended state.

I grepped around a bit and found:

	To allow bus types and drivers to put devices into the suspended state by
	calling pm_runtime_suspend() from their ->remove() routines, the driver core
	executes pm_runtime_put_sync() after running the BUS_NOTIFY_UNBIND_DRIVER
	notifications in __device_release_driver().  This requires bus types and
	drivers to make their ->remove() callbacks avoid races with runtime PM directly,
	but it also allows more flexibility in the handling of devices during the
	removal of their drivers.

Hmm, while I see your point, it's still ugly. I'll think about it.

Best regards
Uwe


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]