| Message ID | 1550391872-6668-1-git-send-email-Lohengrin1024@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc:fix a bug when max_discard is 0 | expand |
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 >
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
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 --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;
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(-)