pm_runtime_resume_and_get in .remove callbacks

pm_runtime_resume_and_get in .remove callbacks

[Uwe Kleine-König]

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

Hello,

there is a big bunch of kernel drivers (here:
drivers/i2c/busses/i2c-sprd.c) that have a remove callback that looks as
follows:

	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);
		if (ret < 0)
			return ret;

		i2c_del_adapter(&i2c_dev->adap);
		clk_disable_unprepare(i2c_dev->clk);

		pm_runtime_put_noidle(i2c_dev->dev);
		pm_runtime_disable(i2c_dev->dev);

		return 0;
	}

If pm_runtime_resume_and_get fails, the i2c adapter isn't removed, but
as the memory backing i2c_dev goes away---it was allocated using
devm_kzalloc in .probe()---the next i2c action will probably access
freed memory.

I'm not familiar enough with pm-runtime stuff, but wonder what
can/should be done about that. The obvious (to me) candidates are:

 - log an error if pm_runtime_resume_and_get() fails, but continue to
   clean up
 - don't check the return value at all

What do you think?

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: pm_runtime_resume_and_get in .remove callbacks

[Rafael J. Wysocki]

On Wed, Jul 13, 2022 at 10:47 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello,
>
> there is a big bunch of kernel drivers (here:
> drivers/i2c/busses/i2c-sprd.c) that have a remove callback that looks as
> follows:
>
>         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);
>                 if (ret < 0)
>                         return ret;
>
>                 i2c_del_adapter(&i2c_dev->adap);
>                 clk_disable_unprepare(i2c_dev->clk);
>
>                 pm_runtime_put_noidle(i2c_dev->dev);
>                 pm_runtime_disable(i2c_dev->dev);
>
>                 return 0;
>         }
>
> If pm_runtime_resume_and_get fails, the i2c adapter isn't removed, but
> as the memory backing i2c_dev goes away---it was allocated using
> devm_kzalloc in .probe()---the next i2c action will probably access
> freed memory.
>
> I'm not familiar enough with pm-runtime stuff, but wonder what
> can/should be done about that. The obvious (to me) candidates are:
>
>  - log an error if pm_runtime_resume_and_get() fails, but continue to
>    clean up
>  - don't check the return value at all
>
> What do you think?

(1) Use pm_runtime_get_sync() instead of pm_runtime_resume_and_get()
and don't check its return value,

or if that is not viable, because something really can run if and only
if the device is operational,

(2) do something like

ret = pm_runtime_resume_and_get(i2c_dev->dev);
i2c_del_adapter(&i2c_dev->adap);
if (ret >= 0)
        clk_disable_unprepare(i2c_dev->clk);

pm_runtime_put_noidle(i2c_dev->dev);
pm_runtime_disable(i2c_dev->dev);

Thanks!

Re: pm_runtime_resume_and_get in .remove callbacks

[Uwe Kleine-König]

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

Hello Rafael,

On Wed, Jul 13, 2022 at 07:47:39PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jul 13, 2022 at 10:47 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > Hello,
> >
> > there is a big bunch of kernel drivers (here:
> > drivers/i2c/busses/i2c-sprd.c) that have a remove callback that looks as
> > follows:
> >
> >         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);
> >                 if (ret < 0)
> >                         return ret;
> >
> >                 i2c_del_adapter(&i2c_dev->adap);
> >                 clk_disable_unprepare(i2c_dev->clk);
> >
> >                 pm_runtime_put_noidle(i2c_dev->dev);
> >                 pm_runtime_disable(i2c_dev->dev);
> >
> >                 return 0;
> >         }
> >
> > If pm_runtime_resume_and_get fails, the i2c adapter isn't removed, but
> > as the memory backing i2c_dev goes away---it was allocated using
> > devm_kzalloc in .probe()---the next i2c action will probably access
> > freed memory.
> >
> > I'm not familiar enough with pm-runtime stuff, but wonder what
> > can/should be done about that. The obvious (to me) candidates are:
> >
> >  - log an error if pm_runtime_resume_and_get() fails, but continue to
> >    clean up
> >  - don't check the return value at all
> >
> > What do you think?
> 
> (1) Use pm_runtime_get_sync() instead of pm_runtime_resume_and_get()
> and don't check its return value,
> 
> or if that is not viable, because something really can run if and only
> if the device is operational,
> 
> (2) do something like
> 
> ret = pm_runtime_resume_and_get(i2c_dev->dev);
> i2c_del_adapter(&i2c_dev->adap);
> if (ret >= 0)
>         clk_disable_unprepare(i2c_dev->clk);
> 
> pm_runtime_put_noidle(i2c_dev->dev);
> pm_runtime_disable(i2c_dev->dev);

