[PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Ellen Wang]

When doing an I2C_SMBUS_BYTE write (one byte write, no address),
the data to be written is in "command" not "data->byte".

Signed-off-by: Ellen Wang <ellen@cumulusnetworks.com>
---
Forgot signed-off-by tag last time, sorry.
---
 drivers/hid/hid-cp2112.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index a3703b8..7afc3fc 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -606,7 +606,7 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
 		if (I2C_SMBUS_READ == read_write)
 			count = cp2112_read_req(buf, addr, read_length);
 		else
-			count = cp2112_write_req(buf, addr, data->byte, NULL,
+			count = cp2112_write_req(buf, addr, command, NULL,
 						 0);
 		break;
 	case I2C_SMBUS_BYTE_DATA:
-- 
1.9.1

Re: [PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Antonio Borneo]

On Tue, Jul 14, 2015 at 6:23 AM, Ellen Wang <ellen@cumulusnetworks.com> wrote:
>
> When doing an I2C_SMBUS_BYTE write (one byte write, no address),
> the data to be written is in "command" not "data->byte".
>
> Signed-off-by: Ellen Wang <ellen@cumulusnetworks.com>
> ---
> Forgot signed-off-by tag last time, sorry.
> ---
>  drivers/hid/hid-cp2112.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> index a3703b8..7afc3fc 100644
> --- a/drivers/hid/hid-cp2112.c
> +++ b/drivers/hid/hid-cp2112.c
> @@ -606,7 +606,7 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
>                 if (I2C_SMBUS_READ == read_write)
>                         count = cp2112_read_req(buf, addr, read_length);
>                 else
> -                       count = cp2112_write_req(buf, addr, data->byte, NULL,
> +                       count = cp2112_write_req(buf, addr, command, NULL,
>                                                  0);
>                 break;
>         case I2C_SMBUS_BYTE_DATA:


Correct!

E.g. in drivers/i2c/i2c-core.c we have:

s32 i2c_smbus_write_byte(const struct i2c_client *client, u8 value)
{
       return i2c_smbus_xfer(client->adapter, client->addr, client->flags,
                             I2C_SMBUS_WRITE, value, I2C_SMBUS_BYTE, NULL);
}

that passes NULL for data, so data->byte will dereference the NULL ptr.
The value is passed in "command".

Jiri,
I agree with Ellen that this driver is not commonly used, so no rush
to push the fix upstream.
But the fix should be marked for stable.

If you need, you can add
Reviewed-by: Antonio Borneo <borneo.antonio@gmail.com>

BR
Antonio

Re: [PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 489 bytes --]

On Mon, Jul 13, 2015 at 03:23:54PM -0700, Ellen Wang wrote:
> When doing an I2C_SMBUS_BYTE write (one byte write, no address),
> the data to be written is in "command" not "data->byte".
> 
> Signed-off-by: Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>

Acked-by: Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>

Wow, that driver seems to be largely untested. Thanks for cleaning it
up! Sidenote: i2c drivers should really be in drivers/i2c ;)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Jiri Kosina]

On Mon, 13 Jul 2015, Ellen Wang wrote:

> When doing an I2C_SMBUS_BYTE write (one byte write, no address),
> the data to be written is in "command" not "data->byte".
> 
> Signed-off-by: Ellen Wang <ellen@cumulusnetworks.com>
> ---
> Forgot signed-off-by tag last time, sorry.

Applied to for-4.3/cp2112.

On Tue, 14 Jul 2015, Wolfram Sang wrote:

> Wow, that driver seems to be largely untested. Thanks for cleaning it
> up! Sidenote: i2c drivers should really be in drivers/i2c ;)

Well, it's sometimes a bit on the borderline where HID drivers should live 
(because they often, in addition to HID layer, make use of some other 
transport). If you want to move it under your wings, please send a patch 
that moves it and I'll Ack it.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Antonio Borneo]

On Tue, Jul 14, 2015 at 8:54 PM, Jiri Kosina <jkosina-IBi9RG/b67k@public.gmane.org> wrote:
> On Mon, 13 Jul 2015, Ellen Wang wrote:
>
>> When doing an I2C_SMBUS_BYTE write (one byte write, no address),
>> the data to be written is in "command" not "data->byte".
>>
>> Signed-off-by: Ellen Wang <ellen-qUQiAmfTcIp+XZJcv9eMoEEOCMrvLtNR@public.gmane.org>
>> ---
>> Forgot signed-off-by tag last time, sorry.
>
> Applied to for-4.3/cp2112.
>
> On Tue, 14 Jul 2015, Wolfram Sang wrote:
>
>> Wow, that driver seems to be largely untested. Thanks for cleaning it
>> up! Sidenote: i2c drivers should really be in drivers/i2c ;)
>
> Well, it's sometimes a bit on the borderline where HID drivers should live
> (because they often, in addition to HID layer, make use of some other
> transport). If you want to move it under your wings, please send a patch
> that moves it and I'll Ack it.


This is a MFD; should be split in GPIO and I2C, then each part moved
in its respective tree.

Anyone already working in this direction?

Regards,
Antonio

Re: [PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 431 bytes --]


> This is a MFD; should be split in GPIO and I2C, then each part moved
> in its respective tree.

Yes, this is the conclusion we drew last time, too. It might seem
overkill for such simple devices, still this case shows how much drivers
can suffer if missing the expertise of the apropriate subsystem. That
being said, world is full of cornercases, I agree.

> Anyone already working in this direction?

I'd be very surprised :)


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Ellen Wang]

On 07/14/2015 08:49 AM, Wolfram Sang wrote:
>
>> This is a MFD; should be split in GPIO and I2C, then each part moved
>> in its respective tree.
>
> Yes, this is the conclusion we drew last time, too. It might seem
> overkill for such simple devices, still this case shows how much drivers
> can suffer if missing the expertise of the apropriate subsystem. That
> being said, world is full of cornercases, I agree.

This driver is a good example of that (missing expertise).  It seems to 
be a fine HID driver but a lot of the I2C details were just wrong.  At 
the same time, I have no idea whether the GPIO part works at all.

>> Anyone already working in this direction?
>
> I'd be very surprised :)

I could and would but I'm really not in a good position to do it.  My 
only sample hardware is in a network switch.

Re: [PATCH v2] HID: cp2112: fix I2C_SMBUS_BYTE write

[Ellen Wang]

On 07/14/2015 05:54 AM, Jiri Kosina wrote:
> On Mon, 13 Jul 2015, Ellen Wang wrote:
>
>> When doing an I2C_SMBUS_BYTE write (one byte write, no address),
>> the data to be written is in "command" not "data->byte".
>>
>> Signed-off-by: Ellen Wang <ellen@cumulusnetworks.com>
>> ---
>> Forgot signed-off-by tag last time, sorry.
>
> Applied to for-4.3/cp2112.

Would it be possible to consider it for stable, as well as the other 
recent commits (6debce6f4e787a8eb4cec94e7afa85fb4f40db27 and 
5ddfb12e90c73cf86881345be422e09c367f6981)?

Thanks!