sound: Improve simplex handling in dsp_open()

[christos]

Summary

If we are in simplex mode, make sure we do not open in both directions
(read/write) and also that we do not open in a direction opposite of
what is already opened. For example, if the device is already doing
playback, we cannot open the device for recording at the same time, and
vice-versa.

Sponsored by: The FreeBSD Foundation
MFC after: 2 days

[dev_submerge.ch]

Overall approach is good, except we shouldn't fail for simplex when duplex is requested. Generic applications will not be prepared to handle this error, but will usually check the results of other operations (ioctl etc). Just search the channels for an already used direction in that case, or decide for one. And then make sure we don't try to allocate the opposite direction.

[christos]

In D45835#1045553, @dev_submerge.ch wrote:

Overall approach is good, except we shouldn't fail for simplex when duplex is requested. Generic applications will not be prepared to handle this error, but will usually check the results of other operations (ioctl etc). Just search the channels for an already used direction in that case, or decide for one. And then make sure we don't try to allocate the opposite direction.

What do you mean by "fail for simplex when duplex is requested"? This check happens only if the device has SD_F_SIMPLEX.

[dev_submerge.ch]

In D45835#1045573, @christos wrote:

In D45835#1045553, @dev_submerge.ch wrote:

Overall approach is good, except we shouldn't fail for simplex when duplex is requested. Generic applications will not be prepared to handle this error, but will usually check the results of other operations (ioctl etc). Just search the channels for an already used direction in that case, or decide for one. And then make sure we don't try to allocate the opposite direction.

What do you mean by "fail for simplex when duplex is requested"? This check happens only if the device has SD_F_SIMPLEX.

Do not return ENOTSUP in case duplex is requested, because the application cannot know beforehand that the device is simplex only.

[christos]

In D45835#1045574, @dev_submerge.ch wrote:

In D45835#1045573, @christos wrote:

In D45835#1045553, @dev_submerge.ch wrote:

Overall approach is good, except we shouldn't fail for simplex when duplex is requested. Generic applications will not be prepared to handle this error, but will usually check the results of other operations (ioctl etc). Just search the channels for an already used direction in that case, or decide for one. And then make sure we don't try to allocate the opposite direction.

What do you mean by "fail for simplex when duplex is requested"? This check happens only if the device has SD_F_SIMPLEX.

Do not return ENOTSUP in case duplex is requested, because the application cannot know beforehand that the device is simplex only.

What would be a reasonable errno? ENXIO? ENODEV?

[christos]

Hmm I think I get what you mean.. That in case the device is SIMPLEX but we request DUPLEX, instead of failing, we should create _one_ channel only, either in a direction that already exists, or choose a default (playback) in case no channels are playing already.

However, wouldn't it be equally confusing if we allocate 1 direction instead of 2, or choose playback as the default, when the application expects to open the device in DUPLEX?

[dev_submerge.ch]

In D45835#1045583, @christos wrote:

Hmm I think I get what you mean.. That in case the device is SIMPLEX but we request DUPLEX, instead of failing, we should create _one_ channel only, either in a direction that already exists, or choose a default (playback) in case no channels are playing already.

Yes, exactly - sorry for the misunderstanding.

However, wouldn't it be equally confusing if we allocate 1 direction instead of 2, or choose playback as the default, when the application expects to open the device in DUPLEX?

Maybe, but from the applications perspective you have to open the device first to get any information. Thus it's a bit of an API problem. I just expect the typical application to handle this more gracefully later on with at least a chance to get device info through ioctls.

[dev_submerge.ch]

Also, in case the device is playback only or recording only, we don't return an error either for a duplex request. The channel allocation part in dsp_open() is explicitly designed to only return an error when there would be no channel at all.

[christos]

In D45835#1045597, @dev_submerge.ch wrote:

In D45835#1045583, @christos wrote:

Hmm I think I get what you mean.. That in case the device is SIMPLEX but we request DUPLEX, instead of failing, we should create _one_ channel only, either in a direction that already exists, or choose a default (playback) in case no channels are playing already.

Yes, exactly - sorry for the misunderstanding.

However, wouldn't it be equally confusing if we allocate 1 direction instead of 2, or choose playback as the default, when the application expects to open the device in DUPLEX?

Maybe, but from the applications perspective you have to open the device first to get any information. Thus it's a bit of an API problem. I just expect the typical application to handle this more gracefully later on with at least a chance to get device info through ioctls.

Yeah, definitely not an ideal approach, but I guess it's the least-worst we can have. :)

Also, in case the device is playback only or recording only, we don't return an error either for a duplex request. The channel allocation part in dsp_open() is explicitly designed to only return an error when there would be no channel at all.

What about the case where we request SIMPLEX (i.e only READ or WRITE) but the device is already playing in the opposite direction? Shouldn't we return an error at least here?

[dev_submerge.ch]

In D45835#1045602, @christos wrote:

What about the case where we request SIMPLEX (i.e only READ or WRITE) but the device is already playing in the opposite direction? Shouldn't we return an error at least here?

Yes, returning an error in that case is consistent with the other checks, like DSP_FIXUP_ERROR().

[christos]

Improve.

[christos]

sys/dev/sound/pcm/dsp.c

334

I am wondering whether we need this field in the first place. It's only used here, and we can just as easily check pcm_getflags() & SD_F_SIMPLEX.

[christos]

sys/dev/sound/pcm/dsp.c

344

Is there a need to loop through all channels? I think checking just the first is enough.

[dev_submerge.ch]

You may want to simplify the direction check and error handling on occasion and consolidate it with DSP_FIXUP_ERROR(), but that's not strictly necessary.

sys/dev/sound/pcm/dsp.c

334

Yes, looks like we can get rid of it.

344

Checking the first should be enough - could the result of CHN_FIRST() be NULL though, in any case?

[christos]

sys/dev/sound/pcm/dsp.c

344

I don't think it can be NULL. This list is updated in dsp_open() and dsp_close(), so if a channel became NULL during runtime either read(), write(), ioctl(), ... would fail and call dsp_close(), so the item would get removed the list. I hope I am not missing something though.

[christos]

Remove dsp_cdevpriv->simplex.

[dev_submerge.ch]

sys/dev/sound/pcm/dsp.c

341–342

Hm, this check gets us into trouble when the device is recording only (duplex open fails with ENOTSUP). Can we defer it to when we know whether the first allocation succeeded? See second inline comment.

413

Can we check here and make sure that we don't try to allocate a second channel in case the device is simplex?

[christos]

sys/dev/sound/pcm/dsp.c

341–342

I think we can check d->reccount and d->playcount to know what we support beforehand:

if (CHN_EMPTY(d, channels.pcm.opened)) {
        if (d->playcount > 0)
	        flags &= ~FREAD;
	else if (d->reccount > 0)
		flags &= ~FWRITE;

Can you test whether this works?

[dev_submerge.ch]

sys/dev/sound/pcm/dsp.c

341–342

Ok, that seems to work.

[christos]

sys/dev/sound/pcm/dsp.c

341–342

Great. :) Then I think this one is good to go.

[christos]

Determine available direction in case of empty channel list.