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

[Takashi Iwai]

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


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
>