lkml.org 
[lkml]   [2018]   [Sep]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] mmc: pwrseq_simple: Fix incorrect handling of GPIO bitmap
    From
    Date
    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

    \
     
     \ /
      Last update: 2018-09-26 10:15    [W:4.135 / U:0.044 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site