| Message ID | 20180420101834.15783-2-0v3rdr0n3@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [v2,1/3] ata: ahci: rpm_put port on port_stop to match rpm_get in port_start | expand |
(cc'ing Hans. Can you please take a look at the patchset?) On Fri, Apr 20, 2018 at 10:18:33AM +0000, 0v3rdr0n3@gmail.com wrote: > From: Samuel Morris <samorris@lexmark.com> > > 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). Thanks.
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@gmail.com wrote: >> From: Samuel Morris <samorris@lexmark.com> >> >> 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 -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. 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 26, 2018 at 8:55 PM, Hans de Goede <hdegoede@redhat.com> 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@gmail.com wrote: >>> >>> From: Samuel Morris <samorris@lexmark.com> >>> >>> 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@intel.com> 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@intel.com> Signed-off-by: Jeff Garzik <jgarzik@redhat.com> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 30-04-18 04:37, Samuel Morris wrote: > On Thu, Apr 26, 2018 at 8:55 PM, Hans de Goede <hdegoede@redhat.com> 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@gmail.com wrote: >>>> >>>> From: Samuel Morris <samorris@lexmark.com> >>>> >>>> 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@intel.com> > 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@intel.com> > Signed-off-by: Jeff Garzik <jgarzik@redhat.com> > > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/ata/Kconfig b/drivers/ata/Kconfig index 2b16e7c8fff3..866663dd11ab 100644 --- a/drivers/ata/Kconfig +++ b/drivers/ata/Kconfig @@ -119,6 +119,16 @@ config SATA_AHCI_PLATFORM If unsure, say N. +config AHCI_HOTPLUG_DISABLED + tristate "Platform AHCI SATA hotplug disable" + default n + help + This option allows the AHCI Platform resources usually kept on + to support hotplug to be runtime suspended, effectively disabling + hotplug, but saving power. + + If unsure, say N. + config AHCI_BRCM tristate "Broadcom AHCI SATA support" depends on ARCH_BRCMSTB || BMIPS_GENERIC || ARCH_BCM_NSP diff --git a/drivers/ata/ahci_platform.c b/drivers/ata/ahci_platform.c index 99f9a895a459..e8f160e99ff5 100644 --- a/drivers/ata/ahci_platform.c +++ b/drivers/ata/ahci_platform.c @@ -68,8 +68,15 @@ static int ahci_probe(struct platform_device *pdev) return rc; } -static SIMPLE_DEV_PM_OPS(ahci_pm_ops, ahci_platform_suspend, - ahci_platform_resume); +#ifdef CONFIG_PM_SLEEP +static const struct dev_pm_ops ahci_pm_ops = { + SET_SYSTEM_SLEEP_PM_OPS(ahci_platform_suspend, ahci_platform_resume) +#ifdef CONFIG_AHCI_HOTPLUG_DISABLED + SET_RUNTIME_PM_OPS(ahci_platform_runtime_suspend, + ahci_platform_runtime_resume, NULL) +#endif +}; +#endif static const struct of_device_id ahci_of_match[] = { { .compatible = "generic-ahci", }, @@ -98,7 +105,9 @@ static struct platform_driver ahci_driver = { .name = DRV_NAME, .of_match_table = ahci_of_match, .acpi_match_table = ahci_acpi_match, +#ifdef CONFIG_PM_SLEEP .pm = &ahci_pm_ops, +#endif }, }; module_platform_driver(ahci_driver); diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c index 46a762442dc5..17fc4f19a53d 100644 --- a/drivers/ata/libahci_platform.c +++ b/drivers/ata/libahci_platform.c @@ -268,10 +268,14 @@ static void ahci_platform_put_resources(struct device *dev, void *res) struct ahci_host_priv *hpriv = res; int c; +#ifdef CONFIG_AHCI_HOTPLUG_DISABLED + pm_runtime_disable(dev); +#else if (hpriv->got_runtime_pm) { pm_runtime_put_sync(dev); pm_runtime_disable(dev); } +#endif for (c = 0; c < AHCI_MAX_CLKS && hpriv->clks[c]; c++) clk_put(hpriv->clks[c]); @@ -493,9 +497,15 @@ struct ahci_host_priv *ahci_platform_get_resources(struct platform_device *pdev) if (rc == -EPROBE_DEFER) goto err_out; } + +#ifdef CONFIG_AHCI_HOTPLUG_DISABLED + pm_runtime_set_active(dev); + pm_runtime_enable(dev); +#else pm_runtime_enable(dev); pm_runtime_get_sync(dev); hpriv->got_runtime_pm = true; +#endif devres_remove_group(dev, NULL); return hpriv; @@ -723,6 +733,21 @@ int ahci_platform_resume_host(struct device *dev) } EXPORT_SYMBOL_GPL(ahci_platform_resume_host); +static int _ahci_platform_suspend(struct device *dev) +{ + struct ata_host *host = dev_get_drvdata(dev); + struct ahci_host_priv *hpriv = host->private_data; + int rc; + + rc = ahci_platform_suspend_host(dev); + if (rc) + return rc; + + ahci_platform_disable_resources(hpriv); + + return 0; +} + /** * ahci_platform_suspend - Suspend an ahci-platform device * @dev: the platform device to suspend @@ -734,20 +759,45 @@ EXPORT_SYMBOL_GPL(ahci_platform_resume_host); * 0 on success otherwise a negative error code */ int ahci_platform_suspend(struct device *dev) +{ + return _ahci_platform_suspend(dev); +} +EXPORT_SYMBOL_GPL(ahci_platform_suspend); + +/** + * ahci_platform_runtime_suspend - Runtime suspend an ahci-platform device + * @dev: the platform device to suspend + * + * This function suspends the host associated with the device, followed by + * disabling all the resources of the device. + * + * RETURNS: + * 0 on success otherwise a negative error code + */ +int ahci_platform_runtime_suspend(struct device *dev) +{ + return _ahci_platform_suspend(dev); +} +EXPORT_SYMBOL_GPL(ahci_platform_runtime_suspend); + +static int _ahci_platform_resume(struct device *dev) { struct ata_host *host = dev_get_drvdata(dev); struct ahci_host_priv *hpriv = host->private_data; int rc; - rc = ahci_platform_suspend_host(dev); + rc = ahci_platform_enable_resources(hpriv); if (rc) return rc; - ahci_platform_disable_resources(hpriv); + rc = ahci_platform_resume_host(dev); + if (rc) { + ahci_platform_disable_resources(hpriv); + return rc; + } return 0; } -EXPORT_SYMBOL_GPL(ahci_platform_suspend); /** * ahci_platform_resume - Resume an ahci-platform device @@ -761,31 +811,37 @@ EXPORT_SYMBOL_GPL(ahci_platform_suspend); */ int ahci_platform_resume(struct device *dev) { - struct ata_host *host = dev_get_drvdata(dev); - struct ahci_host_priv *hpriv = host->private_data; int rc; - rc = ahci_platform_enable_resources(hpriv); + rc = _ahci_platform_resume(dev); if (rc) return rc; - rc = ahci_platform_resume_host(dev); - if (rc) - goto disable_resources; - /* We resumed so update PM runtime state */ pm_runtime_disable(dev); pm_runtime_set_active(dev); pm_runtime_enable(dev); return 0; - -disable_resources: - ahci_platform_disable_resources(hpriv); - - return rc; } EXPORT_SYMBOL_GPL(ahci_platform_resume); + +/** + * ahci_platform_runtime_resume - Runtime resume an ahci-platform device + * @dev: the platform device to resume + * + * This function enables all the resources of the device followed by + * resuming the host associated with the device. + * + * RETURNS: + * 0 on success otherwise a negative error code + */ +int ahci_platform_runtime_resume(struct device *dev) +{ + return _ahci_platform_resume(dev); +} +EXPORT_SYMBOL_GPL(ahci_platform_runtime_resume); + #endif MODULE_DESCRIPTION("AHCI SATA platform library"); diff --git a/include/linux/ahci_platform.h b/include/linux/ahci_platform.h index 1b0a17b22cd3..6396e6982103 100644 --- a/include/linux/ahci_platform.h +++ b/include/linux/ahci_platform.h @@ -42,5 +42,7 @@ int ahci_platform_suspend_host(struct device *dev); int ahci_platform_resume_host(struct device *dev); int ahci_platform_suspend(struct device *dev); int ahci_platform_resume(struct device *dev); +int ahci_platform_runtime_suspend(struct device *dev); +int ahci_platform_runtime_resume(struct device *dev); #endif /* _AHCI_PLATFORM_H */