| Message ID | 20240220090805.2886914-1-rohitner@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | scsi: ufs: core: Fix setup_xfer_req invocation | expand |
On 2/20/24 01:08, Rohit Ner wrote: > Allow variant callback to setup transfers without > restricting the transfers to use legacy doorbell > > Signed-off-by: Rohit Ner <rohitner@google.com> > --- > drivers/ufs/core/ufshcd.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c > index d77b25b79ae3..91e483dd3974 100644 > --- a/drivers/ufs/core/ufshcd.c > +++ b/drivers/ufs/core/ufshcd.c > @@ -2280,6 +2280,9 @@ 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 (hba->vops && hba->vops->setup_xfer_req) > + hba->vops->setup_xfer_req(hba, lrbp->task_tag, > + !!lrbp->cmd); > > if (is_mcq_enabled(hba)) { > int utrd_size = sizeof(struct utp_transfer_req_desc); > @@ -2293,9 +2296,6 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag, > spin_unlock(&hwq->sq_lock); > } else { > spin_lock_irqsave(&hba->outstanding_lock, flags); > - if (hba->vops && hba->vops->setup_xfer_req) > - hba->vops->setup_xfer_req(hba, lrbp->task_tag, > - !!lrbp->cmd); > __set_bit(lrbp->task_tag, &hba->outstanding_reqs); > ufshcd_writel(hba, 1 << lrbp->task_tag, > REG_UTP_TRANSFER_REQ_DOOR_BELL); UFS controllers that are compliant with the JEDEC UFSHCI specification do not need the .setup_xfer_req() callback so I think a better motivation is needed to make this change. Thanks, Bart.
On 2/20/24 09:58, Rohit Ner wrote: > Do you propose to remove this callback altogether? This callback should > either support both transfer modes or none. The only UFSHCI controller I know of that needs this callback is the Exynos UFSHCI controller. As far as I know there are Exynos UFSHCI controllers that support UFSHCI 3.0 but UFSHCI 4.0 Exynos controllers are not yet available. Standard compliant controllers should not use the .setup_xfer_req() callback. As you may know invoking that callback means performing an indirect function call. Indirect function calls are slower than direct calls, especially if speculative execution vulnerability security mitigations are enabled. Bart.
Hi Bart, On 2/21/2024 1:21 AM, Bart Van Assche wrote: > On 2/20/24 01:08, Rohit Ner wrote: >> Allow variant callback to setup transfers without >> restricting the transfers to use legacy doorbell >> >> Signed-off-by: Rohit Ner <rohitner@google.com> >> --- >> drivers/ufs/core/ufshcd.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c >> index d77b25b79ae3..91e483dd3974 100644 >> --- a/drivers/ufs/core/ufshcd.c >> +++ b/drivers/ufs/core/ufshcd.c >> @@ -2280,6 +2280,9 @@ 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 (hba->vops && hba->vops->setup_xfer_req) >> + hba->vops->setup_xfer_req(hba, lrbp->task_tag, >> + !!lrbp->cmd); >> if (is_mcq_enabled(hba)) { >> int utrd_size = sizeof(struct utp_transfer_req_desc); >> @@ -2293,9 +2296,6 @@ void ufshcd_send_command(struct ufs_hba *hba, >> unsigned int task_tag, >> spin_unlock(&hwq->sq_lock); >> } else { >> spin_lock_irqsave(&hba->outstanding_lock, flags); >> - if (hba->vops && hba->vops->setup_xfer_req) >> - hba->vops->setup_xfer_req(hba, lrbp->task_tag, >> - !!lrbp->cmd); >> __set_bit(lrbp->task_tag, &hba->outstanding_reqs); >> ufshcd_writel(hba, 1 << lrbp->task_tag, >> REG_UTP_TRANSFER_REQ_DOOR_BELL); > > UFS controllers that are compliant with the JEDEC UFSHCI specification do > not need the .setup_xfer_req() callback so I think a better motivation is > needed to make this change. I am going to push some BUG fixes for Qualcomm UFSHCI MCQ engine, one of which would count on a vops in ufshcd_send_command(). My original plan was to add a new vops.mcq_setup_xfer_req() to differentiate from the existing one used in legacy mode. But if Rohit moves the existing .setup_xfer_req() up, I can use it instead of introducing the new one. Thanks, Can Guo. > > Thanks, > > Bart.
On 2/21/24 01:13, Can Guo wrote: > I am going to push some BUG fixes for Qualcomm UFSHCI MCQ engine, one of > which would count on a vops in ufshcd_send_command(). My original plan > was to add a new vops.mcq_setup_xfer_req() to differentiate from the > existing one used in legacy mode. But if Rohit moves the existing > .setup_xfer_req() up, I can use it instead of introducing the new one. Hi Can, If an if-statement can be avoided in the hot path by introducing a new callback pointer for MCQ code then I prefer the approach of introducing a new callback instead of moving the setup_xfer_req() call. Thanks, Bart.
On 2/22/2024 1:55 AM, Bart Van Assche wrote: > On 2/21/24 01:13, Can Guo wrote: >> I am going to push some BUG fixes for Qualcomm UFSHCI MCQ engine, one >> of which would count on a vops in ufshcd_send_command(). My original >> plan was to add a new vops.mcq_setup_xfer_req() to differentiate from >> the existing one used in legacy mode. But if Rohit moves the existing >> .setup_xfer_req() up, I can use it instead of introducing the new one. > > Hi Can, > > If an if-statement can be avoided in the hot path by introducing a new > callback pointer for MCQ code then I prefer the approach of introducing > a new callback instead of moving the setup_xfer_req() call. Hi Bart, The if-statement you are mentioning here, is it the if (hba->vops && hba->vops->setup_xfer_req) or if (is_mcq_enabled(hba))? Thanks, Can Guo. > > Thanks, > > Bart. >
On 2/21/24 18:05, Can Guo wrote:
> The if-statement you are mentioning here, is it the if (hba->vops && hba->vops->setup_xfer_req) or if (is_mcq_enabled(hba))?
Hi Can,
I was referring to the latter if-statement, at least if it would occur in the
code that you plan to post and that I haven't seen yet.
Thanks,
Bart.
On 2/21/24 01:13, Can Guo wrote: > I am going to push some BUG fixes for Qualcomm UFSHCI MCQ engine, one of > which would count on a vops in ufshcd_send_command(). My original plan > was to add a new vops.mcq_setup_xfer_req() to differentiate from the > existing one used in legacy mode. But if Rohit moves the existing > .setup_xfer_req() up, I can use it instead of introducing the new one. Hi Can, Can we stick to the current approach of moving the .setup_xfer_req() up, keeping in mind the following pros? 1. Avoid redundant callbacks for setting up transfers 2. Trim the duration for which hba->outstanding_lock is acquired unnecessarily Thanks, Rohit.
On 2/22/24 00:27, Rohit Ner wrote: > Can we stick to the current approach of moving the .setup_xfer_req() > up, keeping in mind the following pros? > 1. Avoid redundant callbacks for setting up transfers > 2. Trim the duration for which hba->outstanding_lock is acquired unnecessarily No, we can't. The Exynos implementation of the .setup_xfer_req() callback is not thread-safe and relies on serialization by the caller. This patch breaks the Exynos driver. A better title for this patch would be "Break the setup_xfer_req() invocation". Bart.
On Thu, Feb 22, 2024 at 8:56 PM Bart Van Assche <bvanassche@acm.org> wrote: > > On 2/22/24 00:27, Rohit Ner wrote: > > Can we stick to the current approach of moving the .setup_xfer_req() > > up, keeping in mind the following pros? > > 1. Avoid redundant callbacks for setting up transfers > > 2. Trim the duration for which hba->outstanding_lock is acquired unnecessarily > > No, we can't. The Exynos implementation of the .setup_xfer_req() callback > is not thread-safe and relies on serialization by the caller. This patch > breaks the Exynos driver. A better title for this patch would be "Break > the setup_xfer_req() invocation". It would be inaccurate to tag this patch as breaking as Can did mention a vops use case in the hotpath for UFSHCI compliant drivers. Having two different setup_xfer_req functions for mcq/lsdb mode just because a particular vendor driver relies on serialization by the caller defeats the purpose of having vops as the core logic is still burdened with quirks. > > Bart. > Rohit.
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c index d77b25b79ae3..91e483dd3974 100644 --- a/drivers/ufs/core/ufshcd.c +++ b/drivers/ufs/core/ufshcd.c @@ -2280,6 +2280,9 @@ 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 (hba->vops && hba->vops->setup_xfer_req) + hba->vops->setup_xfer_req(hba, lrbp->task_tag, + !!lrbp->cmd); if (is_mcq_enabled(hba)) { int utrd_size = sizeof(struct utp_transfer_req_desc); @@ -2293,9 +2296,6 @@ void ufshcd_send_command(struct ufs_hba *hba, unsigned int task_tag, spin_unlock(&hwq->sq_lock); } else { spin_lock_irqsave(&hba->outstanding_lock, flags); - if (hba->vops && hba->vops->setup_xfer_req) - hba->vops->setup_xfer_req(hba, lrbp->task_tag, - !!lrbp->cmd); __set_bit(lrbp->task_tag, &hba->outstanding_reqs); ufshcd_writel(hba, 1 << lrbp->task_tag, REG_UTP_TRANSFER_REQ_DOOR_BELL);
Allow variant callback to setup transfers without restricting the transfers to use legacy doorbell Signed-off-by: Rohit Ner <rohitner@google.com> --- drivers/ufs/core/ufshcd.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)