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

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

[chihhao.chen]

From: "chihhao.chen" <chihhao.chen@mediatek.com>

The following scenario describes an echo test for
Samsung USBC Headset (AKG) with VID/PID (0x04e8/0xa051).

We first start a capture stream(USB IN transfer) in 96Khz/24bit/1ch mode.
In clock find source function, we get value 0x2 for clock selector
and 0x1 for clock source.

Kernel-4.14 behavior
Since clock source is valid so clock selector was not set again.
We pass through this function and start a playback stream(USB OUT transfer)
in 48Khz/32bit/2ch mode. This time we get value 0x1 for clock selector
and 0x1 for clock source. Finally clock id with this setting is 0x9.

Kernel-5.10 behavior
Clock selector was always set one more time even it is valid.
When we start a playback stream, we will get 0x2 for clock selector
and 0x1 for clock source. In this case clock id becomes 0xA.
This is an incorrect clock source setting and results in severe noises.
We see wrong data rate in USB IN transfer.
(From 288 bytes/ms becomes 144 bytes/ms) It should keep in 288 bytes/ms.

This earphone works fine on older kernel version load because
this is a newly-added behavior.

Signed-off-by: chihhao.chen <chihhao.chen@mediatek.com>
---
 sound/usb/clock.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/usb/clock.c b/sound/usb/clock.c
