ZAC fixes

ZAC fixes

[Christoph Hellwig]

Hi Tejun,

this series fixes ZAC support in libata.  The first half of it is
originally from Hannes, but I fixed up your small review comments
from the last run so that we can get it into 4.8.  That includes the
new patch 1 from me.  The last three patches are for Damien
and fix the SCSI to ATA for the REPORT ZONES command, they got some
minor cleanups from me as well.

[PATCH 01/10] libata: return boolean values from ata_is_*

[Christoph Hellwig]

This way we don't have to worry about the exact bit postition of the
test to leak out and any crazy propagation effects in the callers.

Suggested-by: Tejun Heo <tj@kernel.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 include/linux/libata.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index d15c19e..4e5a09c 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1063,32 +1063,32 @@ static inline unsigned int ata_prot_flags(u8 prot)
 	return 0;
 }
 
-static inline int ata_is_atapi(u8 prot)
+static inline bool ata_is_atapi(u8 prot)
 {
 	return ata_prot_flags(prot) & ATA_PROT_FLAG_ATAPI;
 }
 
-static inline int ata_is_nodata(u8 prot)
+static inline bool ata_is_nodata(u8 prot)
 {
 	return !(ata_prot_flags(prot) & ATA_PROT_FLAG_DATA);
 }
 
-static inline int ata_is_pio(u8 prot)
+static inline bool ata_is_pio(u8 prot)
 {
 	return ata_prot_flags(prot) & ATA_PROT_FLAG_PIO;
 }
 
-static inline int ata_is_dma(u8 prot)
+static inline bool ata_is_dma(u8 prot)
 {
 	return ata_prot_flags(prot) & ATA_PROT_FLAG_DMA;
 }
 
-static inline int ata_is_ncq(u8 prot)
+static inline bool ata_is_ncq(u8 prot)
 {
 	return ata_prot_flags(prot) & ATA_PROT_FLAG_NCQ;
 }
 
-static inline int ata_is_data(u8 prot)
+static inline bool ata_is_data(u8 prot)
 {
 	return ata_prot_flags(prot) & ATA_PROT_FLAG_DATA;
 }
@@ -1407,7 +1407,7 @@ static inline bool sata_pmp_attached(struct ata_port *ap)
 	return ap->nr_pmp_links != 0;
 }
 
-static inline int ata_is_host_link(const struct ata_link *link)
+static inline bool ata_is_host_link(const struct ata_link *link)
 {
 	return link == &link->ap->link || link == link->ap->slave_link;
 }
@@ -1422,7 +1422,7 @@ static inline bool sata_pmp_attached(struct ata_port *ap)
 	return false;
 }
 
-static inline int ata_is_host_link(const struct ata_link *link)
+static inline bool ata_is_host_link(const struct ata_link *link)
 {
 	return 1;
 }
-- 
2.1.4

[PATCH 02/10] libata: use ata_is_ncq() accessors

[Christoph Hellwig]

From: Hannes Reinecke <hare@suse.de>

Use accessor functions instead of the raw value.

Signed-off-by: Hannes Reinecke <hare@suse.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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 077daf0..f5eb07e 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 3dca0d1..0447a39 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3130,8 +3130,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 */
-- 
2.1.4

[PATCH 03/10] libsas: use ata_is_ncq() and ata_has_dma() accessors

[Christoph Hellwig]

From: Hannes Reinecke <hare@suse.de>

Use accessors instead of the raw protocol value.

Signed-off-by: Hannes Reinecke <hare@suse.com>
[hch: trivial cleanup of the ata_task assignments]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/scsi/libsas/sas_ata.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 935c430..497bc15 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -233,15 +233,8 @@ 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:
-		task->ata_task.use_ncq = 1;
-		/* fall through */
-	case ATAPI_PROT_DMA:
-	case ATA_PROT_DMA:
-		task->ata_task.dma_xfer = 1;
-		break;
-	}
+	task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
+	task->ata_task.dma_xfer = ata_is_dma(qc->tf.protocol);
 
 	if (qc->scsicmd)
 		ASSIGN_SAS_TASK(qc->scsicmd, task);
