Re: [PATCH 1/4] ALSA: ctl: confirm to return all identical information

[Takashi Iwai <tiwai@suse.de>]

At Fri, 10 Apr 2015 21:00:22 +0900,
Takashi Sakamoto wrote:
> 
> On 2015年04月09日 14:36, Takashi Iwai wrote:
> > At Thu,  9 Apr 2015 02:07:15 +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>
> >> ---
> >>  sound/core/control.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/sound/core/control.c b/sound/core/control.c
> >> index d677c27..6d12e85 100644
> >> --- a/sound/core/control.c
> >> +++ b/sound/core/control.c
> >> @@ -578,6 +578,7 @@ error:
> >>   *
> >>   * Finds the control instance with the given id, and activate or
> >>   * inactivate the control together with notification, if changed.
> >> + * The given ID data is filled by full information.
> >>   *
> >>   * Return: 0 if unchanged, 1 if changed, or a negative error code on failure.
> >>   */
> >> @@ -609,6 +610,7 @@ int snd_ctl_activate_id(struct snd_card *card, struct snd_ctl_elem_id *id,
> >>  	}
> >>  	ret = 1;
> >>   unlock:
> >> +	*id = kctl->id;
> > 
> > This isn't always correct.  When the element is looked by a numid and
> > a kctl has multiple items (count > 0), this will overwrite with a
> > wrong numid.
> 
> It also overwrites with a wrong index, too. I should have used a
> combination of snd_ctl_get_ioff() and snd_ctl_build_ioff() for this
> purpose. Thanks.
> 
> > Takashi
> > 
> >>  	up_write(&card->controls_rwsem);
> >>  	if (ret > 0)
> >>  		snd_ctl_notify(card, SNDRV_CTL_EVENT_MASK_INFO, id);
> >> -- 
> >> 2.1.0
> 
> Well, I'll re-post the patch 01 and 04 with these fixes because the
> others has somewhat changes of API behaviour, expecially patch 03. I'll
> postpone it to next developing period.
> 
> But how about patch 02? I think filling all identical information gives
> advantage to userspace applications, instead of a part of given
> information given.

If each of them should be applied individually, let me know.
But I think this may have the very same problem as patch 1.

> And I have no confidence about giving index value in add operations for
> userspace. In my understanding, the index is an offset from the first
> element in the element set.

No, the index number is just to allow multiple kctls with the same
name.  Using counts > 1 is only one of the implementations for multi
indices.  You can implement one kctl per index, too.  (Many drivers do
so.)


Takashi

> But actually, via the add operation, we can
> add new userspace element with an arbitrary index, like:
> 
> $ amixer controls
> (added by an operation with index=0. As a result, 11 is given as numid
> for this element set.)
> numid=11,iface=MIXER,name='my-enum-element'
> numid=12,iface=MIXER,name='my-enum-element',index=1
> numid=13,iface=MIXER,name='my-enum-element',index=2
> numid=14,iface=MIXER,name='my-enum-element',index=3
>  - there're some other elements. -
> (added by another operation with index=5. As a result, 21 is given as
> numid for this element set.)
> numid=21,iface=MIXER,name='my-enum-element',index=4
> numid=22,iface=MIXER,name='my-enum-element',index=5
> numid=23,iface=MIXER,name='my-enum-element',index=6
> numid=24,iface=MIXER,name='my-enum-element',index=7
> (FYI, addition with index=1-8 causes EBUSY.)
> 
> Of cource, we can do this.
> (added by an operation with 100. As a result, 0 is given as numid for
> this element set.)
> numid=0,iface=MIXER,name='my-enum-element',index=100
> numid=1,iface=MIXER,name='my-enum-element',index=101
> numid=2,iface=MIXER,name='my-enum-element',index=102
> numid=3,iface=MIXER,name='my-enum-element',index=103
> 
> Is this behaviour legal or not?
> 
> # I'm sorry to post these questions just before opening next merge
> window but it takes me
> # a long time to understand ALSA control interface and prepare for some
> helper programs
> # to confirm its behaviours...
> 
> 
> Thanks
> 
> Takashi Sakamoto
> 
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel