| Message ID | 1455370937-501-3-git-send-email-martin@mail.zuhause (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, 13 Feb 2016 14:42:17 +0100, Martin Koegler wrote: > > From: Martin Koegler <martin.koegler@chello.at> > > rawmidi devices expose the card number via IOCTLs, which allows to > find the corresponding device in sysfs. > > The sequencer provides no identifing data. Chromium works around this > issue by scanning rawmidi as well as sequencer devices and matching > them by using assumtions, how the kernel register sequencer devices. > > This changes adds support for exposing the card number for kernel clients > as well as the PID for user client. > > The minor of the API version is changed to distinguish between the zero > initialised reserved field and card number 0. > > Signed-off-by: Martin Koegler <martin.koegler@chello.at> The idea looks good. One remaining question is whether only providing the card number or pid is sufficient for all cases. Some review comments below: > --- > include/uapi/sound/asequencer.h | 5 +++-- > sound/core/seq/seq_clientmgr.c | 18 ++++++++++++++++++ > sound/core/seq/seq_clientmgr.h | 2 ++ > 3 files changed, 23 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/sound/asequencer.h b/include/uapi/sound/asequencer.h > index 5a5fa49..4726579 100644 > --- a/include/uapi/sound/asequencer.h > +++ b/include/uapi/sound/asequencer.h > @@ -25,7 +25,7 @@ > #include <sound/asound.h> > > /** version of the sequencer */ > -#define SNDRV_SEQ_VERSION SNDRV_PROTOCOL_VERSION (1, 0, 1) > +#define SNDRV_SEQ_VERSION SNDRV_PROTOCOL_VERSION (1, 0, 2) > > /** > * definition of sequencer event types > @@ -357,7 +357,8 @@ struct snd_seq_client_info { > unsigned char event_filter[32]; /* event filter bitmap */ > int num_ports; /* RO: number of ports */ > int event_lost; /* number of lost events */ > - char reserved[64]; /* for future use */ > + int owner; /* RO: card number[kernel] / PID[user] */ > + char reserved[64 - sizeof(int)]; /* for future use */ The sizeof(int) is always 4. So let's make it explicit. But, please make sure that this change won't lead to any incompatibility. Some architectures might align with 64bit long, although the 32bit int and the rest char[] should be fine on all known architectures, AFAIK. > }; > > > diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c > index 58e79e0..ac07ab7 100644 > --- a/sound/core/seq/seq_clientmgr.c > +++ b/sound/core/seq/seq_clientmgr.c > @@ -364,6 +364,7 @@ static int snd_seq_open(struct inode *inode, struct file *file) > /* fill client data */ > user->file = file; > sprintf(client->name, "Client-%d", c); > + client->data.user.owner = get_pid(task_pid(current)); > > /* make others aware this new client */ > snd_seq_system_client_ev_client_start(c); > @@ -380,6 +381,7 @@ static int snd_seq_release(struct inode *inode, struct file *file) > seq_free_client(client); > if (client->data.user.fifo) > snd_seq_fifo_delete(&client->data.user.fifo); > + put_pid(client->data.user.owner); > kfree(client); > } Shouldn't the counterpart (put_pid()) be called anywhere else? thanks, Takashi
Takashi Iwai wrote: > Martin Koegler wrote: >> rawmidi devices expose the card number via IOCTLs, which allows to >> find the corresponding device in sysfs. >> >> The sequencer provides no identifing data. Chromium works around this >> issue by scanning rawmidi as well as sequencer devices and matching >> them by using assumtions, how the kernel register sequencer devices. >> >> This changes adds support for exposing the card number for kernel clients >> as well as the PID for user client. >> >> The minor of the API version is changed to distinguish between the zero >> initialised reserved field and card number 0. >> >> Signed-off-by: Martin Koegler <martin.koegler@chello.at> > >> +++ b/include/uapi/sound/asequencer.h >> @@ -357,7 +357,8 @@ struct snd_seq_client_info { >> - char reserved[64]; /* for future use */ >> + int owner; /* RO: card number[kernel] / PID[user] */ >> + char reserved[64 - sizeof(int)]; /* for future use */ > > The sizeof(int) is always 4. So let's make it explicit. > > But, please make sure that this change won't lead to any > incompatibility. Some architectures might align with 64bit long, > although the 32bit int and the rest char[] should be fine on all known > architectures, AFAIK. Or just make it two fields, card and pid. The two values are currently exclusive, but there is no technical reason for this. Regards, Clemens
On Mon, Feb 15, 2016 at 11:30:15AM +0100, Takashi Iwai wrote: > The idea looks good. One remaining question is whether only providing > the card number or pid is sufficient for all cases. I only care about the kernel client. > > @@ -357,7 +357,8 @@ struct snd_seq_client_info { > > unsigned char event_filter[32]; /* event filter bitmap */ > > int num_ports; /* RO: number of ports */ > > int event_lost; /* number of lost events */ > > - char reserved[64]; /* for future use */ > > + int owner; /* RO: card number[kernel] / PID[user] */ > > + char reserved[64 - sizeof(int)]; /* for future use */ > > The sizeof(int) is always 4. So let's make it explicit. > > But, please make sure that this change won't lead to any > incompatibility. Some architectures might align with 64bit long, > although the 32bit int and the rest char[] should be fine on all known > architectures, AFAIK. Sorry, I don't have access to the various non x86-architectures, so I can't test. I will change to int card; int pid; char reserve[56]; More safety would just provide a much more complex (ugly?) structure: struct _int_snd_seq_client_info_old { int client; /* client number to inquire */ snd_seq_client_type_t type; /* client type */ char name[64]; /* client name */ unsigned int filter; /* filter flags */ unsigned char multicast_filter[8]; /* multicast filter bitmap */ unsigned char event_filter[32]; /* event filter bitmap */ int num_ports; /* RO: number of ports */ int event_lost; /* number of lost events */ char reserved[64]; /* for future use */ }; struct _int_snd_seq_client_info_new { int client; /* client number to inquire */ snd_seq_client_type_t type; /* client type */ char name[64]; /* client name */ unsigned int filter; /* filter flags */ unsigned char multicast_filter[8]; /* multicast filter bitmap */ unsigned char event_filter[32]; /* event filter bitmap */ int num_ports; /* RO: number of ports */ int event_lost; /* number of lost events */ int card; int pid; char reserved[64]; /* for future use */ }; struct snd_seq_client_info { int client; /* client number to inquire */ snd_seq_client_type_t type; /* client type */ char name[64]; /* client name */ unsigned int filter; /* filter flags */ unsigned char multicast_filter[8]; /* multicast filter bitmap */ unsigned char event_filter[32]; /* event filter bitmap */ int num_ports; /* RO: number of ports */ int event_lost; /* number of lost events */ int card; int pid; char reserved[64 - sizeof(struct _int_snd_seq_client_info_new) + sizeof(_int_snd_seq_client_info_old)]; /* for future use */ }; This should handle all alignment and type size issues automatically. If you really want this, I can provide the apropriate patches. > > > > diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c > > index 58e79e0..ac07ab7 100644 > > --- a/sound/core/seq/seq_clientmgr.c > > +++ b/sound/core/seq/seq_clientmgr.c > > @@ -364,6 +364,7 @@ static int snd_seq_open(struct inode *inode, struct file *file) > > /* fill client data */ > > user->file = file; > > sprintf(client->name, "Client-%d", c); > > + client->data.user.owner = get_pid(task_pid(current)); > > > > /* make others aware this new client */ > > snd_seq_system_client_ev_client_start(c); > > @@ -380,6 +381,7 @@ static int snd_seq_release(struct inode *inode, struct file *file) > > seq_free_client(client); > > if (client->data.user.fifo) > > snd_seq_fifo_delete(&client->data.user.fifo); > > + put_pid(client->data.user.owner); > > kfree(client); > > } > > Shouldn't the counterpart (put_pid()) be called anywhere else? The only way to free a client structure is seq_free_client1 (static). This is called by snd_seq_open before get_pid [=> irelevant] and seq_free_client (static). seq_free_client is called by snd_seq_release, which calls put_pid and snd_seq_delete_kernel_client, which destroys a kernel client. Are I'm missing a code path? Regards, Martin
On Mon, 15 Feb 2016 19:32:43 +0100, Martin Koegler wrote: > > On Mon, Feb 15, 2016 at 11:30:15AM +0100, Takashi Iwai wrote: > > The idea looks good. One remaining question is whether only providing > > the card number or pid is sufficient for all cases. > > I only care about the kernel client. Well, the question is whether only card number is enough. What if cards provide multiple rawmidi devices? > > > @@ -357,7 +357,8 @@ struct snd_seq_client_info { > > > unsigned char event_filter[32]; /* event filter bitmap */ > > > int num_ports; /* RO: number of ports */ > > > int event_lost; /* number of lost events */ > > > - char reserved[64]; /* for future use */ > > > + int owner; /* RO: card number[kernel] / PID[user] */ > > > + char reserved[64 - sizeof(int)]; /* for future use */ > > > > The sizeof(int) is always 4. So let's make it explicit. > > > > But, please make sure that this change won't lead to any > > incompatibility. Some architectures might align with 64bit long, > > although the 32bit int and the rest char[] should be fine on all known > > architectures, AFAIK. > > Sorry, I don't have access to the various non x86-architectures, so I can't test. Usually it's tested by cross-compiling, so everyone can do it. > > I will change to > int card; > int pid; > char reserve[56]; Yes, this would be better. > More safety would just provide a much more complex (ugly?) structure: > > struct _int_snd_seq_client_info_old { > int client; /* client number to inquire */ > snd_seq_client_type_t type; /* client type */ > char name[64]; /* client name */ > unsigned int filter; /* filter flags */ > unsigned char multicast_filter[8]; /* multicast filter bitmap */ > unsigned char event_filter[32]; /* event filter bitmap */ > int num_ports; /* RO: number of ports */ > int event_lost; /* number of lost events */ > char reserved[64]; /* for future use */ > }; > > struct _int_snd_seq_client_info_new { > int client; /* client number to inquire */ > snd_seq_client_type_t type; /* client type */ > char name[64]; /* client name */ > unsigned int filter; /* filter flags */ > unsigned char multicast_filter[8]; /* multicast filter bitmap */ > unsigned char event_filter[32]; /* event filter bitmap */ > int num_ports; /* RO: number of ports */ > int event_lost; /* number of lost events */ > int card; > int pid; > char reserved[64]; /* for future use */ > }; > > struct snd_seq_client_info { > int client; /* client number to inquire */ > snd_seq_client_type_t type; /* client type */ > char name[64]; /* client name */ > unsigned int filter; /* filter flags */ > unsigned char multicast_filter[8]; /* multicast filter bitmap */ > unsigned char event_filter[32]; /* event filter bitmap */ > int num_ports; /* RO: number of ports */ > int event_lost; /* number of lost events */ > int card; > int pid; > char reserved[64 - sizeof(struct _int_snd_seq_client_info_new) + sizeof(_int_snd_seq_client_info_old)]; /* for future use */ > }; > > This should handle all alignment and type size issues automatically. You'd need to define old and new structs without reserved field. The calculation still works but it's bogus. > If you really want this, I can provide the apropriate patches. No, I never want such a thing :) > > > > > > diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c > > > index 58e79e0..ac07ab7 100644 > > > --- a/sound/core/seq/seq_clientmgr.c > > > +++ b/sound/core/seq/seq_clientmgr.c > > > @@ -364,6 +364,7 @@ static int snd_seq_open(struct inode *inode, struct file *file) > > > /* fill client data */ > > > user->file = file; > > > sprintf(client->name, "Client-%d", c); > > > + client->data.user.owner = get_pid(task_pid(current)); > > > > > > /* make others aware this new client */ > > > snd_seq_system_client_ev_client_start(c); > > > @@ -380,6 +381,7 @@ static int snd_seq_release(struct inode *inode, struct file *file) > > > seq_free_client(client); > > > if (client->data.user.fifo) > > > snd_seq_fifo_delete(&client->data.user.fifo); > > > + put_pid(client->data.user.owner); > > > kfree(client); > > > } > > > > Shouldn't the counterpart (put_pid()) be called anywhere else? > > The only way to free a client structure is seq_free_client1 (static). > This is called by snd_seq_open before get_pid [=> irelevant] and > seq_free_client (static). > seq_free_client is called by snd_seq_release, which calls put_pid and > snd_seq_delete_kernel_client, which destroys a kernel client. OK, I misread the patch where to place the code. Then this should be fine. Takashi
diff --git a/include/uapi/sound/asequencer.h b/include/uapi/sound/asequencer.h index 5a5fa49..4726579 100644 --- a/include/uapi/sound/asequencer.h +++ b/include/uapi/sound/asequencer.h @@ -25,7 +25,7 @@ #include <sound/asound.h> /** version of the sequencer */ -#define SNDRV_SEQ_VERSION SNDRV_PROTOCOL_VERSION (1, 0, 1) +#define SNDRV_SEQ_VERSION SNDRV_PROTOCOL_VERSION (1, 0, 2) /** * definition of sequencer event types @@ -357,7 +357,8 @@ struct snd_seq_client_info { unsigned char event_filter[32]; /* event filter bitmap */ int num_ports; /* RO: number of ports */ int event_lost; /* number of lost events */ - char reserved[64]; /* for future use */ + int owner; /* RO: card number[kernel] / PID[user] */ + char reserved[64 - sizeof(int)]; /* for future use */ }; diff --git a/sound/core/seq/seq_clientmgr.c b/sound/core/seq/seq_clientmgr.c index 58e79e0..ac07ab7 100644 --- a/sound/core/seq/seq_clientmgr.c +++ b/sound/core/seq/seq_clientmgr.c @@ -364,6 +364,7 @@ static int snd_seq_open(struct inode *inode, struct file *file) /* fill client data */ user->file = file; sprintf(client->name, "Client-%d", c); + client->data.user.owner = get_pid(task_pid(current)); /* make others aware this new client */ snd_seq_system_client_ev_client_start(c); @@ -380,6 +381,7 @@ static int snd_seq_release(struct inode *inode, struct file *file) seq_free_client(client); if (client->data.user.fifo) snd_seq_fifo_delete(&client->data.user.fifo); + put_pid(client->data.user.owner); kfree(client); } @@ -1197,6 +1199,21 @@ static void get_client_info(struct snd_seq_client *cptr, info->event_lost = cptr->event_lost; memcpy(info->event_filter, cptr->event_filter, 32); info->num_ports = cptr->num_ports; + + switch (cptr->type) { + case USER_CLIENT: + info->owner = pid_vnr(cptr->data.user.owner); + break; + + case KERNEL_CLIENT: + info->owner = cptr->data.kernel.card ? cptr->data.kernel.card->number : -1; + break; + + default: + info->owner = -1; + break; + } + memset(info->reserved, 0, sizeof(info->reserved)); } @@ -2271,6 +2288,7 @@ int snd_seq_create_kernel_client(struct snd_card *card, int client_index, client->accept_input = 1; client->accept_output = 1; + client->data.kernel.card = card; va_start(args, name_fmt); vsnprintf(client->name, sizeof(client->name), name_fmt, args); diff --git a/sound/core/seq/seq_clientmgr.h b/sound/core/seq/seq_clientmgr.h index 20f0a72..031462e 100644 --- a/sound/core/seq/seq_clientmgr.h +++ b/sound/core/seq/seq_clientmgr.h @@ -33,6 +33,7 @@ struct snd_seq_user_client { struct file *file; /* file struct of client */ /* ... */ + struct pid* owner; /* fifo */ struct snd_seq_fifo *fifo; /* queue for incoming events */ @@ -41,6 +42,7 @@ struct snd_seq_user_client { struct snd_seq_kernel_client { /* ... */ + struct snd_card* card; };