Zeroing the report before setting fields

Zeroing the report before setting fields

[Pete Zaitcev]

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?

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);
 }

Re: Zeroing the report before setting fields

[Dmitry Torokhov]

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

Re: Zeroing the report before setting fields

[Jiri Kosina]

On Fri, 5 Mar 2010, Dmitry Torokhov wrote:

> > 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.

Yes, the patch looks fine to me, thanks for CCing me. 

Pete, could you please send it to me along with your Signed-off-by line, 
so that I could queue it up?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: Zeroing the report before setting fields

[Pete Zaitcev]

On Tue, 9 Mar 2010 13:41:24 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:

> > > 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 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.
> 
> Yes, the patch looks fine to me, thanks for CCing me. 
> 
> Pete, could you please send it to me along with your Signed-off-by line, 
> so that I could queue it up?

Will do, thanks. I was hoping for a comment re. memset.
It is not needed for reports that consist of full bytes,
so it's a little wasteful. I aimed at simplicity, but I don't
know, what do you think?

-- Pete

Re: Zeroing the report before setting fields

[Jiri Kosina]

On Tue, 9 Mar 2010, Pete Zaitcev wrote:

> > > > 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 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.
> > 
> > Yes, the patch looks fine to me, thanks for CCing me. 
> > 
> > Pete, could you please send it to me along with your Signed-off-by line, 
> > so that I could queue it up?
> 
> Will do, thanks. I was hoping for a comment re. memset.
> It is not needed for reports that consist of full bytes,
> so it's a little wasteful. I aimed at simplicity, but I don't
> know, what do you think?

I don't think it's super important (basically nothing can be considered 
super-hot path when dealing with such slow hardware as HID devices :) ), 
but I wouldn't object to adding extra check before calling the memset.

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: Zeroing the report before setting fields

[Pete Zaitcev]

On Sat, 13 Mar 2010 23:25:11 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:

> I don't think it's super important (basically nothing can be considered 
> super-hot path when dealing with such slow hardware as HID devices :) ), 
> but I wouldn't object to adding extra check before calling the memset.

I thought about the issue some more and came to the conclusion that
all the bit counting in my patch was entirely unnecessary (it's still
needed in the code that sets them, of course).

What would you say to the following? I tested it to work with the
affected keyboard. Seems like a simpler way to remove the overlap
and the trailing bits.

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 368fbb0..7f2a3b9 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -940,13 +940,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));
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 56d06cd..1332ed0 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -505,7 +505,7 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
 			return;
 		}
 
