| Message ID | 20170522155224.4810-1-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On May 23 2017 00:52, Takashi Iwai wrote: > This patch simplifies the code of snd_ctl_elem_list() in the following > ways: > > - Avoid a vmalloc() temporary buffer but do copy in each iteration; > the vmalloc buffer was introduced at the time we took the spinlock > for the ctl element management. > > - Use the standard list_for_each_entry() macro > > - Merge two loops into one; > it used to be a loop for skipping until offset becomes zero and > another loop to copy the data. They can be folded into a single > loop easily. > > Signed-off-by: Takashi Iwai <tiwai@suse.de> Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> I have one comment. > --- > sound/core/control.c | 66 +++++++++++++++++++--------------------------------- > 1 file changed, 24 insertions(+), 42 deletions(-) > > diff --git a/sound/core/control.c b/sound/core/control.c > index c109b82eef4b..47080da8451a 100644 > --- a/sound/core/control.c > +++ b/sound/core/control.c > @@ -747,11 +747,11 @@ static int snd_ctl_card_info(struct snd_card *card, struct snd_ctl_file * ctl, > static int snd_ctl_elem_list(struct snd_card *card, > struct snd_ctl_elem_list __user *_list) > { > - struct list_head *plist; > struct snd_ctl_elem_list list; > struct snd_kcontrol *kctl; > - struct snd_ctl_elem_id *dst, *id; > + struct snd_ctl_elem_id id; > unsigned int offset, space, jidx; > + int err = 0; > > if (copy_from_user(&list, _list, sizeof(list))) > return -EFAULT; > @@ -760,52 +760,34 @@ static int snd_ctl_elem_list(struct snd_card *card, > /* try limit maximum space */ > if (space > 16384) > return -ENOMEM; We can get rid of this limitation because allocation in kernel space is not performed anymore. Regards Takashi Sakamoto
On Mon, 22 May 2017 21:55:02 +0200, Takashi Sakamoto wrote: > > Hi, > > On May 23 2017 00:52, Takashi Iwai wrote: > > This patch simplifies the code of snd_ctl_elem_list() in the following > > ways: > > > > - Avoid a vmalloc() temporary buffer but do copy in each iteration; > > the vmalloc buffer was introduced at the time we took the spinlock > > for the ctl element management. > > > > - Use the standard list_for_each_entry() macro > > > > - Merge two loops into one; > > it used to be a loop for skipping until offset becomes zero and > > another loop to copy the data. They can be folded into a single > > loop easily. > > > > Signed-off-by: Takashi Iwai <tiwai@suse.de> > > Reviewed-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > Tested-by: Takashi Sakamoto <o-takashi@sakamocchi.jp> > > I have one comment. > > > --- > > sound/core/control.c | 66 +++++++++++++++++++--------------------------------- > > 1 file changed, 24 insertions(+), 42 deletions(-) > > > > diff --git a/sound/core/control.c b/sound/core/control.c > > index c109b82eef4b..47080da8451a 100644 > > --- a/sound/core/control.c > > +++ b/sound/core/control.c > > @@ -747,11 +747,11 @@ static int snd_ctl_card_info(struct snd_card *card, struct snd_ctl_file * ctl, > > static int snd_ctl_elem_list(struct snd_card *card, > > struct snd_ctl_elem_list __user *_list) > > { > > - struct list_head *plist; > > struct snd_ctl_elem_list list; > > struct snd_kcontrol *kctl; > > - struct snd_ctl_elem_id *dst, *id; > > + struct snd_ctl_elem_id id; > > unsigned int offset, space, jidx; > > + int err = 0; > > > > if (copy_from_user(&list, _list, sizeof(list))) > > return -EFAULT; > > @@ -760,52 +760,34 @@ static int snd_ctl_elem_list(struct snd_card *card, > > /* try limit maximum space */ > > if (space > 16384) > > return -ENOMEM; > > We can get rid of this limitation because allocation in kernel space > is not performed anymore. Yes, likely. thanks, Takashi
diff --git a/sound/core/control.c b/sound/core/control.c index c109b82eef4b..47080da8451a 100644 --- a/sound/core/control.c +++ b/sound/core/control.c @@ -747,11 +747,11 @@ static int snd_ctl_card_info(struct snd_card *card, struct snd_ctl_file * ctl, static int snd_ctl_elem_list(struct snd_card *card, struct snd_ctl_elem_list __user *_list) { - struct list_head *plist; struct snd_ctl_elem_list list; struct snd_kcontrol *kctl; - struct snd_ctl_elem_id *dst, *id; + struct snd_ctl_elem_id id; unsigned int offset, space, jidx; + int err = 0; if (copy_from_user(&list, _list, sizeof(list))) return -EFAULT; @@ -760,52 +760,34 @@ static int snd_ctl_elem_list(struct snd_card *card, /* try limit maximum space */ if (space > 16384) return -ENOMEM; + down_read(&card->controls_rwsem); + list.count = card->controls_count; + list.used = 0; if (space > 0) { - /* allocate temporary buffer for atomic operation */ - dst = vmalloc(space * sizeof(struct snd_ctl_elem_id)); - if (dst == NULL) - return -ENOMEM; - down_read(&card->controls_rwsem); - list.count = card->controls_count; - plist = card->controls.next; - while (plist != &card->controls) { - if (offset == 0) - break; - kctl = snd_kcontrol(plist); - if (offset < kctl->count) - break; - offset -= kctl->count; - plist = plist->next; - } - list.used = 0; - id = dst; - while (space > 0 && plist != &card->controls) { - kctl = snd_kcontrol(plist); - for (jidx = offset; space > 0 && jidx < kctl->count; jidx++) { - snd_ctl_build_ioff(id, kctl, jidx); - id++; - space--; + list_for_each_entry(kctl, &card->controls, list) { + if (offset >= kctl->count) { + offset -= kctl->count; + continue; + } + for (jidx = offset; jidx < kctl->count; jidx++) { + snd_ctl_build_ioff(&id, kctl, jidx); + if (copy_to_user(list.pids + list.used, &id, + sizeof(id))) { + err = -EFAULT; + goto out; + } list.used++; + if (!--space) + goto out; } - plist = plist->next; offset = 0; } - up_read(&card->controls_rwsem); - if (list.used > 0 && - copy_to_user(list.pids, dst, - list.used * sizeof(struct snd_ctl_elem_id))) { - vfree(dst); - return -EFAULT; - } - vfree(dst); - } else { - down_read(&card->controls_rwsem); - list.count = card->controls_count; - up_read(&card->controls_rwsem); } - if (copy_to_user(_list, &list, sizeof(list))) - return -EFAULT; - return 0; + out: + up_read(&card->controls_rwsem); + if (!err && copy_to_user(_list, &list, sizeof(list))) + err = -EFAULT; + return err; } static bool validate_element_member_dimension(struct snd_ctl_elem_info *info)
This patch simplifies the code of snd_ctl_elem_list() in the following ways: - Avoid a vmalloc() temporary buffer but do copy in each iteration; the vmalloc buffer was introduced at the time we took the spinlock for the ctl element management. - Use the standard list_for_each_entry() macro - Merge two loops into one; it used to be a loop for skipping until offset becomes zero and another loop to copy the data. They can be folded into a single loop easily. Signed-off-by: Takashi Iwai <tiwai@suse.de> --- sound/core/control.c | 66 +++++++++++++++++++--------------------------------- 1 file changed, 24 insertions(+), 42 deletions(-)