* [PATCH] Input: i8042 - add TUXEDO Stellaris 16 Gen5 AMD to i8042 quirk table
From: Werner Sembach @ 2024-09-05 16:48 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Werner Sembach, linux-input, linux-kernel
Some TongFang barebones have touchpad and/or keyboard issues after
suspend, fixable with nomux + reset + noloop + nopnp. Luckily, none of
them have an external PS/2 port so this can safely be set for all of
them.
I'm not entirely sure if every device listed really needs all four quirks,
but after testing and production use, no negative effects could be
observed when setting all four.
Signed-off-by: Werner Sembach <wse@tuxedocomputers.com>
Cc: stable@vger.kernel.org
---
drivers/input/serio/i8042-acpipnpio.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/input/serio/i8042-acpipnpio.h b/drivers/input/serio/i8042-acpipnpio.h
index bad238f69a7af..1d73391f127bc 100644
--- a/drivers/input/serio/i8042-acpipnpio.h
+++ b/drivers/input/serio/i8042-acpipnpio.h
@@ -1120,6 +1120,29 @@ static const struct dmi_system_id i8042_dmi_quirk_table[] __initconst = {
},
.driver_data = (void *)(SERIO_QUIRK_NOLOOP)
},
+ /*
+ * Some TongFang barebones have touchpad and/or keyboard issues after
+ * suspend fixable with nomux + reset + noloop + nopnp. Luckily, none of
+ * them have an external PS/2 port so this can safely be set for all of
+ * them.
+ * TongFang barebones come with board_vendor and/or system_vendor set to
+ * a different value for each individual reseller. The only somewhat
+ * universal way to identify them is by board_name.
+ */
+ {
+ .matches = {
+ DMI_MATCH(DMI_BOARD_NAME, "GM6XGxX"),
+ },
+ .driver_data = (void *)(SERIO_QUIRK_NOMUX | SERIO_QUIRK_RESET_ALWAYS |
+ SERIO_QUIRK_NOLOOP | SERIO_QUIRK_NOPNP)
+ },
+ {
+ .matches = {
+ DMI_MATCH(DMI_BOARD_NAME, "GMxXGxX"),
+ },
+ .driver_data = (void *)(SERIO_QUIRK_NOMUX | SERIO_QUIRK_RESET_ALWAYS |
+ SERIO_QUIRK_NOLOOP | SERIO_QUIRK_NOPNP)
+ },
/*
* A lot of modern Clevo barebones have touchpad and/or keyboard issues
* after suspend fixable with nomux + reset + noloop + nopnp. Luckily,
--
2.34.1
^ permalink raw reply related
* [PATCH v2] HID: lg: constify fixed up report descriptor
From: Thomas Weißschuh @ 2024-09-05 15:48 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Marcus Folkesson
Cc: linux-input, linux-kernel, Thomas Weißschuh
Now that the HID core can handle const report descriptors,
constify them where possible.
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Now that the HID core can handle const report descriptors,
constify them where possible.
This is based upon hid/for-6.12/constify-rdesc.
---
Changes in v2:
- Drop all already applied patches
- Rework return logic in hid-lg.c
- Link to v1: https://lore.kernel.org/r/20240828-hid-const-fixup-2-v1-0-663b9210eb69@weissschuh.net
---
drivers/hid/hid-lg.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
index a9be918e2b5c..9a2cfa018bd3 100644
--- a/drivers/hid/hid-lg.c
+++ b/drivers/hid/hid-lg.c
@@ -58,7 +58,7 @@
* These descriptors remove the combined Y axis and instead report
* separate throttle (Y) and brake (RZ).
*/
-static __u8 df_rdesc_fixed[] = {
+static const __u8 df_rdesc_fixed[] = {
0x05, 0x01, /* Usage Page (Desktop), */
0x09, 0x04, /* Usage (Joystick), */
0xA1, 0x01, /* Collection (Application), */
@@ -124,7 +124,7 @@ static __u8 df_rdesc_fixed[] = {
0xC0 /* End Collection */
};
-static __u8 dfp_rdesc_fixed[] = {
+static const __u8 dfp_rdesc_fixed[] = {
0x05, 0x01, /* Usage Page (Desktop), */
0x09, 0x04, /* Usage (Joystick), */
0xA1, 0x01, /* Collection (Application), */
@@ -172,7 +172,7 @@ static __u8 dfp_rdesc_fixed[] = {
0xC0 /* End Collection */
};
-static __u8 fv_rdesc_fixed[] = {
+static const __u8 fv_rdesc_fixed[] = {
0x05, 0x01, /* Usage Page (Desktop), */
0x09, 0x04, /* Usage (Joystick), */
0xA1, 0x01, /* Collection (Application), */
@@ -239,7 +239,7 @@ static __u8 fv_rdesc_fixed[] = {
0xC0 /* End Collection */
};
-static __u8 momo_rdesc_fixed[] = {
+static const __u8 momo_rdesc_fixed[] = {
0x05, 0x01, /* Usage Page (Desktop), */
0x09, 0x04, /* Usage (Joystick), */
0xA1, 0x01, /* Collection (Application), */
@@ -285,7 +285,7 @@ static __u8 momo_rdesc_fixed[] = {
0xC0 /* End Collection */
};
-static __u8 momo2_rdesc_fixed[] = {
+static const __u8 momo2_rdesc_fixed[] = {
0x05, 0x01, /* Usage Page (Desktop), */
0x09, 0x04, /* Usage (Joystick), */
0xA1, 0x01, /* Collection (Application), */
@@ -333,7 +333,7 @@ static __u8 momo2_rdesc_fixed[] = {
0xC0 /* End Collection */
};
-static __u8 ffg_rdesc_fixed[] = {
+static const __u8 ffg_rdesc_fixed[] = {
0x05, 0x01, /* Usage Page (Desktop), */
0x09, 0x04, /* Usage (Joystik), */
0xA1, 0x01, /* Collection (Application), */
@@ -379,7 +379,7 @@ static __u8 ffg_rdesc_fixed[] = {
0xC0 /* End Collection */
};
-static __u8 fg_rdesc_fixed[] = {
+static const __u8 fg_rdesc_fixed[] = {
0x05, 0x01, /* Usage Page (Desktop), */
0x09, 0x04, /* Usage (Joystik), */
0xA1, 0x01, /* Collection (Application), */
@@ -453,8 +453,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
if (*rsize == FG_RDESC_ORIG_SIZE) {
hid_info(hdev,
"fixing up Logitech Wingman Formula GP report descriptor\n");
- rdesc = fg_rdesc_fixed;
*rsize = sizeof(fg_rdesc_fixed);
+ return fg_rdesc_fixed;
} else {
hid_info(hdev,
"rdesc size test failed for formula gp\n");
@@ -466,8 +466,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
if (*rsize == FFG_RDESC_ORIG_SIZE) {
hid_info(hdev,
"fixing up Logitech Wingman Formula Force GP report descriptor\n");
- rdesc = ffg_rdesc_fixed;
*rsize = sizeof(ffg_rdesc_fixed);
+ return ffg_rdesc_fixed;
}
break;
@@ -476,8 +476,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
if (*rsize == DF_RDESC_ORIG_SIZE) {
hid_info(hdev,
"fixing up Logitech Driving Force report descriptor\n");
- rdesc = df_rdesc_fixed;
*rsize = sizeof(df_rdesc_fixed);
+ return df_rdesc_fixed;
}
break;
@@ -485,8 +485,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
if (*rsize == MOMO_RDESC_ORIG_SIZE) {
hid_info(hdev,
"fixing up Logitech Momo Force (Red) report descriptor\n");
- rdesc = momo_rdesc_fixed;
*rsize = sizeof(momo_rdesc_fixed);
+ return momo_rdesc_fixed;
}
break;
@@ -494,8 +494,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
if (*rsize == MOMO2_RDESC_ORIG_SIZE) {
hid_info(hdev,
"fixing up Logitech Momo Racing Force (Black) report descriptor\n");
- rdesc = momo2_rdesc_fixed;
*rsize = sizeof(momo2_rdesc_fixed);
+ return momo2_rdesc_fixed;
}
break;
@@ -503,8 +503,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
if (*rsize == FV_RDESC_ORIG_SIZE) {
hid_info(hdev,
"fixing up Logitech Formula Vibration report descriptor\n");
- rdesc = fv_rdesc_fixed;
*rsize = sizeof(fv_rdesc_fixed);
+ return fv_rdesc_fixed;
}
break;
@@ -512,8 +512,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
if (*rsize == DFP_RDESC_ORIG_SIZE) {
hid_info(hdev,
"fixing up Logitech Driving Force Pro report descriptor\n");
- rdesc = dfp_rdesc_fixed;
*rsize = sizeof(dfp_rdesc_fixed);
+ return dfp_rdesc_fixed;
}
break;
---
base-commit: 03f8dc1d0a3823229a761f202bc1a6e63d5b4ba0
change-id: 20240803-hid-const-fixup-2-7c60ac529531
prerequisite-change-id: 20240730-hid-const-fixup-8b01cbda1b49:v2
prerequisite-patch-id: 92216ced5d79ee5578fbe1c24c994b6fd550d1fb
prerequisite-patch-id: 4dd3e0fa6b0387f2a722c2bb924fc9c3b784f49d
prerequisite-patch-id: 7a5b42060b989b053d2bc71d52e0281815da542d
prerequisite-patch-id: 15809fd82225c2d44cdbed2d570d621ba7378cec
prerequisite-patch-id: baba272935e0f16c67170413cadb682b535b3a6d
prerequisite-patch-id: 4e6017ca6b8df87fe8270fffd43a585eddba88c7
prerequisite-patch-id: 06023fde4515232fdcc4044b252aa42ed9a47885
Best regards,
--
Thomas Weißschuh <linux@weissschuh.net>
^ permalink raw reply related
* Re: [PATCH 03/24] Input: alps - use guard notation when pausing serio port
From: Pali Rohár @ 2024-09-05 15:46 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <20240905041732.2034348-4-dmitry.torokhov@gmail.com>
On Wednesday 04 September 2024 21:17:08 Dmitry Torokhov wrote:
> Using guard notation makes the code more compact and error handling
> more robust by ensuring that serio ports are resumed in all code paths
> when control leaves critical section.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Looks good for me.
Acked-by: Pali Rohár <pali@kernel.org>
> ---
> drivers/input/mouse/alps.c | 4 +---
> 1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 4e37fc3f1a9e..0728b5c08f02 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -1585,7 +1585,7 @@ static void alps_flush_packet(struct timer_list *t)
> struct alps_data *priv = from_timer(priv, t, timer);
> struct psmouse *psmouse = priv->psmouse;
>
> - serio_pause_rx(psmouse->ps2dev.serio);
> + guard(serio_pause_rx)(psmouse->ps2dev.serio);
>
> if (psmouse->pktcnt == psmouse->pktsize) {
>
> @@ -1605,8 +1605,6 @@ static void alps_flush_packet(struct timer_list *t)
> }
> psmouse->pktcnt = 0;
> }
> -
> - serio_continue_rx(psmouse->ps2dev.serio);
> }
>
> static psmouse_ret_t alps_process_byte(struct psmouse *psmouse)
> --
> 2.46.0.469.g59c65b2a67-goog
>
^ permalink raw reply
* Re: [PATCH] Input: alps - use guard notation when acquiring mutex
From: Pali Rohár @ 2024-09-05 15:44 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: linux-input, Erick Archer, linux-kernel
In-Reply-To: <ZtC2lcKuZd-NbDdl@google.com>
On Thursday 29 August 2024 10:57:41 Dmitry Torokhov wrote:
> Hi Pali,
>
> On Sun, Aug 25, 2024 at 10:13:47PM +0200, Pali Rohár wrote:
> >
> > Hello, I'm not familiar with new guards and their usage. But if this is
> > a preferred way for handling mutexes then go ahead.
> >
> > I just looked at the code and I do not see any obvious error neither in
> > old nor in new version.
>
> Is this a Reviewed-by or Acked-by? If neither that is fine too, just
> want to make sure.
>
> Thanks.
>
> --
> Dmitry
Hello, I have looked at it again, and you can add my:
Acked-by: Pali Rohár <pali@kernel.org>
^ permalink raw reply
* Re: [PATCH -next 01/19] HID: core: Use devm_add_action_or_reset helper to manage hid resources
From: Li Zetao @ 2024-09-05 15:41 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: jikos, michael.zaidman, gupt21, djogorchock, rrameshbabu, bonbons,
roderick.colenbrander, david, savicaleksa83, me, jdelvare, linux,
mail, wilken.gottwalt, jonas, mezin.alexander, linux-input,
linux-i2c, linux-hwmon
In-Reply-To: <cyils23bh4xaiw7bydlpapz4ngqpya3c4kesifrdpnme2t4bib@6elk7u3wvhh2>
Hi,
在 2024/9/5 20:53, Benjamin Tissoires 写道:
> On Sep 04 2024, Li Zetao wrote:
>> By adding a custom action to the device, it can bind the hid resource
>> to the hid_device life cycle. The framework automatically close and stop
>> the hid resources before hid_device is released, and the users do not
>> need to pay attention to the timing of hid resource release.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>> drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
>> include/linux/hid.h | 2 ++
>> 2 files changed, 42 insertions(+)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 30de92d0bf0f..71143c0a4a02 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev)
>> }
>> EXPORT_SYMBOL_GPL(hid_hw_close);
>>
>> +static void hid_hw_close_and_stop(void *data)
>> +{
>> + struct hid_device *hdev = data;
>> +
>> + hid_hw_close(hdev);
>> + hid_hw_stop(hdev);
>> +}
>> +
>> +/**
>> + * devm_hid_hw_start_and_open - manage hid resources through custom action
>> + *
>> + * @hdev: hid device
>> + * @connect_mask: which outputs to connect, see HID_CONNECT_*
>> + *
>> + * Bind the hid resource to the hid_device life cycle and register
>> + * an action to release the hid resource. The users do not need to
>> + * pay attention to the release of hid.
>
> The only problem I see here is that this API is not completely "safe",
> in the sense that a driver using it can not use manual kzalloc/kfree.
> It is obvious, but probably not so much while looking at the comments
> from Guenter where he asked you to also remove the .remove() callback.
>
> So in the docs, we should probably state that if the .remove() is any
> different than "hid_hw_close(hdev);hid_hw_stop(hdev);", care should be
> use with that function.I'll add some comments to illustrate a scenario like this.
>
> Cheers,
> Benjamin
>
>> + */
>> +
>> +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask)
>> +{
>> + int ret;
>> +
>> + ret = hid_hw_start(hdev, connect_mask);
>> + if (ret) {
>> + hid_err(hdev, "hw start failed with %d\n", ret);
>> + return ret;
>> + }
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + hid_err(hdev, "hw open failed with %d\n", ret);
>> + hid_hw_stop(hdev);
>> + return ret;
>> + }
>> +
>> + return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev);
>> +}
>> +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open);
>> +
>> /**
>> * hid_hw_request - send report request to device
>> *
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 121d5b8bc867..0ce217aa5f62 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev,
>> void hid_hw_stop(struct hid_device *hdev);
>> int __must_check hid_hw_open(struct hid_device *hdev);
>> void hid_hw_close(struct hid_device *hdev);
>> +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev,
>> + unsigned int connect_mask);
>> void hid_hw_request(struct hid_device *hdev,
>> struct hid_report *report, enum hid_class_request reqtype);
>> int __hid_hw_raw_request(struct hid_device *hdev,
>> --
>> 2.34.1
>>
Thanks,
Li Zetao.
^ permalink raw reply
* Re: [PATCH -next 19/19] hwmon: (nzxt-smart2) Use devm_hid_hw_start_and_open in nzxt_smart2_hid_probe()
From: Li Zetao @ 2024-09-05 15:39 UTC (permalink / raw)
To: Guenter Roeck, jikos, bentiss, michael.zaidman, gupt21,
djogorchock, rrameshbabu, bonbons, roderick.colenbrander, david,
savicaleksa83, me, jdelvare, mail, wilken.gottwalt, jonas,
mezin.alexander
Cc: linux-input, linux-i2c, linux-hwmon
In-Reply-To: <eb7ffcbb-1e6c-448b-a09f-e10fd187a1ef@roeck-us.net>
Hi,
在 2024/9/4 22:14, Guenter Roeck 写道:
> On 9/4/24 05:36, Li Zetao wrote:
>> Currently, the nzxt-smart2 module needs to maintain hid resources
>> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
>
> For all patches:
>
> s/Consider using/Use/
ok
>
>> that hid resources are consistent with the device life cycle, and release
>> hid resources before device is released. At the same time, it can avoid
>> the goto-release encoding, drop the out_hw_close and out_hw_stop
>> lables, and directly return the error code when an error occurs.
>>
>> Signed-off-by: Li Zetao <lizetao1@huawei.com>
>> ---
>> drivers/hwmon/nzxt-smart2.c | 22 +++-------------------
>> 1 file changed, 3 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/hwmon/nzxt-smart2.c b/drivers/hwmon/nzxt-smart2.c
>> index df6fa72a6b59..b5721a42c0d3 100644
>> --- a/drivers/hwmon/nzxt-smart2.c
>> +++ b/drivers/hwmon/nzxt-smart2.c
>> @@ -750,14 +750,10 @@ static int nzxt_smart2_hid_probe(struct
>> hid_device *hdev,
>> if (ret)
>> return ret;
>> - ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
>> + ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
>> if (ret)
>> return ret;
>> - ret = hid_hw_open(hdev);
>> - if (ret)
>> - goto out_hw_stop;
>> -
>> hid_device_io_start(hdev);
>> init_device(drvdata, UPDATE_INTERVAL_DEFAULT_MS);
>> @@ -765,19 +761,10 @@ static int nzxt_smart2_hid_probe(struct
>> hid_device *hdev,
>> drvdata->hwmon =
>> hwmon_device_register_with_info(&hdev->dev, "nzxtsmart2",
>> drvdata,
>> &nzxt_smart2_chip_info, NULL);
>> - if (IS_ERR(drvdata->hwmon)) {
>> - ret = PTR_ERR(drvdata->hwmon);
>> - goto out_hw_close;
>> - }
>> + if (IS_ERR(drvdata->hwmon))
>> + return PTR_ERR(drvdata->hwmon);
>> return 0;
>
> return PTR_ERR_OR_ZERO(drvdata->hwmon);
>
> Also, this can be optimized further.
>
> struct device *hwmon; // and drop from struct drvdata
> ...
> hwmon = devm_hwmon_device_register_with_info(&hdev->dev,
> "nzxtsmart2", drvdata,
> &nzxt_smart2_chip_info, NULL);
>
> return PTR_ERR_OR_ZERO(hwmon);
>
> and drop the remove function entirely.
Benjamin mentioned that there are unsafe scenarios in
hid_hw_close_and_stop, and it is necessary to ensure that the driver can
not use manual kzalloc/kfree. So, to be extra safe, I would delete
.remove in v2.
>
> Thanks,
> Guenter
>
>> -
>> -out_hw_close:
>> - hid_hw_close(hdev);
>> -
>> -out_hw_stop:
>> - hid_hw_stop(hdev);
>> - return ret;
>> }
>> static void nzxt_smart2_hid_remove(struct hid_device *hdev)
>> @@ -785,9 +772,6 @@ static void nzxt_smart2_hid_remove(struct
>> hid_device *hdev)
>> struct drvdata *drvdata = hid_get_drvdata(hdev);
>> hwmon_device_unregister(drvdata->hwmon);
>> -
>> - hid_hw_close(hdev);
>> - hid_hw_stop(hdev);
>> }
>> static const struct hid_device_id nzxt_smart2_hid_id_table[] = {
>
^ permalink raw reply
* Re: [PATCH 13/14] HID: lg: constify fixed up report descriptor
From: Thomas Weißschuh @ 2024-09-05 14:59 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Marcus Folkesson, linux-input, linux-kernel
In-Reply-To: <66uyx73trgi4m5iybuee5ka4vbulh433o4jpqc4uu4teaflaex@av7xsfwjypy5>
On 2024-09-05 16:51:48+0000, Benjamin Tissoires wrote:
> As you can see in the b4 reply, I've now applied all of the patches but
> this one. Please see below.
Thanks!
> On Aug 28 2024, Thomas Weißschuh wrote:
> > Now that the HID core can handle const report descriptors,
> > constify them where possible.
> >
> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > ---
> > drivers/hid/hid-lg.c | 31 +++++++++++++++++--------------
> > 1 file changed, 17 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
> > index a9be918e2b5c..c1feeb1dd077 100644
> > --- a/drivers/hid/hid-lg.c
> > +++ b/drivers/hid/hid-lg.c
> > @@ -58,7 +58,7 @@
> > * These descriptors remove the combined Y axis and instead report
> > * separate throttle (Y) and brake (RZ).
> > */
> > -static __u8 df_rdesc_fixed[] = {
> > +static const __u8 df_rdesc_fixed[] = {
> > 0x05, 0x01, /* Usage Page (Desktop), */
> > 0x09, 0x04, /* Usage (Joystick), */
> > 0xA1, 0x01, /* Collection (Application), */
> > @@ -124,7 +124,7 @@ static __u8 df_rdesc_fixed[] = {
> > 0xC0 /* End Collection */
> > };
> >
> > -static __u8 dfp_rdesc_fixed[] = {
> > +static const __u8 dfp_rdesc_fixed[] = {
> > 0x05, 0x01, /* Usage Page (Desktop), */
> > 0x09, 0x04, /* Usage (Joystick), */
> > 0xA1, 0x01, /* Collection (Application), */
> > @@ -172,7 +172,7 @@ static __u8 dfp_rdesc_fixed[] = {
> > 0xC0 /* End Collection */
> > };
> >
> > -static __u8 fv_rdesc_fixed[] = {
> > +static const __u8 fv_rdesc_fixed[] = {
> > 0x05, 0x01, /* Usage Page (Desktop), */
> > 0x09, 0x04, /* Usage (Joystick), */
> > 0xA1, 0x01, /* Collection (Application), */
> > @@ -239,7 +239,7 @@ static __u8 fv_rdesc_fixed[] = {
> > 0xC0 /* End Collection */
> > };
> >
> > -static __u8 momo_rdesc_fixed[] = {
> > +static const __u8 momo_rdesc_fixed[] = {
> > 0x05, 0x01, /* Usage Page (Desktop), */
> > 0x09, 0x04, /* Usage (Joystick), */
> > 0xA1, 0x01, /* Collection (Application), */
> > @@ -285,7 +285,7 @@ static __u8 momo_rdesc_fixed[] = {
> > 0xC0 /* End Collection */
> > };
> >
> > -static __u8 momo2_rdesc_fixed[] = {
> > +static const __u8 momo2_rdesc_fixed[] = {
> > 0x05, 0x01, /* Usage Page (Desktop), */
> > 0x09, 0x04, /* Usage (Joystick), */
> > 0xA1, 0x01, /* Collection (Application), */
> > @@ -333,7 +333,7 @@ static __u8 momo2_rdesc_fixed[] = {
> > 0xC0 /* End Collection */
> > };
> >
> > -static __u8 ffg_rdesc_fixed[] = {
> > +static const __u8 ffg_rdesc_fixed[] = {
> > 0x05, 0x01, /* Usage Page (Desktop), */
> > 0x09, 0x04, /* Usage (Joystik), */
> > 0xA1, 0x01, /* Collection (Application), */
> > @@ -379,7 +379,7 @@ static __u8 ffg_rdesc_fixed[] = {
> > 0xC0 /* End Collection */
> > };
> >
> > -static __u8 fg_rdesc_fixed[] = {
> > +static const __u8 fg_rdesc_fixed[] = {
> > 0x05, 0x01, /* Usage Page (Desktop), */
> > 0x09, 0x04, /* Usage (Joystik), */
> > 0xA1, 0x01, /* Collection (Application), */
> > @@ -431,6 +431,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > unsigned int *rsize)
> > {
> > struct lg_drv_data *drv_data = hid_get_drvdata(hdev);
> > + const __u8 *ret = NULL;
>
> Not really happy about this, usually "ret" is an int, and this makes
> things slightly harder to read.
Ack.
>
> >
> > if ((drv_data->quirks & LG_RDESC) && *rsize >= 91 && rdesc[83] == 0x26 &&
> > rdesc[84] == 0x8c && rdesc[85] == 0x02) {
> > @@ -453,7 +454,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > if (*rsize == FG_RDESC_ORIG_SIZE) {
> > hid_info(hdev,
> > "fixing up Logitech Wingman Formula GP report descriptor\n");
> > - rdesc = fg_rdesc_fixed;
> > + ret = fg_rdesc_fixed;
>
> can't you just return fg_rdesc_fixed after setting *rsize, like you did
> in the other patches?
The code looked like it wanted to avoid multiple return sites.
I tried to preserve that style, but indeed it looks wonky.
I'll resend it with the changes.
^ permalink raw reply
* Re: [PATCH 13/14] HID: lg: constify fixed up report descriptor
From: Benjamin Tissoires @ 2024-09-05 14:51 UTC (permalink / raw)
To: Thomas Weißschuh
Cc: Jiri Kosina, Marcus Folkesson, linux-input, linux-kernel
In-Reply-To: <20240828-hid-const-fixup-2-v1-13-663b9210eb69@weissschuh.net>
As you can see in the b4 reply, I've now applied all of the patches but
this one. Please see below.
On Aug 28 2024, Thomas Weißschuh wrote:
> Now that the HID core can handle const report descriptors,
> constify them where possible.
>
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> ---
> drivers/hid/hid-lg.c | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/hid-lg.c b/drivers/hid/hid-lg.c
> index a9be918e2b5c..c1feeb1dd077 100644
> --- a/drivers/hid/hid-lg.c
> +++ b/drivers/hid/hid-lg.c
> @@ -58,7 +58,7 @@
> * These descriptors remove the combined Y axis and instead report
> * separate throttle (Y) and brake (RZ).
> */
> -static __u8 df_rdesc_fixed[] = {
> +static const __u8 df_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -124,7 +124,7 @@ static __u8 df_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 dfp_rdesc_fixed[] = {
> +static const __u8 dfp_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -172,7 +172,7 @@ static __u8 dfp_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 fv_rdesc_fixed[] = {
> +static const __u8 fv_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -239,7 +239,7 @@ static __u8 fv_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 momo_rdesc_fixed[] = {
> +static const __u8 momo_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -285,7 +285,7 @@ static __u8 momo_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 momo2_rdesc_fixed[] = {
> +static const __u8 momo2_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystick), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -333,7 +333,7 @@ static __u8 momo2_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 ffg_rdesc_fixed[] = {
> +static const __u8 ffg_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystik), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -379,7 +379,7 @@ static __u8 ffg_rdesc_fixed[] = {
> 0xC0 /* End Collection */
> };
>
> -static __u8 fg_rdesc_fixed[] = {
> +static const __u8 fg_rdesc_fixed[] = {
> 0x05, 0x01, /* Usage Page (Desktop), */
> 0x09, 0x04, /* Usage (Joystik), */
> 0xA1, 0x01, /* Collection (Application), */
> @@ -431,6 +431,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> unsigned int *rsize)
> {
> struct lg_drv_data *drv_data = hid_get_drvdata(hdev);
> + const __u8 *ret = NULL;
Not really happy about this, usually "ret" is an int, and this makes
things slightly harder to read.
>
> if ((drv_data->quirks & LG_RDESC) && *rsize >= 91 && rdesc[83] == 0x26 &&
> rdesc[84] == 0x8c && rdesc[85] == 0x02) {
> @@ -453,7 +454,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == FG_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Wingman Formula GP report descriptor\n");
> - rdesc = fg_rdesc_fixed;
> + ret = fg_rdesc_fixed;
can't you just return fg_rdesc_fixed after setting *rsize, like you did
in the other patches?
(same for all of the other branches)
> *rsize = sizeof(fg_rdesc_fixed);
> } else {
> hid_info(hdev,
> @@ -466,7 +467,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == FFG_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Wingman Formula Force GP report descriptor\n");
> - rdesc = ffg_rdesc_fixed;
> + ret = ffg_rdesc_fixed;
> *rsize = sizeof(ffg_rdesc_fixed);
> }
> break;
> @@ -476,7 +477,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == DF_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Driving Force report descriptor\n");
> - rdesc = df_rdesc_fixed;
> + ret = df_rdesc_fixed;
> *rsize = sizeof(df_rdesc_fixed);
> }
> break;
> @@ -485,7 +486,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == MOMO_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Momo Force (Red) report descriptor\n");
> - rdesc = momo_rdesc_fixed;
> + ret = momo_rdesc_fixed;
> *rsize = sizeof(momo_rdesc_fixed);
> }
> break;
> @@ -494,7 +495,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == MOMO2_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Momo Racing Force (Black) report descriptor\n");
> - rdesc = momo2_rdesc_fixed;
> + ret = momo2_rdesc_fixed;
> *rsize = sizeof(momo2_rdesc_fixed);
> }
> break;
> @@ -503,7 +504,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == FV_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Formula Vibration report descriptor\n");
> - rdesc = fv_rdesc_fixed;
> + ret = fv_rdesc_fixed;
> *rsize = sizeof(fv_rdesc_fixed);
> }
> break;
> @@ -512,7 +513,7 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> if (*rsize == DFP_RDESC_ORIG_SIZE) {
> hid_info(hdev,
> "fixing up Logitech Driving Force Pro report descriptor\n");
> - rdesc = dfp_rdesc_fixed;
> + ret = dfp_rdesc_fixed;
> *rsize = sizeof(dfp_rdesc_fixed);
> }
> break;
> @@ -529,6 +530,8 @@ static const __u8 *lg_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> break;
> }
>
> + if (ret)
> + return ret;
> return rdesc;
> }
>
>
> --
> 2.46.0
>
Cheers,
Benjamin
^ permalink raw reply
* Re: (subset) [PATCH 00/14] HID: constify fixed up report descriptors
From: Benjamin Tissoires @ 2024-09-05 14:49 UTC (permalink / raw)
To: Jiri Kosina, Marcus Folkesson, Thomas Weißschuh
Cc: linux-input, linux-kernel
In-Reply-To: <20240828-hid-const-fixup-2-v1-0-663b9210eb69@weissschuh.net>
On Wed, 28 Aug 2024 09:33:19 +0200, Thomas Weißschuh wrote:
> Now that the HID core can handle const report descriptors,
> constify them where possible.
>
> This is based upon hid/for-6.12/constify-rdesc.
>
>
Applied to hid/hid.git (for-6.12/constify-rdesc), thanks!
[01/14] HID: bigbenff: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/00f6f65bd116
[02/14] HID: dr: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/49e00b5ca0bb
[03/14] HID: holtek-kbd: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/3ce7edfa4f09
[04/14] HID: keytouch: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/b299944af770
[05/14] HID: maltron: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/d8b21af66601
[06/14] HID: xiaomi: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/c06df4c57af8
[07/14] HID: vrc2: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/49cf20b878fa
[08/14] HID: viewsonic: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/4f3ff3a275f9
[09/14] HID: steelseries: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/88ae9ffc7c85
[10/14] HID: pxrc: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/4211f9b11216
[11/14] HID: sony: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/d4781a27add1
[12/14] HID: waltop: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/24b3c515c69b
[14/14] HID: uclogic: constify fixed up report descriptor
https://git.kernel.org/hid/hid/c/03f8dc1d0a38
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: [PATCH v6 3/7] leds: add driver for LEDs from qnap-mcu devices
From: Heiko Stübner @ 2024-09-05 14:50 UTC (permalink / raw)
To: Lee Jones
Cc: robh, krzk+dt, conor+dt, jdelvare, linux, dmitry.torokhov, pavel,
ukleinek, devicetree, linux-kernel, linux-hwmon, linux-arm-kernel,
linux-rockchip, linux-input, linux-leds
In-Reply-To: <20240829162705.GR6858@google.com>
Am Donnerstag, 29. August 2024, 18:27:05 CEST schrieben Sie:
> On Sun, 25 Aug 2024, Heiko Stuebner wrote:
>
> > This adds a driver that connects to the qnap-mcu mfd driver and provides
> > access to the LEDs on it.
> >
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
[...]
> > +{
> > + struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> > + u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
>
> Really not fan of these magic values being used raw like this.
Me neither, I tried my luck with QNAP support to get some sort of
documentation on what these magic characters mean, and it did
even get up to some "product team" but ultimately they decided
against providing any help.
But ok, if it makes you feel more at ease, I'll switch to at least
replaying ascii-values where possible :-) .
> > +static int qnap_mcu_err_led_blink_set(struct led_classdev *led_cdev,
> > + unsigned long *delay_on,
> > + unsigned long *delay_off)
> > +{
> > + struct qnap_mcu_err_led *err_led = cdev_to_qnap_mcu_err_led(led_cdev);
> > + u8 cmd[] = { 0x40, 0x52, 0x30 + err_led->num, 0x30 };
> > +
> > + /* LED is off, nothing to do */
> > + if (err_led->mode == QNAP_MCU_ERR_LED_OFF)
> > + return 0;
> > +
> > + if (*delay_on < 500) {
>
> Setting delay_on based on the current value of delay_on sounds sketchy.
As far as I understood the API, the parameter should indicated the wanted
blink time, while the function then should set in those variables the delay
the driver actually set.
So if the delay_on is < 500, select the fast blink mode, which is this
100/100 variant and for everything >= 500 the slow blink mode.
I.e. you set the trigger to "timer" and this creates the delay_on and
delay_off sysfs files, where you put you wish into and can read the
actually set delays out of.
> > + *delay_on = 100;
> > + *delay_off = 100;
> > + err_led->mode = QNAP_MCU_ERR_LED_BLINK_FAST;
> > + } else {
> > + *delay_on = 500;
> > + *delay_off = 500;
> > + err_led->mode = QNAP_MCU_ERR_LED_BLINK_SLOW;
> > + }
>
> How do you change from a fast to a slow blinking LED and back again?
echo timer > /sys/class/leds/hdd4:red:status/trigger
## creates delay_on + delay_off sysfs files
echo 150 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 100
echo 250 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 100
## switch to slow blink
echo 500 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 500
echo 600 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 500
## switch to fast blink again
echo 150 > /sys/class/leds/hdd4:red:status/delay_on
cat /sys/class/leds/hdd4:red:status/delay_on --> 100
echo heartbeat > /sys/class/leds/hdd4:red:status/trigger
## removes the delay_on + delay_off files again
Heiko
^ permalink raw reply
* Re: [PATCH 0/5] Remove support for platform data from matrix keypad driver
From: Arnd Bergmann @ 2024-09-05 14:36 UTC (permalink / raw)
To: Linus Walleij, Dmitry Torokhov
Cc: Haojian Zhuang, Daniel Mack, Robert Jarzmik, soc,
linux-arm-kernel, linux-kernel, linux-input
In-Reply-To: <CACRpkdZ_y=2WKCLwi5or-=MasvNw2bcxht6a+bFjV=yAfvQhdQ@mail.gmail.com>
On Mon, Aug 26, 2024, at 08:52, Linus Walleij wrote:
> On Fri, Aug 23, 2024 at 6:02 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
>
>> I'm glad that we agree that we do not want the elaborate merge process
>> and instead push the changes through one tree in one shot we just need
>> to decide which one - soc or input. I am fine with using either.
>
> I'm also fine with either, but let's take the input tree because the
> you're in direct control of it so it will be easier.
Sorry I dropped the ball here, I just saw that these five patches
are still in the patchwork waiting for me to apply them.
I'm also happy for them to go through the input tree, and have
marked them as not-for-us in patchwork now. Dmitry, please add
my
Acked-by: Arnd Bergmann <arnd@arndb.de>
and pick them up in your tree. I've checked that there are no
conflicts against contents of the SoC tree. If you prefer me to
pick them up after all, that is also fine, but please resend in
that case.
Arnd
^ permalink raw reply
* Re: [PATCH -next 11/19] HID: wiimote: Use devm_hid_hw_start_and_open in wiimote_hid_probe()
From: Benjamin Tissoires @ 2024-09-05 13:05 UTC (permalink / raw)
To: Li Zetao
Cc: jikos, michael.zaidman, gupt21, djogorchock, rrameshbabu, bonbons,
roderick.colenbrander, david, savicaleksa83, me, jdelvare, linux,
mail, wilken.gottwalt, jonas, mezin.alexander, linux-input,
linux-i2c, linux-hwmon
In-Reply-To: <20240904123607.3407364-12-lizetao1@huawei.com>
On Sep 04 2024, Li Zetao wrote:
> Currently, the wiimote module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> that hid resources are consistent with the device life cycle, and release
> hid resources before device is released. At the same time, it can avoid
> the goto-release encoding, drop the err_close and err_stop lables.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
> drivers/hid/hid-wiimote-core.c | 20 +++-----------------
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 26167cfb696f..28cd9ccbb617 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1780,8 +1780,6 @@ static void wiimote_destroy(struct wiimote_data *wdata)
> wiimote_ext_unload(wdata);
> wiimote_modules_unload(wdata);
> cancel_work_sync(&wdata->queue.worker);
> - hid_hw_close(wdata->hdev);
> - hid_hw_stop(wdata->hdev);
>
> kfree(wdata);
This is wrong because wdata is not devres managed.
So there is probably a race condition where it gets freed while the
underlying bus wasn't stopped.
Cheers,
Benjamin
> }
> @@ -1806,22 +1804,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
> goto err;
> }
>
> - ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> - if (ret) {
> - hid_err(hdev, "HW start failed\n");
> + ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> + if (ret)
> goto err;
> - }
> -
> - ret = hid_hw_open(hdev);
> - if (ret) {
> - hid_err(hdev, "cannot start hardware I/O\n");
> - goto err_stop;
> - }
>
> ret = device_create_file(&hdev->dev, &dev_attr_extension);
> if (ret) {
> hid_err(hdev, "cannot create sysfs attribute\n");
> - goto err_close;
> + goto err;
> }
>
> ret = device_create_file(&hdev->dev, &dev_attr_devtype);
> @@ -1847,10 +1837,6 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>
> err_ext:
> device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> -err_close:
> - hid_hw_close(hdev);
> -err_stop:
> - hid_hw_stop(hdev);
> err:
> input_free_device(wdata->ir);
> input_free_device(wdata->accel);
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH -next 10/19] HID: hid-steam: Use devm_hid_hw_start_and_open in steam_probe()
From: Benjamin Tissoires @ 2024-09-05 13:00 UTC (permalink / raw)
To: Li Zetao
Cc: jikos, michael.zaidman, gupt21, djogorchock, rrameshbabu, bonbons,
roderick.colenbrander, david, savicaleksa83, me, jdelvare, linux,
mail, wilken.gottwalt, jonas, mezin.alexander, linux-input,
linux-i2c, linux-hwmon
In-Reply-To: <20240904123607.3407364-11-lizetao1@huawei.com>
On Sep 04 2024, Li Zetao wrote:
> Currently, the hid-steam module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> that hid resources are consistent with the device life cycle, and release
> hid resources before device is released. At the same time, it can avoid
> the goto-release encoding, drop the err_hw_close and err_hw_stop lables.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
> drivers/hid/hid-steam.c | 18 ++----------------
> 1 file changed, 2 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index bf8b633114be..d393762bf52f 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1236,18 +1236,10 @@ static int steam_probe(struct hid_device *hdev,
> * With the real steam controller interface, do not connect hidraw.
> * Instead, create the client_hid and connect that.
> */
> - ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> + ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
> if (ret)
> goto err_cancel_work;
>
> - ret = hid_hw_open(hdev);
> - if (ret) {
> - hid_err(hdev,
> - "%s:hid_hw_open\n",
> - __func__);
> - goto err_hw_stop;
> - }
> -
> if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> hid_info(hdev, "Steam wireless receiver connected");
> /* If using a wireless adaptor ask for connection status */
> @@ -1261,7 +1253,7 @@ static int steam_probe(struct hid_device *hdev,
> hid_err(hdev,
> "%s:steam_register failed with error %d\n",
> __func__, ret);
> - goto err_hw_close;
> + goto err_cancel_work;
> }
> }
>
> @@ -1283,10 +1275,6 @@ static int steam_probe(struct hid_device *hdev,
> err_steam_unregister:
> if (steam->connected)
> steam_unregister(steam);
> -err_hw_close:
> - hid_hw_close(hdev);
> -err_hw_stop:
> - hid_hw_stop(hdev);
> err_cancel_work:
> cancel_work_sync(&steam->work_connect);
> cancel_delayed_work_sync(&steam->mode_switch);
> @@ -1312,8 +1300,6 @@ static void steam_remove(struct hid_device *hdev)
> if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> hid_info(hdev, "Steam wireless receiver disconnected");
> }
> - hid_hw_close(hdev);
> - hid_hw_stop(hdev);
> steam_unregister(steam);
steam_unregister was called after hid_hw_stop, and now it's before.
Someone should ensure this is correct...
Cheers,
Benjamin
> }
>
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH -next 08/19] HID: hid-picolcd: Use devm_hid_hw_start_and_open in picolcd_probe()
From: Benjamin Tissoires @ 2024-09-05 12:59 UTC (permalink / raw)
To: Li Zetao
Cc: jikos, michael.zaidman, gupt21, djogorchock, rrameshbabu, bonbons,
roderick.colenbrander, david, savicaleksa83, me, jdelvare, linux,
mail, wilken.gottwalt, jonas, mezin.alexander, linux-input,
linux-i2c, linux-hwmon
In-Reply-To: <20240904123607.3407364-9-lizetao1@huawei.com>
On Sep 04 2024, Li Zetao wrote:
> Currently, the hid-picolcd module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> that hid resources are consistent with the device life cycle, and release
> hid resources before device is released. At the same time, it can avoid
> the goto-release encoding, drop the err_cleanup_hid_ll and
> err_cleanup_hid_hw lables.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
> drivers/hid/hid-picolcd_core.c | 22 ++++------------------
> 1 file changed, 4 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index 297103be3381..973822d1b2db 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -551,23 +551,15 @@ static int picolcd_probe(struct hid_device *hdev,
> goto err_cleanup_data;
> }
>
> - error = hid_hw_start(hdev, 0);
> + error = devm_hid_hw_start_and_open(hdev, 0);
> if (error) {
> - hid_err(hdev, "hardware start failed\n");
> + hid_err(hdev, "hardware start and open failed\n");
> goto err_cleanup_data;
> }
>
> - error = hid_hw_open(hdev);
> - if (error) {
> - hid_err(hdev, "failed to open input interrupt pipe for key and IR events\n");
> - goto err_cleanup_hid_hw;
> - }
> -
> error = device_create_file(&hdev->dev, &dev_attr_operation_mode_delay);
> - if (error) {
> - hid_err(hdev, "failed to create sysfs attributes\n");
> - goto err_cleanup_hid_ll;
> - }
> + if (error)
> + goto err_cleanup_data;
>
> error = device_create_file(&hdev->dev, &dev_attr_operation_mode);
> if (error) {
> @@ -589,10 +581,6 @@ static int picolcd_probe(struct hid_device *hdev,
> device_remove_file(&hdev->dev, &dev_attr_operation_mode);
> err_cleanup_sysfs1:
> device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
> -err_cleanup_hid_ll:
> - hid_hw_close(hdev);
> -err_cleanup_hid_hw:
> - hid_hw_stop(hdev);
> err_cleanup_data:
> kfree(data);
> return error;
> @@ -611,8 +599,6 @@ static void picolcd_remove(struct hid_device *hdev)
> picolcd_exit_devfs(data);
> device_remove_file(&hdev->dev, &dev_attr_operation_mode);
> device_remove_file(&hdev->dev, &dev_attr_operation_mode_delay);
> - hid_hw_close(hdev);
> - hid_hw_stop(hdev);
Again, this doesn't seem correct. the spin_lock from below expects the
hw to be stopped, and this patch changes that by postponing the stop
after the remove.
CHeers,
Benjamin
>
> /* Shortcut potential pending reply that will never arrive */
> spin_lock_irqsave(&data->lock, flags);
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH -next 07/19] HID: shield: Use devm_hid_hw_start_and_open in shield_probe()
From: Benjamin Tissoires @ 2024-09-05 12:57 UTC (permalink / raw)
To: Li Zetao
Cc: jikos, michael.zaidman, gupt21, djogorchock, rrameshbabu, bonbons,
roderick.colenbrander, david, savicaleksa83, me, jdelvare, linux,
mail, wilken.gottwalt, jonas, mezin.alexander, linux-input,
linux-i2c, linux-hwmon
In-Reply-To: <20240904123607.3407364-8-lizetao1@huawei.com>
On Sep 04 2024, Li Zetao wrote:
> Currently, the shield module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> that hid resources are consistent with the device life cycle, and release
> hid resources before device is released. At the same time, it can avoid
> the goto-release encoding, drop the err_stop and err_ts_create lables, and
> directly return the error code when an error occurs.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
> drivers/hid/hid-nvidia-shield.c | 20 +++-----------------
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hid/hid-nvidia-shield.c b/drivers/hid/hid-nvidia-shield.c
> index ff9078ad1961..747996a21dd9 100644
> --- a/drivers/hid/hid-nvidia-shield.c
> +++ b/drivers/hid/hid-nvidia-shield.c
> @@ -1070,27 +1070,15 @@ static int shield_probe(struct hid_device *hdev, const struct hid_device_id *id)
>
> ts = container_of(shield_dev, struct thunderstrike, base);
>
> - ret = hid_hw_start(hdev, HID_CONNECT_HIDINPUT);
> + ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDINPUT);
> if (ret) {
> - hid_err(hdev, "Failed to start HID device\n");
> - goto err_ts_create;
> - }
> -
> - ret = hid_hw_open(hdev);
> - if (ret) {
> - hid_err(hdev, "Failed to open HID device\n");
> - goto err_stop;
> + thunderstrike_destroy(ts);
> + return ret;
> }
>
> thunderstrike_device_init_info(shield_dev);
>
> return ret;
> -
> -err_stop:
> - hid_hw_stop(hdev);
> -err_ts_create:
> - thunderstrike_destroy(ts);
> - return ret;
> }
>
> static void shield_remove(struct hid_device *hdev)
> @@ -1100,11 +1088,9 @@ static void shield_remove(struct hid_device *hdev)
>
> ts = container_of(dev, struct thunderstrike, base);
>
> - hid_hw_close(hdev);
> thunderstrike_destroy(ts);
> del_timer_sync(&ts->psy_stats_timer);
> cancel_work_sync(&ts->hostcmd_req_work);
> - hid_hw_stop(hdev);
There is definitely something fishy here. The 2 calls to hid_hw_close
and hid_hw_stop are not one after the other and at the very end of the
.remove(). The patch is changing the behavior, and this might be an
issue.
Cheers,
Benjamin
> }
>
> static const struct hid_device_id shield_devices[] = {
> --
> 2.34.1
>
^ permalink raw reply
* Re: [PATCH -next 01/19] HID: core: Use devm_add_action_or_reset helper to manage hid resources
From: Benjamin Tissoires @ 2024-09-05 12:53 UTC (permalink / raw)
To: Li Zetao
Cc: jikos, michael.zaidman, gupt21, djogorchock, rrameshbabu, bonbons,
roderick.colenbrander, david, savicaleksa83, me, jdelvare, linux,
mail, wilken.gottwalt, jonas, mezin.alexander, linux-input,
linux-i2c, linux-hwmon
In-Reply-To: <20240904123607.3407364-2-lizetao1@huawei.com>
On Sep 04 2024, Li Zetao wrote:
> By adding a custom action to the device, it can bind the hid resource
> to the hid_device life cycle. The framework automatically close and stop
> the hid resources before hid_device is released, and the users do not
> need to pay attention to the timing of hid resource release.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
> drivers/hid/hid-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
> include/linux/hid.h | 2 ++
> 2 files changed, 42 insertions(+)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 30de92d0bf0f..71143c0a4a02 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -2416,6 +2416,46 @@ void hid_hw_close(struct hid_device *hdev)
> }
> EXPORT_SYMBOL_GPL(hid_hw_close);
>
> +static void hid_hw_close_and_stop(void *data)
> +{
> + struct hid_device *hdev = data;
> +
> + hid_hw_close(hdev);
> + hid_hw_stop(hdev);
> +}
> +
> +/**
> + * devm_hid_hw_start_and_open - manage hid resources through custom action
> + *
> + * @hdev: hid device
> + * @connect_mask: which outputs to connect, see HID_CONNECT_*
> + *
> + * Bind the hid resource to the hid_device life cycle and register
> + * an action to release the hid resource. The users do not need to
> + * pay attention to the release of hid.
The only problem I see here is that this API is not completely "safe",
in the sense that a driver using it can not use manual kzalloc/kfree.
It is obvious, but probably not so much while looking at the comments
from Guenter where he asked you to also remove the .remove() callback.
So in the docs, we should probably state that if the .remove() is any
different than "hid_hw_close(hdev);hid_hw_stop(hdev);", care should be
use with that function.
Cheers,
Benjamin
> + */
> +
> +int devm_hid_hw_start_and_open(struct hid_device *hdev, unsigned int connect_mask)
> +{
> + int ret;
> +
> + ret = hid_hw_start(hdev, connect_mask);
> + if (ret) {
> + hid_err(hdev, "hw start failed with %d\n", ret);
> + return ret;
> + }
> +
> + ret = hid_hw_open(hdev);
> + if (ret) {
> + hid_err(hdev, "hw open failed with %d\n", ret);
> + hid_hw_stop(hdev);
> + return ret;
> + }
> +
> + return devm_add_action_or_reset(&hdev->dev, hid_hw_close_and_stop, hdev);
> +}
> +EXPORT_SYMBOL_GPL(devm_hid_hw_start_and_open);
> +
> /**
> * hid_hw_request - send report request to device
> *
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 121d5b8bc867..0ce217aa5f62 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -1125,6 +1125,8 @@ int __must_check hid_hw_start(struct hid_device *hdev,
> void hid_hw_stop(struct hid_device *hdev);
> int __must_check hid_hw_open(struct hid_device *hdev);
> void hid_hw_close(struct hid_device *hdev);
> +int __must_check devm_hid_hw_start_and_open(struct hid_device *hdev,
> + unsigned int connect_mask);
> void hid_hw_request(struct hid_device *hdev,
> struct hid_report *report, enum hid_class_request reqtype);
> int __hid_hw_raw_request(struct hid_device *hdev,
> --
> 2.34.1
>
^ permalink raw reply
* Re: [bug report] selftests/hid: add tests for bpf_hid_hw_request
From: Benjamin Tissoires @ 2024-09-05 12:45 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-input
In-Reply-To: <a8082ffb-68f7-425b-a3f8-f9f14dbf2a92@stanley.mountain>
On Sep 05 2024, Dan Carpenter wrote:
> [ Moving these functions around made them show up as new warnings. ]
Thanks for the report Dan.
>
> Hello Benjamin Tissoires,
>
> Commit 4f7153cf461e ("selftests/hid: add tests for
> bpf_hid_hw_request") from Nov 3, 2022 (linux-next), leads to the
> following Smatch static checker warning:
>
> tools/testing/selftests/hid/hid_common.h:212 uhid_event()
> warn: assigning (-5) to unsigned variable 'answer.u.get_report_reply.err'
>
> tools/testing/selftests/hid/hid_common.h
> 207 case UHID_GET_REPORT:
> 208 UHID_LOG("UHID_GET_REPORT from uhid-dev");
> 209
> 210 answer.type = UHID_GET_REPORT_REPLY;
> 211 answer.u.get_report_reply.id = ev.u.get_report.id;
> --> 212 answer.u.get_report_reply.err = ev.u.get_report.rnum == 1 ? 0 : -EIO;
>
> answer.u.get_report_reply.err is a u16. I couldn't figure out which kind of
> error codes it's supposed to store or how they're used...
It's in drivers/hid/uhid.c:262:
req = &uhid->report_buf.u.get_report_reply;
if (req->err) {
ret = -EIO;
} else {
ret = min3(count, (size_t)req->size, (size_t)UHID_DATA_MAX);
memcpy(buf, req->data, ret);
}
So any non null value triggers an error, and furthermore the hardcoded
value is -EIO, which probably explains why I did not noticed it.
We should either store EIO, either a plain 1...
Cheers,
Benjamin
>
> 213 answer.u.get_report_reply.size = sizeof(feature_data);
> 214 memcpy(answer.u.get_report_reply.data, feature_data, sizeof(feature_data));
> 215
> 216 uhid_write(_metadata, fd, &answer);
> 217
> 218 break;
> 219 case UHID_SET_REPORT:
> 220 UHID_LOG("UHID_SET_REPORT from uhid-dev");
> 221 break;
> 222 default:
>
> regards,
> dan carpenter
^ permalink raw reply
* Re: [RFC PATCH] HID: wacom: Stop mangling tool IDs
From: Benjamin Tissoires @ 2024-09-05 9:01 UTC (permalink / raw)
To: Jiri Kosina
Cc: Gerecke, Jason, linux-input, Benjamin Tissoires, Dmitry Torokhov,
Ping Cheng, Tobita, Tatsunosuke, Erin Skomra, Joshua Dickens,
Peter Hutterer, Jason Gerecke
In-Reply-To: <nycvar.YFH.7.76.2409051045470.31206@cbobk.fhfr.pm>
On Sep 05 2024, Jiri Kosina wrote:
> On Tue, 3 Sep 2024, Gerecke, Jason wrote:
>
> > From: Jason Gerecke <jason.gerecke@wacom.com>
> >
> > In ancient times, an off-by-one-nybble error resulted in the Wacom
> > driver sending "mangled" tool IDs to userspace. This mangling behavior
> > was later enshrined into a function so that devices using the then-new
> > generic codepath could share the same broken behavior. The mangled IDs
> > have not historically been a major problem for userspace since few
> > applications care about the exact numeric value of the IDs. As long as
> > the IDs were unique it didn't much matter. Some programs (cross-
> > platform and remote-desktop applications in particular) /do/ rely on
> > the IDs being correct, however.
> >
> > This patch rids the driver of the mangling behavior.
> >
> > Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> > References: 493630b20389 ("Input: wacom - fix serial number handling on Cintiq 21UX2")
> > References: 82527da319ee ("HID: wacom: Read and internally use corrected Intuos tool IDs")
> > ---
> > I'd like to get the opinion of the kernel maintainers on making a
> > change like this at some point in the future. There are _very_ few
> > userspace uses of these IDs (primarily: drivers, compositors, and
> > tablet control panels) and my plan is to update those bits and then
> > give some time for the changes to roll out to users before re-
> > submitting this for real. I don't expect any kind of breakage since
> > we'll be taking our time with the rollout and userspace needs to
> > have handling for "unknown" IDs anyway (since Wacom periodically
> > releases new pens).
>
> As you are in control of the whole ecosystem anyway (because it's Wacom
> specific), I don't see it as particularly problematic.
>
Yeah, same here. And that change doesn't impact the basic functionality
of the styluses: they'll still be handled properly as pointers, but only
the configuration panel IIUC. Peter mentioned about some slight
differences in libinput which shouldn't impact the users as well IIRC.
Anyway, that's an OK from me too.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: picoLCD: Use backlight power constants
From: Jiri Kosina @ 2024-09-05 8:55 UTC (permalink / raw)
To: Thomas Zimmermann; +Cc: bonbons, bentiss, linux-input
In-Reply-To: <20240731131017.1149175-1-tzimmermann@suse.de>
On Wed, 31 Jul 2024, Thomas Zimmermann wrote:
> Replace FB_BLANK_ constants with their counterparts from the
> backlight subsystem. The values are identical, so there's no
> change in functionality or semantics.
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFC PATCH] HID: wacom: Stop mangling tool IDs
From: Jiri Kosina @ 2024-09-05 8:46 UTC (permalink / raw)
To: Gerecke, Jason
Cc: linux-input, Benjamin Tissoires, Dmitry Torokhov, Ping Cheng,
Tobita, Tatsunosuke, Erin Skomra, Joshua Dickens, Peter Hutterer,
Jason Gerecke
In-Reply-To: <20240903182633.870892-1-jason.gerecke@wacom.com>
On Tue, 3 Sep 2024, Gerecke, Jason wrote:
> From: Jason Gerecke <jason.gerecke@wacom.com>
>
> In ancient times, an off-by-one-nybble error resulted in the Wacom
> driver sending "mangled" tool IDs to userspace. This mangling behavior
> was later enshrined into a function so that devices using the then-new
> generic codepath could share the same broken behavior. The mangled IDs
> have not historically been a major problem for userspace since few
> applications care about the exact numeric value of the IDs. As long as
> the IDs were unique it didn't much matter. Some programs (cross-
> platform and remote-desktop applications in particular) /do/ rely on
> the IDs being correct, however.
>
> This patch rids the driver of the mangling behavior.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> References: 493630b20389 ("Input: wacom - fix serial number handling on Cintiq 21UX2")
> References: 82527da319ee ("HID: wacom: Read and internally use corrected Intuos tool IDs")
> ---
> I'd like to get the opinion of the kernel maintainers on making a
> change like this at some point in the future. There are _very_ few
> userspace uses of these IDs (primarily: drivers, compositors, and
> tablet control panels) and my plan is to update those bits and then
> give some time for the changes to roll out to users before re-
> submitting this for real. I don't expect any kind of breakage since
> we'll be taking our time with the rollout and userspace needs to
> have handling for "unknown" IDs anyway (since Wacom periodically
> releases new pens).
As you are in control of the whole ecosystem anyway (because it's Wacom
specific), I don't see it as particularly problematic.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] dt-bindings: input: touchscreen: edt-ft5x06: Use generic node name
From: Krzysztof Kozlowski @ 2024-09-05 8:46 UTC (permalink / raw)
To: Fabio Estevam, dmitry.torokhov
Cc: robh, krzk+dt, conor+dt, linux-input, devicetree, Fabio Estevam
In-Reply-To: <20240829134428.3347075-1-festevam@gmail.com>
On 29/08/2024 15:44, Fabio Estevam wrote:
> From: Fabio Estevam <festevam@denx.de>
>
> Node names should be generic.
>
> Improve the binding example by using 'touchscreen' as the node name.
>
> Signed-off-by: Fabio Estevam <festevam@denx.de>
> ---
There are other files in touschscreen with the same issue. This is
trivial, non-functional change so do not do it one file per patch. It's
way too much churn. Fix everything or just don't touch.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH -next 11/19] HID: wiimote: Use devm_hid_hw_start_and_open in wiimote_hid_probe()
From: David Rheinsberg @ 2024-09-05 6:35 UTC (permalink / raw)
To: Li Zetao, Jiri Kosina, bentiss, michael.zaidman, gupt21,
djogorchock, rrameshbabu, bonbons, roderick.colenbrander,
savicaleksa83, me, jdelvare, linux, mail, wilken.gottwalt, jonas,
mezin.alexander
Cc: linux-input, linux-i2c, linux-hwmon
In-Reply-To: <20240904123607.3407364-12-lizetao1@huawei.com>
Hi
On Wed, Sep 4, 2024, at 2:35 PM, Li Zetao wrote:
> Currently, the wiimote module needs to maintain hid resources
> by itself. Consider using devm_hid_hw_start_and_open helper to ensure
> that hid resources are consistent with the device life cycle, and release
> hid resources before device is released. At the same time, it can avoid
> the goto-release encoding, drop the err_close and err_stop lables.
>
> Signed-off-by: Li Zetao <lizetao1@huawei.com>
> ---
> drivers/hid/hid-wiimote-core.c | 20 +++-----------------
> 1 file changed, 3 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/hid/hid-wiimote-core.c b/drivers/hid/hid-wiimote-core.c
> index 26167cfb696f..28cd9ccbb617 100644
> --- a/drivers/hid/hid-wiimote-core.c
> +++ b/drivers/hid/hid-wiimote-core.c
> @@ -1780,8 +1780,6 @@ static void wiimote_destroy(struct wiimote_data *wdata)
> wiimote_ext_unload(wdata);
> wiimote_modules_unload(wdata);
> cancel_work_sync(&wdata->queue.worker);
> - hid_hw_close(wdata->hdev);
> - hid_hw_stop(wdata->hdev);
>
> kfree(wdata);
> }
> @@ -1806,22 +1804,14 @@ static int wiimote_hid_probe(struct hid_device *hdev,
> goto err;
> }
>
> - ret = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
> - if (ret) {
> - hid_err(hdev, "HW start failed\n");
> + ret = devm_hid_hw_start_and_open(hdev, HID_CONNECT_HIDRAW);
> + if (ret)
> goto err;
> - }
> -
> - ret = hid_hw_open(hdev);
> - if (ret) {
> - hid_err(hdev, "cannot start hardware I/O\n");
> - goto err_stop;
> - }
>
> ret = device_create_file(&hdev->dev, &dev_attr_extension);
> if (ret) {
> hid_err(hdev, "cannot create sysfs attribute\n");
> - goto err_close;
> + goto err;
> }
>
> ret = device_create_file(&hdev->dev, &dev_attr_devtype);
> @@ -1847,10 +1837,6 @@ static int wiimote_hid_probe(struct hid_device *hdev,
>
> err_ext:
> device_remove_file(&wdata->hdev->dev, &dev_attr_extension);
> -err_close:
> - hid_hw_close(hdev);
> -err_stop:
> - hid_hw_stop(hdev);
> err:
> input_free_device(wdata->ir);
> input_free_device(wdata->accel);
> --
> 2.34.1
Looks good!
Reviewed-by: David Rheinsberg <david@readahead.eu>
Thanks
David
^ permalink raw reply
* [bug report] selftests/hid: add tests for bpf_hid_hw_request
From: Dan Carpenter @ 2024-09-05 6:17 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input
[ Moving these functions around made them show up as new warnings. ]
Hello Benjamin Tissoires,
Commit 4f7153cf461e ("selftests/hid: add tests for
bpf_hid_hw_request") from Nov 3, 2022 (linux-next), leads to the
following Smatch static checker warning:
tools/testing/selftests/hid/hid_common.h:212 uhid_event()
warn: assigning (-5) to unsigned variable 'answer.u.get_report_reply.err'
tools/testing/selftests/hid/hid_common.h
207 case UHID_GET_REPORT:
208 UHID_LOG("UHID_GET_REPORT from uhid-dev");
209
210 answer.type = UHID_GET_REPORT_REPLY;
211 answer.u.get_report_reply.id = ev.u.get_report.id;
--> 212 answer.u.get_report_reply.err = ev.u.get_report.rnum == 1 ? 0 : -EIO;
answer.u.get_report_reply.err is a u16. I couldn't figure out which kind of
error codes it's supposed to store or how they're used...
213 answer.u.get_report_reply.size = sizeof(feature_data);
214 memcpy(answer.u.get_report_reply.data, feature_data, sizeof(feature_data));
215
216 uhid_write(_metadata, fd, &answer);
217
218 break;
219 case UHID_SET_REPORT:
220 UHID_LOG("UHID_SET_REPORT from uhid-dev");
221 break;
222 default:
regards,
dan carpenter
^ permalink raw reply
* [PATCH 24/24] Input: xilinx_ps2 - use guard notation when acquiring spinlock
From: Dmitry Torokhov @ 2024-09-05 4:17 UTC (permalink / raw)
To: linux-input
Cc: Pali Rohár, Helge Deller, K. Y. Srinivasan, Wei Liu,
Dexuan Cui, Samuel Holland, Lyude Paul, Michal Simek,
Hans de Goede, linux-kernel, linux-hyperv, linux-arm-kernel,
linux-sunxi
In-Reply-To: <20240905041732.2034348-1-dmitry.torokhov@gmail.com>
Using guard notation makes the code more compact and error handling
more robust by ensuring that locks are released in all code paths
when control leaves critical section.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/serio/xilinx_ps2.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/input/serio/xilinx_ps2.c b/drivers/input/serio/xilinx_ps2.c
index 1543267d02ac..0316760168e5 100644
--- a/drivers/input/serio/xilinx_ps2.c
+++ b/drivers/input/serio/xilinx_ps2.c
@@ -155,22 +155,17 @@ static irqreturn_t xps2_interrupt(int irq, void *dev_id)
static int sxps2_write(struct serio *pserio, unsigned char c)
{
struct xps2data *drvdata = pserio->port_data;
- unsigned long flags;
u32 sr;
- int status = -1;
- spin_lock_irqsave(&drvdata->lock, flags);
+ guard(spinlock_irqsave)(&drvdata->lock);
/* If the PS/2 transmitter is empty send a byte of data */
sr = in_be32(drvdata->base_address + XPS2_STATUS_OFFSET);
- if (!(sr & XPS2_STATUS_TX_FULL)) {
- out_be32(drvdata->base_address + XPS2_TX_DATA_OFFSET, c);
- status = 0;
- }
+ if (sr & XPS2_STATUS_TX_FULL)
+ return -EAGAIN;
- spin_unlock_irqrestore(&drvdata->lock, flags);
-
- return status;
+ out_be32(drvdata->base_address + XPS2_TX_DATA_OFFSET, c);
+ return 0;
}
/**
--
2.46.0.469.g59c65b2a67-goog
^ permalink raw reply related
* [PATCH 23/24] Input: userio - switch to using cleanup functions
From: Dmitry Torokhov @ 2024-09-05 4:17 UTC (permalink / raw)
To: linux-input
Cc: Pali Rohár, Helge Deller, K. Y. Srinivasan, Wei Liu,
Dexuan Cui, Samuel Holland, Lyude Paul, Michal Simek,
Hans de Goede, linux-kernel, linux-hyperv, linux-arm-kernel,
linux-sunxi
In-Reply-To: <20240905041732.2034348-1-dmitry.torokhov@gmail.com>
Use __free() and guard() primitives to simplify the code and error
handling.
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
drivers/input/serio/userio.c | 141 +++++++++++++++++------------------
1 file changed, 70 insertions(+), 71 deletions(-)
diff --git a/drivers/input/serio/userio.c b/drivers/input/serio/userio.c
index a88e2eee55c3..66c9838a1fa7 100644
--- a/drivers/input/serio/userio.c
+++ b/drivers/input/serio/userio.c
@@ -55,18 +55,15 @@ struct userio_device {
static int userio_device_write(struct serio *id, unsigned char val)
{
struct userio_device *userio = id->port_data;
- unsigned long flags;
- spin_lock_irqsave(&userio->buf_lock, flags);
+ scoped_guard(spinlock_irqsave, &userio->buf_lock) {
+ userio->buf[userio->head] = val;
+ userio->head = (userio->head + 1) % USERIO_BUFSIZE;
- userio->buf[userio->head] = val;
- userio->head = (userio->head + 1) % USERIO_BUFSIZE;
-
- if (userio->head == userio->tail)
- dev_warn(userio_misc.this_device,
- "Buffer overflowed, userio client isn't keeping up");
-
- spin_unlock_irqrestore(&userio->buf_lock, flags);
+ if (userio->head == userio->tail)
+ dev_warn(userio_misc.this_device,
+ "Buffer overflowed, userio client isn't keeping up");
+ }
wake_up_interruptible(&userio->waitq);
@@ -75,9 +72,8 @@ static int userio_device_write(struct serio *id, unsigned char val)
static int userio_char_open(struct inode *inode, struct file *file)
{
- struct userio_device *userio;
-
- userio = kzalloc(sizeof(*userio), GFP_KERNEL);
+ struct userio_device *userio __free(kfree) =
+ kzalloc(sizeof(*userio), GFP_KERNEL);
if (!userio)
return -ENOMEM;
@@ -86,15 +82,13 @@ static int userio_char_open(struct inode *inode, struct file *file)
init_waitqueue_head(&userio->waitq);
userio->serio = kzalloc(sizeof(*userio->serio), GFP_KERNEL);
- if (!userio->serio) {
- kfree(userio);
+ if (!userio->serio)
return -ENOMEM;
- }
userio->serio->write = userio_device_write;
- userio->serio->port_data = userio;
+ userio->serio->port_data = userio;;
- file->private_data = userio;
+ file->private_data = no_free_ptr(userio);
return 0;
}
@@ -118,14 +112,32 @@ static int userio_char_release(struct inode *inode, struct file *file)
return 0;
}
+static size_t userio_fetch_data(struct userio_device *userio, u8 *buf,
+ size_t count, size_t *copylen)
+{
+ size_t available, len;
+
+ guard(spinlock_irqsave)(&userio->buf_lock);
+
+ available = CIRC_CNT_TO_END(userio->head, userio->tail,
+ USERIO_BUFSIZE);
+ len = min(available, count);
+ if (len) {
+ memcpy(buf, &userio->buf[userio->tail], len);
+ userio->tail = (userio->tail + len) % USERIO_BUFSIZE;
+ }
+
+ *copylen = len;
+ return available;
+}
+
static ssize_t userio_char_read(struct file *file, char __user *user_buffer,
size_t count, loff_t *ppos)
{
struct userio_device *userio = file->private_data;
int error;
- size_t nonwrap_len, copylen;
- unsigned char buf[USERIO_BUFSIZE];
- unsigned long flags;
+ size_t available, copylen;
+ u8 buf[USERIO_BUFSIZE];
/*
* By the time we get here, the data that was waiting might have
@@ -135,21 +147,8 @@ static ssize_t userio_char_read(struct file *file, char __user *user_buffer,
* of course).
*/
for (;;) {
- spin_lock_irqsave(&userio->buf_lock, flags);
-
- nonwrap_len = CIRC_CNT_TO_END(userio->head,
- userio->tail,
- USERIO_BUFSIZE);
- copylen = min(nonwrap_len, count);
- if (copylen) {
- memcpy(buf, &userio->buf[userio->tail], copylen);
- userio->tail = (userio->tail + copylen) %
- USERIO_BUFSIZE;
- }
-
- spin_unlock_irqrestore(&userio->buf_lock, flags);
-
- if (nonwrap_len)
+ available = userio_fetch_data(userio, buf, count, ©len);
+ if (available)
break;
/* buffer was/is empty */
@@ -176,40 +175,21 @@ static ssize_t userio_char_read(struct file *file, char __user *user_buffer,
return copylen;
}
-static ssize_t userio_char_write(struct file *file, const char __user *buffer,
- size_t count, loff_t *ppos)
+static int userio_execute_cmd(struct userio_device *userio,
+ const struct userio_cmd *cmd)
{
- struct userio_device *userio = file->private_data;
- struct userio_cmd cmd;
- int error;
-
- if (count != sizeof(cmd)) {
- dev_warn(userio_misc.this_device, "Invalid payload size\n");
- return -EINVAL;
- }
-
- if (copy_from_user(&cmd, buffer, sizeof(cmd)))
- return -EFAULT;
-
- error = mutex_lock_interruptible(&userio->mutex);
- if (error)
- return error;
-
- switch (cmd.type) {
+ switch (cmd->type) {
case USERIO_CMD_REGISTER:
if (!userio->serio->id.type) {
dev_warn(userio_misc.this_device,
"No port type given on /dev/userio\n");
-
- error = -EINVAL;
- goto out;
+ return -EINVAL;
}
if (userio->running) {
dev_warn(userio_misc.this_device,
"Begin command sent, but we're already running\n");
- error = -EBUSY;
- goto out;
+ return -EBUSY;
}
userio->running = true;
@@ -220,32 +200,51 @@ static ssize_t userio_char_write(struct file *file, const char __user *buffer,
if (userio->running) {
dev_warn(userio_misc.this_device,
"Can't change port type on an already running userio instance\n");
- error = -EBUSY;
- goto out;
+ return -EBUSY;
}
- userio->serio->id.type = cmd.data;
+ userio->serio->id.type = cmd->data;
break;
case USERIO_CMD_SEND_INTERRUPT:
if (!userio->running) {
dev_warn(userio_misc.this_device,
"The device must be registered before sending interrupts\n");
- error = -ENODEV;
- goto out;
+ return -ENODEV;
}
- serio_interrupt(userio->serio, cmd.data, 0);
+ serio_interrupt(userio->serio, cmd->data, 0);
break;
default:
- error = -EOPNOTSUPP;
- goto out;
+ return -EOPNOTSUPP;
+ }
+
+ return 0;
+}
+
+static ssize_t userio_char_write(struct file *file, const char __user *buffer,
+ size_t count, loff_t *ppos)
+{
+ struct userio_device *userio = file->private_data;
+ struct userio_cmd cmd;
+ int error;
+
+ if (count != sizeof(cmd)) {
+ dev_warn(userio_misc.this_device, "Invalid payload size\n");
+ return -EINVAL;
+ }
+
+ if (copy_from_user(&cmd, buffer, sizeof(cmd)))
+ return -EFAULT;
+
+ scoped_cond_guard(mutex_intr, return -EINTR, &userio->mutex) {
+ error = userio_execute_cmd(userio, &cmd);
+ if (error)
+ return error;
}
-out:
- mutex_unlock(&userio->mutex);
- return error ?: count;
+ return count;
}
static __poll_t userio_char_poll(struct file *file, poll_table *wait)
--
2.46.0.469.g59c65b2a67-goog
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox