[PATCH] HID: input: Finish TransducerSerialNumber implementation

[PATCH] HID: input: Finish TransducerSerialNumber implementation

[Jason Gerecke]

The commit which introduced TransducerSerialNumber (368c966) is missing
two crucial implementation details. Firstly, the commit does not set the
type/code/bit/max fields as expected later down the code which can cause
the driver to crash when a tablet with this usage is connected. Secondly,
the code to send a MSC_SERIAL event to userspace when this usage is seen
in a report is nowhere to be found. This commit addresses both issues.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/hid-input.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2619f7f..abed624 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -690,6 +690,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 
 		case 0x5b: /* TransducerSerialNumber */
 			set_bit(MSC_SERIAL, input->mscbit);
+			usage->type = EV_MSC;
+			usage->code = MSC_SERIAL;
+			bit = input->mscbit;
+			max = MSC_MAX;
 			break;
 
 		default:  goto unknown;
@@ -1041,6 +1045,11 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
 		input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
 	}
 
+	if (usage->hid == (HID_UP_DIGITIZER | 0x005b)) { /* TransducerSerialNumber */
+		input_event(input, EV_MSC, MSC_SERIAL, value);
+		return;
+	}
+
 	if (usage->hid == (HID_UP_PID | 0x83UL)) { /* Simultaneous Effects Max */
 		dbg_hid("Maximum Effects - %d\n",value);
 		return;
-- 
2.1.0

Re: [PATCH] HID: input: Finish TransducerSerialNumber implementation

[Benjamin Tissoires]

Hi Jason,

On Sep 22 2014 or thereabouts, Jason Gerecke wrote:
> The commit which introduced TransducerSerialNumber (368c966) is missing
> two crucial implementation details. Firstly, the commit does not set the
> type/code/bit/max fields as expected later down the code which can cause
> the driver to crash when a tablet with this usage is connected. Secondly,
> the code to send a MSC_SERIAL event to userspace when this usage is seen
> in a report is nowhere to be found. This commit addresses both issues.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
>  drivers/hid/hid-input.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 2619f7f..abed624 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -690,6 +690,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  
>  		case 0x5b: /* TransducerSerialNumber */
>  			set_bit(MSC_SERIAL, input->mscbit);
> +			usage->type = EV_MSC;
> +			usage->code = MSC_SERIAL;
> +			bit = input->mscbit;
> +			max = MSC_MAX;
>  			break;
>  
>  		default:  goto unknown;
> @@ -1041,6 +1045,11 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>  		input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a) >> 3));
>  	}
>  
> +	if (usage->hid == (HID_UP_DIGITIZER | 0x005b)) { /* TransducerSerialNumber */
> +		input_event(input, EV_MSC, MSC_SERIAL, value);
> +		return;
> +	}
> +

Are you sure that the generic call to 
"input_event(input, usage->type, usage->code, value);" in the end of
hidinput_hid_event() is not sufficient enough?

As long as the usage/code are set in the hid_usage struct, you should be
fine.

Cheers,
Benjamin

