[patch 1/2] ahci: Dont start port DMA engines unless a device is present

[patch 1/2] ahci: Dont start port DMA engines unless a device is present

[Kristen Carlson Accardi]

According to the AHCI spec, you should not set PxCMD.ST unless
a functional device is present, as determined by PxTFD.STS.BSY=0,
PxTFD.STS.DRQ=0, and PxSSTS.DET = 3.

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: linux-2.6.25-rc3/drivers/ata/ahci.c
===================================================================
--- linux-2.6.25-rc3.orig/drivers/ata/ahci.c
+++ linux-2.6.25-rc3/drivers/ata/ahci.c
@@ -783,11 +783,40 @@ static int ahci_scr_write(struct ata_por
 	return -EINVAL;
 }
 
+static int ahci_is_device_present(struct ata_port *ap)
+{
+	void __iomem *port_mmio = ahci_port_base(ap);
+	u8 status;
+
+	/* Make sure PxTFD.STS.BSY and PxTFD.STS.DRQ are 0 */
+	status = ahci_check_status(ap);
+	if (status & (ATA_BUSY | ATA_DRQ))
+		return 0;
+
+	/* Make sure PxSSTS.DET is 3h */
+	status = readl(port_mmio + PORT_SCR_STAT);
+	status &= 0xf;
+	if ((status & 0xf) != 3)
+		return 0;
+	return 1;
+}
+
 static void ahci_start_engine(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
 	u32 tmp;
 
+	/*
+	 * See AHCI specification 1.2 section 10.1.2
+	 *
+	 * Software shall not set PxCMD.ST to '1' until it is
+ 	 * determined that a functional device is present on
+ 	 * the port, as determined by PxTFD.STS.BSY = '0',
+ 	 * PxTFD.STS.DRQ = '0', PxSSTS.DET = 3h
+ 	 */
+	if (!ahci_is_device_present(ap))
+		return;
+
 	/* start DMA */
 	tmp = readl(port_mmio + PORT_CMD);
 	tmp |= PORT_CMD_START;

-- 

Re: [patch 1/2] ahci: Dont start port DMA engines unless a device is present

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> According to the AHCI spec, you should not set PxCMD.ST unless
> a functional device is present, as determined by PxTFD.STS.BSY=0,
> PxTFD.STS.DRQ=0, and PxSSTS.DET = 3.
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> Index: linux-2.6.25-rc3/drivers/ata/ahci.c
> ===================================================================
> --- linux-2.6.25-rc3.orig/drivers/ata/ahci.c
> +++ linux-2.6.25-rc3/drivers/ata/ahci.c
> @@ -783,11 +783,40 @@ static int ahci_scr_write(struct ata_por
>  	return -EINVAL;
>  }
>  
> +static int ahci_is_device_present(struct ata_port *ap)
> +{
> +	void __iomem *port_mmio = ahci_port_base(ap);
> +	u8 status;
> +
> +	/* Make sure PxTFD.STS.BSY and PxTFD.STS.DRQ are 0 */
> +	status = ahci_check_status(ap);
> +	if (status & (ATA_BUSY | ATA_DRQ))
> +		return 0;
> +
> +	/* Make sure PxSSTS.DET is 3h */
> +	status = readl(port_mmio + PORT_SCR_STAT);
> +	status &= 0xf;
> +	if ((status & 0xf) != 3)
> +		return 0;
> +	return 1;
> +}
> +
>  static void ahci_start_engine(struct ata_port *ap)
>  {
>  	void __iomem *port_mmio = ahci_port_base(ap);
>  	u32 tmp;
>  
> +	/*
> +	 * See AHCI specification 1.2 section 10.1.2
> +	 *
> +	 * Software shall not set PxCMD.ST to '1' until it is
> + 	 * determined that a functional device is present on
> + 	 * the port, as determined by PxTFD.STS.BSY = '0',
> + 	 * PxTFD.STS.DRQ = '0', PxSSTS.DET = 3h
> + 	 */
> +	if (!ahci_is_device_present(ap))
> +		return;
> +
>  	/* start DMA */
>  	tmp = readl(port_mmio + PORT_CMD);

This seems incomplete?

If we wish to maintain this invariant, which seems like a good idea and 
the right thing to do, then this patch fails to cover the hotplug case, 
correct?

	Jeff

[patch 2/2] libata: power off unused ports

[Kristen Carlson Accardi]

If a port doesn't support hot plug, there's no reason to keep the phy powered
on unoccupied ports.  

Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>

Index: linux-2.6.25-rc3/drivers/ata/ahci.c
===================================================================
--- linux-2.6.25-rc3.orig/drivers/ata/ahci.c
+++ linux-2.6.25-rc3/drivers/ata/ahci.c
@@ -52,6 +52,7 @@
 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,
@@ -163,6 +164,7 @@ 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_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 */
@@ -783,6 +785,15 @@ static int ahci_scr_write(struct ata_por
 	return -EINVAL;
 }
 
+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);
+}
+
 static int ahci_is_device_present(struct ata_port *ap)
 {
 	void __iomem *port_mmio = ahci_port_base(ap);
@@ -2060,6 +2071,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-2.6.25-rc3/drivers/ata/libata-core.c
===================================================================
--- linux-2.6.25-rc3.orig/drivers/ata/libata-core.c
+++ linux-2.6.25-rc3/drivers/ata/libata-core.c
@@ -1004,6 +1004,19 @@ static void ata_lpm_disable(struct ata_h
 }
 #endif	/* CONFIG_PM */
 
+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_devchk - PATA device presence detection
@@ -2812,6 +2825,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);
 }
 
 /**
@@ -7111,6 +7125,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);
 
@@ -7130,6 +7146,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-2.6.25-rc3/include/linux/libata.h
===================================================================
--- linux-2.6.25-rc3.orig/include/linux/libata.h
+++ linux-2.6.25-rc3/include/linux/libata.h
@@ -194,6 +194,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

-- 

Re: [patch 2/2] libata: power off unused ports

[Andi Kleen]

Kristen Carlson Accardi <kristen.c.accardi@intel.com> writes:

> If a port doesn't support hot plug, there's no reason to keep the phy powered
> on unoccupied ports.  

Would it be possible to add some writable sysfs attribute for this too?

A lot of ports support hot plug, but the users knows they will never actually
hot plug anything there. Exposing it to sysfs would make sense so 
someone could make a conscious decision to save these watts by sacrifying
hotplug-support.

-Andi

Re: [patch 2/2] libata: power off unused ports

[Kristen Carlson Accardi]

On 28 Mar 2008 08:43:40 +0100
Andi Kleen <andi@firstfloor.org> wrote:

> Kristen Carlson Accardi <kristen.c.accardi@intel.com> writes:
> 
> > If a port doesn't support hot plug, there's no reason to keep the phy powered
> > on unoccupied ports.  
> 
> Would it be possible to add some writable sysfs attribute for this too?
> 
> A lot of ports support hot plug, but the users knows they will never actually
> hot plug anything there. Exposing it to sysfs would make sense so 
> someone could make a conscious decision to save these watts by sacrifying
> hotplug-support.
> 
> -Andi
> 

I had thought about extending the sysfs interface for link power 
management to include a new value: off.  Right now we have
min_power, medium_power, and max_performance - we could add
a value for just "off" which means the port is inactive -
but then we'd have to change all the sysfs stuff to check
and see if the user is turning the port back on after it's
been off and then re-init everything, so it might be a large
effort, so if we did that I'd say it should be a separate
effort from this patch series.

Re: [patch 2/2] libata: power off unused ports

[Jeff Garzik]

Kristen Carlson Accardi wrote:
> If a port doesn't support hot plug, there's no reason to keep the phy powered
> on unoccupied ports.  
> 
> Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> 
> Index: linux-2.6.25-rc3/drivers/ata/ahci.c
> ===================================================================
> --- linux-2.6.25-rc3.orig/drivers/ata/ahci.c
> +++ linux-2.6.25-rc3/drivers/ata/ahci.c
> @@ -52,6 +52,7 @@
>  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,
> @@ -163,6 +164,7 @@ 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_HPCP		= (1 << 18), /* port is hot plug capable */

