Em Wed, 28 Apr 2021 12:05:26 +0200 Johan Hovold <johan@xxxxxxxxxx> escreveu: > On Wed, Apr 28, 2021 at 10:31:48AM +0200, Mauro Carvalho Chehab wrote: > > Em Tue, 27 Apr 2021 14:23:20 +0200 > > Johan Hovold <johan@xxxxxxxxxx> escreveu: > > > > You're call, but converting functioning drivers where the authors knew > > > what they were doing just because you're not used to the API and risk > > > introducing new bugs in the process isn't necessarily a good idea. > > > > The problem is that the above assumption is not necessarily true: > > based on the number of drivers that pm_runtime_get_sync() weren't > > decrementing usage_count on errors, several driver authors were not > > familiar enough with the PM runtime behavior by the time the drivers > > were written or converted to use the PM runtime, instead of the > > media .s_power()/.s_stream() callbacks. > > That may very well be the case. My point is just that this work needs to > be done carefully and by people familiar with the code (and runtime pm) > or you risk introducing new issues. Yeah, that's for sure. > I really don't want the bot-warning-suppression crew to start with this > for example. > > > > Especially since the pm_runtime_get_sync() will continue to be used > > > elsewhere, and possibly even in media in cases where you don't need to > > > check for errors (e.g. remove()). > > > > Talking about the remove(), I'm not sure if just ignoring errors > > there would do the right thing. I mean, if pm_runtime_get_sync() > > fails, probably any attempts to disable clocks and other things > > that depend on PM runtime will also (silently) fail. > > > > This may put the device on an unknown PM and keep clock lines enabled > > after its removal. > > Right, a resume failure is a pretty big issue and it's not really clear > how to to even handle that generally. But at remove() time you don't > have much choice but to go on and release resource anyway. > > So unless actually implementing some error handling too, using > pm_runtime_sync_get() without checking for errors is still preferred > over pm_runtime_resume_and_get(). That is > > pm_runtime_get_sync(); > /* cleanup */ > pm_runtime_disable() > pm_runtime_put_noidle(); > > is better than: > > ret = pm_runtime_resume_and_get(); > /* cleanup */ > pm_runtime_disable(); > if (ret == 0) > pm_runtime_put_noidle(); > > unless you also start doing something ret. Perhaps the best would be to use, instead: pm_runtime_get_noresume(); /* cleanup */ pm_runtime_disable() pm_runtime_put_noidle(); pm_runtime_set_suspended(); I mean, at least for my eyes, it doesn't make sense to do a PM resume during driver's removal/unbind time. Regards, Mauro > > Johan Thanks, Mauro
