diff mbox series

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

Message ID 20230817214137.462044-3-ipylypiv@google.com (mailing list archive)
State Superseded
Headers show
Series [1/3] ata: libata: Introduce ata_qc_has_cdl() | expand

Commit Message

Igor Pylypiv Aug. 17, 2023, 9:41 p.m. UTC 
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(-)

Comments

Damien Le Moal Aug. 17, 2023, 11:12 p.m. UTC | #1 
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. */
Igor Pylypiv Aug. 18, 2023, 10:45 p.m. UTC | #2 
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
diff mbox series

Patch

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. */