Re: [PATCH v9 19/34] ALSA: usb-audio: qcom: Introduce QC USB SND offloading support

+config SND_USB_AUDIO_QMI

• tristate "Qualcomm Audio Offload driver"
• depends on QCOM_QMI_HELPERS && SND_USB_AUDIO && USB_XHCI_SIDEBAND
• select SND_PCM

This select is not needed:

config SND_USB_AUDIO tristate "USB Audio/MIDI driver" select SND_HWDEP select SND_RAWMIDI select SND_PCM

+#include <linux/ctype.h> +#include <linux/moduleparam.h> +#include <linux/module.h> +#include <linux/usb.h> +#include <linux/init.h>

alphabetical order?

+#include <linux/usb/hcd.h> +#include <linux/usb/xhci-sideband.h> +#include <linux/usb/quirks.h> +#include <linux/usb/audio.h> +#include <linux/usb/audio-v2.h> +#include <linux/usb/audio-v3.h> +#include <linux/soc/qcom/qmi.h> +#include <linux/iommu.h> +#include <linux/dma-mapping.h> +#include <linux/dma-map-ops.h> +#include <sound/q6usboffload.h>

+#include <sound/control.h> +#include <sound/core.h> +#include <sound/info.h> +#include <sound/pcm.h> +#include <sound/pcm_params.h> +#include <sound/initval.h>

+#include <sound/soc.h> +#include <sound/soc-usb.h> +#include "../usbaudio.h" +#include "../card.h" +#include "../endpoint.h" +#include "../helper.h" +#include "../pcm.h" +#include "../format.h" +#include "../power.h" +#include "usb_audio_qmi_v01.h"

+/* Stream disable request timeout during USB device disconnect */ +#define DEV_RELEASE_WAIT_TIMEOUT 10000 /* in ms */

DEV_RELEASE_WAIT_TIMEOUT_MS?

why 10s btw?

+/* Data interval calculation parameters */ +#define BUS_INTERVAL_FULL_SPEED 1000 /* in us */ +#define BUS_INTERVAL_HIGHSPEED_AND_ABOVE 125 /* in us */ +#define MAX_BINTERVAL_ISOC_EP 16

+#define QMI_STREAM_REQ_CARD_NUM_MASK 0xffff0000 +#define QMI_STREAM_REQ_DEV_NUM_MASK 0xff00 +#define QMI_STREAM_REQ_DIRECTION 0xff

+/* iommu resource parameters and management */ +#define PREPEND_SID_TO_IOVA(iova, sid) ((u64)(((u64)(iova)) | \

• 			(((u64)sid) << 32)))
  

+#define IOVA_BASE 0x1000 +#define IOVA_XFER_RING_BASE (IOVA_BASE + PAGE_SIZE * (SNDRV_CARDS + 1)) +#define IOVA_XFER_BUF_BASE (IOVA_XFER_RING_BASE + PAGE_SIZE * SNDRV_CARDS * 32) +#define IOVA_XFER_RING_MAX (IOVA_XFER_BUF_BASE - PAGE_SIZE) +#define IOVA_XFER_BUF_MAX (0xfffff000 - PAGE_SIZE)

+#define MAX_XFER_BUFF_LEN (24 * PAGE_SIZE)

+struct iova_info {

• struct list_head list;
• unsigned long start_iova;
• size_t size;
• bool in_use;

+};

+struct intf_info {

• unsigned long data_xfer_ring_va;
• size_t data_xfer_ring_size;
• unsigned long sync_xfer_ring_va;
• size_t sync_xfer_ring_size;
• unsigned long xfer_buf_va;
• size_t xfer_buf_size;
• phys_addr_t xfer_buf_pa;
• unsigned int data_ep_pipe;
• unsigned int sync_ep_pipe;
• u8 *xfer_buf;
• u8 intf_num;
• u8 pcm_card_num;
• u8 pcm_dev_num;
• u8 direction;
• bool in_use;

+};

+struct uaudio_qmi_dev {

• struct device *dev;
• u32 sid;
• u32 intr_num;
• struct xhci_ring *sec_ring;
• struct iommu_domain *domain;
• /* list to keep track of available iova */
• struct list_head xfer_ring_list;
• size_t xfer_ring_iova_size;
• unsigned long curr_xfer_ring_iova;
• struct list_head xfer_buf_list;
• size_t xfer_buf_iova_size;
• unsigned long curr_xfer_buf_iova;
• /* bit fields representing pcm card enabled */
• unsigned long card_slot;
• /* indicate event ring mapped or not */
• bool er_mapped;
• /* reference count to number of possible consumers */
• atomic_t qdev_in_use;
• /* idx to last udev card number plugged in */
• unsigned int last_card_num;

+};

+struct uaudio_dev {

• struct usb_device *udev;
• /* audio control interface */
• struct usb_host_interface *ctrl_intf;
• unsigned int usb_core_id;
• atomic_t in_use;
• struct kref kref;
• wait_queue_head_t disconnect_wq;
• /* interface specific */
• int num_intf;
• struct intf_info *info;
• struct snd_usb_audio *chip;
• /* xhci sideband */
• struct xhci_sideband *sb;
• /* SoC USB device */
• struct snd_soc_usb_device *sdev;

+};

these structures feel like a set of kitchen sinks... Or a possible copy-paste, I don't know how one would add all these pointers on their own?

Do you really need all this? Is there not a way to use existing substructures?

+static int get_data_interval_from_si(struct snd_usb_substream *subs,

• u32 service_interval)

+{

• unsigned int bus_intval, bus_intval_mult, binterval;
• if (subs->dev->speed >= USB_SPEED_HIGH)

• bus_intval = BUS_INTERVAL_HIGHSPEED_AND_ABOVE;
  

• else

• bus_intval = BUS_INTERVAL_FULL_SPEED;
  

• if (service_interval % bus_intval)

• return -EINVAL;
  

• bus_intval_mult = service_interval / bus_intval;
• binterval = ffs(bus_intval_mult);
• if (!binterval || binterval > MAX_BINTERVAL_ISOC_EP)

• return -EINVAL;
  

• /* check if another bit is set then bail out */
• bus_intval_mult = bus_intval_mult >> binterval;
• if (bus_intval_mult)

• return -EINVAL;
  

• return (binterval - 1);

+}

