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;
From:   =?ISO-8859-1?Q?J=E9r=F4me?= Pouiller <jerome.pouiller@silabs.com>
To:     Jia-Ju Bai <baijiaju1990@gmail.com>,
        Hillf Danton <hdanton@sina.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        linux-staging@lists.linux.dev,
        linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [BUG] staging: wfx: possible deadlock in wfx_conf_tx() and wfx_add_interface()
Date:   Tue, 01 Feb 2022 12:56:01 +0100
Message-ID: <1710740.8zh8KgFqrM@pc-42>
Organization: Silicon Labs
In-Reply-To: <20220201113303.3883-1-hdanton@sina.com>
References: <6f489bf2-bac0-8030-7ea5-6f5c12daa568@gmail.com> <20220201113303.3883-1-hdanton@sina.com>
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset="iso-8859-1"
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?gNDqpPuthlN5zCAHbDZXQKBkSUZLhk0a0cTD1MznQ6xy8YrCEhbR7deVeu?=
 =?iso-8859-1?Q?0vBnHGKWbgiMYN68+CsB1YmvQq/fKe/PKZHoKKnaDonewGTHegOmIZzW9c?=
 =?iso-8859-1?Q?h0OGHnq1nZzGI2pB1y0m1Afjr8WzXdVfSB1O7P1MJnY/Z7AwBDcwMDpOJf?=
 =?iso-8859-1?Q?lTBY4I4OJ+6NHJMApO05v+7QPwnKKw7jSOFbQAxlWrBg96wyBYvA4POW7t?=
 =?iso-8859-1?Q?y0y7Pii8NI3gEp30xp+4Pw04n/z044udDhpy1uHK5Pe4Ir/LwUngA5Jqzl?=
 =?iso-8859-1?Q?W0Hq+kFmPWTqT0L1TdALQw4zzjC5vvhmGSD6/Dr+9/BR/YKrxCDr7ko8Hw?=
 =?iso-8859-1?Q?rsw/RpVqEHgGYd8J1hZRO6oqBp+OxDxrmg4uv8oeg31c7XbrVKIj6hV78H?=
 =?iso-8859-1?Q?znyLqqPvtO5NafTMKgvWWC5F1A20Nzo5I3yFjUtarhht3dQt6TC+55K8W6?=
 =?iso-8859-1?Q?cGToO2SI+Hhep+HZcnGhYBzhW2ipniODgtr/zwYB/zz643dLdZ5YVJaAMK?=
 =?iso-8859-1?Q?ue2a/GxCfctltSwa9pDGDKpOkFZMozA4CB0BBgJoLMhZyu+Cy8Jho/J5E/?=
 =?iso-8859-1?Q?JIANeNKzLYtuf5XxJUAyXnW+fwmR0ifl6MIEK3vrr8GmPK5kFFcqzWcHr0?=
 =?iso-8859-1?Q?gw/gEVDrIKRodm7LAqX4Ul2cc10Zp9s3/NNa7BS74SeNmiL0xRbZJm2OBL?=
 =?iso-8859-1?Q?B6hkbA8uOvX7JThb/08PBn3hoJqElJf7qzhvzqYH+YT8DR593zWXJ40l3S?=
 =?iso-8859-1?Q?sZMsKwg9TZppouG5lteamWXEBwRGChF23gYbDG0bBHZQF7OVnxxOGOgoIe?=
 =?iso-8859-1?Q?ogCvAUsGfiX3Yv4Gj/+Gkng+XGtF/uURw8Lz7GXit08mamBv5R0UO14YH1?=
 =?iso-8859-1?Q?GA5Q/aQLDRox8sAq+cR0+/kmLiQ9UG5jtBKP9EHIeLtSOdAh4S0fDSXFOc?=
 =?iso-8859-1?Q?mKXVxHyvxILW6jpyN8Lg4gBr0IyIdgbqbnflCRKJ+RG+Ko3QgWQXvWZma0?=
 =?iso-8859-1?Q?vCRsg6q9eJwH3fsi3uP3hxSnPlg9hzEbUJa8P3GFq7ErzS5Vnuob29C+5h?=
 =?iso-8859-1?Q?dSdBk+duydWDzX/6JG+a1dCVZ/CywZvGriihs/SmFB5jks5r+9bEKykHX+?=
 =?iso-8859-1?Q?rX5Fhkcyai+dm64PkhNhmZelpWFCCf1qT3PxVe9/vZwT4nwwmQzsdnCjTk?=
 =?iso-8859-1?Q?oYUYUR5hM6NWWwxUw8rrvvYFDAz0GoFyNhlRZT+F5oWAauUdx58GTSkqTz?=
 =?iso-8859-1?Q?wHSFoYNHNPRzldQ3q9XSisMcck+Qkj5aZPTMoagxZxtKBIfTKnPssMOjB6?=
 =?iso-8859-1?Q?2Dj5k8ODVLQaKK0s38ZQSnk3sDXUT456UIe4NVsvCEnzYPlFlbR09Im2VB?=
 =?iso-8859-1?Q?9ns2nv1a6Tq1TaHCS377IE4cmCVQ+zvU2Pv81e87GRaQrq7ywggCrd3eNO?=
 =?iso-8859-1?Q?Cydrqxnd3jwAuAQoWhNMlpiunrOq7ziVNhQ5QTt4xz5i0UTdsedV7QBi/M?=
 =?iso-8859-1?Q?pVBGQgvkJP887RJm41BegpihOJF3RQKJ64bNZkQnIBKRQINPlxXQufvw7y?=
 =?iso-8859-1?Q?TSB6GHF+2+A/c+g0jMiFsW/4jj1hTLooY2QzZtjdLCCPoiuB5ea9M1abs1?=
 =?iso-8859-1?Q?HGeJP2daRrhnW1ccdmj0sgTvnhWRk9H6MCEtFxlTQE5MZR2uO6Sv4BOmaJ?=
 =?iso-8859-1?Q?CV2PMVWS18ZDAT7fIrU=3D?=
