diff mbox series

mmc:fix a bug when max_discard is 0

Message ID 1550391872-6668-1-git-send-email-Lohengrin1024@gmail.com
State New
Headers show
Series mmc:fix a bug when max_discard is 0 | expand

Commit Message

Jiong Wu Feb. 17, 2019, 8:24 a.m. UTC
The original purpose of the code I fix is to choose max_trim as the final
parameter instead of max_discard if max_trim is less than max_discard. But
there is a uncommon case that max_discard is 0 because max_discard is
overflow the maximum threshold. In this case, we should still choose
max_trim instead of max_discard, however, in the original code(if(max_trim
 < max_discard)), the conditio is false when max_discard is 0. So I fix it
by add "|| max_discard == 0" to cover this case.

Signed-off-by: Jiong Wu <Lohengrin1024@gmail.com>
---
 drivers/mmc/core/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ulf Hansson Feb. 26, 2019, 8:18 a.m. UTC | #1
On Sun, 17 Feb 2019 at 09:26, Jiong Wu <lohengrin1024@gmail.com> wrote:
>
> The original purpose of the code I fix is to choose max_trim as the final
> parameter instead of max_discard if max_trim is less than max_discard. But
> there is a uncommon case that max_discard is 0 because max_discard is
> overflow the maximum threshold. In this case, we should still choose
> max_trim instead of max_discard, however, in the original code(if(max_trim
>  < max_discard)), the conditio is false when max_discard is 0. So I fix it
> by add "|| max_discard == 0" to cover this case.
>
> Signed-off-by: Jiong Wu <Lohengrin1024@gmail.com>

I am guessing this is more of a hypothetical problem rather than a
real one, thus I am not queuing this a fix. If people want this for
stable, they can always send a backport.

So, applied for next, thanks!

Kind regards
Uffe


