diff mbox series

i2c: omap: Improve error reporting for problems during .remove()

Message ID 20230406082354.jwchbl5ir6p4gjw7@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series i2c: omap: Improve error reporting for problems during .remove() | expand

Commit Message

Uwe Kleine-König April 6, 2023, 8:23 a.m. UTC 
If pm_runtime_get() fails in .remove() the driver used to return the
error to the driver core. The only effect of this (compared to returning
zero) is a generic warning that the error value is ignored.

So emit a better warning and return zero to suppress the generic (and
little helpful) message. Also disable runtime PM in the error case.

This prepares changing platform device remove callbacks to return void.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
On Thu, Apr 06, 2023 at 08:44:33AM +0200, Wolfram Sang wrote:
> 
> > > So if there is some clk handling necessary before the register access,
> > > I'm not aware where it's hidden. Is there some bus or omap specific code
> > > that ensures clk handling?
> > 
> > I think the missing part is that the runtime PM calls in the i2c driver
> > cause the parent ti-sysc interconnect target module device to get enabled
> > and clocked before accessing the i2c registers.
> 
> So, this patch is not needed?

The patch as is is wrong. For my quest to drop the return value of
platform driver's remove callbacks, I need this patch instead:

 drivers/i2c/busses/i2c-omap.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)


base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6

Comments

Tony Lindgren April 13, 2023, 5:12 a.m. UTC | #1 
Hi,

* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230406 08:23]:
> --- a/drivers/i2c/busses/i2c-omap.c
> +++ b/drivers/i2c/busses/i2c-omap.c
> @@ -1525,14 +1525,17 @@ static int omap_i2c_remove(struct platform_device *pdev)
>  	int ret;
>  
>  	i2c_del_adapter(&omap->adapter);
> -	ret = pm_runtime_resume_and_get(&pdev->dev);
> +
> +	ret = pm_runtime_get_sync(&pdev->dev);

It's better to use pm_runtime_resume_and_get() nowadays in general as
it does pm_runtime_put_noidle() on errors.

Not sure if there are changes needed here, maybe warn and return early
if needed?

Regards,

Tony
Uwe Kleine-König April 13, 2023, 6:24 a.m. UTC | #2 
Hello Tony,

On Thu, Apr 13, 2023 at 08:12:22AM +0300, Tony Lindgren wrote:
> * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230406 08:23]:
> > --- a/drivers/i2c/busses/i2c-omap.c
> > +++ b/drivers/i2c/busses/i2c-omap.c
> > @@ -1525,14 +1525,17 @@ static int omap_i2c_remove(struct platform_device *pdev)
> >  	int ret;
> >  
> >  	i2c_del_adapter(&omap->adapter);
> > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > +
> > +	ret = pm_runtime_get_sync(&pdev->dev);
> 
> It's better to use pm_runtime_resume_and_get() nowadays in general as
> it does pm_runtime_put_noidle() on errors.

Sticking to pm_runtime_resume_and_get() complicates the change however,
because the function calls pm_runtime_put_sync() already. So with
pm_runtime_resume_and_get() I'd need

	if (ret >= 0)
		pm_runtime_put_sync(&pdev->dev);

instead of a plain

	pm_runtime_put_sync(&pdev->dev);

.

> Not sure if there are changes needed here, maybe warn and return early
> if needed?

The idea of "return early" in a remove callback is exactly what I want
to get rid of.

See
https://lore.kernel.org/linux-spi/20230317084232.142257-3-u.kleine-koenig@pengutronix.de
for an example.

