[PATCH 0/6] libata: Fixup ATA NCQ NON-DATA commands

[PATCH 0/6] libata: Fixup ATA NCQ NON-DATA commands

[Hannes Reinecke]

Hi Tejun,

Damien pointed out that we're not handling ATA NCQ NON-Data commands
correctly. This patchset fixes this.

As usual, comments and reviews are welcome.

Hannes Reinecke (6):
  libata: use ata_is_ncq() accessors
  libsas: use ata_is_ncq() and ata_has_dma() accessors
  ata: add ata_is_fpdma() accessor
  ata: fixup ATA_PROT_NODATA
  libata-eh: decode all taskfile protocols
  ata: Handle ATA NCQ NO-DATA commands correctly

 drivers/ata/libahci.c         |  2 +-
 drivers/ata/libata-core.c     |  8 ++++----
 drivers/ata/libata-eh.c       |  8 ++++++--
 drivers/ata/libata-scsi.c     |  9 ++++++---
 drivers/ata/sata_dwc_460ex.c  |  2 ++
 drivers/ata/sata_fsl.c        |  4 ++--
 drivers/ata/sata_mv.c         |  5 ++---
 drivers/ata/sata_nv.c         | 11 +++++------
 drivers/scsi/libsas/sas_ata.c |  9 ++-------
 include/linux/ata.h           |  1 +
 include/linux/libata.h        |  8 ++++++++
 include/trace/events/libata.h |  1 +
 12 files changed, 40 insertions(+), 28 deletions(-)

-- 
1.8.5.6

[PATCH 1/6] libata: use ata_is_ncq() accessors

[Hannes Reinecke]

Use accessor functions instead of the raw value.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/ata/libahci.c     | 2 +-
 drivers/ata/libata-core.c | 4 ++--
 drivers/ata/libata-scsi.c | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 71b0719..3e69c20 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -1975,7 +1975,7 @@ unsigned int ahci_qc_issue(struct ata_queued_cmd *qc)
 	 */
 	pp->active_link = qc->dev->link;
 
