[PATCH 1/3] ata: libata: Introduce ata_qc_has_cdl()

[PATCH 1/3] ata: libata: Introduce ata_qc_has_cdl()

[Igor Pylypiv]

Introduce the inline helper function ata_qc_has_cdl() to test if
a queued command has a Command Duration Limits descriptor set.

Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
---
 include/linux/libata.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/libata.h b/include/linux/libata.h
index 820f7a3a2749..bc7870f1f527 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -1062,6 +1062,11 @@ static inline bool ata_port_is_frozen(const struct ata_port *ap)
 	return ap->pflags & ATA_PFLAG_FROZEN;
 }
 
+static inline bool ata_qc_has_cdl(struct ata_queued_cmd *qc)
+{
+	return qc->flags & ATA_QCFLAG_HAS_CDL;
+}
+
 extern int ata_std_prereset(struct ata_link *link, unsigned long deadline);
 extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
 				int (*check_ready)(struct ata_link *link));
-- 
2.42.0.rc1.204.g551eb34607-goog

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

[Igor Pylypiv]

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

Set return_fis_on_success for commands that have a CDL descriptor
since any CDL descriptor can be configured with policy 0xD.

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..da67c4f671b2 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);
 
+	/* CDL policy 0xD requires FIS for successful (no error) completions */
+	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);
+
 	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 3/3] 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 |  8 +++++---
 drivers/scsi/pm8001/pm8001_hwi.h |  2 +-
 drivers/scsi/pm8001/pm80xx_hwi.c | 11 ++++++-----
 drivers/scsi/pm8001/pm80xx_hwi.h |  2 +-
 4 files changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
index 73cd25f30ca5..255553dcadb9 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;
 	u32  opc = OPC_INB_SATA_HOST_OPSTART;
 
 	memset(&sata_cmd, 0, sizeof(sata_cmd));
@@ -4124,8 +4124,10 @@ 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);
+	retfis = task->ata_task.return_fis_on_success;
+	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..e839fb53f0e3 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;
 	u32 opc = OPC_INB_SATA_HOST_OPSTART;
 	memset(&sata_cmd, 0, sizeof(sata_cmd));
 
@@ -4487,6 +4487,7 @@ 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);
+	retfis = task->ata_task.return_fis_on_success;
 
 	sata_cmd.sata_fis = task->ata_task.fis;
 	if (likely(!task->ata_task.device_control_reg_update))
@@ -4502,8 +4503,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 		opc = OPC_INB_SATA_DIF_ENC_IO;
 
 		/* set encryption bit */
-		sata_cmd.ncqtag_atap_dir_m_dad =
-			cpu_to_le32(((ncg_tag & 0xff)<<16)|
+		sata_cmd.retfis_ncqtag_atap_dir_m_dad =
+			cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
 				((ATAP & 0x3f) << 10) | 0x20 | dir);
 							/* dad (bit 0-1) is 0 */
 		/* fill in PRD (scatter/gather) table, if any */
@@ -4569,8 +4570,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
 			   "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) |
+		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 */
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 1/3] ata: libata: Introduce ata_qc_has_cdl()

[Damien Le Moal]

On 2023/08/18 6:41, Igor Pylypiv wrote:
> Introduce the inline helper function ata_qc_has_cdl() to test if
> a queued command has a Command Duration Limits descriptor set.
> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  include/linux/libata.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/libata.h b/include/linux/libata.h
> index 820f7a3a2749..bc7870f1f527 100644
> --- a/include/linux/libata.h
> +++ b/include/linux/libata.h
> @@ -1062,6 +1062,11 @@ static inline bool ata_port_is_frozen(const struct ata_port *ap)
>  	return ap->pflags & ATA_PFLAG_FROZEN;
>  }
>  
> +static inline bool ata_qc_has_cdl(struct ata_queued_cmd *qc)
> +{
> +	return qc->flags & ATA_QCFLAG_HAS_CDL;
> +}

This is used in one place only in patch 3, and the only other place we test this
flag is in ata_build_rw_tf(). So I do not think this is necessary. Let's drop this.

