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:   Wed, 11 Nov 2020 12:10:09 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Doug Anderson <dianders@...omium.org>
Cc:     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

Hi,

On 11/11/20 1:04 AM, Dmitry Torokhov wrote:
> 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?

That should keep working. Unless someone mixes a DT-style compatible string
with the PNP0C50 ACPI HID specific DSM for getting the hid_descriptor_address
(instead of a DT style property). But that would be a really weird mix to
use and obviously would go against both the PNP0C50 and the PRP0001 specs.

Regards,

Hans