* [PATCH v2 3/3] arm64: dts: qcom: sdm845-oneplus: add tri-state-key
From: Gergo Koteles @ 2023-02-09 23:25 UTC (permalink / raw)
To: Dmitry Torokhov, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, linux-arm-msm, devicetree,
Caleb Connolly, Gergo Koteles
The tri-state-key is a sound profile switch found on the OnePlus 6,
Android maps the states to "mute", "vibrate" and "ring". Expose them as
ABS_SND_PROFILE events.
The previous GPIO numbers were wrong. Update them to the correct
ones.
Co-developed-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Caleb Connolly <caleb@connolly.tech>
Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
.../boot/dts/qcom/sdm845-oneplus-common.dtsi | 39 ++++++++++++++++++-
1 file changed, 38 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
index 64638ea94db7..e45d4fdead82 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845-oneplus-common.dtsi
@@ -52,6 +52,43 @@ key-vol-up {
};
};
+ tri-state-key {
+ compatible = "gpio-keys";
+ label = "Tri-state key";
+ pinctrl-names = "default";
+ pinctrl-0 = <&tri_state_key_default>;
+ state-top {
+ label = "Tri-state key top";
+ linux,input-type = <EV_ABS>;
+ linux,code = <ABS_SND_PROFILE>;
+ linux,input-value = <0>;
+ gpios = <&tlmm 126 GPIO_ACTIVE_LOW>;
+ debounce-interval = <50>;
+ linux,can-disable;
+ };
+
+ state-middle {
+ label = "Tri-state key middle";
+ linux,input-type = <EV_ABS>;
+ linux,code = <ABS_SND_PROFILE>;
+ linux,input-value = <1>;
+ gpios = <&tlmm 52 GPIO_ACTIVE_LOW>;
+ debounce-interval = <50>;
+ linux,can-disable;
+
+ };
+
+ state-bottom {
+ label = "Tri-state key bottom";
+ linux,input-type = <EV_ABS>;
+ linux,code = <ABS_SND_PROFILE>;
+ linux,input-value = <2>;
+ gpios = <&tlmm 24 GPIO_ACTIVE_LOW>;
+ debounce-interval = <50>;
+ linux,can-disable;
+ };
+ };
+
reserved-memory {
/*
* The rmtfs_mem needs to be guarded due to "XPU limitations"
@@ -754,7 +791,7 @@ &tlmm {
gpio-reserved-ranges = <0 4>, <81 4>;
tri_state_key_default: tri-state-key-default-state {
- pins = "gpio40", "gpio42", "gpio26";
+ pins = "gpio126", "gpio52", "gpio24";
function = "gpio";
drive-strength = <2>;
bias-disable;
--
2.39.1
^ permalink raw reply related
* [PATCH v2 2/3] Input: add ABS_SND_PROFILE
From: Gergo Koteles @ 2023-02-09 23:25 UTC (permalink / raw)
To: Dmitry Torokhov, Jonathan Corbet, Andy Gross, Bjorn Andersson,
Konrad Dybcio, Rob Herring, Krzysztof Kozlowski, Jiri Kosina,
Benjamin Tissoires
Cc: linux-input, linux-kernel, linux-arm-msm, devicetree,
Caleb Connolly, Gergo Koteles
ABS_SND_PROFILE used to describe the state of a multi-value sound profile
switch. This will be used for the tri-state key on OnePlus phones or other
phones.
Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
Documentation/input/event-codes.rst | 6 ++++++
drivers/hid/hid-debug.c | 1 +
include/uapi/linux/input-event-codes.h | 1 +
3 files changed, 8 insertions(+)
diff --git a/Documentation/input/event-codes.rst b/Documentation/input/event-codes.rst
index b4557462edd7..d43336e64d6a 100644
--- a/Documentation/input/event-codes.rst
+++ b/Documentation/input/event-codes.rst
@@ -241,6 +241,12 @@ A few EV_ABS codes have special meanings:
emitted only when the selected profile changes, indicating the newly
selected profile value.
+* ABS_SND_PROFILE:
+
+ - Used to describe the state of a multi-value sound profile switch.
+ An event is emitted only when the selected profile changes,
+ indicating the newly selected profile value.
+
* ABS_MT_<name>:
- Used to describe multitouch input events. Please see
diff --git a/drivers/hid/hid-debug.c b/drivers/hid/hid-debug.c
index e213bdde543a..76fb2ecbbc51 100644
--- a/drivers/hid/hid-debug.c
+++ b/drivers/hid/hid-debug.c
@@ -1018,6 +1018,7 @@ static const char *absolutes[ABS_CNT] = {
[ABS_DISTANCE] = "Distance", [ABS_TILT_X] = "XTilt",
[ABS_TILT_Y] = "YTilt", [ABS_TOOL_WIDTH] = "ToolWidth",
[ABS_VOLUME] = "Volume", [ABS_PROFILE] = "Profile",
+ [ABS_SND_PROFILE] = "SoundProfile",
[ABS_MISC] = "Misc",
[ABS_MT_TOUCH_MAJOR] = "MTMajor",
[ABS_MT_TOUCH_MINOR] = "MTMinor",
diff --git a/include/uapi/linux/input-event-codes.h b/include/uapi/linux/input-event-codes.h
index 022a520e31fc..e765a8f9a3ae 100644
--- a/include/uapi/linux/input-event-codes.h
+++ b/include/uapi/linux/input-event-codes.h
@@ -866,6 +866,7 @@
#define ABS_VOLUME 0x20
#define ABS_PROFILE 0x21
+#define ABS_SND_PROFILE 0x22
#define ABS_MISC 0x28
--
2.39.1
^ permalink raw reply related
* [PATCH v2 1/3] Input: gpio-keys - add support for linux,input-value dts property
From: Gergo Koteles @ 2023-02-09 23:25 UTC (permalink / raw)
To: Dmitry Torokhov, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, linux-arm-msm, devicetree,
Caleb Connolly, Gergo Koteles
In-Reply-To: <20230209232510.91431-1-soyer@irl.hu>
Allows setting the value of EV_ABS events from dts.
This property is included in the gpio-keys.yaml scheme, but was only
implemented for gpio-keys-polled.
Signed-off-by: Gergo Koteles <soyer@irl.hu>
---
drivers/input/keyboard/gpio_keys.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c
index 5496482a38c1..c42f86ad0766 100644
--- a/drivers/input/keyboard/gpio_keys.c
+++ b/drivers/input/keyboard/gpio_keys.c
@@ -770,6 +770,9 @@ gpio_keys_get_devtree_pdata(struct device *dev)
&button->type))
button->type = EV_KEY;
+ fwnode_property_read_u32(child, "linux,input-value",
+ (u32 *)&button->value);
+
button->wakeup =
fwnode_property_read_bool(child, "wakeup-source") ||
/* legacy name */
--
2.39.1
^ permalink raw reply related
* [PATCH v2 0/3] Add tri-state-key for oneplus6
From: Gergo Koteles @ 2023-02-09 23:25 UTC (permalink / raw)
To: Dmitry Torokhov, Andy Gross, Bjorn Andersson, Konrad Dybcio,
Rob Herring, Krzysztof Kozlowski, Jiri Kosina, Benjamin Tissoires
Cc: linux-input, linux-kernel, linux-arm-msm, devicetree,
Caleb Connolly, Gergo Koteles
V1 -> V2: rebase to qcom/for-next
Gergo Koteles (3):
Input: gpio-keys - add support for linux,input-value dts property
Input: add ABS_SND_PROFILE
arm64: dts: qcom: sdm845-oneplus: add tri-state-key
Documentation/input/event-codes.rst | 6 +++
.../boot/dts/qcom/sdm845-oneplus-common.dtsi | 39 ++++++++++++++++++-
drivers/hid/hid-debug.c | 1 +
drivers/input/keyboard/gpio_keys.c | 3 ++
include/uapi/linux/input-event-codes.h | 1 +
5 files changed, 49 insertions(+), 1 deletion(-)
base-commit: 02ac8d2a011b630481d959298a1cc76ca0717f3e
--
2.39.1
^ permalink raw reply
* Re: [PATCH v3 4/4] HID: cp2112: Devicetree Support
From: Bartosz Golaszewski @ 2023-02-09 21:01 UTC (permalink / raw)
To: Rob Herring
Cc: Danny Kaehn, krzysztof.kozlowski+dt, jikos, benjamin.tissoires,
devicetree, linux-input, ethan.twardy
In-Reply-To: <20230206163912.GA182582-robh@kernel.org>
On Mon, 6 Feb 2023 at 17:39, Rob Herring <robh@kernel.org> wrote:
>
> +Bartosz
>
> On Sun, Feb 05, 2023 at 08:54:50AM -0600, Danny Kaehn wrote:
> > Bind i2c and gpio interfaces to subnodes with names
> > "i2c" and "gpio" if they exist, respectively. This
> > allows the gpio and i2c controllers to be described
> > in DT as usual. Additionally, support configuring the
> > i2c bus speed from the clock-frequency property.
> >
> > Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
> > ---
> > drivers/hid/hid-cp2112.c | 22 ++++++++++++++++++++--
> > 1 file changed, 20 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
> > index 27cadadda7c9..aa634accdfb0 100644
> > --- a/drivers/hid/hid-cp2112.c
> > +++ b/drivers/hid/hid-cp2112.c
> > @@ -1234,6 +1234,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > u8 buf[3];
> > struct cp2112_smbus_config_report config;
> > struct gpio_irq_chip *girq;
> > + struct i2c_timings timings;
> > int ret;
> >
> > dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
> > @@ -1292,6 +1293,10 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > goto err_power_normal;
> > }
> >
> > + dev->adap.dev.of_node = of_get_child_by_name(hdev->dev.of_node, "i2c");
> > + i2c_parse_fw_timings(&dev->adap.dev, &timings, true);
> > +
> > + config.clock_speed = cpu_to_be32(timings.bus_freq_hz);
> > config.retry_time = cpu_to_be16(1);
> >
> > ret = cp2112_hid_output(hdev, (u8 *)&config, sizeof(config),
> > @@ -1300,7 +1305,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > hid_err(hdev, "error setting SMBus config\n");
> > if (ret >= 0)
> > ret = -EIO;
> > - goto err_power_normal;
> > + goto err_free_i2c_of;
> > }
> >
> > hid_set_drvdata(hdev, (void *)dev);
> > @@ -1322,7 +1327,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> >
> > if (ret) {
> > hid_err(hdev, "error registering i2c adapter\n");
> > - goto err_power_normal;
> > + goto err_free_i2c_of;
> > }
> >
> > hid_dbg(hdev, "adapter registered\n");
> > @@ -1336,6 +1341,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > dev->gc.ngpio = 8;
> > dev->gc.can_sleep = 1;
> > dev->gc.parent = &hdev->dev;
> > +#if IS_ENABLED(CONFIG_OF_GPIO)
> > + dev->gc.of_node = of_get_child_by_name(hdev->dev.of_node, "gpio");
> > +#endif
>
> The scarcity of CONFIG_OF_GPIO ifdefs in the tree tells me this is
> wrong. I think you want to use the fwnode pointer instead. GPIO
> maintainers should review this.
>
> Rob
Yep, we're moving away from OF in favor of fwnode - so you'd need to
use device_get_named_child_node() and assign gc.fwnode.
Bart
^ permalink raw reply
* Re: [PATCH v2 2/3] HID: logitech-hidpp: Retry commands when device is busy
From: Bastien Nocera @ 2023-02-09 15:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, linux-kernel, Jiri Kosina, Peter F . Patel-Schneider,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230209145050.54ewn3zhjneium2h@mail.corp.redhat.com>
On Thu, 2023-02-09 at 15:50 +0100, Benjamin Tissoires wrote:
> On Feb 06 2023, Bastien Nocera wrote:
> > Handle the busy error coming from the device or receiver. The
> > documentation says a busy error can be returned when:
> > "
> > Device (or receiver) cannot answer immediately to this request
> > for any reason i.e:
> > - already processing a request from the same or another SW
> > - pipe full
> > "
> >
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >
> > Same as v1
> >
> > drivers/hid/hid-logitech-hidpp.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index 1952d8d3b6b2..9e94026de437 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -295,6 +295,7 @@ static int hidpp_send_message_sync(struct
> > hidpp_device *hidpp,
> > */
> > *response = *message;
> >
> > +retry:
> > ret = __hidpp_send_report(hidpp->hid_dev, message);
> >
> > if (ret) {
> > @@ -321,6 +322,10 @@ static int hidpp_send_message_sync(struct
> > hidpp_device *hidpp,
> > response->report_id ==
> > REPORT_ID_HIDPP_VERY_LONG) &&
> > response->fap.feature_index ==
> > HIDPP20_ERROR) {
> > ret = response->fap.params[1];
> > + if (ret == HIDPP20_ERROR_BUSY) {
> > + dbg_hid("%s:got busy hidpp 2.0 error %02X,
> > retrying\n", __func__, ret);
> > + goto retry;
>
> I must confess, I blocked a little bit there to decide whether or not
> using goto here was OK.
>
> But then I reliazed that there is no way to leave that function if
> the
> device is buggy and constantly sends back ERROR_BUSY. So I am not
> very
> found of the idea of having that got after all.
>
> Would you mind respinning that patch with a bounded loop for the
> retries
> instead of using a goto? I'd like the driver to give up after a few
> retries if the device is not fair.
Done in v3.
^ permalink raw reply
* Re: [PATCH v2 3/3] HID: logitech-hidpp: Add myself to authors
From: Bastien Nocera @ 2023-02-09 15:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, linux-kernel, Jiri Kosina, Peter F . Patel-Schneider,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230209145331.qqryivtatswdxvlf@mail.corp.redhat.com>
On Thu, 2023-02-09 at 15:53 +0100, Benjamin Tissoires wrote:
> On Feb 06 2023, Bastien Nocera wrote:
> > Signed-off-by: Bastien Nocera <hadess@hadess.net>
> > ---
> >
> > Same as v1
> >
> > drivers/hid/hid-logitech-hidpp.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-
> > logitech-hidpp.c
> > index 9e94026de437..03b77ca03081 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -30,6 +30,7 @@
> > MODULE_LICENSE("GPL");
> > MODULE_AUTHOR("Benjamin Tissoires
> > <benjamin.tissoires@gmail.com>");
> > MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@logitech.com>");
> > +MODULE_AUTHOR("Bastien Nocera <hadess@hadess.net>");
>
> Just FTR, I have asked Bastien to set himself as an author given all
> of
> the work he has been doing on this and to have one more person to be
> the
> go-to person from folks having an issue.
>
> And Bastien, would you mind adding yourself to the MAINTAINERS file
> too?
> This way you'll get Cc-ed when people submit patches.
Done in v3 specifically for the hidpp file.
^ permalink raw reply
* [PATCH v3 1/2] HID: logitech-hidpp: Retry commands when device is busy
From: Bastien Nocera @ 2023-02-09 15:49 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
Handle the busy error coming from the device or receiver. The
documentation says a busy error can be returned when:
"
Device (or receiver) cannot answer immediately to this request
for any reason i.e:
- already processing a request from the same or another SW
- pipe full
"
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
Use a bounded for loop instead of a goto, to avoid infinite loops.
drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++--------------
1 file changed, 30 insertions(+), 24 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 1952d8d3b6b2..ff427d7a0fa0 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -283,6 +283,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
struct hidpp_report *response)
{
int ret;
+ int max_retries = 3;
mutex_lock(&hidpp->send_mutex);
@@ -295,34 +296,39 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
*/
*response = *message;
- ret = __hidpp_send_report(hidpp->hid_dev, message);
+ for (; max_retries != 0; max_retries--) {
+ ret = __hidpp_send_report(hidpp->hid_dev, message);
- if (ret) {
- dbg_hid("__hidpp_send_report returned err: %d\n", ret);
- memset(response, 0, sizeof(struct hidpp_report));
- goto exit;
- }
+ if (ret) {
+ dbg_hid("__hidpp_send_report returned err: %d\n", ret);
+ memset(response, 0, sizeof(struct hidpp_report));
+ goto exit;
+ }
- if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
- 5*HZ)) {
- dbg_hid("%s:timeout waiting for response\n", __func__);
- memset(response, 0, sizeof(struct hidpp_report));
- ret = -ETIMEDOUT;
- }
+ if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
+ 5*HZ)) {
+ dbg_hid("%s:timeout waiting for response\n", __func__);
+ memset(response, 0, sizeof(struct hidpp_report));
+ ret = -ETIMEDOUT;
+ }
- if (response->report_id == REPORT_ID_HIDPP_SHORT &&
- response->rap.sub_id == HIDPP_ERROR) {
- ret = response->rap.params[1];
- dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
- goto exit;
- }
+ if (response->report_id == REPORT_ID_HIDPP_SHORT &&
+ response->rap.sub_id == HIDPP_ERROR) {
+ ret = response->rap.params[1];
+ dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
+ goto exit;
+ }
- if ((response->report_id == REPORT_ID_HIDPP_LONG ||
- response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
- response->fap.feature_index == HIDPP20_ERROR) {
- ret = response->fap.params[1];
- dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
- goto exit;
+ if ((response->report_id == REPORT_ID_HIDPP_LONG ||
+ response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
+ response->fap.feature_index == HIDPP20_ERROR) {
+ ret = response->fap.params[1];
+ if (ret != HIDPP20_ERROR_BUSY) {
+ dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
+ goto exit;
+ }
+ dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
+ }
}
exit:
--
2.39.1
^ permalink raw reply related
* [PATCH v3 2/2] HID: logitech-hidpp: Add myself to authors
From: Bastien Nocera @ 2023-02-09 15:49 UTC (permalink / raw)
To: linux-input
Cc: linux-kernel, Jiri Kosina, Benjamin Tissoires,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230209154916.462158-1-hadess@hadess.net>
As discussed with HID maintainer Benjamin Tissoires, add myself to the
authors list and MAINTAINERS file.
Signed-off-by: Bastien Nocera <hadess@hadess.net>
---
Updated MAINTAINERS file too.
MAINTAINERS | 7 +++++++
drivers/hid/hid-logitech-hidpp.c | 1 +
2 files changed, 8 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index fb1471cb5ed3..327679567645 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9217,6 +9217,13 @@ L: linux-input@vger.kernel.org
S: Maintained
F: drivers/hid/hid-logitech-*
+HID++ LOGITECH DRIVERS
+R: Filipe Laíns <lains@riseup.net>
+R: Bastien Nocera <hadess@hadess.net>
+L: linux-input@vger.kernel.org
+S: Maintained
+F: drivers/hid/hid-logitech-hidpp.c
+
HID PLAYSTATION DRIVER
M: Roderick Colenbrander <roderick.colenbrander@sony.com>
L: linux-input@vger.kernel.org
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index ff427d7a0fa0..61bad7a50ec7 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -30,6 +30,7 @@
MODULE_LICENSE("GPL");
MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@logitech.com>");
+MODULE_AUTHOR("Bastien Nocera <hadess@hadess.net>");
static bool disable_tap_to_click;
module_param(disable_tap_to_click, bool, 0644);
--
2.39.1
^ permalink raw reply related
* Re: (subset) [PATCH v2 1/3] HID: logitech-hidpp: Add more debug statements
From: Benjamin Tissoires @ 2023-02-09 15:39 UTC (permalink / raw)
To: linux-input, Bastien Nocera
Cc: linux-kernel, Jiri Kosina, Peter F . Patel-Schneider,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230206221256.129198-1-hadess@hadess.net>
On Mon, 06 Feb 2023 23:12:54 +0100, Bastien Nocera wrote:
> This should help us figure out some hairy problems with some devices.
>
>
Applied to hid/hid.git (for-6.3/logitech), thanks!
[1/3] HID: logitech-hidpp: Add more debug statements
https://git.kernel.org/hid/hid/c/db5167cfaa0a
Cheers,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 1/3] HID: logitech-hidpp: Add more debug statements
From: Benjamin Tissoires @ 2023-02-09 14:54 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-input, linux-kernel, Jiri Kosina, Peter F . Patel-Schneider,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230206221256.129198-1-hadess@hadess.net>
On Feb 06 2023, Bastien Nocera wrote:
> This should help us figure out some hairy problems with some devices.
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
FWIW, I am currently testing this patch and will probably push it today.
Cheers,
Benjamin
> ---
>
> Fixed kernel test bot warning:
> drivers/hid/hid-logitech-hidpp.c: In function 'hidpp_send_fap_command_sync':
> >> drivers/hid/hid-logitech-hidpp.c:343:25: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'unsigned int' [-Wformat=]
> 343 | "Invalid number of parameters passed to command (%d != %ld)\n",
> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> drivers/hid/hid-logitech-hidpp.c | 17 ++++++++++++++---
> 1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index f44ba7be3cc5..1952d8d3b6b2 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -338,8 +338,13 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
> struct hidpp_report *message;
> int ret;
>
> - if (param_count > sizeof(message->fap.params))
> + if (param_count > sizeof(message->fap.params)) {
> + hid_dbg(hidpp->hid_dev,
> + "Invalid number of parameters passed to command (%d != %llu)\n",
> + param_count,
> + (unsigned long long) sizeof(message->fap.params));
> return -EINVAL;
> + }
>
> message = kzalloc(sizeof(struct hidpp_report), GFP_KERNEL);
> if (!message)
> @@ -3440,11 +3445,17 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
> ret = hidpp10_enable_scrolling_acceleration(hidpp);
> multiplier = 8;
> }
> - if (ret)
> + if (ret) {
> + hid_dbg(hidpp->hid_dev,
> + "Could not enable hi-res scrolling: %d\n", ret);
> return ret;
> + }
>
> - if (multiplier == 0)
> + if (multiplier == 0) {
> + hid_dbg(hidpp->hid_dev,
> + "Invalid multiplier 0 from device, setting it to 1\n");
> multiplier = 1;
> + }
>
> hidpp->vertical_wheel_counter.wheel_multiplier = multiplier;
> hid_dbg(hidpp->hid_dev, "wheel multiplier = %d\n", multiplier);
> --
> 2.39.1
>
^ permalink raw reply
* Re: [PATCH v2 3/3] HID: logitech-hidpp: Add myself to authors
From: Benjamin Tissoires @ 2023-02-09 14:53 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-input, linux-kernel, Jiri Kosina, Peter F . Patel-Schneider,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230206221256.129198-3-hadess@hadess.net>
On Feb 06 2023, Bastien Nocera wrote:
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>
> Same as v1
>
> drivers/hid/hid-logitech-hidpp.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 9e94026de437..03b77ca03081 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -30,6 +30,7 @@
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("Benjamin Tissoires <benjamin.tissoires@gmail.com>");
> MODULE_AUTHOR("Nestor Lopez Casado <nlopezcasad@logitech.com>");
> +MODULE_AUTHOR("Bastien Nocera <hadess@hadess.net>");
Just FTR, I have asked Bastien to set himself as an author given all of
the work he has been doing on this and to have one more person to be the
go-to person from folks having an issue.
And Bastien, would you mind adding yourself to the MAINTAINERS file too?
This way you'll get Cc-ed when people submit patches.
Cheers,
Benjamin
>
> static bool disable_tap_to_click;
> module_param(disable_tap_to_click, bool, 0644);
> --
> 2.39.1
>
^ permalink raw reply
* Re: [PATCH v2 2/3] HID: logitech-hidpp: Retry commands when device is busy
From: Benjamin Tissoires @ 2023-02-09 14:50 UTC (permalink / raw)
To: Bastien Nocera
Cc: linux-input, linux-kernel, Jiri Kosina, Peter F . Patel-Schneider,
Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <20230206221256.129198-2-hadess@hadess.net>
On Feb 06 2023, Bastien Nocera wrote:
> Handle the busy error coming from the device or receiver. The
> documentation says a busy error can be returned when:
> "
> Device (or receiver) cannot answer immediately to this request
> for any reason i.e:
> - already processing a request from the same or another SW
> - pipe full
> "
>
> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> ---
>
> Same as v1
>
> drivers/hid/hid-logitech-hidpp.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 1952d8d3b6b2..9e94026de437 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -295,6 +295,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> */
> *response = *message;
>
> +retry:
> ret = __hidpp_send_report(hidpp->hid_dev, message);
>
> if (ret) {
> @@ -321,6 +322,10 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
> response->fap.feature_index == HIDPP20_ERROR) {
> ret = response->fap.params[1];
> + if (ret == HIDPP20_ERROR_BUSY) {
> + dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
> + goto retry;
I must confess, I blocked a little bit there to decide whether or not
using goto here was OK.
But then I reliazed that there is no way to leave that function if the
device is buggy and constantly sends back ERROR_BUSY. So I am not very
found of the idea of having that got after all.
Would you mind respinning that patch with a bounded loop for the retries
instead of using a goto? I'd like the driver to give up after a few
retries if the device is not fair.
Cheers,
Benjamin
> + }
> dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> goto exit;
> }
> --
> 2.39.1
>
^ permalink raw reply
* Re: [PATCH] Add support for Logitech G923 Xbox Edition steering wheel in hid-logitech-hidpp. We get the same level of features as the regular G920.
From: Benjamin Tissoires @ 2023-02-09 14:34 UTC (permalink / raw)
To: linux-input, linux-kernel, Walt Holman
In-Reply-To: <20230207195051.16373-1-waltholman09@gmail.com>
On Tue, 07 Feb 2023 13:50:52 -0600, Walt Holman wrote:
>
Applied to hid/hid.git (for-6.3/logitech), thanks!
[1/1] Add support for Logitech G923 Xbox Edition steering wheel in hid-logitech-hidpp. We get the same level of features as the regular G920.
https://git.kernel.org/hid/hid/c/f1fc9fbc8394
Cheers,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply
* Re: [PATCH] Add support for Logitech G923 Xbox Edition steering wheel in hid-logitech-hidpp. We get the same level of features as the regular G920.
From: Walt Holman @ 2023-02-09 14:29 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <20230209141022.oql7n52igjybh3km@mail.corp.redhat.com>
On 2/9/23 08:10, Benjamin Tissoires wrote:
> Hi Walt, we are almost there. I have applied the following to your patch
> (and will push upstream as soon as the CI says it's fine):
>
> * subject must start with the subsystem: "HID: " in our case
> * in your commit description, you should add a blanck line between the
> title of the commit and the description. Otherwise, git-format-patch
> considers everything to be the title, which is a little bit long :)
> * the Signed-of-by line should be added in the commit description, so
> right here, before the first '---' so that it gets included in the
> commit itself
> * for future submissions, please try to keep them numbered. Here, it
> should have been '[PATCH v2] HID: ...' and below the first '---' add
> what changed between v1 and v2.
>
> Again, I have fixed everything, so no need to send another version :)
>
> Cheers,
> Benjamin
>
Thanks Benjamin! I (obviously) don't submit often, but I'll try to keep these things in mind going forward. Thanks for helping to get this done!
-Walt
> On Feb 07 2023, Walt Holman wrote:
>> ---
>> drivers/hid/hid-ids.h | 1 +
>> drivers/hid/hid-logitech-hidpp.c | 3 +++
>> 2 files changed, 4 insertions(+)
>>
>> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
>> index 0f8c11842a3a..de1f385b0ecc 100644
>> --- a/drivers/hid/hid-ids.h
>> +++ b/drivers/hid/hid-ids.h
>> @@ -819,6 +819,7 @@
>> #define USB_DEVICE_ID_LOGITECH_G510_USB_AUDIO 0xc22e
>> #define USB_DEVICE_ID_LOGITECH_G29_WHEEL 0xc24f
>> #define USB_DEVICE_ID_LOGITECH_G920_WHEEL 0xc262
>> +#define USB_DEVICE_ID_LOGITECH_G923_XBOX_WHEEL 0xc26e
>> #define USB_DEVICE_ID_LOGITECH_WINGMAN_F3D 0xc283
>> #define USB_DEVICE_ID_LOGITECH_FORCE3D_PRO 0xc286
>> #define USB_DEVICE_ID_LOGITECH_FLIGHT_SYSTEM_G940 0xc287
>> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
>> index abf2c95e4d0b..4dc833c8a44c 100644
>> --- a/drivers/hid/hid-logitech-hidpp.c
>> +++ b/drivers/hid/hid-logitech-hidpp.c
>> @@ -4347,6 +4347,9 @@ static const struct hid_device_id hidpp_devices[] = {
>> { /* Logitech G920 Wheel over USB */
>> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
>> .driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
>> + { /* Logitech G923 Wheel (Xbox version) over USB */
>> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G923_XBOX_WHEEL),
>> + .driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS },
>> { /* Logitech G Pro Gaming Mouse over USB */
>> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
>>
>> --
>> 2.37.2
>>
>> --signed-of-by: Walt Holman (waltholman09@gmail.com)
>>
>
^ permalink raw reply
* Re: [PATCH] Add support for Logitech G923 Xbox Edition steering wheel in hid-logitech-hidpp. We get the same level of features as the regular G920.
From: Benjamin Tissoires @ 2023-02-09 14:10 UTC (permalink / raw)
To: Walt Holman; +Cc: linux-input, linux-kernel
In-Reply-To: <20230207195051.16373-1-waltholman09@gmail.com>
Hi Walt, we are almost there. I have applied the following to your patch
(and will push upstream as soon as the CI says it's fine):
* subject must start with the subsystem: "HID: " in our case
* in your commit description, you should add a blanck line between the
title of the commit and the description. Otherwise, git-format-patch
considers everything to be the title, which is a little bit long :)
* the Signed-of-by line should be added in the commit description, so
right here, before the first '---' so that it gets included in the
commit itself
* for future submissions, please try to keep them numbered. Here, it
should have been '[PATCH v2] HID: ...' and below the first '---' add
what changed between v1 and v2.
Again, I have fixed everything, so no need to send another version :)
Cheers,
Benjamin
On Feb 07 2023, Walt Holman wrote:
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-logitech-hidpp.c | 3 +++
> 2 files changed, 4 insertions(+)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index 0f8c11842a3a..de1f385b0ecc 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -819,6 +819,7 @@
> #define USB_DEVICE_ID_LOGITECH_G510_USB_AUDIO 0xc22e
> #define USB_DEVICE_ID_LOGITECH_G29_WHEEL 0xc24f
> #define USB_DEVICE_ID_LOGITECH_G920_WHEEL 0xc262
> +#define USB_DEVICE_ID_LOGITECH_G923_XBOX_WHEEL 0xc26e
> #define USB_DEVICE_ID_LOGITECH_WINGMAN_F3D 0xc283
> #define USB_DEVICE_ID_LOGITECH_FORCE3D_PRO 0xc286
> #define USB_DEVICE_ID_LOGITECH_FLIGHT_SYSTEM_G940 0xc287
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index abf2c95e4d0b..4dc833c8a44c 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -4347,6 +4347,9 @@ static const struct hid_device_id hidpp_devices[] = {
> { /* Logitech G920 Wheel over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
> .driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
> + { /* Logitech G923 Wheel (Xbox version) over USB */
> + HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G923_XBOX_WHEEL),
> + .driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS },
> { /* Logitech G Pro Gaming Mouse over USB */
> HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
>
> --
> 2.37.2
>
> --signed-of-by: Walt Holman (waltholman09@gmail.com)
>
^ permalink raw reply
* Re: [PATCH v2] HID: retain initial quirks set up when creating HID devices
From: Benjamin Tissoires @ 2023-02-09 14:00 UTC (permalink / raw)
To: Jiri Kosina, Dmitry Torokhov
Cc: Guenter Roeck, Allen Ballway, Hans de Goede, Alistair Francis,
linux-input, linux-kernel
In-Reply-To: <Y+LYwu3Zs13hdVDy@google.com>
On Tue, 07 Feb 2023 15:03:30 -0800, Dmitry Torokhov wrote:
> In certain circumstances, such as when creating I2C-connected HID
> devices, we want to pass and retain some quirks (axis inversion, etc).
> The source of such quirks may be device tree, or DMI data, or something
> else not readily available to the HID core itself and therefore cannot
> be reconstructed easily. To allow this, introduce "initial_quirks" field
> in hid_device structure and use it when determining the final set of
> quirks.
>
> [...]
Applied to hid/hid.git (for-6.3/multitouch), thanks!
[1/1] HID: retain initial quirks set up when creating HID devices
https://git.kernel.org/hid/hid/c/03a86105556e
Cheers,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply
* Re: [PATCH] input/misc: hp_sdc_rtc: mark an unused function as __maybe_unused
From: Geert Uytterhoeven @ 2023-02-09 13:56 UTC (permalink / raw)
To: Randy Dunlap
Cc: linux-kernel, David Howells, Helge Deller, linux-m68k,
James E.J. Bottomley, linux-parisc, Al Viro, Dmitry Torokhov,
linux-input
In-Reply-To: <20230209010125.23690-1-rdunlap@infradead.org>
On Thu, Feb 9, 2023 at 2:04 AM Randy Dunlap <rdunlap@infradead.org> wrote:
> When CONFIG_PROC_FS is not set, one procfs-related function is not
> used, causing a build error or warning.
> Mark this function as __maybe_unused to quieten the build.
>
> ../drivers/input/misc/hp_sdc_rtc.c:268:12: warning: 'hp_sdc_rtc_proc_show' defined but not used [-Wunused-function]
> 268 | static int hp_sdc_rtc_proc_show(struct seq_file *m, void *v)
> | ^~~~~~~~~~~~~~~~~~~~
>
> Fixes: c18bd9a1ff47 ("hp_sdc_rtc: Don't use create_proc_read_entry()")
> Signed-off-by: Randy Dunlap <rdunlap@infradead.org>
Reviewed-by: Geert Uytterhoeven <geert@linux-m68k.org>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: (subset) [PATCH 0/7] arm: qcom: Fix touchscreen voltage for sc7280-herobrine boards
From: Benjamin Tissoires @ 2023-02-09 13:50 UTC (permalink / raw)
To: Bjorn Andersson, Rob Herring, Krzysztof Kozlowski, Jiri Kosina,
Douglas Anderson
Cc: linux-input, linux-arm-msm, Dmitry Torokhov, devicetree,
Stephen Kitt, Konrad Dybcio, Andy Gross, linux-kernel
In-Reply-To: <20230207024816.525938-1-dianders@chromium.org>
On Mon, 06 Feb 2023 18:48:09 -0800, Douglas Anderson wrote:
> Trying to figure out how to talk to the touchscreen properly on
> sc7280-herobrine boards was a long and difficult process. Many
> Engineering hours were spent deciding how exactly one should talk over
> i2c to a peripheral. In the end, a solution has been found and this
> patch series attempts to implement it in a way that will work for all
> herobrine-based boards.
>
> [...]
Applied to hid/hid.git (for-6.3/i2c-hid), thanks!
[4/7] HID: i2c-hid: goodix: Stop tying the reset line to the regulator
https://git.kernel.org/hid/hid/c/557e05fa9fdd
[5/7] dt-bindings: HID: i2c-hid: goodix: Add mainboard-vddio-supply
https://git.kernel.org/hid/hid/c/1d18c1f3b7d9
[6/7] HID: i2c-hid: goodix: Add mainboard-vddio-supply
https://git.kernel.org/hid/hid/c/eb16f59e8e58
Cheers,
--
Benjamin Tissoires <benjamin.tissoires@redhat.com>
^ permalink raw reply
* Re: [PATCH v2 3/5] HID: dualsense_remove: manually unregister leds
From: Pietro Borrello @ 2023-02-09 12:50 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Hanno Zulla, Pavel Machek, Lee Jones,
Roderick Colenbrander, Sven Eckelmann, linux-leds,
Cristiano Giuffrida, Bos, H.J., Jakob Koschel, linux-input,
linux-kernel, Jiri Kosina, Roderick Colenbrander
In-Reply-To: <20230209085940.53mrapggf6nagkdh@mail.corp.redhat.com>
On Thu, 9 Feb 2023 at 09:59, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> On Jan 31 2023, Pietro Borrello wrote:
> > Unregister the LED controllers before device removal, to prevent
> > unnecessary runs of dualsense_player_led_set_brightness().
> >
> > Fixes: 8c0ab553b072 ("HID: playstation: expose DualSense player LEDs through LED class.")
> > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> >
> > ---
> >
> > Contrary to the other patches in this series, failing to unregister
> > the led controller does not results into a use-after-free thanks
> > to the output_worker_initialized variable and the spinlock checks.
>
> And so we don't need that patch (nor for hid-sony.c) because we have a
> guard against scheduling a worker job when the device is being removed.
>
> So please drop 3,4,5 from this series, they are just making the code
> worse.
Sure.
I kept them only due to the Roderick Colenbrander's comment, but I'm happy
to remove them.
For reference:
> [...] I don't mind the change as it
> prevents the work scheduling functions to get called unnecessarily.
Link: https://lore.kernel.org/lkml/CAEc3jaCEKfqEJSV4=6GRj1Ry97xH+HwVSeEOZReNwkt=rLNvNQ@mail.gmail.com/
Thanks,
Pietro
^ permalink raw reply
* Re: [PATCH v2 1/5] HID: bigben_remove: manually unregister leds
From: Pietro Borrello @ 2023-02-09 12:45 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Hanno Zulla, Pavel Machek, Lee Jones,
Roderick Colenbrander, Sven Eckelmann, linux-leds,
Cristiano Giuffrida, Bos, H.J., Jakob Koschel, linux-input,
linux-kernel, Jiri Kosina, Roderick Colenbrander
In-Reply-To: <20230209085546.ohsonlwgqb7yd3d4@mail.corp.redhat.com>
On Thu, 9 Feb 2023 at 09:55, Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Pietro,
>
> On Jan 31 2023, Pietro Borrello wrote:
> > Unregister the LED controllers before device removal, as
> > bigben_set_led() may schedule bigben->worker after the structure has
> > been freed, causing a use-after-free.
> >
> > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal")
> > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it>
> > ---
> > drivers/hid/hid-bigbenff.c | 5 +++++
> > 1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c
> > index e8b16665860d..d3201b755595 100644
> > --- a/drivers/hid/hid-bigbenff.c
> > +++ b/drivers/hid/hid-bigbenff.c
> > @@ -306,9 +306,14 @@ static enum led_brightness bigben_get_led(struct led_classdev *led)
> >
> > static void bigben_remove(struct hid_device *hid)
> > {
> > + int n;
> > struct bigben_device *bigben = hid_get_drvdata(hid);
> >
> > bigben->removed = true;
> > + for (n = 0; n < NUM_LEDS; n++) {
> > + if (bigben->leds[n])
> > + devm_led_classdev_unregister(&hid->dev, bigben->leds[n]);
> > + }
> > cancel_work_sync(&bigben->worker);
>
> I don't think this is the correct fix. It would seem that we are
> suddenly making the assumption that the devm mechanism would do things
> in the wrong order, when the devm_led_classdev_unregister() should be
> called *before* the devm_free() of the struct bigben_device.
>
> However, you can trigger a bug, and thus we can analyse a little bit
> further what is happening:
>
> * user calls a function on the LED
> * bigben_set_led() is called
> * .remove() is being called at roughly the same time:
> - bigben->removed is set to true
> - cancel_work_sync() is called
> * at that point, bigben_set_led() can not crash because
> led_classdev_unregister() flushes all of its workers, and thus
> prevents the call for dev_kfree(struct bigben_device)
> * but now bigben_set_led() calls schedule_work()
> * led_classdev_unregister() is now done and devm_kfree() is called for
> struct bigben_device
> * now the led worker kicks in, and tries to access struct bigben_device
> and derefences it to get the value of bigben->removed (and
> bigben->report), which crashes.
>
> So without your patch, the problem seems to be that we call a
> schedule_work *after* we set bigben->removed to true and we call
> cancel_work_sync().
Yes, this matches my intuition of what is happening here.
Thank you for the extensive description.
>
> And if you look at the hid-playstation driver, you'll see that the
> schedule_work() call is encapsulated in a spinlock and a check to
> ds->output_worker_initialized.
>
> And this is why you can not reproduce on the hid-playstation driver,
> because it is guarded against scheduling a worker when the driver is
> being removed.
>
> I think I prefer a lot more the playstation solution: having to manually
> call a devm_release_free always feels wrong in a normal path. And also
> by doing so, you might paper another problem that might happen on an
> error path in probe for instance. Also, this means that the pattern you
> saw is specific to some drivers, not all depending on how they make use
> of workers.
>
Yes, I agree this would be much cleaner.
> Would you mind respinning that series with those comments?
Sure, I'll work on that!
Best regards,
Pietro
^ permalink raw reply
* Re: [PATCH v4 03/18] hte: tegra-194: Use proper includes
From: Dipen Patel @ 2023-02-09 11:29 UTC (permalink / raw)
To: Andy Shevchenko, Arnd Bergmann, Linus Walleij,
Bartosz Golaszewski, Tony Lindgren, Thomas Gleixner,
Krzysztof Kozlowski, Geert Uytterhoeven, Michael Ellerman,
Christophe Leroy, Vincenzo Palazzo, Devarsh Thakkar,
Michael Walle, Dmitry Torokhov, Thierry Reding, Stefan Schmidt,
Miquel Raynal, Lee Jones, linux-gpio, linux-doc, linux-kernel,
linux-doc-tw-discuss, linux-arm-kernel, linux-omap,
linux-samsung-soc, linux-m68k, linuxppc-dev, linux-sh, linux-acpi,
linux-tegra, linux-input, linux-media, linux-wpan, netdev,
linux-wireless, brcm80211-dev-list.pdl, SHA-cyfmac-dev-list,
linux-arch, devicetree
Cc: Bartosz Golaszewski, Jonathan Corbet, Alex Shi, Yanteng Si,
Hu Haowen, Russell King, Aaro Koskinen, Janusz Krzysztofik,
Andrew Lunn, Sebastian Hesselbarth, Gregory Clement, Alim Akhtar,
Nicholas Piggin, Yoshinori Sato, Rich Felker, Mun Yew Tham,
Keerthy, Mika Westerberg, Thierry Reding, Jonathan Hunter,
Mauro Carvalho Chehab, Alexander Aring, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Arend van Spriel,
Franky Lin, Hante Meuleman, Kalle Valo, Qiang Zhao, Li Yang,
Rob Herring, Frank Rowand
In-Reply-To: <20230208173343.37582-4-andriy.shevchenko@linux.intel.com>
On 2/8/23 11:03 PM, Andy Shevchenko wrote:
> From: Linus Walleij <linus.walleij@linaro.org>
>
> The test driver uses the gpiod consumer API so include the right
> <linux/gpio/consumer.h> header. This may cause a problem with
> struct of_device_id being implcitly pulled in by the legacy
> header <linux/gpio.h> so include <linux/mod_devicetable.h>
> explicitly as well.
>
> While at it, drop explicit moduleparam.h (it's included with module.h)
> and sort the headers.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/hte/hte-tegra194-test.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hte/hte-tegra194-test.c b/drivers/hte/hte-tegra194-test.c
> index 5d776a185bd6..358d4a10c6a1 100644
> --- a/drivers/hte/hte-tegra194-test.c
> +++ b/drivers/hte/hte-tegra194-test.c
> @@ -6,14 +6,14 @@
> */
>
> #include <linux/err.h>
> -#include <linux/module.h>
> -#include <linux/moduleparam.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/hte.h>
> #include <linux/interrupt.h>
> -#include <linux/gpio.h>
> -#include <linux/timer.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> #include <linux/platform_device.h>
> +#include <linux/timer.h>
> #include <linux/workqueue.h>
> -#include <linux/hte.h>
>
> /*
> * This sample HTE GPIO test driver demonstrates HTE API usage by enabling
Acked-by: Dipen Patel <dipenp@nvidia.com>
^ permalink raw reply
* [PATCH AUTOSEL 5.10 13/13] platform/x86: touchscreen_dmi: Add Chuwi Vi8 (CWI501) DMI match
From: Sasha Levin @ 2023-02-09 11:18 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Hans de Goede, Sasha Levin, markgross, linux-input,
platform-driver-x86
In-Reply-To: <20230209111833.1892896-1-sashal@kernel.org>
From: Hans de Goede <hdegoede@redhat.com>
[ Upstream commit eecf2acd4a580e9364e5087daf0effca60a240b7 ]
Add a DMI match for the CWI501 version of the Chuwi Vi8 tablet,
pointing to the same chuwi_vi8_data as the existing CWI506 version
DMI match.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20230202103413.331459-1-hdegoede@redhat.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/platform/x86/touchscreen_dmi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index bc26acace2c30..b96fbc8dba09d 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -1030,6 +1030,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
DMI_MATCH(DMI_BIOS_DATE, "05/07/2016"),
},
},
+ {
+ /* Chuwi Vi8 (CWI501) */
+ .driver_data = (void *)&chuwi_vi8_data,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Insyde"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "i86"),
+ DMI_MATCH(DMI_BIOS_VERSION, "CHUWI.W86JLBNR01"),
+ },
+ },
{
/* Chuwi Vi8 (CWI506) */
.driver_data = (void *)&chuwi_vi8_data,
--
2.39.0
^ permalink raw reply related
* [PATCH AUTOSEL 5.15 17/17] platform/x86: touchscreen_dmi: Add Chuwi Vi8 (CWI501) DMI match
From: Sasha Levin @ 2023-02-09 11:17 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Hans de Goede, Sasha Levin, markgross, linux-input,
platform-driver-x86
In-Reply-To: <20230209111731.1892569-1-sashal@kernel.org>
From: Hans de Goede <hdegoede@redhat.com>
[ Upstream commit eecf2acd4a580e9364e5087daf0effca60a240b7 ]
Add a DMI match for the CWI501 version of the Chuwi Vi8 tablet,
pointing to the same chuwi_vi8_data as the existing CWI506 version
DMI match.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20230202103413.331459-1-hdegoede@redhat.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/platform/x86/touchscreen_dmi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index 93671037fd598..69ba2c5182610 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -1073,6 +1073,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
DMI_MATCH(DMI_BIOS_DATE, "05/07/2016"),
},
},
+ {
+ /* Chuwi Vi8 (CWI501) */
+ .driver_data = (void *)&chuwi_vi8_data,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Insyde"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "i86"),
+ DMI_MATCH(DMI_BIOS_VERSION, "CHUWI.W86JLBNR01"),
+ },
+ },
{
/* Chuwi Vi8 (CWI506) */
.driver_data = (void *)&chuwi_vi8_data,
--
2.39.0
^ permalink raw reply related
* [PATCH AUTOSEL 6.1 36/38] platform/x86: touchscreen_dmi: Add Chuwi Vi8 (CWI501) DMI match
From: Sasha Levin @ 2023-02-09 11:14 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Hans de Goede, Sasha Levin, markgross, linux-input,
platform-driver-x86
In-Reply-To: <20230209111459.1891941-1-sashal@kernel.org>
From: Hans de Goede <hdegoede@redhat.com>
[ Upstream commit eecf2acd4a580e9364e5087daf0effca60a240b7 ]
Add a DMI match for the CWI501 version of the Chuwi Vi8 tablet,
pointing to the same chuwi_vi8_data as the existing CWI506 version
DMI match.
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Link: https://lore.kernel.org/r/20230202103413.331459-1-hdegoede@redhat.com
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/platform/x86/touchscreen_dmi.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/platform/x86/touchscreen_dmi.c b/drivers/platform/x86/touchscreen_dmi.c
index f00995390fdfe..13802a3c3591d 100644
--- a/drivers/platform/x86/touchscreen_dmi.c
+++ b/drivers/platform/x86/touchscreen_dmi.c
@@ -1097,6 +1097,15 @@ const struct dmi_system_id touchscreen_dmi_table[] = {
DMI_MATCH(DMI_BIOS_DATE, "05/07/2016"),
},
},
+ {
+ /* Chuwi Vi8 (CWI501) */
+ .driver_data = (void *)&chuwi_vi8_data,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "Insyde"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "i86"),
+ DMI_MATCH(DMI_BIOS_VERSION, "CHUWI.W86JLBNR01"),
+ },
+ },
{
/* Chuwi Vi8 (CWI506) */
.driver_data = (void *)&chuwi_vi8_data,
--
2.39.0
^ 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