-		usbhid->out[usbhid->outhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		usbhid->out[usbhid->outhead].raw_report = kzalloc(len, GFP_ATOMIC);
 		if (!usbhid->out[usbhid->outhead].raw_report) {
 			dev_warn(&hid->dev, "output queueing failed\n");
 			return;
@@ -537,7 +537,7 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
 	}
 
 	if (dir == USB_DIR_OUT) {
-		usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC);
+		usbhid->ctrl[usbhid->ctrlhead].raw_report = kzalloc(len, GFP_ATOMIC);
 		if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) {
 			dev_warn(&hid->dev, "control queueing failed\n");
 			return;

Greetings,
-- Pete

Re: Zeroing the report before setting fields

[Jiri Kosina]

On Sun, 21 Mar 2010, Pete Zaitcev wrote:

> > I don't think it's super important (basically nothing can be considered 
> > super-hot path when dealing with such slow hardware as HID devices :) ), 
> > but I wouldn't object to adding extra check before calling the memset.
> 
> I thought about the issue some more and came to the conclusion that
> all the bit counting in my patch was entirely unnecessary (it's still
> needed in the code that sets them, of course).
> 
> What would you say to the following? I tested it to work with the
> affected keyboard. Seems like a simpler way to remove the overlap
> and the trailing bits.
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 368fbb0..7f2a3b9 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -940,13 +940,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));
> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> index 56d06cd..1332ed0 100644
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -505,7 +505,7 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
>  			return;
>  		}
>  
> -		usbhid->out[usbhid->outhead].raw_report = kmalloc(len, GFP_ATOMIC);
> +		usbhid->out[usbhid->outhead].raw_report = kzalloc(len, GFP_ATOMIC);
>  		if (!usbhid->out[usbhid->outhead].raw_report) {
>  			dev_warn(&hid->dev, "output queueing failed\n");
>  			return;
> @@ -537,7 +537,7 @@ static void __usbhid_submit_report(struct hid_device *hid, struct hid_report *re
>  	}
>  
>  	if (dir == USB_DIR_OUT) {
> -		usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC);
> +		usbhid->ctrl[usbhid->ctrlhead].raw_report = kzalloc(len, GFP_ATOMIC);
>  		if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) {
>  			dev_warn(&hid->dev, "control queueing failed\n");
>  			return;

In principle this looks correct. 

The problem is that with previous aproach, we are handling the zeroing in 
the generic HID layer, but after your patch the zeroing is handled less 
generally for USB transport only.

I.e. I see two problems, which are closely related

- you forgot to do the same change to Bluetooth as well
- this introduces a new semantical constraint on HID transport drivers, so 
  that they have to make sure that the reports are always zeroed properly

Frankly, I like the generic aproach more ... if we were to add more 
low-level transport HID implementations later, I am sure this will be 
forgotten multiple times :) (as it isn't directly implied by the HID 
specification, but is rather a workaround for devices that are violating 
the spec by looking at the bits they shouldn't care about).

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.

Re: Zeroing the report before setting fields

[Pete Zaitcev]

On Mon, 22 Mar 2010 17:30:31 +0100 (CET)
Jiri Kosina <jkosina@suse.cz> wrote:

> >  	if (dir == USB_DIR_OUT) {
> > -		usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC);
> > +		usbhid->ctrl[usbhid->ctrlhead].raw_report = kzalloc(len, GFP_ATOMIC);
> >  		if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) {
> >  			dev_warn(&hid->dev, "control queueing failed\n");

> Frankly, I like the generic aproach more ... if we were to add more 
> low-level transport HID implementations later, I am sure this will be 
> forgotten multiple times :) (as it isn't directly implied by the HID 
> specification, but is rather a workaround for devices that are violating 
> the spec by looking at the bits they shouldn't care about).

You are right, I screwed the pooch on the layering. Sorry.

Still, I do not want to go back to counting of field lengths.
Too many what-ifs.

How about this (I tested it but clearly my tests are limited):

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 368fbb0..e290bb3 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -940,13 +940,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));
@@ -966,6 +961,7 @@ void hid_output_report(struct hid_report *report, __u8 *data)
 	if (report->id > 0)
 		*data++ = report->id;
 
+	memset(data, 0, ((report->size - 1) >> 3) + 1);
 	for (n = 0; n < report->maxfield; n++)
 		hid_output_field(report->field[n], data);
 }

Yours,
-- Pete

Re: Zeroing the report before setting fields

[Jiri Kosina]

On Mon, 22 Mar 2010, Pete Zaitcev wrote:

> > >  	if (dir == USB_DIR_OUT) {
> > > -		usbhid->ctrl[usbhid->ctrlhead].raw_report = kmalloc(len, GFP_ATOMIC);
> > > +		usbhid->ctrl[usbhid->ctrlhead].raw_report = kzalloc(len, GFP_ATOMIC);
> > >  		if (!usbhid->ctrl[usbhid->ctrlhead].raw_report) {
> > >  			dev_warn(&hid->dev, "control queueing failed\n");
> 
> > Frankly, I like the generic aproach more ... if we were to add more 
> > low-level transport HID implementations later, I am sure this will be 
> > forgotten multiple times :) (as it isn't directly implied by the HID 
> > specification, but is rather a workaround for devices that are violating 
> > the spec by looking at the bits they shouldn't care about).
> 
> You are right, I screwed the pooch on the layering. Sorry.
> 
> Still, I do not want to go back to counting of field lengths.
> Too many what-ifs.
> 
> How about this (I tested it but clearly my tests are limited):
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 368fbb0..e290bb3 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -940,13 +940,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));
> @@ -966,6 +961,7 @@ void hid_output_report(struct hid_report *report, __u8 *data)
>  	if (report->id > 0)
>  		*data++ = report->id;
>  
> +	memset(data, 0, ((report->size - 1) >> 3) + 1);
>  	for (n = 0; n < report->maxfield; n++)
>  		hid_output_field(report->field[n], data);
>  }

This looks basically fine. Could you please send it with chagelog and your 
Signed-off-by entry?

Thanks,

-- 
Jiri Kosina
SUSE Labs, Novell Inc.