diff mbox series

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

Message ID 20221223233200.26089-7-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series Introduce QC USB SND audio offloading support | expand

Commit Message

Wesley Cheng Dec. 23, 2022, 11:31 p.m. UTC
For USB HCDs that can support multiple USB interrupters, expose functions
that class drivers can utilize for setting up secondary interrupters.
Class drivers can pass this information to its respective clients, i.e.
a dedicated DSP.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
 drivers/usb/core/hcd.c  | 86 +++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h     |  7 ++++
 include/linux/usb/hcd.h | 16 +++++++-
 3 files changed, 108 insertions(+), 1 deletion(-)

Comments

Greg Kroah-Hartman Dec. 24, 2022, 8:54 a.m. UTC | #1
On Fri, Dec 23, 2022 at 03:31:52PM -0800, Wesley Cheng wrote:
> For USB HCDs that can support multiple USB interrupters, expose functions
> that class drivers can utilize for setting up secondary interrupters.
> Class drivers can pass this information to its respective clients, i.e.
> a dedicated DSP.

Where is the locking here that seems to be required when a hcd is
removed from the system and you have data in flight?  What am I missing
here in the design of this?

And yes, HCDs get removed all the time, and will happen more and more in
the future with the design of more systems moving to Thunderbolt/PCIe
designs due to the simplicity of it.

