linux-kernel - RE: [PATCH 1/3] mmc: core: Calculate the discard arg only once

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, 5 Feb 2019 14:06:11 +0000
From:   Avri Altman <Avri.Altman@....com>
To:     Ulf Hansson <ulf.hansson@...aro.org>
CC:     "linux-mmc@...r.kernel.org" <linux-mmc@...r.kernel.org>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Jaehoon Chung <jh80.chung@...sung.com>,
        Shawn Lin <shawn.lin@...k-chips.com>,
        Avi Shchislowski <Avi.Shchislowski@....com>,
        Alex Lemberg <Alex.Lemberg@....com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH 1/3] mmc: core: Calculate the discard arg only once

> On Sun, 3 Feb 2019 at 09:51, Avri Altman <avri.altman@....com> wrote:
> >
> > The discard arg is a read-only ext_csd parameter -
> > set it once on card init.
> 
> I like the idea here. There is really no point checking this for every
> corresponding request, nice!
> 
> However, the "discard arg" isn't specific to eMMC, as it's also used
> for SD cards. So, the change log is a bit miss-leading, I think. Can
> you please clarify this.
Done.

> >
> > +       /* set discard_arg */
> > +       if (mmc_can_discard(card))
> > +               card->discard_arg = MMC_DISCARD_ARG;
> > +       else if (mmc_can_trim(card))
> > +               card->discard_arg = MMC_TRIM_ARG;
> > +       else
> > +               card->discard_arg = MMC_ERASE_ARG;
> 
> You are assigning card->discard_arg only for (e)MMC, while I think you
> should do that also for SD.
> 
> I guess it practice it doesn't matter, because MMC_ERASE_ARG is zero.
> Although, I would prefer to keep the code consistent, so I think you
> should assign card->discard_arg for for SD cards as well.
Done.

> > diff --git a/include/linux/mmc/card.h b/include/linux/mmc/card.h
> > index de73778..447648b 100644
> > --- a/include/linux/mmc/card.h
> > +++ b/include/linux/mmc/card.h
> > @@ -308,6 +308,8 @@ struct mmc_card {
> >         unsigned int    nr_parts;
> >
> >         unsigned int            bouncesz;       /* Bounce buffer size */
> > +
> > +       unsigned int            discard_arg;    /* discard args */
> 
> Please move this a couple of lines above, close to the other "erase"
> related variables.
Done.

> 
> You may even consider to rename the arg to "erase_arg", but I have no
> strong opinion changing to that.
Done.

> 
> >  };
> >
> >  static inline bool mmc_large_sector(struct mmc_card *card)
> > --
> > 1.9.1
> >
> 
> Kind regards
> Uffe