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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxx>

Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxxxxxxxxxxxxx>
---
  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;
  }




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]

  Powered by Linux