linux-kernel - RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

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] [day] [month] [year] [list]

Message-ID: <AM6PR04MB4966A89FA43404FCB8C9E4A080689@AM6PR04MB4966.eurprd04.prod.outlook.com>
Date:   Fri, 19 Mar 2021 10:26:36 +0000
From:   Aisheng Dong <aisheng.dong@....com>
To:     Clark Wang <xiaoning.wang@....com>,
        "shawnguo@...nel.org" <shawnguo@...nel.org>,
        "s.hauer@...gutronix.de" <s.hauer@...gutronix.de>
CC:     "kernel@...gutronix.de" <kernel@...gutronix.de>,
        "festevam@...il.com" <festevam@...il.com>,
        dl-linux-imx <linux-imx@....com>,
        "sumit.semwal@...aro.org" <sumit.semwal@...aro.org>,
        "christian.koenig@....com" <christian.koenig@....com>,
        "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 02/11] i2c: imx-lpi2c: add runtime pm support

[...]

> > > >  	pm_runtime_set_autosuspend_delay(&pdev->dev,
> > > I2C_PM_TIMEOUT);
> > > >  	pm_runtime_use_autosuspend(&pdev->dev);
> > > > -	pm_runtime_get_noresume(&pdev->dev);
> > > > -	pm_runtime_set_active(&pdev->dev);
> > > >  	pm_runtime_enable(&pdev->dev);
> > > >
> > > > +	ret = pm_runtime_get_sync(&pdev->dev);
> > > > +	if (ret < 0) {
> > > > +		pm_runtime_put_noidle(&pdev->dev);
> > > > +		dev_err(&pdev->dev, "failed to enable clock\n");
> > > > +		return ret;
> > > > +	}
> > >
> > > Can't current clk control via rpm work well?
> > > Please describe why need change.
> >
> > I think the previous patch maker might want to use the return value of
> > pm_runtime_get_sync to check whether the clock has been turned on
> > correctly to
> > avoid the kernel panic.
> > Maybe I can change to the method like this.
> > 	pm_runtime_get_noresume(&pdev->dev);
> > 	ret = pm_runtime_set_active(&pdev->dev);
> > 	if (ret < 0)
> > 		goto out;
> > 	pm_runtime_enable(&pdev->dev);
> >
> > Best Regards,
> > Clark Wang
> 
> Sorry, I missed the point before.
> If we use pm_runtime_get_noresume(&pdev->dev); and
> pm_runtime_set_active(&pdev->dev); here, the clk should be enabled by using
> clk_prepare_enable() in the probe function. However, the call of
> clk_prepare_enable() is already in lpi2c_runtime_resume().
> Using get_sync() here can help to reduce the repetitive code, especially ipg
> clk will be added later.

Let's not do for this negligible improvement unless there're any other good benefits.
If really care, move clk operation into an inline function instead.
Another benefit if not doing that is the driver still can work even RPM not enabled.

Regards
Aisheng

> Shall we change to use pm_runtime_get_sync() here?
> 
> Regards,
> Clark Wang
>