linux-kernel - Re: [PATCH V23 01/16] mmc: core: Support UHS-II card control and access

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPDyKFqrkzDTakhzZOsF2mBpko0XG_v5mKManhTQzj1XRJfxxg@mail.gmail.com>
Date: Thu, 24 Oct 2024 14:41:11 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Victor Shih <victorshihgli@...il.com>, Adrian Hunter <adrian.hunter@...el.com>
Cc: linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org, 
	benchuanggli@...il.com, Lucas.Lai@...esyslogic.com.tw, 
	HL.Liu@...esyslogic.com.tw, Greg.tu@...esyslogic.com.tw, dlunev@...omium.org, 
	Jason Lai <jason.lai@...esyslogic.com.tw>, 
	Victor Shih <victor.shih@...esyslogic.com.tw>
Subject: Re: [PATCH V23 01/16] mmc: core: Support UHS-II card control and access

[...]

> > +
> >  static int sd_uhs2_power_up(struct mmc_host *host)
> >  {
> >       int err;
> > @@ -42,12 +70,46 @@ static int sd_uhs2_power_off(struct mmc_host *host)
> >
> >       host->ios.vdd = 0;
> >       host->ios.clock = 0;
> > +     /* Must set UHS2 timing to identify UHS2 mode */
>
> That comment is stale, but...
>
> >       host->ios.timing = MMC_TIMING_LEGACY;
>
> MMC_TIMING_UHS2_SPEED_A is also checked by mmc_card_uhs2()
> which is used a lot.
>
> It might be that ios.timing should be MMC_TIMING_UHS2_SPEED_A
> until after calling host->ops->uhs2_control() ?

I agree, that was also how the code looked before my previous comment for v22.

Victor, sorry for the confusion, I simply didn't realize that
mmc_card_uhs2() was using MMC_TIMING_UHS2_SPEED_A.

It looks like we should set MMC_TIMING_LEGACY, *after* calling
host->ops->uhs2_control(). Just to make sure we have the correct
initial state after powering-off.

I will fix this when applying, see more comments about that below.

>
> >       host->ios.power_mode = MMC_POWER_OFF;
> > +     host->uhs2_sd_tran = false;
> >
> >       return host->ops->uhs2_control(host, UHS2_SET_IOS);
> >  }

[...]

> > +static int sd_uhs2_init_card(struct mmc_host *host, struct mmc_card *oldcard)
> >  {
> >       struct mmc_card *card;
> >       u32 node_id = 0;
> > @@ -131,33 +818,211 @@ static int sd_uhs2_init_card(struct mmc_host *host)
> >       if (err)
> >               return err;
> >
> > -     card = mmc_alloc_card(host, &sd_type);
> > -     if (IS_ERR(card))
> > -             return PTR_ERR(card);
> > +     if (oldcard) {
> > +             card = oldcard;
> > +     } else {
> > +             card = mmc_alloc_card(host, &sd_type);
> > +             if (IS_ERR(card))
> > +                     return PTR_ERR(card);
> > +     }
> >
> >       card->uhs2_config.node_id = node_id;
> >       card->type = MMC_TYPE_SD;
> >
> >       err = sd_uhs2_config_read(host, card);
> >       if (err)
> > -             goto err;
> > +             return err;
>
> This leaks card if !oldcard.  Still need:
>
> err:
>         if (!oldcard)
>                 mmc_remove_card(card);
>
>
> >
> >       err = sd_uhs2_config_write(host, card);
> >       if (err)
> > -             goto err;
> > +             return err;
>
> Ditto
>
> >
> >       host->card = card;
> > +     /* If change speed to Range B, need to GO_DORMANT_STATE */
> > +     if (host->ios.timing == MMC_TIMING_UHS2_SPEED_B ||
> > +         host->ios.timing == MMC_TIMING_UHS2_SPEED_B_HD) {
> > +             err = sd_uhs2_go_dormant_state(host, node_id);
> > +             if (err)
> > +                     return err;
>
> And here host->card is assigned but we have failed.
> The other card types do not set host->card until
> just before return 0.  Then this can be goto err
> instead of return err.

In regards to the error path and how to handle the cleanup of the
mmc_card, indeed things are still not correct.

However, I realize these parts are somewhat a bit tricky to get right.
Therefore, I am simply going to leave this as is when applying and I
am going to send patches that fixes things on top.

In this way we can move things forward and avoid additional new
versions of the complete series to be sent.

> > +     }
> > +
> > +     host->uhs2_sd_tran = true;
> > +
> > +     return 0;
> > +}
> > +
>

[...]

Kind regards
Uffe