RE: [PATCH] mmc: core: Apply trim broken quirk to R1J57L
-Original Message-
From: Bough Chen
Sent: Wednesday, January 27, 2021 3:36 PM
To: Ulf Hansson ; Wang, Xiaolei
; Fabio Estevam
Cc: Pali Rohár ; Lee Jones ;
linux-...@vger.kernel.org; Linux Kernel Mailing List
Subject: RE: [PATCH] mmc: core: Apply trim broken quirk to R1J57L
[Please note this e-mail is from an EXTERNAL e-mail address]
> -Original Message-
> From: Ulf Hansson [mailto:ulf.hans...@linaro.org]
> Sent: 2021年1月21日 21:26
> To: Wang, Xiaolei ; Fabio Estevam
> ; Bough Chen
> Cc: Pali Rohár ; Lee Jones ;
> linux-...@vger.kernel.org; Linux Kernel Mailing List
>
> Subject: Re: [PATCH] mmc: core: Apply trim broken quirk to R1J57L
>
> + Fabio, Haibo
>
> On Thu, 21 Jan 2021 at 10:54, Wang, Xiaolei
>
> wrote:
> >
> > Hi
> >
> > >Are you sure this is an eMMC problem and not a mmc host driver issue?
> >
> > >Can you elaborate more what happens?
> >
> > When I use the mkfs.f2fs tool to format the eMMC file system on the
> > imx8qxp board, because mkfs.f2fs will use secdiscard first, when
> > entering mmc_blk_issue_secdiscard_rq erase, once the parameters are
> passed into MMC_SECURE_TRIM1_ARG, this function will take a long time
> to return .
> > The program has not ended, has been in TASK_UNINTERRUPTIBLE state.
> >
> > I compared the mkfs.ext4 tool to format the file system. Because it
> > directly uses mmc_blk_issue_discard_rq, this is a normal formatting
> phenomenon.
> >
> > mmc_blk_issue_secdiscard_rq and mmc_blk_issue_discard_rq are just
> different commands and parameters sent by the host as a bus.
> > I did not see the description of trim in the data sheet. Could the
> > host driver
> cause this problem?
>
> Yes, it can - and we have had issues like these before. So before
> adding a card quirk, I think we need to make sure this isn't the case.
>
> When using MMC_SECURE_TRIM1_ARG, it's very likely that the request
> takes longer to complete.
>
> The mmc host is responsible for either dealing with busy detection
> with the help of its HW/controller - or if that can't be supported,
> the mmc core falls back to polling the card for busy with a CMD13.
>
> See mmc_do_erase() in /drivers/mmc/core/core.c
>
> >
> > Note:
> > The host driver I use is sdhci-esdhc-imx.c
>
> Alright, I have looped in Fabio and Haibo that knows this driver,
> let's see if they can help.
>
> Hi Ulf and Xiaolei,
> I use the mkfs.f2fs tool format a partition on one Sandisk eMMC chip on imx8
> platform, do not meet issue.
> I add some log, confirm mkfs.f2fs call the mmc_blk_issue_secdiscard_rq(), and
> the arg is MMC_SECURE_TRIM1_ARG.
> For the busy detection, imx usdhc has one register bit represent the dat0
> signal level. So for your mmc device, it should be the mmc device trim
> feature problem.
Hi UIf and haibo
I did the verification again and found that the kernel version I used before
was v5.4. The latest kernel does not have this problem.
This patch is not accurate. I will confirm later whether it is caused by the
missing patch of v5.4.
Thanks
Xiaolei
> Best Regards
> Haibo
> >
> > Thanks
> > Xiaolei
>
> Kind regards
> Uffe
>
> >
> > -Original Message-
> > From: Ulf Hansson
> > Sent: Wednesday, January 20, 2021 9:41 PM
> > To: Wang, Xiaolei
> > Cc: Pali Rohár ; Lee Jones ;
> > linux-...@vger.kernel.org; Linux Kernel Mailing List
> >
> > Subject: Re: [PATCH] mmc: core: Apply trim broken quirk to R1J57L
> >
> > [Please note this e-mail is from an EXTERNAL e-mail address]
> >
> > On Mon, 18 Jan 2021 at 05:27, Xiaolei Wang
> >
> wrote:
> > >
> > > R1J57L mmc chip hw capibility indicates that it supports trim
> > > function, but this function does not work properly, the SDIO bus
> > > does not respond, and the IO has been waiting so set quirks to
> > > skip trim
> >
> > Are you sure this is an eMMC problem and not a mmc host driver issue?
> >
> > Can you elaborate more what happens?
> >
> > Kind regards
> > Uffe
> >
> > >
> > > Signed-off-by: Xiaolei Wang
> > > ---
> > > drivers/mmc/core/quirks.h | 4
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> > > index d68e6e513a4f..63e02391c133 100644
> > > --- a/drivers/mmc/core/quirks.h
> > > +++ b/drivers/mmc/core/quirks.h
> > > @@ -89,6 +89,8 @@ static const struct mmc_fixup __maybe_unused
> mmc_blk_fixups[] = {
> > >
RE: [PATCH] mmc: core: Apply trim broken quirk to R1J57L
Hi
>Are you sure this is an eMMC problem and not a mmc host driver issue?
>Can you elaborate more what happens?
When I use the mkfs.f2fs tool to format the eMMC file system on the imx8qxp
board,
because mkfs.f2fs will use secdiscard first, when entering
mmc_blk_issue_secdiscard_rq erase,
once the parameters are passed into MMC_SECURE_TRIM1_ARG, this function will
take a long time to return .
The program has not ended, has been in TASK_UNINTERRUPTIBLE state.
I compared the mkfs.ext4 tool to format the file system. Because it directly
uses mmc_blk_issue_discard_rq,
this is a normal formatting phenomenon.
mmc_blk_issue_secdiscard_rq and mmc_blk_issue_discard_rq are just different
commands and parameters sent by the host as a bus.
I did not see the description of trim in the data sheet. Could the host driver
cause this problem?
Note:
The host driver I use is sdhci-esdhc-imx.c
Thanks
Xiaolei
-Original Message-
From: Ulf Hansson
Sent: Wednesday, January 20, 2021 9:41 PM
To: Wang, Xiaolei
Cc: Pali Rohár ; Lee Jones ;
linux-...@vger.kernel.org; Linux Kernel Mailing List
Subject: Re: [PATCH] mmc: core: Apply trim broken quirk to R1J57L
[Please note this e-mail is from an EXTERNAL e-mail address]
On Mon, 18 Jan 2021 at 05:27, Xiaolei Wang wrote:
>
> R1J57L mmc chip hw capibility indicates that it supports trim
> function, but this function does not work properly, the SDIO bus does
> not respond, and the IO has been waiting so set quirks to skip trim
Are you sure this is an eMMC problem and not a mmc host driver issue?
Can you elaborate more what happens?
Kind regards
Uffe
>
> Signed-off-by: Xiaolei Wang
> ---
> drivers/mmc/core/quirks.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/mmc/core/quirks.h b/drivers/mmc/core/quirks.h
> index d68e6e513a4f..63e02391c133 100644
> --- a/drivers/mmc/core/quirks.h
> +++ b/drivers/mmc/core/quirks.h
> @@ -89,6 +89,8 @@ static const struct mmc_fixup __maybe_unused
> mmc_blk_fixups[] = {
> MMC_QUIRK_SEC_ERASE_TRIM_BROKEN),
> MMC_FIXUP("VZL00M", CID_MANFID_SAMSUNG, CID_OEMID_ANY, add_quirk_mmc,
> MMC_QUIRK_SEC_ERASE_TRIM_BROKEN),
> + MMC_FIXUP("R1J57L", CID_MANFID_MICRON, CID_OEMID_ANY, add_quirk_mmc,
> + MMC_QUIRK_SEC_ERASE_TRIM_BROKEN),
>
> /*
> * On Some Kingston eMMCs, performing trim can result in @@
> -98,6 +100,8 @@ static const struct mmc_fixup __maybe_unused mmc_blk_fixups[]
> = {
> MMC_QUIRK_TRIM_BROKEN),
> MMC_FIXUP("V10016", CID_MANFID_KINGSTON, CID_OEMID_ANY, add_quirk_mmc,
> MMC_QUIRK_TRIM_BROKEN),
> + MMC_FIXUP("R1J57L", CID_MANFID_MICRON, CID_OEMID_ANY, add_quirk_mmc,
> + MMC_QUIRK_TRIM_BROKEN),
>
> END_FIXUP
> };
> --
> 2.25.1
>
RE: [PATCH v3] regmap: debugfs: Fix a memory leak when calling regmap_attach_dev
Hi Markus > Thank you very much, very good suggestion, > Thanks for another positive feedback. > do I need to re-send a patch to fix this problem, or modify the previous > patch and send it again? > Please convince the involved contributors to integrate a corrected patch > version. Do you mean that I should correct the original patch and explain my changes and send it out? > * Better indentation. * … >> +if (!map->debugfs_name) >> +kfree(map->debugfs_name); > Would this questionable null pointer check result in a memory leak? if (!map->debugfs_name) kfree(map->debugfs_name); This null pointer check is not in the memory leak Thanks xiaolei -Original Message- From: Markus Elfring Sent: Wednesday, December 30, 2020 8:02 PM To: Wang, Xiaolei ; kernel-janit...@vger.kernel.org Cc: linux-kernel@vger.kernel.org; David Lechner ; Greg Kroah-Hartman ; Mark Brown ; Rafael J. Wysocki ; Julia Lawall Subject: Re: [PATCH v3] regmap: debugfs: Fix a memory leak when calling regmap_attach_dev > Thank you very much, very good suggestion, Thanks for another positive feedback. > do I need to re-send a patch to fix this problem, or modify the previous > patch and send it again? Please convince the involved contributors to integrate a corrected patch version. * Better indentation. * … >> +if (!map->debugfs_name) >> +kfree(map->debugfs_name); Would this questionable null pointer check result in a memory leak? Regards, Markus
RE: [PATCH v3] regmap: debugfs: Fix a memory leak when calling regmap_attach_dev
Hi Markus Thank you very much, very good suggestion, do I need to re-send a patch to fix this problem, or modify the previous patch and send it again? Thanks Xiaolei … > +++ b/drivers/base/regmap/regmap-debugfs.c > @@ -582,18 +582,25 @@ void regmap_debugfs_init(struct regmap *map) … > + map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s", > devname, name); > I suggest to reconsider the alignment for function call parameters for this > patch. … > + if (!map->debugfs_name) > + kfree(map->debugfs_name); > Such a null pointer check is redundant. > See also: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/ifnullfree.cocci?id=139711f033f636cc78b6aaf7363252241b9698ef#n2 -Original Message- From: Markus Elfring Sent: Wednesday, December 30, 2020 5:49 PM To: Wang, Xiaolei ; kernel-janit...@vger.kernel.org Cc: linux-kernel@vger.kernel.org; David Lechner ; Greg Kroah-Hartman ; Mark Brown ; Rafael J. Wysocki ; Julia Lawall Subject: Re: [PATCH v3] regmap: debugfs: Fix a memory leak when calling regmap_attach_dev … > +++ b/drivers/base/regmap/regmap-debugfs.c > @@ -582,18 +582,25 @@ void regmap_debugfs_init(struct regmap *map) … > + map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s", > devname, name); I suggest to reconsider the alignment for function call parameters for this patch. … > + if (!map->debugfs_name) > + kfree(map->debugfs_name); Such a null pointer check is redundant. See also: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/scripts/coccinelle/free/ifnullfree.cocci?id=139711f033f636cc78b6aaf7363252241b9698ef#n2 Regards, Markus
RE: regmap: debugfs: fix a memory leak when calling regmap_attach_dev
> Do you mean so?
>
> commit:
> 9b947a13e7f6 ("regmap: use debugfs even when no device")
> Please take another look at known information sources.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=f838f8d2b694cf9d524dc4423e9dd2db13892f3f#n89
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=f838f8d2b694cf9d524dc4423e9dd2db13892f3f#n121
> Which one do you mean by "another null point"?
How do you think about to check if calls of the function “kasprintf” succeeded
here?
https://elixir.bootlin.com/linux/v5.10.3/source/lib/kasprintf.c#L53
Will any more exception handling become relevant also for the implementation of
the function “regmap_debugfs_init”?
-Original Message-
From: Markus Elfring
Sent: Sunday, December 27, 2020 9:40 PM
To: Wang, Xiaolei ; kernel-janit...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org; Greg Kroah-Hartman
; Mark Brown ; Rafael J.
Wysocki
Subject: Re: regmap: debugfs: fix a memory leak when calling regmap_attach_dev
> Do you mean so?
>
> commit:
> 9b947a13e7f6 ("regmap: use debugfs even when no device")
> Please take another look at known information sources.
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=f838f8d2b694cf9d524dc4423e9dd2db13892f3f#n89
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=f838f8d2b694cf9d524dc4423e9dd2db13892f3f#n121
> Which one do you mean by "another null point"?
> How do you think about to check if calls of the function “kasprintf”
> succeeded here?
> https://elixir.bootlin.com/linux/v5.10.3/source/lib/kasprintf.c#L53
> Will any more exception handling become relevant also for the implementation
> of the function “regmap_debugfs_init”?
Thanks for your suggestion, I will modify the commit log and add another null
point check together with corresponding exception handling
Regards,
Xiaolei
RE: [PATCH] regmap: debugfs: fix a memory leak when calling regmap_attach_dev
Hi Markus
> This should be caused by
> commit <9b947a13e7f6> ("")
> I suggest to omit angle brackets from such information.
> * Will another imperative wording become helpful for the commit description?
> * How do you think about to add the tag “Fixes”?
Do you mean so?
commit:
9b947a13e7f6 ("regmap: use debugfs even when no device")
…
> +++ b/drivers/base/regmap/regmap-debugfs.c
…
> @@ -582,8 +582,10 @@ void regmap_debugfs_init(struct regmap *map)
> devname = dev_name(map->dev);
>
> if (name) {
> - map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s",
> + if (!map->debugfs_name) {
> + map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s",
> devname, name);
> + }
…
> I propose to add another null pointer check together with corresponding
> exception handling.
Which one do you mean by "another null point"? Do you mean that if the call
"regmap_debugfs_init" function is repeated, it returns directly?
Regards,
Xiaolei
-Original Message-
From: Markus Elfring
Sent: Saturday, December 26, 2020 10:16 PM
To: Wang, Xiaolei ; kernel-janit...@vger.kernel.org
Cc: linux-kernel@vger.kernel.org; Greg Kroah-Hartman
; Mark Brown ; Rafael J.
Wysocki
Subject: Re: [PATCH] regmap: debugfs: fix a memory leak when calling
regmap_attach_dev
> This should be caused by
> commit <9b947a13e7f6> ("")
I suggest to omit angle brackets from such information.
* Will another imperative wording become helpful for the commit description?
* How do you think about to add the tag “Fixes”?
…
> +++ b/drivers/base/regmap/regmap-debugfs.c
…
> @@ -582,8 +582,10 @@ void regmap_debugfs_init(struct regmap *map)
> devname = dev_name(map->dev);
>
> if (name) {
> - map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s",
> + if (!map->debugfs_name) {
> + map->debugfs_name = kasprintf(GFP_KERNEL, "%s-%s",
> devname, name);
> + }
…
I propose to add another null pointer check together with corresponding
exception handling.
Regards,
Markus
