diff mbox series

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

Message ID 20220214021747.4976-31-damien.lemoal@opensource.wdc.com (mailing list archive)
State Superseded
Headers show
Series libsas and pm8001 fixes | expand

Commit Message

Damien Le Moal Feb. 14, 2022, 2:17 a.m. UTC 
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(-)

Comments

John Garry Feb. 15, 2022, 8:57 a.m. UTC | #1 
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;
>   }
diff mbox series

Patch

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;
 	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:
+	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);
+	}
+
+unlock:
 	spin_unlock_irqrestore(&pm8001_ha->lock, flags);
 	return rc;
 }