This also feels like a generic helper. I don't see what's Qualcomm specific here?

+static unsigned long uaudio_iommu_map(enum mem_type mtype, bool dma_coherent,

• phys_addr_t pa, size_t size, struct sg_table *sgt)
  

+{

• unsigned long va_sg, va = 0;
• bool map = true;
• int i, ret;
• size_t sg_len, total_len = 0;
• struct scatterlist *sg;
• phys_addr_t pa_sg;
• int prot = IOMMU_READ | IOMMU_WRITE;

reverse x-mas tree style?

• if (dma_coherent)

• prot |= IOMMU_CACHE;
  

• switch (mtype) {
• case MEM_EVENT_RING:

• va = IOVA_BASE;
  

• /* er already mapped */
  

• if (uaudio_qdev->er_mapped)
  

• 	map = false;
  

• break;
  

• case MEM_XFER_RING:

• va = uaudio_get_iova(&uaudio_qdev->curr_xfer_ring_iova,
  

• &uaudio_qdev->xfer_ring_iova_size, &uaudio_qdev->xfer_ring_list,
  

• size);
  

• break;
  

• case MEM_XFER_BUF:

• va = uaudio_get_iova(&uaudio_qdev->curr_xfer_buf_iova,
  

• &uaudio_qdev->xfer_buf_iova_size, &uaudio_qdev->xfer_buf_list,
  

• size);
  

• break;
  

• default:

• dev_err(uaudio_qdev->dev, "unknown mem type %d\n", mtype);
  

• }
• if (!va || !map)

• goto done;
  

• if (!sgt)

• goto skip_sgt_map;
  

• va_sg = va;
• for_each_sg(sgt->sgl, sg, sgt->nents, i) {

• sg_len = PAGE_ALIGN(sg->offset + sg->length);
  

• pa_sg = page_to_phys(sg_page(sg));
  

• ret = iommu_map(uaudio_qdev->domain, va_sg, pa_sg, sg_len,
  

• 		prot, GFP_KERNEL);
  

• if (ret) {
  

• 	dev_err(uaudio_qdev->dev, "mapping failed ret%d\n", ret);
  

• 	dev_err(uaudio_qdev->dev,
  

• 		"type:%d, pa:%pa iova:0x%08lx sg_len:%zu\n",
  

• 		mtype, &pa_sg, va_sg, sg_len);
  

• 	uaudio_iommu_unmap(MEM_XFER_BUF, va, size, total_len);
  

• 	va = 0;
  

so it's an error but the function returns 0?

• 	goto done;
  

• }
  

• dev_dbg(uaudio_qdev->dev,
  

• 	"type:%d map pa:%pa to iova:0x%08lx len:%zu offset:%u\n",
  

• 	mtype, &pa_sg, va_sg, sg_len, sg->offset);
  

• va_sg += sg_len;
  

• total_len += sg_len;
  

• }
• if (size != total_len) {

• dev_err(uaudio_qdev->dev, "iova size %zu != mapped iova size %zu\n",
  

• 	size, total_len);
  

• uaudio_iommu_unmap(MEM_XFER_BUF, va, size, total_len);
  

• va = 0;
  

• }
• return va;

+skip_sgt_map:

• dev_dbg(uaudio_qdev->dev, "type:%d map pa:%pa to iova:0x%08lx size:%zu\n",

• mtype, &pa, va, size);
  

• ret = iommu_map(uaudio_qdev->domain, va, pa, size, prot, GFP_KERNEL);
• if (ret)

• dev_err(uaudio_qdev->dev,
  

• 	"failed to map pa:%pa iova:0x%lx type:%d ret:%d\n",
  

• 	&pa, va, mtype, ret);
  

+done:

• return va;

+}

+/* looks up alias, if any, for controller DT node and returns the index */ +static int usb_get_controller_id(struct usb_device *udev) +{

• if (udev->bus->sysdev && udev->bus->sysdev->of_node)

• return of_alias_get_id(udev->bus->sysdev->of_node, "usb");
  

• return -ENODEV;

+}

+/**

• • uaudio_dev_intf_cleanup() - cleanup transfer resources
• • @udev: usb device
• • @info: usb offloading interface
• • Cleans up the transfer ring related resources which are assigned per
• • endpoint from XHCI. This is invoked when the USB endpoints are no
• • longer in use by the adsp.
• */

+static void uaudio_dev_intf_cleanup(struct usb_device *udev,

• struct intf_info *info)

+{

• uaudio_iommu_unmap(MEM_XFER_RING, info->data_xfer_ring_va,

• info->data_xfer_ring_size, info->data_xfer_ring_size);
  

• info->data_xfer_ring_va = 0;
• info->data_xfer_ring_size = 0;
• uaudio_iommu_unmap(MEM_XFER_RING, info->sync_xfer_ring_va,

• info->sync_xfer_ring_size, info->sync_xfer_ring_size);
  

• info->sync_xfer_ring_va = 0;
• info->sync_xfer_ring_size = 0;
• uaudio_iommu_unmap(MEM_XFER_BUF, info->xfer_buf_va,

• info->xfer_buf_size, info->xfer_buf_size);
  

• info->xfer_buf_va = 0;
• usb_free_coherent(udev, info->xfer_buf_size,

• info->xfer_buf, info->xfer_buf_pa);
  

• info->xfer_buf_size = 0;
• info->xfer_buf = NULL;
• info->xfer_buf_pa = 0;
• info->in_use = false;

+}

+/**

• • uaudio_event_ring_cleanup_free() - cleanup secondary event ring
• • @dev: usb offload device
• • Cleans up the secondary event ring that was requested. This will
• • occur when the adsp is no longer transferring data on the USB bus
• • across all endpoints.
• */

