| Message ID | 20180115134101.GA6711@lenoch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ladislav, On 15/01/18 15:41, Ladislav Michl wrote: > Marcus, > > On Mon, Jan 15, 2018 at 02:15:11PM +0100, SF Markus Elfring wrote: >> Omit extra messages for a memory allocation failure in this function. >> >> This issue was detected by using the Coccinelle software. >> >> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net> >> --- >> drivers/mfd/omap-usb-tll.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c >> index 44a5d66314c6..7945efa0152e 100644 >> --- a/drivers/mfd/omap-usb-tll.c >> +++ b/drivers/mfd/omap-usb-tll.c >> @@ -222,10 +222,8 @@ static int usbtll_omap_probe(struct platform_device *pdev) >> dev_dbg(dev, "starting TI HSUSB TLL Controller\n"); >> >> tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); >> - if (!tll) { >> - dev_err(dev, "Memory allocation failed\n"); >> + if (!tll) >> return -ENOMEM; >> - } >> >> res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> tll->base = devm_ioremap_resource(dev, res); >> @@ -258,7 +256,6 @@ static int usbtll_omap_probe(struct platform_device *pdev) >> GFP_KERNEL); >> if (!tll->ch_clk) { >> ret = -ENOMEM; >> - dev_err(dev, "Couldn't allocate memory for channel clocks\n"); > > I'd either leave this one, just to know which allocation failed or better use > something like this (it is pseudo patch only, just to show idea): > > diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c > index 44a5d66314c6..d217211d6b8f 100644 > --- a/drivers/mfd/omap-usb-tll.c > +++ b/drivers/mfd/omap-usb-tll.c > @@ -108,9 +108,9 @@ > (x) != OMAP_EHCI_PORT_MODE_PHY) > > struct usbtll_omap { > - int nch; /* num. of channels */ > - struct clk **ch_clk; > void __iomem *base; > + int nch; /* num. of channels */ > + struct clk ch_clk[0]; How about putting a comment here that says ch_clk needs to be the last member of this structure? > }; > > /*-------------------------------------------------------------------------*/ > @@ -221,18 +221,11 @@ static int usbtll_omap_probe(struct platform_device *pdev) > > dev_dbg(dev, "starting TI HSUSB TLL Controller\n"); > > - tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); > - if (!tll) { > - dev_err(dev, "Memory allocation failed\n"); > - return -ENOMEM; > - } > - > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - tll->base = devm_ioremap_resource(dev, res); > - if (IS_ERR(tll->base)) > - return PTR_ERR(tll->base); > + base = devm_ioremap_resource(dev, res); > + if (IS_ERR(base)) > + return PTR_ERR(base); > > - platform_set_drvdata(pdev, tll); > pm_runtime_enable(dev); > pm_runtime_get_sync(dev); > > @@ -240,27 +233,27 @@ static int usbtll_omap_probe(struct platform_device *pdev) > switch (ver) { > case OMAP_USBTLL_REV1: > case OMAP_USBTLL_REV4: > - tll->nch = OMAP_TLL_CHANNEL_COUNT; > + num = OMAP_TLL_CHANNEL_COUNT; need to declare num. Maybe better call it nch instead? > break; > case OMAP_USBTLL_REV2: > case OMAP_USBTLL_REV3: > - tll->nch = OMAP_REV2_TLL_CHANNEL_COUNT; > + num = OMAP_REV2_TLL_CHANNEL_COUNT; > break; > default: > - tll->nch = OMAP_TLL_CHANNEL_COUNT; > + num = OMAP_TLL_CHANNEL_COUNT; > dev_dbg(dev, > "USB TLL Rev : 0x%x not recognized, assuming %d channels\n", > - ver, tll->nch); > + ver, num); > break; > } > > - tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch, > - GFP_KERNEL); > - if (!tll->ch_clk) { > - ret = -ENOMEM; > - dev_err(dev, "Couldn't allocate memory for channel clocks\n"); > - goto err_clk_alloc; > - } > + tll = devm_kzalloc(dev, sizeof(struct usbtll_omap) + (num * sizeof(...)), GFP_KERNEL); num * sizeof(tll->ch_clk[0]) ? > + if (!tll) > + return -ENOMEM; > + > + tll->nch = num; > + tll->base = base; > + platform_set_drvdata(pdev, tll); > > for (i = 0; i < tll->nch; i++) { > char clkname[] = "usb_tll_hs_usb_chx_clk"; > >> goto err_clk_alloc; >> } > > What do you think? I'll prepare proper patch in case there's an agreement > on above approach. I think it is a good approach. > > Best regards, > ladis > -- > To unsubscribe from this list: send the line "unsubscribe linux-omap" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
>> @@ -258,7 +256,6 @@ static int usbtll_omap_probe(struct platform_device *pdev) >> GFP_KERNEL); >> if (!tll->ch_clk) { >> ret = -ENOMEM; >> - dev_err(dev, "Couldn't allocate memory for channel clocks\n"); > > I'd either leave this one, just to know which allocation failed or better use > something like this … Are you aware on the structure for a Linux allocation failure report? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcus, On Mon, Jan 15, 2018 at 04:38:43PM +0100, SF Markus Elfring wrote: > >> @@ -258,7 +256,6 @@ static int usbtll_omap_probe(struct platform_device *pdev) > >> GFP_KERNEL); > >> if (!tll->ch_clk) { > >> ret = -ENOMEM; > >> - dev_err(dev, "Couldn't allocate memory for channel clocks\n"); > > > > I'd either leave this one, just to know which allocation failed or better use > > something like this … > > Are you aware on the structure for a Linux allocation failure report? Just created one (not OMAP and not this driver, but that does not matter now): ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at mm/slab_common.c:1012 kmalloc_slab+0x38/0xdc Modules linked in: CPU: 0 PID: 1 Comm: swapper Not tainted 4.15.0-rc7-next-20180115 #25 Hardware name: Atmel AT91SAM9 [<c0107950>] (unwind_backtrace) from [<c010565c>] (show_stack+0x10/0x14) [<c010565c>] (show_stack) from [<c010f6cc>] (__warn+0xcc/0xe4) [<c010f6cc>] (__warn) from [<c010f7a4>] (warn_slowpath_null+0x38/0x44) [<c010f7a4>] (warn_slowpath_null) from [<c018ef90>] (kmalloc_slab+0x38/0xdc) [<c018ef90>] (kmalloc_slab) from [<c01a5494>] (__kmalloc_track_caller+0xc/0xb0) [<c01a5494>] (__kmalloc_track_caller) from [<c02fe5a8>] (devm_kmalloc+0x1c/0x58) [<c02fe5a8>] (devm_kmalloc) from [<c03f96ec>] (max9867_i2c_probe+0x1c/0xe0) [<c03f96ec>] (max9867_i2c_probe) from [<c038a708>] (i2c_device_probe+0x270/0x298) [<c038a708>] (i2c_device_probe) from [<c02fb37c>] (driver_probe_device+0x2b4/0x458) [<c02fb37c>] (driver_probe_device) from [<c02fb59c>] (__driver_attach+0x7c/0xec) [<c02fb59c>] (__driver_attach) from [<c02f9840>] (bus_for_each_dev+0x58/0x7c) [<c02f9840>] (bus_for_each_dev) from [<c02fa7cc>] (bus_add_driver+0x1a8/0x220) [<c02fa7cc>] (bus_add_driver) from [<c02fbe7c>] (driver_register+0xa0/0xe0) [<c02fbe7c>] (driver_register) from [<c038aa6c>] (i2c_register_driver+0x74/0xa0) [<c038aa6c>] (i2c_register_driver) from [<c0102570>] (do_one_initcall+0x134/0x15c) [<c0102570>] (do_one_initcall) from [<c0700da8>] (kernel_init_freeable+0x178/0x1b4) [<c0700da8>] (kernel_init_freeable) from [<c050122c>] (kernel_init+0x8/0x100) [<c050122c>] (kernel_init) from [<c01010e0>] (ret_from_fork+0x14/0x34) Exception stack(0xc381bfb0 to 0xc381bff8) bfa0: 00000000 00000000 00000000 00000000 bfc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 bfe0: 00000000 00000000 00000000 00000000 00000013 00000000 ---[ end trace 3c79eadf2363e939 ]--- max9867: probe of 1-0018 failed with error -12 driver was instructed to alloc insane number of bytes using devm_kzalloc in max9867_i2c_probe. Now, if probe function calls devm_kzalloc two times and one of them fails, you cannot easily say which one without looking at assembly listing. Or did I misunderstand your question? Best regards, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>> @@ -258,7 +256,6 @@ static int usbtll_omap_probe(struct platform_device *pdev) >>>> GFP_KERNEL); >>>> if (!tll->ch_clk) { >>>> ret = -ENOMEM; >>>> - dev_err(dev, "Couldn't allocate memory for channel clocks\n"); >>> >>> I'd either leave this one, just to know which allocation failed or better use >>> something like this … >> >> Are you aware on the structure for a Linux allocation failure report? > > Just created one (not OMAP and not this driver, but that does not matter now): Thanks for your example. > ---[ end trace 3c79eadf2363e939 ]--- > max9867: probe of 1-0018 failed with error -12 > > driver was instructed to alloc insane number of bytes using devm_kzalloc in > max9867_i2c_probe. > Now, if probe function calls devm_kzalloc two times and one of them fails, > you cannot easily say which one without looking at assembly listing. Will this situation change with any other implementation for such backtraces? > Or did I misunderstand your question? No. - It seems that we have found a “common wavelength”. Would it become acceptable to move the mentioned memory allocation into an additional function implementation so that you could see a difference from the function call stack dump already? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 15, 2018 at 05:21:47PM +0100, SF Markus Elfring wrote: > >>>> @@ -258,7 +256,6 @@ static int usbtll_omap_probe(struct platform_device *pdev) > >>>> GFP_KERNEL); > >>>> if (!tll->ch_clk) { > >>>> ret = -ENOMEM; > >>>> - dev_err(dev, "Couldn't allocate memory for channel clocks\n"); > >>> > >>> I'd either leave this one, just to know which allocation failed or better use > >>> something like this … > >> > >> Are you aware on the structure for a Linux allocation failure report? > > > > Just created one (not OMAP and not this driver, but that does not matter now): > > Thanks for your example. > > > ---[ end trace 3c79eadf2363e939 ]--- > > max9867: probe of 1-0018 failed with error -12 > > > > driver was instructed to alloc insane number of bytes using devm_kzalloc in > > max9867_i2c_probe. > > Now, if probe function calls devm_kzalloc two times and one of them fails, > > you cannot easily say which one without looking at assembly listing. > > Will this situation change with any other implementation for such backtraces? How much that situation changes depends mainly on that very person who is sending bugreport and his/her ability and willigness to eventually change said implementation. In the other words your question (presumably) expects a world of ideal backtraces, which is (so far) rarely the case. Anyway, if we agree to change the way we allocate driver data here, the issue this debate is about will no longer exist. Best reards, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> Now, if probe function calls devm_kzalloc two times and one of them fails, >>> you cannot easily say which one without looking at assembly listing. >> >> Will this situation change with any other implementation for such backtraces? > > How much that situation changes depends mainly on that very person who is > sending bugreport and his/her ability and willigness to eventually change > said implementation. Have you got any more influence on the selection? Which variant was applied for your example? > In the other words your question (presumably) expects a world of > ideal backtraces, which is (so far) rarely the case. I assume that further software evolution will matter. Does an article like “The ORCs are coming” (by Jonathan Corbet from 2017-07-20) point information out which is also useful for this issue here? https://lwn.net/Articles/728339/ > Anyway, if we agree to change the way we allocate driver data here, > the issue this debate is about will no longer exist. Does your update suggestion contain still any additional error messages for memory allocation failures? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 15, 2018 at 06:06:20PM +0100, SF Markus Elfring wrote: > >>> Now, if probe function calls devm_kzalloc two times and one of them fails, > >>> you cannot easily say which one without looking at assembly listing. > >> > >> Will this situation change with any other implementation for such backtraces? > > > > How much that situation changes depends mainly on that very person who is > > sending bugreport and his/her ability and willigness to eventually change > > said implementation. > > Have you got any more influence on the selection? ? > Which variant was applied for your example? ARM_UNWIND > > In the other words your question (presumably) expects a world of > > ideal backtraces, which is (so far) rarely the case. > > I assume that further software evolution will matter. > > Does an article like “The ORCs are coming” (by Jonathan Corbet from 2017-07-20) > point information out which is also useful for this issue here? > > https://lwn.net/Articles/728339/ I'm aware of this article. Please bear in mind which driver you are modifying. Not everything is desktop or server machine with almost unlimited resources and embedded people are rarely using latest stuff with all that bells and whistles. That said, you might end having only fragment of log in only one of thousands machines in field. And saying technician he needs to use another kernel (upgrade all machines) and wait another several weeks for bug to trigger is no way. So until evolution reaches ARM land, my point stands unchanged: Make it single allocation or leave one of those two messages in place. In practice it probably does not matter if we know which allocation failed. We simply run out of memmory. > > Anyway, if we agree to change the way we allocate driver data here, > > the issue this debate is about will no longer exist. > > Does your update suggestion contain still any additional error messages for > memory allocation failures? Of course not as there will be only one memory allocation in the probe function. Best regards, ladis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> That said, you might end having only fragment of log in only one of thousands > And saying technician he needs to use another kernel (upgrade all machines) > and wait another several weeks for bug to trigger is no way. This was not really my intention here. > So until evolution reaches ARM land, my point stands unchanged: > Make it single allocation Did I indicate a similar design direction (for the preferred stack trace convenience) after your constructive feedback? > or leave one of those two messages in place. Are there any more preferences involved? > In practice it probably does not matter if we know which allocation > failed. We simply run out of memmory. Would anybody insist to know for which driver instance an allocation attempt was performed? >> Does your update suggestion contain still any additional error messages for >> memory allocation failures? > > Of course not as there will be only one memory allocation in the probe function. Thanks for this clarification. - So I hope that your solution approach will be also fine. Will it supersede my proposal? Regards, Markus -- To unsubscribe from this list: send the line "unsubscribe linux-omap" 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/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c index 44a5d66314c6..d217211d6b8f 100644 --- a/drivers/mfd/omap-usb-tll.c +++ b/drivers/mfd/omap-usb-tll.c @@ -108,9 +108,9 @@ (x) != OMAP_EHCI_PORT_MODE_PHY) struct usbtll_omap { - int nch; /* num. of channels */ - struct clk **ch_clk; void __iomem *base; + int nch; /* num. of channels */ + struct clk ch_clk[0]; }; /*-------------------------------------------------------------------------*/ @@ -221,18 +221,11 @@ static int usbtll_omap_probe(struct platform_device *pdev) dev_dbg(dev, "starting TI HSUSB TLL Controller\n"); - tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL); - if (!tll) { - dev_err(dev, "Memory allocation failed\n"); - return -ENOMEM; - } - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - tll->base = devm_ioremap_resource(dev, res); - if (IS_ERR(tll->base)) - return PTR_ERR(tll->base); + base = devm_ioremap_resource(dev, res); + if (IS_ERR(base)) + return PTR_ERR(base); - platform_set_drvdata(pdev, tll); pm_runtime_enable(dev); pm_runtime_get_sync(dev); @@ -240,27 +233,27 @@ static int usbtll_omap_probe(struct platform_device *pdev) switch (ver) { case OMAP_USBTLL_REV1: case OMAP_USBTLL_REV4: - tll->nch = OMAP_TLL_CHANNEL_COUNT; + num = OMAP_TLL_CHANNEL_COUNT; break; case OMAP_USBTLL_REV2: case OMAP_USBTLL_REV3: - tll->nch = OMAP_REV2_TLL_CHANNEL_COUNT; + num = OMAP_REV2_TLL_CHANNEL_COUNT; break; default: - tll->nch = OMAP_TLL_CHANNEL_COUNT; + num = OMAP_TLL_CHANNEL_COUNT; dev_dbg(dev, "USB TLL Rev : 0x%x not recognized, assuming %d channels\n", - ver, tll->nch); + ver, num); break; } - tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch, - GFP_KERNEL); - if (!tll->ch_clk) { - ret = -ENOMEM; - dev_err(dev, "Couldn't allocate memory for channel clocks\n"); - goto err_clk_alloc; - } + tll = devm_kzalloc(dev, sizeof(struct usbtll_omap) + (num * sizeof(...)), GFP_KERNEL); + if (!tll) + return -ENOMEM; + + tll->nch = num; + tll->base = base; + platform_set_drvdata(pdev, tll); for (i = 0; i < tll->nch; i++) { char clkname[] = "usb_tll_hs_usb_chx_clk";