[PATCH] i2c: sprd: Delete i2c adapter in .remove's error path

[PATCH] i2c: sprd: Delete i2c adapter in .remove's error path

[Uwe Kleine-König]

If pm runtime resume fails the .remove callback used to exit early. This
resulted in an error message by the driver core but the device gets
removed anyhow. This lets the registered i2c adapter stay around with an
unbound parent device.

So only skip clk disabling if resume failed, but do delete the adapter.

Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 drivers/i2c/busses/i2c-sprd.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-sprd.c b/drivers/i2c/busses/i2c-sprd.c
index 4fe15cd78907..ffc54fbf814d 100644
--- a/drivers/i2c/busses/i2c-sprd.c
+++ b/drivers/i2c/busses/i2c-sprd.c
@@ -576,12 +576,14 @@ static int sprd_i2c_remove(struct platform_device *pdev)
 	struct sprd_i2c *i2c_dev = platform_get_drvdata(pdev);
 	int ret;
 
-	ret = pm_runtime_resume_and_get(i2c_dev->dev);
+	ret = pm_runtime_get_sync(i2c_dev->dev);
 	if (ret < 0)
-		return ret;
+		dev_err(&pdev->dev, "Failed to resume device (%pe)\n", ERR_PTR(ret));
 
 	i2c_del_adapter(&i2c_dev->adap);
-	clk_disable_unprepare(i2c_dev->clk);
+
+	if (ret >= 0)
+		clk_disable_unprepare(i2c_dev->clk);
 
 	pm_runtime_put_noidle(i2c_dev->dev);
 	pm_runtime_disable(i2c_dev->dev);
-- 
2.39.1

Re: [PATCH] i2c: sprd: Delete i2c adapter in .remove's error path

[Andy Shevchenko]

On Thu, Mar 9, 2023 at 11:58 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> If pm runtime resume fails the .remove callback used to exit early. This
> resulted in an error message by the driver core but the device gets
> removed anyhow. This lets the registered i2c adapter stay around with an
> unbound parent device.
>
> So only skip clk disabling if resume failed, but do delete the adapter.

Still worrisome. I would disable clock independently, but the questions are:
1) why the heck we need this dance with PM runtime for disabling clocks;
2) why can't we use devm_clk_get_enabled() or so in the probe;
?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] i2c: sprd: Delete i2c adapter in .remove's error path

[Uwe Kleine-König]

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

On Thu, Mar 09, 2023 at 01:17:22PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 9, 2023 at 11:58 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > If pm runtime resume fails the .remove callback used to exit early. This
> > resulted in an error message by the driver core but the device gets
> > removed anyhow. This lets the registered i2c adapter stay around with an
> > unbound parent device.
> >
> > So only skip clk disabling if resume failed, but do delete the adapter.
> 
> Still worrisome. I would disable clock independently, but the questions are:

Note that pm-runtime stuff disables the clk, so if resume failed, you
have to assume the clk already being off.

> 1) why the heck we need this dance with PM runtime for disabling clocks;
> 2) why can't we use devm_clk_get_enabled() or so in the probe;

These questions are orthogonal to my patch, right?

Runtime PM might delay suspend, so if you submit two transfers shortly
after another, this might be more effective as the device isn't
suspended in between. (Attention: half-baked knowledge)

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] i2c: sprd: Delete i2c adapter in .remove's error path

[Uwe Kleine-König]

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

Hello Andy, hello Wolfram,

On Thu, Mar 09, 2023 at 01:04:18PM +0100, Uwe Kleine-König wrote:
> On Thu, Mar 09, 2023 at 01:17:22PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 9, 2023 at 11:58 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > If pm runtime resume fails the .remove callback used to exit early. This
> > > resulted in an error message by the driver core but the device gets
> > > removed anyhow. This lets the registered i2c adapter stay around with an
> > > unbound parent device.
> > >
> > > So only skip clk disabling if resume failed, but do delete the adapter.
> > 
> > Still worrisome. I would disable clock independently, but the questions are:
> 
> Note that pm-runtime stuff disables the clk, so if resume failed, you
> have to assume the clk already being off.
> 
> > 1) why the heck we need this dance with PM runtime for disabling clocks;
> > 2) why can't we use devm_clk_get_enabled() or so in the probe;
> 
> These questions are orthogonal to my patch, right?
> 
> Runtime PM might delay suspend, so if you submit two transfers shortly
> after another, this might be more effective as the device isn't
> suspended in between. (Attention: half-baked knowledge)

