[PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Kiwoong Kim]

clk_scaling_lock is to prevent from running clkscaling related operations
with others which might be affected by the operations concurrently.
I think it looks hardware specific.
If the feature isn't supported, I think there is no reasonto prevent from
running other functions, such as ufshcd_queuecommand and
ufshcd_exec_dev_cmd, concurrently.

So I add a condition at some points protecting with clk_scaling_lock.

Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
---
 drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 460d2b4..8471c90 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -2980,7 +2980,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 	/* Protects use of hba->reserved_slot. */
 	lockdep_assert_held(&hba->dev_cmd.lock);
 
-	down_read(&hba->clk_scaling_lock);
+	if (ufshcd_is_clkscaling_supported(hba))
+		down_read(&hba->clk_scaling_lock);
 
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
@@ -2998,7 +2999,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
 out:
-	up_read(&hba->clk_scaling_lock);
+	if (ufshcd_is_clkscaling_supported(hba))
+		up_read(&hba->clk_scaling_lock);
 	return err;
 }
 
@@ -6014,7 +6016,8 @@ static void ufshcd_err_handling_prepare(struct ufs_hba *hba)
 		if (ufshcd_is_clkscaling_supported(hba) &&
 		    hba->clk_scaling.is_enabled)
 			ufshcd_suspend_clkscaling(hba);
-		ufshcd_clk_scaling_allow(hba, false);
+		if (ufshcd_is_clkscaling_supported(hba))
+			ufshcd_clk_scaling_allow(hba, false);
 	}
 	ufshcd_scsi_block_requests(hba);
 	/* Drain ufshcd_queuecommand() */
@@ -6247,7 +6250,8 @@ static void ufshcd_err_handler(struct work_struct *work)
 		 * Hold the scaling lock just in case dev cmds
 		 * are sent via bsg and/or sysfs.
 		 */
-		down_write(&hba->clk_scaling_lock);
+		if (ufshcd_is_clkscaling_supported(hba))
+			down_write(&hba->clk_scaling_lock);
 		hba->force_pmc = true;
 		pmc_err = ufshcd_config_pwr_mode(hba, &(hba->pwr_info));
 		if (pmc_err) {
@@ -6257,7 +6261,8 @@ static void ufshcd_err_handler(struct work_struct *work)
 		}
 		hba->force_pmc = false;
 		ufshcd_print_pwr_info(hba);
-		up_write(&hba->clk_scaling_lock);
+		if (ufshcd_is_clkscaling_supported(hba))
+			up_write(&hba->clk_scaling_lock);
 		spin_lock_irqsave(hba->host->host_lock, flags);
 	}
 
@@ -6753,7 +6758,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	/* Protects use of hba->reserved_slot. */
 	lockdep_assert_held(&hba->dev_cmd.lock);
 
-	down_read(&hba->clk_scaling_lock);
+	if (ufshcd_is_clkscaling_supported(hba))
+		down_read(&hba->clk_scaling_lock);
 
 	lrbp = &hba->lrb[tag];
 	WARN_ON(lrbp->cmd);
@@ -6822,7 +6828,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct ufs_hba *hba,
 	ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR : UFS_QUERY_COMP,
 				    (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
 
-	up_read(&hba->clk_scaling_lock);
+	if (ufshcd_is_clkscaling_supported(hba))
+		up_read(&hba->clk_scaling_lock);
 	return err;
 }
 
-- 
2.7.4

RE: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Avri Altman]

 
> clk_scaling_lock is to prevent from running clkscaling related operations
> with others which might be affected by the operations concurrently.
> I think it looks hardware specific.
> If the feature isn't supported, I think there is no reasonto prevent from
                                                                                         ^^^ reason to 

> running other functions, such as ufshcd_queuecommand and
It is no longer used in queuecommand since 5675c381ea51 and 8d077ede48c1

> ufshcd_exec_dev_cmd, concurrently.
> 
> So I add a condition at some points protecting with clk_scaling_lock.
But you still need a way to serialize device management commands.

Thanks,
Avri

> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
>  drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++-------
>  1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 460d2b4..8471c90 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2980,7 +2980,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>         /* Protects use of hba->reserved_slot. */
>         lockdep_assert_held(&hba->dev_cmd.lock);
> 
> -       down_read(&hba->clk_scaling_lock);
> +       if (ufshcd_is_clkscaling_supported(hba))
> +               down_read(&hba->clk_scaling_lock);
> 
>         lrbp = &hba->lrb[tag];
>         WARN_ON(lrbp->cmd);
> @@ -2998,7 +2999,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>                                     (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
> 
>  out:
> -       up_read(&hba->clk_scaling_lock);
> +       if (ufshcd_is_clkscaling_supported(hba))
> +               up_read(&hba->clk_scaling_lock);
>         return err;
>  }
> 
> @@ -6014,7 +6016,8 @@ static void ufshcd_err_handling_prepare(struct
> ufs_hba *hba)
>                 if (ufshcd_is_clkscaling_supported(hba) &&
>                     hba->clk_scaling.is_enabled)
>                         ufshcd_suspend_clkscaling(hba);
> -               ufshcd_clk_scaling_allow(hba, false);
> +               if (ufshcd_is_clkscaling_supported(hba))
> +                       ufshcd_clk_scaling_allow(hba, false);
>         }
>         ufshcd_scsi_block_requests(hba);
>         /* Drain ufshcd_queuecommand() */
> @@ -6247,7 +6250,8 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>                  * Hold the scaling lock just in case dev cmds
>                  * are sent via bsg and/or sysfs.
>                  */
> -               down_write(&hba->clk_scaling_lock);
> +               if (ufshcd_is_clkscaling_supported(hba))
> +                       down_write(&hba->clk_scaling_lock);
>                 hba->force_pmc = true;
>                 pmc_err = ufshcd_config_pwr_mode(hba, &(hba->pwr_info));
>                 if (pmc_err) {
> @@ -6257,7 +6261,8 @@ static void ufshcd_err_handler(struct work_struct
> *work)
>                 }
>                 hba->force_pmc = false;
>                 ufshcd_print_pwr_info(hba);
> -               up_write(&hba->clk_scaling_lock);
> +               if (ufshcd_is_clkscaling_supported(hba))
> +                       up_write(&hba->clk_scaling_lock);
>                 spin_lock_irqsave(hba->host->host_lock, flags);
>         }
> 
> @@ -6753,7 +6758,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>         /* Protects use of hba->reserved_slot. */
>         lockdep_assert_held(&hba->dev_cmd.lock);
> 
> -       down_read(&hba->clk_scaling_lock);
> +       if (ufshcd_is_clkscaling_supported(hba))
> +               down_read(&hba->clk_scaling_lock);
> 
>         lrbp = &hba->lrb[tag];
>         WARN_ON(lrbp->cmd);
> @@ -6822,7 +6828,8 @@ static int ufshcd_issue_devman_upiu_cmd(struct
> ufs_hba *hba,
>         ufshcd_add_query_upiu_trace(hba, err ? UFS_QUERY_ERR :
> UFS_QUERY_COMP,
>                                     (struct utp_upiu_req *)lrbp->ucd_rsp_ptr);
> 
> -       up_read(&hba->clk_scaling_lock);
> +       if (ufshcd_is_clkscaling_supported(hba))
> +               up_read(&hba->clk_scaling_lock);
>         return err;
>  }
> 
> --
> 2.7.4

RE: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Kiwoong Kim]

> > I think it looks hardware specific.
> > If the feature isn't supported, I think there is no reasonto prevent
> > from
>                                                                                          ^^^
> reason to
> 
> > running other functions, such as ufshcd_queuecommand and
> It is no longer used in queuecommand since 5675c381ea51 and 8d077ede48c1

Yeah, you're right. It's just an example. I just want to tell that the lock also protects things that are not related with clk scaling directly.

> 
> > ufshcd_exec_dev_cmd, concurrently.
> >
> > So I add a condition at some points protecting with clk_scaling_lock.
> But you still need a way to serialize device management commands.
> 
> Thanks,
> Avri

The dev cmd execution period is protected by mutex.
And actual ringing a doorbell is protected by spin lock.

Is there another reason to need clk_scaling_lock even with it?

Thanks.
Kiwoong Kim

Re: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Adrian Hunter]

On 11/02/2022 04:15, Kiwoong Kim wrote:
>>> I think it looks hardware specific.
>>> If the feature isn't supported, I think there is no reasonto prevent
>>> from
>>                                                                                          ^^^
>> reason to
>>
>>> running other functions, such as ufshcd_queuecommand and
>> It is no longer used in queuecommand since 5675c381ea51 and 8d077ede48c1
> 
> Yeah, you're right. It's just an example. I just want to tell that the lock also protects things that are not related with clk scaling directly.
> 
>>
>>> ufshcd_exec_dev_cmd, concurrently.
>>>
>>> So I add a condition at some points protecting with clk_scaling_lock.
>> But you still need a way to serialize device management commands.
>>
>> Thanks,
>> Avri
> 
> The dev cmd execution period is protected by mutex.
> And actual ringing a doorbell is protected by spin lock.
> 
> Is there another reason to need clk_scaling_lock even with it?
> 

