[PATCH 1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

[PATCH 1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

[Sachin Kamat]

Silences the following smatch warning:
drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
returning -1 instead of -ENOMEM is sloppy

Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/hid/usbhid/hiddev.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
index 14599e2..50e2ab8 100644
--- a/drivers/hid/usbhid/hiddev.c
+++ b/drivers/hid/usbhid/hiddev.c
@@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
 	}
 
 	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
-		return -1;
+		return -ENOMEM;
 
 	init_waitqueue_head(&hiddev->wait);
 	INIT_LIST_HEAD(&hiddev->list);
-- 
1.7.4.1

[PATCH 2/2] hid: hid-sensor-hub: Move NULL check to beginning

[Sachin Kamat]

This avoids a possible NULL pointer dereference error when report
is NULL.

Cc: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
 drivers/hid/hid-sensor-hub.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index d9d73e9..29f8071 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -428,6 +428,9 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
 	struct hid_collection *collection = NULL;
 	void *priv = NULL;
 
+	if (!report)
+		goto err_report;
+
 	hid_dbg(hdev, "sensor_hub_raw_event report id:0x%x size:%d type:%d\n",
 			 report->id, size, report->type);
 	hid_dbg(hdev, "maxfield:%d\n", report->maxfield);
@@ -437,9 +440,6 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
 	ptr = raw_data;
 	ptr++; /*Skip report id*/
 
-	if (!report)
-		goto err_report;
-
 	spin_lock_irqsave(&pdata->lock, flags);
 
 	for (i = 0; i < report->maxfield; ++i) {
-- 
1.7.4.1

Re: [PATCH 2/2] hid: hid-sensor-hub: Move NULL check to beginning

[Alan Cox]

On Wed, 21 Nov 2012 11:50:30 +0530
Sachin Kamat <sachin.kamat@linaro.org> wrote:

> This avoids a possible NULL pointer dereference error when report
> is NULL.

This is wrong. Please see the patch I sent a while ago - it can't be
NULL, the check just needs removing.

Re: [PATCH 2/2] hid: hid-sensor-hub: Move NULL check to beginning

[Srinivas Pandruvada]

Hi Jonathan,

Please check and merge Alan Cox's patch. I have forwarded this patch to you.

Thanks,
Srinivas


On 11/21/2012 04:42 AM, Alan Cox wrote:
> On Wed, 21 Nov 2012 11:50:30 +0530
> Sachin Kamat <sachin.kamat@linaro.org> wrote:
>
>> This avoids a possible NULL pointer dereference error when report
>> is NULL.
> This is wrong. Please see the patch I sent a while ago - it can't be
> NULL, the check just needs removing.
>

Re: [PATCH 2/2] hid: hid-sensor-hub: Move NULL check to beginning

[Jiri Kosina]

On Wed, 21 Nov 2012, Srinivas Pandruvada wrote:

> Please check and merge Alan Cox's patch. I have forwarded this patch to you.

I have already applied the patch to my tree.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

[Jiri Kosina]

On Wed, 21 Nov 2012, Sachin Kamat wrote:

> Silences the following smatch warning:
> drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
> returning -1 instead of -ENOMEM is sloppy
> 
> Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> ---
>  drivers/hid/usbhid/hiddev.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> index 14599e2..50e2ab8 100644
> --- a/drivers/hid/usbhid/hiddev.c
> +++ b/drivers/hid/usbhid/hiddev.c
> @@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
>  	}
>  
>  	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
> -		return -1;
> +		return -ENOMEM;
>  
>  	init_waitqueue_head(&hiddev->wait);
>  	INIT_LIST_HEAD(&hiddev->list);

Well, this would make sense only if the callers would be actualling doing 
something useful with that return value. But the only check we are 
performing at callsites is non-zero test ...

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

[Dmitry Torokhov]

On Thu, Nov 22, 2012 at 10:03:44PM +0100, Jiri Kosina wrote:
> On Wed, 21 Nov 2012, Sachin Kamat wrote:
> 
> > Silences the following smatch warning:
> > drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
> > returning -1 instead of -ENOMEM is sloppy
> > 
> > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > ---
> >  drivers/hid/usbhid/hiddev.c |    2 +-
> >  1 files changed, 1 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> > index 14599e2..50e2ab8 100644
> > --- a/drivers/hid/usbhid/hiddev.c
> > +++ b/drivers/hid/usbhid/hiddev.c
> > @@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
> >  	}
> >  
> >  	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
> > -		return -1;
> > +		return -ENOMEM;
> >  
> >  	init_waitqueue_head(&hiddev->wait);
> >  	INIT_LIST_HEAD(&hiddev->list);
> 
> Well, this would make sense only if the callers would be actualling doing 
> something useful with that return value. But the only check we are 
> performing at callsites is non-zero test ...

It is chicken and egg problem - callers can't use the result unless it
is meaningful and callee's do not bother to send anything meaningful
because nobody uses it...

If someone takes time to convert to proper return codes I think it would
be a good thing.

Thanks.

-- 
Dmitry

Re: [PATCH 1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

[Jiri Kosina]

On Mon, 26 Nov 2012, Dmitry Torokhov wrote:

> > > Silences the following smatch warning:
> > > drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
> > > returning -1 instead of -ENOMEM is sloppy
> > > 
> > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
> > > ---
> > >  drivers/hid/usbhid/hiddev.c |    2 +-
> > >  1 files changed, 1 insertions(+), 1 deletions(-)
> > > 
> > > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
> > > index 14599e2..50e2ab8 100644
> > > --- a/drivers/hid/usbhid/hiddev.c
> > > +++ b/drivers/hid/usbhid/hiddev.c
> > > @@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
> > >  	}
> > >  
> > >  	if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
> > > -		return -1;
> > > +		return -ENOMEM;
> > >  
> > >  	init_waitqueue_head(&hiddev->wait);
> > >  	INIT_LIST_HEAD(&hiddev->list);
> > 
> > Well, this would make sense only if the callers would be actualling doing 
> > something useful with that return value. But the only check we are 
> > performing at callsites is non-zero test ...
> 
> It is chicken and egg problem - callers can't use the result unless it
> is meaningful and callee's do not bother to send anything meaningful
> because nobody uses it...
> 
> If someone takes time to convert to proper return codes I think it would
> be a good thing.

Fully agree with you.

But I'd like to have both sides changed together once someone starts 
taking care of fixing it.

Thanks,

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

[Sachin Kamat]

Hi Jiri,

On 27 November 2012 03:39, Jiri Kosina <jkosina@suse.cz> wrote:
> On Mon, 26 Nov 2012, Dmitry Torokhov wrote:
>
>> > > Silences the following smatch warning:
>> > > drivers/hid/usbhid/hiddev.c:897 hiddev_connect() warn:
>> > > returning -1 instead of -ENOMEM is sloppy
>> > >
>> > > Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
>> > > ---
>> > >  drivers/hid/usbhid/hiddev.c |    2 +-
>> > >  1 files changed, 1 insertions(+), 1 deletions(-)
>> > >
>> > > diff --git a/drivers/hid/usbhid/hiddev.c b/drivers/hid/usbhid/hiddev.c
>> > > index 14599e2..50e2ab8 100644
>> > > --- a/drivers/hid/usbhid/hiddev.c
>> > > +++ b/drivers/hid/usbhid/hiddev.c
>> > > @@ -894,7 +894,7 @@ int hiddev_connect(struct hid_device *hid, unsigned int force)
>> > >   }
>> > >
>> > >   if (!(hiddev = kzalloc(sizeof(struct hiddev), GFP_KERNEL)))
>> > > -         return -1;
>> > > +         return -ENOMEM;
>> > >
>> > >   init_waitqueue_head(&hiddev->wait);
>> > >   INIT_LIST_HEAD(&hiddev->list);
>> >
>> > Well, this would make sense only if the callers would be actualling doing
>> > something useful with that return value. But the only check we are
>> > performing at callsites is non-zero test ...
>>
>> It is chicken and egg problem - callers can't use the result unless it
>> is meaningful and callee's do not bother to send anything meaningful
>> because nobody uses it...
>>
>> If someone takes time to convert to proper return codes I think it would
>> be a good thing.
>
> Fully agree with you.
>
> But I'd like to have both sides changed together once someone starts
> taking care of fixing it.

The proposed change is done in the function hiddev_connect which
returns 0 on success.
The caller only checks if the return value is 0 or not
(!hdev->hiddev_connect(...)). In this case
what is the change that you would like to see on the caller side?

>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs



-- 
With warm regards,
Sachin

Re: [PATCH 1/2] hid: usbhid: Return -ENOMEM instead of -1 for memory allocation failure

[Jiri Kosina]

On Tue, 27 Nov 2012, Sachin Kamat wrote:

> > Fully agree with you.
> >
> > But I'd like to have both sides changed together once someone starts
> > taking care of fixing it.
> 
> The proposed change is done in the function hiddev_connect which
> returns 0 on success.
> The caller only checks if the return value is 0 or not
> (!hdev->hiddev_connect(...)). In this case
> what is the change that you would like to see on the caller side?

A check whether the value returned by hiddev_connect() is error code 
(through IS_ERR_VALUE() or something similar), and continue propagating 
the error out of hid_connect().

-- 
Jiri Kosina
SUSE Labs