[PATCH 1/4] libata: Allow NCQ TRIM to be enabled or disabled with a module parameter

[PATCH 1/4] libata: Allow NCQ TRIM to be enabled or disabled with a module parameter

[Martin K. Petersen]

We have started seeing SSD firmware updates introduce support for queued
TRIM. Sadly, in most cases this support is completely untested and can
lead to either errors or data corruption.

Add two libata force flags that can be used to either enable or disable
queued TRIM support.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 Documentation/kernel-parameters.txt | 2 ++
 drivers/ata/libata-core.c           | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 61ab1628a057..a2e4891a714f 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -1774,6 +1774,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 
 			* [no]ncq: Turn on or off NCQ.
 
+			* [no]ncqtrim: Turn off queued DSM TRIM.
+
 			* nohrst, nosrst, norst: suppress hard, soft
                           and both resets.
 
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 4476fb590733..9cebd7872ac6 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -6472,6 +6472,8 @@ static int __init ata_parse_force_one(char **cur,
 		{ "3.0Gbps",	.spd_limit	= 2 },
 		{ "noncq",	.horkage_on	= ATA_HORKAGE_NONCQ },
 		{ "ncq",	.horkage_off	= ATA_HORKAGE_NONCQ },
+		{ "noncqtrim",	.horkage_on	= ATA_HORKAGE_NO_NCQ_TRIM },
+		{ "ncqtrim",	.horkage_off	= ATA_HORKAGE_NO_NCQ_TRIM },
 		{ "dump_id",	.horkage_on	= ATA_HORKAGE_DUMP_ID },
 		{ "pio0",	.xfer_mask	= 1 << (ATA_SHIFT_PIO + 0) },
 		{ "pio1",	.xfer_mask	= 1 << (ATA_SHIFT_PIO + 1) },
-- 
1.9.3

[PATCH 2/4] libata: Expose TRIM capability in sysfs

[Martin K. Petersen]

Create a sysfs "trim" attribute for each ata_device that displays
whether DSM TRIM is "unsupported", "unqueued", "forced_unqueued"
(blacklisted) or "queued".

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 Documentation/ABI/testing/sysfs-ata | 11 +++++++++++
 drivers/ata/libata-transport.c      | 22 ++++++++++++++++++++++
 2 files changed, 33 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-ata b/Documentation/ABI/testing/sysfs-ata
index 0a932155cbba..9231daef3813 100644
--- a/Documentation/ABI/testing/sysfs-ata
+++ b/Documentation/ABI/testing/sysfs-ata
@@ -90,6 +90,17 @@ gscr
 	130:	SATA_PMP_GSCR_SII_GPIO
 	Only valid if the device is a PM.
 
+trim
+
+	Shows the DSM TRIM mode currently used by the device. Valid
+	values are:
+	unsupported:		Drive does not support DSM TRIM
+	unqueued:		Drive supports unqueued DSM TRIM only
+	queued:			Drive supports queued DSM TRIM
+	forced_unqueued:	Drive's unqueued DSM support is known to be
+				buggy and only unqueued TRIM commands
+				are sent
+
 spdn_cnt
 
 	Number of time libata decided to lower the speed of link due to errors.
diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
index 3227b7c8a05f..d6c37bcd416d 100644
--- a/drivers/ata/libata-transport.c
+++ b/drivers/ata/libata-transport.c
@@ -560,6 +560,27 @@ show_ata_dev_gscr(struct device *dev,
 
 static DEVICE_ATTR(gscr, S_IRUGO, show_ata_dev_gscr, NULL);
 
+static ssize_t
+show_ata_dev_trim(struct device *dev,
+		  struct device_attribute *attr, char *buf)
+{
+	struct ata_device *ata_dev = transport_class_to_dev(dev);
+	unsigned char *mode;
+
+	if (!ata_id_has_trim(ata_dev->id))
+		mode = "unsupported";
+	else if (ata_dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM)
+			mode = "forced_unqueued";
+	else if (ata_fpdma_dsm_supported(ata_dev))
+		mode = "queued";
+	else
+		mode = "unqueued";
+
+	return snprintf(buf, 20, "%s\n", mode);
+}
+
+static DEVICE_ATTR(trim, S_IRUGO, show_ata_dev_trim, NULL);
+
 static DECLARE_TRANSPORT_CLASS(ata_dev_class,
 			       "ata_device", NULL, NULL, NULL);
 
@@ -733,6 +754,7 @@ struct scsi_transport_template *ata_attach_transport(void)
 	SETUP_DEV_ATTRIBUTE(ering);
 	SETUP_DEV_ATTRIBUTE(id);
 	SETUP_DEV_ATTRIBUTE(gscr);
+	SETUP_DEV_ATTRIBUTE(trim);
 	BUG_ON(count > ATA_DEV_ATTRS);
 	i->dev_attrs[count] = NULL;
 
-- 
1.9.3

[PATCH 3/4] libata: READ LOG DMA EXT support can be in either page 119 or 120

[Martin K. Petersen]

Support for the READ/WRITE LOG DMA EXT commands can be signaled either
in page 119 or page 120. We should check both pages.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 include/linux/ata.h | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/include/linux/ata.h b/include/linux/ata.h
index b666b773e111..fed36418dd1c 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -704,9 +704,19 @@ static inline bool ata_id_wcache_enabled(const u16 *id)
 
 static inline bool ata_id_has_read_log_dma_ext(const u16 *id)
 {
+	/* Word 86 must have bit 15 set */
 	if (!(id[ATA_ID_CFS_ENABLE_2] & (1 << 15)))
 		return false;
-	return id[ATA_ID_COMMAND_SET_3] & (1 << 3);
+
+	/* READ LOG DMA EXT support can be signaled either from word 119
+	 * or from word 120. The format is the same for both words: Bit
+	 * 15 must be cleared, bit 14 set and bit 3 set.
+	 */
+	if ((id[ATA_ID_COMMAND_SET_3] & 0xC008) == 0x4008 ||
+	    (id[ATA_ID_COMMAND_SET_4] & 0xC008) == 0x4008)
+		return true;
+
+	return false;
 }
 
 static inline bool ata_id_has_sense_reporting(const u16 *id)
-- 
1.9.3

[PATCH 4/4] libata: Fall back to unqueued READ LOG EXT if the DMA variant fails

[Martin K. Petersen]

Some devices advertise support for the READ/WRITE LOG DMA EXT commands
but fail when we try to issue them. This can lead to queued TRIM being
unintentionally disabled since the relevant feature flag is located in a
general purpose log page.

Fall back to unqueued READ LOG EXT if the DMA variant fails while
reading a log page.

Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
---
 drivers/ata/libata-eh.c | 12 +++++++++++-
 include/linux/libata.h  |  1 +
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 07f41be38fbe..2893563d0537 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -1507,13 +1507,17 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 {
 	struct ata_taskfile tf;
 	unsigned int err_mask;
+	bool dma = false;
 
 	DPRINTK("read log page - log 0x%x, page 0x%x\n", log, page);
 
+retry:
 	ata_tf_init(dev, &tf);
-	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id)) {
+	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
+	    !(dev->horkage & ATA_HORKAGE_NO_NCQ_LOG)) {
 		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
 		tf.protocol = ATA_PROT_DMA;
+		dma = true;
 	} else {
 		tf.command = ATA_CMD_READ_LOG_EXT;
 		tf.protocol = ATA_PROT_PIO;
@@ -1527,6 +1531,12 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
 				     buf, sectors * ATA_SECT_SIZE, 0);
 
+	if (err_mask && dma) {
+		dev->horkage |= ATA_HORKAGE_NO_NCQ_LOG;
+		ata_dev_warn(dev, "READ LOG DMA EXT failed, trying unqueued\n");
+		goto retry;
+	}
+
 	DPRINTK("EXIT, err_mask=%x\n", err_mask);
 	return err_mask;
 }
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 8dad4a307bb8..c3ef58014b33 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -424,6 +424,7 @@ enum {
 	ATA_HORKAGE_NOLPM	= (1 << 20),	/* don't use LPM */
 	ATA_HORKAGE_WD_BROKEN_LPM = (1 << 21),	/* some WDs have broken LPM */
 	ATA_HORKAGE_ZERO_AFTER_TRIM = (1 << 22),/* guarantees zero after trim */
+	ATA_HORKAGE_NO_NCQ_LOG	= (1 << 23),	/* don't use NCQ for log read */
 
 	 /* DMA mask for user DMA control: User visible values; DO NOT
 	    renumber */
-- 
1.9.3

Re: [PATCH 1/4] libata: Allow NCQ TRIM to be enabled or disabled with a module parameter

[Hannes Reinecke]

On 05/05/2015 03:54 AM, Martin K. Petersen wrote:
> We have started seeing SSD firmware updates introduce support for queued
> TRIM. Sadly, in most cases this support is completely untested and can
> lead to either errors or data corruption.
> 
> Add two libata force flags that can be used to either enable or disable
> queued TRIM support.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  Documentation/kernel-parameters.txt | 2 ++
>  drivers/ata/libata-core.c           | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 61ab1628a057..a2e4891a714f 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -1774,6 +1774,8 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  
>  			* [no]ncq: Turn on or off NCQ.
>  
> +			* [no]ncqtrim: Turn off queued DSM TRIM.
> +
>  			* nohrst, nosrst, norst: suppress hard, soft
>                            and both resets.
>  
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 4476fb590733..9cebd7872ac6 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -6472,6 +6472,8 @@ static int __init ata_parse_force_one(char **cur,
>  		{ "3.0Gbps",	.spd_limit	= 2 },
>  		{ "noncq",	.horkage_on	= ATA_HORKAGE_NONCQ },
>  		{ "ncq",	.horkage_off	= ATA_HORKAGE_NONCQ },
> +		{ "noncqtrim",	.horkage_on	= ATA_HORKAGE_NO_NCQ_TRIM },
> +		{ "ncqtrim",	.horkage_off	= ATA_HORKAGE_NO_NCQ_TRIM },
>  		{ "dump_id",	.horkage_on	= ATA_HORKAGE_DUMP_ID },
>  		{ "pio0",	.xfer_mask	= 1 << (ATA_SHIFT_PIO + 0) },
>  		{ "pio1",	.xfer_mask	= 1 << (ATA_SHIFT_PIO + 1) },
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/4] libata: Expose TRIM capability in sysfs

[Hannes Reinecke]

On 05/05/2015 03:54 AM, Martin K. Petersen wrote:
> Create a sysfs "trim" attribute for each ata_device that displays
> whether DSM TRIM is "unsupported", "unqueued", "forced_unqueued"
> (blacklisted) or "queued".
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  Documentation/ABI/testing/sysfs-ata | 11 +++++++++++
>  drivers/ata/libata-transport.c      | 22 ++++++++++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-ata b/Documentation/ABI/testing/sysfs-ata
> index 0a932155cbba..9231daef3813 100644
> --- a/Documentation/ABI/testing/sysfs-ata
> +++ b/Documentation/ABI/testing/sysfs-ata
> @@ -90,6 +90,17 @@ gscr
>  	130:	SATA_PMP_GSCR_SII_GPIO
>  	Only valid if the device is a PM.
>  
> +trim
> +
> +	Shows the DSM TRIM mode currently used by the device. Valid
> +	values are:
> +	unsupported:		Drive does not support DSM TRIM
> +	unqueued:		Drive supports unqueued DSM TRIM only
> +	queued:			Drive supports queued DSM TRIM
> +	forced_unqueued:	Drive's unqueued DSM support is known to be
> +				buggy and only unqueued TRIM commands
> +				are sent
> +
>  spdn_cnt
>  
>  	Number of time libata decided to lower the speed of link due to errors.
> diff --git a/drivers/ata/libata-transport.c b/drivers/ata/libata-transport.c
> index 3227b7c8a05f..d6c37bcd416d 100644
> --- a/drivers/ata/libata-transport.c
> +++ b/drivers/ata/libata-transport.c
> @@ -560,6 +560,27 @@ show_ata_dev_gscr(struct device *dev,
>  
>  static DEVICE_ATTR(gscr, S_IRUGO, show_ata_dev_gscr, NULL);
>  
> +static ssize_t
> +show_ata_dev_trim(struct device *dev,
> +		  struct device_attribute *attr, char *buf)
> +{
> +	struct ata_device *ata_dev = transport_class_to_dev(dev);
> +	unsigned char *mode;
> +
> +	if (!ata_id_has_trim(ata_dev->id))
> +		mode = "unsupported";
> +	else if (ata_dev->horkage & ATA_HORKAGE_NO_NCQ_TRIM)
> +			mode = "forced_unqueued";
> +	else if (ata_fpdma_dsm_supported(ata_dev))
> +		mode = "queued";
> +	else
> +		mode = "unqueued";
> +
> +	return snprintf(buf, 20, "%s\n", mode);
> +}
> +
> +static DEVICE_ATTR(trim, S_IRUGO, show_ata_dev_trim, NULL);
> +
>  static DECLARE_TRANSPORT_CLASS(ata_dev_class,
>  			       "ata_device", NULL, NULL, NULL);
>  
> @@ -733,6 +754,7 @@ struct scsi_transport_template *ata_attach_transport(void)
>  	SETUP_DEV_ATTRIBUTE(ering);
>  	SETUP_DEV_ATTRIBUTE(id);
>  	SETUP_DEV_ATTRIBUTE(gscr);
> +	SETUP_DEV_ATTRIBUTE(trim);
>  	BUG_ON(count > ATA_DEV_ATTRS);
>  	i->dev_attrs[count] = NULL;
>  
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/4] libata: READ LOG DMA EXT support can be in either page 119 or 120

[Hannes Reinecke]

On 05/05/2015 03:54 AM, Martin K. Petersen wrote:
> Support for the READ/WRITE LOG DMA EXT commands can be signaled either
> in page 119 or page 120. We should check both pages.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  include/linux/ata.h | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/ata.h b/include/linux/ata.h
> index b666b773e111..fed36418dd1c 100644
> --- a/include/linux/ata.h
> +++ b/include/linux/ata.h
> @@ -704,9 +704,19 @@ static inline bool ata_id_wcache_enabled(const u16 *id)
>  
>  static inline bool ata_id_has_read_log_dma_ext(const u16 *id)
>  {
> +	/* Word 86 must have bit 15 set */
>  	if (!(id[ATA_ID_CFS_ENABLE_2] & (1 << 15)))
>  		return false;
> -	return id[ATA_ID_COMMAND_SET_3] & (1 << 3);
> +
> +	/* READ LOG DMA EXT support can be signaled either from word 119
> +	 * or from word 120. The format is the same for both words: Bit
> +	 * 15 must be cleared, bit 14 set and bit 3 set.
> +	 */
> +	if ((id[ATA_ID_COMMAND_SET_3] & 0xC008) == 0x4008 ||
> +	    (id[ATA_ID_COMMAND_SET_4] & 0xC008) == 0x4008)
> +		return true;
> +
> +	return false;
>  }
>  
>  static inline bool ata_id_has_sense_reporting(const u16 *id)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 4/4] libata: Fall back to unqueued READ LOG EXT if the DMA variant fails

