Hi,
On 30-04-18 04:37, Samuel Morris wrote:
On Thu, Apr 26, 2018 at 8:55 PM, Hans de Goede <hdegoede@xxxxxxxxxx> wrote:
Hi,
On 26-04-18 21:15, Tejun Heo wrote:
(cc'ing Hans. Can you please take a look at the patchset?)
On Fri, Apr 20, 2018 at 10:18:33AM +0000, 0v3rdr0n3@xxxxxxxxx wrote:
From: Samuel Morris <samorris@xxxxxxxxxxx>
A number of resources remain powered to support hotplug. On platforms
I've worked with, allowing the ahci_platform to suspend saves about
150mW. This patch allows the device to be auto suspended if the
config parameter is set.
The idea looks good to me but I really wish it were something which
can be turned on/off runtime rather than baked into a CONFIG option
(we can add a CONFIG option to select the default behavior).
I agree with your assessment, the idea looks good, but the
implementation really needs to change.
Here is how I think this should work:
-Always have runtime_pm callbacks, no #ifdef-ery for these
-The third patch calls runtime_pm_allow() from a weird place, the
proper way with a device being attached to a port or not is to
have the scan_host code do a runtime_pm_get_sync() before scanning
and do a put again when no device is found or keep the reference
when a device is found, this can be done always even for any ata
drivers which do not support runtime_pm, the calls will be nops
there, this also removes the weird #ifdef from the 3th patch
-Then on unplug the ref should be released by calling runtime_pm_put(),
this way on a hot unplug runtime pm will start working after the
unplug
The ata_host_register()->async_port_probe()->ata_scsi_scan_host()->pm_runtime_allow()
is there to balance
ata_host_register()->ata_tport_add()->pm_runtime_forbid(). I don't
really understand exactly why the forbid() is there, but here's the
commit message for that line:
Author: Lin Ming <ming.m.lin@xxxxxxxxx>
Date: Wed Apr 18 09:29:47 2012 +0800
libata: forbid port runtime pm by default, fixing regression
Forbid port runtime pm by default because it has known hotplug issue.
User can allow it by, for example
echo auto > /sys/devices/pci0000:00/0000:00:1f.2/ata2/power/control
Signed-off-by: Lin Ming <ming.m.lin@xxxxxxxxx>
Signed-off-by: Jeff Garzik <jgarzik@xxxxxxxxxx>
I don't suppose either of them are still around to explain? But it
seems hotplug won't work if the transport layer is allowed to runtime
suspend, so they forbid() it, makes sense. Why in the transport
device? I'm not sure.
Ah, so the AHCI code has runtime pm enabled by default (so there
is another pm_runtime_allow() somewhere, but then disables it for
unused ports to make hotpluging something into those ports work.
-Never call runtime_pm_allow() directly from the code, instead
users who want this can echo "auto" to the power/control sysfs
attribute, this will also give more fine grained (per device)
control over this. There are a number of examples in the kernel
of drivers implementing runtime-pm but needing such an echo to
enable it. A good example is almost all USB device drivers.
I think users could still reenable the hotplug functionality in this
case by echoing "on" to the power/control sysfs node. I just wanted to
change boot configuration. Maybe that should just be left to something
like udev though since it can match on specific device ids... That
would get rid of the need for that config switch, and that third
patch.
Yes I think that dropping the third patch and simply making sure that
we can get runtime pm for unused ports by requesting so from userspace
is the best solution.
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
