Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Marek Szyprowski]

Hi All,

On 2018-09-02 14:01, Janusz Krzysztofik wrote:
> Certain GPIO descriptor arrays returned by gpio_get_array() may contain
> information on direct mapping of array members to pins of a single GPIO
> chip in hardware order.  In such cases, bitmaps of values can be passed
> directly from/to the chip's .get/set_multiple() callbacks without
> wasting time on iterations.
>
> Add respective code to gpiod_get/set_array_bitmap_complex() functions.
> Pins not applicable for fast path are processed as before, skipping
> over the 'fast' ones.
>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

I've just noticed that this patch landed in today's linux-next. Sadly it
breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
device-tree source arch/arm/boot/dts/exynos5250-snow.dts).

Booting hangs after detecting MMC cards. Reverting this patch fixes the
boot. I will try later to add some debugs and investigate it further what
really happens when booting hangs.

> ---
>   Documentation/driver-api/gpio/board.rst    | 15 ++++++
>   Documentation/driver-api/gpio/consumer.rst |  8 +++
>   drivers/gpio/gpiolib.c                     | 87 ++++++++++++++++++++++++++++--
>   3 files changed, 105 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst
> index 2c112553df84..c66821e033c2 100644
> --- a/Documentation/driver-api/gpio/board.rst
> +++ b/Documentation/driver-api/gpio/board.rst
> @@ -193,3 +193,18 @@ And the table can be added to the board code as follows::
>   
>   The line will be hogged as soon as the gpiochip is created or - in case the
>   chip was created earlier - when the hog table is registered.
> +
> +Arrays of pins
> +--------------
> +In addition to requesting pins belonging to a function one by one, a device may
> +also request an array of pins assigned to the function.  The way those pins are
> +mapped to the device determines if the array qualifies for fast bitmap
> +processing.  If yes, a bitmap is passed over get/set array functions directly
> +between a caller and a respective .get/set_multiple() callback of a GPIO chip.
> +
> +In order to qualify for fast bitmap processing, the pin mapping must meet the
> +following requirements:
> +- it must belong to the same chip as other 'fast' pins of the function,
> +- its index within the function must match its hardware number within the chip.
> +
> +Open drain and open source pins are excluded from fast bitmap output processing.
> diff --git a/Documentation/driver-api/gpio/consumer.rst b/Documentation/driver-api/gpio/consumer.rst
> index 0afd95a12b10..cf992e5ab976 100644
> --- a/Documentation/driver-api/gpio/consumer.rst
> +++ b/Documentation/driver-api/gpio/consumer.rst
> @@ -388,6 +388,14 @@ array_info should be set to NULL.
>   Note that for optimal performance GPIOs belonging to the same chip should be
>   contiguous within the array of descriptors.
>   
> +Still better performance may be achieved if array indexes of the descriptors
> +match hardware pin numbers of a single chip.  If an array passed to a get/set
> +array function matches the one obtained from gpiod_get_array() and array_info
> +associated with the array is also passed, the function may take a fast bitmap
> +processing path, passing the value_bitmap argument directly to the respective
> +.get/set_multiple() callback of the chip.  That allows for utilization of GPIO
> +banks as data I/O ports without much loss of performance.
> +
>   The return value of gpiod_get_array_value() and its variants is 0 on success
>   or negative on error. Note the difference to gpiod_get_value(), which returns
>   0 or 1 on success to convey the GPIO value. With the array functions, the GPIO
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index cef6ee31fe05..b9d083fb13ee 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2789,7 +2789,36 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   				  struct gpio_array *array_info,
>   				  unsigned long *value_bitmap)
>   {
> -	int i = 0;
> +	int err, i = 0;
> +
> +	/*
> +	 * Validate array_info against desc_array and its size.
> +	 * It should immediately follow desc_array if both
> +	 * have been obtained from the same gpiod_get_array() call.
> +	 */
> +	if (array_info && array_info->desc == desc_array &&
> +	    array_size <= array_info->size &&
> +	    (void *)array_info == desc_array + array_info->size) {
> +		if (!can_sleep)
> +			WARN_ON(array_info->chip->can_sleep);
> +
> +		err = gpio_chip_get_multiple(array_info->chip,
> +					     array_info->get_mask,
> +					     value_bitmap);
> +		if (err)
> +			return err;
> +
> +		if (!raw && !bitmap_empty(array_info->invert_mask, array_size))
> +			bitmap_xor(value_bitmap, value_bitmap,
> +				   array_info->invert_mask, array_size);
> +
> +		if (bitmap_full(array_info->get_mask, array_size))
> +			return 0;
> +
> +		i = find_first_zero_bit(array_info->get_mask, array_size);
> +	} else {
> +		array_info = NULL;
> +	}
>   
>   	while (i < array_size) {
>   		struct gpio_chip *chip = desc_array[i]->gdev->chip;
> @@ -2820,7 +2849,12 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   			int hwgpio = gpio_chip_hwgpio(desc);
>   
>   			__set_bit(hwgpio, mask);
> -			i++;
> +
> +			if (array_info)
> +				find_next_zero_bit(array_info->get_mask,
> +						   array_size, i);
> +			else
> +				i++;
>   		} while ((i < array_size) &&
>   			 (desc_array[i]->gdev->chip == chip));
>   
> @@ -2831,7 +2865,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   			return ret;
>   		}
>   
> -		for (j = first; j < i; j++) {
> +		for (j = first; j < i; ) {
>   			const struct gpio_desc *desc = desc_array[j];
>   			int hwgpio = gpio_chip_hwgpio(desc);
>   			int value = test_bit(hwgpio, bits);
> @@ -2840,6 +2874,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   				value = !value;
>   			__assign_bit(j, value_bitmap, value);
>   			trace_gpio_value(desc_to_gpio(desc), 1, value);
> +
> +			if (array_info)
> +				find_next_zero_bit(array_info->get_mask, i, j);
> +			else
> +				j++;
>   		}
>   
>   		if (mask != fastpath)
> @@ -3041,6 +3080,32 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>   {
>   	int i = 0;
>   
> +	/*
> +	 * Validate array_info against desc_array and its size.
> +	 * It should immediately follow desc_array if both
> +	 * have been obtained from the same gpiod_get_array() call.
> +	 */
> +	if (array_info && array_info->desc == desc_array &&
> +	    array_size <= array_info->size &&
> +	    (void *)array_info == desc_array + array_info->size) {
> +		if (!can_sleep)
> +			WARN_ON(array_info->chip->can_sleep);
> +
> +		if (!raw && !bitmap_empty(array_info->invert_mask, array_size))
> +			bitmap_xor(value_bitmap, value_bitmap,
> +				   array_info->invert_mask, array_size);
> +
> +		gpio_chip_set_multiple(array_info->chip, array_info->set_mask,
> +				       value_bitmap);
> +
> +		if (bitmap_full(array_info->set_mask, array_size))
> +			return 0;
> +
> +		i = find_first_zero_bit(array_info->set_mask, array_size);
> +	} else {
> +		array_info = NULL;
> +	}
> +
>   	while (i < array_size) {
>   		struct gpio_chip *chip = desc_array[i]->gdev->chip;
>   		unsigned long fastpath[2 * BITS_TO_LONGS(FASTPATH_NGPIO)];
> @@ -3068,7 +3133,14 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>   			int hwgpio = gpio_chip_hwgpio(desc);
>   			int value = test_bit(i, value_bitmap);
>   
> -			if (!raw && test_bit(FLAG_ACTIVE_LOW, &desc->flags))
> +			/*
> +			 * Pins applicable for fast input but not for
> +			 * fast output processing may have been already
> +			 * inverted inside the fast path, skip them.
> +			 */
> +			if (!raw && !(array_info &&
> +			    test_bit(i, array_info->invert_mask)) &&
> +			    test_bit(FLAG_ACTIVE_LOW, &desc->flags))
>   				value = !value;
>   			trace_gpio_value(desc_to_gpio(desc), 0, value);
>   			/*
> @@ -3087,7 +3159,12 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>   					__clear_bit(hwgpio, bits);
>   				count++;
>   			}
> -			i++;
> +
> +			if (array_info)
> +				find_next_zero_bit(array_info->set_mask,
> +						   array_size, i);
> +			else
> +				i++;
>   		} while ((i < array_size) &&
>   			 (desc_array[i]->gdev->chip == chip));
>   		/* push collected bits to outputs */
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Janusz Krzysztofik]

On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote:
> Hi All,
> 
> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
> > Certain GPIO descriptor arrays returned by gpio_get_array() may contain
> > information on direct mapping of array members to pins of a single GPIO
> > chip in hardware order.  In such cases, bitmaps of values can be passed
> > directly from/to the chip's .get/set_multiple() callbacks without
> > wasting time on iterations.
> >
> > Add respective code to gpiod_get/set_array_bitmap_complex() functions.
> > Pins not applicable for fast path are processed as before, skipping
> > over the 'fast' ones.
> >
> > Cc: Jonathan Corbet <corbet@lwn.net>
> > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> 
> I've just noticed that this patch landed in today's linux-next. Sadly it
> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
> 
> Booting hangs after detecting MMC cards. Reverting this patch fixes the
> boot. I will try later to add some debugs and investigate it further what
> really happens when booting hangs.

Hi Marek,

Thanks for reporting. Could you please try the following fix?

Thanks,
Janusz

>From d7ecd435bfb4972766b63ac383a43875700c7452 Mon Sep 17 00:00:00 2001
From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Date: Thu, 20 Sep 2018 17:37:21 +0200
Subject: [PATCH] gpiolib: Fix bitmap index not updated

While skipping fast path bits, bitmap index is not updated with next
found zero bit position. Fix it.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/gpio/gpiolib.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a53d17745d21..5bc3447949c9 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			__set_bit(hwgpio, mask);
 
 			if (array_info)
-				find_next_zero_bit(array_info->get_mask,
+				i = find_next_zero_bit(array_info->get_mask,
 						   array_size, i);
 			else
 				i++;
@@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			trace_gpio_value(desc_to_gpio(desc), 1, value);
 
 			if (array_info)
-				find_next_zero_bit(array_info->get_mask, i, j);
+				i = find_next_zero_bit(array_info->get_mask, i,
+						       j);
 			else
 				j++;
 		}
@@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 			}
 
 			if (array_info)
-				find_next_zero_bit(array_info->set_mask,
+				i = find_next_zero_bit(array_info->set_mask,
 						   array_size, i);
 			else
 				i++;
-- 
2.16.4

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Janusz Krzysztofik]

On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote:
> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote:
> > Hi All,
> > 
> > On 2018-09-02 14:01, Janusz Krzysztofik wrote:
> > > Certain GPIO descriptor arrays returned by gpio_get_array() may contain
> > > information on direct mapping of array members to pins of a single GPIO
> > > chip in hardware order.  In such cases, bitmaps of values can be passed
> > > directly from/to the chip's .get/set_multiple() callbacks without
> > > wasting time on iterations.
> > >
> > > Add respective code to gpiod_get/set_array_bitmap_complex() functions.
> > > Pins not applicable for fast path are processed as before, skipping
> > > over the 'fast' ones.
> > >
> > > Cc: Jonathan Corbet <corbet@lwn.net>
> > > Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> > 
> > I've just noticed that this patch landed in today's linux-next. Sadly it
> > breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
> > device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
> > 
> > Booting hangs after detecting MMC cards. Reverting this patch fixes the
> > boot. I will try later to add some debugs and investigate it further what
> > really happens when booting hangs.
> 
> Hi Marek,
> 
> Thanks for reporting. Could you please try the following fix?

Hi again,

I realized the patch was not correct, j, not i, should be updated in second 
hunk. Please try the following one.

Thanks,
Janusz

>From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
Date: Thu, 20 Sep 2018 17:37:21 +0200
Subject: [PATCH] gpiolib: Fix bitmap index not updated

While skipping fast path bits, bitmap index is not updated with next
found zero bit position. Fix it.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/gpio/gpiolib.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a53d17745d21..369bdd358fcc 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			__set_bit(hwgpio, mask);
 
 			if (array_info)
-				find_next_zero_bit(array_info->get_mask,
+				i = find_next_zero_bit(array_info->get_mask,
 						   array_size, i);
 			else
 				i++;
@@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			trace_gpio_value(desc_to_gpio(desc), 1, value);
 
 			if (array_info)
-				find_next_zero_bit(array_info->get_mask, i, j);
+				j = find_next_zero_bit(array_info->get_mask, i,
+						       j);
 			else
 				j++;
 		}
@@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 			}
 
 			if (array_info)
