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

From: Benjamin Tissoires
Date: Wed Sep 29 2021 - 04:55:27 EST

Next message: Lino Sanfilippo: "Re: Re: [PATCH] tpm: fix potential NULL pointer access in tpm_del_char_device()"
Previous message: Uwe Kleine-König: "[PATCH v5 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver"
In reply to: Michal Malý: "Re: [PATCH 2/2] HID: lg4ff: do not return a value for deinit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Wed, Sep 29, 2021 at 9:50 AM Michal Malý
<madcatxster@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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
>
>

Next message: Lino Sanfilippo: "Re: Re: [PATCH] tpm: fix potential NULL pointer access in tpm_del_char_device()"
Previous message: Uwe Kleine-König: "[PATCH v5 00/11] PCI: Drop duplicated tracking of a pci_dev's bound driver"
In reply to: Michal Malý: "Re: [PATCH 2/2] HID: lg4ff: do not return a value for deinit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]