| Message ID | 20220217132956.484818-23-damien.lemoal@opensource.wdc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libsas and pm8001 fixes | expand |
On 17/02/2022 13:29, Damien Le Moal wrote: > In pm8001_chip_set_dev_state_req(), pm8001_chip_fw_flash_update_req() > and pm8001_chip_reg_dev_req() add missing calls to pm8001_tag_free() to > free the allocated tag when pm8001_mpi_build_cmd() fails. > > Similarly, in pm8001_exec_internal_task_abort(), if the chip > ->task_abort method fails, the tag allocated for the abort request task > must be freed. Add the missing call to pm8001_tag_free(). Also remove > the useless ex_err label and use "break" instead of "goto" statements > in the retry loop. > > Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> This looks ok, but I think pm80xx_chip_phy_ctl_req() might be missed. Apart from that and a nit, below: Reviewed-by: John Garry <john.garry@huawei.com> > --- > drivers/scsi/pm8001/pm8001_hwi.c | 9 +++++++++ > drivers/scsi/pm8001/pm8001_sas.c | 33 +++++++++++++++++--------------- > 2 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c > index cc96e58454c8..431fc9160637 100644 > --- a/drivers/scsi/pm8001/pm8001_hwi.c > +++ b/drivers/scsi/pm8001/pm8001_hwi.c > @@ -4458,6 +4458,9 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha, > SAS_ADDR_SIZE); > rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, > sizeof(payload), 0); > + if (rc) > + pm8001_tag_free(pm8001_ha, tag); > + > return rc; > } > > @@ -4870,6 +4873,9 @@ pm8001_chip_fw_flash_update_req(struct pm8001_hba_info *pm8001_ha, > ccb->ccb_tag = tag; > rc = pm8001_chip_fw_flash_update_build(pm8001_ha, &flash_update_info, > tag); > + if (rc) > + pm8001_tag_free(pm8001_ha, tag); > + > return rc; > } > > @@ -4974,6 +4980,9 @@ pm8001_chip_set_dev_state_req(struct pm8001_hba_info *pm8001_ha, > payload.nds = cpu_to_le32(state); > rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, > sizeof(payload), 0); > + if (rc) > + pm8001_tag_free(pm8001_ha, tag); > + > return rc; > > } > diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c > index d0f5feb4f2d3..0440777a9135 100644 > --- a/drivers/scsi/pm8001/pm8001_sas.c > +++ b/drivers/scsi/pm8001/pm8001_sas.c > @@ -834,7 +834,8 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, > > res = pm8001_tag_alloc(pm8001_ha, &ccb_tag); > if (res) > - goto ex_err; > + break; > + > ccb = &pm8001_ha->ccb_info[ccb_tag]; > ccb->device = pm8001_dev; > ccb->ccb_tag = ccb_tag; > @@ -843,36 +844,38 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, > > res = PM8001_CHIP_DISP->task_abort(pm8001_ha, > pm8001_dev, flag, task_tag, ccb_tag); > - > if (res) { > del_timer(&task->slow_task->timer); > - pm8001_dbg(pm8001_ha, FAIL, "Executing internal task failed\n"); > - goto ex_err; > + pm8001_dbg(pm8001_ha, FAIL, > + "Executing internal task failed\n"); > + pm8001_tag_free(pm8001_ha, ccb_tag); > + break; > } > + > wait_for_completion(&task->slow_task->completion); > res = TMF_RESP_FUNC_FAILED; > + > /* Even TMF timed out, return direct. */ > if (task->task_state_flags & SAS_TASK_STATE_ABORTED) { > pm8001_dbg(pm8001_ha, FAIL, "TMF task timeout.\n"); > - goto ex_err; > + break; > } nit: these are really separate changes, but this series is so long already :) > > if (task->task_status.resp == SAS_TASK_COMPLETE && > task->task_status.stat == SAS_SAM_STAT_GOOD) { > res = TMF_RESP_FUNC_COMPLETE; > break; > - > - } else { > - pm8001_dbg(pm8001_ha, EH, > - " Task to dev %016llx response: 0x%x status 0x%x\n", > - SAS_ADDR(dev->sas_addr), > - task->task_status.resp, > - task->task_status.stat); > - sas_free_task(task); > - task = NULL; > } > + > + pm8001_dbg(pm8001_ha, EH, > + " Task to dev %016llx response: 0x%x status 0x%x\n", > + SAS_ADDR(dev->sas_addr), > + task->task_status.resp, > + task->task_status.stat); > + sas_free_task(task); > + task = NULL; > } > -ex_err: > + > BUG_ON(retry == 3 && task != NULL); > sas_free_task(task); > return res;
On 2/18/22 02:41, John Garry wrote: > On 17/02/2022 13:29, Damien Le Moal wrote: >> In pm8001_chip_set_dev_state_req(), pm8001_chip_fw_flash_update_req() >> and pm8001_chip_reg_dev_req() add missing calls to pm8001_tag_free() to >> free the allocated tag when pm8001_mpi_build_cmd() fails. >> >> Similarly, in pm8001_exec_internal_task_abort(), if the chip >> ->task_abort method fails, the tag allocated for the abort request task >> must be freed. Add the missing call to pm8001_tag_free(). Also remove >> the useless ex_err label and use "break" instead of "goto" statements >> in the retry loop. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > > This looks ok, but I think pm80xx_chip_phy_ctl_req() might be missed. It is fixed separately in patch 18. I should have squashed it into this one. forgot :) > > Apart from that and a nit, below: > > Reviewed-by: John Garry <john.garry@huawei.com> > >> --- >> drivers/scsi/pm8001/pm8001_hwi.c | 9 +++++++++ >> drivers/scsi/pm8001/pm8001_sas.c | 33 +++++++++++++++++--------------- >> 2 files changed, 27 insertions(+), 15 deletions(-) >> >> diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c >> index cc96e58454c8..431fc9160637 100644 >> --- a/drivers/scsi/pm8001/pm8001_hwi.c >> +++ b/drivers/scsi/pm8001/pm8001_hwi.c >> @@ -4458,6 +4458,9 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha, >> SAS_ADDR_SIZE); >> rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, >> sizeof(payload), 0); >> + if (rc) >> + pm8001_tag_free(pm8001_ha, tag); >> + >> return rc; >> } >> >> @@ -4870,6 +4873,9 @@ pm8001_chip_fw_flash_update_req(struct pm8001_hba_info *pm8001_ha, >> ccb->ccb_tag = tag; >> rc = pm8001_chip_fw_flash_update_build(pm8001_ha, &flash_update_info, >> tag); >> + if (rc) >> + pm8001_tag_free(pm8001_ha, tag); >> + >> return rc; >> } >> >> @@ -4974,6 +4980,9 @@ pm8001_chip_set_dev_state_req(struct pm8001_hba_info *pm8001_ha, >> payload.nds = cpu_to_le32(state); >> rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, >> sizeof(payload), 0); >> + if (rc) >> + pm8001_tag_free(pm8001_ha, tag); >> + >> return rc; >> >> } >> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c >> index d0f5feb4f2d3..0440777a9135 100644 >> --- a/drivers/scsi/pm8001/pm8001_sas.c >> +++ b/drivers/scsi/pm8001/pm8001_sas.c >> @@ -834,7 +834,8 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, >> >> res = pm8001_tag_alloc(pm8001_ha, &ccb_tag); >> if (res) >> - goto ex_err; >> + break; >> + >> ccb = &pm8001_ha->ccb_info[ccb_tag]; >> ccb->device = pm8001_dev; >> ccb->ccb_tag = ccb_tag; >> @@ -843,36 +844,38 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, >> >> res = PM8001_CHIP_DISP->task_abort(pm8001_ha, >> pm8001_dev, flag, task_tag, ccb_tag); >> - >> if (res) { >> del_timer(&task->slow_task->timer); >> - pm8001_dbg(pm8001_ha, FAIL, "Executing internal task failed\n"); >> - goto ex_err; >> + pm8001_dbg(pm8001_ha, FAIL, >> + "Executing internal task failed\n"); >> + pm8001_tag_free(pm8001_ha, ccb_tag); >> + break; >> } >> + >> wait_for_completion(&task->slow_task->completion); >> res = TMF_RESP_FUNC_FAILED; >> + >> /* Even TMF timed out, return direct. */ >> if (task->task_state_flags & SAS_TASK_STATE_ABORTED) { >> pm8001_dbg(pm8001_ha, FAIL, "TMF task timeout.\n"); >> - goto ex_err; >> + break; >> } > > nit: these are really separate changes, but this series is so long > already :) > >> >> if (task->task_status.resp == SAS_TASK_COMPLETE && >> task->task_status.stat == SAS_SAM_STAT_GOOD) { >> res = TMF_RESP_FUNC_COMPLETE; >> break; >> - >> - } else { >> - pm8001_dbg(pm8001_ha, EH, >> - " Task to dev %016llx response: 0x%x status 0x%x\n", >> - SAS_ADDR(dev->sas_addr), >> - task->task_status.resp, >> - task->task_status.stat); >> - sas_free_task(task); >> - task = NULL; >> } >> + >> + pm8001_dbg(pm8001_ha, EH, >> + " Task to dev %016llx response: 0x%x status 0x%x\n", >> + SAS_ADDR(dev->sas_addr), >> + task->task_status.resp, >> + task->task_status.stat); >> + sas_free_task(task); >> + task = NULL; >> } >> -ex_err: >> + >> BUG_ON(retry == 3 && task != NULL); >> sas_free_task(task); >> return res; >
On 2/18/22 02:41, John Garry wrote: > On 17/02/2022 13:29, Damien Le Moal wrote: >> In pm8001_chip_set_dev_state_req(), pm8001_chip_fw_flash_update_req() >> and pm8001_chip_reg_dev_req() add missing calls to pm8001_tag_free() to >> free the allocated tag when pm8001_mpi_build_cmd() fails. >> >> Similarly, in pm8001_exec_internal_task_abort(), if the chip >> ->task_abort method fails, the tag allocated for the abort request task >> must be freed. Add the missing call to pm8001_tag_free(). Also remove >> the useless ex_err label and use "break" instead of "goto" statements >> in the retry loop. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> > > This looks ok, but I think pm80xx_chip_phy_ctl_req() might be missed. > > Apart from that and a nit, below: [...] >> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c >> index d0f5feb4f2d3..0440777a9135 100644 >> --- a/drivers/scsi/pm8001/pm8001_sas.c >> +++ b/drivers/scsi/pm8001/pm8001_sas.c >> @@ -834,7 +834,8 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, >> >> res = pm8001_tag_alloc(pm8001_ha, &ccb_tag); >> if (res) >> - goto ex_err; >> + break; >> + >> ccb = &pm8001_ha->ccb_info[ccb_tag]; >> ccb->device = pm8001_dev; >> ccb->ccb_tag = ccb_tag; >> @@ -843,36 +844,38 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, >> >> res = PM8001_CHIP_DISP->task_abort(pm8001_ha, >> pm8001_dev, flag, task_tag, ccb_tag); >> - >> if (res) { >> del_timer(&task->slow_task->timer); >> - pm8001_dbg(pm8001_ha, FAIL, "Executing internal task failed\n"); >> - goto ex_err; >> + pm8001_dbg(pm8001_ha, FAIL, >> + "Executing internal task failed\n"); >> + pm8001_tag_free(pm8001_ha, ccb_tag); >> + break; >> } >> + >> wait_for_completion(&task->slow_task->completion); >> res = TMF_RESP_FUNC_FAILED; >> + >> /* Even TMF timed out, return direct. */ >> if (task->task_state_flags & SAS_TASK_STATE_ABORTED) { >> pm8001_dbg(pm8001_ha, FAIL, "TMF task timeout.\n"); >> - goto ex_err; >> + break; >> } > > nit: these are really separate changes, but this series is so long > already :) As it is better to have fixes and cleanups separated, I moved these style changes to another patch and squashed the pm80xx_chip_phy_ctl_req() tag leak patch into this one. So patch count remains the same. Sending v5. All but 2 patches are reviewed now. Thanks !
diff --git a/drivers/scsi/pm8001/pm8001_hwi.c b/drivers/scsi/pm8001/pm8001_hwi.c index cc96e58454c8..431fc9160637 100644 --- a/drivers/scsi/pm8001/pm8001_hwi.c +++ b/drivers/scsi/pm8001/pm8001_hwi.c @@ -4458,6 +4458,9 @@ static int pm8001_chip_reg_dev_req(struct pm8001_hba_info *pm8001_ha, SAS_ADDR_SIZE); rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, sizeof(payload), 0); + if (rc) + pm8001_tag_free(pm8001_ha, tag); + return rc; } @@ -4870,6 +4873,9 @@ pm8001_chip_fw_flash_update_req(struct pm8001_hba_info *pm8001_ha, ccb->ccb_tag = tag; rc = pm8001_chip_fw_flash_update_build(pm8001_ha, &flash_update_info, tag); + if (rc) + pm8001_tag_free(pm8001_ha, tag); + return rc; } @@ -4974,6 +4980,9 @@ pm8001_chip_set_dev_state_req(struct pm8001_hba_info *pm8001_ha, payload.nds = cpu_to_le32(state); rc = pm8001_mpi_build_cmd(pm8001_ha, circularQ, opc, &payload, sizeof(payload), 0); + if (rc) + pm8001_tag_free(pm8001_ha, tag); + return rc; } diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c index d0f5feb4f2d3..0440777a9135 100644 --- a/drivers/scsi/pm8001/pm8001_sas.c +++ b/drivers/scsi/pm8001/pm8001_sas.c @@ -834,7 +834,8 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, res = pm8001_tag_alloc(pm8001_ha, &ccb_tag); if (res) - goto ex_err; + break; + ccb = &pm8001_ha->ccb_info[ccb_tag]; ccb->device = pm8001_dev; ccb->ccb_tag = ccb_tag; @@ -843,36 +844,38 @@ pm8001_exec_internal_task_abort(struct pm8001_hba_info *pm8001_ha, res = PM8001_CHIP_DISP->task_abort(pm8001_ha, pm8001_dev, flag, task_tag, ccb_tag); - if (res) { del_timer(&task->slow_task->timer); - pm8001_dbg(pm8001_ha, FAIL, "Executing internal task failed\n"); - goto ex_err; + pm8001_dbg(pm8001_ha, FAIL, + "Executing internal task failed\n"); + pm8001_tag_free(pm8001_ha, ccb_tag); + break; } + wait_for_completion(&task->slow_task->completion); res = TMF_RESP_FUNC_FAILED; + /* Even TMF timed out, return direct. */ if (task->task_state_flags & SAS_TASK_STATE_ABORTED) { pm8001_dbg(pm8001_ha, FAIL, "TMF task timeout.\n"); - goto ex_err; + break; } if (task->task_status.resp == SAS_TASK_COMPLETE && task->task_status.stat == SAS_SAM_STAT_GOOD) { res = TMF_RESP_FUNC_COMPLETE; break; - - } else { - pm8001_dbg(pm8001_ha, EH, - " Task to dev %016llx response: 0x%x status 0x%x\n", - SAS_ADDR(dev->sas_addr), - task->task_status.resp, - task->task_status.stat); - sas_free_task(task); - task = NULL; } + + pm8001_dbg(pm8001_ha, EH, + " Task to dev %016llx response: 0x%x status 0x%x\n", + SAS_ADDR(dev->sas_addr), + task->task_status.resp, + task->task_status.stat); + sas_free_task(task); + task = NULL; } -ex_err: + BUG_ON(retry == 3 && task != NULL); sas_free_task(task); return res;
In pm8001_chip_set_dev_state_req(), pm8001_chip_fw_flash_update_req() and pm8001_chip_reg_dev_req() add missing calls to pm8001_tag_free() to free the allocated tag when pm8001_mpi_build_cmd() fails. Similarly, in pm8001_exec_internal_task_abort(), if the chip ->task_abort method fails, the tag allocated for the abort request task must be freed. Add the missing call to pm8001_tag_free(). Also remove the useless ex_err label and use "break" instead of "goto" statements in the retry loop. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/scsi/pm8001/pm8001_hwi.c | 9 +++++++++ drivers/scsi/pm8001/pm8001_sas.c | 33 +++++++++++++++++--------------- 2 files changed, 27 insertions(+), 15 deletions(-)