[PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Rohit Ner]

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);
-- 
2.44.0.rc0.258.g7320e95886-goog

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Bart Van Assche]

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.

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Bart Van Assche]

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.

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Can Guo]

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.

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Bart Van Assche]

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.

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Can Guo]

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

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Bart Van Assche]

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.

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Rohit Ner]

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.

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Bart Van Assche]

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.

Re: [PATCH] scsi: ufs: core: Fix setup_xfer_req invocation

[Rohit Ner]

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.