Re: [PATCH 5/5] scsi: ufs: Add a sysfs attribute for triggering the UFS EH

[Adrian Hunter <adrian.hunter@intel.com>]

On 13/10/2021 19:56, Bart Van Assche wrote:
> On 10/13/21 1:09 AM, Adrian Hunter wrote:
>> On 13/10/2021 00:54, Bart Van Assche wrote:
>>> Make it possible to test the impact of the UFS error handler on software
>>> that submits SCSI commands to the UFS driver.
>>
>> Are you sure this isn't better suited to debugfs?
> 
> I will convert this attribute into a debugfs attribute.
> 
>>> +    if (ufshcd_eh_in_progress(hba))
>>> +        return -EBUSY;
>>
>> Does it matter if ufshcd_eh_in_progress()?
> 
> The UFS error handler modifies hba->ufshcd_state and assumes that no other code modifies hba->ufshcd_state while the error handler is in progress. Hence this check.

No the error handler takes care not to overwrite ufshcd_state changes
made by the interrupt handler, by always checking if the state is
UFSHCD_STATE_RESET before changing it to UFSHCD_STATE_OPERATIONAL.

Setting saved_err/saved_uic_err and calling ufshcd_schedule_eh_work()
all under spinlock, would be just like an error interrupt, which can
happen while the error handler is running.

> 
>>> +    } else {
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    scsi_schedule_eh(hba->host);
>>
>> Probably should be:
>>
>>     queue_work(hba->eh_wq, &hba->eh_work);
> 
> No. This patch is intended for Martin Petersen's 5.16/scsi-queue branch. The patch "Revert "scsi: ufs: Synchronize SCSI and UFS error handling"" has been queued on the 5.15/scsi-fixes branch only.
> 
>> However, it might be simpler to replace everything with:
>>
>>     spin_lock(hba->host->host_lock);
>>     hba->saved_err |= <something>;
>>     hba->saved_uic_err |= <something else>;
>>     ufshcd_schedule_eh_work(hba);
>>     spin_unlock(hba->host->host_lock);
>>
>> Perhaps letting the user specify values to determine <something>
>> and <something else>
> 
> I will look into this.
> 
> Thanks,
> 
> Bart.