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.
