* Re: [PATCH 2/2] Input: uinput - Release mutex while unregistering input device
From: Dmitry Torokhov @ 2023-12-08 19:58 UTC (permalink / raw)
To: Vicki Pfau; +Cc: linux-input
In-Reply-To: <20231207063406.556770-3-vi@endrift.com>
Hi Vicki,
On Wed, Dec 06, 2023 at 10:34:06PM -0800, Vicki Pfau wrote:
> Any pending requests may be holding a mutex from its own subsystem, e.g.
> evdev, while waiting to be able to claim the uinput device mutex.
> However, unregistering the device may try to claim that mutex, leading
> to a deadlock. To prevent this from happening, we need to temporarily
> give up the lock before calling input_unregister_device.
I do not think we can simply give up the lock, the whole thing with
UI_DEV_DESTROY allowing reusing connection to create a new input device
was a huge mistake because if you try to do UI_DEV_CREATE again on
the same fd you'll end up reusing whatever is in udev instance,
including the state and the mutex, and will make a huge mess.
I think the only reasonable way forward is change the driver so that no
ioctls are accepted after UI_DEV_DESTROY and then start untangling the
locking issues (possibly by dropping the lock on destroy after setting
the status - I think you will not observe the lockups you mention if
your application will stop using UI_DEV_DESTROY and simply closes the
fd).
>
> Fixes: e8b95728f724 ("Input: uinput - avoid FF flush when destroying device")
This is not the commit that introduced the problem, it has been there
since forever.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: uinput - Allow uinput_request_submit wait interrupting
From: Dmitry Torokhov @ 2023-12-08 19:32 UTC (permalink / raw)
To: Vicki Pfau; +Cc: linux-input
In-Reply-To: <20231207063406.556770-2-vi@endrift.com>
Hi Vicki,
On Wed, Dec 06, 2023 at 10:34:05PM -0800, Vicki Pfau wrote:
> Currently, uinput_request_submit will only fail if the request wait times out.
> However, in other places this wait is interruptable, and in this specific
> location it can lead to issues, such as causing system suspend to hang until
> the request times out.
Could you please explain how a sleeping process can cause suspend to
hang?
> Since the timeout is so long, this can cause the
> appearance of a total system freeze. Making the wait interruptable resolves
> this and possibly further issues.
I think you are trying to find a justification too hard and it does not
make sense, however I agree that allowing to kill the process issuing
the request without waiting for the timeout to expire if the other side
is stuck might be desirable.
I think the best way to use wait_for_completion_killable_timeout()
so that stray signals do not disturb userspace, but the processes can
still be terminated.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] hwmon: (corsair-psu) Fix failure to load when built-in to kernel
From: Guenter Roeck @ 2023-12-08 18:34 UTC (permalink / raw)
To: Aleksa Savic, Wilken Gottwalt
Cc: linux-hwmon, Jean Delvare, linux-kernel, linux-input, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <fe16b501-759a-492b-9835-83ebf8761ae8@gmail.com>
On 12/8/23 08:52, Aleksa Savic wrote:
> On 2023-12-08 16:43:07 GMT+01:00, Guenter Roeck wrote:
>> On 12/8/23 05:57, Wilken Gottwalt wrote:
>>> On Fri, 8 Dec 2023 14:11:44 +0100
>>> Aleksa Savic <savicaleksa83@gmail.com> wrote:
>>>
>>>> On 2023-12-08 14:07:10 GMT+01:00, Aleksa Savic wrote:
>>>>> When built-in to the kernel, the corsair-psu driver fails to register with
>>>>> the following message:
>>>>>
>>>>> "Driver 'corsair-psu' was unable to register with bus_type 'hid'
>>>>> because the bus was not initialized."
>>>>>
>>>>> Fix this by initializing the driver after the HID bus using
>>>>> late_initcall(), as hwmon is built before HID.
>>>>>
>>>>> Fixes: d115b51e0e56 ("hwmon: add Corsair PSU HID controller driver")
>>>>> Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
>>>>> ---
>>>>> drivers/hwmon/corsair-psu.c | 15 ++++++++++++++-
>>>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
>>>>> index 904890598c11..48831a528965 100644
>>>>> --- a/drivers/hwmon/corsair-psu.c
>>>>> +++ b/drivers/hwmon/corsair-psu.c
>>>>> @@ -899,7 +899,20 @@ static struct hid_driver corsairpsu_driver = {
>>>>> .reset_resume = corsairpsu_resume,
>>>>> #endif
>>>>> };
>>>>> -module_hid_driver(corsairpsu_driver);
>>>>> +
>>>>> +static int __init corsairpsu_hid_init(void)
>>>>> +{
>>>>> + return hid_register_driver(&corsairpsu_driver);
>>>>> +}
>>>>> +
>>>>> +static void __exit corsairpsu_hid_exit(void)
>>>>> +{
>>>>> + hid_unregister_driver(&corsairpsu_driver);
>>>>> +}
>>>>> +
>>>>> +/* When compiled into the kernel, initialize after the hid bus */
>>>>> +late_initcall(corsairpsu_hid_init);
>>>>> +module_exit(corsairpsu_hid_exit);
>>>>> MODULE_LICENSE("GPL");
>>>>> MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
>>>>
>>>>
>>>> Woops! Just saw that the same fix was sent yesterday. Please disregard, sorry!
>>>>
>>>> Aleksa
>>>
>>> It is fine. I just start to wonder if there was a change in the subsystem. I
>>> used the driver as built-in in the past for several months and never had that
>>> issue. And now it is a real flood of reports.
>>>
>>
>> Maybe there was a change in the build order, or some subtle change
>> in driver registration code. Question though is _when_ this changed.
>> It would be great if someone could bisect it. For example, bus registration
>> code has been changed significantly in v6.3. I am copying linux-input
>> and the hid maintainers for feedback.
>
> The late_initcall() was also needed in 2020. when corsair-cpro was added in
> 40c3a4454225 ("hwmon: add Corsair Commander Pro driver").
>
> There was also discussion on the list about it:
>
> https://lore.kernel.org/all/3864498.z6qT3ff8q6@marius/
>
> nzxt-smart2 (from the tail of 2021.) also has a comment about the message.
>
> (Just providing references.)
>
Ah yes, thanks for the reminder. We actually had a similar problem with
watchdog drivers and worked around it by deferring watchdog driver registration
until the watchdog core is initialized. That is of course just a kludge.
No idea if a clean(er) solution is even possible.
Guenter
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: i8042: Add a quirk for Framework 16" laptop
From: Mario Limonciello @ 2023-12-08 18:22 UTC (permalink / raw)
To: Rahul Rameshbabu, Dmitry Torokhov; +Cc: linux-input, linux-kernel
In-Reply-To: <87y1e4o9ce.fsf@nvidia.com>
On 12/8/2023 12:08, Rahul Rameshbabu wrote:
> On Fri, 08 Dec, 2023 17:57:05 +0000 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
>> On Wed, Dec 06, 2023 at 03:21:40PM -0600, Mario Limonciello wrote:
>>> The Framework 16" laptop doesn't have a PS/2 keyboard. At bootup the
>>> following messages are emitted:
>>>
>>> i8042: PNP: No PS/2 controller found.
>>> i8042: PNP: Probing ports directly.
>>> i8042: Can't read CTR while initializing i8042
>>> i8042: probe of i8042 failed with error -5
>>>
>>> There are no PNP devices as those listed in `pnp_kbd_devids` but
>>> i8042_pnp_init() ignores this and still runs and will continue to
>>> try to probe.
>>>
>>> As there is no PS/2 keyboard or mouse in this laptop, set a quirk
>>> to avoid this behavior.
>>
>> I believe the proper fix for this is for the firmware not report i8042
>> as present by properly setting up FADT. Please take a look at
>> arch/x86/kernel/acpi/boot.c::acpi_parse_fadt() and how it sets flag
>> X86_LEGACY_I8042_FIRMWARE_ABSENT.
>
> This is along the lines of a point I brought up in the v1 of this patch.
>
> https://lore.kernel.org/linux-input/87v89bgl7a.fsf@nvidia.com/
>
> This means that Framework as a manufacturer will need to provide the
> appropriate fix for what's advertised over ACPI by the device. I think
> that makes sense instead of creating quirk combinations to avoid
> resolving the issue at the ACPI level. I guess the only de-merit is that
> means folks need to depend on vendors with no way to suppress this if a
> vendor does not correct set up FADT.
Got it, thanks guys. I'll raise this with them.
>
>>
>> It will still say "PNP: No PS/2 controller found" which is an
>> "informational" message, but should not try to probe ports directly and
>> report errors.
>
> --
> Thanks,
>
> Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: i8042: Add a quirk for Framework 16" laptop
From: Rahul Rameshbabu @ 2023-12-08 18:08 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Mario Limonciello, linux-input, linux-kernel
In-Reply-To: <ZXNY8a_Zja9rSupQ@google.com>
On Fri, 08 Dec, 2023 17:57:05 +0000 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote:
> On Wed, Dec 06, 2023 at 03:21:40PM -0600, Mario Limonciello wrote:
>> The Framework 16" laptop doesn't have a PS/2 keyboard. At bootup the
>> following messages are emitted:
>>
>> i8042: PNP: No PS/2 controller found.
>> i8042: PNP: Probing ports directly.
>> i8042: Can't read CTR while initializing i8042
>> i8042: probe of i8042 failed with error -5
>>
>> There are no PNP devices as those listed in `pnp_kbd_devids` but
>> i8042_pnp_init() ignores this and still runs and will continue to
>> try to probe.
>>
>> As there is no PS/2 keyboard or mouse in this laptop, set a quirk
>> to avoid this behavior.
>
> I believe the proper fix for this is for the firmware not report i8042
> as present by properly setting up FADT. Please take a look at
> arch/x86/kernel/acpi/boot.c::acpi_parse_fadt() and how it sets flag
> X86_LEGACY_I8042_FIRMWARE_ABSENT.
This is along the lines of a point I brought up in the v1 of this patch.
https://lore.kernel.org/linux-input/87v89bgl7a.fsf@nvidia.com/
This means that Framework as a manufacturer will need to provide the
appropriate fix for what's advertised over ACPI by the device. I think
that makes sense instead of creating quirk combinations to avoid
resolving the issue at the ACPI level. I guess the only de-merit is that
means folks need to depend on vendors with no way to suppress this if a
vendor does not correct set up FADT.
>
> It will still say "PNP: No PS/2 controller found" which is an
> "informational" message, but should not try to probe ports directly and
> report errors.
--
Thanks,
Rahul Rameshbabu
^ permalink raw reply
* Re: [PATCH v2 2/2] Input: i8042: Add a quirk for Framework 16" laptop
From: Dmitry Torokhov @ 2023-12-08 17:57 UTC (permalink / raw)
To: Mario Limonciello; +Cc: linux-input, linux-kernel, rrameshbabu
In-Reply-To: <20231206212140.7458-2-mario.limonciello@amd.com>
On Wed, Dec 06, 2023 at 03:21:40PM -0600, Mario Limonciello wrote:
> The Framework 16" laptop doesn't have a PS/2 keyboard. At bootup the
> following messages are emitted:
>
> i8042: PNP: No PS/2 controller found.
> i8042: PNP: Probing ports directly.
> i8042: Can't read CTR while initializing i8042
> i8042: probe of i8042 failed with error -5
>
> There are no PNP devices as those listed in `pnp_kbd_devids` but
> i8042_pnp_init() ignores this and still runs and will continue to
> try to probe.
>
> As there is no PS/2 keyboard or mouse in this laptop, set a quirk
> to avoid this behavior.
I believe the proper fix for this is for the firmware not report i8042
as present by properly setting up FADT. Please take a look at
arch/x86/kernel/acpi/boot.c::acpi_parse_fadt() and how it sets flag
X86_LEGACY_I8042_FIRMWARE_ABSENT.
It will still say "PNP: No PS/2 controller found" which is an
"informational" message, but should not try to probe ports directly and
report errors.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] hwmon: (corsair-psu) Fix failure to load when built-in to kernel
From: Aleksa Savic @ 2023-12-08 16:52 UTC (permalink / raw)
To: Guenter Roeck, Wilken Gottwalt
Cc: savicaleksa83, linux-hwmon, Jean Delvare, linux-kernel,
linux-input, Jiri Kosina, Benjamin Tissoires
In-Reply-To: <b2b3f17d-0f86-4d40-a471-f44153efd6fe@roeck-us.net>
On 2023-12-08 16:43:07 GMT+01:00, Guenter Roeck wrote:
> On 12/8/23 05:57, Wilken Gottwalt wrote:
>> On Fri, 8 Dec 2023 14:11:44 +0100
>> Aleksa Savic <savicaleksa83@gmail.com> wrote:
>>
>>> On 2023-12-08 14:07:10 GMT+01:00, Aleksa Savic wrote:
>>>> When built-in to the kernel, the corsair-psu driver fails to register with
>>>> the following message:
>>>>
>>>> "Driver 'corsair-psu' was unable to register with bus_type 'hid'
>>>> because the bus was not initialized."
>>>>
>>>> Fix this by initializing the driver after the HID bus using
>>>> late_initcall(), as hwmon is built before HID.
>>>>
>>>> Fixes: d115b51e0e56 ("hwmon: add Corsair PSU HID controller driver")
>>>> Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
>>>> ---
>>>> drivers/hwmon/corsair-psu.c | 15 ++++++++++++++-
>>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
>>>> index 904890598c11..48831a528965 100644
>>>> --- a/drivers/hwmon/corsair-psu.c
>>>> +++ b/drivers/hwmon/corsair-psu.c
>>>> @@ -899,7 +899,20 @@ static struct hid_driver corsairpsu_driver = {
>>>> .reset_resume = corsairpsu_resume,
>>>> #endif
>>>> };
>>>> -module_hid_driver(corsairpsu_driver);
>>>> +
>>>> +static int __init corsairpsu_hid_init(void)
>>>> +{
>>>> + return hid_register_driver(&corsairpsu_driver);
>>>> +}
>>>> +
>>>> +static void __exit corsairpsu_hid_exit(void)
>>>> +{
>>>> + hid_unregister_driver(&corsairpsu_driver);
>>>> +}
>>>> +
>>>> +/* When compiled into the kernel, initialize after the hid bus */
>>>> +late_initcall(corsairpsu_hid_init);
>>>> +module_exit(corsairpsu_hid_exit);
>>>> MODULE_LICENSE("GPL");
>>>> MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
>>>
>>>
>>> Woops! Just saw that the same fix was sent yesterday. Please disregard, sorry!
>>>
>>> Aleksa
>>
>> It is fine. I just start to wonder if there was a change in the subsystem. I
>> used the driver as built-in in the past for several months and never had that
>> issue. And now it is a real flood of reports.
>>
>
> Maybe there was a change in the build order, or some subtle change
> in driver registration code. Question though is _when_ this changed.
> It would be great if someone could bisect it. For example, bus registration
> code has been changed significantly in v6.3. I am copying linux-input
> and the hid maintainers for feedback.
The late_initcall() was also needed in 2020. when corsair-cpro was added in
40c3a4454225 ("hwmon: add Corsair Commander Pro driver").
There was also discussion on the list about it:
https://lore.kernel.org/all/3864498.z6qT3ff8q6@marius/
nzxt-smart2 (from the tail of 2021.) also has a comment about the message.
(Just providing references.)
Aleksa
>
> Either case, I now have two patches and at least the first one was actually
> tested, but no Reviewed-by: or Tested-by: for either of them. While that is
> of course a formality, it would still be useful to show that it is not just
> a random change.
>
> Thanks,
> Guenter
>
^ permalink raw reply
* [PATCH] HID: nintendo: use ida for LED player id
From: Martino Fontana @ 2023-12-08 16:15 UTC (permalink / raw)
To: rymcclel
Cc: benjamin.tissoires, djogorchock, jikos, linux-input,
Martino Fontana
In-Reply-To: <20231208075928.4293-1-rymcclel@gmail.com>
Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
---
drivers/hid/hid-nintendo.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 138f154fe..22a3c97a0 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -28,6 +28,7 @@
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/hid.h>
+#include <linux/idr.h>
#include <linux/input.h>
#include <linux/jiffies.h>
#include <linux/leds.h>
@@ -427,6 +428,7 @@ static const enum led_brightness joycon_player_led_patterns[JC_NUM_LED_PATTERNS]
struct joycon_ctlr {
struct hid_device *hdev;
struct input_dev *input;
+ u32 player_id;
struct led_classdev leds[JC_NUM_LEDS]; /* player leds */
struct led_classdev home_led;
enum joycon_ctlr_state ctlr_state;
@@ -1917,7 +1919,8 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
return ret;
}
-static DEFINE_SPINLOCK(joycon_input_num_spinlock);
+static DEFINE_IDA(nintendo_player_id_allocator);
+
static int joycon_leds_create(struct joycon_ctlr *ctlr)
{
struct hid_device *hdev = ctlr->hdev;
@@ -1928,20 +1931,15 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
char *name;
int ret;
int i;
- unsigned long flags;
- int player_led_pattern;
- static int input_num;
-
- /*
- * Set the player leds based on controller number
- * Because there is no standard concept of "player number", the pattern
- * number will simply increase by 1 every time a controller is connected.
- */
- spin_lock_irqsave(&joycon_input_num_spinlock, flags);
- player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
- spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
/* configure the player LEDs */
+ ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
+ if (ret < 0) {
+ hid_warn(hdev, "Failed to allocate player ID, skipping; ret=%d\n", ret);
+ goto home_led;
+ }
+ ctlr->player_id = ret % JC_NUM_LED_PATTERNS;
+
for (i = 0; i < JC_NUM_LEDS; i++) {
name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
d_name,
@@ -1952,13 +1950,13 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
led = &ctlr->leds[i];
led->name = name;
- led->brightness = joycon_player_led_patterns[player_led_pattern][i];
+ led->brightness = joycon_player_led_patterns[ctlr->player_id][i];
led->max_brightness = 1;
led->brightness_set_blocking =
joycon_player_led_brightness_set;
led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
- led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
+ led_val |= joycon_player_led_patterns[ctlr->player_id][i] << i;
}
mutex_lock(&ctlr->output_mutex);
ret = joycon_set_player_leds(ctlr, 0, led_val);
@@ -2410,6 +2408,7 @@ static void nintendo_hid_remove(struct hid_device *hdev)
spin_unlock_irqrestore(&ctlr->lock, flags);
destroy_workqueue(ctlr->rumble_queue);
+ ida_free(&nintendo_player_id_allocator, ctlr->player_id);
hid_hw_close(hdev);
hid_hw_stop(hdev);
--
2.42.0
^ permalink raw reply related
* Re: [PATCH] HID: nintendo: use slots for led patterns
From: Martino Fontana @ 2023-12-08 16:18 UTC (permalink / raw)
To: rymcclel
Cc: Benjamin Tissoires, Daniel J. Ogorchock, Jiri Kosina, linux-input
Funny timing, I was working on a patch for the same thing too.
Instead of using a bitmask, I used ida_alloc (just like hid-playstation does).
I'll leave my version of the patch here, for comparison's sake.
(It has no description, and it wraps on the 8th player)
I can't properly test either of them, since I have only one controller (though
every time I connect it, only the first LED light up, as expected).
^ permalink raw reply
* Re: [PATCH] hwmon: (corsair-psu) Fix failure to load when built-in to kernel
From: Guenter Roeck @ 2023-12-08 15:43 UTC (permalink / raw)
To: Wilken Gottwalt, Aleksa Savic
Cc: linux-hwmon, Jean Delvare, linux-kernel, linux-input, Jiri Kosina,
Benjamin Tissoires
In-Reply-To: <20231208145742.6def047a@posteo.net>
On 12/8/23 05:57, Wilken Gottwalt wrote:
> On Fri, 8 Dec 2023 14:11:44 +0100
> Aleksa Savic <savicaleksa83@gmail.com> wrote:
>
>> On 2023-12-08 14:07:10 GMT+01:00, Aleksa Savic wrote:
>>> When built-in to the kernel, the corsair-psu driver fails to register with
>>> the following message:
>>>
>>> "Driver 'corsair-psu' was unable to register with bus_type 'hid'
>>> because the bus was not initialized."
>>>
>>> Fix this by initializing the driver after the HID bus using
>>> late_initcall(), as hwmon is built before HID.
>>>
>>> Fixes: d115b51e0e56 ("hwmon: add Corsair PSU HID controller driver")
>>> Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
>>> ---
>>> drivers/hwmon/corsair-psu.c | 15 ++++++++++++++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/hwmon/corsair-psu.c b/drivers/hwmon/corsair-psu.c
>>> index 904890598c11..48831a528965 100644
>>> --- a/drivers/hwmon/corsair-psu.c
>>> +++ b/drivers/hwmon/corsair-psu.c
>>> @@ -899,7 +899,20 @@ static struct hid_driver corsairpsu_driver = {
>>> .reset_resume = corsairpsu_resume,
>>> #endif
>>> };
>>> -module_hid_driver(corsairpsu_driver);
>>> +
>>> +static int __init corsairpsu_hid_init(void)
>>> +{
>>> + return hid_register_driver(&corsairpsu_driver);
>>> +}
>>> +
>>> +static void __exit corsairpsu_hid_exit(void)
>>> +{
>>> + hid_unregister_driver(&corsairpsu_driver);
>>> +}
>>> +
>>> +/* When compiled into the kernel, initialize after the hid bus */
>>> +late_initcall(corsairpsu_hid_init);
>>> +module_exit(corsairpsu_hid_exit);
>>>
>>> MODULE_LICENSE("GPL");
>>> MODULE_AUTHOR("Wilken Gottwalt <wilken.gottwalt@posteo.net>");
>>
>>
>> Woops! Just saw that the same fix was sent yesterday. Please disregard, sorry!
>>
>> Aleksa
>
> It is fine. I just start to wonder if there was a change in the subsystem. I
> used the driver as built-in in the past for several months and never had that
> issue. And now it is a real flood of reports.
>
Maybe there was a change in the build order, or some subtle change
in driver registration code. Question though is _when_ this changed.
It would be great if someone could bisect it. For example, bus registration
code has been changed significantly in v6.3. I am copying linux-input
and the hid maintainers for feedback.
Either case, I now have two patches and at least the first one was actually
tested, but no Reviewed-by: or Tested-by: for either of them. While that is
of course a formality, it would still be useful to show that it is not just
a random change.
Thanks,
Guenter
^ permalink raw reply
* Re: [RFC PATCH v3 2/5] i2c: of: Introduce component probe function
From: Rob Herring @ 2023-12-08 15:10 UTC (permalink / raw)
To: Doug Anderson
Cc: Chen-Yu Tsai, Frank Rowand, Krzysztof Kozlowski, Conor Dooley,
Matthias Brugger, AngeloGioacchino Del Regno, Wolfram Sang,
Benson Leung, Tzung-Bi Shih, chrome-platform, devicetree,
linux-arm-kernel, linux-mediatek, linux-kernel, Johan Hovold,
Hsin-Yi Wang, Dmitry Torokhov, andriy.shevchenko, Jiri Kosina,
linus.walleij, broonie, gregkh, hdegoede, james.clark, james,
keescook, rafael, tglx, Jeff LaBundy, linux-input, linux-i2c
In-Reply-To: <CAD=FV=U_+iQJtV0Wii89DQT1V_fJCeS9wcqA8EJAs-hmmmLLLg@mail.gmail.com>
On Fri, Dec 01, 2023 at 04:57:46PM -0800, Doug Anderson wrote:
> Hi,
>
> On Tue, Nov 28, 2023 at 12:45 AM Chen-Yu Tsai <wenst@chromium.org> wrote:
> >
> > @@ -217,4 +217,114 @@ static int of_i2c_notify(struct notifier_block *nb, unsigned long action,
> > struct notifier_block i2c_of_notifier = {
> > .notifier_call = of_i2c_notify,
> > };
> > +
> > +/*
> > + * Some devices, such as Google Hana Chromebooks, are produced by multiple
> > + * vendors each using their preferred components. Such components are all
> > + * in the device tree. Instead of having all of them enabled and having each
> > + * driver separately try and probe its device while fighting over shared
> > + * resources, they can be marked as "fail-needs-probe" and have a prober
> > + * figure out which one is actually used beforehand.
> > + *
> > + * This prober assumes such drop-in parts are on the same I2C bus, have
> > + * non-conflicting addresses, and can be directly probed by seeing which
> > + * address responds.
> > + *
> > + * TODO:
> > + * - Support handling common regulators and GPIOs.
>
> IMO you should prototype how you're going to handle regulators and
> GPIOs before finalizing the design. I was going to write that you
> should just document that it was up to the caller to power things up
> before calling this function, but then I realized that the caller
> would have to duplicate much of this function in order to do so. In
> the very least they'd have to find the nodes of the relevant devices
> so that they could grab regulators and/or GPIOs. In order to avoid
> this duplication, would the design need to change? Perhaps this would
> be as simple as adding a callback function here that's called with all
> of the nodes before probing? If that's right, it would be nice to have
> that callback from the beginning so we don't need two variants of the
> function...
>
> > + * - Support I2C muxes
> > + */
> > +
> > +/**
> > + * i2c_of_probe_component() - probe for devices of "type" on the same i2c bus
> > + * @dev: &struct device of the caller, only used for dev_* printk messages
> > + * @type: a string to match the device node name prefix to probe for
> > + *
> > + * Probe for possible I2C components of the same "type" on the same I2C bus
> > + * that have their status marked as "fail".
>
> Should document these current limitations with the code:
>
> * Assumes that across the entire device tree the only instances of
> nodes named "type" are ones we're trying to handle second sourcing
> for. In other words if we're searching for "touchscreen" then all
> nodes named "touchscreen" are ones that need to be probed.
named "type" and marked as needs probe.
>
> * Assumes that there is exactly one group of each "type". In other
> words, if we're searching for "touchscreen" then exactly one
> touchscreen will be enabled across the whole tree.
Does that need to be a limitation? If you just keep going thru all
devices, wouldn't that just work?
Rob
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property
From: Stefan Eichenberger @ 2023-12-08 15:05 UTC (permalink / raw)
To: Linus Walleij
Cc: nick, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
In-Reply-To: <CACRpkdao83-nALj2YOq-XHrOh6GEaxufN3Fn+3W52qkL2x+VUQ@mail.gmail.com>
Hi Linus,
On Fri, Dec 08, 2023 at 03:23:54PM +0100, Linus Walleij wrote:
> On Fri, Dec 8, 2023 at 2:11 PM Stefan Eichenberger <eichest@gmail.com> wrote:
>
> > > I can't help but wonder: shouldn't that pretty much be the default behaviour
> > > if wakeup-source is *not* specified?
> > >
> > > I.e. the property kind of describes !wakeup-source.
> >
> > The maxtouch controller has a deep sleep mode which is currently used
> > without powering down vdd and vdda. However, because we have a shared
> > regulator which powers other peripherals that we want to shut down in
> > suspend we need a way to power down vdd and vdda. However, I agree this
> > is not really a feature of the device. The feature would basically be
> > the internal deep sleep mode.
>
> While it is of no concern to the device tree bindings, Linux regulators
> are counting meaning that you need to make all peripherals disable
> their regulators and it will come down.
Yes we are working on that one. This is the last driver we need to allow
disabling the regulator, afterward it should hoepfully work on our
target system.
>
> > I didn't want to change the default
> > behaviour of the driver, so I added this property but maybe I could
> > change it to:
> >
> > atmel,deep-sleep:
> > description: |
> > Use the maxtouch deep-sleep mode instead of powering down vdd and
> > vdda.
> >
> > Or to not change the default behaviour:
> > atmel,no-deep-sleep:
> > description: |
> > Do not use the maxtouch deep-sleep mode but power down vdd and vdda
> > in suspend.
> >
> > As I understand the datasheet even if the maxtouch is using its deep
> > sleep mode it does not act as a wakeup source.
>
> Do you mean it can still work as a wakeup source in deep sleep mode?
> (there is a "not" too much above ...)
Sorry for the confusion. As it is configured now, it can not work as
wakeup source in deep sleep mode. Touch is completely disabled.
> > It is just faster in
> > waking up because it can keep the configuration in memory.
>
> That sounds like a good reason to have the property, because that
> means that if you can control the wakeup latency and specify in the binding
> how much in absolute time units it is affected.
>
> I would define it in positive terms instead of reverse "no-deep-sleep"
> though such as "atmel,fast-wakeup".
>
> And: If you disable the regulators it will probably *not* be able to wake the
> system up, right? And that is just a few lines of code in the driver such as:
>
> go_to_sleep():
> if (!wakeup_source):
> disable_regulators()
So to not change the default behaviour I would have to name it:
atmel,slow-wakeup
or maybe even full wakeup because it does a wakeup from the disabled
state?
atmel,full-wakeup
Exactly, the added code looks more or less exactly as you wrote. And on
resume it does the opposite + configure it:
resume():
if (!wakeup_source):
enable_regulators()
configure_maxtouch()
Regards,
Stefan
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property
From: Linus Walleij @ 2023-12-08 14:23 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: nick, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
In-Reply-To: <ZXMV9gzFbc05IEKg@eichest-laptop>
On Fri, Dec 8, 2023 at 2:11 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> > I can't help but wonder: shouldn't that pretty much be the default behaviour
> > if wakeup-source is *not* specified?
> >
> > I.e. the property kind of describes !wakeup-source.
>
> The maxtouch controller has a deep sleep mode which is currently used
> without powering down vdd and vdda. However, because we have a shared
> regulator which powers other peripherals that we want to shut down in
> suspend we need a way to power down vdd and vdda. However, I agree this
> is not really a feature of the device. The feature would basically be
> the internal deep sleep mode.
While it is of no concern to the device tree bindings, Linux regulators
are counting meaning that you need to make all peripherals disable
their regulators and it will come down.
> I didn't want to change the default
> behaviour of the driver, so I added this property but maybe I could
> change it to:
>
> atmel,deep-sleep:
> description: |
> Use the maxtouch deep-sleep mode instead of powering down vdd and
> vdda.
>
> Or to not change the default behaviour:
> atmel,no-deep-sleep:
> description: |
> Do not use the maxtouch deep-sleep mode but power down vdd and vdda
> in suspend.
>
> As I understand the datasheet even if the maxtouch is using its deep
> sleep mode it does not act as a wakeup source.
Do you mean it can still work as a wakeup source in deep sleep mode?
(there is a "not" too much above ...)
> It is just faster in
> waking up because it can keep the configuration in memory.
That sounds like a good reason to have the property, because that
means that if you can control the wakeup latency and specify in the binding
how much in absolute time units it is affected.
I would define it in positive terms instead of reverse "no-deep-sleep"
though such as "atmel,fast-wakeup".
And: If you disable the regulators it will probably *not* be able to wake the
system up, right? And that is just a few lines of code in the driver such as:
go_to_sleep():
if (!wakeup_source):
disable_regulators()
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH] irq: Resolve that mask_irq/unmask_irq may not be called in pairs
From: Thomas Gleixner @ 2023-12-08 13:52 UTC (permalink / raw)
To: xiongxin, jikos, benjamin.tissoires
Cc: linux-input, xiongxin, stable, Riwen Lu
In-Reply-To: <20231207014003.12919-1-xiongxin@kylinos.cn>
On Thu, Dec 07 2023 at 09:40, xiongxin@kylinos.cn wrote:
> When an interrupt controller uses a function such as handle_level_irq()
> as an interrupt handler and the controller implements the irq_disable()
> callback, the following scenario will appear in the i2c-hid driver in
> the sleep scenario:
>
> in the sleep flow, while the user is still triggering the i2c-hid
> interrupt, we get the following function call:
>
> handle_level_irq()
> -> mask_ack_irq()
> -> mask_irq()
> i2c_hid_core_suspend()
> -> disable_irq()
> -> __irq_disable()
> -> irq_state_set_disabled()
> -> irq_state_set_masked()
>
> irq_thread_fn()
> -> irq_finalize_oneshot()
> -> if (!desc->threads_oneshot && !irqd_irq_disabled() &&
> irqd_irq_masked())
> unmask_threaded_irq()
> -> unmask_irq()
>
> That is, when __irq_disable() is called between mask_irq() and
> irq_finalize_oneshot(), the code in irq_finalize_oneshot() will cause
> the !irqd_irq_disabled() fails to enter the unmask_irq() branch, which
> causes mask_irq/unmask_irq to be called unpaired and the i2c-hid
> interrupt to be masked.
>
> Since mask_irq/unmask_irq and irq_disabled() belong to two different
> hardware registers or policies, the !irqd_irq_disabled() assertion may
> not be used to determine whether unmask_irq() needs to be called.
No. That's fundamentally wrong.
Disabled interrupts are disabled and can only be reenabled by the
corresponding enable call. The existing code is entirely correct.
What you are trying to do is unmasking a disabled interrupt, which
results in inconsistent state.
Which interrupt chip is involved here?
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property
From: Stefan Eichenberger @ 2023-12-08 13:11 UTC (permalink / raw)
To: Linus Walleij
Cc: nick, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
In-Reply-To: <CACRpkdbSs-vebvchxx-Tg+O5CUF5M3vZf-iytuW=ZECnHb2anA@mail.gmail.com>
Hi Krzysztof and Linux,
On Fri, Dec 08, 2023 at 01:54:21PM +0100, Linus Walleij wrote:
> On Thu, Dec 7, 2023 at 12:13 PM Stefan Eichenberger <eichest@gmail.com> wrote:
>
> > From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> >
> > Add a new property to indicate that the device should be powered off in
> > suspend mode.
> >
> > Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
> (...)
> > + atmel,poweroff-in-suspend:
> > + description: |
> > + When this property is set, all supplies are turned off when the system is
> > + going to suspend.
> > + type: boolean
> wakeup-source:
> type: boolean
>
> As Krzysztof says it seems you are describing an operating system feature.
>
> I can't help but wonder: shouldn't that pretty much be the default behaviour
> if wakeup-source is *not* specified?
>
> I.e. the property kind of describes !wakeup-source.
The maxtouch controller has a deep sleep mode which is currently used
without powering down vdd and vdda. However, because we have a shared
regulator which powers other peripherals that we want to shut down in
suspend we need a way to power down vdd and vdda. However, I agree this
is not really a feature of the device. The feature would basically be
the internal deep sleep mode. I didn't want to change the default
behaviour of the driver, so I added this property but maybe I could
change it to:
atmel,deep-sleep:
description: |
Use the maxtouch deep-sleep mode instead of powering down vdd and
vdda.
Or to not change the default behaviour:
atmel,no-deep-sleep:
description: |
Do not use the maxtouch deep-sleep mode but power down vdd and vdda
in suspend.
As I understand the datasheet even if the maxtouch is using its deep
sleep mode it does not act as a wakeup source. It is just faster in
waking up because it can keep the configuration in memory.
Regards,
Stefan
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property
From: Linus Walleij @ 2023-12-08 12:54 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: nick, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linux-input,
devicetree, linux-arm-kernel, linux-kernel, Stefan Eichenberger
In-Reply-To: <20231207111300.80581-2-eichest@gmail.com>
On Thu, Dec 7, 2023 at 12:13 PM Stefan Eichenberger <eichest@gmail.com> wrote:
> From: Stefan Eichenberger <stefan.eichenberger@toradex.com>
>
> Add a new property to indicate that the device should be powered off in
> suspend mode.
>
> Signed-off-by: Stefan Eichenberger <stefan.eichenberger@toradex.com>
(...)
> + atmel,poweroff-in-suspend:
> + description: |
> + When this property is set, all supplies are turned off when the system is
> + going to suspend.
> + type: boolean
wakeup-source:
type: boolean
As Krzysztof says it seems you are describing an operating system feature.
I can't help but wonder: shouldn't that pretty much be the default behaviour
if wakeup-source is *not* specified?
I.e. the property kind of describes !wakeup-source.
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property
From: Krzysztof Kozlowski @ 2023-12-08 12:47 UTC (permalink / raw)
To: Stefan Eichenberger
Cc: nick, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linus.walleij,
linux-input, devicetree, linux-arm-kernel, linux-kernel,
Stefan Eichenberger
In-Reply-To: <ZXMN9EgWRjtJyybr@eichest-laptop>
On 08/12/2023 13:37, Stefan Eichenberger wrote:
> Hi Krzysztof,
>
> On Thu, Dec 07, 2023 at 05:59:08PM +0100, Krzysztof Kozlowski wrote:
>> On 07/12/2023 12:12, Stefan Eichenberger wrote:
>>> diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
>>> index c40799355ed7..047a5101341c 100644
>>> --- a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
>>> +++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
>>> @@ -87,6 +87,12 @@ properties:
>>> - 2 # ATMEL_MXT_WAKEUP_GPIO
>>> default: 0
>>>
>>> + atmel,poweroff-in-suspend:
>>> + description: |
>>> + When this property is set, all supplies are turned off when the system is
>>> + going to suspend.
>>
>> You described the desired Linux feature or behavior, not the actual
>> hardware. The bindings are about the latter, so instead you need to
>> rephrase the property and its description to match actual hardware
>> capabilities/features/configuration etc.
>
> Thanks a lot for the feedback. Would the following be okay as a
> description?
>
> vdda and vdd are switched off in suspend mode.
Are switched by who? Linux driver? Then nothing changed...
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1 1/2] dt-bindings: input: atmel,maxtouch: add poweroff-in-suspend property
From: Stefan Eichenberger @ 2023-12-08 12:37 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: nick, dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt,
nicolas.ferre, alexandre.belloni, claudiu.beznea, linus.walleij,
linux-input, devicetree, linux-arm-kernel, linux-kernel,
Stefan Eichenberger
In-Reply-To: <b9868bd4-6f14-4628-88ea-56d06027739e@linaro.org>
Hi Krzysztof,
On Thu, Dec 07, 2023 at 05:59:08PM +0100, Krzysztof Kozlowski wrote:
> On 07/12/2023 12:12, Stefan Eichenberger wrote:
> > diff --git a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
> > index c40799355ed7..047a5101341c 100644
> > --- a/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
> > +++ b/Documentation/devicetree/bindings/input/atmel,maxtouch.yaml
> > @@ -87,6 +87,12 @@ properties:
> > - 2 # ATMEL_MXT_WAKEUP_GPIO
> > default: 0
> >
> > + atmel,poweroff-in-suspend:
> > + description: |
> > + When this property is set, all supplies are turned off when the system is
> > + going to suspend.
>
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
Thanks a lot for the feedback. Would the following be okay as a
description?
vdda and vdd are switched off in suspend mode.
Best regards,
Stefan
^ permalink raw reply
* Re: [PATCH v1] dt-bindings: input: convert gpio-mouse to json-schema
From: Krzysztof Kozlowski @ 2023-12-08 10:59 UTC (permalink / raw)
To: Anshul Dalal, linux-input, devicetree
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, linux-kernel, linux-kernel-mentees
In-Reply-To: <61053bca-6a6c-4eed-90f8-df43f7c804cb@gmail.com>
On 08/12/2023 11:33, Anshul Dalal wrote:
>
>
> On 12/8/23 15:57, Krzysztof Kozlowski wrote:
>> On 08/12/2023 08:50, Anshul Dalal wrote:
>>> Convert device tree binding documentation for GPIO attached mouse to
>>> json-schema.
>>>
>>> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> You cannot have v1 being already reviewed. This is some newer version.
>>
>>> ---
>>
>> Missing changelog.
>
> This is a copy of an earlier patch[1]. Since the patch had been sitting
> idle for past 10 days, I reposed it with the Reviewed-by tags added in.
> Please let me know if this is the right way or if there is need for a
> changelog.
>
> [1]:
> https://lore.kernel.org/lkml/20231126103029.851742-1-anshulusr@gmail.com/
OK, then it should be: "[PATCH RESEND] ...." with a short changelog like:
---
Resending with collected tags.
...
(Sharing this for future, no need to resend it now)
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v1] dt-bindings: input: convert gpio-mouse to json-schema
From: Anshul Dalal @ 2023-12-08 10:33 UTC (permalink / raw)
To: Krzysztof Kozlowski, linux-input, devicetree
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, linux-kernel, linux-kernel-mentees
In-Reply-To: <c8e4d495-1d4e-40cb-b599-5a01f75f9257@linaro.org>
On 12/8/23 15:57, Krzysztof Kozlowski wrote:
> On 08/12/2023 08:50, Anshul Dalal wrote:
>> Convert device tree binding documentation for GPIO attached mouse to
>> json-schema.
>>
>> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>
> You cannot have v1 being already reviewed. This is some newer version.
>
>> ---
>
> Missing changelog.
This is a copy of an earlier patch[1]. Since the patch had been sitting
idle for past 10 days, I reposed it with the Reviewed-by tags added in.
Please let me know if this is the right way or if there is need for a
changelog.
[1]:
https://lore.kernel.org/lkml/20231126103029.851742-1-anshulusr@gmail.com/
Best regards,
Anshul
^ permalink raw reply
* Re: [PATCH v1] dt-bindings: input: convert gpio-mouse to json-schema
From: Krzysztof Kozlowski @ 2023-12-08 10:27 UTC (permalink / raw)
To: Anshul Dalal, linux-input, devicetree
Cc: Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Linus Walleij, linux-kernel, linux-kernel-mentees
In-Reply-To: <20231208075037.114598-1-anshulusr@gmail.com>
On 08/12/2023 08:50, Anshul Dalal wrote:
> Convert device tree binding documentation for GPIO attached mouse to
> json-schema.
>
> Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
> Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
You cannot have v1 being already reviewed. This is some newer version.
> ---
Missing changelog.
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH] HID: nintendo: use slots for led patterns
From: Ryan McClelland @ 2023-12-08 7:59 UTC (permalink / raw)
To: linux-input; +Cc: djogorchock, benjamin.tissoires, jikos, Ryan McClelland
Previously, the leds pattern would just increment with every controller
connected. This wouldn't take into consideration when controllers are
disconnected. The same controller could be connected and disconnected
with the pattern increasing player count each time.
This changes it so now a bitmask is used defining the free controller
slots for each "player". When a controller is connected, it searches
for the first bit in the bitmask that is available and acquires that
"slot" for it's led pattern.
When a controller is disconnected, it sets the position back to 'free'
allowing for a new controller that is to be connected to obtain that
slot.
Signed-off-by: Ryan McClelland <rymcclel@gmail.com>
---
drivers/hid/hid-nintendo.c | 45 ++++++++++++++++++++++++++++----------
1 file changed, 33 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index 138f154fecef..cfe43a6b5a46 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -410,7 +410,8 @@ static const char * const joycon_player_led_names[] = {
LED_FUNCTION_PLAYER4,
};
#define JC_NUM_LEDS ARRAY_SIZE(joycon_player_led_names)
-#define JC_NUM_LED_PATTERNS 8
+#define JC_NUM_LED_PATTERNS 9
+#define JC_LED_DEF_PATTERN 8
/* Taken from https://www.nintendo.com/my/support/qa/detail/33822 */
static const enum led_brightness joycon_player_led_patterns[JC_NUM_LED_PATTERNS][JC_NUM_LEDS] = {
{ 1, 0, 0, 0 },
@@ -421,8 +422,13 @@ static const enum led_brightness joycon_player_led_patterns[JC_NUM_LED_PATTERNS]
{ 1, 0, 1, 0 },
{ 1, 0, 1, 1 },
{ 0, 1, 1, 0 },
+ { 1, 1, 1, 1 }, /* >8 players */
};
+/* Used to set the number of leds on each controller */
+static DEFINE_SPINLOCK(joycon_input_bm_spinlock);
+static int jc_input_free_bm = GENMASK(7, 0);
+
/* Each physical controller is associated with a joycon_ctlr struct */
struct joycon_ctlr {
struct hid_device *hdev;
@@ -491,6 +497,9 @@ struct joycon_ctlr {
unsigned int imu_delta_samples_count;
unsigned int imu_delta_samples_sum;
unsigned int imu_avg_delta_ms;
+
+ /* led */
+ int player_led_pattern;
};
/* Helper macros for checking controller type */
@@ -1917,7 +1926,6 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
return ret;
}
-static DEFINE_SPINLOCK(joycon_input_num_spinlock);
static int joycon_leds_create(struct joycon_ctlr *ctlr)
{
struct hid_device *hdev = ctlr->hdev;
@@ -1929,17 +1937,24 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
int ret;
int i;
unsigned long flags;
- int player_led_pattern;
- static int input_num;
/*
- * Set the player leds based on controller number
- * Because there is no standard concept of "player number", the pattern
- * number will simply increase by 1 every time a controller is connected.
+ * Set the player leds based on a free slot. If there
+ * are more than 8 controllers. Then load the default
+ * pattern.
*/
- spin_lock_irqsave(&joycon_input_num_spinlock, flags);
- player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
- spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
+ spin_lock_irqsave(&joycon_input_bm_spinlock, flags);
+ if (!jc_input_free_bm) {
+ ctlr->player_led_pattern = JC_LED_DEF_PATTERN;
+ } else {
+ ctlr->player_led_pattern = ffs(jc_input_free_bm) - 1;
+ jc_input_free_bm &= ~(BIT(ctlr->player_led_pattern));
+ }
+ spin_unlock_irqrestore(&joycon_input_bm_spinlock, flags);
+ if (ctlr->player_led_pattern == JC_LED_DEF_PATTERN)
+ hid_info(ctlr->hdev, "more than 8 controllers connected, assigning default led pattern");
+ else
+ hid_info(ctlr->hdev, "assigned player %d led pattern", ctlr->player_led_pattern + 1);
/* configure the player LEDs */
for (i = 0; i < JC_NUM_LEDS; i++) {
@@ -1952,13 +1967,13 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
led = &ctlr->leds[i];
led->name = name;
- led->brightness = joycon_player_led_patterns[player_led_pattern][i];
+ led->brightness = joycon_player_led_patterns[ctlr->player_led_pattern][i];
led->max_brightness = 1;
led->brightness_set_blocking =
joycon_player_led_brightness_set;
led->flags = LED_CORE_SUSPENDRESUME | LED_HW_PLUGGABLE;
- led_val |= joycon_player_led_patterns[player_led_pattern][i] << i;
+ led_val |= joycon_player_led_patterns[ctlr->player_led_pattern][i] << i;
}
mutex_lock(&ctlr->output_mutex);
ret = joycon_set_player_leds(ctlr, 0, led_val);
@@ -2409,6 +2424,12 @@ static void nintendo_hid_remove(struct hid_device *hdev)
ctlr->ctlr_state = JOYCON_CTLR_STATE_REMOVED;
spin_unlock_irqrestore(&ctlr->lock, flags);
+ /* Controller removed, so free the slot */
+ spin_lock_irqsave(&joycon_input_bm_spinlock, flags);
+ if (ctlr->player_led_pattern != JC_LED_DEF_PATTERN)
+ jc_input_free_bm |= BIT(ctlr->player_led_pattern);
+ spin_unlock_irqrestore(&joycon_input_bm_spinlock, flags);
+
destroy_workqueue(ctlr->rumble_queue);
hid_hw_close(hdev);
--
2.34.1
^ permalink raw reply related
* Re: [PATCH v4 8/8] dt-bindings: mfd: dlg,da9063: Convert da9062 to json-schema
From: Krzysztof Kozlowski @ 2023-12-08 7:55 UTC (permalink / raw)
To: Biju Das, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Lee Jones
Cc: Support Opensource, Rafael J. Wysocki, Daniel Lezcano, Zhang Rui,
Lukasz Luba, Steve Twiss, linux-input@vger.kernel.org,
devicetree@vger.kernel.org, linux-pm@vger.kernel.org,
Geert Uytterhoeven, Prabhakar Mahadev Lad, biju.das.au,
linux-renesas-soc@vger.kernel.org
In-Reply-To: <TYCPR01MB11269C774B8FFE9E43455EB61868BA@TYCPR01MB11269.jpnprd01.prod.outlook.com>
On 07/12/2023 18:03, Biju Das wrote:
Trim the quote from irrelevant context, especially if your email client
malforms replies... (because it does)
>>> @@ -35,6 +42,19 @@ properties:
>>> "#interrupt-cells":
>>> const: 2
>>>
>>> + gpio:
>>
>> Old binding did not have such node and nothing in commit msg explained
>> changes from pure conversion.
>
> OK will update the commit message. Check patch complained about undocumented compatible.
>
>>
>>> + type: object
>>> + $ref: /schemas/gpio/gpio.yaml#
>>
>> ?!? Why? First: It's always selected. Second, so you have two gpio
>> controllers? And original binding had 0? Why this is not explained at all?
>> Open the binding and look at its properties.
>
>
> I have referred[1] and [2] to add gpio controller property.
>
>
> [1]
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl-phytec-phycore-som.dtsi?h=next-20231207#n95
>
> [2]
>
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/Documentation/devicetree/bindings/mfd/da9062.txt?h=next-20231207#n38
But does it make sense? Don't just blindly copy things, but actually
investigate whether this is correct DTS.
>
>>
>>
>>> + unevaluatedProperties: false
>>> + properties:
>>> + compatible:
>>> + const: dlg,da9062-gpio
>>> +
>>> + gpio-controller: true
>>
>> And here is the second gpio-controller...
>
> So you mean it is redundant as $ref: /schemas/gpio/gpio.yaml
> has already defined gpio-controller for this object??
I meant this would mean you have two GPIO controllers. Why one device
would have two GPIO controllers? Please answer to this in commit msg, so
there will be no questions/concerns. You have entire commit msg to
explain all weird and unexpected things with this binding.
...
>>> + #include <dt-bindings/interrupt-controller/irq.h>
>>> + #include <dt-bindings/regulator/dlg,da9063-regulator.h>
>>> + i2c {
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + pmic@58 {
>>> + compatible = "dlg,da9062";
>>> + reg = <0x58>;
>>> + #interrupt-cells = <2>;
>>> + interrupt-parent = <&gpio1>;
>>> + interrupts = <2 IRQ_TYPE_LEVEL_LOW>;
>>> + interrupt-controller;
>>> +
>>> + gpio {
>>> + compatible = "dlg,da9062-gpio";
>>> + status = "disabled";
>>
>> Why?
Why disabling? Drop all statuses from all your binding examples.
>> Where are gpio-controller and cells? For this node and for parent? Why
>> this example is incomplete?
>
> I have used a real example [1] here.
>
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/arch/arm/boot/dts/nxp/imx/imx6qdl-phytec-phycore-som.dtsi?h=next-20231207#n95
>
> Currently I don't see any driver is using this compatible other than MFD.
Open the MFD so you will see it...
Best regards,
Krzysztof
^ permalink raw reply
* [PATCH v1] dt-bindings: input: convert gpio-mouse to json-schema
From: Anshul Dalal @ 2023-12-08 7:50 UTC (permalink / raw)
To: linux-input, devicetree
Cc: Anshul Dalal, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Linus Walleij, linux-kernel, linux-kernel-mentees,
Krzysztof Kozlowski
Convert device tree binding documentation for GPIO attached mouse to
json-schema.
Signed-off-by: Anshul Dalal <anshulusr@gmail.com>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
---
.../devicetree/bindings/input/gpio-mouse.txt | 32 ---------
.../devicetree/bindings/input/gpio-mouse.yaml | 68 +++++++++++++++++++
2 files changed, 68 insertions(+), 32 deletions(-)
delete mode 100644 Documentation/devicetree/bindings/input/gpio-mouse.txt
create mode 100644 Documentation/devicetree/bindings/input/gpio-mouse.yaml
diff --git a/Documentation/devicetree/bindings/input/gpio-mouse.txt b/Documentation/devicetree/bindings/input/gpio-mouse.txt
deleted file mode 100644
index 519510a11af9..000000000000
--- a/Documentation/devicetree/bindings/input/gpio-mouse.txt
+++ /dev/null
@@ -1,32 +0,0 @@
-Device-Tree bindings for GPIO attached mice
-
-This simply uses standard GPIO handles to define a simple mouse connected
-to 5-7 GPIO lines.
-
-Required properties:
- - compatible: must be "gpio-mouse"
- - scan-interval-ms: The scanning interval in milliseconds
- - up-gpios: GPIO line phandle to the line indicating "up"
- - down-gpios: GPIO line phandle to the line indicating "down"
- - left-gpios: GPIO line phandle to the line indicating "left"
- - right-gpios: GPIO line phandle to the line indicating "right"
-
-Optional properties:
- - button-left-gpios: GPIO line handle to the left mouse button
- - button-middle-gpios: GPIO line handle to the middle mouse button
- - button-right-gpios: GPIO line handle to the right mouse button
-Example:
-
-#include <dt-bindings/gpio/gpio.h>
-
-gpio-mouse {
- compatible = "gpio-mouse";
- scan-interval-ms = <50>;
- up-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
- down-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
- left-gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
- right-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;
- button-left-gpios = <&gpio0 4 GPIO_ACTIVE_LOW>;
- button-middle-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
- button-right-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
-};
diff --git a/Documentation/devicetree/bindings/input/gpio-mouse.yaml b/Documentation/devicetree/bindings/input/gpio-mouse.yaml
new file mode 100644
index 000000000000..3928ec6aff1d
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/gpio-mouse.yaml
@@ -0,0 +1,68 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/gpio-mouse.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO attached mouse
+
+description: |
+ This simply uses standard GPIO handles to define a simple mouse connected
+ to 5-7 GPIO lines.
+
+maintainers:
+ - Anshul Dalal <anshulusr@gmail.com>
+
+properties:
+ compatible:
+ const: gpio-mouse
+
+ scan-interval-ms:
+ maxItems: 1
+
+ up-gpios:
+ maxItems: 1
+
+ down-gpios:
+ maxItems: 1
+
+ left-gpios:
+ maxItems: 1
+
+ right-gpios:
+ maxItems: 1
+
+ button-left-gpios:
+ maxItems: 1
+
+ button-middle-gpios:
+ maxItems: 1
+
+ button-right-gpios:
+ maxItems: 1
+
+required:
+ - compatible
+ - scan-interval-ms
+ - up-gpios
+ - down-gpios
+ - left-gpios
+ - right-gpios
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/gpio/gpio.h>
+
+ gpio-mouse {
+ compatible = "gpio-mouse";
+ scan-interval-ms = <50>;
+ up-gpios = <&gpio0 0 GPIO_ACTIVE_LOW>;
+ down-gpios = <&gpio0 1 GPIO_ACTIVE_LOW>;
+ left-gpios = <&gpio0 2 GPIO_ACTIVE_LOW>;
+ right-gpios = <&gpio0 3 GPIO_ACTIVE_LOW>;
+ button-left-gpios = <&gpio0 4 GPIO_ACTIVE_LOW>;
+ button-middle-gpios = <&gpio0 5 GPIO_ACTIVE_LOW>;
+ button-right-gpios = <&gpio0 6 GPIO_ACTIVE_LOW>;
+ };
--
2.42.1
^ permalink raw reply related
* [PATCH] x86/vmware: Add TDX hypercall support
From: Alexey Makhalov @ 2023-12-08 2:32 UTC (permalink / raw)
To: linux-kernel, virtualization, bp, hpa, dave.hansen, mingo, tglx
Cc: x86, netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, namit, timothym, akaher,
jsipek, dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms
In-Reply-To: <64074f04-fd72-488b-831a-ad744bbcd950@broadcom.com>
From: Alexey Makhalov <amakhalov@vmware.com>
VMware hypercalls use I/O port, VMCALL or VMMCALL instructions.
Add __tdx_hypercall path to support TDX guests.
No change in high bandwidth hypercalls, as only low bandwidth
ones are supported for TDX guests.
Co-developed-by: Tim Merrifield <timothym@vmware.com>
Signed-off-by: Tim Merrifield <timothym@vmware.com>
Signed-off-by: Alexey Makhalov <amakhalov@vmware.com>
Reviewed-by: Nadav Amit <namit@vmware.com>
---
arch/x86/include/asm/vmware.h | 83 +++++++++++++++++++++++++++++++++++
arch/x86/kernel/cpu/vmware.c | 22 ++++++++++
2 files changed, 105 insertions(+)
diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h
index 719e41260ece..04c698b905ab 100644
--- a/arch/x86/include/asm/vmware.h
+++ b/arch/x86/include/asm/vmware.h
@@ -34,12 +34,65 @@
#define VMWARE_CMD_GETHZ 45
#define VMWARE_CMD_GETVCPU_INFO 68
#define VMWARE_CMD_STEALCLOCK 91
+/*
+ * Hypercall command mask:
+ * bits[6:0] command, range [0, 127]
+ * bits[19:16] sub-command, range [0, 15]
+ */
+#define VMWARE_CMD_MASK 0xf007fULL
#define CPUID_VMWARE_FEATURES_ECX_VMMCALL BIT(0)
#define CPUID_VMWARE_FEATURES_ECX_VMCALL BIT(1)
extern u8 vmware_hypercall_mode;
+#define VMWARE_TDX_VENDOR_LEAF 0x1af7e4909ULL
+#define VMWARE_TDX_HCALL_FUNC 1
+
+extern unsigned long vmware_tdx_hypercall(struct tdx_module_args *args);
+
+/*
+ * TDCALL[TDG.VP.VMCALL] uses rax (arg0) and rcx (arg2), while the use of
+ * rbp (arg6) is discouraged by the TDX specification. Therefore, we
+ * remap those registers to r12, r13 and r14, respectively.
+ */
+static inline
+unsigned long vmware_tdx_hypercall_args(unsigned long cmd, unsigned long in1,
+ unsigned long in3, unsigned long in4,
+ unsigned long in5, unsigned long in6,
+ uint32_t *out1, uint32_t *out2,
+ uint32_t *out3, uint32_t *out4,
+ uint32_t *out5, uint32_t *out6)
+{
+ unsigned long ret;
+
+ struct tdx_module_args args = {
+ .r13 = cmd,
+ .rbx = in1,
+ .rdx = in3,
+ .rsi = in4,
+ .rdi = in5,
+ .r14 = in6,
+ };
+
+ ret = vmware_tdx_hypercall(&args);
+
+ if (out1)
+ *out1 = args.rbx;
+ if (out2)
+ *out2 = args.r13;
+ if (out3)
+ *out3 = args.rdx;
+ if (out4)
+ *out4 = args.rsi;
+ if (out5)
+ *out5 = args.rdi;
+ if (out6)
+ *out6 = args.r14;
+
+ return ret;
+}
+
/*
* The low bandwidth call. The low word of edx is presumed to have OUT bit
* set. The high word of edx may contain input data from the caller.
@@ -67,6 +120,11 @@ unsigned long vmware_hypercall1(unsigned long cmd, unsigned long in1)
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0, 0,
+ NULL, NULL, NULL,
+ NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -85,6 +143,11 @@ unsigned long vmware_hypercall3(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0, 0,
+ out1, out2, NULL,
+ NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=b" (*out1), "=c" (*out2)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -104,6 +167,11 @@ unsigned long vmware_hypercall4(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, 0, 0, 0, 0,
+ out1, out2, out3,
+ NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -123,6 +191,11 @@ unsigned long vmware_hypercall5(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, in3, in4, in5, 0,
+ NULL, out2, NULL,
+ NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=c" (*out2)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
@@ -145,6 +218,11 @@ unsigned long vmware_hypercall6(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, in3, 0, 0, 0,
+ NULL, out2, out3,
+ out4, out5, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=c" (*out2), "=d" (*out3), "=S" (*out4),
"=D" (*out5)
@@ -166,6 +244,11 @@ unsigned long vmware_hypercall7(unsigned long cmd, unsigned long in1,
{
unsigned long out0;
+ if (cpu_feature_enabled(X86_FEATURE_TDX_GUEST))
+ return vmware_tdx_hypercall_args(cmd, in1, in3, in4, in5, 0,
+ out1, out2, out3,
+ NULL, NULL, NULL);
+
asm_inline volatile (VMWARE_HYPERCALL
: "=a" (out0), "=b" (*out1), "=c" (*out2), "=d" (*out3)
: [port] "i" (VMWARE_HYPERVISOR_PORT),
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 3aa1adaed18f..bcf1d0fb3e89 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -428,6 +428,28 @@ static bool __init vmware_legacy_x2apic_available(void)
(eax & BIT(VCPU_LEGACY_X2APIC));
}
+#ifdef CONFIG_INTEL_TDX_GUEST
+unsigned long vmware_tdx_hypercall(struct tdx_module_args *args)
+{
+ if (!hypervisor_is_type(X86_HYPER_VMWARE))
+ return 0;
+
+ if (args->r13 & ~VMWARE_CMD_MASK) {
+ pr_warn("Out of range command %llx\n", args->r13);
+ return 0;
+ }
+
+ args->r10 = VMWARE_TDX_VENDOR_LEAF;
+ args->r11 = VMWARE_TDX_HCALL_FUNC;
+ args->r12 = VMWARE_HYPERVISOR_MAGIC;
+
+ __tdx_hypercall(args);
+
+ return args->r12;
+}
+EXPORT_SYMBOL_GPL(vmware_tdx_hypercall);
+#endif
+
#ifdef CONFIG_AMD_MEM_ENCRYPT
static void vmware_sev_es_hcall_prepare(struct ghcb *ghcb,
struct pt_regs *regs)
--
2.39.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox