[PATCH 0/2] Fix bugs in the insertion of gpiochip.

[PATCH 0/2] Fix bugs in the insertion of gpiochip.

[Bamvor Jian Zhang]

These patches try to fix following bugs which is found by my gpio
mockup driver and testscript[1](will send them later):
1.  Could not check the overlap if the new gpiochip is the secondly
    gpiochip.
2.  Could not check the overlap if the new gpiochip is overlap
    with the left of gpiochip.
3.  Allow overlap of base of different gpiochip.
4.  Allow to insert an empty gpiochip

The first patch fix the first three by rewriting the logic in the
gpiochip_add_to_list.

The second patch fix the fourth bug in gpiochip_add. I do not
found the checker in gpiolib.c. Hope it is not a redundant logic.

[1] https://github.com/bjzhang/linux/tree/gpio-fix-and-mockup-driver

Bamvor Jian Zhang (2):
  gpiolib: improve overlap check of range of gpio
  gpiolib: do not allow to insert an empty gpiochip

 drivers/gpio/gpiolib.c | 44 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 33 insertions(+), 11 deletions(-)

-- 
2.1.4

[PATCH 1/2] gpiolib: improve overlap check of range of gpio

[Bamvor Jian Zhang]

There are limitations for the current checker:
1.  Could not check the overlap if the new gpiochip is the secondly
    gpiochip.
2.  Could not check the overlap if the new gpiochip is overlap
    with the left of gpiochip. E.g. if we insert [c, d] between
    [a,b] and [e, f], and e >= c + d, it will successful even if
    c < a + b.
3.  Allow overlap of base of different gpiochip.

