[PATCH v1] scsi: libsas: set data_dir as DMA_NONE if libata mark qc as NODATA

[PATCH v1] scsi: libsas: set data_dir as DMA_NONE if libata mark qc as NODATA

[Luo Jiaxing]

We found that it will fail every time when set feature to SATA disk by
"sdparm -s WCE=0 /dev/sde".

After checking protocol, we know that MODE SELECT is the SCSI command for
setting WCE, and it do not exist in the SATA protocol. Therefore, this
commands are encapsulated in the SET FEATURE command in SATA protocol.
The difference is that the MODE SELECT command sent to SAS disk contains
data and is sent through the DMA. But when send to SATA disk through
SET FEATURE command, it does not contain data.

I think libsas was not thorough enough in dealing with the situation, at
sas_ata_qc_issue(), task->dma_dir is inherited from qc->dma_dir(qc->dma_dir
is also inherited from SCSI). However, in ATA driver, if qc->tf.protocol is
set to ATA_PROT_NODATA, ata_sg_setup() will not invoked by ata_qc_issue().
It mean that ATA didn't use dma_dir and it's not reliable. So, if libsas
still inherits from qc->dma_dir when there is no data need to be send. As a
result, task with no data are mistakenly considered as carrying data and it
will make LLDD report an error on IO completion.

So, When ATA driver mark tf->protocol as NODATA, dma_dir should be set as
DMA_NONE. And we can see WCE is successfully disable for SATA disk then.

Euler:~ # sdparm -s WCE=0 /dev/sde
     /dev/sde: ATA       ST4000NM0035-1V4  TN03
Euler:~ # sdparm /dev/sde
     /dev/sde: ATA       ST4000NM0035-1V4  TN03
Read write error recovery mode page:
   AWRE        1  [cha: n, def:  1]
   ARRE        0  [cha: n, def:  0]
   PER         0  [cha: n, def:  0]
Caching (SBC) mode page:
   WCE         0  [cha: y, def:  0]
   RCD         0  [cha: n, def:  0]
Control mode page:
   SWP         0  [cha: n, def:  0]

Fixes: fa1c1e8f1ece ("[SCSI] Add SATA support to libsas")

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
Reviewed-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 6a521ba..a488798 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -209,7 +209,10 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 		task->num_scatter = si;
 	}
 
-	task->data_dir = qc->dma_dir;
+	if (qc->tf.protocol == ATA_PROT_NODATA)
+		task->data_dir = DMA_NONE;
+	else
+		task->data_dir = qc->dma_dir;
 	task->scatter = qc->sg;
 	task->ata_task.retry_count = 1;
 	task->task_state_flags = SAS_TASK_STATE_PENDING;
-- 
2.7.4

Re: [PATCH v1] scsi: libsas: set data_dir as DMA_NONE if libata mark qc as NODATA

[Jason Yan]

在 2020/8/26 15:24, Luo Jiaxing 写道:
> We found that it will fail every time when set feature to SATA disk by
> "sdparm -s WCE=0 /dev/sde".
> 
> After checking protocol, we know that MODE SELECT is the SCSI command for
> setting WCE, and it do not exist in the SATA protocol. Therefore, this
> commands are encapsulated in the SET FEATURE command in SATA protocol.
> The difference is that the MODE SELECT command sent to SAS disk contains
> data and is sent through the DMA. But when send to SATA disk through
> SET FEATURE command, it does not contain data.
> 
> I think libsas was not thorough enough in dealing with the situation, at
> sas_ata_qc_issue(), task->dma_dir is inherited from qc->dma_dir(qc->dma_dir
> is also inherited from SCSI). However, in ATA driver, if qc->tf.protocol is
> set to ATA_PROT_NODATA, ata_sg_setup() will not invoked by ata_qc_issue().
> It mean that ATA didn't use dma_dir and it's not reliable. So, if libsas
> still inherits from qc->dma_dir when there is no data need to be send. As a
> result, task with no data are mistakenly considered as carrying data and it
> will make LLDD report an error on IO completion.
> 
> So, When ATA driver mark tf->protocol as NODATA, dma_dir should be set as
> DMA_NONE. And we can see WCE is successfully disable for SATA disk then.
> 
> Euler:~ # sdparm -s WCE=0 /dev/sde
>       /dev/sde: ATA       ST4000NM0035-1V4  TN03
> Euler:~ # sdparm /dev/sde
>       /dev/sde: ATA       ST4000NM0035-1V4  TN03
> Read write error recovery mode page:
>     AWRE        1  [cha: n, def:  1]
>     ARRE        0  [cha: n, def:  0]
>     PER         0  [cha: n, def:  0]
> Caching (SBC) mode page:
>     WCE         0  [cha: y, def:  0]
>     RCD         0  [cha: n, def:  0]
> Control mode page:
>     SWP         0  [cha: n, def:  0]
> 
> Fixes: fa1c1e8f1ece ("[SCSI] Add SATA support to libsas")
> 
> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
> Reviewed-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/libsas/sas_ata.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 6a521ba..a488798 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -209,7 +209,10 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
>   		task->num_scatter = si;
>   	}
>   
> -	task->data_dir = qc->dma_dir;
> +	if (qc->tf.protocol == ATA_PROT_NODATA)
> +		task->data_dir = DMA_NONE;
> +	else
> +		task->data_dir = qc->dma_dir;
>   	task->scatter = qc->sg;
>   	task->ata_task.retry_count = 1;
>   	task->task_state_flags = SAS_TASK_STATE_PENDING;
> 

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Re: [PATCH v1] scsi: libsas: set data_dir as DMA_NONE if libata mark qc as NODATA

[Martin K. Petersen]

On Wed, 26 Aug 2020 15:24:26 +0800, Luo Jiaxing wrote:

> We found that it will fail every time when set feature to SATA disk by
> "sdparm -s WCE=0 /dev/sde".
> 
> After checking protocol, we know that MODE SELECT is the SCSI command for
> setting WCE, and it do not exist in the SATA protocol. Therefore, this
> commands are encapsulated in the SET FEATURE command in SATA protocol.
> The difference is that the MODE SELECT command sent to SAS disk contains
> data and is sent through the DMA. But when send to SATA disk through
> SET FEATURE command, it does not contain data.
> 
> [...]

Applied to 5.9/scsi-fixes, thanks!

[1/1] scsi: libsas: Set data_dir as DMA_NONE if libata marks qc as NODATA
      https://git.kernel.org/mkp/scsi/c/53de092f47ff

-- 
Martin K. Petersen	Oracle Linux Engineering