There is a lock inversion and rwsem read-lock recursion in the devfreq
target callback which can lead to deadlocks.
Specifically, ufshcd_devfreq_scale() already holds a clk_scaling_lock
read lock when toggling the write booster, which involves taking the
dev_cmd mutex before taking another clk_scaling_lock read lock.
This can lead to a deadlock if another thread:
1) tries to acquire the dev_cmd and clk_scaling locks in the correct
order, or
2) takes a clk_scaling write lock before the attempt to take the
clk_scaling read lock a second time.
Fix this by dropping the clk_scaling_lock before toggling the write
booster as was done before commit 0e9d4ca43ba8 ("scsi: ufs: Protect some
contexts from unexpected clock scaling").
While the devfreq callbacks are already serialised, add a second
serialising mutex to handle the unlikely case where a callback triggered
through the devfreq sysfs interface is racing with a request to disable
clock scaling through the UFS controller 'clkscale_enable' sysfs
attribute. This could otherwise lead to the write booster being left
disabled after having disabled clock scaling.
Also take the new mutex in ufshcd_clk_scaling_allow() to make sure that
any pending write booster update has completed on return.
Note that this currently only affects Qualcomm platforms since commit
87bd05016a64 ("scsi: ufs: core: Allow host driver to disable wb toggling
during clock scaling").
The lock inversion (i.e. 1 above) was reported by lockdep as:
======================================================
WARNING: possible circular locking dependency detected
6.1.0-next-20221216 #211 Not tainted
------------------------------------------------------
kworker/u16:2/71 is trying to acquire lock:
ffff076280ba98a0 (&hba->dev_cmd.lock){+.+.}-{3:3}, at: ufshcd_query_flag+0x50/0x1c0
but task is already holding lock:
ffff076280ba9cf0 (&hba->clk_scaling_lock){++++}-{3:3}, at: ufshcd_devfreq_scale+0x2b8/0x380
which lock already depends on the new lock.
[ +0.011606]
the existing dependency chain (in reverse order) is:
-> #1 (&hba->clk_scaling_lock){++++}-{3:3}:
lock_acquire+0x68/0x90
down_read+0x58/0x80
ufshcd_exec_dev_cmd+0x70/0x2c0
ufshcd_verify_dev_init+0x68/0x170
ufshcd_probe_hba+0x398/0x1180
ufshcd_async_scan+0x30/0x320
async_run_entry_fn+0x34/0x150
process_one_work+0x288/0x6c0
worker_thread+0x74/0x450
kthread+0x118/0x120
ret_from_fork+0x10/0x20
-> #0 (&hba->dev_cmd.lock){+.+.}-{3:3}:
__lock_acquire+0x12a0/0x2240
lock_acquire.part.0+0xcc/0x220
lock_acquire+0x68/0x90
__mutex_lock+0x98/0x430
mutex_lock_nested+0x2c/0x40
ufshcd_query_flag+0x50/0x1c0
ufshcd_query_flag_retry+0x64/0x100
ufshcd_wb_toggle+0x5c/0x120
ufshcd_devfreq_scale+0x2c4/0x380
ufshcd_devfreq_target+0xf4/0x230
devfreq_set_target+0x84/0x2f0
devfreq_update_target+0xc4/0xf0
devfreq_monitor+0x38/0x1f0
process_one_work+0x288/0x6c0
worker_thread+0x74/0x450
kthread+0x118/0x120
ret_from_fork+0x10/0x20
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0 CPU1
---- ----
lock(&hba->clk_scaling_lock);
lock(&hba->dev_cmd.lock);
lock(&hba->clk_scaling_lock);
lock(&hba->dev_cmd.lock);
*** DEADLOCK ***
Fixes: 0e9d4ca43ba8 ("scsi: ufs: Protect some contexts from unexpected clock scaling")
Cc: [email protected] # 5.12
Cc: Can Guo <[email protected]>
Signed-off-by: Johan Hovold <[email protected]>
---
This issue has apparently been known for over a year [1] without anyone
bothering to fix it. Aside from the potential deadlocks, this also leads
to developers using Qualcomm platforms not being able to use lockdep to
prevent further issues like this from being introduced in other places.
Johan
[1] https://lore.kernel.org/lkml/[email protected]
drivers/ufs/core/ufshcd.c | 29 +++++++++++++++--------------
include/ufs/ufshcd.h | 2 ++
2 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
index bda61be5f035..5c3821b2fcf8 100644
--- a/drivers/ufs/core/ufshcd.c
+++ b/drivers/ufs/core/ufshcd.c
@@ -1234,12 +1234,14 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
* clock scaling is in progress
*/
ufshcd_scsi_block_requests(hba);
+ mutex_lock(&hba->wb_mutex);
down_write(&hba->clk_scaling_lock);
if (!hba->clk_scaling.is_allowed ||
ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
ret = -EBUSY;
up_write(&hba->clk_scaling_lock);
+ mutex_unlock(&hba->wb_mutex);
ufshcd_scsi_unblock_requests(hba);
goto out;
}
@@ -1251,12 +1253,16 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
return ret;
}
-static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
+static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool scale_up)
{
- if (writelock)
- up_write(&hba->clk_scaling_lock);
- else
- up_read(&hba->clk_scaling_lock);
+ up_write(&hba->clk_scaling_lock);
+
+ /* Enable Write Booster if we have scaled up else disable it */
+ if (ufshcd_enable_wb_if_scaling_up(hba))
+ ufshcd_wb_toggle(hba, scale_up);
+
+ mutex_unlock(&hba->wb_mutex);
+
ufshcd_scsi_unblock_requests(hba);
ufshcd_release(hba);
}
@@ -1273,7 +1279,6 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
{
int ret = 0;
- bool is_writelock = true;
ret = ufshcd_clock_scaling_prepare(hba);
if (ret)
@@ -1302,15 +1307,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
}
}
- /* Enable Write Booster if we have scaled up else disable it */
- if (ufshcd_enable_wb_if_scaling_up(hba)) {
- downgrade_write(&hba->clk_scaling_lock);
- is_writelock = false;
- ufshcd_wb_toggle(hba, scale_up);
- }
-
out_unprepare:
- ufshcd_clock_scaling_unprepare(hba, is_writelock);
+ ufshcd_clock_scaling_unprepare(hba, scale_up);
return ret;
}
@@ -6066,9 +6064,11 @@ static void ufshcd_force_error_recovery(struct ufs_hba *hba)
static void ufshcd_clk_scaling_allow(struct ufs_hba *hba, bool allow)
{
+ mutex_lock(&hba->wb_mutex);
down_write(&hba->clk_scaling_lock);
hba->clk_scaling.is_allowed = allow;
up_write(&hba->clk_scaling_lock);
+ mutex_unlock(&hba->wb_mutex);
}
static void ufshcd_clk_scaling_suspend(struct ufs_hba *hba, bool suspend)
@@ -9793,6 +9793,7 @@ int ufshcd_init(struct ufs_hba *hba, void __iomem *mmio_base, unsigned int irq)
/* Initialize mutex for exception event control */
mutex_init(&hba->ee_ctrl_mutex);
+ mutex_init(&hba->wb_mutex);
init_rwsem(&hba->clk_scaling_lock);
ufshcd_init_clk_gating(hba);
diff --git a/include/ufs/ufshcd.h b/include/ufs/ufshcd.h
index 5cf81dff60aa..727084cd79be 100644
--- a/include/ufs/ufshcd.h
+++ b/include/ufs/ufshcd.h
@@ -808,6 +808,7 @@ struct ufs_hba_monitor {
* @urgent_bkops_lvl: keeps track of urgent bkops level for device
* @is_urgent_bkops_lvl_checked: keeps track if the urgent bkops level for
* device is known or not.
+ * @wb_mutex: used to serialize devfreq and sysfs write booster toggling
* @clk_scaling_lock: used to serialize device commands and clock scaling
* @desc_size: descriptor sizes reported by device
* @scsi_block_reqs_cnt: reference counting for scsi block requests
@@ -951,6 +952,7 @@ struct ufs_hba {
enum bkops_status urgent_bkops_lvl;
bool is_urgent_bkops_lvl_checked;
+ struct mutex wb_mutex;
struct rw_semaphore clk_scaling_lock;
unsigned char desc_size[QUERY_DESC_IDN_MAX];
atomic_t scsi_block_reqs_cnt;
--
2.37.4
On Thu, Dec 22, 2022 at 11:21:21AM +0100, Johan Hovold wrote:
> There is a lock inversion and rwsem read-lock recursion in the devfreq
> target callback which can lead to deadlocks.
>
> Specifically, ufshcd_devfreq_scale() already holds a clk_scaling_lock
> read lock when toggling the write booster, which involves taking the
> dev_cmd mutex before taking another clk_scaling_lock read lock.
>
> This can lead to a deadlock if another thread:
>
> 1) tries to acquire the dev_cmd and clk_scaling locks in the correct
> order, or
>
> 2) takes a clk_scaling write lock before the attempt to take the
> clk_scaling read lock a second time.
>
> Fix this by dropping the clk_scaling_lock before toggling the write
> booster as was done before commit 0e9d4ca43ba8 ("scsi: ufs: Protect some
> contexts from unexpected clock scaling").
>
> While the devfreq callbacks are already serialised, add a second
> serialising mutex to handle the unlikely case where a callback triggered
> through the devfreq sysfs interface is racing with a request to disable
> clock scaling through the UFS controller 'clkscale_enable' sysfs
> attribute. This could otherwise lead to the write booster being left
> disabled after having disabled clock scaling.
>
> Also take the new mutex in ufshcd_clk_scaling_allow() to make sure that
> any pending write booster update has completed on return.
>
> Note that this currently only affects Qualcomm platforms since commit
> 87bd05016a64 ("scsi: ufs: core: Allow host driver to disable wb toggling
> during clock scaling").
>
> The lock inversion (i.e. 1 above) was reported by lockdep as:
>
> ======================================================
> WARNING: possible circular locking dependency detected
> 6.1.0-next-20221216 #211 Not tainted
> ------------------------------------------------------
> kworker/u16:2/71 is trying to acquire lock:
> ffff076280ba98a0 (&hba->dev_cmd.lock){+.+.}-{3:3}, at: ufshcd_query_flag+0x50/0x1c0
>
> but task is already holding lock:
> ffff076280ba9cf0 (&hba->clk_scaling_lock){++++}-{3:3}, at: ufshcd_devfreq_scale+0x2b8/0x380
>
> which lock already depends on the new lock.
> [ +0.011606]
> the existing dependency chain (in reverse order) is:
>
> -> #1 (&hba->clk_scaling_lock){++++}-{3:3}:
> lock_acquire+0x68/0x90
> down_read+0x58/0x80
> ufshcd_exec_dev_cmd+0x70/0x2c0
> ufshcd_verify_dev_init+0x68/0x170
> ufshcd_probe_hba+0x398/0x1180
> ufshcd_async_scan+0x30/0x320
> async_run_entry_fn+0x34/0x150
> process_one_work+0x288/0x6c0
> worker_thread+0x74/0x450
> kthread+0x118/0x120
> ret_from_fork+0x10/0x20
>
> -> #0 (&hba->dev_cmd.lock){+.+.}-{3:3}:
> __lock_acquire+0x12a0/0x2240
> lock_acquire.part.0+0xcc/0x220
> lock_acquire+0x68/0x90
> __mutex_lock+0x98/0x430
> mutex_lock_nested+0x2c/0x40
> ufshcd_query_flag+0x50/0x1c0
> ufshcd_query_flag_retry+0x64/0x100
> ufshcd_wb_toggle+0x5c/0x120
> ufshcd_devfreq_scale+0x2c4/0x380
> ufshcd_devfreq_target+0xf4/0x230
> devfreq_set_target+0x84/0x2f0
> devfreq_update_target+0xc4/0xf0
> devfreq_monitor+0x38/0x1f0
> process_one_work+0x288/0x6c0
> worker_thread+0x74/0x450
> kthread+0x118/0x120
> ret_from_fork+0x10/0x20
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
> CPU0 CPU1
> ---- ----
> lock(&hba->clk_scaling_lock);
> lock(&hba->dev_cmd.lock);
> lock(&hba->clk_scaling_lock);
> lock(&hba->dev_cmd.lock);
>
> *** DEADLOCK ***
>
> Fixes: 0e9d4ca43ba8 ("scsi: ufs: Protect some contexts from unexpected clock scaling")
> Cc: [email protected] # 5.12
> Cc: Can Guo <[email protected]>
> Signed-off-by: Johan Hovold <[email protected]>
Tested-by: Andrew Halaney <[email protected]>
Thanks for taking the time to fix this, I can't get any UFS related
splats to show after a brief round of testing now.
For what it is worth, the change looks good to me as well but I'll leave
it to someone familiar with the UFS core to add a proper Reviewed-by tag.
Thanks,
Andrew
On 12/22/22 02:21, Johan Hovold wrote:
> + /* Enable Write Booster if we have scaled up else disable it */
> + if (ufshcd_enable_wb_if_scaling_up(hba))
> + ufshcd_wb_toggle(hba, scale_up);
Hi Asutosh,
This patch is the second complaint about the mechanism that toggles the
WriteBooster during clock scaling. Can this mechanism be removed entirely?
I think this commit introduced that mechanism: 3d17b9b5ab11 ("scsi: ufs:
Add write booster feature support"; v5.8).
Thanks,
Bart.
> On 12/22/22 02:21, Johan Hovold wrote:
> > + /* Enable Write Booster if we have scaled up else disable it */
> > + if (ufshcd_enable_wb_if_scaling_up(hba))
> > + ufshcd_wb_toggle(hba, scale_up);
>
> Hi Asutosh,
>
> This patch is the second complaint about the mechanism that toggles the
> WriteBooster during clock scaling. Can this mechanism be removed entirely?
commit 87bd05016a64 that introduced UFSHCD_CAP_WB_WITH_CLK_SCALING enables
the platform vendors and OEMs to maintain wb toggling - should they choose so.
Why remove it in its entirety?
Thanks,
Avri
>
> I think this commit introduced that mechanism: 3d17b9b5ab11 ("scsi: ufs:
> Add write booster feature support"; v5.8).
>
> Thanks,
>
> Bart.
On Tue, Jan 03 2023 at 13:45 -0800, Bart Van Assche wrote:
>On 12/22/22 02:21, Johan Hovold wrote:
>>+ /* Enable Write Booster if we have scaled up else disable it */
>>+ if (ufshcd_enable_wb_if_scaling_up(hba))
>>+ ufshcd_wb_toggle(hba, scale_up);
>
>Hi Asutosh,
>
>This patch is the second complaint about the mechanism that toggles
>the WriteBooster during clock scaling. Can this mechanism be removed
>entirely?
>
>I think this commit introduced that mechanism: 3d17b9b5ab11 ("scsi:
>ufs: Add write booster feature support"; v5.8).
>
>Thanks,
>
>Bart.
Hello Bart,
Load based toggling of WB seemed fine to me then.
I haven't thought about another method to toggle WriteBooster yet.
Let me see if I can come up with something.
IMT if you have a mechanism in mind, please let me know.
-asd
>
On 1/4/23 06:10, Asutosh Das wrote:
> Load based toggling of WB seemed fine to me then.
> I haven't thought about another method to toggle WriteBooster yet.
> Let me see if I can come up with something.
> IMT if you have a mechanism in mind, please let me know.
Hi Asutosh,
Which UFS devices need this mechanism? All UFS devices I'm familiar with
can achieve wire speed for large write requests without enabling the
WriteBooster.
Thanks,
Bart.
On 1/3/23 00:24, Avri Altman wrote:
>
>> On 12/22/22 02:21, Johan Hovold wrote:
>>> + /* Enable Write Booster if we have scaled up else disable it */
>>> + if (ufshcd_enable_wb_if_scaling_up(hba))
>>> + ufshcd_wb_toggle(hba, scale_up);
>>
>> Hi Asutosh,
>>
>> This patch is the second complaint about the mechanism that toggles the
>> WriteBooster during clock scaling. Can this mechanism be removed entirely?
> commit 87bd05016a64 that introduced UFSHCD_CAP_WB_WITH_CLK_SCALING enables
> the platform vendors and OEMs to maintain wb toggling - should they choose so.
> Why remove it in its entirety?
Hi Avri,
I'm in favor of keeping kernel code simple :-)
UFSHCD_CAP_WB_WITH_CLK_SCALING controls whether or not clock scaling
affects the WriteBooster depending on which host controller is in use.
Shouldn't this depend on which UFS device is present instead of on the
type of host controller?
Thanks,
Bart.
> On 1/4/23 06:10, Asutosh Das wrote:
> > Load based toggling of WB seemed fine to me then.
> > I haven't thought about another method to toggle WriteBooster yet.
> > Let me see if I can come up with something.
> > IMT if you have a mechanism in mind, please let me know.
>
> Hi Asutosh,
>
> Which UFS devices need this mechanism? All UFS devices I'm familiar with can
> achieve wire speed for large write requests without enabling the WriteBooster.
This feature assures SLC-performance for writes to the WriteBooster buffer.
So enabling it is advantageous as far as write performance.
As for the toggling functionality, compared to e.g. enabling it on init and leave it on,
some flash vendors require it because of device health considerations.
This is not the case for us, so let others to comment.
Thanks,
Avri
>
> Thanks,
>
> Bart.
On 12/22/22 02:21, Johan Hovold wrote:
> diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> index bda61be5f035..5c3821b2fcf8 100644
> --- a/drivers/ufs/core/ufshcd.c
> +++ b/drivers/ufs/core/ufshcd.c
> @@ -1234,12 +1234,14 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> * clock scaling is in progress
> */
> ufshcd_scsi_block_requests(hba);
> + mutex_lock(&hba->wb_mutex);
> down_write(&hba->clk_scaling_lock);
>
> if (!hba->clk_scaling.is_allowed ||
> ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
> ret = -EBUSY;
> up_write(&hba->clk_scaling_lock);
> + mutex_unlock(&hba->wb_mutex);
> ufshcd_scsi_unblock_requests(hba);
> goto out;
> }
> @@ -1251,12 +1253,16 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> return ret;
> }
Please add an __acquires(&hba->wb_mutex) annotation for sparse.
> -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
> +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool scale_up)
> {
> - if (writelock)
> - up_write(&hba->clk_scaling_lock);
> - else
> - up_read(&hba->clk_scaling_lock);
> + up_write(&hba->clk_scaling_lock);
> +
> + /* Enable Write Booster if we have scaled up else disable it */
> + if (ufshcd_enable_wb_if_scaling_up(hba))
> + ufshcd_wb_toggle(hba, scale_up);
> +
> + mutex_unlock(&hba->wb_mutex);
> +
> ufshcd_scsi_unblock_requests(hba);
> ufshcd_release(hba);
> }
Please add a __releases(&hba->wb_mutex) annotation for sparse.
> @@ -1273,7 +1279,6 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
> static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
> {
> int ret = 0;
> - bool is_writelock = true;
>
> ret = ufshcd_clock_scaling_prepare(hba);
> if (ret)
> @@ -1302,15 +1307,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
> }
> }
>
> - /* Enable Write Booster if we have scaled up else disable it */
> - if (ufshcd_enable_wb_if_scaling_up(hba)) {
> - downgrade_write(&hba->clk_scaling_lock);
> - is_writelock = false;
> - ufshcd_wb_toggle(hba, scale_up);
> - }
> -
> out_unprepare:
> - ufshcd_clock_scaling_unprepare(hba, is_writelock);
> + ufshcd_clock_scaling_unprepare(hba, scale_up);
> return ret;
> }
This patch moves the ufshcd_wb_toggle() from before the out_unprepare
label to after the out_unprepare label (into
ufshcd_clock_scaling_unprepare()). Does this change perhaps introduce a
new call to ufshcd_wb_toggle() in error paths?
Thanks,
Bart.
>> On 1/4/23 06:10, Asutosh Das wrote:
>> > Load based toggling of WB seemed fine to me then.
>> > I haven't thought about another method to toggle WriteBooster yet.
>> > Let me see if I can come up with something.
>> > IMT if you have a mechanism in mind, please let me know.
>>
>> Hi Asutosh,
>>
>> Which UFS devices need this mechanism? All UFS devices I'm familiar with can
>> achieve wire speed for large write requests without enabling the WriteBooster.
>This feature assures SLC-performance for writes to the WriteBooster buffer.
>So enabling it is advantageous as far as write performance.
I agree with you. Also, it can be used in various ways.
>As for the toggling functionality, compared to e.g. enabling it on init and leave it on,
>some flash vendors require it because of device health considerations.
>This is not the case for us, so let others to comment.
In our case, it does not matter whether to toggle or not.
To make the code simple, it seems to be a good way to enable it on init and leave it on.
Considering device health, WB can be disabled through lifetime check.
Thanks,
Jinyoung.
>
>Thanks,
>Avri
>>
>> Thanks,
>>
>> Bart.
On Fri, Jan 06 2023 at 18:25 -0800, Jinyoung CHOI wrote:
>>> On 1/4/23 06:10, Asutosh Das wrote:
>>> > Load based toggling of WB seemed fine to me then.
>>> > I haven't thought about another method to toggle WriteBooster yet.
>>> > Let me see if I can come up with something.
>>> > IMT if you have a mechanism in mind, please let me know.
>>>
>>> Hi Asutosh,
>>>
>>> Which UFS devices need this mechanism? All UFS devices I'm familiar with can
>>> achieve wire speed for large write requests without enabling the WriteBooster.
>>This feature assures SLC-performance for writes to the WriteBooster buffer.
>>So enabling it is advantageous as far as write performance.
>
>I agree with you. Also, it can be used in various ways.
>
>>As for the toggling functionality, compared to e.g. enabling it on init and leave it on,
>>some flash vendors require it because of device health considerations.
>>This is not the case for us, so let others to comment.
>
>In our case, it does not matter whether to toggle or not.
>To make the code simple, it seems to be a good way to enable it on init and leave it on.
>Considering device health, WB can be disabled through lifetime check.
>
>Thanks,
>Jinyoung.
>
Hi Jinyoung / Avri / Bart
My understanding during the WriteBooster development was that the endurance of
the SLC buffer is considerably less and hence it's *not* a good idea to always
keep it ON. From the spec,
"
7279 Whenever the endurance of the WriteBooster Buffer is consumed completely, a write command is
7280 processed as if WriteBooster feature was disabled. Therefore, it is recommended to set fWriteBoosterEn to
7281 one, only when WriteBooster performance is needed, so that WriteBooster feature can be used for a longer
7282 time.
"
Going by this toggling it to ON when the load is high and performance is needed seemed reasonable.
Now then, if you confirm that there's no considerable difference in the WB buffer lifetime by
either always keeping it on at init OR on-demand, then this toggle code should be removed.
I will talk to other UFS device vendors who may not be active in this list and get back.
-asd
>>
>>Thanks,
>>Avri
>>>
>>> Thanks,
>>>
>>> Bart.
>
On Thu, Jan 05, 2023 at 10:38:58AM -0800, Bart Van Assche wrote:
> On 12/22/22 02:21, Johan Hovold wrote:
> > diff --git a/drivers/ufs/core/ufshcd.c b/drivers/ufs/core/ufshcd.c
> > index bda61be5f035..5c3821b2fcf8 100644
> > --- a/drivers/ufs/core/ufshcd.c
> > +++ b/drivers/ufs/core/ufshcd.c
> > @@ -1234,12 +1234,14 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> > * clock scaling is in progress
> > */
> > ufshcd_scsi_block_requests(hba);
> > + mutex_lock(&hba->wb_mutex);
> > down_write(&hba->clk_scaling_lock);
> >
> > if (!hba->clk_scaling.is_allowed ||
> > ufshcd_wait_for_doorbell_clr(hba, DOORBELL_CLR_TOUT_US)) {
> > ret = -EBUSY;
> > up_write(&hba->clk_scaling_lock);
> > + mutex_unlock(&hba->wb_mutex);
> > ufshcd_scsi_unblock_requests(hba);
> > goto out;
> > }
> > @@ -1251,12 +1253,16 @@ static int ufshcd_clock_scaling_prepare(struct ufs_hba *hba)
> > return ret;
> > }
>
> Please add an __acquires(&hba->wb_mutex) annotation for sparse.
>
> > -static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
> > +static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool scale_up)
> > {
> > - if (writelock)
> > - up_write(&hba->clk_scaling_lock);
> > - else
> > - up_read(&hba->clk_scaling_lock);
> > + up_write(&hba->clk_scaling_lock);
> > +
> > + /* Enable Write Booster if we have scaled up else disable it */
> > + if (ufshcd_enable_wb_if_scaling_up(hba))
> > + ufshcd_wb_toggle(hba, scale_up);
> > +
> > + mutex_unlock(&hba->wb_mutex);
> > +
> > ufshcd_scsi_unblock_requests(hba);
> > ufshcd_release(hba);
> > }
>
> Please add a __releases(&hba->wb_mutex) annotation for sparse.
This would actually introduce new sparse warnings as mutex_lock/unlock()
are not sparse annotated:
drivers/ufs/core/ufshcd.c:1254:9: warning: context imbalance in 'ufshcd_clock_scaling_prepare' - wrong count at exit
drivers/ufs/core/ufshcd.c:1266:9: warning: context imbalance in 'ufshcd_clock_scaling_unprepare' - wrong count at exit
drivers/ufs/core/ufshcd.c:1281:12: warning: context imbalance in 'ufshcd_devfreq_scale' - wrong count at exit
I guess it's not worth adding explicit __acquire()/__release() to these
helpers either (e.g. as they are only used in one function).
> > @@ -1273,7 +1279,6 @@ static void ufshcd_clock_scaling_unprepare(struct ufs_hba *hba, bool writelock)
> > static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
> > {
> > int ret = 0;
> > - bool is_writelock = true;
> >
> > ret = ufshcd_clock_scaling_prepare(hba);
> > if (ret)
> > @@ -1302,15 +1307,8 @@ static int ufshcd_devfreq_scale(struct ufs_hba *hba, bool scale_up)
> > }
> > }
> >
> > - /* Enable Write Booster if we have scaled up else disable it */
> > - if (ufshcd_enable_wb_if_scaling_up(hba)) {
> > - downgrade_write(&hba->clk_scaling_lock);
> > - is_writelock = false;
> > - ufshcd_wb_toggle(hba, scale_up);
> > - }
> > -
> > out_unprepare:
> > - ufshcd_clock_scaling_unprepare(hba, is_writelock);
> > + ufshcd_clock_scaling_unprepare(hba, scale_up);
> > return ret;
> > }
>
> This patch moves the ufshcd_wb_toggle() from before the out_unprepare
> label to after the out_unprepare label (into
> ufshcd_clock_scaling_unprepare()). Does this change perhaps introduce a
> new call to ufshcd_wb_toggle() in error paths?
Thanks for spotting that. I'll leave the setting unchanged on errors in
v2.
Johan