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.
>
> -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.
>
> So how does this sound?
>
> 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
