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.
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?
Thanks,
Bart.