index 52de522..14456f6 100644
--- a/sound/usb/clock.c
+++ b/sound/usb/clock.c
@@ -324,6 +324,12 @@ static int __uac_clock_find_source(struct snd_usb_audio *chip,
 					      sources[ret - 1],
 					      visited, validate);
 		if (ret > 0) {
+			/*
+			 * For Samsung USBC Headset (AKG), setting clock selector again
+			 * will result in incorrect default clock setting problems
+			 */
+			if (chip->usb_id == USB_ID(0x04e8, 0xa051))
+				return ret;
 			err = uac_clock_selector_set_val(chip, entity_id, cur);
 			if (err < 0)
 				return err;
-- 
1.7.9.5

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

[Takashi Iwai]

On Sat, 24 Jul 2021 06:23:41 +0200,
<chihhao.chen@mediatek.com> wrote:
> 
> From: "chihhao.chen" <chihhao.chen@mediatek.com>
> 
> The following scenario describes an echo test for
> Samsung USBC Headset (AKG) with VID/PID (0x04e8/0xa051).
> 
> We first start a capture stream(USB IN transfer) in 96Khz/24bit/1ch mode.
> In clock find source function, we get value 0x2 for clock selector
> and 0x1 for clock source.
> 
> Kernel-4.14 behavior
> Since clock source is valid so clock selector was not set again.
> We pass through this function and start a playback stream(USB OUT transfer)
> in 48Khz/32bit/2ch mode. This time we get value 0x1 for clock selector
> and 0x1 for clock source. Finally clock id with this setting is 0x9.
> 
> Kernel-5.10 behavior
> Clock selector was always set one more time even it is valid.
> When we start a playback stream, we will get 0x2 for clock selector
> and 0x1 for clock source. In this case clock id becomes 0xA.
> This is an incorrect clock source setting and results in severe noises.
> We see wrong data rate in USB IN transfer.
> (From 288 bytes/ms becomes 144 bytes/ms) It should keep in 288 bytes/ms.
> 
> This earphone works fine on older kernel version load because
> this is a newly-added behavior.
> 
> Signed-off-by: chihhao.chen <chihhao.chen@mediatek.com>

Thanks for the patch.

This looks like a regression introduced by the recent commit
d2e8f641257d ("ALSA: usb-audio: Explicitly set up the clock
selector"), which is a fix for certain devices.  Too bad that the
behavior really depends on the device...

Maybe we need to introduce some flag to handle this commonly, but for
now, let's take the fix as is.


Takashi

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

[Geraldo Nascimento]

On Sat, Jul 24, 2021 at 8:05 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> This looks like a regression introduced by the recent commit
> d2e8f641257d ("ALSA: usb-audio: Explicitly set up the clock
> selector"), which is a fix for certain devices.  Too bad that the
> behavior really depends on the device...

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.

In that case the clock selector must be set to the only clock source.

This way we keep the generalization without breaking devices with more
than one clock source.

Just an idea.

Thank you,
Geraldo Nascimento

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

[Geraldo Nascimento]

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

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

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

[Takashi Iwai]

On Sat, 24 Jul 2021 17:04:13 +0200,
Geraldo Nascimento wrote:
> 
> On Sat, Jul 24, 2021 at 8:05 AM Takashi Iwai <tiwai@suse.de> wrote:
> >
> > This looks like a regression introduced by the recent commit
> > d2e8f641257d ("ALSA: usb-audio: Explicitly set up the clock
> > selector"), which is a fix for certain devices.  Too bad that the
> > behavior really depends on the device...
> 
> 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.
> 
> In that case the clock selector must be set to the only clock source.
> 
> This way we keep the generalization without breaking devices with more
> than one clock source.
> 
> Just an idea.

I don't think it's easy to generalize.  All those bugs are more or
less BIOS bugs, and a logic doesn't apply always, just because it's a
bug :)  For example, setting the clock selector itself should be a
valid operation from the specification POV, while this leads to
breakage on some devices.  So, even if we add a more generic
workaround, we need to see which side effect is more commonly seen at
first.


Takashi

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

[Geraldo Nascimento]

On Sun, Jul 25, 2021 at 7:44 AM Takashi Iwai <tiwai@suse.de> wrote:
>
> On Sat, 24 Jul 2021 17:04:13 +0200,
> Geraldo Nascimento wrote:
> >
> > On Sat, Jul 24, 2021 at 8:05 AM Takashi Iwai <tiwai@suse.de> wrote:
> > >
> > > This looks like a regression introduced by the recent commit
> > > d2e8f641257d ("ALSA: usb-audio: Explicitly set up the clock
> > > selector"), which is a fix for certain devices.  Too bad that the
> > > behavior really depends on the device...
> >
> > 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.
> >
> > In that case the clock selector must be set to the only clock source.
> >
> > This way we keep the generalization without breaking devices with more
> > than one clock source.
> >
> > Just an idea.
>
> I don't think it's easy to generalize.  All those bugs are more or
> less BIOS bugs, and a logic doesn't apply always, just because it's a
> bug :)  For example, setting the clock selector itself should be a
> valid operation from the specification POV, while this leads to
> breakage on some devices.  So, even if we add a more generic
> workaround, we need to see which side effect is more commonly seen at
> first.
>
>
> Takashi

Hello,

Like I said in one of the other emails in this thread, it's hard to
pinpoint a cause for the breakage of Samsung USBC Headset (AKG) with
VID/PID (0x04e8/0xa051) without the lsusb -v of the device in
question.

But from the description Chihhao Chen gave in the original message,
I'm *guessing* the Clock Source for the Samsung USB Headset (AKG) runs
at 48000hz and that we'd see a 2x Clock Multiplier in the lsusb -v

This is all a wild guess, without the lsusb -v it's impossible to be
sure, but if I'm right then the valid setting for the Microphone's
Clock Selector is the Clock Multiplier, not the Clock Source, which,
remember, runs at half the clock, hence why Chihhao Chen sees half the
data rate for USB IN.

Unfortunately our kernel code presently *does* always set the Clock
Selector to the Clock Source, which is a bad assumption to make in my
humble opinion.

The only valid case for setting the Clock Selector to the Clock Source
is when there's precisely one Clock Selector, precisely one Clock
Source and no Clock Multipliers.

In that special case we may be able to touch the setting of the only
Clock Selector to match the only Clock Source.

And, frankly, the only reason we're forced to do that explicitly is
because some Behringer gear (Archwave AG DACs) gets confused and seems
to somehow keep the old rate on the Clock Selector upon sample rate
change.

Thank you,
Geraldo Nascimento

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

[chihhao chen]

Hello,

Attach USB descriptor of clock source and selectior for this earphone.

AC Clock Source Descriptor:
------------------------------

Value	Valuename
0x08	bLength
0x24	bDescriptorType
0x0A	bDescriptorSubtype
0x09	bClockID
0x03	bmAttributes
0x07	bmControls
0x00	bAssocTerminal
0x00	iClockSource
Hex dump: 
0x08 0x24 0x0A 0x09 0x03 0x07 0x00 0x00 

AC Clock Selector Descriptor:
------------------------------

Value	Valuename
0x09	bLength
0x24	bDescriptorType
0x0B	bDescriptorSubtype
0x0B	bClockID
0x02	bNrInPins
0x09	baCSourceID(1)
0x0A	baCSourceID(2)
0x03	bmControls
0x00	iClockSelector
Hex dump: 
0x09 0x24 0x0B 0x0B 0x02 0x09 0x0A 0x03 0x00 

AC Clock Source Descriptor:
------------------------------

Value	Valuename
0x08	bLength
0x24	bDescriptorType
0x0A	bDescriptorSubtype
0x0A	bClockID
0x03	bmAttributes
0x07	bmControls
0x00	bAssocTerminal
0x00	iClockSource
Hex dump: 
0x08 0x24 0x0A 0x0A 0x03 0x07 0x00 0x00 

AC Clock Selector Descriptor:
------------------------------

Value	Valuename
0x09	bLength
0x24	bDescriptorType
0x0B	bDescriptorSubtype
0x0C	bClockID
0x02	bNrInPins
0x09	baCSourceID(1)
0x0A	baCSourceID(2)
0x03	bmControls
0x00	iClockSelector
Hex dump: 
0x09 0x24 0x0B 0x0C 0x02 0x09 0x0A 0x03 0x00

Thanks
Chihhao

On Mon, 2021-07-26 at 02:16 +0000, Geraldo Nascimento wrote:
> On Sun, Jul 25, 2021 at 7:44 AM Takashi Iwai <tiwai@suse.de> wrote:
> > 
> > On Sat, 24 Jul 2021 17:04:13 +0200,
> > Geraldo Nascimento wrote:
> > > 
> > > On Sat, Jul 24, 2021 at 8:05 AM Takashi Iwai <tiwai@suse.de>
> > > wrote:
> > > > 
> > > > This looks like a regression introduced by the recent commit
> > > > d2e8f641257d ("ALSA: usb-audio: Explicitly set up the clock
> > > > selector"), which is a fix for certain devices.  Too bad that
> > > > the
> > > > behavior really depends on the device...
> > > 
> > > 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.
> > > 
> > > In that case the clock selector must be set to the only clock
> > > source.
> > > 
> > > This way we keep the generalization without breaking devices with
> > > more
> > > than one clock source.
> > > 
> > > Just an idea.
> > 
> > I don't think it's easy to generalize.  All those bugs are more or
> > less BIOS bugs, and a logic doesn't apply always, just because it's
> > a
> > bug :)  For example, setting the clock selector itself should be a
> > valid operation from the specification POV, while this leads to
> > breakage on some devices.  So, even if we add a more generic
> > workaround, we need to see which side effect is more commonly seen
> > at
> > first.
> > 
> > 
> > Takashi
> 
> Hello,
> 
> Like I said in one of the other emails in this thread, it's hard to
> pinpoint a cause for the breakage of Samsung USBC Headset (AKG) with
> VID/PID (0x04e8/0xa051) without the lsusb -v of the device in
> question.
> 
> But from the description Chihhao Chen gave in the original message,
> I'm *guessing* the Clock Source for the Samsung USB Headset (AKG)
> runs
> at 48000hz and that we'd see a 2x Clock Multiplier in the lsusb -v
> 
> This is all a wild guess, without the lsusb -v it's impossible to be
> sure, but if I'm right then the valid setting for the Microphone's
> Clock Selector is the Clock Multiplier, not the Clock Source, which,
> remember, runs at half the clock, hence why Chihhao Chen sees half
> the
> data rate for USB IN.
> 
> Unfortunately our kernel code presently *does* always set the Clock
> Selector to the Clock Source, which is a bad assumption to make in my
> humble opinion.
> 
> The only valid case for setting the Clock Selector to the Clock
> Source
> is when there's precisely one Clock Selector, precisely one Clock
> Source and no Clock Multipliers.
> 
> In that special case we may be able to touch the setting of the only
> Clock Selector to match the only Clock Source.
> 
> And, frankly, the only reason we're forced to do that explicitly is
> because some Behringer gear (Archwave AG DACs) gets confused and
> seems
> to somehow keep the old rate on the Clock Selector upon sample rate
> change.
> 
> Thank you,
> Geraldo Nascimento

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

[Geraldo Nascimento]

On Mon, Jul 26, 2021 at 8:42 AM chihhao chen <chihhao.chen@mediatek.com> wrote:
>
> Hello,
>
> Attach USB descriptor of clock source and selectior for this earphone.
>
> AC Clock Source Descriptor:
> ------------------------------
>
> Value   Valuename
> 0x08    bLength
> 0x24    bDescriptorType
> 0x0A    bDescriptorSubtype
> 0x09    bClockID
> 0x03    bmAttributes
> 0x07    bmControls
> 0x00    bAssocTerminal
> 0x00    iClockSource
> Hex dump:
> 0x08 0x24 0x0A 0x09 0x03 0x07 0x00 0x00
>
> AC Clock Selector Descriptor:
> ------------------------------
>
> Value   Valuename
> 0x09    bLength
> 0x24    bDescriptorType
> 0x0B    bDescriptorSubtype
> 0x0B    bClockID
> 0x02    bNrInPins
> 0x09    baCSourceID(1)
> 0x0A    baCSourceID(2)
> 0x03    bmControls
> 0x00    iClockSelector
> Hex dump:
> 0x09 0x24 0x0B 0x0B 0x02 0x09 0x0A 0x03 0x00
>
> AC Clock Source Descriptor:
> ------------------------------
>
> Value   Valuename
> 0x08    bLength
> 0x24    bDescriptorType
> 0x0A    bDescriptorSubtype
> 0x0A    bClockID
> 0x03    bmAttributes
> 0x07    bmControls
> 0x00    bAssocTerminal
> 0x00    iClockSource
> Hex dump:
> 0x08 0x24 0x0A 0x0A 0x03 0x07 0x00 0x00
>
> AC Clock Selector Descriptor:
> ------------------------------
>
> Value   Valuename
> 0x09    bLength
> 0x24    bDescriptorType
> 0x0B    bDescriptorSubtype
> 0x0C    bClockID
> 0x02    bNrInPins
> 0x09    baCSourceID(1)
> 0x0A    baCSourceID(2)
> 0x03    bmControls
> 0x00    iClockSelector
> Hex dump:
> 0x09 0x24 0x0B 0x0C 0x02 0x09 0x0A 0x03 0x00
>
> Thanks
> Chihhao

Thank you, Chihhao.

So I was wrong about Samsung USBC Headset (AKG) with VID/PID
(0x04e8/0xa051) having a Clock Multiplier.

There are two Clock Sources, both linked to the USB SOF with fixed sample rate.

Plus two Clock Selectors which are host-programmable and can be set to
either of the two Clock Sources.


I'm still at a loss to explain what is going wrong here.

Would a printk() reveal the first explicit
uac_clock_selector_set_val() on the Clock Selector associated with
USB_IN sets Clock Source ID to pin 1 with Clock Source ID 0x9?

Or is it the other way around, i.e. it sets the Clock Source ID to pin
2 with Clock ID 0xA for the capture stream Clock Selector?


Chihhao Chen, could you please try the following patch for debugging
purposes and share what is printed in dmesg?

Please try one time with your fix applied and one time without, i.e.
with an otherwise unmodified vanilla kernel.

Thank you,
Geraldo Nascimento

--- clock.c.orig    2021-07-17 12:15:06.416028360 -0000
+++ clock.c    2021-07-26 20:45:58.713881962 -0000
@@ -300,6 +300,7 @@ static int __uac_clock_find_source(struc
         /* the entity ID we are looking for is a selector.
          * find out what it currently selects */
         ret = uac_clock_selector_get_val(chip, clock_id);
+        printk(KERN_ERR "FOR EP %x: Clock Selector %x has pin %d for
Clock Source ID %x selected\n", (unsigned int)fmt->endpoint, clock_id,
ret, sources[ret - 1]);
         if (ret < 0) {
             if (!chip->autoclock)
                 return ret;
@@ -324,6 +325,7 @@ static int __uac_clock_find_source(struc
                           sources[ret - 1],
                           visited, validate);
         if (ret > 0) {
+            printk(KERN_ERR "FOR EP %x: Found Source! Clock Selector
%x has pin %d for Clock Source ID %x about to be reselected\n",
(unsigned int)fmt->endpoint, entity_id, cur, sources[cur - 1]);
             err = uac_clock_selector_set_val(chip, entity_id, cur);
             if (err < 0)
                 return err;
@@ -344,6 +346,7 @@ static int __uac_clock_find_source(struc
             if (ret < 0)
                 continue;

+            printk(KERN_ERR "FOR EP %x: Found source by trial and
error! Clock Selector %x has pin %d for Clock Source ID %x about to be
selected\n", (unsigned int)fmt->endpoint, entity_id, i, sources[i -
1]);
             err = uac_clock_selector_set_val(chip, entity_id, i);
             if (err < 0)
                 continue;

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

[chihhao.chen]

From: chihhao chen <chihhao.chen@mediatek.com>

Hello

<6>[  150.347456][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
<6>[  150.347517][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
<6>[  150.384289][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
<6>[  150.390920][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
<6>[  150.438156][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
<6>[  150.438226][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
<6>[  150.473547][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
<6>[  150.480165][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
<6>[  150.513375][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
<6>[  150.513439][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
<6>[  150.546161][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
<6>[  150.552678][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
<6>[  150.584347][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
<6>[  150.584418][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
<6>[  150.617760][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
<6>[  150.624253][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
<6>[  150.657906][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
<6>[  150.657982][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
<6>[  150.689571][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
<6>[  150.696109][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected

Chihhao

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

[Geraldo Nascimento]

On Tue, Jul 27, 2021 at 10:28 AM <chihhao.chen@mediatek.com> wrote:
>
> From: chihhao chen <chihhao.chen@mediatek.com>
>
> Hello
>
> <6>[  150.347456][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
> <6>[  150.347517][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.384289][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
> <6>[  150.390920][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.438156][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
> <6>[  150.438226][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.473547][ T2768] __uac_clock_find_source: FOR EP 81: Clock Selector c has pin 2 for Clock Source ID a selected
> <6>[  150.480165][ T2768] __uac_clock_find_source: FOR EP 81: Found Source! Clock Selector c has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.513375][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
> <6>[  150.513439][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.546161][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
> <6>[  150.552678][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.584347][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
> <6>[  150.584418][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.617760][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
> <6>[  150.624253][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.657906][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
> <6>[  150.657982][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
> <6>[  150.689571][ T2768] __uac_clock_find_source: FOR EP 1: Clock Selector b has pin 2 for Clock Source ID a selected
> <6>[  150.696109][ T2768] __uac_clock_find_source: FOR EP 1: Found Source! Clock Selector b has pin 2 for Clock Source ID a about to be reselected
>
> Chihhao
>

Thank you. Chihhao Chen!

I see both EPs have their Clock Selectors (ID 0xC for EP 81 and ID 0xB
for EP 1) selected to pin 2, i.e. Clock Source ID 0xA.

I'm assuming this log is for the vanilla kernel without Chihhao's fix.
Please correct me if I'm wrong, Chihhao.

From the original commit message for the fix, we know the correct
setting for Clock Selector 0xB should be pin 1, with Clock Source ID
0x9.


Takashi Iwai already shared his perspective that this is a firmware
bug on the device.

I have a hunch that the firmware bug is setting both Clock Selectors
at the same time regardless of which one we want to select
specifically.


Chihhao, please try the below patch and perform another echo test.

Let us know if the echo test works or if it still fails and please
remember to share with us the relevant dmesg logs.


Thank you,
Geraldo Nascimento


--- clock.c.orig        2021-07-17 12:15:06.416028360 -0000
+++ clock.c     2021-07-27 17:36:16.954774954 -0000
@@ -324,9 +324,45 @@ static int __uac_clock_find_source(struc
                                              sources[ret - 1],
                                              visited, validate);
                if (ret > 0) {
-                       err = uac_clock_selector_set_val(chip, entity_id, cur);
-                       if (err < 0)
-                               return err;
+                       if (chip->usb_id == USB_ID(0x04e8, 0xa051)) {
+                               if (entity_id == 0xc) {
+                                       err =
uac_clock_selector_set_val(chip, entity_id, 2);
+                                       if (err < 0)
+                                               return err;
+
+                                       err =
uac_clock_selector_get_val(chip, 0xc);
+                                       if (err > 0) {
+                                               printk(KERN_ERR
"__uac_clock_find_source: Clock Selector 0xc has pin %d selected",
err);
+                                       }
+
+                                       err =
uac_clock_selector_get_val(chip, 0xb);
+                                       if (err > 0) {
+                                               printk(KERN_ERR
"__uac_clock_find_source: Clock Selector 0xb has pin %d selected",
err);
+                                       }
+                               }
+
+                               else if (entity_id == 0xb) {
+                                       err =
uac_clock_selector_set_val(chip, entity_id, 1);
+                                       if (err < 0)
+                                               return err;
+
+                                       err =
uac_clock_selector_get_val(chip, 0xc);
+                                       if (err > 0) {
+                                               printk(KERN_ERR
"__uac_clock_find_source: Clock Selector 0xc has pin %d selected",
err);
+                                       }
+
+                                       err =
uac_clock_selector_get_val(chip, 0xb);
+                                       if (err > 0) {
+                                               printk(KERN_ERR
"__uac_clock_find_source: Clock Selector 0xb has pin %d selected",
err);
+                                       }
+                               }
+                       }
+
+                       else {
+                               err = uac_clock_selector_set_val(chip,
entity_id, cur);
+                               if (err < 0)
+                                       return err;
+                       }
                }

                if (!validate || ret > 0 || !chip->autoclock)

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

[Geraldo Nascimento]

> Chihhao, please try the below patch and perform another echo test.
>
> Let us know if the echo test works or if it still fails and please
> remember to share with us the relevant dmesg logs.
>

Chihhao Chen,

when I said echo test I meant just test the device and observe if
there are noises related to incorrect clock setting.


I then realized a bit too late that probably in your programming
culture "echo test" refers to the whole technique you used to hook the
kernel and debug the issue to produce the fix.

If you call that technique "echo test" I'm sorry, I didn't know it was
called that way.


I just meant to briefly test to hear if there is noise even when the
clock parameters are correctly hardcoded.

My intent with that patch is to try to prove there's a firmware bug in
action just like Takashi Iwai suggested.


My apologies,
Geraldo Nascimento

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

[chihhao.chen]

From: chihhao chen <chihhao.chen@mediatek.com>

Hi Geraldo Nascimento,

For echo test, it means we use this earphone to receive and play sounds at the same time.
We found in this case serious noise problem happens.

Log as follows with your patch
<6>[  175.960387][T401365] __uac_clock_find_source: Clock Selector 0xc has pin 2 selected
<6>[  175.966980][T401365] __uac_clock_find_source: Clock Selector 0xb has pin 2 selected
<6>[  176.026251][T400354] __uac_clock_find_source: Clock Selector 0xc has pin 1 selected
<6>[  176.032406][T400354] __uac_clock_find_source: Clock Selector 0xb has pin 1 selected

There is no noise and I think this should be a firmware bug.

Thanks
Chihhao

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

[Geraldo Nascimento]

On Thu, Aug 5, 2021 at 7:54 AM <chihhao.chen@mediatek.com> wrote:
>
> From: chihhao chen <chihhao.chen@mediatek.com>
>
> Hi Geraldo Nascimento,

Hi Chihhao Chen!

>
> For echo test, it means we use this earphone to receive and play sounds at the same time.
> We found in this case serious noise problem happens.
>

That's what I understood initially, and only a little later I became
afraid echo test was the name of the debugging technique you used :)

Thanks for the clarification.

> Log as follows with your patch
> <6>[  175.960387][T401365] __uac_clock_find_source: Clock Selector 0xc has pin 2 selected
> <6>[  175.966980][T401365] __uac_clock_find_source: Clock Selector 0xb has pin 2 selected
> <6>[  176.026251][T400354] __uac_clock_find_source: Clock Selector 0xc has pin 1 selected
> <6>[  176.032406][T400354] __uac_clock_find_source: Clock Selector 0xb has pin 1 selected
>
> There is no noise and I think this should be a firmware bug.

From the log I'm afraid my worst assumptions were right.

Regardless of which Clock Selector we want to set, the firmware will
always set them both.

We should contact Samsung now that we have at least a sketch of a bug report...

Thanks,
Geraldo Nascimento

>
> Thanks
> Chihhao
>

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

[Geraldo Nascimento]

Chihhao Chen,

Since you're the one with the hardware I believe the bug report for
Samsung should be done by your side.

Maybe Takashi Iwai has somebody in mind that develops audio code for
Samsung and has already contributed to ALSA?

It's best if Takashi Iwai helps you out, and you take the shortcut to
the corporate structure, otherwise the importance of our findings may
be underestimated.

Chihhao Chen, please include the following link in your bug report to
Samsung: https://lore.kernel.org/patchwork/patch/1466711/

I'd politely remind them that the headset requires a very specific
quirk to work with recent Linux, that the patch has entered Linux-Next
however it's a workaround because the headset is apparently breaking
UAC compliance when it sets both Clock Selectors at the same time,
regardless of what Clock Selector we tell it to set via USB Control
Message.

But you're free to do as you wish. I'm sure you'll be fine.

Good luck,
Geraldo Nascimento

On Thu, Aug 5, 2021 at 11:31 PM Geraldo Nascimento
<geraldogabriel@gmail.com> wrote:
>
> Em Qui, 5 de ago de 2021 12:50, Geraldo Nascimento <geraldogabriel@gmail.com> escreveu:
>>
>> > There is no noise and I think this should be a firmware bug.
>
>
> The fact that there's no noise during the echo test itself doesn't mean it's not a firmware bug.
>
> It may just mean the MIC is able to support both 48KHz and 96KHz.
>
> Because from the log we see both Clock Selectors end up selected to pin 1.
>
> Thank you,
> Geraldo Nascimento