From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 3/3] ALSA: core: set kcontrol's count field correctly Date: Wed, 24 Aug 2011 14:05:56 +0200 Message-ID: References: <20110824031234.5279.46501.stgit@localhost6.localdomain6> <20110824031243.5279.93859.stgit@localhost6.localdomain6> <20110824061757.GF3551@guanqun-laptop.sh.intel.com> <20110824114613.GA1811@guanqun-laptop.ccr.corp.intel.com> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mx2.suse.de (cantor2.suse.de [195.135.220.15]) by alsa0.perex.cz (Postfix) with ESMTP id ED35610393C for ; Wed, 24 Aug 2011 14:05:56 +0200 (CEST) In-Reply-To: <20110824114613.GA1811@guanqun-laptop.ccr.corp.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: alsa-devel-bounces@alsa-project.org Errors-To: alsa-devel-bounces@alsa-project.org To: Lu Guanqun Cc: ALSA , "clemens@ladisch.de" List-Id: alsa-devel@alsa-project.org At Wed, 24 Aug 2011 19:46:13 +0800, Lu Guanqun wrote: > > On Wed, Aug 24, 2011 at 04:42:05PM +0800, Takashi Iwai wrote: > > --- > > diff --git a/include/sound/asound.h b/include/sound/asound.h > > index 5d6074f..9f5f55d 100644 > > --- a/include/sound/asound.h > > +++ b/include/sound/asound.h > > @@ -813,6 +813,33 @@ struct snd_ctl_elem_info { > > unsigned char reserved[64-4*sizeof(unsigned short)]; > > }; > > > > +/* The struct used for ELEM_ADD and ELEM_REPLACE ioctls; > > + * This is is identical with snd_ctl_elem_info, but just owner field is > > + * different, keeping the number of elements to be created. > > + * The unused fields are stripped for simplicity. > > + */ > > +struct snd_ctl_elem_add_info { > > + struct snd_ctl_elem_id id; /* element ID */ > > + snd_ctl_elem_type_t type; /* value type */ > > + unsigned int access; /* value access (bitmask) */ > > + unsigned int count; /* count of values */ > > + __kernel_pid_t elem_count; /* # of elements to create (0=1) */ > > + union { > > + struct { > > + long min; /* R: minimum value */ > > + long max; /* R: maximum value */ > > + long step; /* R: step (0 variable) */ > > + } integer; > > + struct { > > + long long min; /* R: minimum value */ > > + long long max; /* R: maximum value */ > > + long long step; /* R: step (0 variable) */ > > + } integer64; > > + unsigned char reserved[128]; > > + } value; > > + unsigned char reserved[64]; > > +}; > > + > > struct snd_ctl_elem_value { > > struct snd_ctl_elem_id id; /* W: element ID */ > > unsigned int indirect: 1; /* W: indirect access - obsoleted */ > > @@ -854,8 +881,8 @@ struct snd_ctl_tlv { > > #define SNDRV_CTL_IOCTL_ELEM_LOCK _IOW('U', 0x14, struct snd_ctl_elem_id) > > #define SNDRV_CTL_IOCTL_ELEM_UNLOCK _IOW('U', 0x15, struct snd_ctl_elem_id) > > #define SNDRV_CTL_IOCTL_SUBSCRIBE_EVENTS _IOWR('U', 0x16, int) > > -#define SNDRV_CTL_IOCTL_ELEM_ADD _IOWR('U', 0x17, struct snd_ctl_elem_info) > > -#define SNDRV_CTL_IOCTL_ELEM_REPLACE _IOWR('U', 0x18, struct snd_ctl_elem_info) > > +#define SNDRV_CTL_IOCTL_ELEM_ADD _IOWR('U', 0x17, struct snd_ctl_elem_add_info) > > +#define SNDRV_CTL_IOCTL_ELEM_REPLACE _IOWR('U', 0x18, struct snd_ctl_elem_add_info) > > #define SNDRV_CTL_IOCTL_ELEM_REMOVE _IOWR('U', 0x19, struct snd_ctl_elem_id) > > #define SNDRV_CTL_IOCTL_TLV_READ _IOWR('U', 0x1a, struct snd_ctl_tlv) > > #define SNDRV_CTL_IOCTL_TLV_WRITE _IOWR('U', 0x1b, struct snd_ctl_tlv) > > diff --git a/sound/core/control.c b/sound/core/control.c > > index dc2a440..15c171d 100644 > > --- a/sound/core/control.c > > +++ b/sound/core/control.c > > @@ -1064,7 +1064,7 @@ static void snd_ctl_elem_user_free(struct snd_kcontrol *kcontrol) > > } > > > > static int snd_ctl_elem_add(struct snd_ctl_file *file, > > - struct snd_ctl_elem_info *info, int replace) > > + struct snd_ctl_elem_add_info *info, int replace) > > { > > struct snd_card *card = file->card; > > struct snd_kcontrol kctl, *_kctl; > > @@ -1099,7 +1099,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, > > if (err < 0) > > return err; > > memcpy(&kctl.id, &info->id, sizeof(info->id)); > > - kctl.count = info->owner ? info->owner : 1; > > + kctl.count = info->elem_count ? info->elem_count : 1; > > Hi Takashi, > > I don't how this approach can solve this problem. There is no problem. You can't copy the data from ELEM_INFO for ELEM_ADD/REPLACE. It was a wrong assumption to reuse the data. Instead, you had to set up the struct field manually. That is, the test procedure was simply wrong. > There's still an > implicit assumption here that user space application will clear > info->elem_count to zero before it issues the REPLACE ioctl, right? Yes, the point is that it's no longer same structure. So, you can't copy from the result from ELEM_INFO ioctl any more. Instead, you need to set up the struct field manually. > > access |= SNDRV_CTL_ELEM_ACCESS_USER; > > kctl.info = snd_ctl_elem_user_info; > > if (access & SNDRV_CTL_ELEM_ACCESS_READ) > > @@ -1139,7 +1139,7 @@ static int snd_ctl_elem_add(struct snd_ctl_file *file, > > ue = kzalloc(sizeof(struct user_element) + private_size, GFP_KERNEL); > > if (ue == NULL) > > return -ENOMEM; > > - ue->info = *info; > > + memcpy(&ue->info, info, sizeof(*info)); > > Should we add some assertion here checking the size of these two > structure should be the same? It could, at most by BUILD_BUG_ON(). thanks, Takashi