Re: [PATCH 2/2] snd-usb-audio: Add duplex mode for Digidesign Mbox 1 and enable mixer

[Damien Zammit <damien.zammit@gmail.com>]

On 10/11/14 04:27, Takashi Iwai wrote:
> At Sun, 09 Nov 2014 15:04:42 +0100,
> Clemens Ladisch wrote:
>>
>> Takashi Iwai wrote:
>>> I think the only point is the check in create_composite_quirk(), where
>>> it marks the iface as claimed and skips the next entry that has been
>>> already claimed.  However, the current code looks inconsistent -- it
>>> allows multiple entries only if the iface matches with the current
>>> one.  Fixing it like below would make things working.
>>>
>>> It's a quick idea, so a bit more reviews would be needed, though.
>>> Clemens, what do you think?
>>
>> Reviewed-by: Clemens Ladisch <clemens@ladisch.de>


>>> ---
>>> --- a/sound/usb/quirks.c
>>> +++ b/sound/usb/quirks.c
>>> @@ -58,9 +58,17 @@ static int create_composite_quirk(struct snd_usb_audio *chip,
>>>  		err = snd_usb_create_quirk(chip, iface, driver, quirk);
>>>  		if (err < 0)
>>>  			return err;
>>> -		if (quirk->ifnum != probed_ifnum)
>>> +	}
>>> +
>>> +	for (quirk = quirk->data; quirk->ifnum >= 0; ++quirk) {
>>> +		iface = usb_ifnum_to_if(chip->dev, quirk->ifnum);
>>> +		if (!iface)
>>> +			continue;
>>> +		if (quirk->ifnum != probed_ifnum &&
>>> +		    !usb_interface_claimed(iface))
>>>  			usb_driver_claim_interface(driver, iface, (void *)-1L);
>>>  	}
>>> +
>>>  	return 0;
>>>  }
>>>
>>

When I tried the patch on the hardware with my quirk, I got a kernel oops.

