[PATCH v2 0/2] Returning FIS on success for CDL

[PATCH v2 0/2] Returning FIS on success for CDL

[Igor Pylypiv]

This patch series plumbs libata's request for a result taskfile
(ATA_QCFLAG_RESULT_TF) through libsas to pm80xx LLDD. Other libsas LLDDs
can start using the newly added return_fis_on_success as well, if needed.

For Command Duration Limits policy 0xD (command completes without
an error) libata needs FIS in order to detect the ATA_SENSE bit and read
the Sense Data for Successful NCQ Commands log (0Fh). pm80xx HBAs do not
return FIS on success by default, hence, the driver is updated to set
the RETFIS bit (Return FIS on good completion) when requested by libsas.

Changes since v1:
- Dropped the ata_qc_has_cdl() helper
- Changed the condition when return_fis_on_success is getting set from
  "qc->flags & ATA_QCFLAG_HAS_CDL" to "qc->flags & ATA_QCFLAG_RESULT_TF".
  The latter is more generic solution that works for non-CDL use cases
  as well.
- Updated the retfis setting code in pm80xx to avoid potential compiler
  issues with single bit fields.

Igor Pylypiv (2):
  scsi: libsas: Add return_fis_on_success to sas_ata_task
  scsi: pm80xx: Set RETFIS when requested by libsas

 drivers/scsi/libsas/sas_ata.c    |  3 +++
 drivers/scsi/pm8001/pm8001_hwi.c | 11 ++++++++---
 drivers/scsi/pm8001/pm8001_hwi.h |  2 +-
 drivers/scsi/pm8001/pm80xx_hwi.c | 17 ++++++++++-------
 drivers/scsi/pm8001/pm80xx_hwi.h |  2 +-
 include/scsi/libsas.h            |  1 +
 6 files changed, 24 insertions(+), 12 deletions(-)

-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v2 1/2] scsi: libsas: Add return_fis_on_success to sas_ata_task

[Igor Pylypiv]

Set return_fis_on_success when libata requests result taskfile.

For Command Duration Limits policy 0xD (command completes without
an error) libata needs FIS in order to detect the ATA_SENSE bit and
read the Sense Data for Successful NCQ Commands log (0Fh).

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/libsas/sas_ata.c | 3 +++
 include/scsi/libsas.h         | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 77714a495cbb..e74b60d9c4b3 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -207,6 +207,9 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
 	task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
 	task->ata_task.dma_xfer = ata_is_dma(qc->tf.protocol);
 
+	if (qc->flags & ATA_QCFLAG_RESULT_TF)
+		task->ata_task.return_fis_on_success = 1;
+
 	if (qc->scsicmd)
 		ASSIGN_SAS_TASK(qc->scsicmd, task);
 
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 159823e0afbf..9e2c69c13dd3 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -550,6 +550,7 @@ struct sas_ata_task {
 	u8     use_ncq:1;
 	u8     set_affil_pol:1;
 	u8     stp_affil_pol:1;
+	u8     return_fis_on_success:1;
 
 	u8     device_control_reg_update:1;
 
-- 
2.42.0.rc1.204.g551eb34607-goog

[PATCH v2 2/2] scsi: pm80xx: Set RETFIS when requested by libsas

[Igor Pylypiv]

By default PM80xx HBAs return FIS only when a drive reports an error.
The RETFIS bit forces the controller to populate FIS even when a drive
reports no error.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 drivers/scsi/pm8001/pm8001_hwi.c |  9 ++++++---
 drivers/scsi/pm8001/pm8001_hwi.h |  2 +-
 drivers/scsi/pm8001/pm80xx_hwi.c | 24 +++++++++++-------------
 drivers/scsi/pm8001/pm80xx_hwi.h |  2 +-
 4 files changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index 73cd25f30ca5..649724a1d134 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.c
+++ b/drivers/scsi/pm8001/pm8001_hwi.c
@@ -4095,7 +4095,7 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 	u32 hdr_tag, ncg_tag = 0;
 	u64 phys_addr;
 	u32 ATAP = 0x0;
-	u32 dir;
+	u32 dir, retfis = 0;
 	u32  opc = OPC_INB_SATA_HOST_OPSTART;
 
 	memset(&sata_cmd, 0, sizeof(sata_cmd));
@@ -4124,8 +4124,11 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 	sata_cmd.tag = cpu_to_le32(tag);
 	sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
 	sata_cmd.data_len = cpu_to_le32(task->total_xfer_len);
-	sata_cmd.ncqtag_atap_dir_m =
-		cpu_to_le32(((ncg_tag & 0xff)<<16)|((ATAP & 0x3f) << 10) | dir);
+	if (task->ata_task.return_fis_on_success)
+		retfis = 1;
+	sata_cmd.retfis_ncqtag_atap_dir_m =
+		cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
+			    ((ATAP & 0x3f) << 10) | dir);
 	sata_cmd.sata_fis = task->ata_task.fis;
 	if (likely(!task->ata_task.device_control_reg_update))
 		sata_cmd.sata_fis.flags |= 0x80;/* C=1: update ATA cmd reg */
diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
index 961d0465b923..fc2127dcb58d 100644
--- a/drivers/scsi/pm8001/pm8001_hwi.h
+++ b/drivers/scsi/pm8001/pm8001_hwi.h
@@ -515,7 +515,7 @@ struct sata_start_req {
 	__le32	tag;
 	__le32	device_id;
 	__le32	data_len;
-	__le32	ncqtag_atap_dir_m;
+	__le32	retfis_ncqtag_atap_dir_m;
 	struct host_to_dev_fis	sata_fis;
 	u32	reserved1;
 	u32	reserved2;
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 39a12ee94a72..a5021577a15f 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -4457,7 +4457,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 	u64 phys_addr, end_addr;
 	u32 end_addr_high, end_addr_low;
 	u32 ATAP = 0x0;
-	u32 dir;
+	u32 dir, retfis = 0;
 	u32 opc = OPC_INB_SATA_HOST_OPSTART;
 	memset(&sata_cmd, 0, sizeof(sata_cmd));
 
@@ -4487,7 +4487,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 	sata_cmd.tag = cpu_to_le32(tag);
 	sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
 	sata_cmd.data_len = cpu_to_le32(task->total_xfer_len);
-
+	if (task->ata_task.return_fis_on_success)
+		retfis = 1;
 	sata_cmd.sata_fis = task->ata_task.fis;
 	if (likely(!task->ata_task.device_control_reg_update))
 		sata_cmd.sata_fis.flags |= 0x80;/* C=1: update ATA cmd reg */
@@ -4500,12 +4501,10 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 			   "Encryption enabled.Sending Encrypt SATA cmd 0x%x\n",
 			   sata_cmd.sata_fis.command);
 		opc = OPC_INB_SATA_DIF_ENC_IO;
