[PATCH 00/14] i2c-hid cleanup and bug fixes

[PATCH 00/14] i2c-hid cleanup and bug fixes

[Benjamin Tissoires]

Hi guys,

Jean made a recent review of i2c-hid, and before this driver goes into
Linus' tree, here are some cleanups.

Patch 2 has not been detected by Jean, but it appeared while playing with
buffers allocation. So this is the only change not asked by the previous
review.

I still need to work on the mutex to protect the potential race on .open and
.close, but meanwhile, here is a first batch of patches.

Cheers,
Benjamin

Benjamin Tissoires (14):
  HID: i2c-hid: change I2C name
  HID: i2c-hid: fix memory corruption due to missing hid declarations
  HID: i2c-hid: enhance Kconfig
  HID: i2c-hid: fix checkpatch.pl warning
  HID: i2c-hid: fix i2c_hid_dbg macro
  HID: i2c-hid: remove unused static declarations
  HID: i2c-hid: fix return paths
  HID: i2c-hid: fix error messages
  HID: i2c-hid: i2c_hid_get_report may fail
  HID: i2c-hid: reorder allocation/free of buffers
  HID: i2c-hid: remove unneeded test in i2c_hid_remove
  HID: i2c-hid: remove extra .irq field in struct i2c_hid
  HID: i2c-hid: also call i2c_hid_free_buffers in i2c_hid_remove
  HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

 drivers/hid/i2c-hid/Kconfig   |   7 +-
 drivers/hid/i2c-hid/i2c-hid.c | 184 ++++++++++++++++++++----------------------
 2 files changed, 89 insertions(+), 102 deletions(-)

-- 
1.8.0.1

[PATCH 01/14] HID: i2c-hid: change I2C name

[Benjamin Tissoires]

no I2C driver has "i2c" in its name. It makes more sense to call this
i2c driver "hid".

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 67ab5b7..0fbb229 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -949,7 +949,7 @@ static int i2c_hid_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
 
 static const struct i2c_device_id i2c_hid_id_table[] = {
-	{ "i2c_hid", 0 },
+	{ "hid", 0 },
 	{ },
 };
 MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);
-- 
1.8.0.1

Re: [PATCH 01/14] HID: i2c-hid: change I2C name

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:42 +0100, Benjamin Tissoires wrote:
> no I2C driver has "i2c" in its name. It makes more sense to call this
> i2c driver "hid".
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 67ab5b7..0fbb229 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -949,7 +949,7 @@ static int i2c_hid_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
>  
>  static const struct i2c_device_id i2c_hid_id_table[] = {
> -	{ "i2c_hid", 0 },
> +	{ "hid", 0 },
>  	{ },
>  };
>  MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 01/14] HID: i2c-hid: change I2C name

[Jiri Kosina]

On Tue, 4 Dec 2012, Jean Delvare wrote:

> > no I2C driver has "i2c" in its name. It makes more sense to call this
> > i2c driver "hid".
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 67ab5b7..0fbb229 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -949,7 +949,7 @@ static int i2c_hid_resume(struct device *dev)
> >  static SIMPLE_DEV_PM_OPS(i2c_hid_pm, i2c_hid_suspend, i2c_hid_resume);
> >  
> >  static const struct i2c_device_id i2c_hid_id_table[] = {
> > -	{ "i2c_hid", 0 },
> > +	{ "hid", 0 },
> >  	{ },
> >  };
> >  MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

[PATCH 02/14] HID: i2c-hid: fix memory corruption due to missing hid declaration

[Benjamin Tissoires]

HID descriptors contains 4 bytes of reserved field.
The previous implementation was overriding the next fields in struct i2c_hid.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 0fbb229..ad771a7 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -68,6 +68,7 @@ struct i2c_hid_desc {
 	__le16 wVendorID;
 	__le16 wProductID;
 	__le16 wVersionID;
+	__le32 reserved;
 } __packed;
 
 struct i2c_hid_cmd {
@@ -778,7 +779,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	}
 
 	dsize = le16_to_cpu(hdesc->wHIDDescLength);
-	if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
+	if (!dsize || dsize > sizeof(struct i2c_hid_desc)) {
 		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
 			dsize);
 		return -ENODEV;
-- 
1.8.0.1

Re: [PATCH 02/14] HID: i2c-hid: fix memory corruption due to missing hid declaration

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:43 +0100, Benjamin Tissoires wrote:
> HID descriptors contains 4 bytes of reserved field.
> The previous implementation was overriding the next fields in struct i2c_hid.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 0fbb229..ad771a7 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -68,6 +68,7 @@ struct i2c_hid_desc {
>  	__le16 wVendorID;
>  	__le16 wProductID;
>  	__le16 wVersionID;
> +	__le32 reserved;
>  } __packed;
>  
>  struct i2c_hid_cmd {
> @@ -778,7 +779,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>  	}
>  
>  	dsize = le16_to_cpu(hdesc->wHIDDescLength);
> -	if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
> +	if (!dsize || dsize > sizeof(struct i2c_hid_desc)) {

Shouldn't !dsize rather be dsize < 4? You're reading hdesc->bcdVersion,
which is only defined if dsize >= 4 if I understand the code correctly.

>  		dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
>  			dsize);
>  		return -ENODEV;


-- 
Jean Delvare

Re: [PATCH 02/14] HID: i2c-hid: fix memory corruption due to missing hid declaration

[Benjamin Tissoires]

On Tue, Dec 4, 2012 at 10:42 PM, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue,  4 Dec 2012 16:27:43 +0100, Benjamin Tissoires wrote:
>> HID descriptors contains 4 bytes of reserved field.
>> The previous implementation was overriding the next fields in struct i2c_hid.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/i2c-hid/i2c-hid.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index 0fbb229..ad771a7 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -68,6 +68,7 @@ struct i2c_hid_desc {
>>       __le16 wVendorID;
>>       __le16 wProductID;
>>       __le16 wVersionID;
>> +     __le32 reserved;
>>  } __packed;
>>
>>  struct i2c_hid_cmd {
>> @@ -778,7 +779,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>>       }
>>
>>       dsize = le16_to_cpu(hdesc->wHIDDescLength);
>> -     if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
>> +     if (!dsize || dsize > sizeof(struct i2c_hid_desc)) {
>
> Shouldn't !dsize rather be dsize < 4? You're reading hdesc->bcdVersion,
> which is only defined if dsize >= 4 if I understand the code correctly.

Yes, you are right. Thanks.

Jiri, this patch is a prerequisite for "[PATCH 10/14] HID: i2c-hid:
reorder allocation/free of buffers".
could you please what for a v2 before applying 10/14?

Cheers,
Benjamin

>
>>               dev_err(&client->dev, "weird size of HID descriptor (%u)\n",
>>                       dsize);
>>               return -ENODEV;
>
>
> --
> Jean Delvare

Re: [PATCH 02/14] HID: i2c-hid: fix memory corruption due to missing hid declaration

[Jiri Kosina]

On Wed, 5 Dec 2012, Benjamin Tissoires wrote:

> > On Tue,  4 Dec 2012 16:27:43 +0100, Benjamin Tissoires wrote:
> >> HID descriptors contains 4 bytes of reserved field.
> >> The previous implementation was overriding the next fields in struct i2c_hid.
> >>
> >> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> >> ---
> >>  drivers/hid/i2c-hid/i2c-hid.c | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> >> index 0fbb229..ad771a7 100644
> >> --- a/drivers/hid/i2c-hid/i2c-hid.c
> >> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> >> @@ -68,6 +68,7 @@ struct i2c_hid_desc {
> >>       __le16 wVendorID;
> >>       __le16 wProductID;
> >>       __le16 wVersionID;
> >> +     __le32 reserved;
> >>  } __packed;
> >>
> >>  struct i2c_hid_cmd {
> >> @@ -778,7 +779,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> >>       }
> >>
> >>       dsize = le16_to_cpu(hdesc->wHIDDescLength);
> >> -     if (!dsize || dsize > HID_MAX_DESCRIPTOR_SIZE) {
> >> +     if (!dsize || dsize > sizeof(struct i2c_hid_desc)) {
> >
> > Shouldn't !dsize rather be dsize < 4? You're reading hdesc->bcdVersion,
> > which is only defined if dsize >= 4 if I understand the code correctly.
> 
> Yes, you are right. Thanks.
> 
> Jiri, this patch is a prerequisite for "[PATCH 10/14] HID: i2c-hid:
> reorder allocation/free of buffers".
> could you please what for a v2 before applying 10/14?

Absolutely. I still have to review 9, 10, 11, 12, 13, 14, so am dropping 2 
and 10 from my attention for now.

-- 
Jiri Kosina
SUSE Labs

[PATCH 03/14] HID: i2c-hid: enhance Kconfig

[Benjamin Tissoires]

The "comment" part can never be displayed, so we can remove it.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/Kconfig | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
index 5b7d4d8..64ce108 100644
--- a/drivers/hid/i2c-hid/Kconfig
+++ b/drivers/hid/i2c-hid/Kconfig
@@ -7,15 +7,12 @@ config I2C_HID
 	depends on I2C && INPUT
 	select HID
 	---help---
-	  Say Y here if you want to use the HID over i2c protocol
-	  implementation.
+	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
+	  other HID based devices which is connected to your computer via I2C.
 
 	  If unsure, say N.
 
 	  This support is also available as a module.  If so, the module
 	  will be called i2c-hid.
 
-comment "Input core support is needed for HID over I2C input layer"
-	depends on I2C_HID && INPUT=n
-
 endmenu
-- 
1.8.0.1

Re: [PATCH 03/14] HID: i2c-hid: enhance Kconfig

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:44 +0100, Benjamin Tissoires wrote:
> The "comment" part can never be displayed, so we can remove it.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/Kconfig | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
> index 5b7d4d8..64ce108 100644
> --- a/drivers/hid/i2c-hid/Kconfig
> +++ b/drivers/hid/i2c-hid/Kconfig
> @@ -7,15 +7,12 @@ config I2C_HID
>  	depends on I2C && INPUT
>  	select HID
>  	---help---
> -	  Say Y here if you want to use the HID over i2c protocol
> -	  implementation.
> +	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
> +	  other HID based devices which is connected to your computer via I2C.
>  
>  	  If unsure, say N.
>  
>  	  This support is also available as a module.  If so, the module
>  	  will be called i2c-hid.
>  
> -comment "Input core support is needed for HID over I2C input layer"
> -	depends on I2C_HID && INPUT=n
> -
>  endmenu

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 03/14] HID: i2c-hid: enhance Kconfig

[Jiri Kosina]

On Tue, 4 Dec 2012, Jean Delvare wrote:

> > The "comment" part can never be displayed, so we can remove it.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/Kconfig | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/Kconfig b/drivers/hid/i2c-hid/Kconfig
> > index 5b7d4d8..64ce108 100644
> > --- a/drivers/hid/i2c-hid/Kconfig
> > +++ b/drivers/hid/i2c-hid/Kconfig
> > @@ -7,15 +7,12 @@ config I2C_HID
> >  	depends on I2C && INPUT
> >  	select HID
> >  	---help---
> > -	  Say Y here if you want to use the HID over i2c protocol
> > -	  implementation.
> > +	  Say Y here if you use a keyboard, a touchpad, a touchscreen, or any
> > +	  other HID based devices which is connected to your computer via I2C.
> >  
> >  	  If unsure, say N.
> >  
> >  	  This support is also available as a module.  If so, the module
> >  	  will be called i2c-hid.
> >  
> > -comment "Input core support is needed for HID over I2C input layer"
> > -	depends on I2C_HID && INPUT=n
> > -
> >  endmenu
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Applied.

-- 
Jiri Kosina
SUSE Labs

[PATCH 04/14] HID: i2c-hid: fix checkpatch.pl warning

[Benjamin Tissoires]

We should not initialize to 0 static declarations.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index ad771a7..9ee6555 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -46,7 +46,7 @@
 #define I2C_HID_PWR_SLEEP	0x01
 
 /* debug option */
-static bool debug = false;
+static bool debug;
 module_param(debug, bool, 0444);
 MODULE_PARM_DESC(debug, "print a lot of debug information");
 
-- 
1.8.0.1

Re: [PATCH 04/14] HID: i2c-hid: fix checkpatch.pl warning

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:45 +0100, Benjamin Tissoires wrote:
> We should not initialize to 0 static declarations.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index ad771a7..9ee6555 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -46,7 +46,7 @@
>  #define I2C_HID_PWR_SLEEP	0x01
>  
>  /* debug option */
> -static bool debug = false;
> +static bool debug;
>  module_param(debug, bool, 0444);
>  MODULE_PARM_DESC(debug, "print a lot of debug information");
>  

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 04/14] HID: i2c-hid: fix checkpatch.pl warning

[Jiri Kosina]

On Tue, 4 Dec 2012, Jean Delvare wrote:

> > We should not initialize to 0 static declarations.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index ad771a7..9ee6555 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -46,7 +46,7 @@
> >  #define I2C_HID_PWR_SLEEP	0x01
> >  
> >  /* debug option */
> > -static bool debug = false;
> > +static bool debug;
> >  module_param(debug, bool, 0444);
> >  MODULE_PARM_DESC(debug, "print a lot of debug information");
> >  
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

[PATCH 05/14] HID: i2c-hid: fix i2c_hid_dbg macro

[Benjamin Tissoires]

This avoids the problematic case:

if (condition)
	i2c_hid_dbg(ihid, "Blah blah %d\n", i);
else
	do_something_very_important();

Which looks correct, however with the previous macro definition,
this expands to the unexpected:

if (condition) {
	if (debug)	\
		dev_printk(KERN_DEBUG, &ihid->client->dev,
				"Blah blah %d\n", i);
	else
		do_something_very_important();
}

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 9ee6555..54950be 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -50,9 +50,11 @@ static bool debug;
 module_param(debug, bool, 0444);
 MODULE_PARM_DESC(debug, "print a lot of debug information");
 
-#define i2c_hid_dbg(ihid, fmt, arg...)	\
-	if (debug)			\
-		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)
+#define i2c_hid_dbg(ihid, fmt, arg...)					  \
+do {									  \
+	if (debug)							  \
+		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
+} while (0)
 
 struct i2c_hid_desc {
 	__le16 wHIDDescLength;
-- 
1.8.0.1

Re: [PATCH 05/14] HID: i2c-hid: fix i2c_hid_dbg macro

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:46 +0100, Benjamin Tissoires wrote:
> This avoids the problematic case:
> 
> if (condition)
> 	i2c_hid_dbg(ihid, "Blah blah %d\n", i);
> else
> 	do_something_very_important();
> 
> Which looks correct, however with the previous macro definition,
> this expands to the unexpected:
> 
> if (condition) {
> 	if (debug)	\
> 		dev_printk(KERN_DEBUG, &ihid->client->dev,
> 				"Blah blah %d\n", i);
> 	else
> 		do_something_very_important();
> }
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 9ee6555..54950be 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -50,9 +50,11 @@ static bool debug;
>  module_param(debug, bool, 0444);
>  MODULE_PARM_DESC(debug, "print a lot of debug information");
>  
> -#define i2c_hid_dbg(ihid, fmt, arg...)	\
> -	if (debug)			\
> -		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)
> +#define i2c_hid_dbg(ihid, fmt, arg...)					  \
> +do {									  \
> +	if (debug)							  \
> +		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
> +} while (0)
>  
>  struct i2c_hid_desc {
>  	__le16 wHIDDescLength;

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 05/14] HID: i2c-hid: fix i2c_hid_dbg macro

[Jiri Kosina]

On Tue, 4 Dec 2012, Jean Delvare wrote:

> > This avoids the problematic case:
> > 
> > if (condition)
> > 	i2c_hid_dbg(ihid, "Blah blah %d\n", i);
> > else
> > 	do_something_very_important();
> > 
> > Which looks correct, however with the previous macro definition,
> > this expands to the unexpected:
> > 
> > if (condition) {
> > 	if (debug)	\
> > 		dev_printk(KERN_DEBUG, &ihid->client->dev,
> > 				"Blah blah %d\n", i);
> > 	else
> > 		do_something_very_important();
> > }
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 8 +++++---
> >  1 file changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 9ee6555..54950be 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -50,9 +50,11 @@ static bool debug;
> >  module_param(debug, bool, 0444);
> >  MODULE_PARM_DESC(debug, "print a lot of debug information");
> >  
> > -#define i2c_hid_dbg(ihid, fmt, arg...)	\
> > -	if (debug)			\
> > -		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)
> > +#define i2c_hid_dbg(ihid, fmt, arg...)					  \
> > +do {									  \
> > +	if (debug)							  \
> > +		dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg); \
> > +} while (0)
> >  
> >  struct i2c_hid_desc {
> >  	__le16 wHIDDescLength;
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Applied.

-- 
Jiri Kosina
SUSE Labs

[PATCH 06/14] HID: i2c-hid: remove unused static declarations

[Benjamin Tissoires]

These definitions are not used here, but are defined by the specification.
Keeping some of them for documentation purposes.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 25 ++++++++++---------------
 1 file changed, 10 insertions(+), 15 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 54950be..4452611 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -106,22 +106,17 @@ static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01),
 							  .wait = true };
 static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
 static const struct i2c_hid_cmd hid_set_report_cmd =	{ I2C_HID_CMD(0x03) };