Under which conditions is this bit set?

The conclusion reached by this patch seems correct, but I am not sure 
about the premise...

I was under the impression that AHCI ports were hotplug capable, from 
libata's point of view, simply due to the fundamentals of SATA.



Thinking about the bigger pictures, powering off the phy is something we 
want to do in a lot more cases than this, but there is a stumbling 
block:  we wander into the realm of policy.

For most users most of the time, empty SATA ports are needlessly 
powered.  The problem is that, at any given moment, a device may be 
hot-plugged, so we must be ready for that.  We need some way for the 
user to let the driver know that they will not be hotplugging anything 
anytime soon, permitting power savings to be enabled.

A compromise solution that avoids adding a userspace "knob" has also 
been proposed (by Tejun, I think?):  power up the phy every N seconds, 
check for device, power down phy if nothing.  That should provide some 
power savings, though not as much as with a "knob" switched to "hotplug: 
off"

	Jeff

Re: [patch 2/2] libata: power off unused ports

[Kristen Carlson Accardi]

On Sat, 12 Apr 2008 00:28:13 -0400
Jeff Garzik <jeff@garzik.org> wrote:

> Kristen Carlson Accardi wrote:
> > If a port doesn't support hot plug, there's no reason to keep the phy powered
> > on unoccupied ports.  
> > 
> > Signed-off-by:  Kristen Carlson Accardi <kristen.c.accardi@intel.com>
> > 
> > Index: linux-2.6.25-rc3/drivers/ata/ahci.c
> > ===================================================================
> > --- linux-2.6.25-rc3.orig/drivers/ata/ahci.c
> > +++ linux-2.6.25-rc3/drivers/ata/ahci.c
> > @@ -52,6 +52,7 @@
> >  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,
> > @@ -163,6 +164,7 @@ 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_HPCP		= (1 << 18), /* port is hot plug capable */
> 
> Under which conditions is this bit set?
> 
> The conclusion reached by this patch seems correct, but I am not sure 
> about the premise...
> 
> I was under the impression that AHCI ports were hotplug capable, from 
> libata's point of view, simply due to the fundamentals of SATA.

