Hi Pete,
On Thu, Mar 04, 2010 at 02:33:49PM -0700, Pete Zaitcev wrote:
> Hi, Dmitry et. al.:
>
> I was looking at the way unused bits of report are being zeroed, and
> it seems like there must be a bug. Currently, the zeroing is done in
> hid_output_field and it covers any bits between the last used bit and
> the end of the byte. But in case of, say, my keyboard, NumLock is mask
> 0x01 and CapsLock is 0x02. Invoking hid_output_field for NumLock
> definitely zeroes across CapsLock. The only reason this works is that
> the fields are sorted by the offset.
>
> I am not sure it's safe at all times, so why don't we pull the
> zeroing outside and do it once per report? Please see the attached.
> I tested it with various keyboards (including those that blow up
> on RHEL 5), and it works, but the question is, do we want it?
>
I think Jiri is the most qualified to answer questions like that about
HID (CCed) buit I think what you are proposing is reasonable and would
make the code safer indeed.
> Cheers,
> -- Pete
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index eabe5f8..14b45b1 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -929,6 +929,15 @@ exit:
> kfree(value);
> }
>
> +static unsigned int hid_field_length(struct hid_field *field)
> +{
> + unsigned count = field->report_count;
> + unsigned offset = field->report_offset;
> + unsigned size = field->report_size;
> +
> + return offset + count * size;
> +}
> +
> /*
> * Output the field into the report.
> */
> @@ -938,13 +947,8 @@ static void hid_output_field(struct hid_field *field, __u8 *data)
> unsigned count = field->report_count;
> unsigned offset = field->report_offset;
> unsigned size = field->report_size;
> - unsigned bitsused = offset + count * size;
> unsigned n;
>
> - /* make sure the unused bits in the last byte are zeros */
> - if (count > 0 && size > 0 && (bitsused % 8) != 0)
> - data[(bitsused-1)/8] &= (1 << (bitsused % 8)) - 1;
> -
> for (n = 0; n < count; n++) {
> if (field->logical_minimum < 0) /* signed values */
> implement(data, offset + n * size, size, s32ton(field->value[n], size));
> @@ -960,10 +964,19 @@ static void hid_output_field(struct hid_field *field, __u8 *data)
> void hid_output_report(struct hid_report *report, __u8 *data)
> {
> unsigned n;
> + unsigned length, bitsused;
>
> if (report->id > 0)
> *data++ = report->id;
>
> + length = 0;
> + for (n = 0; n < report->maxfield; n++) {
> + bitsused = hid_field_length(report->field[n]);
> + if (bitsused > length)
> + length = bitsused;
> + }
> + memset(data, 0, (length + 7) / 8);
> +
> for (n = 0; n < report->maxfield; n++)
> hid_output_field(report->field[n], data);
> }
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