> +
>  extern int ata_std_prereset(struct ata_link *link, unsigned long deadline);
>  extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
>  				int (*check_ready)(struct ata_link *link));

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2023/08/18 6:41, Igor Pylypiv wrote:
> 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).
> 
> Set return_fis_on_success for commands that have a CDL descriptor
> since any CDL descriptor can be configured with policy 0xD.
> 
> 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..da67c4f671b2 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);
>  
> +	/* CDL policy 0xD requires FIS for successful (no error) completions */
> +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);

From the comments on patch 1, this should be:

	if (qc->flags & ATA_QCFLAG_HAS_CDL)
		task->ata_task.return_sdb_fis_on_success = 1;

Note the renaming to "return_sdb_fis_on_success" to be clear about the FIS type.

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

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2023/08/18 6:41, Igor Pylypiv wrote:
> By default PM80xx HBAs return FIS only when a drive reports an error.

s/FIS/SDB FIS or even better "Set Device Bits (SDB) FIS" to be clear.

> The RETFIS bit forces the controller to populate FIS even when a drive
> reports no error.

And here s/FIS/SDB FIS

> 
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> ---
>  drivers/scsi/pm8001/pm8001_hwi.c |  8 +++++---
>  drivers/scsi/pm8001/pm8001_hwi.h |  2 +-
>  drivers/scsi/pm8001/pm80xx_hwi.c | 11 ++++++-----
>  drivers/scsi/pm8001/pm80xx_hwi.h |  2 +-
>  4 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> index 73cd25f30ca5..255553dcadb9 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;
>  	u32  opc = OPC_INB_SATA_HOST_OPSTART;
>  
>  	memset(&sata_cmd, 0, sizeof(sata_cmd));
> @@ -4124,8 +4124,10 @@ 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);
> +	retfis = task->ata_task.return_fis_on_success;

While I think this should be OK, I think it would be safer to do:

	u32 dir, retfis = 0;

	...

	if (task->ata_task.return_fis_on_success)
		retfis = 1;

to avoid issues with funky compilers doing some tricky handling of single bit
fields.

> +	sata_cmd.retfis_ncqtag_atap_dir_m =
> +		cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> +				((ATAP & 0x3f) << 10) | dir);

Please align this line with "(retfis << 24)" above.

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

Naming this field from what is set in it is unusual... Not sure how the
controller spce named this field, but we should use that and stop changing it's
name whenever we change the bits that are set.

>  	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..e839fb53f0e3 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;
>  	u32 opc = OPC_INB_SATA_HOST_OPSTART;
>  	memset(&sata_cmd, 0, sizeof(sata_cmd));
>  
> @@ -4487,6 +4487,7 @@ 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);
> +	retfis = task->ata_task.return_fis_on_success;
>  
>  	sata_cmd.sata_fis = task->ata_task.fis;
>  	if (likely(!task->ata_task.device_control_reg_update))
> @@ -4502,8 +4503,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  		opc = OPC_INB_SATA_DIF_ENC_IO;
>  
>  		/* set encryption bit */
> -		sata_cmd.ncqtag_atap_dir_m_dad =
> -			cpu_to_le32(((ncg_tag & 0xff)<<16)|
> +		sata_cmd.retfis_ncqtag_atap_dir_m_dad =
> +			cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
>  				((ATAP & 0x3f) << 10) | 0x20 | dir);

Same comments here.

>  							/* dad (bit 0-1) is 0 */
>  		/* fill in PRD (scatter/gather) table, if any */
> @@ -4569,8 +4570,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
>  			   "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) |
> +		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 */
> 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. */

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote:

Hello Igor,

> 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).
> 
> Set return_fis_on_success for commands that have a CDL descriptor
> since any CDL descriptor can be configured with policy 0xD.
> 
> 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..da67c4f671b2 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);
>  
> +	/* CDL policy 0xD requires FIS for successful (no error) completions */
> +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);

