Message ID | 20220217132956.484818-29-damien.lemoal@opensource.wdc.com |
---|---|
State | Superseded |
Headers | show |
Series | libsas and pm8001 fixes | expand |
On Thu, Feb 17, 2022 at 2:30 PM Damien Le Moal <damien.lemoal@opensource.wdc.com> 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. The unnecessary local variable t is also removed. > > Additionally, avoid repeatedly declaring "struct task_status_struct *ts" > to handle error cases by declaring this variable for the entire function > scope. This allows simplifying the error cases, and together with the > addition of blank lines make the code more readable. > > Finally, 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> > Reviewed-by: John Garry <john.garry@huawei.com> Reviewed-by: Jack Wang <jinpu.wang@ionos.com> > --- > drivers/scsi/pm8001/pm8001_sas.c | 174 ++++++++++++++----------------- > 1 file changed, 80 insertions(+), 94 deletions(-) > > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c > index 52507bc8f963..37aba0335f18 100644 > --- a/drivers/scsi/pm8001/pm8001_sas.c > +++ b/drivers/scsi/pm8001/pm8001_sas.c > @@ -373,129 +373,115 @@ 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 task_status_struct *ts = &task->task_status; > + enum sas_protocol task_proto = task->task_proto; > 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 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) { > - struct task_status_struct *tsm = &t->task_status; > - tsm->resp = SAS_TASK_UNDELIVERED; > - tsm->stat = SAS_PHY_DOWN; > + 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); > - if (pm8001_ha->controller_fatal_error) { > - struct task_status_struct *ts = &t->task_status; > > + 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; > } > + > pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec device\n"); > + > 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)]; > - if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) { > - if (sas_protocol_ata(task_proto)) { > - struct task_status_struct *ts = &t->task_status; > - ts->resp = SAS_TASK_UNDELIVERED; > - ts->stat = SAS_PHY_DOWN; > > - spin_unlock_irqrestore(&pm8001_ha->lock, flags); > - t->task_done(t); > - spin_lock_irqsave(&pm8001_ha->lock, flags); > - continue; > - } else { > - struct task_status_struct *ts = &t->task_status; > - ts->resp = SAS_TASK_UNDELIVERED; > - ts->stat = SAS_PHY_DOWN; > - t->task_done(t); > - continue; > - } > - } > + pm8001_dev = dev->lldd_dev; > + port = &pm8001_ha->port[sas_find_local_port_id(dev)]; > > - ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_dev, t); > - if (!ccb) { > - rc = -SAS_QUEUE_FULL; > - goto err_out; > + 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); > } > + rc = -ENODEV; > + goto err_out; > + } > > - 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; > - } > + ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_dev, task); > + if (!ccb) { > + rc = -SAS_QUEUE_FULL; > + goto err_out; > + } > + > + 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 err_out_ccb; > } > - } 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; > + } > > + 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); > err_out_ccb: > - pm8001_ccb_free(pm8001_ha, 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: > + pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec failed[%d]!\n", rc); > + } > + > spin_unlock_irqrestore(&pm8001_ha->lock, flags); > + > return rc; > } > > -- > 2.34.1 >
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c index 52507bc8f963..37aba0335f18 100644 --- a/drivers/scsi/pm8001/pm8001_sas.c +++ b/drivers/scsi/pm8001/pm8001_sas.c @@ -373,129 +373,115 @@ 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 task_status_struct *ts = &task->task_status; + enum sas_protocol task_proto = task->task_proto; 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 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) { - struct task_status_struct *tsm = &t->task_status; - tsm->resp = SAS_TASK_UNDELIVERED; - tsm->stat = SAS_PHY_DOWN; + 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); - if (pm8001_ha->controller_fatal_error) { - struct task_status_struct *ts = &t->task_status; + 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; } + pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec device\n"); + 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)]; - if (DEV_IS_GONE(pm8001_dev) || !port->port_attached) { - if (sas_protocol_ata(task_proto)) { - struct task_status_struct *ts = &t->task_status; - ts->resp = SAS_TASK_UNDELIVERED; - ts->stat = SAS_PHY_DOWN; - spin_unlock_irqrestore(&pm8001_ha->lock, flags); - t->task_done(t); - spin_lock_irqsave(&pm8001_ha->lock, flags); - continue; - } else { - struct task_status_struct *ts = &t->task_status; - ts->resp = SAS_TASK_UNDELIVERED; - ts->stat = SAS_PHY_DOWN; - t->task_done(t); - continue; - } - } + pm8001_dev = dev->lldd_dev; + port = &pm8001_ha->port[sas_find_local_port_id(dev)]; - ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_dev, t); - if (!ccb) { - rc = -SAS_QUEUE_FULL; - goto err_out; + 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); } + rc = -ENODEV; + goto err_out; + } - 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; - } + ccb = pm8001_ccb_alloc(pm8001_ha, pm8001_dev, task); + if (!ccb) { + rc = -SAS_QUEUE_FULL; + goto err_out; + } + + 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 err_out_ccb; } - } 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; + } + 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); err_out_ccb: - pm8001_ccb_free(pm8001_ha, 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: + pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec failed[%d]!\n", rc); + } + spin_unlock_irqrestore(&pm8001_ha->lock, flags); + return rc; }