[PATCH 0/2] HID: lg: fix init of devices when FF is off

STALE1846d

5 messages, 2 authors, 2021-09-29 · open the first message on its own page

[PATCH 0/2] HID: lg: fix init of devices when FF is off

From: Benjamin Tissoires <hidden>
Date: 2021-09-28 08:39:21

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

[PATCH 1/2] HID: lg: do not return an error when FF is disabled

From: Benjamin Tissoires <hidden>
Date: 2021-09-28 08:39:19

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 <redacted>
---
 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

[PATCH 2/2] HID: lg4ff: do not return a value for deinit

From: Benjamin Tissoires <hidden>
Date: 2021-09-28 08:39:26

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 <redacted>
---
 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

Re: [PATCH 2/2] HID: lg4ff: do not return a value for deinit

From: Michal Malý <hidden>
Date: 2021-09-29 07:51:01

On Tue, 2021-09-28 at 10:39 +0200, Benjamin Tissoires wrote:

quoted hunk

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 <redacted>
---
 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

Re: [PATCH 2/2] HID: lg4ff: do not return a value for deinit

From: Benjamin Tissoires <hidden>
Date: 2021-09-29 08:55:27

On Wed, Sep 29, 2021 at 9:50 AM Michal Malý
[off-list ref] wrote:

On Tue, 2021-09-28 at 10:39 +0200, Benjamin Tissoires wrote:

quoted

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 <redacted>
---
 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 :)

quoted hunk

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

quoted hunk

+
 /* 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.

quoted hunk

        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

quoted hunk

        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 :)

quoted hunk

        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.

quoted hunk

+#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?

quoted hunk

+#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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help