This patch fix these issues by checking the overlap of both right and
left gpiochip in the same loop statement.

Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
---
 drivers/gpio/gpiolib.c | 41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6798355..cc135d9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -191,29 +191,48 @@ static int gpiochip_add_to_list(struct gpio_chip *chip)
 {
 	struct list_head *pos;
 	struct gpio_chip *_chip;
+	struct gpio_chip *_chip_prev = NULL;
 	int err = 0;
 
-	/* find where to insert our chip */
-	list_for_each(pos, &gpio_chips) {
-		_chip = list_entry(pos, struct gpio_chip, list);
-		/* shall we insert before _chip? */
-		if (_chip->base >= chip->base + chip->ngpio)
-			break;
+	if (list_empty(&gpio_chips)) {
+		pos = gpio_chips.next;
+		goto found;
 	}
 
-	/* are we stepping on the chip right before? */
-	if (pos != &gpio_chips && pos->prev != &gpio_chips) {
-		_chip = list_entry(pos->prev, struct gpio_chip, list);
-		if (_chip->base + _chip->ngpio > chip->base) {
+	list_for_each(pos, &gpio_chips) {
+		_chip = list_entry(pos, struct gpio_chip, list);
+		if (_chip->base == chip->base) {
 			dev_err(chip->dev,
-			       "GPIO integer space overlap, cannot add chip\n");
+			       "GPIO base overlap<%d>, cannot add chip\n",
+			       chip->base);
 			err = -EBUSY;
+			goto err;
 		}
+		if (_chip->base >= chip->base + chip->ngpio) {
+			/* we are the before the first existence gpio*/
+			if (pos->prev == &gpio_chips) {
+				goto found;
+			} else {
+				if (_chip_prev->base + _chip_prev->ngpio
+						<= chip->base)
+					goto found;
+			}
+		}
+		_chip_prev = _chip;
 	}
+	if (_chip->base + _chip->ngpio <= chip->base)
+		goto found;
 
+	dev_err(chip->dev,
+	       "GPIO integer space overlap, cannot add chip\n");
+	err = -EBUSY;
+	goto err;
+
+found:
 	if (!err)
 		list_add_tail(&chip->list, pos);
 
+err:
 	return err;
 }
 
-- 
2.1.4

[PATCH 2/2] gpiolib: do not allow to insert an empty gpiochip

[Bamvor Jian Zhang]

We need to check if number of gpio is positive if there is no
such check in devicetree or acpi or whatever called before
gpiochip_add.

I suppose that devicetree and acpi do not allow insert gpiochip
with zero number but I do not know if it is enough to ignore
this check in gpiochip_add.

Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
---
 drivers/gpio/gpiolib.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index cc135d9..868ff15 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -327,6 +327,9 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (!descs)
 		return -ENOMEM;
 
+	if (chip->ngpio == 0)
+		return -EINVAL;
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	if (base < 0) {
-- 
2.1.4

Re: [PATCH 1/2] gpiolib: improve overlap check of range of gpio

[Linus Walleij]

On Sat, Nov 14, 2015 at 9:38 AM, Bamvor Jian Zhang
<bamvor.zhangjian@linaro.org> wrote:

> There are limitations for the current checker:
> 1.  Could not check the overlap if the new gpiochip is the secondly
>     gpiochip.
> 2.  Could not check the overlap if the new gpiochip is overlap
>     with the left of gpiochip. E.g. if we insert [c, d] between
>     [a,b] and [e, f], and e >= c + d, it will successful even if
>     c < a + b.
> 3.  Allow overlap of base of different gpiochip.
>
> This patch fix these issues by checking the overlap of both right and
> left gpiochip in the same loop statement.
>
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

Very interesting patch!

> +++ b/drivers/gpio/gpiolib.c
> @@ -191,29 +191,48 @@ static int gpiochip_add_to_list(struct gpio_chip *chip)
>  {
>         struct list_head *pos;
>         struct gpio_chip *_chip;
> +       struct gpio_chip *_chip_prev = NULL;

Syntactic comment: we have too many things named "chip" (something).
I also hate _underscore in variable names.

Please take this opportunity to rename:

"_chip" to "iterator"
"_chip_prev" to "previous"

That they are both gpio_chip pointers is obvious from context.

This renaming makes the code more readable.

>         int err = 0;
>
> -       /* find where to insert our chip */
> -       list_for_each(pos, &gpio_chips) {
> -               _chip = list_entry(pos, struct gpio_chip, list);
> -               /* shall we insert before _chip? */
> -               if (_chip->base >= chip->base + chip->ngpio)
> -                       break;
> +       if (list_empty(&gpio_chips)) {
> +               pos = gpio_chips.next;
> +               goto found;
>         }
>
> -       /* are we stepping on the chip right before? */
> -       if (pos != &gpio_chips && pos->prev != &gpio_chips) {
> -               _chip = list_entry(pos->prev, struct gpio_chip, list);
> -               if (_chip->base + _chip->ngpio > chip->base) {
> +       list_for_each(pos, &gpio_chips) {
> +               _chip = list_entry(pos, struct gpio_chip, list);
> +               if (_chip->base == chip->base) {
>                         dev_err(chip->dev,
> -                              "GPIO integer space overlap, cannot add chip\n");
> +                              "GPIO base overlap<%d>, cannot add chip\n",
> +                              chip->base);
>                         err = -EBUSY;
> +                       goto err;

OK that is the obvious case...

>                 }
> +               if (_chip->base >= chip->base + chip->ngpio) {

So if the iterator is above this chip.

> +                       /* we are the before the first existence gpio*/
> +                       if (pos->prev == &gpio_chips) {
> +                               goto found;

This comment needs proper english. It should say:
"iterator is at the first GPIO chip so there is no previous one"

Can't you just check if _chip_prev == NULL?

> +                       } else {
> +                               if (_chip_prev->base + _chip_prev->ngpio
> +                                               <= chip->base)
> +                                       goto found;

Here we are also below the previous chip, so above the current
iterator and below the previous one so we found a "hole".
Insert a comment that "we found a hole in the GPIO chip bases".

> +                       }
> +               }
> +               _chip_prev = _chip;
>         }
> +       if (_chip->base + _chip->ngpio <= chip->base)
> +               goto found;

And here we are above the last chip in the list.
Insert a comment about that too.
"We are beyond the last chip in the list".

Yours,
Linus Walleij

Re: [PATCH 2/2] gpiolib: do not allow to insert an empty gpiochip

[Linus Walleij]

On Sat, Nov 14, 2015 at 9:38 AM, Bamvor Jian Zhang
<bamvor.zhangjian@linaro.org> wrote:

> We need to check if number of gpio is positive if there is no
> such check in devicetree or acpi or whatever called before
> gpiochip_add.
>
> I suppose that devicetree and acpi do not allow insert gpiochip
> with zero number but I do not know if it is enough to ignore
> this check in gpiochip_add.
>
> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>

> +       if (chip->ngpio == 0)
> +               return -EINVAL;

Print a warning with chip_err(chip, "tried to insert a GPIO chip with
zero lines\n");

Yours,
Linus Walleij

Re: [PATCH 1/2] gpiolib: improve overlap check of range of gpio

[Bamvor Zhang Jian]

Hi, Linus

On 11/14/2015 07:01 PM, Linus Walleij wrote:
> On Sat, Nov 14, 2015 at 9:38 AM, Bamvor Jian Zhang
> <bamvor.zhangjian@linaro.org> wrote:
>
>> There are limitations for the current checker:
>> 1.  Could not check the overlap if the new gpiochip is the secondly
>>     gpiochip.
>> 2.  Could not check the overlap if the new gpiochip is overlap
>>     with the left of gpiochip. E.g. if we insert [c, d] between
>>     [a,b] and [e, f], and e >= c + d, it will successful even if
>>     c < a + b.
>> 3.  Allow overlap of base of different gpiochip.
>>
>> This patch fix these issues by checking the overlap of both right and
>> left gpiochip in the same loop statement.
>>
>> Signed-off-by: Bamvor Jian Zhang <bamvor.zhangjian@linaro.org>
>
> Very interesting patch!
>
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -191,29 +191,48 @@ static int gpiochip_add_to_list(struct gpio_chip *chip)
>>  {
>>         struct list_head *pos;
>>         struct gpio_chip *_chip;
>> +       struct gpio_chip *_chip_prev = NULL;
>
> Syntactic comment: we have too many things named "chip" (something).
> I also hate _underscore in variable names.
>
> Please take this opportunity to rename:
>
> "_chip" to "iterator"
> "_chip_prev" to "previous"
>
> That they are both gpio_chip pointers is obvious from context.
>
> This renaming makes the code more readable.
>
>>         int err = 0;
>>
>> -       /* find where to insert our chip */
>> -       list_for_each(pos, &gpio_chips) {
>> -               _chip = list_entry(pos, struct gpio_chip, list);
>> -               /* shall we insert before _chip? */
>> -               if (_chip->base >= chip->base + chip->ngpio)
>> -                       break;
>> +       if (list_empty(&gpio_chips)) {
>> +               pos = gpio_chips.next;
>> +               goto found;
>>         }
>>
>> -       /* are we stepping on the chip right before? */
>> -       if (pos != &gpio_chips && pos->prev != &gpio_chips) {
>> -               _chip = list_entry(pos->prev, struct gpio_chip, list);
>> -               if (_chip->base + _chip->ngpio > chip->base) {
>> +       list_for_each(pos, &gpio_chips) {
>> +               _chip = list_entry(pos, struct gpio_chip, list);
>> +               if (_chip->base == chip->base) {
>>                         dev_err(chip->dev,
>> -                              "GPIO integer space overlap, cannot add chip\n");
>> +                              "GPIO base overlap<%d>, cannot add chip\n",
>> +                              chip->base);
>>                         err = -EBUSY;
>> +                       goto err;
>
> OK that is the obvious case...
>
>>                 }
>> +               if (_chip->base >= chip->base + chip->ngpio) {
>
> So if the iterator is above this chip.
>
>> +                       /* we are the before the first existence gpio*/
>> +                       if (pos->prev == &gpio_chips) {
>> +                               goto found;
>
> This comment needs proper english. It should say:
> "iterator is at the first GPIO chip so there is no previous one"
>
> Can't you just check if _chip_prev == NULL?
Yes, it is more clear.
>
>> +                       } else {
>> +                               if (_chip_prev->base + _chip_prev->ngpio
>> +                                               <= chip->base)
>> +                                       goto found;
>
> Here we are also below the previous chip, so above the current
> iterator and below the previous one so we found a "hole".
> Insert a comment that "we found a hole in the GPIO chip bases".
Yes, we found a valid range between _chip_prev and chip. Is it more clear
if we use ranges(means [base,base+ngpio]) instead of bases?
>
>> +                       }
>> +               }
>> +               _chip_prev = _chip;
>>         }
>> +       if (_chip->base + _chip->ngpio <= chip->base)
>> +               goto found;
>
> And here we are above the last chip in the list.
> Insert a comment about that too.
> "We are beyond the last chip in the list".
Yeap. I am sorry for the variable naming and comments. I will update them
in the next patch. Except for the above comment, any comments or suggestions
for the logic?

Thanks

Bamvor

>
> Yours,
> Linus Walleij
>

Re: [PATCH 1/2] gpiolib: improve overlap check of range of gpio

[Linus Walleij]

On Sat, Nov 14, 2015 at 12:20 PM, Bamvor Zhang Jian
<bamvor.zhangjian@linaro.org> wrote:

>> Here we are also below the previous chip, so above the current
>> iterator and below the previous one so we found a "hole".
>> Insert a comment that "we found a hole in the GPIO chip bases".
>
> Yes, we found a valid range between _chip_prev and chip. Is it more clear
> if we use ranges(means [base,base+ngpio]) instead of bases?

Maybe, maybe both. Suggest something that makes sense to you
in your updated patch.

>> And here we are above the last chip in the list.
>> Insert a comment about that too.
>> "We are beyond the last chip in the list".
>
> Yeap. I am sorry for the variable naming and comments. I will update them
> in the next patch.

The naming is not your fault, there was a bad precedent.

> Except for the above comment, any comments or suggestions
> for the logic?

No it makes sense.

Yours,
Linus Walleij