[alsa-devel] [PATCH 0/2 RFC] control core refactoring
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.ht...
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(-)
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; }
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)
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;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);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
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;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);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
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;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);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
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(-)
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; }
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
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; }
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)
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;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);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
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;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);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
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;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);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
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
participants (2)
-
Takashi Iwai -
Takashi Sakamoto