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: Fri, 10 Apr 2015 19:32:58 +0900 Message-ID: <5527A6DA.50800@sakamocchi.jp> References: <1428512108-1852-1-git-send-email-o-takashi@sakamocchi.jp> <1428622981-9747-1-git-send-email-o-takashi@sakamocchi.jp> <1428622981-9747-3-git-send-email-o-takashi@sakamocchi.jp> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" 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 AE1E3260606 for ; Fri, 10 Apr 2015 12:33:04 +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 10 2015 16:48, Takashi Iwai wrote: > At Fri, 10 Apr 2015 08:43:01 +0900, > Takashi Sakamoto wrote: >> >> snd_ctl_read() has nested loop and complicated condition for return >> value. This is not better for reading. >> >> This commit applies refactoring with two loops. >> >> Signed-off-by: Takashi Sakamoto >> --- >> sound/core/control.c | 77 ++++++++++++++++++++++++++++++++-------------------- >> 1 file changed, 47 insertions(+), 30 deletions(-) >> >> diff --git a/sound/core/control.c b/sound/core/control.c >> index de19d56..da6497d 100644 >> --- a/sound/core/control.c >> +++ b/sound/core/control.c >> @@ -1520,58 +1520,75 @@ 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; >> + ssize_t result; >> >> 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); >> - 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); >> + while (list_empty(&ctl->events)) { >> + if (file->f_flags & O_NONBLOCK) { >> + result = -EAGAIN; >> + goto unlock; >> } >> + >> + /* The card was disconnected. The queued events are dropped. */ >> + if (ctl->card->shutdown) { >> + result = -ENODEV; >> + goto unlock; >> + } >> + >> + >> + 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; >> + result = -EFAULT; >> + break; > > This still changes the behavior. The read returns the size that has > been read at first even when an error happens if some data was already > read. The error code is returned at the next read. This is a design > problem of read syscall. I'm unaware of it... These code should be: if (copy_to_user(buffer, &ev, sizeof(struct snd_ctl_event))) { if (result == 0) result = -EFAULT; break; And I realize sound/firewire/fireworks/fireworks_hwdep.c has the same bug. I'll post another patch for this bug. Thanks Takashi Sakamoto > Takashi > >> } >> - 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: >> +unlock: >> spin_unlock_irq(&ctl->read_lock); >> - __end: >> - return result > 0 ? result : err; >> + return result; >> } >> >> static unsigned int snd_ctl_poll(struct file *file, poll_table * wait) >> -- >> 2.1.0