Hi!
> > > As far as I know the patch has gone nowhere. I believe that
> > > Jeff wanted something more flexible than the module parameter that
> > > I provided to override the BIOS options. I am not working on this,
> > > I figured he had a pretty firm idea what he wanted so he was better
> > > equipped to write the patch.
> >
> > Thanks Kristen,
> >
> > Can you say which laptops you had tested this on where it saved power?
> > (Did you test any Thinkpads, in particular?) I'm wondering if it's
> > worth trying to forward port your patch as a private mod to my kernel;
> > 30 to 40 minutes of extra battery life is nothing to sneeze at!
> >
> > - Ted
> >
>
> I tested this on an Intel mobile software development platform
> with a newer mobile ICH - the power savings were measured at the actual
> component (via probes on the ICH), so I did not measure the power
> savings at the wall socket, although I would expect the power savings
> to be even greater on the other side of the power supply. So in short,
> yes, I think it's worth it to give it a try - the patch is pretty
> unintrusive, so it should be that difficult a port to do.
Can you repost the patch? I believe we should push it and only add
complex enable/disable functionality if someone needs it...
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Pavel Machek wrote:
> Hi!
>
>>>> As far as I know the patch has gone nowhere. I believe that
>>>> Jeff wanted something more flexible than the module parameter that
>>>> I provided to override the BIOS options. I am not working on this,
>>>> I figured he had a pretty firm idea what he wanted so he was better
>>>> equipped to write the patch.
>>> Thanks Kristen,
>>>
>>> Can you say which laptops you had tested this on where it saved power?
>>> (Did you test any Thinkpads, in particular?) I'm wondering if it's
>>> worth trying to forward port your patch as a private mod to my kernel;
>>> 30 to 40 minutes of extra battery life is nothing to sneeze at!
>>>
>>> - Ted
>>>
>> I tested this on an Intel mobile software development platform
>> with a newer mobile ICH - the power savings were measured at the actual
>> component (via probes on the ICH), so I did not measure the power
>> savings at the wall socket, although I would expect the power savings
>> to be even greater on the other side of the power supply. So in short,
>> yes, I think it's worth it to give it a try - the patch is pretty
>> unintrusive, so it should be that difficult a port to do.
>
> Can you repost the patch? I believe we should push it and only add
> complex enable/disable functionality if someone needs it...
If you are talking about SATA -- incorrect.
The patch deals with policy, and the user MUST have the ability to
control this stuff. Otherwise you create a situation where the user
might be denied hotplug use in valid cases, or similar negative situations.
Jeff
> If you are talking about SATA -- incorrect.
>
> The patch deals with policy, and the user MUST have the ability to
> control this stuff. Otherwise you create a situation where the user
> might be denied hotplug use in valid cases, or similar negative situations.
The policy isn't however complicated. Tejun added the stuff for forcing
cable type and mode on setup and has therefore written all the per device
setup code we might need. Alternatively a single
foo=1/0
option has been fine for acpi and will do fine for this. Total additional
cost - 1 line.
Alan
Alan Cox wrote:
>> If you are talking about SATA -- incorrect.
>>
>> The patch deals with policy, and the user MUST have the ability to
>> control this stuff. Otherwise you create a situation where the user
>> might be denied hotplug use in valid cases, or similar negative situations.
>
> The policy isn't however complicated. Tejun added the stuff for forcing
> cable type and mode on setup and has therefore written all the per device
> setup code we might need. Alternatively a single
>
> foo=1/0
>
> option has been fine for acpi and will do fine for this. Total additional
> cost - 1 line.
The key requirement is per-port control. Ideally via hdparm or another
userspace tool, but kernel command line (module options) or sysfs would
be just fine too. And agreed, the minimal you need is simply 1/0 for
the port's policy.
Jeff
> The key requirement is per-port control. Ideally via hdparm or another
> userspace tool, but kernel command line (module options) or sysfs would
> be just fine too. And agreed, the minimal you need is simply 1/0 for
> the port's policy.
We don't need it per port Jeff, you are being quite silly here. Right now
its permanently foo=0 for all ports and nobody has suffered anything too
horrible so being able to turn it off for all ports is clearly quite
sufficient for the neat future. If someone wants it per port they'll send
you a patch later.
Alan
Alan Cox wrote:
>> The key requirement is per-port control. Ideally via hdparm or another
>> userspace tool, but kernel command line (module options) or sysfs would
>> be just fine too. And agreed, the minimal you need is simply 1/0 for
>> the port's policy.
>
> We don't need it per port Jeff, you are being quite silly here. Right now
> its permanently foo=0 for all ports and nobody has suffered anything too
> horrible so being able to turn it off for all ports is clearly quite
> sufficient for the neat future.
Er, huh? foo=0 means hotplug continues to work on the unused ports.
Nobody has suffered because we default to enabling all the goodies.
Jeff
Jeff Garzik wrote:
> Alan Cox wrote:
>>> If you are talking about SATA -- incorrect.
>>>
>>> The patch deals with policy, and the user MUST have the ability to
>>> control this stuff. Otherwise you create a situation where the user
>>> might be denied hotplug use in valid cases, or similar negative
>>> situations.
>>
>> The policy isn't however complicated. Tejun added the stuff for forcing
>> cable type and mode on setup and has therefore written all the per device
>> setup code we might need. Alternatively a single
>>
>> foo=1/0
>>
>> option has been fine for acpi and will do fine for this. Total additional
>> cost - 1 line.
>
> The key requirement is per-port control. Ideally via hdparm or another
> userspace tool, but kernel command line (module options) or sysfs would
> be just fine too. And agreed, the minimal you need is simply 1/0 for
> the port's policy.
..
Btw.. hdparm-8.7 (unreleased) can grok /sys now, so that interface is
as good as any from a userspace viewpoint now.
For the power-off of unused ports, the current patch still sounds
extremely vendor-specific (Intel).
Does it actually work (demonstrate, please) on any other hardware ?
I would still like to see a far more generic solution, with periodic polling
and the like, which would permit use on *any* machines (eg. data centers)
without loss of hotplug capability on those ports.
But that's probably just wishful thinking at this point.
Cheers
> > We don't need it per port Jeff, you are being quite silly here. Right now
> > its permanently foo=0 for all ports and nobody has suffered anything too
> > horrible so being able to turn it off for all ports is clearly quite
> > sufficient for the neat future.
>
> Er, huh? foo=0 means hotplug continues to work on the unused ports.
> Nobody has suffered because we default to enabling all the goodies.
Thats a simple question of what "=0" means - is it "disable=1" or
"enable=1" or to quote Alice in Wonderland
'When I use a word,' Humpty Dumpty said, in a rather scornful
tone,' it means just what I choose it to mean, neither more nor
less
It really doesn't matter if it is on by default or off by default,
whether 0 means off or on, what matters is that the single switch is
quite sufficient initially to improve matters for everyone wanting to use
the feature without harming anyone else.
Sure you might want to make it per port later but someone who wants to
optimise that peculiar case, has a problem and can test it can deal with
that.
Alan
Mark Lord wrote:
> For the power-off of unused ports, the current patch still sounds
> extremely vendor-specific (Intel).
>
> Does it actually work (demonstrate, please) on any other hardware ?
Ding... correct. We need something per-port that's not Intel specific.
Jeff
On Mon, 02 Jun 2008 05:48:16 -0400
Jeff Garzik <[email protected]> wrote:
> Alan Cox wrote:
> >> The key requirement is per-port control. Ideally via hdparm or another
> >> userspace tool, but kernel command line (module options) or sysfs would
> >> be just fine too. And agreed, the minimal you need is simply 1/0 for
> >> the port's policy.
> >
> > We don't need it per port Jeff, you are being quite silly here. Right now
> > its permanently foo=0 for all ports and nobody has suffered anything too
> > horrible so being able to turn it off for all ports is clearly quite
> > sufficient for the neat future.
>
> Er, huh? foo=0 means hotplug continues to work on the unused ports.
> Nobody has suffered because we default to enabling all the goodies.
>
> Jeff
>
>
well - I disagree about nobody suffering. I would argue that most
users don't use hotplug and would prefer power savings to be the
default, but the patch I sent implemented a module parameter to allow
disable power savings if the user desires hotplug on every port.
Also keep in mind that this patch would still allow hotplug on a per
port basis if the BIOS tells us that this is either an external SATA
port or a port with some kind of externally accessible mechanism.
Here's the patch I sent so you can all review it again.
power off unused ports
If the port isn't either a drive bay or an external SATA port, do not
keep the phy powered on if the port is unoccupied. This saves .75 watts
on most laptops. A module parameter can be used to override this
behavior and allow the phy's to be powered up regardless of whether it
has externally accessible SATA ports.
Signed-off-by: Kristen Carlson Accardi <[email protected]>
---
drivers/ata/ahci.c | 21 +++++++++++++++++++++
drivers/ata/libata-core.c | 24 ++++++++++++++++++++++++
include/linux/libata.h | 1 +
3 files changed, 46 insertions(+)
Index: linux-ahci-phy/drivers/ata/ahci.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/ahci.c 2008-05-08 14:29:02.000000000 -0700
+++ linux-ahci-phy/drivers/ata/ahci.c 2008-05-08 14:31:05.000000000 -0700
@@ -53,9 +53,13 @@ static int ahci_skip_host_reset;
module_param_named(skip_host_reset, ahci_skip_host_reset, int, 0444);
MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)");
+static int ahci_power_save = 1;
+module_param_named(power_save, ahci_power_save, int, 0444);
+MODULE_PARM_DESC(power_save, "Power off unused ports (0=don't power off, 1=power off)");
static int ahci_enable_alpm(struct ata_port *ap,
enum link_pm policy);
static void ahci_disable_alpm(struct ata_port *ap);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
enum {
AHCI_PCI_BAR = 5,
@@ -166,6 +170,8 @@ enum {
PORT_CMD_ASP = (1 << 27), /* Aggressive Slumber/Partial */
PORT_CMD_ALPE = (1 << 26), /* Aggressive Link PM enable */
PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
+ PORT_CMD_ESP = (1 << 21), /* External SATA Port */
+ PORT_CMD_HPCP = (1 << 18), /* port is hot plug capable */
PORT_CMD_PMP = (1 << 17), /* PMP attached */
PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
PORT_CMD_FIS_ON = (1 << 14), /* FIS DMA engine running */
@@ -1900,6 +1906,18 @@ static int ahci_pci_device_resume(struct
}
#endif
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+ void __iomem *port_mmio = ahci_port_base(ap);
+ u8 cmd;
+
+ if (!ahci_power_save)
+ return 1;
+
+ cmd = readl(port_mmio + PORT_CMD);
+ return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
+}
+
static int ahci_port_start(struct ata_port *ap)
{
struct device *dev = ap->host->dev;
@@ -1951,6 +1969,9 @@ static int ahci_port_start(struct ata_po
ap->private_data = pp;
+ /* set some flags based on port capabilities */
+ if (!ahci_is_hotplug_capable(ap))
+ ap->flags |= ATA_FLAG_NO_HOTPLUG;
/* engage engines, captain */
return ahci_port_resume(ap);
}
Index: linux-ahci-phy/drivers/ata/libata-core.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/libata-core.c 2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/drivers/ata/libata-core.c 2008-05-08 14:29:50.000000000 -0700
@@ -162,6 +162,19 @@ MODULE_DESCRIPTION("Library module for A
MODULE_LICENSE("GPL");
MODULE_VERSION(DRV_VERSION);
+static void ata_phy_offline(struct ata_link *link)
+{
+ u32 scontrol;
+ int rc;
+
+ /* set DET to 4 */
+ rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+ if (rc)
+ return;
+ scontrol &= ~0xf;
+ scontrol |= (1 << 2);
+ sata_scr_write(link, SCR_CONTROL, scontrol);
+}
/**
* ata_force_cbl - force cable type according to libata.force
@@ -2671,6 +2684,7 @@ void ata_port_disable(struct ata_port *a
ap->link.device[0].class = ATA_DEV_NONE;
ap->link.device[1].class = ATA_DEV_NONE;
ap->flags |= ATA_FLAG_DISABLED;
+ ata_phy_offline(&ap->link);
}
/**
@@ -5609,6 +5623,8 @@ int ata_host_register(struct ata_host *h
if (ap->ops->error_handler) {
struct ata_eh_info *ehi = &ap->link.eh_info;
unsigned long flags;
+ int device_attached = 0;
+ struct ata_device *dev;
ata_port_probe(ap);
@@ -5627,6 +5643,14 @@ int ata_host_register(struct ata_host *h
/* wait for EH to finish */
ata_port_wait_eh(ap);
+ ata_link_for_each_dev(dev, &ap->link)
+ if (ata_dev_enabled(dev))
+ device_attached++;
+ if (!device_attached &&
+ (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+ /* no device present, disable port */
+ ata_port_disable(ap);
+ }
} else {
DPRINTK("ata%u: bus probe begin\n", ap->print_id);
rc = ata_bus_probe(ap);
Index: linux-ahci-phy/include/linux/libata.h
===================================================================
--- linux-ahci-phy.orig/include/linux/libata.h 2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/include/linux/libata.h 2008-05-08 14:29:50.000000000 -0700
@@ -193,6 +193,7 @@ enum {
ATA_FLAG_AN = (1 << 18), /* controller supports AN */
ATA_FLAG_PMP = (1 << 19), /* controller supports PMP */
ATA_FLAG_IPM = (1 << 20), /* driver can handle IPM */
+ ATA_FLAG_NO_HOTPLUG = (1 << 21), /* port doesn't support HP */
/* The following flag belongs to ap->pflags but is kept in
* ap->flags because it's referenced in many LLDs and will be
On Mon, 02 Jun 2008 09:03:04 -0400
Mark Lord <[email protected]> wrote:
> Jeff Garzik wrote:
> > Alan Cox wrote:
> >>> If you are talking about SATA -- incorrect.
> >>>
> >>> The patch deals with policy, and the user MUST have the ability to
> >>> control this stuff. Otherwise you create a situation where the user
> >>> might be denied hotplug use in valid cases, or similar negative
> >>> situations.
> >>
> >> The policy isn't however complicated. Tejun added the stuff for forcing
> >> cable type and mode on setup and has therefore written all the per device
> >> setup code we might need. Alternatively a single
> >>
> >> foo=1/0
> >>
> >> option has been fine for acpi and will do fine for this. Total additional
> >> cost - 1 line.
> >
> > The key requirement is per-port control. Ideally via hdparm or another
> > userspace tool, but kernel command line (module options) or sysfs would
> > be just fine too. And agreed, the minimal you need is simply 1/0 for
> > the port's policy.
> ..
>
> Btw.. hdparm-8.7 (unreleased) can grok /sys now, so that interface is
> as good as any from a userspace viewpoint now.
>
> For the power-off of unused ports, the current patch still sounds
> extremely vendor-specific (Intel).
Wrong - this patch is implemented according to the AHCI spec and
has absolutely nothing vendor specific in it.
>
> Does it actually work (demonstrate, please) on any other hardware ?
If someone would like to test it on another AHCI compliant chipset
that would be great. I have none.
On Mon, Jun 2, 2008 at 12:07 PM, Jeff Garzik <[email protected]> wrote:
> Mark Lord wrote:
>>
>> For the power-off of unused ports, the current patch still sounds
>> extremely vendor-specific (Intel).
>>
>> Does it actually work (demonstrate, please) on any other hardware ?
>
> Ding... correct. We need something per-port that's not Intel specific.
>
> Jeff
But, isn't Alan correct that the new logic is useful to some and with
a simple boot option is can be used to enable / disable it.
So if it is defaulted it to disabled, and the end user is allowed to
have a boot option to enable it, you get a reasonable first step for
now.
Greg
--
Greg Freemyer
Litigation Triage Solutions Specialist
http://www.linkedin.com/in/gregfreemyer
First 99 Days Litigation White Paper -
http://www.norcrossgroup.com/forms/whitepapers/99%20Days%20whitepaper.pdf
The Norcross Group
The Intersection of Evidence & Technology
http://www.norcrossgroup.com
On Mon, 02 Jun 2008 12:07:02 -0400
Jeff Garzik <[email protected]> wrote:
> Mark Lord wrote:
> > For the power-off of unused ports, the current patch still sounds
> > extremely vendor-specific (Intel).
> >
> > Does it actually work (demonstrate, please) on any other hardware ?
>
> Ding... correct. We need something per-port that's not Intel specific.
>
> Jeff
>
>
>
Can you tell me what part of this patch is vendor specific? Maybe
I am misunderstanding what you are saying. Here's that patch again
in case you need to look at it again.
power off unused ports
If the port isn't either a drive bay or an external SATA port, do not
keep the phy powered on if the port is unoccupied. This saves .75 watts
on most laptops. A module parameter can be used to override this
behavior and allow the phy's to be powered up regardless of whether it
has externally accessible SATA ports.
Signed-off-by: Kristen Carlson Accardi <[email protected]>
---
drivers/ata/ahci.c | 21 +++++++++++++++++++++
drivers/ata/libata-core.c | 24 ++++++++++++++++++++++++
include/linux/libata.h | 1 +
3 files changed, 46 insertions(+)
Index: linux-ahci-phy/drivers/ata/ahci.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/ahci.c 2008-05-08 14:29:02.000000000 -0700
+++ linux-ahci-phy/drivers/ata/ahci.c 2008-05-08 14:31:05.000000000 -0700
@@ -53,9 +53,13 @@ static int ahci_skip_host_reset;
module_param_named(skip_host_reset, ahci_skip_host_reset, int, 0444);
MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)");
+static int ahci_power_save = 1;
+module_param_named(power_save, ahci_power_save, int, 0444);
+MODULE_PARM_DESC(power_save, "Power off unused ports (0=don't power off, 1=power off)");
static int ahci_enable_alpm(struct ata_port *ap,
enum link_pm policy);
static void ahci_disable_alpm(struct ata_port *ap);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
enum {
AHCI_PCI_BAR = 5,
@@ -166,6 +170,8 @@ enum {
PORT_CMD_ASP = (1 << 27), /* Aggressive Slumber/Partial */
PORT_CMD_ALPE = (1 << 26), /* Aggressive Link PM enable */
PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
+ PORT_CMD_ESP = (1 << 21), /* External SATA Port */
+ PORT_CMD_HPCP = (1 << 18), /* port is hot plug capable */
PORT_CMD_PMP = (1 << 17), /* PMP attached */
PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
PORT_CMD_FIS_ON = (1 << 14), /* FIS DMA engine running */
@@ -1900,6 +1906,18 @@ static int ahci_pci_device_resume(struct
}
#endif
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+ void __iomem *port_mmio = ahci_port_base(ap);
+ u8 cmd;
+
+ if (!ahci_power_save)
+ return 1;
+
+ cmd = readl(port_mmio + PORT_CMD);
+ return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
+}
+
static int ahci_port_start(struct ata_port *ap)
{
struct device *dev = ap->host->dev;
@@ -1951,6 +1969,9 @@ static int ahci_port_start(struct ata_po
ap->private_data = pp;
+ /* set some flags based on port capabilities */
+ if (!ahci_is_hotplug_capable(ap))
+ ap->flags |= ATA_FLAG_NO_HOTPLUG;
/* engage engines, captain */
return ahci_port_resume(ap);
}
Index: linux-ahci-phy/drivers/ata/libata-core.c
===================================================================
--- linux-ahci-phy.orig/drivers/ata/libata-core.c 2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/drivers/ata/libata-core.c 2008-05-08 14:29:50.000000000 -0700
@@ -162,6 +162,19 @@ MODULE_DESCRIPTION("Library module for A
MODULE_LICENSE("GPL");
MODULE_VERSION(DRV_VERSION);
+static void ata_phy_offline(struct ata_link *link)
+{
+ u32 scontrol;
+ int rc;
+
+ /* set DET to 4 */
+ rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+ if (rc)
+ return;
+ scontrol &= ~0xf;
+ scontrol |= (1 << 2);
+ sata_scr_write(link, SCR_CONTROL, scontrol);
+}
/**
* ata_force_cbl - force cable type according to libata.force
@@ -2671,6 +2684,7 @@ void ata_port_disable(struct ata_port *a
ap->link.device[0].class = ATA_DEV_NONE;
ap->link.device[1].class = ATA_DEV_NONE;
ap->flags |= ATA_FLAG_DISABLED;
+ ata_phy_offline(&ap->link);
}
/**
@@ -5609,6 +5623,8 @@ int ata_host_register(struct ata_host *h
if (ap->ops->error_handler) {
struct ata_eh_info *ehi = &ap->link.eh_info;
unsigned long flags;
+ int device_attached = 0;
+ struct ata_device *dev;
ata_port_probe(ap);
@@ -5627,6 +5643,14 @@ int ata_host_register(struct ata_host *h
/* wait for EH to finish */
ata_port_wait_eh(ap);
+ ata_link_for_each_dev(dev, &ap->link)
+ if (ata_dev_enabled(dev))
+ device_attached++;
+ if (!device_attached &&
+ (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+ /* no device present, disable port */
+ ata_port_disable(ap);
+ }
} else {
DPRINTK("ata%u: bus probe begin\n", ap->print_id);
rc = ata_bus_probe(ap);
Index: linux-ahci-phy/include/linux/libata.h
===================================================================
--- linux-ahci-phy.orig/include/linux/libata.h 2008-05-08 14:28:57.000000000 -0700
+++ linux-ahci-phy/include/linux/libata.h 2008-05-08 14:29:50.000000000 -0700
@@ -193,6 +193,7 @@ enum {
ATA_FLAG_AN = (1 << 18), /* controller supports AN */
ATA_FLAG_PMP = (1 << 19), /* controller supports PMP */
ATA_FLAG_IPM = (1 << 20), /* driver can handle IPM */
+ ATA_FLAG_NO_HOTPLUG = (1 << 21), /* port doesn't support HP */
/* The following flag belongs to ap->pflags but is kept in
* ap->flags because it's referenced in many LLDs and will be
Kristen Carlson Accardi wrote:
> On Mon, 02 Jun 2008 09:03:04 -0400
> Mark Lord <[email protected]> wrote:
>
>> Jeff Garzik wrote:
>>> Alan Cox wrote:
>>>>> If you are talking about SATA -- incorrect.
>>>>>
>>>>> The patch deals with policy, and the user MUST have the ability to
>>>>> control this stuff. Otherwise you create a situation where the user
>>>>> might be denied hotplug use in valid cases, or similar negative
>>>>> situations.
>>>> The policy isn't however complicated. Tejun added the stuff for forcing
>>>> cable type and mode on setup and has therefore written all the per device
>>>> setup code we might need. Alternatively a single
>>>>
>>>> foo=1/0
>>>>
>>>> option has been fine for acpi and will do fine for this. Total additional
>>>> cost - 1 line.
>>> The key requirement is per-port control. Ideally via hdparm or another
>>> userspace tool, but kernel command line (module options) or sysfs would
>>> be just fine too. And agreed, the minimal you need is simply 1/0 for
>>> the port's policy.
>> ..
>>
>> Btw.. hdparm-8.7 (unreleased) can grok /sys now, so that interface is
>> as good as any from a userspace viewpoint now.
>>
>> For the power-off of unused ports, the current patch still sounds
>> extremely vendor-specific (Intel).
>
> Wrong - this patch is implemented according to the AHCI spec and
> has absolutely nothing vendor specific in it.
ITHM AHCI-specific, and not usable by non-AHCI SATA controllers -- which
we would want in any solution.
Jeff
Kristen Carlson Accardi wrote:
> Can you tell me what part of this patch is vendor specific? Maybe
> I am misunderstanding what you are saying. Here's that patch again
> in case you need to look at it again.
It's specific to AHCI, and does not cover other controllers.
Certainly PORT_CMD_HPCP must be, but the concept itself is not at all
AHCI-specific.
Jeff
On Mon, 02 Jun 2008 13:45:15 -0400
Jeff Garzik <[email protected]> wrote:
> Kristen Carlson Accardi wrote:
> > Can you tell me what part of this patch is vendor specific? Maybe
> > I am misunderstanding what you are saying. Here's that patch again
> > in case you need to look at it again.
>
>
> It's specific to AHCI, and does not cover other controllers.
>
> Certainly PORT_CMD_HPCP must be, but the concept itself is not at all
> AHCI-specific.
>
> Jeff
>
>
@@ -5627,6 +5643,14 @@ int ata_host_register(struct ata_host *h
/* wait for EH to finish */
ata_port_wait_eh(ap);
+ ata_link_for_each_dev(dev, &ap->link)
+ if (ata_dev_enabled(dev))
+ device_attached++;
+ if (!device_attached &&
+ (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+ /* no device present, disable port */
+ ata_port_disable(ap);
+ }
} else {
Here you can see that it would be very easy for another driver
to just add code to set the NO_HOTPLUG flag and then this code
will work for them as well, since we power off the phy using
DET which is specified by SATA.
here's that code:
+static void ata_phy_offline(struct ata_link *link)
+{
+ u32 scontrol;
+ int rc;
+
+ /* set DET to 4 */
+ rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+ if (rc)
+ return;
+ scontrol &= ~0xf;
+ scontrol |= (1 << 2);
+ sata_scr_write(link, SCR_CONTROL, scontrol);
+}
while it's true that I only modified ahci.c to set
the flag, any other driver writer can do that for the
other drivers based on whatever they want to use to
determine whether to set the NO_HOTPLUG flag.
Kristen Carlson Accardi wrote:
> Here you can see that it would be very easy for another driver
> to just add code to set the NO_HOTPLUG flag and then this code
> will work for them as well, since we power off the phy using
> DET which is specified by SATA.
> here's that code:
Agreed. The main discussion in this sub-thread is more about user
interface. The user interface in this case, a module option, is
specific to AHCI.
Surely you can see how it is a bit silly to force each driver writer to
create the same user interface in each driver, to support a generic concept.
Jeff
On Mon, 02 Jun 2008 14:15:27 -0400
Jeff Garzik <[email protected]> wrote:
> Kristen Carlson Accardi wrote:
> > Here you can see that it would be very easy for another driver
> > to just add code to set the NO_HOTPLUG flag and then this code
> > will work for them as well, since we power off the phy using
> > DET which is specified by SATA.
>
> > here's that code:
>
> Agreed. The main discussion in this sub-thread is more about user
> interface. The user interface in this case, a module option, is
> specific to AHCI.
>
> Surely you can see how it is a bit silly to force each driver writer to
> create the same user interface in each driver, to support a generic concept.
>
> Jeff
>
>
Not all drivers will need a user interface to turn off hotplug
I would think. At any rate - I would think it'd be better to let
driver writers decide how they want their drivers to behave wrt
hotplug and power instead of forcing a generic policy on everyone.
This patch would provide users of AHCI controllers a way to save
power now, while you work on the grand scheme for polling/turning on off
hotplug via sysfs. It's an interim solution that impacts nobody but
ahci users and is can be easily integrated into whatever solution you
eventually work out.
Kristen Carlson Accardi wrote:
> On Mon, 02 Jun 2008 14:15:27 -0400
> Jeff Garzik <[email protected]> wrote:
>
>
>> Kristen Carlson Accardi wrote:
>>
>>> Here you can see that it would be very easy for another driver
>>> to just add code to set the NO_HOTPLUG flag and then this code
>>> will work for them as well, since we power off the phy using
>>> DET which is specified by SATA.
>>>
>>> here's that code:
>>>
>> Agreed. The main discussion in this sub-thread is more about user
>> interface. The user interface in this case, a module option, is
>> specific to AHCI.
>>
>> Surely you can see how it is a bit silly to force each driver writer to
>> create the same user interface in each driver, to support a generic concept.
>>
>> Jeff
>>
>>
>>
> Not all drivers will need a user interface to turn off hotplug
> I would think. At any rate - I would think it'd be better to let
> driver writers decide how they want their drivers to behave wrt
> hotplug and power instead of forcing a generic policy on everyone.
>
> This patch would provide users of AHCI controllers a way to save
> power now, while you work on the grand scheme for polling/turning on off
> hotplug via sysfs. It's an interim solution that impacts nobody but
> ahci users and is can be easily integrated into whatever solution you
> eventually work out.
>
I like the patch - it seems that it will help a lot of users out near
term in a very positive way while we iterate on the broader solution,
ric
Kristen Carlson Accardi wrote:
> On Mon, 02 Jun 2008 14:15:27 -0400
> Jeff Garzik <[email protected]> wrote:
>
>> Kristen Carlson Accardi wrote:
>>> Here you can see that it would be very easy for another driver
>>> to just add code to set the NO_HOTPLUG flag and then this code
>>> will work for them as well, since we power off the phy using
>>> DET which is specified by SATA.
>>> here's that code:
>> Agreed. The main discussion in this sub-thread is more about user
>> interface. The user interface in this case, a module option, is
>> specific to AHCI.
>>
>> Surely you can see how it is a bit silly to force each driver writer to
>> create the same user interface in each driver, to support a generic concept.
>>
>> Jeff
>>
>>
> Not all drivers will need a user interface to turn off hotplug
> I would think. At any rate - I would think it'd be better to let
> driver writers decide how they want their drivers to behave wrt
> hotplug and power instead of forcing a generic policy on everyone.
>
> This patch would provide users of AHCI controllers a way to save
> power now, while you work on the grand scheme for polling/turning on off
> hotplug via sysfs. It's an interim solution that impacts nobody but
> ahci users and is can be easily integrated into whatever solution you
> eventually work out.
It's a one-off driver-specific interface that will have to be supported
even after the same feature is available via libata core... that's not
the path to scalable, sustainable engineering.
I know user interfaces are annoying because you have to think about
chips other than your own, but that's life. Other hardware vendors have
to do it too.
Letting each driver have a different user interface is /unfriendly/ to
both developers users. It's easiest for Intel kernel developers, but
that is not our target audience :)
The biggest power savings for the largest amount of users can be had if
you take a moment and figure out what's best for Linux, rather than what
is best for Intel.
Because you can be damned sure SATA users with non-AHCI chips want this
power savings too... let's not put roadblocks to that in place in the
beginning (by adding one-off interfaces).
Jeff
Ric Wheeler wrote:
> Kristen Carlson Accardi wrote:
>> Not all drivers will need a user interface to turn off hotplug
>> I would think. At any rate - I would think it'd be better to let
>> driver writers decide how they want their drivers to behave wrt
>> hotplug and power instead of forcing a generic policy on everyone.
>>
>> This patch would provide users of AHCI controllers a way to save
>> power now, while you work on the grand scheme for polling/turning on off
>> hotplug via sysfs. It's an interim solution that impacts nobody but
>> ahci users and is can be easily integrated into whatever solution you
>> eventually work out.
> I like the patch - it seems that it will help a lot of users out near
> term in a very positive way while we iterate on the broader solution,
A better patch would enable the _possibility_ of power savings on
non-AHCI chips, and not add a one-off AHCI-specific user interface that
must be supported for years to come.
Jeff
Jeff Garzik wrote:
> Ric Wheeler wrote:
>> Kristen Carlson Accardi wrote:
>>> Not all drivers will need a user interface to turn off hotplug
>>> I would think. At any rate - I would think it'd be better to let
>>> driver writers decide how they want their drivers to behave wrt
>>> hotplug and power instead of forcing a generic policy on everyone.
>>>
>>> This patch would provide users of AHCI controllers a way to save
>>> power now, while you work on the grand scheme for polling/turning on
>>> off
>>> hotplug via sysfs. It's an interim solution that impacts nobody but
>>> ahci users and is can be easily integrated into whatever solution you
>>> eventually work out.
>
>> I like the patch - it seems that it will help a lot of users out
>> near term in a very positive way while we iterate on the broader
>> solution,
>
> A better patch would enable the _possibility_ of power savings on
> non-AHCI chips, and not add a one-off AHCI-specific user interface
> that must be supported for years to come.
>
> Jeff
I think that the patch does that first part pretty reasonably for all
chips as Kristen explained before.
If the user interface is the only obstacle and the base code seems to be
flexible enough for any device to take advantage of, it would still seem
to be a positive step forward to put in that base functionality (even
without the module param) to enable power saving.
It is always good to get the complete solution in (how hard can the user
interface be to code once we agree on what it should be ;-)), but this
seems to be a good first step.
ric
Ric Wheeler wrote:
> Jeff Garzik wrote:
>> Ric Wheeler wrote:
>>> Kristen Carlson Accardi wrote:
>>>> Not all drivers will need a user interface to turn off hotplug
>>>> I would think. At any rate - I would think it'd be better to let
>>>> driver writers decide how they want their drivers to behave wrt
>>>> hotplug and power instead of forcing a generic policy on everyone.
>>>>
>>>> This patch would provide users of AHCI controllers a way to save
>>>> power now, while you work on the grand scheme for polling/turning on
>>>> off
>>>> hotplug via sysfs. It's an interim solution that impacts nobody but
>>>> ahci users and is can be easily integrated into whatever solution you
>>>> eventually work out.
>>
>>> I like the patch - it seems that it will help a lot of users out
>>> near term in a very positive way while we iterate on the broader
>>> solution,
>>
>> A better patch would enable the _possibility_ of power savings on
>> non-AHCI chips, and not add a one-off AHCI-specific user interface
>> that must be supported for years to come.
>>
>> Jeff
> I think that the patch does that first part pretty reasonably for all
> chips as Kristen explained before.
>
> If the user interface is the only obstacle and the base code seems to be
> flexible enough for any device to take advantage of, it would still seem
> to be a positive step forward to put in that base functionality (even
> without the module param) to enable power saving.
>
> It is always good to get the complete solution in (how hard can the user
> interface be to code once we agree on what it should be ;-)), but this
> seems to be a good first step.
A prep step without the module param (ie. user interface) is fine.
That's how we deployed Async Notification. Kristen's AN stuff went into
libata well before the SCSI API was settled upon.
Jeff
On Mon, Jun 02, 2008 at 02:40:51PM -0400, Jeff Garzik wrote:
> A better patch would enable the _possibility_ of power savings on
> non-AHCI chips, and not add a one-off AHCI-specific user interface that
> must be supported for years to come.
So how about somethig like the following, where the module parameter
just gets moved to libata rather than ahci? Entirely untested, I don't
have ahci on this machine.
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 97f83fb..5913ebb 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -56,6 +56,7 @@ MODULE_PARM_DESC(skip_host_reset, "skip global host reset (0=don't skip, 1=skip)
static int ahci_enable_alpm(struct ata_port *ap,
enum link_pm policy);
static void ahci_disable_alpm(struct ata_port *ap);
+static int ahci_is_hotplug_capable(struct ata_port *ap);
enum {
AHCI_PCI_BAR = 5,
@@ -166,6 +167,8 @@ enum {
PORT_CMD_ASP = (1 << 27), /* Aggressive Slumber/Partial */
PORT_CMD_ALPE = (1 << 26), /* Aggressive Link PM enable */
PORT_CMD_ATAPI = (1 << 24), /* Device is ATAPI */
+ PORT_CMD_ESP = (1 << 21), /* External SATA Port */
+ PORT_CMD_HPCP = (1 << 18), /* port is hot plug capable */
PORT_CMD_PMP = (1 << 17), /* PMP attached */
PORT_CMD_LIST_ON = (1 << 15), /* cmd list DMA engine running */
PORT_CMD_FIS_ON = (1 << 14), /* FIS DMA engine running */
@@ -1900,6 +1903,15 @@ static int ahci_pci_device_resume(struct pci_dev *pdev)
}
#endif
+static int ahci_is_hotplug_capable(struct ata_port *ap)
+{
+ void __iomem *port_mmio = ahci_port_base(ap);
+ u8 cmd;
+
+ cmd = readl(port_mmio + PORT_CMD);
+ return ((cmd & PORT_CMD_HPCP) || (cmd & PORT_CMD_ESP));
+}
+
static int ahci_port_start(struct ata_port *ap)
{
struct device *dev = ap->host->dev;
@@ -1951,6 +1963,9 @@ static int ahci_port_start(struct ata_port *ap)
ap->private_data = pp;
+ /* set some flags based on port capabilities */
+ if (!ahci_is_hotplug_capable(ap))
+ ap->flags |= ATA_FLAG_NO_HOTPLUG;
/* engage engines, captain */
return ahci_port_resume(ap);
}
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 3c89f20..209efe5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -157,11 +157,28 @@ int libata_allow_tpm = 0;
module_param_named(allow_tpm, libata_allow_tpm, int, 0444);
MODULE_PARM_DESC(allow_tpm, "Permit the use of TPM commands");
+static int sata_power_save = 1;
+module_param_named(sata_power_save, sata_power_save, int, 0644);
+MODULE_PARM_DESC(sata_power_save, "Power off unused ports (0=don't power off, 1=power off)");
+
MODULE_AUTHOR("Jeff Garzik");
MODULE_DESCRIPTION("Library module for ATA devices");
MODULE_LICENSE("GPL");
MODULE_VERSION(DRV_VERSION);
+static void ata_phy_offline(struct ata_link *link)
+{
+ u32 scontrol;
+ int rc;
+
+ /* set DET to 4 */
+ rc = sata_scr_read(link, SCR_CONTROL, &scontrol);
+ if (rc)
+ return;
+ scontrol &= ~0xf;
+ scontrol |= (1 << 2);
+ sata_scr_write(link, SCR_CONTROL, scontrol);
+}
/**
* ata_force_cbl - force cable type according to libata.force
@@ -2678,6 +2695,7 @@ void ata_port_disable(struct ata_port *ap)
ap->link.device[0].class = ATA_DEV_NONE;
ap->link.device[1].class = ATA_DEV_NONE;
ap->flags |= ATA_FLAG_DISABLED;
+ ata_phy_offline(&ap->link);
}
/**
@@ -5614,6 +5632,8 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
if (ap->ops->error_handler) {
struct ata_eh_info *ehi = &ap->link.eh_info;
unsigned long flags;
+ int device_attached = 0;
+ struct ata_device *dev;
ata_port_probe(ap);
@@ -5632,6 +5652,14 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
/* wait for EH to finish */
ata_port_wait_eh(ap);
+ ata_link_for_each_dev(dev, &ap->link)
+ if (ata_dev_enabled(dev))
+ device_attached++;
+ if (!device_attached && sata_power_save &&
+ (ap->flags & ATA_FLAG_NO_HOTPLUG)) {
+ /* no device present, disable port */
+ ata_port_disable(ap);
+ }
} else {
DPRINTK("ata%u: bus probe begin\n", ap->print_id);
rc = ata_bus_probe(ap);
--
Matthew Garrett | [email protected]
On Mon, 02 Jun 2008 14:38:55 -0400
Jeff Garzik <[email protected]> wrote:
> Kristen Carlson Accardi wrote:
> > On Mon, 02 Jun 2008 14:15:27 -0400
> > Jeff Garzik <[email protected]> wrote:
> >
> >> Kristen Carlson Accardi wrote:
> >>> Here you can see that it would be very easy for another driver
> >>> to just add code to set the NO_HOTPLUG flag and then this code
> >>> will work for them as well, since we power off the phy using
> >>> DET which is specified by SATA.
> >>> here's that code:
> >> Agreed. The main discussion in this sub-thread is more about user
> >> interface. The user interface in this case, a module option, is
> >> specific to AHCI.
> >>
> >> Surely you can see how it is a bit silly to force each driver writer to
> >> create the same user interface in each driver, to support a generic concept.
> >>
> >> Jeff
> >>
> >>
> > Not all drivers will need a user interface to turn off hotplug
> > I would think. At any rate - I would think it'd be better to let
> > driver writers decide how they want their drivers to behave wrt
> > hotplug and power instead of forcing a generic policy on everyone.
> >
> > This patch would provide users of AHCI controllers a way to save
> > power now, while you work on the grand scheme for polling/turning on off
> > hotplug via sysfs. It's an interim solution that impacts nobody but
> > ahci users and is can be easily integrated into whatever solution you
> > eventually work out.
>
> It's a one-off driver-specific interface that will have to be supported
> even after the same feature is available via libata core... that's not
> the path to scalable, sustainable engineering.
Jeff - I think you are misunderstanding what the module parameter
is doing. It is not a substitute for the sysfs interface that you
proposed. You would not want to get rid of it ever. What it does
is set the default of the AHCI driver to favor the BIOS settings
for which ports are hotpluggable. the parameter is there to allow
users to override the BIOS settings at module load time. Once you
get your sysfs interface in, you would then be able to override
the default at run time.
>
> I know user interfaces are annoying because you have to think about
> chips other than your own, but that's life. Other hardware vendors have
> to do it too.
User interfaces are annoying because it takes 2 maintainers to agree
on them, that is all. Surely you remember how long it took AN to
get in? It was about 9 months. This patch can go in now while we
spend 9 months deciding how to do the ultimate sysfs interface.
>
> Letting each driver have a different user interface is /unfriendly/ to
> both developers users. It's easiest for Intel kernel developers, but
> that is not our target audience :)
>
> The biggest power savings for the largest amount of users can be had if
> you take a moment and figure out what's best for Linux, rather than what
> is best for Intel.
>
> Because you can be damned sure SATA users with non-AHCI chips want this
> power savings too... let's not put roadblocks to that in place in the
> beginning (by adding one-off interfaces).
As I've been trying to explain to you. a) this isn't a one off interface.
b) this patch doesn't prevent you at all from adding a generic interface
later. Perhaps you have a non-technical reason for not wanting this
patch, but I'd like to make sure there's nothing here that you are
just not understanding or something that I can fix. I obviously can't
fix your idea that I only care about Intel chips, not matter how utterly
wrong and not supported by the facts that is.