[Hannes Reinecke]

On 05/05/2015 03:54 AM, Martin K. Petersen wrote:
> Some devices advertise support for the READ/WRITE LOG DMA EXT commands
> but fail when we try to issue them. This can lead to queued TRIM being
> unintentionally disabled since the relevant feature flag is located in a
> general purpose log page.
> 
> Fall back to unqueued READ LOG EXT if the DMA variant fails while
> reading a log page.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>  drivers/ata/libata-eh.c | 12 +++++++++++-
>  include/linux/libata.h  |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 07f41be38fbe..2893563d0537 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1507,13 +1507,17 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  {
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
> +	bool dma = false;
>  
>  	DPRINTK("read log page - log 0x%x, page 0x%x\n", log, page);
>  
> +retry:
>  	ata_tf_init(dev, &tf);
> -	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id)) {
> +	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
> +	    !(dev->horkage & ATA_HORKAGE_NO_NCQ_LOG)) {
>  		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
>  		tf.protocol = ATA_PROT_DMA;
> +		dma = true;
>  	} else {
>  		tf.command = ATA_CMD_READ_LOG_EXT;
>  		tf.protocol = ATA_PROT_PIO;
> @@ -1527,6 +1531,12 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>  	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
>  				     buf, sectors * ATA_SECT_SIZE, 0);
>  
> +	if (err_mask && dma) {
> +		dev->horkage |= ATA_HORKAGE_NO_NCQ_LOG;
> +		ata_dev_warn(dev, "READ LOG DMA EXT failed, trying unqueued\n");
> +		goto retry;
> +	}
> +
>  	DPRINTK("EXIT, err_mask=%x\n", err_mask);
>  	return err_mask;
>  }
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 8dad4a307bb8..c3ef58014b33 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -424,6 +424,7 @@ enum {
>  	ATA_HORKAGE_NOLPM	= (1 << 20),	/* don't use LPM */
>  	ATA_HORKAGE_WD_BROKEN_LPM = (1 << 21),	/* some WDs have broken LPM */
>  	ATA_HORKAGE_ZERO_AFTER_TRIM = (1 << 22),/* guarantees zero after trim */
> +	ATA_HORKAGE_NO_NCQ_LOG	= (1 << 23),	/* don't use NCQ for log read */
>  
>  	 /* DMA mask for user DMA control: User visible values; DO NOT
>  	    renumber */
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 1/4] libata: Allow NCQ TRIM to be enabled or disabled with a module parameter