Best regards
Uwe
Tony Lindgren April 13, 2023, 6:39 a.m. UTC | #3 
* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 06:24]:
> Hello Tony,
> 
> On Thu, Apr 13, 2023 at 08:12:22AM +0300, Tony Lindgren wrote:
> > * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230406 08:23]:
> > > --- a/drivers/i2c/busses/i2c-omap.c
> > > +++ b/drivers/i2c/busses/i2c-omap.c
> > > @@ -1525,14 +1525,17 @@ static int omap_i2c_remove(struct platform_device *pdev)
> > >  	int ret;
> > >  
> > >  	i2c_del_adapter(&omap->adapter);
> > > -	ret = pm_runtime_resume_and_get(&pdev->dev);
> > > +
> > > +	ret = pm_runtime_get_sync(&pdev->dev);
> > 
> > It's better to use pm_runtime_resume_and_get() nowadays in general as
> > it does pm_runtime_put_noidle() on errors.
> 
> Sticking to pm_runtime_resume_and_get() complicates the change however,
> because the function calls pm_runtime_put_sync() already. So with
> pm_runtime_resume_and_get() I'd need
> 
> 	if (ret >= 0)
> 		pm_runtime_put_sync(&pdev->dev);
> 
> instead of a plain
> 
> 	pm_runtime_put_sync(&pdev->dev);

In that case you still need to do pm_runtime_put_noidle()
on errors, so not sure what's the best way here.

> > Not sure if there are changes needed here, maybe warn and return early
> > if needed?
> 
> The idea of "return early" in a remove callback is exactly what I want
> to get rid of.
> 
> See
> https://lore.kernel.org/linux-spi/20230317084232.142257-3-u.kleine-koenig@pengutronix.de
> for an example.

Oh OK. Care to clarify a bit why we are not allowed to return errors
on remove though? Are we getting rid of the return value for remove?
Sorry if I'm not following the cunning plan here :)

Regards,

Tony
Uwe Kleine-König April 13, 2023, 7:07 a.m. UTC | #4 
On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> Oh OK. Care to clarify a bit why we are not allowed to return errors
> on remove though? Are we getting rid of the return value for remove?
> Sorry if I'm not following the cunning plan here :)

Yes, that's the plan. If you look at the caller of the remove functions
(before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):

static void platform_remove(struct device *_dev)
{
        struct platform_driver *drv = to_platform_driver(_dev->driver);
        struct platform_device *dev = to_platform_device(_dev);

        if (drv->remove) {
                int ret = drv->remove(dev);

                if (ret)
                        dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
        }
        dev_pm_domain_detach(_dev, true);
}

you see it's pointless to return an error value. But the prototype
seduces driver authors to do it yielding to error that can easily
prevented if .remove returns void. See also
5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
of the quest.

Best regards
Uwe
Tony Lindgren April 13, 2023, 7:11 a.m. UTC | #5 
* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:07]:
> On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> > Oh OK. Care to clarify a bit why we are not allowed to return errors
> > on remove though? Are we getting rid of the return value for remove?
> > Sorry if I'm not following the cunning plan here :)
> 
> Yes, that's the plan. If you look at the caller of the remove functions
> (before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):
> 
> static void platform_remove(struct device *_dev)
> {
>         struct platform_driver *drv = to_platform_driver(_dev->driver);
>         struct platform_device *dev = to_platform_device(_dev);
> 
>         if (drv->remove) {
>                 int ret = drv->remove(dev);
> 
>                 if (ret)
>                         dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
>         }
>         dev_pm_domain_detach(_dev, true);
> }
> 
> you see it's pointless to return an error value. But the prototype
> seduces driver authors to do it yielding to error that can easily
> prevented if .remove returns void. See also
> 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
> of the quest.

OK thanks. So maybe check the pm_runtime_get_sync() and on error do
pm_runtime_put_noidle(), or pm_runtime_resume_and_get(). Both ways
are fine for me, maybe you already figured it out.

Regards,

Tony
Uwe Kleine-König April 13, 2023, 7:37 a.m. UTC | #6 
Hello Tony,

