Re: [PATCH RFC] scsi: ufs-core: Do not use clk_scaling_lock in ufshcd_queuecommand()

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

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.