The error handler really should have exclusive access.  One
of the places you change does explain that:

 		 * Hold the scaling lock just in case dev cmds
 		 * are sent via bsg and/or sysfs.
 		 */
-		down_write(&hba->clk_scaling_lock);
+		if (ufshcd_is_clkscaling_supported(hba))
+			down_write(&hba->clk_scaling_lock);

RE: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Kiwoong Kim]

> The error handler really should have exclusive access.  One of the places
> you change does explain that:
> 
>  		 * Hold the scaling lock just in case dev cmds
>  		 * are sent via bsg and/or sysfs.
>  		 */
> -		down_write(&hba->clk_scaling_lock);
> +		if (ufshcd_is_clkscaling_supported(hba))
> +			down_write(&hba->clk_scaling_lock); 


Yeah.., I saw the comment but didn't get why.

Is there anyone who knows why it's necessary for all SoCs?
At lease, I know there is no reason to forbid concurrent executions of dev cmd and power mode change.

If there's nothing, how about adding a quick to ignore it?

Thanks.
Kiwoong Kim

Re: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Adrian Hunter]

On 12/02/2022 06:44, Kiwoong Kim wrote:
>> The error handler really should have exclusive access.  One of the places
>> you change does explain that:
>>
>>  		 * Hold the scaling lock just in case dev cmds
>>  		 * are sent via bsg and/or sysfs.
>>  		 */
>> -		down_write(&hba->clk_scaling_lock);
>> +		if (ufshcd_is_clkscaling_supported(hba))
>> +			down_write(&hba->clk_scaling_lock); 
> 
> 
> Yeah.., I saw the comment but didn't get why.
> 
> Is there anyone who knows why it's necessary for all SoCs?
> At lease, I know there is no reason to forbid concurrent executions of dev cmd and power mode change.
> 
> If there's nothing, how about adding a quick to ignore it?

Is it worth it?

The error handler really should have exclusive access.
Have you considered, for example, races of ufshcd_reset_and restore() and dev commands, tm commands, UIC commands.
I suspect more locking is needed not less.

Re: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Bean Huo]

On Sat, 2022-02-12 at 13:44 +0900, Kiwoong Kim wrote:
> > The error handler really should have exclusive access.  One of the
> > places
> > you change does explain that:
> > 
> >  		 * Hold the scaling lock just in case dev cmds
> >  		 * are sent via bsg and/or sysfs.
> >  		 */
> > -		down_write(&hba->clk_scaling_lock);
> > +		if (ufshcd_is_clkscaling_supported(hba))
> > +			down_write(&hba->clk_scaling_lock); 
> 
> Yeah.., I saw the comment but didn't get why.
> 
> Is there anyone who knows why it's necessary for all SoCs?
> At lease, I know there is no reason to forbid concurrent executions
> of dev cmd and power mode change.
> 
> If there's nothing, how about adding a quick to ignore it?
> 
> Thanks.
> Kiwoong Kim
> 

The name of clk_scaling_lock has explained everything, for the platform
which doesn't support load-based clk scaling, doesn't need to hold this
lock.

Acked-by: Bean Huo <beanhuo@micron.com>

Re: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Bart Van Assche]

On 2/15/22 03:00, Bean Huo wrote:
> The name of clk_scaling_lock has explained everything, for the platform
> which doesn't support load-based clk scaling, doesn't need to hold this
> lock.

Hi Bean,

Have you noticed Adrian's reply? I agree with Adrian that this patch 
breaks the UFS error handler.

Bart.

RE: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Kiwoong Kim]

> The name of clk_scaling_lock has explained everything, for the platform
> which doesn't support load-based clk scaling, doesn't need to hold this
> lock.
> 
> Acked-by: Bean Huo <beanhuo@micron.com>

Okay, thank you for your information.
I still have no idea of why the lock is required for ufshcd_exec_dev_cmd from the UFS specification w/o any featuring.

RE: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Avri Altman]

> > > I think it looks hardware specific.
> > > If the feature isn't supported, I think there is no reasonto prevent
> > > from
> >
> > ^^^ reason to
> >
> > > running other functions, such as ufshcd_queuecommand and
> > It is no longer used in queuecommand since 5675c381ea51 and
> > 8d077ede48c1
> 
> Yeah, you're right. It's just an example. I just want to tell that the lock also
> protects things that are not related with clk scaling directly.
OK.