-				find_next_zero_bit(array_info->set_mask,
+				i = find_next_zero_bit(array_info->set_mask,
 						   array_size, i);
 			else
 				i++;
-- 
2.16.4

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Marek Szyprowski]

Hi Janusz,

On 2018-09-20 18:21, Janusz Krzysztofik wrote:
> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote:
>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote:
>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may contain
>>>> information on direct mapping of array members to pins of a single GPIO
>>>> chip in hardware order.  In such cases, bitmaps of values can be passed
>>>> directly from/to the chip's .get/set_multiple() callbacks without
>>>> wasting time on iterations.
>>>>
>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions.
>>>> Pins not applicable for fast path are processed as before, skipping
>>>> over the 'fast' ones.
>>>>
>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>> I've just noticed that this patch landed in today's linux-next. Sadly it
>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
>>>
>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the
>>> boot. I will try later to add some debugs and investigate it further what
>>> really happens when booting hangs.
>> Hi Marek,
>>
>> Thanks for reporting. Could you please try the following fix?
> Hi again,
>
> I realized the patch was not correct, j, not i, should be updated in second
> hunk. Please try the following one.
>
> Thanks,
> Janusz
>
> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
> From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> Date: Thu, 20 Sep 2018 17:37:21 +0200
> Subject: [PATCH] gpiolib: Fix bitmap index not updated
> While skipping fast path bits, bitmap index is not updated with next
> found zero bit position. Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

This one also doesn't help. A quick compare of logs with this version and
a working system shows, that with your patch (and fix) there are no calls to
gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If
you need any more information (what kind of logs will help?), let me know.

> ---
>   drivers/gpio/gpiolib.c | 7 ++++---
>   1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a53d17745d21..369bdd358fcc 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   			__set_bit(hwgpio, mask);
>   
>   			if (array_info)
> -				find_next_zero_bit(array_info->get_mask,
> +				i = find_next_zero_bit(array_info->get_mask,
>   						   array_size, i);
>   			else
>   				i++;
> @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>   			trace_gpio_value(desc_to_gpio(desc), 1, value);
>   
>   			if (array_info)
> -				find_next_zero_bit(array_info->get_mask, i, j);
> +				j = find_next_zero_bit(array_info->get_mask, i,
> +						       j);
>   			else
>   				j++;
>   		}
> @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>   			}
>   
>   			if (array_info)
> -				find_next_zero_bit(array_info->set_mask,
> +				i = find_next_zero_bit(array_info->set_mask,
>   						   array_size, i);
>   			else
>   				i++;

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Janusz Krzysztofik]

Hi Marek,

2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
> Hi Janusz,
>
> On 2018-09-20 18:21, Janusz Krzysztofik wrote:
>> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote:
>>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote:
>>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
>>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may
>>>>> contain
>>>>> information on direct mapping of array members to pins of a single
>>>>> GPIO
>>>>> chip in hardware order.  In such cases, bitmaps of values can be
>>>>> passed
>>>>> directly from/to the chip's .get/set_multiple() callbacks without
>>>>> wasting time on iterations.
>>>>>
>>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions.
>>>>> Pins not applicable for fast path are processed as before, skipping
>>>>> over the 'fast' ones.
>>>>>
>>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>>> I've just noticed that this patch landed in today's linux-next. Sadly
>>>> it
>>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
>>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
>>>>
>>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the
>>>> boot. I will try later to add some debugs and investigate it further
>>>> what
>>>> really happens when booting hangs.
>>> Hi Marek,
>>>
>>> Thanks for reporting. Could you please try the following fix?
>> Hi again,
>>
>> I realized the patch was not correct, j, not i, should be updated in
>> second
>> hunk. Please try the following one.
>>
>> Thanks,
>> Janusz
>>
>> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
>> From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>> Date: Thu, 20 Sep 2018 17:37:21 +0200
>> Subject: [PATCH] gpiolib: Fix bitmap index not updated
>> While skipping fast path bits, bitmap index is not updated with next
>> found zero bit position. Fix it.
>>
>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>
> This one also doesn't help. A quick compare of logs with this version and
> a working system shows, that with your patch (and fix) there are no calls
> to
> gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If
> you need any more information (what kind of logs will help?), let me know.

There is a debug message on array_info content available at the end of
gpiod_get_array(), could you please activate it and post the message so
we can understand better what is going on?

On the other hand, I've had a look your device-tree configuration and
it looks like that specific setup won't benefit from the fast bitmap path.
You have pin 2 at position 0 and pin 1 at position 1 of the array.
Hence, the fast bitmap path covers only pin 1, and pin 2 is processed
by the old path with apparently buggy code for skipping over fast pins.

As a temporary workaround, you could try to revert the order of pins in
your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code
should work for you again by taking the original old path, not skipping
over fast pins.  Results of such check may also help us to better
understand and resolve the issue.

Thanks,
Janusz

>
>> ---
>>   drivers/gpio/gpiolib.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>> index a53d17745d21..369bdd358fcc 100644
>> --- a/drivers/gpio/gpiolib.c
>> +++ b/drivers/gpio/gpiolib.c
>> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool
>> can_sleep,
>>   			__set_bit(hwgpio, mask);
>>
>>   			if (array_info)
>> -				find_next_zero_bit(array_info->get_mask,
>> +				i = find_next_zero_bit(array_info->get_mask,
>>   						   array_size, i);
>>   			else
>>   				i++;
>> @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool
>> can_sleep,
>>   			trace_gpio_value(desc_to_gpio(desc), 1, value);
>>
>>   			if (array_info)
>> -				find_next_zero_bit(array_info->get_mask, i, j);
>> +				j = find_next_zero_bit(array_info->get_mask, i,
>> +						       j);
>>   			else
>>   				j++;
>>   		}
>> @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool
>> can_sleep,
>>   			}
>>
>>   			if (array_info)
>> -				find_next_zero_bit(array_info->set_mask,
>> +				i = find_next_zero_bit(array_info->set_mask,
>>   						   array_size, i);
>>   			else
>>   				i++;
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
>

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Janusz Krzysztofik]

Hi Marek,

2018-09-21 12:51 GMT+02:00, Janusz Krzysztofik <jmkrzyszt@gmail.com>:
> Hi Marek,
>
> 2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
>> Hi Janusz,
>>
>> On 2018-09-20 18:21, Janusz Krzysztofik wrote:
>>> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik
>>> wrote:
>>>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski
>>>> wrote:
>>>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
>>>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may
>>>>>> contain
>>>>>> information on direct mapping of array members to pins of a single
>>>>>> GPIO
>>>>>> chip in hardware order.  In such cases, bitmaps of values can be
>>>>>> passed
>>>>>> directly from/to the chip's .get/set_multiple() callbacks without
>>>>>> wasting time on iterations.
>>>>>>
>>>>>> Add respective code to gpiod_get/set_array_bitmap_complex()
>>>>>> functions.
>>>>>> Pins not applicable for fast path are processed as before, skipping
>>>>>> over the 'fast' ones.
>>>>>>
>>>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>>>> I've just noticed that this patch landed in today's linux-next. Sadly
>>>>> it
>>>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
>>>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
>>>>>
>>>>> Booting hangs after detecting MMC cards. Reverting this patch fixes
>>>>> the
>>>>> boot. I will try later to add some debugs and investigate it further
>>>>> what
>>>>> really happens when booting hangs.
>>>> Hi Marek,
>>>>
>>>> Thanks for reporting. Could you please try the following fix?
>>> Hi again,
>>>
>>> I realized the patch was not correct, j, not i, should be updated in
>>> second
>>> hunk. Please try the following one.
>>>
>>> Thanks,
>>> Janusz
>>>
>>> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
>>> From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>> Date: Thu, 20 Sep 2018 17:37:21 +0200
>>> Subject: [PATCH] gpiolib: Fix bitmap index not updated
>>> While skipping fast path bits, bitmap index is not updated with next
>>> found zero bit position. Fix it.
>>>
>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>
>> This one also doesn't help. A quick compare of logs with this version and
>> a working system shows, that with your patch (and fix) there are no calls
>> to
>> gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If
>> you need any more information (what kind of logs will help?), let me
>> know.

