Message-ID: <CAN+gG=FjVswyx1W4y6NtbNnBu2AWgZZfxVr0h00ZYujCOz3v7Q@mail.gmail.com>
Date:	Mon, 3 Dec 2012 12:32:03 +0100
From:	Benjamin Tissoires <benjamin.tissoires@...il.com>
To:	Jean Delvare <khali@...ux-fr.org>
Cc:	Dmitry Torokhov <dmitry.torokhov@...il.com>,
	Jiri Kosina <jkosina@...e.cz>,
	Stephane Chatty <chatty@...c.fr>, fabien.andre@...il.com,
	scott.liu@....com.tw, JJ Ding <jj_ding@....com.tw>,
	Jiri Slaby <jslaby@...e.cz>,
	Shubhrajyoti Datta <omaplinuxkernel@...il.com>,
	linux-i2c@...r.kernel.org, linux-input@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] i2c-hid: introduce HID over i2c specification implementation

Hi Jean,

On Fri, Nov 30, 2012 at 3:56 PM, Jean Delvare <khali@...ux-fr.org> wrote:
> Hi Benjamin, Jiri,
>
> Sorry for the late review. But better late than never I guess...

Sure! Thanks for the review. As the driver is already in Jiri's tree,
I'll do small incremental patches based on this release.

I'll try to address all of your comments.

I have a few answers to some of your remarks (I fully agree with all
of the others):

>
> On Mon, 12 Nov 2012 15:42:59 +0100, Benjamin Tissoires wrote:
>> Microsoft published the protocol specification of HID over i2c:
>> http://msdn.microsoft.com/en-us/library/windows/hardware/hh852380.aspx
>>
>> This patch introduces an implementation of this protocol.
>>
>> This implementation does not includes the ACPI part of the specification.
>> This will come when ACPI 5.0 devices enumeration will be available.
>>
>> Once the ACPI part is done, OEM will not have to declare HID over I2C
>> devices in their platform specific driver.
>>
>> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@...il.com>
>> ---
>> (..)
>>  drivers/hid/Kconfig           |   2 +
>>  drivers/hid/Makefile          |   1 +
>>  drivers/hid/i2c-hid/Kconfig   |  21 +
>>  drivers/hid/i2c-hid/Makefile  |   5 +
>>  drivers/hid/i2c-hid/i2c-hid.c | 974 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/i2c/i2c-hid.h   |  35 ++
>>  6 files changed, 1038 insertions(+)
>>  create mode 100644 drivers/hid/i2c-hid/Kconfig
>>  create mode 100644 drivers/hid/i2c-hid/Makefile
>>  create mode 100644 drivers/hid/i2c-hid/i2c-hid.c
>>  create mode 100644 include/linux/i2c/i2c-hid.h
>
> Looks much better. I still have several random comments which you may
> be interested in.
>
> (...)
>> +/* debug option */
>> +static bool debug = false;
>
> Whether you like it or not, that's still an error for checkpatch:
> ERROR: do not initialise statics to 0 or NULL
> #240: FILE: drivers/hid/i2c-hid/i2c-hid.c:49:
> +static bool debug = false;
>
> The rationale is that the compiler can write more efficient code if
> initializations to 0 or NULL are implicit.

ok, will do.

>
>> +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)
>
> This is considered an error by checkpatch too:
> ERROR: Macros with complex values should be enclosed in parenthesis
> #244: FILE: drivers/hid/i2c-hid/i2c-hid.c:53:
> +#define i2c_hid_dbg(ihid, fmt, arg...) \
> +       if (debug)                      \
> +               dev_printk(KERN_DEBUG, &(ihid)->client->dev, fmt, ##arg)
>
> And I wouldn't disagree, as the construct above can lead to bugs which
> are hard to see... Consider the following piece of code:
>
>         if (condition)
>                 i2c_hid_dbg(ihid, "Blah blah %d\n", i);
>         else
>                 do_something_very_important();
>
> It looks correct, however with the macro definition above, 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();
>         }
>
> That is, the condition to call do_something_very_important() is
> condition && !debug, and not !condition as the code suggests. This is
> only an example and your driver doesn't currently use any such
> construct. Still I believe this should be fixed.

With your explanation, you've convinced me. I was not sure on how
should I handle this warning as I copied this macro from one in hid.
I will fix the two then.

