| Message ID | 1436826234-2447-1-git-send-email-ellen@cumulusnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@cumulusnetworks.com> Acked-by: Wolfram Sang <wsa@the-dreams.de> Wow, that driver seems to be largely untested. Thanks for cleaning it up! Sidenote: i2c drivers should really be in drivers/i2c ;)
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.
On Tue, Jul 14, 2015 at 8:54 PM, Jiri Kosina <jkosina@suse.com> 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. > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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 :)
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. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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! -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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:
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(-)