linux-kernel - Re: [PATCH] HID: wacom: add USB

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]

Message-ID: <CANRwn3QGovTdAhbVtc1MYzogEir+P3niiRCUPDG80XQH6eHxKg@mail.gmail.com>
Date:   Fri, 28 Jul 2017 08:15:28 -0700
From:   Jason Gerecke <killertofu@...il.com>
To:     Arnd Bergmann <arnd@...db.de>
Cc:     Jiri Kosina <jikos@...nel.org>,
        Jason Gerecke <jason.gerecke@...om.com>,
        Benjamin Tissoires <benjamin.tissoires@...hat.com>,
        "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] HID: wacom: add USB_HID dependency

On Fri, Jul 28, 2017 at 7:29 AM, Arnd Bergmann <arnd@...db.de> wrote:
> On Fri, Jul 28, 2017 at 4:24 PM, Jason Gerecke <killertofu@...il.com> wrote:
>> On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <arnd@...db.de> wrote:
>>> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@...il.com> wrote:
>
>>> #ifdef CONFIG_USB_HID
>>> extern bool hid_is_using_usb_driver(struct hid_device *hdev)
>>> #else
>>> static inline bool hid_is_using_usb_driver(struct hid_device *hdev)
>>> {
>>>        return false;
>>> }
>>> #endif
>>>
>>> but is it worth it to avoid the dependency?
>>>
>>>        Arnd
>>
>> I was thinking something more along the lines of the following since
>> the idea of per-transport helper functions was dismissed earlier:
>>
>> #ifdef CONFIG_USB_HID
>>                 if (hid_is_using_ll_driver(wacom->hdev, &usb_hid_driver)) {
>
> I would consider that rather ugly, a driver shouldn't really use
> #ifdef like this, but you can hide stuff like this in a header. The method
> I proposed also has the advantage of avoiding exporting the
> usb_hid_driver object. Drivers shouldn't really need to access this,
> and wacom_sys.c is the only remaining user of the export.
>
>       Arnd

The exports and hid_is_using_ll_driver were actually introduced in the
patch immediately preceding the change to wacom_sys.c which triggered
this error (making it the "first", not "last" user).

That said, after reading through the patch discussion[1] again, I see
that my memory is faulty: per-transport functions were *not*
dismissed. Rather, a more-generic function that is fed the
hid_ll_driver of interest was suggested instead. Given that these
exports are liable to cause this same issue for future users, perhaps
providing per-transport functions is the better option after all.

I could accept either the strict dependency you originally suggested
or a modified header, but don't see much reason for the former.
Checking if a HID device is using a particular transport shouldn't
require that that transport even be available IMO, but that's
ultimately not my call...

Jiri? Benjamin?

[1]: https://patchwork.kernel.org/patch/9815539/

Jason