Changeset View
Standalone View
sys/dev/sound/pcm/dsp.c
| Show All 31 Lines | |||||
| * SUCH DAMAGE. | * SUCH DAMAGE. | ||||
| */ | */ | ||||
| #ifdef HAVE_KERNEL_OPTION_HEADERS | #ifdef HAVE_KERNEL_OPTION_HEADERS | ||||
| #include "opt_snd.h" | #include "opt_snd.h" | ||||
| #endif | #endif | ||||
| #include <dev/sound/pcm/sound.h> | #include <dev/sound/pcm/sound.h> | ||||
| #include <dev/sound/pcm/vchan.h> | |||||
| #include <sys/ctype.h> | #include <sys/ctype.h> | ||||
| #include <sys/lock.h> | #include <sys/lock.h> | ||||
| #include <sys/rwlock.h> | #include <sys/rwlock.h> | ||||
| #include <sys/sysent.h> | #include <sys/sysent.h> | ||||
| #include <vm/vm.h> | #include <vm/vm.h> | ||||
| #include <vm/vm_object.h> | #include <vm/vm_object.h> | ||||
| #include <vm/vm_page.h> | #include <vm/vm_page.h> | ||||
| ▲ Show 20 Lines • Show All 105 Lines • ▼ Show 20 Lines | |||||
| dsp_unlock_chans(struct dsp_cdevpriv *priv, uint32_t prio) | dsp_unlock_chans(struct dsp_cdevpriv *priv, uint32_t prio) | ||||
| { | { | ||||
| if (priv->rdch != NULL && (prio & SD_F_PRIO_RD)) | if (priv->rdch != NULL && (prio & SD_F_PRIO_RD)) | ||||
| CHN_UNLOCK(priv->rdch); | CHN_UNLOCK(priv->rdch); | ||||
| if (priv->wrch != NULL && (prio & SD_F_PRIO_WR)) | if (priv->wrch != NULL && (prio & SD_F_PRIO_WR)) | ||||
| CHN_UNLOCK(priv->wrch); | CHN_UNLOCK(priv->wrch); | ||||
| } | } | ||||
| static int | |||||
| dsp_chn_alloc(struct snddev_info *d, struct pcm_channel **ch, int direction, | |||||
| int flags, struct thread *td) | |||||
| { | |||||
| struct pcm_channel *c; | |||||
| char *comm; | |||||
| pid_t pid; | |||||
| int err, fmt, spd; | |||||
| bool vdir_enabled; | |||||
| KASSERT(d != NULL && ch != NULL && | |||||
| (direction == PCMDIR_PLAY || direction == PCMDIR_REC), | |||||
| ("%s(): invalid d=%p ch=%p direction=%d", | |||||
| __func__, d, ch, direction)); | |||||
| PCM_BUSYASSERT(d); | |||||
| fmt = SND_FORMAT(AFMT_U8, 1, 0); | |||||
| spd = DSP_DEFAULT_SPEED; | |||||
| pid = td->td_proc->p_pid; | |||||
| comm = td->td_proc->p_comm; | |||||
| vdir_enabled = (direction == PCMDIR_PLAY && d->flags & SD_F_PVCHANS) || | |||||
| (direction == PCMDIR_REC && d->flags & SD_F_RVCHANS); | |||||
| *ch = NULL; | |||||
| CHN_FOREACH(c, d, channels.pcm.primary) { | |||||
| CHN_LOCK(c); | |||||
| if (c->direction != direction) { | |||||
| CHN_UNLOCK(c); | |||||
dev_submerge.ch: Since we do not take vchan enabled / disabled into account here, this may select a primary… | |||||
Done Inline ActionsI included the vchan enabled/disabled check here as well, as suggested in your comment below. However, if vchans are disabled for a given direction, would having >1 primary channels in that direction make a difference in the first place? christos: I included the vchan enabled/disabled check here as well, as suggested in your comment below. | |||||
Done Inline ActionsNow I'm confused - I only see the c->flags & CHN_F_HAS_VCHAN which checks for the presence of vchans, not whether they are enabled? Or do you mean you included these checks locally but didn't update the diff here yet? And yes, if vchans are disabled, with a vchan still running on the first playback channel, this first playback channel would always be selected because CHN_F_HAS_VCHAN is set. Despite the second playback channel possibly being free to use, with CHN_F_BUSY not set. dev_submerge.ch: Now I'm confused - I only see the `c->flags & CHN_F_HAS_VCHAN` which checks for the presence of… | |||||
Done Inline ActionsSorry, I meant that I applied the change locally. This should be enough IMHO. christos: Sorry, I meant that I applied the change locally. This should be enough IMHO. | |||||
| continue; | |||||
| } | |||||
| /* Find an available primary channel to use. */ | |||||
| if ((c->flags & CHN_F_BUSY) == 0 || | |||||
| (vdir_enabled && (c->flags & CHN_F_HAS_VCHAN))) | |||||
| break; | |||||
Done Inline ActionsShouldn't this also break out of the loop when vdir_enabled is not set? I think the logic order is mixed up, did you mean (c->flags & CHN_F_BUSY) == 0 || (vdir_enabled && c->flags & CHN_F_HAS_VCHAN)? dev_submerge.ch: Shouldn't this also break out of the loop when `vdir_enabled` is not set? I think the logic… | |||||
Done Inline ActionsYes, I noticed this right after I submitted. I meant to write if ((c->flags & CHN_F_BUSY) == 0 || (vdir_enabled && (c->flags & CHN_F_HAS_VCHAN))) christos: Yes, I noticed this right after I submitted. I meant to write
```
if ((c->flags & CHN_F_BUSY)… | |||||
| CHN_UNLOCK(c); | |||||
| } | |||||
| if (c == NULL) | |||||
| return (EBUSY); | |||||
| /* | |||||
| * We can have the following cases: | |||||
Done Inline ActionsWe probably need this condition in the loop too, I'd suggest a variable like bool vchan_enabled = ... for readability. dev_submerge.ch: We probably need this condition in the loop too, I'd suggest a variable like `bool… | |||||
| * - vchans are enabled, add a new vchan to the primary channel. | |||||
| * - vchans are disabled, use the primary channel directly. | |||||
| */ | |||||
| if (vdir_enabled && ((c->flags & CHN_F_BUSY) == 0 || | |||||
| c->flags & CHN_F_HAS_VCHAN)) { | |||||
| err = vchan_create(c, ch); | |||||
| CHN_UNLOCK(c); | |||||
| if (err != 0) | |||||
| return (err); | |||||
| CHN_LOCK(*ch); | |||||
| } else if ((c->flags & CHN_F_BUSY) == 0) { | |||||
Done Inline ActionsSince d is not locked here, and the flags do not represent the current state of opened (v)channels, could we end up with using a busy primary channel here? Or a vchan on top of a directly opened primary channel? dev_submerge.ch: Since `d` is not locked here, and the flags do not represent the current state of opened… | |||||
Done Inline Actionsd is in fact "locked". SD_F_BUSY is set, which acts as a kind of sleepable lock. Also which flags are you referring to? The PCM or the channel ones? christos: `d` is in fact "locked". `SD_F_BUSY` is set, which acts as a kind of sleepable lock. Also which… | |||||
Done Inline ActionsI meant the d->flags, but you're right, they are protected through SD_F_BUSY. What I apparently can do is to enable vchans with sysctl dev.pcm.0.play.vchans = 1 while playing on a primary channel, and then open a vchan for playback on top of that. It leads to a timeout and inconsistent state afterwards, not good. I think there's a conceptual problem here with the flags not representing the actual state of channels running. We have to know whether the primary channel is used directly or only for vchans. dev_submerge.ch: I meant the `d->flags`, but you're right, they are protected through `SD_F_BUSY`. What I… | |||||
Done Inline ActionsDo you think the new diff addresses the issue properly? The bug you mention is fixed. christos: Do you think the new diff addresses the issue properly? The bug you mention is fixed. | |||||
| *ch = c; | |||||
| } else { | |||||
| CHN_UNLOCK(c); | |||||
| return (ENODEV); | |||||
| } | |||||
| (*ch)->flags |= CHN_F_BUSY; | |||||
| if (flags & O_NONBLOCK) | |||||
| (*ch)->flags |= CHN_F_NBIO; | |||||
| if (flags & O_EXCL) | |||||
| (*ch)->flags |= CHN_F_EXCLUSIVE; | |||||
| (*ch)->pid = pid; | |||||
| strlcpy((*ch)->comm, (comm != NULL) ? comm : CHN_COMM_UNKNOWN, | |||||
| sizeof((*ch)->comm)); | |||||
| if ((err = chn_reset(*ch, fmt, spd)) != 0) | |||||
| return (err); | |||||
| chn_vpc_reset(*ch, SND_VOL_C_PCM, 0); | |||||
| CHN_UNLOCK(*ch); | |||||
| return (0); | |||||
| } | |||||
| #define DSP_F_VALID(x) ((x) & (FREAD | FWRITE)) | #define DSP_F_VALID(x) ((x) & (FREAD | FWRITE)) | ||||
| #define DSP_F_DUPLEX(x) (((x) & (FREAD | FWRITE)) == (FREAD | FWRITE)) | #define DSP_F_DUPLEX(x) (((x) & (FREAD | FWRITE)) == (FREAD | FWRITE)) | ||||
| #define DSP_F_SIMPLEX(x) (!DSP_F_DUPLEX(x)) | #define DSP_F_SIMPLEX(x) (!DSP_F_DUPLEX(x)) | ||||
| #define DSP_F_READ(x) ((x) & FREAD) | #define DSP_F_READ(x) ((x) & FREAD) | ||||
| #define DSP_F_WRITE(x) ((x) & FWRITE) | #define DSP_F_WRITE(x) ((x) & FWRITE) | ||||
| static void | static void | ||||
| dsp_close(void *data) | dsp_close(void *data) | ||||
| { | { | ||||
| struct dsp_cdevpriv *priv = data; | struct dsp_cdevpriv *priv = data; | ||||
| struct pcm_channel *rdch, *wrch; | struct pcm_channel *rdch, *wrch, *parent; | ||||
| struct snddev_info *d; | struct snddev_info *d; | ||||
| int sg_ids; | int sg_ids; | ||||
| if (priv == NULL) | if (priv == NULL) | ||||
| return; | return; | ||||
| d = priv->sc; | d = priv->sc; | ||||
| /* At this point pcm_unregister() will destroy all channels anyway. */ | /* At this point pcm_unregister() will destroy all channels anyway. */ | ||||
| Show All 29 Lines | if (rdch != NULL) { | ||||
| * first. | * first. | ||||
| */ | */ | ||||
| PCM_SG_LOCK(); | PCM_SG_LOCK(); | ||||
| sg_ids = chn_syncdestroy(rdch); | sg_ids = chn_syncdestroy(rdch); | ||||
| PCM_SG_UNLOCK(); | PCM_SG_UNLOCK(); | ||||
| if (sg_ids != 0) | if (sg_ids != 0) | ||||
| free_unr(pcmsg_unrhdr, sg_ids); | free_unr(pcmsg_unrhdr, sg_ids); | ||||
| if (rdch->flags & CHN_F_VIRTUAL) { | |||||
| parent = rdch->parentchannel; | |||||
| CHN_LOCK(parent); | |||||
| CHN_LOCK(rdch); | CHN_LOCK(rdch); | ||||
| vchan_destroy(rdch); | |||||
| CHN_UNLOCK(parent); | |||||
| } else { | |||||
| CHN_LOCK(rdch); | |||||
| chn_abort(rdch); /* won't sleep */ | chn_abort(rdch); /* won't sleep */ | ||||
| rdch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP | | rdch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP | | ||||
| CHN_F_DEAD | CHN_F_EXCLUSIVE); | CHN_F_DEAD | CHN_F_EXCLUSIVE); | ||||
| chn_reset(rdch, 0, 0); | chn_reset(rdch, 0, 0); | ||||
| chn_release(rdch); | chn_release(rdch); | ||||
| } | } | ||||
| } | |||||
| if (wrch != NULL) { | if (wrch != NULL) { | ||||
| /* | /* | ||||
| * Please see block above. | * Please see block above. | ||||
| */ | */ | ||||
| PCM_SG_LOCK(); | PCM_SG_LOCK(); | ||||
| sg_ids = chn_syncdestroy(wrch); | sg_ids = chn_syncdestroy(wrch); | ||||
| PCM_SG_UNLOCK(); | PCM_SG_UNLOCK(); | ||||
| if (sg_ids != 0) | if (sg_ids != 0) | ||||
| free_unr(pcmsg_unrhdr, sg_ids); | free_unr(pcmsg_unrhdr, sg_ids); | ||||
| if (wrch->flags & CHN_F_VIRTUAL) { | |||||
| parent = wrch->parentchannel; | |||||
| CHN_LOCK(parent); | |||||
| CHN_LOCK(wrch); | CHN_LOCK(wrch); | ||||
| vchan_destroy(wrch); | |||||
| CHN_UNLOCK(parent); | |||||
| } else { | |||||
| CHN_LOCK(wrch); | |||||
| chn_flush(wrch); /* may sleep */ | chn_flush(wrch); /* may sleep */ | ||||
| wrch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP | | wrch->flags &= ~(CHN_F_RUNNING | CHN_F_MMAP | | ||||
| CHN_F_DEAD | CHN_F_EXCLUSIVE); | CHN_F_DEAD | CHN_F_EXCLUSIVE); | ||||
| chn_reset(wrch, 0, 0); | chn_reset(wrch, 0, 0); | ||||
| chn_release(wrch); | chn_release(wrch); | ||||
| } | } | ||||
| } | |||||
| PCM_LOCK(d); | PCM_LOCK(d); | ||||
| } | } | ||||
| PCM_RELEASE(d); | PCM_RELEASE(d); | ||||
| PCM_UNLOCK(d); | PCM_UNLOCK(d); | ||||
| PCM_GIANT_LEAVE(d); | PCM_GIANT_LEAVE(d); | ||||
| skip: | skip: | ||||
| free(priv, M_DEVBUF); | free(priv, M_DEVBUF); | ||||
| priv = NULL; | priv = NULL; | ||||
| } | } | ||||
| static int | static int | ||||
| dsp_open(struct cdev *i_dev, int flags, int mode, struct thread *td) | dsp_open(struct cdev *i_dev, int flags, int mode, struct thread *td) | ||||
| { | { | ||||
| struct dsp_cdevpriv *priv; | struct dsp_cdevpriv *priv; | ||||
| struct pcm_channel *rdch, *wrch, *ch; | struct pcm_channel *ch; | ||||
| struct snddev_info *d; | struct snddev_info *d; | ||||
| uint32_t fmt, spd; | int error, dir; | ||||
| int error, rderror, wrerror, dir; | |||||
| /* Kind of impossible.. */ | /* Kind of impossible.. */ | ||||
| if (i_dev == NULL || td == NULL) | if (i_dev == NULL || td == NULL) | ||||
| return (ENODEV); | return (ENODEV); | ||||
| d = i_dev->si_drv1; | d = i_dev->si_drv1; | ||||
| if (!DSP_REGISTERED(d)) | if (!DSP_REGISTERED(d)) | ||||
| return (EBADF); | return (EBADF); | ||||
| if (PCM_CHANCOUNT(d) >= PCM_MAXCHANS) | |||||
| return (ENOMEM); | |||||
Done Inline ActionsIncrease of the channel counters is not protected under the same mutex lock that we hold here. The comparison should at least be >=, in case multiple channels are opened at once and the total of the counters exceeds 10000. dev_submerge.ch: Increase of the channel counters is not protected under the same mutex lock that we hold here. | |||||
| priv = malloc(sizeof(*priv), M_DEVBUF, M_WAITOK | M_ZERO); | priv = malloc(sizeof(*priv), M_DEVBUF, M_WAITOK | M_ZERO); | ||||
| priv->sc = d; | priv->sc = d; | ||||
| priv->rdch = NULL; | |||||
| priv->wrch = NULL; | |||||
| priv->volch = NULL; | |||||
| error = devfs_set_cdevpriv(priv, dsp_close); | error = devfs_set_cdevpriv(priv, dsp_close); | ||||
| if (error != 0) | if (error != 0) | ||||
| return (error); | return (error); | ||||
| PCM_GIANT_ENTER(d); | PCM_GIANT_ENTER(d); | ||||
| /* Lock snddev so nobody else can monkey with it. */ | /* Lock snddev so nobody else can monkey with it. */ | ||||
| ▲ Show 20 Lines • Show All 46 Lines • ▼ Show 20 Lines | dsp_open(struct cdev *i_dev, int flags, int mode, struct thread *td) | ||||
| /* | /* | ||||
| * That is just enough. Acquire and unlock pcm lock so | * That is just enough. Acquire and unlock pcm lock so | ||||
| * the other will just have to wait until we finish doing | * the other will just have to wait until we finish doing | ||||
| * everything. | * everything. | ||||
| */ | */ | ||||
| PCM_ACQUIRE(d); | PCM_ACQUIRE(d); | ||||
| PCM_UNLOCK(d); | PCM_UNLOCK(d); | ||||
| fmt = SND_FORMAT(AFMT_U8, 1, 0); | if (DSP_F_WRITE(flags)) { | ||||
| spd = DSP_DEFAULT_SPEED; | error = dsp_chn_alloc(d, &priv->wrch, PCMDIR_PLAY, flags, td); | ||||
| if (error != 0) { | |||||
| rdch = NULL; | |||||
| wrch = NULL; | |||||
| rderror = 0; | |||||
| wrerror = 0; | |||||
| if (DSP_F_READ(flags)) { | |||||
| /* open for read */ | |||||
| rderror = pcm_chnalloc(d, &rdch, PCMDIR_REC, | |||||
| td->td_proc->p_pid, td->td_proc->p_comm); | |||||
| if (rderror == 0 && chn_reset(rdch, fmt, spd) != 0) | |||||
| rderror = ENXIO; | |||||
| if (rderror != 0) { | |||||
| if (rdch != NULL) | |||||
| chn_release(rdch); | |||||
| if (!DSP_F_DUPLEX(flags)) { | |||||
| PCM_RELEASE_QUICK(d); | PCM_RELEASE_QUICK(d); | ||||
| PCM_GIANT_EXIT(d); | PCM_GIANT_EXIT(d); | ||||
| return (rderror); | return (error); | ||||
| } | } | ||||
| rdch = NULL; | PCM_LOCK(d); | ||||
| } else { | CHN_INSERT_HEAD(d, priv->wrch, channels.pcm.opened); | ||||
| if (flags & O_NONBLOCK) | PCM_UNLOCK(d); | ||||
| rdch->flags |= CHN_F_NBIO; | |||||
| if (flags & O_EXCL) | |||||
| rdch->flags |= CHN_F_EXCLUSIVE; | |||||
| chn_vpc_reset(rdch, SND_VOL_C_PCM, 0); | |||||
| CHN_UNLOCK(rdch); | |||||
| } | } | ||||
Done Inline ActionsWhat happens in duplex mode when only the recording dsp_chn_alloc() call fails? Is dsp_close() actually called when dsp_open() fails? And if that's the case, we have an inconsistency between priv->wrch and the channels.pcm.opened list. We call CHN_REMOVE() there... dev_submerge.ch: What happens in duplex mode when only the recording `dsp_chn_alloc()` call fails? Is `dsp_close… | |||||
Done Inline ActionsSo, dsp_close() will be called, but because priv->wrch will not have yet been added to the opened list, the CHN_REMOVE() will most likely cause a problem. I think we could retain the wrch and rdch variables, and assign them to priv->wrch and priv->rdch respectively, once we get past the dsp_chn_alloc()s. christos: So, `dsp_close()` will be called, but because `priv->wrch` will not have yet been added to the… | |||||
| } | if (DSP_F_READ(flags)) { | ||||
| error = dsp_chn_alloc(d, &priv->rdch, PCMDIR_REC, flags, td); | |||||
| if (DSP_F_WRITE(flags)) { | if (error != 0) { | ||||
| /* open for write */ | |||||
| wrerror = pcm_chnalloc(d, &wrch, PCMDIR_PLAY, | |||||
| td->td_proc->p_pid, td->td_proc->p_comm); | |||||
| if (wrerror == 0 && chn_reset(wrch, fmt, spd) != 0) | |||||
| wrerror = ENXIO; | |||||
| if (wrerror != 0) { | |||||
| if (wrch != NULL) | |||||
| chn_release(wrch); | |||||
| if (!DSP_F_DUPLEX(flags)) { | |||||
| if (rdch != NULL) { | |||||
| /* | |||||
| * Lock, and release previously created | |||||
| * record channel | |||||
| */ | |||||
| CHN_LOCK(rdch); | |||||
| chn_release(rdch); | |||||
| } | |||||
| PCM_RELEASE_QUICK(d); | PCM_RELEASE_QUICK(d); | ||||
| PCM_GIANT_EXIT(d); | PCM_GIANT_EXIT(d); | ||||
| return (wrerror); | return (error); | ||||
| } | } | ||||
| wrch = NULL; | |||||
| } else { | |||||
| if (flags & O_NONBLOCK) | |||||
| wrch->flags |= CHN_F_NBIO; | |||||
| if (flags & O_EXCL) | |||||
| wrch->flags |= CHN_F_EXCLUSIVE; | |||||
| chn_vpc_reset(wrch, SND_VOL_C_PCM, 0); | |||||
| CHN_UNLOCK(wrch); | |||||
| } | |||||
| } | |||||
| PCM_LOCK(d); | PCM_LOCK(d); | ||||
| CHN_INSERT_HEAD(d, priv->rdch, channels.pcm.opened); | |||||
| if (wrch == NULL && rdch == NULL) { | |||||
| PCM_RELEASE(d); | |||||
| PCM_UNLOCK(d); | PCM_UNLOCK(d); | ||||
| PCM_GIANT_EXIT(d); | |||||
| if (wrerror != 0) | |||||
| return (wrerror); | |||||
| if (rderror != 0) | |||||
| return (rderror); | |||||
| return (EINVAL); | |||||
| } | } | ||||
| if (rdch != NULL) | |||||
| CHN_INSERT_HEAD(d, rdch, channels.pcm.opened); | |||||
| if (wrch != NULL) | |||||
| CHN_INSERT_HEAD(d, wrch, channels.pcm.opened); | |||||
| priv->rdch = rdch; | |||||
| priv->wrch = wrch; | |||||
Done Inline ActionsI think we still have an issue here, when allocation of the playback channel succeeds, but recording channel fails. We're basically "leaking" the playback channel now, with no chance for dsp_close() to clean up the channels because it's lacking info about the allocation. I would suggest to treat the cases for playback and recording separately, handle allocation success or failure independently per direction. Complete the registration of e.g. playback channel in the open list, even if the subsequent recording channel allocation fails. If the data is consistent, we can let dsp_close() do the cleanup afterwards. dev_submerge.ch: I think we still have an issue here, when allocation of the playback channel succeeds, but… | |||||
Done Inline ActionsGood idea. christos: Good idea. | |||||
| PCM_RELEASE(d); | PCM_RELEASE_QUICK(d); | ||||
| PCM_UNLOCK(d); | |||||
| PCM_GIANT_LEAVE(d); | PCM_GIANT_LEAVE(d); | ||||
| return (0); | return (0); | ||||
| } | } | ||||
| static __inline int | static __inline int | ||||
| dsp_io_ops(struct dsp_cdevpriv *priv, struct uio *buf) | dsp_io_ops(struct dsp_cdevpriv *priv, struct uio *buf) | ||||
| { | { | ||||
| ▲ Show 20 Lines • Show All 2,516 Lines • Show Last 20 Lines | |||||
Since we do not take vchan enabled / disabled into account here, this may select a primary channel that is incompatible with what we need. While this just defers the error in case of one primary channel per direction, it may prevent choosing another (compatible) one if there are multiple primary channels per direction.