+static void uaudio_event_ring_cleanup_free(struct uaudio_dev *dev) +{

• clear_bit(dev->chip->card->number, &uaudio_qdev->card_slot);
• /* all audio devices are disconnected */
• if (!uaudio_qdev->card_slot) {

• uaudio_iommu_unmap(MEM_EVENT_RING, IOVA_BASE, PAGE_SIZE,
  

• 	PAGE_SIZE);
  

• xhci_sideband_remove_interrupter(uadev[dev->chip->card->number].sb);
  

• }

+}

+static void uaudio_dev_cleanup(struct uaudio_dev *dev)

there should be a comment that this assumes a mutex is locked in the caller.

+{

• int if_idx;
• if (!dev->udev)

• return;
  

• /* free xfer buffer and unmap xfer ring and buf per interface */
• for (if_idx = 0; if_idx < dev->num_intf; if_idx++) {

• if (!dev->info[if_idx].in_use)
  

• 	continue;
  

• uaudio_dev_intf_cleanup(dev->udev, &dev->info[if_idx]);
  

• dev_dbg(uaudio_qdev->dev, "release resources: intf# %d card# %d\n",
  

• 	dev->info[if_idx].intf_num, dev->chip->card->number);
  

• }
• dev->num_intf = 0;
• /* free interface info */
• kfree(dev->info);
• dev->info = NULL;
• uaudio_event_ring_cleanup_free(dev);
• dev->udev = NULL;

+}

+/**

• • disable_audio_stream() - disable usb snd endpoints
• • @subs: usb substream
• • Closes the USB SND endpoints associated with the current audio stream
• • used. This will decrement the USB SND endpoint opened reference count.
• */

+static void disable_audio_stream(struct snd_usb_substream *subs) +{

• struct snd_usb_audio *chip = subs->stream->chip;
• snd_usb_hw_free(subs);
• snd_usb_autosuspend(chip);

+}

+/* QMI service disconnect handlers */ +static void qmi_disconnect_work(struct work_struct *w) +{

• struct intf_info *info;
• int idx, if_idx;
• struct snd_usb_substream *subs;
• struct snd_usb_audio *chip;
• mutex_lock(&qdev_mutex);
• /* find all active intf for set alt 0 and cleanup usb audio dev */
• for (idx = 0; idx < SNDRV_CARDS; idx++) {

• if (!atomic_read(&uadev[idx].in_use))
  

• 	continue;
  

• chip = uadev[idx].chip;
  

• for (if_idx = 0; if_idx < uadev[idx].num_intf; if_idx++) {
  

• 	if (!uadev[idx].info || !uadev[idx].info[if_idx].in_use)
  

• 		continue;
  

• 	info = &uadev[idx].info[if_idx];
  

• 	subs = find_substream(info->pcm_card_num,
  

• 				info->pcm_dev_num,
  

• 				info->direction);
  

• 	if (!subs || !chip || atomic_read(&chip->shutdown)) {
  

• 		dev_err(&subs->dev->dev,
  

• 			"no sub for c#%u dev#%u dir%u\n",
  

• 			info->pcm_card_num,
  

• 			info->pcm_dev_num,
  

• 			info->direction);
  

• 		continue;
  

• 	}
  

• 	disable_audio_stream(subs);
  

• }
  

• atomic_set(&uadev[idx].in_use, 0);
  

• mutex_lock(&chip->mutex);
  

• uaudio_dev_cleanup(&uadev[idx]);
  

• mutex_unlock(&chip->mutex);
  

• }
• mutex_unlock(&qdev_mutex);

+}

+/**

• • qmi_bye_cb() - qmi bye message callback
• • @handle: QMI handle
• • @node: id of the dying node
• • This callback is invoked when the QMI bye control message is received
• • from the QMI client. Handle the message accordingly by ensuring that
• • the USB offload path is disabled and cleaned up. At this point, ADSP
• • is not utilizing the USB bus.
• */

+static void qmi_bye_cb(struct qmi_handle *handle, unsigned int node) +{

• struct uaudio_qmi_svc *svc = uaudio_svc;
• if (svc->uaudio_svc_hdl != handle)

• return;
  

• if (svc->client_connected && svc->client_sq.sq_node == node) {

• queue_work(svc->uaudio_wq, &svc->qmi_disconnect_work);
  

• svc->client_sq.sq_node = 0;
  

• svc->client_sq.sq_port = 0;
  

• svc->client_sq.sq_family = 0;
  

• svc->client_connected = false;
  

• }

+}

+/**

• • qmi_svc_disconnect_cb() - qmi client disconnected
• • @handle: QMI handle
• • @node: id of the dying node
• • @port: port of the dying client
• • Invoked when the remote QMI client is disconnected. Handle this event
• • the same way as when the QMI bye message is received. This will ensure
• • the USB offloading path is disabled and cleaned up.
• */

+static void qmi_svc_disconnect_cb(struct qmi_handle *handle,

• 		  unsigned int node, unsigned int port)
  

+{

• struct uaudio_qmi_svc *svc;
• if (uaudio_svc == NULL)

• return;
  

• svc = uaudio_svc;
• if (svc->uaudio_svc_hdl != handle)

• return;
  

• if (svc->client_connected && svc->client_sq.sq_node == node &&

• 	svc->client_sq.sq_port == port) {
  

• queue_work(svc->uaudio_wq, &svc->qmi_disconnect_work);
  

• svc->client_sq.sq_node = 0;
  

• svc->client_sq.sq_port = 0;
  

• svc->client_sq.sq_family = 0;
  

• svc->client_connected = false;
  

this feels racy, shouldn't all these reset values be set in the work function?

• }

+}

+/* QMI client callback handlers from QMI interface */ +static struct qmi_ops uaudio_svc_ops_options = {

• .bye = qmi_bye_cb,
• .del_client = qmi_svc_disconnect_cb,

+};