In ata_qc_complete(), for a successful command, we call fill_result_tf()
if (qc->flags & ATA_QCFLAG_RESULT_TF):
https://github.com/torvalds/linux/blob/v6.5-rc6/drivers/ata/libata-core.c#L4926

My point is, I think that you should set
task->ata_task.return_fis_on_success = ata_qc_wants_result(qc);

where ata_qc_wants_result()
returns true if ATA_QCFLAG_RESULT_TF is set.

(ata_set_tf_cdl() will set both ATA_QCFLAG_HAS_CDL and ATA_QCFLAG_RESULT_TF).

That way, e.g. an internal command (i.e. a command issued by
ata_exec_internal_sg()), which always has ATA_QCFLAG_RESULT_TF set,
will always gets an up to date tf status and tf error value back,
because the SAS HBA will send a FIS back.

If we don't do this, then libsas will instead fill in the tf status and
tf error from the last command that returned a FIS (which might be out
of date).


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

Considering that libsas return value is defined like this:
https://github.com/torvalds/linux/blob/v6.5-rc6/include/scsi/libsas.h#L507

Basically, if you returned a FIS in resp->ending_fis, you should return
SAS_PROTO_RESPONSE. If you didn't return a FIS for your command, then
you return SAS_SAM_STAT_GOOD (or if it is an error, a SAS_ error code
that is not SAS_PROTO_RESPONSE, as you didn't return a FIS).

Since you have implemented this only for pm80xx, how about adding something
like this to sas_ata_task_done:

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 77714a495cbb..e1c56c2c00a5 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -114,6 +114,15 @@ static void sas_ata_task_done(struct sas_task *task)
                }
        }
 
+       /*
+        * If a FIS was requested for a good command, and the lldd returned
+        * SAS_SAM_STAT_GOOD instead of SAS_PROTO_RESPONSE, then the lldd
+        * has not implemented support for sas_ata_task.return_fis_on_success
+        * Warn about this once. If we don't return FIS on success, then we
+        * won't be able to return an up to date TF.status and TF.error.
+        */
+       WARN_ON_ONCE(ata_qc_wants_result(qc) && stat->stat == SAS_SAM_STAT_GOOD);
+
        if (stat->stat == SAS_PROTO_RESPONSE ||
            stat->stat == SAS_SAM_STAT_GOOD ||
            (stat->stat == SAS_SAM_STAT_CHECK_CONDITION &&




Kind regards,
Niklas

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

[Niklas Cassel]

On Fri, Aug 18, 2023 at 08:12:02AM +0900, Damien Le Moal wrote:
> On 2023/08/18 6:41, Igor Pylypiv wrote:
> > 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).
> > 
> > Set return_fis_on_success for commands that have a CDL descriptor
> > since any CDL descriptor can be configured with policy 0xD.
> > 
> > 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..da67c4f671b2 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);
> >  
> > +	/* CDL policy 0xD requires FIS for successful (no error) completions */
> > +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);
> 
> From the comments on patch 1, this should be:
> 
> 	if (qc->flags & ATA_QCFLAG_HAS_CDL)
> 		task->ata_task.return_sdb_fis_on_success = 1;
> 
> Note the renaming to "return_sdb_fis_on_success" to be clear about the FIS type.

I'm not sure if I agree with this comment.

According to the pm80xx programmers manual,
setting the RETFIS bit enables the HBA to return a FIS for a successful
command:

PIO Read: Nothing is returned
PIO Write: Device To Host FIS received from the device.
DMA Read: Device To Host FIS received from the device.
DMA Write: Device To Host FIS received from the device.
FPDMA Read: Set Device Bit FIS received from the device.
FPDMA Write: Set Device Bit FIS received from the device.
Non-Data: Device To Host FIS received from the device.

So the FIS you get back can also be e.g. a D2H FIS, if you send
e.g. a DMA read command.

If the RETFIS bit is not set, and the command was successful,
no FIS will be returned.

So if you really want to rename this bit, then we would also need to
change the logic in pm80xx to be something like:
if (ata_is_ncq() && task->ata_task.return_sdb_fis_on_success)
	set_RETFIS_bit;

Doesn't it make more sense for this generic libsas flag to keep its
current name, i.e. it means that we enable FIS reception for successful
commands, regardless of FIS type?


Kind regards,
Niklas

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

[Damien Le Moal]

On 2023/08/18 8:50, Niklas Cassel wrote:
> On Fri, Aug 18, 2023 at 08:12:02AM +0900, Damien Le Moal wrote:
>> On 2023/08/18 6:41, Igor Pylypiv wrote:
>>> 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).
>>>
>>> Set return_fis_on_success for commands that have a CDL descriptor
>>> since any CDL descriptor can be configured with policy 0xD.
>>>
>>> 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..da67c4f671b2 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);
>>>  
>>> +	/* CDL policy 0xD requires FIS for successful (no error) completions */
>>> +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);
>>
>> From the comments on patch 1, this should be:
>>
>> 	if (qc->flags & ATA_QCFLAG_HAS_CDL)
>> 		task->ata_task.return_sdb_fis_on_success = 1;
>>
>> Note the renaming to "return_sdb_fis_on_success" to be clear about the FIS type.
> 
> I'm not sure if I agree with this comment.
> 
> According to the pm80xx programmers manual,
> setting the RETFIS bit enables the HBA to return a FIS for a successful
> command:
> 
> PIO Read: Nothing is returned
> PIO Write: Device To Host FIS received from the device.
> DMA Read: Device To Host FIS received from the device.
> DMA Write: Device To Host FIS received from the device.
> FPDMA Read: Set Device Bit FIS received from the device.
> FPDMA Write: Set Device Bit FIS received from the device.
> Non-Data: Device To Host FIS received from the device.
> 
> So the FIS you get back can also be e.g. a D2H FIS, if you send
> e.g. a DMA read command.

Right. Forgot about non NCQ commands :)
So no need for renaming to sdb_fis. Apologies for the noise.

> 
> If the RETFIS bit is not set, and the command was successful,
> no FIS will be returned.
> 
> So if you really want to rename this bit, then we would also need to
> change the logic in pm80xx to be something like:
> if (ata_is_ncq() && task->ata_task.return_sdb_fis_on_success)
> 	set_RETFIS_bit;
> 
> Doesn't it make more sense for this generic libsas flag to keep its
> current name, i.e. it means that we enable FIS reception for successful
> commands, regardless of FIS type?

Yes.

> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2023/08/18 8:36, Niklas Cassel wrote:
> On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote:
> 
> Hello Igor,
> 
>> 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).
>>
>> Set return_fis_on_success for commands that have a CDL descriptor
>> since any CDL descriptor can be configured with policy 0xD.
>>
>> 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..da67c4f671b2 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);
>>  
>> +	/* CDL policy 0xD requires FIS for successful (no error) completions */
>> +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);
> 
> In ata_qc_complete(), for a successful command, we call fill_result_tf()
> if (qc->flags & ATA_QCFLAG_RESULT_TF):
> https://github.com/torvalds/linux/blob/v6.5-rc6/drivers/ata/libata-core.c#L4926
> 
> My point is, I think that you should set
> task->ata_task.return_fis_on_success = ata_qc_wants_result(qc);
> 
> where ata_qc_wants_result()
> returns true if ATA_QCFLAG_RESULT_TF is set.

I do not think we need that helper. Testing the flag directly would be fine.
If you really insist on introducing the helper, then at least go through libata
and replace all direct tests of that flag with the helper. But I do not think it
is worth the churn.

> 
> (ata_set_tf_cdl() will set both ATA_QCFLAG_HAS_CDL and ATA_QCFLAG_RESULT_TF).
> 
> That way, e.g. an internal command (i.e. a command issued by
> ata_exec_internal_sg()), which always has ATA_QCFLAG_RESULT_TF set,
> will always gets an up to date tf status and tf error value back,
> because the SAS HBA will send a FIS back.
> 
> If we don't do this, then libsas will instead fill in the tf status and
> tf error from the last command that returned a FIS (which might be out
> of date).
> 
> 
>> +
>>  	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
>>
> 
> Considering that libsas return value is defined like this:
> https://github.com/torvalds/linux/blob/v6.5-rc6/include/scsi/libsas.h#L507
> 
> Basically, if you returned a FIS in resp->ending_fis, you should return
> SAS_PROTO_RESPONSE. If you didn't return a FIS for your command, then
> you return SAS_SAM_STAT_GOOD (or if it is an error, a SAS_ error code
> that is not SAS_PROTO_RESPONSE, as you didn't return a FIS).
> 
> Since you have implemented this only for pm80xx, how about adding something
> like this to sas_ata_task_done:
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 77714a495cbb..e1c56c2c00a5 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -114,6 +114,15 @@ static void sas_ata_task_done(struct sas_task *task)
>                 }
>         }
>  
> +       /*
> +        * If a FIS was requested for a good command, and the lldd returned
> +        * SAS_SAM_STAT_GOOD instead of SAS_PROTO_RESPONSE, then the lldd
> +        * has not implemented support for sas_ata_task.return_fis_on_success
> +        * Warn about this once. If we don't return FIS on success, then we
> +        * won't be able to return an up to date TF.status and TF.error.
> +        */
> +       WARN_ON_ONCE(ata_qc_wants_result(qc) && stat->stat == SAS_SAM_STAT_GOOD);
> +
>         if (stat->stat == SAS_PROTO_RESPONSE ||
>             stat->stat == SAS_SAM_STAT_GOOD ||
>             (stat->stat == SAS_SAM_STAT_CHECK_CONDITION &&
> 
> 
> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research

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

[Damien Le Moal]

On 2023/08/18 8:36, Niklas Cassel wrote:
> On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote:
> 
> Hello Igor,
> 
>> 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).
>>
>> Set return_fis_on_success for commands that have a CDL descriptor
>> since any CDL descriptor can be configured with policy 0xD.
>>
>> 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..da67c4f671b2 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);
>>  
>> +	/* CDL policy 0xD requires FIS for successful (no error) completions */
>> +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);
> 
> In ata_qc_complete(), for a successful command, we call fill_result_tf()
> if (qc->flags & ATA_QCFLAG_RESULT_TF):
> https://github.com/torvalds/linux/blob/v6.5-rc6/drivers/ata/libata-core.c#L4926
> 
> My point is, I think that you should set
> task->ata_task.return_fis_on_success = ata_qc_wants_result(qc);
> 
> where ata_qc_wants_result()
> returns true if ATA_QCFLAG_RESULT_TF is set.
> 
> (ata_set_tf_cdl() will set both ATA_QCFLAG_HAS_CDL and ATA_QCFLAG_RESULT_TF).
> 
> That way, e.g. an internal command (i.e. a command issued by
> ata_exec_internal_sg()), which always has ATA_QCFLAG_RESULT_TF set,
> will always gets an up to date tf status and tf error value back,
> because the SAS HBA will send a FIS back.

+1

> 
> If we don't do this, then libsas will instead fill in the tf status and
> tf error from the last command that returned a FIS (which might be out
> of date).

-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

On Fri, Aug 18, 2023 at 09:08:26AM +0900, Damien Le Moal wrote:
> On 2023/08/18 8:36, Niklas Cassel wrote:
> > On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote:
> > 
> > Hello Igor,
> > 
> >> 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).
> >>
> >> Set return_fis_on_success for commands that have a CDL descriptor
> >> since any CDL descriptor can be configured with policy 0xD.
> >>
> >> 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..da67c4f671b2 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);
> >>  
> >> +	/* CDL policy 0xD requires FIS for successful (no error) completions */
> >> +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);
> > 
> > In ata_qc_complete(), for a successful command, we call fill_result_tf()
> > if (qc->flags & ATA_QCFLAG_RESULT_TF):
> > https://github.com/torvalds/linux/blob/v6.5-rc6/drivers/ata/libata-core.c#L4926
> > 
> > My point is, I think that you should set
> > task->ata_task.return_fis_on_success = ata_qc_wants_result(qc);
> > 
> > where ata_qc_wants_result()
> > returns true if ATA_QCFLAG_RESULT_TF is set.
> 
> I do not think we need that helper. Testing the flag directly would be fine.
> If you really insist on introducing the helper, then at least go through libata
> and replace all direct tests of that flag with the helper. But I do not think it
> is worth the churn.

