linux-kernel - Re: [PATCH v2] ALSA: hda/tegra: enable clock during probe

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 for Android: free password hash cracker in your pocket

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

Message-ID: <2224002.YA0vJHJrjz@aspire.rjw.lan>
Date:   Tue, 05 Feb 2019 12:52:17 +0100
From:   "Rafael J. Wysocki" <rjw@...ysocki.net>
To:     Thierry Reding <thierry.reding@...il.com>
Cc:     Jon Hunter <jonathanh@...dia.com>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Takashi Iwai <tiwai@...e.de>,
        Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>,
        Sameer Pujar <spujar@...dia.com>,
        Jaroslav Kysela <perex@...ex.cz>,
        "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." 
        <alsa-devel@...a-project.org>, mkumard@...dia.com,
        rlokhande@...dia.com, sharadg@...dia.com,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        linux-tegra@...r.kernel.org, Linux PM <linux-pm@...r.kernel.org>
Subject: Re: [PATCH v2] ALSA: hda/tegra: enable clock during probe

On Monday, February 4, 2019 12:05:10 PM CET Thierry Reding wrote:
> 
> --FCuugMFkClbJLl1L
> Content-Type: text/plain; charset=us-ascii
> Content-Disposition: inline
> Content-Transfer-Encoding: quoted-printable
> 
> On Mon, Feb 04, 2019 at 09:53:32AM +0000, Jon Hunter wrote:
> >=20
> >=20
> > On 04/02/2019 08:45, Thierry Reding wrote:
> >=20
> > ...
> >=20
> > > The idea was, as I was saying below, to reuse dev_pm_ops even if
> > > !CONFIG_PM. So pm_runtime_enable() could be something like this:
> > >=20
> > > 	pm_runtime_enable(dev)
> > > 	{
> > > 		if (!CONFIG_PM)
> > > 			if (dev->pm_ops->resume)
> > > 				dev->pm_ops->resume(dev);
> > >=20
> > > 		...
> > > 	}
> > >=20
> > > But that's admittedly somewhat of a stretch. This could of course be
> > > made somewhat nicer by adding an explicit variant, say:
> > >=20
> > > 	pm_runtime_enable_foo(dev)
> > > 	{
> > > 		if (!CONFIG_PM && dev->pm_ops->resume)
> > > 			return dev->pm_ops->resume(dev);
> > >=20
> > > 		return 0;
> > > 	}
> > >=20
> > > Maybe the fact that I couldn't come up with a good name is a good
> > > indication that this is a bad idea...
> >=20
> > How about some new APIs called ...
> >=20
> > pm_runtime_enable_get()
> > pm_runtime_enable_get_sync()
> > pm_runtime_put_disable() (implies a put_sync)
> >=20
> > ... and in these APIs we add ...
> >=20
> > pm_runtime_enable_get(dev)
> > {
> > 	if (!CONFIG_PM && dev->pm_ops->resume)
> > 		return dev->pm_ops->resume(dev);

No, we're not adding this to the core.

While in principle you could provide a set of pointers to the routines
you want to be called when CONFIG_PM is unset, IMO it is just cleaner
and more straightforward (and fewer lines of code for that matter) to add
a simple conditional to each ->probe() and ->remove().

[cut]

> > None, but seems overkill just for this case.
> 
> But that's precisely the point. It's not just about this case. We've
> already got some drivers where we do a similar dance just to be able to
> support the, let's admit it, exotic case where somebody turns off PM. I
> think supporting !PM might have made sense at a point where we had no
> support for power management at all. But I think we've come a long way
> since then, and while we may still have some ways to go in some areas,
> we do fairly decent runtime PM most of the time, to the point where I no
> longer see any benefits in !PM.

This IMO is an excellent point.

Trying to handle the !CONFIG_PM case only really makes sense if you know
what to do to turn the device on without relying on things like PM domains
etc which are not even available if CONFIG_PM is not set.

IOW, if any of the platforms your driver is expected to work with require
something like genpd to even make the device functional, I wouldn't bother.

Thanks,
Rafael