Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [BUG] usb: typec: ucsi: possible deadlock in ucsi_pr_swap() and
 ucsi_handle_connector_change()
To:     Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>, kyletso@google.com,
        jackp@codeaurora.org, andy.shevchenko@gmail.com,
        unixbhaskar@gmail.com, subbaram@codeaurora.org,
        mrana@codeaurora.org,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        linux-kernel <linux-kernel@vger.kernel.org>
References: <037de7ac-e210-bdf5-ec7a-8c0c88a0be20@gmail.com>
 <YgPQB9BYJcDzbd02@kuha.fi.intel.com>
From:   Jia-Ju Bai <baijiaju1990@gmail.com>
Message-ID: <43d6c3b5-17e6-63a8-21fa-3ff9f478ada7@gmail.com>
Date:   Thu, 10 Feb 2022 10:24:42 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Thunderbird/68.4.1
MIME-Version: 1.0
In-Reply-To: <YgPQB9BYJcDzbd02@kuha.fi.intel.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 8bit
Content-Language: en-US
Precedence: bulk


On 2022/2/9 22:30, Heikki Krogerus wrote:
> On Wed, Feb 09, 2022 at 11:50:57AM +0800, Jia-Ju Bai wrote:
>> Hello,
>>
>> My static analysis tool reports a possible deadlock in the ucsi driver in
>> Linux 5.16:
>>
>> ucsi_pr_swap()
>>  ? mutex_lock(&con->lock); --> Line 962 (Lock A)
>>  ? wait_for_completion_timeout(&con->complete, ...) --> Line 981 (Wait X)
>>
>> ucsi_handle_connector_change()
>>  ? mutex_lock(&con->lock); --> Line 763 (Lock A)
>>  ? complete(&con->complete); --> Line 782 (Wake X)
>>  ? complete(&con->complete); --> Line 807 (Wake X)
>>
>> When ucsi_pr_swap() is executed, "Wait X" is performed by holding "Lock A".
>> If ucsi_handle_connector_change() is executed at this time, "Wake X" cannot
>> be performed to wake up "Wait X" in ucsi_handle_connector_change(), because
>> "Lock A" has been already held by ucsi_handle_connector_change(), causing a
>> possible deadlock.
>> I find that "Wait X" is performed with a timeout, to relieve the possible
>> deadlock; but I think this timeout can cause inefficient execution.
>>
>> I am not quite sure whether this possible problem is real.
>> Any feedback would be appreciated, thanks :)
> This is probable a regression from commit ad74b8649bea ("usb: typec:
> ucsi: Preliminary support for alternate modes"). Can you test does
> this patch fix the issue (attached)?

Hi Heikki,

Thanks for the reply and patch.
After the patch is used, my tool does not report this deadlock in the 
ucsi driver.
Thus, I think this patch should be okay to fix the deadlock :)


Best wishes,
Jia-Ju Bai