diff mbox series

scsi: ufs: core: Fix setup_xfer_req invocation

Message ID 20240220090805.2886914-1-rohitner@google.com
State New
Headers
Series scsi: ufs: core: Fix setup_xfer_req invocation |

Commit Message

Rohit Ner Feb. 20, 2024, 9:08 a.m. UTC 
  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(-)

Comments

Bart Van Assche Feb. 20, 2024, 5:21 p.m. UTC | #1 
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.
Bart Van Assche Feb. 20, 2024, 6:19 p.m. UTC | #2 
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.
Can Guo Feb. 21, 2024, 9:13 a.m. UTC | #3 
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.
Bart Van Assche Feb. 21, 2024, 5:55 p.m. UTC | #4 
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.
Can Guo Feb. 22, 2024, 2:05 a.m. UTC | #5 
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.
>
Bart Van Assche Feb. 22, 2024, 3:09 a.m. UTC | #6 
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.
Rohit Ner Feb. 22, 2024, 8:27 a.m. UTC | #7 
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.
Bart Van Assche Feb. 22, 2024, 3:25 p.m. UTC | #8 
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.
diff mbox series

Patch

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);