The HPCP bit is set by the BIOS to indicate policy for that port.
For example, a server may set this bit to indicate that there are
removable drive bays on it.  A laptop may not set this bit because nobody
is going to open it up and yank out the cable while it's running.  It is
safe to use this bit to figure out whether or not it's worth checking
for hot plug events on that port in practice, although you are right it
is physically possible for us to still do hot plug if we just pop open
the machine and go for it.

> 
> 
> 
> Thinking about the bigger pictures, powering off the phy is something we 
> want to do in a lot more cases than this, but there is a stumbling 
> block:  we wander into the realm of policy.
> 
> For most users most of the time, empty SATA ports are needlessly 
> powered.  The problem is that, at any given moment, a device may be 
> hot-plugged, so we must be ready for that.  We need some way for the 
> user to let the driver know that they will not be hotplugging anything 
> anytime soon, permitting power savings to be enabled.
> 
> A compromise solution that avoids adding a userspace "knob" has also 
> been proposed (by Tejun, I think?):  power up the phy every N seconds, 
> check for device, power down phy if nothing.  That should provide some 
> power savings, though not as much as with a "knob" switched to "hotplug: 
> off"
> 
> 	Jeff
> 
>

I think that for the common case - we should use HPCP to determine if
the suggested use of the port is for hot plug.  I can see your point
about wanting to give the user the option to just disregard the intended
use of the port and do what they want, but I say we don't make that
the default behavior.  And, I don't like the idea of adding another
wakeup in the driver to do polling - seems like 99% of users are
going to be just fine with a knob - and that they should only
have to use the knob to override the default (or how bout a 
module param?).  I don't think we should compromise power for a 
feature that most people are unlikely to use (if HPCP is not set).
 

Re: [patch 2/2] libata: power off unused ports

[Mark Lord]

Kristen Carlson Accardi wrote:
> On Sat, 12 Apr 2008 00:28:13 -0400
> Jeff Garzik <jeff@garzik.org> wrote:
..
>> Thinking about the bigger pictures, powering off the phy is something we 
>> want to do in a lot more cases than this, but there is a stumbling 
>> block:  we wander into the realm of policy.
>>
>> For most users most of the time, empty SATA ports are needlessly 
>> powered.  The problem is that, at any given moment, a device may be 
>> hot-plugged, so we must be ready for that.  We need some way for the 
>> user to let the driver know that they will not be hotplugging anything 
>> anytime soon, permitting power savings to be enabled.
>>
>> A compromise solution that avoids adding a userspace "knob" has also 
>> been proposed (by Tejun, I think?):  power up the phy every N seconds, 
>> check for device, power down phy if nothing.  That should provide some 
>> power savings, though not as much as with a "knob" switched to "hotplug: 
>> off"
..

How long does a poll actually take, in real time?
Probably a hundred milliseconds or two, to see if the PHY syncs up?