-- 
2.1.4

Re: [PATCH 03/10] libsas: use ata_is_ncq() and ata_has_dma() accessors

[Tejun Heo]

On Thu, Jul 14, 2016 at 09:05:44AM +0900, Christoph Hellwig wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Use accessors instead of the raw protocol value.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> [hch: trivial cleanup of the ata_task assignments]
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied 1-3 to libata/for-4.8.

Thanks.

-- 
tejun

[PATCH 04/10] ata: add ata_is_fpdma() accessor

[Christoph Hellwig]

From: Hannes Reinecke <hare@suse.de>

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().

Signed-off-by: Hannes Reinecke <hare@suse.com>
[hch: changed ata_is_fpdma to return bool, updated patch description]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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 4e5a09c..77833f6 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 bool ata_is_data(u8 prot)
 	return ata_prot_flags(prot) & ATA_PROT_FLAG_DATA;
 }
 
+static inline bool 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) ||
-- 
2.1.4

Re: [PATCH 04/10] ata: add ata_is_fpdma() accessor

[Tejun Heo]

Hello,

I'm still a bit confused about this patch.

On Thu, Jul 14, 2016 at 09:05:45AM +0900, Christoph Hellwig wrote:
> 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)) {

ata_is_fpdma() tests NCQ or DMA.  Is this right?

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

Let's get rid of the parentheses while at it.

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

This is actually testing !DMA && !NCQ and an equivalent conversion but
the rest aren't.

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

And I don't know why testing NCQ or DMA would be safe for places like
this.

> +	ATA_PROT_FLAG_FPDMA	= ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA,
...
> +static inline bool ata_is_fpdma(u8 prot)
> +{
> +	return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA;
> +}

???

Thanks.

-- 
tejun

Re: [PATCH 04/10] ata: add ata_is_fpdma() accessor

[Hannes Reinecke]

On 07/14/2016 04:37 PM, Tejun Heo wrote:
> Hello,
> 
> I'm still a bit confused about this patch.
> 
> On Thu, Jul 14, 2016 at 09:05:45AM +0900, Christoph Hellwig wrote:
>> 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)) {
> 
> ata_is_fpdma() tests NCQ or DMA.  Is this right?
> 
Yes. The 'sata_fsl' driver has no means (or at least none which I could
detect) allowing it to send NCQ NON-DATA commands.
So for this driver the check for ncq is actually a check for fpdma.

>> --- 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));
> 
> Let's get rid of the parentheses while at it.
> 
Okay.

>>  
>>  	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))
> 
> This is actually testing !DMA && !NCQ and an equivalent conversion but
> the rest aren't.
> 
Yes.

>> 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;
> 
> And I don't know why testing NCQ or DMA would be safe for places like
> this.
> 
Again, this driver cannot handle NCQ NON-DATA command, so it always has
to assume that NCQ _actually_ means FPDMA.

>> +	ATA_PROT_FLAG_FPDMA	= ATA_PROT_FLAG_NCQ | ATA_PROT_FLAG_DMA,
> ...
>> +static inline bool ata_is_fpdma(u8 prot)
>> +{
>> +	return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA;
>> +}
> 
> ???
> 
Yeah, strictly speaking there is no need for ATA_PROT_FLAG_FPDMA.
I'll be removing it if you insist.

Cheers,

Hannes

Re: [PATCH 04/10] ata: add ata_is_fpdma() accessor

[Tejun Heo]

Hello, Hannes.

On Thu, Jul 14, 2016 at 05:15:51PM +0200, Hannes Reinecke wrote:
> >> +static inline bool ata_is_fpdma(u8 prot)
> >> +{
> >> +	return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA;
> >> +}
> > 
> > ???
> > 
> Yeah, strictly speaking there is no need for ATA_PROT_FLAG_FPDMA.
> I'll be removing it if you insist.

