From patchwork Sun Mar 12 14:32:47 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Tomasz Kramkowski X-Patchwork-Id: 9619409 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 4E53C60417 for ; Sun, 12 Mar 2017 14:33:00 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3C2E528440 for ; Sun, 12 Mar 2017 14:33:00 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 2EBD628477; Sun, 12 Mar 2017 14:33:00 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.8 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_HI,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 3ECD128440 for ; Sun, 12 Mar 2017 14:32:59 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933406AbdCLOc6 (ORCPT ); Sun, 12 Mar 2017 10:32:58 -0400 Received: from erebus.the-tk.com ([109.74.205.187]:56684 "EHLO erebus.the-tk.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932364AbdCLOc5 (ORCPT ); Sun, 12 Mar 2017 10:32:57 -0400 Received: from erebus.the-tk.com (localhost [127.0.0.1]) by erebus.the-tk.com (OpenSMTPD) with ESMTP id a5896882; Sun, 12 Mar 2017 14:32:52 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=the-tk.com; h=date:from:to :cc:subject:message-id:references:mime-version:content-type :in-reply-to; s=sel0; bh=CMezT8LaDlbVQZ6F+BUJTVEyNHM=; b=NxcXJsN QVZ8dzdaaXsrNCQapgzrea0lw8x4mkUyuHrMYFH5WTLeKQLL9/UoFSzo9dQlv91u 8SLyD6YmzkbqjmfeGTvzw4u3cQt6p0MQ7FUom2qPVVYCY8UCzksrZkusqfV5Jq7R Cb+SavwYP/UgB7+QTIfzzewcnSmAIVp9c4FI= DomainKey-Signature: a=rsa-sha1; c=nofws; d=the-tk.com; h=date:from:to :cc:subject:message-id:references:mime-version:content-type :in-reply-to; q=dns; s=sel0; b=zyZyrz/Uc2jWiWKZ6HQ+P/9F1DIJEKSHf Nf1BXHERgWB1KRAgbadyB9aLdT5jHzyrQhtR6uKDWKJ8bYwBKJObfdI13+yW2V+o ce18LmkwXnMWl9ZQnB1YSe+5rFflg3C+kMVSjqzSJARv9TjhaVoT9BwTGkNGqPvK jGoROpUE6I= Received: from localhost (cpc95214-derb16-2-0-cust970.8-3.cable.virginm.net [86.15.167.203]) by erebus.the-tk.com (OpenSMTPD) with ESMTPSA id 121670b5 (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256:NO); Sun, 12 Mar 2017 14:32:52 +0000 (UTC) Date: Sun, 12 Mar 2017 14:32:47 +0000 From: Tomasz Kramkowski To: Benjamin Tissoires Cc: Jiri Kosina , 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 Message-ID: <20170312143247.GA938@gaia.local> References: <20170308215211.24263-1-tk@the-tk.com> <20170309081606.GA28052@mail.corp.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20170309081606.GA28052@mail.corp.redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-input-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-input@vger.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP 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); } /*