linux-kernel - RE: [PATCH] mmc: core: improve rationality of bus width setting for HS400es

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 10 Jul 2018 10:44:26 +0000
From:   Fang Hongjie(方洪杰) 
        <hongjiefang@...micro.com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
CC:     Adrian Hunter <adrian.hunter@...el.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        "Bastian Stender" <bst@...gutronix.de>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Chanho Min <chanho.min@....com>,
        "Kyle Roeschley" <kyle.roeschley@...com>,
        "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] mmc: core: improve rationality of bus width setting for
 HS400es


> On 10 July 2018 at 07:04, Fang Hongjie(方洪杰) <hongjiefang@...micro.com>
> wrote:
> >> On 9 July 2018 at 08:47, Hongjie Fang <hongjiefang@...micro.com> wrote:
> >> > mmc_select_hs400es() calls mmc_select_bus_width() which will try to
> >> > set 4bit transfer mode if fail to set 8bit mode. The problem is that
> >> > the bus width should not be set to 4bit in HS400es mode.
> >>
> >> I guess it fails because there is something wrong. Can you please
> >> elaborate under what circumstance the problem occurs?
> >>
> > This problem very occasionally occurred when system resume from
> > suspend state. The card stuck in the programming state after setting
> > 8bit mode. After that, mmc_select_bus_width() will continue to set
> > 4bit mode and return without errors. Of course, the following R/W
> > requests will be all failing.
> 
> I see. So it's the mmc_switch() operation that fails, when trying 8-bit. Hmm.
> 
> >
> > The problem scene is hard to reproduce. Maybe it happened
> > because there is something wrong about bus clock or core voltage
> > stability when system do resume.
> 
> Yeah, that needs further debugging.
> 
> Don't know what host driver you are using, but potentially you could
> play with a couple host specific mechanism. The ->host->card_busy()
> callback, host->max_busy_timeout and MMC_CAP_WAIT_WHILE_BUSY.
> Depending on these, __mmc_switch() changes its policy when polling for
> busy. Have a look at __mmc_switch() and mmc_poll_for_busy(), to see
> what goes on.
> 
Thanks.
I will do the further debugging according to your advice.

> >
> >> >
> >> > Signed-off-by: Hongjie Fang <hongjiefang@...micro.com>
> >> > ---
> >> >  drivers/mmc/core/mmc.c | 23 +++++++++++++++++++++--
> >> >  1 file changed, 21 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/mmc/core/mmc.c b/drivers/mmc/core/mmc.c
> >> > index 4466f5d..94c3cc5 100644
> >> > --- a/drivers/mmc/core/mmc.c
> >> > +++ b/drivers/mmc/core/mmc.c
> >> > @@ -1337,9 +1337,28 @@ static int mmc_select_hs400es(struct mmc_card
> *card)
> >> >         if (err)
> >> >                 goto out_err;
> >> >
> >> > -       err = mmc_select_bus_width(card);
> >> > -       if (err < 0)
> >> > +       /* Switch card to 8 bit mode */
> >> > +       err = mmc_switch(card, EXT_CSD_CMD_SET_NORMAL,
> >> > +                        EXT_CSD_BUS_WIDTH,
> >> > +                        EXT_CSD_BUS_WIDTH_8,
> >> > +                        card->ext_csd.generic_cmd6_time);
> >> > +       if (err) {
> >> > +               pr_err("%s: switch to 8 bit mode for hs400es failed, err:%d\n",
> >> > +                       mmc_hostname(host), err);
> >> > +               goto out_err;
> >> > +       }
> >> > +
> >> > +       mmc_set_bus_width(host, MMC_BUS_WIDTH_8);
> >> > +
> >> > +       if (!(host->caps & MMC_CAP_BUS_WIDTH_TEST))
> >> > +               err = mmc_compare_ext_csds(card, MMC_BUS_WIDTH_8);
> >> > +       else
> >> > +               err = mmc_bus_test(card, MMC_BUS_WIDTH_8);
> >> > +       if (err) {
> >> > +               pr_err("%s: test 8 bit mode for hs400es failed, err:%d\n",
> >> > +                       mmc_hostname(host), err);
> >> >                 goto out_err;
> >>
> >> Lot's open coding, no I don't like it.
> >>
> >> Wouldn't it instead be possible to check the return value from
> >> mmc_select_bus_width(), as it should tell what width it manage to
> >> select. Then if it isn't 8-bit, then we should bail out. Does that
> >> work?
> >>
> > After setting 4bit mode, it can't work because it cannot back to
> > HS200 mode like the HS400 mode.
> > For HS400es, if fail to set 8bit mode, maybe it is more reasonable
> > that return error directly.
> 
> Yeah. Care to send a patch for that?
OK. I will try to send a new patch that let mmc_select_bus_width()
return error directly when fail to set 8bit for HS400es.

> 
> [...]
> 
> Kind regards
> Uffe

B&R
Hongjie