> 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
