> On 3 Jan 2015, at 9:35 am, David Gwynne <[email protected]> wrote:
>
>>
>> On 2 Jan 2015, at 9:55 pm, Mark Kettenis <[email protected]> wrote:
>>
>>> Date: Fri, 2 Jan 2015 16:15:07 +1000
>>> From: David Gwynne <[email protected]>
>>>
>>> can someone test this?
>>>
>>> it allocates storage for the volume change details rather than cast
>>> arguments to a single global task.
>>>
>>> adds some safety while there if audio0 is a hotplug device.
>>>
>>> ok?
>>
>> The problem with this approach is that things silently fail when
>> memory allocation fails. Exactly the problem that workq_add_task()
>> had. Perhaps the best thing is to store the "event" in the softc,
>> although then you'd probably need some locking to make sure the task
>> sees a consistent dir/out pair.
>
> is "silently fail" a pun here?
>
> you can lose events either way. if you get more than one keypress before the
> task runs, you'll lose. if you are in a situation where you cant allocate
> memory, you lose.
>
> i think the code generally is bogus so im trying to do as little as possible
> to it. the more i look at it the more i want to just remove it.
>
> its been unreliable in all incarnations.
did anyone try this diff?
dlg
>
>>
>>> Index: audio.c
>>> ===================================================================
>>> RCS file: /cvs/src/sys/dev/audio.c,v
>>> retrieving revision 1.125
>>> diff -u -p -r1.125 audio.c
>>> --- audio.c 19 Dec 2014 22:44:58 -0000 1.125
>>> +++ audio.c 2 Jan 2015 06:08:39 -0000
>>> @@ -465,11 +465,6 @@ audioattach(struct device *parent, struc
>>> }
>>> DPRINTF(("audio_attach: inputs ports=0x%x, output ports=0x%x\n",
>>> sc->sc_inports.allports, sc->sc_outports.allports));
>>> -
>>> -#if NWSKBD > 0
>>> - task_set(&sc->sc_mixer_task, wskbd_set_mixervolume_callback, NULL,
>>> - NULL);
>>> -#endif /* NWSKBD > 0 */
>>> }
>>>
>>> int
>>> @@ -3432,27 +3427,39 @@ filt_audiowrite(struct knote *kn, long h
>>> }
>>>
>>> #if NWSKBD > 0
>>> +struct wskbd_vol_change {
>>> + struct task t;
>>> + long dir;
>>> + long out;
>>> +};
>>> +
>>> int
>>> wskbd_set_mixervolume(long dir, long out)
>>> {
>>> struct audio_softc *sc;
>>> + struct wskbd_vol_change *ch;
>>>
>>> if (audio_cd.cd_ndevs == 0 || (sc = audio_cd.cd_devs[0]) == NULL) {
>>> DPRINTF(("wskbd_set_mixervolume: audio_cd\n"));
>>> return (ENXIO);
>>> }
>>>
>>> - task_del(systq, &sc->sc_mixer_task);
>>> - task_set(&sc->sc_mixer_task, wskbd_set_mixervolume_callback,
>>> - (void *)dir, (void *)out);
>>> - task_add(systq, &sc->sc_mixer_task);
>>> + ch = malloc(sizeof(*ch), M_TEMP, M_NOWAIT);
>>> + if (ch == NULL)
>>> + return (ENOMEM);
>>> +
>>> + task_set(&ch->t, wskbd_set_mixervolume_callback, ch, NULL);
>>> + ch->dir = dir;
>>> + ch->out = out;
>>> + task_add(systq, &ch->t);
>>>
>>> return (0);
>>> }
>>>
>>> void
>>> -wskbd_set_mixervolume_callback(void *arg1, void *arg2)
>>> +wskbd_set_mixervolume_callback(void *xch, void *null)
>>> {
>>> + struct wskbd_vol_change *ch = xch;
>>> struct audio_softc *sc;
>>> struct au_mixer_ports *ports;
>>> mixer_devinfo_t mi;
>>> @@ -3461,19 +3468,19 @@ wskbd_set_mixervolume_callback(void *arg
>>> u_int gain;
>>> int error;
>>>
>>> - if (audio_cd.cd_ndevs == 0 || (sc = audio_cd.cd_devs[0]) == NULL) {
>>> - DPRINTF(("%s: audio_cd\n", __func__));
>>> - return;
>>> - }
>>> + dir = ch->dir;
>>> + out = ch->out;
>>> + free(ch, M_TEMP, sizeof(*ch));
>>>
>>> - dir = (long)arg1;
>>> - out = (long)arg2;
>>> + sc = (struct audio_softc *)device_lookup(&audio_cd, 0);
>>> + if (sc == NULL)
>>> + return;
>>>
>>> ports = out ? &sc->sc_outports : &sc->sc_inports;
>>>
>>> if (ports->master == -1) {
>>> DPRINTF(("%s: master == -1\n", __func__));
>>> - return;
>>> + goto done;
>>> }
>>>
>>> if (dir == 0) {
>>> @@ -3482,7 +3489,7 @@ wskbd_set_mixervolume_callback(void *arg
>>> error = au_get_mute(sc, ports, &mute);
>>> if (error != 0) {
>>> DPRINTF(("%s: au_get_mute: %d\n", __func__, error));
>>> - return;
>>> + goto done;
>>> }
>>>
>>> mute = !mute;
>>> @@ -3490,7 +3497,7 @@ wskbd_set_mixervolume_callback(void *arg
>>> error = au_set_mute(sc, ports, mute);
>>> if (error != 0) {
>>> DPRINTF(("%s: au_set_mute: %d\n", __func__, error));
>>> - return;
>>> + goto done;
>>> }
>>> } else {
>>> /* Raise or lower volume */
>>> @@ -3499,7 +3506,7 @@ wskbd_set_mixervolume_callback(void *arg
>>> error = sc->hw_if->query_devinfo(sc->hw_hdl, &mi);
>>> if (error != 0) {
>>> DPRINTF(("%s: query_devinfo: %d\n", __func__, error));
>>> - return;
>>> + goto done;
>>> }
>>>
>>> au_get_gain(sc, ports, &gain, &balance);
>>> @@ -3512,8 +3519,11 @@ wskbd_set_mixervolume_callback(void *arg
>>> error = au_set_gain(sc, ports, gain, balance);
>>> if (error != 0) {
>>> DPRINTF(("%s: au_set_gain: %d\n", __func__, error));
>>> - return;
>>> + goto done;
>>> }
>>> }
>>> +
>>> +done:
>>> + device_unref(&sc->dev);
>>> }
>>> #endif /* NWSKBD > 0 */