-static const struct i2c_hid_cmd hid_get_idle_cmd =	{ I2C_HID_CMD(0x04) };
-static const struct i2c_hid_cmd hid_set_idle_cmd =	{ I2C_HID_CMD(0x05) };
-static const struct i2c_hid_cmd hid_get_protocol_cmd =	{ I2C_HID_CMD(0x06) };
-static const struct i2c_hid_cmd hid_set_protocol_cmd =	{ I2C_HID_CMD(0x07) };
 static const struct i2c_hid_cmd hid_set_power_cmd =	{ I2C_HID_CMD(0x08) };
-/* read/write data register */
-static const struct i2c_hid_cmd hid_data_cmd = {
-		.registerIndex = offsetof(struct i2c_hid_desc, wDataRegister),
-		.opcode = 0x00,
-		.length = 2 };
-/* write output reports */
-static const struct i2c_hid_cmd hid_out_cmd = {
-		.registerIndex = offsetof(struct i2c_hid_desc,
-			wOutputRegister),
-		.opcode = 0x00,
-		.length = 2 };
+
+/*
+ * These definitions are not used here, but are defined by the spec.
+ * Keeping them here for documentation purposes.
+ *
+ * static const struct i2c_hid_cmd hid_get_idle_cmd = { I2C_HID_CMD(0x04) };
+ * static const struct i2c_hid_cmd hid_set_idle_cmd = { I2C_HID_CMD(0x05) };
+ * static const struct i2c_hid_cmd hid_get_protocol_cmd = { I2C_HID_CMD(0x06) };
+ * static const struct i2c_hid_cmd hid_set_protocol_cmd = { I2C_HID_CMD(0x07) };
+ */
 
 /* The main device structure */
 struct i2c_hid {
-- 
1.8.0.1

Re: [PATCH 06/14] HID: i2c-hid: remove unused static declarations

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:47 +0100, Benjamin Tissoires wrote:
> These definitions are not used here, but are defined by the specification.
> Keeping some of them for documentation purposes.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 54950be..4452611 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -106,22 +106,17 @@ static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01),
>  							  .wait = true };
>  static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
>  static const struct i2c_hid_cmd hid_set_report_cmd =	{ I2C_HID_CMD(0x03) };
> -static const struct i2c_hid_cmd hid_get_idle_cmd =	{ I2C_HID_CMD(0x04) };
> -static const struct i2c_hid_cmd hid_set_idle_cmd =	{ I2C_HID_CMD(0x05) };
> -static const struct i2c_hid_cmd hid_get_protocol_cmd =	{ I2C_HID_CMD(0x06) };
> -static const struct i2c_hid_cmd hid_set_protocol_cmd =	{ I2C_HID_CMD(0x07) };
>  static const struct i2c_hid_cmd hid_set_power_cmd =	{ I2C_HID_CMD(0x08) };
> -/* read/write data register */
> -static const struct i2c_hid_cmd hid_data_cmd = {
> -		.registerIndex = offsetof(struct i2c_hid_desc, wDataRegister),
> -		.opcode = 0x00,
> -		.length = 2 };
> -/* write output reports */
> -static const struct i2c_hid_cmd hid_out_cmd = {
> -		.registerIndex = offsetof(struct i2c_hid_desc,
> -			wOutputRegister),
> -		.opcode = 0x00,
> -		.length = 2 };
> +
> +/*
> + * These definitions are not used here, but are defined by the spec.
> + * Keeping them here for documentation purposes.
> + *
> + * static const struct i2c_hid_cmd hid_get_idle_cmd = { I2C_HID_CMD(0x04) };
> + * static const struct i2c_hid_cmd hid_set_idle_cmd = { I2C_HID_CMD(0x05) };
> + * static const struct i2c_hid_cmd hid_get_protocol_cmd = { I2C_HID_CMD(0x06) };
> + * static const struct i2c_hid_cmd hid_set_protocol_cmd = { I2C_HID_CMD(0x07) };
> + */
>  
>  /* The main device structure */
>  struct i2c_hid {

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 06/14] HID: i2c-hid: remove unused static declarations

[Jiri Kosina]

On Tue, 4 Dec 2012, Jean Delvare wrote:

> > These definitions are not used here, but are defined by the specification.
> > Keeping some of them for documentation purposes.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 25 ++++++++++---------------
> >  1 file changed, 10 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 54950be..4452611 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -106,22 +106,17 @@ static const struct i2c_hid_cmd hid_reset_cmd =		{ I2C_HID_CMD(0x01),
> >  							  .wait = true };
> >  static const struct i2c_hid_cmd hid_get_report_cmd =	{ I2C_HID_CMD(0x02) };
> >  static const struct i2c_hid_cmd hid_set_report_cmd =	{ I2C_HID_CMD(0x03) };
> > -static const struct i2c_hid_cmd hid_get_idle_cmd =	{ I2C_HID_CMD(0x04) };
> > -static const struct i2c_hid_cmd hid_set_idle_cmd =	{ I2C_HID_CMD(0x05) };
> > -static const struct i2c_hid_cmd hid_get_protocol_cmd =	{ I2C_HID_CMD(0x06) };
> > -static const struct i2c_hid_cmd hid_set_protocol_cmd =	{ I2C_HID_CMD(0x07) };
> >  static const struct i2c_hid_cmd hid_set_power_cmd =	{ I2C_HID_CMD(0x08) };
> > -/* read/write data register */
> > -static const struct i2c_hid_cmd hid_data_cmd = {
> > -		.registerIndex = offsetof(struct i2c_hid_desc, wDataRegister),
> > -		.opcode = 0x00,
> > -		.length = 2 };
> > -/* write output reports */
> > -static const struct i2c_hid_cmd hid_out_cmd = {
> > -		.registerIndex = offsetof(struct i2c_hid_desc,
> > -			wOutputRegister),
> > -		.opcode = 0x00,
> > -		.length = 2 };
> > +
> > +/*
> > + * These definitions are not used here, but are defined by the spec.
> > + * Keeping them here for documentation purposes.
> > + *
> > + * static const struct i2c_hid_cmd hid_get_idle_cmd = { I2C_HID_CMD(0x04) };
> > + * static const struct i2c_hid_cmd hid_set_idle_cmd = { I2C_HID_CMD(0x05) };
> > + * static const struct i2c_hid_cmd hid_get_protocol_cmd = { I2C_HID_CMD(0x06) };
> > + * static const struct i2c_hid_cmd hid_set_protocol_cmd = { I2C_HID_CMD(0x07) };
> > + */
> >  
> >  /* The main device structure */
> >  struct i2c_hid {
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Applied.

-- 
Jiri Kosina
SUSE Labs

[PATCH 07/14] HID: i2c-hid: fix return paths

[Benjamin Tissoires]

Forwards appropriate return values.
As noone use the error returned by i2c_hid_get_input, let's make it
returning void.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 4452611..d6fdb3e 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -245,7 +245,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
 	if (ret) {
 		dev_err(&client->dev,
 			"failed to retrieve report from device.\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	return 0;
@@ -290,7 +290,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
 		reportType, args, args_len, NULL, 0);
 	if (ret) {
 		dev_err(&client->dev, "failed to set a report to device.\n");
-		return -EINVAL;
+		return ret;
 	}
 
 	return data_len;
@@ -334,7 +334,7 @@ static int i2c_hid_hwreset(struct i2c_client *client)
 	return 0;
 }
 
-static int i2c_hid_get_input(struct i2c_hid *ihid)
+static void i2c_hid_get_input(struct i2c_hid *ihid)
 {
 	int ret, ret_size;
 	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
@@ -342,11 +342,11 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
 	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
 	if (ret != size) {
 		if (ret < 0)
-			return ret;
+			return;
 
 		dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
 			__func__, ret, size);
-		return ret;
+		return;
 	}
 
 	ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
@@ -355,13 +355,13 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
 		/* host or device initiated RESET completed */
 		if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags))
 			wake_up(&ihid->wait);
