Re: [PATCH v2 8/8] scsi: ufs: Fix a deadlock between PM and the SCSI error handler

From: Bart Van Assche <bvanassche@acm.org>
To: Asutosh Das <quic_asutoshd@quicinc.com>
Cc: "Martin K . Petersen" <martin.petersen@oracle.com>,
	Jaegeuk Kim <jaegeuk@kernel.org>,
	linux-scsi@vger.kernel.org,
	Adrian Hunter <adrian.hunter@intel.com>,
	"James E.J. Bottomley" <jejb@linux.ibm.com>,
	Bean Huo <beanhuo@micron.com>, Avri Altman <avri.altman@wdc.com>,
	Jinyoung Choi <j-young.choi@samsung.com>
Subject: Re: [PATCH v2 8/8] scsi: ufs: Fix a deadlock between PM and the SCSI error handler
Date: Wed, 28 Sep 2022 16:09:41 -0700	[thread overview]
Message-ID: <8261e86c-4e0d-0ec0-e0fb-0552964f6c5b@acm.org> (raw)
In-Reply-To: <20220927193012.GE15228@asutoshd-linux1.qualcomm.com>

On 9/27/22 12:30, Asutosh Das wrote:
> On Tue, Sep 27 2022 at 11:45 -0700, Bart Van Assche wrote:
>> +static enum scsi_timeout_action ufshcd_eh_timed_out(struct scsi_cmnd *scmd)
>> +{
>> +    struct ufs_hba *hba = shost_priv(scmd->device->host);
>> +    bool reset_controller = false;
>> +    int tag, ret;
>> +
>> +    if (!hba->system_suspending) {
>> +        /* Activate the error handler in the SCSI core. */
>> +        return SCSI_EH_NOT_HANDLED;
>> +    }
>> +
>> +    /*
>> +     * Handle errors directly to prevent a deadlock between
>> +     * ufshcd_set_dev_pwr_mode() and ufshcd_err_handler().
>> +     */
>> +    for_each_set_bit(tag, &hba->outstanding_reqs, hba->nutrs) {
>> +        ret = ufshcd_try_to_abort_task(hba, tag);
>> +        dev_info(hba->dev, "Aborting tag %d / CDB %#02x %s\n", tag,
>> +             hba->lrb[tag].cmd ? hba->lrb[tag].cmd->cmnd[0] : -1,
>> +             ret == 0 ? "succeeded" : "failed");
>> +        if (ret != 0) {
>> +            reset_controller = true;
>> +            break;
>> +        }
>> +    }
>> +    for_each_set_bit(tag, &hba->outstanding_tasks, hba->nutmrs) {
>> +        ret = ufshcd_clear_tm_cmd(hba, tag);
> 
> If reset_controller is true, then the HC would be reset and it would
> anyway clear up all resources. Would this be needed if reset_controller is true?

Probably not.

>> +        dev_info(hba->dev, "Aborting TMF %d %s\n", tag,
>> +             ret == 0 ? "succeeded" : "failed");
>> +        if (ret != 0) {
>> +            reset_controller = true;
>> +            break;
>> +        }
>> +    }
>> +    if (reset_controller) {
>> +        dev_info(hba->dev, "Resetting controller\n");
>> +        ufshcd_reset_and_restore(hba);
>> +        if (ufshcd_clear_cmds(hba, 0xffffffff))
>
> ufshcd_reset_and_restore() would reset the host and the device.
> So is the ufshcd_clear_cmds() needed after that?

I will leave out this ufshcd_clear_cmds() call.

Thanks for the feedback.

Bart.