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

[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

[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

[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
>

[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