December 2016 - Alsa-devel - mailman.alsa-project.org

[alsa-devel] [PATCH] Support both DSD LE/BE Amanero firmware versions
by Jussi Laako 28 Dec '16

28 Dec '16

Add DSD support for both little endian (DSD_U32_LE) and big endian (DSD_U32_BE) version of the Amanero firmware. Signed-off-by: Jussi Laako <jussi(a)sonarnerd.net> --- sound/usb/quirks.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/sound/usb/quirks.c b/sound/usb/quirks.c index b3fd238..f5c68ea 100644 --- a/sound/usb/quirks.c +++ b/sound/usb/quirks.c @@ -1359,6 +1359,21 @@ u64 snd_usb_interface_dsd_format_quirks(struct snd_usb_audio *chip, if (fp->altsetting == 3) return SNDRV_PCM_FMTBIT_DSD_U32_BE; break; + + /* Amanero Combo384 USB interface with native DSD support */ + case USB_ID(0x16d0, 0x071a): + if (fp->altsetting == 2) { + switch (chip->dev->descriptor.bcdDevice) { + case 0x199: + return SNDRV_PCM_FMTBIT_DSD_U32_LE; + case 0x19b: + return SNDRV_PCM_FMTBIT_DSD_U32_BE; + default: + break; + } + } + break; + default: break; } -- 2.7.4

2 1

[alsa-devel] [PATCH] ALSA: use designated initializers
by Kees Cook 28 Dec '16

28 Dec '16

Prepare to mark sensitive kernel structures for randomization by making sure they're using designated initializers. These were identified during allyesconfig builds of x86, arm, and arm64, with most initializer fixes extracted from grsecurity. Signed-off-by: Kees Cook <keescook(a)chromium.org> --- sound/synth/emux/emux_seq.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/sound/synth/emux/emux_seq.c b/sound/synth/emux/emux_seq.c index a0209204ae48..55579f6b8cb2 100644 --- a/sound/synth/emux/emux_seq.c +++ b/sound/synth/emux/emux_seq.c @@ -33,13 +33,13 @@ static int snd_emux_unuse(void *private_data, struct snd_seq_port_subscribe *inf * MIDI emulation operators */ static struct snd_midi_op emux_ops = { - snd_emux_note_on, - snd_emux_note_off, - snd_emux_key_press, - snd_emux_terminate_note, - snd_emux_control, - snd_emux_nrpn, - snd_emux_sysex, + .note_on = snd_emux_note_on, + .note_off = snd_emux_note_off, + .key_press = snd_emux_key_press, + .note_terminate = snd_emux_terminate_note, + .control = snd_emux_control, + .nrpn = snd_emux_nrpn, + .sysex = snd_emux_sysex, }; -- 2.7.4 -- Kees Cook Nexus Security

3 2

Re: [alsa-devel] [PATCH] ALSA: cs5535audio: fix unused warnings on resume/suspend
by Takashi Iwai 28 Dec '16

28 Dec '16

On Sat, 17 Dec 2016 01:25:27 +0100, Jérémy Lefaure wrote: > > When CONFIG_PM_SLEEP is disabled, SIMPLE_DEV_PM_OPS does not use > snd_cs5535audio_resume and snd_cs5535audio_suspend functions: > > sound/pci/cs5535audio/cs5535audio_pm.c:77:12: warning: ‘snd_cs5535audio_resume’ defined but not used [-Wunused-function] > static int snd_cs5535audio_resume(struct device *dev) > ^~~~~~~~~~~~~~~~~~~~~~ > sound/pci/cs5535audio/cs5535audio_pm.c:58:12: warning: ‘snd_cs5535audio_suspend’ defined but not used [-Wunused-function] > static int snd_cs5535audio_suspend(struct device *dev) > ^~~~~~~~~~~~~~~~~~~~~~~ > > Adding __maybe_unused to the declaration of these functions removes the > warnings. > > Signed-off-by: Jérémy Lefaure <jeremy.lefaure(a)lse.epita.fr> Applied, thanks. Takashi > --- > sound/pci/cs5535audio/cs5535audio_pm.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/sound/pci/cs5535audio/cs5535audio_pm.c b/sound/pci/cs5535audio/cs5535audio_pm.c > index 06ac5d8da362..82bd10b68a77 100644 > --- a/sound/pci/cs5535audio/cs5535audio_pm.c > +++ b/sound/pci/cs5535audio/cs5535audio_pm.c > @@ -55,7 +55,7 @@ static void snd_cs5535audio_stop_hardware(struct cs5535audio *cs5535au) > > } > > -static int snd_cs5535audio_suspend(struct device *dev) > +static int __maybe_unused snd_cs5535audio_suspend(struct device *dev) > { > struct snd_card *card = dev_get_drvdata(dev); > struct cs5535audio *cs5535au = card->private_data; > @@ -74,7 +74,7 @@ static int snd_cs5535audio_suspend(struct device *dev) > return 0; > } > > -static int snd_cs5535audio_resume(struct device *dev) > +static int __maybe_unused snd_cs5535audio_resume(struct device *dev) > { > struct snd_card *card = dev_get_drvdata(dev); > struct cs5535audio *cs5535au = card->private_data; > -- > 2.11.0 > >

1 0

[alsa-devel] [PATCH 0/2] ASoC: Dell IoT Gateway audio support
by Shrirang Bagul 27 Dec '16

27 Dec '16

These patches add basic audio support on new Dell Edge IoT Gateways. RT5660 codec is connected to SSP2 port on Intel Atom Baytrail E3815 SoC. The machine driver is based on bytcr_rt5640. These devices have line-in and line-out jacks with line-out mute enable gpio described in ACPI tables. Shrirang Bagul (2): ASoC: rt5660: Add ACPI support ASoC: Intel: boards: Add Baytrail RT5660 machine driver sound/soc/codecs/rt5660.c | 26 ++ sound/soc/intel/Kconfig | 13 + sound/soc/intel/atom/sst/sst_acpi.c | 2 + sound/soc/intel/boards/Makefile | 2 + sound/soc/intel/boards/bytcr_rt5660.c | 573 ++++++++++++++++++++++++++++++++++ 5 files changed, 616 insertions(+) create mode 100644 sound/soc/intel/boards/bytcr_rt5660.c -- 2.9.3

3 9

[alsa-devel] [snd-usb-audio] BUG: NULL pointer dereference at 0000000000000070
by Dave Young 25 Dec '16

25 Dec '16

Hi, With recent mainline kernel, I see a BUG, it is easy to reproduce, just plugging the usb microphone, bisected the first bad commit is: 16200948d8353fe29a473a394d7d26790deae0e7 is the first bad commit commit 16200948d8353fe29a473a394d7d26790deae0e7 Author: Takashi Iwai <tiwai(a)suse.de> Date: Mon Dec 5 11:19:38 2016 +0100 ALSA: usb-audio: Fix race at stopping the stream We've got a kernel crash report showing like: Unable to handle kernel NULL pointer dereference at virtual address 00000008 pgd = a1d7c000 [snip] The BUG dmesg itself is below: [54029.102610] input: Samson Technologies Samson Meteor Mic as /devices/pci0000:00/0000:00:14.0/usb2/2-3/2-3:1.3/0003:17A0:0310.0003/input/input19 [54029.154424] hid-generic 0003:17A0:0310.0003: input: USB HID v1.00 Device [Samson Technologies Samson Meteor Mic] on usb-0000:00:14.0-3/input3 [54029.202035] usbcore: registered new interface driver snd-usb-audio [54029.242705] BUG: unable to handle kernel NULL pointer dereference at 0000000000000070 [54029.271667] IP: retire_playback_urb+0x5/0xd0 [snd_usb_audio] [54029.300462] PGD 0 [54029.300462] [54029.355691] Oops: 0000 [#1] SMP [54029.383215] Modules linked in: snd_usb_audio snd_usbmidi_lib snd_rawmidi macvtap macvlan tun ccm rfcomm fuse snd_hda_codec_hdmi cmac bnep kvm_intel kvm irqbypass i915 arc4 intel_gtt drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm snd_hda_codec_realtek snd_hda_codec_generic iwlmvm mac80211 rtsx_pci_sdmmc iwlwifi snd_hda_intel snd_hda_codec snd_hwdep snd_hda_core cfg80211 snd_seq snd_seq_device btusb btrtl thinkpad_acpi btbcm pcspkr input_leds btintel serio_raw bluetooth snd_pcm e1000e snd_timer ptp rtsx_pci snd i2c_i801 pps_core rfkill mfd_core soundcore video nfsd auth_rpcgss nfs_acl lockd grace sunrpc [54029.480514] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G W 4.9.0+ #209 [54029.514169] Hardware name: LENOVO 20ARS1BJ02/20ARS1BJ02, BIOS GJET91WW (2.41 ) 09/21/2016 [54029.548395] task: ffff9c2894a18bc0 task.stack: ffffa69dc0cd0000 [54029.582630] RIP: 0010:retire_playback_urb+0x5/0xd0 [snd_usb_audio] [54029.617049] RSP: 0018:ffff9c289f243cd0 EFLAGS: 00010086 [54029.651439] RAX: ffffffffc031bac0 RBX: ffff9c2868a8a000 RCX: 0000000000000001 [54029.686222] RDX: 0000000000000000 RSI: ffff9c288e3e3a00 RDI: 0000000000000000 [54029.721046] RBP: ffff9c289f243d00 R08: 0000000000000001 R09: ffff9c289e803b00 [54029.755850] R10: ffff9c28848f3380 R11: ffff9c289038d0b0 R12: ffff9c2868a8a140 [54029.790482] R13: ffff9c288e3e3a00 R14: 0000000000000000 R15: ffff9c288e3e0390 [54029.824649] FS: 0000000000000000(0000) GS:ffff9c289f240000(0000) knlGS:0000000000000000 [54029.859458] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 [54029.894609] CR2: 0000000000000070 CR3: 000000021e008000 CR4: 00000000001406e0 [54029.929476] Call Trace: [54029.963968] <IRQ> [54029.998291] ? snd_complete_urb+0x80/0x260 [snd_usb_audio] [54030.033359] __usb_hcd_giveback_urb+0x76/0x100 [54030.068352] usb_hcd_giveback_urb+0x3c/0xc0 [54030.103167] xhci_giveback_urb_in_irq.isra.23+0x6f/0xa0 [54030.138683] finish_td.constprop.39+0x175/0x260 [54030.173733] xhci_irq+0x9f0/0x1450 [54030.208790] ? try_to_wake_up+0x1f2/0x390 [54030.243696] ? usb_hcd_poll_rh_status+0x190/0x190 [54030.278521] xhci_msi_irq+0x11/0x20 [54030.313376] __handle_irq_event_percpu+0x7e/0x1a0 [54030.348615] handle_irq_event_percpu+0x32/0x80 [54030.383917] handle_irq_event+0x2c/0x50 [54030.419012] handle_edge_irq+0x9f/0x120 [54030.454042] handle_irq+0x73/0x130 [54030.488522] ? _local_bh_enable+0x21/0x50 [54030.522777] do_IRQ+0x46/0xd0 [54030.556882] common_interrupt+0x90/0x90 [54030.591095] RIP: 0010:cpuidle_enter_state+0x134/0x2a0 [54030.625661] RSP: 0018:ffffa69dc0cd3e60 EFLAGS: 00000246 ORIG_RAX: ffffffffffffff2c [54030.660438] RAX: 0000000000000000 RBX: 00003123a9d13159 RCX: 000000000000001f [54030.695705] RDX: 00003123a9d13159 RSI: ffff9c289f254f98 RDI: 0000000000000000 [54030.731113] RBP: ffffa69dc0cd3e98 R08: cccccccccccccccd R09: 0000000000000018 [54030.766539] R10: 000000000000019c R11: 00000000000000a7 R12: 0000000000000004 [54030.802207] R13: 0000000000000004 R14: ffff9c289f25db08 R15: 00003123a9c9b583 [54030.837897] </IRQ> [54030.873227] cpuidle_enter+0x17/0x20 [54030.908827] call_cpuidle+0x23/0x40 [54030.944343] do_idle+0x189/0x200 [54030.979754] cpu_startup_entry+0x71/0x80 [54031.015166] start_secondary+0x142/0x160 [54031.050630] start_cpu+0x14/0x14 [54031.085944] Code: e9 03 41 5e 5d f7 f1 89 c0 c3 41 8b 76 64 4c 89 e7 e8 f0 fe ff ff eb c4 0f 1f 40 00 66 2e 0f 1f 84 00 00 00 00 00 0f 1f 44 00 00 <48> 8b 4f 70 31 d2 8b 86 88 00 00 00 f7 b1 98 15 00 00 85 c0 75 [54031.124608] RIP: retire_playback_urb+0x5/0xd0 [snd_usb_audio] RSP: ffff9c289f243cd0 [54031.162852] CR2: 0000000000000070

1 0

[alsa-devel] How to keep track of the amount of queued samples?
by Jan Deinhard 23 Dec '16

23 Dec '16

Hi, I would like to understand the following quote which I copied from https://www.kernel.org/doc/Documentation/sound/alsa/timestamping.txt "The ALSA API provides two basic pieces of information, avail and delay, which combined with the trigger and current system timestamps allow for applications to keep track of the 'fullness' of the ring buffer and the amount of queued samples." Querying avail and delay is straight forward. But I have no idea how to use these values in combination with trigger_htstamp and the current system timestamp to calculate the fullness of the ring buffer. Can someone provide some insight? What exactly is meant by "current system timestamp"? Is it provied by ALSA (htstamp?) or should I use clock_gettime(CLOCK_MONOTONIC, &system_time); to get that timestamp? Thanks, Jan

2 1

[alsa-devel] [PATCH 1/2] ucm: Automatically load the best config file based on the card long name
by mengdong.lin＠linux.intel.com 23 Dec '16

23 Dec '16

From: Mengdong Lin <mengdong.lin(a)linux.intel.com> Intel DSP platform drivers are used by many different devices. For user space to differentiate them, ASoC machine driver may include the DMI info (vendor, product and board) in card long name. Possible card long names are: broadwell-rt286-Dell Inc.-XPS 13 9343-0310JH broadwell-rt286-Intel Corp.-Broadwell Client platform-Wilson Beach SDS bytcr-rt5640-ASUSTeK COMPUTER INC.-T100TA-T100TA bytcr-rt5651-Circuitco-Minnowboard Max D0 PLATFORM-MinnowBoard MAX ... And user space can define configuration files including fields separated by '.' as below: broadwell-rt286 broadwell-rt286.Dell.XPS bytcr-rt5640 bytcr-rt5640.ASUS.T100 bytcr-rt5651.MinnowboardMax ... When being asked to load configuration file of a card, UCM will try to find the card long name from the local machine, and then scan all available configuration file names, search every field of config file name in the card long name. The more characters match, the higher score the file has. Finally, the file with the highest score will be loaded to configure the sound card. Signed-off-by: Mengdong Lin <mengdong.lin(a)linux.intel.com> diff --git a/src/ucm/parser.c b/src/ucm/parser.c index c98373a..ff75da7 100644 --- a/src/ucm/parser.c +++ b/src/ucm/parser.c @@ -55,6 +55,9 @@ static const char * const component_dir[] = { NULL, /* terminator */ }; +static int filename_filter(const struct dirent *dirent); +static int is_component_directory(const char *dir); + static int parse_sequence(snd_use_case_mgr_t *uc_mgr, struct list_head *base, snd_config_t *cfg); @@ -1328,6 +1331,210 @@ static int parse_master_file(snd_use_case_mgr_t *uc_mgr, snd_config_t *cfg) return 0; } +/* trim SPACE (0x20) from the string */ +static void trim_space(char *str) +{ + int i, j = 0; + + for (i = 0; str[i] && i < MAX_FILE; i++) { + if (str[i] != 0x20) + str[j++] = str[i]; + } + + str[j] = '\0'; +} + +/* find and store the card long name if the card is in this machine */ +static int get_card_long_name(snd_use_case_mgr_t *mgr) +{ + const char *card_name = mgr->card_name; + snd_ctl_t *handle; + int card, err, dev, idx; + snd_ctl_card_info_t *info; + const char *_name, *_long_name; + + snd_ctl_card_info_alloca(&info); + + card = -1; + if (snd_card_next(&card) < 0 || card < 0) { + uc_error("no soundcards found..."); + return -1; + } + + while (card >= 0) { + char name[32]; + + sprintf(name, "hw:%d", card); + err = snd_ctl_open(&handle, name, 0); + if (err < 0) { + uc_error("control open (%i): %s", card, + snd_strerror(err)); + goto next_card; + } + + err = snd_ctl_card_info(handle, info); + if (err < 0) { + uc_error("control hardware info (%i): %s", card, + snd_strerror(err)); + snd_ctl_close(handle); + goto next_card; + } + + _name = snd_ctl_card_info_get_name(info); + if (!strncmp(card_name, _name, 32)) { + _long_name = snd_ctl_card_info_get_longname(info); + strncpy(mgr->card_long_name, _long_name, + MAX_CARD_LONG_NAME); + snd_ctl_close(handle); + return 0; + } + + snd_ctl_close(handle); +next_card: + if (snd_card_next(&card) < 0) { + uc_error("snd_card_next"); + break; + } + } + + return -1; +} + +/* This function will find the best device-specific configuration file based + * on the sound card long name. + * Different devices may share the same sound driver and thus the same sound + * card name (short name), but they may still need different device-specific + * configurations. For user space to differentiate them, kernel drivers may + * include the DMI info (vendor, product and board) in the card long name. + * And user space can define configuration file names appending DMI keywords + * to the card name, like: + * bytcr-rt5640.ASUS.T100 + * bytcr-rt5651.MinnowBoard + * + * When being asked to load the configuration file for a card, this function + * will try to find the card long name from the local machine, and then scan + * all available configuration file names, search every field of the config + * file name in the card long name. The more characters match, the higher + * score the file has. Finally, the file with the highest score will be loaded. + */ +static void find_best_config_file(snd_use_case_mgr_t *mgr) +{ + const char *card_name = mgr->card_name; + char name[MAX_FILE]; + char *env = getenv(ALSA_CONFIG_UCM_VAR); + struct dirent **namelist; + char card_long_name[MAX_CARD_LONG_NAME]; + char *lpos, *lpos_next, *lname_end; + char *d_name, *dpos, *dpos_next, *d_name_end; + int d_name_len, field_len, long_name_len; + int i, cnt, err; + int score, score_max = 0, d_index = -1; + int is_first_field; + + if (get_card_long_name(mgr) < 0) { + /* cannot get long name, use card name as the conf file name */ + strcpy(mgr->conf_file_name, mgr->card_name); + return; + } + + strncpy(card_long_name, mgr->card_long_name, MAX_CARD_LONG_NAME); + trim_space(card_long_name); /* trim SPACE for matching */ + lname_end = card_long_name + strlen(card_long_name); + + /* get list of card directories under /usr/share/alsa/ucm */ + snprintf(name, sizeof(name)-1, + "%s", env ? env : ALSA_USE_CASE_DIR); + name[MAX_FILE-1] = '\0'; + + err = scandir(name, &namelist, filename_filter, alphasort); + if (err < 0) { + err = -errno; + uc_error("error: could not scan directory %s: %s", + name, strerror(-err)); + return; + } + cnt = err; + + /* scan the card directory names */ + for (i = 0; i < cnt; i++) { + + /* Skip the directories for component devices */ + if (is_component_directory(namelist[i]->d_name)) + continue; + + score = 0; + lpos = card_long_name; + + d_name = namelist[i]->d_name; + d_name_len = strlen(d_name); + d_name_end = d_name + d_name_len; + is_first_field = 1; + dpos = d_name; + + /* scan fields in the card directory name, separated by '.' */ + while (1) { + dpos_next = strchr(dpos, '.'); + if (dpos_next == dpos) /* start with '.' */ + goto next_field; + + if (!dpos_next) /* last field */ + field_len = d_name_end - dpos; + else + field_len = dpos_next - dpos; + + memcpy(name, dpos, field_len); + name[field_len] = '\0'; + + /* search the field in the given card long name */ + lpos_next = strstr(lpos, name); + if (!lpos_next) { + /* first field is the card name, must match */ + if (is_first_field) + break; + goto next_field; + } else { + /* The more characters match, + * the higher the score is. + */ + score += field_len; + if (lpos_next + field_len >= lname_end) + break; /* reach end of long name */ + + /* Skip the matched field, + * for searching next field. + */ + lpos = lpos_next + field_len; + } + +next_field: + if (!dpos_next) + break; /* no more fields */ + + dpos = dpos_next + 1; /* skip the separator '.' */ + is_first_field = 0; + if (dpos >= d_name_end) + break; + } + + if (score > score_max) { + score_max = score; + d_index = i; + } else if (score == 0 && score_max > 0) { + /* passed the card directories that can match, + * since directories are in alphabetical order. + */ + break; + } + } + + if (d_index >= 0) + strncpy(mgr->conf_file_name, namelist[d_index]->d_name, + MAX_CARD_LONG_NAME); + else + strncpy(mgr->conf_file_name, mgr->card_name, + MAX_CARD_LONG_NAME); +} + static int load_master_config(const char *card_name, snd_config_t **cfg) { char filename[MAX_FILE]; @@ -1355,7 +1562,9 @@ int uc_mgr_import_master_config(snd_use_case_mgr_t *uc_mgr) snd_config_t *cfg; int err; - err = load_master_config(uc_mgr->card_name, &cfg); + find_best_config_file(uc_mgr); + + err = load_master_config(uc_mgr->conf_file_name, &cfg); if (err < 0) return err; err = parse_master_file(uc_mgr, cfg); diff --git a/src/ucm/ucm_local.h b/src/ucm/ucm_local.h index 6d3302f..299a5b9 100644 --- a/src/ucm/ucm_local.h +++ b/src/ucm/ucm_local.h @@ -41,6 +41,7 @@ #include "use-case.h" #define MAX_FILE 256 +#define MAX_CARD_LONG_NAME 80 #define ALSA_USE_CASE_DIR ALSA_CONFIG_DIR "/ucm" #define SEQUENCE_ELEMENT_TYPE_CDEV 1 @@ -190,6 +191,8 @@ struct use_case_verb { */ struct snd_use_case_mgr { char *card_name; + char card_long_name[MAX_CARD_LONG_NAME]; + char conf_file_name[MAX_CARD_LONG_NAME]; char *comment; /* use case verb, devices and modifier configs parsed from files */ -- 2.7.4

5 11

[alsa-devel] [PATCH 0/2] ASoC: Use DMI name in sound card long name
by mengdong.lin＠linux.intel.com 22 Dec '16

22 Dec '16

From: Mengdong Lin <mengdong.lin(a)linux.intel.com> This series is based on Liam's idea to use DMI name as the sound card short name in the audio workshop. Now we add DMI info to the card long name: - Not change the card name (short name), keep it simple. So the card ID won't change as well. - Use short name and DMI name together to make a readable and unqique long name. Machine drivers can use a new API to get this feature. In user space, when the sound server ask UCM to open a sound card, UCM will try to find the best configuration file based on the card long name, and only if not avaiable, fallback to the default configuration file that matches the card name (short name). Please see patch: ucm: Automatically load the best config file based on the card long name History: v1 from RFC: Give up using a table to map DMI info to a simple string, since we cannot maintain such a table for each machine. Just do verbatim copy of DMI info and let user space decide how to use the DMI info in card long name to find the best UCM config file. If device driver adds a flavor string, just append it after the DMI info fields to the card long name. Liam Girdwood (1): ASoC: core: Add API to use DMI name in sound card long name Mengdong Lin (1): ASoC: Intel: Use DMI name for sound card long name in Broadwell machine driver include/sound/soc.h | 4 ++ sound/soc/intel/boards/broadwell.c | 2 + sound/soc/soc-core.c | 81 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 87 insertions(+) -- 2.5.0

5 8

[alsa-devel] [PATCH] Revert "ALSA: usb-audio: Fix race at stopping the stream"
by Takashi Iwai 21 Dec '16

21 Dec '16

This reverts commit 16200948d8353fe29a473a394d7d26790deae0e7. The commit was intended to cover the race condition, but it introduced yet another regression for devices with the implicit feedback, leading to a kernel panic due to NULL-dereference in an irq context. As the race condition that was addressed by the commit is very rare and the regression is much worse, let's revert the commit for rc1, and fix the issue properly in a later patch. Fixes: 16200948d835 ("ALSA: usb-audio: Fix race at stopping the stream") Reported-by: Ioan-Adrian Ratiu <adi(a)adirat.com> Cc: <stable(a)vger.kernel.org> Signed-off-by: Takashi Iwai <tiwai(a)suse.de> --- Linus, could you take this manually for rc1? I'm traveling and having a difficulty to set up a git pull request right now. Sorry for inconvenience. Thanks! Takashi sound/usb/endpoint.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c index a2cdf3370afe..15d1d5c63c3c 100644 --- a/sound/usb/endpoint.c +++ b/sound/usb/endpoint.c @@ -384,9 +384,6 @@ static void snd_complete_urb(struct urb *urb) if (unlikely(atomic_read(&ep->chip->shutdown))) goto exit_clear; - if (unlikely(!test_bit(EP_FLAG_RUNNING, &ep->flags))) - goto exit_clear; - if (usb_pipeout(ep->pipe)) { retire_outbound_urb(ep, ctx); /* can be stopped during retire callback */ @@ -537,11 +534,6 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep) alive, ep->ep_num); clear_bit(EP_FLAG_STOPPING, &ep->flags); - ep->data_subs = NULL; - ep->sync_slave = NULL; - ep->retire_data_urb = NULL; - ep->prepare_data_urb = NULL; - return 0; } @@ -1028,6 +1020,10 @@ void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep) if (--ep->use_count == 0) { deactivate_urbs(ep, false); + ep->data_subs = NULL; + ep->sync_slave = NULL; + ep->retire_data_urb = NULL; + ep->prepare_data_urb = NULL; set_bit(EP_FLAG_STOPPING, &ep->flags); } } -- 2.11.0

1 0

Re: [alsa-devel] [PATCH] ALSA: snd-usb: fix IRQ triggered NULL pointer dereference
by Takashi Iwai 21 Dec '16

21 Dec '16

On Tue, 20 Dec 2016 14:04:56 +0100, Takashi Sakamoto wrote: > > Hi, > > On Dec 20 2016 20:47, Ioan-Adrian Ratiu wrote: > > On Tue, 20 Dec 2016, Takashi Sakamoto <o-takashi(a)sakamocchi.jp> wrote: > >> --- > >> > >> Hi, > >> > >>> Commit 16200948d835 ("ALSA: usb-audio: Fix race at stopping the stream") > >>> fixes a race-codition but it also introduces another really nasty data race > >>> regression which makes my usb sound card [1] completely useless, throwing > >>> the kernel into a panic if anything from userspace tries to start playback. > >>> > >>> The problem is this: ep->data_subs is now set to NULL every time inside > >>> wait_clear_urbs(). ep->data_subs is initalized only in one place in > >>> start_endpoints(), then it is immediately wiped by the pre-existing call to > >>> wait_clear_urbs() inside snd_usb_endpoint_start(). > >>> > >>> To ilustrate, this is what happens in the non-irq part of the code: > >>> > >>> Step 1 (inside start_endpoints): ep->data_subs = subs; > >>> Step 2 (inside start_endpoints): snd_usb_endpoint_start(ep, can_sleep); > >>> Step 3 (inside snd_usb_endpoint_start): wait_clear_urbs(ep); > >>> Step 4 (inside wait_clear_urbs): ep->data_subs = NULL; > >>> > >>> Here's a simplified call stack for the IRQ part (full one at the end): > >>> > >>> (NULL dereference, param "subs" is passed NULL value of ep->data_subs) > >>> retire_playback_urb > >>> retire_outbound_urb > >>> snd_complete_urb > >>> (...) > >>> xhci_irq > >>> > >>> Looking at the git log there seems to be quite a lot of history in this > >>> part of the codebase, dating back to 2012 or earlier. My fix is based on > >>> 015618b90 ("ALSA: snd-usb: Fix URB cancellation at stream start") and > >>> e9ba389c5 ("ALSA: usb-audio: Fix scheduling-while-atomic bug in PCM capture > >>> stream") with a few modifications of my own. My idea is to do the > >>> ep->data_subs wiping before endpoint initialization in a manner similar > >>> to these older commits by using stop_endpoints() in snd_usb_pcm_prepare() > >>> and at the same time keep the ep->data_subs = NULL in wait_clear_urbs() to > >>> not trigger the recently fixed stream stopping race again. > >>> > >>> Full call stack: > >>> > >>> [ 131.093240] BUG: unable to handle kernel NULL pointer dereference at 0000000000000010 > >>> [ 131.094313] IP: retire_playback_urb+0x1b/0x160 [snd_usb_audio] > >>> [ 131.095046] PGD 0 > >>> [ 131.095047] > >>> [ 131.096509] Oops: 0000 [#1] PREEMPT SMP > >>> [ 131.097255] Modules linked in: fuse snd_usb_audio snd_usbmidi_lib snd_rawmidi snd_seq_device ctr ccm arc4 ath9k intel_rapl ath9k_common x86_pkg_temp_thermal ath9k_hw intel_powerclamp coretemp joydev mousedev ath kvm_intel mac80211 kvm > >>> input_leds hid_generic psmouse irqbypass usbhid hid crct10dif_pclmul crc32_pclmul serio_raw crc32c_intel atkbd ghash_clmulni_intel snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel pcbc aesni_intel libps2 cfg8 > >>> 0211 aes_x86_64 crypto_simd dell_laptop glue_helper r8169 dell_smbios snd_hda_codec sr_mod cryptd led_class mii rfkill snd_hwdep cdrom snd_hda_core ac dcdbas i8042 xhci_pci serio xhci_hcd snd_pcm tpm_tis battery tpm_tis_core lpc_ich tpm > >>> snd_timer evdev shpchp i2c_i801 sch_fq_codel > >>> [ 131.105551] CPU: 2 PID: 165 Comm: irq/29-xhci_hcd Not tainted 4.9.0-gd824cdc58ba0 #10 > >>> [ 131.107516] Hardware name: Dell Inc. Inspiron 3521/018DYG, BIOS A14 07/31/2015 > >>> [ 131.109592] task: ffff880154a70000 task.stack: ffffc90000f48000 > >>> [ 131.111746] RIP: 0010:retire_playback_urb+0x1b/0x160 [snd_usb_audio] > >>> [ 131.113899] RSP: 0018:ffffc90000f4bc10 EFLAGS: 00010082 > >>> [ 131.116080] RAX: ffffffffa04cabe0 RBX: 0000000000000000 RCX: 0000000000000000 > >>> [ 131.118284] RDX: 0000000000000000 RSI: ffff8801435a4c00 RDI: 0000000000000000 > >>> [ 131.120505] RBP: ffffc90000f4bc40 R08: 0000000000000001 R09: 0000000000000001 > >>> [ 131.122807] R10: 0000000000000001 R11: ffffffff82f60d6d R12: ffff8801432a0238 > >>> [ 131.125265] R13: ffff8801435a4c00 R14: 0000000000000000 R15: ffff8801535c83b8 > >>> [ 131.127723] FS: 0000000000000000(0000) GS:ffff88015a000000(0000) knlGS:0000000000000000 > >>> [ 131.130217] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > >>> [ 131.132568] CR2: 0000000000000010 CR3: 0000000001e0b000 CR4: 00000000001406e0 > >>> [ 131.135007] Call Trace: > >>> [ 131.137329] snd_complete_urb+0x190/0x2b0 [snd_usb_audio] > >>> [ 131.139526] __usb_hcd_giveback_urb+0x8e/0x160 > >>> [ 131.141948] usb_hcd_giveback_urb+0x43/0x110 > >>> [ 131.144104] xhci_giveback_urb_in_irq.isra.22+0x7d/0xb0 [xhci_hcd] > >>> [ 131.146521] finish_td.constprop.38+0x1de/0x2f0 [xhci_hcd] > >>> [ 131.148736] xhci_irq+0x13a2/0x1ca0 [xhci_hcd] > >>> [ 131.150972] ? trace_hardirqs_on+0xd/0x10 > >>> [ 131.153209] ? _raw_spin_unlock_irq+0x2c/0x50 > >>> [ 131.155390] ? irq_thread+0xb5/0x1d0 > >>> [ 131.157771] xhci_msi_irq+0x11/0x20 [xhci_hcd] > >>> [ 131.159938] irq_forced_thread_fn+0x2f/0x70 > >>> [ 131.162072] ? irq_thread+0xb5/0x1d0 > >>> [ 131.164141] irq_thread+0x149/0x1d0 > >>> [ 131.166132] ? irq_finalize_oneshot.part.2+0xe0/0xe0 > >>> [ 131.168142] ? wake_threads_waitq+0x30/0x30 > >>> [ 131.170149] kthread+0x10f/0x150 > >>> [ 131.172144] ? irq_thread_dtor+0xc0/0xc0 > >>> [ 131.174139] ? kthread_create_on_node+0x40/0x40 > >>> [ 131.176116] ret_from_fork+0x2a/0x40 > >>> [ 131.178074] Code: 8b 77 64 4c 89 e7 e8 e5 fe ff ff eb c3 0f 1f 00 0f 1f 44 00 00 55 31 d2 48 89 e5 41 57 41 56 41 55 41 54 53 48 89 fb 48 83 ec 08 <48> 8b 47 10 48 8b 4f 70 48 c7 c7 88 7b 4d a0 4c 8b a0 78 01 00 > >>> [ 131.182382] RIP: retire_playback_urb+0x1b/0x160 [snd_usb_audio] RSP: ffffc90000f4bc10 > >>> [ 131.184562] CR2: 0000000000000010 > >>> > >>> [1] 041e:3232 Creative Technology SoundBlaster X-FI HD > >>> [2] http://mailman.alsa-project.org/pipermail/alsa-devel/2016-December/115425.h… > >> > >> I can regenerate this bug with my EMU 0404 USB. My understanding of this bug is > >> quite similar to your perspective. This bug is quite critical. In my > >> understanding, we encounter this bug in all of cases in which snd-usb-audio is > >> applied for PCM playback substream. > >> > >> Below workaround can also suppress this bug. > >> > >> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > >> index a2cdf33..9b34f76 100644 > >> --- a/sound/usb/endpoint.c > >> +++ b/sound/usb/endpoint.c > >> @@ -537,11 +537,14 @@ static int wait_clear_urbs(struct snd_usb_endpoint *ep) > >> alive, ep->ep_num); > >> clear_bit(EP_FLAG_STOPPING, &ep->flags); > >> > >> - ep->data_subs = NULL; > >> ep->sync_slave = NULL; > >> ep->retire_data_urb = NULL; > >> ep->prepare_data_urb = NULL; > >> > >> + msleep(200); > >> + > >> + ep->data_subs = NULL; > >> + > >> return 0; > >> } > >> > >> If initialization of 'struct snd_usb_endpoint.data_subs' can be done after all > >> of queued URBs are processed and corresponding complete interrupts are cought, > >> we can solve this critical bug. > >> > >>> Signed-off-by: Ioan-Adrian Ratiu <adi(a)adirat.com> > >>> --- > >>> sound/usb/endpoint.c | 11 ++--------- > >>> sound/usb/endpoint.h | 2 +- > >>> sound/usb/pcm.c | 13 ++++++------- > >>> 3 files changed, 9 insertions(+), 17 deletions(-) > >>> > >>> diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > >>> index a2cdf3370afe..4465f324c2c2 100644 > >>> --- a/sound/usb/endpoint.c > >>> +++ b/sound/usb/endpoint.c > >>> @@ -920,9 +920,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, > >>> /** > >>> * snd_usb_endpoint_start: start an snd_usb_endpoint > >>> * > >>> - * @ep: the endpoint to start > >>> - * @can_sleep: flag indicating whether the operation is executed in > >>> - * non-atomic context > >>> + * @ep: the endpoint to start > >>> * > >>> * A call to this function will increment the use count of the endpoint. > >>> * In case it is not already running, the URBs for this endpoint will be > >>> @@ -932,7 +930,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, > >>> * > >>> * Returns an error if the URB submission failed, 0 in all other cases. > >>> */ > >>> -int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) > >>> +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep) > >>> { > >>> int err; > >>> unsigned int i; > >>> @@ -944,11 +942,6 @@ int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep) > >>> if (++ep->use_count != 1) > >>> return 0; > >>> > >>> - /* just to be sure */ > >>> - deactivate_urbs(ep, false); > >>> - if (can_sleep) > >>> - wait_clear_urbs(ep); > >>> - > >>> ep->active_mask = 0; > >>> ep->unlink_mask = 0; > >>> ep->phase = 0; > >>> diff --git a/sound/usb/endpoint.h b/sound/usb/endpoint.h > >>> index 6428392d8f62..584f295d7c77 100644 > >>> --- a/sound/usb/endpoint.h > >>> +++ b/sound/usb/endpoint.h > >>> @@ -18,7 +18,7 @@ int snd_usb_endpoint_set_params(struct snd_usb_endpoint *ep, > >>> struct audioformat *fmt, > >>> struct snd_usb_endpoint *sync_ep); > >>> > >>> -int snd_usb_endpoint_start(struct snd_usb_endpoint *ep, bool can_sleep); > >>> +int snd_usb_endpoint_start(struct snd_usb_endpoint *ep); > >>> void snd_usb_endpoint_stop(struct snd_usb_endpoint *ep); > >>> void snd_usb_endpoint_sync_pending_stop(struct snd_usb_endpoint *ep); > >>> int snd_usb_endpoint_activate(struct snd_usb_endpoint *ep); > >>> diff --git a/sound/usb/pcm.c b/sound/usb/pcm.c > >>> index 34c6d4f2c0b6..db26f767f851 100644 > >>> --- a/sound/usb/pcm.c > >>> +++ b/sound/usb/pcm.c > >>> @@ -218,7 +218,7 @@ int snd_usb_init_pitch(struct snd_usb_audio *chip, int iface, > >>> } > >>> } > >>> > >>> -static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) > >>> +static int start_endpoints(struct snd_usb_substream *subs) > >>> { > >>> int err; > >>> > >>> @@ -231,7 +231,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) > >>> dev_dbg(&subs->dev->dev, "Starting data EP @%p\n", ep); > >>> > >>> ep->data_subs = subs; > >>> - err = snd_usb_endpoint_start(ep, can_sleep); > >>> + err = snd_usb_endpoint_start(ep); > >>> if (err < 0) { > >>> clear_bit(SUBSTREAM_FLAG_DATA_EP_STARTED, &subs->flags); > >>> return err; > >>> @@ -260,7 +260,7 @@ static int start_endpoints(struct snd_usb_substream *subs, bool can_sleep) > >>> dev_dbg(&subs->dev->dev, "Starting sync EP @%p\n", ep); > >>> > >>> ep->sync_slave = subs->data_endpoint; > >>> - err = snd_usb_endpoint_start(ep, can_sleep); > >>> + err = snd_usb_endpoint_start(ep); > >>> if (err < 0) { > >>> clear_bit(SUBSTREAM_FLAG_SYNC_EP_STARTED, &subs->flags); > >>> return err; > >>> @@ -809,8 +809,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) > >>> goto unlock; > >>> } > >>> > >>> - snd_usb_endpoint_sync_pending_stop(subs->sync_endpoint); > >>> - snd_usb_endpoint_sync_pending_stop(subs->data_endpoint); > >>> + stop_endpoints(subs, true); > >>> > >>> ret = set_format(subs, subs->cur_audiofmt); > >>> if (ret < 0) > >>> @@ -850,7 +849,7 @@ static int snd_usb_pcm_prepare(struct snd_pcm_substream *substream) > >>> /* for playback, submit the URBs now; otherwise, the first hwptr_done > >>> * updates for all URBs would happen at the same time when starting */ > >>> if (subs->direction == SNDRV_PCM_STREAM_PLAYBACK) > >>> - ret = start_endpoints(subs, true); > >>> + return start_endpoints(subs); > >>> > >>> unlock: > >>> snd_usb_unlock_shutdown(subs->stream->chip); > >>> @@ -1666,7 +1665,7 @@ static int snd_usb_substream_capture_trigger(struct snd_pcm_substream *substream > >>> > >>> switch (cmd) { > >>> case SNDRV_PCM_TRIGGER_START: > >>> - err = start_endpoints(subs, false); > >>> + err = start_endpoints(subs); > >>> if (err < 0) > >>> return err; > >> > >> This patch works better, but not the best. It's a bit intrusive. > > > > I disagree. Being intrusive is not a good reason to reject a patch, > > especially if the alternative is to insert random magic number delays > > in hopes of synchronizing process and irq contexts to avoid critical > > errors. > > I apologize if you got some negative opinions in my previous message. > > My intention to show the ugly workaround is not to merge it instead of > yours, but clear what causes this bug. > > For example, we can also apply below patch for this bug, to solve it > roughly. > > diff --git a/sound/usb/endpoint.c b/sound/usb/endpoint.c > index a2cdf33..6ec4eb1 100644 > --- a/sound/usb/endpoint.c > +++ b/sound/usb/endpoint.c > @@ -162,7 +162,7 @@ int snd_usb_endpoint_next_packet_size(struct > snd_usb_endpoint *ep) > static void retire_outbound_urb(struct snd_usb_endpoint *ep, > struct snd_urb_ctx *urb_ctx) > { > - if (ep->retire_data_urb) > + if (ep->retire_data_urb && ep->data_subs) > ep->retire_data_urb(ep->data_subs, urb_ctx->urb); > } > > But this is just a workaround and can't fix it thoroughly. The main > issue is synchronization between interrupt/process contexts. > > > Please take the time to fully analyze my patch and let's have a > > discussion based on it, not reject it outright and replace it with > > a quick and dirty delay hack. > > OK. I'll deliberately check it again so that I have no overlooks. I > with this thread also catch the other developers enough helpful to > you. (I just eventually caught your patch in LKML and not developer > for this category of devices.) Sorry for the late reply, as I've been (still) off and had bad net connections. About your fix: Sakamoto-san is right, it's no good way to fix this kind or problem. The easiest option right now is just to revert my previous fix, as it obviously introduces another regression. The correct fix will be taken after that. I'm going to prepare a revert patch and ask Linus to take it before rc1. thanks, Takashi > > > Ionel > > > >> > >> What we need to synchronize process contexts to interrupt context till all of > >> queued URBs are handled. This seems to be a main purpose of wait_clear_bits, > >> while current implementation of snd_complete_urb()/wait_clear_bits() is not > >> enough. > >> > >> Thanks for your reporting. > >> > >> Takashi Sakamoto > >> -- > >> 2.9.3 > > > Thanks > > Takashi Sakamoto >

1 0