On 01/11/2021 20:35, Bart Van Assche wrote:
> On 11/1/21 2:16 AM, Adrian Hunter wrote:
>> On 29/10/2021 19:31, Bart Van Assche wrote:
>>> @@ -5985,9 +5920,12 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
>>> ufshcd_clk_scaling_allow(hba, false);
>>> }
>>> ufshcd_scsi_block_requests(hba);
>>> - /* Drain ufshcd_queuecommand() */
>>> - down_write(&hba->clk_scaling_lock);
>>> - up_write(&hba->clk_scaling_lock);
>>> + /*
>>> + * Drain ufshcd_queuecommand(). Since ufshcd_queuecommand() does not
>>> + * sleep, calling synchronize_rcu() is sufficient to wait for ongoing
>>> + * ufshcd_queuecommand calls.
>>> + */
>>> + synchronize_rcu();
>>
>> This depends upon block layer internals, so it must be called via a block
>> layer function i.e. the block layer must guarantee this will always work. > Also, scsi_unjam_host() does not look like it uses RCU when issuing
>> requests.
>
> I will add an rcu_read_lock() / rcu_read_unlock() pair in ufshcd_queuecommand().
> I think that addresses both points mentioned above.
>
>>> cancel_work_sync(&hba->eeh_work);
>>> }
>>>
>>> @@ -6212,11 +6150,8 @@ static void ufshcd_err_handler(struct work_struct *work)
>>> */
>>> if (needs_restore) {
>>> spin_unlock_irqrestore(hba->host->host_lock, flags);
>>> - /*
>>> - * Hold the scaling lock just in case dev cmds
>>> - * are sent via bsg and/or sysfs.
>>> - */
>>> - down_write(&hba->clk_scaling_lock);
>>> + /* Block TMF submission, e.g. through BSG or sysfs. */
>>
>> What about dev cmds?
>
> ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd() both allocate a
> request from hba->cmd_queue. blk_get_request() indirectly increments
> q->q_usage_counter and blk_mq_freeze_queue() waits until that counter drops
> to zero. Hence, hba->cmd_queue freezing only finishes after all pending dev
> cmds have finished. Additionally, if hba->cmd_queue is frozen,
> blk_get_request() blocks until it is unfrozen. So dev cmds are covered.
The queue is not usually frozen when the error handler runs.
>
>> Also, there is the outstanding question of synchronization for the call to
>> ufshcd_reset_and_restore() further down.
>
> Please clarify this comment further. I assume that you are referring to the
> ufshcd_reset_and_restore() call guarded by if (needs_reset)? It is not clear
> to me what the outstanding question is?
What is to stop it racing with BSG, sysfs, debugfs, abort, reset etc?
>
> Thanks,
>
> Bart.
