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@xxxxxxxxxxxxxxx> > --- [...] > --- /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@xxxxxxxxxxxxxxx> > + * 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@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel