| Message ID | 20180925192433.1377-1-jmkrzyszt@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap | expand |
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
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
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
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); } }
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(-)