From: Markus Elfring <[email protected]>
Date: Mon, 15 Jan 2018 14:09:28 +0100
Three update suggestions were taken into account
from static source code analysis.
Markus Elfring (3):
Delete two error messages for a failed memory allocation
Improve a size determination
Return an error code only as a constant
drivers/mfd/omap-usb-tll.c | 20 +++++---------------
1 file changed, 5 insertions(+), 15 deletions(-)
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 15 Jan 2018 13:39:25 +0100
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 <[email protected]>
---
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");
goto err_clk_alloc;
}
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 15 Jan 2018 13:43:53 +0100
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/mfd/omap-usb-tll.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
index 7945efa0152e..c8b4ad40910a 100644
--- a/drivers/mfd/omap-usb-tll.c
+++ b/drivers/mfd/omap-usb-tll.c
@@ -220,8 +220,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
int i, ver;
dev_dbg(dev, "starting TI HSUSB TLL Controller\n");
-
- tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL);
+ tll = devm_kzalloc(dev, sizeof(*tll), GFP_KERNEL);
if (!tll)
return -ENOMEM;
--
2.15.1
From: Markus Elfring <[email protected]>
Date: Mon, 15 Jan 2018 14:00:45 +0100
* Return an error code without storing it in an intermediate variable.
* Delete the label "err_clk_alloc" and local variable "ret"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/mfd/omap-usb-tll.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
index c8b4ad40910a..6c0be886fd87 100644
--- a/drivers/mfd/omap-usb-tll.c
+++ b/drivers/mfd/omap-usb-tll.c
@@ -216,7 +216,6 @@ static int usbtll_omap_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct resource *res;
struct usbtll_omap *tll;
- int ret = 0;
int i, ver;
dev_dbg(dev, "starting TI HSUSB TLL Controller\n");
@@ -254,8 +253,9 @@ static int usbtll_omap_probe(struct platform_device *pdev)
tll->ch_clk = devm_kzalloc(dev, sizeof(struct clk *) * tll->nch,
GFP_KERNEL);
if (!tll->ch_clk) {
- ret = -ENOMEM;
- goto err_clk_alloc;
+ pm_runtime_put_sync(dev);
+ pm_runtime_disable(dev);
+ return -ENOMEM;
}
for (i = 0; i < tll->nch; i++) {
@@ -278,12 +278,6 @@ static int usbtll_omap_probe(struct platform_device *pdev)
spin_unlock(&tll_lock);
return 0;
-
-err_clk_alloc:
- pm_runtime_put_sync(dev);
- pm_runtime_disable(dev);
-
- return ret;
}
/**
--
2.15.1
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 <[email protected]>
> ---
> 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];
};
/*-------------------------------------------------------------------------*/
@@ -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";
> goto err_clk_alloc;
> }
What do you think? I'll prepare proper patch in case there's an agreement
on above approach.
Best regards,
ladis
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 <[email protected]>
>> ---
>> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
cheers,
-roger
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
>> @@ -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
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
>>>> @@ -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
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
>>> 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
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
> 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
On Mon, Jan 15, 2018 at 07:12:37PM +0100, SF Markus Elfring wrote:
> Thanks for this clarification. - So I hope that your solution approach
> will be also fine. Will it supersede my proposal?
Who knows, perhaps it would be the best if you could judge yourself...
8<--------
Subject: [PATCH] mfd/omap-usb-tll: Allocate driver data at once
Allocating memory to store clk array together with driver
data simplifies error unwinding and allows deleting memory
allocation failure message as there is now only single point
where allocation could fail.
Signed-off-by: Ladislav Michl <[email protected]>
---
drivers/mfd/omap-usb-tll.c | 57 +++++++++++++++++++---------------------------
1 file changed, 24 insertions(+), 33 deletions(-)
diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
index 44a5d66314c6..f327fe6d3292 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;
+ void __iomem *base;
+ int nch; /* num. of channels */
+ struct clk *ch_clk[0]; /* must be the last member */
};
/*-------------------------------------------------------------------------*/
@@ -216,53 +216,50 @@ static int usbtll_omap_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct resource *res;
struct usbtll_omap *tll;
- int ret = 0;
- int i, ver;
+ void __iomem *base;
+ int i, nch, ver;
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);
- ver = usbtll_read(tll->base, OMAP_USBTLL_REVISION);
+ ver = usbtll_read(base, OMAP_USBTLL_REVISION);
switch (ver) {
case OMAP_USBTLL_REV1:
case OMAP_USBTLL_REV4:
- tll->nch = OMAP_TLL_CHANNEL_COUNT;
+ nch = OMAP_TLL_CHANNEL_COUNT;
break;
case OMAP_USBTLL_REV2:
case OMAP_USBTLL_REV3:
- tll->nch = OMAP_REV2_TLL_CHANNEL_COUNT;
+ nch = OMAP_REV2_TLL_CHANNEL_COUNT;
break;
default:
- tll->nch = OMAP_TLL_CHANNEL_COUNT;
+ nch = OMAP_TLL_CHANNEL_COUNT;
dev_dbg(dev,
"USB TLL Rev : 0x%x not recognized, assuming %d channels\n",
- ver, tll->nch);
+ ver, nch);
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(*tll) + sizeof(tll->ch_clk[nch]),
+ GFP_KERNEL);
+ if (!tll) {
+ pm_runtime_put_sync(dev);
+ pm_runtime_disable(dev);
+ return -ENOMEM;
}
- for (i = 0; i < tll->nch; i++) {
+ tll->base = base;
+ tll->nch = nch;
+ platform_set_drvdata(pdev, tll);
+
+ for (i = 0; i < nch; i++) {
char clkname[] = "usb_tll_hs_usb_chx_clk";
snprintf(clkname, sizeof(clkname),
@@ -282,12 +279,6 @@ static int usbtll_omap_probe(struct platform_device *pdev)
spin_unlock(&tll_lock);
return 0;
-
-err_clk_alloc:
- pm_runtime_put_sync(dev);
- pm_runtime_disable(dev);
-
- return ret;
}
/**
--
2.15.1
>> So I hope that your solution approach will be also fine.
>> Will it supersede my proposal?
>
> Who knows, perhaps it would be the best if you could judge yourself...
I am also curious on how other contributors will respond to
your following update suggestion.
> 8<--------
>
> Subject: [PATCH] mfd/omap-usb-tll: Allocate driver data at once
Would it have been clearer to use this information as the subject
for this reply already?
Are you fine with that this change approach was recorded in
a possibly questionable format?
https://patchwork.kernel.org/patch/10165193/
> Allocating memory to store clk array together with driver
> data simplifies error unwinding and allows deleting memory
> allocation failure message as there is now only single point
> where allocation could fail.
* Will it matter to mention the previous software situation a bit
in such a commit description?
* Would you like to add a tag “link”?
* Are you going to “supersede” any more source code adjustments
around questionable error messages?
Regards,
Markus
On Mon, Jan 15, 2018 at 08:04:03PM +0100, SF Markus Elfring wrote:
> >> So I hope that your solution approach will be also fine.
> >> Will it supersede my proposal?
> >
> > Who knows, perhaps it would be the best if you could judge yourself...
>
> I am also curious on how other contributors will respond to
> your following update suggestion.
>
>
> > 8<--------
> >
> > Subject: [PATCH] mfd/omap-usb-tll: Allocate driver data at once
>
> Would it have been clearer to use this information as the subject
> for this reply already?
>
> Are you fine with that this change approach was recorded in
> a possibly questionable format?
> https://patchwork.kernel.org/patch/10165193/
Sure. It does not seem anyone involved cares about patchwork.
> > Allocating memory to store clk array together with driver
> > data simplifies error unwinding and allows deleting memory
> > allocation failure message as there is now only single point
> > where allocation could fail.
>
> * Will it matter to mention the previous software situation a bit
> in such a commit description?
>
> * Would you like to add a tag “link”?
Are you okay with this one?
https://lkml.org/lkml/2018/1/15/411
> * Are you going to “supersede” any more source code adjustments
> around questionable error messages?
I'm going to handle it usual way:
- wait for feedback and modify accordingly
- collect tags
- resend as v2
Also, patch contains all your changes, so you should be credited
somehow - hence the need for v2 anyway.
What about:
[marcus: simplified error unwinding, error message removal]
Signed-off-by: Markus Elfring <[email protected]>
Feel free to propose anything else.
Best regards,
ladis
> dev_dbg(dev,
> "USB TLL Rev : 0x%x not recognized, assuming %d channels\n",
> - ver, tll->nch);
> + ver, nch);
> break;
Does this format string need an other indentation when you touch this statement?
Regards,
Markus
On Mon, Jan 15, 2018 at 08:26:00PM +0100, SF Markus Elfring wrote:
> > dev_dbg(dev,
> > "USB TLL Rev : 0x%x not recognized, assuming %d channels\n",
> > - ver, tll->nch);
> > + ver, nch);
> > break;
>
> Does this format string need an other indentation when you touch this statement?
Well, lets that the chance and make it shorter as well:
default:
- tll->nch = OMAP_TLL_CHANNEL_COUNT;
- dev_dbg(dev,
- "USB TLL Rev : 0x%x not recognized, assuming %d channels\n",
- ver, tll->nch);
+ nch = OMAP_TLL_CHANNEL_COUNT;
+ dev_dbg(dev, "rev 0x%x not recognized, assuming %d channels\n",
+ ver, nch);
break;
Other proposals?
>> * Would you like to add a tag “link”?
>
> Are you okay with this one?
> https://lkml.org/lkml/2018/1/15/411
Yes.
> - resend as v2
I was unsure about your patch handling from the previous replies.
> Also, patch contains all your changes, so you should be credited
> somehow - hence the need for v2 anyway.
Thanks.
> What about:
> [marcus: simplified error unwinding, error message removal]
^
I would prefer my name written as in the other places.
Will this software development dialogue evolve in further constructive ways?
Regards,
Markus
On Mon, 15 Jan 2018, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 15 Jan 2018 13:43:53 +0100
>
> Replace the specification of a data structure by a pointer dereference
> as the parameter for the operator "sizeof" to make the corresponding size
> determination a bit safer according to the Linux coding style convention.
>
> This issue was detected by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/mfd/omap-usb-tll.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/mfd/omap-usb-tll.c b/drivers/mfd/omap-usb-tll.c
> index 7945efa0152e..c8b4ad40910a 100644
> --- a/drivers/mfd/omap-usb-tll.c
> +++ b/drivers/mfd/omap-usb-tll.c
> @@ -220,8 +220,7 @@ static int usbtll_omap_probe(struct platform_device *pdev)
> int i, ver;
>
> dev_dbg(dev, "starting TI HSUSB TLL Controller\n");
> -
Keep the space between.
> - tll = devm_kzalloc(dev, sizeof(struct usbtll_omap), GFP_KERNEL);
> + tll = devm_kzalloc(dev, sizeof(*tll), GFP_KERNEL);
> if (!tll)
> return -ENOMEM;
>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 15 Jan 2018, SF Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 15 Jan 2018 14:00:45 +0100
>
> * Return an error code without storing it in an intermediate variable.
>
> * Delete the label "err_clk_alloc" and local variable "ret"
> which became unnecessary with this refactoring.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/mfd/omap-usb-tll.c | 12 +++---------
> 1 file changed, 3 insertions(+), 9 deletions(-)
Applied, thanks.
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
On Mon, 22 Jan 2018, Lee Jones wrote:
> On Mon, 15 Jan 2018, SF Markus Elfring wrote:
>
> > From: Markus Elfring <[email protected]>
> > Date: Mon, 15 Jan 2018 14:00:45 +0100
> >
> > * Return an error code without storing it in an intermediate variable.
> >
> > * Delete the label "err_clk_alloc" and local variable "ret"
> > which became unnecessary with this refactoring.
> >
> > Signed-off-by: Markus Elfring <[email protected]>
> > ---
> > drivers/mfd/omap-usb-tll.c | 12 +++---------
> > 1 file changed, 3 insertions(+), 9 deletions(-)
>
> Applied, thanks.
This patch does not apply.
Please rebase and resend.
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
>> Applied, thanks.
>
> This patch does not apply.
>
> Please rebase and resend.
Did you notice that this update suggestion could eventually be superseded
by the software change “[PATCH v2] mfd: omap-usb-tll: Allocate driver data at once”?
https://lkml.org/lkml/2018/1/23/277
https://patchwork.kernel.org/patch/10165339/
https://lkml.kernel.org/r/<20180115202505.GA2628@lenoch>
Regards,
Markus
On Tue, 23 Jan 2018, SF Markus Elfring wrote:
> >> Applied, thanks.
> >
> > This patch does not apply.
> >
> > Please rebase and resend.
>
> Did you notice that this update suggestion could eventually be superseded
> by the software change “[PATCH v2] mfd: omap-usb-tll: Allocate driver data at once”?
How would I? It looks like a completely different patch.
In future, please either reply to the original patch with the
follow-up patch OR reply to the original patch to say it's been
superseded, and provide an indication of which patch superseded it.
> https://lkml.org/lkml/2018/1/23/277
> https://patchwork.kernel.org/patch/10165339/
> https://lkml.kernel.org/r/<20180115202505.GA2628@lenoch>
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Lee,
On Tue, Jan 23, 2018 at 03:04:08PM +0000, Lee Jones wrote:
> On Tue, 23 Jan 2018, SF Markus Elfring wrote:
>
> > >> Applied, thanks.
> > >
> > > This patch does not apply.
> > >
> > > Please rebase and resend.
> >
> > Did you notice that this update suggestion could eventually be superseded
> > by the software change “[PATCH v2] mfd: omap-usb-tll: Allocate driver data at once”?
>
> How would I? It looks like a completely different patch.
>
> In future, please either reply to the original patch with the
> follow-up patch OR reply to the original patch to say it's been
> superseded, and provide an indication of which patch superseded it.
this is my fault. I should point out that v2 superseded also other
patches in the serie (see previous discussion, how that happened).
I'm sorry for this.
> > https://lkml.org/lkml/2018/1/23/277
> > https://patchwork.kernel.org/patch/10165339/
> > https://lkml.kernel.org/r/<20180115202505.GA2628@lenoch>
>
> --
> Lee Jones
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
On Tue, 23 Jan 2018, Ladislav Michl wrote:
> Lee,
>
> On Tue, Jan 23, 2018 at 03:04:08PM +0000, Lee Jones wrote:
> > On Tue, 23 Jan 2018, SF Markus Elfring wrote:
> >
> > > >> Applied, thanks.
> > > >
> > > > This patch does not apply.
> > > >
> > > > Please rebase and resend.
> > >
> > > Did you notice that this update suggestion could eventually be superseded
> > > by the software change “[PATCH v2] mfd: omap-usb-tll: Allocate driver data at once”?
> >
> > How would I? It looks like a completely different patch.
> >
> > In future, please either reply to the original patch with the
> > follow-up patch OR reply to the original patch to say it's been
> > superseded, and provide an indication of which patch superseded it.
>
> this is my fault. I should point out that v2 superseded also other
> patches in the serie (see previous discussion, how that happened).
>
> I'm sorry for this.
Not a problem.
--
Lee Jones
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog