| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <32b77d2d-340e-49c9-986d-9f9f37e43cda@linux.intel.com>
Date: Tue, 17 Oct 2023 18:21:54 -0500
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Wesley Cheng <quic_wcheng@...cinc.com>, mathias.nyman@...el.com,
gregkh@...uxfoundation.org, lgirdwood@...il.com,
broonie@...nel.org, perex@...ex.cz, tiwai@...e.com,
agross@...nel.org, andersson@...nel.org, konrad.dybcio@...aro.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
conor+dt@...nel.org, srinivas.kandagatla@...aro.org,
bgoswami@...cinc.com, Thinh.Nguyen@...opsys.com
Cc: linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
alsa-devel@...a-project.org, linux-arm-msm@...r.kernel.org,
devicetree@...r.kernel.org
Subject: 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(¶ms);
> +
> + m = hw_param_mask(¶ms, SNDRV_PCM_HW_PARAM_FORMAT);
> + snd_mask_leave(m, pcm_format);
> +
> + i = hw_param_interval(¶ms, SNDRV_PCM_HW_PARAM_CHANNELS);
> + snd_interval_setinteger(i);
> + i->min = i->max = channels;
> +
> + i = hw_param_interval(¶ms, 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, ¶ms);
> + 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...
Powered by blists - more mailing lists