> ---
>  drivers/mmc/core/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> index 5bd58b9..7b1bd95 100644
> --- a/drivers/mmc/core/core.c
> +++ b/drivers/mmc/core/core.c
> @@ -2383,7 +2383,7 @@ unsigned int mmc_calc_max_discard(struct mmc_card *card)
>         max_discard = mmc_do_calc_max_discard(card, MMC_ERASE_ARG);
>         if (max_discard && mmc_can_trim(card)) {
>                 max_trim = mmc_do_calc_max_discard(card, MMC_TRIM_ARG);
> -               if (max_trim < max_discard)
> +               if (max_trim < max_discard || max_discard == 0)
>                         max_discard = max_trim;
>         } else if (max_discard < card->erase_size) {
>                 max_discard = 0;
> --
> 2.7.4
>
Ulf Hansson Feb. 28, 2019, 8:18 a.m. UTC | #2
On Thu, 28 Feb 2019 at 02:41, Jiong wu <lohengrin1024@gmail.com> wrote:
>
> In fact, it happened.
>
> For instance, a type of Toshiba 128G emmc(THGBMHT0C8LBAIG) has ext_csd that ERASE_TIMEOUT_MULT is 0x73, and TRIM_MULT
>  is 0x01. So the card->ext_csd.hc_erase_timeout is 0x73 * 300ms(34.5s), which is larger than host->max_busy_timeout(30s) of kirin710. Consequently, it came out a condition that max_discard is calculated as 0.
>
> I suggest you to accept this fix into mainstream, firstly, it's exactly a logic mistake and can really be encountered. secondly, the change is small and clear, the function and correctness has been verified.

Thanks for clarifying! However, by looking at the code once more, I
suspect your analyze is in-correct. Please have a look at the below
comment.

>
> Thank you.
>
> Jiong Wu
>
>
> -------- 原始邮件 --------
> 主题:Re: [PATCH] mmc:fix a bug when max_discard is 0
> 发件人:Ulf Hansson
> 收件人:Jiong Wu
> 抄送:Shawn Lin ,Adrian Hunter ,Avri Altman ,Wolfram Sang ,Sergio Valverde ,Martin Hicks ,linux-mmc@vger.kernel.org
>
> On Sun, 17 Feb 2019 at 09:26, Jiong Wu <lohengrin1024@gmail.com> wrote:
> >
> > The original purpose of the code I fix is to choose max_trim as the final
> > parameter instead of max_discard if max_trim is less than max_discard. But
> > there is a uncommon case that max_discard is 0 because max_discard is
> > overflow the maximum threshold. In this case, we should still choose
> > max_trim instead of max_discard, however, in the original code(if(max_trim
> >  < max_discard)), the conditio is false when max_discard is 0. So I fix it
> > by add "|| max_discard == 0" to cover this case.
> >
> > Signed-off-by: Jiong Wu <Lohengrin1024@gmail.com>
>
> I am guessing this is more of a hypothetical problem rather than a
> real one, thus I am not queuing this a fix. If people want this for
> stable, they can always send a backport.
>
> So, applied for next, thanks!
>
> Kind regards
> Uffe
>
>
> > ---
> >  drivers/mmc/core/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > index 5bd58b9..7b1bd95 100644
> > --- a/drivers/mmc/core/core.c
> > +++ b/drivers/mmc/core/core.c
> > @@ -2383,7 +2383,7 @@ unsigned int mmc_calc_max_discard(struct mmc_card *card)
> >         max_discard = mmc_do_calc_max_discard(card, MMC_ERASE_ARG);
> >         if (max_discard && mmc_can_trim(card)) {
> >                 max_trim = mmc_do_calc_max_discard(card, MMC_TRIM_ARG);
> > -               if (max_trim < max_discard)
> > +               if (max_trim < max_discard || max_discard == 0)

We have already checked that max_discard has a non-zero value, a few
lines above, so I think this change make no sense and has no impact.

> >                         max_discard = max_trim;
> >         } else if (max_discard < card->erase_size) {
> >                 max_discard = 0;
> > --
> > 2.7.4
> >

So, I am dropping the patch until we have sorted this out. It seems
like there is a different problem.

Kind regards
Uffe
Jiong Wu Feb. 28, 2019, 2:59 p.m. UTC | #3
On Thu, Feb 28, 2019 at 4:18 PM Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Thu, 28 Feb 2019 at 02:41, Jiong wu <lohengrin1024@gmail.com> wrote:
> >
> > In fact, it happened.
> >
> > For instance, a type of Toshiba 128G emmc(THGBMHT0C8LBAIG) has ext_csd that ERASE_TIMEOUT_MULT is 0x73, and TRIM_MULT
> >  is 0x01. So the card->ext_csd.hc_erase_timeout is 0x73 * 300ms(34.5s), which is larger than host->max_busy_timeout(30s) of kirin710. Consequently, it came out a condition that max_discard is calculated as 0.
> >
> > I suggest you to accept this fix into mainstream, firstly, it's exactly a logic mistake and can really be encountered. secondly, the change is small and clear, the function and correctness has been verified.
>
> Thanks for clarifying! However, by looking at the code once more, I
> suspect your analyze is in-correct. Please have a look at the below
> comment.
>
> >
> > Thank you.
> >
> > Jiong Wu
> >
> >
> > -------- 原始邮件 --------
> > 主题:Re: [PATCH] mmc:fix a bug when max_discard is 0
> > 发件人:Ulf Hansson
> > 收件人:Jiong Wu
> > 抄送:Shawn Lin ,Adrian Hunter ,Avri Altman ,Wolfram Sang ,Sergio Valverde ,Martin Hicks ,linux-mmc@vger.kernel.org
> >
> > On Sun, 17 Feb 2019 at 09:26, Jiong Wu <lohengrin1024@gmail.com> wrote:
> > >
> > > The original purpose of the code I fix is to choose max_trim as the final
> > > parameter instead of max_discard if max_trim is less than max_discard. But
> > > there is a uncommon case that max_discard is 0 because max_discard is
> > > overflow the maximum threshold. In this case, we should still choose
> > > max_trim instead of max_discard, however, in the original code(if(max_trim
> > >  < max_discard)), the conditio is false when max_discard is 0. So I fix it
> > > by add "|| max_discard == 0" to cover this case.
> > >
> > > Signed-off-by: Jiong Wu <Lohengrin1024@gmail.com>
> >
> > I am guessing this is more of a hypothetical problem rather than a
> > real one, thus I am not queuing this a fix. If people want this for
> > stable, they can always send a backport.
> >
> > So, applied for next, thanks!
> >
> > Kind regards
> > Uffe
> >
> >
> > > ---
> > >  drivers/mmc/core/core.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
> > > index 5bd58b9..7b1bd95 100644
> > > --- a/drivers/mmc/core/core.c
> > > +++ b/drivers/mmc/core/core.c
> > > @@ -2383,7 +2383,7 @@ unsigned int mmc_calc_max_discard(struct mmc_card *card)
> > >         max_discard = mmc_do_calc_max_discard(card, MMC_ERASE_ARG);
> > >         if (max_discard && mmc_can_trim(card)) {
> > >                 max_trim = mmc_do_calc_max_discard(card, MMC_TRIM_ARG);
> > > -               if (max_trim < max_discard)
> > > +               if (max_trim < max_discard || max_discard == 0)
>
> We have already checked that max_discard has a non-zero value, a few
> lines above, so I think this change make no sense and has no impact.

yes, you are right, I didn't notice that, some one update another
patch to fix the problem,
as follow: https://patchwork.kernel.org/patch/10207571/

 - if (mmc_can_trim(card)) {
+ if (max_discard && mmc_can_trim(card)) {

However, I think it's not a perfect solution, it just checks the value
of max_discard, and
refuse a max_discard of value 0 to participate in the process which
could replace
max_discard with max_trim. It's an omission. As I mentioned in last
email, sometimes,
the max_discard is overflowed and becomes 0, when ERASE_TIMEOUT_MULT is too
large. At this time, we should use max_trim to replace max_discard,
rather than just
skip this process.

I'll submit another patch with removing the above non-zero check of
max_discard, if you
agree with me. Just like this,
- if (max_discard && mmc_can_trim(card)) {
+if (mmc_can_trim(card)) {
      max_trim = mmc_do_calc_max_discard(card, MMC_TRIM_ARG);
-      if (max_trim < max_discard)
+     if (max_trim < max_discard || max_discard == 0)
            max_discard = max_trim;

>
> > >                         max_discard = max_trim;
> > >         } else if (max_discard < card->erase_size) {
> > >                 max_discard = 0;
> > > --
> > > 2.7.4
> > >
>
> So, I am dropping the patch until we have sorted this out. It seems
> like there is a different problem.
>
> Kind regards
> Uffe
diff mbox series

Patch

diff --git a/drivers/mmc/core/core.c b/drivers/mmc/core/core.c
index 5bd58b9..7b1bd95 100644
--- a/drivers/mmc/core/core.c
+++ b/drivers/mmc/core/core.c
@@ -2383,7 +2383,7 @@  unsigned int mmc_calc_max_discard(struct mmc_card *card)
 	max_discard = mmc_do_calc_max_discard(card, MMC_ERASE_ARG);
 	if (max_discard && mmc_can_trim(card)) {
 		max_trim = mmc_do_calc_max_discard(card, MMC_TRIM_ARG);
-		if (max_trim < max_discard)
+		if (max_trim < max_discard || max_discard == 0)
 			max_discard = max_trim;
 	} else if (max_discard < card->erase_size) {
 		max_discard = 0;