From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Tomasz Kramkowski <tk@the-tk.com>
Cc: Jiri Kosina <jikos@kernel.org>,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] HID: clamp input to logical range if no null state
Date: Mon, 13 Mar 2017 10:47:20 +0100 [thread overview]
Message-ID: <20170313094720.GC4378@mail.corp.redhat.com> (raw)
In-Reply-To: <20170312143247.GA938@gaia.local>
On Mar 12 2017 or thereabouts, Tomasz Kramkowski wrote:
> On Thu, Mar 09, 2017 at 09:16:06AM +0100, Benjamin Tissoires wrote:
> > We have a "clamp()" function in the kernel that does the job directly
> > and which is more readable. Also, this makes testing the out of range
> > values twice.
> >
> > How about:
> >
> > diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> > index cf8256a..781f400 100644
> > --- a/drivers/hid/hid-input.c
> > +++ b/drivers/hid/hid-input.c
> > @@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
> >
> > /*
> > * Ignore out-of-range values as per HID specification,
> > - * section 5.10 and 6.2.25.
> > + * section 5.10 and 6.2.25, when NULL state bit is present.
> > + * When it's not, clamp the value to match Microsoft's input
> > + * driver as mentioned in "Required HID usages for digitizers":
> > + * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
> > *
> > * The logical_minimum < logical_maximum check is done so that we
> > * don't unintentionally discard values sent by devices which
> > * don't specify logical min and max.
> > */
> > if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
> > - (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
> > - (field->logical_minimum < field->logical_maximum) &&
> > - (value < field->logical_minimum ||
> > - value > field->logical_maximum)) {
> > - dbg_hid("Ignoring out-of-range value %x\n", value);
> > - return;
> > + (field->logical_minimum < field->logical_maximum)) {
> > }
>
> Yes, I don't mind the expansion of the comment and the usage of clamp (I
> didn't know this existed, but I will use it in the future). However if
> there is anything I would change, it would be this:
>
> ---
> drivers/hid/hid-input.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index cf8256aac2bd..a1ebdd7d4d4d 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1150,19 +1150,26 @@ void hidinput_hid_event(struct hid_device *hid, struct hid_field *field, struct
>
> /*
> * Ignore out-of-range values as per HID specification,
> - * section 5.10 and 6.2.25.
> + * section 5.10 and 6.2.25, when NULL state bit is present.
> + * When it's not, clamp the value to match Microsoft's input
> + * driver as mentioned in "Required HID usages for digitizers":
> + * https://msdn.microsoft.com/en-us/library/windows/hardware/dn672278(v=vs.85).asp
> *
> * The logical_minimum < logical_maximum check is done so that we
> * don't unintentionally discard values sent by devices which
> * don't specify logical min and max.
> */
> if ((field->flags & HID_MAIN_ITEM_VARIABLE) &&
> - (field->flags & HID_MAIN_ITEM_NULL_STATE) &&
> - (field->logical_minimum < field->logical_maximum) &&
> - (value < field->logical_minimum ||
> - value > field->logical_maximum)) {
> - dbg_hid("Ignoring out-of-range value %x\n", value);
> - return;
> + (field->logical_minimum < field->logical_maximum)) {
> + if (field->flags & HID_MAIN_ITEM_NULL_STATE &&
> + (value < field->logical_minimum ||
> + value > field->logical_maximum)) {
> + dbg_hid("Ignoring out-of-range value %x\n", value);
> + return;
> + }
> + value = clamp(value,
> + field->logical_minimum,
> + field->logical_maximum);
> }
>
> /*
> --
> 2.12.0
>
> For me it is a bit clearer on what is happening and still avoids doing
> the range check twice. But ultimately it is all up to you guys.
Works for me.
>
> I can get both versions of this patch tested at some point in the next
> few days and re-submit whichever one you prefer as a v2.
>
> I'm not sure what the procedures are on this, should I put a
> "Suggested-by:" for your suggested change to my patch, or is that not
> applicable here?
No need to add suggested-by. This tag, IMO, is there to give credit on
an idea, while here it was more a common effort :)
Just add my Acked-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
if you resubmit it - after the tests :)
Cheers,
Benjamin
>
> As always, thanks for your time.
>
> --
> Tomasz Kramkowski | GPG: 40B037BA0A5B8680 | Web: https://the-tk.com/
prev parent reply other threads:[~2017-03-13 9:47 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-03-08 21:52 [PATCH 1/1] HID: clamp input to logical range if no null state Tomasz Kramkowski
2017-03-09 8:16 ` Benjamin Tissoires
2017-03-12 14:32 ` Tomasz Kramkowski
2017-03-13 9:47 ` Benjamin Tissoires [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20170313094720.GC4378@mail.corp.redhat.com \
--to=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=tk@the-tk.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).