* [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).