-		return 0;
+		return;
 	}
 
 	if (ret_size > size) {
 		dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
 			__func__, size, ret_size);
-		return -EIO;
+		return;
 	}
 
 	i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
@@ -370,7 +370,7 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
 		hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
 				ret_size - 2, 1);
 
-	return 0;
+	return;
 }
 
 static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
@@ -430,8 +430,10 @@ static void i2c_hid_init_reports(struct hid_device *hid)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
 
-	if (!inbuf)
+	if (!inbuf) {
+		dev_err(&client->dev, "can not retrieve initial reports\n");
 		return;
+	}
 
 	list_for_each_entry(report,
 		&hid->report_enum[HID_INPUT_REPORT].report_list, list)
@@ -715,9 +717,7 @@ static int i2c_hid_hidinput_input_event(struct input_dev *dev,
 		return -1;
 	}
 
-	hid_set_field(field, offset, value);
-
-	return 0;
+	return hid_set_field(field, offset, value);
 }
 
 static struct hid_ll_driver i2c_hid_ll_driver = {
-- 
1.8.0.1

Re: [PATCH 07/14] HID: i2c-hid: fix return paths

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:48 +0100, Benjamin Tissoires wrote:
> Forwards appropriate return values.
> As noone use the error returned by i2c_hid_get_input, let's make it
> returning void.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 4452611..d6fdb3e 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -245,7 +245,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
>  	if (ret) {
>  		dev_err(&client->dev,
>  			"failed to retrieve report from device.\n");
> -		return -EINVAL;
> +		return ret;
>  	}
>  
>  	return 0;
> @@ -290,7 +290,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>  		reportType, args, args_len, NULL, 0);
>  	if (ret) {
>  		dev_err(&client->dev, "failed to set a report to device.\n");
> -		return -EINVAL;
> +		return ret;
>  	}
>  
>  	return data_len;
> @@ -334,7 +334,7 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static int i2c_hid_get_input(struct i2c_hid *ihid)
> +static void i2c_hid_get_input(struct i2c_hid *ihid)
>  {
>  	int ret, ret_size;
>  	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
> @@ -342,11 +342,11 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
>  	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
>  	if (ret != size) {
>  		if (ret < 0)
> -			return ret;
> +			return;
>  
>  		dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
>  			__func__, ret, size);
> -		return ret;
> +		return;
>  	}
>  
>  	ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
> @@ -355,13 +355,13 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
>  		/* host or device initiated RESET completed */
>  		if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags))
>  			wake_up(&ihid->wait);
> -		return 0;
> +		return;
>  	}
>  
>  	if (ret_size > size) {
>  		dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
>  			__func__, size, ret_size);
> -		return -EIO;
> +		return;
>  	}
>  
>  	i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
> @@ -370,7 +370,7 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
>  		hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
>  				ret_size - 2, 1);
>  
> -	return 0;
> +	return;
>  }
>  
>  static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
> @@ -430,8 +430,10 @@ static void i2c_hid_init_reports(struct hid_device *hid)
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
>  
> -	if (!inbuf)
> +	if (!inbuf) {
> +		dev_err(&client->dev, "can not retrieve initial reports\n");
>  		return;
> +	}

This change would have been better located in patch 07/14 IMHO, but
that's a detail.

>  
>  	list_for_each_entry(report,
>  		&hid->report_enum[HID_INPUT_REPORT].report_list, list)
> @@ -715,9 +717,7 @@ static int i2c_hid_hidinput_input_event(struct input_dev *dev,
>  		return -1;
>  	}
>  
> -	hid_set_field(field, offset, value);
> -
> -	return 0;
> +	return hid_set_field(field, offset, value);
>  }
>  
>  static struct hid_ll_driver i2c_hid_ll_driver = {

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 07/14] HID: i2c-hid: fix return paths

[Jiri Kosina]

On Tue, 4 Dec 2012, Benjamin Tissoires wrote:

> Forwards appropriate return values.
> As noone use the error returned by i2c_hid_get_input, let's make it
> returning void.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Applied.

> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 4452611..d6fdb3e 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -245,7 +245,7 @@ static int i2c_hid_get_report(struct i2c_client *client, u8 reportType,
>  	if (ret) {
>  		dev_err(&client->dev,
>  			"failed to retrieve report from device.\n");
> -		return -EINVAL;
> +		return ret;
>  	}
>  
>  	return 0;
> @@ -290,7 +290,7 @@ static int i2c_hid_set_report(struct i2c_client *client, u8 reportType,
>  		reportType, args, args_len, NULL, 0);
>  	if (ret) {
>  		dev_err(&client->dev, "failed to set a report to device.\n");
> -		return -EINVAL;
> +		return ret;
>  	}
>  
>  	return data_len;
> @@ -334,7 +334,7 @@ static int i2c_hid_hwreset(struct i2c_client *client)
>  	return 0;
>  }
>  
> -static int i2c_hid_get_input(struct i2c_hid *ihid)
> +static void i2c_hid_get_input(struct i2c_hid *ihid)
>  {
>  	int ret, ret_size;
>  	int size = le16_to_cpu(ihid->hdesc.wMaxInputLength);
> @@ -342,11 +342,11 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
>  	ret = i2c_master_recv(ihid->client, ihid->inbuf, size);
>  	if (ret != size) {
>  		if (ret < 0)
> -			return ret;
> +			return;
>  
>  		dev_err(&ihid->client->dev, "%s: got %d data instead of %d\n",
>  			__func__, ret, size);
> -		return ret;
> +		return;
>  	}
>  
>  	ret_size = ihid->inbuf[0] | ihid->inbuf[1] << 8;
> @@ -355,13 +355,13 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
>  		/* host or device initiated RESET completed */
>  		if (test_and_clear_bit(I2C_HID_RESET_PENDING, &ihid->flags))
>  			wake_up(&ihid->wait);
> -		return 0;
> +		return;
>  	}
>  
>  	if (ret_size > size) {
>  		dev_err(&ihid->client->dev, "%s: incomplete report (%d/%d)\n",
>  			__func__, size, ret_size);
> -		return -EIO;
> +		return;
>  	}
>  
>  	i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
> @@ -370,7 +370,7 @@ static int i2c_hid_get_input(struct i2c_hid *ihid)
>  		hid_input_report(ihid->hid, HID_INPUT_REPORT, ihid->inbuf + 2,
>  				ret_size - 2, 1);
>  
> -	return 0;
> +	return;
>  }
>  
>  static irqreturn_t i2c_hid_irq(int irq, void *dev_id)
> @@ -430,8 +430,10 @@ static void i2c_hid_init_reports(struct hid_device *hid)
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	u8 *inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
>  
> -	if (!inbuf)
> +	if (!inbuf) {
> +		dev_err(&client->dev, "can not retrieve initial reports\n");
>  		return;
> +	}
>  
>  	list_for_each_entry(report,
>  		&hid->report_enum[HID_INPUT_REPORT].report_list, list)
> @@ -715,9 +717,7 @@ static int i2c_hid_hidinput_input_event(struct input_dev *dev,
>  		return -1;
>  	}
>  
> -	hid_set_field(field, offset, value);
> -
> -	return 0;
> +	return hid_set_field(field, offset, value);
>  }
>  
>  static struct hid_ll_driver i2c_hid_ll_driver = {
> -- 
> 1.8.0.1
> 

-- 
Jiri Kosina
SUSE Labs

[PATCH 08/14] HID: i2c-hid: fix error messages

[Benjamin Tissoires]

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index d6fdb3e..da04948 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -741,10 +741,10 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
 			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
 			client->name, ihid);
 	if (ret < 0) {
-		dev_dbg(&client->dev,
+		dev_warn(&client->dev,
 			"Could not register for %s interrupt, irq = %d,"
 			" ret = %d\n",
-		client->name, client->irq, ret);
+			client->name, client->irq, ret);
 
 		return ret;
 	}
@@ -770,7 +770,8 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 			__func__, 4, ihid->hdesc_buffer);
 
 	if (ret) {
-		dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n",
+		dev_err(&client->dev,
+			"unable to fetch the size of HID descriptor (ret=%d)\n",
 			ret);
 		return -ENODEV;
 	}
@@ -785,7 +786,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
 	/* check bcdVersion == 1.0 */
 	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
 		dev_err(&client->dev,
-			"unexpected HID descriptor bcdVersion (0x%04x)\n",
+			"unexpected HID descriptor bcdVersion (0x%04hx)\n",
 			le16_to_cpu(hdesc->bcdVersion));
 		return -ENODEV;
 	}
