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: Fri, 10 Apr 2015 12:43:43 +0200 Message-ID: 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> <5527A6DA.50800@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 39131265D6A for ; Fri, 10 Apr 2015 12:43:46 +0200 (CEST) In-Reply-To: <5527A6DA.50800@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 Fri, 10 Apr 2015 19:32:58 +0900, Takashi Sakamoto wrote: > > 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; Well, it's not much better than the original code, IMO. > And I realize sound/firewire/fireworks/fireworks_hwdep.c has the same > bug. I'll post another patch for this bug. This is an implementation detail and I believe it rather depends on each driver. (But I should reread POSIX definition again before concluding it.) That said, it isn't wrong to return -EFAULT immediately, per se. The problem here is that you change the existing behavior. Especially a core code like this should be treated carefully even for such a small behavior change. Takashi