* [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling.
2012-03-23 9:51 [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling Dan Carpenter
@ 2012-03-23 9:53 ` santosh nayak
2012-03-23 9:58 ` santosh prasad nayak
` (5 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: santosh nayak @ 2012-03-23 9:53 UTC (permalink / raw)
To: kernel-janitors
From: Santosh Nayak <santoshprasadnayak@gmail.com>
Replace kmalloc+memset pair by kzalloc() in 'wl_wds_device_alloc()'.
Add error handling to avoid null derefernce.
Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
---
drivers/staging/wlags49_h2/wl_netdev.c | 8 ++++++--
1 files changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/staging/wlags49_h2/wl_netdev.c b/drivers/staging/wlags49_h2/wl_netdev.c
index 9c16f54..426ba04 100644
--- a/drivers/staging/wlags49_h2/wl_netdev.c
+++ b/drivers/staging/wlags49_h2/wl_netdev.c
@@ -1511,8 +1511,12 @@ void wl_wds_device_alloc( struct wl_private *lp )
for( count = 0; count < NUM_WDS_PORTS; count++ ) {
struct net_device *dev_wds = NULL;
- dev_wds = kmalloc( sizeof( struct net_device ), GFP_KERNEL );
- memset( dev_wds, 0, sizeof( struct net_device ));
+ dev_wds = kzalloc(sizeof(struct net_device), GFP_KERNEL);
+ if (unlikely(!dev_wds)) {
+ printk(KERN_ERR "%s: failed to alloc memory\n", __func__);
+ DBG_LEAVE(DbgInfo);
+ return;
+ }
ether_setup( dev_wds );
--
1.7.4.4
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling.
2012-03-23 9:51 [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling Dan Carpenter
2012-03-23 9:53 ` santosh nayak
@ 2012-03-23 9:58 ` santosh prasad nayak
2012-03-23 10:40 ` Dan Carpenter
` (4 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: santosh prasad nayak @ 2012-03-23 9:58 UTC (permalink / raw)
To: kernel-janitors
On Fri, Mar 23, 2012 at 3:21 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> On Fri, Mar 23, 2012 at 03:11:45PM +0530, santosh nayak wrote:
>> - dev_wds = kmalloc( sizeof( struct net_device ), GFP_KERNEL );
>> - memset( dev_wds, 0, sizeof( struct net_device ));
>> + dev_wds = kzalloc(sizeof(struct net_device), GFP_KERNEL);
>> + if (unlikely(!dev_wds)) {
>> + printk(KERN_ERR "%s: failed to alloc memory\n", __func__);
>
> Don't resend, but kmalloc() already prints out way more extensive
> and useful error messages than this. No need to print an error
> message yourself normally.
>
Do you mean in the following format ?
+ if (unlikely(!dev_wds)) {
+ DBG_LEAVE(DbgInfo);
+ return;
+ }
regards
Santosh
> regards,
> dan carpenter
>
>
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling.
2012-03-23 9:51 [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling Dan Carpenter
2012-03-23 9:53 ` santosh nayak
2012-03-23 9:58 ` santosh prasad nayak
@ 2012-03-23 10:40 ` Dan Carpenter
2012-03-23 14:56 ` Greg KH
` (3 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Dan Carpenter @ 2012-03-23 10:40 UTC (permalink / raw)
To: kernel-janitors
[-- Attachment #1: Type: text/plain, Size: 998 bytes --]
On Fri, Mar 23, 2012 at 03:27:10PM +0530, santosh prasad nayak wrote:
> On Fri, Mar 23, 2012 at 3:21 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> > On Fri, Mar 23, 2012 at 03:11:45PM +0530, santosh nayak wrote:
> >> - dev_wds = kmalloc( sizeof( struct net_device ), GFP_KERNEL );
> >> - memset( dev_wds, 0, sizeof( struct net_device ));
> >> + dev_wds = kzalloc(sizeof(struct net_device), GFP_KERNEL);
> >> + if (unlikely(!dev_wds)) {
> >> + printk(KERN_ERR "%s: failed to alloc memory\n", __func__);
> >
> > Don't resend, but kmalloc() already prints out way more extensive
> > and useful error messages than this. No need to print an error
> > message yourself normally.
> >
>
> Do you mean in the following format ?
>
> + if (unlikely(!dev_wds)) {
> + DBG_LEAVE(DbgInfo);
> + return;
> + }
No need to resend. It's just for next time something to consider.
regards,
dan carpenter
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling.
2012-03-23 9:51 [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling Dan Carpenter
` (2 preceding siblings ...)
2012-03-23 10:40 ` Dan Carpenter
@ 2012-03-23 14:56 ` Greg KH
2012-03-23 15:41 ` Greg KH
` (2 subsequent siblings)
6 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2012-03-23 14:56 UTC (permalink / raw)
To: kernel-janitors
On Fri, Mar 23, 2012 at 03:11:45PM +0530, santosh nayak wrote:
> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>
> Replace kmalloc+memset pair by kzalloc() in 'wl_wds_device_alloc()'.
> Add error handling to avoid null derefernce.
>
> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
> ---
> drivers/staging/wlags49_h2/wl_netdev.c | 8 ++++++--
> 1 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/wlags49_h2/wl_netdev.c b/drivers/staging/wlags49_h2/wl_netdev.c
> index 9c16f54..426ba04 100644
> --- a/drivers/staging/wlags49_h2/wl_netdev.c
> +++ b/drivers/staging/wlags49_h2/wl_netdev.c
> @@ -1511,8 +1511,12 @@ void wl_wds_device_alloc( struct wl_private *lp )
> for( count = 0; count < NUM_WDS_PORTS; count++ ) {
> struct net_device *dev_wds = NULL;
>
> - dev_wds = kmalloc( sizeof( struct net_device ), GFP_KERNEL );
> - memset( dev_wds, 0, sizeof( struct net_device ));
> + dev_wds = kzalloc(sizeof(struct net_device), GFP_KERNEL);
> + if (unlikely(!dev_wds)) {
NEVER use unlikely unless you are working on scheduler code, or
something else equally time critical. So, take this into consideration,
and Dan's comments, and please redo this, I can't take it as-is.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling.
2012-03-23 9:51 [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling Dan Carpenter
` (3 preceding siblings ...)
2012-03-23 14:56 ` Greg KH
@ 2012-03-23 15:41 ` Greg KH
2012-03-23 15:42 ` santosh prasad nayak
2012-03-27 20:53 ` Ezequiel García
6 siblings, 0 replies; 8+ messages in thread
From: Greg KH @ 2012-03-23 15:41 UTC (permalink / raw)
To: kernel-janitors
On Fri, Mar 23, 2012 at 09:00:19PM +0530, santosh prasad nayak wrote:
> Can you please explain whats the disadvantage of having 'unlikely()' here ?
> I have seen it at several places in driver side.
And I bet they are used wrong there. Again, unless you are in some very
performance critical section of code, don't use unlikely().
The big thing is that it just is not needed at all. Don't try to be
smarter than the compiler if you don't need it, _AND_ you haven't
actually measured it.
greg k-h
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling.
2012-03-23 9:51 [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling Dan Carpenter
` (4 preceding siblings ...)
2012-03-23 15:41 ` Greg KH
@ 2012-03-23 15:42 ` santosh prasad nayak
2012-03-27 20:53 ` Ezequiel García
6 siblings, 0 replies; 8+ messages in thread
From: santosh prasad nayak @ 2012-03-23 15:42 UTC (permalink / raw)
To: kernel-janitors
On Fri, Mar 23, 2012 at 8:26 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Mar 23, 2012 at 03:11:45PM +0530, santosh nayak wrote:
>> From: Santosh Nayak <santoshprasadnayak@gmail.com>
>>
>> Replace kmalloc+memset pair by kzalloc() in 'wl_wds_device_alloc()'.
>> Add error handling to avoid null derefernce.
>>
>> Signed-off-by: Santosh Nayak <santoshprasadnayak@gmail.com>
>> ---
>> drivers/staging/wlags49_h2/wl_netdev.c | 8 ++++++--
>> 1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/staging/wlags49_h2/wl_netdev.c b/drivers/staging/wlags49_h2/wl_netdev.c
>> index 9c16f54..426ba04 100644
>> --- a/drivers/staging/wlags49_h2/wl_netdev.c
>> +++ b/drivers/staging/wlags49_h2/wl_netdev.c
>> @@ -1511,8 +1511,12 @@ void wl_wds_device_alloc( struct wl_private *lp )
>> for( count = 0; count < NUM_WDS_PORTS; count++ ) {
>> struct net_device *dev_wds = NULL;
>>
>> - dev_wds = kmalloc( sizeof( struct net_device ), GFP_KERNEL );
>> - memset( dev_wds, 0, sizeof( struct net_device ));
>> + dev_wds = kzalloc(sizeof(struct net_device), GFP_KERNEL);
>> + if (unlikely(!dev_wds)) {
>
> NEVER use unlikely unless you are working on scheduler code, or
> something else equally time critical. So, take this into consideration,
> and Dan's comments, and please redo this, I can't take it as-is.
>
Greg,
I will include your suggestion and resend it.
Can you please explain whats the disadvantage of having 'unlikely()' here ?
I have seen it at several places in driver side.
regards
Santosh
> thanks,
>
> greg k-h
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling.
2012-03-23 9:51 [PATCH] staging: wlags49_h2: Replace kmalloc+memset by kzalloc and add error handling Dan Carpenter
` (5 preceding siblings ...)
2012-03-23 15:42 ` santosh prasad nayak
@ 2012-03-27 20:53 ` Ezequiel García
6 siblings, 0 replies; 8+ messages in thread
From: Ezequiel García @ 2012-03-27 20:53 UTC (permalink / raw)
To: kernel-janitors
Hi,
On Fri, Mar 23, 2012 at 12:41 PM, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> And I bet they are used wrong there. Again, unless you are in some very
> performance critical section of code, don't use unlikely().
>
> The big thing is that it just is not needed at all. Don't try to be
> smarter than the compiler if you don't need it, _AND_ you haven't
> actually measured it.
Nice answer. I did a quick grep and I found this scary result:
~/devel/v4l-dvb $ grep -r -n "unlikely(" drivers/ | wc -l
5018
I wonder perhaps we could add a check for unlikely in checkpatch script?
Something like: "are you sure about this"?
Also, perhaps this quote from [1] should be somewhere in Documentation:
"How should I use it ?
You should use it only in cases when the likeliest branch is very very
very likely, or when the unlikeliest branch is very very very
unlikely."
[1] http://kernelnewbies.org/FAQ/LikelyUnlikely
--
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
^ permalink raw reply [flat|nested] 8+ messages in thread