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

On 11/1/21 2:16 AM, Adrian Hunter wrote:

On 29/10/2021 19:31, Bart Van Assche wrote:

quoted

@@ -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.

quoted

      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.