@@ -871,7 +872,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
 	hid->product = le16_to_cpu(ihid->hdesc.wProductID);
 
-	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
+	snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
 		 client->name, hid->vendor, hid->product);
 
 	ret = hid_add_device(hid);
-- 
1.8.0.1

Re: [PATCH 08/14] HID: i2c-hid: fix error messages

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:49 +0100, Benjamin Tissoires wrote:
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index d6fdb3e..da04948 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -741,10 +741,10 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
>  			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>  			client->name, ihid);
>  	if (ret < 0) {
> -		dev_dbg(&client->dev,
> +		dev_warn(&client->dev,
>  			"Could not register for %s interrupt, irq = %d,"
>  			" ret = %d\n",
> -		client->name, client->irq, ret);
> +			client->name, client->irq, ret);
>  
>  		return ret;
>  	}
> @@ -770,7 +770,8 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>  			__func__, 4, ihid->hdesc_buffer);
>  
>  	if (ret) {
> -		dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n",
> +		dev_err(&client->dev,
> +			"unable to fetch the size of HID descriptor (ret=%d)\n",
>  			ret);
>  		return -ENODEV;
>  	}
> @@ -785,7 +786,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
>  	/* check bcdVersion == 1.0 */
>  	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
>  		dev_err(&client->dev,
> -			"unexpected HID descriptor bcdVersion (0x%04x)\n",
> +			"unexpected HID descriptor bcdVersion (0x%04hx)\n",
>  			le16_to_cpu(hdesc->bcdVersion));
>  		return -ENODEV;
>  	}
> @@ -871,7 +872,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
>  	hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
>  	hid->product = le16_to_cpu(ihid->hdesc.wProductID);
>  
> -	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
> +	snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
>  		 client->name, hid->vendor, hid->product);
>  
>  	ret = hid_add_device(hid);

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 08/14] HID: i2c-hid: fix error messages

[Jiri Kosina]

On Wed, 5 Dec 2012, Jean Delvare wrote:

> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 11 ++++++-----
> >  1 file changed, 6 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index d6fdb3e..da04948 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -741,10 +741,10 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
> >  			IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> >  			client->name, ihid);
> >  	if (ret < 0) {
> > -		dev_dbg(&client->dev,
> > +		dev_warn(&client->dev,
> >  			"Could not register for %s interrupt, irq = %d,"
> >  			" ret = %d\n",
> > -		client->name, client->irq, ret);
> > +			client->name, client->irq, ret);
> >  
> >  		return ret;
> >  	}
> > @@ -770,7 +770,8 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> >  			__func__, 4, ihid->hdesc_buffer);
> >  
> >  	if (ret) {
> > -		dev_err(&client->dev, "HID_DESCR_LENGTH_CMD Fail (ret=%d)\n",
> > +		dev_err(&client->dev,
> > +			"unable to fetch the size of HID descriptor (ret=%d)\n",
> >  			ret);
> >  		return -ENODEV;
> >  	}
> > @@ -785,7 +786,7 @@ static int __devinit i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> >  	/* check bcdVersion == 1.0 */
> >  	if (le16_to_cpu(hdesc->bcdVersion) != 0x0100) {
> >  		dev_err(&client->dev,
> > -			"unexpected HID descriptor bcdVersion (0x%04x)\n",
> > +			"unexpected HID descriptor bcdVersion (0x%04hx)\n",
> >  			le16_to_cpu(hdesc->bcdVersion));
> >  		return -ENODEV;
> >  	}
> > @@ -871,7 +872,7 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
> >  	hid->vendor = le16_to_cpu(ihid->hdesc.wVendorID);
> >  	hid->product = le16_to_cpu(ihid->hdesc.wProductID);
> >  
> > -	snprintf(hid->name, sizeof(hid->name), "%s %04X:%04X",
> > +	snprintf(hid->name, sizeof(hid->name), "%s %04hX:%04hX",
> >  		 client->name, hid->vendor, hid->product);
> >  
> >  	ret = hid_add_device(hid);
> 
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Applied.

-- 
Jiri Kosina
SUSE Labs

[PATCH 09/14] HID: i2c-hid: i2c_hid_get_report may fail

[Benjamin Tissoires]

If i2c_hid_get_report fails, exit i2c_hid_init_report.
The printk log is already called by i2c_hid_get_report, so no need
to add some more printks.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index da04948..dcacfc4 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -400,9 +400,10 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
 	unsigned int size, ret_size;
 
 	size = i2c_hid_get_report_length(report);
-	i2c_hid_get_report(client,
+	if (i2c_hid_get_report(client,
 			report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
-			report->id, buffer, size);
+			report->id, buffer, size))
+		return;
 
 	i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
 
-- 
1.8.0.1

Re: [PATCH 09/14] HID: i2c-hid: i2c_hid_get_report may fail

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:50 +0100, Benjamin Tissoires wrote:
> If i2c_hid_get_report fails, exit i2c_hid_init_report.
> The printk log is already called by i2c_hid_get_report, so no need
> to add some more printks.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index da04948..dcacfc4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -400,9 +400,10 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
>  	unsigned int size, ret_size;
>  
>  	size = i2c_hid_get_report_length(report);
> -	i2c_hid_get_report(client,
> +	if (i2c_hid_get_report(client,
>  			report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> -			report->id, buffer, size);
> +			report->id, buffer, size))
> +		return;
>  
>  	i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
>  

OK, although I don't quite get the rationale for not reporting the
errors from i2c_hid_init_report() and i2c_hid_init_reports() to their
respective callers. Does the device have any chance to work properly if
i2c_hid_init_reports() fails?

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 09/14] HID: i2c-hid: i2c_hid_get_report may fail

[Benjamin Tissoires]

On Wed, Dec 5, 2012 at 10:59 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue,  4 Dec 2012 16:27:50 +0100, Benjamin Tissoires wrote:
>> If i2c_hid_get_report fails, exit i2c_hid_init_report.
>> The printk log is already called by i2c_hid_get_report, so no need
>> to add some more printks.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/i2c-hid/i2c-hid.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index da04948..dcacfc4 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -400,9 +400,10 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
>>       unsigned int size, ret_size;
>>
>>       size = i2c_hid_get_report_length(report);
>> -     i2c_hid_get_report(client,
>> +     if (i2c_hid_get_report(client,
>>                       report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
>> -                     report->id, buffer, size);
>> +                     report->id, buffer, size))
>> +             return;
>>
>>       i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);
>>
>
> OK, although I don't quite get the rationale for not reporting the
> errors from i2c_hid_init_report() and i2c_hid_init_reports() to their
> respective callers. Does the device have any chance to work properly if
> i2c_hid_init_reports() fails?

Hi Jean,

first thanks for the review you have started.

For your question, the answer is yes, the device works properly even
if i2c_hid_init_reports().
IIRC, the only required steps are:
- get HID descriptor
- get HID report descriptors
- send reset
- set power on

 i2c_hid_init_reports is not part of the specification for the boot
process, and the Windows driver does not retrieve the reports for
input reports at all (it does it though for the features).
Actually, the device I have does not implement get_report for inputs
(it returns 0), but it's not a problem for it to work.

I put the whole init_reports in place to get the features values
before sending them to the hid driver, and also to copy the behavior
of usbhid.

Cheers,
Benjamin

>
> Reviewed-by: Jean Delvare <khali@linux-fr.org>
>
> --
> Jean Delvare

Re: [PATCH 09/14] HID: i2c-hid: i2c_hid_get_report may fail

[Jiri Kosina]

On Tue, 4 Dec 2012, Benjamin Tissoires wrote:

> If i2c_hid_get_report fails, exit i2c_hid_init_report.
> The printk log is already called by i2c_hid_get_report, so no need
> to add some more printks.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index da04948..dcacfc4 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -400,9 +400,10 @@ static void i2c_hid_init_report(struct hid_report *report, u8 *buffer,
>  	unsigned int size, ret_size;
>  
>  	size = i2c_hid_get_report_length(report);
> -	i2c_hid_get_report(client,
> +	if (i2c_hid_get_report(client,
>  			report->type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> -			report->id, buffer, size);
> +			report->id, buffer, size))
> +		return;
>  
>  	i2c_hid_dbg(ihid, "report (len=%d): %*ph\n", size, size, ihid->inbuf);

Applied.

-- 
Jiri Kosina
SUSE Labs

[PATCH 10/14] HID: i2c-hid: reorder allocation/free of buffers

[Benjamin Tissoires]

Simplifies i2c_hid_alloc_buffers tests, and makes this function
responsible of the assignment of ihid->bufsize.
The condition for the reallocation in i2c_hid_start is then simpler.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 68 ++++++++++++++++++-------------------------
 1 file changed, 28 insertions(+), 40 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index dcacfc4..4e3f80b 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -465,48 +465,38 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
 	}
 }
 