One more question. You said before that booting hanged after detecting MMC
cards.  Without the fix, I could imagine it keeps iterating with index not
updated and simply never returns from gpiod_get/set_array_bitmap_complex().
Is the behaviour you observe the same with the fix applied?

Thanks,
Janusz

> There is a debug message on array_info content available at the end of
> gpiod_get_array(), could you please activate it and post the message so
> we can understand better what is going on?
>
> On the other hand, I've had a look your device-tree configuration and
> it looks like that specific setup won't benefit from the fast bitmap path.
> You have pin 2 at position 0 and pin 1 at position 1 of the array.
> Hence, the fast bitmap path covers only pin 1, and pin 2 is processed
> by the old path with apparently buggy code for skipping over fast pins.
>
> As a temporary workaround, you could try to revert the order of pins in
> your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code
> should work for you again by taking the original old path, not skipping
> over fast pins.  Results of such check may also help us to better
> understand and resolve the issue.
>
> Thanks,
> Janusz
>
>>
>>> ---
>>>   drivers/gpio/gpiolib.c | 7 ++++---
>>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
>>> index a53d17745d21..369bdd358fcc 100644
>>> --- a/drivers/gpio/gpiolib.c
>>> +++ b/drivers/gpio/gpiolib.c
>>> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool
>>> can_sleep,
>>>   			__set_bit(hwgpio, mask);
>>>
>>>   			if (array_info)
>>> -				find_next_zero_bit(array_info->get_mask,
>>> +				i = find_next_zero_bit(array_info->get_mask,
>>>   						   array_size, i);
>>>   			else
>>>   				i++;
>>> @@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool
>>> can_sleep,
>>>   			trace_gpio_value(desc_to_gpio(desc), 1, value);
>>>
>>>   			if (array_info)
>>> -				find_next_zero_bit(array_info->get_mask, i, j);
>>> +				j = find_next_zero_bit(array_info->get_mask, i,
>>> +						       j);
>>>   			else
>>>   				j++;
>>>   		}
>>> @@ -3192,7 +3193,7 @@ int gpiod_set_array_value_complex(bool raw, bool
>>> can_sleep,
>>>   			}
>>>
>>>   			if (array_info)
>>> -				find_next_zero_bit(array_info->set_mask,
>>> +				i = find_next_zero_bit(array_info->set_mask,
>>>   						   array_size, i);
>>>   			else
>>>   				i++;
>>
>> Best regards
>> --
>> Marek Szyprowski, PhD
>> Samsung R&D Institute Poland
>>
>>
>

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Marek Szyprowski]

Hi Janusz,


On 2018-09-21 12:51, Janusz Krzysztofik wrote:
> 2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
>> On 2018-09-20 18:21, Janusz Krzysztofik wrote:
>>> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote:
>>>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski wrote:
>>>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
>>>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may
>>>>>> contain
>>>>>> information on direct mapping of array members to pins of a single
>>>>>> GPIO
>>>>>> chip in hardware order.  In such cases, bitmaps of values can be
>>>>>> passed
>>>>>> directly from/to the chip's .get/set_multiple() callbacks without
>>>>>> wasting time on iterations.
>>>>>>
>>>>>> Add respective code to gpiod_get/set_array_bitmap_complex() functions.
>>>>>> Pins not applicable for fast path are processed as before, skipping
>>>>>> over the 'fast' ones.
>>>>>>
>>>>>> Cc: Jonathan Corbet <corbet@lwn.net>
>>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>>>> I've just noticed that this patch landed in today's linux-next. Sadly
>>>>> it
>>>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
>>>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
>>>>>
>>>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the
>>>>> boot. I will try later to add some debugs and investigate it further
>>>>> what
>>>>> really happens when booting hangs.
>>>> Hi Marek,
>>>>
>>>> Thanks for reporting. Could you please try the following fix?
>>> Hi again,
>>>
>>> I realized the patch was not correct, j, not i, should be updated in
>>> second
>>> hunk. Please try the following one.
>>>
>>> Thanks,
>>> Janusz
>>>
>>> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
>>> From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>>> Date: Thu, 20 Sep 2018 17:37:21 +0200
>>> Subject: [PATCH] gpiolib: Fix bitmap index not updated
>>> While skipping fast path bits, bitmap index is not updated with next
>>> found zero bit position. Fix it.
>>>
>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>> This one also doesn't help. A quick compare of logs with this version and
>> a working system shows, that with your patch (and fix) there are no calls
>> to
>> gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If
>> you need any more information (what kind of logs will help?), let me know.
> There is a debug message on array_info content available at the end of
> gpiod_get_array(), could you please activate it and post the message so
> we can understand better what is going on?

With debug enabled on next-20180919:
[    2.499153] pwrseq_simple mmc3_pwrseq: GPIO array info: chip=gpx0, 
size=2, get_mask=2, set_mask=2, invert_mask=2

On next-20180920 I get no this message and booting hangs.

Same with next-20180920 + your second fix from this thread.

I will try to debug this more on Monday.

> On the other hand, I've had a look your device-tree configuration and
> it looks like that specific setup won't benefit from the fast bitmap path.
> You have pin 2 at position 0 and pin 1 at position 1 of the array.
> Hence, the fast bitmap path covers only pin 1, and pin 2 is processed
> by the old path with apparently buggy code for skipping over fast pins.
>
> As a temporary workaround, you could try to revert the order of pins in
> your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code
> should work for you again by taking the original old path, not skipping
> over fast pins.  Results of such check may also help us to better
> understand and resolve the issue.

Changing the order of mmc pwrseq gpio pins fixes boot hang.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Janusz Krzysztofik]

On Friday, September 21, 2018 4:14:06 PM CEST Marek Szyprowski wrote:
> Hi Janusz,
> 
> 
> On 2018-09-21 12:51, Janusz Krzysztofik wrote:
> > 2018-09-21 10:18 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
> >> On 2018-09-20 18:21, Janusz Krzysztofik wrote:
> >>> On Thursday, September 20, 2018 5:48:22 PM CEST Janusz Krzysztofik wrote:
> >>>> On Thursday, September 20, 2018 12:11:48 PM CEST Marek Szyprowski 
wrote:
> >>>>> On 2018-09-02 14:01, Janusz Krzysztofik wrote:
> >>>>>> Certain GPIO descriptor arrays returned by gpio_get_array() may
> >>>>>> contain
> >>>>>> information on direct mapping of array members to pins of a single
> >>>>>> GPIO
> >>>>>> chip in hardware order.  In such cases, bitmaps of values can be
> >>>>>> passed
> >>>>>> directly from/to the chip's .get/set_multiple() callbacks without
> >>>>>> wasting time on iterations.
> >>>>>>
> >>>>>> Add respective code to gpiod_get/set_array_bitmap_complex() 
functions.
> >>>>>> Pins not applicable for fast path are processed as before, skipping
> >>>>>> over the 'fast' ones.
> >>>>>>
> >>>>>> Cc: Jonathan Corbet <corbet@lwn.net>
> >>>>>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> >>>>> I've just noticed that this patch landed in today's linux-next. Sadly
> >>>>> it
> >>>>> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
> >>>>> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).
> >>>>>
> >>>>> Booting hangs after detecting MMC cards. Reverting this patch fixes the
> >>>>> boot. I will try later to add some debugs and investigate it further
> >>>>> what
> >>>>> really happens when booting hangs.
> >>>> Hi Marek,
> >>>>
> >>>> Thanks for reporting. Could you please try the following fix?
> >>> Hi again,
> >>>
> >>> I realized the patch was not correct, j, not i, should be updated in
> >>> second
> >>> hunk. Please try the following one.
> >>>
> >>> Thanks,
> >>> Janusz
> >>>
> >>> >From a919c504850f6cb40e8e81267a3a37537f7c4fd4 Mon Sep 17 00:00:00 2001
> >>> From: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> >>> Date: Thu, 20 Sep 2018 17:37:21 +0200
> >>> Subject: [PATCH] gpiolib: Fix bitmap index not updated
> >>> While skipping fast path bits, bitmap index is not updated with next
> >>> found zero bit position. Fix it.
> >>>
> >>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> >> This one also doesn't help. A quick compare of logs with this version and
> >> a working system shows, that with your patch (and fix) there are no calls
> >> to
> >> gpx0-2 pin (which are a part of mmc pwrseq), what causes mmc failure. If
> >> you need any more information (what kind of logs will help?), let me 
know.
> > There is a debug message on array_info content available at the end of
> > gpiod_get_array(), could you please activate it and post the message so
> > we can understand better what is going on?
> 
> With debug enabled on next-20180919:
> [    2.499153] pwrseq_simple mmc3_pwrseq: GPIO array info: chip=gpx0, 
> size=2, get_mask=2, set_mask=2, invert_mask=2

Looks good to me, i..e., in line with what one could expect.  However, ...

