Re: [PATCH RESEND] HID: New hid-cp2112 driver.

[David Herrmann]

Hi

On Tue, Sep 3, 2013 at 11:51 PM, David Barksdale <dbarksdale@xxxxxxxxxxx> wrote:
> On 08/29/2013 11:43 AM, David Herrmann wrote:
>>
>> Hi
>>
>> On Tue, Aug 27, 2013 at 5:01 PM, David Barksdale <dbarksdale@xxxxxxxxxxx>
>> wrote:
>>>
>>> This patch adds support for the Silicon Labs CP2112
>>> "Single-Chip HID USB to SMBus Master Bridge."
>>>
>>> I wrote this to support a USB temperature and humidity
>>> sensor I've been working on. It's been tested by using
>>> SMBus byte-read, byte-data-read/write, and word-data-read
>>> transfer modes to talk to an I2C sensor. The other
>>> transfer modes have not been tested.
>>>
>>> Signed-off-by: David Barksdale <dbarksdale@xxxxxxxxxxx>
>>>
>>> ---
>>>   drivers/hid/Kconfig      |   6 +
>>>   drivers/hid/Makefile     |   1 +
>>>   drivers/hid/hid-core.c   |   3 +
>>>   drivers/hid/hid-cp2112.c | 504
>>> +++++++++++++++++++++++++++++++++++++++++++++++
>>>   4 files changed, 514 insertions(+)
>>>
>>> diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>>> index 14ef6ab..1833948 100644
>>> --- a/drivers/hid/Kconfig
>>> +++ b/drivers/hid/Kconfig
>>> @@ -175,6 +175,12 @@ config HID_PRODIKEYS
>>>            multimedia keyboard, but will lack support for the musical
>>> keyboard
>>>            and some additional multimedia keys.
>>>
>>> +config HID_CP2112
>>> +       tristate "Silicon Labs CP2112 HID USB-to-SMBus Bridge support"
>>> +       depends on USB_HID
>>> +       ---help---
>>> +       Support for Silicon Labs CP2112 HID USB to SMBus Master Bridge.
>>> +
>>>   config HID_CYPRESS
>>>          tristate "Cypress mouse and barcode readers" if EXPERT
>>>          depends on HID
>>> diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile
>>> index 6f68728..a88a5c4 100644
>>> --- a/drivers/hid/Makefile
>>> +++ b/drivers/hid/Makefile
>>> @@ -41,6 +41,7 @@ obj-$(CONFIG_HID_AUREAL)        += hid-aureal.o
>>>   obj-$(CONFIG_HID_BELKIN)       += hid-belkin.o
>>>   obj-$(CONFIG_HID_CHERRY)       += hid-cherry.o
>>>   obj-$(CONFIG_HID_CHICONY)      += hid-chicony.o
>>> +obj-$(CONFIG_HID_CP2112)       += hid-cp2112.o
>>>   obj-$(CONFIG_HID_CYPRESS)      += hid-cypress.o
>>>   obj-$(CONFIG_HID_DRAGONRISE)   += hid-dr.o
>>>   obj-$(CONFIG_HID_EMS_FF)       += hid-emsff.o
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index 36668d1..b472a0d 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -1568,6 +1568,9 @@ static const struct hid_device_id
>>> hid_have_special_driver[] = {
>>>          { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>>> USB_DEVICE_ID_CHICONY_WIRELESS2) },
>>>          { HID_USB_DEVICE(USB_VENDOR_ID_CHICONY,
>>> USB_DEVICE_ID_CHICONY_AK1D) },
>>>          { HID_USB_DEVICE(USB_VENDOR_ID_CREATIVELABS,
>>> USB_DEVICE_ID_PRODIKEYS_PCMIDI) },
>>> +#if IS_ENABLED(CONFIG_HID_CP2112)
>>> +       { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, 0xEA90) },
>>> +#endif
>>
>>
>> Why is that #if needed?
>
>
> There are similar #ifs for CONFIG_HID_LENOVO_TPKBD, CONFIG_HID_LOGITECH_DJ,
> and CONFIG_HID_ROCCAT. I thought it was a good idea.

