From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 7ADB1C77B75 for ; Fri, 19 May 2023 15:07:31 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232168AbjESPHa (ORCPT ); Fri, 19 May 2023 11:07:30 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43556 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229468AbjESPH1 (ORCPT ); Fri, 19 May 2023 11:07:27 -0400 Received: from netrider.rowland.org (netrider.rowland.org [192.131.102.5]) by lindbergh.monkeyblade.net (Postfix) with SMTP id 5987A109 for ; Fri, 19 May 2023 08:07:26 -0700 (PDT) Received: (qmail 40772 invoked by uid 1000); 19 May 2023 11:07:25 -0400 Date: Fri, 19 May 2023 11:07:25 -0400 From: Alan Stern To: Badhri Jagan Sridharan Cc: gregkh@linuxfoundation.org, colin.i.king@gmail.com, xuetao09@huawei.com, quic_eserrao@quicinc.com, water.zhangjiantao@huawei.com, peter.chen@freescale.com, balbi@ti.com, francesco@dolcini.it, alistair@alistair23.me, stephan@gerhold.net, bagasdotme@gmail.com, luca@z3ntu.xyz, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Francesco Dolcini Subject: Re: [PATCH v2] usb: gadget: udc: core: Offload usb_udc_vbus_handler processing Message-ID: References: <20230519043041.1593578-1-badhri@google.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, May 19, 2023 at 10:49:49AM -0400, Alan Stern wrote: > On Fri, May 19, 2023 at 04:30:41AM +0000, Badhri Jagan Sridharan wrote: > > chipidea udc calls usb_udc_vbus_handler from udc_start gadget > > ops causing a deadlock. Avoid this by offloading usb_udc_vbus_handler > > processing. > > Look, this is way overkill. > > usb_udc_vbus_handler() has only two jobs to do: set udc->vbus and call > usb_udc_connect_control(). Furthermore, it gets called from only two > drivers: chipidea and max3420. > > Why not have the callers set udc->vbus themselves and then call > usb_gadget_{dis}connect() directly? Then we could eliminate > usb_udc_vbus_handler() entirely. And the unnecessary calls -- the ones > causing deadlocks -- from within udc_start() and udc_stop() handlers can > be removed with no further consequence. > > This approach simplifies and removes code. Whereas your approach > complicates and adds code for no good reason. I changed my mind. After looking more closely, I found the comment in gadget.h about ->disconnect() callbacks happening in interrupt context. This means we cannot use a mutex to protect the associated state, and therefore the connect_lock _must_ be a spinlock, not a mutex. This also probably means that udc_start and udc_stop callbacks should not be invoked with the lock held. In fact, you might want to avoid using the lock at all with gadget_bind_driver() and gadget_unbind_driver() -- use it only in the functions that these routines call. So it appears the whole connect_lock thing needs to be redesigned with these ideas in mind. However, it's still true that the UDC drivers shouldn't try to set the connection state from within their udc_start and udc_stop callbacks, because the core takes care of this automatically. Alan Stern