I agree that there is no need for a helper.


Kind regards,
Niklas

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

[Niklas Cassel]

On Fri, Aug 18, 2023 at 01:36:39AM +0200, Niklas Cassel wrote:
> On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote:

(snip)

> Considering that libsas return value is defined like this:
> https://github.com/torvalds/linux/blob/v6.5-rc6/include/scsi/libsas.h#L507
> 
> Basically, if you returned a FIS in resp->ending_fis, you should return
> SAS_PROTO_RESPONSE. If you didn't return a FIS for your command, then
> you return SAS_SAM_STAT_GOOD (or if it is an error, a SAS_ error code
> that is not SAS_PROTO_RESPONSE, as you didn't return a FIS).
> 
> Since you have implemented this only for pm80xx, how about adding something
> like this to sas_ata_task_done:
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 77714a495cbb..e1c56c2c00a5 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -114,6 +114,15 @@ static void sas_ata_task_done(struct sas_task *task)
>                 }
>         }
>  
> +       /*
> +        * If a FIS was requested for a good command, and the lldd returned
> +        * SAS_SAM_STAT_GOOD instead of SAS_PROTO_RESPONSE, then the lldd
> +        * has not implemented support for sas_ata_task.return_fis_on_success
> +        * Warn about this once. If we don't return FIS on success, then we
> +        * won't be able to return an up to date TF.status and TF.error.
> +        */
> +       WARN_ON_ONCE(ata_qc_wants_result(qc) && stat->stat == SAS_SAM_STAT_GOOD);
> +
>         if (stat->stat == SAS_PROTO_RESPONSE ||
>             stat->stat == SAS_SAM_STAT_GOOD ||
>             (stat->stat == SAS_SAM_STAT_CHECK_CONDITION &&
> 

Note that some drivers, e.g. aic94xx, already seem to:
always copy to ending_fis, and sets SAS_PROTO_RESPONSE,
both for successful and error commands. So it would already work.

Other drivers like isci, hisi, and mvsas seem to always copy
to ending_fis, but incorrectly sets status to SAS_PROTO_RESPONSE
only when there was an error.
They should probably also set status to SAS_PROTO_RESPONSE in the
success case, since they did copy the FIS also in the success case.


Kind regards,
Niklas

Re: [PATCH 1/3] ata: libata: Introduce ata_qc_has_cdl()

[Igor Pylypiv]

On Fri, Aug 18, 2023 at 08:11:39AM +0900, Damien Le Moal wrote:
> On 2023/08/18 6:41, Igor Pylypiv wrote:
> > Introduce the inline helper function ata_qc_has_cdl() to test if
> > a queued command has a Command Duration Limits descriptor set.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  include/linux/libata.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 820f7a3a2749..bc7870f1f527 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -1062,6 +1062,11 @@ static inline bool ata_port_is_frozen(const struct ata_port *ap)
> >  	return ap->pflags & ATA_PFLAG_FROZEN;
> >  }
> >  
> > +static inline bool ata_qc_has_cdl(struct ata_queued_cmd *qc)
> > +{
> > +	return qc->flags & ATA_QCFLAG_HAS_CDL;
> > +}
> 
> This is used in one place only in patch 3, and the only other place we test this
> flag is in ata_build_rw_tf(). So I do not think this is necessary. Let's drop this.

Thanks Damien! I'll drop this patch in v2.
As discussed in PATCH 2/3 we'll check for ATA_QCFLAG_RESULT_TF instead.

> 
> > +
> >  extern int ata_std_prereset(struct ata_link *link, unsigned long deadline);
> >  extern int ata_wait_after_reset(struct ata_link *link, unsigned long deadline,
> >  				int (*check_ready)(struct ata_link *link));
> 
> -- 
> Damien Le Moal
> Western Digital Research
>

Thanks,
Igor

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

[Igor Pylypiv]

On Thu, Aug 17, 2023 at 11:36:43PM +0000, Niklas Cassel wrote:
> On Thu, Aug 17, 2023 at 02:41:36PM -0700, Igor Pylypiv wrote:
> 
> Hello Igor,
> 
> > 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).
> > 
> > Set return_fis_on_success for commands that have a CDL descriptor
> > since any CDL descriptor can be configured with policy 0xD.
> > 
> > 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..da67c4f671b2 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);
> >  
> > +	/* CDL policy 0xD requires FIS for successful (no error) completions */
> > +	task->ata_task.return_fis_on_success = ata_qc_has_cdl(qc);
> 
> In ata_qc_complete(), for a successful command, we call fill_result_tf()
> if (qc->flags & ATA_QCFLAG_RESULT_TF):
> https://github.com/torvalds/linux/blob/v6.5-rc6/drivers/ata/libata-core.c#L4926
> 
> My point is, I think that you should set
> task->ata_task.return_fis_on_success = ata_qc_wants_result(qc);
> 
> where ata_qc_wants_result()
> returns true if ATA_QCFLAG_RESULT_TF is set.
> 
> (ata_set_tf_cdl() will set both ATA_QCFLAG_HAS_CDL and ATA_QCFLAG_RESULT_TF).
> 
> That way, e.g. an internal command (i.e. a command issued by
> ata_exec_internal_sg()), which always has ATA_QCFLAG_RESULT_TF set,
> will always gets an up to date tf status and tf error value back,
> because the SAS HBA will send a FIS back.
> 
> If we don't do this, then libsas will instead fill in the tf status and
> tf error from the last command that returned a FIS (which might be out
> of date).

