Re: Zeroing the report before setting fields

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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