> 
> >
> > > ufshcd_exec_dev_cmd, concurrently.
> > >
> > > So I add a condition at some points protecting with clk_scaling_lock.
> > But you still need a way to serialize device management commands.
> >
> > Thanks,
> > Avri
> 
> The dev cmd execution period is protected by mutex.
> And actual ringing a doorbell is protected by spin lock.
> 
> Is there another reason to need clk_scaling_lock even with it?
Right.

Acked-by: Avri Altman <avri.altman@wdc.com>

> 
> Thanks.
> Kiwoong Kim
> 

Re: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Bart Van Assche]

On 2/4/22 23:39, Kiwoong Kim wrote:
> clk_scaling_lock is to prevent from running clkscaling related operations
> with others which might be affected by the operations concurrently.
> I think it looks hardware specific.
> If the feature isn't supported, I think there is no reasonto prevent from
> running other functions, such as ufshcd_queuecommand and
> ufshcd_exec_dev_cmd, concurrently.
> 
> So I add a condition at some points protecting with clk_scaling_lock.
> 
> Signed-off-by: Kiwoong Kim <kwmad.kim@samsung.com>
> ---
>   drivers/scsi/ufs/ufshcd.c | 21 ++++++++++++++-------
>   1 file changed, 14 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
> index 460d2b4..8471c90 100644
> --- a/drivers/scsi/ufs/ufshcd.c
> +++ b/drivers/scsi/ufs/ufshcd.c
> @@ -2980,7 +2980,8 @@ static int ufshcd_exec_dev_cmd(struct ufs_hba *hba,
>   	/* Protects use of hba->reserved_slot. */
>   	lockdep_assert_held(&hba->dev_cmd.lock);
>   
> -	down_read(&hba->clk_scaling_lock);
> +	if (ufshcd_is_clkscaling_supported(hba))
> +		down_read(&hba->clk_scaling_lock);
>   
>   	lrbp = &hba->lrb[tag];
>   	WARN_ON(lrbp->cmd);

I don't like this patch at all. This patch makes testing the UFS driver 
more complicated without having any clear benefit. Additionally, adding 
if-statements in front of locking makes static source code analysis 
harder and is an anti-pattern. Please don't do this.

Bart.

RE: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Kiwoong Kim]

> > -	down_read(&hba->clk_scaling_lock);
> > +	if (ufshcd_is_clkscaling_supported(hba))
> > +		down_read(&hba->clk_scaling_lock);
> >
> >   	lrbp = &hba->lrb[tag];
> >   	WARN_ON(lrbp->cmd);
> 
> I don't like this patch at all. This patch makes testing the UFS driver
> more complicated without having any clear benefit. Additionally, adding
> if-statements in front of locking makes static source code analysis harder
> and is an anti-pattern. Please don't do this.
> 
> Bart. 

The benefit that I think is not blocking dev cmd during submitting a scsi cmd.
Rather, I don't understand why this lock is required if a SoC doesn't support clk scaling.

The period of ringing doorbells has been already protected by spin lock.

Thanks.
Kiwoong Kim

Re: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Bart Van Assche]

On 2/14/22 22:03, Kiwoong Kim wrote:
> The benefit that I think is not blocking dev cmd during submitting a scsi cmd.

Hi Kiwoong,

Both ufshcd_exec_dev_cmd() and previous versions of 
ufshcd_queuecommand() obtain a reader lock on the clock scaling lock so 
concurrent submission of both command types is allowed. I'm not aware of 
any version of the UFS driver that serializes device commands against 
SCSI commands.

Additionally, please take a look at commit 8d077ede48c1 ("scsi: ufs: 
Optimize the command queueing code"). That patch removes the clock 
scaling lock from ufshcd_queuecommand().

Thanks,

Bart.

RE: [PATCH v1] scsi: ufs: remove clk_scaling_lock when clkscaling isn't supported.

[Kiwoong Kim]

> On 2/14/22 22:03, Kiwoong Kim wrote:
> > The benefit that I think is not blocking dev cmd during submitting a
> scsi cmd.
> 
> Hi Kiwoong,
> 
> Both ufshcd_exec_dev_cmd() and previous versions of
> ufshcd_queuecommand() obtain a reader lock on the clock scaling lock so
> concurrent submission of both command types is allowed. I'm not aware of
> any version of the UFS driver that serializes device commands against SCSI
> commands.
> 
> Additionally, please take a look at commit 8d077ede48c1 ("scsi: ufs:
> Optimize the command queueing code"). That patch removes the clock scaling
> lock from ufshcd_queuecommand().
> 
> Thanks,
> 
> Bart.

Okay, I got it.