[V2,1/2] gpiolib: improve overlap check of range of gpio
diff mbox

Message ID 1447650167-29070-2-git-send-email-bamvor.zhangjian@linaro.org
State New
Headers show

Commit Message

Bamvor Jian Zhang Nov. 16, 2015, 5:02 a.m. UTC
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(-)

Comments

Linus Walleij Nov. 17, 2015, 2:14 p.m. UTC | #1
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
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bamvor Jian Zhang Nov. 17, 2015, 2:47 p.m. UTC | #2
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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

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;
 }
 
 /**