Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230511073428.10264-1-u.kleine-koenig@pengutronix.de>
In-Reply-To: <20230511073428.10264-1-u.kleine-koenig@pengutronix.de>
From:   "Rafael J. Wysocki" <rafael@kernel.org>
Date:   Thu, 11 May 2023 12:18:09 +0200
Message-ID: <CAJZ5v0gNPt=rq+pQtmoGL5nxzDQboOK4d6h7=NoY=LueVhZjAQ@mail.gmail.com>
Subject: Re: [PATCH] driver core: Call pm_runtime_put_sync() only after device_remove()
To:     =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= 
        <u.kleine-koenig@pengutronix.de>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        linux-kernel@vger.kernel.org, kernel@pengutronix.de
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8BIT
Precedence: bulk

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
>