* [PATCH 0/2] HID: lg: fix init of devices when FF is off @ 2021-09-28 8:39 Benjamin Tissoires 2021-09-28 8:39 ` [PATCH 1/2] HID: lg: do not return an error when FF is disabled Benjamin Tissoires 2021-09-28 8:39 ` [PATCH 2/2] HID: lg4ff: do not return a value for deinit Benjamin Tissoires 0 siblings, 2 replies; 5+ messages in thread From: Benjamin Tissoires @ 2021-09-28 8:39 UTC (permalink / raw) To: Jiri Kosina, Michal Malý Cc: linux-input, linux-kernel, Benjamin Tissoires Hi, foreword: this was only compiled tested and would greatly benefit from some testing with real hardware. This was discovered in RHEL 8. There, we disable most of the FF bits and it appears returning -1 when the bits are disabled just make the device disappear. Cheers, Benjamin Benjamin Tissoires (2): HID: lg: do not return an error when FF is disabled HID: lg4ff: do not return a value for deinit drivers/hid/hid-lg.h | 6 +++--- drivers/hid/hid-lg4ff.c | 5 ++--- drivers/hid/hid-lg4ff.h | 6 +++--- 3 files changed, 8 insertions(+), 9 deletions(-) -- 2.26.3 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] HID: lg: do not return an error when FF is disabled 2021-09-28 8:39 [PATCH 0/2] HID: lg: fix init of devices when FF is off Benjamin Tissoires @ 2021-09-28 8:39 ` Benjamin Tissoires 2021-09-28 8:39 ` [PATCH 2/2] HID: lg4ff: do not return a value for deinit Benjamin Tissoires 1 sibling, 0 replies; 5+ messages in thread From: Benjamin Tissoires @ 2021-09-28 8:39 UTC (permalink / raw) To: Jiri Kosina, Michal Malý Cc: linux-input, linux-kernel, Benjamin Tissoires Found when CONFIG_LOGIRUMBLEPAD2_FF was disabled: lg2ff_init() returns -1, and lg_probe() checks the return value and bails out, laterally disabling the device. Fixes: 2dbf635ea1c0 "HID: hid-lg: Check return values from lg[N]ff_init()" Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/hid/hid-lg.h | 6 +++--- drivers/hid/hid-lg4ff.h | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-lg.h b/drivers/hid/hid-lg.h index 3d8902ba1c6c..dbec5e8fc142 100644 --- a/drivers/hid/hid-lg.h +++ b/drivers/hid/hid-lg.h @@ -10,19 +10,19 @@ struct lg_drv_data { #ifdef CONFIG_LOGITECH_FF int lgff_init(struct hid_device *hdev); #else -static inline int lgff_init(struct hid_device *hdev) { return -1; } +static inline int lgff_init(struct hid_device *hdev) { return 0; } #endif #ifdef CONFIG_LOGIRUMBLEPAD2_FF int lg2ff_init(struct hid_device *hdev); #else -static inline int lg2ff_init(struct hid_device *hdev) { return -1; } +static inline int lg2ff_init(struct hid_device *hdev) { return 0; } #endif #ifdef CONFIG_LOGIG940_FF int lg3ff_init(struct hid_device *hdev); #else -static inline int lg3ff_init(struct hid_device *hdev) { return -1; } +static inline int lg3ff_init(struct hid_device *hdev) { return 0; } #endif #endif diff --git a/drivers/hid/hid-lg4ff.h b/drivers/hid/hid-lg4ff.h index e5c55d515ac2..25bc88cd877e 100644 --- a/drivers/hid/hid-lg4ff.h +++ b/drivers/hid/hid-lg4ff.h @@ -16,8 +16,8 @@ static inline int lg4ff_adjust_input_event(struct hid_device *hid, struct hid_fi struct hid_usage *usage, s32 value, struct lg_drv_data *drv_data) { return 0; } static inline int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *rd, int size, struct lg_drv_data *drv_data) { return 0; } -static inline int lg4ff_init(struct hid_device *hdev) { return -1; } -static inline int lg4ff_deinit(struct hid_device *hdev) { return -1; } +static inline int lg4ff_init(struct hid_device *hdev) { return 0; } +static inline int lg4ff_deinit(struct hid_device *hdev) { return 0; } #endif #endif -- 2.26.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] HID: lg4ff: do not return a value for deinit 2021-09-28 8:39 [PATCH 0/2] HID: lg: fix init of devices when FF is off Benjamin Tissoires 2021-09-28 8:39 ` [PATCH 1/2] HID: lg: do not return an error when FF is disabled Benjamin Tissoires @ 2021-09-28 8:39 ` Benjamin Tissoires 2021-09-29 7:43 ` Michal Malý 1 sibling, 1 reply; 5+ messages in thread From: Benjamin Tissoires @ 2021-09-28 8:39 UTC (permalink / raw) To: Jiri Kosina, Michal Malý Cc: linux-input, linux-kernel, Benjamin Tissoires When removing a device, we can not do much if there is an error while removing it. Use the common pattern of returning void there so we are not tempted to check on the return value. And honestly, we were not looking at it, so why bother? Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> --- drivers/hid/hid-lg4ff.c | 5 ++--- drivers/hid/hid-lg4ff.h | 4 ++-- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c index 5e6a0cef2a06..ad75c86e0bf5 100644 --- a/drivers/hid/hid-lg4ff.c +++ b/drivers/hid/hid-lg4ff.c @@ -1445,7 +1445,7 @@ int lg4ff_init(struct hid_device *hid) return error; } -int lg4ff_deinit(struct hid_device *hid) +void lg4ff_deinit(struct hid_device *hid) { struct lg4ff_device_entry *entry; struct lg_drv_data *drv_data; @@ -1453,7 +1453,7 @@ int lg4ff_deinit(struct hid_device *hid) drv_data = hid_get_drvdata(hid); if (!drv_data) { hid_err(hid, "Error while deinitializing device, no private driver data.\n"); - return -1; + return; } entry = drv_data->device_props; if (!entry) @@ -1489,5 +1489,4 @@ int lg4ff_deinit(struct hid_device *hid) kfree(entry); out: dbg_hid("Device successfully unregistered\n"); - return 0; } diff --git a/drivers/hid/hid-lg4ff.h b/drivers/hid/hid-lg4ff.h index 25bc88cd877e..4440e4ea2267 100644 --- a/drivers/hid/hid-lg4ff.h +++ b/drivers/hid/hid-lg4ff.h @@ -10,14 +10,14 @@ int lg4ff_adjust_input_event(struct hid_device *hid, struct hid_field *field, int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *rd, int size, struct lg_drv_data *drv_data); int lg4ff_init(struct hid_device *hdev); -int lg4ff_deinit(struct hid_device *hdev); +void lg4ff_deinit(struct hid_device *hdev); #else static inline int lg4ff_adjust_input_event(struct hid_device *hid, struct hid_field *field, struct hid_usage *usage, s32 value, struct lg_drv_data *drv_data) { return 0; } static inline int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *rd, int size, struct lg_drv_data *drv_data) { return 0; } static inline int lg4ff_init(struct hid_device *hdev) { return 0; } -static inline int lg4ff_deinit(struct hid_device *hdev) { return 0; } +static inline void lg4ff_deinit(struct hid_device *hdev) { return; } #endif #endif -- 2.26.3 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] HID: lg4ff: do not return a value for deinit 2021-09-28 8:39 ` [PATCH 2/2] HID: lg4ff: do not return a value for deinit Benjamin Tissoires @ 2021-09-29 7:43 ` Michal Malý 2021-09-29 8:55 ` Benjamin Tissoires 0 siblings, 1 reply; 5+ messages in thread From: Michal Malý @ 2021-09-29 7:43 UTC (permalink / raw) To: Benjamin Tissoires, Jiri Kosina; +Cc: linux-input, linux-kernel On Tue, 2021-09-28 at 10:39 +0200, Benjamin Tissoires wrote: > When removing a device, we can not do much if there is an error while > removing it. Use the common pattern of returning void there so we are > not tempted to check on the return value. > And honestly, we were not looking at it, so why bother? > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > --- > drivers/hid/hid-lg4ff.c | 5 ++--- > drivers/hid/hid-lg4ff.h | 4 ++-- > 2 files changed, 4 insertions(+), 5 deletions(-) > > diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c > index 5e6a0cef2a06..ad75c86e0bf5 100644 > --- a/drivers/hid/hid-lg4ff.c > +++ b/drivers/hid/hid-lg4ff.c > @@ -1445,7 +1445,7 @@ int lg4ff_init(struct hid_device *hid) > return error; > } > > -int lg4ff_deinit(struct hid_device *hid) > +void lg4ff_deinit(struct hid_device *hid) > { > struct lg4ff_device_entry *entry; > struct lg_drv_data *drv_data; > @@ -1453,7 +1453,7 @@ int lg4ff_deinit(struct hid_device *hid) > drv_data = hid_get_drvdata(hid); > if (!drv_data) { > hid_err(hid, "Error while deinitializing device, no > private driver data.\n"); > - return -1; > + return; > } > entry = drv_data->device_props; > if (!entry) > @@ -1489,5 +1489,4 @@ int lg4ff_deinit(struct hid_device *hid) > kfree(entry); > out: > dbg_hid("Device successfully unregistered\n"); > - return 0; > } > diff --git a/drivers/hid/hid-lg4ff.h b/drivers/hid/hid-lg4ff.h > index 25bc88cd877e..4440e4ea2267 100644 > --- a/drivers/hid/hid-lg4ff.h > +++ b/drivers/hid/hid-lg4ff.h > @@ -10,14 +10,14 @@ int lg4ff_adjust_input_event(struct hid_device > *hid, struct hid_field *field, > int lg4ff_raw_event(struct hid_device *hdev, struct hid_report > *report, > u8 *rd, int size, struct lg_drv_data *drv_data); > int lg4ff_init(struct hid_device *hdev); > -int lg4ff_deinit(struct hid_device *hdev); > +void lg4ff_deinit(struct hid_device *hdev); > #else > static inline int lg4ff_adjust_input_event(struct hid_device *hid, > struct hid_field *field, > struct hid_usage *usage, s32 > value, struct lg_drv_data *drv_data) { return 0; } > static inline int lg4ff_raw_event(struct hid_device *hdev, struct > hid_report *report, > u8 *rd, int size, struct lg_drv_data *drv_data) { > return 0; } > static inline int lg4ff_init(struct hid_device *hdev) { return 0; } > -static inline int lg4ff_deinit(struct hid_device *hdev) { return 0; } > +static inline void lg4ff_deinit(struct hid_device *hdev) { return; } > #endif > > #endif I'll try to dig up some lg4ff hardware to make sure but AFAICT skipping the init routines will make all of the devices look like standard HID stuff. Mind that disabling lg4ff disables more than just FF for the affected Logitech wheels but that probably doesn't matter. Perhaps a bit better approach would be to remove the special handling of these devices from the hid-lg driver altogether when the respective submodules are switched off. That way the devices should be handled just by the generic HID code and we won't need the dud functions at all. I only checked that this compiles. I can test this with real HW if you find this acceptable. MM --- diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c index d40af911df63..f698c2f6e810 100644 --- a/drivers/hid/hid-lg.c +++ b/drivers/hid/hid-lg.c @@ -49,6 +49,19 @@ #define FFG_RDESC_ORIG_SIZE 85 #define FG_RDESC_ORIG_SIZE 82 +#ifndef CONFIG_LOGITECH_FF +static int lgff_init(struct hid_device *hid) { return 0; } +#endif +#ifndef CONFIG_LOGIRUMBLEPAD2_FF +static int lg2ff_init(struct hid_device *hid) { return 0; } +#endif +#ifndef CONFIG_LOGIG940_FF +static int lg3ff_init(struct hid_device *hid) { return 0; } +#endif +#ifndef CONFIG_LOGIWHEELS_FF +static int lg4ff_init(struct hid_device *hid) { return 0; } +#endif + /* Fixed report descriptors for Logitech Driving Force (and Pro) * wheel controllers * @@ -729,9 +742,11 @@ static int lg_event(struct hid_device *hdev, struct hid_field *field, -value); return 1; } +#ifdef CONFIG_LOGIWHEELS_FF if (drv_data->quirks & LG_FF4) { return lg4ff_adjust_input_event(hdev, field, usage, value, drv_data); } +#endif return 0; } @@ -741,8 +756,10 @@ static int lg_raw_event(struct hid_device *hdev, struct hid_report *report, { struct lg_drv_data *drv_data = hid_get_drvdata(hdev); +#ifdef CONFIG_LOGIWHEELS_FF if (drv_data->quirks & LG_FF4) return lg4ff_raw_event(hdev, report, rd, size, drv_data); +#endif return 0; } @@ -844,8 +861,10 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id) static void lg_remove(struct hid_device *hdev) { struct lg_drv_data *drv_data = hid_get_drvdata(hdev); +#ifdef CONFIG_LOGIWHEELS_FF if (drv_data->quirks & LG_FF4) lg4ff_deinit(hdev); +#endif hid_hw_stop(hdev); kfree(drv_data); } @@ -869,45 +888,54 @@ static const struct hid_device_id lg_devices[] = { .driver_data = LG_NOGET }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DUAL_ACTION), .driver_data = LG_NOGET }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WHEEL), - .driver_data = LG_NOGET | LG_FF4 }, - - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD_CORD), - .driver_data = LG_FF2 }, +#ifdef CONFIG_LOGITECH_FF { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD), .driver_data = LG_FF }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2_2), .driver_data = LG_FF }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G29_WHEEL), - .driver_data = LG_FF4 }, + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_F3D), .driver_data = LG_FF }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_FORCE3D_PRO), .driver_data = LG_FF }, +#endif +#ifdef CONFIG_LOGIRUMBLEPAD2_FF + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD_CORD), + .driver_data = LG_FF2 }, + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL), + .driver_data = LG_FF2 }, + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2), + .driver_data = LG_NOGET | LG_FF2 }, +#endif +#ifdef CONFIG_LOGIG940_FF + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_FLIGHT_SYSTEM_G940), + .driver_data = LG_FF3 }, +#endif +#ifdef CONFIG_LOGIWHEELS_FF { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOMO_WHEEL), .driver_data = LG_NOGET | LG_FF4 }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2), .driver_data = LG_FF4 }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL), - .driver_data = LG_FF2 }, + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WHEEL), + .driver_data = LG_NOGET | LG_FF4 }, + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFP_WHEEL), + .driver_data = LG_NOGET | LG_FF4 }, + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G27_WHEEL), + .driver_data = LG_FF4 }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G25_WHEEL), .driver_data = LG_FF4 }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFGT_WHEEL), + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G29_WHEEL), .driver_data = LG_FF4 }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G27_WHEEL), + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFGT_WHEEL), .driver_data = LG_FF4 }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFP_WHEEL), + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_FFG), .driver_data = LG_NOGET | LG_FF4 }, +#else /* Wii wheel still needs a bit of special handling */ { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WII_WHEEL), - .driver_data = LG_FF4 }, + .driver_data = 0 }, +#endif { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_FG), .driver_data = LG_NOGET }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_FFG), - .driver_data = LG_NOGET | LG_FF4 }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2), - .driver_data = LG_NOGET | LG_FF2 }, - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_FLIGHT_SYSTEM_G940), - .driver_data = LG_FF3 }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR), .driver_data = LG_RDESC_REL_ABS }, { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER), diff --git a/drivers/hid/hid-lg.h b/drivers/hid/hid-lg.h index 3d8902ba1c6c..5bcb918f4741 100644 --- a/drivers/hid/hid-lg.h +++ b/drivers/hid/hid-lg.h @@ -9,20 +9,14 @@ struct lg_drv_data { #ifdef CONFIG_LOGITECH_FF int lgff_init(struct hid_device *hdev); -#else -static inline int lgff_init(struct hid_device *hdev) { return -1; } #endif #ifdef CONFIG_LOGIRUMBLEPAD2_FF int lg2ff_init(struct hid_device *hdev); -#else -static inline int lg2ff_init(struct hid_device *hdev) { return -1; } #endif #ifdef CONFIG_LOGIG940_FF int lg3ff_init(struct hid_device *hdev); -#else -static inline int lg3ff_init(struct hid_device *hdev) { return -1; } #endif #endif diff --git a/drivers/hid/hid-lg4ff.h b/drivers/hid/hid-lg4ff.h index e5c55d515ac2..c529135bba96 100644 --- a/drivers/hid/hid-lg4ff.h +++ b/drivers/hid/hid-lg4ff.h @@ -11,13 +11,6 @@ int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report, u8 *rd, int size, struct lg_drv_data *drv_data); int lg4ff_init(struct hid_device *hdev); int lg4ff_deinit(struct hid_device *hdev); -#else -static inline int lg4ff_adjust_input_event(struct hid_device *hid, struct hid_field *field, - struct hid_usage *usage, s32 value, struct lg_drv_data *drv_data) { return 0; } -static inline int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report, - u8 *rd, int size, struct lg_drv_data *drv_data) { return 0; } -static inline int lg4ff_init(struct hid_device *hdev) { return -1; } -static inline int lg4ff_deinit(struct hid_device *hdev) { return -1; } #endif #endif ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] HID: lg4ff: do not return a value for deinit 2021-09-29 7:43 ` Michal Malý @ 2021-09-29 8:55 ` Benjamin Tissoires 0 siblings, 0 replies; 5+ messages in thread From: Benjamin Tissoires @ 2021-09-29 8:55 UTC (permalink / raw) To: Michal Malý; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml On Wed, Sep 29, 2021 at 9:50 AM Michal Malý <madcatxster@devoid-pointer.net> wrote: > > On Tue, 2021-09-28 at 10:39 +0200, Benjamin Tissoires wrote: > > When removing a device, we can not do much if there is an error while > > removing it. Use the common pattern of returning void there so we are > > not tempted to check on the return value. > > And honestly, we were not looking at it, so why bother? > > > > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> > > --- > > drivers/hid/hid-lg4ff.c | 5 ++--- > > drivers/hid/hid-lg4ff.h | 4 ++-- > > 2 files changed, 4 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c > > index 5e6a0cef2a06..ad75c86e0bf5 100644 > > --- a/drivers/hid/hid-lg4ff.c > > +++ b/drivers/hid/hid-lg4ff.c > > @@ -1445,7 +1445,7 @@ int lg4ff_init(struct hid_device *hid) > > return error; > > } > > > > -int lg4ff_deinit(struct hid_device *hid) > > +void lg4ff_deinit(struct hid_device *hid) > > { > > struct lg4ff_device_entry *entry; > > struct lg_drv_data *drv_data; > > @@ -1453,7 +1453,7 @@ int lg4ff_deinit(struct hid_device *hid) > > drv_data = hid_get_drvdata(hid); > > if (!drv_data) { > > hid_err(hid, "Error while deinitializing device, no > > private driver data.\n"); > > - return -1; > > + return; > > } > > entry = drv_data->device_props; > > if (!entry) > > @@ -1489,5 +1489,4 @@ int lg4ff_deinit(struct hid_device *hid) > > kfree(entry); > > out: > > dbg_hid("Device successfully unregistered\n"); > > - return 0; > > } > > diff --git a/drivers/hid/hid-lg4ff.h b/drivers/hid/hid-lg4ff.h > > index 25bc88cd877e..4440e4ea2267 100644 > > --- a/drivers/hid/hid-lg4ff.h > > +++ b/drivers/hid/hid-lg4ff.h > > @@ -10,14 +10,14 @@ int lg4ff_adjust_input_event(struct hid_device > > *hid, struct hid_field *field, > > int lg4ff_raw_event(struct hid_device *hdev, struct hid_report > > *report, > > u8 *rd, int size, struct lg_drv_data *drv_data); > > int lg4ff_init(struct hid_device *hdev); > > -int lg4ff_deinit(struct hid_device *hdev); > > +void lg4ff_deinit(struct hid_device *hdev); > > #else > > static inline int lg4ff_adjust_input_event(struct hid_device *hid, > > struct hid_field *field, > > struct hid_usage *usage, s32 > > value, struct lg_drv_data *drv_data) { return 0; } > > static inline int lg4ff_raw_event(struct hid_device *hdev, struct > > hid_report *report, > > u8 *rd, int size, struct lg_drv_data *drv_data) { > > return 0; } > > static inline int lg4ff_init(struct hid_device *hdev) { return 0; } > > -static inline int lg4ff_deinit(struct hid_device *hdev) { return 0; } > > +static inline void lg4ff_deinit(struct hid_device *hdev) { return; } > > #endif > > > > #endif > > I'll try to dig up some lg4ff hardware to make sure but AFAICT skipping > the init routines will make all of the devices look like standard HID > stuff. Mind that disabling lg4ff disables more than just FF for the > affected Logitech wheels but that probably doesn't matter. > > Perhaps a bit better approach would be to remove the special handling > of these devices from the hid-lg driver altogether when the respective > submodules are switched off. That way the devices should be handled > just by the generic HID code and we won't need the dud functions at > all. I'm OK with that approach. there are a few bits in the following code that need changes, see my replies inline. > > I only checked that this compiles. I can test this with real HW if you > find this acceptable. That would be great, I don't have the HW :) > > MM > > --- > > diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c > index d40af911df63..f698c2f6e810 100644 > --- a/drivers/hid/hid-lg.c > +++ b/drivers/hid/hid-lg.c > @@ -49,6 +49,19 @@ > #define FFG_RDESC_ORIG_SIZE 85 > #define FG_RDESC_ORIG_SIZE 82 > > +#ifndef CONFIG_LOGITECH_FF > +static int lgff_init(struct hid_device *hid) { return 0; } > +#endif > +#ifndef CONFIG_LOGIRUMBLEPAD2_FF > +static int lg2ff_init(struct hid_device *hid) { return 0; } > +#endif > +#ifndef CONFIG_LOGIG940_FF > +static int lg3ff_init(struct hid_device *hid) { return 0; } > +#endif > +#ifndef CONFIG_LOGIWHEELS_FF > +static int lg4ff_init(struct hid_device *hid) { return 0; } > +#endif I would rather keep those definitions in hid-lg.h, not inlined in the code > + > /* Fixed report descriptors for Logitech Driving Force (and Pro) > * wheel controllers > * > @@ -729,9 +742,11 @@ static int lg_event(struct hid_device *hdev, struct hid_field *field, > -value); > return 1; > } > +#ifdef CONFIG_LOGIWHEELS_FF > if (drv_data->quirks & LG_FF4) { > return lg4ff_adjust_input_event(hdev, field, usage, value, drv_data); > } > +#endif Let's not sprinkle the code with #ifdef. Just add an inline lg4ff_adjust_input_event() in hid-lg4ff.h that returns 0. > > return 0; > } > @@ -741,8 +756,10 @@ static int lg_raw_event(struct hid_device *hdev, struct hid_report *report, > { > struct lg_drv_data *drv_data = hid_get_drvdata(hdev); > > +#ifdef CONFIG_LOGIWHEELS_FF > if (drv_data->quirks & LG_FF4) > return lg4ff_raw_event(hdev, report, rd, size, drv_data); > +#endif same here > > return 0; > } > @@ -844,8 +861,10 @@ static int lg_probe(struct hid_device *hdev, const struct hid_device_id *id) > static void lg_remove(struct hid_device *hdev) > { > struct lg_drv_data *drv_data = hid_get_drvdata(hdev); > +#ifdef CONFIG_LOGIWHEELS_FF > if (drv_data->quirks & LG_FF4) > lg4ff_deinit(hdev); > +#endif same here :) > hid_hw_stop(hdev); > kfree(drv_data); > } > @@ -869,45 +888,54 @@ static const struct hid_device_id lg_devices[] = { > .driver_data = LG_NOGET }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DUAL_ACTION), > .driver_data = LG_NOGET }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WHEEL), > - .driver_data = LG_NOGET | LG_FF4 }, > - > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD_CORD), > - .driver_data = LG_FF2 }, > +#ifdef CONFIG_LOGITECH_FF > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD), > .driver_data = LG_FF }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2_2), > .driver_data = LG_FF }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G29_WHEEL), > - .driver_data = LG_FF4 }, > + > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_F3D), > .driver_data = LG_FF }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_FORCE3D_PRO), > .driver_data = LG_FF }, > +#endif > +#ifdef CONFIG_LOGIRUMBLEPAD2_FF > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD_CORD), > + .driver_data = LG_FF2 }, > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL), > + .driver_data = LG_FF2 }, > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2), > + .driver_data = LG_NOGET | LG_FF2 }, That's the part I was not entirely sure: I *think* not having NOGET when using the generic path is OK now, but having a real hardware test would be nice (if you still have this one). Worse case I should be able to get this tested through the RHEL bug, but that would take some time to get. > +#endif > +#ifdef CONFIG_LOGIG940_FF > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_FLIGHT_SYSTEM_G940), > + .driver_data = LG_FF3 }, > +#endif > +#ifdef CONFIG_LOGIWHEELS_FF > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOMO_WHEEL), > .driver_data = LG_NOGET | LG_FF4 }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_MOMO_WHEEL2), > .driver_data = LG_FF4 }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_VIBRATION_WHEEL), > - .driver_data = LG_FF2 }, > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WHEEL), > + .driver_data = LG_NOGET | LG_FF4 }, > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFP_WHEEL), > + .driver_data = LG_NOGET | LG_FF4 }, > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G27_WHEEL), > + .driver_data = LG_FF4 }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G25_WHEEL), > .driver_data = LG_FF4 }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFGT_WHEEL), > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G29_WHEEL), > .driver_data = LG_FF4 }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G27_WHEEL), > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFGT_WHEEL), > .driver_data = LG_FF4 }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_DFP_WHEEL), > + { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_FFG), > .driver_data = LG_NOGET | LG_FF4 }, > +#else /* Wii wheel still needs a bit of special handling */ > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WII_WHEEL), > - .driver_data = LG_FF4 }, > + .driver_data = 0 }, I am not sure about this bit. Can you expand on it a bit? > +#endif > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_FG), > .driver_data = LG_NOGET }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_WINGMAN_FFG), > - .driver_data = LG_NOGET | LG_FF4 }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_RUMBLEPAD2), > - .driver_data = LG_NOGET | LG_FF2 }, > - { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_FLIGHT_SYSTEM_G940), > - .driver_data = LG_FF3 }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACENAVIGATOR), > .driver_data = LG_RDESC_REL_ABS }, > { HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_SPACETRAVELLER), > diff --git a/drivers/hid/hid-lg.h b/drivers/hid/hid-lg.h > index 3d8902ba1c6c..5bcb918f4741 100644 > --- a/drivers/hid/hid-lg.h > +++ b/drivers/hid/hid-lg.h > @@ -9,20 +9,14 @@ struct lg_drv_data { > > #ifdef CONFIG_LOGITECH_FF > int lgff_init(struct hid_device *hdev); > -#else > -static inline int lgff_init(struct hid_device *hdev) { return -1; } > #endif > > #ifdef CONFIG_LOGIRUMBLEPAD2_FF > int lg2ff_init(struct hid_device *hdev); > -#else > -static inline int lg2ff_init(struct hid_device *hdev) { return -1; } > #endif > > #ifdef CONFIG_LOGIG940_FF > int lg3ff_init(struct hid_device *hdev); > -#else > -static inline int lg3ff_init(struct hid_device *hdev) { return -1; } > #endif > > #endif > diff --git a/drivers/hid/hid-lg4ff.h b/drivers/hid/hid-lg4ff.h > index e5c55d515ac2..c529135bba96 100644 > --- a/drivers/hid/hid-lg4ff.h > +++ b/drivers/hid/hid-lg4ff.h > @@ -11,13 +11,6 @@ int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report, > u8 *rd, int size, struct lg_drv_data *drv_data); > int lg4ff_init(struct hid_device *hdev); > int lg4ff_deinit(struct hid_device *hdev); > -#else > -static inline int lg4ff_adjust_input_event(struct hid_device *hid, struct hid_field *field, > - struct hid_usage *usage, s32 value, struct lg_drv_data *drv_data) { return 0; } > -static inline int lg4ff_raw_event(struct hid_device *hdev, struct hid_report *report, > - u8 *rd, int size, struct lg_drv_data *drv_data) { return 0; } > -static inline int lg4ff_init(struct hid_device *hdev) { return -1; } > -static inline int lg4ff_deinit(struct hid_device *hdev) { return -1; } > #endif I would keep all of those static inline defines here, as mentioned above. This patch is also missing a change in hid-quirks.c: we need to remove them from the hid_have_special_driver[] list, or they will simply not bind to anything when they are plugged with the FF disabled. We could also add #ifdef in hid_have_special_driver[], but if it works without, that would be better actually. Cheers, Benjamin > > #endif > > ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2021-09-29 8:55 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-09-28 8:39 [PATCH 0/2] HID: lg: fix init of devices when FF is off Benjamin Tissoires 2021-09-28 8:39 ` [PATCH 1/2] HID: lg: do not return an error when FF is disabled Benjamin Tissoires 2021-09-28 8:39 ` [PATCH 2/2] HID: lg4ff: do not return a value for deinit Benjamin Tissoires 2021-09-29 7:43 ` Michal Malý 2021-09-29 8:55 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).