> On next-20180920 I get no this message and booting hangs.
> 
> Same with next-20180920 + your second fix from this thread.
> 
> I will try to debug this more on Monday.
> 
> > On the other hand, I've had a look your device-tree configuration and
> > it looks like that specific setup won't benefit from the fast bitmap path.
> > You have pin 2 at position 0 and pin 1 at position 1 of the array.
> > Hence, the fast bitmap path covers only pin 1, and pin 2 is processed
> > by the old path with apparently buggy code for skipping over fast pins.
> >
> > As a temporary workaround, you could try to revert the order of pins in
> > your dts file (pin 1 at position 0, pin 2 at 1) and the mmc pwrseq code
> > should work for you again by taking the original old path, not skipping
> > over fast pins.  Results of such check may also help us to better
> > understand and resolve the issue.
> 
> Changing the order of mmc pwrseq gpio pins fixes boot hang.

Not being able to discover more coding bugs in the code modified by the series, 
I'm wondering if the reason for the issue you are observing comes from the 
fact both pins are no longer manipulated together within a single 
.set_multiple() chip callback. I'm working on a fix which prevents from that.

Thanks,
Janusz

[PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path

[Janusz Krzysztofik]

While investigating possible reasons of GPIO fast bitmap processing
related boot hang on Samsung Snow Chromebook, reported by Marek
Szyprowski (thanks!), I've discovered one coding bug, addressed by
PATCH 1/2 of this series, and one potential regression introduced at
design level of the solution, hopefully fixed by PATCH 2/2.  See
commit messages for details.

Janusz Krzysztofik (2):
      gpiolib: Fix missing updates of bitmap index
      gpiolib: Fix array members of same chip processed separately

The fixes should resolve the boot hang observed by Marek, however the
second change excludes that particular case from fast bitmap processing
and restores the old behaviour.  Hence, it is possible still another
issue which have had an influence on that boot hang exists in the code.
In order to fully verify the fix, it would have to be tested on a
platform where an array of GPIO descriptors is used which starts from
at least two consecutive pins of one GPIO chip in hardware order,
starting ftom 0, followed by one or more pins belonging to other
chip(s).

In order to verify if separate calls to .set() chip callback for each
pin instead of one call to .set_multiple() is actually the reason of
boot hang on Samsung Snow Chromebook, the affected driver -
drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
modified for testing purposes so it calls gpiod_set_value() for each
pin instead of gpiod_set_array_value() for all of them.  If that would
also result in boot hang, we could be sure the issue was really the
one addressed by the second fix.  Marek, could you please try to
perform such test?

Thanks,
Janusz


diffstat:
 Documentation/driver-api/gpio/board.rst |   19 +++++++++----
 drivers/gpio/gpiolib.c                  |   46 +++++++++++++++++++++-----------
 2 files changed, 45 insertions(+), 20 deletions(-)

[PATCH 1/2] gpiolib: Fix missing updates of bitmap index

[Janusz Krzysztofik]

In new code introduced by commit b17566a6b08b ("gpiolib: Implement fast
processing path in get/set array"), bitmap index is not updated with
next found zero bit position as it should while skipping over pins
already processed via fast bitmap path, possibly resulting in an
infinite loop.  Fix it.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 drivers/gpio/gpiolib.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index a53d17745d21..7d9536a79a66 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2880,8 +2880,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			__set_bit(hwgpio, mask);
 
 			if (array_info)
-				find_next_zero_bit(array_info->get_mask,
-						   array_size, i);
+				i = find_next_zero_bit(array_info->get_mask,
+						       array_size, i);
 			else
 				i++;
 		} while ((i < array_size) &&
@@ -2905,7 +2905,8 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			trace_gpio_value(desc_to_gpio(desc), 1, value);
 
 			if (array_info)
-				find_next_zero_bit(array_info->get_mask, i, j);
+				j = find_next_zero_bit(array_info->get_mask, i,
+						       j);
 			else
 				j++;
 		}
@@ -3192,8 +3193,8 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 			}
 
 			if (array_info)
-				find_next_zero_bit(array_info->set_mask,
-						   array_size, i);
+				i = find_next_zero_bit(array_info->set_mask,
+						       array_size, i);
 			else
 				i++;
 		} while ((i < array_size) &&
-- 
2.16.4

Re: [PATCH 1/2] gpiolib: Fix missing updates of bitmap index

[Linus Walleij]

On Mon, Sep 24, 2018 at 1:52 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> In new code introduced by commit b17566a6b08b ("gpiolib: Implement fast
> processing path in get/set array"), bitmap index is not updated with
> next found zero bit position as it should while skipping over pins
> already processed via fast bitmap path, possibly resulting in an
> infinite loop.  Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Patch applied!

Thanks for working on getting this into shape!

Yours,
Linus Walleij

[PATCH] gpiolib: Fix incorrect use of find_next_zero_bit()

[Janusz Krzysztofik]

Commit b17566a6b08b ("gpiolib: Implement fast processing path in
get/set array"), already fixed to some extent with commit 5d581d7e8cdc
("gpiolib: Fix missing updates of bitmap index"), introduced a new mode
of processing bitmaps where bits applicable for fast bitmap processing
path are supposed to be skipped while iterating bits which don't apply.
Unfortunately, find_next_zero_bit() function supposed to skip over
those fast bits is always called with a 'start' argument equal to an
index of last zero bit found and returns that index value again an
again, causing an infinite loop.

Fix it by incrementing the index uncoditionally before
find_next_zero_bit() is optionally called.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
Marek,

Could you please test it on top of next-20180920 with "gpiolib: Fix
missing updates of bitmap index" and optionally "mmc: pwrseq_simple:
Fix incorrect handling of GPIO bitmap" also applied?

Thanks,
Janusz


 drivers/gpio/gpiolib.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 6ae13e3e05f1..940b543e966d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2878,12 +2878,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 			int hwgpio = gpio_chip_hwgpio(desc);
 
 			__set_bit(hwgpio, mask);
+			i++;
 
 			if (array_info)
 				i = find_next_zero_bit(array_info->get_mask,
 						       array_size, i);
-			else
-				i++;
 		} while ((i < array_size) &&
 			 (desc_array[i]->gdev->chip == chip));
 
@@ -2903,12 +2902,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
 				value = !value;
 			__assign_bit(j, value_bitmap, value);
 			trace_gpio_value(desc_to_gpio(desc), 1, value);
+			j++;
 
 			if (array_info)
 				j = find_next_zero_bit(array_info->get_mask, i,
 						       j);
-			else
-				j++;
 		}
 
 		if (mask != fastpath)
@@ -3191,12 +3189,11 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
 					__clear_bit(hwgpio, bits);
 				count++;
 			}
+			i++;
 
 			if (array_info)
 				i = find_next_zero_bit(array_info->set_mask,
 						       array_size, i);
-			else
-				i++;
 		} while ((i < array_size) &&
 			 (desc_array[i]->gdev->chip == chip));
 		/* push collected bits to outputs */
-- 
2.16.4

Re: [PATCH] gpiolib: Fix incorrect use of find_next_zero_bit()

[Marek Szyprowski]

Hi Janusz,

On 2018-09-29 14:20, Janusz Krzysztofik wrote:
> Commit b17566a6b08b ("gpiolib: Implement fast processing path in
> get/set array"), already fixed to some extent with commit 5d581d7e8cdc
> ("gpiolib: Fix missing updates of bitmap index"), introduced a new mode
> of processing bitmaps where bits applicable for fast bitmap processing
> path are supposed to be skipped while iterating bits which don't apply.
> Unfortunately, find_next_zero_bit() function supposed to skip over
> those fast bits is always called with a 'start' argument equal to an
> index of last zero bit found and returns that index value again an
> again, causing an infinite loop.
>
> Fix it by incrementing the index uncoditionally before
> find_next_zero_bit() is optionally called.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> ---
> Marek,
>
> Could you please test it on top of next-20180920 with "gpiolib: Fix
> missing updates of bitmap index" and optionally "mmc: pwrseq_simple:
> Fix incorrect handling of GPIO bitmap" also applied?

This patch finally fixes the boot issue on Samsung Chromebook Snow.
Thanks!

>
> Thanks,
> Janusz
>
>
>  drivers/gpio/gpiolib.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 6ae13e3e05f1..940b543e966d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2878,12 +2878,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>  			int hwgpio = gpio_chip_hwgpio(desc);
>  
>  			__set_bit(hwgpio, mask);
> +			i++;
>  
>  			if (array_info)
>  				i = find_next_zero_bit(array_info->get_mask,
>  						       array_size, i);
> -			else
> -				i++;
>  		} while ((i < array_size) &&
>  			 (desc_array[i]->gdev->chip == chip));
>  
> @@ -2903,12 +2902,11 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>  				value = !value;
>  			__assign_bit(j, value_bitmap, value);
>  			trace_gpio_value(desc_to_gpio(desc), 1, value);
> +			j++;
>  
>  			if (array_info)
>  				j = find_next_zero_bit(array_info->get_mask, i,
>  						       j);
> -			else
> -				j++;
>  		}
>  
>  		if (mask != fastpath)
> @@ -3191,12 +3189,11 @@ int gpiod_set_array_value_complex(bool raw, bool can_sleep,
>  					__clear_bit(hwgpio, bits);
>  				count++;
>  			}
> +			i++;
>  
>  			if (array_info)
>  				i = find_next_zero_bit(array_info->set_mask,
>  						       array_size, i);
> -			else
> -				i++;
>  		} while ((i < array_size) &&
>  			 (desc_array[i]->gdev->chip == chip));
>  		/* push collected bits to outputs */

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] gpiolib: Fix incorrect use of find_next_zero_bit()

[Linus Walleij]

On Sat, Sep 29, 2018 at 2:19 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Commit b17566a6b08b ("gpiolib: Implement fast processing path in
> get/set array"), already fixed to some extent with commit 5d581d7e8cdc
> ("gpiolib: Fix missing updates of bitmap index"), introduced a new mode
> of processing bitmaps where bits applicable for fast bitmap processing
> path are supposed to be skipped while iterating bits which don't apply.
> Unfortunately, find_next_zero_bit() function supposed to skip over
> those fast bits is always called with a 'start' argument equal to an
> index of last zero bit found and returns that index value again an
> again, causing an infinite loop.
>
> Fix it by incrementing the index uncoditionally before
> find_next_zero_bit() is optionally called.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Patch applied with Marek's Tested-by.

Thanks to both of you for digging in and fixing this up!
Now we are in good shape for the v4.20 cycle :)

Yours,
Linus Walleij

[PATCH 2/2] gpiolib: Fix array members of same chip processed separately

[Janusz Krzysztofik]

New code introduced by commit bf9346f5d47b ("gpiolib: Identify arrays
matching GPIO hardware") forcibly tries to find an array member which
has its array index number equal to its hardware pin number and set
up an array info for possible fast bitmap processing of all arrray
pins belonging to that chip which also satisfy that numbering rule.

Depending on array content, it may happen that consecutive array
members which belong to the same chip but don't have array indexes
equal to their pin hardware numbers will be split into groups, some of
them processed together via the fast bitmap path, and rest of them
separetely.  However, applications may expect all those pins being
processed together with a single call to .set_multiple() chip callback,
like that was done before the change.

Limit applicability of fast bitmap processing path to cases where all
pins of consecutive array members starting from 0 which belong to the
same chip have their hardware numbers equal to their corresponding
array indexes.  That should still speed up processing of applications
using whole GPIO banks as I/O ports, while not breaking simultaneous
manipulation of consecutive pins of the same chip which don't follow
the equal numbering rule.

Cc: Jonathan Corbet <corbet@lwn.net>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
 Documentation/driver-api/gpio/board.rst | 19 +++++++++++++-----
 drivers/gpio/gpiolib.c                  | 35 +++++++++++++++++++++++----------
 2 files changed, 39 insertions(+), 15 deletions(-)

diff --git a/Documentation/driver-api/gpio/board.rst b/Documentation/driver-api/gpio/board.rst
index c66821e033c2..a0f294e2e250 100644
--- a/Documentation/driver-api/gpio/board.rst
+++ b/Documentation/driver-api/gpio/board.rst
@@ -202,9 +202,18 @@ mapped to the device determines if the array qualifies for fast bitmap
 processing.  If yes, a bitmap is passed over get/set array functions directly
 between a caller and a respective .get/set_multiple() callback of a GPIO chip.
 
-In order to qualify for fast bitmap processing, the pin mapping must meet the
+In order to qualify for fast bitmap processing, the array must meet the
 following requirements:
-- it must belong to the same chip as other 'fast' pins of the function,
-- its index within the function must match its hardware number within the chip.
-
-Open drain and open source pins are excluded from fast bitmap output processing.
+- pin hardware number of array member 0 must also be 0,
+- pin hardware numbers of consecutive array members which belong to the same
+  chip as member 0 does must also match their array indexes.
+
+Otherwise fast bitmap processing path is not used in order to avoid consecutive
+pins which belong to the same chip but are not in hardware order being processed
+separately.
+
+If the array applies for fast bitmap processing path, pins which belong to
+different chips than member 0 does, as well as those with indexes different from
+their hardware pin numbers, are excluded from the fast path, both input and
+output.  Moreover, open drain and open source pins are excluded from fast bitmap
+output processing.
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 7d9536a79a66..6ae13e3e05f1 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -4376,11 +4376,10 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 
 		chip = gpiod_to_chip(desc);
 		/*
-		 * Select a chip of first array member
-		 * whose index matches its pin hardware number
-		 * as a candidate for fast bitmap processing.
+		 * If pin hardware number of array member 0 is also 0, select
+		 * its chip as a candidate for fast bitmap processing path.
 		 */
-		if (!array_info && gpio_chip_hwgpio(desc) == descs->ndescs) {
+		if (descs->ndescs == 0 && gpio_chip_hwgpio(desc) == 0) {
 			struct gpio_descs *array;
 
 			bitmap_size = BITS_TO_LONGS(chip->ngpio > count ?
@@ -4414,14 +4413,30 @@ struct gpio_descs *__must_check gpiod_get_array(struct device *dev,
 				   count - descs->ndescs);
 			descs->info = array_info;
 		}
-		/*
-		 * Unmark members which don't qualify for fast bitmap
-		 * processing (different chip, not in hardware order)
-		 */
-		if (array_info && (chip != array_info->chip ||
-		    gpio_chip_hwgpio(desc) != descs->ndescs)) {
+		/* Unmark array members which don't belong to the 'fast' chip */
+		if (array_info && array_info->chip != chip) {
 			__clear_bit(descs->ndescs, array_info->get_mask);
 			__clear_bit(descs->ndescs, array_info->set_mask);
+		}
+		/*
+		 * Detect array members which belong to the 'fast' chip
+		 * but their pins are not in hardware order.
+		 */
+		else if (array_info &&
+			   gpio_chip_hwgpio(desc) != descs->ndescs) {
+			/*
+			 * Don't use fast path if all array members processed so
+			 * far belong to the same chip as this one but its pin
+			 * hardware number is different from its array index.
+			 */
+			if (bitmap_full(array_info->get_mask, descs->ndescs)) {
+				array_info = NULL;
+			} else {
+				__clear_bit(descs->ndescs,
+					    array_info->get_mask);
+				__clear_bit(descs->ndescs,
+					    array_info->set_mask);
+			}
 		} else if (array_info) {
 			/* Exclude open drain or open source from fast output */
 			if (gpiochip_line_is_open_drain(chip, descs->ndescs) ||
-- 
2.16.4

Re: [PATCH 2/2] gpiolib: Fix array members of same chip processed separately

[Linus Walleij]

On Mon, Sep 24, 2018 at 1:52 AM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> New code introduced by commit bf9346f5d47b ("gpiolib: Identify arrays
> matching GPIO hardware") forcibly tries to find an array member which
> has its array index number equal to its hardware pin number and set
> up an array info for possible fast bitmap processing of all arrray
> pins belonging to that chip which also satisfy that numbering rule.
>
> Depending on array content, it may happen that consecutive array
> members which belong to the same chip but don't have array indexes
> equal to their pin hardware numbers will be split into groups, some of
> them processed together via the fast bitmap path, and rest of them
> separetely.  However, applications may expect all those pins being
> processed together with a single call to .set_multiple() chip callback,
> like that was done before the change.
>
> Limit applicability of fast bitmap processing path to cases where all
> pins of consecutive array members starting from 0 which belong to the
> same chip have their hardware numbers equal to their corresponding
> array indexes.  That should still speed up processing of applications
> using whole GPIO banks as I/O ports, while not breaking simultaneous
> manipulation of consecutive pins of the same chip which don't follow
> the equal numbering rule.
>
> Cc: Jonathan Corbet <corbet@lwn.net>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Patch applied!

Yours,
Linus Walleij

Re: [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path

[Marek Szyprowski]

Hi Janusz,

On 2018-09-24 01:53, Janusz Krzysztofik wrote:
> While investigating possible reasons of GPIO fast bitmap processing
> related boot hang on Samsung Snow Chromebook, reported by Marek
> Szyprowski (thanks!), I've discovered one coding bug, addressed by
> PATCH 1/2 of this series, and one potential regression introduced at
> design level of the solution, hopefully fixed by PATCH 2/2.  See
> commit messages for details.
>
> Janusz Krzysztofik (2):
>        gpiolib: Fix missing updates of bitmap index
>        gpiolib: Fix array members of same chip processed separately
>
> The fixes should resolve the boot hang observed by Marek, however the
> second change excludes that particular case from fast bitmap processing
> and restores the old behaviour.

I confirm, that the above 2 patches fixes boot issue on Samsung Snow
Chromebook with next-20180920.

Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>

> Hence, it is possible still another
> issue which have had an influence on that boot hang exists in the code.
> In order to fully verify the fix, it would have to be tested on a
> platform where an array of GPIO descriptors is used which starts from
> at least two consecutive pins of one GPIO chip in hardware order,
> starting ftom 0, followed by one or more pins belonging to other
> chip(s).
>
> In order to verify if separate calls to .set() chip callback for each
> pin instead of one call to .set_multiple() is actually the reason of
> boot hang on Samsung Snow Chromebook, the affected driver -
> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
> modified for testing purposes so it calls gpiod_set_value() for each
> pin instead of gpiod_set_array_value() for all of them.  If that would
> also result in boot hang, we could be sure the issue was really the
> one addressed by the second fix.  Marek, could you please try to
> perform such test?

Yes, I've just tested next-20180920 only with the first patch from this
patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c.
It boots fine, so indeed the issue is in handling of arrays of gpios.

Just to be sure I did it right, this is my change to the mentioned file:

diff --git a/drivers/mmc/core/pwrseq_simple.c 
b/drivers/mmc/core/pwrseq_simple.c
index 7f882a2bb872..9397dc1f2e38 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct 
mmc_pwrseq_simple *pwrseq,
                                               int value)
  {
         struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
+       int i;

-       if (!IS_ERR(reset_gpios)) {
-               DECLARE_BITMAP(values, BITS_PER_TYPE(value));
-               int nvalues = reset_gpios->ndescs;
-
-               values[0] = value;
-
-               gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
-                                              reset_gpios->info, values);
-       }
+       if (!IS_ERR(reset_gpios))
+               for (i = 0; i < reset_gpios->ndescs; i++)
+ gpiod_set_value_cansleep(reset_gpios->desc[i], value);
  }

  static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)


Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path

[Janusz Krzysztofik]

Hi Marek,

2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
> Hi Janusz,
>
> On 2018-09-24 01:53, Janusz Krzysztofik wrote:
>> While investigating possible reasons of GPIO fast bitmap processing
>> related boot hang on Samsung Snow Chromebook, reported by Marek
>> Szyprowski (thanks!), I've discovered one coding bug, addressed by
>> PATCH 1/2 of this series, and one potential regression introduced at
>> design level of the solution, hopefully fixed by PATCH 2/2.  See
>> commit messages for details.
>>
>> Janusz Krzysztofik (2):
>>        gpiolib: Fix missing updates of bitmap index
>>        gpiolib: Fix array members of same chip processed separately
>>
>> The fixes should resolve the boot hang observed by Marek, however the
>> second change excludes that particular case from fast bitmap processing
>> and restores the old behaviour.
>
> I confirm, that the above 2 patches fixes boot issue on Samsung Snow
> Chromebook with next-20180920.
>
> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>
>> Hence, it is possible still another
>> issue which have had an influence on that boot hang exists in the code.
>> In order to fully verify the fix, it would have to be tested on a
>> platform where an array of GPIO descriptors is used which starts from
>> at least two consecutive pins of one GPIO chip in hardware order,
>> starting ftom 0, followed by one or more pins belonging to other
>> chip(s).
>>
>> In order to verify if separate calls to .set() chip callback for each
>> pin instead of one call to .set_multiple() is actually the reason of
>> boot hang on Samsung Snow Chromebook, the affected driver -
>> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
>> modified for testing purposes so it calls gpiod_set_value() for each
>> pin instead of gpiod_set_array_value() for all of them.  If that would
>> also result in boot hang, we could be sure the issue was really the
>> one addressed by the second fix.  Marek, could you please try to
>> perform such test?
>
> Yes, I've just tested next-20180920 only with the first patch from this
> patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c.
> It boots fine, so indeed the issue is in handling of arrays of gpios.
>
> Just to be sure I did it right, this is my change to the mentioned file:

Yeah, that's what I had on mind.  However, I'd be more lucky if it didn't work
for you.  Setting the pins sequentially, not simultaneously as before, was
exactly what I hoped was the reason of the hang.

> diff --git a/drivers/mmc/core/pwrseq_simple.c
> b/drivers/mmc/core/pwrseq_simple.c
> index 7f882a2bb872..9397dc1f2e38 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct
> mmc_pwrseq_simple *pwrseq,
>                                                int value)
>   {
>          struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
> +       int i;
>
> -       if (!IS_ERR(reset_gpios)) {
> -               DECLARE_BITMAP(values, BITS_PER_TYPE(value));
> -               int nvalues = reset_gpios->ndescs;
> -
> -               values[0] = value;
> -
> -               gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
> -                                              reset_gpios->info, values);
> -       }
> +       if (!IS_ERR(reset_gpios))
> +               for (i = 0; i < reset_gpios->ndescs; i++)

The only difference from the behaviour when the hang was occurring is now
the order the pins are manipulated.  Maybe that matters?
Could you please retry the same with the order of pins reversed, either in
the .dts file or here inside this for loop?

Thanks,
Janusz

> + gpiod_set_value_cansleep(reset_gpios->desc[i], value);
>   }
>
>   static void mmc_pwrseq_simple_pre_power_on(struct mmc_host *host)
>
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
>

Re: [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path

[Marek Szyprowski]

Hi Janusz,

On 2018-09-24 13:08, Janusz Krzysztofik wrote:
> 2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
>> On 2018-09-24 01:53, Janusz Krzysztofik wrote:
>>> While investigating possible reasons of GPIO fast bitmap processing
>>> related boot hang on Samsung Snow Chromebook, reported by Marek
>>> Szyprowski (thanks!), I've discovered one coding bug, addressed by
>>> PATCH 1/2 of this series, and one potential regression introduced at
>>> design level of the solution, hopefully fixed by PATCH 2/2.  See
>>> commit messages for details.
>>>
>>> Janusz Krzysztofik (2):
>>>         gpiolib: Fix missing updates of bitmap index
>>>         gpiolib: Fix array members of same chip processed separately
>>>
>>> The fixes should resolve the boot hang observed by Marek, however the
>>> second change excludes that particular case from fast bitmap processing
>>> and restores the old behaviour.
>> I confirm, that the above 2 patches fixes boot issue on Samsung Snow
>> Chromebook with next-20180920.
>>
>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>
>>> Hence, it is possible still another
>>> issue which have had an influence on that boot hang exists in the code.
>>> In order to fully verify the fix, it would have to be tested on a
>>> platform where an array of GPIO descriptors is used which starts from
>>> at least two consecutive pins of one GPIO chip in hardware order,
>>> starting ftom 0, followed by one or more pins belonging to other
>>> chip(s).
>>>
>>> In order to verify if separate calls to .set() chip callback for each
>>> pin instead of one call to .set_multiple() is actually the reason of
>>> boot hang on Samsung Snow Chromebook, the affected driver -
>>> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
>>> modified for testing purposes so it calls gpiod_set_value() for each
>>> pin instead of gpiod_set_array_value() for all of them.  If that would
>>> also result in boot hang, we could be sure the issue was really the
>>> one addressed by the second fix.  Marek, could you please try to
>>> perform such test?
>> Yes, I've just tested next-20180920 only with the first patch from this
>> patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c.
>> It boots fine, so indeed the issue is in handling of arrays of gpios.
>>
>> Just to be sure I did it right, this is my change to the mentioned file:
> Yeah, that's what I had on mind.  However, I'd be more lucky if it didn't work
> for you.  Setting the pins sequentially, not simultaneously as before, was
> exactly what I hoped was the reason of the hang.
>
>> diff --git a/drivers/mmc/core/pwrseq_simple.c
>> b/drivers/mmc/core/pwrseq_simple.c
>> index 7f882a2bb872..9397dc1f2e38 100644
>> --- a/drivers/mmc/core/pwrseq_simple.c
>> +++ b/drivers/mmc/core/pwrseq_simple.c
>> @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct
>> mmc_pwrseq_simple *pwrseq,
>>                                                 int value)
>>    {
>>           struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
>> +       int i;
>>
>> -       if (!IS_ERR(reset_gpios)) {
>> -               DECLARE_BITMAP(values, BITS_PER_TYPE(value));
>> -               int nvalues = reset_gpios->ndescs;
>> -
>> -               values[0] = value;
>> -
>> -               gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
>> -                                              reset_gpios->info, values);
>> -       }
>> +       if (!IS_ERR(reset_gpios))
>> +               for (i = 0; i < reset_gpios->ndescs; i++)
> The only difference from the behaviour when the hang was occurring is now
> the order the pins are manipulated.  Maybe that matters?
> Could you please retry the same with the order of pins reversed, either in
> the .dts file or here inside this for loop?

I've switched the order of pins in dts and next-20180920 + first patch + 
above
change also boots fine.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH 0/2] gpiolib: Fix issues introduced by fast bitmap processing path

[Janusz Krzysztofik]

Hi Marek,

2018-09-24 13:38 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
> Hi Janusz,
>
> On 2018-09-24 13:08, Janusz Krzysztofik wrote:
>> 2018-09-24 11:43 GMT+02:00, Marek Szyprowski <m.szyprowski@samsung.com>:
>>> On 2018-09-24 01:53, Janusz Krzysztofik wrote:
>>>> While investigating possible reasons of GPIO fast bitmap processing
>>>> related boot hang on Samsung Snow Chromebook, reported by Marek
>>>> Szyprowski (thanks!), I've discovered one coding bug, addressed by
>>>> PATCH 1/2 of this series, and one potential regression introduced at
>>>> design level of the solution, hopefully fixed by PATCH 2/2.  See
>>>> commit messages for details.
>>>>
>>>> Janusz Krzysztofik (2):
>>>>         gpiolib: Fix missing updates of bitmap index
>>>>         gpiolib: Fix array members of same chip processed separately
>>>>
>>>> The fixes should resolve the boot hang observed by Marek, however the
>>>> second change excludes that particular case from fast bitmap processing
>>>> and restores the old behaviour.
>>> I confirm, that the above 2 patches fixes boot issue on Samsung Snow
>>> Chromebook with next-20180920.
>>>
>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>>
>>>> Hence, it is possible still another
>>>> issue which have had an influence on that boot hang exists in the code.
>>>> In order to fully verify the fix, it would have to be tested on a
>>>> platform where an array of GPIO descriptors is used which starts from
>>>> at least two consecutive pins of one GPIO chip in hardware order,
>>>> starting ftom 0, followed by one or more pins belonging to other
>>>> chip(s).
>>>>
>>>> In order to verify if separate calls to .set() chip callback for each
>>>> pin instead of one call to .set_multiple() is actually the reason of
>>>> boot hang on Samsung Snow Chromebook, the affected driver -
>>>> drivers/mmc/core/pwrseq_simple.c - would have to be temporarily
>>>> modified for testing purposes so it calls gpiod_set_value() for each
>>>> pin instead of gpiod_set_array_value() for all of them.  If that would
>>>> also result in boot hang, we could be sure the issue was really the
>>>> one addressed by the second fix.  Marek, could you please try to
>>>> perform such test?
>>> Yes, I've just tested next-20180920 only with the first patch from this
>>> patchset and the mentioned change to drivers/mmc/core/pwrseq_simple.c.
>>> It boots fine, so indeed the issue is in handling of arrays of gpios.
>>>
>>> Just to be sure I did it right, this is my change to the mentioned file:
>> Yeah, that's what I had on mind.  However, I'd be more lucky if it didn't
>> work
>> for you.  Setting the pins sequentially, not simultaneously as before,
>> was
>> exactly what I hoped was the reason of the hang.
>>
>>> diff --git a/drivers/mmc/core/pwrseq_simple.c
>>> b/drivers/mmc/core/pwrseq_simple.c
>>> index 7f882a2bb872..9397dc1f2e38 100644
>>> --- a/drivers/mmc/core/pwrseq_simple.c
>>> +++ b/drivers/mmc/core/pwrseq_simple.c
>>> @@ -38,16 +38,11 @@ static void mmc_pwrseq_simple_set_gpios_value(struct
>>> mmc_pwrseq_simple *pwrseq,
>>>                                                 int value)
>>>    {
>>>           struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
>>> +       int i;
>>>
>>> -       if (!IS_ERR(reset_gpios)) {
>>> -               DECLARE_BITMAP(values, BITS_PER_TYPE(value));
>>> -               int nvalues = reset_gpios->ndescs;
>>> -
>>> -               values[0] = value;
>>> -
>>> -               gpiod_set_array_value_cansleep(nvalues,
>>> reset_gpios->desc,
>>> -                                              reset_gpios->info,
>>> values);
>>> -       }
>>> +       if (!IS_ERR(reset_gpios))
>>> +               for (i = 0; i < reset_gpios->ndescs; i++)
>> The only difference from the behaviour when the hang was occurring is now
>> the order the pins are manipulated.  Maybe that matters?
>> Could you please retry the same with the order of pins reversed, either
>> in
>> the .dts file or here inside this for loop?
>
> I've switched the order of pins in dts and next-20180920 + first patch +
> above
> change also boots fine.

Thanks for performing those tests.

Since we are not able to reproduce the issue by any means other than
using the original code introduced by fast bitmap processing changes,
regardless of the first fix being applied or not, and we are only able to
resolve the hangup by excluding affected use case from the fast path,
we have to assume one or more bugs which affect mixed arrays, i.e.,
those which apply for fast bitmap processing only in part, may still exist
in the code introduced by the fast bitmap processing series.  I hope we
are able to resolve it soon, before the changes reach mainline.

Thanks,
Janusz


> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
>

[PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Janusz Krzysztofik]

Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
get/set array") changed the way GPIO values are passed to
gpiod_get/set_array_value() and friends.  The updated code of
mmc_pwrseq_simple_set_gpios_value() incorrectly uses the 'value'
argument as a bitmap of GPIO values and assigns it directly to the
'values' bitmap variable passed to gpiod_set_array_value_cansleep()
instead of filling that bitmap with bits equal to the 'value' argument.
As a result, boot hanging caused by incorrectly handled MMC device
has been observed.

As a side effect of that incorrect interpreation of the 'value'
argument, wrong assumption is taken about the 'values' bitmap size 
never exceding the number of bits of the 'value' argument type.

Fix it.

Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
Hi,

I hope I've finally identified the root cause of the boot hang reported
by Marek Szyprowski.  I've reviewed the code of other divers updated for
the modified GPIO API and found no more issues of that kind.

Marek, can you please test this fix on top of next-20180920 with the fix
"gpiolib: Fix missing updates of bitmap index" also applied?

I've assumed this fix, if tested successfully, will be merged via GPIO
tree, hence I've selected Linus as the main recipient of this message.

Thanks,
Janusz

 drivers/mmc/core/pwrseq_simple.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 7f882a2bb872..ece34c734693 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -40,13 +40,22 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
 	struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
 
 	if (!IS_ERR(reset_gpios)) {
-		DECLARE_BITMAP(values, BITS_PER_TYPE(value));
+		unsigned long *values;
 		int nvalues = reset_gpios->ndescs;
 
-		values[0] = value;
+		values = bitmap_alloc(nvalues, GFP_KERNEL);
+		if (!values)
+			return;
+
+		if (value)
+			bitmap_fill(values, nvalues);
+		else
+			bitmap_zero(values, nvalues);
 
 		gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
 					       reset_gpios->info, values);
+
+		kfree(values);
 	}
 }
 
-- 
2.16.4

Re: [PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Linus Walleij]

On Tue, Sep 25, 2018 at 9:23 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
> get/set array") changed the way GPIO values are passed to
> gpiod_get/set_array_value() and friends.  The updated code of
> mmc_pwrseq_simple_set_gpios_value() incorrectly uses the 'value'
> argument as a bitmap of GPIO values and assigns it directly to the
> 'values' bitmap variable passed to gpiod_set_array_value_cansleep()
> instead of filling that bitmap with bits equal to the 'value' argument.
> As a result, boot hanging caused by incorrectly handled MMC device
> has been observed.
>
> As a side effect of that incorrect interpreation of the 'value'
> argument, wrong assumption is taken about the 'values' bitmap size
> never exceding the number of bits of the 'value' argument type.
>
> Fix it.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Nice!

Provided this works, Ulf can I have your ACK so I can queue this
with the rest of the gpio array rework in the GPIO tree?

Yours,
Linus Walleij

Re: [PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Marek Szyprowski]

Hi Janusz,

On 2018-09-25 21:24, Janusz Krzysztofik wrote:
> Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
> get/set array") changed the way GPIO values are passed to
> gpiod_get/set_array_value() and friends.  The updated code of
> mmc_pwrseq_simple_set_gpios_value() incorrectly uses the 'value'
> argument as a bitmap of GPIO values and assigns it directly to the
> 'values' bitmap variable passed to gpiod_set_array_value_cansleep()
> instead of filling that bitmap with bits equal to the 'value' argument.
> As a result, boot hanging caused by incorrectly handled MMC device
> has been observed.
>
> As a side effect of that incorrect interpreation of the 'value'
> argument, wrong assumption is taken about the 'values' bitmap size
> never exceding the number of bits of the 'value' argument type.
>
> Fix it.
>
> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
> Hi,
>
> I hope I've finally identified the root cause of the boot hang reported
> by Marek Szyprowski.  I've reviewed the code of other divers updated for
> the modified GPIO API and found no more issues of that kind.
>
> Marek, can you please test this fix on top of next-20180920 with the fix
> "gpiolib: Fix missing updates of bitmap index" also applied?

Yes, I've just did such test (next-20180920 + "gpiolib: Fix missing
updates of bitmap index" + "mmc: pwrseq_simple: Fix incorrect handling
of GPIO bitmap") and sadly it doesn't fix the boot hang.

With some more debugs in mmc_pwrseq_simple I've noticed that
gpiod_set_array_value_cansleep() never ends and busyloops somewhere.
I'm checking this now.

> I've assumed this fix, if tested successfully, will be merged via GPIO
> tree, hence I've selected Linus as the main recipient of this message.
>
> Thanks,
> Janusz
>
>   drivers/mmc/core/pwrseq_simple.c | 13 +++++++++++--
>   1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
> index 7f882a2bb872..ece34c734693 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -40,13 +40,22 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
>   	struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
>   
>   	if (!IS_ERR(reset_gpios)) {
> -		DECLARE_BITMAP(values, BITS_PER_TYPE(value));
> +		unsigned long *values;
>   		int nvalues = reset_gpios->ndescs;
>   
> -		values[0] = value;
> +		values = bitmap_alloc(nvalues, GFP_KERNEL);
> +		if (!values)
> +			return;
> +
> +		if (value)
> +			bitmap_fill(values, nvalues);
> +		else
> +			bitmap_zero(values, nvalues);
>   
>   		gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
>   					       reset_gpios->info, values);
> +
> +		kfree(values);
>   	}
>   }
>   

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Marek Szyprowski]

