linux-kernel - Re: pm_runtime_resume_and

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20220720060627.r7ifcxu6uopgsasw@pengutronix.de>
Date:   Wed, 20 Jul 2022 08:06:27 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     "Rafael J. Wysocki" <rafael@...nel.org>
Cc:     Linux PM <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: pm_runtime_resume_and_get in .remove callbacks

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@...gutronix.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/ |

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)