mbox series

[v7,00/33] Introduce QC USB SND audio offloading support

Message ID 20230921214843.18450-1-quic_wcheng@quicinc.com
Headers show
Series Introduce QC USB SND audio offloading support | expand

Message

Wesley Cheng Sept. 21, 2023, 9:48 p.m. UTC
Several Qualcomm based chipsets can support USB audio offloading to a
dedicated audio DSP, which can take over issuing transfers to the USB
host controller.  The intention is to reduce the load on the main
processors in the SoC, and allow them to be placed into lower power modes.
There are several parts to this design:
  1. Adding ASoC binding layer
  2. Create a USB backend for Q6DSP
  3. Introduce XHCI interrupter support
  4. Create vendor ops for the USB SND driver

      USB                          |            ASoC
--------------------------------------------------------------------
                                   |  _________________________
                                   | |sm8250 platform card     |
                                   | |_________________________|
                                   |         |           |
                                   |      ___V____   ____V____
                                   |     |Q6USB   | |Q6AFE    |  
                                   |     |"codec" | |"cpu"    |
                                   |     |________| |_________|
                                   |         ^  ^        ^
                                   |         |  |________|
                                   |      ___V____    |
                                   |     |SOC-USB |   |
   ________       ________               |        |   |
  |USB SND |<--->|QC offld|<------------>|________|   |
  |(card.c)|     |        |<----------                |
  |________|     |________|___     | |                |
      ^               ^       |    | |    ____________V_________
      |               |       |    | |   |APR/GLINK             |
   __ V_______________V_____  |    | |   |______________________|
  |USB SND (endpoint.c)     | |    | |              ^
  |_________________________| |    | |              |
              ^               |    | |   ___________V___________
              |               |    | |->|audio DSP              |
   ___________V_____________  |    |    |_______________________|
  |XHCI HCD                 |<-    |
  |_________________________|      |


Adding ASoC binding layer:
soc-usb: Intention is to treat a USB port similar to a headphone jack.
The port is always present on the device, but cable/pin status can be
enabled/disabled.  Expose mechanisms for USB backend ASoC drivers to
communicate with USB SND.

Create a USB backend for Q6DSP:
q6usb: Basic backend driver that will be responsible for maintaining the
resources needed to initiate a playback stream using the Q6DSP.  Will
be the entity that checks to make sure the connected USB audio device
supports the requested PCM format.  If it does not, the PCM open call will
fail, and userpsace ALSA can take action accordingly.

Introduce XHCI interrupter support:
XHCI HCD supports multiple interrupters, which allows for events to be routed
to different event rings.  This is determined by "Interrupter Target" field
specified in Section "6.4.1.1 Normal TRB" of the XHCI specification.

Events in the offloading case will be routed to an event ring that is assigned
to the audio DSP.

Create vendor ops for the USB SND driver:
qc_audio_offload: This particular driver has several components associated
with it:
- QMI stream request handler
- XHCI interrupter and resource management
- audio DSP memory management

When the audio DSP wants to enable a playback stream, the request is first
received by the ASoC platform sound card.  Depending on the selected route,
ASoC will bring up the individual DAIs in the path.  The Q6USB backend DAI
will send an AFE port start command (with enabling the USB playback path), and
the audio DSP will handle the request accordingly.

Part of the AFE USB port start handling will have an exchange of control
messages using the QMI protocol.  The qc_audio_offload driver will populate the
buffer information:
- Event ring base address
- EP transfer ring base address

and pass it along to the audio DSP.  All endpoint management will now be handed
over to the DSP, and the main processor is not involved in transfers.

Overall, implementing this feature will still expose separate sound card and PCM
devices for both the platorm card and USB audio device:
 0 [SM8250MTPWCD938]: sm8250 - SM8250-MTP-WCD9380-WSA8810-VA-D
                      SM8250-MTP-WCD9380-WSA8810-VA-DMIC
 1 [Audio          ]: USB-Audio - USB Audio
                      Generic USB Audio at usb-xhci-hcd.1.auto-1.4, high speed

This is to ensure that userspace ALSA entities can decide which route to take
when executing the audio playback.  In the above, if card#1 is selected, then
USB audio data will take the legacy path over the USB PCM drivers, etc...

This feature was validated using:
- tinymix: set/enable the multimedia path to route to USB backend
- tinyplay: issue playback on platform card

Changelog
--------------------------------------------
Changes in v7:
- Fixed dt check error for q6usb bindings
- Updated q6usb property from qcom,usb-audio-intr-num --> qcom,usb-audio-intr-idx
- Removed separate DWC3 HC interrupters num property, and place limits to XHCI one.
- Modified xhci_ring_to_sgtable() to use assigned IOVA/DMA address to fetch pages, as
it is not ensured event ring allocated is always done in the vmalloc range.

Changes in v6:
- Fixed limits and description on several DT bindings (XHCI and Q6USB)
- Fixed patch subjects to follow other ALSA/ASoC notations.

USB SND
- Addressed devices which expose multiple audio (UAC) interfaces.  These devices will
create a single USB sound card with multiple audio streams, and receive multiple
interface probe routines.  QC offload was not properly considering cases with multiple
probe calls.
- Renamed offload module name and kconfig to fit within the SND domain.
- Renamed attach/detach endpoint API to keep the hw_params notation.

Changes in v5:
- Removed some unnescessary files that were included
- Fixed some typos mentioned
- Addressed dt-binding issues and added hc-interrupters definition to usb-xhci.yaml

XHCI:
- Moved secondary skip events API to xhci-ring and updated implementation
   - Utilized existing XHCI APIs, such as inc_deq and xhci_update_erst_dequeue()

USB SND
- Renamed and reworked the APIs in "sound: usb: Export USB SND APIs for modules" patch to
include suggestions to utilize snd_usb_hw_params/free and to avoid generic naming.
- Added a resume_cb() op for completion sake.
- Addressed some locking concerns with regards to when registering for platform hooks.
- Added routine to disconnect all offloaded devices during module unbind.

ASoC
- Replaced individual PCM parameter arguments in snd_soc_usb_connect() with new
snd_soc_usb_device structure to pass along PCM info.
- Modified snd_jack set report to notify HEADPHONE event, as we do not support record path.

Changes in v4:
- Rebased to xhci/for-usb-next
- Addressed some dt-bindings comments

XHCI:
- Pulled in latest changes from Mathias' feature_interrupters branch:
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters

- Fixed commit text and signage for the XHCI sideband/interrupter related changes
- Added some logic to address the FIXME tags mentioned throughout the commits, such
as handling multi segment rings and building the SGT, locking concerns, and ep
cleanup operations.
- Removed some fixme tags for conditions that may not be needed/addressed.
- Repurposed the new endpoint stop sync API to be utilized in other places.
- Fixed potential compile issue if XHCI sideband config is not defined.

ASoC:
- Added sound jack control into the Q6USB driver.  Allows for userpsace to know when
an offload capable device is connected.

USB SND:
- Avoided exporting _snd_pcm_hw_param_set based on Takashi's recommendation.
- Split USB QMI packet header definitions into a separate commit.  This is used to
properly allow the QMI interface driver to parse and route QMI packets accordingly
- Added a "depends on" entry when enabling QC audio offload to avoid compile time
issues.

Changes in v3:
- Changed prefix from RFC to PATCH
- Rebased entire series to usb-next
- Updated copyright years

XHCI:
- Rebased changes on top of XHCI changes merged into usb-next, and only added
changes that were still under discussion.
- Added change to read in the "num-hc-interrupters" device property.