[   62.317456] BUG: unable to handle kernel NULL pointer dereference at
0000000000000010
[   62.317461] IP: [<ffffffffa04a5f34>] create_composite_quirk+0x84/0xf0
[snd_usb_audio]
[   62.317469] PGD 0
[   62.317471] Oops: 0000 [#1] PREEMPT SMP
[   62.317474] Modules linked in: snd_usb_audio(OE+) snd_usbmidi_lib(OE)
snd_hwdep(O) snd_pcm(O) snd_seq_midi(O) snd_seq_midi_event(O)
snd_rawmidi(O) snd_seq(O) snd_seq_device(O) snd_timer(O) snd(O)
soundcore(O) binfmt_misc dvb_pll mt352 cx88_dvb videobuf_dvb
cx88_vp3054_i2c dvb_core rc_dntv_live_dvb_t kvm_amd kvm cx8800 cx8802
crct10dif_pclmul cx88xx crc32_pclmul ghash_clmulni_intel aesni_intel
btcx_risc tveeprom aes_x86_64 videobuf_dma_sg lrw rc_core gf128mul
videobuf_core dm_multipath glue_helper v4l2_common scsi_dh ablk_helper
videodev cryptd shpchp serio_raw i2c_algo_bit k10temp i2c_piix4 mac_hid
parport_pc ppdev lp parport btrfs(E) raid10(E) raid456(E)
async_raid6_recov(E) async_memcpy(E) async_pq(E) async_xor(E)
async_tx(E) xor(E) raid6_pq(E) raid0(E) multipath(E) linear(E)
dm_mirror(E) dm_region_hash(E) dm_log(E) raid1(E) hid_generic(E)
usbhid(E) hid(E) psmouse(E) firewire_ohci(E) firewire_core(E)
crc_itu_t(E) r8169(E) mii(E) sdhci_pci(E) sdhci(E) ahci(E) libahci(E)
[   62.317514] CPU: 1 PID: 1549 Comm: insmod Tainted: G           OE
3.18.0-rc2+ #3
[   62.317516] Hardware name: ASUS F2A85-M, BIOS 4.0-5272-g07d881a-dirty
01/16/2014
[   62.317518] task: ffff880036638000 ti: ffff880036004000 task.ti:
ffff880036004000
[   62.317519] RIP: 0010:[<ffffffffa04a5f34>]  [<ffffffffa04a5f34>]
create_composite_quirk+0x84/0xf0 [snd_usb_audio]
[   62.317526] RSP: 0018:ffff880036007b08  EFLAGS: 00010286
[   62.317528] RAX: 0000000000000000 RBX: 0000000000000000 RCX:
0000000000000001
[   62.317529] RDX: 0000000000000008 RSI: 00000000ffffffff RDI:
ffff8804049b6400
[   62.317530] RBP: ffff880036007b28 R08: 0000000000000008 R09:
ffff88040e801b00
[   62.317531] R10: ffffffffa04221e0 R11: ffff880404f33890 R12:
0000000000000000
[   62.317532] R13: ffffffffa04b2f60 R14: ffff880036556d00 R15:
0000000000000000
[   62.317534] FS:  00007fa6ac676740(0000) GS:ffff88041ec80000(0000)
knlGS:0000000000000000
[   62.317535] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[   62.317536] CR2: 0000000000000010 CR3: 00000000bad77000 CR4:
00000000000407e0
[   62.317538] Stack:
[   62.317539]  ffff880404f33800 0000000000000000 ffff880036556d00
ffffffffa04a8660
[   62.317541]  ffff880036007b38 ffffffffa04a5e7a ffff880036007bb8
ffffffffa0498888
[   62.317544]  ffff880404510cc8 0000000000000000 ffff880404f33890
ffff8804049b5c00
[   62.317546] Call Trace:
[   62.317554]  [<ffffffffa04a5e7a>] snd_usb_create_quirk+0x1a/0x50
[snd_usb_audio]
[   62.317560]  [<ffffffffa0498888>] usb_audio_probe+0x108/0x8d0
[snd_usb_audio]
[   62.317566]  [<ffffffff8158268f>] usb_probe_interface+0x1df/0x330
[   62.317569]  [<ffffffff814c4aad>] driver_probe_device+0x12d/0x3e0
[   62.317572]  [<ffffffff814c4e3b>] __driver_attach+0x9b/0xa0
[   62.317574]  [<ffffffff814c4da0>] ? __device_attach+0x40/0x40
[   62.317576]  [<ffffffff814c29d3>] bus_for_each_dev+0x63/0xa0
[   62.317578]  [<ffffffff814c448e>] driver_attach+0x1e/0x20
[   62.317580]  [<ffffffff814c4090>] bus_add_driver+0x180/0x240
[   62.317583]  [<ffffffff814c56a4>] driver_register+0x64/0xf0
[   62.317585]  [<ffffffff81580cf2>] usb_register_driver+0x82/0x160
[   62.317589]  [<ffffffffa04c1000>] ? 0xffffffffa04c1000
[   62.317594]  [<ffffffffa04c101e>] usb_audio_driver_init+0x1e/0x1000
[snd_usb_audio]
[   62.317597]  [<ffffffff8100212c>] do_one_initcall+0xbc/0x1f0
[   62.317601]  [<ffffffff811aa53c>] ? __vunmap+0x9c/0x110
[   62.317604]  [<ffffffff810f3252>] load_module+0x1d92/0x2820
[   62.317607]  [<ffffffff810ef0d0>] ? store_uevent+0x40/0x40
[   62.317610]  [<ffffffff810f3e56>] SyS_finit_module+0x86/0xb0
[   62.317613]  [<ffffffff81772aed>] system_call_fastpath+0x16/0x1b
[   62.317615] Code: 00 00 00 75 d2 48 89 d9 4c 89 ea 48 89 c6 4c 89 f7
e8 41 ff ff ff 85 c0 78 6f 48 83 c3 20 0f bf 73 10 66 85 f6 79 bd 48 8b
5b 18 <0f> bf 73 10 66 85 f6 79 10 eb 51 90 48 83 c3 20 0f bf 73 10 66
[   62.317638] RIP  [<ffffffffa04a5f34>]
create_composite_quirk+0x84/0xf0 [snd_usb_audio]
[   62.317645]  RSP <ffff880036007b08>
[   62.317646] CR2: 0000000000000010
[   62.317648] ---[ end trace d7b86fed44940082 ]---


My patch looks like this:

diff --git a/sound/usb/quirks-table.h b/sound/usb/quirks-table.h
index c657752..773859c 100644
--- a/sound/usb/quirks-table.h
+++ b/sound/usb/quirks-table.h
@@ -2944,27 +2944,53 @@ YAMAHA_DEVICE(0x7010, "UB99"),
 		.data = (const struct snd_usb_audio_quirk[]){
 			{
 				.ifnum = 0,
-				.type = QUIRK_IGNORE_INTERFACE,
+				.type = QUIRK_AUDIO_STANDARD_MIXER,
 			},
 			{
 				.ifnum = 1,
 				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
-				.data = &(const struct audioformat) {
-					.formats = SNDRV_PCM_FMTBIT_S24_3BE,
+				.data = &(const struct audioformat){
+					.formats =
+						SNDRV_PCM_FMTBIT_S24_3BE,
 					.channels = 2,
 					.iface = 1,
 					.altsetting = 1,
 					.altset_idx = 1,
-					.attributes = UAC_EP_CS_ATTR_SAMPLE_RATE,
+					.attributes = 0x4,
 					.endpoint = 0x02,
-					.ep_attr = 0x01,
-					.rates = SNDRV_PCM_RATE_44100 |
-						 SNDRV_PCM_RATE_48000,
-					.rate_min = 44100,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC |
+						USB_ENDPOINT_SYNC_SYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_48000,
+					.rate_min = 48000,
 					.rate_max = 48000,
-					.nr_rates = 2,
+					.nr_rates = 1,
 					.rate_table = (unsigned int[]) {
-						44100, 48000
+						48000
+					}
+				}
+			},
+			{
+				.ifnum = 2,
+				.type = QUIRK_AUDIO_FIXED_ENDPOINT,
+				.data = &(const struct audioformat){
+					.formats =
+						SNDRV_PCM_FMTBIT_S24_3BE,
+					.channels = 2,
+					.iface = 1,
+					.altsetting = 1,
+					.altset_idx = 1,
+					.attributes = 0x4,
+					.endpoint = 0x81,
+					.ep_attr = USB_ENDPOINT_XFER_ISOC |
+						USB_ENDPOINT_SYNC_ASYNC,
+					.maxpacksize = 0x130,
+					.rates = SNDRV_PCM_RATE_48000,
+					.rate_min = 48000,
+					.rate_max = 48000,
+					.nr_rates = 1,
+					.rate_table = (unsigned int[]) {
+						48000
 					}
 				}
 			},


I think it is something to do with the usb_ifnum_to_if() function not
liking to be called when the interface is not claimed, or something like
that?  Or did I miss something silly?

Damien