Why would you not disable the clk if the resume failed?

Is it an option to not call one of the resume variants at all and only
call pm_runtime_disable()?

Thanks for your input,
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: pm_runtime_resume_and_get in .remove callbacks

[Rafael J. Wysocki]

On Wed, Jul 20, 2022 at 8:06 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> Hello Rafael,
>
> On Wed, Jul 13, 2022 at 07:47:39PM +0200, Rafael J. Wysocki wrote:
> > On Wed, Jul 13, 2022 at 10:47 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > Hello,
> > >
> > > there is a big bunch of kernel drivers (here:
> > > drivers/i2c/busses/i2c-sprd.c) that have a remove callback that looks as
> > > follows:
> > >
> > >         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);
> > >                 if (ret < 0)
> > >                         return ret;
> > >
> > >                 i2c_del_adapter(&i2c_dev->adap);
> > >                 clk_disable_unprepare(i2c_dev->clk);
> > >
> > >                 pm_runtime_put_noidle(i2c_dev->dev);
> > >                 pm_runtime_disable(i2c_dev->dev);
> > >
> > >                 return 0;
> > >         }
> > >
> > > If pm_runtime_resume_and_get fails, the i2c adapter isn't removed, but
> > > as the memory backing i2c_dev goes away---it was allocated using
> > > devm_kzalloc in .probe()---the next i2c action will probably access
> > > freed memory.
> > >
> > > I'm not familiar enough with pm-runtime stuff, but wonder what
> > > can/should be done about that. The obvious (to me) candidates are:
> > >
> > >  - log an error if pm_runtime_resume_and_get() fails, but continue to
> > >    clean up
> > >  - don't check the return value at all
> > >
> > > What do you think?
> >
> > (1) Use pm_runtime_get_sync() instead of pm_runtime_resume_and_get()
> > and don't check its return value,
> >
> > or if that is not viable, because something really can run if and only
> > if the device is operational,
> >
> > (2) do something like
> >
> > ret = pm_runtime_resume_and_get(i2c_dev->dev);
> > i2c_del_adapter(&i2c_dev->adap);
> > if (ret >= 0)
> >         clk_disable_unprepare(i2c_dev->clk);
> >
> > pm_runtime_put_noidle(i2c_dev->dev);
> > pm_runtime_disable(i2c_dev->dev);
>
> Why would you not disable the clk if the resume failed?

I thought that it might lead to problems if the device that failed to
resume was expected to be accessible.

If that's not the case, you can simply do (1).

> Is it an option to not call one of the resume variants at all and only
> call pm_runtime_disable()?

That depends on whether or not you need to manipulate the hardware in
the del/disable part.  If you need to access it there, it is better to
resume I think.  Otherwise, you don't have to do anything, but then
the next probe needs to be prepared for finding the device in the
suspended state.

Re: pm_runtime_resume_and_get in .remove callbacks

[Uwe Kleine-König]

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

Hello Rafael,

On Wed, Jul 20, 2022 at 12:19:09PM +0200, Rafael J. Wysocki wrote:
> On Wed, Jul 20, 2022 at 8:06 AM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> > On Wed, Jul 13, 2022 at 07:47:39PM +0200, Rafael J. Wysocki wrote:
> > > (1) Use pm_runtime_get_sync() instead of pm_runtime_resume_and_get()
> > > and don't check its return value,
> > >
> > > or if that is not viable, because something really can run if and only
> > > if the device is operational,
> > >
> > > (2) do something like
> > >
> > > ret = pm_runtime_resume_and_get(i2c_dev->dev);
> > > i2c_del_adapter(&i2c_dev->adap);
> > > if (ret >= 0)
> > >         clk_disable_unprepare(i2c_dev->clk);
> > >
> > > pm_runtime_put_noidle(i2c_dev->dev);
> > > pm_runtime_disable(i2c_dev->dev);
> >
> > Why would you not disable the clk if the resume failed?
> 
> I thought that it might lead to problems if the device that failed to
> resume was expected to be accessible.
> 
> If that's not the case, you can simply do (1).
> 
> > Is it an option to not call one of the resume variants at all and only
> > call pm_runtime_disable()?
> 
> That depends on whether or not you need to manipulate the hardware in
> the del/disable part.  If you need to access it there, it is better to
> resume I think.  Otherwise, you don't have to do anything, but then
> the next probe needs to be prepared for finding the device in the
> suspended state.

OK, thanks for your time. I think I understood it good enough to tackle
some of the problems I identified.

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 --]