Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1698777 > unrolled thread

[PATCH] HID: wacom: add USB_HID dependency

Back to article view | Back to linux.kernel

Contents

  [PATCH] HID: wacom: add USB_HID dependency Arnd Bergmann <arnd@arndb.de> - 2017-07-28 15:20 +0200
    Re: [PATCH] HID: wacom: add USB_HID dependency Jason Gerecke <killertofu@gmail.com> - 2017-07-28 16:10 +0200
      Re: [PATCH] HID: wacom: add USB_HID dependency Arnd Bergmann <arnd@arndb.de> - 2017-07-28 16:20 +0200
        Re: [PATCH] HID: wacom: add USB_HID dependency Jason Gerecke <killertofu@gmail.com> - 2017-07-28 16:30 +0200
          Re: [PATCH] HID: wacom: add USB_HID dependency Arnd Bergmann <arnd@arndb.de> - 2017-07-28 16:30 +0200
            Re: [PATCH] HID: wacom: add USB_HID dependency Jason Gerecke <killertofu@gmail.com> - 2017-07-28 17:20 +0200

#1698777 — [PATCH] HID: wacom: add USB_HID dependency

The driver has gained a compile-time dependency that we should
express in Kconfig to avoid this link error:

drivers/hid/wacom_sys.o: In function `wacom_parse_and_register':
wacom_sys.c:(.text+0x2eec): undefined reference to `usb_hid_driver'

Fixes: 09dc28acaec7 ("HID: wacom: Improve generic name generation")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/hid/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
index 3cd60f460b61..0a3117cc29e7 100644
--- a/drivers/hid/Kconfig
+++ b/drivers/hid/Kconfig
@@ -924,7 +924,7 @@ config HID_UDRAW_PS3
 
 config HID_WACOM
 	tristate "Wacom Intuos/Graphire tablet support (USB)"
-	depends on HID
+	depends on USB_HID
 	select POWER_SUPPLY
 	select NEW_LEDS
 	select LEDS_CLASS
-- 
2.9.0

[toc] | [next] | [standalone]

#1698819

On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <arnd@arndb.de> wrote:
>The driver has gained a compile-time dependency that we should
>express in Kconfig to avoid this link error:
>

Would conditional compilation be an acceptable alternative to adding a dependency? The USB_HID code is only used to check if the driver is working with a USB device. With USB_HID disabled, there's no need for the check so there's no need for the dependency.

>drivers/hid/wacom_sys.o: In function `wacom_parse_and_register':
>wacom_sys.c:(.text+0x2eec): undefined reference to `usb_hid_driver'
>
>Fixes: 09dc28acaec7 ("HID: wacom: Improve generic name generation")
>Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>---
> drivers/hid/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/drivers/hid/Kconfig b/drivers/hid/Kconfig
>index 3cd60f460b61..0a3117cc29e7 100644
>--- a/drivers/hid/Kconfig
>+++ b/drivers/hid/Kconfig
>@@ -924,7 +924,7 @@ config HID_UDRAW_PS3
> 
> config HID_WACOM
> 	tristate "Wacom Intuos/Graphire tablet support (USB)"
>-	depends on HID
>+	depends on USB_HID
> 	select POWER_SUPPLY
> 	select NEW_LEDS
> 	select LEDS_CLASS


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

[toc] | [prev] | [next] | [standalone]

#1698833

On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <arnd@arndb.de> wrote:
>>The driver has gained a compile-time dependency that we should
>>express in Kconfig to avoid this link error:
>>
>
> Would conditional compilation be an acceptable alternative to adding
> a dependency? The USB_HID code is only used to check if the driver
> is working with a USB device. With USB_HID disabled, there's no need
> for the check so there's no need for the dependency.

I think that should work, e.g. you could replace the hid_is_using_ll_driver
and 'extern' defintions with a helper per ll-driver like

#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

[toc] | [prev] | [next] | [standalone]

#1698842

On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>> On July 28, 2017 6:18:00 AM PDT, Arnd Bergmann <arnd@arndb.de> wrote:
>>>The driver has gained a compile-time dependency that we should
>>>express in Kconfig to avoid this link error:
>>>
>>
>> Would conditional compilation be an acceptable alternative to adding
>> a dependency? The USB_HID code is only used to check if the driver
>> is working with a USB device. With USB_HID disabled, there's no need
>> for the check so there's no need for the dependency.
>
> I think that should work, e.g. you could replace the hid_is_using_ll_driver
> and 'extern' defintions with a helper per ll-driver like
>
> #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)) {
[...]
                }
#endif

Jason

[toc] | [prev] | [next] | [standalone]

#1698850

On Fri, Jul 28, 2017 at 4:24 PM, Jason Gerecke <killertofu@gmail.com> wrote:
> On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.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

[toc] | [prev] | [next] | [standalone]

#1698896

On Fri, Jul 28, 2017 at 7:29 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Fri, Jul 28, 2017 at 4:24 PM, Jason Gerecke <killertofu@gmail.com> wrote:
>> On Fri, Jul 28, 2017 at 7:18 AM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Fri, Jul 28, 2017 at 4:07 PM, Jason Gerecke <killertofu@gmail.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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web