On 02/11/2021 22:49, Bart Van Assche wrote:
> On 11/1/21 11:11 PM, Adrian Hunter wrote:
>> On 01/11/2021 20:35, Bart Van Assche wrote:
>>> On 11/1/21 2:16 AM, Adrian Hunter wrote:
>>>> 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?
>
> The patch below should address all feedback that has been provided so far.
> Instead of removing the clock scaling lock entirely, it is only removed from
> ufshcd_queuecommand().
>
>
> Subject: [PATCH] scsi: ufs: Remove the clock scaling lock from the command queueing code
>
> Remove the clock scaling lock from ufshcd_queuecommand() since it is a
> performance bottleneck. Freeze request queues from inside
> ufshcd_clock_scaling_prepare() to wait for ongoing SCSI and dev cmds.
> Insert a rcu_read_lock() / rcu_read_unlock() pair in ufshcd_queuecommand().
> Use synchronize_rcu() to wait for ongoing ufshcd_queuecommand() calls.
>
> Signed-off-by: Bart Van Assche <bvanassche@xxxxxxx>
> ---
> drivers/scsi/ufs/ufshcd.c | 97 ++++++++++-----------------------------
> drivers/scsi/ufs/ufshcd.h | 1 +
> 2 files changed, 26 insertions(+), 72 deletions(-)
>
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 35a1af70f705..9d964b979aa2 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -1069,65 +1069,6 @@ static bool ufshcd_is_devfreq_scaling_required(struct ufs_hba *hba,
> return false;
> }
>
> -static int ufshcd_wait_for_doorbell_clr(struct ufs_hba *hba,
> - u64 wait_timeout_us)
> -{
> - unsigned long flags;
> - int ret = 0;
> - u32 tm_doorbell;
> - u32 tr_doorbell;
> - bool timeout = false, do_last_check = false;
> - ktime_t start;
> -
> - ufshcd_hold(hba, false);
> - spin_lock_irqsave(hba->host->host_lock, flags);
> - /*
> - * Wait for all the outstanding tasks/transfer requests.
> - * Verify by checking the doorbell registers are clear.
> - */
> - start = ktime_get();
> - do {
> - if (hba->ufshcd_state != UFSHCD_STATE_OPERATIONAL) {
> - ret = -EBUSY;
> - goto out;
> - }
> -
> - tm_doorbell = ufshcd_readl(hba, REG_UTP_TASK_REQ_DOOR_BELL);
> - tr_doorbell = ufshcd_readl(hba, REG_UTP_TRANSFER_REQ_DOOR_BELL);
> - if (!tm_doorbell && !tr_doorbell) {
> - timeout = false;
> - break;
> - } else if (do_last_check) {
> - break;
> - }
> -
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> - schedule();
> - if (ktime_to_us(ktime_sub(ktime_get(), start)) >
> - wait_timeout_us) {
> - timeout = true;
> - /*
> - * We might have scheduled out for long time so make
> - * sure to check if doorbells are cleared by this time
> - * or not.
> - */
> - do_last_check = true;
> - }
> - spin_lock_irqsave(hba->host->host_lock, flags);
> - } while (tm_doorbell || tr_doorbell);
> -
> - if (timeout) {
> - dev_err(hba->dev,
> - "%s: timedout waiting for doorbell to clear (tm=0x%x, tr=0x%x)\n",
> - __func__, tm_doorbell, tr_doorbell);
> - ret = -EBUSY;
> - }
> -out:
> - spin_unlock_irqrestore(hba->host->host_lock, flags);
> - ufshcd_release(hba);
> - return ret;
> -}
> -
> /**
> * ufshcd_scale_gear - scale up/down UFS gear
> * @hba: per adapter instance
> @@ -1175,20 +1116,29 @@ static int ufshcd_scale_gear(struct ufs_hba *hba, bool scale_up)
>
> static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> {
> - #define DOORBELL_CLR_TOUT_US (1000 * 1000) /* 1 sec */
> int ret = 0;
> +
> /*
> - * make sure that there are no outstanding requests when
> - * clock scaling is in progress
> + * Make sure that there are no outstanding requests while clock scaling
> + * is in progress. Since the error handler may submit TMFs, limit the
> + * time during which to block hba->tmf_queue in order not to block the
> + * UFS error handler.
> + *
> + * Since ufshcd_exec_dev_cmd() and ufshcd_issue_devman_upiu_cmd() lock
> + * the clk_scaling_lock before calling blk_get_request(), lock
> + * clk_scaling_lock before freezing the request queues to prevent a
> + * deadlock.
> */
> - ufshcd_scsi_block_requests(hba);
How are requests from LUN queues blocked?
> down_write(&hba->clk_scaling_lock);
> -
> + blk_freeze_queue_start(hba->cmd_queue);
> + blk_freeze_queue_start(hba->tmf_queue);
> if (!hba->clk_scaling.is_allowed ||
> - ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
As above, couldn't there be requests from LUN queues?
> + blk_mq_freeze_queue_wait_timeout(hba->cmd_queue, HZ) <= 0 ||
> + blk_mq_freeze_queue_wait_timeout(hba->tmf_queue, HZ / 10) <= 0) {
> ret = -EBUSY;
> + blk_mq_unfreeze_queue(hba->tmf_queue);
> + blk_mq_unfreeze_queue(hba->cmd_queue);
> up_write(&hba->clk_scaling_lock);
> - ufshcd_scsi_unblock_requests(hba);
> goto out;
> }
>
> @@ -1201,11 +1151,12 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
>
> static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
> {
> + blk_mq_unfreeze_queue(hba->tmf_queue);
> + blk_mq_unfreeze_queue(hba->cmd_queue);
> if (writelock)
> up_write(&hba->clk_scaling_lock);
> else
> up_read(&hba->clk_scaling_lock);
> - ufshcd_scsi_unblock_requests(hba);
> ufshcd_release(hba);
> }
>
> @@ -2698,8 +2649,11 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>
> WARN_ONCE(tag < 0, "Invalid tag %d\n", tag);
>
> - if (!down_read_trylock(&hba->clk_scaling_lock))
> - return SCSI_MLQUEUE_HOST_BUSY;
> + /*
> + * Allows the UFS error handler to wait for prior ufshcd_queuecommand()
> + * calls.
> + */
> + rcu_read_lock();
>
> switch (hba->ufshcd_state) {
> case UFSHCD_STATE_OPERATIONAL:
> @@ -2785,7 +2739,7 @@ static int ufshcd_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
>
> ufshcd_send_command(hba, tag);
> out:
> - up_read(&hba->clk_scaling_lock);
> + rcu_read_unlock();
>
> if (ufs_trigger_eh()) {
> unsigned long flags;
> @@ -5991,8 +5945,7 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
> }
> ufshcd_scsi_block_requests(hba);
> /* Drain ufshcd_queuecommand() */
> - down_write(&hba->clk_scaling_lock);
> - up_write(&hba->clk_scaling_lock);
> + synchronize_rcu();
> cancel_work_sync(&hba->eeh_work);
> }
>
> diff --git a/drivers/scsi/ufs/ufshcd.h b/drivers/scsi/ufs/ufshcd.h
> index a911ad72de7a..c8c2bddb1d33 100644
> --- a/drivers/scsi/ufs/ufshcd.h
> +++ b/drivers/scsi/ufs/ufshcd.h
> @@ -778,6 +778,7 @@ struct ufs_hba_monitor {
> * @clk_list_head: UFS host controller clocks list node head
> * @pwr_info: holds current power mode
> * @max_pwr_info: keeps the device max valid pwm
> + * @clk_scaling_lock: used to serialize device commands and clock scaling
> * @desc_size: descriptor sizes reported by device
> * @urgent_bkops_lvl: keeps track of urgent bkops level for device
> * @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
