[v3,30/31] scsi: pm8001: Simplify pm8001_task_exec()

The main part of the pm8001_task_exec() function uses a do {} while(0)
loop that is useless and only makes the code harder to read. Remove this
loop. Also, the unnecessary local variable t is removed.
Additionnally, handling of the running_req counter is fixed to avoid
decrementing it without a corresponding incrementation in the case of an
invalid task protocol.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/pm8001/pm8001_sas.c | 160 ++++++++++++++-----------------
 1 file changed, 73 insertions(+), 87 deletions(-)

On 14/02/2022 02:17, Damien Le Moal wrote:
> The main part of the pm8001_task_exec() function uses a do {} while(0)
> loop that is useless and only makes the code harder to read. Remove this
> loop. Also, the unnecessary local variable t is removed.
> Additionnally, handling of the running_req counter is fixed to avoid
> decrementing it without a corresponding incrementation in the case of an
> invalid task protocol.
> 

Hi Damien,

I think that this is much improved, but there are still issues with the 
code but they are outside the scope of your change.

Just some minor comments below.

Regardless,

Reviewed-by: John Garry <john.garry@huawei.com>

> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/scsi/pm8001/pm8001_sas.c | 160 ++++++++++++++-----------------
>   1 file changed, 73 insertions(+), 87 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 6c4aa04c9144..980afde2a0ab 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -373,32 +373,32 @@ static int sas_find_local_port_id(struct domain_device *dev)
>     * @is_tmf: if it is task management task.
>     * @tmf: the task management IU
>     */
> -static int pm8001_task_exec(struct sas_task *task,
> -	gfp_t gfp_flags, int is_tmf, struct pm8001_tmf_task *tmf)
> +static int pm8001_task_exec(struct sas_task *task, gfp_t gfp_flags, int is_tmf,
> +			    struct pm8001_tmf_task *tmf)
>   {
>   	struct domain_device *dev = task->dev;
> +	struct pm8001_device *pm8001_dev = dev->lldd_dev;
>   	struct pm8001_hba_info *pm8001_ha;
> -	struct pm8001_device *pm8001_dev;
>   	struct pm8001_port *port = NULL;
> -	struct sas_task *t = task;
> -	struct task_status_struct *ts = &t->task_status;
> +	struct task_status_struct *ts = &task->task_status;
> +	enum sas_protocol task_proto = task->task_proto;

nit: I think that these can be better arranged in reverse fir-tree 
style, but no big deal

>   	struct pm8001_ccb_info *ccb;
> -	u32 rc = 0, n_elem = 0;
> -	unsigned long flags = 0;
> -	enum sas_protocol task_proto = t->task_proto;
> +	unsigned long flags;
> +	u32 n_elem = 0;
> +	int rc = 0;
>   
>   	if (!dev->port) {
>   		ts->resp = SAS_TASK_UNDELIVERED;
>   		ts->stat = SAS_PHY_DOWN;
>   		if (dev->dev_type != SAS_SATA_DEV)
> -			t->task_done(t);
> +			task->task_done(task);
>   		return 0;
>   	}
>   
> -	pm8001_ha = pm8001_find_ha_by_dev(task->dev);
> +	pm8001_ha = pm8001_find_ha_by_dev(dev);
>   	if (pm8001_ha->controller_fatal_error) {
>   		ts->resp = SAS_TASK_UNDELIVERED;
> -		t->task_done(t);
> +		task->task_done(task);
>   		return 0;
>   	}
>   
> @@ -406,92 +406,78 @@ static int pm8001_task_exec(struct sas_task *task,
>   
>   	spin_lock_irqsave(&pm8001_ha->lock, flags);
>   
> -	do {
> -		dev = t->dev;
> -		pm8001_dev = dev->lldd_dev;
> -		port = &pm8001_ha->port[sas_find_local_port_id(dev)];
> +	pm8001_dev = dev->lldd_dev;
> +	port = &pm8001_ha->port[sas_find_local_port_id(dev)];
>   
> -		if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) {
> -			ts->resp = SAS_TASK_UNDELIVERED;
> -			ts->stat = SAS_PHY_DOWN;
> -			if (sas_protocol_ata(task_proto)) {
> -				spin_unlock_irqrestore(&pm8001_ha->lock, flags);
> -				t->task_done(t);
> -				spin_lock_irqsave(&pm8001_ha->lock, flags);
> -			} else {
> -				t->task_done(t);
> -			}
> -			continue;
> +	if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) {
> +		ts->resp = SAS_TASK_UNDELIVERED;
> +		ts->stat = SAS_PHY_DOWN;
> +		if (sas_protocol_ata(task_proto)) {
> +			spin_unlock_irqrestore(&pm8001_ha->lock, flags);
> +			task->task_done(task);
> +			spin_lock_irqsave(&pm8001_ha->lock, flags);
> +		} else {
> +			task->task_done(task);
>   		}
> +		goto unlock;
> +	}
>   
> -		ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_dev, t);
> -		if (!ccb) {
> -			rc = -SAS_QUEUE_FULL;
> -			goto err_out;
> -		}
> +	ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_dev, task);
> +	if (!ccb) {
> +		rc = -SAS_QUEUE_FULL;
> +		goto unlock;
> +	}
>   
> -		if (!sas_protocol_ata(task_proto)) {
> -			if (t->num_scatter) {
> -				n_elem = dma_map_sg(pm8001_ha->dev,
> -					t->scatter,
> -					t->num_scatter,
> -					t->data_dir);
> -				if (!n_elem) {
> -					rc = -ENOMEM;
> -					goto err_out_ccb;
> -				}
> +	if (!sas_protocol_ata(task_proto)) {
> +		if (task->num_scatter) {
> +			n_elem = dma_map_sg(pm8001_ha->dev, task->scatter,
> +					    task->num_scatter, task->data_dir);
> +			if (!n_elem) {
> +				rc = -ENOMEM;
> +				goto ccb_free;
>   			}
> -		} else {
> -			n_elem = t->num_scatter;
>   		}
> +	} else {
> +		n_elem = task->num_scatter;
> +	}
>   
> -		t->lldd_task = ccb;
> -		ccb->n_elem = n_elem;
> +	task->lldd_task = ccb;
> +	ccb->n_elem = n_elem;
>   
> -		switch (task_proto) {
> -		case SAS_PROTOCOL_SMP:
> -			atomic_inc(&pm8001_dev->running_req);
> -			rc = pm8001_task_prep_smp(pm8001_ha, ccb);
> -			break;
> -		case SAS_PROTOCOL_SSP:
> -			atomic_inc(&pm8001_dev->running_req);
> -			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:
> -			atomic_inc(&pm8001_dev->running_req);
> -			rc = pm8001_task_prep_ata(pm8001_ha, ccb);
> -			break;
> -		default:
> -			dev_printk(KERN_ERR, pm8001_ha->dev,
> -				"unknown sas_task proto: 0x%x\n", task_proto);
> -			rc = -EINVAL;
> -			break;
> -		}
> +	atomic_inc(&pm8001_dev->running_req);
>   
> -		if (rc) {
> -			pm8001_dbg(pm8001_ha, IO, "rc is %x\n", rc);
> -			atomic_dec(&pm8001_dev->running_req);
> -			goto err_out_ccb;
> -		}
> -		/* TODO: select normal or high priority */
> -	} while (0);
> -	rc = 0;
> -	goto out_done;
> +	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;
> +	default:
> +		dev_printk(KERN_ERR, pm8001_ha->dev,
> +			   "unknown sas_task proto: 0x%x\n", task_proto);
> +		rc = -EINVAL;
> +		break;
> +	}
>   
> -err_out_ccb:
> -	pm8001_ccb_free(pm8001_ha, ccb);
> -err_out:
> -	dev_printk(KERN_ERR, pm8001_ha->dev, "pm8001 exec failed[%d]!\n", rc);
> -	if (!sas_protocol_ata(task_proto))
> -		if (n_elem)
> -			dma_unmap_sg(pm8001_ha->dev, t->scatter, t->num_scatter,
> -				t->data_dir);
> -out_done:

I know it's a personal preference, but I preferred the old labels; 
however I did not like the goto out_done. I think that the old code 
should just have had 2x spin_unlock_irqrestore + return statements.

> +	if (rc) {
> +		atomic_dec(&pm8001_dev->running_req);
> +		if (!sas_protocol_ata(task_proto) && n_elem)
> +			dma_unmap_sg(pm8001_ha->dev, task->scatter,
> +				     task->num_scatter, task->data_dir);
> +ccb_free:
> +		pm8001_ccb_free(pm8001_ha, ccb);
> +		dev_err(pm8001_ha->dev, "pm8001 exec failed[%d]!\n", rc);

Comment on current code, so feel free to ignore: pm8001_dbg should be 
used always (and improved to be able to do so)

> +	}
> +
> +unlock:
>   	spin_unlock_irqrestore(&pm8001_ha->lock, flags);
>   	return rc;
>   }