>  	if (usage->hid == (HID_UP_PID | 0x83UL)) { /* Simultaneous Effects Max */
>  		dbg_hid("Maximum Effects - %d\n",value);
>  		return;
> -- 
> 2.1.0
> 

RE: [PATCH] HID: input: Finish TransducerSerialNumber implementation

[Jason Gerecke]

> -----Original Message-----
> From: Benjamin Tissoires [mailto:benjamin.tissoires@redhat.com]
> Sent: Tuesday, September 23, 2014 8:12 AM
> To: Jason Gerecke
> Cc: linux-input@vger.kernel.org; jkosina@suse.cz; pinglinux@gmail.com;
> Jason Gerecke
> Subject: Re: [PATCH] HID: input: Finish TransducerSerialNumber
> implementation
> 
> Hi Jason,
> 
> On Sep 22 2014 or thereabouts, Jason Gerecke wrote:
> > The commit which introduced TransducerSerialNumber (368c966) is
> > missing two crucial implementation details. Firstly, the commit does
> > not set the type/code/bit/max fields as expected later down the code
> > which can cause the driver to crash when a tablet with this usage is
> > connected. Secondly, the code to send a MSC_SERIAL event to userspace
> > when this usage is seen in a report is nowhere to be found. This commit
> addresses both issues.
> >
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > ---
> >  drivers/hid/hid-input.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c index
> > 2619f7f..abed624 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -690,6 +690,10 @@ static void hidinput_configure_usage(struct
> > hid_input *hidinput, struct hid_fiel
> >
> >  		case 0x5b: /* TransducerSerialNumber */
> >  			set_bit(MSC_SERIAL, input->mscbit);
> > +			usage->type = EV_MSC;
> > +			usage->code = MSC_SERIAL;
> > +			bit = input->mscbit;
> > +			max = MSC_MAX;
> >  			break;
> >
> >  		default:  goto unknown;
> > @@ -1041,6 +1045,11 @@ void hidinput_hid_event(struct hid_device *hid,
> struct hid_field *field, struct
> >  		input_event(input, EV_KEY, BTN_TOUCH, value > a + ((b - a)
> >> 3));
> >  	}
> >
> > +	if (usage->hid == (HID_UP_DIGITIZER | 0x005b)) { /*
> TransducerSerialNumber */
> > +		input_event(input, EV_MSC, MSC_SERIAL, value);
> > +		return;
> > +	}
> > +
> 
> Are you sure that the generic call to
> "input_event(input, usage->type, usage->code, value);" in the end of
> hidinput_hid_event() is not sufficient enough?
> 
> As long as the usage/code are set in the hid_usage struct, you should be fine.
> 
> Cheers,
> Benjamin
> 

Hmmm... This might be an issue with the HID descriptor, actually. It looks like my hardware doesn't set the logical minimum and maximum for the field, so the (quite out-of-range) value is ignored. I start seeing data after patching the descriptor to add these in, but the code is set to MSC_PULSELED instead of MSC_SERIAL. I suspect that the existing call to 'set_bit' in the TransducerSerialNumber case is to blame (which makes it act like 'map_foo' instead of 'map_foo_clear').

Updated patch forthcoming...

Jason

> >  	if (usage->hid == (HID_UP_PID | 0x83UL)) { /* Simultaneous Effects
> Max */
> >  		dbg_hid("Maximum Effects - %d\n",value);
> >  		return;
> > --
> > 2.1.0
> >

[PATCH] HID: input: Fix TransducerSerialNumber implementation

[Jason Gerecke]

The commit which introduced TransducerSerialNumber (368c966) is missing
two crucial implementation details. Firstly, the commit does not set the
type/code/bit/max fields as expected later down the code which can cause
the driver to crash when a tablet with this usage is connected. Secondly,
the call to 'set_bit' causes MSC_PULSELED to be sent instead of the
expected MSC_SERIAL. This commit addreses both issues.

Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
---
 drivers/hid/hid-input.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index 2619f7f..cb1b3fa 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -689,7 +689,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
 			break;
 
 		case 0x5b: /* TransducerSerialNumber */
-			set_bit(MSC_SERIAL, input->mscbit);
+			usage->type = EV_MSC;
+			usage->code = MSC_SERIAL;
+			bit = input->mscbit;
+			max = MSC_MAX;
 			break;
 
 		default:  goto unknown;
-- 
2.1.0

Re: [PATCH] HID: input: Fix TransducerSerialNumber implementation

[Jason Gerecke]

On Tue, Sep 23, 2014 at 11:09 AM, Jason Gerecke <killertofu@gmail.com> wrote:
> The commit which introduced TransducerSerialNumber (368c966) is missing
> two crucial implementation details. Firstly, the commit does not set the
> type/code/bit/max fields as expected later down the code which can cause
> the driver to crash when a tablet with this usage is connected. Secondly,
> the call to 'set_bit' causes MSC_PULSELED to be sent instead of the
> expected MSC_SERIAL. This commit addreses both issues.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
>  drivers/hid/hid-input.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 2619f7f..cb1b3fa 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -689,7 +689,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                         break;
>
>                 case 0x5b: /* TransducerSerialNumber */
> -                       set_bit(MSC_SERIAL, input->mscbit);
> +                       usage->type = EV_MSC;
> +                       usage->code = MSC_SERIAL;
> +                       bit = input->mscbit;
> +                       max = MSC_MAX;
>                         break;
>
>                 default:  goto unknown;
> --
> 2.1.0
>

This patch still seems to be in limbo. Anyone (Ping? Benjamin?) care
to provide an ACK or review?

Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one  /
(That is to say, eight) to the two,     /
But you can’t take seven from three,    /
So you look at the sixty-fours....
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] HID: input: Fix TransducerSerialNumber implementation

[Benjamin Tissoires]

On Oct 28 2014 or thereabouts, Jason Gerecke wrote:
> On Tue, Sep 23, 2014 at 11:09 AM, Jason Gerecke <killertofu@gmail.com> wrote:
> > The commit which introduced TransducerSerialNumber (368c966) is missing
> > two crucial implementation details. Firstly, the commit does not set the
> > type/code/bit/max fields as expected later down the code which can cause
> > the driver to crash when a tablet with this usage is connected. Secondly,
> > the call to 'set_bit' causes MSC_PULSELED to be sent instead of the
> > expected MSC_SERIAL. This commit addreses both issues.
> >
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > ---
> >  drivers/hid/hid-input.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index 2619f7f..cb1b3fa 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -689,7 +689,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
> >                         break;
> >
> >                 case 0x5b: /* TransducerSerialNumber */
> > -                       set_bit(MSC_SERIAL, input->mscbit);
> > +                       usage->type = EV_MSC;
> > +                       usage->code = MSC_SERIAL;
> > +                       bit = input->mscbit;
> > +                       max = MSC_MAX;
> >                         break;
> >
> >                 default:  goto unknown;
> > --
> > 2.1.0
> >
> 
> This patch still seems to be in limbo. Anyone (Ping? Benjamin?) care
> to provide an ACK or review?
> 

Sorry for that, I was persuaded I already have answered to this one.

The patch makes sense, but I think it will be better to extend
hid_map_usage() with MSC bits and define map_msc_clear(). The code will
be more consistent with the rest this way.

Anyway, if Jiri does not find this to be mandatory, then this code is:
Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

Fwd: [PATCH] HID: input: Fix TransducerSerialNumber implementation

[Ping Cheng]

On Tue, Sep 23, 2014 at 11:09 AM, Jason Gerecke <killertofu@gmail.com> wrote:
>
> The commit which introduced TransducerSerialNumber (368c966) is missing
> two crucial implementation details. Firstly, the commit does not set the
> type/code/bit/max fields as expected later down the code which can cause
> the driver to crash when a tablet with this usage is connected. Secondly,
> the call to 'set_bit' causes MSC_PULSELED to be sent instead of the
> expected MSC_SERIAL. This commit addreses both issues.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>


Reviewed-by: Ping Cheng <pingc@wacom.com>

Thank you, Jason, for the fix.

Ping

> ---
>  drivers/hid/hid-input.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 2619f7f..cb1b3fa 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -689,7 +689,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>                         break;
>
>                 case 0x5b: /* TransducerSerialNumber */
> -                       set_bit(MSC_SERIAL, input->mscbit);
> +                       usage->type = EV_MSC;
> +                       usage->code = MSC_SERIAL;
> +                       bit = input->mscbit;
> +                       max = MSC_MAX;
>                         break;
>
>                 default:  goto unknown;
> --
> 2.1.0
>

Re: [PATCH] HID: input: Fix TransducerSerialNumber implementation

[Jiri Kosina]

On Tue, 23 Sep 2014, Jason Gerecke wrote:

> The commit which introduced TransducerSerialNumber (368c966) is missing
> two crucial implementation details. Firstly, the commit does not set the
> type/code/bit/max fields as expected later down the code which can cause
> the driver to crash when a tablet with this usage is connected. Secondly,
> the call to 'set_bit' causes MSC_PULSELED to be sent instead of the
> expected MSC_SERIAL. This commit addreses both issues.
> 
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> ---
>  drivers/hid/hid-input.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index 2619f7f..cb1b3fa 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -689,7 +689,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>  			break;
>  
>  		case 0x5b: /* TransducerSerialNumber */
> -			set_bit(MSC_SERIAL, input->mscbit);
> +			usage->type = EV_MSC;
> +			usage->code = MSC_SERIAL;
> +			bit = input->mscbit;
> +			max = MSC_MAX;
>  			break;
>  

Apologizes for the delay. I have now queued this for 3.18 still.

-- 
Jiri Kosina
SUSE Labs