| Message ID | 20190412122935.4092-4-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ALSA: seq: Yet a few more fixes / cleanups | expand |
Hi Takashi, On Fri, 12 Apr 2019, Takashi Iwai wrote: > +/* Unlock and unref the given client; for OSS sequencer use only */ > +void snd_seq_client_ioctl_unlock(int clientid) > +{ > + struct snd_seq_client *client; > + > + client = snd_seq_client_use_ptr(clientid); > + if (WARN_ON(!client)) > + return; > + mutex_unlock(&client->ioctl_mutex); > + snd_use_lock_free(&client->use_lock); > + snd_seq_client_unlock(client); is that double-unlock intentional? snd_seq_client_unlock() seems to call the same snd_use_lock_free for the same handle as on the previous line. Br, Kai
On Mon, 15 Apr 2019 08:37:18 +0200, Kai Vehmanen wrote: > > Hi Takashi, > > On Fri, 12 Apr 2019, Takashi Iwai wrote: > > > +/* Unlock and unref the given client; for OSS sequencer use only */ > > +void snd_seq_client_ioctl_unlock(int clientid) > > +{ > > + struct snd_seq_client *client; > > + > > + client = snd_seq_client_use_ptr(clientid); > > + if (WARN_ON(!client)) > > + return; > > + mutex_unlock(&client->ioctl_mutex); > > + snd_use_lock_free(&client->use_lock); > > + snd_seq_client_unlock(client); > > is that double-unlock intentional? snd_seq_client_unlock() seems to call > the same snd_use_lock_free for the same handle as on the previous line. Yes, it's intentional. The trick is snd_seq_client_ioctl_lock() doesn't call snd_seq_client_unlock(), i.e. keeps refcount up. This is, in turn, released in snd_seq_client_ioctl_unlock(). snd_seq_client_ioctl_unlock() gets its refcount at use_ptr(), hence this releases twice. Maybe it's worth for more comments. thanks, Takashi
Hi, On Mon, 15 Apr 2019, Takashi Iwai wrote: > > > +void snd_seq_client_ioctl_unlock(int clientid) > > > +{ > > > + struct snd_seq_client *client; > > > + > > > + client = snd_seq_client_use_ptr(clientid); > > > + if (WARN_ON(!client)) > > > + return; > > > + mutex_unlock(&client->ioctl_mutex); > > > + snd_use_lock_free(&client->use_lock); > > > + snd_seq_client_unlock(client); > > > > is that double-unlock intentional? snd_seq_client_unlock() seems to call > > the same snd_use_lock_free for the same handle as on the previous line. > > Yes, it's intentional. The trick is snd_seq_client_ioctl_lock() > doesn't call snd_seq_client_unlock(), i.e. keeps refcount up. This > is, in turn, released in snd_seq_client_ioctl_unlock(). > snd_seq_client_ioctl_unlock() gets its refcount at use_ptr(), hence > this releases twice. Maybe it's worth for more comments. ah yes, ok thanks, just checking. :) I was misled by the asymmetric names. As you are taking the refcount with two client_use_ptr() calls in both lock and unlock, would it be more logical to have two snd_seq_client_unlock() calls in unlock? Otherwise patchset looks good!
On Mon, 15 Apr 2019 10:52:01 +0200, Kai Vehmanen wrote: > > Hi, > > On Mon, 15 Apr 2019, Takashi Iwai wrote: > > > > > +void snd_seq_client_ioctl_unlock(int clientid) > > > > +{ > > > > + struct snd_seq_client *client; > > > > + > > > > + client = snd_seq_client_use_ptr(clientid); > > > > + if (WARN_ON(!client)) > > > > + return; > > > > + mutex_unlock(&client->ioctl_mutex); > > > > + snd_use_lock_free(&client->use_lock); > > > > + snd_seq_client_unlock(client); > > > > > > is that double-unlock intentional? snd_seq_client_unlock() seems to call > > > the same snd_use_lock_free for the same handle as on the previous line. > > > > Yes, it's intentional. The trick is snd_seq_client_ioctl_lock() > > doesn't call snd_seq_client_unlock(), i.e. keeps refcount up. This > > is, in turn, released in snd_seq_client_ioctl_unlock(). > > snd_seq_client_ioctl_unlock() gets its refcount at use_ptr(), hence > > this releases twice. Maybe it's worth for more comments. > > ah yes, ok thanks, just checking. :) I was misled by the asymmetric > names. As you are taking the refcount with two client_use_ptr() calls in > both lock and unlock, would it be more logical to have two > snd_seq_client_unlock() calls in unlock? Well I thought it might be misleading to call twice, but yeah, that's rather confusing. I'll change to two sequential unlock calls with more comments. thanks, Takashi
diff --git a/sound/core/seq/oss/seq_oss_device.h b/sound/core/seq/oss/seq_oss_device.h index 2d0e9eaf13aa..77eb1fe1155c 100644 --- a/sound/core/seq/oss/seq_oss_device.h +++ b/sound/core/seq/oss/seq_oss_device.h @@ -30,6 +30,7 @@ #include <sound/rawmidi.h> #include <sound/seq_kernel.h> #include <sound/info.h> +#include "../seq_clientmgr.h" /* max. applications */ #define SNDRV_SEQ_OSS_MAX_CLIENTS 16 @@ -150,11 +151,16 @@ snd_seq_oss_dispatch(struct seq_oss_devinfo *dp, struct snd_seq_event *ev, int a return snd_seq_kernel_client_dispatch(dp->cseq, ev, atomic, hop); } -/* ioctl */ +/* ioctl for writeq */ static inline int snd_seq_oss_control(struct seq_oss_devinfo *dp, unsigned int type, void *arg) { - return snd_seq_kernel_client_ctl(dp->cseq, type, arg); + int err; + + snd_seq_client_ioctl_lock(dp->cseq); + err = snd_seq_kernel_client_ctl(dp->cseq, type, arg); + snd_seq_client_ioctl_unlock(dp->cseq); + return err; } /* fill the addresses in header */ diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 0af5b1440b33..a5c9d59eb5b8 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -179,6 +179,36 @@ struct snd_seq_client *snd_seq_client_use_ptr(int clientid) return client; } +/* Take refcount and perform ioctl_mutex lock on the given client; + * used only for OSS sequencer + * Unlock via snd_seq_client_ioctl_unlock() below + */ +bool snd_seq_client_ioctl_lock(int clientid) +{ + struct snd_seq_client *client; + + client = snd_seq_client_use_ptr(clientid); + if (!client) + return false; + mutex_lock(&client->ioctl_mutex); + return true; +} +EXPORT_SYMBOL_GPL(snd_seq_client_ioctl_lock); + +/* Unlock and unref the given client; for OSS sequencer use only */ +void snd_seq_client_ioctl_unlock(int clientid) +{ + struct snd_seq_client *client; + + client = snd_seq_client_use_ptr(clientid); + if (WARN_ON(!client)) + return; + mutex_unlock(&client->ioctl_mutex); + snd_use_lock_free(&client->use_lock); + snd_seq_client_unlock(client); +} +EXPORT_SYMBOL_GPL(snd_seq_client_ioctl_unlock); + static void usage_alloc(struct snd_seq_usage *res, int num) { res->cur += num; @@ -2247,11 +2277,15 @@ int snd_seq_kernel_client_enqueue(int client, struct snd_seq_event *ev, if (cptr == NULL) return -EINVAL; - if (! cptr->accept_output) + if (!cptr->accept_output) { result = -EPERM; - else /* send it */ + } else { /* send it */ + mutex_lock(&cptr->ioctl_mutex); result = snd_seq_client_enqueue_event(cptr, ev, file, blocking, - false, 0, NULL); + false, 0, + &cptr->ioctl_mutex); + mutex_unlock(&cptr->ioctl_mutex); + } snd_seq_client_unlock(cptr); return result; diff --git a/sound/core/seq/seq_clientmgr.h b/sound/core/seq/seq_clientmgr.h index 66ad3d547916..28a51dcc0190 100644 --- a/sound/core/seq/seq_clientmgr.h +++ b/sound/core/seq/seq_clientmgr.h @@ -97,6 +97,10 @@ int snd_seq_kernel_client_write_poll(int clientid, struct file *file, poll_table int snd_seq_client_notify_subscription(int client, int port, struct snd_seq_port_subscribe *info, int evtype); +/* only for OSS sequencer */ +bool snd_seq_client_ioctl_lock(int clientid); +void snd_seq_client_ioctl_unlock(int clientid); + extern int seq_client_load[15]; #endif
OSS sequencer emulation still allows to queue and issue the events that manipulate the client pool concurrently in a racy way. This patch serializes the access like the normal sequencer write / ioctl via taking the client ioctl_mutex. Since the access to the sequencer client is done indirectly via a client id number, a new helper to take/release the mutex is introduced. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/seq/oss/seq_oss_device.h | 10 ++++++++-- sound/core/seq/seq_clientmgr.c | 40 ++++++++++++++++++++++++++++++++++--- sound/core/seq/seq_clientmgr.h | 4 ++++ 3 files changed, 49 insertions(+), 5 deletions(-)