Hi again,

On 2018-09-26 10:14, Marek Szyprowski wrote:
> On 2018-09-25 21:24, Janusz Krzysztofik wrote:
>> Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
>> get/set array") changed the way GPIO values are passed to
>> gpiod_get/set_array_value() and friends.  The updated code of
>> mmc_pwrseq_simple_set_gpios_value() incorrectly uses the 'value'
>> argument as a bitmap of GPIO values and assigns it directly to the
>> 'values' bitmap variable passed to gpiod_set_array_value_cansleep()
>> instead of filling that bitmap with bits equal to the 'value' argument.
>> As a result, boot hanging caused by incorrectly handled MMC device
>> has been observed.
>>
>> As a side effect of that incorrect interpreation of the 'value'
>> argument, wrong assumption is taken about the 'values' bitmap size
>> never exceding the number of bits of the 'value' argument type.
>>
>> Fix it.
>>
>> Reported-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
>> ---
>> Hi,
>>
>> I hope I've finally identified the root cause of the boot hang reported
>> by Marek Szyprowski.  I've reviewed the code of other divers updated for
>> the modified GPIO API and found no more issues of that kind.
>>
>> Marek, can you please test this fix on top of next-20180920 with the fix
>> "gpiolib: Fix missing updates of bitmap index" also applied?
>
> Yes, I've just did such test (next-20180920 + "gpiolib: Fix missing
> updates of bitmap index" + "mmc: pwrseq_simple: Fix incorrect handling
> of GPIO bitmap") and sadly it doesn't fix the boot hang.
>
> With some more debugs in mmc_pwrseq_simple I've noticed that
> gpiod_set_array_value_cansleep() never ends and busyloops somewhere.
> I'm checking this now.

