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

[John Garry <john.garry@huawei.com>]

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