* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-06-05 8:44 UTC (permalink / raw)
To: Jiri Kosina, Bastien Nocera
Cc: linux-input, linux-kernel, Benjamin Tissoires,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado,
Mark Lord, Linux kernel regressions list
In-Reply-To: <nycvar.YFH.7.76.2306031440380.29760@cbobk.fhfr.pm>
On 03.06.23 14:41, Jiri Kosina wrote:
> On Wed, 31 May 2023, Jiri Kosina wrote:
>
>>> If an attempt at contacting a receiver or a device fails because the
>>> receiver or device never responds, don't restart the communication, only
>>> restart it if the receiver or device answers that it's busy, as originally
>>> intended.
>>>
>>> This was the behaviour on communication timeout before commit 586e8fede795
>>> ("HID: logitech-hidpp: Retry commands when device is busy").
>>>
>>> This fixes some overly long waits in a critical path on boot, when
>>> checking whether the device is connected by getting its HID++ version.
>>>
>>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
>>> Suggested-by: Mark Lord <mlord@pobox.com>
>>> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412
> [...]
>>
>> I have applied this even before getting confirmation from the reporters in
>> bugzilla, as it's the right thing to do anyway.
>
> Unfortunately it doesn't seem to cure the reported issue (while reverting
> 586e8fede79 does):
BTW, remind me again: was fixing this by reverting 586e8fede79 for now a
option? I guess it's not, but if I'm wrong I wonder if that might at
this point be the best way forward.
> https://bugzilla.kernel.org/show_bug.cgi?id=217523#c2
FWIW, another comment showed up there:
```
> --- Comment #6 from vova7890 ---
> Same problem. I researched this some time ago. I noticed that if I add a small
> delay between commands to the dongle - everything goes fine. Repeated
> request(586e8fede7953b1695b5ccc6112eff9b052e79ac) made the situation more
> visible
```
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.
#regzbot ^backmonitor:
https://lore.kernel.org/all/15e5d50f-95fc-c7c9-0918-015f24c6fc6d@leemhuis.info/
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
From: Conor Dooley @ 2023-06-05 10:20 UTC (permalink / raw)
To: Cong Yang
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
linux-kernel
In-Reply-To: <20230605060524.1178164-2-yangcong5@huaqin.corp-partner.google.com>
[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]
Hey Cong Yang,
On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> Add an ilitek touch screen chip ili9882t.
Could you add a comment here mentioning the relationship between these
chips?
On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
> .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> index 05e6f2df604c..f0e7ffdce605 100644
> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> @@ -15,11 +15,14 @@ description:
>
> properties:
> compatible:
> - items:
> - - const: elan,ekth6915
> + enum:
> + - elan,ekth6915
> + - ilitek,ili9882t
>
> reg:
> - const: 0x10
> + enum:
> + - 0x10
> + - 0x41
Is 0x10 only valid for the elan,ekth6915 & 0x41 for the ilitek one?
If so, please add some enforcement of the values based on the
compatible.
>
> interrupts:
> maxItems: 1
> @@ -29,11 +32,13 @@ properties:
>
> vcc33-supply:
> description: The 3.3V supply to the touchscreen.
> + If using ili9882t then this supply will not be needed.
>
> vccio-supply:
> description:
> The IO supply to the touchscreen. Need not be specified if this is the
> same as the 3.3V supply.
> + If using ili9882t, the IO supply is required.
There's no need for these sort of comments, you can rely on the required
sections to describe these relationships.
Cheers,
Conor.
>
> required:
> - compatible
> @@ -41,6 +46,18 @@ required:
> - interrupts
> - vcc33-supply
>
> +if:
> + properties:
> + compatible:
> + contains:
> + const: ilitek,ili9882t
> +then:
> + required:
> + - compatible
> + - reg
> + - interrupts
> + - vccio-supply
> +
> additionalProperties: false
>
> examples:
> --
> 2.25.1
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
From: Krzysztof Kozlowski @ 2023-06-05 10:34 UTC (permalink / raw)
To: Cong Yang, robh+dt, krzysztof.kozlowski+dt, conor+dt,
dmitry.torokhov, jikos, benjamin.tissoires, dianders, hsinyi
Cc: linux-input, devicetree, linux-kernel
In-Reply-To: <20230605060524.1178164-2-yangcong5@huaqin.corp-partner.google.com>
On 05/06/2023 08:05, Cong Yang wrote:
> Add an ilitek touch screen chip ili9882t.
>
> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> ---
> .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> 1 file changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> index 05e6f2df604c..f0e7ffdce605 100644
> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> @@ -15,11 +15,14 @@ description:
>
> properties:
> compatible:
> - items:
> - - const: elan,ekth6915
> + enum:
> + - elan,ekth6915
> + - ilitek,ili9882t
>
> reg:
> - const: 0x10
> + enum:
> + - 0x10
> + - 0x41
>
> interrupts:
> maxItems: 1
> @@ -29,11 +32,13 @@ properties:
>
> vcc33-supply:
> description: The 3.3V supply to the touchscreen.
> + If using ili9882t then this supply will not be needed.
What does it mean "will not be needed"? Describe the hardware, not your
drivers.
I don't think you tested your DTS. Submit DTS users, because I do not
believe you are testing your patches. You already got such comment and I
don't see much of improvements here.
>
> vccio-supply:
> description:
> The IO supply to the touchscreen. Need not be specified if this is the
> same as the 3.3V supply.
> + If using ili9882t, the IO supply is required.
Don't repeat constraints in free form text.
>
> required:
> - compatible
> @@ -41,6 +46,18 @@ required:
> - interrupts
> - vcc33-supply
>
> +if:
Keep it in allOf. Will save you one indentation later.
> + properties:
> + compatible:
> + contains:
> + const: ilitek,ili9882t
> +then:
> + required:
> + - compatible
> + - reg
> + - interrupts
Don't duplicate.
> + - vccio-supply
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 28/43] input: keypad: ep93xx: add DT support for Cirrus EP93xx
From: Andy Shevchenko @ 2023-06-05 11:26 UTC (permalink / raw)
To: Nikita Shubin
Cc: Alexander Sverdlin, Arnd Bergmann, Linus Walleij, Dmitry Torokhov,
Jonathan Cameron, Michael Peters, Kris Bahnsen, linux-input,
linux-kernel
In-Reply-To: <61b9be215d7eb9908b2dd6750f1598cbb6842f65.camel@maquefel.me>
On Sun, Jun 04, 2023 at 10:14:52PM +0300, Nikita Shubin wrote:
> On Thu, 2023-06-01 at 19:54 +0300, Andy Shevchenko wrote:
> > On Thu, Jun 01, 2023 at 08:45:33AM +0300, Nikita Shubin wrote:
...
> > > +static int ep93xx_keypad_flags;
> > > +module_param(ep93xx_keypad_flags, int, 0);
> > > +MODULE_PARM_DESC(ep93xx_keypad_flags, "EP93XX keypad flags.");
> >
> > Why? This pretty much looks like missing DT description.
>
> From other patches from this series, i learned NOT to add new DT
> entities, not even with vendor prefix, no way!
>
> May be i missing something of course...
We do not add module parameters to a new code either. So this will be
a dead end.
...
> Was never used in different ways than initializing all to zeroes
> including flags since 2.6 (didn't look before through), so i would
> prefer to drop this completely than moving it into device tree.
This sounds the best!
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Malte Starostik @ 2023-06-05 11:24 UTC (permalink / raw)
To: bagasdotme; +Cc: basavaraj.natikar, linux-input, linux, regressions, stable
In-Reply-To: <ZG3ipauL9FTnQJiC@debian.me>
Hello,
chiming in here as I'm experiencing what looks like the exact same issue, also
on a Lenovo Z13 notebook, also on Arch:
Oops during startup in task udev-worker followed by udev-worker blocking all
attempts to suspend or cleanly shutdown/reboot the machine - in fact I first
noticed because the machine surprised with repeatedly running out of battery
after it had supposedly been in standby but couldn't. Only then I noticed the
error on boot.
bisect result:
904e28c6de083fa4834cdbd0026470ddc30676fc is the first bad commit
commit 904e28c6de083fa4834cdbd0026470ddc30676fc
Merge: a738688177dc 2f7f4efb9411
Author: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date: Wed Feb 22 10:44:31 2023 +0100
Merge branch 'for-6.3/hid-bpf' into for-linus
Initial support of HID-BPF (Benjamin Tissoires)
The history is a little long for this series, as it was intended to be
sent for v6.2. However some last minute issues forced us to postpone it
to v6.3.
Conflicts:
* drivers/hid/i2c-hid/Kconfig:
commit bf7660dab30d ("HID: stop drivers from selecting CONFIG_HID")
conflicts with commit 2afac81dd165 ("HID: fix I2C_HID not selected
when I2C_HID_OF_ELAN is")
the resolution is simple enough: just drop the "default" and "select"
lines as the new commit from Arnd is doing
BR Malte
> On Wed, May 24, 2023 at 02:10:31PM +0800, Haochen Tong wrote:
> > > What last kernel version before this regression occurs? Do you mean
> > > v6.2?
> > >
> >
> > I was using 6.2.12 (Arch Linux distro kernel) before seeing this
regression.
>
> Can you perform bisection to find the culprit that introduces the
> regression? Since you're on Arch Linux, see its wiki article [1] for
> instructions.
>
> Thanks.
>
> [1]: https://wiki.archlinux.org/title/Bisecting_bugs_with_Git
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Benjamin Tissoires @ 2023-06-05 14:20 UTC (permalink / raw)
To: Mark Lord
Cc: Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <ccae168a-4be0-f085-8d8e-03bfbd71ac8f@pobox.com>
On Jun 03 2023, Mark Lord wrote:
>
> On 2023-06-03 08:41 AM, Jiri Kosina wrote:
> > On Wed, 31 May 2023, Jiri Kosina wrote:
> >
> >>> If an attempt at contacting a receiver or a device fails because the
> >>> receiver or device never responds, don't restart the communication, only
> >>> restart it if the receiver or device answers that it's busy, as originally
> >>> intended.
> >>>
> >>> This was the behaviour on communication timeout before commit 586e8fede795
> >>> ("HID: logitech-hidpp: Retry commands when device is busy").
> >>>
> >>> This fixes some overly long waits in a critical path on boot, when
> >>> checking whether the device is connected by getting its HID++ version.
> >>>
> >>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> >>> Suggested-by: Mark Lord <mlord@pobox.com>
> >>> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412
> >>> ---
> >>> 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 0fcfd85fea0f..2246044b1639 100644
> >>> --- a/drivers/hid/hid-logitech-hidpp.c
> >>> +++ b/drivers/hid/hid-logitech-hidpp.c
> >>> @@ -314,6 +314,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> >>> dbg_hid("%s:timeout waiting for response\n", __func__);
> >>> memset(response, 0, sizeof(struct hidpp_report));
> >>> ret = -ETIMEDOUT;
> >>> + goto exit;
> >>> }
> >>>
> >>
> >> I have applied this even before getting confirmation from the reporters in
> >> bugzilla, as it's the right thing to do anyway.
> >
> > Unfortunately it doesn't seem to cure the reported issue (while reverting
> > 586e8fede79 does): https://bugzilla.kernel.org/show_bug.cgi?id=217523#c2
>
> I wonder if this code could be re-worked to not even do this (waiting)
> from the _probe() function? It ought to be able to throw it on a workqueue
> or something, rather than stalling system boot for a minimum of 5-seconds
> (or much longer as as-is).
That's an option, but the fact that I can not replicate locally with the
exact same hardware seems to indicate that we would just be papering
over the issue.
Here, I admittely have the USB receiver running through USB-C ports, and
the communication never fails and I get immediate bring ups of the
devices. Which means I am not hitting that path.
The hidpp driver should have everything ready to delay the init in a
workqueue, but the impacted users would still get a delay when they plug
in the device (which is better than stalling the boot, I agree).
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Benjamin Tissoires @ 2023-06-05 14:27 UTC (permalink / raw)
To: Linux regressions mailing list
Cc: Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado,
Mark Lord
In-Reply-To: <15bb2507-a145-7f1b-8e84-58aeb02484b9@leemhuis.info>
On Jun 05 2023, Linux regression tracking (Thorsten Leemhuis) wrote:
>
> On 03.06.23 14:41, Jiri Kosina wrote:
> > On Wed, 31 May 2023, Jiri Kosina wrote:
> >
> >>> If an attempt at contacting a receiver or a device fails because the
> >>> receiver or device never responds, don't restart the communication, only
> >>> restart it if the receiver or device answers that it's busy, as originally
> >>> intended.
> >>>
> >>> This was the behaviour on communication timeout before commit 586e8fede795
> >>> ("HID: logitech-hidpp: Retry commands when device is busy").
> >>>
> >>> This fixes some overly long waits in a critical path on boot, when
> >>> checking whether the device is connected by getting its HID++ version.
> >>>
> >>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> >>> Suggested-by: Mark Lord <mlord@pobox.com>
> >>> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> >>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412
> > [...]
> >>
> >> I have applied this even before getting confirmation from the reporters in
> >> bugzilla, as it's the right thing to do anyway.
> >
> > Unfortunately it doesn't seem to cure the reported issue (while reverting
> > 586e8fede79 does):
>
> BTW, remind me again: was fixing this by reverting 586e8fede79 for now a
> option? I guess it's not, but if I'm wrong I wonder if that might at
> this point be the best way forward.
Could be. I don't think we thought at simply reverting it because it is
required for some new supoprted devices because they might differ
slightly from what we currently supported.
That being said, Bastien will be unavailable for at least a week AFAIU,
so maybe we should revert that patch.
>
> > https://bugzilla.kernel.org/show_bug.cgi?id=217523#c2
>
> FWIW, another comment showed up there:
>
> ```
> > --- Comment #6 from vova7890 ---
> > Same problem. I researched this some time ago. I noticed that if I add a small
> > delay between commands to the dongle - everything goes fine. Repeated
> > request(586e8fede7953b1695b5ccc6112eff9b052e79ac) made the situation more
> > visible
I don't think I ever had to add any delays between commands. The USB
stack should be capable of forwarding the commands just fine. So unless
the receiver is of a different hardware (but same VID/PID) that might
expose a problem elsewhere (in the USB controller maybe???). Just a long
shot, but maybe getting the config of the impacted users and what are
the USB controllers/drivers they are using might gives us a better
understanding.
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Mark Lord @ 2023-06-05 14:31 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <jafrw2wirzegnf7v2pcm52sao5yc4futa7ok4ccmywigj3y2c4@pep4b3ebus7s>
On 2023-06-05 10:20 AM, Benjamin Tissoires wrote:
>
> On Jun 03 2023, Mark Lord wrote:
..
>> I wonder if this code could be re-worked to not even do this (waiting)
>> from the _probe() function? It ought to be able to throw it on a workqueue
>> or something, rather than stalling system boot for a minimum of 5-seconds
>> (or much longer as as-is).
>
> That's an option, but the fact that I can not replicate locally with the
> exact same hardware seems to indicate that we would just be papering
> over the issue.
>
> Here, I admittely have the USB receiver running through USB-C ports, and
> the communication never fails and I get immediate bring ups of the
> devices. Which means I am not hitting that path.
>
> The hidpp driver should have everything ready to delay the init in a
> workqueue, but the impacted users would still get a delay when they plug
> in the device (which is better than stalling the boot, I agree).
..
Oddly, it's only a boot-time thing.
If I unplug the Logitech Unifying receiver, wait a few seconds,
and then plug it back in.. my mouse, keyboard, and touchpad all work immediately.
Unlike during boot.
--
Mark Lord
^ permalink raw reply
* [PATCH 2/2] Input: tps65219-pwrbutton - Use regmap_set_bits()
From: Uwe Kleine-König @ 2023-06-05 16:14 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Markus Schneider-Pargmann, linux-input, kernel
In-Reply-To: <20230605161458.117361-1-u.kleine-koenig@pengutronix.de>
regmap_set_bits() is equivalent to regmap_update_bits() if mask == val.
The probe function uses regmap_clear_bits() to enable irqs, so
symmetrically make use of regmap_set_bits() to disable them. There is no
semantic difference.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/input/misc/tps65219-pwrbutton.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/input/misc/tps65219-pwrbutton.c b/drivers/input/misc/tps65219-pwrbutton.c
index b2d9e5d2bcfd..eeb9f2334ab4 100644
--- a/drivers/input/misc/tps65219-pwrbutton.c
+++ b/drivers/input/misc/tps65219-pwrbutton.c
@@ -123,9 +123,8 @@ static void tps65219_pb_remove(struct platform_device *pdev)
int ret;
/* Disable interrupt for the pushbutton */
- ret = regmap_update_bits(tps->regmap, TPS65219_REG_MASK_CONFIG,
- TPS65219_REG_MASK_INT_FOR_PB_MASK,
- TPS65219_REG_MASK_INT_FOR_PB_MASK);
+ ret = regmap_set_bits(tps->regmap, TPS65219_REG_MASK_CONFIG,
+ TPS65219_REG_MASK_INT_FOR_PB_MASK);
if (ret)
dev_warn(&pdev->dev, "Failed to disable irq (%pe)\n", ERR_PTR(ret));
}
--
2.39.2
^ permalink raw reply related
* [PATCH 1/2] Input: tps65219-pwrbutton - Convert to .remove_new()
From: Uwe Kleine-König @ 2023-06-05 16:14 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Markus Schneider-Pargmann, linux-input, kernel
The .remove() callback for a platform driver returns an int which makes
many driver authors wrongly assume it's possible to do error handling by
returning an error code. However the value returned is ignored (apart from
emitting a warning).
To improve here there is a quest to make the remove callback return void.
In the first step of this quest all drivers are converted to .remove_new()
which already returns void. Eventually after all drivers are converted,
.remove_new() is renamed to .remove().
Before this driver might have returned an error. In this case emit a
warning that tells more about the problem than the generic warning by
the core, and instead of making the remove callback return zero
unconditionally, convert to .remove_new() which is equivalent.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/input/misc/tps65219-pwrbutton.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/input/misc/tps65219-pwrbutton.c b/drivers/input/misc/tps65219-pwrbutton.c
index 245134bdb59e..b2d9e5d2bcfd 100644
--- a/drivers/input/misc/tps65219-pwrbutton.c
+++ b/drivers/input/misc/tps65219-pwrbutton.c
@@ -117,14 +117,17 @@ static int tps65219_pb_probe(struct platform_device *pdev)
return 0;
}
-static int tps65219_pb_remove(struct platform_device *pdev)
+static void tps65219_pb_remove(struct platform_device *pdev)
{
struct tps65219 *tps = dev_get_drvdata(pdev->dev.parent);
+ int ret;
/* Disable interrupt for the pushbutton */
- return regmap_update_bits(tps->regmap, TPS65219_REG_MASK_CONFIG,
- TPS65219_REG_MASK_INT_FOR_PB_MASK,
- TPS65219_REG_MASK_INT_FOR_PB_MASK);
+ ret = regmap_update_bits(tps->regmap, TPS65219_REG_MASK_CONFIG,
+ TPS65219_REG_MASK_INT_FOR_PB_MASK,
+ TPS65219_REG_MASK_INT_FOR_PB_MASK);
+ if (ret)
+ dev_warn(&pdev->dev, "Failed to disable irq (%pe)\n", ERR_PTR(ret));
}
static const struct platform_device_id tps65219_pwrbtn_id_table[] = {
@@ -135,7 +138,7 @@ MODULE_DEVICE_TABLE(platform, tps65219_pwrbtn_id_table);
static struct platform_driver tps65219_pb_driver = {
.probe = tps65219_pb_probe,
- .remove = tps65219_pb_remove,
+ .remove_new = tps65219_pb_remove,
.driver = {
.name = "tps65219_pwrbutton",
},
base-commit: ac9a78681b921877518763ba0e89202254349d1b
--
2.39.2
^ permalink raw reply related
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Mark Lord @ 2023-06-05 16:25 UTC (permalink / raw)
To: Benjamin Tissoires, Linux regressions mailing list
Cc: Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <7ko33em3pqdaeghkt6wumzks6fz2lzztmqyhyzvv3kisjovmvr@mojlmkmrqlml>
On 2023-06-05 10:27 AM, Benjamin Tissoires wrote:
>
> On Jun 05 2023, Linux regression tracking (Thorsten Leemhuis) wrote:
>>
>> On 03.06.23 14:41, Jiri Kosina wrote:
>>> On Wed, 31 May 2023, Jiri Kosina wrote:
>>>
>>>>> If an attempt at contacting a receiver or a device fails because the
>>>>> receiver or device never responds, don't restart the communication, only
>>>>> restart it if the receiver or device answers that it's busy, as originally
>>>>> intended.
>>>>>
>>>>> This was the behaviour on communication timeout before commit 586e8fede795
>>>>> ("HID: logitech-hidpp: Retry commands when device is busy").
>>>>>
>>>>> This fixes some overly long waits in a critical path on boot, when
>>>>> checking whether the device is connected by getting its HID++ version.
>>>>>
>>>>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
>>>>> Suggested-by: Mark Lord <mlord@pobox.com>
>>>>> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412
>>> [...]
>>>>
>>>> I have applied this even before getting confirmation from the reporters in
>>>> bugzilla, as it's the right thing to do anyway.
>>>
>>> Unfortunately it doesn't seem to cure the reported issue (while reverting
>>> 586e8fede79 does):
>>
>> BTW, remind me again: was fixing this by reverting 586e8fede79 for now a
>> option? I guess it's not, but if I'm wrong I wonder if that might at
>> this point be the best way forward.
>
> Could be. I don't think we thought at simply reverting it because it is
> required for some new supoprted devices because they might differ
> slightly from what we currently supported.
>
> That being said, Bastien will be unavailable for at least a week AFAIU,
> so maybe we should revert that patch.
>
>>
>>> https://bugzilla.kernel.org/show_bug.cgi?id=217523#c2
Pardon me, but I don't understand what that "retry" loop
is even attempting to do inside function hidpp_send_message_sync().
It appears to unconditionally loop until:
(1) the __hidpp_send_report() fails,
or (2) it gets a HIDPP_ERROR,
or (3) it gets a HIDPP20_ERROR other than HIDPP20_ERROR_BUSY,
or (4) until it has looped 3 times, which appears to be the normal exit.
It doesn't seem to have any provision to exit the loop earlier on "success"
(whatever that is).
And so when it finally does exit after the 3 iterations,
it then returns the last value of "ret",
which will be zero from the __hidpp_send_report() call,
or sometimes the most recent non-BUSY HIDPP20_ERROR seen.
Obviously I'm missing something, as otherwise this code would never have
passed review and made it into the Linux kernel in the first place. Right?
What is this code trying to do? And what am I not seeing?
--
Mark Lord
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Benjamin Tissoires @ 2023-06-05 17:04 UTC (permalink / raw)
To: Mark Lord
Cc: Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <2c10eb8f-8804-d47f-7b15-5da56ffb5414@pobox.com>
On Jun 05 2023, Mark Lord wrote:
>
> On 2023-06-05 10:27 AM, Benjamin Tissoires wrote:
> >
> > On Jun 05 2023, Linux regression tracking (Thorsten Leemhuis) wrote:
> >>
> >> On 03.06.23 14:41, Jiri Kosina wrote:
> >>> On Wed, 31 May 2023, Jiri Kosina wrote:
> >>>
> >>>>> If an attempt at contacting a receiver or a device fails because the
> >>>>> receiver or device never responds, don't restart the communication, only
> >>>>> restart it if the receiver or device answers that it's busy, as originally
> >>>>> intended.
> >>>>>
> >>>>> This was the behaviour on communication timeout before commit 586e8fede795
> >>>>> ("HID: logitech-hidpp: Retry commands when device is busy").
> >>>>>
> >>>>> This fixes some overly long waits in a critical path on boot, when
> >>>>> checking whether the device is connected by getting its HID++ version.
> >>>>>
> >>>>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
> >>>>> Suggested-by: Mark Lord <mlord@pobox.com>
> >>>>> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> >>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412
> >>> [...]
> >>>>
> >>>> I have applied this even before getting confirmation from the reporters in
> >>>> bugzilla, as it's the right thing to do anyway.
> >>>
> >>> Unfortunately it doesn't seem to cure the reported issue (while reverting
> >>> 586e8fede79 does):
> >>
> >> BTW, remind me again: was fixing this by reverting 586e8fede79 for now a
> >> option? I guess it's not, but if I'm wrong I wonder if that might at
> >> this point be the best way forward.
> >
> > Could be. I don't think we thought at simply reverting it because it is
> > required for some new supoprted devices because they might differ
> > slightly from what we currently supported.
> >
> > That being said, Bastien will be unavailable for at least a week AFAIU,
> > so maybe we should revert that patch.
> >
> >>
> >>> https://bugzilla.kernel.org/show_bug.cgi?id=217523#c2
>
> Pardon me, but I don't understand what that "retry" loop
> is even attempting to do inside function hidpp_send_message_sync().
>
> It appears to unconditionally loop until:
> (1) the __hidpp_send_report() fails,
> or (2) it gets a HIDPP_ERROR,
> or (3) it gets a HIDPP20_ERROR other than HIDPP20_ERROR_BUSY,
> or (4) until it has looped 3 times, which appears to be the normal exit.
>
> It doesn't seem to have any provision to exit the loop earlier on "success"
> (whatever that is).
>
> And so when it finally does exit after the 3 iterations,
> it then returns the last value of "ret",
> which will be zero from the __hidpp_send_report() call,
> or sometimes the most recent non-BUSY HIDPP20_ERROR seen.
>
> Obviously I'm missing something, as otherwise this code would never have
> passed review and made it into the Linux kernel in the first place. Right?
Ouch, very much ouch :(
So that one is on me, sorry, I completely missed that and trusted the
local tests. We are human and can make mistakes. And TBH a lot of people
have been staring at this code for a while now without noticing that.
Would you mind giving a shot at the following (untested) patch:
---
From 7d03a6b765571d7c518c85e7e74f6f9d498fa354 Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Date: Mon, 5 Jun 2023 18:56:36 +0200
Subject: [PATCH] HID: hidpp: terminate retry loop on success
It seems we forgot the normal case to terminate the retry loop,
making us asking 3 times each command, which is probably a little bit
too much.
And remove the ugly "goto exit" that can be replaced by a simpler "break"
Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
Suggested-by: Mark Lord <mlord@pobox.com>
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-logitech-hidpp.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 2246044b1639..5e1a412fd28f 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -286,7 +286,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
struct hidpp_report *message,
struct hidpp_report *response)
{
- int ret;
+ int ret = -1;
int max_retries = 3;
mutex_lock(&hidpp->send_mutex);
@@ -300,13 +300,13 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
*/
*response = *message;
- for (; max_retries != 0; max_retries--) {
+ for (; max_retries != 0 && ret; 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;
+ break;
}
if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
@@ -314,14 +314,14 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
dbg_hid("%s:timeout waiting for response\n", __func__);
memset(response, 0, sizeof(struct hidpp_report));
ret = -ETIMEDOUT;
- goto exit;
+ break;
}
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;
+ break;
}
if ((response->report_id == REPORT_ID_HIDPP_LONG ||
@@ -330,13 +330,12 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
ret = response->fap.params[1];
if (ret != HIDPP20_ERROR_BUSY) {
dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
- goto exit;
+ break;
}
dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
}
}
-exit:
mutex_unlock(&hidpp->send_mutex);
return ret;
--
2.40.1
---
>
> What is this code trying to do? And what am I not seeing?
It was supposed to retry on specific errors only, but it was retrying
on success too...
Cheers,
Benjamin
^ permalink raw reply related
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Mark Lord @ 2023-06-05 17:47 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <ansxam7w4aiyyqh4e2g2elnd37qfbeywkl4q4rcezasw64kqc4@2c54ppsnhegm>
On 2023-06-05 01:04 PM, Benjamin Tissoires wrote:
>
>
> On Jun 05 2023, Mark Lord wrote:
>>
>> On 2023-06-05 10:27 AM, Benjamin Tissoires wrote:
>>>
>>> On Jun 05 2023, Linux regression tracking (Thorsten Leemhuis) wrote:
>>>>
>>>> On 03.06.23 14:41, Jiri Kosina wrote:
>>>>> On Wed, 31 May 2023, Jiri Kosina wrote:
>>>>>
>>>>>>> If an attempt at contacting a receiver or a device fails because the
>>>>>>> receiver or device never responds, don't restart the communication, only
>>>>>>> restart it if the receiver or device answers that it's busy, as originally
>>>>>>> intended.
>>>>>>>
>>>>>>> This was the behaviour on communication timeout before commit 586e8fede795
>>>>>>> ("HID: logitech-hidpp: Retry commands when device is busy").
>>>>>>>
>>>>>>> This fixes some overly long waits in a critical path on boot, when
>>>>>>> checking whether the device is connected by getting its HID++ version.
>>>>>>>
>>>>>>> Signed-off-by: Bastien Nocera <hadess@hadess.net>
>>>>>>> Suggested-by: Mark Lord <mlord@pobox.com>
>>>>>>> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
>>>>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=217412
>>>>> [...]
>>>>>>
>>>>>> I have applied this even before getting confirmation from the reporters in
>>>>>> bugzilla, as it's the right thing to do anyway.
>>>>>
>>>>> Unfortunately it doesn't seem to cure the reported issue (while reverting
>>>>> 586e8fede79 does):
>>>>
>>>> BTW, remind me again: was fixing this by reverting 586e8fede79 for now a
>>>> option? I guess it's not, but if I'm wrong I wonder if that might at
>>>> this point be the best way forward.
>>>
>>> Could be. I don't think we thought at simply reverting it because it is
>>> required for some new supoprted devices because they might differ
>>> slightly from what we currently supported.
>>>
>>> That being said, Bastien will be unavailable for at least a week AFAIU,
>>> so maybe we should revert that patch.
>>>
>>>>
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=217523#c2
>>
>> Pardon me, but I don't understand what that "retry" loop
>> is even attempting to do inside function hidpp_send_message_sync().
>>
>> It appears to unconditionally loop until:
>> (1) the __hidpp_send_report() fails,
>> or (2) it gets a HIDPP_ERROR,
>> or (3) it gets a HIDPP20_ERROR other than HIDPP20_ERROR_BUSY,
>> or (4) until it has looped 3 times, which appears to be the normal exit.
>>
>> It doesn't seem to have any provision to exit the loop earlier on "success"
>> (whatever that is).
>>
>> And so when it finally does exit after the 3 iterations,
>> it then returns the last value of "ret",
>> which will be zero from the __hidpp_send_report() call,
>> or sometimes the most recent non-BUSY HIDPP20_ERROR seen.
>>
>> Obviously I'm missing something, as otherwise this code would never have
>> passed review and made it into the Linux kernel in the first place. Right?
>
> Ouch, very much ouch :(
>
> So that one is on me, sorry, I completely missed that and trusted the
> local tests. We are human and can make mistakes. And TBH a lot of people
> have been staring at this code for a while now without noticing that.
>
> Would you mind giving a shot at the following (untested) patch:
>
> ---
>>From 7d03a6b765571d7c518c85e7e74f6f9d498fa354 Mon Sep 17 00:00:00 2001
> From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Date: Mon, 5 Jun 2023 18:56:36 +0200
> Subject: [PATCH] HID: hidpp: terminate retry loop on success
>
> It seems we forgot the normal case to terminate the retry loop,
> making us asking 3 times each command, which is probably a little bit
> too much.
>
> And remove the ugly "goto exit" that can be replaced by a simpler "break"
>
> Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> Suggested-by: Mark Lord <mlord@pobox.com>
> Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-logitech-hidpp.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 2246044b1639..5e1a412fd28f 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -286,7 +286,7 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> struct hidpp_report *message,
> struct hidpp_report *response)
> {
> - int ret;
> + int ret = -1;
> int max_retries = 3;
>
> mutex_lock(&hidpp->send_mutex);
> @@ -300,13 +300,13 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> */
> *response = *message;
>
> - for (; max_retries != 0; max_retries--) {
> + for (; max_retries != 0 && ret; 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;
> + break;
> }
>
> if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
> @@ -314,14 +314,14 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> dbg_hid("%s:timeout waiting for response\n", __func__);
> memset(response, 0, sizeof(struct hidpp_report));
> ret = -ETIMEDOUT;
> - goto exit;
> + break;
> }
>
> 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;
> + break;
> }
>
> if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> @@ -330,13 +330,12 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> ret = response->fap.params[1];
> if (ret != HIDPP20_ERROR_BUSY) {
> dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> - goto exit;
> + break;
> }
> dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
> }
> }
>
> -exit:
> mutex_unlock(&hidpp->send_mutex);
> return ret;
>
>
Tested-by: Mark Lord <mlord@pobox.com>
That works fine for me on top of 6.3.6,
and I don't even see the ETIMEDOUT happening there either (added a printk() for it).
I am unable to test on 6.4.0-rc5, because that kernel doesn't work with my USB3 docking station.
Cheers
--
Mark Lord
^ permalink raw reply
* Re: [PATCH v4 1/4] Input: ads7846 - Convert to use software nodes
From: Guenter Roeck @ 2023-06-05 18:33 UTC (permalink / raw)
To: Linus Walleij
Cc: Aaro Koskinen, Janusz Krzysztofik, Tony Lindgren, Russell King,
Daniel Mack, Haojian Zhuang, Robert Jarzmik, Thomas Bogendoerfer,
Dmitry Torokhov, Mark Brown, Bartosz Golaszewski, Andreas Kemnade,
Helge Deller, Ulf Hansson, linux-omap, linux-arm-kernel,
linux-kernel, linux-mips, linux-input, linux-spi, linux-fbdev,
dri-devel, linux-mmc
In-Reply-To: <20230430-nokia770-regression-v4-1-9b6dc5536b17@linaro.org>
Hi,
On Mon, May 08, 2023 at 11:20:06PM +0200, Linus Walleij wrote:
> The Nokia 770 is using GPIOs from the global numberspace on the
> CBUS node to pass down to the LCD controller. This regresses when we
> let the OMAP GPIO driver use dynamic GPIO base.
>
...
>
> Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Fixes: 92bf78b33b0b ("gpio: omap: use dynamic allocation of base")
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
This patch causes a crash with the mcimx7d-sabre qemu emulation.
Backtrace and bisect results below.
Source:
pdev_id = device_get_match_data(dev);
if (!pdev_id) {
dev_err(dev, "Unknown device model\n");
return ERR_PTR(-EINVAL);
}
pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
return ERR_PTR(-ENOMEM);
pdata->model = (unsigned long)pdev_id->driver_data; <--- crash
It appears that device_get_match_data() already returns driver_data.
A little debugging shows that pdev_id is indeed 0x1EA6 or 7846.
Guenter
---
Stack backtrace:
[ 7.254561] wm8960 3-001a: Failed to issue reset
[ 7.257313] wm8960: probe of 3-001a failed with error -110
[ 7.258170] i2c i2c-3: IMX I2C adapter registered
[ 7.265584] hw perfevents: enabled with armv7_cortex_a7 PMU driver, 5 counters available
[ 7.280312] stmp_reset_block((ptrval)): module reset timeout
[ 7.280573] mxs-dma: probe of 33000000.dma-controller failed with error -110
[ 7.309572] 8<--- cut here ---
[ 7.309785] Unhandled fault: page domain fault (0x01b) at 0x00001eba
[ 7.309916] [00001eba] *pgd=00000000
[ 7.310327] Internal error: : 1b [#1] SMP ARM
[ 7.310504] Modules linked in:
[ 7.310701] CPU: 0 PID: 29 Comm: kworker/u4:2 Tainted: G N 6.4.0-rc5-next-20230605 #1
[ 7.310868] Hardware name: Freescale i.MX7 Dual (Device Tree)
[ 7.311058] Workqueue: events_unbound deferred_probe_work_func
[ 7.311482] PC is at ads7846_probe+0x9f0/0xfa0
[ 7.311576] LR is at _raw_spin_unlock_irqrestore+0x50/0x64
[ 7.311659] pc : [<c0cf2ab0>] lr : [<c1128bf4>] psr: a0000013
[ 7.311742] sp : d0949bf0 ip : 00000001 fp : 00000000
[ 7.311814] r10: c1b96954 r9 : cbdcd208 r8 : c499aec0
[ 7.311889] r7 : c4b64400 r6 : c4b4d040 r5 : c4b64400 r4 : 00001ea6
[ 7.311978] r3 : 00000000 r2 : c41dd000 r1 : 0a3d1000 r0 : c499aec0
[ 7.312102] Flags: NzCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment none
[ 7.312211] Control: 10c5387d Table: 848c406a DAC: 00000051
[ 7.312305] Register r0 information: slab kmalloc-128 start c499ae80 pointer offset 64 size 128
[ 7.312688] Register r1 information: non-paged memory
[ 7.312832] Register r2 information: slab kmalloc-4k start c41dd000 pointer offset 0 size 4096
[ 7.312973] Register r3 information: NULL pointer
[ 7.313045] Register r4 information: non-paged memory
[ 7.313116] Register r5 information: slab kmalloc-1k start c4b64400 pointer offset 0 size 1024
[ 7.313247] Register r6 information: slab kmalloc-4k start c4b4d000 pointer offset 64 size 4096
[ 7.313379] Register r7 information: slab kmalloc-1k start c4b64400 pointer offset 0 size 1024
[ 7.313509] Register r8 information: slab kmalloc-128 start c499ae80 pointer offset 64 size 128
[ 7.313640] Register r9 information: non-slab/vmalloc memory
[ 7.313751] Register r10 information: non-slab/vmalloc memory
[ 7.313836] Register r11 information: NULL pointer
[ 7.313908] Register r12 information: non-paged memory
[ 7.313992] Process kworker/u4:2 (pid: 29, stack limit = 0x(ptrval))
[ 7.314107] Stack: (0xd0949bf0 to 0xd094a000)
[ 7.314223] 9be0: 00000000 c4b64400 c4b64400 c1bc0a9c
[ 7.314337] 9c00: c4b69000 c234b3f4 cbdcd208 c0e01338 ffffffff 00000000 00000000 00000000
[ 7.314441] 9c20: 00000000 00000000 00000000 d0b2c62c 00000000 00000000 c4b64400 c1bc0a9c
[ 7.314543] 9c40: 00000000 c234b3f4 cbdcd208 c1b96954 00000000 c0b73a98 c4b64400 00000000
[ 7.314650] 9c60: c1bc0aac c0a4a434 c4b64400 c1bc0aac c4b64400 00000016 c234b3f4 c0a4a6d4
[ 7.314754] 9c80: c134db68 3fffffff c234b44c c1bc0aac c4b64400 00000016 c234b3f4 c0a4a81c
[ 7.314856] 9ca0: 00000001 c1bc0aac d0949cfc c4b64400 c234b3f4 c0a4a948 00000000 c41ea600
[ 7.314957] 9cc0: d0949cfc c0a4a8b4 c234b3f4 c0a4849c c4b688bc c41ea6e4 c461d3d8 d0b2c62c
[ 7.315059] 9ce0: c4b688bc c4b64400 00000001 c4b64444 c41ea600 c0a4aca8 c413ac88 c4b64400
[ 7.315161] 9d00: 00000001 d0b2c62c c4b64400 c4b64400 00000000 c0a49590 c4b64400 00000000
[ 7.315263] 9d20: 00000000 c4b68800 c234b3f4 c0a47164 c41dd000 c4b64400 00000000 c422f810
[ 7.315365] 9d40: c422f810 d0b2c62c 00000000 c4b64400 c4b68800 c422f810 cbdcd260 c170bd30
[ 7.315471] 9d60: c170bd24 c0b77d4c c4b68b1c c4b64400 c4b64400 00000000 cbdcd260 c0b77e34
[ 7.315574] 9d80: c4b68800 cbdcd1fc c4b64400 c0b786e4 00000000 c1b7dac4 c4020000 c422f810
[ 7.315675] 9da0: d0949d9c 000f4240 00000000 d0b2c62c 00000000 c4b68800 ffffffed c422f800
[ 7.315777] 9dc0: c422f810 c4b68d40 c4b68800 00000000 c4020000 c0b8102c 00000000 c423c080
[ 7.315878] 9de0: c4b68d40 00000000 c423c040 00000000 00000000 d0b2c62c 00000000 c422f810
[ 7.315980] 9e00: c1baff90 00000000 c1b96f6c 00000000 c1c20420 c0a4cd34 c422f810 00000000
[ 7.316080] 9e20: c1baff90 00000000 c1b96f6c c0a4a434 c422f810 c1baff90 c422f810 00000016
[ 7.316182] 9e40: c1b96f6c c0a4a6d4 c1330f38 3ffffffb c234b44c c1baff90 c422f810 00000016
[ 7.316283] 9e60: c1b96f6c c0a4a81c 00000001 c1baff90 d0949ec4 c422f810 c1b96f6c c0a4a948
[ 7.316384] 9e80: 00000000 c40dec00 d0949ec4 c0a4a8b4 c1b96f6c c0a4849c c422e0cc c40dece4
[ 7.316486] 9ea0: c4603a58 d0b2c62c c422e0cc c422f810 00000001 c422f854 c40dec00 c0a4aca8
[ 7.316587] 9ec0: c413ac88 c422f810 00000001 d0b2c62c c423a434 c422f810 c1b96e8c c0a49590
[ 7.316689] 9ee0: c423a434 c422f810 c1b96e8c c1b96e90 c1b96f6c c0a4a1d8 c1b96f00 c429f680
[ 7.316791] 9f00: c400ce00 c4020000 c1a093d4 c400ce15 c1c20420 c0149e40 00000001 00000000
[ 7.316893] 9f20: c0149d78 c4020000 c1b96f00 c209b3cc 00000000 c16e587c 00000000 d0b2c62c
[ 7.316996] 9f40: c4020000 c429f680 c4020000 c429f6ac c4020040 c1a05d40 00000088 c41dd000
[ 7.317099] 9f60: c4020000 c014a294 00000000 c429c780 c41dd000 c014a268 c429f680 c429f780
[ 7.317201] 9f80: d0889ec4 00000000 00000000 c0154358 c429c780 c015425c 00000000 00000000
[ 7.317303] 9fa0: 00000000 00000000 00000000 c010012c 00000000 00000000 00000000 00000000
[ 7.317404] 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
[ 7.317506] 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
[ 7.317682] ads7846_probe from spi_probe+0x84/0xa8
[ 7.317785] spi_probe from really_probe+0xc4/0x2e0
[ 7.317862] really_probe from __driver_probe_device+0x84/0x1a0
[ 7.317948] __driver_probe_device from driver_probe_device+0x2c/0xc4
[ 7.318037] driver_probe_device from __device_attach_driver+0x94/0xc4
[ 7.318125] __device_attach_driver from bus_for_each_drv+0x84/0xdc
[ 7.318214] bus_for_each_drv from __device_attach+0xac/0x1d8
[ 7.318296] __device_attach from bus_probe_device+0x8c/0x90
[ 7.318377] bus_probe_device from device_add+0x560/0x774
[ 7.318455] device_add from __spi_add_device+0x58/0xe8
[ 7.318533] __spi_add_device from spi_add_device+0x58/0x8c
[ 7.318613] spi_add_device from spi_register_controller+0x784/0xbfc
[ 7.318705] spi_register_controller from spi_imx_probe+0x390/0x604
[ 7.318795] spi_imx_probe from platform_probe+0x58/0xbc
[ 7.318872] platform_probe from really_probe+0xc4/0x2e0
[ 7.318947] really_probe from __driver_probe_device+0x84/0x1a0
[ 7.319029] __driver_probe_device from driver_probe_device+0x2c/0xc4
[ 7.319116] driver_probe_device from __device_attach_driver+0x94/0xc4
[ 7.319205] __device_attach_driver from bus_for_each_drv+0x84/0xdc
[ 7.319291] bus_for_each_drv from __device_attach+0xac/0x1d8
[ 7.319372] __device_attach from bus_probe_device+0x8c/0x90
[ 7.319454] bus_probe_device from deferred_probe_work_func+0x8c/0xc0
[ 7.319542] deferred_probe_work_func from process_one_work+0x288/0x6b0
[ 7.319635] process_one_work from worker_thread+0x2c/0x480
[ 7.319715] worker_thread from kthread+0xfc/0x120
[ 7.319788] kthread from ret_from_fork+0x14/0x28
Bisect result:
# bad: [9ca10bfb8aa8fbf19ee22e702c8cf9b66ea73a54] Add linux-next specific files for 20230605
# good: [9561de3a55bed6bdd44a12820ba81ec416e705a7] Linux 6.4-rc5
git bisect start 'HEAD' 'v6.4-rc5'
# bad: [8e3a5ba24fdd336ddd8096257588f5ad5a934ffa] Merge branch 'drm-next' of git://git.freedesktop.org/git/drm/drm.git
git bisect bad 8e3a5ba24fdd336ddd8096257588f5ad5a934ffa
# bad: [376395b3a956c450a8f52a02acf0e9d7cbb77ac9] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/kdave/linux.git
git bisect bad 376395b3a956c450a8f52a02acf0e9d7cbb77ac9
# bad: [332f472c4163f902f588816c8b28a00a461ed325] Merge branch 'for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/shawnguo/linux.git
git bisect bad 332f472c4163f902f588816c8b28a00a461ed325
# good: [159f9e8420bb86f7b154acec72d93528a3f9b0f7] Merge branch 'mm-everything' of git://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm
git bisect good 159f9e8420bb86f7b154acec72d93528a3f9b0f7
# good: [dc7fd8b3a8838ce715fed6c58a90187380084582] Merge branch 'for-next/core' of git://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux
git bisect good dc7fd8b3a8838ce715fed6c58a90187380084582
# bad: [de6a558f810c3de395eaa58bce1063e24d8a78b8] Merge branch 'at91-next' of git://git.kernel.org/pub/scm/linux/kernel/git/at91/linux.git
git bisect bad de6a558f810c3de395eaa58bce1063e24d8a78b8
# good: [3ea093976845afd968dfc9cf36eb8d5ba63b7c0e] Merge branch 'soc/dt' into for-next
git bisect good 3ea093976845afd968dfc9cf36eb8d5ba63b7c0e
# bad: [b26078a2b9df1dfd725a0e171d2b0ad0259cfef1] Merge branch 'arm/fixes' into for-next
git bisect bad b26078a2b9df1dfd725a0e171d2b0ad0259cfef1
# bad: [8e0285ab95a9baf374f2c13eb152221c8ecb3f28] ARM/musb: omap2: Remove global GPIO numbers from TUSB6010
git bisect bad 8e0285ab95a9baf374f2c13eb152221c8ecb3f28
# bad: [e519f0bb64efc2c9c8b67bb2d114dda458bdc34d] ARM/mmc: Convert old mmci-omap to GPIO descriptors
git bisect bad e519f0bb64efc2c9c8b67bb2d114dda458bdc34d
# good: [4c40db6249ff1da335b276bdd6c3c3462efbc2ab] ARM: omap1: Remove reliance on GPIO numbers from PalmTE
git bisect good 4c40db6249ff1da335b276bdd6c3c3462efbc2ab
# bad: [767d83361aaa6a1ecb4d5b89eeb38a267239917a] Input: ads7846 - Convert to use software nodes
git bisect bad 767d83361aaa6a1ecb4d5b89eeb38a267239917a
# good: [480c82daa3e41873421dc2c9e2918ad7e21d7a0b] ARM: omap1: Remove reliance on GPIO numbers from SX1
git bisect good 480c82daa3e41873421dc2c9e2918ad7e21d7a0b
# first bad commit: [767d83361aaa6a1ecb4d5b89eeb38a267239917a] Input: ads7846 - Convert to use software nodes
^ permalink raw reply
* Re: [PATCH] HID: logitech-hidpp: Handle timeout differently from busy
From: Jiri Kosina @ 2023-06-05 20:03 UTC (permalink / raw)
To: Mark Lord
Cc: Benjamin Tissoires, Bastien Nocera, linux-input, linux-kernel,
Peter F . Patel-Schneider, Filipe Laíns, Nestor Lopez Casado
In-Reply-To: <8c65b3a0-6040-52e7-7e6d-1a18cbe7a22e@pobox.com>
On Mon, 5 Jun 2023, Mark Lord wrote:
> Tested-by: Mark Lord <mlord@pobox.com>
>
> That works fine for me on top of 6.3.6, and I don't even see the
> ETIMEDOUT happening there either (added a printk() for it).
>
> I am unable to test on 6.4.0-rc5, because that kernel doesn't work with
> my USB3 docking station.
Thanks a lot, Mark!
Applied and will be sending to Linus soon.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
From: cong yang @ 2023-06-06 2:06 UTC (permalink / raw)
To: Conor Dooley
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
linux-kernel
In-Reply-To: <20230605-anyway-grab-f7a35aa199fb@spud>
Hi,Conor,
On Mon, Jun 5, 2023 at 6:20 PM Conor Dooley <conor@kernel.org> wrote:
>
> Hey Cong Yang,
>
> On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
> > Add an ilitek touch screen chip ili9882t.
>
> Could you add a comment here mentioning the relationship between these
> chips?
Okay, I will add in V3 version.
> On Mon, Jun 05, 2023 at 02:05:23PM +0800, Cong Yang wrote:
>
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> > .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > index 05e6f2df604c..f0e7ffdce605 100644
> > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > @@ -15,11 +15,14 @@ description:
> >
> > properties:
> > compatible:
> > - items:
> > - - const: elan,ekth6915
> > + enum:
> > + - elan,ekth6915
> > + - ilitek,ili9882t
> >
> > reg:
> > - const: 0x10
> > + enum:
> > + - 0x10
> > + - 0x41
>
> Is 0x10 only valid for the elan,ekth6915 & 0x41 for the ilitek one?
> If so, please add some enforcement of the values based on the
> compatible.
I don't think 0x10 is the only address for ekth6915,(nor is 0x41 the
only address for ili9882t). It depends on the hardware design.
>
> >
> > interrupts:
> > maxItems: 1
> > @@ -29,11 +32,13 @@ properties:
> >
>
>
> > vcc33-supply:
> > description: The 3.3V supply to the touchscreen.
> > + If using ili9882t then this supply will not be needed.
> >
> > vccio-supply:
> > description:
> > The IO supply to the touchscreen. Need not be specified if this is the
> > same as the 3.3V supply.
> > + If using ili9882t, the IO supply is required.
>
> There's no need for these sort of comments, you can rely on the required
> sections to describe these relationships.
Got it ,thanks.
>
> Cheers,
> Conor.
>
> >
> > required:
> > - compatible
> > @@ -41,6 +46,18 @@ required:
> > - interrupts
> > - vcc33-supply
> >
> > +if:
> > + properties:
> > + compatible:
> > + contains:
> > + const: ilitek,ili9882t
> > +then:
> > + required:
> > + - compatible
> > + - reg
> > + - interrupts
> > + - vccio-supply
> > +
> > additionalProperties: false
> >
> > examples:
> > --
> > 2.25.1
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
From: cong yang @ 2023-06-06 2:18 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
linux-kernel
In-Reply-To: <6818f4b0-4222-c3bb-c55f-bc0d26d7a681@linaro.org>
Hi,Krzysztof
On Mon, Jun 5, 2023 at 6:34 PM Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 05/06/2023 08:05, Cong Yang wrote:
> > Add an ilitek touch screen chip ili9882t.
> >
> > Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
> > ---
> > .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
> > 1 file changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > index 05e6f2df604c..f0e7ffdce605 100644
> > --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
> > @@ -15,11 +15,14 @@ description:
> >
> > properties:
> > compatible:
> > - items:
> > - - const: elan,ekth6915
> > + enum:
> > + - elan,ekth6915
> > + - ilitek,ili9882t
> >
> > reg:
> > - const: 0x10
> > + enum:
> > + - 0x10
> > + - 0x41
> >
> > interrupts:
> > maxItems: 1
> > @@ -29,11 +32,13 @@ properties:
> >
> > vcc33-supply:
> > description: The 3.3V supply to the touchscreen.
> > + If using ili9882t then this supply will not be needed.
>
> What does it mean "will not be needed"? Describe the hardware, not your
> drivers.
>
> I don't think you tested your DTS. Submit DTS users, because I do not
> believe you are testing your patches. You already got such comment and I
> don't see much of improvements here.
I ran make dt_binding_check in the codebase root directory before
sending the V2 Patch, and there were no errors or warnings (the V1
version run reported some errors). Is there some other way to test DTS
?
>
> >
> > vccio-supply:
> > description:
> > The IO supply to the touchscreen. Need not be specified if this is the
> > same as the 3.3V supply.
> > + If using ili9882t, the IO supply is required.
>
> Don't repeat constraints in free form text.
Got it.thanks.
> >
> > required:
> > - compatible
> > @@ -41,6 +46,18 @@ required:
> > - interrupts
> > - vcc33-supply
> >
> > +if:
>
> Keep it in allOf. Will save you one indentation later.
Got it.thanks.
>
> > + properties:
> > + compatible:
> > + contains:
> > + const: ilitek,ili9882t
> > +then:
> > + required:
> > + - compatible
> > + - reg
> > + - interrupts
>
> Don't duplicate.
Got it.thanks.
>
> > + - vccio-supply
>
>
> Best regards,
> Krzysztof
>
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Bagas Sanjaya @ 2023-06-06 2:36 UTC (permalink / raw)
To: Malte Starostik, Benjamin Tissoires
Cc: basavaraj.natikar, linux-input, linux, regressions, stable
In-Reply-To: <3250319.ancTxkQ2z5@zen>
[-- Attachment #1: Type: text/plain, Size: 1656 bytes --]
On Mon, Jun 05, 2023 at 01:24:25PM +0200, Malte Starostik wrote:
> Hello,
>
> chiming in here as I'm experiencing what looks like the exact same issue, also
> on a Lenovo Z13 notebook, also on Arch:
> Oops during startup in task udev-worker followed by udev-worker blocking all
> attempts to suspend or cleanly shutdown/reboot the machine - in fact I first
> noticed because the machine surprised with repeatedly running out of battery
> after it had supposedly been in standby but couldn't. Only then I noticed the
> error on boot.
>
> bisect result:
> 904e28c6de083fa4834cdbd0026470ddc30676fc is the first bad commit
> commit 904e28c6de083fa4834cdbd0026470ddc30676fc
> Merge: a738688177dc 2f7f4efb9411
> Author: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Date: Wed Feb 22 10:44:31 2023 +0100
>
> Merge branch 'for-6.3/hid-bpf' into for-linus
Hmm, seems like bad bisect (bisected to HID-BPF which IMO isn't related
to amd_sfh). Can you repeat the bisection?
Anyway, tl;dr:
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
And telling regzbot:
#regzbot introduced: 904e28c6de083f
#regzbot title: HID-BPF feature causes amd_sfh kernel oops during boot and suspend/reboot
Thanks.
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Bagas Sanjaya @ 2023-06-06 2:39 UTC (permalink / raw)
To: Haochen Tong, stable,
Linux regression tracking (Thorsten Leemhuis)
Cc: regressions, linux-input, Basavaraj Natikar
In-Reply-To: <ZG3ipauL9FTnQJiC@debian.me>
[-- Attachment #1: Type: text/plain, Size: 751 bytes --]
On Wed, May 24, 2023 at 05:10:45PM +0700, Bagas Sanjaya wrote:
> On Wed, May 24, 2023 at 02:10:31PM +0800, Haochen Tong wrote:
> > > What last kernel version before this regression occurs? Do you mean
> > > v6.2?
> > >
> >
> > I was using 6.2.12 (Arch Linux distro kernel) before seeing this regression.
>
> Can you perform bisection to find the culprit that introduces the
> regression? Since you're on Arch Linux, see its wiki article [1] for
> instructions.
>
Haochen, any news on this? Has the bisection been done and any result?
Another reporter had concluded possibly bad bisect [1].
Thanks.
[1]: https://lore.kernel.org/regressions/3250319.ancTxkQ2z5@zen/
--
An old man doll... just what I always wanted! - Clara
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Haochen Tong @ 2023-06-06 3:41 UTC (permalink / raw)
To: Bagas Sanjaya, stable,
Linux regression tracking (Thorsten Leemhuis)
Cc: regressions, linux-input, Basavaraj Natikar
In-Reply-To: <ZH6cd6_8EUrRY0W0@debian.me>
On 6/6/23 10:39, Bagas Sanjaya wrote:
> On Wed, May 24, 2023 at 05:10:45PM +0700, Bagas Sanjaya wrote:
>> On Wed, May 24, 2023 at 02:10:31PM +0800, Haochen Tong wrote:
>>>> What last kernel version before this regression occurs? Do you mean
>>>> v6.2?
>>>>
>>>
>>> I was using 6.2.12 (Arch Linux distro kernel) before seeing this regression.
>>
>> Can you perform bisection to find the culprit that introduces the
>> regression? Since you're on Arch Linux, see its wiki article [1] for
>> instructions.
>>
>
> Haochen, any news on this? Has the bisection been done and any result?
> Another reporter had concluded possibly bad bisect [1].
>
> Thanks.
>
> [1]: https://lore.kernel.org/regressions/3250319.ancTxkQ2z5@zen/
>
Hi,
Sorry for the late reply. I haven't gotten enough time for it yet.
I took a look at the git logs, and it doesn't look like the modules
involved in the original stack trace (amd_sfh, hid_sensor_hub,
hid_sensor_iio_common, hid_sensor_gyro_3d) has received any significant
changes between v6.2 and v6.3. IMHO, the bisect done by Malte might
indicate that the issue could be a problem outside of these modules.
Also, I've upgrade from 6.3.3 to 6.3.5 a week ago and this issue hasn't
happened so far in 4 reboots. However, there still doesn't seem to be
any changes regarding these modules, so I'm not sure if it's fixed
elsewhere or I'm just being lucky. It would be nice if someone can
confirm or disprove this.
Thanks,
^ permalink raw reply
* Re: [PATCH v2] selftests/input: Introduce basic tests for evdev ioctls
From: Peter Hutterer @ 2023-06-06 5:04 UTC (permalink / raw)
To: Enric Balletbo i Serra
Cc: linux-kselftest, linux-kernel, Shuah Khan, Dmitry Torokhov,
Javier Martinez Canillas, Dana Elfassy, linux-input, phuttere,
Benjamin Tissoires
In-Reply-To: <20230530102627.87284-1-eballetbo@kernel.org>
On Tue, May 30, 2023 at 12:26:27PM +0200, Enric Balletbo i Serra wrote:
> This provides a basic infrastructure for the creation of tests for the evdev
> interface. Most of this code is adapted from the libevdev wrapper library. While
> most of evdev ioctls are covered and tested using libevdev tests there are some
> evdev ioctls that aren't because are not supported (and will not be supported)
> by libevdev [1]. So, adding, at least those tests, would make sense.
>
> The test creates an uinput device (and an evdev device) so you can
> call the wanted ioctl from userspace. So, to run those tests you need
> to have support for uinput and evdev as well.
>
> [1] For example, libevdev doesn't support setting EV_REP because it's inherently
> racy - one libevdev context to set those values via the ioctl would cause all
> other libevdev contexts on the same device to be out of sync. Since we do not
> get notifications when the values changed, libevdev's buffered values for EV_REP
> will remain whatever they were initially.
>
> Signed-off-by: Enric Balletbo i Serra <eballetbo@kernel.org>
thanks, this mostly LGTM but there's still a bug left in the vararg
handling.
[...]
> +#include <dirent.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/uinput.h>
> +#include <poll.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include "../kselftest_harness.h"
> +
> +#define TEST_DEVICE_NAME "selftest input device"
> +
> +struct selftest_uinput {
> + int uinput_fd; /** file descriptor to uinput */
> + int evdev_fd; /** file descriptor to evdev */
> + char *name; /** device name */
> + char *syspath; /** /sys path */
> + char *devnode; /** device node */
nitpick: none of name, syspath, devnode are used in the tests and it's
likely they'll never need to be so there's no reason to strdup them
here. You could change fetch_syspath_and_devnode() to open_devnode() and
return the opened fd, meaning you can reduce the code even more.
[...]
> +
> +TEST(eviocgname_get_device_name)
> +{
> + struct selftest_uinput *uidev;
> + char buf[256];
> + int rc;
> +
> + rc = selftest_uinput_create_device(&uidev);
this one and the others without extra arguments need to be:
rc = selftest_uinput_create_device(&uidev, -1);
otherwise the vararg loop is going to keep the room warm for no reason.
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: input: touchscreen: Add ilitek 9882T touchscreen chip
From: Krzysztof Kozlowski @ 2023-06-06 6:21 UTC (permalink / raw)
To: cong yang
Cc: robh+dt, krzysztof.kozlowski+dt, conor+dt, dmitry.torokhov, jikos,
benjamin.tissoires, dianders, hsinyi, linux-input, devicetree,
linux-kernel
In-Reply-To: <CAHwB_NLOrecxxP0kGS2ycXAw93XOSOiR+qPo50D292tDNKiqQQ@mail.gmail.com>
On 06/06/2023 04:18, cong yang wrote:
> Hi,Krzysztof
>
> On Mon, Jun 5, 2023 at 6:34 PM Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 05/06/2023 08:05, Cong Yang wrote:
>>> Add an ilitek touch screen chip ili9882t.
>>>
>>> Signed-off-by: Cong Yang <yangcong5@huaqin.corp-partner.google.com>
>>> ---
>>> .../bindings/input/elan,ekth6915.yaml | 23 ++++++++++++++++---
>>> 1 file changed, 20 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
>>> index 05e6f2df604c..f0e7ffdce605 100644
>>> --- a/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
>>> +++ b/Documentation/devicetree/bindings/input/elan,ekth6915.yaml
>>> @@ -15,11 +15,14 @@ description:
>>>
>>> properties:
>>> compatible:
>>> - items:
>>> - - const: elan,ekth6915
>>> + enum:
>>> + - elan,ekth6915
>>> + - ilitek,ili9882t
>>>
>>> reg:
>>> - const: 0x10
>>> + enum:
>>> + - 0x10
>>> + - 0x41
>>>
>>> interrupts:
>>> maxItems: 1
>>> @@ -29,11 +32,13 @@ properties:
>>>
>>> vcc33-supply:
>>> description: The 3.3V supply to the touchscreen.
>>> + If using ili9882t then this supply will not be needed.
>>
>> What does it mean "will not be needed"? Describe the hardware, not your
>> drivers.
>>
>> I don't think you tested your DTS. Submit DTS users, because I do not
>> believe you are testing your patches. You already got such comment and I
>> don't see much of improvements here.
>
> I ran make dt_binding_check in the codebase root directory before
> sending the V2 Patch, and there were no errors or warnings (the V1
> version run reported some errors). Is there some other way to test DTS
> ?
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
Best regards,
Krzysztof
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Linux regression tracking (Thorsten Leemhuis) @ 2023-06-06 6:56 UTC (permalink / raw)
To: Bagas Sanjaya, Malte Starostik, Benjamin Tissoires
Cc: basavaraj.natikar, linux-input, linux, regressions, stable
In-Reply-To: <ZH6buB8TcMd5aT_1@debian.me>
On 06.06.23 04:36, Bagas Sanjaya wrote:
> On Mon, Jun 05, 2023 at 01:24:25PM +0200, Malte Starostik wrote:
>> Hello,
>>
>> chiming in here as I'm experiencing what looks like the exact same issue, also
>> on a Lenovo Z13 notebook, also on Arch:
>> Oops during startup in task udev-worker followed by udev-worker blocking all
>> attempts to suspend or cleanly shutdown/reboot the machine - in fact I first
>> noticed because the machine surprised with repeatedly running out of battery
>> after it had supposedly been in standby but couldn't. Only then I noticed the
>> error on boot.
>>
>> bisect result:
>> 904e28c6de083fa4834cdbd0026470ddc30676fc is the first bad commit
>> commit 904e28c6de083fa4834cdbd0026470ddc30676fc
>> Merge: a738688177dc 2f7f4efb9411
>> Author: Benjamin Tissoires <benjamin.tissoires@redhat.com>
>> Date: Wed Feb 22 10:44:31 2023 +0100
>>
>> Merge branch 'for-6.3/hid-bpf' into for-linus
>
> Hmm, seems like bad bisect (bisected to HID-BPF which IMO isn't related
> to amd_sfh). Can you repeat the bisection?
Well, amd_sfh afaics apparently interacts with HID (see trace earlier in
the thread), so it's not that far away. But it's a merge commit, which
is possible, but doesn't happen every day. So a recheck might really be
a good idea.
> Anyway, tl;dr:
>
>> A: http://en.wikipedia.org/wiki/Top_post
>> Q: Were do I find info about this thing called top-posting?
> [...]
BTW, I'm not sure if this really is helpful. Teaching this to upcoming
kernel developers is definitely worth it, but I wonder if pushing this
on all reporters might do more harm than good. I also wonder if asking
them a bit more kindly might be wiser (e.g. instead of "Anyway, tl;dr:"
something like "BTW, please do not top-post:" or something like that maybe).
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.
^ permalink raw reply
* Re: [PATCH v2] selftests/input: Introduce basic tests for evdev ioctls
From: Muhammad Usama Anjum @ 2023-06-06 8:08 UTC (permalink / raw)
To: Enric Balletbo i Serra, linux-kselftest, linux-kernel
Cc: Muhammad Usama Anjum, Shuah Khan, Dmitry Torokhov,
Javier Martinez Canillas, Dana Elfassy, linux-input, phuttere,
Benjamin Tissoires
In-Reply-To: <20230530102627.87284-1-eballetbo@kernel.org>
On 5/30/23 3:26 PM, Enric Balletbo i Serra wrote:
> This provides a basic infrastructure for the creation of tests for the evdev
> interface. Most of this code is adapted from the libevdev wrapper library. While
> most of evdev ioctls are covered and tested using libevdev tests there are some
> evdev ioctls that aren't because are not supported (and will not be supported)
> by libevdev [1]. So, adding, at least those tests, would make sense.
>
> The test creates an uinput device (and an evdev device) so you can
> call the wanted ioctl from userspace. So, to run those tests you need
> to have support for uinput and evdev as well.
>
> [1] For example, libevdev doesn't support setting EV_REP because it's inherently
> racy - one libevdev context to set those values via the ioctl would cause all
> other libevdev contexts on the same device to be out of sync. Since we do not
> get notifications when the values changed, libevdev's buffered values for EV_REP
> will remain whatever they were initially.
>
> Signed-off-by: Enric Balletbo i Serra <eballetbo@kernel.org>
Acked-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> ---
> Test output:
>
> TAP version 13
> 1..3
> # Starting 3 tests from 1 test cases.
> # RUN global.eviocgname_get_device_name ...
> # OK global.eviocgname_get_device_name
> ok 1 global.eviocgname_get_device_name
> # RUN global.eviocgrep_get_repeat_settings ...
> # OK global.eviocgrep_get_repeat_settings
> ok 2 global.eviocgrep_get_repeat_settings
> # RUN global.eviocsrep_set_repeat_settings ...
> # OK global.eviocsrep_set_repeat_settings
> ok 3 global.eviocsrep_set_repeat_settings
> # PASSED: 3 / 3 tests passed.
> # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> tools/testing/selftests/Makefile | 1 +
> tools/testing/selftests/input/.gitignore | 2 +
> tools/testing/selftests/input/Makefile | 5 +
> tools/testing/selftests/input/config | 3 +
> tools/testing/selftests/input/evioc-test.c | 259 +++++++++++++++++++++
> 5 files changed, 270 insertions(+)
> create mode 100644 tools/testing/selftests/input/.gitignore
> create mode 100644 tools/testing/selftests/input/Makefile
> create mode 100644 tools/testing/selftests/input/config
> create mode 100644 tools/testing/selftests/input/evioc-test.c
>
> diff --git a/tools/testing/selftests/Makefile b/tools/testing/selftests/Makefile
> index 90a62cf75008..29fc77168aa7 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -28,6 +28,7 @@ TARGETS += futex
> TARGETS += gpio
> TARGETS += hid
> TARGETS += intel_pstate
> +TARGETS += input
> TARGETS += iommu
> TARGETS += ipc
> TARGETS += ir
> diff --git a/tools/testing/selftests/input/.gitignore b/tools/testing/selftests/input/.gitignore
> new file mode 100644
> index 000000000000..37f5dff3255b
> --- /dev/null
> +++ b/tools/testing/selftests/input/.gitignore
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +evioc-test
> diff --git a/tools/testing/selftests/input/Makefile b/tools/testing/selftests/input/Makefile
> new file mode 100644
> index 000000000000..031729be0628
> --- /dev/null
> +++ b/tools/testing/selftests/input/Makefile
> @@ -0,0 +1,5 @@
> +# SPDX-License-Identifier: GPL-2.0
> +CFLAGS += -D_GNU_SOURCE -std=gnu99 -Wl,-no-as-needed -Wall $(KHDR_INCLUDES)
> +
> +TEST_GEN_PROGS := evioc-test
> +include ../lib.mk
> diff --git a/tools/testing/selftests/input/config b/tools/testing/selftests/input/config
> new file mode 100644
> index 000000000000..b7512f3e6d8d
> --- /dev/null
> +++ b/tools/testing/selftests/input/config
> @@ -0,0 +1,3 @@
> +CONFIG_INPUT=y
> +CONFIG_INPUT_EVDEV=y
> +CONFIG_INPUT_UINPUT=m
> diff --git a/tools/testing/selftests/input/evioc-test.c b/tools/testing/selftests/input/evioc-test.c
> new file mode 100644
> index 000000000000..1b951d41ee47
> --- /dev/null
> +++ b/tools/testing/selftests/input/evioc-test.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Red Hat, Inc.
> + *
> + * Part of the code in this file is inspired and copied from the libevdev wrapper library
> + * for evdev devices written by Peter Hutterer.
> + */
> +
> +#include <dirent.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <linux/uinput.h>
> +#include <poll.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <time.h>
> +#include <unistd.h>
> +
> +#include "../kselftest_harness.h"
> +
> +#define TEST_DEVICE_NAME "selftest input device"
> +
> +struct selftest_uinput {
> + int uinput_fd; /** file descriptor to uinput */
> + int evdev_fd; /** file descriptor to evdev */
> + char *name; /** device name */
> + char *syspath; /** /sys path */
> + char *devnode; /** device node */
> +};
> +
> +static int is_event_device(const struct dirent *dent)
> +{
> + return strncmp("event", dent->d_name, 5) == 0;
> +}
> +
> +static char *fetch_device_node(const char *path)
> +{
> + struct dirent **namelist;
> + char *devnode = NULL;
> + int ndev, i;
> +
> + ndev = scandir(path, &namelist, is_event_device, alphasort);
> + if (ndev <= 0)
> + return NULL;
> +
> + /* ndev should only ever be 1 */
> +
> + for (i = 0; i < ndev; i++) {
> + if (!devnode && asprintf(&devnode, "/dev/input/%s",
> + namelist[i]->d_name) == -1)
> + devnode = NULL;
> + free(namelist[i]);
> + }
> +
> + free(namelist);
> +
> + return devnode;
> +}
> +
> +static int fetch_syspath_and_devnode(struct selftest_uinput *uidev)
> +{
> +#define SYS_INPUT_DIR "/sys/devices/virtual/input/"
> + char buf[sizeof(SYS_INPUT_DIR) + 64] = SYS_INPUT_DIR;
> + int rc;
> +
> + rc = ioctl(uidev->uinput_fd,
> + UI_GET_SYSNAME(sizeof(buf) - strlen(SYS_INPUT_DIR)),
> + &buf[strlen(SYS_INPUT_DIR)]);
> + if (rc == -1) {
> + fprintf(stderr, "cannot get the sysfs name of the uinput device (%d)\n", rc);
> + return rc;
> + }
> +
> + uidev->syspath = strdup(buf);
> + uidev->devnode = fetch_device_node(buf);
> +
> + return uidev->devnode ? 0 : -1;
> +#undef SYS_INPUT_DIR
> +}
> +
> +static void selftest_uinput_destroy(struct selftest_uinput *uidev)
> +{
> + if (!uidev)
> + return;
> +
> + if (uidev->uinput_fd >= 0)
> + ioctl(uidev->uinput_fd, UI_DEV_DESTROY, NULL);
> +
> + close(uidev->evdev_fd);
> + close(uidev->uinput_fd);
> +
> + free(uidev->syspath);
> + free(uidev->devnode);
> + free(uidev->name);
> + free(uidev);
> +}
> +
> +static int selftest_uinput_create_device(struct selftest_uinput **uidev, ...)
> +{
> + struct selftest_uinput *new_device;
> + struct uinput_setup setup;
> + va_list args;
> + int rc, fd;
> + int type;
> +
> + new_device = calloc(1, sizeof(struct selftest_uinput));
> + if (!new_device)
> + return -ENOMEM;
> +
> + memset(&setup, 0, sizeof(setup));
> + strncpy(setup.name, TEST_DEVICE_NAME, UINPUT_MAX_NAME_SIZE - 1);
> + setup.id.vendor = 0x1234; /* sample vendor */
> + setup.id.product = 0x5678; /* sample product */
> + setup.id.bustype = BUS_USB;
> +
> + fd = open("/dev/uinput", O_WRONLY | O_NONBLOCK);
> + if (fd < 0) {
> + fprintf(stderr, "cannot open uinput (%d): %m\n", errno);
> + goto error;
> + }
> +
> + va_start(args, uidev);
> + rc = 0;
> + do {
> + type = va_arg(args, int);
> + if (type == -1)
> + break;
> + rc = ioctl(fd, UI_SET_EVBIT, type);
> + } while (rc == 0);
> + va_end(args);
> +
> + rc = ioctl(fd, UI_DEV_SETUP, &setup);
> + if (rc == -1)
> + goto error;
> +
> + rc = ioctl(fd, UI_DEV_CREATE, NULL);
> + if (rc == -1)
> + goto error;
> +
> + new_device->name = strdup(TEST_DEVICE_NAME);
> + new_device->uinput_fd = fd;
> +
> + if (fetch_syspath_and_devnode(new_device) == -1) {
> + fprintf(stderr, "unable to fetch syspath or device node.\n");
> + errno = ENODEV;
> + goto error;
> + }
> +
> + fd = open(new_device->devnode, O_RDONLY);
> + if (fd < 0) {
> + fprintf(stderr, "cannot open uinput (%d): %m\n", errno);
> + goto error;
> + }
> + new_device->evdev_fd = fd;
> +
> + *uidev = new_device;
> +
> + return 0;
> +
> +error:
> + rc = -errno;
> + selftest_uinput_destroy(new_device);
> + return rc;
> +}
> +
> +TEST(eviocgname_get_device_name)
> +{
> + struct selftest_uinput *uidev;
> + char buf[256];
> + int rc;
> +
> + rc = selftest_uinput_create_device(&uidev);
> + ASSERT_EQ(0, rc);
> + ASSERT_NE(NULL, uidev);
> +
> + memset(buf, 0, sizeof(buf));
> + /* ioctl to get the name */
> + rc = ioctl(uidev->evdev_fd, EVIOCGNAME(sizeof(buf) - 1), buf);
> + ASSERT_GE(rc, 0);
> + ASSERT_STREQ(TEST_DEVICE_NAME, buf);
> +
> + selftest_uinput_destroy(uidev);
> +}
> +
> +TEST(eviocgrep_get_repeat_settings)
> +{
> + struct selftest_uinput *uidev;
> + int rep_values[2];
> + int rc;
> +
> + memset(rep_values, 0, sizeof(rep_values));
> +
> + rc = selftest_uinput_create_device(&uidev);
> + ASSERT_EQ(0, rc);
> + ASSERT_NE(NULL, uidev);
> +
> + /* ioctl to get the repeat rates values */
> + rc = ioctl(uidev->evdev_fd, EVIOCGREP, rep_values);
> + /* should fail because EV_REP is not set */
> + ASSERT_EQ(-1, rc);
> +
> + selftest_uinput_destroy(uidev);
> +
> + rc = selftest_uinput_create_device(&uidev, EV_REP, -1);
> + ASSERT_EQ(0, rc);
> + ASSERT_NE(NULL, uidev);
> +
> + /* ioctl to get the repeat rates values */
> + rc = ioctl(uidev->evdev_fd, EVIOCGREP, rep_values);
> + ASSERT_EQ(0, rc);
> + /* should get the default delay and period values set by the kernel */
> + ASSERT_EQ(rep_values[0], 250);
> + ASSERT_EQ(rep_values[1], 33);
> +
> + selftest_uinput_destroy(uidev);
> +}
> +
> +TEST(eviocsrep_set_repeat_settings)
> +{
> + struct selftest_uinput *uidev;
> + int rep_values[2];
> + int rc;
> +
> + memset(rep_values, 0, sizeof(rep_values));
> +
> + rc = selftest_uinput_create_device(&uidev);
> + ASSERT_EQ(0, rc);
> + ASSERT_NE(NULL, uidev);
> +
> + /* ioctl to set the repeat rates values */
> + rc = ioctl(uidev->evdev_fd, EVIOCSREP, rep_values);
> + /* should fail because EV_REP is not set */
> + ASSERT_EQ(-1, rc);
> +
> + selftest_uinput_destroy(uidev);
> +
> + rc = selftest_uinput_create_device(&uidev, EV_REP, -1);
> + ASSERT_EQ(0, rc);
> + ASSERT_NE(NULL, uidev);
> +
> + /* ioctl to set the repeat rates values */
> + rep_values[0] = 500;
> + rep_values[1] = 200;
> + rc = ioctl(uidev->evdev_fd, EVIOCSREP, rep_values);
> + ASSERT_EQ(0, rc);
> +
> + /* ioctl to get the repeat rates values */
> + rc = ioctl(uidev->evdev_fd, EVIOCGREP, rep_values);
> + ASSERT_EQ(0, rc);
> + /* should get the delay and period values set previously */
> + ASSERT_EQ(rep_values[0], 500);
> + ASSERT_EQ(rep_values[1], 200);
> +
> + selftest_uinput_destroy(uidev);
> +}
> +
> +TEST_HARNESS_MAIN
--
BR,
Muhammad Usama Anjum
^ permalink raw reply
* Re: amd_sfh driver causes kernel oops during boot
From: Benjamin Tissoires @ 2023-06-06 8:08 UTC (permalink / raw)
To: Linux regressions mailing list
Cc: Bagas Sanjaya, Malte Starostik, basavaraj.natikar, linux-input,
linux, stable, Mario Limonciello
In-Reply-To: <46d5acd2-5961-a496-4c1a-68e5c47021fb@leemhuis.info>
On Jun 06 2023, Linux regression tracking (Thorsten Leemhuis) wrote:
>
> On 06.06.23 04:36, Bagas Sanjaya wrote:
> > On Mon, Jun 05, 2023 at 01:24:25PM +0200, Malte Starostik wrote:
> >> Hello,
> >>
> >> chiming in here as I'm experiencing what looks like the exact same issue, also
> >> on a Lenovo Z13 notebook, also on Arch:
> >> Oops during startup in task udev-worker followed by udev-worker blocking all
> >> attempts to suspend or cleanly shutdown/reboot the machine - in fact I first
> >> noticed because the machine surprised with repeatedly running out of battery
> >> after it had supposedly been in standby but couldn't. Only then I noticed the
> >> error on boot.
> >>
> >> bisect result:
> >> 904e28c6de083fa4834cdbd0026470ddc30676fc is the first bad commit
> >> commit 904e28c6de083fa4834cdbd0026470ddc30676fc
> >> Merge: a738688177dc 2f7f4efb9411
> >> Author: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >> Date: Wed Feb 22 10:44:31 2023 +0100
> >>
> >> Merge branch 'for-6.3/hid-bpf' into for-linus
> >
> > Hmm, seems like bad bisect (bisected to HID-BPF which IMO isn't related
> > to amd_sfh). Can you repeat the bisection?
>
> Well, amd_sfh afaics apparently interacts with HID (see trace earlier in
> the thread), so it's not that far away. But it's a merge commit, which
> is possible, but doesn't happen every day. So a recheck might really be
> a good idea.
Let's not rule out that there is a bad interaction between HID-BPF and
AMD SFH. HID-BPF is able to process any incoming HID event, whether it
comes from AND SFH, USB, BT, I2C or anything else.
However, looking at the stack trace in the initial report[0], it seems
we are getting the oops/stack traces while we are still in amd_sfh:
list_add corruption. next is NULL.
WARNING: CPU: 5 PID: 433 at lib/list_debug.c:25 __list_add_valid+0x57/0xa0
...
RIP: 0010:__list_add_valid+0x57/0xa0
...
Call Trace:
<TASK>
amd_sfh_get_report+0xba/0x110 [amd_sfh 78bf82e66cdb2ccf24cbe871a0835ef4eedddb17]
...
If HID-BPF were involved, we should see a call to hid_input_report() IMO.
Also AMD SFH calls hid_input_report() in a workqueue, so I would expect
a different stack trace.
I have a suspicion on commit 7bcfdab3f0c6 ("HID: amd_sfh: if no sensors are enabled,
clean up") because the stack trace says that there is a bad list_add,
which could happen if the object is not correctly initialized.
However, that commit was present in v6.2, so it might not be that one.
Back to the merge commit: the hid-bpf tree was merged in the hid tree
while it took its branch during the v6.1 cycle. So that might be the
reason you get this as a result of bisection because the AMD SFH code in
the hid-bpf branch is the one from the v6.1 kernel, and when you merge
it to the v6.2+ branch, you get a different code for that driver.
Cheers,
Benjamin
[0] https://lore.kernel.org/regressions/f40e3897-76f1-2cd0-2d83-e48d87130eab@hexchain.org/#t
^ permalink raw reply
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