These are used for devices that work with and without special drivers.
If you don't have the special-driver compiled in, you want the generic
HID driver to bind to these devices (Jiri may correct me if I am
wrong).

However, for your device this doesn't make any sense. hid-generic has
no idea of smbus bridges. So yeah, just drop the #if.

>
>>>          { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>>> USB_DEVICE_ID_CYPRESS_BARCODE_1) },
>>>          { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>>> USB_DEVICE_ID_CYPRESS_BARCODE_2) },
>>>          { HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS,
>>> USB_DEVICE_ID_CYPRESS_BARCODE_3) },
>>> diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
>>> new file mode 100644
>>> index 0000000..8737d18
>>> --- /dev/null
>>> +++ b/drivers/hid/hid-cp2112.c
>>> @@ -0,0 +1,504 @@
>>> +/*
>>> +  hid-cp2112.c - Silicon Labs HID USB to SMBus master bridge
>>> +  Copyright (c) 2013 Uplogix, Inc.
>>> +  David Barksdale <dbarksdale@xxxxxxxxxxx>
>>> +
>>> +  This program is free software; you can redistribute it and/or modify
>>> +  it under the terms of the GNU General Public License as published by
>>> +  the Free Software Foundation; either version 2 of the License, or
>>> +  (at your option) any later version.
>>> +
>>> +  This program is distributed in the hope that it will be useful,
>>> +  but WITHOUT ANY WARRANTY; without even the implied warranty of
>>> +  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>> +  GNU General Public License for more details.
>>> +
>>> +  You should have received a copy of the GNU General Public License
>>> +  along with this program; if not, write to the Free Software
>>> +  Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>>> + */
>>> +
>>> +#include <linux/hid.h>
>>> +#include <linux/i2c.h>
>>> +#include <linux/module.h>
>>> +#include "hid-ids.h"
>>> +
>>> +enum {
>>> +       GET_VERSION_INFO = 0x05,
>>> +       SMBUS_CONFIG = 0x06,
>>> +       DATA_READ_REQUEST = 0x10,
>>> +       DATA_WRITE_READ_REQUEST = 0x11,
>>> +       DATA_READ_FORCE_SEND = 0x12,
>>> +       DATA_READ_RESPONSE = 0x13,
>>> +       DATA_WRITE_REQUEST = 0x14,
>>> +       TRANSFER_STATUS_REQUEST = 0x15,
>>> +       TRANSFER_STATUS_RESPONSE = 0x16,
>>> +       CANCEL_TRANSFER = 0x17,
>>> +};
>>> +
>>> +enum {
>>> +       STATUS0_IDLE = 0x00,
>>> +       STATUS0_BUSY = 0x01,
>>> +       STATUS0_COMPLETE = 0x02,
>>> +       STATUS0_ERROR = 0x03,
>>> +};
>>> +
>>> +enum {
>>> +       STATUS1_TIMEOUT_NACK = 0x00,
>>> +       STATUS1_TIMEOUT_BUS = 0x01,
>>> +       STATUS1_ARBITRATION_LOST = 0x02,
>>> +       STATUS1_READ_INCOMPLETE = 0x03,
>>> +       STATUS1_WRITE_INCOMPLETE = 0x04,
>>> +       STATUS1_SUCCESS = 0x05,
>>> +};
>>
>>
>> If you don't add any prefix to these functions (especially
>> SMBUS_CONFIG) it is quite likely that at some point your driver will
>> get compile-errors. Any particular reason not to do that? (same
>> applies to all the function/global-vars below)
>
>
> Prefixes added. I think the STATUS* ones will be unique enough without
> another prefix.

It was SMBUS_CONFIG in particular that bothered me. So yeah, if names
look unique enough, no need to add prefixes.

