[PATCH 0/2 RFC] control core refactoring

[PATCH 0/2 RFC] control core refactoring

[Takashi Sakamoto]

This patchset is for refactoring control core code.

The first patch is re-send
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-February/087700.html

The second patch is new. As long as I investigated, the changed code
was written in 1999 and its basic design have never been changed. In
my opinion, the design is a bit complicated and refactoring it is
better to decrease maintaining cost.

Takashi Sakamoto (2):
  ALSA: ctl: evaluate macro instead of numerical value
  ALSA: ctl: refactoring for read operation

 sound/core/control.c | 87 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 54 insertions(+), 33 deletions(-)

-- 
2.1.0

[PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value

[Takashi Sakamoto]

SNDRV_CTL_TLV_OP_XXX is defined but not used in core code. Instead,
raw numerical value is evaluated.

This commit replaces these values to these macros for better looking.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 2ab7ee5..de19d56 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1114,7 +1114,7 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
 	int change = 0;
 	void *new_data;
 
-	if (op_flag > 0) {
+	if (op_flag == SNDRV_CTL_TLV_OP_WRITE) {
 		if (size > 1024 * 128)	/* sane value */
 			return -EINVAL;
 
@@ -1411,9 +1411,12 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 		goto __kctl_end;
 	}
 	vd = &kctl->vd[tlv.numid - kctl->id.numid];
-	if ((op_flag == 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
-	    (op_flag > 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
-	    (op_flag < 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0)) {
+	if ((op_flag == SNDRV_CTL_TLV_OP_READ &&
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
+	    (op_flag == SNDRV_CTL_TLV_OP_WRITE &&
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
+	    (op_flag == SNDRV_CTL_TLV_OP_CMD &&
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0)) {
 	    	err = -ENXIO;
 	    	goto __kctl_end;
 	}
@@ -1430,7 +1433,7 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 			return 0;
 		}
 	} else {
-		if (op_flag) {
+		if (op_flag != SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
 			err = -ENXIO;
 			goto __kctl_end;
 		}
-- 
2.1.0

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

[Takashi Sakamoto]

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@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;
+		}
+
+		/* 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;
 }
 
 static unsigned int snd_ctl_poll(struct file *file, poll_table * wait)
-- 
2.1.0

Re: [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@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

Re: [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@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

Re: [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@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

[PATCH 0/2] control core refactoring

[Takashi Sakamoto]

This patchset is revised version of my previous one.

[alsa-devel] [PATCH 0/2 RFC] control core refactoring
http://mailman.alsa-project.org/pipermail/alsa-devel/2015-April/090202.html

Changes:
 - 1/2 patch:
  * nothing
 - 2/2 patch:
  * use ssize_t instead of size_t as original code uses.
  * use goto statement to gather unlocking codes in the end of the function.

Takashi Sakamoto (2):
  ALSA: ctl: evaluate macro instead of numerical value
  ALSA: ctl: refactoring for read operation

 sound/core/control.c | 90 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 55 insertions(+), 35 deletions(-)

-- 
2.1.0

[PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value

[Takashi Sakamoto]

SNDRV_CTL_TLV_OP_XXX is defined but not used in core code. Instead,
raw numerical value is evaluated.

This commit replaces these values to these macros for better looking.

Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>
---
 sound/core/control.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index 2ab7ee5..de19d56 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1114,7 +1114,7 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
 	int change = 0;
 	void *new_data;
 
-	if (op_flag > 0) {
+	if (op_flag == SNDRV_CTL_TLV_OP_WRITE) {
 		if (size > 1024 * 128)	/* sane value */
 			return -EINVAL;
 
@@ -1411,9 +1411,12 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 		goto __kctl_end;
 	}
 	vd = &kctl->vd[tlv.numid - kctl->id.numid];
-	if ((op_flag == 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
-	    (op_flag > 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
-	    (op_flag < 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0)) {
+	if ((op_flag == SNDRV_CTL_TLV_OP_READ &&
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
+	    (op_flag == SNDRV_CTL_TLV_OP_WRITE &&
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
+	    (op_flag == SNDRV_CTL_TLV_OP_CMD &&
+	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0)) {
 	    	err = -ENXIO;
 	    	goto __kctl_end;
 	}
@@ -1430,7 +1433,7 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 			return 0;
 		}
 	} else {
-		if (op_flag) {
+		if (op_flag != SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
 			err = -ENXIO;
 			goto __kctl_end;
 		}
-- 
2.1.0

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

[Takashi Sakamoto]

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;
 		}
-		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

Re: [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@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
> 

Re: [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value

[Takashi Iwai]

At Fri, 10 Apr 2015 08:43:00 +0900,
Takashi Sakamoto wrote:
> 
> SNDRV_CTL_TLV_OP_XXX is defined but not used in core code. Instead,
> raw numerical value is evaluated.
> 
> This commit replaces these values to these macros for better looking.
> 
> Signed-off-by: Takashi Sakamoto <o-takashi@sakamocchi.jp>

Applied, thanks.


Takashi

> ---
>  sound/core/control.c | 13 ++++++++-----
>  1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/sound/core/control.c b/sound/core/control.c
> index 2ab7ee5..de19d56 100644
> --- a/sound/core/control.c
> +++ b/sound/core/control.c
> @@ -1114,7 +1114,7 @@ static int snd_ctl_elem_user_tlv(struct snd_kcontrol *kcontrol,
>  	int change = 0;
>  	void *new_data;
>  
> -	if (op_flag > 0) {
> +	if (op_flag == SNDRV_CTL_TLV_OP_WRITE) {
>  		if (size > 1024 * 128)	/* sane value */
>  			return -EINVAL;
>  
> @@ -1411,9 +1411,12 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
>  		goto __kctl_end;
>  	}
>  	vd = &kctl->vd[tlv.numid - kctl->id.numid];
> -	if ((op_flag == 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
> -	    (op_flag > 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
> -	    (op_flag < 0 && (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0)) {
> +	if ((op_flag == SNDRV_CTL_TLV_OP_READ &&
> +	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_READ) == 0) ||
> +	    (op_flag == SNDRV_CTL_TLV_OP_WRITE &&
> +	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_WRITE) == 0) ||
> +	    (op_flag == SNDRV_CTL_TLV_OP_CMD &&
> +	     (vd->access & SNDRV_CTL_ELEM_ACCESS_TLV_COMMAND) == 0)) {
>  	    	err = -ENXIO;
>  	    	goto __kctl_end;
>  	}
> @@ -1430,7 +1433,7 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
>  			return 0;
>  		}
>  	} else {
> -		if (op_flag) {
> +		if (op_flag != SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
>  			err = -ENXIO;
>  			goto __kctl_end;
>  		}
> -- 
> 2.1.0
> 

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

[Takashi Sakamoto]

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

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

[Takashi Iwai]

At Fri, 10 Apr 2015 19:32:58 +0900,
Takashi Sakamoto wrote:
> 
> 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;

Well, it's not much better than the original code, IMO.

> And I realize sound/firewire/fireworks/fireworks_hwdep.c has the same
> bug. I'll post another patch for this bug.

This is an implementation detail and I believe it rather depends on
each driver.  (But I should reread POSIX definition again before
concluding it.)

That said, it isn't wrong to return -EFAULT immediately, per se.
The problem here is that you change the existing behavior.  Especially
a core code like this should be treated carefully even for such a
small behavior change.


Takashi

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

[Takashi Sakamoto]

On Apr 10 2015 19:43, Takashi Iwai wrote:
>> 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;
> 
> Well, it's not much better than the original code, IMO.

OK. I dropped this patch.

>> And I realize sound/firewire/fireworks/fireworks_hwdep.c has the same
>> bug. I'll post another patch for this bug.
> 
> This is an implementation detail and I believe it rather depends on
> each driver.  (But I should reread POSIX definition again before
> concluding it.)
> 
> That said, it isn't wrong to return -EFAULT immediately, per se.
> The problem here is that you change the existing behavior.  Especially
> a core code like this should be treated carefully even for such a
> small behavior change.

Thanks

Takashi Sakamoto

Re: [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value

[Takashi Iwai]

At Fri, 10 Apr 2015 09:49:48 +0200,
Takashi Iwai wrote:
> 
> At Fri, 10 Apr 2015 08:43:00 +0900,
> Takashi Sakamoto wrote:
> > @@ -1430,7 +1433,7 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
> >  			return 0;
> >  		}
> >  	} else {
> > -		if (op_flag) {
> > +		if (op_flag != SNDRV_CTL_ELEM_ACCESS_TLV_READ) {

Grrr, this must be SNDRV_CTL_TLV_OP_READ.  Fixed by the patch below.


Takashi

-- 8< --
From: Takashi Iwai <tiwai@suse.de>
Subject: [PATCH] ALSA: control: Fix a typo of SNDRV_CTL_ELEM_ACCESS_TLV_* with
 SNDRV_CTL_TLV_OP_*

The commit [39d118677baa: ALSA: ctl: evaluate macro instead of
numerical value] replaced the numbers with constants, but one place
was replaced wrongly with a different type.  Fixed now.

Fixes: 39d118677baa ('ALSA: ctl: evaluate macro instead of numerical value')
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/core/control.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/core/control.c b/sound/core/control.c
index ccb1ca26a71e..be5b97cd8dc3 100644
--- a/sound/core/control.c
+++ b/sound/core/control.c
@@ -1432,7 +1432,7 @@ static int snd_ctl_tlv_ioctl(struct snd_ctl_file *file,
 			return 0;
 		}
 	} else {
-		if (op_flag != SNDRV_CTL_ELEM_ACCESS_TLV_READ) {
+		if (op_flag != SNDRV_CTL_TLV_OP_READ) {
 			err = -ENXIO;
 			goto __kctl_end;
 		}
-- 
2.3.5