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

[Igor Pylypiv <ipylypiv@google.com>]

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