[Bug 289441] /dev/dsp O_NONBLOCK flag effective when set by fctnl(), ineffective when set by open()

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=289441

Damjan Jovanovic <damjan.jov@gmail.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |patch

--- Comment #2 from Damjan Jovanovic <damjan.jov@gmail.com> ---
(In reply to Damjan Jovanovic from comment #1)

This patch fixes the problem - simply setting CHN_F_NBIO after calling
chn_reset() (instead of before), is enough to make write() non-blocking like it
should be:

---snip---
diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
index aa6ad4a59778..f1c6fc840edb 100644
--- a/sys/dev/sound/pcm/dsp.c
+++ b/sys/dev/sound/pcm/dsp.c
@@ -221,17 +221,17 @@ dsp_chn_alloc(struct snddev_info *d, struct pcm_channel
**ch, int direction,
                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, (*ch)->format, (*ch)->speed)) != 0)
                return (err);
+       (*ch)->flags |= CHN_F_BUSY;
+       if (flags & O_NONBLOCK)
+               (*ch)->flags |= CHN_F_NBIO;
+       if (flags & O_EXCL)
+               (*ch)->flags |= CHN_F_EXCLUSIVE;
        chn_vpc_reset(*ch, SND_VOL_C_PCM, 0);

        CHN_UNLOCK(*ch);
---snip---

So yes, what I proposed in comment 1, where chn_reset() clears CHN_F_NBIO,
causing later calls to write() to block, must be correct.

But is this enough? Setting CHN_F_NBIO after calling chn_reset(), like the
above patch does, will fix open(). But will it remain working? The chn_reset()
function is called from a large number of other places. Here is its hierarchy
of callers:

chn_reset()
  dsp_chn_alloc()
    dsp_open()
  dsp_close()
  chn_init()
    pcm_addchan()
      (device drivers?)
    vchan_create()
      dsp_chn_alloc() (but happens before setting CHN_F_NBIO)
  chn_notify()
    chn_resizebuf()
      chn_setlatency()
        chn_reset()
        chn_notify()
        dsp_oss_policy()
          dsp_ioctl() with SNDCTL_DSP_POLICY
      chn_setblocksize()
        dsp_ioctl() with AIOSSIZE or SNDCTL_DSP_SETBLKSIZE or
SNDCTL_DSP_SETFRAGMENT
    vchan_trigger()
  sysctl_dev_pcm_vchanrate()
  sysctl_dev_pcm_vchanformat()
  vchan_create()
    dsp_chn_alloc() (but happens before setting CHN_F_NBIO)
  vchan_destroy()
    dsp_close()
    vchan_create() (on failure)

And device drivers also call some of those functions.

So could chn_reset() get called while a channel is in use, clearing the
CHN_F_NBIO flag despite the fact open() set it correctly? If so, then it might
be better to add CHN_F_NBIO to the CHN_F_RESET flags instead, so that
chn_reset() preserves it.

But if I understand correctly, after a channel is closed, it can be reused by
another process. If so, then where should CHN_F_NBIO get cleared, so the next
user starts with a blocking channel?

If my concerns are valid, then this patch might be better. Add CHN_F_NBIO to
the CHN_F_RESET flags, so chn_reset() preserves it, but in dsp_chn_alloc(),
also clear CHN_F_NBIO if O_NONBLOCK is unset, so that reused channels have the
correct initial blocking mode:

---snip---
diff --git a/sys/dev/sound/pcm/channel.h b/sys/dev/sound/pcm/channel.h
index fab182b22774..9ad21d219001 100644
--- a/sys/dev/sound/pcm/channel.h
+++ b/sys/dev/sound/pcm/channel.h
@@ -408,7 +408,7 @@ enum {

 #define CHN_F_RESET            (CHN_F_BUSY | CHN_F_DEAD |              \
                                 CHN_F_VIRTUAL | CHN_F_HAS_VCHAN |      \
-                                CHN_F_VCHAN_DYNAMIC |                  \
+                                CHN_F_VCHAN_DYNAMIC | CHN_F_NBIO |     \
                                 CHN_F_PASSTHROUGH | CHN_F_EXCLUSIVE)

 #define CHN_F_MMAP_INVALID     (CHN_F_DEAD | CHN_F_RUNNING)
diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
index aa6ad4a59778..8a972b42a70d 100644
--- a/sys/dev/sound/pcm/dsp.c
+++ b/sys/dev/sound/pcm/dsp.c
@@ -222,6 +222,7 @@ dsp_chn_alloc(struct snddev_info *d, struct pcm_channel
**ch, int direction,
        }

        (*ch)->flags |= CHN_F_BUSY;
+       (*ch)->flags &= ~CHN_F_NBIO;
        if (flags & O_NONBLOCK)
                (*ch)->flags |= CHN_F_NBIO;
        if (flags & O_EXCL)
---snip---

Can someone more familiar with OSS internals please comment?

-- 
You are receiving this mail because:
You are the assignee for the bug.