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;
}