[Tejun Heo]

On Mon, May 04, 2015 at 09:54:18PM -0400, Martin K. Petersen wrote:
> We have started seeing SSD firmware updates introduce support for queued
> TRIM. Sadly, in most cases this support is completely untested and can
> lead to either errors or data corruption.
> 
> Add two libata force flags that can be used to either enable or disable
> queued TRIM support.
> 
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

Applied 1-4 to libata/for-4.2.

Thanks.

-- 
tejun

Re: [PATCH 1/4] libata: Allow NCQ TRIM to be enabled or disabled with a module parameter

[Hannes Reinecke]

On 05/05/2015 03:31 PM, Tejun Heo wrote:
> On Mon, May 04, 2015 at 09:54:18PM -0400, Martin K. Petersen wrote:
>> We have started seeing SSD firmware updates introduce support for queued
>> TRIM. Sadly, in most cases this support is completely untested and can
>> lead to either errors or data corruption.
>>
>> Add two libata force flags that can be used to either enable or disable
>> queued TRIM support.
>>
>> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> Applied 1-4 to libata/for-4.2.
> 
> Thanks.
> 
As a side note: queue TRIM requires 'SEND FPDMA QUEUED', which is an
_optional_ part of the NCQ feature set.
And unfortunately there are no identify bits telling us whether SEND
FPDMA QUEUE is actually implemented (or RECEIVE FPDMA QUEUED, for
that matter).
I've been advocating to implement this (it would come in very handy
for the SMR/ZAC stuff) but to no avail so far.
Needless to say, none of the drives I have implement SEND FPDMA
QUEUED...