+/* kref release callback when all streams are disabled */ +static void uaudio_dev_release(struct kref *kref) +{

• struct uaudio_dev *dev = container_of(kref, struct uaudio_dev, kref);
• uaudio_event_ring_cleanup_free(dev);
• atomic_set(&dev->in_use, 0);
• wake_up(&dev->disconnect_wq);

+}

+/**

• • enable_audio_stream() - enable usb snd endpoints
• • @subs: usb substream
• • @pcm_format: pcm format requested
• • @channels: number of channels
• • @cur_rate: sample rate
• • @datainterval: interval
• • Opens all USB SND endpoints used for the data interface. This will increment
• • the USB SND endpoint's opened count. Requests to keep the interface resumed
• • until the audio stream is stopped. Will issue the USB set interface control
• • message to enable the data interface.
• */

+static int enable_audio_stream(struct snd_usb_substream *subs,

• 		snd_pcm_format_t pcm_format,
  

• 		unsigned int channels, unsigned int cur_rate,
  

• 		int datainterval)
  

+{

• struct snd_usb_audio *chip = subs->stream->chip;
• struct snd_pcm_hw_params params;
• struct snd_mask *m;
• struct snd_interval *i;
• int ret;
• _snd_pcm_hw_params_any(&params);
• m = hw_param_mask(&params, SNDRV_PCM_HW_PARAM_FORMAT);
• snd_mask_leave(m, pcm_format);
• i = hw_param_interval(&params, SNDRV_PCM_HW_PARAM_CHANNELS);
• snd_interval_setinteger(i);
• i->min = i->max = channels;
• i = hw_param_interval(&params, SNDRV_PCM_HW_PARAM_RATE);
• snd_interval_setinteger(i);
• i->min = i->max = cur_rate;
• pm_runtime_barrier(&chip->intf[0]->dev);
• snd_usb_autoresume(chip);
• ret = snd_usb_hw_params(subs, &params);
• if (ret < 0)

• goto put_suspend;
  

• if (!atomic_read(&chip->shutdown)) {

• ret = snd_usb_lock_shutdown(chip);
  

• if (ret < 0)
  

• 	goto detach_ep;
  

• if (subs->sync_endpoint) {
  

• 	ret = snd_usb_endpoint_prepare(chip, subs->sync_endpoint);
  

• 	if (ret < 0)
  

• 		goto unlock;
  

• }
  

• ret = snd_usb_endpoint_prepare(chip, subs->data_endpoint);
  

• if (ret < 0)
  

• 	goto unlock;
  

• snd_usb_unlock_shutdown(chip);
  

• dev_dbg(uaudio_qdev->dev,
  

• 	"selected %s iface:%d altsetting:%d datainterval:%dus\n",
  

• 	subs->direction ? "capture" : "playback",
  

• 	subs->cur_audiofmt->iface, subs->cur_audiofmt->altsetting,
  

• 	(1 << subs->cur_audiofmt->datainterval) *
  

• 	(subs->dev->speed >= USB_SPEED_HIGH ?
  

• 	BUS_INTERVAL_HIGHSPEED_AND_ABOVE :
  

• 	BUS_INTERVAL_FULL_SPEED));
  

• }
• return 0;

+unlock:

• snd_usb_unlock_shutdown(chip);

+detach_ep:

• snd_usb_hw_free(subs);

+put_suspend:

• snd_usb_autosuspend(chip);
• return ret;

+}

+/* returns usb hcd sysdev */ +static struct device *usb_get_usb_backend(struct usb_device *udev) +{

• if (udev->bus->sysdev && udev->bus->sysdev->of_node)

• return udev->bus->sysdev;
  

• return NULL;

+}

+/**

• • prepare_qmi_response() - prepare stream enable response
• • @subs: usb substream
• • @req_msg: QMI request message
• • @resp: QMI response buffer
• • @info_idx: usb interface array index
• • Prepares the QMI response for a USB QMI stream enable request. Will parse
• • out the parameters within the stream enable request, in order to match
• • requested audio profile to the ones exposed by the USB device connected.
• • In addition, will fetch the XHCI transfer resources needed for the handoff to
• • happen. This includes, transfer ring and buffer addresses and secondary event
• • ring address. These parameters will be communicated as part of the USB QMI
• • stream enable response.
• */

+static int prepare_qmi_response(struct snd_usb_substream *subs,

• struct qmi_uaudio_stream_req_msg_v01 *req_msg,
  

• struct qmi_uaudio_stream_resp_msg_v01 *resp, int info_idx)
  