I wonder if my reply was enough to sort out Andy's concerns?! If so,
would be great to get this patch applied.

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] i2c: sprd: Delete i2c adapter in .remove's error path

[Andi Shyti]

Hi Uwe,

On Thu, Mar 09, 2023 at 01:04:18PM +0100, Uwe Kleine-König wrote:
> On Thu, Mar 09, 2023 at 01:17:22PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 9, 2023 at 11:58 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > If pm runtime resume fails the .remove callback used to exit early. This
> > > resulted in an error message by the driver core but the device gets
> > > removed anyhow. This lets the registered i2c adapter stay around with an
> > > unbound parent device.
> > >
> > > So only skip clk disabling if resume failed, but do delete the adapter.
> > 
> > Still worrisome. I would disable clock independently, but the questions are:
> 
> Note that pm-runtime stuff disables the clk, so if resume failed, you
> have to assume the clk already being off.

this is because resume fails only of the clock fails to be
enabled and in that case we shouldn't worry about it...

> > 1) why the heck we need this dance with PM runtime for disabling clocks;
> > 2) why can't we use devm_clk_get_enabled() or so in the probe;
> 
> These questions are orthogonal to my patch, right?

... but (orthoginally to this patch) I guess Andy is asking what
happens to the clock when the remove is called through another
path. Maybe a next patch can use the devm_clk...() wrapper.

As for this one, I'm good with it:

Reviewed-by: Andi Shyti <andi.shyti@kernel.org>

Andi

> Runtime PM might delay suspend, so if you submit two transfers shortly
> after another, this might be more effective as the device isn't
> suspended in between. (Attention: half-baked knowledge)
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | https://www.pengutronix.de/ |

Re: [PATCH] i2c: sprd: Delete i2c adapter in .remove's error path

[Wolfram Sang]

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

On Thu, Mar 09, 2023 at 10:58:19AM +0100, Uwe Kleine-König wrote:
> If pm runtime resume fails the .remove callback used to exit early. This
> resulted in an error message by the driver core but the device gets
> removed anyhow. This lets the registered i2c adapter stay around with an
> unbound parent device.
> 
> So only skip clk disabling if resume failed, but do delete the adapter.
> 
> Fixes: 8b9ec0719834 ("i2c: Add Spreadtrum I2C controller driver")
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied to for-current, thanks!


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

Re: [PATCH] i2c: sprd: Delete i2c adapter in .remove's error path

[Geert Uytterhoeven]

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

 	Hi Andy,

Just noticed this patch is being backported to stable...

On Thu, 9 Mar 2023, Andy Shevchenko wrote:
> On Thu, Mar 9, 2023 at 11:58 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
>>
>> If pm runtime resume fails the .remove callback used to exit early. This
>> resulted in an error message by the driver core but the device gets
>> removed anyhow. This lets the registered i2c adapter stay around with an
>> unbound parent device.
>>
>> So only skip clk disabling if resume failed, but do delete the adapter.
>
> Still worrisome. I would disable clock independently, but the questions are:
> 1) why the heck we need this dance with PM runtime for disabling clocks;

Exactly. There is no point in calling pm_runtime_get_sync() and
pm_runtime_put_noidle() here, as no hardware is accessed in between,
is it?

> 2) why can't we use devm_clk_get_enabled() or so in the probe;
> ?

Because that helper didn't exist when this driver was introduced ;-)

Gr{oetje,eeting}s,

 						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
 							    -- Linus Torvalds