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@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxx>
>
>> ---
>> 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;
>
--
Damien Le Moal
Western Digital Research