Maybe it's an idea to poke the powers that be about additional
identify bits for this ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		               zSeries & Storage
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

Re: [PATCH 2/4] libata: Expose TRIM capability in sysfs

[Christoph Hellwig]

> +	forced_unqueued:	Drive's unqueued DSM support is known to be
> +				buggy and only unqueued TRIM commands
> +				are sent

Shouldn't the first "unqueued" be a "queued"

Re: [PATCH 2/4] libata: Expose TRIM capability in sysfs

[Martin K. Petersen]

>>>>> "Christoph" == Christoph Hellwig <hch@infradead.org> writes:

>> + forced_unqueued: Drive's unqueued DSM support is known to be +
>> buggy and only unqueued TRIM commands + are sent

Christoph> Shouldn't the first "unqueued" be a "queued"

Yes, sorry about that.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 1/4] libata: Allow NCQ TRIM to be enabled or disabled with a module parameter

[Martin K. Petersen]

>>>>> "Hannes" == Hannes Reinecke <hare@suse.de> writes:

Hannes> As a side note: queue TRIM requires 'SEND FPDMA QUEUED', which
Hannes> is an _optional_ part of the NCQ feature set.  And unfortunately
Hannes> there are no identify bits telling us whether SEND FPDMA QUEUE
Hannes> is actually implemented (or RECEIVE FPDMA QUEUED, for that
Hannes> matter).

