* Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Krzysztof Kozlowski @ 2023-03-19 14:09 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <57b22a4f-3b7e-d62d-d9a7-f9f4e052c253@gmail.com>
On 17/03/2023 11:39, Jan Jasper de Kroon wrote:
>
> Op 16-03-2023 om 20:25 schreef Krzysztof Kozlowski:
>> On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
>>> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
>>> device tree binding. When set to true, the touchscreen controller will
>>> be held in reset mode during system suspend, reducing power consumption.
>>> If not present, the property defaults to false.
>>>
>>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
>> Don't attach new patchsets to some other threads. It messes with our
>> tools and reading/reviewing process.
> Thank you for bringing this to my attention. I apologize for any
> inconvenience caused by attaching the patchset to the wrong threads. As a
> new user of LKML, I'm still learning the appropriate protocol for
> submitting patches. Going forward, I will ensure to attach patchsets to
> the correct threads.
>>> ---
>>> Changes from v2 to v3:
>>> - Used imperative mood instead of "This patch adds".
>>> - Dropped "I am submitting this patch to..." as it is redundant.
>>> - Removed the paragraph related to the related patch sent to the
>>> linux-input mailing list as it is not necessary.
>>> - Renamed the hold-in-reset-in-suspend function to
>>> goodix-hold-in-reset to prevent potential naming conflicts with other
>>> functions in the codebase. No functional changes were made.
>>>
>>> Changes from v1 to v2:
>>> - Updated subject prefix to match subsystem.
>>> - Added more detailed description of the change.
>>> - Fixed formatting issues in commit message.
>>> .../devicetree/bindings/input/touchscreen/goodix.yaml | 8 ++++++++
>>> 1 file changed, 8 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> index 3d016b87c8df..197f8db9acc2 100644
>>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>> @@ -56,6 +56,13 @@ properties:
>>> touchscreen-size-y: true
>>> touchscreen-swapped-x-y: true
>>>
>>> + goodix-hold-in-reset:
>> That's not a vendor prefix... missing coma.
> Thank you for pointing out the mistake in the vendor prefix. I appreciate
> your feedback and apologize for any inconvenience caused. I wasn't aware
> of the correct vendor prefix style, but I've learned from developer Hans
> de Goede that it should be "goodix,hold-in-reset." I will make sure to
> correct this in my local branch and ensure that it is applied correctly in
> the future. Thanks again for bringing this to my attention.
>>> + description: |
>>> + When set to true, the touchscreen controller will be held in reset mode
>>> + during system suspend. This can help reduce power consumption, but may
>>> + cause the touchscreen to take longer to resume when the system is woken
>>> + up from suspend.
>> Anyway, my concerns were not answered, so to be clear:
>>
>> NAK till you answer them. Do not send new versions without answering
>> existing concerns and discussion.
> Thank you again for reviewing my patchset and providing feedback. I
> appreciate your time and effort in ensuring the quality and suitability
> of the DeviceTree.
>
> Regarding the concerns you raised about the proposed feature, I would
> like to address them directly. You mentioned that the property does not
> look suitable for Devicetree because it describes system policies that are
> not within the scope of Devicetree. While I understand your point, I
> believe this property is appropriate for Devicetree for the following
> reasons:
>
> - The property directly relates to the hardware configuration of the
> device, specifically the touchscreen controller, and is not a software
> policy.
Keeping device in reset state is not hardware configuration but driver
behavior. You did not Cc us on all patches for some reason, so it's
difficult to judge what exactly your driver is doing.
>
> - The property is required for proper system operation and is not optional
> in specific device use cases. To be more specific in the case of the
> PinePhone Original and Pro. The original commit message of the driver
> implementation in driver/input/touchscreen contained the following:
> "It consumes quite a bit of power (~40mW) during system sleep, and more
> when the screen is touched."
> Because the phone is usually kept in your pocket, so prone to a lot of
> screen touches, this is highly undesired behavior for the touchscreen in
> this case. This in my opinion makes it a mandatory property in this
> situation.
Why then the touchscree should not be kept in reset for other devices?
IOW, this should be always used. If you now say "I prefer to keep or not
keep it in reset for my device" - it's a policy.
>
> - The property is not a user-facing configuration option and is not meant
> to be changed by the end-user.
Does not matter.
>
> - The property, although in separate device specific kernel, and still
> called 'poweroff-in-suspend' is already in use on specific devices,
> including the PinePhone Original and PinePhone Pro.
I could not find such property in the kernel.
>
> However, I understand your concern that Devicetree should not be used for
> policies. To address this concern, I would like to propose the following
> changes to the property description:
> 1. Remove the sentence about reducing power consumption, as this could be
> considered a policy.
> 2. Emphasize that the property is a required hardware configuration and
> not an optional feature on certain devices.
> 3. Recommend that any changes to the property value should only be made by
> experienced system administrators and not end-users.
Please answer - why this should not be enabled always.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Jan Jasper de Kroon @ 2023-03-19 16:38 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <947f9c1f-fd48-cb18-e495-7e275f1792b9@linaro.org>
Op 19-03-2023 om 15:09 schreef Krzysztof Kozlowski:
> On 17/03/2023 11:39, Jan Jasper de Kroon wrote:
>> Op 16-03-2023 om 20:25 schreef Krzysztof Kozlowski:
>>> On 16/03/2023 16:29, Jan Jasper de Kroon wrote:
>>>> Add an optional 'goodix-hold-in-reset', to the Goodix touchscreen
>>>> device tree binding. When set to true, the touchscreen controller will
>>>> be held in reset mode during system suspend, reducing power consumption.
>>>> If not present, the property defaults to false.
>>>>
>>>> Signed-off-by: Jan Jasper de Kroon <jajadekroon@gmail.com>
>>> Don't attach new patchsets to some other threads. It messes with our
>>> tools and reading/reviewing process.
>> Thank you for bringing this to my attention. I apologize for any
>> inconvenience caused by attaching the patchset to the wrong threads. As a
>> new user of LKML, I'm still learning the appropriate protocol for
>> submitting patches. Going forward, I will ensure to attach patchsets to
>> the correct threads.
>>>> ---
>>>> Changes from v2 to v3:
>>>> - Used imperative mood instead of "This patch adds".
>>>> - Dropped "I am submitting this patch to..." as it is redundant.
>>>> - Removed the paragraph related to the related patch sent to the
>>>> linux-input mailing list as it is not necessary.
>>>> - Renamed the hold-in-reset-in-suspend function to
>>>> goodix-hold-in-reset to prevent potential naming conflicts with other
>>>> functions in the codebase. No functional changes were made.
>>>>
>>>> Changes from v1 to v2:
>>>> - Updated subject prefix to match subsystem.
>>>> - Added more detailed description of the change.
>>>> - Fixed formatting issues in commit message.
>>>> .../devicetree/bindings/input/touchscreen/goodix.yaml | 8 ++++++++
>>>> 1 file changed, 8 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> index 3d016b87c8df..197f8db9acc2 100644
>>>> --- a/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> +++ b/Documentation/devicetree/bindings/input/touchscreen/goodix.yaml
>>>> @@ -56,6 +56,13 @@ properties:
>>>> touchscreen-size-y: true
>>>> touchscreen-swapped-x-y: true
>>>>
>>>> + goodix-hold-in-reset:
>>> That's not a vendor prefix... missing coma.
>> Thank you for pointing out the mistake in the vendor prefix. I appreciate
>> your feedback and apologize for any inconvenience caused. I wasn't aware
>> of the correct vendor prefix style, but I've learned from developer Hans
>> de Goede that it should be "goodix,hold-in-reset." I will make sure to
>> correct this in my local branch and ensure that it is applied correctly in
>> the future. Thanks again for bringing this to my attention.
>>>> + description: |
>>>> + When set to true, the touchscreen controller will be held in reset mode
>>>> + during system suspend. This can help reduce power consumption, but may
>>>> + cause the touchscreen to take longer to resume when the system is woken
>>>> + up from suspend.
>>> Anyway, my concerns were not answered, so to be clear:
>>>
>>> NAK till you answer them. Do not send new versions without answering
>>> existing concerns and discussion.
>> Thank you again for reviewing my patchset and providing feedback. I
>> appreciate your time and effort in ensuring the quality and suitability
>> of the DeviceTree.
>>
>> Regarding the concerns you raised about the proposed feature, I would
>> like to address them directly. You mentioned that the property does not
>> look suitable for Devicetree because it describes system policies that are
>> not within the scope of Devicetree. While I understand your point, I
>> believe this property is appropriate for Devicetree for the following
>> reasons:
>>
>> - The property directly relates to the hardware configuration of the
>> device, specifically the touchscreen controller, and is not a software
>> policy.
> Keeping device in reset state is not hardware configuration but driver
> behavior. You did not Cc us on all patches for some reason, so it's
> difficult to judge what exactly your driver is doing.
Thank you for your response. I apologize for not including all the
necessary information in my previous messages. Like you are already aware,
the DT patch is only one part of the solution, and the driver part has
been submitted to the linux-input mailing list. Here is the link to the
latest patch submission:
https://lore.kernel.org/all/20230316152159.66922-1-jajadekroon@gmail.com/
I understand that it may have been difficult to judge what the driver is
doing without the complete context. The original patch consists of two
'out-of-tree' commits, one that attempts to power off the touchscreen device
controller completely, including the regulators:
https://github.com/megous/linux/commit/a38e3e2900c69f5b9961aca8e003c21950453857
and another that reverts disabling the regulators:
https://github.com/megous/linux/commit/cafc7adf456c03eb4564c2ba750a5345b9c6ceed
The reason for this is that different peripherals are attached to the same
power supply in the case of the PinePhone Original and PinePhone Pro.
I hope this clarifies part of the situation. If you have any further
questions or concerns, please do not hesitate to let me know.
>
>> - The property is required for proper system operation and is not optional
>> in specific device use cases. To be more specific in the case of the
>> PinePhone Original and Pro. The original commit message of the driver
>> implementation in driver/input/touchscreen contained the following:
>> "It consumes quite a bit of power (~40mW) during system sleep, and more
>> when the screen is touched."
>> Because the phone is usually kept in your pocket, so prone to a lot of
>> screen touches, this is highly undesired behavior for the touchscreen in
>> this case. This in my opinion makes it a mandatory property in this
>> situation.
> Why then the touchscree should not be kept in reset for other devices?
> IOW, this should be always used. If you now say "I prefer to keep or not
> keep it in reset for my device" - it's a policy.
Thank you for your question. While it's true that keeping the touchscreen
in reset state during system sleep can reduce power consumption for other
devices, the decision to use this property should be based on the specific
use case and hardware configuration of each device. In the case of the
PinePhone Original and Pro, the touchscreen's power consumption during
system sleep is significant, and the device is often kept in a pocket, so
accidental screen touches can occur frequently, leading to further power
drain. As such, keeping the touchscreen in reset state is necessary for
proper system operation in these specific devices. However, for other
devices with different hardware configurations and use cases, the decision
to use this property should be based on a thorough assessment of the power
consumption and potential impact on system behavior.
>
>
>> - The property is not a user-facing configuration option and is not meant
>> to be changed by the end-user.
> Does not matter.
>
>> - The property, although in separate device specific kernel, and still
>> called 'poweroff-in-suspend' is already in use on specific devices,
>> including the PinePhone Original and PinePhone Pro.
> I could not find such property in the kernel.
I apologize for the confusion, but the current mainline kernel doesn't
include this property. As I stated to support the PinePhone Original and
PinePhone Pro, the community makes use of a forked mainline kernel, with
a lot of out-of-tree patches/commits, mainly maintained by developer
Ondrej Jirman. For the PinePhone Original, the DeviceTree configuration
in the PinePhone DTS gets set in the following commit:
https://github.com/megous/linux/commit/74fc0a5f0527afdccb67ce3be690f0ae18c8eca6
For the PinePhone Pro it is set in the following commit, at line 466:
https://github.com/megous/linux/commit/471c5f33ba74c3d498ccc1eb69c098623b193926#
The property here is still called "poweroff-in-suspend".
>
>> However, I understand your concern that Devicetree should not be used for
>> policies. To address this concern, I would like to propose the following
>> changes to the property description:
>> 1. Remove the sentence about reducing power consumption, as this could be
>> considered a policy.
>> 2. Emphasize that the property is a required hardware configuration and
>> not an optional feature on certain devices.
>> 3. Recommend that any changes to the property value should only be made by
>> experienced system administrators and not end-users.
> Please answer - why this should not be enabled always.
One reason why the Touchscreen Controller should not be kept in reset
always is that some devices may have a use case where the touchscreen
needs to remain active even during system sleep, and keeping it in reset
would cause issues with that case. However, in the case of the
battery-powered PinePhone Original and Pro, keeping the touchscreen
controller in reset during system sleep is required for proper system
operation. Having the device in your pocket makes unintentional screen
touches almost unavoidable, and this property enabled is necessary for
these devices. In the case of the PinePhone Original and Pro, enabling
this feature we do consider its impact on battery life or in other words
power consumption.
But bottomlined, enabling this feature should be evaluated on a
case-by-case basis, taking into consideration the specific device use case
and hardware configurations. Thank you for your feedback.
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: [regression] Bug 217182 - Dell Latitude E7450 Trackpoint not working as expected
From: Pali Rohár @ 2023-03-19 16:51 UTC (permalink / raw)
To: Thorsten Leemhuis
Cc: Steffen, Linux kernel regressions list, Dmitry Torokhov,
open list:HID CORE LAYER, LKML, lanelone
In-Reply-To: <b7de0060-8555-65f7-c687-1326a5762929@leemhuis.info>
On Wednesday 15 March 2023 10:50:46 Thorsten Leemhuis wrote:
> On 13.03.23 11:20, Linux regression tracking (Thorsten Leemhuis) wrote:
> >
> > Hi, Thorsten here, the Linux kernel's regression tracker.
> >
> > I noticed a regression report in bugzilla.kernel.org. As many (most?)
> > kernel developer don't keep an eye on it, I decided to forward it by
> > mail. Quoting from https://bugzilla.kernel.org/show_bug.cgi?id=217182 :
> >
> >> Steffen 2023-03-12 11:17:02 UTC
> >>
> >> Hi,
> >> I'm new to this bug tracking system and just a user, not a developer.
> >>
> >> I use a Dell Latitude E7450 running with the Debian unstable based
> >> distribution Siduction.
> >> After upgrading to Linux kernel 6.2.x the Trackpoint does not work
> >> anymore as expected. It uses an Alps PS/2 DualPoint Stick.
> >> Even slight touches in any direction send the cursor to the top right
> >> corner of the screen where it stays regardless of actual pointer
> >> movement. This also appears in console mode. One can retrieve the
> >> pointer again with the touchpad, but I'm more precise with the
> >> Trackpoint and prefer using that.
> >> The Trackpoint works well in kernel 6.1.14
> >> Can somebody help?
> >
> > See the ticket for more details.
>
> FWIW, another user reported seeing the same problem on a different machine:
>
> ```
> Radek 2023-03-14 08:00:31 UTC
>
> I am experiencing the same issue with kernel >6.2.x on Toshiba Z30
> with Alps PS/2 DualPoint trackpoint / touchpad. The cursor jumps top
> right even with the lowest sensitivity settings. It works just fine
> with 6.1 or lower.
> ```
>
> Pali, or anybody else: anyone any idea what's might be wrong there?
> There afaics is only one recent commit to the alps driver (292a089d78d3
> ("treewide: Convert del_timer*() to timer_shutdown*()")); it it maybe
> worth trying to revert it?
>
> Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
> --
> Everything you wanna know about Linux kernel regression tracking:
> https://linux-regtracking.leemhuis.info/about/#tldr
> If I did something stupid, please tell me, as explained on that page.
Could you try following patch?
https://lore.kernel.org/linux-input/20230318144206.14309-1-msizanoen@qtmlabs.xyz/
^ permalink raw reply
* Re: [PATCH] input: alps: fix compatibility with -funsigned-char
From: Pali Rohár @ 2023-03-19 16:54 UTC (permalink / raw)
To: msizanoen
Cc: Dmitry Torokhov, Hans de Goede, stable, linux-input, linux-kernel
In-Reply-To: <1fd818c2-4e68-8760-9123-de4fa1920c6b@qtmlabs.xyz>
On Sunday 19 March 2023 16:56:11 msizanoen wrote:
> Patch confirmed working as expected on real hardware.
>
> Tested-by: msizanoen <msizanoen@qtmlabs.xyz>
Thank you for testing. Patch looks good, you can add my:
Reviewed-by: Pali Rohár <pali@kernel.org>
Anyway, for future, what do you think about using of s8 and u8 types?
It could prevent this signdness char nightmare.
> On 3/18/23 21:42, msizanoen wrote:
> > The AlpsPS/2 code previously relied on the assumption that `char` is a
> > signed type, which was true on x86 platforms (the only place where this
> > driver is used) before kernel 6.2. However, on 6.2 and later, this
> > assumption is broken due to the introduction of -funsigned-char as a new
> > global compiler flag.
> >
> > Fix this by explicitly specifying the signedness of `char` when sign
> > extending the values received from the device.
> >
> > Fixes: f3f33c677699 ("Input: alps - Rushmore and v7 resolution support")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: msizanoen <msizanoen@qtmlabs.xyz>
> > ---
> > drivers/input/mouse/alps.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> > index 989228b5a0a4..1c570d373b30 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2294,20 +2294,20 @@ static int alps_get_v3_v7_resolution(struct psmouse *psmouse, int reg_pitch)
> > if (reg < 0)
> > return reg;
> > - x_pitch = (char)(reg << 4) >> 4; /* sign extend lower 4 bits */
> > + x_pitch = (signed char)(reg << 4) >> 4; /* sign extend lower 4 bits */
> > x_pitch = 50 + 2 * x_pitch; /* In 0.1 mm units */
> > - y_pitch = (char)reg >> 4; /* sign extend upper 4 bits */
> > + y_pitch = (signed char)reg >> 4; /* sign extend upper 4 bits */
> > y_pitch = 36 + 2 * y_pitch; /* In 0.1 mm units */
> > reg = alps_command_mode_read_reg(psmouse, reg_pitch + 1);
> > if (reg < 0)
> > return reg;
> > - x_electrode = (char)(reg << 4) >> 4; /* sign extend lower 4 bits */
> > + x_electrode = (signed char)(reg << 4) >> 4; /* sign extend lower 4 bits */
> > x_electrode = 17 + x_electrode;
> > - y_electrode = (char)reg >> 4; /* sign extend upper 4 bits */
> > + y_electrode = (signed char)reg >> 4; /* sign extend upper 4 bits */
> > y_electrode = 13 + y_electrode;
> > x_phys = x_pitch * (x_electrode - 1); /* In 0.1 mm units */
^ permalink raw reply
* Re: [PATCH] input: alps: fix compatibility with -funsigned-char
From: Jason A. Donenfeld @ 2023-03-19 17:45 UTC (permalink / raw)
To: msizanoen
Cc: Pali Rohár, Dmitry Torokhov, Hans de Goede, stable,
linux-input, linux-kernel
In-Reply-To: <20230318144206.14309-1-msizanoen@qtmlabs.xyz>
Might be wise to clean up a few other ones in that file? Or not. Up to
you:
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 989228b5a0a4..afbf67c2488a 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -852,8 +852,8 @@ static void alps_process_packet_v6(struct psmouse *psmouse)
x = y = z = 0;
/* Divide 4 since trackpoint's speed is too fast */
- input_report_rel(dev2, REL_X, (char)x / 4);
- input_report_rel(dev2, REL_Y, -((char)y / 4));
+ input_report_rel(dev2, REL_X, (signed char)x / 4);
+ input_report_rel(dev2, REL_Y, -((signed char)y / 4));
psmouse_report_standard_buttons(dev2, packet[3]);
@@ -1104,8 +1104,8 @@ static void alps_process_trackstick_packet_v7(struct psmouse *psmouse)
((packet[3] & 0x20) << 1);
z = (packet[5] & 0x3f) | ((packet[3] & 0x80) >> 1);
- input_report_rel(dev2, REL_X, (char)x);
- input_report_rel(dev2, REL_Y, -((char)y));
+ input_report_rel(dev2, REL_X, (signed char)x);
+ input_report_rel(dev2, REL_Y, -((signed char)y));
input_report_abs(dev2, ABS_PRESSURE, z);
psmouse_report_standard_buttons(dev2, packet[1]);
diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
index 5f0d75a45c80..43a1116c5852 100644
--- a/drivers/input/mouse/elan_i2c_core.c
+++ b/drivers/input/mouse/elan_i2c_core.c
@@ -382,7 +382,7 @@ static unsigned int elan_convert_resolution(u8 val, u8 pattern)
* ((value from firmware) + 3) * 100 = dpi
*/
int res = pattern <= 0x01 ?
- (int)(char)val * 10 + 790 : ((int)(char)val + 3) * 100;
+ (int)(signed char)val * 10 + 790 : ((int)(signed char)val + 3) * 100;
/*
* We also have to convert dpi to dots/mm (*10/254 to avoid floating
* point).
^ permalink raw reply related
* Re: [PATCH v3 1/2] dt-bindings: input: touchscreen: Add 'goodix-hold-in-reset' property to Goodix
From: Krzysztof Kozlowski @ 2023-03-19 18:31 UTC (permalink / raw)
To: Jan Jasper de Kroon
Cc: alexandre.belloni, broonie, devicetree, dmitry.torokhov, kernel,
krzysztof.kozlowski+dt, linux-input, robh+dt
In-Reply-To: <c4c3f402-ea3b-ba0a-d9bd-23a642f09946@gmail.com>
On 19/03/2023 17:38, Jan Jasper de Kroon wrote:
>>
>>> - The property is required for proper system operation and is not optional
>>> in specific device use cases. To be more specific in the case of the
>>> PinePhone Original and Pro. The original commit message of the driver
>>> implementation in driver/input/touchscreen contained the following:
>>> "It consumes quite a bit of power (~40mW) during system sleep, and more
>>> when the screen is touched."
>>> Because the phone is usually kept in your pocket, so prone to a lot of
>>> screen touches, this is highly undesired behavior for the touchscreen in
>>> this case. This in my opinion makes it a mandatory property in this
>>> situation.
>> Why then the touchscree should not be kept in reset for other devices?
>> IOW, this should be always used. If you now say "I prefer to keep or not
>> keep it in reset for my device" - it's a policy.
> Thank you for your question. While it's true that keeping the touchscreen
> in reset state during system sleep can reduce power consumption for other
> devices, the decision to use this property should be based on the specific
> use case and hardware configuration of each device. In the case of the
> PinePhone Original and Pro, the touchscreen's power consumption during
> system sleep is significant, and the device is often kept in a pocket, so
> accidental screen touches can occur frequently, leading to further power
> drain. As such, keeping the touchscreen in reset state is necessary for
> proper system operation in these specific devices. However, for other
> devices with different hardware configurations and use cases, the decision
> to use this property should be based on a thorough assessment of the power
> consumption and potential impact on system behavior.
Why? Even if energy consumption for these other devices is very low, it
is still reasonable to keep the touchscreen off during suspend. Why
anyone would like otherwise?
Wakeup could be the reason, but for this we have property.
>>
>>
>>> - The property is not a user-facing configuration option and is not meant
>>> to be changed by the end-user.
>> Does not matter.
>>
>>> - The property, although in separate device specific kernel, and still
>>> called 'poweroff-in-suspend' is already in use on specific devices,
>>> including the PinePhone Original and PinePhone Pro.
>> I could not find such property in the kernel.
> I apologize for the confusion, but the current mainline kernel doesn't
> include this property. As I stated to support the PinePhone Original and
> PinePhone Pro, the community makes use of a forked mainline kernel, with
> a lot of out-of-tree patches/commits, mainly maintained by developer
> Ondrej Jirman. For the PinePhone Original, the DeviceTree configuration
> in the PinePhone DTS gets set in the following commit:
> https://github.com/megous/linux/commit/74fc0a5f0527afdccb67ce3be690f0ae18c8eca6
> For the PinePhone Pro it is set in the following commit, at line 466:
> https://github.com/megous/linux/commit/471c5f33ba74c3d498ccc1eb69c098623b193926#
> The property here is still called "poweroff-in-suspend".
Whatever forks are doing is rarely argument for us. Did that property
pass DT maintainers review? No.
>>
>>> However, I understand your concern that Devicetree should not be used for
>>> policies. To address this concern, I would like to propose the following
>>> changes to the property description:
>>> 1. Remove the sentence about reducing power consumption, as this could be
>>> considered a policy.
>>> 2. Emphasize that the property is a required hardware configuration and
>>> not an optional feature on certain devices.
>>> 3. Recommend that any changes to the property value should only be made by
>>> experienced system administrators and not end-users.
>> Please answer - why this should not be enabled always.
> One reason why the Touchscreen Controller should not be kept in reset
> always is that some devices may have a use case where the touchscreen
> needs to remain active even during system sleep, and keeping it in reset
> would cause issues with that case.
Use case is rather policy... Except wakeup, but for this we have property.
> However, in the case of the
> battery-powered PinePhone Original and Pro, keeping the touchscreen
> controller in reset during system sleep is required for proper system
> operation.
The question was "why not enabled always". How this is related?
> Having the device in your pocket makes unintentional screen
> touches almost unavoidable, and this property enabled is necessary for
> these devices. In the case of the PinePhone Original and Pro, enabling
> this feature we do consider its impact on battery life or in other words
> power consumption.
You keep repeating the same and email is very long.
> But bottomlined, enabling this feature should be evaluated on a
> case-by-case basis, taking into consideration the specific device use case
> and hardware configurations. Thank you for your feedback.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH] input: alps: fix compatibility with -funsigned-char
From: Pali Rohár @ 2023-03-19 19:43 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: msizanoen, Dmitry Torokhov, Hans de Goede, stable, linux-input,
linux-kernel
In-Reply-To: <ZBdKJJ+HJaB0mdNR@zx2c4.com>
On Sunday 19 March 2023 18:45:08 Jason A. Donenfeld wrote:
> Might be wise to clean up a few other ones in that file? Or not. Up to
> you:
>
> diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
> index 989228b5a0a4..afbf67c2488a 100644
> --- a/drivers/input/mouse/alps.c
> +++ b/drivers/input/mouse/alps.c
> @@ -852,8 +852,8 @@ static void alps_process_packet_v6(struct psmouse *psmouse)
> x = y = z = 0;
>
> /* Divide 4 since trackpoint's speed is too fast */
> - input_report_rel(dev2, REL_X, (char)x / 4);
> - input_report_rel(dev2, REL_Y, -((char)y / 4));
> + input_report_rel(dev2, REL_X, (signed char)x / 4);
> + input_report_rel(dev2, REL_Y, -((signed char)y / 4));
Anyway, is casting here needed at all? Is not just plain -(y / 4) enough?
>
> psmouse_report_standard_buttons(dev2, packet[3]);
>
> @@ -1104,8 +1104,8 @@ static void alps_process_trackstick_packet_v7(struct psmouse *psmouse)
> ((packet[3] & 0x20) << 1);
> z = (packet[5] & 0x3f) | ((packet[3] & 0x80) >> 1);
>
> - input_report_rel(dev2, REL_X, (char)x);
> - input_report_rel(dev2, REL_Y, -((char)y));
> + input_report_rel(dev2, REL_X, (signed char)x);
> + input_report_rel(dev2, REL_Y, -((signed char)y));
> input_report_abs(dev2, ABS_PRESSURE, z);
>
> psmouse_report_standard_buttons(dev2, packet[1]);
> diff --git a/drivers/input/mouse/elan_i2c_core.c b/drivers/input/mouse/elan_i2c_core.c
> index 5f0d75a45c80..43a1116c5852 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -382,7 +382,7 @@ static unsigned int elan_convert_resolution(u8 val, u8 pattern)
> * ((value from firmware) + 3) * 100 = dpi
> */
> int res = pattern <= 0x01 ?
> - (int)(char)val * 10 + 790 : ((int)(char)val + 3) * 100;
> + (int)(signed char)val * 10 + 790 : ((int)(signed char)val + 3) * 100;
> /*
> * We also have to convert dpi to dots/mm (*10/254 to avoid floating
> * point).
Please move elantech change into separate commit/patch. As it has
nothing with alps driver. It is completely different hardware.
^ permalink raw reply
* [PATCH v9 0/3] Firmware Support for USB-HID Devices and CP2112
From: Danny Kaehn @ 2023-03-19 20:47 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy
This patchset allows USB-HID devices to have DeviceTree bindings through sharing
the USB fwnode with the HID driver, and adds such a binding and driver
implementation for the CP2112 USB to SMBus Bridge (which necessitated the
USB-HID change). This change allows a CP2112 permanently attached in hardware to
be described in DT and ACPI and interoperate with other drivers.
Changes in v9:
- Add _ADR-based ACPI binding of child nodes (I2C is _ADR Zero, GPIO is _ADR One)
- Use a loop-based approach for assigning child nodes within probe().
As a consequence, hid-cp2112.c no longer maintains references to the
child fwnodes during the lifetime of the device. (plese correct if this
is actually needed for this use-case)
Changes in v8:
- Apply Review tags retroactively to patches previously reviewed
Changes in v7:
- Use dev_fwnode when calling fwnod_handle_put in i2c_adapter in hid-cp2112.c
- Capitalize I2C and GPIO in commit message for patch 0003
Changes in v6:
- Fix fwnode_handle reference leaks in hid-cp21112.c
- Simplify hog node pattern in silabs,cp2112.yaml
Changes in v5:
- Use fwnode API instead of of_node api in hid-core.c and hid-cp2112.c
- Include sda-gpios and scl-gpios in silabs,cp2112.yaml
- Additional fixups to silabs,cp2112.yaml to address comments
- Submit threaded interrupt bugfix separately from this patchset, as requested
Changes in v4:
- Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/i2c
Changes in v3:
- Additional fixups to silabs,cp2112.yaml to address comments
Changes in v2:
- Added more detail to silabs,cp2112.yaml dt-binding
- Moved silabs,cp2112.yaml to /Documentation/devicetree/bindings/input
- Added support for setting smbus clock-frequency from DT in hid-cp2112.c
- Added freeing of of_nodes on error paths of _probe in hid-cp2112.c
Danny Kaehn (3):
dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
HID: usbhid: Share USB device firmware node with child HID device
HID: cp2112: Fwnode Support
.../bindings/i2c/silabs,cp2112.yaml | 113 ++++++++++++++++++
drivers/hid/hid-cp2112.c | 18 +++
drivers/hid/usbhid/hid-core.c | 2 +
3 files changed, 133 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
--
2.25.1
^ permalink raw reply
* [PATCH v9 2/3] HID: usbhid: Share USB device firmware node with child HID device
From: Danny Kaehn @ 2023-03-19 20:48 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy
In-Reply-To: <20230319204802.1364-1-kaehndan@gmail.com>
USB HID core now shares its fwnode with its child HID device.
Since there can only be one HID device on a USB interface, it is redundant
to specify a hid node under the USB device. This allows usb HID device
drivers to be described in firmware and make use of device properties.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/hid/usbhid/hid-core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 257dd73e37bf..090260d99c84 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -19,6 +19,7 @@
#include <linux/list.h>
#include <linux/mm.h>
#include <linux/mutex.h>
+#include <linux/property.h>
#include <linux/spinlock.h>
#include <asm/unaligned.h>
#include <asm/byteorder.h>
@@ -1374,6 +1375,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
hid->hiddev_report_event = hiddev_report_event;
#endif
hid->dev.parent = &intf->dev;
+ device_set_node(&hid->dev, dev_fwnode(&intf->dev));
hid->bus = BUS_USB;
hid->vendor = le16_to_cpu(dev->descriptor.idVendor);
hid->product = le16_to_cpu(dev->descriptor.idProduct);
--
2.25.1
^ permalink raw reply related
* [PATCH v9 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
From: Danny Kaehn @ 2023-03-19 20:48 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy, Krzysztof Kozlowski,
Rob Herring
In-Reply-To: <20230319204802.1364-1-kaehndan@gmail.com>
This is a USB HID device which includes an I2C controller and 8 GPIO pins.
The binding allows describing the chip's gpio and i2c controller in DT
using the subnodes named "gpio" and "i2c", respectively. This is
intended to be used in configurations where the CP2112 is permanently
connected in hardware.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.org>
---
.../bindings/i2c/silabs,cp2112.yaml | 113 ++++++++++++++++++
1 file changed, 113 insertions(+)
create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
new file mode 100644
index 000000000000..a27509627804
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
@@ -0,0 +1,113 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/i2c/silabs,cp2112.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CP2112 HID USB to SMBus/I2C Bridge
+
+maintainers:
+ - Danny Kaehn <kaehndan@gmail.com>
+
+description:
+ The CP2112 is a USB HID device which includes an integrated I2C controller
+ and 8 GPIO pins. Its GPIO pins can each be configured as inputs, open-drain
+ outputs, or push-pull outputs.
+
+properties:
+ compatible:
+ const: usb10c4,ea90
+
+ reg:
+ maxItems: 1
+ description: The USB port number on the host controller
+
+ i2c:
+ description: The SMBus/I2C controller node for the CP2112
+ $ref: /schemas/i2c/i2c-controller.yaml#
+ unevaluatedProperties: false
+
+ properties:
+ sda-gpios:
+ maxItems: 1
+
+ scl-gpios:
+ maxItems: 1
+
+ clock-frequency:
+ minimum: 10000
+ default: 100000
+ maximum: 400000
+
+ gpio:
+ description: The GPIO controller node for the CP2112
+ type: object
+ unevaluatedProperties: false
+
+ properties:
+ interrupt-controller: true
+ "#interrupt-cells":
+ const: 2
+
+ gpio-controller: true
+ "#gpio-cells":
+ const: 2
+
+ gpio-line-names:
+ minItems: 1
+ maxItems: 8
+
+ patternProperties:
+ "-hog(-[0-9]+)?$":
+ type: object
+
+ required:
+ - gpio-hog
+
+required:
+ - compatible
+ - reg
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/interrupt-controller/irq.h>
+ #include <dt-bindings/gpio/gpio.h>
+
+ usb {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ device@1 {
+ compatible = "usb10c4,ea90";
+ reg = <1>;
+
+ i2c {
+ #address-cells = <1>;
+ #size-cells = <0>;
+ sda-gpios = <&cp2112_gpio 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+ scl-gpios = <&cp2112_gpio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
+
+ temp@48 {
+ compatible = "national,lm75";
+ reg = <0x48>;
+ };
+ };
+
+ cp2112_gpio: gpio {
+ gpio-controller;
+ interrupt-controller;
+ #gpio-cells = <2>;
+ gpio-line-names = "CP2112_SDA", "CP2112_SCL", "TEST2",
+ "TEST3","TEST4", "TEST5", "TEST6";
+
+ fan-rst-hog {
+ gpio-hog;
+ gpios = <7 GPIO_ACTIVE_HIGH>;
+ output-high;
+ line-name = "FAN_RST";
+ };
+ };
+ };
+ };
--
2.25.1
^ permalink raw reply related
* [PATCH v9 3/3] HID: cp2112: Fwnode Support
From: Danny Kaehn @ 2023-03-19 20:48 UTC (permalink / raw)
To: robh+dt, krzysztof.kozlowski+dt, jikos, benjamin.tissoires
Cc: bartosz.golaszewski, andriy.shevchenko, dmitry.torokhov,
devicetree, linux-input, ethan.twardy
In-Reply-To: <20230319204802.1364-1-kaehndan@gmail.com>
Support describing the CP2112's I2C and GPIO interfaces in firmware.
I2C and GPIO child nodes can either be children with names "i2c" and
"gpio", or, for ACPI, device nodes with _ADR Zero and One,
respectively.
Additionally, support configuring the I2C bus speed from the
clock-frequency device property.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
---
drivers/hid/hid-cp2112.c | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 27cadadda7c9..9e327763fd90 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1234,6 +1234,10 @@ 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;
+ struct fwnode_handle *child;
+ u32 addr;
+ const char *name;
int ret;
dev = devm_kzalloc(&hdev->dev, sizeof(*dev), GFP_KERNEL);
@@ -1247,6 +1251,17 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
mutex_init(&dev->lock);
+ device_for_each_child_node(&hdev->dev, child) {
+ name = fwnode_get_name(child);
+ ret = acpi_get_local_address(ACPI_HANDLE_FWNODE(child), &addr);
+
+ if ((name && strcmp("i2c", name) == 0) || (!ret && addr == 0))
+ device_set_node(&dev->adap.dev, child);
+ else if ((name && strcmp("gpio", name)) == 0 ||
+ (!ret && addr == 1))
+ dev->gc.fwnode = child;
+ }
+
ret = hid_parse(hdev);
if (ret) {
hid_err(hdev, "parse failed\n");
@@ -1292,6 +1307,9 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
goto err_power_normal;
}
+ 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),
--
2.25.1
^ permalink raw reply related
* [PATCH v2] input: alps: fix compatibility with -funsigned-char
From: msizanoen @ 2023-03-20 0:17 UTC (permalink / raw)
To: msizanoen
Cc: dmitry.torokhov, hdegoede, linux-input, linux-kernel, pali,
stable
In-Reply-To: <20230318144206.14309-1-msizanoen@qtmlabs.xyz>
The AlpsPS/2 code previously relied on the assumption that `char` is a
signed type, which was true on x86 platforms (the only place where this
driver is used) before kernel 6.2. However, on 6.2 and later, this
assumption is broken due to the introduction of -funsigned-char as a new
global compiler flag.
Fix this by explicitly specifying the signedness of `char` when sign
extending the values received from the device.
v2:
Add explicit signedness to more places
Fixes: f3f33c677699 ("Input: alps - Rushmore and v7 resolution support")
Cc: stable@vger.kernel.org
Signed-off-by: msizanoen <msizanoen@qtmlabs.xyz>
---
drivers/input/mouse/alps.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c
index 989228b5a0a4..523ba1196c72 100644
--- a/drivers/input/mouse/alps.c
+++ b/drivers/input/mouse/alps.c
@@ -852,8 +852,8 @@ static void alps_process_packet_v6(struct psmouse *psmouse)
x = y = z = 0;
/* Divide 4 since trackpoint's speed is too fast */
- input_report_rel(dev2, REL_X, (char)x / 4);
- input_report_rel(dev2, REL_Y, -((char)y / 4));
+ input_report_rel(dev2, REL_X, (signed char)x / 4);
+ input_report_rel(dev2, REL_Y, -((signed char)y / 4));
psmouse_report_standard_buttons(dev2, packet[3]);
@@ -1104,8 +1104,8 @@ static void alps_process_trackstick_packet_v7(struct psmouse *psmouse)
((packet[3] & 0x20) << 1);
z = (packet[5] & 0x3f) | ((packet[3] & 0x80) >> 1);
- input_report_rel(dev2, REL_X, (char)x);
- input_report_rel(dev2, REL_Y, -((char)y));
+ input_report_rel(dev2, REL_X, (signed char)x);
+ input_report_rel(dev2, REL_Y, -((signed char)y));
input_report_abs(dev2, ABS_PRESSURE, z);
psmouse_report_standard_buttons(dev2, packet[1]);
@@ -2294,20 +2294,20 @@ static int alps_get_v3_v7_resolution(struct psmouse *psmouse, int reg_pitch)
if (reg < 0)
return reg;
- x_pitch = (char)(reg << 4) >> 4; /* sign extend lower 4 bits */
+ x_pitch = (signed char)(reg << 4) >> 4; /* sign extend lower 4 bits */
x_pitch = 50 + 2 * x_pitch; /* In 0.1 mm units */
- y_pitch = (char)reg >> 4; /* sign extend upper 4 bits */
+ y_pitch = (signed char)reg >> 4; /* sign extend upper 4 bits */
y_pitch = 36 + 2 * y_pitch; /* In 0.1 mm units */
reg = alps_command_mode_read_reg(psmouse, reg_pitch + 1);
if (reg < 0)
return reg;
- x_electrode = (char)(reg << 4) >> 4; /* sign extend lower 4 bits */
+ x_electrode = (signed char)(reg << 4) >> 4; /* sign extend lower 4 bits */
x_electrode = 17 + x_electrode;
- y_electrode = (char)reg >> 4; /* sign extend upper 4 bits */
+ y_electrode = (signed char)reg >> 4; /* sign extend upper 4 bits */
y_electrode = 13 + y_electrode;
x_phys = x_pitch * (x_electrode - 1); /* In 0.1 mm units */
--
2.39.2
^ permalink raw reply related
* RE: BUG: hid-sensor-ids code includes binary data in device name
From: Xu, Even @ 2023-03-20 0:30 UTC (permalink / raw)
To: todd.e.brandt@linux.intel.com, Philipp Jungkamp,
srinivas pandruvada, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org
Cc: Jonathan.Cameron@huawei.com, jkosina@suse.cz, Brandt, Todd E
In-Reply-To: <ebed4596ab82f7d99673ea6660800ed2fb0e1245.camel@linux.intel.com>
Got it, Thanks Todd!
Best Regards,
Even Xu
-----Original Message-----
From: Todd Brandt <todd.e.brandt@linux.intel.com>
Sent: Friday, March 17, 2023 11:38 PM
To: Xu, Even <even.xu@intel.com>; Philipp Jungkamp <p.jungkamp@gmx.net>; srinivas pandruvada <srinivas.pandruvada@linux.intel.com>; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org
Cc: Jonathan.Cameron@huawei.com; jkosina@suse.cz; Brandt, Todd E <todd.e.brandt@intel.com>
Subject: Re: BUG: hid-sensor-ids code includes binary data in device name
On Fri, 2023-03-17 at 05:49 +0000, Xu, Even wrote:
> Hi, All,
>
> Sorry for response later.
>
> From below description, it seems not a buffer overrun issue, it's just
> caused by NULL terminated string, right?
>
Correct, the subject may be a bit misleading, it's just a for loop reading past the end of a string because of the lack of a NULL char.
The patch adds the NULL char.
> Best Regards,
> Even Xu
>
> -----Original Message-----
> From: Todd Brandt <todd.e.brandt@linux.intel.com>
> Sent: Saturday, March 11, 2023 7:36 AM
> To: Philipp Jungkamp <p.jungkamp@gmx.net>; srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com>; linux-input@vger.kernel.org;
> linux-kernel@vger.kernel.org; Xu, Even <even.xu@intel.com>
> Cc: Jonathan.Cameron@huawei.com; jkosina@suse.cz; Brandt, Todd E
> <todd.e.brandt@intel.com>
> Subject: Re: BUG: hid-sensor-ids code includes binary data in device
> name
>
> On Fri, 2023-03-10 at 15:35 +0100, Philipp Jungkamp wrote:
> > Hello,
> >
> > on v3 of the patchset I had this comment on the 'real_usage'
> > initialization:
> >
> > > > - char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> > > > + char real_usage[HID_SENSOR_USAGE_LENGTH];
> > > > struct platform_device *custom_pdev;
> > > > const char *dev_name;
> > > > char *c;
> > > >
> > > > - /* copy real usage id */
> > > > - memcpy(real_usage, known_sensor_luid[index], 4);
> > > > + memcpy(real_usage, match->luid, 4);
> > > > + real_usage[4] = '\0';
> > >
> > > Why the change in approach for setting the NULL character?
> > > Doesn't seem relevant to main purpose of this patch.
> >
> > Based on the comment, I changed that in the final v4 revision to:
> >
> > > - char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> > > + char real_usage[HID_SENSOR_USAGE_LENGTH];
> > > struct platform_device *custom_pdev;
> > > const char *dev_name;
> > > char *c;
> > >
> > > - /* copy real usage id */
> > > - memcpy(real_usage, known_sensor_luid[index], 4);
> > > + memcpy(real_usage, match->luid, 4);
> >
> > I ommitted the line adding the null terminator to the string but
> > kept that I didn't initialize the 'real_usage' as { 0 } anymore. The
> > string now misses the null terminator which leads to the broken
> > utf-8.
> >
> > The simple fix is to reintroduce the 0 initialization in
> > hid_sensor_register_platform_device. E.g.
> >
> > - char real_usage[HID_SENSOR_USAGE_LENGTH];
> > + char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> >
>
> I didn't realize that the issue was a buffer overrun. I tested the
> kernel built with this simple fix and it works ok now. i.e. this patch
> is is all that's needed:
>
> diff --git a/drivers/hid/hid-sensor-custom.c b/drivers/hid/hid-
> sensor- custom.c index 3e3f89e01d81..d85398721659 100644
> --- a/drivers/hid/hid-sensor-custom.c
> +++ b/drivers/hid/hid-sensor-custom.c
> @@ -940,7 +940,7 @@ hid_sensor_register_platform_device(struct
> platform_device *pdev,
> struct hid_sensor_hub_device
> *hsdev,
> const struct
> hid_sensor_custom_match *match) {
> - char real_usage[HID_SENSOR_USAGE_LENGTH];
> + char real_usage[HID_SENSOR_USAGE_LENGTH] = { 0 };
> struct platform_device *custom_pdev;
> const char *dev_name;
> char *c;
>
> > Where do I need to submit a patch for this? And on which tree should
> > I base the patch?
> >
>
> The change is so small it shouldn't require any rebasing. Just send
> the patch to these emails (from MAINTAINERS):
>
> HID SENSOR HUB DRIVERS
> M: Jiri Kosina <jikos@kernel.org>
> M: Jonathan Cameron <jic23@kernel.org>
> M: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> L: linux-input@vger.kernel.org
> L: linux-iio@vger.kernel.org
>
> > I'm sorry for the problems my patch caused.
> >
>
> No problem. It actually made sleepgraph better because it exposed a
> bug in the ftrace processing code. I wasn't properly handling the
> corner case where ftrace had binary data in it.
>
> > Regards,
> > Philipp Jungkamp
> >
> > On Fri, 2023-03-10 at 01:51 -0800, srinivas pandruvada wrote:
> > > +Even
> > >
> > > On Thu, 2023-03-09 at 15:33 -0800, Todd Brandt wrote:
> > > > Hi all, I've run into an issue in 6.3.0-rc1 that causes problems
> > > > with ftrace and I've bisected it to this commit:
> > > >
> > > > commit 98c062e8245199fa9121141a0bf1035dc45ae90e (HEAD,
> > > > refs/bisect/bad)
> > > > Author: Philipp Jungkamp p.jungkamp@gmx.net
> > > > Date: Fri Nov 25 00:38:38 2022 +0100
> > > >
> > > > HID: hid-sensor-custom: Allow more custom iio sensors
> > > >
> > > > The known LUID table for established/known custom HID
> > > > sensors was
> > > > limited to sensors with "INTEL" as manufacturer. But some
> > > > vendors such
> > > > as Lenovo also include fairly standard iio sensors (e.g.
> > > > ambient
> > > > light)
> > > > in their custom sensors.
> > > >
> > > > Expand the known custom sensors table by a tag used for the
> > > > platform
> > > > device name and match sensors based on the LUID as well as
> > > > optionally
> > > > on model and manufacturer properties.
> > > >
> > > > Signed-off-by: Philipp Jungkamp p.jungkamp@gmx.net
> > > > Reviewed-by: Jonathan Cameron Jonathan.Cameron@huawei.com
> > > > Acked-by: Srinivas Pandruvada
> > > > srinivas.pandruvada@linux.intel.com
> > > > Signed-off-by: Jiri Kosina jkosina@suse.cz
> > > >
> > > > You're using raw data as part of the devname in the "real_usage"
> > > > string, but it includes chars other than ASCII, and those chars
> > > > end up being printed out in the ftrace log which is meant to be
> > > > ASCII only.
> > > >
> > > > - /* HID-SENSOR-INT-REAL_USAGE_ID */
> > > > - dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-INT-%s",
> > > > real_usage);
> > > > + /* HID-SENSOR-TAG-REAL_USAGE_ID */
> > > > + dev_name = kasprintf(GFP_KERNEL, "HID-SENSOR-%s-%s",
> > > > + match->tag, real_usage);
> > > >
> > > > My sleepgraph tool started to crash because it read these lines
> > > > from
> > > > ftrace:
> > > >
> > > > device_pm_callback_start: platform HID-SENSOR-INT-
> > > > 020b?.39.auto,
> > > > parent: 001F:8087:0AC2.0003, [suspend]
> > > > device_pm_callback_end: platform HID-SENSOR-INT-020b?.39.auto,
> > > > err=0
> > > >
> > >
> > > Here tag is:
> > > .tag = "INT",
> > > .luid = "020B000000000000",
> > >
> > >
> > > The LUID is still a string. Probably too long for a dev_name.
> > >
> > > Even,
> > >
> > > Please check.
> > >
> > > Thanks.
> > > Srinivas
> > >
> > >
> > > > The "HID-SENSOR-INT-020b?.39.auto" string includes a binary char
> > > > that kills
> > > > python3 code that loops through an ascii file as such:
> > > >
> > > > File "/usr/bin/sleepgraph", line 5579, in executeSuspend
> > > > for line in fp:
> > > > File "/usr/lib/python3.10/codecs.py", line 322, in decode
> > > > (result, consumed) = self._buffer_decode(data, self.errors,
> > > > final)
> > > > UnicodeDecodeError: 'utf-8' codec can't decode byte 0xff in
> > > > position
> > > > 1568: invalid start byte
> > > >
> > > > I've updated sleepgraph to handle random non-ascii chars, but
> > > > other tools may suffer the same fate. Can you rewrite this to
> > > > ensure that no binary chars make it into the devname?
> > > >
> >
> >
>
^ permalink raw reply
* [PATCH AUTOSEL 6.2 02/30] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded
From: Sasha Levin @ 2023-03-20 0:52 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Danny Kaehn, Benjamin Tissoires, Sasha Levin, jikos, linux-input
In-Reply-To: <20230320005258.1428043-1-sashal@kernel.org>
From: Danny Kaehn <kaehndan@gmail.com>
[ Upstream commit 37f5b858a66543b2b67c0288280af623985abc29 ]
The CP2112 generates interrupts from a polling routine on a thread,
and can only support threaded interrupts. This patch configures the
gpiochip irq chip with this flag, disallowing consumers to request
a hard IRQ from this driver, which resulted in a segfault previously.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Link: https://lore.kernel.org/r/20230210170044.11835-1-kaehndan@gmail.com
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-cp2112.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 1e16b0fa310d1..27cadadda7c9d 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1354,6 +1354,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
girq->parents = NULL;
girq->default_type = IRQ_TYPE_NONE;
girq->handler = handle_simple_irq;
+ girq->threaded = true;
ret = gpiochip_add_data(&dev->gc, dev);
if (ret < 0) {
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 6.2 04/30] HID: logitech-hidpp: Add support for Logitech MX Master 3S mouse
From: Sasha Levin @ 2023-03-20 0:52 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Rafał Szalecki, Bastien Nocera, Jiri Kosina, Sasha Levin,
jikos, benjamin.tissoires, linux-input
In-Reply-To: <20230320005258.1428043-1-sashal@kernel.org>
From: Rafał Szalecki <perexist7@gmail.com>
[ Upstream commit db50f7a3983f0154e730f1147ef729e0c5c2f90c ]
Add signature for the Logitech MX Master 3S mouse over Bluetooth.
Signed-off-by: Rafał Szalecki <perexist7@gmail.com>
Reviewed-by: Bastien Nocera <hadess@hadess.net>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-logitech-hidpp.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 5efc591a02a03..3c00e6ac8e76a 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4378,6 +4378,8 @@ static const struct hid_device_id hidpp_devices[] = {
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e) },
{ /* MX Master 3 mouse over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023) },
+ { /* MX Master 3S mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb034) },
{}
};
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 6.2 05/30] HID: intel-ish-hid: ipc: Fix potential use-after-free in work function
From: Sasha Levin @ 2023-03-20 0:52 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Reka Norman, Srinivas Pandruvada, Jiri Kosina, Sasha Levin, jikos,
benjamin.tissoires, liqiong, linux-input
In-Reply-To: <20230320005258.1428043-1-sashal@kernel.org>
From: Reka Norman <rekanorman@chromium.org>
[ Upstream commit 8ae2f2b0a28416ed2f6d8478ac8b9f7862f36785 ]
When a reset notify IPC message is received, the ISR schedules a work
function and passes the ISHTP device to it via a global pointer
ishtp_dev. If ish_probe() fails, the devm-managed device resources
including ishtp_dev are freed, but the work is not cancelled, causing a
use-after-free when the work function tries to access ishtp_dev. Use
devm_work_autocancel() instead, so that the work is automatically
cancelled if probe fails.
Signed-off-by: Reka Norman <rekanorman@chromium.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 15e14239af829..a49c6affd7c4c 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -5,6 +5,7 @@
* Copyright (c) 2014-2016, Intel Corporation.
*/
+#include <linux/devm-helpers.h>
#include <linux/sched.h>
#include <linux/spinlock.h>
#include <linux/delay.h>
@@ -621,7 +622,6 @@ static void recv_ipc(struct ishtp_device *dev, uint32_t doorbell_val)
case MNG_RESET_NOTIFY:
if (!ishtp_dev) {
ishtp_dev = dev;
- INIT_WORK(&fw_reset_work, fw_reset_work_fn);
}
schedule_work(&fw_reset_work);
break;
@@ -940,6 +940,7 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
{
struct ishtp_device *dev;
int i;
+ int ret;
dev = devm_kzalloc(&pdev->dev,
sizeof(struct ishtp_device) + sizeof(struct ish_hw),
@@ -975,6 +976,12 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
list_add_tail(&tx_buf->link, &dev->wr_free_list);
}
+ ret = devm_work_autocancel(&pdev->dev, &fw_reset_work, fw_reset_work_fn);
+ if (ret) {
+ dev_err(dev->devc, "Failed to initialise FW reset work\n");
+ return NULL;
+ }
+
dev->ops = &ish_hw_ops;
dev->devc = &pdev->dev;
dev->mtu = IPC_PAYLOAD_SIZE - sizeof(struct ishtp_msg_hdr);
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 6.1 05/29] HID: intel-ish-hid: ipc: Fix potential use-after-free in work function
From: Sasha Levin @ 2023-03-20 0:53 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Reka Norman, Srinivas Pandruvada, Jiri Kosina, Sasha Levin, jikos,
benjamin.tissoires, liqiong, linux-input
In-Reply-To: <20230320005413.1428452-1-sashal@kernel.org>
From: Reka Norman <rekanorman@chromium.org>
[ Upstream commit 8ae2f2b0a28416ed2f6d8478ac8b9f7862f36785 ]
When a reset notify IPC message is received, the ISR schedules a work
function and passes the ISHTP device to it via a global pointer
ishtp_dev. If ish_probe() fails, the devm-managed device resources
including ishtp_dev are freed, but the work is not cancelled, causing a
use-after-free when the work function tries to access ishtp_dev. Use
devm_work_autocancel() instead, so that the work is automatically
cancelled if probe fails.
Signed-off-by: Reka Norman <rekanorman@chromium.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 15e14239af829..a49c6affd7c4c 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -5,6 +5,7 @@
* Copyright (c) 2014-2016, Intel Corporation.
*/
+#include <linux/devm-helpers.h>
#include <linux/sched.h>
#include <linux/spinlock.h>
#include <linux/delay.h>
@@ -621,7 +622,6 @@ static void recv_ipc(struct ishtp_device *dev, uint32_t doorbell_val)
case MNG_RESET_NOTIFY:
if (!ishtp_dev) {
ishtp_dev = dev;
- INIT_WORK(&fw_reset_work, fw_reset_work_fn);
}
schedule_work(&fw_reset_work);
break;
@@ -940,6 +940,7 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
{
struct ishtp_device *dev;
int i;
+ int ret;
dev = devm_kzalloc(&pdev->dev,
sizeof(struct ishtp_device) + sizeof(struct ish_hw),
@@ -975,6 +976,12 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
list_add_tail(&tx_buf->link, &dev->wr_free_list);
}
+ ret = devm_work_autocancel(&pdev->dev, &fw_reset_work, fw_reset_work_fn);
+ if (ret) {
+ dev_err(dev->devc, "Failed to initialise FW reset work\n");
+ return NULL;
+ }
+
dev->ops = &ish_hw_ops;
dev->devc = &pdev->dev;
dev->mtu = IPC_PAYLOAD_SIZE - sizeof(struct ishtp_msg_hdr);
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 6.1 02/29] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded
From: Sasha Levin @ 2023-03-20 0:53 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Danny Kaehn, Benjamin Tissoires, Sasha Levin, jikos, linux-input
In-Reply-To: <20230320005413.1428452-1-sashal@kernel.org>
From: Danny Kaehn <kaehndan@gmail.com>
[ Upstream commit 37f5b858a66543b2b67c0288280af623985abc29 ]
The CP2112 generates interrupts from a polling routine on a thread,
and can only support threaded interrupts. This patch configures the
gpiochip irq chip with this flag, disallowing consumers to request
a hard IRQ from this driver, which resulted in a segfault previously.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Link: https://lore.kernel.org/r/20230210170044.11835-1-kaehndan@gmail.com
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-cp2112.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 1e16b0fa310d1..27cadadda7c9d 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1354,6 +1354,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
girq->parents = NULL;
girq->default_type = IRQ_TYPE_NONE;
girq->handler = handle_simple_irq;
+ girq->threaded = true;
ret = gpiochip_add_data(&dev->gc, dev);
if (ret < 0) {
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.15 02/17] HID: uhid: Over-ride the default maximum data buffer value with our own
From: Sasha Levin @ 2023-03-20 0:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Lee Jones, Jiri Kosina, Sasha Levin, david.rheinsberg, jikos,
benjamin.tissoires, linux-input
In-Reply-To: <20230320005521.1428820-1-sashal@kernel.org>
From: Lee Jones <lee@kernel.org>
[ Upstream commit 1c5d4221240a233df2440fe75c881465cdf8da07 ]
The default maximum data buffer size for this interface is UHID_DATA_MAX
(4k). When data buffers are being processed, ensure this value is used
when ensuring the sanity, rather than a value between the user provided
value and HID_MAX_BUFFER_SIZE (16k).
Signed-off-by: Lee Jones <lee@kernel.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/uhid.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index fc06d8bb42e0f..ba0ca652b9dab 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -395,6 +395,7 @@ struct hid_ll_driver uhid_hid_driver = {
.parse = uhid_hid_parse,
.raw_request = uhid_hid_raw_request,
.output_report = uhid_hid_output_report,
+ .max_buffer_size = UHID_DATA_MAX,
};
EXPORT_SYMBOL_GPL(uhid_hid_driver);
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.15 03/17] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded
From: Sasha Levin @ 2023-03-20 0:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Danny Kaehn, Benjamin Tissoires, Sasha Levin, jikos, linux-input
In-Reply-To: <20230320005521.1428820-1-sashal@kernel.org>
From: Danny Kaehn <kaehndan@gmail.com>
[ Upstream commit 37f5b858a66543b2b67c0288280af623985abc29 ]
The CP2112 generates interrupts from a polling routine on a thread,
and can only support threaded interrupts. This patch configures the
gpiochip irq chip with this flag, disallowing consumers to request
a hard IRQ from this driver, which resulted in a segfault previously.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Link: https://lore.kernel.org/r/20230210170044.11835-1-kaehndan@gmail.com
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-cp2112.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 172f20e88c6c9..d902fe43cb818 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1352,6 +1352,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
girq->parents = NULL;
girq->default_type = IRQ_TYPE_NONE;
girq->handler = handle_simple_irq;
+ girq->threaded = true;
ret = gpiochip_add_data(&dev->gc, dev);
if (ret < 0) {
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.15 05/17] HID: intel-ish-hid: ipc: Fix potential use-after-free in work function
From: Sasha Levin @ 2023-03-20 0:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Reka Norman, Srinivas Pandruvada, Jiri Kosina, Sasha Levin, jikos,
benjamin.tissoires, liqiong, linux-input
In-Reply-To: <20230320005521.1428820-1-sashal@kernel.org>
From: Reka Norman <rekanorman@chromium.org>
[ Upstream commit 8ae2f2b0a28416ed2f6d8478ac8b9f7862f36785 ]
When a reset notify IPC message is received, the ISR schedules a work
function and passes the ISHTP device to it via a global pointer
ishtp_dev. If ish_probe() fails, the devm-managed device resources
including ishtp_dev are freed, but the work is not cancelled, causing a
use-after-free when the work function tries to access ishtp_dev. Use
devm_work_autocancel() instead, so that the work is automatically
cancelled if probe fails.
Signed-off-by: Reka Norman <rekanorman@chromium.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index 45e0c7b1c9ec6..6c942dd1abca2 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -5,6 +5,7 @@
* Copyright (c) 2014-2016, Intel Corporation.
*/
+#include <linux/devm-helpers.h>
#include <linux/sched.h>
#include <linux/spinlock.h>
#include <linux/delay.h>
@@ -621,7 +622,6 @@ static void recv_ipc(struct ishtp_device *dev, uint32_t doorbell_val)
case MNG_RESET_NOTIFY:
if (!ishtp_dev) {
ishtp_dev = dev;
- INIT_WORK(&fw_reset_work, fw_reset_work_fn);
}
schedule_work(&fw_reset_work);
break;
@@ -936,6 +936,7 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
{
struct ishtp_device *dev;
int i;
+ int ret;
dev = devm_kzalloc(&pdev->dev,
sizeof(struct ishtp_device) + sizeof(struct ish_hw),
@@ -971,6 +972,12 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
list_add_tail(&tx_buf->link, &dev->wr_free_list);
}
+ ret = devm_work_autocancel(&pdev->dev, &fw_reset_work, fw_reset_work_fn);
+ if (ret) {
+ dev_err(dev->devc, "Failed to initialise FW reset work\n");
+ return NULL;
+ }
+
dev->ops = &ish_hw_ops;
dev->devc = &pdev->dev;
dev->mtu = IPC_PAYLOAD_SIZE - sizeof(struct ishtp_msg_hdr);
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.10 02/15] HID: cp2112: Fix driver not registering GPIO IRQ chip as threaded
From: Sasha Levin @ 2023-03-20 0:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Danny Kaehn, Benjamin Tissoires, Sasha Levin, jikos, linux-input
In-Reply-To: <20230320005559.1429040-1-sashal@kernel.org>
From: Danny Kaehn <kaehndan@gmail.com>
[ Upstream commit 37f5b858a66543b2b67c0288280af623985abc29 ]
The CP2112 generates interrupts from a polling routine on a thread,
and can only support threaded interrupts. This patch configures the
gpiochip irq chip with this flag, disallowing consumers to request
a hard IRQ from this driver, which resulted in a segfault previously.
Signed-off-by: Danny Kaehn <kaehndan@gmail.com>
Link: https://lore.kernel.org/r/20230210170044.11835-1-kaehndan@gmail.com
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/hid-cp2112.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/hid-cp2112.c b/drivers/hid/hid-cp2112.c
index 172f20e88c6c9..d902fe43cb818 100644
--- a/drivers/hid/hid-cp2112.c
+++ b/drivers/hid/hid-cp2112.c
@@ -1352,6 +1352,7 @@ static int cp2112_probe(struct hid_device *hdev, const struct hid_device_id *id)
girq->parents = NULL;
girq->default_type = IRQ_TYPE_NONE;
girq->handler = handle_simple_irq;
+ girq->threaded = true;
ret = gpiochip_add_data(&dev->gc, dev);
if (ret < 0) {
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.10 01/15] HID: uhid: Over-ride the default maximum data buffer value with our own
From: Sasha Levin @ 2023-03-20 0:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Lee Jones, Jiri Kosina, Sasha Levin, david.rheinsberg, jikos,
benjamin.tissoires, linux-input
From: Lee Jones <lee@kernel.org>
[ Upstream commit 1c5d4221240a233df2440fe75c881465cdf8da07 ]
The default maximum data buffer size for this interface is UHID_DATA_MAX
(4k). When data buffers are being processed, ensure this value is used
when ensuring the sanity, rather than a value between the user provided
value and HID_MAX_BUFFER_SIZE (16k).
Signed-off-by: Lee Jones <lee@kernel.org>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/uhid.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/hid/uhid.c b/drivers/hid/uhid.c
index fc06d8bb42e0f..ba0ca652b9dab 100644
--- a/drivers/hid/uhid.c
+++ b/drivers/hid/uhid.c
@@ -395,6 +395,7 @@ struct hid_ll_driver uhid_hid_driver = {
.parse = uhid_hid_parse,
.raw_request = uhid_hid_raw_request,
.output_report = uhid_hid_output_report,
+ .max_buffer_size = UHID_DATA_MAX,
};
EXPORT_SYMBOL_GPL(uhid_hid_driver);
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 4.19 3/9] HID: intel-ish-hid: ipc: Fix potential use-after-free in work function
From: Sasha Levin @ 2023-03-20 0:57 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Reka Norman, Srinivas Pandruvada, Jiri Kosina, Sasha Levin, jikos,
benjamin.tissoires, liqiong, linux-input
In-Reply-To: <20230320005707.1429405-1-sashal@kernel.org>
From: Reka Norman <rekanorman@chromium.org>
[ Upstream commit 8ae2f2b0a28416ed2f6d8478ac8b9f7862f36785 ]
When a reset notify IPC message is received, the ISR schedules a work
function and passes the ISHTP device to it via a global pointer
ishtp_dev. If ish_probe() fails, the devm-managed device resources
including ishtp_dev are freed, but the work is not cancelled, causing a
use-after-free when the work function tries to access ishtp_dev. Use
devm_work_autocancel() instead, so that the work is automatically
cancelled if probe fails.
Signed-off-by: Reka Norman <rekanorman@chromium.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index e00b9dbe220f1..abc94235100c6 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -13,6 +13,7 @@
* more details.
*/
+#include <linux/devm-helpers.h>
#include <linux/sched.h>
#include <linux/spinlock.h>
#include <linux/delay.h>
@@ -618,7 +619,6 @@ static void recv_ipc(struct ishtp_device *dev, uint32_t doorbell_val)
case MNG_RESET_NOTIFY:
if (!ishtp_dev) {
ishtp_dev = dev;
- INIT_WORK(&fw_reset_work, fw_reset_work_fn);
}
schedule_work(&fw_reset_work);
break;
@@ -909,6 +909,7 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
{
struct ishtp_device *dev;
int i;
+ int ret;
dev = devm_kzalloc(&pdev->dev,
sizeof(struct ishtp_device) + sizeof(struct ish_hw),
@@ -945,6 +946,12 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
list_add_tail(&tx_buf->link, &dev->wr_free_list_head.link);
}
+ ret = devm_work_autocancel(&pdev->dev, &fw_reset_work, fw_reset_work_fn);
+ if (ret) {
+ dev_err(dev->devc, "Failed to initialise FW reset work\n");
+ return NULL;
+ }
+
dev->ops = &ish_hw_ops;
dev->devc = &pdev->dev;
dev->mtu = IPC_PAYLOAD_SIZE - sizeof(struct ishtp_msg_hdr);
--
2.39.2
^ permalink raw reply related
* [PATCH AUTOSEL 5.10 04/15] HID: intel-ish-hid: ipc: Fix potential use-after-free in work function
From: Sasha Levin @ 2023-03-20 0:55 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Reka Norman, Srinivas Pandruvada, Jiri Kosina, Sasha Levin, jikos,
benjamin.tissoires, liqiong, linux-input
In-Reply-To: <20230320005559.1429040-1-sashal@kernel.org>
From: Reka Norman <rekanorman@chromium.org>
[ Upstream commit 8ae2f2b0a28416ed2f6d8478ac8b9f7862f36785 ]
When a reset notify IPC message is received, the ISR schedules a work
function and passes the ISHTP device to it via a global pointer
ishtp_dev. If ish_probe() fails, the devm-managed device resources
including ishtp_dev are freed, but the work is not cancelled, causing a
use-after-free when the work function tries to access ishtp_dev. Use
devm_work_autocancel() instead, so that the work is automatically
cancelled if probe fails.
Signed-off-by: Reka Norman <rekanorman@chromium.org>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/hid/intel-ish-hid/ipc/ipc.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/intel-ish-hid/ipc/ipc.c b/drivers/hid/intel-ish-hid/ipc/ipc.c
index a45ac7fa417b9..b950873c300b3 100644
--- a/drivers/hid/intel-ish-hid/ipc/ipc.c
+++ b/drivers/hid/intel-ish-hid/ipc/ipc.c
@@ -5,6 +5,7 @@
* Copyright (c) 2014-2016, Intel Corporation.
*/
+#include <linux/devm-helpers.h>
#include <linux/sched.h>
#include <linux/spinlock.h>
#include <linux/delay.h>
@@ -594,7 +595,6 @@ static void recv_ipc(struct ishtp_device *dev, uint32_t doorbell_val)
case MNG_RESET_NOTIFY:
if (!ishtp_dev) {
ishtp_dev = dev;
- INIT_WORK(&fw_reset_work, fw_reset_work_fn);
}
schedule_work(&fw_reset_work);
break;
@@ -885,6 +885,7 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
{
struct ishtp_device *dev;
int i;
+ int ret;
dev = devm_kzalloc(&pdev->dev,
sizeof(struct ishtp_device) + sizeof(struct ish_hw),
@@ -920,6 +921,12 @@ struct ishtp_device *ish_dev_init(struct pci_dev *pdev)
list_add_tail(&tx_buf->link, &dev->wr_free_list);
}
+ ret = devm_work_autocancel(&pdev->dev, &fw_reset_work, fw_reset_work_fn);
+ if (ret) {
+ dev_err(dev->devc, "Failed to initialise FW reset work\n");
+ return NULL;
+ }
+
dev->ops = &ish_hw_ops;
dev->devc = &pdev->dev;
dev->mtu = IPC_PAYLOAD_SIZE - sizeof(struct ishtp_msg_hdr);
--
2.39.2
^ 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