| Message ID | 1359822572-26009-6-git-send-email-acourbot@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > This makes the code both simpler and faster compared to parsing the GPIO > number space. > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Tue, 5 Feb 2013 18:15:54 +0100, Linus Walleij <linus.walleij@linaro.org> wrote: > On Sat, Feb 2, 2013 at 5:29 PM, Alexandre Courbot <acourbot@nvidia.com> wrote: > > > This makes the code both simpler and faster compared to parsing the GPIO > > number space. > > > > Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Applied, thanks. However, this code is incorrect (it was incorrect before you touched it, so not your fault). Moving it to a list makes it a lot worse though because it introduces the possibility of dereferencing an invalid pointer. The hooks need to grab the spinlock before parsing the list. I've got a patch which I'll post. I'd appreciate it if you can test also. In actual fact, I'd like to replace the spinlock with a mutex, but that requires some auditing to make sure there aren't any users from an atomic context. g.
On Sat, Feb 9, 2013 at 6:37 PM, Grant Likely <grant.likely@secretlab.ca> wrote: > However, this code is incorrect (it was incorrect before you touched it, > so not your fault). Moving it to a list makes it a lot worse though > because it introduces the possibility of dereferencing an invalid > pointer. The hooks need to grab the spinlock before parsing the list. > > I've got a patch which I'll post. I'd appreciate it if you can test > also. > > In actual fact, I'd like to replace the spinlock with a mutex, but that > requires some auditing to make sure there aren't any users from an > atomic context. I thought about doing that as well actually, but left it for later. It looks safe at first sight, but as you mention we cannot exclude a few users will break as a consequence of such a switch. In any case, I plan to give it a try once that big chunk is merged. Alex.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c index 92f9ee4..e473ded 100644 --- a/drivers/gpio/gpiolib.c +++ b/drivers/gpio/gpiolib.c @@ -1889,45 +1889,28 @@ static void gpiolib_dbg_show(struct seq_file *s, struct gpio_chip *chip) static void *gpiolib_seq_start(struct seq_file *s, loff_t *pos) { struct gpio_chip *chip = NULL; - unsigned int gpio; - void *ret = NULL; - loff_t index = 0; + loff_t index = *pos; /* REVISIT this isn't locked against gpio_chip removal ... */ - for (gpio = 0; gpio_is_valid(gpio); gpio++) { - if (gpio_desc[gpio].chip == chip) - continue; - - chip = gpio_desc[gpio].chip; - if (!chip) - continue; - - if (index++ >= *pos) { - ret = chip; - break; - } - } - s->private = ""; - return ret; + list_for_each_entry(chip, &gpio_chips, list) + if (index-- == 0) + return chip; + + return NULL; } static void *gpiolib_seq_next(struct seq_file *s, void *v, loff_t *pos) { struct gpio_chip *chip = v; - unsigned int gpio; void *ret = NULL; - /* skip GPIOs provided by the current chip */ - for (gpio = chip->base + chip->ngpio; gpio_is_valid(gpio); gpio++) { - chip = gpio_desc[gpio].chip; - if (chip) { - ret = chip; - break; - } - } + if (list_is_last(&chip->list, &gpio_chips)) + ret = NULL; + else + ret = list_entry(chip->list.next, struct gpio_chip, list); s->private = "\n"; ++*pos;
This makes the code both simpler and faster compared to parsing the GPIO number space. Signed-off-by: Alexandre Courbot <acourbot@nvidia.com> --- drivers/gpio/gpiolib.c | 37 ++++++++++--------------------------- 1 file changed, 10 insertions(+), 27 deletions(-)