Re: [PATCH] ALSA: usb-audio: fix incorrect clock source setting

[Geraldo Nascimento]

I tried to convey in code what I had in mind.

It's a rough sketch and very much untested.

--- clock.5.14-rc2.c    2021-07-24 18:30:09.773718208 -0000
+++ clock-one-to-one.c  2021-07-24 18:35:52.276412366 -0000
@@ -54,6 +54,61 @@ static void *find_uac_clock_desc(struct
        return NULL;
 }

+/* Behringer UFX1604 / UFX1204 have a simple one-to-one
+ * topology where there is only one Clock Selector, only
+ * one Clock Source linked to USB SOF and no Clock Multipliers.
+ *
+ * This function checks for the presence of such a
+ * one-to-one clock selector / clock source topology
+ * so that it's possible to safely set the one and only
+ * Clock Selector to the one and only Clock Source
+ * upon sample rate change without breaking devices
+ * with more complicated topologies.
+ */
+
+static bool one_to_one_clock_topology(struct usb_host_interface
*iface, int proto)
+{
+        int clock_sources, clock_selectors, clock_multipliers = 0;
+       int source_version, selector_version, multiplier_version;
+       int found_count;
+
+       void *cs = NULL;
+
+       if (proto == UAC_VERSION_3) {
+               source_version = UAC3_CLOCK_SOURCE;
+               selector_version = UAC3_CLOCK_SELECTOR;
+               multiplier_version = UAC3_CLOCK_MULTIPLIER;
+       }
+
+       else {
+               source_version = UAC2_CLOCK_SOURCE;
+               selector_version = UAC2_CLOCK_SELECTOR;
+               multiplier_version = UAC2_CLOCK_MULTIPLIER;
+       }
+
+        if ((found_count = snd_usb_count_csint_desc(iface->extra,
iface->extralen,
+                                             cs, source_version)) > 0) {
+               clock_sources = found_count;
+        }
+
+        if ((found_count = snd_usb_count_csint_desc(iface->extra,
iface->extralen,
+                                             cs, selector_version)) > 0) {
+               clock_selectors = found_count;
+        }
+
+        if ((found_count = snd_usb_count_csint_desc(iface->extra,
iface->extralen,
+                                             cs, multiplier_version)) > 0) {
+               clock_multipliers = found_count;
+        }
+
+       if ((clock_sources == 1) && (clock_selectors == 1) &&
(clock_multipliers == 0)) {
+               return true;
+       }
+
+       return false;
+}
+
+
 static bool validate_clock_source(void *p, int id, int proto)
 {
        union uac23_clock_source_desc *cs = p;
@@ -323,7 +378,7 @@ static int __uac_clock_find_source(struc
                ret = __uac_clock_find_source(chip, fmt,
                                              sources[ret - 1],
                                              visited, validate);
-               if (ret > 0) {
+               if (ret > 0 &&
one_to_one_clock_topology(chip->ctrl_intf, proto)) {
                        err = uac_clock_selector_set_val(chip, entity_id, cur);
                        if (err < 0)
                                return err;



--- helper.5.14-rc2.c   2021-07-24 18:30:25.042526253 -0000
+++ helper-one-to-one.c 2021-07-24 18:35:45.019503597 -0000
@@ -64,6 +64,29 @@ void *snd_usb_find_csint_desc(void *buff
 }

 /*
+ * find every class-specified interface descriptor with the given subtype
+ * and return how many did it find
+ */
+int snd_usb_count_csint_desc(void *buffer, int buflen, void *after,
u8 dsubtype)
+{
+       int count = 0;
+        unsigned char *p = after;
+
+        while ((p = snd_usb_find_desc(buffer, buflen, p,
+                                      USB_DT_CS_INTERFACE)) != NULL) {
+                if (p[0] >= 3 && p[2] == dsubtype)
+                        count++;
+        }
+
+       if (count > 0) {
+               return count;
+       }
+
+        return 0;
+}
+
+
+/*
  * Wrapper for usb_control_msg().
  * Allocates a temp buffer to prevent dmaing from/to the stack.
  */



--- helper.5.14-rc2.h   2021-07-24 18:30:35.219398312 -0000
+++ helper-one-to-one.h 2021-07-24 18:29:34.139166195 -0000
@@ -7,6 +7,8 @@ unsigned int snd_usb_combine_bytes(unsig
 void *snd_usb_find_desc(void *descstart, int desclen, void *after, u8 dtype);
 void *snd_usb_find_csint_desc(void *descstart, int desclen, void
*after, u8 dsubtype);

+int snd_usb_count_csint_desc(void *descstart, int desclen, void
*after, u8 dsubtype);
+
 int snd_usb_ctl_msg(struct usb_device *dev, unsigned int pipe,
                    __u8 request, __u8 requesttype, __u16 value, __u16 index,
                    void *data, __u16 size);

On Sat, Jul 24, 2021 at 3:20 PM Geraldo Nascimento
<geraldogabriel@xxxxxxxxx> wrote:
>
> > Dr. Iwai, perhaps we could restrict the generalized fix for the
> > Behringer UFX1604 / UFX1204 with some simple logic to devices that
> > only have *one* clock source.
>
> Okay, rereading the original commit log from Cihhao Chen I gather
> Samsung USBC Headset (AKG) with VID/PID (0x04e8/0xa051) actually has
> two clock selectors and only one clock source.
>
> Correct me if I'm wrong.
>
> This is complicated by the fact I haven't been able to find a lsusb -v
> of Samsung USBC Headset (AKG) with VID/PID (0x04e8/0xa051)
>
> Even so, my proposition still stands: devices with only one clock
> source and only one clock selector should be able to handle us
> selecting the clock selector to the only clock source.