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

[Takashi Sakamoto]

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?


Thanks.

Takashi Sakamoto