diff mbox

[RFC] add DMA setup FIS auto-activate feature

Message ID 1248320734.9035.22.camel@sli10-desk.sh.intel.com
State Not Applicable
Delegated to: David Miller
Headers show

Commit Message

Shaohua Li July 23, 2009, 3:45 a.m. UTC 
hi,
The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
transfers. I had an attempt to add it, though my test doesn't show obvious
performance improvement (not worse too), I wonder why not add this feature?

Signed-off-by: Shaohua Li <shaohua.li@intel.com>


--
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

Comments

Tejun Heo July 23, 2009, 3:50 a.m. UTC | #1 
Shaohua Li wrote:
> hi,
> The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> transfers. I had an attempt to add it, though my test doesn't show obvious
> performance improvement (not worse too), I wonder why not add this feature?
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

Eh... It's not clear whether all controllers support AA and it's also
not clear whether devices which claim to support AA actually work
properly when AA is enabled.  For optional features in ATA(PI), the
best we can do is probably to allow enabling it from userland or bios.
It used to be better for ATA compared to ATAPI but with the
introduction of SSDs, the situation has gotten worse rapidly.  :-(

Thanks.
Jeff Garzik July 23, 2009, 3:59 a.m. UTC | #2 
Shaohua Li wrote:
> hi,
> The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> transfers. I had an attempt to add it, though my test doesn't show obvious
> performance improvement (not worse too), I wonder why not add this feature?
> 
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>

I agree, we should turn it on -- but doesn't this also have a controller 
dependency?

IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0 
hardware may not behave properly in response, no?

	Jeff


--
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
Shaohua Li July 23, 2009, 5:40 a.m. UTC | #3 
On Thu, Jul 23, 2009 at 11:59:11AM +0800, Jeff Garzik wrote:
> Shaohua Li wrote:
> > hi,
> > The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> > transfers. I had an attempt to add it, though my test doesn't show obvious
> > performance improvement (not worse too), I wonder why not add this feature?
> > 
> > Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> 
> I agree, we should turn it on -- but doesn't this also have a controller 
> dependency?
> 
> IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0 
> hardware may not behave properly in response, no?
It appears the AHCI spec doesn't define a HBA capability about this
feature, but I'm likely wrong as I'm not quite familar with SATA.

Thanks,
Shaohua
--
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
Tejun Heo July 23, 2009, 5:45 a.m. UTC | #4 
Shaohua Li wrote:
>> IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0 
>> hardware may not behave properly in response, no?
> It appears the AHCI spec doesn't define a HBA capability about this
> feature, but I'm likely wrong as I'm not quite familar with SATA.

IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement
the spec correctly, it should work.  But even for ahcis, if we enable
it by default, I'm fairly sure we'll be met by a number of unpleasant
surprises.  Not sure whether doing that would be worth the trouble or
not.

Thanks.
Jeff Garzik July 23, 2009, 6:07 a.m. UTC | #5 
Tejun Heo wrote:
> IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement

AHCI?  Perhaps.   But AA is not in the _Serial ATA_ 1.0 spec...

It was added in SATA II, and the SATA II specs specifically mention that 
AA was not present in SATA 1.0.

	Jeff


--
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
Tejun Heo July 23, 2009, 6:12 a.m. UTC | #6 
Jeff Garzik wrote:
> Tejun Heo wrote:
>> IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement
> 
> AHCI?  Perhaps.   But AA is not in the _Serial ATA_ 1.0 spec...
> 
> It was added in SATA II, and the SATA II specs specifically mention that
> AA was not present in SATA 1.0.

Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
and let the drivers set it.  It would probably be better to set it
during -rc's and then disable it for release for a few devel cycles.

Thanks.
Robert Hancock July 23, 2009, 6:18 a.m. UTC | #7 
On 07/22/2009 11:45 PM, Tejun Heo wrote:
> Shaohua Li wrote:
>>> IIRC, not all NCQ-capable controllers support this feature...  SATA 1.0
>>> hardware may not behave properly in response, no?
>> It appears the AHCI spec doesn't define a HBA capability about this
>> feature, but I'm likely wrong as I'm not quite familar with SATA.
>
> IIRC, AA is in the ahci spec from rev 1.0, so if controllers implement
> the spec correctly, it should work.  But even for ahcis, if we enable
> it by default, I'm fairly sure we'll be met by a number of unpleasant
> surprises.  Not sure whether doing that would be worth the trouble or
> not.

There's definitely a controller dependency here, as SATA 1.0 hardware 
won't respond properly if this feature is turned on. Likely we need a 
host flag to indicate whether the controller supports auto-activate. It 
seems like AHCI shouldn't be a problem, but it's not clear if any other 
controller types would support it.

As far as busted hardware not handling it properly.. well if it just 
ignores the enabling then that's not a problem as things will just work 
as before. There could be devices that claim support, don't ignore it 
but don't handle it properly.. but realistically the only way we can 
find out if that's the case is turning it on and see what breaks.
--
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
Jeff Garzik July 23, 2009, 6:48 a.m. UTC | #8 
Tejun Heo wrote:
> Jeff Garzik wrote:
>> AA was added in SATA II, and the SATA II specs specifically mention that
>> AA was not present in SATA 1.0.
> 
> Yeap, it wasn't in SATA 1.0 but it was in ahci 1.0 so _theoretically_
> all ahcis should be fine with it.  We can introduce ATA_FLAG_FPDMA_AA
> and let the drivers set it.


Yep -- that was the logical conclusion of my rhetorical question at the 
beginning of this thread ;-)

	Jeff