+{

• struct usb_interface *iface;
• struct usb_host_interface *alts;
• struct usb_interface_descriptor *altsd;
• struct usb_interface_assoc_descriptor *assoc;
• struct usb_host_endpoint *ep;
• struct uac_format_type_i_continuous_descriptor *fmt;
• struct uac_format_type_i_discrete_descriptor *fmt_v1;
• struct uac_format_type_i_ext_descriptor *fmt_v2;
• struct uac1_as_header_descriptor *as;
• struct q6usb_offload *data;
• int ret;
• int protocol, card_num, pcm_dev_num;
• void *hdr_ptr;
• u8 *xfer_buf;
• unsigned int data_ep_pipe = 0, sync_ep_pipe = 0;
• u32 len, mult, remainder, xfer_buf_len;
• unsigned long va, tr_data_va = 0, tr_sync_va = 0;
• phys_addr_t xhci_pa, xfer_buf_pa, tr_data_pa = 0, tr_sync_pa = 0;
• struct sg_table *sgt;
• struct sg_table xfer_buf_sgt;
• struct page *pg;
• bool dma_coherent;

consider simplifying or splitting in different functions? you have 20 lines and probably 30-odd variables. This is a bit beyond what reviewers can handle...

• iface = usb_ifnum_to_if(subs->dev, subs->cur_audiofmt->iface);
• if (!iface) {

• dev_err(uaudio_qdev->dev, "interface # %d does not exist\n",
  

• 	subs->cur_audiofmt->iface);
  

• ret = -ENODEV;
  

• goto err;
  

• }
• assoc = iface->intf_assoc;
• pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8;
• xfer_buf_len = req_msg->xfer_buff_size;
• card_num = uaudio_qdev->last_card_num;
• alts = &iface->altsetting[subs->cur_audiofmt->altset_idx];
• altsd = get_iface_desc(alts);
• protocol = altsd->bInterfaceProtocol;
• /* get format type */
• if (protocol != UAC_VERSION_3) {

• fmt = snd_usb_find_csint_desc(alts->extra, alts->extralen, NULL,
  

• 		UAC_FORMAT_TYPE);
  

• if (!fmt) {
  

• 	dev_err(uaudio_qdev->dev,
  

• 		"%u:%d : no UAC_FORMAT_TYPE desc\n",
  

• 		subs->cur_audiofmt->iface,
  

• 		subs->cur_audiofmt->altset_idx);
  

• 	ret = -ENODEV;
  

• 	goto err;
  

• }
  

• }
• if (!uadev[card_num].ctrl_intf) {

• dev_err(uaudio_qdev->dev, "audio ctrl intf info not cached\n");
  

• ret = -ENODEV;
  

• goto err;
  

• }
• if (protocol != UAC_VERSION_3) {

• hdr_ptr = snd_usb_find_csint_desc(uadev[card_num].ctrl_intf->extra,
  

• 		uadev[card_num].ctrl_intf->extralen, NULL,
  

• 		UAC_HEADER);
  

• if (!hdr_ptr) {
  

• 	dev_err(uaudio_qdev->dev, "no UAC_HEADER desc\n");
  

• 	ret = -ENODEV;
  

• 	goto err;
  

• }
  

• }
• if (protocol == UAC_VERSION_1) {

• struct uac1_ac_header_descriptor *uac1_hdr = hdr_ptr;
  

• as = snd_usb_find_csint_desc(alts->extra, alts->extralen, NULL,
  

• 	UAC_AS_GENERAL);
  

• if (!as) {
  

• 	dev_err(uaudio_qdev->dev,
  

• 		"%u:%d : no UAC_AS_GENERAL desc\n",
  

• 		subs->cur_audiofmt->iface,
  

• 		subs->cur_audiofmt->altset_idx);
  

• 	ret = -ENODEV;
  

• 	goto err;
  

• }
  

• resp->data_path_delay = as->bDelay;
  

• resp->data_path_delay_valid = 1;
  

• fmt_v1 = (struct uac_format_type_i_discrete_descriptor *)fmt;
  

• resp->usb_audio_subslot_size = fmt_v1->bSubframeSize;
  

• resp->usb_audio_subslot_size_valid = 1;
  

• resp->usb_audio_spec_revision = le16_to_cpu(uac1_hdr->bcdADC);
  

• resp->usb_audio_spec_revision_valid = 1;
  

• } else if (protocol == UAC_VERSION_2) {

• struct uac2_ac_header_descriptor *uac2_hdr = hdr_ptr;
  

• fmt_v2 = (struct uac_format_type_i_ext_descriptor *)fmt;
  

• resp->usb_audio_subslot_size = fmt_v2->bSubslotSize;
  

• resp->usb_audio_subslot_size_valid = 1;
  

• resp->usb_audio_spec_revision = le16_to_cpu(uac2_hdr->bcdADC);
  

• resp->usb_audio_spec_revision_valid = 1;
  

• } else if (protocol == UAC_VERSION_3) {

• if (assoc->bFunctionSubClass ==
  

• 			UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0) {
  

• 	dev_err(uaudio_qdev->dev, "full adc is not supported\n");
  

• 	ret = -EINVAL;
  

• }
  

• switch (le16_to_cpu(get_endpoint(alts, 0)->wMaxPacketSize)) {
  

• case UAC3_BADD_EP_MAXPSIZE_SYNC_MONO_16:
  

• case UAC3_BADD_EP_MAXPSIZE_SYNC_STEREO_16:
  

• case UAC3_BADD_EP_MAXPSIZE_ASYNC_MONO_16:
  

• case UAC3_BADD_EP_MAXPSIZE_ASYNC_STEREO_16: {
  

• 	resp->usb_audio_subslot_size = 0x2;
  

• 	break;
  

• }
  

• case UAC3_BADD_EP_MAXPSIZE_SYNC_MONO_24:
  

• case UAC3_BADD_EP_MAXPSIZE_SYNC_STEREO_24:
  

• case UAC3_BADD_EP_MAXPSIZE_ASYNC_MONO_24:
  

• case UAC3_BADD_EP_MAXPSIZE_ASYNC_STEREO_24: {
  

• 	resp->usb_audio_subslot_size = 0x3;
  

• 	break;
  

• }
  

• default:
  

• 	dev_err(uaudio_qdev->dev,
  

• 		"%d: %u: Invalid wMaxPacketSize\n",
  

• 		subs->cur_audiofmt->iface,
  

• 		subs->cur_audiofmt->altset_idx);
  

• 	ret = -EINVAL;
  

• 	goto err;
  

• }
  

• resp->usb_audio_subslot_size_valid = 1;
  

• } else {

• dev_err(uaudio_qdev->dev, "unknown protocol version %x\n",
  

• 	protocol);
  

• ret = -ENODEV;
  

• goto err;
  

• }

these 100-odd lines look like duplicated code. Why would we redo the parsing of UAC3 stuff in a QCOM-specific driver?

• resp->slot_id = subs->dev->slot_id;
• resp->slot_id_valid = 1;
• memcpy(&resp->std_as_opr_intf_desc, &alts->desc, sizeof(alts->desc));
• resp->std_as_opr_intf_desc_valid = 1;
• ep = usb_pipe_endpoint(subs->dev, subs->data_endpoint->pipe);
• if (!ep) {

• dev_err(uaudio_qdev->dev, "data ep # %d context is null\n",
  

• 	subs->data_endpoint->ep_num);
  

• ret = -ENODEV;
  

• goto err;
  

• }
• data_ep_pipe = subs->data_endpoint->pipe;
• memcpy(&resp->std_as_data_ep_desc, &ep->desc, sizeof(ep->desc));
• resp->std_as_data_ep_desc_valid = 1;
• ret = xhci_sideband_add_endpoint(uadev[card_num].sb, ep);
• if (ret < 0) {

• dev_err(uaudio_qdev->dev, "failed to add data ep to sideband\n");
  

• ret = -ENODEV;
  

• goto err;
  

• }
• sgt = xhci_sideband_get_endpoint_buffer(uadev[card_num].sb, ep);
• if (!sgt) {

• dev_err(uaudio_qdev->dev, "failed to get data ep ring address\n");
  

• ret = -ENODEV;
  

• goto drop_data_ep;
  

• }
• pg = sg_page(sgt->sgl);
• tr_data_pa = page_to_phys(pg);
• resp->xhci_mem_info.tr_data.pa = sg_dma_address(sgt->sgl);
• sg_free_table(sgt);
• if (subs->sync_endpoint) {

• ep = usb_pipe_endpoint(subs->dev, subs->sync_endpoint->pipe);
  

• if (!ep) {
  

• 	dev_err(uaudio_qdev->dev, "implicit fb on data ep\n");
  

• 	goto skip_sync_ep;
  

• }
  

• sync_ep_pipe = subs->sync_endpoint->pipe;
  

• memcpy(&resp->std_as_sync_ep_desc, &ep->desc, sizeof(ep->desc));
  

• resp->std_as_sync_ep_desc_valid = 1;
  

• ret = xhci_sideband_add_endpoint(uadev[card_num].sb, ep);
  

• if (ret < 0) {
  

• 	dev_err(uaudio_qdev->dev,
  

• 		"failed to add sync ep to sideband\n");
  

• 	ret = -ENODEV;
  

• 	goto drop_data_ep;
  

• }
  

• sgt = xhci_sideband_get_endpoint_buffer(uadev[card_num].sb, ep);
  

• if (!sgt) {
  

• 	dev_err(uaudio_qdev->dev, "failed to get sync ep ring address\n");
  

• 	ret = -ENODEV;
  

• 	goto drop_sync_ep;
  

• }
  

• pg = sg_page(sgt->sgl);
  

• tr_sync_pa = page_to_phys(pg);
  

• resp->xhci_mem_info.tr_sync.pa = sg_dma_address(sgt->sgl);
  

• sg_free_table(sgt);
  

• }

+skip_sync_ep:

• data = snd_soc_usb_find_priv_data(usb_get_usb_backend(subs->dev));
• if (!data)

• goto drop_sync_ep;
  

• uaudio_qdev->domain = data->domain;
• uaudio_qdev->sid = data->sid;
• uaudio_qdev->intr_num = data->intr_num;
• uaudio_qdev->dev = data->dev;
• resp->interrupter_num_valid = 1;
• resp->controller_num_valid = 0;
• ret = usb_get_controller_id(subs->dev);
• if (ret >= 0) {

• resp->controller_num = ret;
  

• resp->controller_num_valid = 1;
  

• }
• /* map xhci data structures PA memory to iova */
• dma_coherent = dev_is_dma_coherent(subs->dev->bus->sysdev);
• /* event ring */
• ret = xhci_sideband_create_interrupter(uadev[card_num].sb, uaudio_qdev->intr_num);
• if (ret < 0) {

• dev_err(uaudio_qdev->dev, "failed to fetch interrupter\n");
  

• ret = -ENODEV;
  

• goto drop_sync_ep;
  

• }
• sgt = xhci_sideband_get_event_buffer(uadev[card_num].sb);
• if (!sgt) {

• dev_err(uaudio_qdev->dev, "failed to get event ring address\n");
  

• ret = -ENODEV;
  

• goto free_sec_ring;
  

• }
• xhci_pa = page_to_phys(sg_page(sgt->sgl));
• resp->xhci_mem_info.evt_ring.pa = sg_dma_address(sgt->sgl);
• sg_free_table(sgt);
• if (!xhci_pa) {

• dev_err(uaudio_qdev->dev,
  

• 	"failed to get sec event ring address\n");
  

• ret = -ENODEV;
  

• goto free_sec_ring;
  

• }
• resp->interrupter_num = xhci_sideband_interrupter_id(uadev[card_num].sb);
• va = uaudio_iommu_map(MEM_EVENT_RING, dma_coherent, xhci_pa, PAGE_SIZE,

• 	NULL);
  

• if (!va) {

• ret = -ENOMEM;
  

• goto free_sec_ring;
  

• }
• resp->xhci_mem_info.evt_ring.va = PREPEND_SID_TO_IOVA(va,

• 				uaudio_qdev->sid);
  

• resp->xhci_mem_info.evt_ring.size = PAGE_SIZE;
• uaudio_qdev->er_mapped = true;
• resp->speed_info = get_speed_info(subs->dev->speed);
• if (resp->speed_info == USB_QMI_DEVICE_SPEED_INVALID_V01) {

• ret = -ENODEV;
  

• goto unmap_er;
  

• }
• resp->speed_info_valid = 1;
• /* data transfer ring */
• va = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_data_pa,

• 	PAGE_SIZE, NULL);
  