[..snip..]
>>> +       }
>>> +       dev->hdev = hdev;
>>> +       hid_set_drvdata(hdev, (void *)dev);
>>> +       dev->adap.owner         = THIS_MODULE;
>>> +       dev->adap.class         = I2C_CLASS_HWMON;
>>> +       dev->adap.algo          = &smbus_algorithm;
>>> +       dev->adap.algo_data     = dev;
>>> +       snprintf(dev->adap.name, sizeof(dev->adap.name),
>>> +               "CP2112 SMBus Bridge on hiddev%d", hdev->minor);
>>> +       init_waitqueue_head(&dev->wait);
>>> +       hid_device_io_start(hdev);
>>> +       ret = i2c_add_adapter(&dev->adap);
>>> +       hid_device_io_stop(hdev);
>>
>>
>> You should call hid_device_io_stop() only on failure. Otherwise,
>> hid-core drops any incoming events until you return. If smbus can
>> handle missing events, this is fine.
>>
>> In case you don't care for I/O between this and your handler above,
>> you can call hid_hw_close() here. Note that ->raw_event() is only
>> called while the device is open.
>
>
> I'll call hid_device_io_stop() and hid_hw_close() here since I don't care
> for I/O or events until I have a transaction to perform.

Yepp, sounds good.

>
>>> +       if (ret) {
>>> +               hid_err(hdev, "error registering i2c adapter\n");
>>> +               kfree(dev);
>>> +               hid_set_drvdata(hdev, NULL);
>>
>>
>> Drop this hid_set_drvdata()
>> hid_hw_stop() missing (and hid_hw_close())
>
>
> Done.
>
>
>>> +       }
>>> +       hid_dbg(hdev, "adapter registered\n");
>>> +       return ret;
>>> +}
>>> +
>>> +static void cp2112_remove(struct hid_device *hdev)
>>> +{
>>> +       struct cp2112_device *dev = hid_get_drvdata(hdev);
>>> +
>>> +       if (dev) {
>>
>>
>> Why if (dev)? Any reason dev might be NULL?
>
>
> This is probably left over from some debugging, just to be safe. Removed.
>
>
>>> +               i2c_del_adapter(&dev->adap);
>>> +               wake_up_interruptible(&dev->wait);
>>
>>
>> How can you have waiters on dev->wait if you free() it in the line
>> below? There ought to be some *_sync() call here which waits for all
>> possible pending calls to finish.
>
>
> I'm having some trouble with this. I've added some printk's to understand
> what happens when I yank the USB cable. For testing I generate transactions
> using the i2c-dev driver and i2cdetect utility.
> On one occasion it looks like the last cp2112_xfer() completes, then it is
> called several more times after the cable is yanked which fail on
> hid_output_raw_report() returning -EPIPE. Then cp2112_remove() is finally
> called (your version below) and returns. Then cp2112_xfer() is called yet
> again and barfs because everything's been freed.
> What can I wait on to know that my .smbus_xfer function won't be called
> after i2c_del_adapter()?

Maybe Benjamin can help here as I am not very familiar with i2c core-code.

I did look at it (and other i2c bus-drivers) and i2c_del_adapter
should be enough.

The thing is, your i2c-xfer callback is synchronous. Once
hid->remove() is called, no other HID callback will be called,
anymore. Furthermore, if the device is gone, all HID-I/O calls will
return -EIO immediately. So you should just drop the
wake_up_interruptible() (or move it in front of i2c_del_adapter() in
case you don't care for driver-unload while devices are still active).

Furthermore, the xfer callback shouldn't be called by smbus-core after
i2c_del_adapter() returns. If it is, you should definitely report that
to the i2c mailing-list.

Obviously, for debugging, you should set the i2c-drvdata to NULL after
i2c_del_adapter() and test for that in your xfer callback (may add a
WARN_ON()). This is racy, but it should avoid the use-after-free. Once
you figured it out, you can remove them again. Feel free to CC
i2c-mailing-lists if you are not sure.

And also feel free to send v2 even if there are still bugs. It's often
easier to comment on the recent-state than just an old ML thread.

Thanks
David

>
>>> +               kfree(dev);
>>> +               hid_set_drvdata(hdev, NULL);
>>
>>
>> Not needed.
>>
>>> +       }
>>> +       hid_hw_stop(hdev);
>>
>>
>> I recommend calling hid_hw_stop() before destroying your context. Not
>> strictly needed, but it makes it clear that no I/O is possible during
>> _remove().
>>
>> So you can reduce this function to:
>>
>> hid_hw_stop(hdev);
>> i2c_del_adapter(&dev->adap);
>> wake_up_interruptible(&dev->wait);
>> your_whatever_wait_sync_call()
>> kfree(dev);
>
>
> Done.
>
>
>>> +}
>>> +
>>> +static int cp2112_raw_event(struct hid_device *hdev, struct hid_report
>>> *report,
>>> +       uint8_t *data, int size)
>>> +{
>>> +       struct cp2112_device *dev = hid_get_drvdata(hdev);
>>> +
>>> +       switch (data[0]) {
>>> +       case TRANSFER_STATUS_RESPONSE:
>>> +               hid_dbg(hdev, "xfer status: %02x %02x %04x %04x\n",
>>> +                       data[1], data[2], htons(*(uint16_t *)&data[3]),
>>> +                       htons(*(uint16_t *)&data[5]));
>>> +               switch (data[1]) {
>>> +               case STATUS0_IDLE:
>>> +                       dev->xfer_status = -EAGAIN;
>>> +                       break;
>>> +               case STATUS0_BUSY:
>>> +                       dev->xfer_status = -EBUSY;
>>> +                       break;
>>> +               case STATUS0_COMPLETE:
>>> +                       dev->xfer_status = ntohs(*(uint16_t *)&data[5]);
>>> +                       break;
>>> +               case STATUS0_ERROR:
>>> +                       switch (data[2]) {
>>> +                       case STATUS1_TIMEOUT_NACK:
>>> +                       case STATUS1_TIMEOUT_BUS:
>>> +                               dev->xfer_status = -ETIMEDOUT;
>>> +                               break;
>>> +                       default:
>>> +                               dev->xfer_status = -EIO;
>>> +                       }
>>> +                       break;
>>> +               default:
>>> +                       dev->xfer_status = -EINVAL;
>>> +                       break;
>>> +               }
>>> +               atomic_set(&dev->xfer_avail, 1);
>>> +               break;
>>> +       case DATA_READ_RESPONSE:
>>> +               hid_dbg(hdev, "read response: %02x %02x\n", data[1],
>>> data[2]);
>>> +               dev->read_length = data[2];
>>> +               if (dev->read_length > sizeof(dev->read_data))
>>> +                       dev->read_length = sizeof(dev->read_data);
>>> +               memcpy(dev->read_data, &data[3], dev->read_length);
>>> +               atomic_set(&dev->read_avail, 1);
>>> +               break;
>>> +       default:
>>> +               hid_err(hdev, "unknown report\n");
>>> +               return 0;
>>> +       }
>>> +       wake_up_interruptible(&dev->wait);
>>> +       return 1;
>>> +}
>>> +
>>> +static struct hid_driver cp2112_driver = {
>>> +       .name = "cp2112",
>>> +       .id_table = cp2112_devices,
>>> +       .probe = cp2112_probe,
>>> +       .remove = cp2112_remove,
>>> +       .raw_event = cp2112_raw_event,
>>> +};
>>> +
>>> +static int __init cp2112_init(void)
>>> +{
>>> +       return hid_register_driver(&cp2112_driver);
>>> +}
>>> +
>>> +static void __exit cp2112_exit(void)
>>> +{
>>> +       hid_unregister_driver(&cp2112_driver);
>>> +}
>>> +
>>> +module_init(cp2112_init);
>>> +module_exit(cp2112_exit);
>>
>>
>> Use:
>> module_hid_driver(&cp2112_driver);
>> to save some lines here.
>
>
> Done.
>
>
>> Cheers
>> David
>>
>>> +MODULE_DESCRIPTION("Silicon Labs HID USB to SMBus master bridge");
>>> +MODULE_AUTHOR("David Barksdale <dbarksdale@xxxxxxxxxxx>");
>>> +MODULE_LICENSE("GPL");
>>> +
>>> --
>>> tg: (584d88b..) upstream/hid-cp2112 (depends on: upstream/master)
>>> --
>
>
> Thank you David for all the helpful comments.
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/