linux-kernel - Re: [PATCH v5 1/4] HID: i2c-hid: Reorganize so ACPI and OF are separate modules

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] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Nov 2020 16:04:58 -0800
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Doug Anderson <dianders@...omium.org>
Cc:     Hans de Goede <hdegoede@...hat.com>, Jiri Kosina <jkosina@...e.cz>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kai-Heng Feng <kai.heng.feng@...onical.com>,
        Rob Herring <robh+dt@...nel.org>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        Stephen Boyd <swboyd@...omium.org>,
        Andrea Borgia <andrea@...gia.bo.it>,
        Jiri Kosina <jikos@...nel.org>,
        Masahiro Yamada <masahiroy@...nel.org>,
        Pavel Balan <admin@...ma.net>,
        Xiaofei Tan <tanxiaofei@...wei.com>,
        You-Sheng Yang <vicamo.yang@...onical.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v5 1/4] HID: i2c-hid: Reorganize so ACPI and OF are
 separate modules

On Tue, Nov 10, 2020 at 02:17:27PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Tue, Nov 10, 2020 at 1:01 AM Hans de Goede <hdegoede@...hat.com> wrote:
> >
> > Hi,
> >
> > On 11/9/20 10:36 PM, Douglas Anderson wrote:
> > > This patch rejiggers the i2c-hid code so that the OF (Open Firmware
> > > aka Device Tree) and ACPI support is separated out a bit.  The OF and
> > > ACPI drivers are now separate modules that wrap the core module.
> > >
> > > Essentially, what we're doing here:
> > > * Make "power up" and "power down" a function that can be (optionally)
> > >   implemented by a given user of the i2c-hid core.
> > > * The OF and ACPI modules are drivers on their own, so they implement
> > >   probe / remove / suspend / resume / shutdown.  The core code
> > >   provides implementations that OF and ACPI can call into.
> > >
> > > We'll organize this so that we now have 3 modules: the old i2c-hid
> > > module becomes the "core" module and two new modules will depend on
> > > it, handling probing the specific device.
> > >
> > > As part of this work, we'll remove the i2c-hid "platform data"
> > > concept since it's not needed.
> > >
> > > Signed-off-by: Douglas Anderson <dianders@...omium.org>
> > > ---
> > >
> > > Changes in v5:
> > > - Add shutdown_tail op and use it in ACPI.
> > > - i2chid_subclass_data => i2chid_ops.
> > > - power_up_device => power_up (same with power_down).
> > > - subclass => ops.
> > >
> >
> > Thanks this looks good to now, 2 small remarks below (since you are
> > going to do a v6 anyways). Feel free to ignore these remarks if
> > you prefer to keep things as is.
> >
> > And feel free to add my reviewed-by to v6 of this patch:
> >
> > Reviewed-by: Hans de Goede <hdegoede@...hat.com>
> 
> Thanks!
> 
> 
> > > +static const struct i2c_device_id i2c_hid_acpi_id_table[] = {
> > > +     { "hid", 0 },
> > > +     { "hid-over-i2c", 0 },
> > > +     { },
> > > +};
> > > +MODULE_DEVICE_TABLE(i2c, i2c_hid_acpi_id_table);
> >
> > Hmm, I do not think these old-style i2c-ids are necessarry at
> > all in this driver. I expect all use-cases to use either
> > of or acpi matches.
> >
> > This was already present in the code before though, so
> > please ignore this remark. This is just something which
> > I noticed and thought was worth while pointing out as
> > a future cleanup.
> 
> Yeah, I wasn't sure if there was anyone using them.
> 
> Hrm.  Thinking about it, though, is it really OK for two drivers to
> both have the same table listed?  I'm not sure how that would work.
> Do you know?
> 
> I don't know a ton about ACPI, but for device tree I know i2c has a
> fallback mode.  Specifically having this table means that we'll match
> compatible strings such as:
> 
>   "zipzapzing,hid"
>   "kapowzers,hid-over-i2c"
> 
> In other words it'll ignore the vendor part and just match on the
> second half.  Just to make sure I wasn't remembering that from a dream
> I tried it and it worked.  I don't see any mainline device trees that
> look like that, though.  I could delete it, though it doesn't really
> take up much space and it seems nice to keep it working in case anyone
> was relying on it?
> 
> For ACPI is there a similar fallback?  If not then it seems like it'd
> be easy to remove it from there...

Just a random thought - will all this still be working with ACPI PRP0001
and DT-style compatible string and properties in _DSD?

Thanks.

-- 
Dmitry