scsi: ufs: core: Fix setup_xfer_req invocation

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

Posted by Rohit Ner 1 year, 11 months ago

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

Posted by Bart Van Assche 1 year, 11 months ago

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

Posted by Can Guo 1 year, 11 months ago

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

Posted by Rohit Ner 1 year, 11 months ago

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

Posted by Bart Van Assche 1 year, 11 months ago

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

Posted by Rohit Ner 1 year, 1 month ago

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.

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

Posted by Bart Van Assche 1 year, 11 months ago

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

Posted by Can Guo 1 year, 11 months ago


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

Posted by Bart Van Assche 1 year, 11 months ago

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.