NetBSD Problem Report #52437
From www@NetBSD.org Fri Jul 28 05:29:32 2017
Return-Path: <www@NetBSD.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
(using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits))
(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (verified OK))
by mollari.NetBSD.org (Postfix) with ESMTPS id CE6127A183
for <gnats-bugs@gnats.NetBSD.org>; Fri, 28 Jul 2017 05:29:32 +0000 (UTC)
Message-Id: <20170728052722.E946C7A290@mollari.NetBSD.org>
Date: Fri, 28 Jul 2017 05:27:22 +0000 (UTC)
From: isaki@NetBSD.org
Reply-To: isaki@NetBSD.org
To: gnats-bugs@NetBSD.org
Subject: Refactoring audio_set_vchan_defaults()
X-Send-Pr-Version: www-1.0
>Number: 52437
>Category: kern
>Synopsis: Refactoring audio_set_vchan_defaults()
>Confidential: no
>Severity: non-critical
>Priority: low
>Responsible: kern-bug-people
>State: closed
>Class: sw-bug
>Submitter-Id: net
>Arrival-Date: Fri Jul 28 05:30:00 +0000 2017
>Closed-Date: Sat Jul 29 03:13:16 +0000 2017
>Last-Modified: Mon Apr 16 14:15:02 +0000 2018
>Originator: Tetsuya Isaki
>Release: NetBSD-current
>Organization:
NetBSD
>Environment:
source code review
>Description:
Refactoring audio_set_vchan_defaults()
audio_set_vchan_defaults() in audio.c is too complicated.
1)
audio_set_vchan_defaults()' 3rd argument
''const struct audio_format *format'' looks like read-only input parameter
because it has const qualifier.
And where audio_set_vchan_defaults() is called, all 3rd arguments are
''&sc->sc_format[0]''.
But audio_set_vchan_defaults() also takes audio_softc *sc (as its 1st
argument) and updates sc->sc_format[0] !
It's bad design.
2)
What is the difference between
sc->sc_{channels, precision, frequency} and
sc->sc_vchan_params.{channels, precision, sample_rate} ?
They look the same.
I wrote a patch to solve these two problems.
It seems to work for my poor environment but I don't know details.
Please review it. Thanks.
Index: sys/dev/audio.c
===================================================================
RCS file: /cvsroot/src/sys/dev/audio.c,v
retrieving revision 1.375
diff -u -r1.375 audio.c
--- sys/dev/audio.c 28 Jul 2017 03:58:54 -0000 1.375
+++ sys/dev/audio.c 28 Jul 2017 04:54:04 -0000
@@ -387,8 +387,7 @@
struct virtual_channel *);
static int
audio_query_encoding(struct audio_softc *, struct audio_encoding *);
-static int audio_set_vchan_defaults
- (struct audio_softc *, u_int, const struct audio_format *);
+static int audio_set_vchan_defaults(struct audio_softc *, u_int);
static int vchan_autoconfig(struct audio_softc *);
int au_get_lr_value(struct audio_softc *, mixer_ctrl_t *, int *, int *);
int au_set_lr_value(struct audio_softc *, mixer_ctrl_t *, int, int);
@@ -509,16 +508,6 @@
sc->sc_format[0].frequency_type = 1;
sc->sc_format[0].frequency[0] = 44100;
- sc->sc_vchan_params.sample_rate = 44100;
-#if BYTE_ORDER == LITTLE_ENDIAN
- sc->sc_vchan_params.encoding = AUDIO_ENCODING_SLINEAR_LE;
-#else
- sc->sc_vchan_params.encoding = AUDIO_ENCODING_SLINEAR_BE;
-#endif
- sc->sc_vchan_params.precision = 16;
- sc->sc_vchan_params.validbits = 16;
- sc->sc_vchan_params.channels = 2;
-
sc->sc_trigger_started = false;
sc->sc_rec_started = false;
sc->sc_dying = false;
@@ -535,9 +524,6 @@
vc->sc_lastinfovalid = false;
vc->sc_swvol = 255;
vc->sc_recswvol = 255;
- sc->sc_frequency = 44100;
- sc->sc_precision = 16;
- sc->sc_channels = 2;
if (auconv_create_encodings(sc->sc_format, VAUDIO_NFORMATS,
&sc->sc_encodings) != 0) {
@@ -4138,9 +4124,11 @@
return 0;
}
+/*
+ * set some parameters from sc->sc_vchan_params.
+ */
static int
-audio_set_vchan_defaults(struct audio_softc *sc, u_int mode,
- const struct audio_format *format)
+audio_set_vchan_defaults(struct audio_softc *sc, u_int mode)
{
struct audio_chan *chan;
struct virtual_channel *vc;
@@ -4154,38 +4142,30 @@
return EINVAL;
vc = chan->vc;
- sc->sc_vchan_params.sample_rate = sc->sc_frequency;
-#if BYTE_ORDER == LITTLE_ENDIAN
- sc->sc_vchan_params.encoding = AUDIO_ENCODING_SLINEAR_LE;
-#else
- sc->sc_vchan_params.encoding = AUDIO_ENCODING_SLINEAR_BE;
-#endif
- sc->sc_vchan_params.precision = sc->sc_precision;
- sc->sc_vchan_params.validbits = sc->sc_precision;
- sc->sc_vchan_params.channels = sc->sc_channels;
-
/* default parameters */
vc->sc_rparams = sc->sc_vchan_params;
vc->sc_pparams = sc->sc_vchan_params;
vc->sc_blkset = false;
AUDIO_INITINFO(&ai);
- ai.record.sample_rate = sc->sc_frequency;
- ai.record.encoding = format->encoding;
- ai.record.channels = sc->sc_channels;
- ai.record.precision = sc->sc_precision;
+ ai.record.sample_rate = sc->sc_vchan_params.sample_rate;
+ ai.record.encoding = sc->sc_vchan_params.encoding;
+ ai.record.channels = sc->sc_vchan_params.channels;
+ ai.record.precision = sc->sc_vchan_params.precision;
ai.record.pause = false;
- ai.play.sample_rate = sc->sc_frequency;
- ai.play.encoding = format->encoding;
- ai.play.channels = sc->sc_channels;
- ai.play.precision = sc->sc_precision;
+ ai.play.sample_rate = sc->sc_vchan_params.sample_rate;
+ ai.play.encoding = sc->sc_vchan_params.encoding;
+ ai.play.channels = sc->sc_vchan_params.channels;
+ ai.play.precision = sc->sc_vchan_params.precision;
ai.play.pause = false;
ai.mode = mode;
- sc->sc_format[0].channels = sc->sc_channels;
- sc->sc_format[0].precision = sc->sc_precision;
- sc->sc_format[0].validbits = sc->sc_precision;
- sc->sc_format[0].frequency[0] = sc->sc_frequency;
+ sc->sc_format[0].encoding = sc->sc_vchan_params.encoding;
+ sc->sc_format[0].channels = sc->sc_vchan_params.channels;
+ sc->sc_format[0].precision = sc->sc_vchan_params.precision;
+ sc->sc_format[0].validbits = sc->sc_vchan_params.precision;
+ sc->sc_format[0].frequency_type = 1;
+ sc->sc_format[0].frequency[0] = sc->sc_vchan_params.sample_rate;
auconv_delete_encodings(sc->sc_encodings);
error = auconv_create_encodings(sc->sc_format, VAUDIO_NFORMATS,
@@ -5339,8 +5319,8 @@
sc->sc_trigger_started = false;
sc->sc_rec_started = false;
- audio_set_vchan_defaults(sc, AUMODE_PLAY | AUMODE_PLAY_ALL |
- AUMODE_RECORD, &sc->sc_format[0]);
+ audio_set_vchan_defaults(sc,
+ AUMODE_PLAY | AUMODE_PLAY_ALL | AUMODE_RECORD);
audio_mixer_restore(sc);
SIMPLEQ_FOREACH(chan, &sc->sc_audiochan, entries) {
@@ -5667,7 +5647,7 @@
mix_func(struct audio_softc *sc, struct audio_ringbuffer *cb,
struct virtual_channel *vc)
{
- switch (sc->sc_precision) {
+ switch (sc->sc_vchan_params.precision) {
case 8:
mix_func8(sc, cb, vc);
break;
@@ -5719,7 +5699,7 @@
recswvol_func(struct audio_softc *sc, struct audio_ringbuffer *cb,
size_t blksize, struct virtual_channel *vc)
{
- switch (sc->sc_precision) {
+ switch (sc->sc_vchan_params.precision) {
case 8:
recswvol_func8(sc, cb, blksize, vc);
break;
@@ -5854,7 +5834,7 @@
if (vc == chan->vc && sc->hw_if->set_params != NULL) {
sc->sc_ready = true;
- if (sc->sc_precision == 8)
+ if (sc->sc_vchan_params.precision == 8)
play->encoding = rec->encoding = AUDIO_ENCODING_SLINEAR;
error = sc->hw_if->set_params(sc->hw_hdl, setmode, usemode,
play, rec, pfil, rfil);
@@ -5943,7 +5923,7 @@
node = *rnode;
sc = node.sysctl_data;
- t = sc->sc_frequency;
+ t = sc->sc_vchan_params.sample_rate;
node.sysctl_data = &t;
error = sysctl_lookup(SYSCTLFN_CALL(&node));
if (error || newp == NULL)
@@ -5962,9 +5942,9 @@
return EINVAL;
}
- sc->sc_frequency = t;
- error = audio_set_vchan_defaults(sc, AUMODE_PLAY | AUMODE_PLAY_ALL
- | AUMODE_RECORD, &sc->sc_format[0]);
+ sc->sc_vchan_params.sample_rate = t;
+ error = audio_set_vchan_defaults(sc,
+ AUMODE_PLAY | AUMODE_PLAY_ALL | AUMODE_RECORD);
if (error)
aprint_error_dev(sc->sc_dev, "Error setting frequency, "
"please check hardware capabilities\n");
@@ -5984,7 +5964,7 @@
node = *rnode;
sc = node.sysctl_data;
- t = sc->sc_precision;
+ t = sc->sc_vchan_params.precision;
node.sysctl_data = &t;
error = sysctl_lookup(SYSCTLFN_CALL(&node));
if (error || newp == NULL)
@@ -6003,20 +5983,20 @@
return EINVAL;
}
- sc->sc_precision = t;
+ sc->sc_vchan_params.precision = t;
- if (sc->sc_precision != 8) {
- sc->sc_format[0].encoding =
+ if (sc->sc_vchan_params.precision != 8) {
+ sc->sc_vchan_params.encoding =
#if BYTE_ORDER == LITTLE_ENDIAN
AUDIO_ENCODING_SLINEAR_LE;
#else
AUDIO_ENCODING_SLINEAR_BE;
#endif
} else
- sc->sc_format[0].encoding = AUDIO_ENCODING_SLINEAR_LE;
+ sc->sc_vchan_params.encoding = AUDIO_ENCODING_SLINEAR_LE;
- error = audio_set_vchan_defaults(sc, AUMODE_PLAY | AUMODE_PLAY_ALL
- | AUMODE_RECORD, &sc->sc_format[0]);
+ error = audio_set_vchan_defaults(sc,
+ AUMODE_PLAY | AUMODE_PLAY_ALL | AUMODE_RECORD);
if (error)
aprint_error_dev(sc->sc_dev, "Error setting precision, "
"please check hardware capabilities\n");
@@ -6036,7 +6016,7 @@
node = *rnode;
sc = node.sysctl_data;
- t = sc->sc_channels;
+ t = sc->sc_vchan_params.channels;
node.sysctl_data = &t;
error = sysctl_lookup(SYSCTLFN_CALL(&node));
if (error || newp == NULL)
@@ -6055,9 +6035,9 @@
return EINVAL;
}
- sc->sc_channels = t;
- error = audio_set_vchan_defaults(sc, AUMODE_PLAY | AUMODE_PLAY_ALL
- | AUMODE_RECORD, &sc->sc_format[0]);
+ sc->sc_vchan_params.channels = t;
+ error = audio_set_vchan_defaults(sc,
+ AUMODE_PLAY | AUMODE_PLAY_ALL | AUMODE_RECORD);
if (error)
aprint_error_dev(sc->sc_dev, "Error setting channels, "
"please check hardware capabilities\n");
@@ -6079,24 +6059,32 @@
mutex_enter(sc->sc_lock);
+#if BYTE_ORDER == LITTLE_ENDIAN
+ sc->sc_vchan_params.encoding = AUDIO_ENCODING_SLINEAR_LE;
+#else
+ sc->sc_vchan_params.encoding = AUDIO_ENCODING_SLINEAR_BE;
+#endif
+
for (i = 0; i < __arraycount(auto_config_precision); i++) {
- sc->sc_precision = auto_config_precision[i];
+ sc->sc_vchan_params.precision = auto_config_precision[i];
+ sc->sc_vchan_params.validbits = auto_config_precision[i];
for (j = 0; j < __arraycount(auto_config_channels); j++) {
- sc->sc_channels = auto_config_channels[j];
+ sc->sc_vchan_params.channels = auto_config_channels[j];
for (k = 0; k < __arraycount(auto_config_freq); k++) {
- sc->sc_frequency = auto_config_freq[k];
+ sc->sc_vchan_params.sample_rate =
+ auto_config_freq[k];
error = audio_set_vchan_defaults(sc,
AUMODE_PLAY | AUMODE_PLAY_ALL |
- AUMODE_RECORD, &sc->sc_format[0]);
+ AUMODE_RECORD);
if (vc->sc_npfilters > 0 &&
(vc->sc_mpr.s.param.precision !=
- sc->sc_precision ||
+ sc->sc_vchan_params.precision ||
vc->sc_mpr.s.param.validbits !=
- sc->sc_precision ||
- vc->sc_mpr.s.param.
- sample_rate != sc->sc_frequency ||
- vc->sc_mpr.s.param.
- channels != sc->sc_channels))
+ sc->sc_vchan_params.precision ||
+ vc->sc_mpr.s.param.sample_rate !=
+ sc->sc_vchan_params.sample_rate ||
+ vc->sc_mpr.s.param.channels !=
+ sc->sc_vchan_params.channels))
error = EINVAL;
if (error == 0) {
@@ -6104,8 +6092,9 @@
"Virtual format configured - "
"Format SLINEAR, precision %d, "
"channels %d, frequency %d\n",
- sc->sc_precision, sc->sc_channels,
- sc->sc_frequency);
+ sc->sc_vchan_params.precision,
+ sc->sc_vchan_params.channels,
+ sc->sc_vchan_params.sample_rate);
mutex_exit(sc->sc_lock);
return 0;
Index: sys/dev/audiovar.h
===================================================================
RCS file: /cvsroot/src/sys/dev/audiovar.h,v
retrieving revision 1.58
diff -u -r1.58 audiovar.h
--- sys/dev/audiovar.h 27 Jul 2017 08:42:47 -0000 1.58
+++ sys/dev/audiovar.h 28 Jul 2017 04:54:04 -0000
@@ -289,9 +289,6 @@
/* These are changeable by sysctl to set the vchan common format */
struct sysctllog *sc_log; /* sysctl log */
- int sc_channels;
- int sc_precision;
- int sc_frequency;
struct audio_info sc_ai; /* Recent info for dev sound */
bool sc_aivalid;
#define VAUDIO_NFORMATS 1
>How-To-Repeat:
see source code.
>Fix:
N/A
>Release-Note:
>Audit-Trail:
From: Nathanial Sloss <nat@netbsd.org>
To: gnats-bugs@netbsd.org
Cc:
Subject: Re: kern/52437: Refactoring audio_set_vchan_defaults()
Date: Fri, 28 Jul 2017 21:34:35 +1000
Hi,
Looks good to me. Please commit it.
Best regards,
Nat
From: "Tetsuya Isaki" <isaki@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52437 CVS commit: src/sys/dev
Date: Sat, 29 Jul 2017 03:05:51 +0000
Module Name: src
Committed By: isaki
Date: Sat Jul 29 03:05:51 UTC 2017
Modified Files:
src/sys/dev: audio.c audiovar.h
Log Message:
Improve audio_set_vchan_defaults().
- Correct confused input/output parameters.
- Remove sc->{sc_channels, sc_precision, sc_frequency}. They are
the same as sc->sc_vchan_params.{channels, precision, sample_rate}.
The input parameter of audio_set_vchan_defaults() is now only
sc->sc_vchan_params.
Fix PR kern/52437
To generate a diff of this commit:
cvs rdiff -u -r1.375 -r1.376 src/sys/dev/audio.c
cvs rdiff -u -r1.58 -r1.59 src/sys/dev/audiovar.h
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
State-Changed-From-To: open->closed
State-Changed-By: isaki@NetBSD.org
State-Changed-When: Sat, 29 Jul 2017 03:13:16 +0000
State-Changed-Why:
Commited. Thank you for review, nat@.
From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc:
Subject: PR/52437 CVS commit: [netbsd-8] src/sys/dev
Date: Mon, 16 Apr 2018 14:11:44 +0000
Module Name: src
Committed By: martin
Date: Mon Apr 16 14:11:44 UTC 2018
Modified Files:
src/sys/dev [netbsd-8]: auconv.c audio.c audiovar.h aurateconv.c
Log Message:
Pull up following revision(s) (requested by nat in ticket #759):
sys/dev/audio.c: revision 1.367
sys/dev/audio.c: revision 1.369
sys/dev/audio.c: revision 1.376 (patch)
sys/dev/audio.c: revision 1.377 (patch)
sys/dev/audio.c: revision 1.378
sys/dev/audio.c: revision 1.382 (patch)
sys/dev/audio.c: revision 1.383 (patch)
sys/dev/audio.c: revision 1.384
sys/dev/audio.c: revision 1.388
sys/dev/audio.c: revision 1.389
sys/dev/audio.c: revision 1.395
sys/dev/audio.c: revision 1.396 (patch)
sys/dev/audio.c: revision 1.397
sys/dev/audio.c: revision 1.403
sys/dev/audio.c: revision 1.451
sys/dev/audio.c: revision 1.452
sys/dev/audiovar.h: revision 1.59 (patch)
sys/dev/aurateconv.c: revision 1.21 (patch)
sys/dev/auconv.c: revision 1.31 (patch)
sys/dev/audiovar.h: revision 1.60 (patch)
Broadcast all conditional variables if in being deactivated so no readers
or writers get stuck.
Fix a panic caused by opening pad(4)'s mixer before the corresponding
audio device has attached.
Addresses PR kern/52424.
Improve audio_set_vchan_defaults().
- Correct confused input/output parameters.
- Remove sc->{sc_channels, sc_precision, sc_frequency}. They are
the same as sc->sc_vchan_params.{channels, precision, sample_rate}.
The input parameter of audio_set_vchan_defaults() is now only
sc->sc_vchan_params.
Fix PR kern/52437
const-ify.
0 -> NULL in audioattach()
"bits" sounds better than "name" for argument name.
I feel expression (name / NBBY) a little strange.
The audio module will now compile with WARNS=5.
Typo in debug message.
Remove a duplicated line.
Add missing initialization of sc_rfilters in audioattach().
Remove dead codes.
sc->sc_opens never changes in this loop and as a result of
previous sc_audiochan cleanup "sc_opens == 0" is the same as
"sc_audiochan is empty".
Clean up audio_allocbufs().
As a result of sc_audiochan cleanup, it is easy to access sc_hwvc.
Clean up audio_open().
As a result of sc_audiochan cleanup, this loop is the same as last + 1.
hw_if->set_params is mandatory, so it will never be NULL.
CID-1427745: kill possible buffer overflows.
Revert my wrong r1.380 and add a comment instead.
To generate a diff of this commit:
cvs rdiff -u -r1.26.2.4 -r1.26.2.5 src/sys/dev/auconv.c
cvs rdiff -u -r1.357.2.10 -r1.357.2.11 src/sys/dev/audio.c
cvs rdiff -u -r1.55.2.5 -r1.55.2.6 src/sys/dev/audiovar.h
cvs rdiff -u -r1.19.42.1 -r1.19.42.2 src/sys/dev/aurateconv.c
Please note that diffs are not public domain; they are subject to the
copyright notices on the relevant files.
>Unformatted:
(Contact us)
$NetBSD: query-full-pr,v 1.43 2018/01/16 07:36:43 maya Exp $
$NetBSD: gnats_config.sh,v 1.9 2014/08/02 14:16:04 spz Exp $
Copyright © 1994-2017
The NetBSD Foundation, Inc. ALL RIGHTS RESERVED.
Home