Hi Niklas,

Thanks for the suggestion! I'll update the check to ATA_QCFLAG_RESULT_TF in v2.

> 
> 
> > +
> >  	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
> > 
> 
> Considering that libsas return value is defined like this:
> https://github.com/torvalds/linux/blob/v6.5-rc6/include/scsi/libsas.h#L507
> 
> Basically, if you returned a FIS in resp->ending_fis, you should return
> SAS_PROTO_RESPONSE. If you didn't return a FIS for your command, then
> you return SAS_SAM_STAT_GOOD (or if it is an error, a SAS_ error code
> that is not SAS_PROTO_RESPONSE, as you didn't return a FIS).
> 
> Since you have implemented this only for pm80xx, how about adding something
> like this to sas_ata_task_done:
> 
> diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> index 77714a495cbb..e1c56c2c00a5 100644
> --- a/drivers/scsi/libsas/sas_ata.c
> +++ b/drivers/scsi/libsas/sas_ata.c
> @@ -114,6 +114,15 @@ static void sas_ata_task_done(struct sas_task *task)
>                 }
>         }
>  
> +       /*
> +        * If a FIS was requested for a good command, and the lldd returned
> +        * SAS_SAM_STAT_GOOD instead of SAS_PROTO_RESPONSE, then the lldd
> +        * has not implemented support for sas_ata_task.return_fis_on_success
> +        * Warn about this once. If we don't return FIS on success, then we
> +        * won't be able to return an up to date TF.status and TF.error.
> +        */
> +       WARN_ON_ONCE(ata_qc_wants_result(qc) && stat->stat == SAS_SAM_STAT_GOOD);

I'm a bit hesitant about adding this warning. pm80xx manual states that FIS
is not getting returned for PIO Read commands. ata_dev_read_id() sends
ATA_CMD_ID_ATA (0xEC) PIO command during bus probe resulting in this warning
getting triggered every time. Checking for !PIO doesn't seem to be the right
thing to do. I'll hold off from adding the warning.

