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

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

[Bamvor Jian Zhang]

The first version of these patches could be found [1].

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.

Changes since v1
1.  Update comment and print according to suggestion given by Linus.
2.  Delete the dedicated checking for base overlap. The other logic
    in the patch 1/2 already cover it.

[1] http://www.spinics.net/lists/linux-gpio/msg09594.html
[2] 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 | 64 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 19 deletions(-)

-- 
2.1.4

[PATCH V2 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 | 59 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 40 insertions(+), 19 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6798355..270d60b 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -182,7 +182,7 @@ EXPORT_SYMBOL_GPL(gpiod_get_direction);
 
 /*
  * Add a new chip to the global chips list, keeping the list of chips sorted
- * by base order.
+ * by range(means [base, base + ngpio - 1]) order.
  *
  * Return -EBUSY if the new chip overlaps with some other chip's integer
  * space.
@@ -190,31 +190,52 @@ EXPORT_SYMBOL_GPL(gpiod_get_direction);
 static int gpiochip_add_to_list(struct gpio_chip *chip)
 {
 	struct list_head *pos;
-	struct gpio_chip *_chip;
-	int err = 0;
+	struct gpio_chip *iterator;
+	struct gpio_chip *previous = NULL;
+	int ret = 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) {
-			dev_err(chip->dev,
-			       "GPIO integer space overlap, cannot add chip\n");
-			err = -EBUSY;
+	list_for_each(pos, &gpio_chips) {
+		iterator = list_entry(pos, struct gpio_chip, list);
+		if (iterator->base >= chip->base + chip->ngpio) {
+			/*
+			 * Iterator is the first GPIO chip so there is no
+			 * previous one
+			 */
+			if (previous == NULL) {
+				goto found;
+			} else {
+				/*
+				 * We found a valid range(means
+				 * [base, base + ngpio - 1]) between previous
+				 * and iterator chip.
+				 */
+				if (previous->base + previous->ngpio
+						<= chip->base)
+					goto found;
+			}
 		}
+		previous = iterator;
 	}
+	/* We are beyond the last chip in the list */
+	if (iterator->base + iterator->ngpio <= chip->base)
+		goto found;
+
+	dev_err(chip->dev,
+	       "GPIO integer space overlap, cannot add chip\n");
+	goto err;
 
-	if (!err)
-		list_add_tail(&chip->list, pos);
+found:
+	list_add_tail(&chip->list, pos);
+	return ret;
 
-	return err;
+err:
+	ret = -EBUSY;
+	return ret;
 }
 
 /**
-- 
2.1.4

[PATCH V2 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 | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 270d60b..150af91 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -329,6 +329,11 @@ int gpiochip_add(struct gpio_chip *chip)
 	if (!descs)
 		return -ENOMEM;
 
+	if (chip->ngpio == 0) {
+		chip_err(chip, "tried to insert a GPIO chip with zero lines\n");
+		return -EINVAL;
+	}
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	if (base < 0) {
-- 
2.1.4

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

[Linus Walleij]

On Mon, Nov 16, 2015 at 6:02 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>

Patch applied with some tweaks. Had to rebase it because
I renamed ->dev to ->parent in the GPIO tree, then I found
it possible to get rid of the ret variable altogether.

Yours,
Linus Walleij

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

[Linus Walleij]

On Mon, Nov 16, 2015 at 6:02 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>

Patch applied.

Yours,
Linus Walleij

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

[Bamvor Zhang Jian]

Hi, Linus

On 11/17/2015 10:14 PM, Linus Walleij wrote:
> On Mon, Nov 16, 2015 at 6:02 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>
>
> Patch applied with some tweaks. Had to rebase it because
> I renamed ->dev to ->parent in the GPIO tree, then I found
> it possible to get rid of the ret variable altogether.
Oh, I am sorry. I notice you parent patch but I worked on your "for-next"
branch. I will take care next time.

Thanks your help.

Regards

Bamvor
>
> Yours,
> Linus Walleij
>