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.
>
> 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 !
--
Damien Le Moal
Western Digital Research
