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;
Message-ID: <8ed7760a-471d-19a2-0a3b-1e0fc3a27281@suse.com>
Date:   Thu, 7 Apr 2022 10:01:43 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH 02/11] drivers: usb: host: Fix deadlock in
 oxu_bus_suspend()
Content-Language: en-US
To:     Duoming Zhou <duoming@zju.edu.cn>, linux-kernel@vger.kernel.org
CC:     chris@zankel.net, jcmvbkbc@gmail.com, mustafa.ismail@intel.com,
        shiraz.saleem@intel.com, jgg@ziepe.ca, wg@grandegger.com,
        mkl@pengutronix.de, davem@davemloft.net, kuba@kernel.org,
        pabeni@redhat.com, jes@trained-monkey.org,
        gregkh@linuxfoundation.org, jirislaby@kernel.org,
        alexander.deucher@amd.com, linux-xtensa@linux-xtensa.org,
        linux-rdma@vger.kernel.org, linux-can@vger.kernel.org,
        netdev@vger.kernel.org, linux-hippi@sunsite.dk,
        linux-staging@lists.linux.dev, linux-serial@vger.kernel.org,
        linux-usb@vger.kernel.org
References: <cover.1649310812.git.duoming@zju.edu.cn>
 <8b1201dc7554a2ab3ca555a2b6e2747761603d19.1649310812.git.duoming@zju.edu.cn>
From:   Oliver Neukum <oneukum@suse.com>
In-Reply-To: <8b1201dc7554a2ab3ca555a2b6e2747761603d19.1649310812.git.duoming@zju.edu.cn>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 2
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?T9SPt2wJrGUHywXJ/sCDf0aK6Xe6Yhg2Jr7+YedWfqMdlpdQ97Q9nieTLi5a?=
 =?us-ascii?Q?MdIrw9Vj+TD50byWNO53xVTEM2VizRhVkO9QW5tTAl4QTjg7D1w+QejVo1om?=
 =?us-ascii?Q?mh+2Ch7Dh+FD0aQlwGJBv1EqT7zm5o7Z2G6VafZzjAmgKV70+Pbi0Z7PtXyS?=
 =?us-ascii?Q?TcdvKRIuMqFRWUe63oY6cnKvbyMqCvC+2EmzdyYINlXf+J19TJzU0z0dc/jy?=
 =?us-ascii?Q?wIKufrMfAI6oP6i4nkXbung3SKqTa1Sz77pSutmg1m5/eptSv7f9guWcMAzy?=
 =?us-ascii?Q?EjpsPTPbUlUhIVJ2qfhgu0XM1V5jV+7GallXx5LpE8et/SF1ClfljBLGcdLr?=
 =?us-ascii?Q?QZQ6Et8kUc4FMnC4EObVhmwcOcCqovlNyTOnYl1B5eCv4oSGCXr2h7FbPaIg?=
 =?us-ascii?Q?+q6HU7OVhiD+oWsTj8Myj45v7eZykW1/R8VXd2bvyIkZx/5LBGF4bG+gGDBg?=
 =?us-ascii?Q?JTChcyBgOzaGFSV1vtTHfowh3T4QFUr+WsyzQ/B7N4uAHYYj0Ae0xMW6xvtw?=
 =?us-ascii?Q?TS2osd1oc888ONad0blb4q94sb5ByjAH1HopMTX9NoAHmt2aGDhub+0QOgg+?=
 =?us-ascii?Q?yQBRGvZJwjYuZGVxUN7geSL3KiZA9knYDDhxyOvJtt1m5zTqv3XcX5vHnjDm?=
 =?us-ascii?Q?Je9Gif8lxfwK3yftZ2+lBmftMlMkITZM1fHbCN+8O8pDO3YISfcOn1Liu/na?=
 =?us-ascii?Q?ruyRM2AQ3CUl/ejMohJPdufuVisAzqVVhiqHTsfSpAr+gKWCTtXEHeHneSzC?=
 =?us-ascii?Q?cxDZ1HIhNRJxmJ9cFMn0WjaBBpBcCRb9WZP/v4WnT2uGXE7Ohw6BWsUFu9Yk?=
 =?us-ascii?Q?qZaoe8yMVwL8EzIb50A7193TbOAjMU+P0Iyj/Yq+WhDIul5iv/+0uQuCBuLE?=
 =?us-ascii?Q?PaGJMnWCHCxBl2SGIZJmtD/Sl0xwC1+xwX76m8I3qqnU9/0Pl9xAE2D/1goo?=
 =?us-ascii?Q?zsQLLsBRtDQWgzllyNEwej2wB/KaqG8Qex7rtDauoGASiQkSVTNsxv/WMfjt?=
 =?us-ascii?Q?uA9IduIT2yyo7xgYGX7KoUON4EEM/cfK/I5h3awpM3HLDp761DypRPDp+RFb?=
 =?us-ascii?Q?ITX21KcwL0odxXN6onVhk3O2HerlOqh9K6d+7XTy1sQTkGiRlUiSwAhn48T3?=
 =?us-ascii?Q?Nr519KwhUuuehapwxU628+oLWSdwGnhHjF2zZDZsC1o0M2X4hRx4LdCYo2d8?=
 =?us-ascii?Q?1oIozVD8lzi5s9ILGSCVhBStFMzzt8Urm8uHeyL5ZvaH094J7e/zlZxuXSMT?=
 =?us-ascii?Q?UYIk4ybG6f5idQuTsu5gRectV+v7m0afVl9tkrbNYp9eOiSv7jEHJfOtiN6X?=
 =?us-ascii?Q?M1G7yINZuIfM97yJGU+JGqhbX4RqmVmNzAFSfh0oxe+g6K9O2aNtXoNseAmv?=
 =?us-ascii?Q?fFBtiHJYgAeozBLZTeDDlsSghjnAro/1kmgrvMe0MHYaWhkwAQtBKci5Nubk?=
 =?us-ascii?Q?PwlQXFXqKKO9+z0W3py9qFOjZYarTOiHQtnK0ShtGmlUYKaP1Sc6hhHqh3e0?=
 =?us-ascii?Q?eyHR/IuHp8yqtWTqI3IMf08BzDCVrHFskYnylYZ8ANDIXr9Z/hBYgGTwxYgo?=
 =?us-ascii?Q?kREIhiEk1oMQ5CYPmoc8hMK78WB4crLuz+hd7sS1RDrVtBdEsj0cgTX0QGTV?=
 =?us-ascii?Q?PtNN1DKH3PSC/u3zXS8Ha3wQxiabxLvmYGOTEwDkHl9du2Qct1eF07O894IP?=
 =?us-ascii?Q?TC/a0bOS1ZzzjwzHKrGa4WypKFRGArmfFmoHi1MY/+CxjrBcr73ZtV/baXG+?=
 =?us-ascii?Q?HWUcPmcVL5IKS4BjGgT0WNvg+Ho82SA2k5zTGmG663RYceOj2VxszbaQIlU7?=