ASoC:
- qusb6 USB backend
  - Incorporated suggestions to fetch iommu information with existing APIs
  - Added two new sound kcontrols to fetch offload status and offload device
    selection.
    - offload status - will return the card and pcm device in use
        tinymix -D 0 get 1 --> 1, 0 (offload in progress on card#1 pcm#0)

    - device selection - set the card and pcm device to enable offload on. Ex.:
        tinymix -D 0 set 1 2 0  --> sets offload on card#2 pcm#0
                                    (this should be the USB card)

USB SND:
- Fixed up some locking related concerns for registering platform ops.
   - Moved callbacks under the register_mutex, so that 
- Modified APIs to properly pass more information about the USB SND device, so
that the Q6USB backend can build a device list/map, in order to monitor offload
status and device selection.

Changes in v2:

XHCI:
- Replaced XHCI and HCD changes with Mathias' XHCI interrupter changes
in his tree:
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters

Adjustments made to Mathias' changes:
  - Created xhci-intr.h to export/expose interrupter APIs versus exposing xhci.h.
    Moved dependent structures to this file as well. (so clients can parse out
    information from "struct xhci_interrupter")
  - Added some basic locking when requesting interrupters.
  - Fixed up some sanity checks.
  - Removed clearing of the ERSTBA during freeing of the interrupter. (pending
    issue where SMMU fault occurs if DMA addr returned is 64b - TODO)

- Clean up pending events in the XHCI secondary interrupter.  While testing USB
bus suspend, it was seen that on bus resume, the xHCI HC would run into a command
timeout.
- Added offloading APIs to xHCI to fetch transfer and event ring information.

ASoC:
- Modified soc-usb to allow for multiple USB port additions.  For this to work,
the USB offload driver has to have a reference to the USB backend by adding
a "usb-soc-be" DT entry to the device saved into XHCI sysdev.
- Created separate dt-bindings for defining USB_RX port.
- Increased APR timeout to accommodate the situation where the AFE port start
command could be delayed due to having to issue a USB bus resume while
handling the QMI stream start command.

USB SND:
- Added a platform ops during usb_audio_suspend().  This allows for the USB
offload driver to halt the audio stream when system enters PM suspend.  This
ensures the audio DSP is not issuing transfers on the USB bus.
- Do not override platform ops if they are already populated.
- Introduce a shared status variable between the USB offload and USB SND layers,
to ensure that only one path is active at a time.  If the USB bus is occupied,
then userspace is notified that the path is busy.

Mathias Nyman (3):
  xhci: add support to allocate several interrupters
  xhci: add helper to stop endpoint and wait for completion
  xhci: sideband: add initial api to register a sideband entity

Wesley Cheng (30):
  usb: host: xhci-mem: Cleanup pending secondary event ring events
  usb: host: xhci-mem: Allow for interrupter clients to choose specific
    index
  ASoC: Add SOC USB APIs for adding an USB backend
  ASoC: dt-bindings: qcom,q6dsp-lpass-ports: Add USB_RX port
  ASoC: qcom: qdsp6: Introduce USB AFE port to q6dsp
  ASoC: qdsp6: q6afe: Increase APR timeout
  ASoC: qcom: qdsp6: Add USB backend ASoC driver for Q6
  ALSA: usb-audio: Introduce USB SND platform op callbacks
  ALSA: usb-audio: Export USB SND APIs for modules
  dt-bindings: usb: dwc3: Limit num-hc-interrupters definition
  dt-bindings: usb: xhci: Add num-hc-interrupters definition
  usb: dwc3: Specify maximum number of XHCI interrupters
  usb: host: xhci-plat: Set XHCI max interrupters if property is present
  ALSA: usb-audio: qcom: Add USB QMI definitions
  ALSA: usb-audio: qcom: Introduce QC USB SND offloading support
  ALSA: usb-audio: Check for support for requested audio format
  ASoC: usb: Add PCM format check API for USB backend
  ASoC: qcom: qdsp6: Ensure PCM format is supported by USB audio device
  ALSA: usb-audio: Prevent starting of audio stream if in use
  ASoC: dt-bindings: Add Q6USB backend
  ASoC: dt-bindings: Update example for enabling USB offload on SM8250
  ASoC: qcom: qdsp6: q6afe: Split USB AFE dev_token param into separate
    API
  ALSA: usb-audio: qcom: Populate PCM and USB chip information
  ASoC: qcom: qdsp6: Add support to track available USB PCM devices
  ASoC: qcom: qdsp6: Add SND kcontrol to select offload device
  ASoC: qcom: qdsp6: Add SND kcontrol for fetching offload status
  ASoC: qcom: qdsp6: Add headphone jack for offload connection status
  ALSA: usb-audio: qcom: Use card and PCM index from QMI request
  ALSA: usb-audio: Allow for rediscovery of connected USB SND devices
  ASoC: usb: Rediscover USB SND devices on USB port add

 .../devicetree/bindings/sound/qcom,q6usb.yaml |   55 +
 .../bindings/sound/qcom,sm8250.yaml           |   15 +
 .../devicetree/bindings/usb/snps,dwc3.yaml    |    4 +
 .../devicetree/bindings/usb/usb-xhci.yaml     |    6 +
 drivers/usb/dwc3/core.c                       |   12 +
 drivers/usb/dwc3/core.h                       |    2 +
 drivers/usb/dwc3/host.c                       |    5 +-
 drivers/usb/host/Kconfig                      |    9 +
 drivers/usb/host/Makefile                     |    4 +
 drivers/usb/host/xhci-debugfs.c               |    2 +-
 drivers/usb/host/xhci-hub.c                   |   29 +-
 drivers/usb/host/xhci-mem.c                   |  102 +-
 drivers/usb/host/xhci-plat.c                  |    2 +
 drivers/usb/host/xhci-ring.c                  |   48 +-
 drivers/usb/host/xhci-sideband.c              |  198 ++
 drivers/usb/host/xhci.c                       |  111 +-
 drivers/usb/host/xhci.h                       |  105 +-
 .../sound/qcom,q6dsp-lpass-ports.h            |    1 +
 include/linux/usb/xhci-intr.h                 |   86 +
 include/linux/usb/xhci-sideband.h             |   57 +
 include/sound/q6usboffload.h                  |   20 +
 include/sound/soc-usb.h                       |   51 +
 sound/soc/Makefile                            |    2 +-
 sound/soc/qcom/Kconfig                        |    4 +
 sound/soc/qcom/qdsp6/Makefile                 |    1 +
 sound/soc/qcom/qdsp6/q6afe-dai.c              |   56 +
 sound/soc/qcom/qdsp6/q6afe.c                  |  206 +-
 sound/soc/qcom/qdsp6/q6afe.h                  |   36 +-
 sound/soc/qcom/qdsp6/q6dsp-lpass-ports.c      |   23 +
 sound/soc/qcom/qdsp6/q6dsp-lpass-ports.h      |    1 +
 sound/soc/qcom/qdsp6/q6routing.c              |    9 +
 sound/soc/qcom/qdsp6/q6usb.c                  |  450 ++++
 sound/soc/soc-usb.c                           |  200 ++
 sound/usb/Kconfig                             |   15 +
 sound/usb/Makefile                            |    2 +-
 sound/usb/card.c                              |  123 ++
 sound/usb/card.h                              |   23 +
 sound/usb/endpoint.c                          |    1 +
 sound/usb/helper.c                            |    1 +
 sound/usb/pcm.c                               |   94 +-
 sound/usb/pcm.h                               |   11 +
 sound/usb/qcom/Makefile                       |    2 +
 sound/usb/qcom/qc_audio_offload.c             | 1854 +++++++++++++++++
 sound/usb/qcom/usb_audio_qmi_v01.c            |  892 ++++++++
 sound/usb/qcom/usb_audio_qmi_v01.h            |  162 ++
 45 files changed, 4896 insertions(+), 196 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/sound/qcom,q6usb.yaml
 create mode 100644 drivers/usb/host/xhci-sideband.c
 create mode 100644 include/linux/usb/xhci-intr.h
 create mode 100644 include/linux/usb/xhci-sideband.h
 create mode 100644 include/sound/q6usboffload.h
 create mode 100644 include/sound/soc-usb.h
 create mode 100644 sound/soc/qcom/qdsp6/q6usb.c
 create mode 100644 sound/soc/soc-usb.c
 create mode 100644 sound/usb/qcom/Makefile
 create mode 100644 sound/usb/qcom/qc_audio_offload.c
 create mode 100644 sound/usb/qcom/usb_audio_qmi_v01.c
 create mode 100644 sound/usb/qcom/usb_audio_qmi_v01.h

Comments

Mark Brown Sept. 27, 2023, 2:48 p.m. UTC | #1
On Thu, Sep 21, 2023 at 02:48:16PM -0700, Wesley Cheng wrote:

> +static struct device_node *snd_soc_find_phandle(struct device *dev)
> +{
> +	struct device_node *node;
> +
> +	node = of_parse_phandle(dev->of_node, "usb-soc-be", 0);

Very nitpicky but this function possibly wants a _usb_ in the name, not
that it *super* matters with it being static.  Or it could just be
inlined into the only user and not worry about the naming at all.

> +/**
> + * snd_soc_usb_get_priv_data() - Retrieve private data stored
> + * @dev: device reference
> + *
> + * Fetch the private data stored in the USB SND SOC structure.
> + *
> + */
> +void *snd_soc_usb_get_priv_data(struct device *dev)
> +{
> +	struct snd_soc_usb *ctx;
> +
> +	ctx = snd_soc_find_usb_ctx(dev);
> +	if (!ctx) {
> +		/* Check if backend device */
> +		mutex_lock(&ctx_mutex);
> +		list_for_each_entry(ctx, &usb_ctx_list, list) {
> +			if (dev->of_node == ctx->dev->of_node) {
> +				mutex_unlock(&ctx_mutex);
> +				goto out;
> +			}
> +		}
> +		mutex_unlock(&ctx_mutex);
> +		ctx = NULL;
> +	}

This seems a lot more expensive than I'd expect for a get_priv_data
operation, usually it's just a container_of() or other constant time
pulling out of a pointer rather than a linked list walk - the sort of
thing that people put at the start of functions and do all the time.
If we need this I think it needs a name that's more clearly tied to the
use case.

I didn't actually find the user of this though?
Mark Brown Sept. 27, 2023, 2:50 p.m. UTC | #2
On Thu, Sep 21, 2023 at 02:48:19PM -0700, Wesley Cheng wrote:
> For USB offloading situations, the AFE port start command will result in a
> QMI handshake between the Q6DSP and the main processor.  Depending on if
> the USB bus is suspended, this routine would require more time to complete,
> as resuming the USB bus has some overhead associated with it.  Increase the
> timeout to 3s to allow for sufficient time for the USB QMI stream enable
> handshake to complete.

...

> -#define TIMEOUT_MS 1000
> +#define TIMEOUT_MS 3000

That seems worryingly large but if it's what the hardware/firmware needs
I guess there's nothing doing - even the 1s that's being replaced would
be nasty if we ever actually hit it.
Mark Brown Sept. 27, 2023, 3:02 p.m. UTC | #3
On Thu, Sep 21, 2023 at 02:48:39PM -0700, Wesley Cheng wrote:

> Add a kcontrol to the platform sound card to fetch the current offload
> status.  This can allow for userspace to ensure/check which USB SND
> resources are actually busy versus having to attempt opening the USB SND
> devices, which will result in an error if offloading is active.

> +static int q6usb_prepare(struct snd_pcm_substream *substream,
> +               struct snd_soc_dai *dai)
> +{
> +       struct q6usb_port_data *data = dev_get_drvdata(dai->dev);
> + 
> +       mutex_lock(&data->mutex);
> +       data->status[data->sel_card_idx].running = true;
> +       mutex_unlock(&data->mutex);

These updates of running should really have a snd_ctl_notify() so that
UIs can know to update when the value changes while they're open.

> +static int q6usb_mixer_get_offload_status(struct snd_kcontrol *kcontrol,
> +				   struct snd_ctl_elem_value *ucontrol)
> +{

> +	running = q6usb_find_running(data);
> +	if (running < 0) {
> +		card_idx = -1;
> +		pcm_idx = -1;
> +	} else {
> +		card_idx = running;
> +		pcm_idx = data->status[running].pcm_index;
> +	}
> +
> +	ucontrol->value.integer.value[0] = card_idx;
> +	ucontrol->value.integer.value[1] = pcm_idx;

This feels a bit messy but I'm not sure what we'd do that's better so
unless someone else has better ideas let's go with this.  Possibly we
should standardise this as a new control type for joining cards up so at
least if there's further needs for this we can use the same solution?
Mark Brown Sept. 27, 2023, 3:04 p.m. UTC | #4
On Thu, Sep 21, 2023 at 02:48:10PM -0700, Wesley Cheng wrote:
> Several Qualcomm based chipsets can support USB audio offloading to a
> dedicated audio DSP, which can take over issuing transfers to the USB
> host controller.  The intention is to reduce the load on the main
> processors in the SoC, and allow them to be placed into lower power modes.

I had a few small comments in reply to some of the patches but overall
the ASoC sides of this look fine to me.  I didn't really look at the USB
side at all, I'm not sure I understand it enough to have any useful
thoughts anyway.

Thanks for taking on this work and pushing it forwards!
Greg KH Sept. 27, 2023, 3:46 p.m. UTC | #5
On Wed, Sep 27, 2023 at 05:04:06PM +0200, Mark Brown wrote:
> On Thu, Sep 21, 2023 at 02:48:10PM -0700, Wesley Cheng wrote:
> > Several Qualcomm based chipsets can support USB audio offloading to a
> > dedicated audio DSP, which can take over issuing transfers to the USB
> > host controller.  The intention is to reduce the load on the main
> > processors in the SoC, and allow them to be placed into lower power modes.
> 
> I had a few small comments in reply to some of the patches but overall
> the ASoC sides of this look fine to me.  I didn't really look at the USB
> side at all, I'm not sure I understand it enough to have any useful
> thoughts anyway.

Thanks for reviewing those portions, I'll look at the USB bits next week
when I get back from traveling.

greg k-h
Wesley Cheng Sept. 27, 2023, 7:57 p.m. UTC | #6
Hi Mark,

On 9/27/2023 7:48 AM, Mark Brown wrote:
> On Thu, Sep 21, 2023 at 02:48:16PM -0700, Wesley Cheng wrote:
> 
>> +static struct device_node *snd_soc_find_phandle(struct device *dev)
>> +{
>> +	struct device_node *node;
>> +
>> +	node = of_parse_phandle(dev->of_node, "usb-soc-be", 0);
> 
> Very nitpicky but this function possibly wants a _usb_ in the name, not
> that it *super* matters with it being static.  Or it could just be
> inlined into the only user and not worry about the naming at all.
> 

Thanks for the review!  Sure, let me reshuffle things around a bit and 
just get rid of this function entirely and inline it to the API below.

>> +/**
>> + * snd_soc_usb_get_priv_data() - Retrieve private data stored
>> + * @dev: device reference
>> + *
>> + * Fetch the private data stored in the USB SND SOC structure.
>> + *
>> + */
>> +void *snd_soc_usb_get_priv_data(struct device *dev)
>> +{
>> +	struct snd_soc_usb *ctx;
>> +
>> +	ctx = snd_soc_find_usb_ctx(dev);
>> +	if (!ctx) {
>> +		/* Check if backend device */
>> +		mutex_lock(&ctx_mutex);
>> +		list_for_each_entry(ctx, &usb_ctx_list, list) {
>> +			if (dev->of_node == ctx->dev->of_node) {
>> +				mutex_unlock(&ctx_mutex);
>> +				goto out;
>> +			}
>> +		}
>> +		mutex_unlock(&ctx_mutex);
>> +		ctx = NULL;
>> +	}
> 
> This seems a lot more expensive than I'd expect for a get_priv_data
> operation, usually it's just a container_of() or other constant time
> pulling out of a pointer rather than a linked list walk - the sort of
> thing that people put at the start of functions and do all the time.
> If we need this I think it needs a name that's more clearly tied to the
> use case.
> 
> I didn't actually find the user of this though?

So the end user of this would be the qc_audio_offload driver, within 
prepare_qmi_response().  This is to fetch some information about the 
DPCM backend during the stream enable request.

Previously, I limited the # of snd_soc_usb ports to be registered to 
one, but that would affect the scalability of this layer.  Hence, adding 
a list instead increased the complexity.  Will rename this accordingly.

Thanks
Wesley Cheng
Wesley Cheng Sept. 27, 2023, 8:01 p.m. UTC | #7
Hi Mark,

On 9/27/2023 7:50 AM, Mark Brown wrote:
> On Thu, Sep 21, 2023 at 02:48:19PM -0700, Wesley Cheng wrote:
>> For USB offloading situations, the AFE port start command will result in a
>> QMI handshake between the Q6DSP and the main processor.  Depending on if
>> the USB bus is suspended, this routine would require more time to complete,
>> as resuming the USB bus has some overhead associated with it.  Increase the
>> timeout to 3s to allow for sufficient time for the USB QMI stream enable
>> handshake to complete.
> 
> ...
> 
>> -#define TIMEOUT_MS 1000
>> +#define TIMEOUT_MS 3000
> 
> That seems worryingly large but if it's what the hardware/firmware needs
> I guess there's nothing doing - even the 1s that's being replaced would
> be nasty if we ever actually hit it.

I may have gone overkill with the delay, but when I measured the 
duration of the AFE start command it took ~1.5-2s.  It has to also 
account for the overhead within handling the USB QMI request in the 
qc_audio_offload driver.

Thanks
Wesley Cheng
Wesley Cheng Sept. 27, 2023, 8:10 p.m. UTC | #8
Hi Mark,

On 9/27/2023 8:02 AM, Mark Brown wrote:
> On Thu, Sep 21, 2023 at 02:48:39PM -0700, Wesley Cheng wrote:
> 
>> Add a kcontrol to the platform sound card to fetch the current offload
>> status.  This can allow for userspace to ensure/check which USB SND
>> resources are actually busy versus having to attempt opening the USB SND
>> devices, which will result in an error if offloading is active.
> 
>> +static int q6usb_prepare(struct snd_pcm_substream *substream,
>> +               struct snd_soc_dai *dai)
>> +{
>> +       struct q6usb_port_data *data = dev_get_drvdata(dai->dev);
>> +
>> +       mutex_lock(&data->mutex);
>> +       data->status[data->sel_card_idx].running = true;
>> +       mutex_unlock(&data->mutex);
> 
> These updates of running should really have a snd_ctl_notify() so that
> UIs can know to update when the value changes while they're open.
> 

Sure, me review some of the APIs again and add the notify call where 
necessary.

>> +static int q6usb_mixer_get_offload_status(struct snd_kcontrol *kcontrol,
>> +				   struct snd_ctl_elem_value *ucontrol)
>> +{
> 
>> +	running = q6usb_find_running(data);
>> +	if (running < 0) {
>> +		card_idx = -1;
>> +		pcm_idx = -1;
>> +	} else {
>> +		card_idx = running;
>> +		pcm_idx = data->status[running].pcm_index;
>> +	}
>> +
>> +	ucontrol->value.integer.value[0] = card_idx;
>> +	ucontrol->value.integer.value[1] = pcm_idx;
> 
> This feels a bit messy but I'm not sure what we'd do that's better so
> unless someone else has better ideas let's go with this.  Possibly we
> should standardise this as a new control type for joining cards up so at
> least if there's further needs for this we can use the same solution?

I'm all ears for any suggestions from other users :).  I think its a bit 
difficult to tell since this is the first iteration of adding this 
feature.  Pierre gave me some great feedback from the 
userspace/application level, and tried my best to accommodate for those 
requirements since it would be the main entity interacting with these 
controls.

Thanks
Wesley Cheng
Mathias Nyman Sept. 28, 2023, 10:31 a.m. UTC | #9
On 22.9.2023 0.48, Wesley Cheng wrote:
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> Modify the XHCI drivers to accommodate for handling multiple event rings in
> case there are multiple interrupters.  Add the required APIs so clients are
> able to allocate/request for an interrupter ring, and pass this information
> back to the client driver.  This allows for users to handle the resource
> accordingly, such as passing the event ring base address to an audio DSP.
> There is no actual support for multiple MSI/MSI-X vectors.
> 
> Factoring out XHCI interrupter APIs and structures done by Wesley Cheng, in
> order to allow for USB class drivers to utilze them.
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   drivers/usb/host/xhci-debugfs.c |  2 +-
>   drivers/usb/host/xhci-mem.c     | 93 ++++++++++++++++++++++++++++++---
>   drivers/usb/host/xhci-ring.c    |  2 +-
>   drivers/usb/host/xhci.c         | 49 ++++++++++-------
>   drivers/usb/host/xhci.h         | 77 +--------------------------
>   include/linux/usb/xhci-intr.h   | 86 ++++++++++++++++++++++++++++++
>   6 files changed, 207 insertions(+), 102 deletions(-)
>   create mode 100644 include/linux/usb/xhci-intr.h
> 
> diff --git a/drivers/usb/host/xhci-debugfs.c b/drivers/usb/host/xhci-debugfs.c
> index 99baa60ef50f..15a8402ee8a1 100644
> --- a/drivers/usb/host/xhci-debugfs.c
> +++ b/drivers/usb/host/xhci-debugfs.c
> @@ -693,7 +693,7 @@ void xhci_debugfs_init(struct xhci_hcd *xhci)
>   				     "command-ring",
>   				     xhci->debugfs_root);
>   
> -	xhci_debugfs_create_ring_dir(xhci, &xhci->interrupter->event_ring,
> +	xhci_debugfs_create_ring_dir(xhci, &xhci->interrupters[0]->event_ring,
>   				     "event-ring",
>   				     xhci->debugfs_root);
>   
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 8714ab5bf04d..2f9228d7d22d 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -1837,6 +1837,26 @@ xhci_free_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir)
>   	kfree(ir);
>   }
>   
> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrupter *ir)
> +{
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	unsigned int intr_num;
> +
> +	/* interrupter 0 is primary interrupter, don't touch it */
> +	if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters) {
> +		xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
> +		return;
> +	}
> +
> +	/* fixme, should we check xhci->interrupter[intr_num] == ir */
> +	spin_lock(&xhci->lock);

Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is used in interrupt handler.


> +	intr_num = ir->intr_num;
> +	xhci_free_interrupter(xhci, ir);
> +	xhci->interrupters[intr_num] = NULL;
> +	spin_unlock(&xhci->lock);

likewise

> +}
> +EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
> +
>   void xhci_mem_cleanup(struct xhci_hcd *xhci)
>   {
>   	struct device	*dev = xhci_to_hcd(xhci)->self.sysdev;
> @@ -1844,9 +1864,13 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>   
>   	cancel_delayed_work_sync(&xhci->cmd_timer);
>   
> -	xhci_free_interrupter(xhci, xhci->interrupter);
> -	xhci->interrupter = NULL;
> -	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed primary event ring");
> +	for (i = 0; i < xhci->max_interrupters; i++) {
> +		if (xhci->interrupters[i]) {
> +			xhci_free_interrupter(xhci, xhci->interrupters[i]);
> +			xhci->interrupters[i] = NULL;
> +		}
> +	}
> +	xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed interrupters");
>   
>   	if (xhci->cmd_ring)
>   		xhci_ring_free(xhci, xhci->cmd_ring);
> @@ -1916,6 +1940,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>   	for (i = 0; i < xhci->num_port_caps; i++)
>   		kfree(xhci->port_caps[i].psi);
>   	kfree(xhci->port_caps);
> +	kfree(xhci->interrupters);
>   	xhci->num_port_caps = 0;
>   
>   	xhci->usb2_rhub.ports = NULL;
> @@ -1924,6 +1949,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>   	xhci->rh_bw = NULL;
>   	xhci->ext_caps = NULL;
>   	xhci->port_caps = NULL;
> +	xhci->interrupters = NULL;
>   
>   	xhci->page_size = 0;
>   	xhci->page_shift = 0;
> @@ -2276,6 +2302,13 @@ xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
>   		return -EINVAL;
>   	}
>   
> +	if (xhci->interrupters[intr_num]) {
> +		xhci_warn(xhci, "Interrupter %d\n already set up", intr_num);
> +		return -EINVAL;
> +	}
> +
> +	xhci->interrupters[intr_num] = ir;
> +	ir->intr_num = intr_num;
>   	ir->ir_set = &xhci->run_regs->ir_set[intr_num];
>   
>   	/* set ERST count with the number of entries in the segment table */
> @@ -2295,10 +2328,53 @@ xhci_add_interrupter(struct xhci_hcd *xhci, struct xhci_interrupter *ir,
>   	return 0;
>   }
>   
> +struct xhci_interrupter *
> +xhci_create_secondary_interrupter(struct usb_hcd *hcd)
> +{
> +	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +	struct xhci_interrupter *ir;
> +	unsigned int i;
> +	int err = -ENOSPC;
> +
> +	if (!xhci->interrupters)
> +		return NULL;
> +
> +	ir = xhci_alloc_interrupter(xhci, GFP_KERNEL);
> +	if (!ir)
> +		return NULL;
> +
> +	spin_lock_irq(&xhci->lock);
> +
> +	/* Find available secondary interrupter, interrupter 0 is reserverd for primary */

reserved

> +	for (i = 1; i < xhci->max_interrupters; i++) {
> +		if (xhci->interrupters[i] == NULL) {
> +			err = xhci_add_interrupter(xhci, ir, i);
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_irq(&xhci->lock);
> +	if (err) {
> +		xhci_warn(xhci, "Failed to add secondary interrupter, max interrupters %d\n",
> +			xhci->max_interrupters);
> +		xhci_free_interrupter(xhci, ir);
> +		ir = NULL;
> +		goto out;
> +	}
> +
> +	xhci_dbg(xhci, "Add secondary interrupter %d, max interrupters %d\n",
> +		 i, xhci->max_interrupters);
> +
> +out:
> +	return ir;
> +}
> +EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter);
> +
>   int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>   {
> -	dma_addr_t	dma;
> +	struct xhci_interrupter *ir;
>   	struct device	*dev = xhci_to_hcd(xhci)->self.sysdev;
> +	dma_addr_t	dma;
>   	unsigned int	val, val2;
>   	u64		val_64;
>   	u32		page_size, temp;
> @@ -2422,11 +2498,14 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>   	/* Allocate and set up primary interrupter 0 with an event ring. */
>   	xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>   		       "Allocating primary event ring");
> -	xhci->interrupter = xhci_alloc_interrupter(xhci, flags);
> -	if (!xhci->interrupter)
> +	xhci->interrupters = kcalloc_node(xhci->max_interrupters, sizeof(*xhci->interrupters),
> +					  flags, dev_to_node(dev));
> +
> +	ir = xhci_alloc_interrupter(xhci, flags);
> +	if (!ir)
>   		goto fail;
>   
> -	if (xhci_add_interrupter(xhci, xhci->interrupter, 0))
> +	if (xhci_add_interrupter(xhci, ir, 0))
>   		goto fail;
>   
>   	xhci->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX;
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 1dde53f6eb31..93233cf5ff21 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3074,7 +3074,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
>   	writel(status, &xhci->op_regs->status);
>   
>   	/* This is the handler of the primary interrupter */
> -	ir = xhci->interrupter;
> +	ir = xhci->interrupters[0];
>   	if (!hcd->msi_enabled) {
>   		u32 irq_pending;
>   		irq_pending = readl(&ir->ir_set->irq_pending);
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e1b1b64a0723..3fd2b58ee1d3 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -456,7 +456,7 @@ static int xhci_init(struct usb_hcd *hcd)
>   
>   static int xhci_run_finished(struct xhci_hcd *xhci)
>   {
> -	struct xhci_interrupter *ir = xhci->interrupter;
> +	struct xhci_interrupter *ir = xhci->interrupters[0];
>   	unsigned long	flags;
>   	u32		temp;
>   
> @@ -508,7 +508,7 @@ int xhci_run(struct usb_hcd *hcd)
>   	u64 temp_64;
>   	int ret;
>   	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> -	struct xhci_interrupter *ir = xhci->interrupter;
> +	struct xhci_interrupter *ir = xhci->interrupters[0];
>   	/* Start the xHCI host controller running only after the USB 2.0 roothub
>   	 * is setup.
>   	 */
> @@ -572,7 +572,7 @@ void xhci_stop(struct usb_hcd *hcd)
>   {
>   	u32 temp;
>   	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> -	struct xhci_interrupter *ir = xhci->interrupter;
> +	struct xhci_interrupter *ir = xhci->interrupters[0];
>   
>   	mutex_lock(&xhci->mutex);
>   
> @@ -668,36 +668,49 @@ EXPORT_SYMBOL_GPL(xhci_shutdown);
>   #ifdef CONFIG_PM
>   static void xhci_save_registers(struct xhci_hcd *xhci)
>   {
> -	struct xhci_interrupter *ir = xhci->interrupter;
> +	struct xhci_interrupter *ir;
> +	unsigned int i;
>   
>   	xhci->s3.command = readl(&xhci->op_regs->command);
>   	xhci->s3.dev_nt = readl(&xhci->op_regs->dev_notification);
>   	xhci->s3.dcbaa_ptr = xhci_read_64(xhci, &xhci->op_regs->dcbaa_ptr);
>   	xhci->s3.config_reg = readl(&xhci->op_regs->config_reg);
>   
> -	if (!ir)
> -		return;
> +	/* save both primary and all secondary interrupters */
> +	for (i = 0; i < xhci->max_interrupters; i++) {
> +		ir = xhci->interrupters[i];
> +		if (!ir)
> +			continue;
>   
> -	ir->s3_erst_size = readl(&ir->ir_set->erst_size);
> -	ir->s3_erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
> -	ir->s3_erst_dequeue = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
> -	ir->s3_irq_pending = readl(&ir->ir_set->irq_pending);
> -	ir->s3_irq_control = readl(&ir->ir_set->irq_control);
> +		ir->s3_erst_size = readl(&ir->ir_set->erst_size);
> +		ir->s3_erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
> +		ir->s3_erst_dequeue = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
> +		ir->s3_irq_pending = readl(&ir->ir_set->irq_pending);
> +		ir->s3_irq_control = readl(&ir->ir_set->irq_control);
> +	}
>   }
>   
>   static void xhci_restore_registers(struct xhci_hcd *xhci)
>   {
> -	struct xhci_interrupter *ir = xhci->interrupter;
> +	struct xhci_interrupter *ir;
> +	unsigned int i;
>   
>   	writel(xhci->s3.command, &xhci->op_regs->command);
>   	writel(xhci->s3.dev_nt, &xhci->op_regs->dev_notification);
>   	xhci_write_64(xhci, xhci->s3.dcbaa_ptr, &xhci->op_regs->dcbaa_ptr);
>   	writel(xhci->s3.config_reg, &xhci->op_regs->config_reg);
> -	writel(ir->s3_erst_size, &ir->ir_set->erst_size);
> -	xhci_write_64(xhci, ir->s3_erst_base, &ir->ir_set->erst_base);
> -	xhci_write_64(xhci, ir->s3_erst_dequeue, &ir->ir_set->erst_dequeue);
> -	writel(ir->s3_irq_pending, &ir->ir_set->irq_pending);
> -	writel(ir->s3_irq_control, &ir->ir_set->irq_control);
> +
> +	for (i = 0; i < xhci->max_interrupters; i++) {
> +		ir = xhci->interrupters[i];
> +		if (!ir)
> +			continue;
> +
> +		writel(ir->s3_erst_size, &ir->ir_set->erst_size);
> +		xhci_write_64(xhci, ir->s3_erst_base, &ir->ir_set->erst_base);
> +		xhci_write_64(xhci, ir->s3_erst_dequeue, &ir->ir_set->erst_dequeue);
> +		writel(ir->s3_irq_pending, &ir->ir_set->irq_pending);
> +		writel(ir->s3_irq_control, &ir->ir_set->irq_control);
> +	}
>   }
>   
>   static void xhci_set_cmd_ring_deq(struct xhci_hcd *xhci)
> @@ -1059,7 +1072,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>   		xhci_dbg(xhci, "// Disabling event ring interrupts\n");
>   		temp = readl(&xhci->op_regs->status);
>   		writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
> -		xhci_disable_interrupter(xhci->interrupter);
> +		xhci_disable_interrupter(xhci->interrupters[0]);
>   
>   		xhci_dbg(xhci, "cleaning up memory\n");
>   		xhci_mem_cleanup(xhci);

All code above looks like it should be its own patch.

The header shuffling below part of somethine else.

> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 7e282b4522c0..d706a27ec0a3 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -17,6 +17,7 @@
>   #include <linux/kernel.h>
>   #include <linux/usb/hcd.h>
>   #include <linux/io-64-nonatomic-lo-hi.h>
> +#include <linux/usb/xhci-intr.h>
>   
>   /* Code sharing between pci-quirks and xhci hcd */
>   #include	"xhci-ext-caps.h"
> @@ -1541,18 +1542,6 @@ static inline const char *xhci_trb_type_string(u8 type)
>   #define AVOID_BEI_INTERVAL_MIN	8
>   #define AVOID_BEI_INTERVAL_MAX	32
>   
> -struct xhci_segment {
> -	union xhci_trb		*trbs;
> -	/* private to HCD */
> -	struct xhci_segment	*next;
> -	dma_addr_t		dma;
> -	/* Max packet sized bounce buffer for td-fragmant alignment */
> -	dma_addr_t		bounce_dma;
> -	void			*bounce_buf;
> -	unsigned int		bounce_offs;
> -	unsigned int		bounce_len;
> -};
> -
>   enum xhci_cancelled_td_status {
>   	TD_DIRTY = 0,
>   	TD_HALTED,
> @@ -1585,16 +1574,6 @@ struct xhci_cd {
>   	union xhci_trb		*cmd_trb;
>   };
>   
> -enum xhci_ring_type {
> -	TYPE_CTRL = 0,
> -	TYPE_ISOC,
> -	TYPE_BULK,
> -	TYPE_INTR,
> -	TYPE_STREAM,
> -	TYPE_COMMAND,
> -	TYPE_EVENT,
> -};
> -
>   static inline const char *xhci_ring_type_string(enum xhci_ring_type type)
>   {
>   	switch (type) {
> @@ -1615,46 +1594,6 @@ static inline const char *xhci_ring_type_string(enum xhci_ring_type type)
>   	}
>   
>   	return "UNKNOWN";
> -}
> -
> -struct xhci_ring {
> -	struct xhci_segment	*first_seg;
> -	struct xhci_segment	*last_seg;
> -	union  xhci_trb		*enqueue;
> -	struct xhci_segment	*enq_seg;
> -	union  xhci_trb		*dequeue;
> -	struct xhci_segment	*deq_seg;
> -	struct list_head	td_list;
> -	/*
> -	 * Write the cycle state into the TRB cycle field to give ownership of
> -	 * the TRB to the host controller (if we are the producer), or to check
> -	 * if we own the TRB (if we are the consumer).  See section 4.9.1.
> -	 */
> -	u32			cycle_state;
> -	unsigned int		stream_id;
> -	unsigned int		num_segs;
> -	unsigned int		num_trbs_free; /* used only by xhci DbC */
> -	unsigned int		bounce_buf_len;
> -	enum xhci_ring_type	type;
> -	bool			last_td_was_short;
> -	struct radix_tree_root	*trb_address_map;
> -};
> -
> -struct xhci_erst_entry {
> -	/* 64-bit event ring segment address */
> -	__le64	seg_addr;
> -	__le32	seg_size;
> -	/* Set to zero */
> -	__le32	rsvd;
> -};
> -
> -struct xhci_erst {
> -	struct xhci_erst_entry	*entries;
> -	unsigned int		num_entries;
> -	/* xhci->event_ring keeps track of segment dma addresses */
> -	dma_addr_t		erst_dma_addr;
> -	/* Num entries the ERST can contain */
> -	unsigned int		erst_size;
>   };
>   
>   struct xhci_scratchpad {
> @@ -1707,18 +1646,6 @@ struct xhci_bus_state {
>   	unsigned long		resuming_ports;
>   };
>   
> -struct xhci_interrupter {
> -	struct xhci_ring	*event_ring;
> -	struct xhci_erst	erst;
> -	struct xhci_intr_reg __iomem *ir_set;
> -	unsigned int		intr_num;
> -	/* For interrupter registers save and restore over suspend/resume */
> -	u32	s3_irq_pending;
> -	u32	s3_irq_control;
> -	u32	s3_erst_size;
> -	u64	s3_erst_base;
> -	u64	s3_erst_dequeue;
> -};
>   /*
>    * It can take up to 20 ms to transition from RExit to U0 on the
>    * Intel Lynx Point LP xHCI host.
> @@ -1799,7 +1726,7 @@ struct xhci_hcd {
>   	struct reset_control *reset;
>   	/* data structures */
>   	struct xhci_device_context_array *dcbaa;
> -	struct xhci_interrupter *interrupter;
> +	struct xhci_interrupter **interrupters;
>   	struct xhci_ring	*cmd_ring;
>   	unsigned int            cmd_ring_state;
>   #define CMD_RING_STATE_RUNNING         (1 << 0)
> diff --git a/include/linux/usb/xhci-intr.h b/include/linux/usb/xhci-intr.h
> new file mode 100644
> index 000000000000..e0091ee2c73a
> --- /dev/null
> +++ b/include/linux/usb/xhci-intr.h
> @@ -0,0 +1,86 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __LINUX_XHCI_INTR_H
> +#define __LINUX_XHCI_INTR_H
> +
> +#include <linux/kernel.h>
> +
> +struct xhci_erst_entry {
> +	/* 64-bit event ring segment address */
> +	__le64	seg_addr;
> +	__le32	seg_size;
> +	/* Set to zero */
> +	__le32	rsvd;
> +};
> +
> +enum xhci_ring_type {
> +	TYPE_CTRL = 0,
> +	TYPE_ISOC,
> +	TYPE_BULK,
> +	TYPE_INTR,
> +	TYPE_STREAM,
> +	TYPE_COMMAND,
> +	TYPE_EVENT,
> +};
> +
> +struct xhci_erst {
> +	struct xhci_erst_entry	*entries;
> +	unsigned int		num_entries;
> +	/* xhci->event_ring keeps track of segment dma addresses */
> +	dma_addr_t		erst_dma_addr;
> +	/* Num entries the ERST can contain */
> +	unsigned int		erst_size;
> +};
> +
> +struct xhci_segment {
> +	union xhci_trb		*trbs;
> +	/* private to HCD */
> +	struct xhci_segment	*next;
> +	dma_addr_t		dma;
> +	/* Max packet sized bounce buffer for td-fragmant alignment */
> +	dma_addr_t		bounce_dma;
> +	void			*bounce_buf;
> +	unsigned int		bounce_offs;
> +	unsigned int		bounce_len;
> +};
> +
> +struct xhci_ring {
> +	struct xhci_segment	*first_seg;
> +	struct xhci_segment	*last_seg;
> +	union  xhci_trb		*enqueue;
> +	struct xhci_segment	*enq_seg;
> +	union  xhci_trb		*dequeue;
> +	struct xhci_segment	*deq_seg;
> +	struct list_head	td_list;
> +	/*
> +	 * Write the cycle state into the TRB cycle field to give ownership of
> +	 * the TRB to the host controller (if we are the producer), or to check
> +	 * if we own the TRB (if we are the consumer).  See section 4.9.1.
> +	 */
> +	u32			cycle_state;
> +	unsigned int		stream_id;
> +	unsigned int		num_segs;
> +	unsigned int		num_trbs_free;
> +	unsigned int		num_trbs_free_temp;
> +	unsigned int		bounce_buf_len;
> +	enum xhci_ring_type	type;
> +	bool			last_td_was_short;
> +	struct radix_tree_root	*trb_address_map;
> +};
> +
> +struct xhci_interrupter {
> +	struct xhci_ring	*event_ring;
> +	struct xhci_erst	erst;
> +	struct xhci_intr_reg __iomem *ir_set;
> +	unsigned int		intr_num;
> +	/* For interrupter registers save and restore over suspend/resume */
> +	u32	s3_irq_pending;
> +	u32	s3_irq_control;
> +	u32	s3_erst_size;
> +	u64	s3_erst_base;
> +	u64	s3_erst_dequeue;
> +};
> +
> +struct xhci_interrupter *
> +xhci_create_secondary_interrupter(struct usb_hcd *hcd);
> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrupter *ir);
> +#endif
> 

Not convinced we want to share all these xhci private structures in a separate
header outside of the xhci code.

As much as possible should be abstracted and added to the xhci sideband
API in patch 3/33 instead of sharing these.
  
Thanks
Mathias
Mathias Nyman Sept. 28, 2023, 1:31 p.m. UTC | #10
On 22.9.2023 0.48, Wesley Cheng wrote:
> From: Mathias Nyman <mathias.nyman@linux.intel.com>
> 
> Expose xhci_stop_endpoint_sync() which is a synchronous variant of
> xhci_queue_stop_endpoint().  This is useful for client drivers that are
> using the secondary interrupters, and need to stop/clean up the current
> session.  The stop endpoint command handler will also take care of cleaning
> up the ring.
> 
> Modifications to repurpose the new API into existing stop endpoint
> sequences was implemented by Wesley Cheng.
> 
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
>   drivers/usb/host/xhci-hub.c | 29 +++---------------
>   drivers/usb/host/xhci.c     | 60 +++++++++++++++++++++++++++----------
>   drivers/usb/host/xhci.h     |  2 ++
>   3 files changed, 50 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 0054d02239e2..2f7309bdc922 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -489,7 +489,6 @@ EXPORT_SYMBOL_GPL(xhci_find_slot_id_by_port);
>   static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>   {
>   	struct xhci_virt_device *virt_dev;
> -	struct xhci_command *cmd;
>   	unsigned long flags;
>   	int ret;
>   	int i;
> @@ -501,10 +500,6 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>   
>   	trace_xhci_stop_device(virt_dev);
>   
> -	cmd = xhci_alloc_command(xhci, true, GFP_NOIO);
> -	if (!cmd)
> -		return -ENOMEM;
> -
>   	spin_lock_irqsave(&xhci->lock, flags);
>   	for (i = LAST_EP_INDEX; i > 0; i--) {
>   		if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) {
> @@ -521,7 +516,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>   			if (!command) {
>   				spin_unlock_irqrestore(&xhci->lock, flags);
>   				ret = -ENOMEM;
> -				goto cmd_cleanup;
> +				goto out;
>   			}
>   
>   			ret = xhci_queue_stop_endpoint(xhci, command, slot_id,
> @@ -529,30 +524,14 @@ static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
>   			if (ret) {
>   				spin_unlock_irqrestore(&xhci->lock, flags);
>   				xhci_free_command(xhci, command);
> -				goto cmd_cleanup;
> +				goto out;
>   			}
>   		}
>   	}
> -	ret = xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
> -	if (ret) {
> -		spin_unlock_irqrestore(&xhci->lock, flags);
> -		goto cmd_cleanup;
> -	}
> -
> -	xhci_ring_cmd_db(xhci);
>   	spin_unlock_irqrestore(&xhci->lock, flags);
> +	ret = xhci_stop_endpoint_sync(xhci, &virt_dev->eps[0], suspend);

I didn't take this new xhci_stop_endpoint_sync() helper into use as it causes an extra
xhci spinlock release and reacquire here.

Also the memory allocation flags differ, GFP_NOIO is turned into GFP_KERNEL after this change.

>   
> -	/* Wait for last stop endpoint command to finish */
> -	wait_for_completion(cmd->completion);
> -
> -	if (cmd->status == COMP_COMMAND_ABORTED ||
> -	    cmd->status == COMP_COMMAND_RING_STOPPED) {
> -		xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
> -		ret = -ETIME;
> -	}
> -
> -cmd_cleanup:
> -	xhci_free_command(xhci, cmd);
> +out:
>   	return ret;
>   }
>   
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 3fd2b58ee1d3..163d533d6200 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -2758,6 +2758,46 @@ static int xhci_reserve_bandwidth(struct xhci_hcd *xhci,
>   	return -ENOMEM;
>   }
>   
> +/*
> + * Synchronous XHCI stop endpoint helper.  Issues the stop endpoint command and
> + * waits for the command completion before returning.
> + */
> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct xhci_virt_ep *ep, int suspend)
> +{
> +	struct xhci_command *command;
> +	unsigned long flags;
> +	int ret;
> +
> +	command = xhci_alloc_command(xhci, true, GFP_KERNEL);
> +	if (!command)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&xhci->lock, flags);
> +	ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id,
> +				       ep->ep_index, suspend);
> +	if (ret < 0) {
> +		spin_unlock_irqrestore(&xhci->lock, flags);
> +		goto out;
> +	}
> +
> +	xhci_ring_cmd_db(xhci);
> +	spin_unlock_irqrestore(&xhci->lock, flags);
> +
> +	ret = wait_for_completion_timeout(command->completion, msecs_to_jiffies(3000));
> +	if (!ret)
> +		xhci_warn(xhci, "%s: Unable to stop endpoint.\n",
> +				__func__);
> +
> +	if (command->status == COMP_COMMAND_ABORTED ||
> +	    command->status == COMP_COMMAND_RING_STOPPED) {
> +		xhci_warn(xhci, "Timeout while waiting for stop endpoint command\n");
> +		ret = -ETIME;
> +	}
> +out:
> +	xhci_free_command(xhci, command);
> +
> +	return ret;
> +}
>   
>   /* Issue a configure endpoint command or evaluate context command
>    * and wait for it to finish.
> @@ -3078,7 +3118,7 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
>   	struct xhci_virt_device *vdev;
>   	struct xhci_virt_ep *ep;
>   	struct xhci_input_control_ctx *ctrl_ctx;
> -	struct xhci_command *stop_cmd, *cfg_cmd;
> +	struct xhci_command *cfg_cmd;
>   	unsigned int ep_index;
>   	unsigned long flags;
>   	u32 ep_flag;
> @@ -3118,10 +3158,6 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
>   	if (ep_flag == SLOT_FLAG || ep_flag == EP0_FLAG)
>   		return;
>   
> -	stop_cmd = xhci_alloc_command(xhci, true, GFP_NOWAIT);
> -	if (!stop_cmd)
> -		return;
> -
>   	cfg_cmd = xhci_alloc_command_with_ctx(xhci, true, GFP_NOWAIT);
>   	if (!cfg_cmd)
>   		goto cleanup;
> @@ -3144,23 +3180,16 @@ static void xhci_endpoint_reset(struct usb_hcd *hcd,
>   		goto cleanup;
>   	}
>   
> -	err = xhci_queue_stop_endpoint(xhci, stop_cmd, udev->slot_id,
> -					ep_index, 0);
> +	spin_unlock_irqrestore(&xhci->lock, flags);
> +

Same here, extra unlock -> lock, and GFP flags differ.


> +	err = xhci_stop_endpoint_sync(xhci, ep, 0);

Thanks
Mathias
Wesley Cheng Sept. 28, 2023, 10:10 p.m. UTC | #11
Hi Mathias,

On 9/28/2023 6:31 AM, Mathias Nyman wrote:
> On 22.9.2023 0.48, Wesley Cheng wrote:
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> Expose xhci_stop_endpoint_sync() which is a synchronous variant of
>> xhci_queue_stop_endpoint().  This is useful for client drivers that are
>> using the secondary interrupters, and need to stop/clean up the current
>> session.  The stop endpoint command handler will also take care of 
>> cleaning
>> up the ring.
>>
>> Modifications to repurpose the new API into existing stop endpoint
>> sequences was implemented by Wesley Cheng.
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/host/xhci-hub.c | 29 +++---------------
>>   drivers/usb/host/xhci.c     | 60 +++++++++++++++++++++++++++----------
>>   drivers/usb/host/xhci.h     |  2 ++
>>   3 files changed, 50 insertions(+), 41 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index 0054d02239e2..2f7309bdc922 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -489,7 +489,6 @@ EXPORT_SYMBOL_GPL(xhci_find_slot_id_by_port);
>>   static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int 
>> suspend)
>>   {
>>       struct xhci_virt_device *virt_dev;
>> -    struct xhci_command *cmd;
>>       unsigned long flags;
>>       int ret;
>>       int i;
>> @@ -501,10 +500,6 @@ static int xhci_stop_device(struct xhci_hcd 
>> *xhci, int slot_id, int suspend)
>>       trace_xhci_stop_device(virt_dev);
>> -    cmd = xhci_alloc_command(xhci, true, GFP_NOIO);
>> -    if (!cmd)
>> -        return -ENOMEM;
>> -
>>       spin_lock_irqsave(&xhci->lock, flags);
>>       for (i = LAST_EP_INDEX; i > 0; i--) {
>>           if (virt_dev->eps[i].ring && virt_dev->eps[i].ring->dequeue) {
>> @@ -521,7 +516,7 @@ static int xhci_stop_device(struct xhci_hcd *xhci, 
>> int slot_id, int suspend)
>>               if (!command) {
>>                   spin_unlock_irqrestore(&xhci->lock, flags);
>>                   ret = -ENOMEM;
>> -                goto cmd_cleanup;
>> +                goto out;
>>               }
>>               ret = xhci_queue_stop_endpoint(xhci, command, slot_id,
>> @@ -529,30 +524,14 @@ static int xhci_stop_device(struct xhci_hcd 
>> *xhci, int slot_id, int suspend)
>>               if (ret) {
>>                   spin_unlock_irqrestore(&xhci->lock, flags);
>>                   xhci_free_command(xhci, command);
>> -                goto cmd_cleanup;
>> +                goto out;
>>               }
>>           }
>>       }
>> -    ret = xhci_queue_stop_endpoint(xhci, cmd, slot_id, 0, suspend);
>> -    if (ret) {
>> -        spin_unlock_irqrestore(&xhci->lock, flags);
>> -        goto cmd_cleanup;
>> -    }
>> -
>> -    xhci_ring_cmd_db(xhci);
>>       spin_unlock_irqrestore(&xhci->lock, flags);
>> +    ret = xhci_stop_endpoint_sync(xhci, &virt_dev->eps[0], suspend);
> 
> I didn't take this new xhci_stop_endpoint_sync() helper into use as it 
> causes an extra
> xhci spinlock release and reacquire here.
> 
> Also the memory allocation flags differ, GFP_NOIO is turned into 
> GFP_KERNEL after this change.
> 

Thanks for the review.  I agree with the points made.  I wasn't sure if 
the extra unlock/lock would cause issues if we've already queued the 
stop ep for the other eps used by the device.

I think addressing the flags might be straightforward, we can just pass 
it in as an argument.  At least for this change in particular, is the 
concern that there could be another XHCI command queued before the stop 
endpoint command is?

>> -    /* Wait for last stop endpoint command to finish */
>> -    wait_for_completion(cmd->completion);
>> -
>> -    if (cmd->status == COMP_COMMAND_ABORTED ||
>> -        cmd->status == COMP_COMMAND_RING_STOPPED) {
>> -        xhci_warn(xhci, "Timeout while waiting for stop endpoint 
>> command\n");
>> -        ret = -ETIME;
>> -    }
>> -
>> -cmd_cleanup:
>> -    xhci_free_command(xhci, cmd);
>> +out:
>>       return ret;
>>   }
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index 3fd2b58ee1d3..163d533d6200 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -2758,6 +2758,46 @@ static int xhci_reserve_bandwidth(struct 
>> xhci_hcd *xhci,
>>       return -ENOMEM;
>>   }
>> +/*
>> + * Synchronous XHCI stop endpoint helper.  Issues the stop endpoint 
>> command and
>> + * waits for the command completion before returning.
>> + */
>> +int xhci_stop_endpoint_sync(struct xhci_hcd *xhci, struct 
>> xhci_virt_ep *ep, int suspend)
>> +{
>> +    struct xhci_command *command;
>> +    unsigned long flags;
>> +    int ret;
>> +
>> +    command = xhci_alloc_command(xhci, true, GFP_KERNEL);
>> +    if (!command)
>> +        return -ENOMEM;
>> +
>> +    spin_lock_irqsave(&xhci->lock, flags);
>> +    ret = xhci_queue_stop_endpoint(xhci, command, ep->vdev->slot_id,
>> +                       ep->ep_index, suspend);
>> +    if (ret < 0) {
>> +        spin_unlock_irqrestore(&xhci->lock, flags);
>> +        goto out;
>> +    }
>> +
>> +    xhci_ring_cmd_db(xhci);
>> +    spin_unlock_irqrestore(&xhci->lock, flags);
>> +
>> +    ret = wait_for_completion_timeout(command->completion, 
>> msecs_to_jiffies(3000));
>> +    if (!ret)
>> +        xhci_warn(xhci, "%s: Unable to stop endpoint.\n",
>> +                __func__);
>> +
>> +    if (command->status == COMP_COMMAND_ABORTED ||
>> +        command->status == COMP_COMMAND_RING_STOPPED) {
>> +        xhci_warn(xhci, "Timeout while waiting for stop endpoint 
>> command\n");
>> +        ret = -ETIME;
>> +    }
>> +out:
>> +    xhci_free_command(xhci, command);
>> +
>> +    return ret;
>> +}
>>   /* Issue a configure endpoint command or evaluate context command
>>    * and wait for it to finish.
>> @@ -3078,7 +3118,7 @@ static void xhci_endpoint_reset(struct usb_hcd 
>> *hcd,
>>       struct xhci_virt_device *vdev;
>>       struct xhci_virt_ep *ep;
>>       struct xhci_input_control_ctx *ctrl_ctx;
>> -    struct xhci_command *stop_cmd, *cfg_cmd;
>> +    struct xhci_command *cfg_cmd;
>>       unsigned int ep_index;
>>       unsigned long flags;
>>       u32 ep_flag;
>> @@ -3118,10 +3158,6 @@ static void xhci_endpoint_reset(struct usb_hcd 
>> *hcd,
>>       if (ep_flag == SLOT_FLAG || ep_flag == EP0_FLAG)
>>           return;
>> -    stop_cmd = xhci_alloc_command(xhci, true, GFP_NOWAIT);
>> -    if (!stop_cmd)
>> -        return;
>> -
>>       cfg_cmd = xhci_alloc_command_with_ctx(xhci, true, GFP_NOWAIT);
>>       if (!cfg_cmd)
>>           goto cleanup;
>> @@ -3144,23 +3180,16 @@ static void xhci_endpoint_reset(struct usb_hcd 
>> *hcd,
>>           goto cleanup;
>>       }
>> -    err = xhci_queue_stop_endpoint(xhci, stop_cmd, udev->slot_id,
>> -                    ep_index, 0);
>> +    spin_unlock_irqrestore(&xhci->lock, flags);
>> +
> 
> Same here, extra unlock -> lock, and GFP flags differ.
> 
> 

My intention here (minus the GFP flags) was that the locking was mainly 
for setting the EP state flag -- EP_SOFT_CLEAR_TOGGLE.  If that was set, 
then new TD queues are blocked.  Seems like that was why there is a 
check like this afterwards:

if (!list_empty(&ep->ring->td_list)) {

So I believed that releasing the lock here was going to be ok, because 
by that point since the flag is set, nothing would be able to be added 
to the td_list.

Thanks
Wesley Cheng
Wesley Cheng Oct. 2, 2023, 8:07 p.m. UTC | #12
Hi Mathias,

On 9/28/2023 3:31 AM, Mathias Nyman wrote:
> On 22.9.2023 0.48, Wesley Cheng wrote:
>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>
>> Modify the XHCI drivers to accommodate for handling multiple event 
>> rings in
>> case there are multiple interrupters.  Add the required APIs so 
>> clients are
>> able to allocate/request for an interrupter ring, and pass this 
>> information
>> back to the client driver.  This allows for users to handle the resource
>> accordingly, such as passing the event ring base address to an audio DSP.
>> There is no actual support for multiple MSI/MSI-X vectors.
>>
>> Factoring out XHCI interrupter APIs and structures done by Wesley 
>> Cheng, in
>> order to allow for USB class drivers to utilze them.
>>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Co-developed-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
>> ---
>>   drivers/usb/host/xhci-debugfs.c |  2 +-
>>   drivers/usb/host/xhci-mem.c     | 93 ++++++++++++++++++++++++++++++---
>>   drivers/usb/host/xhci-ring.c    |  2 +-
>>   drivers/usb/host/xhci.c         | 49 ++++++++++-------
>>   drivers/usb/host/xhci.h         | 77 +--------------------------
>>   include/linux/usb/xhci-intr.h   | 86 ++++++++++++++++++++++++++++++
>>   6 files changed, 207 insertions(+), 102 deletions(-)
>>   create mode 100644 include/linux/usb/xhci-intr.h
>>
>> diff --git a/drivers/usb/host/xhci-debugfs.c 
>> b/drivers/usb/host/xhci-debugfs.c
>> index 99baa60ef50f..15a8402ee8a1 100644
>> --- a/drivers/usb/host/xhci-debugfs.c
>> +++ b/drivers/usb/host/xhci-debugfs.c
>> @@ -693,7 +693,7 @@ void xhci_debugfs_init(struct xhci_hcd *xhci)
>>                        "command-ring",
>>                        xhci->debugfs_root);
>> -    xhci_debugfs_create_ring_dir(xhci, &xhci->interrupter->event_ring,
>> +    xhci_debugfs_create_ring_dir(xhci, 
>> &xhci->interrupters[0]->event_ring,
>>                        "event-ring",
>>                        xhci->debugfs_root);
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 8714ab5bf04d..2f9228d7d22d 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -1837,6 +1837,26 @@ xhci_free_interrupter(struct xhci_hcd *xhci, 
>> struct xhci_interrupter *ir)
>>       kfree(ir);
>>   }
>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct 
>> xhci_interrupter *ir)
>> +{
>> +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +    unsigned int intr_num;
>> +
>> +    /* interrupter 0 is primary interrupter, don't touchit */
>> +    if (!ir || !ir->intr_num || ir->intr_num >= 
>> xhci->max_interrupters) {
>> +        xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
>> +        return;
>> +    }
>> +
>> +    /* fixme, should we check xhci->interrupter[intr_num] == ir */
>> +    spin_lock(&xhci->lock);
> 
> Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is used 
> in interrupt handler.
> 
> 
>> +    intr_num = ir->intr_num;
>> +    xhci_free_interrupter(xhci, ir);
>> +    xhci->interrupters[intr_num] = NULL;
>> +    spin_unlock(&xhci->lock);
> 
> likewise
> 

Let me check these again.  In general, I think I will use both the 
xhci->mutex and xhci->lock where needed, because I believe we'd run into 
sleep while atomic issues while freeing the DMA memory.  Will rework 
this and submit in the next rev.

>> +}
>> +EXPORT_SYMBOL_GPL(xhci_remove_secondary_interrupter);
>> +
>>   void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>   {
>>       struct device    *dev = xhci_to_hcd(xhci)->self.sysdev;
>> @@ -1844,9 +1864,13 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>       cancel_delayed_work_sync(&xhci->cmd_timer);
>> -    xhci_free_interrupter(xhci, xhci->interrupter);
>> -    xhci->interrupter = NULL;
>> -    xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed primary event 
>> ring");
>> +    for (i = 0; i < xhci->max_interrupters; i++) {
>> +        if (xhci->interrupters[i]) {
>> +            xhci_free_interrupter(xhci, xhci->interrupters[i]);
>> +            xhci->interrupters[i] = NULL;
>> +        }
>> +    }
>> +    xhci_dbg_trace(xhci, trace_xhci_dbg_init, "Freed interrupters");
>>       if (xhci->cmd_ring)
>>           xhci_ring_free(xhci, xhci->cmd_ring);
>> @@ -1916,6 +1940,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>       for (i = 0; i < xhci->num_port_caps; i++)
>>           kfree(xhci->port_caps[i].psi);
>>       kfree(xhci->port_caps);
>> +    kfree(xhci->interrupters);
>>       xhci->num_port_caps = 0;
>>       xhci->usb2_rhub.ports = NULL;
>> @@ -1924,6 +1949,7 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
>>       xhci->rh_bw = NULL;
>>       xhci->ext_caps = NULL;
>>       xhci->port_caps = NULL;
>> +    xhci->interrupters = NULL;
>>       xhci->page_size = 0;
>>       xhci->page_shift = 0;
>> @@ -2276,6 +2302,13 @@ xhci_add_interrupter(struct xhci_hcd *xhci, 
>> struct xhci_interrupter *ir,
>>           return -EINVAL;
>>       }
>> +    if (xhci->interrupters[intr_num]) {
>> +        xhci_warn(xhci, "Interrupter%d\n already set up", intr_num);
>> +        return -EINVAL;
>> +    }
>> +
>> +    xhci->interrupters[intr_num] = ir;
>> +    ir->intr_num = intr_num;
>>       ir->ir_set = &xhci->run_regs->ir_set[intr_num];
>>       /* set ERST count with the number of entries in the segment 
>> table */
>> @@ -2295,10 +2328,53 @@ xhci_add_interrupter(struct xhci_hcd *xhci, 
>> struct xhci_interrupter *ir,
>>       return 0;
>>   }
>> +struct xhci_interrupter *
>> +xhci_create_secondary_interrupter(struct usb_hcd *hcd)
>> +{
>> +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> +    struct xhci_interrupter *ir;
>> +    unsigned int i;
>> +    int err = -ENOSPC;
>> +
>> +    if (!xhci->interrupters)
>> +        return NULL;
>> +
>> +    ir = xhci_alloc_interrupter(xhci, GFP_KERNEL);
>> +    if (!ir)
>> +        return NULL;
>> +
>> +    spin_lock_irq(&xhci->lock);
>> +
>> +    /* Find available secondary interrupter, interrupter0 is 
>> reserverd for primary */
> 
> reserved
> 
>> +    for (i = 1; i < xhci->max_interrupters; i++) {
>> +        if (xhci->interrupters[i] == NULL) {
>> +            err = xhci_add_interrupter(xhci, ir, i);
>> +            break;
>> +        }
>> +    }
>> +
>> +    spin_unlock_irq(&xhci->lock);
>> +    if (err) {
>> +        xhci_warn(xhci, "Failed to add secondary interrupter, max 
>> interrupters %d\n",
>> +            xhci->max_interrupters);
>> +        xhci_free_interrupter(xhci, ir);
>> +        ir = NULL;
>> +        goto out;
>> +    }
>> +
>> +    xhci_dbg(xhci, "Add secondary interrupter %d, max interrupters 
>> %d\n",
>> +         i, xhci->max_interrupters);
>> +
>> +out:
>> +    return ir;
>> +}
>> +EXPORT_SYMBOL_GPL(xhci_create_secondary_interrupter);
>> +
>>   int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>>   {
>> -    dma_addr_t    dma;
>> +    struct xhci_interrupter *ir;
>>       struct device    *dev = xhci_to_hcd(xhci)->self.sysdev;
>> +    dma_addr_t    dma;
>>       unsigned int    val, val2;
>>       u64        val_64;
>>       u32        page_size, temp;
>> @@ -2422,11 +2498,14 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t 
>> flags)
>>       /* Allocate and set up primary interrupter 0 with an event ring. */
>>       xhci_dbg_trace(xhci, trace_xhci_dbg_init,
>>                  "Allocating primary event ring");
>> -    xhci->interrupter = xhci_alloc_interrupter(xhci, flags);
>> -    if (!xhci->interrupter)
>> +    xhci->interrupters = kcalloc_node(xhci->max_interrupters, 
>> sizeof(*xhci->interrupters),
>> +                      flags, dev_to_node(dev));
>> +
>> +    ir = xhci_alloc_interrupter(xhci, flags);
>> +    if (!ir)
>>           goto fail;
>> -    if (xhci_add_interrupter(xhci, xhci->interrupter, 0))
>> +    if (xhci_add_interrupter(xhci, ir, 0))
>>           goto fail;
>>       xhci->isoc_bei_interval = AVOID_BEI_INTERVAL_MAX;
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 1dde53f6eb31..93233cf5ff21 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -3074,7 +3074,7 @@ irqreturn_t xhci_irq(struct usb_hcd *hcd)
>>       writel(status, &xhci->op_regs->status);
>>       /* This is the handler of the primary interrupter */
>> -    ir = xhci->interrupter;
>> +    ir = xhci->interrupters[0];
>>       if (!hcd->msi_enabled) {
>>           u32 irq_pending;
>>           irq_pending = readl(&ir->ir_set->irq_pending);
>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>> index e1b1b64a0723..3fd2b58ee1d3 100644
>> --- a/drivers/usb/host/xhci.c
>> +++ b/drivers/usb/host/xhci.c
>> @@ -456,7 +456,7 @@ static int xhci_init(struct usb_hcd *hcd)
>>   static int xhci_run_finished(struct xhci_hcd *xhci)
>>   {
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir = xhci->interrupters[0];
>>       unsigned long    flags;
>>       u32        temp;
>> @@ -508,7 +508,7 @@ int xhci_run(struct usb_hcd *hcd)
>>       u64 temp_64;
>>       int ret;
>>       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir = xhci->interrupters[0];
>>       /* Start the xHCI host controller runningonly after the USB 2.0 
>> roothub
>>        * is setup.
>>        */
>> @@ -572,7 +572,7 @@ void xhci_stop(struct usb_hcd *hcd)
>>   {
>>       u32 temp;
>>       struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir = xhci->interrupters[0];
>>       mutex_lock(&xhci->mutex);
>> @@ -668,36 +668,49 @@ EXPORT_SYMBOL_GPL(xhci_shutdown);
>>   #ifdef CONFIG_PM
>>   static void xhci_save_registers(struct xhci_hcd *xhci)
>>   {
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir;
>> +    unsigned int i;
>>       xhci->s3.command = readl(&xhci->op_regs->command);
>>       xhci->s3.dev_nt = readl(&xhci->op_regs->dev_notification);
>>       xhci->s3.dcbaa_ptr = xhci_read_64(xhci,&xhci->op_regs->dcbaa_ptr);
>>       xhci->s3.config_reg = readl(&xhci->op_regs->config_reg);
>> -    if (!ir)
>> -        return;
>> +    /* save both primary and all secondary interrupters */
>> +    for (i = 0; i < xhci->max_interrupters; i++) {
>> +        ir = xhci->interrupters[i];
>> +        if (!ir)
>> +            continue;
>> -    ir->s3_erst_size = readl(&ir->ir_set->erst_size);
>> -    ir->s3_erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
>> -    ir->s3_erst_dequeue = xhci_read_64(xhci, &ir->ir_set->erst_dequeue);
>> -    ir->s3_irq_pending = readl(&ir->ir_set->irq_pending);
>> -    ir->s3_irq_control = readl(&ir->ir_set->irq_control);
>> +        ir->s3_erst_size = readl(&ir->ir_set->erst_size);
>> +        ir->s3_erst_base = xhci_read_64(xhci, &ir->ir_set->erst_base);
>> +        ir->s3_erst_dequeue = xhci_read_64(xhci, 
>> &ir->ir_set->erst_dequeue);
>> +        ir->s3_irq_pending = readl(&ir->ir_set->irq_pending);
>> +        ir->s3_irq_control = readl(&ir->ir_set->irq_control);
>> +    }
>>   }
>>   static void xhci_restore_registers(struct xhci_hcd *xhci)
>>   {
>> -    struct xhci_interrupter *ir = xhci->interrupter;
>> +    struct xhci_interrupter *ir;
>> +    unsigned int i;
>>       writel(xhci->s3.command, &xhci->op_regs->command);
>>       writel(xhci->s3.dev_nt, &xhci->op_regs->dev_notification);
>>       xhci_write_64(xhci, xhci->s3.dcbaa_ptr, &xhci->op_regs->dcbaa_ptr);
>>       writel(xhci->s3.config_reg, &xhci->op_regs->config_reg);
>> -    writel(ir->s3_erst_size, &ir->ir_set->erst_size);
>> -    xhci_write_64(xhci, ir->s3_erst_base, &ir->ir_set->erst_base);
>> -    xhci_write_64(xhci, ir->s3_erst_dequeue, &ir->ir_set->erst_dequeue);
>> -    writel(ir->s3_irq_pending, &ir->ir_set->irq_pending);
>> -    writel(ir->s3_irq_control, &ir->ir_set->irq_control);
>> +
>> +    for (i = 0; i < xhci->max_interrupters; i++) {
>> +        ir = xhci->interrupters[i];
>> +        if (!ir)
>> +            continue;
>> +
>> +        writel(ir->s3_erst_size, &ir->ir_set->erst_size);
>> +        xhci_write_64(xhci, ir->s3_erst_base, &ir->ir_set->erst_base);
>> +        xhci_write_64(xhci, ir->s3_erst_dequeue, 
>> &ir->ir_set->erst_dequeue);
>> +        writel(ir->s3_irq_pending, &ir->ir_set->irq_pending);
>> +        writel(ir->s3_irq_control, &ir->ir_set->irq_control);
>> +    }
>>   }
>>   static void xhci_set_cmd_ring_deq(struct xhci_hcd *xhci)
>> @@ -1059,7 +1072,7 @@ int xhci_resume(struct xhci_hcd *xhci, 
>> pm_message_t msg)
>>           xhci_dbg(xhci, "// Disabling event ring interrupts\n");
>>           temp = readl(&xhci->op_regs->status);
>>           writel((temp & ~0x1fff) | STS_EINT, &xhci->op_regs->status);
>> -        xhci_disable_interrupter(xhci->interrupter);
>> +        xhci_disable_interrupter(xhci->interrupters[0]);
>>           xhci_dbg(xhci, "cleaning up memory\n");
>>           xhci_mem_cleanup(xhci);
> 
> All code above looks like it should be its own patch.
> 
> The header shuffling below part of somethine else.
>  >> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 7e282b4522c0..d706a27ec0a3 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -17,6 +17,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/usb/hcd.h>
>>   #include <linux/io-64-nonatomic-lo-hi.h>
>> +#include <linux/usb/xhci-intr.h>
>>   /* Code sharing between pci-quirks and xhci hcd */
>>   #include    "xhci-ext-caps.h"
>> @@ -1541,18 +1542,6 @@ static inline const char 
>> *xhci_trb_type_string(u8 type)
>>   #define AVOID_BEI_INTERVAL_MIN    8
>>   #define AVOID_BEI_INTERVAL_MAX    32
>> -struct xhci_segment {
>> -    union xhci_trb        *trbs;
>> -    /* private to HCD */
>> -    struct xhci_segment    *next;
>> -    dma_addr_t       dma;
>> -    /* Max packet sized bounce buffer for td-fragmant alignment */
>> -    dma_addr_t       bounce_dma;
>> -    void            *bounce_buf;
>> -    unsigned int        bounce_offs;
>> -    unsigned int        bounce_len;
>> -};
>> -
>>   enum xhci_cancelled_td_status {
>>       TD_DIRTY = 0,
>>       TD_HALTED,
>> @@ -1585,16 +1574,6 @@ struct xhci_cd {
>>       union xhci_trb        *cmd_trb;
>>   };
>> -enum xhci_ring_type {
>> -    TYPE_CTRL = 0,
>> -    TYPE_ISOC,
>> -    TYPE_BULK,
>> -    TYPE_INTR,
>> -    TYPE_STREAM,
>> -    TYPE_COMMAND,
>> -    TYPE_EVENT,
>> -};
>> -
>>   static inline const char *xhci_ring_type_string(enum xhci_ring_type 
>> type)
>>   {
>>       switch (type) {
>> @@ -1615,46 +1594,6 @@ static inline const char 
>> *xhci_ring_type_string(enum xhci_ring_type type)
>>       }
>>       return "UNKNOWN";
>> -}
>> -
>> -struct xhci_ring {
>> -    struct xhci_segment    *first_seg;
>> -    struct xhci_segment    *last_seg;
>> -    union  xhci_trb        *enqueue;
>> -    struct xhci_segment    *enq_seg;
>> -    union  xhci_trb        *dequeue;
>> -    struct xhci_segment    *deq_seg;
>> -    struct list_head    td_list;
>> -    /*
>> -     * Write the cycle state into the TRB cycle field to give 
>> ownership of
>> -     * the TRB to the host controller (if we are the producer), or to 
>> check
>> -     * if we own the TRB (if we are the consumer).  See section 4.9.1.
>> -     */
>> -    u32            cycle_state;
>> -    unsigned int        stream_id;
>> -    unsigned int        num_segs;
>> -    unsigned int        num_trbs_free; /* used only by xhci DbC */
>> -    unsigned int        bounce_buf_len;
>> -    enum xhci_ring_type    type;
>> -    bool            last_td_was_short;
>> -    struct radix_tree_root    *trb_address_map;
>> -};
>> -
>> -struct xhci_erst_entry {
>> -    /* 64-bit event ring segment address */
>> -    __le64    seg_addr;
>> -    __le32    seg_size;
>> -    /* Set to zero */
>> -    __le32    rsvd;
>> -};
>> -
>> -struct xhci_erst {
>> -    struct xhci_erst_entry    *entries;
>> -    unsigned int        num_entries;
>> -    /* xhci->event_ring keeps track of segment dma addresses */
>> -    dma_addr_t       erst_dma_addr;
>> -    /* Num entries the ERST can contain */
>> -    unsigned int        erst_size;
>>   };
>>   struct xhci_scratchpad {
>> @@ -1707,18 +1646,6 @@ struct xhci_bus_state {
>>       unsigned long        resuming_ports;
>>   };
>> -struct xhci_interrupter {
>> -    struct xhci_ring    *event_ring;
>> -    struct xhci_erst    erst;
>> -    struct xhci_intr_reg __iomem *ir_set;
>> -    unsigned int        intr_num;
>> -    /* For interrupter registers save and restore over suspend/resume */
>> -    u32    s3_irq_pending;
>> -    u32    s3_irq_control;
>> -    u32    s3_erst_size;
>> -    u64    s3_erst_base;
>> -    u64    s3_erst_dequeue;
>> -};
>>   /*
>>    * It can take up to 20 ms to transition from RExit to U0 onthe
>>    * Intel Lynx Point LP xHCI host.
>> @@ -1799,7 +1726,7 @@ struct xhci_hcd {
>>       struct reset_control *reset;
>>       /* data structures */
>>       struct xhci_device_context_array *dcbaa;
>> -    struct xhci_interrupter *interrupter;
>> +    struct xhci_interrupter **interrupters;
>>       struct xhci_ring    *cmd_ring;
>>       unsigned int            cmd_ring_state;
>>   #define CMD_RING_STATE_RUNNING         (1 << 0)
>> diff --git a/include/linux/usb/xhci-intr.h 
>> b/include/linux/usb/xhci-intr.h
>> new file mode 100644
>> index 000000000000..e0091ee2c73a
>> --- /dev/null
>> +++ b/include/linux/usb/xhci-intr.h
>> @@ -0,0 +1,86 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __LINUX_XHCI_INTR_H
>> +#define __LINUX_XHCI_INTR_H
>> +
>> +#include <linux/kernel.h>
>> +
>> +struct xhci_erst_entry {
>> +    /* 64-bit event ring segment address */
>> +    __le64    seg_addr;
>> +    __le32    seg_size;
>> +    /* Set to zero */
>> +    __le32    rsvd;
>> +};
>> +
>> +enum xhci_ring_type {
>> +    TYPE_CTRL = 0,
>> +    TYPE_ISOC,
>> +    TYPE_BULK,
>> +    TYPE_INTR,
>> +    TYPE_STREAM,
>> +    TYPE_COMMAND,
>> +    TYPE_EVENT,
>> +};
>> +
>> +struct xhci_erst {
>> +    struct xhci_erst_entry    *entries;
>> +    unsigned int        num_entries;
>> +    /* xhci->event_ring keeps track of segment dma addresses */
>> +    dma_addr_t       erst_dma_addr;
>> +    /* Num entries the ERST can contain */
>> +    unsigned int        erst_size;
>> +};
>> +
>> +struct xhci_segment {
>> +    union xhci_trb        *trbs;
>> +    /* private to HCD */
>> +    struct xhci_segment    *next;
>> +    dma_addr_t       dma;
>> +    /* Max packet sized bounce buffer for td-fragmant alignment */
>> +    dma_addr_t       bounce_dma;
>> +    void            *bounce_buf;
>> +    unsigned int        bounce_offs;
>> +    unsigned int        bounce_len;
>> +};
>> +
>> +struct xhci_ring {
>> +    struct xhci_segment    *first_seg;
>> +    struct xhci_segment    *last_seg;
>> +    union  xhci_trb        *enqueue;
>> +    struct xhci_segment    *enq_seg;
>> +    union  xhci_trb        *dequeue;
>> +    struct xhci_segment    *deq_seg;
>> +    struct list_head    td_list;
>> +    /*
>> +     * Write the cycle state into the TRB cycle field to give 
>> ownership of
>> +     * the TRB to the host controller (if we are the producer), or to 
>> check
>> +     * if we own the TRB (if we are the consumer).  See section 4.9.1.
>> +     */
>> +    u32            cycle_state;
>> +    unsigned int        stream_id;
>> +    unsigned int        num_segs;
>> +    unsigned int        num_trbs_free;
>> +    unsigned int        num_trbs_free_temp;
>> +    unsigned int        bounce_buf_len;
>> +    enum xhci_ring_type    type;
>> +    bool            last_td_was_short;
>> +    struct radix_tree_root    *trb_address_map;
>> +};
>> +
>> +struct xhci_interrupter {
>> +    struct xhci_ring    *event_ring;
>> +    struct xhci_erst    erst;
>> +    struct xhci_intr_reg __iomem *ir_set;
>> +    unsigned int        intr_num;
>> +    /* For interrupter registers save and restore over suspend/resume */
>> +    u32    s3_irq_pending;
>> +    u32    s3_irq_control;
>> +    u32    s3_erst_size;
>> +    u64    s3_erst_base;
>> +    u64    s3_erst_dequeue;
>> +};
>> +
>> +struct xhci_interrupter *
>> +xhci_create_secondary_interrupter(struct usb_hcd *hcd);
>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct 
>> xhci_interrupter *ir);
>> +#endif
>>
> 
> Not convinced we want to share all these xhci private structures in a 
> separate
> header outside of the xhci code.
> 
> As much as possible should be abstracted and added to the xhci sideband
> API in patch 3/33 instead of sharing these.

It gets a bit difficult because xhci_create_secondary_interrupter() will 
return struct xhci_interrupter, so that the class (offload) driver can 
fetch information about the event ring.  So part of that is that the 
class driver has to reference struct xhci_ring as well.

Instead of exposing all these into a header file, what do you think 
about adding the drivers/xhci path as an include directory in the class 
driver make arguments in the makefile?

Thanks
Wesley Cheng
Mathias Nyman Oct. 4, 2023, 2:02 p.m. UTC | #13
On 2.10.2023 23.07, Wesley Cheng wrote:
> Hi Mathias,
> 
> On 9/28/2023 3:31 AM, Mathias Nyman wrote:
>> On 22.9.2023 0.48, Wesley Cheng wrote:
>>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>
>>> Modify the XHCI drivers to accommodate for handling multiple event rings in
>>> case there are multiple interrupters.  Add the required APIs so clients are
>>> able to allocate/request for an interrupter ring, and pass this information
>>> back to the client driver.  This allows for users to handle the resource
>>> accordingly, such as passing the event ring base address to an audio DSP.
>>> There is no actual support for multiple MSI/MSI-X vectors.
>>>
>>> Factoring out XHCI interrupter APIs and structures done by Wesley Cheng, in
>>> order to allow for USB class drivers to utilze them.
>>>
>>>   }
>>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct xhci_interrupter *ir)
>>> +{
>>> +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>> +    unsigned int intr_num;
>>> +
>>> +    /* interrupter 0 is primary interrupter, don't touchit */
>>> +    if (!ir || !ir->intr_num || ir->intr_num >= xhci->max_interrupters) {
>>> +        xhci_dbg(xhci, "Invalid secondary interrupter, can't remove\n");
>>> +        return;
>>> +    }
>>> +
>>> +    /* fixme, should we check xhci->interrupter[intr_num] == ir */
>>> +    spin_lock(&xhci->lock);
>>
>> Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is used in interrupt handler.
>>
>>
>>> +    intr_num = ir->intr_num;
>>> +    xhci_free_interrupter(xhci, ir);
>>> +    xhci->interrupters[intr_num] = NULL;
>>> +    spin_unlock(&xhci->lock);
>>
>> likewise
>>
> 
> Let me check these again.  In general, I think I will use both the xhci->mutex and 
> xhci->lock where needed, because I believe we'd run into sleep while atomic issues
> while freeing the DMA memory.  Will rework this and submit in the next rev.
> 

Maybe we need to split xhci_free_interrupter() into separate remove and free functions

Did some work on this, and on the sideband api in general.

Code still has a lot of FIXMEs, and it's completely untested, but to avoid us
from doing duplicate work I pushed it to my feature_interrupters branch anyway

git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git feature_interrupters
https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters

Thanks
-Mathias
Wesley Cheng Oct. 4, 2023, 6:35 p.m. UTC | #14
Hi Mathias,

On 10/4/2023 7:02 AM, Mathias Nyman wrote:
> On 2.10.2023 23.07, Wesley Cheng wrote:
>> Hi Mathias,
>>
>> On 9/28/2023 3:31 AM, Mathias Nyman wrote:
>>> On 22.9.2023 0.48, Wesley Cheng wrote:
>>>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>>
>>>> Modify the XHCI drivers to accommodate for handling multiple event 
>>>> rings in
>>>> case there are multiple interrupters.  Add the required APIs so 
>>>> clients are
>>>> able to allocate/request for an interrupter ring, and pass this 
>>>> information
>>>> back to the client driver.  This allows for users to handle the 
>>>> resource
>>>> accordingly, such as passing the event ring base address to an audio 
>>>> DSP.
>>>> There is no actual support for multiple MSI/MSI-X vectors.
>>>>
>>>> Factoring out XHCI interrupter APIs and structures done by Wesley 
>>>> Cheng, in
>>>> order to allow for USB class drivers to utilze them.
>>>>
>>>>   }
>>>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct 
>>>> xhci_interrupter *ir)
>>>> +{
>>>> +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>> +    unsigned int intr_num;
>>>> +
>>>> +    /* interrupter 0 is primary interrupter, don't touchit */
>>>> +    if (!ir || !ir->intr_num || ir->intr_num >= 
>>>> xhci->max_interrupters) {
>>>> +        xhci_dbg(xhci, "Invalid secondary interrupter, can't 
>>>> remove\n");
>>>> +        return;
>>>> +    }
>>>> +
>>>> +    /* fixme, should we check xhci->interrupter[intr_num] == ir */
>>>> +    spin_lock(&xhci->lock);
>>>
>>> Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is 
>>> used in interrupt handler.
>>>
>>>
>>>> +    intr_num = ir->intr_num;
>>>> +    xhci_free_interrupter(xhci, ir);
>>>> +    xhci->interrupters[intr_num] = NULL;
>>>> +    spin_unlock(&xhci->lock);
>>>
>>> likewise
>>>
>>
>> Let me check these again.  In general, I think I will use both the 
>> xhci->mutex and xhci->lock where needed, because I believe we'd run 
>> into sleep while atomic issues
>> while freeing the DMA memory.  Will rework this and submit in the next 
>> rev.
>>
> 
> Maybe we need to split xhci_free_interrupter() into separate remove and 
> free functions
> 

Thanks for sharing the work you've been doing.  Yes, I did something 
similar as well on my end, but will refactor in your code and re-test.

> Did some work on this, and on the sideband api in general.
> 
> Code still has a lot of FIXMEs, and it's completely untested, but to 
> avoid us
> from doing duplicate work I pushed it to my feature_interrupters branch 
> anyway
> 
> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git 
> feature_interrupters
> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters 
> 

Ok.  Initial look at it seems like it will be fine, but will integrate 
and make changes where needed.

Thanks
Wesley Cheng
Wesley Cheng Oct. 4, 2023, 11:42 p.m. UTC | #15
Hi Mathias,

On 10/4/2023 11:35 AM, Wesley Cheng wrote:
> Hi Mathias,
> 
> On 10/4/2023 7:02 AM, Mathias Nyman wrote:
>> On 2.10.2023 23.07, Wesley Cheng wrote:
>>> Hi Mathias,
>>>
>>> On 9/28/2023 3:31 AM, Mathias Nyman wrote:
>>>> On 22.9.2023 0.48, Wesley Cheng wrote:
>>>>> From: Mathias Nyman <mathias.nyman@linux.intel.com>
>>>>>
>>>>> Modify the XHCI drivers to accommodate for handling multiple event 
>>>>> rings in
>>>>> case there are multiple interrupters.  Add the required APIs so 
>>>>> clients are
>>>>> able to allocate/request for an interrupter ring, and pass this 
>>>>> information
>>>>> back to the client driver.  This allows for users to handle the 
>>>>> resource
>>>>> accordingly, such as passing the event ring base address to an 
>>>>> audio DSP.
>>>>> There is no actual support for multiple MSI/MSI-X vectors.
>>>>>
>>>>> Factoring out XHCI interrupter APIs and structures done by Wesley 
>>>>> Cheng, in
>>>>> order to allow for USB class drivers to utilze them.
>>>>>
>>>>>   }
>>>>> +void xhci_remove_secondary_interrupter(struct usb_hcd *hcd, struct 
>>>>> xhci_interrupter *ir)
>>>>> +{
>>>>> +    struct xhci_hcd *xhci = hcd_to_xhci(hcd);
>>>>> +    unsigned int intr_num;
>>>>> +
>>>>> +    /* interrupter 0 is primary interrupter, don't touchit */
>>>>> +    if (!ir || !ir->intr_num || ir->intr_num >= 
>>>>> xhci->max_interrupters) {
>>>>> +        xhci_dbg(xhci, "Invalid secondary interrupter, can't 
>>>>> remove\n");
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>>> +    /* fixme, should we check xhci->interrupter[intr_num] == ir */
>>>>> +    spin_lock(&xhci->lock);
>>>>
>>>> Needs to be spin_lock_irq() ir spin_lock_irqsave() as xhci->lock is 
>>>> used in interrupt handler.
>>>>
>>>>
>>>>> +    intr_num = ir->intr_num;
>>>>> +    xhci_free_interrupter(xhci, ir);
>>>>> +    xhci->interrupters[intr_num] = NULL;
>>>>> +    spin_unlock(&xhci->lock);
>>>>
>>>> likewise
>>>>
>>>
>>> Let me check these again.  In general, I think I will use both the 
>>> xhci->mutex and xhci->lock where needed, because I believe we'd run 
>>> into sleep while atomic issues
>>> while freeing the DMA memory.  Will rework this and submit in the 
>>> next rev.
>>>
>>
>> Maybe we need to split xhci_free_interrupter() into separate remove 
>> and free functions
>>
> 
> Thanks for sharing the work you've been doing.  Yes, I did something 
> similar as well on my end, but will refactor in your code and re-test.
> 
>> Did some work on this, and on the sideband api in general.
>>
>> Code still has a lot of FIXMEs, and it's completely untested, but to 
>> avoid us
>> from doing duplicate work I pushed it to my feature_interrupters 
>> branch anyway
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git 
>> feature_interrupters
>> https://git.kernel.org/pub/scm/linux/kernel/git/mnyman/xhci.git/log/?h=feature_interrupters 
>>
> 
> Ok.  Initial look at it seems like it will be fine, but will integrate 
> and make changes where needed.
> 

Had to make some minor tweaks here and there, but nothing major.  Was 
able to validate the changes on my end, and they look good.  Will test a 
bit more, and include these in my next submission.  Will try to address 
your FIXME tags as well.

Thanks
Wesley Cheng