RE: [PATCH v2 13/19] scsi: ufs: Fix a race in the completion path

[Avri Altman <Avri.Altman@wdc.com>]

> 
> The following unlikely races can be triggered by the completion path
> (ufshcd_trc_handler()):
> - After the UTRLCNR register has been read from interrupt context and
>   before it is cleared, the UFS error handler reads the UTRLCNR register.
>   Hold the SCSI host lock until the UTRLCNR register has been cleared to
>   prevent that this register is accessed from another CPU before it has
>   been cleared.
> - After the doorbell register has been read and before outstanding_reqs
>   is cleared, the error handler reads the doorbell register. This can also
>   result in double completions. Fix this by clearing outstanding_reqs
>   before calling ufshcd_transfer_req_compl().
> 
> Due to this change ufshcd_trc_handler() no longer updates
> outstanding_reqs
> atomically. Hence protect all other outstanding_reqs changes with the SCSI
> host lock.
But isn't the whole point of REG_UTP_TRANSFER_REQ_LIST_COMPL is to eliminate the host lock
As a source of contention?

> 
> This patch is a performance improvement because it reduces the number of
> atomic operations in the hot path (test_and_clear_bit()).
Both Can & Stanley reported a performance improvement of RR with "Optimize host lock..".
Can those short numerical studies can be repeated with this patch?

Thanks,
Avri

> 
> See also commit a45f937110fa ("scsi: ufs: Optimize host lock on transfer
> requests send/compl paths").
> 
> Cc: Adrian Hunter <adrian.hunter@intel.com>
> Cc: Stanley Chu <stanley.chu@mediatek.com>
> Cc: Can Guo <cang@codeaurora.org>
> Cc: Asutosh Das <asutoshd@codeaurora.org>
> Cc: Avri Altman <avri.altman@wdc.com>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/ufs/ufshcd.c | 36 +++++++++++++++++-------------------
>  1 file changed, 17 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 83a32b71240e..996b95ab74aa 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2088,6 +2088,7 @@ static inline
>  void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag)
>  {
>         struct ufshcd_lrb *lrbp = &hba->lrb[task_tag];
> +       unsigned long flags;
> 
>         lrbp->issue_time_stamp = ktime_get();
>         lrbp->compl_time_stamp = ktime_set(0, 0);
> @@ -2096,19 +2097,12 @@ void ufshcd_send_command(struct ufs_hba
> *hba, unsigned int task_tag)
>         ufshcd_clk_scaling_start_busy(hba);
>         if (unlikely(ufshcd_should_inform_monitor(hba, lrbp)))
>                 ufshcd_start_monitor(hba, lrbp);
> -       if (ufshcd_has_utrlcnr(hba)) {
> -               set_bit(task_tag, &hba->outstanding_reqs);
> -               ufshcd_writel(hba, 1 << task_tag,
> -                             REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -       } else {
> -               unsigned long flags;
> 
> -               spin_lock_irqsave(hba->host->host_lock, flags);
> -               set_bit(task_tag, &hba->outstanding_reqs);
> -               ufshcd_writel(hba, 1 << task_tag,
> -                             REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -               spin_unlock_irqrestore(hba->host->host_lock, flags);
> -       }
> +       spin_lock_irqsave(hba->host->host_lock, flags);
> +       __set_bit(task_tag, &hba->outstanding_reqs);
> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> +
> +       ufshcd_writel(hba, 1 << task_tag,
> REG_UTP_TRANSFER_REQ_DOOR_BELL);
>         /* Make sure that doorbell is committed immediately */
>         wmb();
>  }
> @@ -2890,7 +2884,9 @@ static int ufshcd_wait_for_dev_cmd(struct ufs_hba
> *hba,
>                  * we also need to clear the outstanding_request
>                  * field in hba
>                  */
> -               clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
> +               spin_lock_irqsave(hba->host->host_lock, flags);
> +               __clear_bit(lrbp->task_tag, &hba->outstanding_reqs);
> +               spin_unlock_irqrestore(hba->host->host_lock, flags);
>         }
> 
>         return err;
> @@ -5197,8 +5193,6 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>         bool update_scaling = false;
> 
>         for_each_set_bit(index, &completed_reqs, hba->nutrs) {
> -               if (!test_and_clear_bit(index, &hba->outstanding_reqs))
> -                       continue;
>                 lrbp = &hba->lrb[index];
>                 lrbp->compl_time_stamp = ktime_get();
>                 cmd = lrbp->cmd;
> @@ -5241,6 +5235,7 @@ static void ufshcd_transfer_req_compl(struct
> ufs_hba *hba,
>  static irqreturn_t ufshcd_trc_handler(struct ufs_hba *hba, bool use_utrlcnr)
>  {
>         unsigned long completed_reqs;
> +       unsigned long flags;
> 
>         /* Resetting interrupt aggregation counters first and reading the
>          * DOOR_BELL afterward allows us to handle all the completed requests.
> @@ -5253,6 +5248,7 @@ static irqreturn_t ufshcd_trc_handler(struct
> ufs_hba *hba, bool use_utrlcnr)
>             !(hba->quirks & UFSHCI_QUIRK_SKIP_RESET_INTR_AGGR))
>                 ufshcd_reset_intr_aggr(hba);
> 
> +       spin_lock_irqsave(hba->host->host_lock, flags);
>         if (use_utrlcnr) {
>                 completed_reqs = ufshcd_readl(hba,
>                                               REG_UTP_TRANSFER_REQ_LIST_COMPL);
> @@ -5260,14 +5256,16 @@ static irqreturn_t ufshcd_trc_handler(struct
> ufs_hba *hba, bool use_utrlcnr)
>                         ufshcd_writel(hba, completed_reqs,
>                                       REG_UTP_TRANSFER_REQ_LIST_COMPL);
>         } else {
> -               unsigned long flags;
>                 u32 tr_doorbell;
> 
> -               spin_lock_irqsave(hba->host->host_lock, flags);
>                 tr_doorbell = ufshcd_readl(hba,
> REG_UTP_TRANSFER_REQ_DOOR_BELL);
> -               completed_reqs = tr_doorbell ^ hba->outstanding_reqs;
> -               spin_unlock_irqrestore(hba->host->host_lock, flags);
> +               completed_reqs = ~tr_doorbell & hba->outstanding_reqs;
>         }
> +       WARN_ONCE(completed_reqs & ~hba->outstanding_reqs,
> +                 "completed: %#lx; outstanding: %#lx\n", completed_reqs,
> +                 hba->outstanding_reqs);
> +       hba->outstanding_reqs &= ~completed_reqs;
> +       spin_unlock_irqrestore(hba->host->host_lock, flags);
> 
>         if (completed_reqs) {
>                 ufshcd_transfer_req_compl(hba, completed_reqs);