-
-		/* set encryption bit */
-		sata_cmd.ncqtag_atap_dir_m_dad =
-			cpu_to_le32(((ncg_tag & 0xff)<<16)|
-				((ATAP & 0x3f) << 10) | 0x20 | dir);
-							/* dad (bit 0-1) is 0 */
+		/* set encryption bit; dad (bits 0-1) is 0 */
+		sata_cmd.retfis_ncqtag_atap_dir_m_dad =
+			cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
+				    ((ATAP & 0x3f) << 10) | 0x20 | dir);
 		/* fill in PRD (scatter/gather) table, if any */
 		if (task->num_scatter > 1) {
 			pm8001_chip_make_sg(task->scatter,
@@ -4568,11 +4567,10 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 		pm8001_dbg(pm8001_ha, IO,
 			   "Sending Normal SATA command 0x%x inb %x\n",
 			   sata_cmd.sata_fis.command, q_index);
-		/* dad (bit 0-1) is 0 */
-		sata_cmd.ncqtag_atap_dir_m_dad =
-			cpu_to_le32(((ncg_tag & 0xff)<<16) |
-					((ATAP & 0x3f) << 10) | dir);
-
+		/* dad (bits 0-1) is 0 */
+		sata_cmd.retfis_ncqtag_atap_dir_m_dad =
+			cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
+				    ((ATAP & 0x3f) << 10) | dir);
 		/* fill in PRD (scatter/gather) table, if any */
 		if (task->num_scatter > 1) {
 			pm8001_chip_make_sg(task->scatter,
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.h b/drivers/scsi/pm8001/pm80xx_hwi.h
index acf6e3005b84..eb8fd37b2066 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.h
+++ b/drivers/scsi/pm8001/pm80xx_hwi.h
@@ -731,7 +731,7 @@ struct sata_start_req {
 	__le32	tag;
 	__le32	device_id;
 	__le32	data_len;
-	__le32	ncqtag_atap_dir_m_dad;
+	__le32	retfis_ncqtag_atap_dir_m_dad;
 	struct host_to_dev_fis	sata_fis;
 	u32	reserved1;
 	u32	reserved2;	/* dword 11. rsvd for normal I/O. */
-- 
2.42.0.rc1.204.g551eb34607-goog

Re: [PATCH v2 1/2] scsi: libsas: Add return_fis_on_success to sas_ata_task

[Niklas Cassel]

On Sat, Aug 19, 2023 at 02:30:39PM -0700, Igor Pylypiv wrote:
> Set return_fis_on_success when libata requests result taskfile.
> 
> For Command Duration Limits policy 0xD (command completes without
> an error) libata needs FIS in order to detect the ATA_SENSE bit and
> read the Sense Data for Successful NCQ Commands log (0Fh).
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/scsi/libsas/sas_ata.c | 3 +++
>  include/scsi/libsas.h         | 1 +
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 77714a495cbb..e74b60d9c4b3 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -207,6 +207,9 @@ static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
>  	task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
>  	task->ata_task.dma_xfer = ata_is_dma(qc->tf.protocol);
>  
> +	if (qc->flags & ATA_QCFLAG_RESULT_TF)
> +		task->ata_task.return_fis_on_success = 1;
> +
>  	if (qc->scsicmd)
>  		ASSIGN_SAS_TASK(qc->scsicmd, task);
>  
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index 159823e0afbf..9e2c69c13dd3 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -550,6 +550,7 @@ struct sas_ata_task {
>  	u8     use_ncq:1;
>  	u8     set_affil_pol:1;
>  	u8     stp_affil_pol:1;
> +	u8     return_fis_on_success:1;
>  
>  	u8     device_control_reg_update:1;
>  
> -- 
> 2.42.0.rc1.204.g551eb34607-goog
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v2 2/2] scsi: pm80xx: Set RETFIS when requested by libsas

[Niklas Cassel]

On Sat, Aug 19, 2023 at 02:30:40PM -0700, Igor Pylypiv wrote:
> By default PM80xx HBAs return FIS only when a drive reports an error.
> The RETFIS bit forces the controller to populate FIS even when a drive
> reports no error.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |  9 ++++++---
>  drivers/scsi/pm8001/pm8001_hwi.h |  2 +-
>  drivers/scsi/pm8001/pm80xx_hwi.c | 24 +++++++++++-------------
>  drivers/scsi/pm8001/pm80xx_hwi.h |  2 +-
>  4 files changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> index 73cd25f30ca5..649724a1d134 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.c
> +++ b/drivers/scsi/pm8001/pm8001_hwi.c
> @@ -4095,7 +4095,7 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  	u32 hdr_tag, ncg_tag = 0;
>  	u64 phys_addr;
>  	u32 ATAP = 0x0;
> -	u32 dir;
> +	u32 dir, retfis = 0;
>  	u32  opc = OPC_INB_SATA_HOST_OPSTART;
>  
>  	memset(&sata_cmd, 0, sizeof(sata_cmd));
> @@ -4124,8 +4124,11 @@ static int pm8001_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  	sata_cmd.tag = cpu_to_le32(tag);
>  	sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
>  	sata_cmd.data_len = cpu_to_le32(task->total_xfer_len);
> -	sata_cmd.ncqtag_atap_dir_m =
> -		cpu_to_le32(((ncg_tag & 0xff)<<16)|((ATAP & 0x3f) << 10) | dir);
> +	if (task->ata_task.return_fis_on_success)
> +		retfis = 1;
> +	sata_cmd.retfis_ncqtag_atap_dir_m =
> +		cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> +			    ((ATAP & 0x3f) << 10) | dir);
>  	sata_cmd.sata_fis = task->ata_task.fis;
>  	if (likely(!task->ata_task.device_control_reg_update))
>  		sata_cmd.sata_fis.flags |= 0x80;/* C=1: update ATA cmd reg */
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
> index 961d0465b923..fc2127dcb58d 100644
> --- a/drivers/scsi/pm8001/pm8001_hwi.h
> +++ b/drivers/scsi/pm8001/pm8001_hwi.h
> @@ -515,7 +515,7 @@ struct sata_start_req {
>  	__le32	tag;
>  	__le32	device_id;
>  	__le32	data_len;
> -	__le32	ncqtag_atap_dir_m;
> +	__le32	retfis_ncqtag_atap_dir_m;
>  	struct host_to_dev_fis	sata_fis;
>  	u32	reserved1;
>  	u32	reserved2;
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 39a12ee94a72..a5021577a15f 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -4457,7 +4457,7 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  	u64 phys_addr, end_addr;
>  	u32 end_addr_high, end_addr_low;
>  	u32 ATAP = 0x0;
> -	u32 dir;
> +	u32 dir, retfis = 0;
>  	u32 opc = OPC_INB_SATA_HOST_OPSTART;
>  	memset(&sata_cmd, 0, sizeof(sata_cmd));
>  
> @@ -4487,7 +4487,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  	sata_cmd.tag = cpu_to_le32(tag);
>  	sata_cmd.device_id = cpu_to_le32(pm8001_ha_dev->device_id);
>  	sata_cmd.data_len = cpu_to_le32(task->total_xfer_len);
> -
> +	if (task->ata_task.return_fis_on_success)
> +		retfis = 1;
>  	sata_cmd.sata_fis = task->ata_task.fis;
>  	if (likely(!task->ata_task.device_control_reg_update))
>  		sata_cmd.sata_fis.flags |= 0x80;/* C=1: update ATA cmd reg */
> @@ -4500,12 +4501,10 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  			   "Encryption enabled.Sending Encrypt SATA cmd 0x%x\n",
>  			   sata_cmd.sata_fis.command);
>  		opc = OPC_INB_SATA_DIF_ENC_IO;
> -
> -		/* set encryption bit */
> -		sata_cmd.ncqtag_atap_dir_m_dad =
> -			cpu_to_le32(((ncg_tag & 0xff)<<16)|
> -				((ATAP & 0x3f) << 10) | 0x20 | dir);
> -							/* dad (bit 0-1) is 0 */
> +		/* set encryption bit; dad (bits 0-1) is 0 */
> +		sata_cmd.retfis_ncqtag_atap_dir_m_dad =
> +			cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> +				    ((ATAP & 0x3f) << 10) | 0x20 | dir);
>  		/* fill in PRD (scatter/gather) table, if any */
>  		if (task->num_scatter > 1) {
>  			pm8001_chip_make_sg(task->scatter,
> @@ -4568,11 +4567,10 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  		pm8001_dbg(pm8001_ha, IO,
>  			   "Sending Normal SATA command 0x%x inb %x\n",
>  			   sata_cmd.sata_fis.command, q_index);
> -		/* dad (bit 0-1) is 0 */
> -		sata_cmd.ncqtag_atap_dir_m_dad =
> -			cpu_to_le32(((ncg_tag & 0xff)<<16) |
> -					((ATAP & 0x3f) << 10) | dir);
> -
> +		/* dad (bits 0-1) is 0 */
> +		sata_cmd.retfis_ncqtag_atap_dir_m_dad =
> +			cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> +				    ((ATAP & 0x3f) << 10) | dir);
>  		/* fill in PRD (scatter/gather) table, if any */
>  		if (task->num_scatter > 1) {
>  			pm8001_chip_make_sg(task->scatter,
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.h b/drivers/scsi/pm8001/pm80xx_hwi.h
> index acf6e3005b84..eb8fd37b2066 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.h
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.h
> @@ -731,7 +731,7 @@ struct sata_start_req {
>  	__le32	tag;
>  	__le32	device_id;
>  	__le32	data_len;
> -	__le32	ncqtag_atap_dir_m_dad;
> +	__le32	retfis_ncqtag_atap_dir_m_dad;
>  	struct host_to_dev_fis	sata_fis;
>  	u32	reserved1;
>  	u32	reserved2;	/* dword 11. rsvd for normal I/O. */
> -- 
> 2.42.0.rc1.204.g551eb34607-goog
> 

Reviewed-by: Niklas Cassel <niklas.cassel@wdc.com>

Re: [PATCH v2 0/2] Returning FIS on success for CDL

[Damien Le Moal]

On 8/20/23 06:30, Igor Pylypiv wrote:
> This patch series plumbs libata's request for a result taskfile
> (ATA_QCFLAG_RESULT_TF) through libsas to pm80xx LLDD. Other libsas LLDDs
> can start using the newly added return_fis_on_success as well, if needed.
> 
> For Command Duration Limits policy 0xD (command completes without
> an error) libata needs FIS in order to detect the ATA_SENSE bit and read
> the Sense Data for Successful NCQ Commands log (0Fh). pm80xx HBAs do not
> return FIS on success by default, hence, the driver is updated to set
> the RETFIS bit (Return FIS on good completion) when requested by libsas.

For both patches:

Reviewed-by: Damien Le Moal <dlemoal@kernel.org>

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH v2 0/2] Returning FIS on success for CDL

[Martin K. Petersen]

Igor,

> This patch series plumbs libata's request for a result taskfile
> (ATA_QCFLAG_RESULT_TF) through libsas to pm80xx LLDD. Other libsas
> LLDDs can start using the newly added return_fis_on_success as well,
> if needed.
>
> For Command Duration Limits policy 0xD (command completes without an
> error) libata needs FIS in order to detect the ATA_SENSE bit and read
> the Sense Data for Successful NCQ Commands log (0Fh). pm80xx HBAs do
> not return FIS on success by default, hence, the driver is updated to
> set the RETFIS bit (Return FIS on good completion) when requested by
> libsas.

Applied to 6.6/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2 0/2] Returning FIS on success for CDL

[Martin K. Petersen]

On Sat, 19 Aug 2023 14:30:38 -0700, Igor Pylypiv wrote:

> This patch series plumbs libata's request for a result taskfile
> (ATA_QCFLAG_RESULT_TF) through libsas to pm80xx LLDD. Other libsas LLDDs
> can start using the newly added return_fis_on_success as well, if needed.
> 
> For Command Duration Limits policy 0xD (command completes without
> an error) libata needs FIS in order to detect the ATA_SENSE bit and read
> the Sense Data for Successful NCQ Commands log (0Fh). pm80xx HBAs do not
> return FIS on success by default, hence, the driver is updated to set
> the RETFIS bit (Return FIS on good completion) when requested by libsas.
> 
> [...]

Applied to 6.6/scsi-queue, thanks!

[1/2] scsi: libsas: Add return_fis_on_success to sas_ata_task
      https://git.kernel.org/mkp/scsi/c/72875018f638
[2/2] scsi: pm80xx: Set RETFIS when requested by libsas
      https://git.kernel.org/mkp/scsi/c/545432959551

-- 
Martin K. Petersen	Oracle Linux Engineering