X-MS-Exchange-AntiSpam-MessageData-1: vDNhRNnWoiNPGg==
X-OriginatorOrg: suse.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c60fd6fc-55c9-4762-b7d9-08da186ce077
X-MS-Exchange-CrossTenant-AuthSource: VI1PR0401MB2526.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Apr 2022 08:01:53.1416
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: f7a17af6-1c5c-4a36-aa8b-f5be247aa4ba
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 7fjZPmdWT0psiNNmwya64OxxthZhzquIVbI0XfaO0GAVpFvvI98W/ZOpyNQE32g3KnivHZElI3uTsoQufYQPUA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR04MB5629
X-Spam-Status: No, score=-5.7 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H4,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=ham autolearn_force=no
        version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 07.04.22 08:33, Duoming Zhou wrote:
> There is a deadlock in oxu_bus_suspend(), which is shown below:
>
>    (Thread 1)              |      (Thread 2)
>                            | timer_action()
> oxu_bus_suspend()          |  mod_timer()
>  spin_lock_irq() //(1)     |  (wait a time)
>  ...                       | oxu_watchdog()
>  del_timer_sync()          |  spin_lock_irq() //(2)
>  (wait timer to stop)      |  ...
>
> We hold oxu->lock in position (1) of thread 1, and use
> del_timer_sync() to wait timer to stop, but timer handler
> also need oxu->lock in position (2) of thread 2. As a result,
> oxu_bus_suspend() will block forever.
>
> This patch extracts del_timer_sync() from the protection of
> spin_lock_irq(), which could let timer handler to obtain
> the needed lock.
Good catch.
> Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
> ---
>  drivers/usb/host/oxu210hp-hcd.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/host/oxu210hp-hcd.c b/drivers/usb/host/oxu210hp-=
hcd.c
> index b741670525e..ee403df3309 100644
> --- a/drivers/usb/host/oxu210hp-hcd.c
> +++ b/drivers/usb/host/oxu210hp-hcd.c
> @@ -3909,8 +3909,10 @@ static int oxu_bus_suspend(struct usb_hcd *hcd)
>  		}
>  	}
> =20
> +	spin_unlock_irq(&oxu->lock);
>  	/* turn off now-idle HC */
>  	del_timer_sync(&oxu->watchdog);
> +	spin_lock_irq(&oxu->lock);
>  	ehci_halt(oxu);
>  	hcd->state =3D HC_STATE_SUSPENDED;
> =20

What is the lock protecting at that stage? Why not just drop it earlier.

=C2=A0=C2=A0=C2=A0 Regards
=C2=A0=C2=A0=C2=A0 =C2=A0=C2=A0=C2=A0 Oliver