* [PATCH 0/2 RFC] control core refactoring
@ 2015-04-08 16:55 Takashi Sakamoto
2015-04-08 16:55 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto
` (2 more replies)
0 siblings, 3 replies; 15+ messages in thread
From: Takashi Sakamoto @ 2015-04-08 16:55 UTC (permalink / raw)
To: clemens, tiwai; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value 2015-04-08 16:55 [PATCH 0/2 RFC] control core refactoring Takashi Sakamoto @ 2015-04-08 16:55 ` Takashi Sakamoto 2015-04-08 16:55 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto 2015-04-09 23:42 ` [PATCH 0/2] control core refactoring Takashi Sakamoto 2 siblings, 0 replies; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-08 16:55 UTC (permalink / raw) To: clemens, tiwai; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-08 16:55 [PATCH 0/2 RFC] control core refactoring Takashi Sakamoto 2015-04-08 16:55 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto @ 2015-04-08 16:55 ` Takashi Sakamoto 2015-04-09 5:33 ` Takashi Iwai 2015-04-09 23:42 ` [PATCH 0/2] control core refactoring Takashi Sakamoto 2 siblings, 1 reply; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-08 16:55 UTC (permalink / raw) To: clemens, tiwai; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-08 16:55 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto @ 2015-04-09 5:33 ` Takashi Iwai 2015-04-09 7:35 ` Takashi Sakamoto 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2015-04-09 5:33 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, clemens 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-09 5:33 ` Takashi Iwai @ 2015-04-09 7:35 ` Takashi Sakamoto 2015-04-09 8:17 ` Takashi Iwai 0 siblings, 1 reply; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-09 7:35 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, clemens 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-09 7:35 ` Takashi Sakamoto @ 2015-04-09 8:17 ` Takashi Iwai 0 siblings, 0 replies; 15+ messages in thread From: Takashi Iwai @ 2015-04-09 8:17 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, clemens 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 0/2] control core refactoring 2015-04-08 16:55 [PATCH 0/2 RFC] control core refactoring Takashi Sakamoto 2015-04-08 16:55 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto 2015-04-08 16:55 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto @ 2015-04-09 23:42 ` Takashi Sakamoto 2015-04-09 23:43 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto 2015-04-09 23:43 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto 2 siblings, 2 replies; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-09 23:42 UTC (permalink / raw) To: clemens, tiwai; +Cc: alsa-devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value 2015-04-09 23:42 ` [PATCH 0/2] control core refactoring Takashi Sakamoto @ 2015-04-09 23:43 ` Takashi Sakamoto 2015-04-10 7:49 ` Takashi Iwai 2015-04-09 23:43 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto 1 sibling, 1 reply; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-09 23:43 UTC (permalink / raw) To: clemens, tiwai; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value 2015-04-09 23:43 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto @ 2015-04-10 7:49 ` Takashi Iwai 2015-04-12 7:20 ` Takashi Iwai 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2015-04-10 7:49 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, clemens 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value 2015-04-10 7:49 ` Takashi Iwai @ 2015-04-12 7:20 ` Takashi Iwai 0 siblings, 0 replies; 15+ messages in thread From: Takashi Iwai @ 2015-04-12 7:20 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, clemens 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-09 23:42 ` [PATCH 0/2] control core refactoring Takashi Sakamoto 2015-04-09 23:43 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto @ 2015-04-09 23:43 ` Takashi Sakamoto 2015-04-10 7:48 ` Takashi Iwai 1 sibling, 1 reply; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-09 23:43 UTC (permalink / raw) To: clemens, tiwai; +Cc: alsa-devel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-09 23:43 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto @ 2015-04-10 7:48 ` Takashi Iwai 2015-04-10 10:32 ` Takashi Sakamoto 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2015-04-10 7:48 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, clemens 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 > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-10 7:48 ` Takashi Iwai @ 2015-04-10 10:32 ` Takashi Sakamoto 2015-04-10 10:43 ` Takashi Iwai 0 siblings, 1 reply; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-10 10:32 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, clemens 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-10 10:32 ` Takashi Sakamoto @ 2015-04-10 10:43 ` Takashi Iwai 2015-04-10 10:52 ` Takashi Sakamoto 0 siblings, 1 reply; 15+ messages in thread From: Takashi Iwai @ 2015-04-10 10:43 UTC (permalink / raw) To: Takashi Sakamoto; +Cc: alsa-devel, clemens 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ALSA: ctl: refactoring for read operation 2015-04-10 10:43 ` Takashi Iwai @ 2015-04-10 10:52 ` Takashi Sakamoto 0 siblings, 0 replies; 15+ messages in thread From: Takashi Sakamoto @ 2015-04-10 10:52 UTC (permalink / raw) To: Takashi Iwai; +Cc: alsa-devel, clemens 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2015-04-12 7:20 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-04-08 16:55 [PATCH 0/2 RFC] control core refactoring Takashi Sakamoto 2015-04-08 16:55 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto 2015-04-08 16:55 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto 2015-04-09 5:33 ` Takashi Iwai 2015-04-09 7:35 ` Takashi Sakamoto 2015-04-09 8:17 ` Takashi Iwai 2015-04-09 23:42 ` [PATCH 0/2] control core refactoring Takashi Sakamoto 2015-04-09 23:43 ` [PATCH 1/2] ALSA: ctl: evaluate macro instead of numerical value Takashi Sakamoto 2015-04-10 7:49 ` Takashi Iwai 2015-04-12 7:20 ` Takashi Iwai 2015-04-09 23:43 ` [PATCH 2/2] ALSA: ctl: refactoring for read operation Takashi Sakamoto 2015-04-10 7:48 ` Takashi Iwai 2015-04-10 10:32 ` Takashi Sakamoto 2015-04-10 10:43 ` Takashi Iwai 2015-04-10 10:52 ` Takashi Sakamoto
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.