I busyloops inside the internal do { } while loop (lines 3163-3201) in
gpiod_set_array_value_complex(). 'i' is never incremented.

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

[RFT PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Janusz Krzysztofik]

Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
get/set array") changed the way GPIO values are passed to
gpiod_get/set_array_value() and friends.  The new code introduced into
mmc_pwrseq_simple_set_gpios_value() incorrectly interpretes the 'value'
argument as a bitmap of GPIO values and assigns it directly to the
'values' bitmap variable passed to gpiod_set_array_value_cansleep()
instead of filling that bitmap with bits equal to the 'value' argument.
As a result, only member 0 of the array is handled correctly.

Moreover, wrong assumption is taken about the 'values' bitmap size not
exceding the number of bits of the 'value' argument type.

Fix it.

Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
---
Hi,

I think that patch has been missed while we were resolving issues 
related to GPIO fast bitmap processing.  Since all issues other than the 
one addreessed by this patch have been been hopefully identified and 
fixed, GPIO tree seems now to be in good shape in regard to that. 
However, I believe pwrseq_simple is still broken.  Hence, I'm 
resubmitting this patch to Ulf for inclusion in MMC tree, Cc: many other 
people who are kindly requested to test it if possible.

I've identified the following DT files representing devices which may be 
affected (have more than one GPIO assigned to pwrseq_simple):
- arch/arm/boot/dts/imx6qdl-sr-som-brcm.dtsi
- arch/arm/boot/dts/exynos5250-snow-common.dtsi
- arch/arm/boot/dts/imx6sl-warp.dts
- arch/arm/boot/dts/omap3-igep0030.dts
- arch/arm/boot/dts/omap3-igep0020.dts
- arch/arm/boot/dts/rk3036-kylin.dts
- arch/arm64/boot/dts/rockchip/rk3368-r88.dts
- arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi

