| Message ID | CACRpkdZOCJXHxcYv0CU76J0r2OpxdtDuOiJ9cenrvJCHVsBeAA@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Linus, On Mon, Jul 2, 2018 at 8:42 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > What about this: > > From facb3799f479bfd4dad99c25c9c5d4c77b14c9b0 Mon Sep 17 00:00:00 2001 > From: Linus Walleij <linus.walleij@linaro.org> > Date: Mon, 2 Jul 2018 13:35:01 +0200 > Subject: [PATCH] mmc: slot-gpio: Allocate GPIO labels dynamically > > The use of string pointers in the MMC slot GPIO context is > pretty dubious, allocating some 2*len extra bytes for each > label of the ro and wp pins. > > Tidy this up using kasprintf() with dynamic allocation of > labels for these strings. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Your proposed solution also worked fine, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Linus Anson Huang Best Regards! > -----Original Message----- > From: Linus Walleij [mailto:linus.walleij@linaro.org] > Sent: Monday, July 2, 2018 7:42 PM > To: Anson Huang <anson.huang@nxp.com> > Cc: Ulf Hansson <ulf.hansson@linaro.org>; Masahiro Yamada > <yamada.masahiro@socionext.com>; Adrian Hunter > <adrian.hunter@intel.com>; evgreen@chromium.org; Shawn Lin > <shawn.lin@rock-chips.com>; Fabio Estevam <fabio.estevam@nxp.com>; > linux-mmc <linux-mmc@vger.kernel.org>; linux-kernel@vger.kernel.org; > dl-linux-imx <linux-imx@nxp.com> > Subject: Re: [PATCH V2] mmc: core: cd_label must be last entry of mmc_gpio > struct > > On Mon, Jul 2, 2018 at 3:32 AM Anson Huang <Anson.Huang@nxp.com> > wrote: > > > commit bfd694d5e21c ("mmc: core: Add tunable delay before detecting > > card after card is inserted") adds > > "u32 cd_debounce_delay_ms" to the last of mmc_gpio struct and cause > > "char cd_label[0]" NOT work as string pointer of card detect label, > > when "cat /proc/interrupts", the devname for card detect gpio is > > incorrect as below: > > > > 144: 0 gpio-mxc 22 Edge ▒ > > 161: 0 gpio-mxc 7 Edge ▒ > > > > Move the cd_label field down to fix this, and drop the zero from the > > array size to prevent future similar bugs, the result is correct as > > below: > > > > 144: 0 gpio-mxc 22 Edge 2198000.mmc cd > > 161: 0 gpio-mxc 7 Edge 2190000.mmc cd > > > > Fixes: bfd694d5e21c ("mmc: core: Add tunable delay before detecting > > card after card is inserted") > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > Tested-by: Fabio Estevam <fabio.estevam@nxp.com> > > Curious. > > > --- a/drivers/mmc/core/slot-gpio.c > > +++ b/drivers/mmc/core/slot-gpio.c > > @@ -27,8 +27,8 @@ struct mmc_gpio { > > bool override_cd_active_level; > > irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id); > > char *ro_label; > > - char cd_label[0]; > > u32 cd_debounce_delay_ms; > > + char cd_label[]; > > Not that I am the smartest in the world when it comes to how the C compilers > work this out. > > But isn't that equivalent to: > char *cd_label; > ? I think C compiler will check structure if it has flexible array, the flexible array much be at end of a structure, and its size can be dynamic changed by mallocing a memory for this structure, there are many such case in kernel. If the flexible array is NOT at the end of a struct, error " error: flexible array member not at end of struct" will come out by C compiler. > > Look at this from drivers/mmc/core/slot-gpio.c: > > ctx->ro_label = ctx->cd_label + len; > ctx->cd_debounce_delay_ms = 200; > snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent)); > snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent)); > host->slot.handler_priv = ctx; > host->slot.cd_irq = -EINVAL; > > So now that ro_label is char * that points len chars into the struct and that is all > fine as the ctx is allocated like that: > > struct mmc_gpio *ctx = devm_kzalloc(host->parent, > sizeof(*ctx) + 2 * len, GFP_KERNEL); > > And I've seen this being done to allocate a state with some dynamic strings > after it. > > But I just don't like this, it seems fragile and now it broke. I think the difference of current implementation and using two string pointers are, current implementation can only alloc memory for mmc_gpio structure once, the ro_label's address can be got by cd_label's address. If using two string pointers for ro_label and cd_label, we have to alloc twice for them separately. > > What about this: > > From facb3799f479bfd4dad99c25c9c5d4c77b14c9b0 Mon Sep 17 00:00:00 > 2001 > From: Linus Walleij <linus.walleij@linaro.org> > Date: Mon, 2 Jul 2018 13:35:01 +0200 > Subject: [PATCH] mmc: slot-gpio: Allocate GPIO labels dynamically > > The use of string pointers in the MMC slot GPIO context is pretty dubious, > allocating some 2*len extra bytes for each label of the ro and wp pins. > > Tidy this up using kasprintf() with dynamic allocation of labels for these strings. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/core/slot-gpio.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c index > ef05e0039378..c417801b366e 100644 > --- a/drivers/mmc/core/slot-gpio.c > +++ b/drivers/mmc/core/slot-gpio.c > @@ -27,7 +27,7 @@ struct mmc_gpio { > bool override_cd_active_level; > irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id); > char *ro_label; > - char cd_label[0]; > + char *cd_label; > u32 cd_debounce_delay_ms; > }; > > @@ -47,13 +47,18 @@ int mmc_gpio_alloc(struct mmc_host *host) { > size_t len = strlen(dev_name(host->parent)) + 4; len is no need any more. > struct mmc_gpio *ctx = devm_kzalloc(host->parent, > - sizeof(*ctx) + 2 * len, GFP_KERNEL); > + sizeof(*ctx), GFP_KERNEL); > > if (ctx) { > - ctx->ro_label = ctx->cd_label + len; > ctx->cd_debounce_delay_ms = 200; > - snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent)); > - snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent)); > + ctx->cd_label = devm_kasprintf(host->parent, GFP_KERNEL, > + "%s cd", dev_name(host->parent)); > + if (!ctx->cd_label) > + return -ENOMEM; > + ctx->ro_label = devm_kasprintf(host->parent, GFP_KERNEL, > + "%s ro", dev_name(host->parent)); > + if (!ctx->ro_label) > + return -ENOMEM; > host->slot.handler_priv = ctx; > host->slot.cd_irq = -EINVAL; > } > -- > 2.17.0 > > I can send it as a separate patch if you like it. I think either way is OK, since flexible array is used in kernel code quite commonly, so I prefer to make code change as small as possible, the original patch can also prevent similar bug in future. And like below commit Fabio pointed out, it also has same kind of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks. Anson. > > Yours, > Linus Walleij
On Mon, Jul 2, 2018 at 11:13 PM, Anson Huang <anson.huang@nxp.com> wrote: > I think either way is OK, since flexible array is used in kernel code quite commonly, > so I prefer to make code change as small as possible, the original patch can also prevent > similar bug in future. And like below commit Fabio pointed out, it also has same kind > of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks. I am also fine with this patch or the one from Linus. Maybe Anson's patch could be applied to 4.18-rc as a bug fix and Linus' patch could be applied to 4.19-rc1 as a cleanup/improvement? Ulf, What would you prefer? Thanks -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 3 July 2018 at 04:25, Fabio Estevam <festevam@gmail.com> wrote: > On Mon, Jul 2, 2018 at 11:13 PM, Anson Huang <anson.huang@nxp.com> wrote: > >> I think either way is OK, since flexible array is used in kernel code quite commonly, >> so I prefer to make code change as small as possible, the original patch can also prevent >> similar bug in future. And like below commit Fabio pointed out, it also has same kind >> of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks. > > I am also fine with this patch or the one from Linus. > > Maybe Anson's patch could be applied to 4.18-rc as a bug fix and > Linus' patch could be applied to 4.19-rc1 as a cleanup/improvement? Good idea! I have queued up Anson's v2 patch as a fix for 4.18. Linus, can please re-post a re-based version of your change on top!? I guess you can add Anson's and Fabio's reviewed by tags for it as well. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 2, 2018 at 2:42 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Mon, Jul 2, 2018 at 3:32 AM Anson Huang <Anson.Huang@nxp.com> wrote: >> - char cd_label[0]; >> + char cd_label[]; > > Not that I am the smartest in the world when it comes to how the > C compilers work this out. > > But isn't that equivalent to: > char *cd_label; > ? No. sizeof(foo) == bar in the original code and with your change it would be sizeof(foo) == bar + sizeof(void *) On top of that, the actual memory is going in one chunk and requires only one allocation. But, I like your below stuff, though comments > From facb3799f479bfd4dad99c25c9c5d4c77b14c9b0 Mon Sep 17 00:00:00 2001 > From: Linus Walleij <linus.walleij@linaro.org> > Date: Mon, 2 Jul 2018 13:35:01 +0200 > Subject: [PATCH] mmc: slot-gpio: Allocate GPIO labels dynamically > > The use of string pointers in the MMC slot GPIO context is > pretty dubious, allocating some 2*len extra bytes for each > label of the ro and wp pins. > > Tidy this up using kasprintf() with dynamic allocation of > labels for these strings. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > --- > drivers/mmc/core/slot-gpio.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c > index ef05e0039378..c417801b366e 100644 > --- a/drivers/mmc/core/slot-gpio.c > +++ b/drivers/mmc/core/slot-gpio.c > @@ -27,7 +27,7 @@ struct mmc_gpio { > bool override_cd_active_level; > irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id); > char *ro_label; > - char cd_label[0]; > + char *cd_label; > u32 cd_debounce_delay_ms; > }; > > @@ -47,13 +47,18 @@ int mmc_gpio_alloc(struct mmc_host *host) > { > size_t len = strlen(dev_name(host->parent)) + 4; Do you need this still? > struct mmc_gpio *ctx = devm_kzalloc(host->parent, > - sizeof(*ctx) + 2 * len, GFP_KERNEL); > + sizeof(*ctx), GFP_KERNEL); > > if (ctx) { > - ctx->ro_label = ctx->cd_label + len; > ctx->cd_debounce_delay_ms = 200; > - snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent)); > - snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent)); > + ctx->cd_label = devm_kasprintf(host->parent, GFP_KERNEL, > + "%s cd", dev_name(host->parent)); > + if (!ctx->cd_label) > + return -ENOMEM; > + ctx->ro_label = devm_kasprintf(host->parent, GFP_KERNEL, > + "%s ro", dev_name(host->parent)); > + if (!ctx->ro_label) > + return -ENOMEM; > host->slot.handler_priv = ctx; > host->slot.cd_irq = -EINVAL; > }
On Tue, Jul 3, 2018 at 7:35 AM Ulf Hansson <ulf.hansson@linaro.org> wrote: > On 3 July 2018 at 04:25, Fabio Estevam <festevam@gmail.com> wrote: > > On Mon, Jul 2, 2018 at 11:13 PM, Anson Huang <anson.huang@nxp.com> wrote: > > > >> I think either way is OK, since flexible array is used in kernel code quite commonly, > >> so I prefer to make code change as small as possible, the original patch can also prevent > >> similar bug in future. And like below commit Fabio pointed out, it also has same kind > >> of fix: a158531f3c92 ("gpio: 74x164: Fix crash during .remove()"). Thanks. > > > > I am also fine with this patch or the one from Linus. > > > > Maybe Anson's patch could be applied to 4.18-rc as a bug fix and > > Linus' patch could be applied to 4.19-rc1 as a cleanup/improvement? > > Good idea! > > I have queued up Anson's v2 patch as a fix for 4.18. > > Linus, can please re-post a re-based version of your change on top!? I > guess you can add Anson's and Fabio's reviewed by tags for it as well. Sure thing! I'll fold in Andy's suggested fix as well. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/core/slot-gpio.c b/drivers/mmc/core/slot-gpio.c index ef05e0039378..c417801b366e 100644 --- a/drivers/mmc/core/slot-gpio.c +++ b/drivers/mmc/core/slot-gpio.c @@ -27,7 +27,7 @@ struct mmc_gpio { bool override_cd_active_level; irqreturn_t (*cd_gpio_isr)(int irq, void *dev_id); char *ro_label; - char cd_label[0]; + char *cd_label; u32 cd_debounce_delay_ms; }; @@ -47,13 +47,18 @@ int mmc_gpio_alloc(struct mmc_host *host) { size_t len = strlen(dev_name(host->parent)) + 4; struct mmc_gpio *ctx = devm_kzalloc(host->parent, - sizeof(*ctx) + 2 * len, GFP_KERNEL); + sizeof(*ctx), GFP_KERNEL); if (ctx) { - ctx->ro_label = ctx->cd_label + len; ctx->cd_debounce_delay_ms = 200; - snprintf(ctx->cd_label, len, "%s cd", dev_name(host->parent)); - snprintf(ctx->ro_label, len, "%s ro", dev_name(host->parent)); + ctx->cd_label = devm_kasprintf(host->parent, GFP_KERNEL, + "%s cd", dev_name(host->parent)); + if (!ctx->cd_label) + return -ENOMEM; + ctx->ro_label = devm_kasprintf(host->parent, GFP_KERNEL, + "%s ro", dev_name(host->parent)); + if (!ctx->ro_label) + return -ENOMEM; host->slot.handler_priv = ctx; host->slot.cd_irq = -EINVAL; }