• if (!va) {

• ret = -ENOMEM;
  

• goto unmap_er;
  

• }
• tr_data_va = va;
• resp->xhci_mem_info.tr_data.va = PREPEND_SID_TO_IOVA(va,

• 				uaudio_qdev->sid);
  

• resp->xhci_mem_info.tr_data.size = PAGE_SIZE;
• /* sync transfer ring */
• if (!resp->xhci_mem_info.tr_sync.pa)

• goto skip_sync;
  

• xhci_pa = resp->xhci_mem_info.tr_sync.pa;
• va = uaudio_iommu_map(MEM_XFER_RING, dma_coherent, tr_sync_pa,

• 	PAGE_SIZE, NULL);
  

• if (!va) {

• ret = -ENOMEM;
  

• goto unmap_data;
  

• }
• tr_sync_va = va;
• resp->xhci_mem_info.tr_sync.va = PREPEND_SID_TO_IOVA(va,

• 				uaudio_qdev->sid);
  

• resp->xhci_mem_info.tr_sync.size = PAGE_SIZE;

+skip_sync:

• /* xfer buffer, multiple of 4K only */
• if (!xfer_buf_len)

• xfer_buf_len = PAGE_SIZE;
  

• mult = xfer_buf_len / PAGE_SIZE;
• remainder = xfer_buf_len % PAGE_SIZE;
• len = mult * PAGE_SIZE;
• len += remainder ? PAGE_SIZE : 0;
• if (len > MAX_XFER_BUFF_LEN) {

• dev_err(uaudio_qdev->dev,
  

• 	"req buf len %d > max buf len %lu, setting %lu\n",
  

• 	len, MAX_XFER_BUFF_LEN, MAX_XFER_BUFF_LEN);
  

• len = MAX_XFER_BUFF_LEN;
  

• }
• xfer_buf = usb_alloc_coherent(subs->dev, len, GFP_KERNEL, &xfer_buf_pa);
• if (!xfer_buf) {

• ret = -ENOMEM;
  

• goto unmap_sync;
  

• }
• dma_get_sgtable(subs->dev->bus->sysdev, &xfer_buf_sgt, xfer_buf, xfer_buf_pa,

• 	len);
  