Yeah. At least in this case one would assume the command is supported
when queued TRIM is advertised.

But as you saw from my patch series, I have one particular drive that
advertises support for queued READ LOG and yet it fails when you attempt
to use it.

*sigh*

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 4/4] libata: Fall back to unqueued READ LOG EXT if the DMA variant fails

[David Milburn]

Hi Martin,

On 05/04/2015 08:54 PM, Martin K. Petersen wrote:
> Some devices advertise support for the READ/WRITE LOG DMA EXT commands
> but fail when we try to issue them. This can lead to queued TRIM being
> unintentionally disabled since the relevant feature flag is located in a
> general purpose log page.
>
> Fall back to unqueued READ LOG EXT if the DMA variant fails while
> reading a log page.

Have you seen both commands fail on any of the Samsung SSD 840 EVO
drives?

# dmesg | grep Samsung
[    2.254443] ata10.00: ATA-9: Samsung SSD 840 EVO 1TB, EXT0BB6Q, max 
UDMA/133

<dmesg output>
[    2.254390] ata10.00: READ LOG DMA EXT failed, trying unqueued
[    2.254441] ata10.00: failed to get NCQ Send/Recv Log Emask 0x1
<dmesg output>

(In this case it also failed READ LOG EXT)

This is linux-4.1.0-rc7 patched with the following from
http://git.kernel.org/cgit/linux/kernel/git/tj/libata.git/log/?h=for-4.2

