Allow variant callback to setup transfers without
restricting the transfers to use legacy doorbell
Signed-off-by: Rohit Ner <[email protected]>
---
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);
--
2.44.0.rc0.258.g7320e95886-goog
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 <[email protected]>
> ---
> 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 <[email protected]>
>> ---
>> 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.