| 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: <52c0eb21-0a69-7a2b-da8a-2d33ad4e11d2@codethink.co.uk>
Date: Tue, 8 May 2018 10:43:03 +0100
From: Jorge <jorge.sanjuan@...ethink.co.uk>
To: Ruslan Bilovol <ruslan.bilovol@...il.com>
Cc: Takashi Iwai <tiwai@...e.com>, alsa-devel@...a-project.org,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/4] ALSA: usb-audio: UAC3. Add support for mixer unit.
On 04/05/18 01:57, Ruslan Bilovol wrote:
> On Fri, Apr 27, 2018 at 8:06 PM, Jorge Sanjuan
> <jorge.sanjuan@...ethink.co.uk> wrote:
>> This adds support for the MIXER UNIT in UAC3. All the information
>> is obtained from the (HIGH CAPABILITY) Cluster's header. We don't
>> read the rest of the logical cluster to obtain the channel config
>> as that wont make any difference in the current mixer behaviour.
>>
>> The name of the mixer unit is not yet requested as there is not
>> support for the UAC3 Class Specific String requests.
>>
>> Tested in an UAC3 device working as a HEADSET with a basic mixer
>> unit (same as the one in the BADD spec) with no controls.
>
> So, after deeper looking into the code and after testing this patch,
> in your usecase (mixer with no controls) you'll never execute
> build_mixer_unit_ctl(), correct? So did you try to just fix issues with
> incorrect parsing of mixer unit descriptor?
>
>>
>> Signed-off-by: Jorge Sanjuan <jorge.sanjuan@...ethink.co.uk>
>> ---
>> include/uapi/linux/usb/audio.h | 19 +++++++--
>> sound/usb/mixer.c | 88 ++++++++++++++++++++++++++++++++++++++----
>> 2 files changed, 97 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/uapi/linux/usb/audio.h b/include/uapi/linux/usb/audio.h
>> index 3a78e7145689..13d98e6e0db1 100644
>> --- a/include/uapi/linux/usb/audio.h
>> +++ b/include/uapi/linux/usb/audio.h
>> @@ -285,9 +285,22 @@ static inline __u8 uac_mixer_unit_iChannelNames(struct uac_mixer_unit_descriptor
>> static inline __u8 *uac_mixer_unit_bmControls(struct uac_mixer_unit_descriptor *desc,
>> int protocol)
>> {
>> - return (protocol == UAC_VERSION_1) ?
>> - &desc->baSourceID[desc->bNrInPins + 4] :
>> - &desc->baSourceID[desc->bNrInPins + 6];
>> + switch (protocol) {
>> + case UAC_VERSION_1:
>> + return &desc->baSourceID[desc->bNrInPins + 4];
>> + case UAC_VERSION_2:
>> + return &desc->baSourceID[desc->bNrInPins + 6];
>> + case UAC_VERSION_3:
>> + return &desc->baSourceID[desc->bNrInPins + 2];
>> + default:
>> + return NULL;
>> + }
>> +}
>> +
>> +static inline __u16 uac3_mixer_unit_wClusterDescrID(struct uac_mixer_unit_descriptor *desc)
>> +{
>> + return (desc->baSourceID[desc->bNrInPins + 1] << 8) |
>> + desc->baSourceID[desc->bNrInPins];
>> }
>>
>> static inline __u8 uac_mixer_unit_iMixer(struct uac_mixer_unit_descriptor *desc)
>> diff --git a/sound/usb/mixer.c b/sound/usb/mixer.c
>> index 301ad61ed426..3503f4840ec3 100644
>> --- a/sound/usb/mixer.c
>> +++ b/sound/usb/mixer.c
>> @@ -719,6 +719,66 @@ static int get_term_name(struct mixer_build *state, struct usb_audio_term *iterm
>> }
>>
>> /*
>> + * Get logical cluster information for UAC3 devices.
>> + */
>> +static int get_cluster_channels_v3(struct mixer_build *state, unsigned int cluster_id)
>> +{
>> + struct uac3_cluster_header_descriptor c_header;
>> + int err;
>> +
>> + err = snd_usb_ctl_msg(state->chip->dev,
>> + usb_rcvctrlpipe(state->chip->dev, 0),
>> + UAC3_CS_REQ_HIGH_CAPABILITY_DESCRIPTOR,
>> + USB_RECIP_INTERFACE | USB_TYPE_CLASS | USB_DIR_IN,
>> + cluster_id,
>> + snd_usb_ctrl_intf(state->chip),
>> + &c_header, sizeof(c_header));
>> + if (err < 0)
>> + goto error;
>> + if (err != sizeof(c_header)) {
>> + err = -EIO;
>> + goto error;
>> + }
>> +
>> + return c_header.bNrChannels;
>> +
>> +error:
>> + usb_audio_err(state->chip, "cannot request logical cluster ID: %d (err: %d)\n", cluster_id, err);
>> + return err;
>> +}
>> +
>> +/*
>> + * Get number of channels for a Mixer Unit.
>> + */
>> +static int uac_mixer_unit_get_channels(struct mixer_build *state,
>> + struct uac_mixer_unit_descriptor *desc)
>> +{
>> + int mu_channels;
>> +
>> + if (desc->bLength < 11)
>> + return -EINVAL;
>> + if (!desc->bNrInPins)
>> + return -EINVAL;
>> +
>> + switch (state->mixer->protocol) {
>> + case UAC_VERSION_1:
>> + case UAC_VERSION_2:
>> + default:
>> + mu_channels = uac_mixer_unit_bNrChannels(desc);
>> + break;
>> + case UAC_VERSION_3:
>> + mu_channels = get_cluster_channels_v3(state,
>> + uac3_mixer_unit_wClusterDescrID(desc));
>> + break;
>> + }
>> +
>> + if (!mu_channels)
>> + return -EINVAL;
>> +
>> + return mu_channels;
>> +}
>> +
>> +/*
>> * parse the source unit recursively until it reaches to a terminal
>> * or a branched unit.
>> */
>> @@ -865,6 +925,18 @@ static int check_input_term(struct mixer_build *state, int id,
>> term->name = le16_to_cpu(d->wClockSourceStr);
>> return 0;
>> }
>> + case UAC3_MIXER_UNIT: {
>> + struct uac_mixer_unit_descriptor *d = p1;
>> +
>> + err = uac_mixer_unit_get_channels(state, d);
>> + if (err < 0)
>> + return err;
>> +
>> + term->channels = err;
>> + term->type = d->bDescriptorSubtype << 16; /* virtual type */
>> +
>> + return 0;
>> + }
>> default:
>> return -ENODEV;
>> }
>> @@ -1797,11 +1869,10 @@ static int parse_audio_feature_unit(struct mixer_build *state, int unitid,
>> */
>> static void build_mixer_unit_ctl(struct mixer_build *state,
>> struct uac_mixer_unit_descriptor *desc,
>> - int in_pin, int in_ch, int unitid,
>> - struct usb_audio_term *iterm)
>> + int in_pin, int in_ch, int num_outs,
>> + int unitid, struct usb_audio_term *iterm)
>> {
>> struct usb_mixer_elem_info *cval;
>> - unsigned int num_outs = uac_mixer_unit_bNrChannels(desc);
>> unsigned int i, len;
>> struct snd_kcontrol *kctl;
>> const struct usbmix_name_map *map;
>> @@ -1878,14 +1949,17 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>> int input_pins, num_ins, num_outs;
>> int pin, ich, err;
>>
>> - if (desc->bLength < 11 || !(input_pins = desc->bNrInPins) ||
>> - !(num_outs = uac_mixer_unit_bNrChannels(desc))) {
>> + err = uac_mixer_unit_get_channels(state, desc);
>> + if (err < 0) {
>> usb_audio_err(state->chip,
>> "invalid MIXER UNIT descriptor %d\n",
>> unitid);
>> - return -EINVAL;
>> + return err;
>> }
>>
>> + num_outs = err;
>> + input_pins = desc->bNrInPins;
>> +
>> num_ins = 0;
>> ich = 0;
>> for (pin = 0; pin < input_pins; pin++) {
>> @@ -1912,7 +1986,7 @@ static int parse_audio_mixer_unit(struct mixer_build *state, int unitid,
>> }
>> }
>> if (ich_has_controls)
>> - build_mixer_unit_ctl(state, desc, pin, ich,
>> + build_mixer_unit_ctl(state, desc, pin, ich, num_outs,
>> unitid, &iterm);
>
> So with current sources we will never reach this place. In your
> usecase (mixer with no
> controls) it obviously won't go inside.
> However, I created a mixer with controls but still
> build_mixer_unit_ctl() isn't executed.
> This is because UAC3 input terminal parsing always returns 0 channels, this is
> what still needs to be implemented (see comment "REVISIT: UAC3 IT doesn't
> have channels/cfg" in check_input_term)
Thanks for testing this!
I missed that one with the number of input channels. Regarding the
"REVISIT: UAC3 IT doesn't have channels/cfg" comment, we can easily fix
that using the helper function I added in this patch for reading the
logical cluster's number of channels `get_cluster_channels_v3`. The
channel config bitmap I don't see it being used at all so parsing
iterm.channels should be enough. Any thoughts?
- Jorge
>
> Beside of that, other part of this patch seems to work as expected, at least
> uac_mixer_unit_get_channels() gives correct results - I checked it for
> two-channels config, that is different comparing to BADD.
>
> Thanks,
> Ruslan
>
Powered by blists - more mailing lists