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

[Takashi Iwai]

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.


Takashi