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

[Avri Altman]

> On Sun, 3 Feb 2019 at 09:51, Avri Altman <avri.altman@xxxxxxx> 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