From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Iwai Subject: Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation Date: Thu, 09 Apr 2015 10:17:50 +0200 Message-ID: References: <1428512108-1852-1-git-send-email-o-takashi@sakamocchi.jp> <1428512108-1852-3-git-send-email-o-takashi@sakamocchi.jp> <55262BAA.9060004@sakamocchi.jp> 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 765E12651D2 for ; Thu, 9 Apr 2015 10:17:52 +0200 (CEST) In-Reply-To: <55262BAA.9060004@sakamocchi.jp> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org To: Takashi Sakamoto Cc: alsa-devel@alsa-project.org, clemens@ladisch.de List-Id: alsa-devel@alsa-project.org At Thu, 09 Apr 2015 16:35:06 +0900, Takashi Sakamoto wrote: > > On Apr 09 2015 14:33, Takashi Iwai wrote: > > At Thu, 9 Apr 2015 01:55:08 +0900, > > Takashi Sakamoto wrote: > >> > >> snd_ctl_read() has nested loop and complicated condition for return > >> status. This is not better for reading. > >> > >> This commit applies refactoring with two loops. > >> > >> Signed-off-by: Takashi Sakamoto > >> --- > >> sound/core/control.c | 74 ++++++++++++++++++++++++++++++++-------------------- > >> 1 file changed, 46 insertions(+), 28 deletions(-) > >> > >> diff --git a/sound/core/control.c b/sound/core/control.c > >> index de19d56..6870baf 100644 > >> --- a/sound/core/control.c > >> +++ b/sound/core/control.c > >> @@ -1520,58 +1520,76 @@ static ssize_t snd_ctl_read(struct file *file, char __user *buffer, > >> size_t count, loff_t * offset) > >> { > >> struct snd_ctl_file *ctl; > >> - int err = 0; > >> - ssize_t result = 0; > >> + struct snd_ctl_event ev; > >> + struct snd_kctl_event *kev; > >> + wait_queue_t wait; > >> + size_t result; > >> + int err; > >> > >> ctl = file->private_data; > >> if (snd_BUG_ON(!ctl || !ctl->card)) > >> return -ENXIO; > >> if (!ctl->subscribed) > >> return -EBADFD; > >> + > >> + /* The size of given buffer should be larger than at least one event. */ > >> if (count < sizeof(struct snd_ctl_event)) > >> return -EINVAL; > >> + > >> + /* Block till any events were queued. */ > >> spin_lock_irq(&ctl->read_lock); > >> - while (count >= sizeof(struct snd_ctl_event)) { > >> - struct snd_ctl_event ev; > >> - struct snd_kctl_event *kev; > >> - while (list_empty(&ctl->events)) { > >> - wait_queue_t wait; > >> - if ((file->f_flags & O_NONBLOCK) != 0 || result > 0) { > >> - err = -EAGAIN; > >> - goto __end_lock; > >> - } > >> - init_waitqueue_entry(&wait, current); > >> - add_wait_queue(&ctl->change_sleep, &wait); > >> - set_current_state(TASK_INTERRUPTIBLE); > >> + while (list_empty(&ctl->events)) { > >> + if (file->f_flags & O_NONBLOCK) { > >> + spin_unlock_irq(&ctl->read_lock); > >> + return -EAGAIN; > > > > It's better not to spread the unlock call allover the places but just > > "goto unlock". > > > >> + } > >> + > >> + /* The card was disconnected. The queued events are dropped. */ > >> + if (ctl->card->shutdown) { > >> spin_unlock_irq(&ctl->read_lock); > >> - schedule(); > >> - remove_wait_queue(&ctl->change_sleep, &wait); > >> - if (ctl->card->shutdown) > >> - return -ENODEV; > >> - if (signal_pending(current)) > >> - return -ERESTARTSYS; > >> - spin_lock_irq(&ctl->read_lock); > >> + return -ENODEV; > >> } > >> + > >> + > >> + init_waitqueue_entry(&wait, current); > >> + add_wait_queue(&ctl->change_sleep, &wait); > >> + set_current_state(TASK_INTERRUPTIBLE); > >> + spin_unlock_irq(&ctl->read_lock); > >> + > >> + schedule(); > >> + > >> + remove_wait_queue(&ctl->change_sleep, &wait); > >> + > >> + if (signal_pending(current)) > >> + return -ERESTARTSYS; > >> + > >> + spin_lock_irq(&ctl->read_lock); > >> + } > >> + > >> + /* Copy events till the buffer filled, or no events are remained. */ > >> + result = 0; > >> + while (count >= sizeof(struct snd_ctl_event)) { > >> + if (list_empty(&ctl->events)) > >> + break; > >> kev = snd_kctl_event(ctl->events.next); > >> + list_del(&kev->list); > >> + > >> ev.type = SNDRV_CTL_EVENT_ELEM; > >> ev.data.elem.mask = kev->mask; > >> ev.data.elem.id = kev->id; > >> - list_del(&kev->list); > >> - spin_unlock_irq(&ctl->read_lock); > >> kfree(kev); > >> if (copy_to_user(buffer, &ev, sizeof(struct snd_ctl_event))) { > >> err = -EFAULT; > >> - goto __end; > >> + break; > >> } > >> - spin_lock_irq(&ctl->read_lock); > >> + > >> buffer += sizeof(struct snd_ctl_event); > >> count -= sizeof(struct snd_ctl_event); > >> result += sizeof(struct snd_ctl_event); > >> } > >> - __end_lock: > >> + > >> spin_unlock_irq(&ctl->read_lock); > >> - __end: > >> - return result > 0 ? result : err; > >> + return result; > > > > You can't ignore the error. -EFAULT should be still returned when it > > happens at the first read. > > Exactly. It's my fault. I should have assign it to result, instead of err. > > Well, I'd like to post revised version, but it's just before a week to > open merge window for Linux 4.01. Should I postpone the posting a few > weeks later? These two patches are fine to take now as they are no intrusive changes. thanks, Takashi