Re: [RFC PATCH 06/14] usb: core: hcd: Introduce USB HCD APIs for interrupter management

From: Alan Stern <stern@rowland.harvard.edu>
To: Wesley Cheng <quic_wcheng@quicinc.com>
Cc: Oliver Neukum <oneukum@suse.com>,
	srinivas.kandagatla@linaro.org, mathias.nyman@intel.com,
	perex@perex.cz, broonie@kernel.org, lgirdwood@gmail.com,
	andersson@kernel.org, krzysztof.kozlowski+dt@linaro.org,
	gregkh@linuxfoundation.org, Thinh.Nguyen@synopsys.com,
	bgoswami@quicinc.com, tiwai@suse.com, robh+dt@kernel.org,
	agross@kernel.org, linux-kernel@vger.kernel.org,
	linux-arm-msm@vger.kernel.org, alsa-devel@alsa-project.org,
	devicetree@vger.kernel.org, linux-usb@vger.kernel.org,
	quic_jackp@quicinc.com, quic_plai@quicinc.com
Subject: Re: [RFC PATCH 06/14] usb: core: hcd: Introduce USB HCD APIs for interrupter management
Date: Wed, 28 Dec 2022 20:41:56 -0500	[thread overview]
Message-ID: <Y6zwZOquZOTZfnvP@rowland.harvard.edu> (raw)
In-Reply-To: <559030ff-112b-e0a8-b278-72f909724496@quicinc.com>

On Wed, Dec 28, 2022 at 12:31:16PM -0800, Wesley Cheng wrote:
> Hi Alan,
> 
> On 12/28/2022 7:16 AM, Alan Stern wrote:
> > On Wed, Dec 28, 2022 at 09:59:03AM +0100, Oliver Neukum wrote:
> > > 
> > > 
> > > On 27.12.22 22:07, Wesley Cheng wrote:
> > > 
> > > > 
> > > > Hmmm...maybe I should change the name of the API then to avoid the confusion.  Yes, usb_hcd_flush_endpoint() does ensure that URBs submitted to the EP are stopped.  However, with this offloading concept, we aren't actually submitting URBs from the main processor, so the ep->urb_list will be empty.
> > > > 
> > > > This means the usb_hcd_flush_endpoint() API won't actually do anything.  What we need is to ensure that we send a XHCI stop ep command to the controller.
> > > 
> > > That is a concept specific to XHCI, yet you are adding a generic
> > > API. The namin should reflect that. usb_quiesce_endpoint() ?
> > 
> > Or even xhci_send_stop_ep_cmd(), which is what the routine is intended
> > to do.
> > 
> 
> Just to clarify, you're talking about renaming the API that was added in the
> XHCI driver, correct?

To be precise, we're talking about renaming your usb_hcd_stop_endpoint() 
function, although similar arguments probably apply to your 
usb_free_interrupter(), usb_set_interrupter(), and 
usb_hcd_get_transfer_resource() routines.

You wrote earlier:

	The XHCI driver is the one that maintains the list of 
	interrupters that are available, so the locking was placed in 
	the XHCI driver versus adding it in the core hcd layer.

The "stop ep" functionality and other interrupter management things you 
want to add seem a lot like this locking stuff.  Since you decided to 
put the locking in the xhci-hcd driver instead of the core HCD layer, it 
would be logical to do the same with the "stop ep" and other routines.  
Which means there shouldn't be any need to make changes to hcd.c or 
include/linux/usb/hcd.h.

Alan Stern