| Message ID | 1423494126-24054-1-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
At Tue, 10 Feb 2015 00:02:06 +0900, Takashi Sakamoto wrote: > > When event originator doesn't set numerical ID in identical information, > the event data includes no numerical ID, thus userspace applications > cannot identify the control just by unique ID in event data. > > This commit fix this bug so as the event data includes all of identical > information. > > Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> You can't overwrite id pointer in this case. It's a caller's object. One way to fix would be to copy the id instance. Another way would be to change up/down_write() with _read(), and include snd_ctl_notify() call with kctl->id inside the semaphore lock. The former would be less changes but consume the stack significantly. thanks, Takashi > --- > sound/core/control.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/sound/core/control.c b/sound/core/control.c > index 6a72b3e..884fddd 100644 > --- a/sound/core/control.c > +++ b/sound/core/control.c > @@ -587,6 +587,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, > } > ret = 1; > unlock: > + *id = kctl->id; > up_write(&card->controls_rwsem); > if (ret > 0) > snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); > -- > 2.1.0 >
On Feb 10 2015 00:28, Takashi Iwai wrote: > At Tue, 10 Feb 2015 00:02:06 +0900, > Takashi Sakamoto wrote: >> >> When event originator doesn't set numerical ID in identical information, >> the event data includes no numerical ID, thus userspace applications >> cannot identify the control just by unique ID in event data. >> >> This commit fix this bug so as the event data includes all of identical >> information. >> >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > You can't overwrite id pointer in this case. It's a caller's object. Exactly. I missed it... > One way to fix would be to copy the id instance. Another way would be > to change up/down_write() with _read(), and include snd_ctl_notify() > call with kctl->id inside the semaphore lock. > > The former would be less changes but consume the stack significantly. If any sub-effects are allowed, we can use caller's data via the pointer, like: memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id); Then: snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); But this is not better. Mmm... > thanks, > > Takashi > > >> --- >> sound/core/control.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/sound/core/control.c b/sound/core/control.c >> index 6a72b3e..884fddd 100644 >> --- a/sound/core/control.c >> +++ b/sound/core/control.c >> @@ -587,6 +587,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, >> } >> ret = 1; >> unlock: >> + *id = kctl->id; >> up_write(&card->controls_rwsem); >> if (ret > 0) >> snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); >> -- >> 2.1.0
At Tue, 10 Feb 2015 01:01:15 +0900, Takashi Sakamoto wrote: > > On Feb 10 2015 00:28, Takashi Iwai wrote: > > At Tue, 10 Feb 2015 00:02:06 +0900, > > Takashi Sakamoto wrote: > >> > >> When event originator doesn't set numerical ID in identical information, > >> the event data includes no numerical ID, thus userspace applications > >> cannot identify the control just by unique ID in event data. > >> > >> This commit fix this bug so as the event data includes all of identical > >> information. > >> > >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > > > You can't overwrite id pointer in this case. It's a caller's object. > > Exactly. I missed it... > > > One way to fix would be to copy the id instance. Another way would be > > to change up/down_write() with _read(), and include snd_ctl_notify() > > call with kctl->id inside the semaphore lock. > > > > The former would be less changes but consume the stack significantly. > > If any sub-effects are allowed, we can use caller's data via the > pointer, like: > memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id); > Then: > snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); That's what I suggested as the first example. Takashi
At Mon, 09 Feb 2015 17:02:51 +0100, Takashi Iwai wrote: > > At Tue, 10 Feb 2015 01:01:15 +0900, > Takashi Sakamoto wrote: > > > > On Feb 10 2015 00:28, Takashi Iwai wrote: > > > At Tue, 10 Feb 2015 00:02:06 +0900, > > > Takashi Sakamoto wrote: > > >> > > >> When event originator doesn't set numerical ID in identical information, > > >> the event data includes no numerical ID, thus userspace applications > > >> cannot identify the control just by unique ID in event data. > > >> > > >> This commit fix this bug so as the event data includes all of identical > > >> information. > > >> > > >> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > > > > > You can't overwrite id pointer in this case. It's a caller's object. > > > > Exactly. I missed it... > > > > > One way to fix would be to copy the id instance. Another way would be > > > to change up/down_write() with _read(), and include snd_ctl_notify() > > > call with kctl->id inside the semaphore lock. > > > > > > The former would be less changes but consume the stack significantly. > > > > If any sub-effects are allowed, we can use caller's data via the > > pointer, like: > > memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id); > > Then: > > snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); > > That's what I suggested as the first example. ... and I think this is a better way in the end. Simpler, better. The stack usage is very likely acceptable. Takashi
On Feb 10 2015 01:18, Takashi Iwai wrote: > At Mon, 09 Feb 2015 17:02:51 +0100, > Takashi Iwai wrote: >> >> At Tue, 10 Feb 2015 01:01:15 +0900, >> Takashi Sakamoto wrote: >>> >>> On Feb 10 2015 00:28, Takashi Iwai wrote: >>>> At Tue, 10 Feb 2015 00:02:06 +0900, >>>> Takashi Sakamoto wrote: >>>>> >>>>> When event originator doesn't set numerical ID in identical information, >>>>> the event data includes no numerical ID, thus userspace applications >>>>> cannot identify the control just by unique ID in event data. >>>>> >>>>> This commit fix this bug so as the event data includes all of identical >>>>> information. >>>>> >>>>> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> >>>> >>>> You can't overwrite id pointer in this case. It's a caller's object. >>> >>> Exactly. I missed it... >>> >>>> One way to fix would be to copy the id instance. Another way would be >>>> to change up/down_write() with _read(), and include snd_ctl_notify() >>>> call with kctl->id inside the semaphore lock. >>>> >>>> The former would be less changes but consume the stack significantly. >>> >>> If any sub-effects are allowed, we can use caller's data via the >>> pointer, like: >>> memcpy(id, &kctl->id, sizeof(struct snd_ctl_elem_id); >>> Then: >>> snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id); >> >> That's what I suggested as the first example. I misunderstood to use heap. > ... and I think this is a better way in the end. Simpler, better. > The stack usage is very likely acceptable. I think so. I'll submit again with some comments about the sub-effect to callers. Thanks Takashi Sakamoto
diff --git a/sound/core/control.c b/sound/core/control.c index 6a72b3e..884fddd 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -587,6 +587,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id, } ret = 1; unlock: + *id = kctl->id; up_write(&card->controls_rwsem); if (ret > 0) snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
When event originator doesn't set numerical ID in identical information, the event data includes no numerical ID, thus userspace applications cannot identify the control just by unique ID in event data. This commit fix this bug so as the event data includes all of identical information. Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> --- sound/core/control.c | 1 + 1 file changed, 1 insertion(+)