• va = uaudio_iommu_map(MEM_XFER_BUF, dma_coherent, xfer_buf_pa, len,

• 	&xfer_buf_sgt);
  

• if (!va) {

• ret = -ENOMEM;
  

• goto unmap_sync;
  

• }
• resp->xhci_mem_info.xfer_buff.pa = xfer_buf_pa;
• resp->xhci_mem_info.xfer_buff.size = len;
• resp->xhci_mem_info.xfer_buff.va = PREPEND_SID_TO_IOVA(va,

• 				uaudio_qdev->sid);
  

• resp->xhci_mem_info_valid = 1;
• sg_free_table(&xfer_buf_sgt);
• if (!atomic_read(&uadev[card_num].in_use)) {

• kref_init(&uadev[card_num].kref);
  

• init_waitqueue_head(&uadev[card_num].disconnect_wq);
  

• uadev[card_num].num_intf =
  

• 	subs->dev->config->desc.bNumInterfaces;
  

• uadev[card_num].info = kcalloc(uadev[card_num].num_intf,
  

• 	sizeof(struct intf_info), GFP_KERNEL);
  

• if (!uadev[card_num].info) {
  

• 	ret = -ENOMEM;
  

• 	goto unmap_sync;
  

• }
  

• uadev[card_num].udev = subs->dev;
  

• atomic_set(&uadev[card_num].in_use, 1);
  

• } else {

• kref_get(&uadev[card_num].kref);
  

• }
• uadev[card_num].usb_core_id = resp->controller_num;
• /* cache intf specific info to use it for unmap and free xfer buf */
• uadev[card_num].info[info_idx].data_xfer_ring_va = tr_data_va;
• uadev[card_num].info[info_idx].data_xfer_ring_size = PAGE_SIZE;
• uadev[card_num].info[info_idx].sync_xfer_ring_va = tr_sync_va;
• uadev[card_num].info[info_idx].sync_xfer_ring_size = PAGE_SIZE;
• uadev[card_num].info[info_idx].xfer_buf_va = va;
• uadev[card_num].info[info_idx].xfer_buf_pa = xfer_buf_pa;
• uadev[card_num].info[info_idx].xfer_buf_size = len;
• uadev[card_num].info[info_idx].data_ep_pipe = data_ep_pipe;
• uadev[card_num].info[info_idx].sync_ep_pipe = sync_ep_pipe;
• uadev[card_num].info[info_idx].xfer_buf = xfer_buf;
• uadev[card_num].info[info_idx].pcm_card_num = card_num;
• uadev[card_num].info[info_idx].pcm_dev_num = pcm_dev_num;
• uadev[card_num].info[info_idx].direction = subs->direction;
• uadev[card_num].info[info_idx].intf_num = subs->cur_audiofmt->iface;
• uadev[card_num].info[info_idx].in_use = true;
• set_bit(card_num, &uaudio_qdev->card_slot);
• return 0;

+unmap_sync:

• usb_free_coherent(subs->dev, len, xfer_buf, xfer_buf_pa);
• uaudio_iommu_unmap(MEM_XFER_RING, tr_sync_va, PAGE_SIZE, PAGE_SIZE);

+unmap_data:

• uaudio_iommu_unmap(MEM_XFER_RING, tr_data_va, PAGE_SIZE, PAGE_SIZE);

+unmap_er:

• uaudio_iommu_unmap(MEM_EVENT_RING, IOVA_BASE, PAGE_SIZE, PAGE_SIZE);

+free_sec_ring:

• xhci_sideband_remove_interrupter(uadev[card_num].sb);

+drop_sync_ep:

• if (subs->sync_endpoint)

• xhci_sideband_remove_endpoint(uadev[card_num].sb,
  

• 	usb_pipe_endpoint(subs->dev, subs->sync_endpoint->pipe));
  

+drop_data_ep:

• xhci_sideband_remove_endpoint(uadev[card_num].sb,

• 	usb_pipe_endpoint(subs->dev, subs->data_endpoint->pipe));
  

+err:

• return ret;

+}

this is really the largest function I've seen in a while... Can this use helpers or be more modular?

+/**

• • handle_uaudio_stream_req() - handle stream enable/disable request
• • @handle: QMI client handle
• • @sq: qrtr socket
• • @txn: QMI transaction context
• • @decoded_msg: decoded QMI message
• • Main handler for the QMI stream enable/disable requests. This executes the
• • corresponding enable/disable stream apis, respectively.
• */