> +/**
> + * usb_set_interruper - Reserve an interrupter

Where is an "interrupter" defined?  I don't know what this term means
sorry, is this in the USB specification somewhere?


> + * @udev: usb device which requested the interrupter
> + * @intr_num: interrupter number to reserve
> + * @dma: iova address to event ring
> + *
> + * Request for a specific interrupter to be reserved for a USB class driver.
> + * This will return the base address to the event ring that was allocated to
> + * the specific interrupter.
> + **/
> +phys_addr_t usb_set_interruper(struct usb_device *udev, int intr_num,
> +							dma_addr_t *dma)
> +{
> +	struct usb_hcd *hcd;
> +	phys_addr_t pa = 0;
> +
> +	hcd = bus_to_hcd(udev->bus);
> +	if (hcd->driver->update_interrupter)
> +		pa = hcd->driver->update_interrupter(hcd, intr_num, dma);
> +
> +	return pa;

Wait, you return a physical address?  What are you going to do with
that?  And what guarantees that the address is valid after you return it
(again, remember memory and devices can be removed at any point in time.

thanks,

greg k-h
Alan Stern Dec. 24, 2022, 3:29 p.m. UTC | #2
On Fri, Dec 23, 2022 at 03:31:52PM -0800, Wesley Cheng wrote:
> For USB HCDs that can support multiple USB interrupters, expose functions
> that class drivers can utilize for setting up secondary interrupters.
> Class drivers can pass this information to its respective clients, i.e.
> a dedicated DSP.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>  drivers/usb/core/hcd.c  | 86 +++++++++++++++++++++++++++++++++++++++++
>  include/linux/usb.h     |  7 ++++
>  include/linux/usb/hcd.h | 16 +++++++-
>  3 files changed, 108 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 8300baedafd2..90ead90faf1d 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c

> +/**
> + * usb_hcd_stop_endpoint - Halt USB EP transfers
> + * @udev: usb device
> + * @ep: usb ep to stop
> + *
> + * Stop pending transfers on a specific USB endpoint.
> + **/
> +int usb_hcd_stop_endpoint(struct usb_device *udev,
> +					struct usb_host_endpoint *ep)
> +{
> +	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
> +	int ret = 0;
> +
> +	if (hcd->driver->stop_endpoint)
> +		ret = hcd->driver->stop_endpoint(hcd, udev, ep);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_hcd_stop_endpoint);

You know, there already is a function that does this.  It's named 
usb_hcd_flush_endpoint().  No need to add another function that does the 
same thing.

Alan Stern
Wesley Cheng Dec. 27, 2022, 9:07 p.m. UTC | #3
Hi Alan,

On 12/24/2022 7:29 AM, Alan Stern wrote:
> On Fri, Dec 23, 2022 at 03:31:52PM -0800, Wesley Cheng wrote:
>> For USB HCDs that can support multiple USB interrupters, expose functions
>> that class drivers can utilize for setting up secondary interrupters.
>> Class drivers can pass this information to its respective clients, i.e.
>> a dedicated DSP.
>>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/core/hcd.c  | 86 +++++++++++++++++++++++++++++++++++++++++
>>   include/linux/usb.h     |  7 ++++
>>   include/linux/usb/hcd.h | 16 +++++++-
>>   3 files changed, 108 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 8300baedafd2..90ead90faf1d 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
> 
>> +/**
>> + * usb_hcd_stop_endpoint - Halt USB EP transfers
>> + * @udev: usb device
>> + * @ep: usb ep to stop
>> + *
>> + * Stop pending transfers on a specific USB endpoint.
>> + **/
>> +int usb_hcd_stop_endpoint(struct usb_device *udev,
>> +					struct usb_host_endpoint *ep)
>> +{
>> +	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
>> +	int ret = 0;
>> +
>> +	if (hcd->driver->stop_endpoint)
>> +		ret = hcd->driver->stop_endpoint(hcd, udev, ep);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_hcd_stop_endpoint);
> 
> You know, there already is a function that does this.  It's named
> usb_hcd_flush_endpoint().  No need to add another function that does the
> same thing.
> 

Thanks for the suggestion and review.

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.

Thanks
Wesley Cheng
Wesley Cheng Dec. 27, 2022, 9:13 p.m. UTC | #4
Hi Greg,

On 12/24/2022 12:54 AM, Greg KH wrote:
> On Fri, Dec 23, 2022 at 03:31:52PM -0800, Wesley Cheng wrote:
>> For USB HCDs that can support multiple USB interrupters, expose functions
>> that class drivers can utilize for setting up secondary interrupters.
>> Class drivers can pass this information to its respective clients, i.e.
>> a dedicated DSP.
> 
> Where is the locking here that seems to be required when a hcd is
> removed from the system and you have data in flight?  What am I missing
> here in the design of this?

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.

> 
> And yes, HCDs get removed all the time, and will happen more and more in
> the future with the design of more systems moving to Thunderbolt/PCIe
> designs due to the simplicity of it.
> 

As part of the HCD removal, it has to first ensure that class driver 
interfaces, and the connected udevs are removed first.  qc_audio_offload 
will first handle the transfer cleanup and stopping of the audio stream 
before returning from the disconnect callback. (this includes ensuring 
that the interrupter is released)

This concept is how all usb class drivers are currently implemented. 
When the HCD remove occurs, the class drivers are the ones responsible 
for ensuring that all URBs are stopped, and removed before it returns 
from its respective disconnect callback.

>> +/**
>> + * usb_set_interruper - Reserve an interrupter
> 
> Where is an "interrupter" defined?  I don't know what this term means
> sorry, is this in the USB specification somewhere?
> 

Interrupter is defined in the XHCI spec, refer to "section 4.17 
Interrupters"

> 
>> + * @udev: usb device which requested the interrupter
>> + * @intr_num: interrupter number to reserve
>> + * @dma: iova address to event ring
>> + *
>> + * Request for a specific interrupter to be reserved for a USB class driver.
>> + * This will return the base address to the event ring that was allocated to
>> + * the specific interrupter.
>> + **/
>> +phys_addr_t usb_set_interruper(struct usb_device *udev, int intr_num,
>> +							dma_addr_t *dma)
>> +{
>> +	struct usb_hcd *hcd;
>> +	phys_addr_t pa = 0;
>> +
>> +	hcd = bus_to_hcd(udev->bus);
>> +	if (hcd->driver->update_interrupter)
>> +		pa = hcd->driver->update_interrupter(hcd, intr_num, dma);
>> +
>> +	return pa;
> 
> Wait, you return a physical address?  What are you going to do with
> that?  And what guarantees that the address is valid after you return it
> (again, remember memory and devices can be removed at any point in time.
> 

The interrupter is basically another event ring which is the buffer 
allocated for the controller to copy events into.  Since the audio dsp 
now takes over handling of the endpoint events, then it needs to know 
where the buffer resides.  Will fix the interruper typo as well.

The allocation and freeing of this event ring follows how XHCI allocates 
and frees the main event ring as well.  This API just reserves the 
interrupter for the class driver, and return the previously allocated 
(during XHCI init) memory address.

Thanks
Wesley Cheng
Oliver Neukum Dec. 28, 2022, 8:59 a.m. UTC | #5
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() ?

	Regards
		Oliver
Alan Stern Dec. 28, 2022, 3:16 p.m. UTC | #6
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.

Alan Stern
Wesley Cheng Dec. 28, 2022, 8:31 p.m. UTC | #7
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?

Thanks
Wesley Cheng
Alan Stern Dec. 29, 2022, 1:41 a.m. UTC | #8
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
diff mbox series

Patch

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 8300baedafd2..90ead90faf1d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1579,6 +1579,92 @@  int usb_hcd_submit_urb (struct urb *urb, gfp_t mem_flags)
 	return status;
 }
 
+/**
+ * usb_free_interrupter - free an interrupter
+ * @udev: usb device which requested the interrupter
+ * @intr_num: interrupter number to free
+ *
+ * Release the USB HCD interrupter that was reserved by a USB class driver.
+ **/
+int usb_free_interrupter(struct usb_device *udev, int intr_num)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+	int ret = 0;
+
+	if (hcd->driver->free_interrupter)
+		ret = hcd->driver->free_interrupter(hcd, intr_num);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_free_interrupter);
+
+/**
+ * usb_set_interruper - Reserve an interrupter
+ * @udev: usb device which requested the interrupter
+ * @intr_num: interrupter number to reserve
+ * @dma: iova address to event ring
+ *
+ * Request for a specific interrupter to be reserved for a USB class driver.
+ * This will return the base address to the event ring that was allocated to
+ * the specific interrupter.
+ **/
+phys_addr_t usb_set_interruper(struct usb_device *udev, int intr_num,
+							dma_addr_t *dma)
+{
+	struct usb_hcd *hcd;
+	phys_addr_t pa = 0;
+
+	hcd = bus_to_hcd(udev->bus);
+	if (hcd->driver->update_interrupter)
+		pa = hcd->driver->update_interrupter(hcd, intr_num, dma);
+
+	return pa;
+}
+EXPORT_SYMBOL_GPL(usb_set_interruper);
+
+/**
+ * usb_hcd_get_transfer_resource - Retrieve USB EP resource information
+ * @udev: usb device
+ * @ep: usb ep to retrieve resource information
+ * @dma: iova address to transfer resource
+ *
+ * Request for USB EP transfer resource which is used for submitting
+ * transfers to the USB HCD.
+ **/
+phys_addr_t usb_hcd_get_transfer_resource(struct usb_device *udev,
+				struct usb_host_endpoint *ep, dma_addr_t *dma)
+{
+	struct usb_hcd *hcd;
+	phys_addr_t pa = 0;
+
+	hcd = bus_to_hcd(udev->bus);
+	if (hcd->driver->get_transfer_resource)
+		pa = hcd->driver->get_transfer_resource(udev, ep, dma);
+
+	return pa;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_get_transfer_resource);
+
+/**
+ * usb_hcd_stop_endpoint - Halt USB EP transfers
+ * @udev: usb device
+ * @ep: usb ep to stop
+ *
+ * Stop pending transfers on a specific USB endpoint.
+ **/
+int usb_hcd_stop_endpoint(struct usb_device *udev,
+					struct usb_host_endpoint *ep)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+	int ret = 0;
+
+	if (hcd->driver->stop_endpoint)
+		ret = hcd->driver->stop_endpoint(hcd, udev, ep);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(usb_hcd_stop_endpoint);
+
 /*-------------------------------------------------------------------------*/
 
 /* this makes the hcd giveback() the urb more quickly, by kicking it
diff --git a/include/linux/usb.h b/include/linux/usb.h
index d4afeeec1e1a..2f71cd4fb6e0 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1724,6 +1724,13 @@  static inline void usb_fill_int_urb(struct urb *urb,
 	urb->start_frame = -1;
 }
 
+extern int usb_free_interrupter(struct usb_device *udev, int intr_num);
+extern phys_addr_t usb_set_interruper(struct usb_device *udev,
+						int intr_num, dma_addr_t *dma);
+extern int usb_hcd_stop_endpoint(struct usb_device *udev,
+						struct usb_host_endpoint *ep);
+extern phys_addr_t usb_hcd_get_transfer_resource(struct usb_device *udev,
+						struct usb_host_endpoint *ep, dma_addr_t *dma);
 extern void usb_init_urb(struct urb *urb);
 extern struct urb *usb_alloc_urb(int iso_packets, gfp_t mem_flags);
 extern void usb_free_urb(struct urb *urb);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index b51c07111729..f5bce80b2e40 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -343,7 +343,21 @@  struct hc_driver {
 	int	(*free_streams)(struct usb_hcd *hcd, struct usb_device *udev,
 		struct usb_host_endpoint **eps, unsigned int num_eps,
 		gfp_t mem_flags);
-
+	/* Frees an interrupter from the current client, and makes it available
+	 * for use.
+	 */
+	int (*free_interrupter)(struct usb_hcd *hcd, int intr_num);
+	/* Request an interrupter from the current allocated pool.  Will provide
+	 * the address to the allocated ring.
+	 */
+	phys_addr_t (*update_interrupter)(struct usb_hcd *hcd, int intr_num,
+		dma_addr_t *dma);
+	/* Fetch transfer/ep ring address */
+	phys_addr_t (*get_transfer_resource)(struct usb_device *udev,
+		struct usb_host_endpoint *ep, dma_addr_t *dma);
+	/* Stop transfers on a particular endpoint */
+	int	(*stop_endpoint)(struct usb_hcd *hcd, struct usb_device *udev,
+		struct usb_host_endpoint *ep);
 	/* Bandwidth computation functions */
 	/* Note that add_endpoint() can only be called once per endpoint before
 	 * check_bandwidth() or reset_bandwidth() must be called.