linux-kernel - Re: [PATCH] mmc: core: Initial support for SD express card/host

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: <CAPDyKFr6eSZr2V-=YvAEZH9SmsE2SZ9j5ZS5zZEFHBqwyWRfpw@mail.gmail.com>
Date:   Thu, 16 Jul 2020 19:04:21 +0200
From:   Ulf Hansson <ulf.hansson@...aro.org>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Christoph Hellwig <hch@....de>, Arnd Bergmann <arnd@...db.de>,
        Rui Feng <rui_feng@...lsil.com.cn>,
        linux-nvme@...ts.infradead.org,
        Linux PCI <linux-pci@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Adrian Hunter <adrian.hunter@...el.com>
Subject: Re: [PATCH] mmc: core: Initial support for SD express card/host

On Thu, 16 Jul 2020 at 18:14, Greg Kroah-Hartman
<gregkh@...uxfoundation.org> wrote:
>
> On Thu, Jul 16, 2020 at 04:15:34PM +0200, Ulf Hansson wrote:
> > +int mmc_send_if_cond_pcie(struct mmc_host *host, u32 ocr)
> > +{
> > +     u32 resp = 0;
> > +     u8 pcie_bits = 0;
> > +     int ret;
> > +
> > +     if (host->caps2 & MMC_CAP2_SD_EXP) {
> > +             /* Probe card for SD express support via PCIe. */
> > +             pcie_bits = 0x10;
> > +             if (host->caps2 & MMC_CAP2_SD_EXP_1_2V)
> > +                     /* Probe also for 1.2V support. */
> > +                     pcie_bits = 0x30;
> > +     }
> > +
> > +     ret = __mmc_send_if_cond(host, ocr, pcie_bits, &resp);
> > +     if (ret)
> > +             return 0;
> > +
> > +     /* Continue with the SD express init, if the card supports it. */
> > +     resp &= 0x3000;
> > +     if (pcie_bits && resp) {
> > +             if (resp == 0x3000)
>
> 0x3000 should be some defined value, right?  Otherwise it just looks
> like magic bits :)

Yeah, I was considering that, but there are already lots of magic bits
around here in this code. On top of that, the bits are shifted,
depending on how they are used.

We should probably look into doing a cleanup, so this gets clearer overall.

>
> > --- a/include/linux/mmc/host.h
> > +++ b/include/linux/mmc/host.h
> > @@ -60,6 +60,8 @@ struct mmc_ios {
> >  #define MMC_TIMING_MMC_DDR52 8
> >  #define MMC_TIMING_MMC_HS200 9
> >  #define MMC_TIMING_MMC_HS400 10
> > +#define MMC_TIMING_SD_EXP    11
> > +#define MMC_TIMING_SD_EXP_1_2V       12
> >
> >       unsigned char   signal_voltage;         /* signalling voltage (1.8V or 3.3V) */
> >
> > @@ -172,6 +174,9 @@ struct mmc_host_ops {
> >        */
> >       int     (*multi_io_quirk)(struct mmc_card *card,
> >                                 unsigned int direction, int blk_size);
> > +
> > +     /* Initialize an SD express card, mandatory for MMC_CAP2_SD_EXP. */
> > +     int     (*init_sd_express)(struct mmc_host *host, struct mmc_ios *ios);
> >  };
> >
> >  struct mmc_cqe_ops {
> > @@ -357,6 +362,8 @@ struct mmc_host {
> >  #define MMC_CAP2_HS200_1_2V_SDR      (1 << 6)        /* can support */
> >  #define MMC_CAP2_HS200               (MMC_CAP2_HS200_1_8V_SDR | \
> >                                MMC_CAP2_HS200_1_2V_SDR)
> > +#define MMC_CAP2_SD_EXP              (1 << 7)        /* SD express via PCIe */
>
> BIT(7)?
>
> > +#define MMC_CAP2_SD_EXP_1_2V (1 << 8)        /* SD express 1.2V */
>
> BIT(8)?

I can change to that, but it wouldn't be consistent with existing
code. Again, probably better targeted as a separate bigger cleanup on
top.

>
> thanks,
>
> greg k-h

Thanks for reviewing!

Kind regards
Uffe