>
> (...)
> static int i2c_hid_alloc_buffers(struct i2c_hid *ihid)
> {
>         /* 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);
>         ihid->argsbuf = kzalloc(args_len, GFP_KERNEL);
>         ihid->cmdbuf = kzalloc(sizeof(union command) + args_len, GFP_KERNEL);
>
>         if (!ihid->inbuf || !ihid->argsbuf || !ihid->cmdbuf) {
>                 i2c_hid_free_buffers(hid);
>                 return -ENOMEM;
>         }
>
>         return 0;
> }

Much better, thanks!

>
>> +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;
>> +}
>> +
>> +static int i2c_hid_get_raw_report(struct hid_device *hid,
>> +             unsigned char report_number, __u8 *buf, size_t count,
>> +             unsigned char report_type)
>> +{
>> +     struct i2c_client *client = hid->driver_data;
>> +     struct i2c_hid *ihid = i2c_get_clientdata(client);
>> +     int ret;
>> +
>> +     if (report_type == HID_OUTPUT_REPORT)
>> +             return -EINVAL;
>> +
>> +     if (count > ihid->bufsize)
>> +             count = ihid->bufsize;
>> +
>> +     ret = i2c_hid_get_report(client,
>> +                     report_type == HID_FEATURE_REPORT ? 0x03 : 0x01,
>> +                     report_number, ihid->inbuf, count);
>> +
>> +     if (ret < 0)
>> +             return ret;
>> +
>> +     count = ihid->inbuf[0] | (ihid->inbuf[1] << 8);
>> +
>> +     memcpy(buf, ihid->inbuf + 2, count);
>
> What guarantee do you have that count is not larger than buf's length?
> I hope you don't just count on all hardware out there being nice and
> sane, do you? ;)

Hehe, this function is never called from the device, but from the user
space. It's called by hidraw_get_report in drivers/hid/hidraw.c, and
the caller makes the allocation of buf with a size of count.
There is an other usage in hid-input.c with "buf, sizeof(buf)," as arguments.
So this should never be a problem as long as anybody else call this
function without making sure count is the right size.

>
>> +
>> +     return count;
>> +}
>(...)
>> +static int __devinit i2c_hid_init_irq(struct i2c_client *client)
>> +{
>> +     struct i2c_hid *ihid = i2c_get_clientdata(client);
>> +     int ret;
>> +
>> +     dev_dbg(&client->dev, "Requesting IRQ: %d\n", client->irq);
>> +
>> +     ret = request_threaded_irq(client->irq, NULL, i2c_hid_irq,
>> +                     IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
>> +                     client->name, ihid);
>> +     if (ret < 0) {
>> +             dev_dbg(&client->dev,
>> +                     "Could not register for %s interrupt, irq = %d,"
>> +                     " ret = %d\n",
>> +             client->name, client->irq, ret);
>
> dev_warn()? Indentation is broken too.
>
>> +
>> +             return ret;
>> +     }
>> +
>> +     ihid->irq = client->irq;
>
> I don't think you need this. Everywhere you use ihid->irq, you have
> client at hand so you could as well use client->irq. This would avoid
> inconsistencies like free_irq(ihid->irq, ihid) in probing error path
> vs. free_irq(client->irq, ihid) in removal path, or
> enable_irq_wake(ihid->irq) vs. disable_irq_wake(client->irq) in
> suspend/resume.

Yep, these inconsistency are pretty bad. IIRC, I used that to know if
the setup was fine for the irq, but it may comes from an earlier
version where I had polling, which is not allowed by the spec.
So definitively, I'll have to rework that.

>
>> +
>> +     return 0;
>> +}
>> +
>(...)
>> +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;
>
> This just can't happen...?
>
>> +
>> +     hid = ihid->hid;
>> +     hid_destroy_device(hid);
>> +
>> +     free_irq(client->irq, ihid);
>> +
>
> Is there any guarantee that i2c_hid_stop() has been called before
> i2c_hid_remove() is? If not, you're missing a call to
> i2c_hid_free_buffers() here. BTW I'm not quite sure why
> i2c_hid_remove() frees the buffers in the first place, but then again I
> don't know a thing about the HID infrastructure.

Calling i2c_hid_stop() is the responsibility of the hid driver at his
remove. By hid driver, I mean the driver that relies on hid to handle
the device (hid-generic in 80% of the cases) So as long as this hid
driver is loaded, we can not remove i2c_hid as it is used by the hid
driver. So it seems that this guarantees that i2c_hid_stop() has been
called before i2c_hid_remove() is.

But now that I think of it, there are cases where i2c_hid_stop() is
not called: when the hid driver failed to probe. So definitively,
there is a mem leak here. Thanks.


>
>> +     kfree(ihid);
>> +
>> +     return 0;
>> +}
>> +(...)
>> +static const struct i2c_device_id i2c_hid_id_table[] = {
>> +     { "i2c_hid", 0 },
>
> I am just realizing "i2c_hid" is a little redundant as an i2c device
> name. Can we make this just "hid"?

I know that it already have been used by one Nvidia team and by Elan
for internal tests. So I don't know if it's possible to change it now
(though it's not a big deal).
Anyway, "hid" is a little weird to me (but this is because I started
hacking the kernel from there), as it's really short and does not
contain i2c :)

Cheers,
Benjamin

>
>> +     { },
>> +};
>> +MODULE_DEVICE_TABLE(i2c, i2c_hid_id_table);
>
> --
> Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists