NULL dereference on I2C_SMBUS_BYTE write

[squeakbat]

I get a NULL dereference on trying to write a single byte to a slave. The problem is that in the I2C_SMBUS_BYTE write case, the one-byte data is in "command" not in "data". Here are the diffs (plus a couple optional fixes to shut the compiler up on 64-bit machines):

diff --git a/hid-cp2112.c b/hid-cp2112.c
index b9ba12a..cf1e16b 100644
--- a/hid-cp2112.c
+++ b/hid-cp2112.c
@@ -361,7 +361,7 @@ static int cp2112_read(struct cp2112_device *dev, u8 *data, size_t size)
        if (ret)
                return ret;

-       hid_dbg(hdev, "read %d of %d bytes requested\n",
+       hid_dbg(hdev, "read %d of %zd bytes requested\n",
                dev->read_length, size);

        if (size > dev->read_length)
@@ -442,8 +442,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,
-                                                0);
+                       count = cp2112_write_req(buf, addr, command, NULL, 0);
                break;
        case I2C_SMBUS_BYTE_DATA:
                read_length = 1;
@@ -552,7 +551,7 @@ static int cp2112_xfer(struct i2c_adapter *adap, u16 addr,
        if (ret < 0)
                goto power_normal;
        if (ret != read_length) {
-               hid_warn(hdev, "short read: %d < %d\n", ret, read_length);
+               hid_warn(hdev, "short read: %d < %zd\n", ret, read_length);
                ret = -EIO;
                goto power_normal;
        }

[squeakbat]

Ick. Sorry about the colorized diffs. I didn't think it was going to do that.

[kaklik]

Hello, the newer version of this code is included in almost all distribution kernels with version 3.15+. I have tested it on my Ubuntu 14.10. machine with distribution package kernel. And it seems to work correctly in theirs new version, but you should write an email to original autor of the code - David Barksdale dbarksdale uplogix.com with further feature requests. We are currently using the HIDAPI interface implemented in our code: https://github.com/MLAB-project/MLAB-I2c-modules intead of this kernel module because HIDAPI is multiplatform access method.

[squeakbat]

Thanks for the reply.

Yeah. I'm using an older version: 3.2.16 debian. I had to backport the driver. At user-level, I'm testing with i2cget from lmsensors, but that shouldn't matter (or maybe it does?).

I'll look at the version that's in the mainline kernel to see if it's different. Should have done that in the first place. Are you saying this version is no longer maintained or is maintained elsewhere (mainline)?

I need this (ideally) in the kernel driver form. It's not exactly an embedded platform and there's a lot of other code that wants to see i2c devices exposed in the regular way (in /dev and whatnot). Upgrading to a newer kernel is an option but not for the short term. (Sigh. Code in the real world is always full of constraints.)

[squeakbat]

I looked at the mainline version. I see it has a master_xfer function. But there are also more issues with it. For example, it doesn't work with eeproms because cp2112_i2c_xfer() doesn't accept more than one messages at a time, and reading eeproms always generates a write-read sequence. Also, it only processes one CP2112_DATA_READ_RESPONSE so reads longer than 61 bytes don't work.

Are you the maintainer for the mainline version?

[kaklik]

Yes, the driver is restricted to work only with responses which are shorter than CP2112's internal buffer which is 61 bytes in length.

No I am not the maintainer of the mainline kernel version. The maintainer is David Barksdale. His email is in preamble of the source code.

[squeakbat]

All it needs is to iterate on cp2112_read() until it gets all the bytes.

Just as an example, the at24 eeprom driver generates reads larger than 61 bytes so just failing on large reads isn't really practical.

I'll work on submitting a patch for mainline. Thanks.