[PATCH] HID: wacom: add USB_HID dependency

[PATCH] HID: wacom: add USB_HID dependency

[Arnd Bergmann]

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

Re: [PATCH] HID: wacom: add USB_HID dependency

[Jason Gerecke]

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.

Re: [PATCH] HID: wacom: add USB_HID dependency

[Arnd Bergmann]

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

Re: [PATCH] HID: wacom: add USB_HID dependency

[Jason Gerecke]

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

Re: [PATCH] HID: wacom: add USB_HID dependency

[Arnd Bergmann]

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

Re: [PATCH] HID: wacom: add USB_HID dependency

[Jason Gerecke]

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

Re: [PATCH] HID: wacom: add USB_HID dependency

[Jiri Kosina]

On Fri, 28 Jul 2017, Jason Gerecke wrote:

> I could accept either the strict dependency you originally suggested
> or a modified header, but don't see much reason for the former.

Well, until any better solution is proposed (in the form of patch), I'm 
applying Arnd's patch introducing the hard dependency. We can always 
revisit this later.

The dependency is currently simply there, because we do rely on the actual 
existence of usb_hid_driver structure for the purpose of testing against 
it.

Thanks,

-- 
Jiri Kosina
SUSE Labs