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 07:33:35 +0200 Message-ID: References: <1428512108-1852-1-git-send-email-o-takashi@sakamocchi.jp> <1428512108-1852-3-git-send-email-o-takashi@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 E418E26578E for ; Thu, 9 Apr 2015 07:33:36 +0200 (CEST) In-Reply-To: <1428512108-1852-3-git-send-email-o-takashi@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, 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. Takashi