No, it's just really confusing to have a function named ata_is_fpdma()
and then have it test whether the protocol is NCQ or DMA.  If you
wanna do that, name it ata_is_dma_or_fpdma().  ata_is_fpdma() should
test (prot & ATA_PROT_FLAG_FPDMA) == ATA_PROT_FLAG_FPDMA.

Thanks.

-- 
tejun

Re: [PATCH 04/10] ata: add ata_is_fpdma() accessor

[Hannes Reinecke]

On 07/14/2016 05:19 PM, Tejun Heo wrote:
> Hello, Hannes.
> 
> On Thu, Jul 14, 2016 at 05:15:51PM +0200, Hannes Reinecke wrote:
>>>> +static inline bool ata_is_fpdma(u8 prot)
>>>> +{
>>>> +	return ata_prot_flags(prot) & ATA_PROT_FLAG_FPDMA;
>>>> +}
>>>
>>> ???
>>>
>> Yeah, strictly speaking there is no need for ATA_PROT_FLAG_FPDMA.
>> I'll be removing it if you insist.
> 
> No, it's just really confusing to have a function named ata_is_fpdma()
> and then have it test whether the protocol is NCQ or DMA.  If you
> wanna do that, name it ata_is_dma_or_fpdma().  ata_is_fpdma() should
> test (prot & ATA_PROT_FLAG_FPDMA) == ATA_PROT_FLAG_FPDMA.
> 
Ah. Now I see your point.

That was unintentional, and your suggestion is in fact correct.
I'll be fixing it up.

Cheers,

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

Re: [PATCH 04/10] ata: add ata_is_fpdma() accessor

[Christoph Hellwig]

Hi Rejun, hi Hannes,

Damien and I spent some time going over this patch today and we think
it's best to simply drop it.  

Re: [PATCH 04/10] ata: add ata_is_fpdma() accessor

[Hannes Reinecke]

On 07/15/2016 08:13 AM, Christoph Hellwig wrote:
> Hi Rejun, hi Hannes,
> 
> Damien and I spent some time going over this patch today and we think
> it's best to simply drop it.  
> 
Sure, fine with me.

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 04/10] ata: add ata_is_fpdma() accessor

[Christoph Hellwig]

On Fri, Jul 15, 2016 at 09:07:30AM +0200, Hannes Reinecke wrote:
> > Damien and I spent some time going over this patch today and we think
> > it's best to simply drop it.  
> > 
> Sure, fine with me.

Great.  I've verified that the remaining patches will apply and work
fine when simply dropping it.

[PATCH 05/10] ata: fixup ATA_PROT_NODATA

[Christoph Hellwig]

From: Hannes Reinecke <hare@suse.de>

The taskfile protocol is a numeric value, and can not be ORed.  Currently
this is harmless as the protocol is always zeroed before, but if it ever
has a non-zero value the ORing would create incorrect results.

Signed-off-by: Hannes Reinecke <hare@suse.de>
[hch: updated patch description]
Signed-off-by: Christoph Hellwig <hch@lst.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 f5eb07e..522848a 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");
-- 
2.1.4

[PATCH 06/10] libata-eh: decode all taskfile protocols

[Christoph Hellwig]

From: Hannes Reinecke <hare@suse.de>

Some taskfile protocol values where missing in ata_eh_link_report().

Signed-off-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Christoph Hellwig <hch@lst.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",
 			};
-- 
2.1.4

Re: [PATCH 06/10] libata-eh: decode all taskfile protocols

[Sergei Shtylyov]

Hello.

On 7/14/2016 3:05 AM, Christoph Hellwig wrote:

> From: Hannes Reinecke <hare@suse.de>
>
> Some taskfile protocol values where missing in ata_eh_link_report().

    Were -- perhaps Tejun could fix...

>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
[...]

MBR, Sergei

Re: [PATCH 06/10] libata-eh: decode all taskfile protocols

[Tejun Heo]

On Thu, Jul 14, 2016 at 09:05:47AM +0900, Christoph Hellwig wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> Some taskfile protocol values where missing in ata_eh_link_report().
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied 5-6 to libata/for-4.8.

For the rest, I'll wait for the response on 4.

Thanks.

-- 
tejun

Re: [PATCH 06/10] libata-eh: decode all taskfile protocols

[Christoph Hellwig]

On Thu, Jul 14, 2016 at 10:48:09AM -0400, Tejun Heo wrote:
> On Thu, Jul 14, 2016 at 09:05:47AM +0900, Christoph Hellwig wrote:
> > From: Hannes Reinecke <hare@suse.de>
> > 
> > Some taskfile protocol values where missing in ata_eh_link_report().
> > 
> > Signed-off-by: Hannes Reinecke <hare@suse.de>
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> 
> Applied 5-6 to libata/for-4.8.
> 
> For the rest, I'll wait for the response on 4.

FYI, 8-10 are totally independent of 4.  While 5 kinda depends on 4
to work correctly, but I also don't actually see it in your tree.

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

[Christoph Hellwig]

From: Hannes Reinecke <hare@suse.de>

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>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 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 0447a39..901b46a 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3082,6 +3082,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;
 
@@ -3548,7 +3551,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 d20b1ee..35857d1 100644
--- a/include/linux/ata.h
+++ b/include/linux/ata.h
@@ -530,6 +530,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 77833f6..ce05d8d 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))
-- 
2.1.4

[PATCH 08/10] libata-scsi: Fix translation of REPORT ZONES command

[Christoph Hellwig]

From: Damien Le Moal <damien.lemoal@hgst.com>

Include reporting options when translating REPORT ZONES commmand to
ATA NCQ, and make sure we only look at the actually specified bits
in the CDB for the options.

Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com>
[hch: update patch description]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 901b46a..45d8ae63 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3468,7 +3468,7 @@ static unsigned int ata_scsi_zbc_in_xlat(struct ata_queued_cmd *qc)
 		goto invalid_param_len;
 	}
 	sect = n_block / 512;
-	options = cdb[14];
+	options = cdb[14] & 0xbf;
 
 	if (ata_ncq_enabled(qc->dev) &&
 	    ata_fpdma_zac_mgmt_in_supported(qc->dev)) {
@@ -3478,7 +3478,7 @@ static unsigned int ata_scsi_zbc_in_xlat(struct ata_queued_cmd *qc)
 		tf->nsect = qc->tag << 3;
 		tf->feature = sect & 0xff;
 		tf->hob_feature = (sect >> 8) & 0xff;
-		tf->auxiliary = ATA_SUBCMD_ZAC_MGMT_IN_REPORT_ZONES;
+		tf->auxiliary = ATA_SUBCMD_ZAC_MGMT_IN_REPORT_ZONES | (options << 8);
 	} else {
 		tf->command = ATA_CMD_ZAC_MGMT_IN;
 		tf->feature = ATA_SUBCMD_ZAC_MGMT_IN_REPORT_ZONES;
-- 
2.1.4

[PATCH 09/10] libata-scsi: Fix ZBC management out command translation

[Christoph Hellwig]

From: Damien Le Moal <damien.lemoal@hgst.com>

The subcommand for NCQ NON-DATA must be specified in the feature
(low byte), not the high-order count byte.  Also make sure to properly
cast the all bit to a u16 before shiting it by 8 to avoid undefined
behavior.

Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com>
[hch: split the original patch into two, updated changelog]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 45d8ae63..d978153 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3553,9 +3553,9 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
 	    ata_fpdma_zac_mgmt_out_supported(qc->dev)) {
 		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->feature = ATA_SUBCMD_NCQ_NON_DATA_ZAC_MGMT_OUT;
 		tf->nsect = qc->tag << 3;
-		tf->auxiliary = sa | (reset_all & 0x1) << 8;
+		tf->auxiliary = sa | ((u16)(reset_all & 0x1) << 8);
 	} else {
 		tf->protocol = ATA_PROT_NODATA;
 		tf->command = ATA_CMD_ZAC_MGMT_OUT;
-- 
2.1.4

Re: [PATCH 09/10] libata-scsi: Fix ZBC management out command translation

[Sergei Shtylyov]

On 7/14/2016 3:05 AM, Christoph Hellwig wrote:

> From: Damien Le Moal <damien.lemoal@hgst.com>
>
> The subcommand for NCQ NON-DATA must be specified in the feature
> (low byte), not the high-order count byte.  Also make sure to properly
> cast the all bit to a u16 before shiting it by 8 to avoid undefined

    Shifting. :-)