-	if (qc->tf.protocol == ATA_PROT_NCQ)
+	if (ata_is_ncq(qc->tf.protocol))
 		writel(1 << qc->tag, port_mmio + PORT_SCR_ACT);
 
 	if (pp->fbs_enabled && pp->fbs_last_dev != qc->dev->link->pmp) {
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 6be7770..e798915 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -4842,7 +4842,7 @@ int ata_std_qc_defer(struct ata_queued_cmd *qc)
 {
 	struct ata_link *link = qc->dev->link;
 
-	if (qc->tf.protocol == ATA_PROT_NCQ) {
+	if (ata_is_ncq(qc->tf.protocol)) {
 		if (!ata_tag_valid(link->active_tag))
 			return 0;
 	} else {
@@ -5007,7 +5007,7 @@ void __ata_qc_complete(struct ata_queued_cmd *qc)
 		ata_sg_clean(qc);
 
 	/* command should be marked inactive atomically with qc completion */
-	if (qc->tf.protocol == ATA_PROT_NCQ) {
+	if (ata_is_ncq(qc->tf.protocol)) {
 		link->sactive &= ~(1 << qc->tag);
 		if (!link->sactive)
 			ap->nr_active_links--;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bfec66f..94bcd76 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3125,8 +3125,8 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 		tf->command = cdb[9];
 	}
 
-	/* For NCQ commands with FPDMA protocol, copy the tag value */
-	if (tf->protocol == ATA_PROT_NCQ)
+	/* For NCQ commands copy the tag value */
+	if (ata_is_ncq(tf->protocol))
 		tf->nsect = qc->tag << 3;
 
 	/* enforce correct master/slave bit */
-- 
1.8.5.6

[PATCH 2/6] libsas: use ata_is_ncq() and ata_has_dma() accessors

[Hannes Reinecke]

Use accessors instead of the raw protocol value.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/libsas/sas_ata.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 935c430..e1da52c 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -233,15 +233,10 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 	task->task_state_flags = SAS_TASK_STATE_PENDING;
 	qc->lldd_task = task;
 
-	switch (qc->tf.protocol) {
-	case ATA_PROT_NCQ:
+	if (ata_is_ncq(qc->tf.protocol))
 		task->ata_task.use_ncq = 1;
-		/* fall through */
-	case ATAPI_PROT_DMA:
-	case ATA_PROT_DMA:
+	if (ata_is_dma(qc->tf.protocol))
 		task->ata_task.dma_xfer = 1;
-		break;
-	}
 
 	if (qc->scsicmd)
 		ASSIGN_SAS_TASK(qc->scsicmd, task);
-- 
1.8.5.6

Re: [PATCH 2/6] libsas: use ata_is_ncq() and ata_has_dma() accessors

[Tejun Heo]

On Mon, Jun 20, 2016 at 01:39:12PM +0200, Hannes Reinecke wrote:
> Use accessors instead of the raw protocol value.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/libsas/sas_ata.c | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 935c430..e1da52c 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -233,15 +233,10 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
>  	task->task_state_flags = SAS_TASK_STATE_PENDING;
>  	qc->lldd_task = task;
>  
> -	switch (qc->tf.protocol) {
> -	case ATA_PROT_NCQ:
> +	if (ata_is_ncq(qc->tf.protocol))
>  		task->ata_task.use_ncq = 1;
> -		/* fall through */
> -	case ATAPI_PROT_DMA:
> -	case ATA_PROT_DMA:
> +	if (ata_is_dma(qc->tf.protocol))
>  		task->ata_task.dma_xfer = 1;
> -		break;
> -	}

As you're cleaning it up anyway, can you please make ata_is_*()
functions return bool and do

	task->ata_task.use_ncq = ata_is_ncq()

instead?

-- 
tejun

Re: [PATCH 2/6] libsas: use ata_is_ncq() and ata_has_dma() accessors

[Hannes Reinecke]

On 06/20/2016 05:40 PM, Tejun Heo wrote:
> On Mon, Jun 20, 2016 at 01:39:12PM +0200, Hannes Reinecke wrote:
>> Use accessors instead of the raw protocol value.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.com>
>> ---
>>  drivers/scsi/libsas/sas_ata.c | 9 ++-------
>>  1 file changed, 2 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
>> index 935c430..e1da52c 100644
>> --- a/drivers/scsi/libsas/sas_ata.c
>> +++ b/drivers/scsi/libsas/sas_ata.c
>> @@ -233,15 +233,10 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
>>  	task->task_state_flags = SAS_TASK_STATE_PENDING;
>>  	qc->lldd_task = task;
>>  
>> -	switch (qc->tf.protocol) {
>> -	case ATA_PROT_NCQ:
>> +	if (ata_is_ncq(qc->tf.protocol))
>>  		task->ata_task.use_ncq = 1;
>> -		/* fall through */
>> -	case ATAPI_PROT_DMA:
>> -	case ATA_PROT_DMA:
>> +	if (ata_is_dma(qc->tf.protocol))
>>  		task->ata_task.dma_xfer = 1;
>> -		break;
>> -	}
> 
> As you're cleaning it up anyway, can you please make ata_is_*()
> functions return bool and do
> 
> 	task->ata_task.use_ncq = ata_is_ncq()
> 
> instead?
> 
Sure, will do with the next round.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
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)

[PATCH 3/6] ata: add ata_is_fpdma() accessor

[Hannes Reinecke]

Add an accessor ata_is_fpdma() and convert drivers to use it.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/ata/sata_fsl.c |  4 ++--
 drivers/ata/sata_mv.c  |  5 ++---
 drivers/ata/sata_nv.c  | 11 +++++------
 include/linux/libata.h |  6 ++++++
 4 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/ata/sata_fsl.c b/drivers/ata/sata_fsl.c
index a723ae9..acfb865 100644
--- a/drivers/ata/sata_fsl.c
+++ b/drivers/ata/sata_fsl.c
@@ -534,7 +534,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc)
 	VPRINTK("Dumping cfis : 0x%x, 0x%x, 0x%x\n",
 		cd->cfis[0], cd->cfis[1], cd->cfis[2]);
 
-	if (qc->tf.protocol == ATA_PROT_NCQ) {
+	if (ata_is_fpdma(qc->tf.protocol)) {
 		VPRINTK("FPDMA xfer,Sctor cnt[0:7],[8:15] = %d,%d\n",
 			cd->cfis[3], cd->cfis[11]);
 	}
@@ -551,7 +551,7 @@ static void sata_fsl_qc_prep(struct ata_queued_cmd *qc)
 					    &ttl_dwords, cd_paddr,
 					    host_priv->data_snoop);
 
-	if (qc->tf.protocol == ATA_PROT_NCQ)
+	if (ata_is_fpdma(qc->tf.protocol))
 		desc_info |= FPDMA_QUEUED_CMD;
 
 	sata_fsl_setup_cmd_hdr_entry(pp, tag, desc_info, ttl_dwords,
diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
index bd74ee5..a81b1f8 100644
--- a/drivers/ata/sata_mv.c
+++ b/drivers/ata/sata_mv.c
@@ -1173,7 +1173,7 @@ static void mv_set_irq_coalescing(struct ata_host *host,
 static void mv_start_edma(struct ata_port *ap, void __iomem *port_mmio,
 			 struct mv_port_priv *pp, u8 protocol)
 {
-	int want_ncq = (protocol == ATA_PROT_NCQ);
+	int want_ncq = (ata_is_fpdma(protocol));
 
 	if (pp->pp_flags & MV_PP_FLAG_EDMA_EN) {
 		int using_ncq = ((pp->pp_flags & MV_PP_FLAG_NCQ_EN) != 0);
@@ -2156,8 +2156,7 @@ static void mv_qc_prep_iie(struct ata_queued_cmd *qc)
 	unsigned in_index;
 	u32 flags = 0;
 
-	if ((tf->protocol != ATA_PROT_DMA) &&
-	    (tf->protocol != ATA_PROT_NCQ))
+	if (!ata_is_fpdma(tf->protocol))
 		return;
 	if (tf->command == ATA_CMD_DSM)
 		return;  /* use bmdma for this */
diff --git a/drivers/ata/sata_nv.c b/drivers/ata/sata_nv.c
index 734f563..89e36aa 100644
--- a/drivers/ata/sata_nv.c
+++ b/drivers/ata/sata_nv.c
@@ -1407,7 +1407,7 @@ static void nv_adma_qc_prep(struct ata_queued_cmd *qc)
 	cpb->next_cpb_idx	= 0;
 
 	/* turn on NCQ flags for NCQ commands */
-	if (qc->tf.protocol == ATA_PROT_NCQ)
+	if (ata_is_fpdma(qc->tf.protocol))
 		ctl_flags |= NV_CPB_CTL_QUEUE | NV_CPB_CTL_FPDMA;
 
 	VPRINTK("qc->flags = 0x%lx\n", qc->flags);
@@ -1432,15 +1432,14 @@ static unsigned int nv_adma_qc_issue(struct ata_queued_cmd *qc)
 {
 	struct nv_adma_port_priv *pp = qc->ap->private_data;
 	void __iomem *mmio = pp->ctl_block;
-	int curr_ncq = (qc->tf.protocol == ATA_PROT_NCQ);
+	int curr_ncq = ata_is_fpdma(qc->tf.protocol);
 
 	VPRINTK("ENTER\n");
 
 	/* We can't handle result taskfile with NCQ commands, since
 	   retrieving the taskfile switches us out of ADMA mode and would abort
 	   existing commands. */
-	if (unlikely(qc->tf.protocol == ATA_PROT_NCQ &&
-		     (qc->flags & ATA_QCFLAG_RESULT_TF))) {
+	if (unlikely(curr_ncq && (qc->flags & ATA_QCFLAG_RESULT_TF))) {
 		ata_dev_err(qc->dev, "NCQ w/ RESULT_TF not allowed\n");
 		return AC_ERR_SYSTEM;
 	}
@@ -1991,7 +1990,7 @@ static int nv_swncq_port_start(struct ata_port *ap)
 
 static void nv_swncq_qc_prep(struct ata_queued_cmd *qc)
 {
-	if (qc->tf.protocol != ATA_PROT_NCQ) {
+	if (!ata_is_fpdma(qc->tf.protocol)) {
 		ata_bmdma_qc_prep(qc);
 		return;
 	}
@@ -2067,7 +2066,7 @@ static unsigned int nv_swncq_qc_issue(struct ata_queued_cmd *qc)
 	struct ata_port *ap = qc->ap;
 	struct nv_swncq_port_priv *pp = ap->private_data;
 
-	if (qc->tf.protocol != ATA_PROT_NCQ)
+	if (!ata_is_fpdma(qc->tf.protocol))
 		return ata_bmdma_qc_issue(qc);
 
 	DPRINTK("Enter\n");
diff --git a/include/linux/libata.h b/include/linux/libata.h
index d15c19e..264414c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -152,6 +152,7 @@ enum {
 	ATA_PROT_FLAG_DATA	= ATA_PROT_FLAG_PIO | ATA_PROT_FLAG_DMA,
 	ATA_PROT_FLAG_NCQ	= (1 << 2), /* is NCQ */
 	ATA_PROT_FLAG_ATAPI	= (1 << 3), /* is ATAPI */
+	ATA_PROT_FLAG_FPDMA	= ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA,
 
 	/* struct ata_device stuff */
 	ATA_DFLAG_LBA		= (1 << 0), /* device supports LBA */
@@ -1093,6 +1094,11 @@ static inline int ata_is_data(u8 prot)
 	return ata_prot_flags(prot) & ATA_PROT_FLAG_DATA;
 }
 
+static inline int ata_is_fpdma(u8 prot)
+{
+	return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA;
+}
+
 static inline int is_multi_taskfile(struct ata_taskfile *tf)
 {
 	return (tf->command == ATA_CMD_READ_MULTI) ||
-- 
1.8.5.6

Re: [PATCH 3/6] ata: add ata_is_fpdma() accessor

[Tejun Heo]

On Mon, Jun 20, 2016 at 01:39:13PM +0200, Hannes Reinecke wrote:
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index d15c19e..264414c 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -152,6 +152,7 @@ enum {
>  	ATA_PROT_FLAG_DATA	= ATA_PROT_FLAG_PIO | ATA_PROT_FLAG_DMA,
>  	ATA_PROT_FLAG_NCQ	= (1 << 2), /* is NCQ */
>  	ATA_PROT_FLAG_ATAPI	= (1 << 3), /* is ATAPI */
> +	ATA_PROT_FLAG_FPDMA	= ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA,
>  
>  	/* struct ata_device stuff */
>  	ATA_DFLAG_LBA		= (1 << 0), /* device supports LBA */
> @@ -1093,6 +1094,11 @@ static inline int ata_is_data(u8 prot)
>  	return ata_prot_flags(prot) & ATA_PROT_FLAG_DATA;
>  }
>  
> +static inline int ata_is_fpdma(u8 prot)
> +{
> +	return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA;
> +}
> +

This is NCQ or DMA which isn't the same thing as some of the tests
it's replacing.  Is this intentional?

Thanks.

-- 
tejun

Re: [PATCH 3/6] ata: add ata_is_fpdma() accessor

[Hannes Reinecke]

On 06/20/2016 05:42 PM, Tejun Heo wrote:
> On Mon, Jun 20, 2016 at 01:39:13PM +0200, Hannes Reinecke wrote:
>> diff --git a/include/linux/libata.h b/include/linux/libata.h
>> index d15c19e..264414c 100644
>> --- a/include/linux/libata.h
>> +++ b/include/linux/libata.h
>> @@ -152,6 +152,7 @@ enum {
>>  	ATA_PROT_FLAG_DATA	= ATA_PROT_FLAG_PIO | ATA_PROT_FLAG_DMA,
>>  	ATA_PROT_FLAG_NCQ	= (1 << 2), /* is NCQ */
>>  	ATA_PROT_FLAG_ATAPI	= (1 << 3), /* is ATAPI */
>> +	ATA_PROT_FLAG_FPDMA	= ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA,
>>  
>>  	/* struct ata_device stuff */
>>  	ATA_DFLAG_LBA		= (1 << 0), /* device supports LBA */
>> @@ -1093,6 +1094,11 @@ static inline int ata_is_data(u8 prot)
>>  	return ata_prot_flags(prot) & ATA_PROT_FLAG_DATA;
>>  }
>>  
>> +static inline int ata_is_fpdma(u8 prot)
>> +{
>> +	return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA;
>> +}
>> +
> 
> This is NCQ or DMA which isn't the same thing as some of the tests
> it's replacing.  Is this intentional?
> 
Yes. Most of the SATA drivers (with the exception of ahci) don't know
about NCQ NON DATA commands, so they use the test for 'NCQ' as a
shorthand for 'NCQ command with DMA data'.
With the introduction of the ATA_PROT_NODATA protocol variable these two
are no longer equivalent, as you can have NCQ commands without DMA.
As I haven't vetted any of those adapters for NCQ NON DATA commands, and
these driver internally also assume that any NCQ command will have
DMA-able data, I thought it prudent to introduce an ata_is_fpdma() flag,
which retains the original meaning of ata_is_ncq().

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
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/6] ata: add ata_is_fpdma() accessor

[Tejun Heo]

Hello,

On Tue, Jun 21, 2016 at 07:49:26AM +0200, Hannes Reinecke wrote:
> Yes. Most of the SATA drivers (with the exception of ahci) don't know
> about NCQ NON DATA commands, so they use the test for 'NCQ' as a
> shorthand for 'NCQ command with DMA data'.
> With the introduction of the ATA_PROT_NODATA protocol variable these two
> are no longer equivalent, as you can have NCQ commands without DMA.
> As I haven't vetted any of those adapters for NCQ NON DATA commands, and
> these driver internally also assume that any NCQ command will have
> DMA-able data, I thought it prudent to introduce an ata_is_fpdma() flag,
> which retains the original meaning of ata_is_ncq().

In that case, the patch description needs to be way beefier.  As it
is, it looks like a trivial cleanup patch which it isn't.

Thanks.

-- 
tejun

[PATCH 4/6] ata: fixup ATA_PROT_NODATA

[Hannes Reinecke]

The taskfile protocol is a numeric value, and can not be ORed.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/libata-core.c | 4 ++--
 drivers/ata/libata-eh.c   | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index e798915..d200102 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -1238,7 +1238,7 @@ static int ata_read_native_max_address(struct ata_device *dev, u64 *max_sectors)
 	} else
 		tf.command = ATA_CMD_READ_NATIVE_MAX;
 
-	tf.protocol |= ATA_PROT_NODATA;
+	tf.protocol = ATA_PROT_NODATA;
 	tf.device |= ATA_LBA;
 
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
@@ -1297,7 +1297,7 @@ static int ata_set_max_sectors(struct ata_device *dev, u64 new_sectors)
 		tf.device |= (new_sectors >> 24) & 0xf;
 	}
 
-	tf.protocol |= ATA_PROT_NODATA;
+	tf.protocol = ATA_PROT_NODATA;
 	tf.device |= ATA_LBA;
 
 	tf.lbal = (new_sectors >> 0) & 0xff;
diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 61dc7a9..7832e55 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3177,7 +3177,7 @@ static void ata_eh_park_issue_cmd(struct ata_device *dev, int park)
 	}
 
 	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
-	tf.protocol |= ATA_PROT_NODATA;
+	tf.protocol = ATA_PROT_NODATA;
 	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
 	if (park && (err_mask || tf.lbal != 0xc4)) {
 		ata_dev_err(dev, "head unload failed!\n");
-- 
1.8.5.6

Re: [PATCH 4/6] ata: fixup ATA_PROT_NODATA

[Tejun Heo]

On Mon, Jun 20, 2016 at 01:39:14PM +0200, Hannes Reinecke wrote:
> The taskfile protocol is a numeric value, and can not be ORed.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>  drivers/ata/libata-core.c | 4 ++--
>  drivers/ata/libata-eh.c   | 2 +-
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e798915..d200102 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -1238,7 +1238,7 @@ static int ata_read_native_max_address(struct ata_device *dev, u64 *max_sectors)
>  	} else
>  		tf.command = ATA_CMD_READ_NATIVE_MAX;
>  
> -	tf.protocol |= ATA_PROT_NODATA;
> +	tf.protocol = ATA_PROT_NODATA;
>  	tf.device |= ATA_LBA;

Did the above lead to any actual brekage or was tf.protocol guaranteed
to be zero always?  I wish the patch description described what the
stake of the patch is.

Thanks.

-- 
tejun

Re: [PATCH 4/6] ata: fixup ATA_PROT_NODATA

[Hannes Reinecke]

On 06/20/2016 05:44 PM, Tejun Heo wrote:
> On Mon, Jun 20, 2016 at 01:39:14PM +0200, Hannes Reinecke wrote:
>> The taskfile protocol is a numeric value, and can not be ORed.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>  drivers/ata/libata-core.c | 4 ++--
>>  drivers/ata/libata-eh.c   | 2 +-
>>  2 files changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>> index e798915..d200102 100644
>> --- a/drivers/ata/libata-core.c
>> +++ b/drivers/ata/libata-core.c
>> @@ -1238,7 +1238,7 @@ static int ata_read_native_max_address(struct ata_device *dev, u64 *max_sectors)
>>  	} else
>>  		tf.command = ATA_CMD_READ_NATIVE_MAX;
>>  
>> -	tf.protocol |= ATA_PROT_NODATA;
>> +	tf.protocol = ATA_PROT_NODATA;
>>  	tf.device |= ATA_LBA;
> 
> Did the above lead to any actual brekage or was tf.protocol guaranteed
> to be zero always?  I wish the patch description described what the
> stake of the patch is.
> 
This is actually a cleanup; if tf.protocl is '0' we won't see any issues
here.
But if (for some obsure reason) tf.protocol is _not_ zero we'll run into
very obscure problems.
Plus the OR here might give others the wrong impression, and might lead
to a confusion between ATA_PROT_XXX (which is a scalar value and cannot
be ORed) and ATA_PROT_FLAG_XXX (which is a bit value and can be ORed).

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
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)

[PATCH 5/6] libata-eh: decode all taskfile protocols

[Hannes Reinecke]

Some taskfile protocol values where missing in ata_eh_link_report().

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/ata/libata-eh.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 7832e55..5688b86 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2607,9 +2607,12 @@ static void ata_eh_link_report(struct ata_link *link)
 				[DMA_FROM_DEVICE]	= "in",
 			};
 			static const char *prot_str[] = {
+				[ATA_PROT_UNKNOWN]	= "unknown",
+				[ATA_PROT_NODATA]	= "nodata",
 				[ATA_PROT_PIO]		= "pio",
 				[ATA_PROT_DMA]		= "dma",
 				[ATA_PROT_NCQ]		= "ncq",
+				[ATAPI_PROT_NODATA]	= "nodata",
 				[ATAPI_PROT_PIO]	= "pio",
 				[ATAPI_PROT_DMA]	= "dma",
 			};
-- 
1.8.5.6

[PATCH 6/6] ata: Handle ATA NCQ NO-DATA commands correctly

[Hannes Reinecke]

Add a new taskfile protocol ATA_PROT_NCQ_NODATA to handle
ATA NCQ NO-DATA commands correctly.
And fixup ata_scsi_zbc_out_xlat() to use it.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/ata/libata-eh.c       | 3 ++-
 drivers/ata/libata-scsi.c     | 5 ++++-
 drivers/ata/sata_dwc_460ex.c  | 2 ++
 include/linux/ata.h           | 1 +
 include/linux/libata.h        | 2 ++
 include/trace/events/libata.h | 1 +
 6 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 5688b86..d551378 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -2611,7 +2611,8 @@ static void ata_eh_link_report(struct ata_link *link)
 				[ATA_PROT_NODATA]	= "nodata",
 				[ATA_PROT_PIO]		= "pio",
 				[ATA_PROT_DMA]		= "dma",
-				[ATA_PROT_NCQ]		= "ncq",
+				[ATA_PROT_NCQ]		= "ncq dma",
+				[ATA_PROT_NCQ_NODATA]	= "ncq nodata",
 				[ATAPI_PROT_NODATA]	= "nodata",
 				[ATAPI_PROT_PIO]	= "pio",
 				[ATAPI_PROT_DMA]	= "dma",
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 94bcd76..d80840c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3077,6 +3077,9 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
 		goto invalid_fld;
 	}
 
+	if (ata_is_ncq(tf->protocol) && (cdb[2] & 0x3) == 0)
+		tf->protocol = ATA_PROT_NCQ_NODATA;
+
 	/* enable LBA */
 	tf->flags |= ATA_TFLAG_LBA;
 
@@ -3537,7 +3540,7 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
 
 	if (ata_ncq_enabled(qc->dev) &&
 	    ata_fpdma_zac_mgmt_out_supported(qc->dev)) {
-		tf->protocol = ATA_PROT_NCQ;
+		tf->protocol = ATA_PROT_NCQ_NODATA;
 		tf->command = ATA_CMD_NCQ_NON_DATA;
 		tf->hob_nsect = ATA_SUBCMD_NCQ_NON_DATA_ZAC_MGMT_OUT;
 		tf->nsect = qc->tag << 3;
diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 00c2af1..fa1530a 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -290,6 +290,8 @@ static const char *get_prot_descript(u8 protocol)
 		return "ATA DMA";
 	case ATA_PROT_NCQ:
 		return "ATA NCQ";
+	case ATA_PROT_NCQ_NODATA:
+		return "ATA NCQ no data";
 	case ATAPI_PROT_NODATA:
 		return "ATAPI no data";
 	case ATAPI_PROT_PIO:
diff --git a/include/linux/ata.h b/include/linux/ata.h
index 99346be..5581c20 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -526,6 +526,7 @@ enum ata_tf_protocols {
 	ATA_PROT_PIO,		/* PIO data xfer */
 	ATA_PROT_DMA,		/* DMA */
 	ATA_PROT_NCQ,		/* NCQ */
+	ATA_PROT_NCQ_NODATA,	/* NCQ no data */
 	ATAPI_PROT_NODATA,	/* packet command, no data */
 	ATAPI_PROT_PIO,		/* packet command, PIO data xfer*/
 	ATAPI_PROT_DMA,		/* packet command with special DMA sauce */
diff --git a/include/linux/libata.h b/include/linux/libata.h
index 264414c..a6d2762 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1054,6 +1054,8 @@ static inline unsigned int ata_prot_flags(u8 prot)
 		return ATA_PROT_FLAG_DMA;
 	case ATA_PROT_NCQ:
 		return ATA_PROT_FLAG_DMA | ATA_PROT_FLAG_NCQ;
+	case ATA_PROT_NCQ_NODATA:
+		return ATA_PROT_FLAG_NCQ;
 	case ATAPI_PROT_NODATA:
 		return ATA_PROT_FLAG_ATAPI;
 	case ATAPI_PROT_PIO:
diff --git a/include/trace/events/libata.h b/include/trace/events/libata.h
index 75fff86..2fbbf99 100644
--- a/include/trace/events/libata.h
+++ b/include/trace/events/libata.h
@@ -126,6 +126,7 @@
 		ata_protocol_name(ATA_PROT_PIO),	\
 		ata_protocol_name(ATA_PROT_DMA),	\
 		ata_protocol_name(ATA_PROT_NCQ),	\
+		ata_protocol_name(ATA_PROT_NCQ_NODATA),	\
 		ata_protocol_name(ATAPI_PROT_NODATA),	\
 		ata_protocol_name(ATAPI_PROT_PIO),	\
 		ata_protocol_name(ATAPI_PROT_DMA))
-- 
1.8.5.6

Re: [PATCH 6/6] ata: Handle ATA NCQ NO-DATA commands correctly

[Damien Le Moal]

Hannes,

On 6/20/16 20:41, Hannes Reinecke wrote:
> Add a new taskfile protocol ATA_PROT_NCQ_NODATA to handle
> ATA NCQ NO-DATA commands correctly.
> And fixup ata_scsi_zbc_out_xlat() to use it.
>
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/ata/libata-eh.c       | 3 ++-
>  drivers/ata/libata-scsi.c     | 5 ++++-
>  drivers/ata/sata_dwc_460ex.c  | 2 ++
>  include/linux/ata.h           | 1 +
>  include/linux/libata.h        | 2 ++
>  include/trace/events/libata.h | 1 +
>  6 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 5688b86..d551378 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -2611,7 +2611,8 @@ static void ata_eh_link_report(struct ata_link *link)
>  				[ATA_PROT_NODATA]	= "nodata",
>  				[ATA_PROT_PIO]		= "pio",
>  				[ATA_PROT_DMA]		= "dma",
> -				[ATA_PROT_NCQ]		= "ncq",
> +				[ATA_PROT_NCQ]		= "ncq dma",
> +				[ATA_PROT_NCQ_NODATA]	= "ncq nodata",
>  				[ATAPI_PROT_NODATA]	= "nodata",
>  				[ATAPI_PROT_PIO]	= "pio",
>  				[ATAPI_PROT_DMA]	= "dma",
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 94bcd76..d80840c 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -3077,6 +3077,9 @@ static unsigned int ata_scsi_pass_thru(struct ata_queued_cmd *qc)
>  		goto invalid_fld;
>  	}
>
> +	if (ata_is_ncq(tf->protocol) && (cdb[2] & 0x3) == 0)
> +		tf->protocol = ATA_PROT_NCQ_NODATA;
> +
>  	/* enable LBA */
>  	tf->flags |= ATA_TFLAG_LBA;
>
> @@ -3537,7 +3540,7 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
>
>  	if (ata_ncq_enabled(qc->dev) &&
>  	    ata_fpdma_zac_mgmt_out_supported(qc->dev)) {
> -		tf->protocol = ATA_PROT_NCQ;
> +		tf->protocol = ATA_PROT_NCQ_NODATA;
>  		tf->command = ATA_CMD_NCQ_NON_DATA;
>  		tf->hob_nsect = ATA_SUBCMD_NCQ_NON_DATA_ZAC_MGMT_OUT;
>  		tf->nsect = qc->tag << 3;

While you are at it, ATA_SUBCMD_NCQ_NON_DATA_ZAC_MGMT_OUT should go into 
tf->feature, not tf->hob_nsect. And you can also remove the "&0x1" mask 
of reset_all in the non-NCQ version of the command below that code (see 
the patch I sent a while ago togetner with the report of the NCQ NON 
DATA command problem.

Thanks !

Best regards.

-- 
Damien Le Moal, Ph.D.
Sr. Manager, System Software Group, HGST Research,
HGST, a Western Digital company
Damien.LeMoal@hgst.com
(+81) 0466-98-3593 (ext. 513593)
1 kirihara-cho, Fujisawa,
Kanagawa, 252-0888 Japan
www.hgst.com
Western Digital Corporation (and its subsidiaries) E-mail Confidentiality Notice & Disclaimer:

This e-mail and any files transmitted with it may contain confidential or legally privileged information of WDC and/or its affiliates, and are intended solely for the use of the individual or entity to which they are addressed. If you are not the intended recipient, any disclosure, copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited. If you have received this e-mail in error, please notify the sender immediately and delete the e-mail in its entirety from your system.