diff mbox

[1/2] mfd: twl-core: Return directly after a failed platform_device_alloc() in add_numbered_child()

Message ID 3c168185-ec2d-f1f7-fe70-8a230b884ed2@users.sourceforge.net (mailing list archive)
State New, archived
Headers show

Commit Message

SF Markus Elfring May 16, 2016, 6:26 a.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Sun, 15 May 2016 19:20:28 +0200

The platform_device_put() function was called in one case by the
add_numbered_child() function during error handling even if the passed
variable "pdev" contained a null pointer.

* Change an error message.

* Return directly in this case.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/mfd/twl-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Julia Lawall May 16, 2016, 6:51 a.m. UTC | #1 
On Mon, 16 May 2016, SF Markus Elfring wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Sun, 15 May 2016 19:20:28 +0200
> 
> The platform_device_put() function was called in one case by the
> add_numbered_child() function during error handling even if the passed
> variable "pdev" contained a null pointer.
> 
> * Change an error message.

Why?  Is dev_err needed?  Doesn't it already print out the device name?

In any case, the only source of failure is failure of a kzalloc in 
platform_device_alloc, which means that a complete backtrace would be 
generated, so it is not clear that any message is needed at all.

julia

> 
> * Return directly in this case.
> 
> This issue was detected by using the Coccinelle software.
> 
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
> ---
>  drivers/mfd/twl-core.c | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index 831696e..dc34e69 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -623,9 +623,10 @@ add_numbered_child(unsigned mod_no, const char *name, int num,
>  
>  	pdev = platform_device_alloc(name, num);
>  	if (!pdev) {
> -		dev_dbg(&twl->client->dev, "can't alloc dev\n");
> -		status = -ENOMEM;
> -		goto err;
> +		dev_err(&twl->client->dev,
> +			"Allocation failed for device: %s\n",
> +			name);
> +		return ERR_PTR(-ENOMEM);
>  	}
>  
>  	pdev->dev.parent = &twl->client->dev;
> -- 
> 2.8.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
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
SF Markus Elfring May 16, 2016, 7:54 a.m. UTC | #2 
>> * Change an error message.
> 
> Why?  Is dev_err needed?

I interpreted Lee's response in this way.
https://lkml.org/lkml/2016/1/11/104

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
Julia Lawall May 16, 2016, 8:07 a.m. UTC | #3 
On Mon, 16 May 2016, SF Markus Elfring wrote:

> >> * Change an error message.
> > 
> > Why?  Is dev_err needed?
> 
> I interpreted Lee's response in this way.
> https://lkml.org/lkml/2016/1/11/104

OK.  He didn't ask for the message to be changed though.  It's a bit 
unfortunate that it now takes up multiple lines.  And I believe it also 
prints redundant information.  Perhaps he will have some further thoughts 
on the matter.

julia
--
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
Lee Jones May 17, 2016, 6 a.m. UTC | #4 
On Mon, 16 May 2016, Julia Lawall wrote:

> On Mon, 16 May 2016, SF Markus Elfring wrote:
> 
> > >> * Change an error message.
> > > 
> > > Why?  Is dev_err needed?
> > 
> > I interpreted Lee's response in this way.
> > https://lkml.org/lkml/2016/1/11/104
> 
> OK.  He didn't ask for the message to be changed though.  It's a bit 
> unfortunate that it now takes up multiple lines.  And I believe it also 
> prints redundant information.  Perhaps he will have some further thoughts 
> on the matter.

Yes, Julia is right.  We normally don't print anything for OOM
errors since Linux reports on them already.

Please remove the print altogether.
SF Markus Elfring May 17, 2016, 2:15 p.m. UTC | #5 
> Please remove the print altogether.

Would you like to omit any extra logging statements at more source code places?

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 mbox

Patch

diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
index 831696e..dc34e69 100644
--- a/drivers/mfd/twl-core.c
+++ b/drivers/mfd/twl-core.c
@@ -623,9 +623,10 @@  add_numbered_child(unsigned mod_no, const char *name, int num,
 
 	pdev = platform_device_alloc(name, num);
 	if (!pdev) {
-		dev_dbg(&twl->client->dev, "can't alloc dev\n");
-		status = -ENOMEM;
-		goto err;
+		dev_err(&twl->client->dev,
+			"Allocation failed for device: %s\n",
+			name);
+		return ERR_PTR(-ENOMEM);
 	}
 
 	pdev->dev.parent = &twl->client->dev;