bug in sound driver?

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

bug in sound driver?

Wall, Stephen
I am working on a driver that is a modified copy of the sound driver, and I've found something that might be a bug.

In sys/dev/sound/pcm/dsp.c, the funcion dap_io_ops() (called by dsp_read() and dsp_write()) calls getchns() to get pointers to the channel control structures.  getchns() also locks the mutexs on those channels.

After calling getchns(), dsp_io_ops() checks to see if a NULL pointer was returns, OR if channels flags field does not have CHN_F_BUSY set.  If either of those are true, it release Giant and exits.  That's fine if the pointer is NULL, since the lock wouldn't have happened but if the BUSY flag is not set, then dsp_io_ops() (and dsp_read/write()) returns with the channel still locked.

In my driver, I am seeing kernel panics in some situations with this message as a result:
Sleeping thread (tid xxx, pid yyy) owns a non-sleepable lock

Is this a bug in the sound driver?  Should it check these two conditions separately and call relchns() if it fails due to the BUSY flag?


--
Stephen Wall
Senior Staff Software Engineer
585.924.7550
[1600261365931]
REDCOM Laboratories, Inc.
One Redcom Center
Victor, NY 14564-0995
www.redcom.com

DUNS 09-166-5919 | CAGE 1U548
Woman Owned Small Business
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: bug in sound driver?

Wall, Stephen
> In sys/dev/sound/pcm/dsp.c, the funcion dap_io_ops() (called by dsp_read() and dsp_write())

Sorry, 'dsp_io_ops()'
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: bug in sound driver?

Konstantin Belousov
In reply to this post by Wall, Stephen
On Thu, Nov 19, 2020 at 11:24:13PM +0000, Wall, Stephen wrote:

> I am working on a driver that is a modified copy of the sound driver, and I've found something that might be a bug.
>
> In sys/dev/sound/pcm/dsp.c, the funcion dap_io_ops() (called by dsp_read() and dsp_write()) calls getchns() to get pointers to the channel control structures.  getchns() also locks the mutexs on those channels.
>
> After calling getchns(), dsp_io_ops() checks to see if a NULL pointer was returns, OR if channels flags field does not have CHN_F_BUSY set.  If either of those are true, it release Giant and exits.  That's fine if the pointer is NULL, since the lock wouldn't have happened but if the BUSY flag is not set, then dsp_io_ops() (and dsp_read/write()) returns with the channel still locked.
>
> In my driver, I am seeing kernel panics in some situations with this message as a result:
> Sleeping thread (tid xxx, pid yyy) owns a non-sleepable lock
>
> Is this a bug in the sound driver?  Should it check these two conditions separately and call relchns() if it fails due to the BUSY flag?
>

I think you are right, please check that the following fixes the issue:

diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
index 4df035f99c8..0593a585b0f 100644
--- a/sys/dev/sound/pcm/dsp.c
+++ b/sys/dev/sound/pcm/dsp.c
@@ -857,6 +857,8 @@ dsp_io_ops(struct cdev *i_dev, struct uio *buf)
  getchns(i_dev, &rdch, &wrch, prio);
 
  if (*ch == NULL || !((*ch)->flags & CHN_F_BUSY)) {
+ if (rdch != NULL || wrch != NULL)
+ relchns(i_dev, rdch, wrch, prio);
  PCM_GIANT_EXIT(d);
  return (EBADF);
  }
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: bug in sound driver?

Wall, Stephen
On Friday, November 20, 2020 6:34 AM, Konstantin Belousov wrote:

> diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
> index 4df035f99c8..0593a585b0f 100644
> --- a/sys/dev/sound/pcm/dsp.c
> +++ b/sys/dev/sound/pcm/dsp.c
> @@ -857,6 +857,8 @@ dsp_io_ops(struct cdev *i_dev, struct uio *buf)
>          getchns(i_dev, &rdch, &wrch, prio);
>
>          if (*ch == NULL || !((*ch)->flags & CHN_F_BUSY)) {
> +               if (rdch != NULL || wrch != NULL)
> +                       relchns(i_dev, rdch, wrch, prio);
>                  PCM_GIANT_EXIT(d);
>                  return (EBADF);
>         }

That's better than what I used, I hadn't considered that getchns() might return only one channel.  I've confirmed that this eliminates the "non-sleepable lock" panic in our driver.  Note that I've never seen this with the sound driver, only with our copy of it.  But, I also don't have any USB audio devices (aside from the device we are building, which is not supported by the sound driver).

-spw
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
To unsubscribe, send any mail to "[hidden email]"
Reply | Threaded
Open this post in threaded view
|

Re: bug in sound driver?

Konstantin Belousov
On Fri, Nov 20, 2020 at 01:44:26PM +0000, Wall, Stephen wrote:

> On Friday, November 20, 2020 6:34 AM, Konstantin Belousov wrote:
> > diff --git a/sys/dev/sound/pcm/dsp.c b/sys/dev/sound/pcm/dsp.c
> > index 4df035f99c8..0593a585b0f 100644
> > --- a/sys/dev/sound/pcm/dsp.c
> > +++ b/sys/dev/sound/pcm/dsp.c
> > @@ -857,6 +857,8 @@ dsp_io_ops(struct cdev *i_dev, struct uio *buf)
> >          getchns(i_dev, &rdch, &wrch, prio);
> >
> >          if (*ch == NULL || !((*ch)->flags & CHN_F_BUSY)) {
> > +               if (rdch != NULL || wrch != NULL)
> > +                       relchns(i_dev, rdch, wrch, prio);
> >                  PCM_GIANT_EXIT(d);
> >                  return (EBADF);
> >         }
>
> That's better than what I used, I hadn't considered that getchns() might return only one channel.  I've confirmed that this eliminates the "non-sleepable lock" panic in our driver.  Note that I've never seen this with the sound driver, only with our copy of it.  But, I also don't have any USB audio devices (aside from the device we are building, which is not supported by the sound driver).

Committed as r367892, thank you for the report.
_______________________________________________
[hidden email] mailing list
https://lists.freebsd.org/mailman/listinfo/freebsd-drivers
To unsubscribe, send any mail to "[hidden email]"