X-OriginatorOrg: silabs.com
X-MS-Exchange-CrossTenant-Network-Message-Id: dce87822-fdd5-4790-c993-08d9e579d59a
X-MS-Exchange-CrossTenant-AuthSource: PH0PR11MB5657.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 01 Feb 2022 11:56:08.9630
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 54dbd822-5231-4b20-944d-6f4abcd541fb
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: NkuOWSJu1VCD+X1YPu69AXEJSEfqKwDSmPtU6lCZu8AoxADhx5uS/ntha2DvFXOhkZx8ilKZg1/VQZOKo/iJKQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN8PR11MB3810
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello,

On Tuesday 1 February 2022 12:33:03 CET Hillf Danton wrote:
> On Tue, 1 Feb 2022 15:09:34 +0800 Jia-Ju Bai wrote:
> > Hello,
> >
> > My static analysis tool reports a possible deadlock in the wfx driver i=
n
> > Linux 5.16:
> >
> > wfx_conf_tx()
> >    mutex_lock(&wdev->conf_mutex); --> Line 225 (Lock A)
> >    wfx_update_pm()
> >      wait_for_completion_timeout(&wvif->set_pm_mode_complete, ...); -->
> > Line 3019 (Wait X)
> >
> > wfx_add_interface()
> >    mutex_lock(&wdev->conf_mutex); --> Line 737 (Lock A)
> >    complete(&wvif->set_pm_mode_complete); --> Line 758 (Wake X)
> >
> > When wfx_conf_tx() is executed, "Wait X" is performed by holding "Lock
> > A". If wfx_add_interface() is executed at this time, "Wake X" cannot be
> > performed to wake up "Wait X" in wfx_conf_tx(), because "Lock A" has
> > been already hold by wfx_conf_tx(), 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 execu=
tion.
> >
> > I am not quite sure whether this possible problem is real and how to fi=
x
> > it if it is real.
> > Any feedback would be appreciated, thanks :)
> >
> >
> > Best wishes,
> > Jia-Ju Bai
>=20
> Hey Jia-Ju
>=20
> Thank you for reporting it.
>=20
> Given the init_completion() prior to complete() in wfx_add_interface(),
> no waiter is waken up by the complete(), so it has nothing to do with
> the waiter in the conf path.

Absolutely. The completion is done by wfx_hif_pm_mode_complete_indication()
(which is not behind a mutex).

> BTW if the unusual wfx init is a real use case then we can add a new help=
er.

Indeed, it could make the code better. I don't know if there would be other
users.


--=20
J=E9r=F4me Pouiller