So with a knob set to, say 2 seconds (sysfs), then this would
save 90% of the power of a permanent "off".  That's pretty good,
and setting that same knob to "infinity" (-1 ?) would achieve 100%.
Definitely a good way to go, IMHO.

> I think that for the common case - we should use HPCP to determine if
> the suggested use of the port is for hot plug.  I can see your point
> about wanting to give the user the option to just disregard the intended
> use of the port and do what they want, but I say we don't make that
> the default behavior.  And, I don't like the idea of adding another
> wakeup in the driver to do polling - seems like 99% of users are
> going to be just fine with a knob - and that they should only
> have to use the knob to override the default (or how bout a 
> module param?).  I don't think we should compromise power for a 
> feature that most people are unlikely to use (if HPCP is not set).
..

I worry that this is far too x86 / vendor specific.
Most of the SATA ports I have here, for example, probably lack this HPCP bit
even on x86, and on other arch's it likely doesn't exist at all, right?

For arch/system/device that does have HPCP implemented, then sure,
it could be used to automatically tweak the sysfs knob.
But from userspace perhaps, rather than kernel ?

Thanks

Re: [patch 2/2] libata: power off unused ports

[Kristen Carlson Accardi]

On Tue, 15 Apr 2008 15:02:40 -0400
Mark Lord <liml@rtr.ca> wrote:

> Kristen Carlson Accardi wrote:
> > On Sat, 12 Apr 2008 00:28:13 -0400
> > Jeff Garzik <jeff@garzik.org> wrote:
> ..
> >> Thinking about the bigger pictures, powering off the phy is something we 
> >> want to do in a lot more cases than this, but there is a stumbling 
> >> block:  we wander into the realm of policy.
> >>
> >> For most users most of the time, empty SATA ports are needlessly 
> >> powered.  The problem is that, at any given moment, a device may be 
> >> hot-plugged, so we must be ready for that.  We need some way for the 
> >> user to let the driver know that they will not be hotplugging anything 
> >> anytime soon, permitting power savings to be enabled.
> >>
> >> A compromise solution that avoids adding a userspace "knob" has also 
> >> been proposed (by Tejun, I think?):  power up the phy every N seconds, 
> >> check for device, power down phy if nothing.  That should provide some 
> >> power savings, though not as much as with a "knob" switched to "hotplug: 
> >> off"
> ..
> 
> How long does a poll actually take, in real time?
> Probably a hundred milliseconds or two, to see if the PHY syncs up?
> 
> So with a knob set to, say 2 seconds (sysfs), then this would
> save 90% of the power of a permanent "off".  That's pretty good,
> and setting that same knob to "infinity" (-1 ?) would achieve 100%.
> Definitely a good way to go, IMHO.
> 
> > I think that for the common case - we should use HPCP to determine if
> > the suggested use of the port is for hot plug.  I can see your point
> > about wanting to give the user the option to just disregard the intended
> > use of the port and do what they want, but I say we don't make that
> > the default behavior.  And, I don't like the idea of adding another
> > wakeup in the driver to do polling - seems like 99% of users are
> > going to be just fine with a knob - and that they should only
> > have to use the knob to override the default (or how bout a 
> > module param?).  I don't think we should compromise power for a 
> > feature that most people are unlikely to use (if HPCP is not set).
> ..
> 
> I worry that this is far too x86 / vendor specific.
> Most of the SATA ports I have here, for example, probably lack this HPCP bit
> even on x86, and on other arch's it likely doesn't exist at all, right?
> 
> For arch/system/device that does have HPCP implemented, then sure,
> it could be used to automatically tweak the sysfs knob.
> But from userspace perhaps, rather than kernel ?
> 
> Thanks
> 

the HPCP bit is defined in the AHCI specification, so it is
architecture independent - as far as actual implementations
go, I don't know which system vendors have implemented it -
this should be set by firmware according to the spec (section
10.1.1), so you can have the same vendor/device chipset with
this set differently depending on the system configuration.  
I've also realized we want to leave phy powered on
by default if ESP is set as well, since that is another likely
case where the user would want hot plug on by default.  I think
the sysfs knob can override the default, but the kernel should
set the default to be based on the most likely usage scenario -
phy off if this port isn't exposed externally for hot 
plug either because it's an external sata port of because it's
got "signal and power connectors are externally accessible via 
a joint signal and power connector for blindmate device hot plug"
(e.g. removable drive bays).