> +
>         if (stat->stat == SAS_PROTO_RESPONSE ||
>             stat->stat == SAS_SAM_STAT_GOOD ||
>             (stat->stat == SAS_SAM_STAT_CHECK_CONDITION &&
> 

Thanks,
Igor

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

[Igor Pylypiv]

On Fri, Aug 18, 2023 at 08:12:13AM +0900, Damien Le Moal wrote:
> On 2023/08/18 6:41, Igor Pylypiv wrote:
> > By default PM80xx HBAs return FIS only when a drive reports an error.
> 
> s/FIS/SDB FIS or even better "Set Device Bits (SDB) FIS" to be clear.
> 
> > The RETFIS bit forces the controller to populate FIS even when a drive
> > reports no error.
> 
> And here s/FIS/SDB FIS

Keeping "FIS" per discussion in PATCH 2/3 (SDB FIS applies only to NCQ).

> 
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > ---
> >  drivers/scsi/pm8001/pm8001_hwi.c |  8 +++++---
> >  drivers/scsi/pm8001/pm8001_hwi.h |  2 +-
> >  drivers/scsi/pm8001/pm80xx_hwi.c | 11 ++++++-----
> >  drivers/scsi/pm8001/pm80xx_hwi.h |  2 +-
> >  4 files changed, 13 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c
> > index 73cd25f30ca5..255553dcadb9 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;
> >  	u32  opc = OPC_INB_SATA_HOST_OPSTART;
> >  
> >  	memset(&sata_cmd, 0, sizeof(sata_cmd));
> > @@ -4124,8 +4124,10 @@ 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);
> > +	retfis = task->ata_task.return_fis_on_success;
> 
> While I think this should be OK, I think it would be safer to do:
> 
> 	u32 dir, retfis = 0;
> 
> 	...
> 
> 	if (task->ata_task.return_fis_on_success)
> 		retfis = 1;
> 
> to avoid issues with funky compilers doing some tricky handling of single bit
> fields.
> 
> > +	sata_cmd.retfis_ncqtag_atap_dir_m =
> > +		cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> > +				((ATAP & 0x3f) << 10) | dir);
> 
> Please align this line with "(retfis << 24)" above.

Thanks Damien! I'll update the code in v2.
 
> >  	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;
> 
> Naming this field from what is set in it is unusual... Not sure how the
> controller spce named this field, but we should use that and stop changing it's
> name whenever we change the bits that are set.

I see this naming as "what can be set" rather than "what is set" (yes, retfis
wasn't there for some reason). These four bytes are assentially a bitfield.

While we can change this to a bitfield I would like to keep the current single
filed as there are other fields that follow the same naming pattern
e.g. ase_sh_lm_slr_phyid, phyid_npip_portstate, phyid_portid, etc.

> 
> >  	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..e839fb53f0e3 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;
> >  	u32 opc = OPC_INB_SATA_HOST_OPSTART;
> >  	memset(&sata_cmd, 0, sizeof(sata_cmd));
> >  
> > @@ -4487,6 +4487,7 @@ 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);
> > +	retfis = task->ata_task.return_fis_on_success;
> >  
> >  	sata_cmd.sata_fis = task->ata_task.fis;
> >  	if (likely(!task->ata_task.device_control_reg_update))
> > @@ -4502,8 +4503,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> >  		opc = OPC_INB_SATA_DIF_ENC_IO;
> >  
> >  		/* set encryption bit */
> > -		sata_cmd.ncqtag_atap_dir_m_dad =
> > -			cpu_to_le32(((ncg_tag & 0xff)<<16)|
> > +		sata_cmd.retfis_ncqtag_atap_dir_m_dad =
> > +			cpu_to_le32((retfis << 24) | ((ncg_tag & 0xff) << 16) |
> >  				((ATAP & 0x3f) << 10) | 0x20 | dir);
> 
> Same comments here.
> 
> >  							/* dad (bit 0-1) is 0 */
> >  		/* fill in PRD (scatter/gather) table, if any */
> > @@ -4569,8 +4570,8 @@ static int pm80xx_chip_sata_req(struct pm8001_hba_info *pm8001_ha,
> >  			   "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) |
> > +		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 */
> > 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. */
> 
> -- 
> Damien Le Moal
> Western Digital Research

Thank you,
Igor