Please start with checking if pwrseq_simple from linux-next works for 
you and if not, please test if this patch fixes the issue.

Thanks,
Janusz


 drivers/mmc/core/pwrseq_simple.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
index 7f882a2bb872..ece34c734693 100644
--- a/drivers/mmc/core/pwrseq_simple.c
+++ b/drivers/mmc/core/pwrseq_simple.c
@@ -40,13 +40,22 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
 	struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
 
 	if (!IS_ERR(reset_gpios)) {
-		DECLARE_BITMAP(values, BITS_PER_TYPE(value));
+		unsigned long *values;
 		int nvalues = reset_gpios->ndescs;
 
-		values[0] = value;
+		values = bitmap_alloc(nvalues, GFP_KERNEL);
+		if (!values)
+			return;
+
+		if (value)
+			bitmap_fill(values, nvalues);
+		else
+			bitmap_zero(values, nvalues);
 
 		gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
 					       reset_gpios->info, values);
+
+		kfree(values);
 	}
 }
 
-- 
2.16.4

Re: [RFT PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Marek Szyprowski]

Hi Janusz,

On 2018-10-12 21:09, Janusz Krzysztofik wrote:
> Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
> get/set array") changed the way GPIO values are passed to
> gpiod_get/set_array_value() and friends.  The new code introduced into
> mmc_pwrseq_simple_set_gpios_value() incorrectly interpretes the 'value'
> argument as a bitmap of GPIO values and assigns it directly to the
> 'values' bitmap variable passed to gpiod_set_array_value_cansleep()
> instead of filling that bitmap with bits equal to the 'value' argument.
> As a result, only member 0 of the array is handled correctly.
>
> Moreover, wrong assumption is taken about the 'values' bitmap size not
> exceding the number of bits of the 'value' argument type.
>
> Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>
> ---
> Hi,
>
> I think that patch has been missed while we were resolving issues 
> related to GPIO fast bitmap processing.  Since all issues other than the 
> one addreessed by this patch have been been hopefully identified and 
> fixed, GPIO tree seems now to be in good shape in regard to that. 
> However, I believe pwrseq_simple is still broken.  Hence, I'm 
> resubmitting this patch to Ulf for inclusion in MMC tree, Cc: many other 
> people who are kindly requested to test it if possible.
>
> I've identified the following DT files representing devices which may be 
> affected (have more than one GPIO assigned to pwrseq_simple):
> - arch/arm/boot/dts/imx6qdl-sr-som-brcm.dtsi
> - arch/arm/boot/dts/exynos5250-snow-common.dtsi

On Samsung Snow Chromebook it doesn't change anything. Board boots and
detects WiFi SDIO card before and after applying it on top on Linux
next-20181012.

Tested-by: Marek Szyprowski <m.szyprowski

> - arch/arm/boot/dts/imx6sl-warp.dts
> - arch/arm/boot/dts/omap3-igep0030.dts
> - arch/arm/boot/dts/omap3-igep0020.dts
> - arch/arm/boot/dts/rk3036-kylin.dts
> - arch/arm64/boot/dts/rockchip/rk3368-r88.dts
> - arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
>
> Please start with checking if pwrseq_simple from linux-next works for 
> you and if not, please test if this patch fixes the issue.
>
> Thanks,
> Janusz
>
>
>  drivers/mmc/core/pwrseq_simple.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
> index 7f882a2bb872..ece34c734693 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -40,13 +40,22 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
>  	struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
>  
>  	if (!IS_ERR(reset_gpios)) {
> -		DECLARE_BITMAP(values, BITS_PER_TYPE(value));
> +		unsigned long *values;
>  		int nvalues = reset_gpios->ndescs;
>  
> -		values[0] = value;
> +		values = bitmap_alloc(nvalues, GFP_KERNEL);
> +		if (!values)
> +			return;
> +
> +		if (value)
> +			bitmap_fill(values, nvalues);
> +		else
> +			bitmap_zero(values, nvalues);
>  
>  		gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
>  					       reset_gpios->info, values);
> +
> +		kfree(values);
>  	}
>  }
>  

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [RFT PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Ulf Hansson]

On 12 October 2018 at 21:09, Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:
> Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
> get/set array") changed the way GPIO values are passed to
> gpiod_get/set_array_value() and friends.  The new code introduced into
> mmc_pwrseq_simple_set_gpios_value() incorrectly interpretes the 'value'
> argument as a bitmap of GPIO values and assigns it directly to the
> 'values' bitmap variable passed to gpiod_set_array_value_cansleep()
> instead of filling that bitmap with bits equal to the 'value' argument.
> As a result, only member 0 of the array is handled correctly.
>
> Moreover, wrong assumption is taken about the 'values' bitmap size not
> exceding the number of bits of the 'value' argument type.
>
> Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Acked-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

> ---
> Hi,
>
> I think that patch has been missed while we were resolving issues
> related to GPIO fast bitmap processing.  Since all issues other than the
> one addreessed by this patch have been been hopefully identified and
> fixed, GPIO tree seems now to be in good shape in regard to that.
> However, I believe pwrseq_simple is still broken.  Hence, I'm
> resubmitting this patch to Ulf for inclusion in MMC tree, Cc: many other
> people who are kindly requested to test it if possible.
>
> I've identified the following DT files representing devices which may be
> affected (have more than one GPIO assigned to pwrseq_simple):
> - arch/arm/boot/dts/imx6qdl-sr-som-brcm.dtsi
> - arch/arm/boot/dts/exynos5250-snow-common.dtsi
> - arch/arm/boot/dts/imx6sl-warp.dts
> - arch/arm/boot/dts/omap3-igep0030.dts
> - arch/arm/boot/dts/omap3-igep0020.dts
> - arch/arm/boot/dts/rk3036-kylin.dts
> - arch/arm64/boot/dts/rockchip/rk3368-r88.dts
> - arch/arm64/boot/dts/amlogic/meson-gxbb-vega-s95.dtsi
>
> Please start with checking if pwrseq_simple from linux-next works for
> you and if not, please test if this patch fixes the issue.
>
> Thanks,
> Janusz
>
>
>  drivers/mmc/core/pwrseq_simple.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/mmc/core/pwrseq_simple.c b/drivers/mmc/core/pwrseq_simple.c
> index 7f882a2bb872..ece34c734693 100644
> --- a/drivers/mmc/core/pwrseq_simple.c
> +++ b/drivers/mmc/core/pwrseq_simple.c
> @@ -40,13 +40,22 @@ static void mmc_pwrseq_simple_set_gpios_value(struct mmc_pwrseq_simple *pwrseq,
>         struct gpio_descs *reset_gpios = pwrseq->reset_gpios;
>
>         if (!IS_ERR(reset_gpios)) {
> -               DECLARE_BITMAP(values, BITS_PER_TYPE(value));
> +               unsigned long *values;
>                 int nvalues = reset_gpios->ndescs;
>
> -               values[0] = value;
> +               values = bitmap_alloc(nvalues, GFP_KERNEL);
> +               if (!values)
> +                       return;
> +
> +               if (value)
> +                       bitmap_fill(values, nvalues);
> +               else
> +                       bitmap_zero(values, nvalues);
>
>                 gpiod_set_array_value_cansleep(nvalues, reset_gpios->desc,
>                                                reset_gpios->info, values);
> +
> +               kfree(values);
>         }
>  }
>
> --
> 2.16.4
>

Re: [RFT PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap

[Linus Walleij]

On Fri, Oct 12, 2018 at 9:07 PM Janusz Krzysztofik <jmkrzyszt@gmail.com> wrote:

> Commit b9762bebc633 ("gpiolib: Pass bitmaps, not integer arrays, to
> get/set array") changed the way GPIO values are passed to
> gpiod_get/set_array_value() and friends.  The new code introduced into
> mmc_pwrseq_simple_set_gpios_value() incorrectly interpretes the 'value'
> argument as a bitmap of GPIO values and assigns it directly to the
> 'values' bitmap variable passed to gpiod_set_array_value_cansleep()
> instead of filling that bitmap with bits equal to the 'value' argument.
> As a result, only member 0 of the array is handled correctly.
>
> Moreover, wrong assumption is taken about the 'values' bitmap size not
> exceding the number of bits of the 'value' argument type.
>
> Fix it.
>
> Signed-off-by: Janusz Krzysztofik <jmkrzyszt@gmail.com>

Patch applied with Ulf's and Marek's tags!

Yours,
Linus Walleij

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Dan Carpenter]

On Thu, Sep 20, 2018 at 05:48:22PM +0200, Janusz Krzysztofik wrote:
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index a53d17745d21..5bc3447949c9 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -2880,7 +2880,7 @@ int gpiod_get_array_value_complex(bool raw, bool can_sleep,
>  			__set_bit(hwgpio, mask);
>  
>  			if (array_info)
> -				find_next_zero_bit(array_info->get_mask,
> +				i = find_next_zero_bit(array_info->get_mask,
>  						   array_size, i);

We could mark find_next_zero_bit() and friends as a __must_check
functions so we avoid this bug in the future.  I have a more complicated
idea how to detect these bugs in a generic way using Smatch but it will
take longer to implement.

regards,
dan carpenter

Re: [PATCH v7 4/4] gpiolib: Implement fast processing path in get/set array

[Linus Walleij]

On Thu, Sep 20, 2018 at 3:11 AM Marek Szyprowski
<m.szyprowski@samsung.com> wrote:

> I've just noticed that this patch landed in today's linux-next. Sadly it
> breaks booting of Exynos5250-based Samsung Snow Chromebook (ARM 32bit,
> device-tree source arch/arm/boot/dts/exynos5250-snow.dts).

Thanks for testing on this platform!

> Booting hangs after detecting MMC cards. Reverting this patch fixes the
> boot. I will try later to add some debugs and investigate it further what
> really happens when booting hangs.

How typical. I hope we can fix it, because this should mean speedups
for your platform.

Yours,
Linus Walleij