From mboxrd@z Thu Jan 1 00:00:00 1970 From: Takashi Sakamoto Subject: Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation Date: Thu, 09 Apr 2015 16:35:06 +0900 Message-ID: <55262BAA.9060004@sakamocchi.jp> 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 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from smtp311.phy.lolipop.jp (smtp311.phy.lolipop.jp [210.157.22.79]) by alsa0.perex.cz (Postfix) with ESMTP id 63D242606E1 for ; Thu, 9 Apr 2015 09:35:09 +0200 (CEST) In-Reply-To: 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 Iwai Cc: alsa-devel@alsa-project.org, clemens@ladisch.de List-Id: alsa-devel@alsa-project.org 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? Thanks. Takashi Sakamoto