[alsa-devel] [PATCH 2/2] ALSA: ctl: refactoring for read operation

[Takashi Iwai]

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 <o-takashi at sakamocchi.jp>
> >> ---
> >>   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