> behavior.
>
> Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com>
> [hch: split the original patch into two, updated changelog]
> Signed-off-by: Christoph Hellwig <hch@lst.de>
[...]

MBR, Sergei

[PATCH 10/10] libata-scsi: minor cleanup for ata_scsi_zbc_out_xlat

[Christoph Hellwig]

From: Damien Le Moal <damien.lemoal@hgst.com>

The reset_all variable name is misleading as this bit is also applicable to
open, close, and finish actions. So rename that variable to "all" and remove
the unnecessary mask operation that's already done earlier.

Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com>
[hch: split from the previous patch]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 drivers/ata/libata-scsi.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index d978153..7d0e60b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -3520,7 +3520,7 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
 	struct scsi_cmnd *scmd = qc->scsicmd;
 	struct ata_device *dev = qc->dev;
 	const u8 *cdb = scmd->cmnd;
-	u8 reset_all, sa;
+	u8 all, sa;
 	u64 block;
 	u32 n_block;
 	u16 fp = (u16)-1;
@@ -3547,7 +3547,7 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
 	if (block > dev->n_sectors)
 		goto out_of_range;
 
-	reset_all = cdb[14] & 0x1;
+	all = cdb[14] & 0x1;
 
 	if (ata_ncq_enabled(qc->dev) &&
 	    ata_fpdma_zac_mgmt_out_supported(qc->dev)) {
@@ -3555,12 +3555,12 @@ static unsigned int ata_scsi_zbc_out_xlat(struct ata_queued_cmd *qc)
 		tf->command = ATA_CMD_NCQ_NON_DATA;
 		tf->feature = ATA_SUBCMD_NCQ_NON_DATA_ZAC_MGMT_OUT;
 		tf->nsect = qc->tag << 3;
-		tf->auxiliary = sa | ((u16)(reset_all & 0x1) << 8);
+		tf->auxiliary = sa | ((u16)all << 8);
 	} else {
 		tf->protocol = ATA_PROT_NODATA;
 		tf->command = ATA_CMD_ZAC_MGMT_OUT;
 		tf->feature = sa;
-		tf->hob_feature = reset_all & 0x1;
+		tf->hob_feature = all;
 	}
 	tf->lbah = (block >> 16) & 0xff;
 	tf->lbam = (block >> 8) & 0xff;
-- 
2.1.4

Re: [PATCH 10/10] libata-scsi: minor cleanup for ata_scsi_zbc_out_xlat

[Tejun Heo]

On Thu, Jul 14, 2016 at 09:05:51AM +0900, Christoph Hellwig wrote:
> From: Damien Le Moal <damien.lemoal@hgst.com>
> 
> The reset_all variable name is misleading as this bit is also applicable to
> open, close, and finish actions. So rename that variable to "all" and remove
> the unnecessary mask operation that's already done earlier.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@hgst.com>
> [hch: split from the previous patch]
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Applied 7-10 to libata/for-4.8.

So, I dropped 4 and applied the rest.  Can you please check whether
the tree looks okay just in case?

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/libata.git for-4.8

Thanks.

-- 
tejun