Re: [PATCH 3/4] scsi: pm8001: Use libsas internal abort support

[John Garry]

On 03/03/2022 16:36, Damien Le Moal wrote:
> > -	rc = send_task_abort(pm8001_ha, opc, device_id, flag,
> > -		task_tag, cmd_tag);
> > +	rc = send_task_abort(pm8001_ha, opc, device_id, abort->type,
> > +		abort->tag, ccb->ccb_tag);
> Nit: Can you indent this together with "(" ? I find it easier to read:)

ok, I can align it.

> >   	if (rc != TMF_RESP_FUNC_COMPLETE)
> >   		pm8001_dbg(pm8001_ha, EH, "rc= %d\n", rc);
> >   	return rc;
> > diff --git a/drivers/scsi/pm8001/pm8001_hwi.h b/drivers/scsi/pm8001/pm8001_hwi.h
> > index d1f3aa93325b..961d0465b923 100644
> > --- a/drivers/scsi/pm8001/pm8001_hwi.h
> > +++ b/drivers/scsi/pm8001/pm8001_hwi.h
> > @@ -434,11 +434,6 @@ struct task_abort_req {
> >   	u32	reserved[11];
> >   } __attribute__((packed, aligned(4)));
> >   
> > -/* These flags used for SSP SMP & SATA Abort */
> > -#define ABORT_MASK		0x3
> > -#define ABORT_SINGLE		0x0
> > -#define ABORT_ALL		0x1
> > -
> >   /**
> >    * brief the data structure of SSP SATA SMP Abort Response
> >    * use to describe SSP SMP & SATA Abort Response ( 64 bytes)
> > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> > index ac9c40c95070..d1224674173e 100644
> > --- a/drivers/scsi/pm8001/pm8001_sas.c
> > +++ b/drivers/scsi/pm8001/pm8001_sas.c
> > @@ -324,6 +324,18 @@ static int pm8001_task_prep_ata(struct pm8001_hba_info *pm8001_ha,
> >   	return PM8001_CHIP_DISP->sata_req(pm8001_ha, ccb);
> >   }
> >   
> > +/**
> > +  * pm8001_task_prep_internal_abort - the dispatcher function, prepare data
> > +  *				      for internal abort task
> > +  * @pm8001_ha: our hba card information
> > +  * @ccb: the ccb which attached to sata task
> > +  */
> > +static int pm8001_task_prep_internal_abort(struct pm8001_hba_info *pm8001_ha,
> > +					   struct pm8001_ccb_info *ccb)
> > +{
> > +	return PM8001_CHIP_DISP->task_abort(pm8001_ha, ccb);
> > +}
> > +
> >   /**
> >     * pm8001_task_prep_ssp_tm - the dispatcher function, prepare task management data
> >     * @pm8001_ha: our hba card information
> > @@ -367,6 +379,43 @@ static int sas_find_local_port_id(struct domain_device *dev)
> >   #define DEV_IS_GONE(pm8001_dev)	\
> >   	((!pm8001_dev || (pm8001_dev->dev_type == SAS_PHY_UNUSED)))
> >   
> > +
> > +static int pm8001_deliver_command(struct pm8001_hba_info *pm8001_ha,
> > +				  struct pm8001_ccb_info *ccb)
> > +{
> > +	struct sas_task *task = ccb->task;
> > +	enum sas_protocol task_proto = task->task_proto;
> > +	struct sas_tmf_task *tmf = task->tmf;
> > +	int is_tmf = !!tmf;
> > +	int rc;
> > +
> > +	switch (task_proto) {
> > +	case SAS_PROTOCOL_SMP:
> > +		rc = pm8001_task_prep_smp(pm8001_ha, ccb);
> > +		break;
> > +	case SAS_PROTOCOL_SSP:
> > +		if (is_tmf)
> > +			rc = pm8001_task_prep_ssp_tm(pm8001_ha, ccb, tmf);
> > +		else
> > +			rc = pm8001_task_prep_ssp(pm8001_ha, ccb);
> > +		break;
> > +	case SAS_PROTOCOL_SATA:
> > +	case SAS_PROTOCOL_STP:
> > +		rc = pm8001_task_prep_ata(pm8001_ha, ccb);
> > +		break;
> > +	case SAS_PROTOCOL_INTERNAL_ABORT:
> > +		rc = pm8001_task_prep_internal_abort(pm8001_ha, ccb);
> > +		break;
> > +	default:
> > +		dev_err(pm8001_ha->dev, "unknown sas_task proto: 0x%x\n",
> > +			task_proto);
> > +		rc = -EINVAL;
> > +		break;
> > +	}
> > +
> > +	return rc;
> rc variable is not very useful here. Why not use return directly for each case ?


ok, I can make that change.

> > +}
> > +
> >   /**
> >     * pm8001_queue_command - register for upper layer used, all IO commands sent
> >     * to HBA are from this interface.
> > @@ -379,16 +428,15 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
> >   	enum sas_protocol task_proto = task->task_proto;
> >   	struct domain_device *dev = task->dev;
> >   	struct pm8001_device *pm8001_dev = dev->lldd_dev;
> > +	bool internal_abort = sas_is_internal_abort(task);
> >   	struct pm8001_hba_info *pm8001_ha;
> >   	struct pm8001_port *port = NULL;
> >   	struct pm8001_ccb_info *ccb;
> > -	struct sas_tmf_task *tmf = task->tmf;
> > -	int is_tmf = !!task->tmf;
> >   	unsigned long flags;
> >   	u32 n_elem = 0;
> >   	int rc = 0;
> >   
> > -	if (!dev->port) {
> > +	if (!internal_abort && !dev->port) {
> >   		ts->resp = SAS_TASK_UNDELIVERED;
> >   		ts->stat = SAS_PHY_DOWN;
> >   		if (dev->dev_type != SAS_SATA_DEV)
> > @@ -410,7 +458,8 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
> >   	pm8001_dev = dev->lldd_dev;
> >   	port = &pm8001_ha->port[sas_find_local_port_id(dev)];
> >   
> > -	if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) {
> > +	if (!internal_abort && (DEV_IS_GONE(pm8001_dev) ||
> > +				!port->port_attached)) {
> Wrapping the line after "&&" would make this a lot cleaner and easier to read.

Agreed, I can do it.

But if you can see any neater ways to skip these checks for internal 
abort in the common queue command path then I would be glad to hear it.

> >   		ts->resp = SAS_TASK_UNDELIVERED;
> >   		ts->stat = SAS_PHY_DOWN;
> >   		if (sas_protocol_ata(task_proto)) {
> > @@ -448,27 +497,7 @@ int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
> >   
Thanks,
John