From: Can Guo <cang@codeaurora.org>
To: Bart Van Assche <bvanassche@acm.org>
Cc: asutoshd@codeaurora.org, nguyenb@codeaurora.org,
hongwus@codeaurora.org, ziqichen@codeaurora.org,
linux-scsi@vger.kernel.org, kernel-team@android.com,
Alim Akhtar <alim.akhtar@samsung.com>,
Avri Altman <avri.altman@wdc.com>,
"James E.J. Bottomley" <jejb@linux.ibm.com>,
"Martin K. Petersen" <martin.petersen@oracle.com>,
Bean Huo <beanhuo@micron.com>,
Stanley Chu <stanley.chu@mediatek.com>,
Jaegeuk Kim <jaegeuk@kernel.org>,
Adrian Hunter <adrian.hunter@intel.com>,
open list <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] scsi: ufs: Fix a possible dead lock in clock scaling
Date: Thu, 30 Sep 2021 11:57:38 +0800 [thread overview]
Message-ID: <27e9265371e96d0bcc06139ce5f0e026@codeaurora.org> (raw)
In-Reply-To: <644dcd92-25ae-e951-d9f3-607306a02370@acm.org>
On 2021-09-30 02:15, Bart Van Assche wrote:
> On 9/28/21 8:31 PM, Can Guo wrote:
>> On 2021-09-18 01:27, Bart Van Assche wrote:
>>> On 9/16/21 6:51 PM, Can Guo wrote:
>>>> Assume a scenario where task A and B call ufshcd_devfreq_scale()
>>>> simultaneously. After task B calls downgrade_write() [1], but before
>>>> it
>>>> calls down_read() [3], if task A calls down_write() [2], when task B
>>>> calls
>>>> down_read() [3], it will lead to dead lock.
>>>
>>> Something is wrong with the above description. The downgrade_write()
>>> call is
>>> not followed by down_read() but by up_read(). Additionally, I don't
>>> see how
>>> concurrent calls of ufshcd_devfreq_scale() could lead to a deadlock.
>>
>> As mentioned in the commit msg, the down_read() [3] is from
>> ufshcd_wb_ctrl().
>>
>> Task A -
>> down_write [2]
>> ufshcd_clock_scaling_prepare
>> ufshcd_devfreq_scale
>> ufshcd_clkscale_enable_store
>>
>> Task B -
>> down_read [3]
>> ufshcd_exec_dev_cmd
>> ufshcd_query_flag
>> ufshcd_wb_ctrl
>> downgrade_write [1]
>> ufshcd_devfreq_scale
>> ufshcd_devfreq_target
>> devfreq_set_target
>> update_devfreq
>> devfreq_performance_handler
>> governor_store
>>
>>
>>> If one thread calls downgrade_write() and another thread calls
>>> down_write()
>>> immediately, that down_write() call will block until the other thread
>>> has called up_read()
>>> without triggering a deadlock.
>>
>> Since the down_write() caller is blocked, the down_read() caller,
>> which comes after
>> down_write(), is blocked too, no? downgrade_write() keeps lock owner
>> as it is, but
>> it does not change the fact that readers and writers can be blocked by
>> each other.
>
> Please use the upstream function names when posting upstream patches.
> I think that
> ufshcd_wb_ctrl() has been renamed into ufshcd_wb_toggle().
>
> So the deadlock is caused by nested locking - one task holding a
> reader lock, another
> task calling down_write() and next the first task grabbing the reader
> lock recursively?
> I prefer one of the following two solutions above the patch that has
> been posted since
> I expect that both alternatives will result in easier to maintain UFS
> code:
> - Fix the down_read() implementation. Making down_read() wait in case
> of nested locking
> seems wrong to me.
> - Modify the UFS driver such that it does not lock
> hba->clk_scaling_lock recursively.
My current change is the 2nd solution - drop the hba->clk_scaling_lock
before calls ufshcd_wb_toggle() to avoid recursive lock.
Thanks,
Can Guo.
>
> Thanks,
>
> Bart.
next prev parent reply other threads:[~2021-09-30 3:58 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-17 1:51 [PATCH] scsi: ufs: Fix a possible dead lock in clock scaling Can Guo
2021-09-17 17:27 ` Bart Van Assche
2021-09-29 3:31 ` Can Guo
2021-09-29 18:15 ` Bart Van Assche
2021-09-30 3:57 ` Can Guo [this message]
2021-09-30 6:02 ` Adrian Hunter
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=27e9265371e96d0bcc06139ce5f0e026@codeaurora.org \
--to=cang@codeaurora.org \
--cc=adrian.hunter@intel.com \
--cc=alim.akhtar@samsung.com \
--cc=asutoshd@codeaurora.org \
--cc=avri.altman@wdc.com \
--cc=beanhuo@micron.com \
--cc=bvanassche@acm.org \
--cc=hongwus@codeaurora.org \
--cc=jaegeuk@kernel.org \
--cc=jejb@linux.ibm.com \
--cc=kernel-team@android.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=nguyenb@codeaurora.org \
--cc=stanley.chu@mediatek.com \
--cc=ziqichen@codeaurora.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).