libata: Allow NCQ TRIM to be enabled or disabled with a module parameter
libata: Expose TRIM capability in sysfs
libata: READ LOG DMA EXT support can be in either page 119 or 120
libata: Fall back to unqueued READ LOG EXT if the DMA variant fails
libata: Fix regression when the NCQ Send and Receive log page is absent

Thanks,
David


>
> Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> ---
>   drivers/ata/libata-eh.c | 12 +++++++++++-
>   include/linux/libata.h  |  1 +
>   2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 07f41be38fbe..2893563d0537 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -1507,13 +1507,17 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>   {
>   	struct ata_taskfile tf;
>   	unsigned int err_mask;
> +	bool dma = false;
>
>   	DPRINTK("read log page - log 0x%x, page 0x%x\n", log, page);
>
> +retry:
>   	ata_tf_init(dev, &tf);
> -	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id)) {
> +	if (dev->dma_mode && ata_id_has_read_log_dma_ext(dev->id) &&
> +	    !(dev->horkage & ATA_HORKAGE_NO_NCQ_LOG)) {
>   		tf.command = ATA_CMD_READ_LOG_DMA_EXT;
>   		tf.protocol = ATA_PROT_DMA;
> +		dma = true;
>   	} else {
>   		tf.command = ATA_CMD_READ_LOG_EXT;
>   		tf.protocol = ATA_PROT_PIO;
> @@ -1527,6 +1531,12 @@ unsigned int ata_read_log_page(struct ata_device *dev, u8 log,
>   	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_FROM_DEVICE,
>   				     buf, sectors * ATA_SECT_SIZE, 0);
>
> +	if (err_mask && dma) {
> +		dev->horkage |= ATA_HORKAGE_NO_NCQ_LOG;
> +		ata_dev_warn(dev, "READ LOG DMA EXT failed, trying unqueued\n");
> +		goto retry;
> +	}
> +
>   	DPRINTK("EXIT, err_mask=%x\n", err_mask);
>   	return err_mask;
>   }
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 8dad4a307bb8..c3ef58014b33 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -424,6 +424,7 @@ enum {
>   	ATA_HORKAGE_NOLPM	= (1 << 20),	/* don't use LPM */
>   	ATA_HORKAGE_WD_BROKEN_LPM = (1 << 21),	/* some WDs have broken LPM */
>   	ATA_HORKAGE_ZERO_AFTER_TRIM = (1 << 22),/* guarantees zero after trim */
> +	ATA_HORKAGE_NO_NCQ_LOG	= (1 << 23),	/* don't use NCQ for log read */
>
>   	 /* DMA mask for user DMA control: User visible values; DO NOT
>   	    renumber */
>

Re: [PATCH 4/4] libata: Fall back to unqueued READ LOG EXT if the DMA variant fails

[Martin K. Petersen]

>>>>> "David" == David Milburn <dmilburn@redhat.com> writes:

David> Have you seen both commands fail on any of the Samsung SSD 840
David> EVO drives?

David> # dmesg | grep Samsung [ 2.254443] ata10.00: ATA-9: Samsung SSD
David> 840 EVO 1TB, EXT0BB6Q, max UDMA/133

I have an 840 EVO in my test box. However, yours is running an older
firmware that predates queued TRIM support. So I guess that's
expected...

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 4/4] libata: Fall back to unqueued READ LOG EXT if the DMA variant fails

[David Milburn]

On Sun, Jun 14, 2015 at 07:45:49PM -0400, Martin K. Petersen wrote:
> >>>>> "David" == David Milburn <dmilburn@redhat.com> writes:
> 
> David> Have you seen both commands fail on any of the Samsung SSD 840
> David> EVO drives?
> 
> David> # dmesg | grep Samsung [ 2.254443] ata10.00: ATA-9: Samsung SSD
> David> 840 EVO 1TB, EXT0BB6Q, max UDMA/133
> 
> I have an 840 EVO in my test box. However, yours is running an older
> firmware that predates queued TRIM support. So I guess that's
> expected...

Ahh, ok, would it be ok to slightly change the error message to indicate
the drive doesn't support queued TRIM.

Thanks.

Signed-off-by: David Milburn <dmilburn@redhat.com>
---
 drivers/ata/libata-core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 8e533f7..601d4f2 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2128,6 +2128,7 @@ static int ata_dev_config_ncq(struct ata_device *dev,
 					     0, ap->sector_buf, 1);
 		if (err_mask) {
 			ata_dev_dbg(dev,
+				    "Drive doesn't support queued TRIM, "
 				    "failed to get NCQ Send/Recv Log Emask 0x%x\n",
 				    err_mask);
 		} else {
-- 
1.8.3.1

Re: [PATCH 4/4] libata: Fall back to unqueued READ LOG EXT if the DMA variant fails

[Martin K. Petersen]

>>>>> "David" == David Milburn <dmilburn@redhat.com> writes:

David> Ahh, ok, would it be ok to slightly change the error message to
David> indicate the drive doesn't support queued TRIM.

I don't have a problem with your more elaborate debug message.

It is a clear bug in the drive firmware, though. The ACS spec mandates
that the log page shall be supported if IDENTIFY DEVICE word 77, bit 6
is set.

-- 
Martin K. Petersen	Oracle Linux Engineering