-static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
+static void i2c_hid_free_buffers(struct i2c_hid *ihid)
+{
+	kfree(ihid->inbuf);
+	kfree(ihid->argsbuf);
+	kfree(ihid->cmdbuf);
+	ihid->inbuf = NULL;
+	ihid->cmdbuf = NULL;
+	ihid->argsbuf = NULL;
+	ihid->bufsize = 0;
+}
+
+static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
 {
 	/* the worst case is computed from the set_report command with a
 	 * reportID > 15 and the maximum report length */
 	int args_len = sizeof(__u8) + /* optional ReportID byte */
 		       sizeof(__u16) + /* data register */
 		       sizeof(__u16) + /* size of the report */
-		       ihid->bufsize; /* report */
-
-	ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
-
-	if (!ihid->inbuf)
-		return -ENOMEM;
+		       report_size; /* report */
 
+	ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
 	ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
-
-	if (!ihid->argsbuf) {
-		kfree(ihid->inbuf);
-		return -ENOMEM;
-	}
-
 	ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
 
-	if (!ihid->cmdbuf) {
-		kfree(ihid->inbuf);
-		kfree(ihid->argsbuf);
-		ihid->inbuf = NULL;
-		ihid->argsbuf = NULL;
+	if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
+		i2c_hid_free_buffers(ihid);
 		return -ENOMEM;
 	}
 
-	return 0;
-}
+	ihid->bufsize = report_size;
 
-static void i2c_hid_free_buffers(struct i2c_hid *ihid)
-{
-	kfree(ihid->inbuf);
-	kfree(ihid->argsbuf);
-	kfree(ihid->cmdbuf);
-	ihid->inbuf = NULL;
-	ihid->cmdbuf = NULL;
-	ihid->argsbuf = NULL;
+	return 0;
 }
 
 static int i2c_hid_get_raw_report(struct hid_device *hid,
@@ -611,22 +601,19 @@ static int i2c_hid_start(struct hid_device *hid)
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	int ret;
-	int old_bufsize = ihid->bufsize;
+	unsigned int bufsize = HID_MIN_BUFFER_SIZE;
 
-	ihid->bufsize = HID_MIN_BUFFER_SIZE;
-	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &ihid->bufsize);
-	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &ihid->bufsize);
-	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &ihid->bufsize);
+	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
+	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
+	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
 
