linux-kernel - Re: [PATCH v4 3/4] HID: logitech: Add feature 0x0001: FeatureSet

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAO-hwJLAwodSXpz-_T0nGqnjHY3cVAu9GYjvp8SW8grTXX+VYg@mail.gmail.com>
Date:   Fri, 11 Oct 2019 10:45:54 +0200
From:   Benjamin Tissoires <benjamin.tissoires@...hat.com>
To:     Mazin Rezk <mnrzk@...tonmail.com>
Cc:     "linux-input@...r.kernel.org" <linux-input@...r.kernel.org>,
        "jikos@...nel.org" <jikos@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Filipe Laíns <lains@...hlinux.org>
Subject: Re: [PATCH v4 3/4] HID: logitech: Add feature 0x0001: FeatureSet

On Fri, Oct 11, 2019 at 10:33 AM Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
>
> On Fri, Oct 11, 2019 at 2:57 AM Mazin Rezk <mnrzk@...tonmail.com> wrote:
> >
> > On Saturday, October 5, 2019 9:04 PM, Mazin Rezk <mnrzk@...tonmail.com> wrote:
> >
> > > This patch adds support for the 0x0001 (FeatureSet) feature. This feature
> > > is used to look up the feature ID of a feature index on a device and list
> > > the total count of features on the device.
> > >
> > > I also added the hidpp20_get_features function which iterates through all
> > > feature indexes on the device and stores a map of them in features an
> > > hidpp_device struct. This function runs when an HID++ 2.0 device is probed.
> >
> > Changes in the version:
> >  - Renamed hidpp20_featureset_get_feature to
> >    hidpp20_featureset_get_feature_id.
> >
> >  - Re-ordered hidpp20_featureset_get_count and
> >    hidpp20_featureset_get_feature_id based on their command IDs.
> >
> >  - Made feature_count initialize to 0 before running hidpp20_get_features.
>
> I still need to decide whether or not we need to take this one. We
> historically did not do this to mimic the Windows driver at the time.
> However, the driver is full of quirks that could be detected instead
> of hardcoded thanks to this functions. So we might want to switch to
> auto-detection of those quirks so we can keep 'quirks' for actual
> defects that can't be auto-detected.
>
> But, if we want to go this route, we need to actually make use of it.
> So this patch should be part of a series where we use it, not added by
> its own.
>
> Can you drop it from the series?
> And maybe possibly work on a cleanup of some of the auto detection,
> like the HIDPP_QUIRK_HI_RES_SCROLL_X2121 which you can easily test?
> But this would need to happen in a second series, once this one gets
> merged in.

While reviewing 4/4, I realized why you need this one.
See my comments in 4/4, I still believe the case is not strong enough
to spend some time to enable the feature for barely no gain.

Cheers,
Benjamin