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 14:17:27 -0800
From:   Doug Anderson <dianders@...omium.org>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Jiri Kosina <jkosina@...e.cz>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        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

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...


> <snip>
>
> > diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> > index aeff1ffb0c8b..9551ba96dc49 100644
> > --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> > +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> > @@ -35,12 +35,8 @@
> >  #include <linux/kernel.h>
> >  #include <linux/hid.h>
> >  #include <linux/mutex.h>
> > -#include <linux/acpi.h>
> > -#include <linux/of.h>
> >  #include <linux/regulator/consumer.h>
>
> I think you can drop this regulator include here now too ?

Good point.


> > diff --git a/drivers/hid/i2c-hid/i2c-hid-of.c b/drivers/hid/i2c-hid/i2c-hid-of.c
> > new file mode 100644
> > index 000000000000..15d533be2b24
> > --- /dev/null
> > +++ b/drivers/hid/i2c-hid/i2c-hid-of.c
> <snip>
>
> > +static const struct dev_pm_ops i2c_hid_of_pm = {
> > +     SET_SYSTEM_SLEEP_PM_OPS(i2c_hid_core_suspend, i2c_hid_core_resume)
> > +};
>
> This dev_pm_ops struct is identical to the one in i2c-hid-acpi.c
> and the one which you introduce later in i2c-hid-of-goodix.c
> is also identical, so that is 3 copies.
>
> Maybe just put a shared dev_pm_ops struct in the i2c-core
> (and don't export the suspend/resume handlers) ?

Sounds good.

-Doug