-	if (ihid->bufsize > old_bufsize || !ihid->inbuf || !ihid->cmdbuf) {
+	if (bufsize > ihid->bufsize) {
 		i2c_hid_free_buffers(ihid);
 
-		ret = i2c_hid_alloc_buffers(ihid);
+		ret = i2c_hid_alloc_buffers(ihid, bufsize);
 
-		if (ret) {
-			ihid->bufsize = old_bufsize;
+		if (ret)
 			return ret;
-		}
 	}
 
 	if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
@@ -844,8 +831,9 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 	/* we need to allocate the command buffer without knowing the maximum
 	 * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the
 	 * real computation later. */
-	ihid->bufsize = HID_MIN_BUFFER_SIZE;
-	i2c_hid_alloc_buffers(ihid);
+	ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
+	if (ret < 0)
+		goto err;
 
 	ret = i2c_hid_fetch_hid_descriptor(ihid);
 	if (ret < 0)
-- 
1.8.0.1

Re: [PATCH 10/14] HID: i2c-hid: reorder allocation/free of buffers

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:51 +0100, Benjamin Tissoires wrote:
> Simplifies i2c_hid_alloc_buffers tests, and makes this function
> responsible of the assignment of ihid->bufsize.
> The condition for the reallocation in i2c_hid_start is then simpler.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 68 ++++++++++++++++++-------------------------
>  1 file changed, 28 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index dcacfc4..4e3f80b 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -465,48 +465,38 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
>  	}
>  }
>  
> -static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
> +static void i2c_hid_free_buffers(struct i2c_hid *ihid)
> +{
> +	kfree(ihid->inbuf);
> +	kfree(ihid->argsbuf);
> +	kfree(ihid->cmdbuf);
> +	ihid->inbuf = NULL;
> +	ihid->cmdbuf = NULL;
> +	ihid->argsbuf = NULL;
> +	ihid->bufsize = 0;
> +}
> +
> +static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
>  {
>  	/* the worst case is computed from the set_report command with a
>  	 * reportID > 15 and the maximum report length */
>  	int args_len = sizeof(__u8) + /* optional ReportID byte */
>  		       sizeof(__u16) + /* data register */
>  		       sizeof(__u16) + /* size of the report */
> -		       ihid->bufsize; /* report */
> -
> -	ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
> -
> -	if (!ihid->inbuf)
> -		return -ENOMEM;
> +		       report_size; /* report */
>  
> +	ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
>  	ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
> -
> -	if (!ihid->argsbuf) {
> -		kfree(ihid->inbuf);
> -		return -ENOMEM;
> -	}
> -
>  	ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
>  
> -	if (!ihid->cmdbuf) {
> -		kfree(ihid->inbuf);
> -		kfree(ihid->argsbuf);
> -		ihid->inbuf = NULL;
> -		ihid->argsbuf = NULL;
> +	if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
> +		i2c_hid_free_buffers(ihid);
>  		return -ENOMEM;
>  	}
>  
> -	return 0;
> -}
> +	ihid->bufsize = report_size;
>  
> -static void i2c_hid_free_buffers(struct i2c_hid *ihid)
> -{
> -	kfree(ihid->inbuf);
> -	kfree(ihid->argsbuf);
> -	kfree(ihid->cmdbuf);
> -	ihid->inbuf = NULL;
> -	ihid->cmdbuf = NULL;
> -	ihid->argsbuf = NULL;
> +	return 0;
>  }
>  
>  static int i2c_hid_get_raw_report(struct hid_device *hid,
> @@ -611,22 +601,19 @@ static int i2c_hid_start(struct hid_device *hid)
>  	struct i2c_client *client = hid->driver_data;
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	int ret;
> -	int old_bufsize = ihid->bufsize;
> +	unsigned int bufsize = HID_MIN_BUFFER_SIZE;
>  
> -	ihid->bufsize = HID_MIN_BUFFER_SIZE;
> -	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &ihid->bufsize);
> -	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &ihid->bufsize);
> -	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &ihid->bufsize);
> +	i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
> +	i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
> +	i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
>  
> -	if (ihid->bufsize > old_bufsize || !ihid->inbuf || !ihid->cmdbuf) {
> +	if (bufsize > ihid->bufsize) {
>  		i2c_hid_free_buffers(ihid);
>  
> -		ret = i2c_hid_alloc_buffers(ihid);
> +		ret = i2c_hid_alloc_buffers(ihid, bufsize);
>  
> -		if (ret) {
> -			ihid->bufsize = old_bufsize;
> +		if (ret)
>  			return ret;
> -		}
>  	}
>  
>  	if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
> @@ -844,8 +831,9 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
>  	/* we need to allocate the command buffer without knowing the maximum
>  	 * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the
>  	 * real computation later. */
> -	ihid->bufsize = HID_MIN_BUFFER_SIZE;
> -	i2c_hid_alloc_buffers(ihid);
> +	ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
> +	if (ret < 0)
> +		goto err;
>  
>  	ret = i2c_hid_fetch_hid_descriptor(ihid);
>  	if (ret < 0)

Yes, looks much better.

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

Re: [PATCH 10/14] HID: i2c-hid: reorder allocation/free of buffers

[Benjamin Tissoires]

On Wed, Dec 5, 2012 at 11:10 AM, Jean Delvare <khali@linux-fr.org> wrote:
> On Tue,  4 Dec 2012 16:27:51 +0100, Benjamin Tissoires wrote:
>> Simplifies i2c_hid_alloc_buffers tests, and makes this function
>> responsible of the assignment of ihid->bufsize.
>> The condition for the reallocation in i2c_hid_start is then simpler.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
>> ---
>>  drivers/hid/i2c-hid/i2c-hid.c | 68 ++++++++++++++++++-------------------------
>>  1 file changed, 28 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
>> index dcacfc4..4e3f80b 100644
>> --- a/drivers/hid/i2c-hid/i2c-hid.c
>> +++ b/drivers/hid/i2c-hid/i2c-hid.c
>> @@ -465,48 +465,38 @@ static void i2c_hid_find_max_report(struct hid_device *hid, unsigned int type,
>>       }
>>  }
>>
>> -static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
>> +static void i2c_hid_free_buffers(struct i2c_hid *ihid)
>> +{
>> +     kfree(ihid->inbuf);
>> +     kfree(ihid->argsbuf);
>> +     kfree(ihid->cmdbuf);
>> +     ihid->inbuf = NULL;
>> +     ihid->cmdbuf = NULL;
>> +     ihid->argsbuf = NULL;
>> +     ihid->bufsize = 0;
>> +}
>> +
>> +static int i2c_hid_alloc_buffers(struct i2c_hid *ihid, size_t report_size)
>>  {
>>       /* the worst case is computed from the set_report command with a
>>        * reportID > 15 and the maximum report length */
>>       int args_len = sizeof(__u8) + /* optional ReportID byte */
>>                      sizeof(__u16) + /* data register */
>>                      sizeof(__u16) + /* size of the report */
>> -                    ihid->bufsize; /* report */
>> -
>> -     ihid->inbuf = kzalloc(ihid->bufsize, GFP_KERNEL);
>> -
>> -     if (!ihid->inbuf)
>> -             return -ENOMEM;
>> +                    report_size; /* report */
>>
>> +     ihid->inbuf = kzalloc(report_size, GFP_KERNEL);
>>       ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
>> -
>> -     if (!ihid->argsbuf) {
>> -             kfree(ihid->inbuf);
>> -             return -ENOMEM;
>> -     }
>> -
>>       ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
>>
>> -     if (!ihid->cmdbuf) {
>> -             kfree(ihid->inbuf);
>> -             kfree(ihid->argsbuf);
>> -             ihid->inbuf = NULL;
>> -             ihid->argsbuf = NULL;
>> +     if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
>> +             i2c_hid_free_buffers(ihid);
>>               return -ENOMEM;
>>       }
>>
>> -     return 0;
>> -}
>> +     ihid->bufsize = report_size;
>>
>> -static void i2c_hid_free_buffers(struct i2c_hid *ihid)
>> -{
>> -     kfree(ihid->inbuf);
>> -     kfree(ihid->argsbuf);
>> -     kfree(ihid->cmdbuf);
>> -     ihid->inbuf = NULL;
>> -     ihid->cmdbuf = NULL;
>> -     ihid->argsbuf = NULL;
>> +     return 0;
>>  }
>>
>>  static int i2c_hid_get_raw_report(struct hid_device *hid,
>> @@ -611,22 +601,19 @@ static int i2c_hid_start(struct hid_device *hid)
>>       struct i2c_client *client = hid->driver_data;
>>       struct i2c_hid *ihid = i2c_get_clientdata(client);
>>       int ret;
>> -     int old_bufsize = ihid->bufsize;
>> +     unsigned int bufsize = HID_MIN_BUFFER_SIZE;
>>
>> -     ihid->bufsize = HID_MIN_BUFFER_SIZE;
>> -     i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &ihid->bufsize);
>> -     i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &ihid->bufsize);
>> -     i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &ihid->bufsize);
>> +     i2c_hid_find_max_report(hid, HID_INPUT_REPORT, &bufsize);
>> +     i2c_hid_find_max_report(hid, HID_OUTPUT_REPORT, &bufsize);
>> +     i2c_hid_find_max_report(hid, HID_FEATURE_REPORT, &bufsize);
>>
>> -     if (ihid->bufsize > old_bufsize || !ihid->inbuf || !ihid->cmdbuf) {
>> +     if (bufsize > ihid->bufsize) {
>>               i2c_hid_free_buffers(ihid);
>>
>> -             ret = i2c_hid_alloc_buffers(ihid);
>> +             ret = i2c_hid_alloc_buffers(ihid, bufsize);
>>
>> -             if (ret) {
>> -                     ihid->bufsize = old_bufsize;
>> +             if (ret)
>>                       return ret;
>> -             }
>>       }
>>
>>       if (!(hid->quirks & HID_QUIRK_NO_INIT_REPORTS))
>> @@ -844,8 +831,9 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
>>       /* we need to allocate the command buffer without knowing the maximum
>>        * size of the reports. Let's use HID_MIN_BUFFER_SIZE, then we do the
>>        * real computation later. */
>> -     ihid->bufsize = HID_MIN_BUFFER_SIZE;
>> -     i2c_hid_alloc_buffers(ihid);
>> +     ret = i2c_hid_alloc_buffers(ihid, HID_MIN_BUFFER_SIZE);
>> +     if (ret < 0)
>> +             goto err;
>>
>>       ret = i2c_hid_fetch_hid_descriptor(ihid);
>>       if (ret < 0)
>
> Yes, looks much better.
>
> Reviewed-by: Jean Delvare <khali@linux-fr.org>

Thanks Jean,

as mentioned in 2/14, this patch need the fix in 2/14 to be able to
work. Jiri, I'll try to send a v2 ASAP so that you can pick both in
the right order (otherwise, the bisect may fall between the two).

Cheers,
Benjamin

>
> --
> Jean Delvare

[PATCH 11/14] HID: i2c-hid: remove unneeded test in i2c_hid_remove

[Benjamin Tissoires]

ihid can not be null, so there are no reasons to test it.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 4e3f80b..2b49929 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -890,9 +890,6 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
 	struct hid_device *hid;
 
-	if (WARN_ON(!ihid))
-		return -1;
-
 	hid = ihid->hid;
 	hid_destroy_device(hid);
 
-- 
1.8.0.1

Re: [PATCH 11/14] HID: i2c-hid: remove unneeded test in i2c_hid_remove

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:52 +0100, Benjamin Tissoires wrote:
> ihid can not be null, so there are no reasons to test it.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 3 ---
>  1 file changed, 3 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 4e3f80b..2b49929 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -890,9 +890,6 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
>  	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  	struct hid_device *hid;
>  
> -	if (WARN_ON(!ihid))
> -		return -1;
> -
>  	hid = ihid->hid;
>  	hid_destroy_device(hid);
>  

This leaves <linux/bug.h> included for no good reason. Please remove it
too.

-- 
Jean Delvare

Re: [PATCH 11/14] HID: i2c-hid: remove unneeded test in i2c_hid_remove

[Jiri Kosina]

On Wed, 5 Dec 2012, Jean Delvare wrote:

> > ihid can not be null, so there are no reasons to test it.
> > 
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> > ---
> >  drivers/hid/i2c-hid/i2c-hid.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> > index 4e3f80b..2b49929 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid.c
> > @@ -890,9 +890,6 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
> >  	struct i2c_hid *ihid = i2c_get_clientdata(client);
> >  	struct hid_device *hid;
> >  
> > -	if (WARN_ON(!ihid))
> > -		return -1;
> > -
> >  	hid = ihid->hid;
> >  	hid_destroy_device(hid);
> >  
> 
> This leaves <linux/bug.h> included for no good reason. Please remove it
> too.

I have applied the patch, and removed the superfluous include as well, 
thanks for spotting that, Jean.

-- 
Jiri Kosina
SUSE Labs

[PATCH 12/14] HID: i2c-hid: remove extra .irq field in struct i2c_hid

[Benjamin Tissoires]

There is no point in keeping the irq in i2c_hid as it's already
there in client.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 2b49929..bea4b13 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -136,8 +136,6 @@ struct i2c_hid {
 
 	unsigned long		flags;		/* device flags */
 
-	int			irq;		/* the interrupt line irq */
-
 	wait_queue_head_t	wait;		/* For waiting the interrupt */
 };
 
@@ -737,8 +735,6 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
 		return ret;
 	}
 
-	ihid->irq = client->irq;
-
 	return 0;
 }
 
@@ -841,12 +837,12 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 
 	ret = i2c_hid_init_irq(client);
 	if (ret < 0)
-		goto err;
+		goto err_irq;
 
 	hid = hid_allocate_device();
 	if (IS_ERR(hid)) {
 		ret = PTR_ERR(hid);
-		goto err;
+		goto err_irq;
 	}
 
 	ihid->hid = hid;
@@ -876,10 +872,10 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
 err_mem_free:
 	hid_destroy_device(hid);
 
-err:
-	if (ihid->irq)
-		free_irq(ihid->irq, ihid);
+err_irq:
+	free_irq(client->irq, ihid);
 