--
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
Sergei Shtylyov July 23, 2009, 9:55 a.m. UTC | #9 
Hello.

Shaohua Li wrote:

> hi,
> The SATA spec defines DMA setup FIS auto-activate optimization for FPDMA
> transfers. I had an attempt to add it, though my test doesn't show obvious
> performance improvement (not worse too), I wonder why not add this feature?
>
> Signed-off-by: Shaohua Li <shaohua.li@intel.com>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 2c6aeda..53cc355 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2303,6 +2303,7 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>  {
>  	struct ata_port *ap = dev->link->ap;
>  	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
> +	const u16 *id = dev->id;
>   

   Why introduce the variable if you're only using it once?

>  	if (!ata_id_has_ncq(dev->id)) {
>   

   Should be changed to just 'id'...

>  		desc[0] = '\0';
> @@ -2317,6 +2318,9 @@ static void ata_dev_config_ncq(struct ata_device *dev,
>  		dev->flags |= ATA_DFLAG_NCQ;
>  	}
>  
> +	if (ata_id_has_daa(id))
> +		ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE, SATA_DAA);
> +
>   

MBR, Sergei


--
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 mbox

Patch

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 2c6aeda..53cc355 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2303,6 +2303,7 @@  static void ata_dev_config_ncq(struct ata_device *dev,
 {
 	struct ata_port *ap = dev->link->ap;
 	int hdepth = 0, ddepth = ata_id_queue_depth(dev->id);
+	const u16 *id = dev->id;
 
 	if (!ata_id_has_ncq(dev->id)) {
 		desc[0] = '\0';
@@ -2317,6 +2318,9 @@  static void ata_dev_config_ncq(struct ata_device *dev,
 		dev->flags |= ATA_DFLAG_NCQ;
 	}
 
+	if (ata_id_has_daa(id))
+		ata_dev_set_feature(dev, SETFEATURES_SATA_ENABLE, SATA_DAA);
+
 	if (hdepth >= ddepth)
 		snprintf(desc, desc_sz, "NCQ (depth %d)", ddepth);
 	else
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 9c75921..bb5b6ba 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -306,6 +306,7 @@  enum {
 	/* SETFEATURE Sector counts for SATA features */
 	SATA_AN			= 0x05,  /* Asynchronous Notification */
 	SATA_DIPM		= 0x03,  /* Device Initiated Power Management */
+	SATA_DAA		= 0x02,  /* DMA Setup FIS Auto-Activate */
 
 	/* feature values for SET_MAX */
 	ATA_SET_MAX_ADDR	= 0x00,
@@ -525,6 +526,9 @@  static inline int ata_is_data(u8 prot)
 #define ata_id_has_atapi_AN(id)	\
 	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
 	  ((id)[78] & (1 << 5)) )
+#define ata_id_has_daa(id)	\
+	( (((id)[76] != 0x0000) && ((id)[76] != 0xffff)) && \
+	  ((id)[78] & (1 << 2)) )
 #define ata_id_iordy_disable(id) ((id)[ATA_ID_CAPABILITY] & (1 << 10))
 #define ata_id_has_iordy(id) ((id)[ATA_ID_CAPABILITY] & (1 << 11))
 #define ata_id_u32(id,n)	\