+static void handle_uaudio_stream_req(struct qmi_handle *handle,

• 	struct sockaddr_qrtr *sq,
  

• 	struct qmi_txn *txn,
  

• 	const void *decoded_msg)
  

+{

• struct qmi_uaudio_stream_req_msg_v01 *req_msg;
• struct qmi_uaudio_stream_resp_msg_v01 resp = {{0}, 0};
• struct snd_usb_substream *subs;
• struct snd_usb_audio *chip = NULL;
• struct uaudio_qmi_svc *svc = uaudio_svc;
• struct intf_info *info;
• struct usb_host_endpoint *ep;
• u8 pcm_card_num, pcm_dev_num, direction;
• int info_idx = -EINVAL, datainterval = -EINVAL, ret = 0;
• if (!svc->client_connected) {

• svc->client_sq = *sq;
  

• svc->client_connected = true;
  

• }
• mutex_lock(&qdev_mutex);
• req_msg = (struct qmi_uaudio_stream_req_msg_v01 *)decoded_msg;
• if (!req_msg->audio_format_valid || !req_msg->bit_rate_valid ||

•    !req_msg->number_of_ch_valid || !req_msg->xfer_buff_size_valid) {
  

• ret = -EINVAL;
  

this looks like copy pasted code, this function return void so all uses of 'ret' are not so useful, are they?

• goto response;
  

• }
• if (!uaudio_qdev) {

• ret = -EINVAL;
  

• goto response;
  

• }
• direction = (req_msg->usb_token & QMI_STREAM_REQ_DIRECTION);
• pcm_dev_num = (req_msg->usb_token & QMI_STREAM_REQ_DEV_NUM_MASK) >> 8;
• pcm_card_num = req_msg->enable ? uaudio_qdev->last_card_num :

• 		ffs(uaudio_qdev->card_slot) - 1;
  

• if (pcm_card_num >= SNDRV_CARDS) {

• ret = -EINVAL;
  

• goto response;
  

• }
• if (req_msg->audio_format > USB_QMI_PCM_FORMAT_U32_BE) {

• ret = -EINVAL;
  

• goto response;
  

• }
• subs = find_substream(pcm_card_num, pcm_dev_num, direction);
• chip = uadev[pcm_card_num].chip;
• if (!subs || !chip || atomic_read(&chip->shutdown)) {

• ret = -ENODEV;
  

• goto response;
  

• }
• info_idx = info_idx_from_ifnum(pcm_card_num, subs->cur_audiofmt ?

• 	subs->cur_audiofmt->iface : -1, req_msg->enable);
  

• if (atomic_read(&chip->shutdown) || !subs->stream || !subs->stream->pcm

• 	|| !subs->stream->chip) {
  

• ret = -ENODEV;
  

• goto response;
  

• }
• if (req_msg->enable) {

• if (info_idx < 0 || chip->system_suspend) {
  

• 	ret = -EBUSY;
  

• 	goto response;
  

• }
  

• }
• if (req_msg->service_interval_valid) {

• ret = get_data_interval_from_si(subs,
  

• 				req_msg->service_interval);
  

• if (ret == -EINVAL)
  

• 	goto response;
  

• datainterval = ret;
  

• }
• uadev[pcm_card_num].ctrl_intf = chip->ctrl_intf;
• if (req_msg->enable) {

• ret = enable_audio_stream(subs,
  

• 		map_pcm_format(req_msg->audio_format),
  

• 		req_msg->number_of_ch, req_msg->bit_rate,
  

• 		datainterval);
  

• if (!ret)
  

• 	ret = prepare_qmi_response(subs, req_msg, &resp,
  

• 			info_idx);
  

• } else {

• info = &uadev[pcm_card_num].info[info_idx];
  

• if (info->data_ep_pipe) {
  

• 	ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
  

• 				info->data_ep_pipe);
  

• 	if (ep)
  

• 		xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
  

• 				ep);
  

• 	xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb, ep);
  

• 	info->data_ep_pipe = 0;
  

• }
  

• if (info->sync_ep_pipe) {
  

• 	ep = usb_pipe_endpoint(uadev[pcm_card_num].udev,
  

• 				info->sync_ep_pipe);
  

• 	if (ep)
  

• 		xhci_sideband_stop_endpoint(uadev[pcm_card_num].sb,
  

• 				ep);
  

• 	xhci_sideband_remove_endpoint(uadev[pcm_card_num].sb, ep);
  

• 	info->sync_ep_pipe = 0;
  

• }
  

• disable_audio_stream(subs);
  

• }

+response:

• if (!req_msg->enable && ret != -EINVAL && ret != -ENODEV) {

• mutex_lock(&chip->mutex);
  

• if (info_idx >= 0) {
  

• 	info = &uadev[pcm_card_num].info[info_idx];
  

• 	uaudio_dev_intf_cleanup(
  

• 			uadev[pcm_card_num].udev,
  

• 			info);
  

• }
  

• if (atomic_read(&uadev[pcm_card_num].in_use))
  

• 	kref_put(&uadev[pcm_card_num].kref,
  

• 			uaudio_dev_release);
  

• mutex_unlock(&chip->mutex);
  

• }
• mutex_unlock(&qdev_mutex);
• resp.usb_token = req_msg->usb_token;
• resp.usb_token_valid = 1;
• resp.internal_status = ret;
• resp.internal_status_valid = 1;
• resp.status = ret ? USB_QMI_STREAM_REQ_FAILURE_V01 : ret;
• resp.status_valid = 1;
• ret = qmi_send_response(svc->uaudio_svc_hdl, sq, txn,

• 	QMI_UAUDIO_STREAM_RESP_V01,
  

• 	QMI_UAUDIO_STREAM_RESP_MSG_V01_MAX_MSG_LEN,
  

• 	qmi_uaudio_stream_resp_msg_v01_ei, &resp);
  

ret is not used?

+}

I stopped here...