On Thu, Apr 13, 2023 at 10:11:24AM +0300, Tony Lindgren wrote:
> * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:07]:
> > On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> > > Oh OK. Care to clarify a bit why we are not allowed to return errors
> > > on remove though? Are we getting rid of the return value for remove?
> > > Sorry if I'm not following the cunning plan here :)
> > 
> > Yes, that's the plan. If you look at the caller of the remove functions
> > (before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):
> > 
> > static void platform_remove(struct device *_dev)
> > {
> >         struct platform_driver *drv = to_platform_driver(_dev->driver);
> >         struct platform_device *dev = to_platform_device(_dev);
> > 
> >         if (drv->remove) {
> >                 int ret = drv->remove(dev);
> > 
> >                 if (ret)
> >                         dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
> >         }
> >         dev_pm_domain_detach(_dev, true);
> > }
> > 
> > you see it's pointless to return an error value. But the prototype
> > seduces driver authors to do it yielding to error that can easily
> > prevented if .remove returns void. See also
> > 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
> > of the quest.
> 
> OK thanks. So maybe check the pm_runtime_get_sync() and on error do
> pm_runtime_put_noidle(), or pm_runtime_resume_and_get(). Both ways
> are fine for me, maybe you already figured it out.

Is this an Ack for my patch?

Best regards
Uwe
Tony Lindgren April 13, 2023, 7:40 a.m. UTC | #7 
* Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:37]:
> Hello Tony,
> 
> On Thu, Apr 13, 2023 at 10:11:24AM +0300, Tony Lindgren wrote:
> > * Uwe Kleine-König <u.kleine-koenig@pengutronix.de> [230413 07:07]:
> > > On Thu, Apr 13, 2023 at 09:39:15AM +0300, Tony Lindgren wrote:
> > > > Oh OK. Care to clarify a bit why we are not allowed to return errors
> > > > on remove though? Are we getting rid of the return value for remove?
> > > > Sorry if I'm not following the cunning plan here :)
> > > 
> > > Yes, that's the plan. If you look at the caller of the remove functions
> > > (before 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c):
> > > 
> > > static void platform_remove(struct device *_dev)
> > > {
> > >         struct platform_driver *drv = to_platform_driver(_dev->driver);
> > >         struct platform_device *dev = to_platform_device(_dev);
> > > 
> > >         if (drv->remove) {
> > >                 int ret = drv->remove(dev);
> > > 
> > >                 if (ret)
> > >                         dev_warn(_dev, "remove callback returned a non-zero value. This will be ignored.\n");
> > >         }
> > >         dev_pm_domain_detach(_dev, true);
> > > }
> > > 
> > > you see it's pointless to return an error value. But the prototype
> > > seduces driver authors to do it yielding to error that can easily
> > > prevented if .remove returns void. See also
> > > 5c5a7680e67ba6fbbb5f4d79fa41485450c1985c for some background and details
> > > of the quest.
> > 
> > OK thanks. So maybe check the pm_runtime_get_sync() and on error do
> > pm_runtime_put_noidle(), or pm_runtime_resume_and_get(). Both ways
> > are fine for me, maybe you already figured it out.
> 
> Is this an Ack for my patch?

Yes looking at it again:

Reviewed-by: Tony Lindgren <tony@atomide.com>

Thanks,

Tony
Wolfram Sang April 13, 2023, 4:49 p.m. UTC | #8 
On Thu, Apr 06, 2023 at 10:23:54AM +0200, Uwe Kleine-König wrote:
> If pm_runtime_get() fails in .remove() the driver used to return the
> error to the driver core. The only effect of this (compared to returning
> zero) is a generic warning that the error value is ignored.
> 
> So emit a better warning and return zero to suppress the generic (and
> little helpful) message. Also disable runtime PM in the error case.
> 
> This prepares changing platform device remove callbacks to return void.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to for-next, thanks!
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-omap.c b/drivers/i2c/busses/i2c-omap.c
index f9ae520aed22..2b4e2be51318 100644
--- a/drivers/i2c/busses/i2c-omap.c
+++ b/drivers/i2c/busses/i2c-omap.c
@@ -1525,14 +1525,17 @@  static int omap_i2c_remove(struct platform_device *pdev)
 	int ret;
 
 	i2c_del_adapter(&omap->adapter);
-	ret = pm_runtime_resume_and_get(&pdev->dev);
+
+	ret = pm_runtime_get_sync(&pdev->dev);
 	if (ret < 0)
-		return ret;
+		dev_err(omap->dev, "Failed to resume hardware, skip disable\n");
+	else
+		omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);
 
-	omap_i2c_write_reg(omap, OMAP_I2C_CON_REG, 0);
 	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
+
 	return 0;
 }