| Message ID | 1428512108-1852-3-git-send-email-o-takashi@sakamocchi.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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
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)
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(-)