[alsa-devel] [Linux-kernel] [PATCH 1/6] ALSA: usb: ADC3: Add initial BADD spec support
Ben Hutchings
ben.hutchings at codethink.co.uk
Wed Dec 13 23:48:53 CET 2017
On Wed, 2017-11-29 at 10:55 +0000, Jorge Sanjuan wrote:
> The Basic Audio Device 3 (BADD) spec requires the host to
> have "inferred" descriptors when a BADD profile ID is specified.
> This descriptor generator creates a buffer with known descriptors
> for each profile that can be then read to create the alsa audio
> device(s). The UAC3 compliant devices should have one configuration
> that is BADD compliant.
>
> The USB request from host are bypassed and these buffers are read
> instead.
>
> This patch series covers (for now) the following topologies:
>
> - HEADPHONE.
> - HEADSET.
>
> For more information refer to the spec at:
>
> http://www.usb.org/developers/docs/devclass_docs/
>
> Signed-off-by: Jorge Sanjuan <jorge.sanjuan at codethink.co.uk>
> ---
[...]
> --- /dev/null
> +++ b/sound/usb/badd.c
> @@ -0,0 +1,495 @@
> +/*
> + * USB: Audio Class 3: support for BASIC AUDIO DEVICE v.3
> + *
> + * Author: Jorge Sanjuan <jorge.sanjuan at codethink.co.uk>
> + * Copyright (C) 2017 Codethink Ltd.
> + *
> + * This program is free software; you can redistribute it and/or modify
Drop the GPL boilerplate and add an SPDX licence identifier instead
(see commit b24413180f56).
[...]
> +struct uac3_feature_unit_descriptor inf_feat_unit_id2 = {
> + .bLength = 0x0F, /* 0x13 if stereo */
> + .bDescriptorType = USB_DT_CS_INTERFACE,
> + .bDescriptorSubtype = UAC3_FEATURE_UNIT,
> +
> + .bUnitID = 0x02,
> + /* bSourceID -> Profile dependent */
> + .bmaControls[0] = 0x03, /* Mute */
> + .bmaControls[1] = 0x0C, /* Chn1 Vol */
> + /* If stereo */
> + //.bmaControls[2] = 0x0C, /* Chn2 Vol */
Kernel style is to use only /* */ comments, not // comments.
[...]
> +/**
> + * badd_gen_cluster_desc - This is to bypass the GET_HIGH_CAPABILITY
> + * UAC3 requests for getting cluster descriptors and, instead, generate
> + * the cluster on the host side as per BADD specification.
> + */
A kernel-doc comment mst have a one-line summary and then a description
of each parameter. This function probably doesn't need that kind of
documentation, so you could drop the extra * from the comment opener.
> +void * badd_gen_cluster_desc(unsigned int m_s)
No space after the *. Use checkpatch.pl to check for trivial style
issues like this.
> +{
> + struct uac3_cluster_header_descriptor cluster;
> + struct uac3_cluster_information_segment_descriptor segment;
> + struct uac3_cluster_end_segment_descriptor end;
> + void *buffer;
> + int pos = 0;
> + int length;
> +
> + end.wLength = 0x03;
I think this needs to be a little-endian, in which case you need to use
cpu_to_le16(0x03).
Similarly for all the other 16/32-bit fields in descriptors.
> + end.bSegmentType = UAC3_END_SEGMENT;
> +
> + if (m_s == 0x01) { /* Mono */
Rather than commenting that this constant means Mono, you should name
it with an enum or macro. (Or if the variable is actually a number of
channels, then you should rename it.)
> + length = 0x10;
Where does this number come from? Shouldn't it be written as
sizeof(cluster) + sizeof(segment) + sizeof(end)?
[...]
> +/**
> + * badd_gen_csint_desc - generates a buffer with the inferred
> + * descriptors that are predefined in the BADD specfication.
A kernel-doc summary has to be a single line. This is not just a
recommendation, it's a hard constraint of the script.
There are a lot more magic numbers in here that ought to be replaced by
either named constants or an expression using sizeof().
[...]
> --- a/sound/usb/stream.c
> +++ b/sound/usb/stream.c
> @@ -37,6 +37,7 @@
> #include "format.h"
> #include "clock.h"
> #include "stream.h"
> +#include "badd.h"
>
> /*
> * free a substream
> @@ -485,7 +486,11 @@ static int parse_uac_endpoint_attributes(struct snd_usb_audio *chip,
> struct usb_interface_descriptor *altsd = get_iface_desc(alts);
> int attributes = 0;
>
> - csep = snd_usb_find_desc(alts->endpoint[0].extra, alts->endpoint[0].extralen, NULL, USB_DT_CS_ENDPOINT);
> + if (protocol == UAC_VERSION_3 &&
> + chip->badd_profile > UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0)
This looks excessively indented.
> + csep = badd_get_ep_dec();
> + else
> + csep = snd_usb_find_desc(alts->endpoint[0].extra, alts->endpoint[0].extralen, NULL, USB_DT_CS_ENDPOINT);
>
> /* Creamware Noah has this descriptor after the 2nd endpoint */
> if (!csep && altsd->bNumEndpoints >= 2)
[...]
> @@ -715,11 +723,33 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
> struct uac3_as_header_descriptor *as;
> struct uac3_cluster_header_descriptor *cluster;
> struct uac3_hc_descriptor_header hc_header;
> + struct usb_host_interface *badd_ctrl_intf;
> + void *badd_extra;
> + int badd_extralen;
> u16 cluster_id, wLength;
>
> - as = snd_usb_find_csint_desc(alts->extra,
> - alts->extralen,
> - NULL, UAC_AS_GENERAL);
> + badd_ctrl_intf = kzalloc(sizeof(*badd_ctrl_intf), GFP_KERNEL);
Missing an error check here.
> + memcpy(badd_ctrl_intf, chip->ctrl_intf, sizeof(*badd_ctrl_intf));
> +
> + if (chip->badd_profile > UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0) {
> +
> + as = badd_get_as_iface(stream, ep_packsize);
> +
> + err = badd_gen_csint_desc(&badd_extra, chip->badd_profile, as->wClusterDescrID);
> + if (err <= 0 || !badd_extra) {
> + dev_err(&dev->dev,
> + "%u:%d : Cannot set BADD profile 0x%x. err=%d. badd_extra %p\n",
> + iface_no, altno, chip->badd_profile, err, badd_extra);
> + return err;
> + }
> +
> + badd_extralen = err;
> + badd_ctrl_intf->extra = badd_extra;
> + badd_ctrl_intf->extralen = badd_extralen;
I don't think the badd_extra or badd_extralen variables are really
needed here - they just seem to complicate this.
> + } else
> + as = snd_usb_find_csint_desc(alts->extra,
> + alts->extralen,
> + NULL, UAC_AS_GENERAL);
>
> if (!as) {
> dev_err(&dev->dev,
> @@ -743,53 +773,64 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
> continue;
> }
>
> - /*
> - * Get number of channels and channel map through
> - * High Capability Cluster Descriptor
> - *
> - * First step: get High Capability header and
> - * read size of Cluster Descriptor
> - */
> - err = snd_usb_ctl_msg(chip->dev,
> - usb_rcvctrlpipe(chip->dev, 0),
> - UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> - cluster_id,
> - snd_usb_ctrl_intf(chip),
> - &hc_header, sizeof(hc_header));
> - if (err < 0)
> - return err;
> - else if (err != sizeof(hc_header)) {
> - dev_err(&dev->dev,
> - "%u:%d : can't get High Capability descriptor\n",
> - iface_no, altno);
> - return -EIO;
> - }
> -
> - /*
> - * Second step: allocate needed amount of memory
> - * and request Cluster Descriptor
> - */
> - wLength = le16_to_cpu(hc_header.wLength);
> - cluster = kzalloc(wLength, GFP_KERNEL);
> - if (!cluster)
> - return -ENOMEM;
> - err = snd_usb_ctl_msg(chip->dev,
> - usb_rcvctrlpipe(chip->dev, 0),
> - UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> - USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> - cluster_id,
> - snd_usb_ctrl_intf(chip),
> - cluster, wLength);
> - if (err < 0) {
> - kfree(cluster);
> - return err;
> - } else if (err != wLength) {
> - dev_err(&dev->dev,
> - "%u:%d : can't get Cluster Descriptor\n",
> - iface_no, altno);
> - kfree(cluster);
> - return -EIO;
> + if (chip->badd_profile > UAC3_FUNCTION_SUBCLASS_FULL_ADC_3_0) {
> +
> + cluster = badd_gen_cluster_desc(cluster_id);
> + if (!cluster) {
> + dev_err(&dev->dev,
> + "%u:%d : can't get Cluster Descriptor\n",
> + iface_no, altno);
> + return -ENOMEM;
> + }
> + } else {
> + /*
> + * Get number of channels and channel map through
> + * High Capability Cluster Descriptor
> + *
> + * First step: get High Capability header and
> + * read size of Cluster Descriptor
> + */
> + err = snd_usb_ctl_msg(chip->dev,
> + usb_rcvctrlpipe(chip->dev, 0),
> + UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> + USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> + cluster_id,
> + snd_usb_ctrl_intf(chip),
> + &hc_header, sizeof(hc_header));
> + if (err < 0)
> + return err;
> + else if (err != sizeof(hc_header)) {
> + dev_err(&dev->dev,
> + "%u:%d : can't get High Capability descriptor\n",
> + iface_no, altno);
> + return -EIO;
> + }
> +
> + /*
> + * Second step: allocate needed amount of memory
> + * and request Cluster Descriptor
> + */
> + wLength = le16_to_cpu(hc_header.wLength);
> + cluster = kzalloc(wLength, GFP_KERNEL);
> + if (!cluster)
> + return -ENOMEM;
> + err = snd_usb_ctl_msg(chip->dev,
> + usb_rcvctrlpipe(chip->dev, 0),
> + UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
> + USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
> + cluster_id,
> + snd_usb_ctrl_intf(chip),
> + cluster, wLength);
> + if (err < 0) {
> + kfree(cluster);
> + return err;
> + } else if (err != wLength) {
> + dev_err(&dev->dev,
> + "%u:%d : can't get Cluster Descriptor\n",
> + iface_no, altno);
> + kfree(cluster);
> + return -EIO;
> + }
This is a large block of code getting indented many levels deep. I
can't help thinking that it also ought to be turned into a separate
function.
All the error cases in this block will now leak badd_ctrl_inf, and it
would be easier to avoid that if it's turned into a separate function.
> }
>
> num_channels = cluster->bNrChannels;
> @@ -802,7 +843,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
> /* lookup the terminal associated to this interface
> * to extract the clock */
> input_term = snd_usb_find_input_terminal_descriptor(
> - chip->ctrl_intf,
> + badd_ctrl_intf,
> as->bTerminalLink);
>
> if (input_term) {
> @@ -810,7 +851,7 @@ int snd_usb_parse_audio_interface(struct snd_usb_audio *chip, int iface_no)
> break;
> }
>
> - output_term = snd_usb_find_output_terminal_descriptor(chip->ctrl_intf,
> + output_term = snd_usb_find_output_terminal_descriptor(badd_ctrl_intf,
> as->bTerminalLink);
> if (output_term) {
> clock = output_term->bCSourceID;
[...]
It looks like badd_ctrl_inf *always* gets leaked.
Ben.
--
Ben Hutchings
Software Developer, Codethink Ltd.
More information about the Alsa-devel
mailing list