Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation

[Takashi Sakamoto <o-takashi@sakamocchi.jp>]

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 <o-takashi@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.

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