Re: [Linux-kernel] [PATCH 1/6] ALSA: usb: ADC3: Add initial BADD spec support

[Ben Hutchings <ben.hutchings@codethink.co.uk>]

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@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@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.

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel