[alsa-devel] [PATCH 0/8] Fixes for HDSPM, especially AES(32)
Hi!
Florian Faber, Fredrik Lingvall and me have gone through an e-mail debug session during the past few weeks.
Here's our result containing several fixes for RME cards.
Cheers
Adrian Knoth (8): ALSA: hpdsm - RME AES(32): Fix missing channel mappings ALSA: hdspm - Fix buffer handling on RME MADI/MADIface/AES(32) ALSA: hdspm - Restrict channel count on RME AES/AES32 ALSA: hdspm - Fix DS/QS output channel mappings on RME MADI/MADIface ALSA: hdspm - Remove input selector on MADIface ALSA: hdspm - fix sync check on AES32 ALSA: hdspm - prevent reading unitialized stack memory ALSA: hdspm - Fix lock/sync reporting on MADI and AES32
sound/pci/rme9652/hdspm.c | 130 +++++++++++++++++++++++++++++++++++--------- 1 files changed, 103 insertions(+), 27 deletions(-)
On RME AES and AES(32), none of the required information (max_channels_in, max_channels_out, channel mappings, port names) was set, leading to the BUG below.
This patch adds the missing bits, thus fixing the bug.
125.058768] ------------[ cut here ]------------ [ 125.058773] WARNING: at sound/pci/rme9652/hdspm.c:5389 snd_hdspm_ioctl+0x10c/0x1d8 [snd_hdspm]() [ 125.058775] Hardware name: PRIMERGY RX100 S6 [ 125.058777] BUG? (info->channel >= hdspm->max_channels_out) [ 125.058778] Modules linked in: ipmi_watchdog ipmi_poweroff ipmi_si ipmi_devintf ipmi_msghandler snd_hdspm power_meter e1000e snd_rawmidi i2c_i801 [ 125.058787] Pid: 3652, comm: audacity Tainted: G W 2.6.36-gentoo-r5 #5 [ 125.058788] Call Trace: [ 125.058792] [<ffffffff8103db3a>] warn_slowpath_common+0x80/0x98 [ 125.058796] [<ffffffff8103dbe6>] warn_slowpath_fmt+0x41/0x43 [ 125.058800] [<ffffffffa006761a>] snd_hdspm_ioctl+0x10c/0x1d8 [snd_hdspm] [ 125.058803] [<ffffffff813fd626>] snd_pcm_channel_info+0x73/0x7c [ 125.058806] [<ffffffff814001e9>] snd_pcm_common_ioctl1+0x326/0xb01 [ 125.058809] [<ffffffff810c604c>] ? __do_fault+0x361/0x3a6 [ 125.058812] [<ffffffff81400e23>] snd_pcm_playback_ioctl1+0x20a/0x227 [ 125.058815] [<ffffffff811e599c>] ? file_has_perm+0x90/0x9e [ 125.058818] [<ffffffff81400e6a>] snd_pcm_playback_ioctl+0x2a/0x2e [ 125.058821] [<ffffffff810f2c69>] do_vfs_ioctl+0x404/0x453 [ 125.058824] [<ffffffff810f2d09>] sys_ioctl+0x51/0x74 [ 125.058827] [<ffffffff81002aab>] system_call_fastpath+0x16/0x1b [ 125.058830] ---[ end trace 5bddb08e5d4cbeb1 ]---
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de Signed-off-by: Florian Faber faber@faberman.de Signed-off-by: Fredrik Lingvall fredrik.lingvall@gmail.com
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 6de88b0..9258897 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -667,6 +667,12 @@ static char *texts_ports_aio_out_qs[] = { "Phone.L", "Phone.R" };
+static char *texts_ports_aes32[] = { + "AES.1", "AES.2", "AES.3", "AES.4", "AES.5", "AES.6", "AES.7", + "AES.8", "AES.9.", "AES.10", "AES.11", "AES.12", "AES.13", "AES.14", + "AES.15", "AES.16" +}; + /* These tables map the ALSA channels 1..N to the channels that we need to use in order to find the relevant channel buffer. RME refers to this kind of mapping as between "the ADAT channel and @@ -816,6 +822,17 @@ static char channel_map_aio_out_qs[HDSPM_MAX_CHANNELS] = { -1, -1, -1, -1, -1, -1, -1, -1 };
+static char channel_map_aes32[HDSPM_MAX_CHANNELS] = { + 0, 1, 2, 3, 4, 5, 6, 7, + 8, 9, 10, 11, 12, 13, 14, 15, + -1, -1, -1, -1, -1, -1, -1, -1, + -1, -1, -1, -1, -1, -1, -1, -1, + -1, -1, -1, -1, -1, -1, -1, -1, + -1, -1, -1, -1, -1, -1, -1, -1, + -1, -1, -1, -1, -1, -1, -1, -1, + -1, -1, -1, -1, -1, -1, -1, -1 +}; + struct hdspm_midi { struct hdspm *hdspm; int id; @@ -6396,6 +6413,29 @@ static int __devinit snd_hdspm_create(struct snd_card *card,
switch (hdspm->io_type) { case AES32: + hdspm->ss_in_channels = hdspm->ss_out_channels = 16; + hdspm->ds_in_channels = hdspm->ds_out_channels = 16; + hdspm->qs_in_channels = hdspm->qs_out_channels = 16; + + hdspm->channel_map_in_ss = hdspm->channel_map_out_ss = + channel_map_aes32; + hdspm->channel_map_in_ds = hdspm->channel_map_out_ds = + channel_map_aes32; + hdspm->channel_map_in_qs = hdspm->channel_map_out_qs = + channel_map_aes32; + hdspm->port_names_in_ss = hdspm->port_names_out_ss = + texts_ports_aes32; + hdspm->port_names_in_ds = hdspm->port_names_out_ds = + texts_ports_aes32; + hdspm->port_names_in_qs = hdspm->port_names_out_qs = + texts_ports_aes32; + + hdspm->max_channels_out = hdspm->max_channels_in = 16; + hdspm->port_names_in = hdspm->port_names_out = + texts_ports_aes32; + hdspm->channel_map_in = hdspm->channel_map_out = + channel_map_aes32; + break;
case MADI:
Only RayDAT and AIO provide sane buffer pointers that can be used with HDSPM_BufferPositionMask, on all other cards, this would result in a wrong HW pointer leading to xruns and these messages:
[260808.916788] BUG: pcmC0D0p:0, pos = 2976, buffer size = 1024, period size = 512 [260808.961124] BUG: pcmC0D0c:0, pos = 4944, buffer size = 1024, period size = 512
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 9258897..509a35a 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -1231,8 +1231,17 @@ static snd_pcm_uframes_t hdspm_hw_pointer(struct hdspm *hdspm) int position;
position = hdspm_read(hdspm, HDSPM_statusRegister); - position &= HDSPM_BufferPositionMask; - position /= 4; /* Bytes per sample */ + + switch (hdspm->io_type) { + case RayDAT: + case AIO: + position &= HDSPM_BufferPositionMask; + position /= 4; /* Bytes per sample */ + break; + default: + position = (position & HDSPM_BufferID) ? + (hdspm->period_bytes / 4) : 0; + }
return position; }
Without calling an appropriate rule, AES/AES32 cards would announce a theoretical channel count of 64 (HDSPM_MAX_CHANNELS), leading to the already known bug:
[37422.640481] ------------[ cut here ]------------ [37422.640487] WARNING: at sound/pci/rme9652/hdspm.c:5449 snd_hdspm_ioctl+0x18f/0x202 [snd_hdspm]() [37422.640489] Hardware name: PRIMERGY RX100 S6 [37422.640490] BUG? (info->channel >= hdspm->max_channels_in) [37422.640492] Modules linked in: snd_hdspm snd_seq_midi ipmi_watchdog ipmi_poweroff ipmi_si ipmi_devintf ipmi_msghandler i2c_i801 e1000e snd_rawmidi power_meter [last unloaded: snd_hdspm] [37422.640501] Pid: 22231, comm: jackd Tainted: G D W 2.6.36-gentoo-r5 #5 [37422.640502] Call Trace: [37422.640508] [<ffffffff8103db3a>] warn_slowpath_common+0x80/0x98 [37422.640511] [<ffffffff8103dbe6>] warn_slowpath_fmt+0x41/0x43 [37422.640514] [<ffffffff81034306>] ? get_parent_ip+0x11/0x42 [37422.640518] [<ffffffffa0055763>] snd_hdspm_ioctl+0x18f/0x202 [snd_hdspm] [37422.640522] [<ffffffff813fd626>] snd_pcm_channel_info+0x73/0x7c [37422.640525] [<ffffffff814001e9>] snd_pcm_common_ioctl1+0x326/0xb01 [37422.640527] [<ffffffff81034306>] ? get_parent_ip+0x11/0x42 [37422.640531] [<ffffffff8105be6c>] ? __srcu_read_unlock+0x3b/0x59 [37422.640533] [<ffffffff81400bce>] snd_pcm_capture_ioctl1+0x20a/0x227 [37422.640537] [<ffffffff811e599c>] ? file_has_perm+0x90/0x9e [37422.640540] [<ffffffff81400c15>] snd_pcm_capture_ioctl+0x2a/0x2e [37422.640543] [<ffffffff810f2c69>] do_vfs_ioctl+0x404/0x453 [37422.640546] [<ffffffff810f2d09>] sys_ioctl+0x51/0x74 [37422.640549] [<ffffffff81002aab>] system_call_fastpath+0x16/0x1b [37422.640552] ---[ end trace 0cd919cd68118082 ]---
We already have all the right values in place, we simply have to inform the upper layers about this restriction.
Note that snd_hdspm_hw_rule_rate_out_channels and snd_hdspm_hw_rule_rate_in_channels must not be called on AES32, because the channel count is always 16, no matter of the samplerate in use.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 509a35a..17939b9 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -5836,17 +5836,19 @@ static int snd_hdspm_playback_open(struct snd_pcm_substream *substream) snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &hdspm_hw_constraints_aes32_sample_rates); } else { - snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, - snd_hdspm_hw_rule_out_channels, hdspm, - SNDRV_PCM_HW_PARAM_CHANNELS, -1); - snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, - snd_hdspm_hw_rule_out_channels_rate, hdspm, - SNDRV_PCM_HW_PARAM_RATE, -1); - snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, snd_hdspm_hw_rule_rate_out_channels, hdspm, SNDRV_PCM_HW_PARAM_CHANNELS, -1); } + + snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, + snd_hdspm_hw_rule_out_channels, hdspm, + SNDRV_PCM_HW_PARAM_CHANNELS, -1); + + snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, + snd_hdspm_hw_rule_out_channels_rate, hdspm, + SNDRV_PCM_HW_PARAM_RATE, -1); + return 0; }
@@ -5904,17 +5906,19 @@ static int snd_hdspm_capture_open(struct snd_pcm_substream *substream) snd_pcm_hw_constraint_list(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, &hdspm_hw_constraints_aes32_sample_rates); } else { - snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, - snd_hdspm_hw_rule_in_channels, hdspm, - SNDRV_PCM_HW_PARAM_CHANNELS, -1); - snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, - snd_hdspm_hw_rule_in_channels_rate, hdspm, - SNDRV_PCM_HW_PARAM_RATE, -1); - snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_RATE, - snd_hdspm_hw_rule_rate_in_channels, hdspm, - SNDRV_PCM_HW_PARAM_CHANNELS, -1); + snd_hdspm_hw_rule_rate_in_channels, hdspm, + SNDRV_PCM_HW_PARAM_CHANNELS, -1); } + + snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, + snd_hdspm_hw_rule_in_channels, hdspm, + SNDRV_PCM_HW_PARAM_CHANNELS, -1); + + snd_pcm_hw_rule_add(runtime, 0, SNDRV_PCM_HW_PARAM_CHANNELS, + snd_hdspm_hw_rule_in_channels_rate, hdspm, + SNDRV_PCM_HW_PARAM_RATE, -1); + return 0; }
Caused by two typos, no output channel mappings were assigned for MADI/MADIface at double/quad speed.
The channel mapping is indeed identical to the single speed mapping, the cards will simply use the first N channels.
Signed-off-by: Florian Faber faber@faberman.de Signed-off-by: Fredrik Lingvall fredrik.lingvall@gmail.com Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 17939b9..7246302 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6462,9 +6462,9 @@ static int __devinit snd_hdspm_create(struct snd_card *card,
hdspm->channel_map_in_ss = hdspm->channel_map_out_ss = channel_map_unity_ss; - hdspm->channel_map_in_ds = hdspm->channel_map_out_ss = + hdspm->channel_map_in_ds = hdspm->channel_map_out_ds = channel_map_unity_ss; - hdspm->channel_map_in_qs = hdspm->channel_map_out_ss = + hdspm->channel_map_in_qs = hdspm->channel_map_out_qs = channel_map_unity_ss;
hdspm->port_names_in_ss = hdspm->port_names_out_ss =
In contrast to the RME MADI card, coax/optical selection on the MADIface is done via a physical switch located at the breakout box. Obviously, the driver cannot switch ports in software.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 7246302..ea49ffe 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -4272,8 +4272,7 @@ static struct snd_kcontrol_new snd_hdspm_controls_madiface[] = { HDSPM_SYNC_CHECK("MADI SyncCheck", 0), HDSPM_TX_64("TX 64 channels mode", 0), HDSPM_C_TMS("Clear Track Marker", 0), - HDSPM_SAFE_MODE("Safe Mode", 0), - HDSPM_INPUT_SELECT("Input Select", 0), + HDSPM_SAFE_MODE("Safe Mode", 0) };
static struct snd_kcontrol_new snd_hdspm_controls_aio[] = {
Fredrik Lingvall fredrik.lingvall@gmail.com has discovered wrong frequency and sync detection on AES32. According to him, the provided patch fixes these issues.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index ea49ffe..0b0293f 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -1082,7 +1082,7 @@ static int hdspm_external_sample_rate(struct hdspm *hdspm) case AES32: status2 = hdspm_read(hdspm, HDSPM_statusRegister2); status = hdspm_read(hdspm, HDSPM_statusRegister); - timecode = hdspm_read(hdspm, HDSPM_timecodeRegister); + timecode = hdspm_read(hdspm, HDSPM_timecodeRegister);
syncref = hdspm_autosync_ref(hdspm);
@@ -2115,6 +2115,29 @@ static int snd_hdspm_get_autosync_sample_rate(struct snd_kcontrol *kcontrol, hdspm_get_s1_sample_rate(hdspm, ucontrol->id.index-1); } + + case AES32: + + switch (kcontrol->private_value) { + case 0: /* WC */ + ucontrol->value.enumerated.item[0] = + hdspm_get_wc_sample_rate(hdspm); + break; + case 9: /* TCO */ + ucontrol->value.enumerated.item[0] = + hdspm_get_tco_sample_rate(hdspm); + break; + case 10: /* SYNC_IN */ + ucontrol->value.enumerated.item[0] = + hdspm_get_sync_in_sample_rate(hdspm); + break; + default: /* AES1 to AES8 */ + ucontrol->value.enumerated.item[0] = + hdspm_get_s1_sample_rate(hdspm, + kcontrol->private_value-1); + break; + + } default: break; } @@ -3803,9 +3826,9 @@ static int snd_hdspm_get_sync_check(struct snd_kcontrol *kcontrol, val = hdspm_tco_sync_check(hdspm); break; case 10 /* SYNC IN */: val = hdspm_sync_in_sync_check(hdspm); break; - default: + default: /* AES1 to AES8 */ val = hdspm_aes_sync_check(hdspm, - ucontrol->id.index-1); + kcontrol->private_value-1); }
}
Original patch by Dan Rosenberg drosenberg@vsecurity.com under commit e68d3b316ab7b02a074edc4f770e6a746390cb7d. I'm copying his text here:
The SNDRV_HDSPM_IOCTL_GET_CONFIG_INFO ioctl in hdspm.c allow unprivileged users to read uninitialized kernel stack memory, because several fields of the hdspm_config struct declared on the stack are not altered or zeroed before being copied back to the user. This patch takes care of it.
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index 0b0293f..ced1406 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -6081,6 +6081,7 @@ static int snd_hdspm_hwdep_ioctl(struct snd_hwdep *hw, struct file *file,
case SNDRV_HDSPM_IOCTL_GET_CONFIG:
+ memset(&info, 0, sizeof(info)); spin_lock_irq(&hdspm->lock); info.pref_sync_ref = hdspm_pref_sync_ref(hdspm); info.wordclock_sync_check = hdspm_wc_sync_check(hdspm);
Signed-off-by: Adrian Knoth adi@drcomp.erfurt.thur.de
diff --git a/sound/pci/rme9652/hdspm.c b/sound/pci/rme9652/hdspm.c index ced1406..5dbf620 100644 --- a/sound/pci/rme9652/hdspm.c +++ b/sound/pci/rme9652/hdspm.c @@ -3700,8 +3700,8 @@ static int hdspm_sync_in_sync_check(struct hdspm *hdspm) case MADI: case AES32: status = hdspm_read(hdspm, HDSPM_statusRegister2); - lock = (status & 0x400000) ? 1 : 0; - sync = (status & 0x800000) ? 1 : 0; + lock = (status & HDSPM_syncInLock) ? 1 : 0; + sync = (status & HDSPM_syncInSync) ? 1 : 0; break;
case MADIface:
At Wed, 23 Feb 2011 11:43:07 +0100, Adrian Knoth wrote:
Hi!
Florian Faber, Fredrik Lingvall and me have gone through an e-mail debug session during the past few weeks.
Here's our result containing several fixes for RME cards.
Thanks, I applied these now to sound git tree.
Takashi
Cheers
Adrian Knoth (8): ALSA: hpdsm - RME AES(32): Fix missing channel mappings ALSA: hdspm - Fix buffer handling on RME MADI/MADIface/AES(32) ALSA: hdspm - Restrict channel count on RME AES/AES32 ALSA: hdspm - Fix DS/QS output channel mappings on RME MADI/MADIface ALSA: hdspm - Remove input selector on MADIface ALSA: hdspm - fix sync check on AES32 ALSA: hdspm - prevent reading unitialized stack memory ALSA: hdspm - Fix lock/sync reporting on MADI and AES32
sound/pci/rme9652/hdspm.c | 130 +++++++++++++++++++++++++++++++++++--------- 1 files changed, 103 insertions(+), 27 deletions(-)
-- 1.7.2.3
participants (2)
-
Adrian Knoth -
Takashi Iwai