+err:
 	i2c_hid_free_buffers(ihid);
 	kfree(ihid);
 	return ret;
@@ -904,10 +900,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
 static int i2c_hid_suspend(struct device *dev)
 {
 	struct i2c_client *client = to_i2c_client(dev);
-	struct i2c_hid *ihid = i2c_get_clientdata(client);
 
 	if (device_may_wakeup(&client->dev))
-		enable_irq_wake(ihid->irq);
+		enable_irq_wake(client->irq);
 
 	/* Save some power */
 	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);
-- 
1.8.0.1

Re: [PATCH 12/14] HID: i2c-hid: remove extra .irq field in struct i2c_hid

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:53 +0100, Benjamin Tissoires wrote:
> There is no point in keeping the irq in i2c_hid as it's already
> there in client.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 17 ++++++-----------
>  1 file changed, 6 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 2b49929..bea4b13 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -136,8 +136,6 @@ struct i2c_hid {
>  
>  	unsigned long		flags;		/* device flags */
>  
> -	int			irq;		/* the interrupt line irq */
> -
>  	wait_queue_head_t	wait;		/* For waiting the interrupt */
>  };
>  
> @@ -737,8 +735,6 @@ static int __devinit i2c_hid_init_irq(struct i2c_client *client)
>  		return ret;
>  	}
>  
> -	ihid->irq = client->irq;
> -
>  	return 0;
>  }
>  
> @@ -841,12 +837,12 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
>  
>  	ret = i2c_hid_init_irq(client);
>  	if (ret < 0)
> -		goto err;
> +		goto err_irq;

Looks wrong. If i2c_hid_init_irq() failed, the irq couldn't be requested so you don't want to free it.

>  
>  	hid = hid_allocate_device();
>  	if (IS_ERR(hid)) {
>  		ret = PTR_ERR(hid);
> -		goto err;
> +		goto err_irq;
>  	}
>  
>  	ihid->hid = hid;
> @@ -876,10 +872,10 @@ static int __devinit i2c_hid_probe(struct i2c_client *client,
>  err_mem_free:
>  	hid_destroy_device(hid);
>  
> -err:
> -	if (ihid->irq)
> -		free_irq(ihid->irq, ihid);
> +err_irq:
> +	free_irq(client->irq, ihid);
>  
> +err:
>  	i2c_hid_free_buffers(ihid);
>  	kfree(ihid);
>  	return ret;
> @@ -904,10 +900,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
>  static int i2c_hid_suspend(struct device *dev)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> -	struct i2c_hid *ihid = i2c_get_clientdata(client);
>  
>  	if (device_may_wakeup(&client->dev))
> -		enable_irq_wake(ihid->irq);
> +		enable_irq_wake(client->irq);
>  
>  	/* Save some power */
>  	i2c_hid_set_power(client, I2C_HID_PWR_SLEEP);


-- 
Jean Delvare

[PATCH 13/14] HID: i2c-hid: also call i2c_hid_free_buffers in i2c_hid_remove

[Benjamin Tissoires]

In the case where the hid driver in charge of handling the hid part
of the device (hid-generic for instance) fails at probe, neither
i2c_hid_start nor i2c_hid_stop are called.
Thus, the buffers allocated in i2c_hid_probe are never freed.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index bea4b13..62988f1 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -891,6 +891,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
 
 	free_irq(client->irq, ihid);
 
+	if (ihid->bufsize)
+		i2c_hid_free_buffers(ihid);
+
 	kfree(ihid);
 
 	return 0;
-- 
1.8.0.1

Re: [PATCH 13/14] HID: i2c-hid: also call i2c_hid_free_buffers in i2c_hid_remove

[Jiri Kosina]

On Tue, 4 Dec 2012, Benjamin Tissoires wrote:

> In the case where the hid driver in charge of handling the hid part
> of the device (hid-generic for instance) fails at probe, neither
> i2c_hid_start nor i2c_hid_stop are called.
> Thus, the buffers allocated in i2c_hid_probe are never freed.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index bea4b13..62988f1 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -891,6 +891,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
>  
>  	free_irq(client->irq, ihid);
>  
> +	if (ihid->bufsize)
> +		i2c_hid_free_buffers(ihid);
> +
>  	kfree(ihid);

Applied.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH 13/14] HID: i2c-hid: also call i2c_hid_free_buffers in i2c_hid_remove

[Jean Delvare]

On Tue,  4 Dec 2012 16:27:54 +0100, Benjamin Tissoires wrote:
> In the case where the hid driver in charge of handling the hid part
> of the device (hid-generic for instance) fails at probe, neither
> i2c_hid_start nor i2c_hid_stop are called.
> Thus, the buffers allocated in i2c_hid_probe are never freed.
> 
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index bea4b13..62988f1 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -891,6 +891,9 @@ static int __devexit i2c_hid_remove(struct i2c_client *client)
>  
>  	free_irq(client->irq, ihid);
>  
> +	if (ihid->bufsize)
> +		i2c_hid_free_buffers(ihid);
> +
>  	kfree(ihid);
>  
>  	return 0;

Reviewed-by: Jean Delvare <khali@linux-fr.org>

-- 
Jean Delvare

[PATCH 14/14] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

[Benjamin Tissoires]

The previous memcpy implementation relied on the size advertized by the
device. There were no guarantees that buf was big enough.

Some gymnastic is also required with the +2/-2 to take into account
the first 2 bytes where the total length is supplied by the device.

Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
---
 drivers/hid/i2c-hid/i2c-hid.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
index 62988f1..de3566f 100644
--- a/drivers/hid/i2c-hid/i2c-hid.c
+++ b/drivers/hid/i2c-hid/i2c-hid.c
@@ -503,13 +503,14 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 {
 	struct i2c_client *client = hid->driver_data;
 	struct i2c_hid *ihid = i2c_get_clientdata(client);
+	size_t rcount;
 	int ret;
 
 	if (report_type == HID_OUTPUT_REPORT)
 		return -EINVAL;
 
-	if (count > ihid->bufsize)
-		count = ihid->bufsize;
+	if (count > ihid->bufsize - 2)
+		count = ihid->bufsize - 2;
 
 	ret = i2c_hid_get_report(client,
 			report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
@@ -518,7 +519,13 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
 	if (ret < 0)
 		return ret;
 
-	count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+	rcount = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
+
+	if (!rcount)
+		return 0;
+
+	if (count > rcount - 2)
+		count = rcount - 2;
 
 	memcpy(buf, ihid->inbuf + 2, count);
 
-- 
1.8.0.1

Re: [PATCH 14/14] HID: i2c-hid: fix i2c_hid_get_raw_report count mismatches

[Benjamin Tissoires]

Hi,

sorry, but when I re-read it, it seems that I missed a +2 in the call
of i2c_hid_get_report.

So Jean, Jiri, please ignore this one. I'm really sorry if you already
started reviewing it.

I'll send a v2 with the missing patches early this afternoon.

Cheers,
Benjamin

On Tue, Dec 4, 2012 at 4:27 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> The previous memcpy implementation relied on the size advertized by the
> device. There were no guarantees that buf was big enough.
>
> Some gymnastic is also required with the +2/-2 to take into account
> the first 2 bytes where the total length is supplied by the device.
>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>
> ---
>  drivers/hid/i2c-hid/i2c-hid.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid.c b/drivers/hid/i2c-hid/i2c-hid.c
> index 62988f1..de3566f 100644
> --- a/drivers/hid/i2c-hid/i2c-hid.c
> +++ b/drivers/hid/i2c-hid/i2c-hid.c
> @@ -503,13 +503,14 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>  {
>         struct i2c_client *client = hid->driver_data;
>         struct i2c_hid *ihid = i2c_get_clientdata(client);
> +       size_t rcount;
>         int ret;
>
>         if (report_type == HID_OUTPUT_REPORT)
>                 return -EINVAL;
>
> -       if (count > ihid->bufsize)
> -               count = ihid->bufsize;
> +       if (count > ihid->bufsize - 2)
> +               count = ihid->bufsize - 2;
>
>         ret = i2c_hid_get_report(client,
>                         report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
> @@ -518,7 +519,13 @@ static int i2c_hid_get_raw_report(struct hid_device *hid,
>         if (ret < 0)
>                 return ret;
>
> -       count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
> +       rcount = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
> +
> +       if (!rcount)
> +               return 0;
> +
> +       if (count > rcount - 2)
> +               count = rcount - 2;
>
>         memcpy(buf, ihid->inbuf + 2, count);
>
> --
> 1.8.0.1
>