* [PATCH v2] HID: logitech-dj: allow mice to report multimedia keycodes
From: Yaraslau Furman @ 2024-02-22 10:18 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires
Cc: Lucas Zampieri, Yaraslau Furman, Filipe Laíns,
open list:HID LOGITECH DRIVERS, open list
In-Reply-To: <20240130111700.11666-1-yaro330@gmail.com>
Multimedia buttons can be bound to the mouse's extra keys in Windows application.
Let Linux receive those keycodes.
Signed-off-by: Yaraslau Furman <yaro330@gmail.com>
---
drivers/hid/hid-logitech-dj.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index e6a8b6d8eab7..3c3c497b6b91 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -965,9 +965,7 @@ static void logi_hidpp_dev_conn_notif_equad(struct hid_device *hdev,
}
break;
case REPORT_TYPE_MOUSE:
- workitem->reports_supported |= STD_MOUSE | HIDPP;
- if (djrcv_dev->type == recvr_type_mouse_only)
- workitem->reports_supported |= MULTIMEDIA;
+ workitem->reports_supported |= STD_MOUSE | HIDPP | MULTIMEDIA;
break;
}
}
--
2.43.0
^ permalink raw reply related
* Re: Future handling of complex RGB devices on Linux v2
From: Hans de Goede @ 2024-02-22 10:46 UTC (permalink / raw)
To: Pavel Machek, Werner Sembach
Cc: Lee Jones, jikos, linux-kernel, Jelle van der Waa, Miguel Ojeda,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds
In-Reply-To: <ZdZ2kMASawJ9wdZj@duo.ucw.cz>
Hi,
On 2/21/24 23:17, Pavel Machek wrote:
> Hi!
>
>> so after more feedback from the OpenRGB maintainers I came up with an even
>> more generic proposal:
>> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>
>>> evaluate-set-command ioctl taking:
>>> {
>>> enum command /* one of supported_commands */
>>> union data
>>> {
>>> char raw[3072],
>>> {
>>> <input struct for command 0>
>>> },
>
> Yeah, so ... this is not a interface. This is a backdoor to pass
> arbitrary data. That's not going to fly.
Pavel, Note the data will be interpreted by a kernel driver and
not passed directly to the hw.
With that said I tend to agree that this seems to be a bit too
generic.
Werner, it seems you are basically re-inventing ioctls here.
If you are going to use per vendor specific data structs for various
commands and have those defined in the kernel userspace API headers,
then this means that userspace will already need updated versions
of those headers to support new vendors / new laptop models if
the commands change for a new model.
So what you are basically providing here is a generic interface
to pass a cmd number + a cmd-bumber-specific data struct and
we already have that it is called an ioctl.
So I think that the conclusion of this whole discussion is that
with the exception of a get-dev-info ioctl, we simply want
vendor specific ioctls, using 1 ioctl per command.
Given that these devices are all different in various ways
and that we only want this for devices which cannot be accessed
from userspace directly (so a limit set of devices) I really
think that just doing custom ioctls per vendor is best.
This certainly is the most KISS approach. This proposal
in essence is just an arbitrary command multiplexer /
demultiplexer and ioctls already are exactly that.
With the added advantage of being able to directly use
pass the vendor-cmd-specific struct to the ioctl instead
of having to first embed it in some other struct.
Regards,
Hans
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Gregor Riepl @ 2024-02-22 11:38 UTC (permalink / raw)
To: Werner Sembach
Cc: Lee Jones, jikos, linux-kernel, Jelle van der Waa, Miguel Ojeda,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds,
Pavel Machek, Hans de Goede
In-Reply-To: <b6d79727-ae94-44b1-aa88-069416435c14@redhat.com>
> This certainly is the most KISS approach. This proposal
> in essence is just an arbitrary command multiplexer /
> demultiplexer and ioctls already are exactly that.
>
> With the added advantage of being able to directly use
> pass the vendor-cmd-specific struct to the ioctl instead
> of having to first embed it in some other struct.
There's also the question of how much complexity needs to remain in the
kernel, if vendor-specific ioctls are made available.
Does every vendor driver implement a complex mapping to hardware
registers? What about drivers that basically implement no mapping at all
and simply forward all data to the hardware without any checking? The
latter case would match Pavel's concerns, although I don't see how this
is any different from the situation today, where userspace talks
directly to the hardware via libusb etc.
To be honest, I think the kernel shouldn't include too much high-level
complexity. If there is a desire to implement a generic display device
on top of the RGB device, this should be a configurable service running
in user space. The kernel should provide an interface to expose this
emulated display as a "real" display to applications - unless this can
also be done entirely in user space in a generic way.
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 04/16] bpf/helpers: introduce sleepable bpf_timers
From: Toke Høiland-Jørgensen @ 2024-02-22 11:50 UTC (permalink / raw)
To: Benjamin Tissoires, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Andrii Nakryiko, Martin KaFai Lau,
Eduard Zingerman, Song Liu, Yonghong Song, KP Singh,
Stanislav Fomichev, Hao Luo, Jiri Olsa, Jiri Kosina,
Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest,
Benjamin Tissoires
In-Reply-To: <20240221-hid-bpf-sleepable-v3-4-1fb378ca6301@kernel.org>
Benjamin Tissoires <bentiss@kernel.org> writes:
> @@ -1245,6 +1294,7 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callb
> ret = -EPERM;
> goto out;
> }
> + down(&t->sleepable_lock);
> prev = t->prog;
> if (prev != prog) {
> /* Bump prog refcnt once. Every bpf_timer_set_callback()
> @@ -1261,6 +1311,7 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callb
> t->prog = prog;
> }
> rcu_assign_pointer(t->callback_fn, callback_fn);
> + up(&t->sleepable_lock);
> out:
> __bpf_spin_unlock_irqrestore(&timer->lock);
> return ret;
> @@ -1282,7 +1333,7 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
>
> if (in_nmi())
> return -EOPNOTSUPP;
> - if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN))
> + if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN | BPF_F_TIMER_SLEEPABLE))
> return -EINVAL;
> __bpf_spin_lock_irqsave(&timer->lock);
> t = timer->timer;
> @@ -1299,7 +1350,10 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
> if (flags & BPF_F_TIMER_CPU_PIN)
> mode |= HRTIMER_MODE_PINNED;
>
> - hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
> + if (flags & BPF_F_TIMER_SLEEPABLE)
> + schedule_work(&t->work);
> + else
> + hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
> out:
> __bpf_spin_unlock_irqrestore(&timer->lock);
> return ret;
I think it's a little weird to just ignore the timeout parameter when
called with the sleepable flag. But I guess it can work at least as a
first pass; however, in that case we should enforce that the caller
passes in a timeout of 0, so that if we do add support for a timeout for
sleepable timers in the future, callers will be able to detect this.
-Toke
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Hans de Goede @ 2024-02-22 12:39 UTC (permalink / raw)
To: Gregor Riepl, Werner Sembach
Cc: Lee Jones, jikos, linux-kernel, Jelle van der Waa, Miguel Ojeda,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds,
Pavel Machek
In-Reply-To: <a21f6c49-2c05-4496-965c-a7524ed38634@gmail.com>
Hi,
On 2/22/24 12:38, Gregor Riepl wrote:
>> This certainly is the most KISS approach. This proposal
>> in essence is just an arbitrary command multiplexer /
>> demultiplexer and ioctls already are exactly that.
>>
>> With the added advantage of being able to directly use
>> pass the vendor-cmd-specific struct to the ioctl instead
>> of having to first embed it in some other struct.
>
> There's also the question of how much complexity needs to remain in the kernel, if vendor-specific ioctls are made available.
>
> Does every vendor driver implement a complex mapping to hardware registers? What about drivers that basically implement no mapping at all and simply forward all data to the hardware without any checking? The latter case would match Pavel's concerns, although I don't see how this is any different from the situation today, where userspace talks directly to the hardware via libusb etc.
This whole discussion got started by embedded-controller driven
keyboards in laptops with per key RGB lighting. We cannot just
allow userspace raw-access to the embedded-controller.
So these per vendor ioctl commands will need to do the minimum
to make sure userspace cannot do bad things. But yes complex
stuff like figuring out which LED(s) maps to say the enter key
should be left to userspace.
Especially since this can differ per keyboardlayout.
> To be honest, I think the kernel shouldn't include too much high-level complexity. If there is a desire to implement a generic display device on top of the RGB device, this should be a configurable service running in user space. The kernel should provide an interface to expose this emulated display as a "real" display to applications - unless this can also be done entirely in user space in a generic way.
We really need to stop seeing per key addressable RGB keyboards as displays:
1. Some "pixels" are non square
2. Not all "pixels" have the same width-height ratio
3. Not all rows have the same amount of pixels
4. There are holes in the rows like between the enter key and then numpad
5. Some "pixels" have multiple LEDs beneath them. These might be addressable
per LEDs are the sub-pixels ? What about a 2 key wide backspace key vs
the 1 key wide + another key (some non US layouts) in place of the backspace?
This will be "2 pixels" in some layout and 1 pixel with maybe / maybe-not
2 subpixels where the sub-pixels may/may not be individually addressable ?
For all these reasons the display analogy really is a bit fit for these keyboards
we tried to come up with a universal coordinate system for these at the beginning
of the thread and we failed ...
Regards,
Hans
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v3
From: Werner Sembach @ 2024-02-22 13:14 UTC (permalink / raw)
To: Hans de Goede
Cc: Lee Jones, jikos, linux-kernel, Jelle van der Waa, Miguel Ojeda,
dri-devel@lists.freedesktop.org, linux-input, ojeda, linux-leds,
Pavel Machek, Gregor Riepl
In-Reply-To: <825129ea-d389-4c6c-8a23-39f05572e4b4@redhat.com>
Hi,
Thanks everyone for the exhaustive feedback. And at least this thread is a good
comprehesive reference for the future ^^.
To recap the hopefully final UAPI for complex RGB lighting devices:
- By default there is a singular /sys/class/leds/* entry that treats the device
as if it was a single zone RGB keyboard backlight with no special effects.
- There is an accompanying misc device with the sysfs attributes "name",
"device_type", "firmware_version_string", "serial_number" for device
identification and "use_leds_uapi" that defaults to 1.
- If set to 0 the /sys/class/leds/* entry disappears. The driver should
keep the last state the backlight was in active if possible.
- If set 1 it appears again. The driver should bring it back to a static 1
zone setting while avoiding flicker if possible.
- If the device is not controllable by for example hidraw, the misc device might
also implement additional ioctls or sysfs attributes to allow a more complex low
level control for the keyboard backlight. This is will be a highly vendor
specific UAPI.
- The actual logic interacting with this low level UAPI is implemented by a
userspace driver
Implementation wise: For the creation of the misc device with the use_leds_uapi
switch a helper function/macro might be useful? Wonder if it should go into
leds.h, led-class-multicolor.h, or a new header file?
- Out of my head it would look something like this:
led_classdev_add_optional_misc_control(
struct led_classdev *led_cdev,
char* name,
char* device_type,
char* firmware_version_string,
char* serial_number,
void (*deregister_led)(struct led_classdev *led_cdev),
void (*reregister_led)(struct led_classdev *led_cdev))
Let me know your thoughts and hopefully I can start implementing it soon for one
of our devices.
Kind regards,
Werner Sembach
^ permalink raw reply
* Re: [PATCH] Input: synaptics-rmi4 - fix use after free (more completely) -- but not fully ;)
From: Mathias Krause @ 2024-02-22 14:08 UTC (permalink / raw)
To: Dan Carpenter, Dmitry Torokhov, Torsten Hilbrich, Nick Dyer
Cc: Jiapeng Chong, linux-input, linux-kernel, Brad Spengler,
Sasha Levin
In-Reply-To: <e77756aa-c0f1-44a8-bcd6-a46fe954b195@moroto.mountain>
Hi Dan, Torsten,
On 22.02.24 09:53, Dan Carpenter wrote:
> From: Brad Spengler <spender@grsecurity.net>
>
> Grsecurity uses memory poisoning and detected a use after free in
> rmi_unregister_function(). A fix for this was merged into the
> grsecurity patchset. I detected the user after free later using static
^^^^ use after free
> analysis and sent a partial fix as commit eb988e46da2e ("Input:
> synaptics-rmi4 - fix use after free in rmi_unregister_function()").
> However, my patch meant that the grsecurity patchset didn't apply cleanly
> and forced everyone to re-visit this issue.
Indeed. We even noticed that your patch was incomplete back then but
urgent matters kept us from following-up. Then came holidays and this
simply went forgotten :(
>
> The bug is that we must not call device_del() until after calling
> irq_dispose_mapping().
Unfortunately, this is only half the truth. We investigated this further
and there's another bug that got introduced in commit 24d28e4f1271
("Input: synaptics-rmi4 - convert irq distribution to irq_domain"). The
IRQ domain has a UAF issue as well. I'll send the patch soon -- wanted
to do so this week, but, again, more urgent matters interrupted this.
>
> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
> Closes: https://lore.kernel.org/all/1932038e-2776-04ac-5fcd-b15bb3cd088d@secunet.com/
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215604
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> drivers/input/rmi4/rmi_bus.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
> index 1b45b1d3077d..02acc81b9d3e 100644
> --- a/drivers/input/rmi4/rmi_bus.c
> +++ b/drivers/input/rmi4/rmi_bus.c
> @@ -275,12 +275,11 @@ void rmi_unregister_function(struct rmi_function *fn)
> rmi_dbg(RMI_DEBUG_CORE, &fn->dev, "Unregistering F%02X.\n",
> fn->fd.function_number);
>
> - device_del(&fn->dev);
> - of_node_put(fn->dev.of_node);
> -
> for (i = 0; i < fn->num_of_irqs; i++)
> irq_dispose_mapping(fn->irq[i]);
>
> + device_del(&fn->dev);
> + of_node_put(fn->dev.of_node);
> put_device(&fn->dev);
> }
>
Acked-by: Mathias Krause <minipli@grsecurity.net>
^ permalink raw reply
* Re: [PATCH] Input: synaptics-rmi4 - fix use after free (more completely) -- but not fully ;)
From: Mathias Krause @ 2024-02-22 14:19 UTC (permalink / raw)
To: Dan Carpenter, Dmitry Torokhov, Torsten Hilbrich, Nick Dyer
Cc: Jiapeng Chong, linux-input, linux-kernel, Brad Spengler,
Sasha Levin
In-Reply-To: <49c935cf-4bf5-456f-8bc0-d6dbf0f8cf0f@grsecurity.net>
On 22.02.24 15:08, Mathias Krause wrote:
>>
>> The bug is that we must not call device_del() until after calling
>> irq_dispose_mapping().
>
> Unfortunately, this is only half the truth. We investigated this further
> and there's another bug that got introduced in commit 24d28e4f1271
> ("Input: synaptics-rmi4 - convert irq distribution to irq_domain"). The
> IRQ domain has a UAF issue as well. I'll send the patch soon -- wanted
> to do so this week, but, again, more urgent matters interrupted this.
Unfortunately, I send that Email too fast. Looking at the backtrace
again, it's just the other bug that needs fixing (a UAF in
rmi_driver_remove()). Will sent a patch really soon now!
>
>>
>> Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
>> Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
>> Closes: https://lore.kernel.org/all/1932038e-2776-04ac-5fcd-b15bb3cd088d@secunet.com/
>> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=215604
>> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
>> ---
>> drivers/input/rmi4/rmi_bus.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/rmi4/rmi_bus.c b/drivers/input/rmi4/rmi_bus.c
>> index 1b45b1d3077d..02acc81b9d3e 100644
>> --- a/drivers/input/rmi4/rmi_bus.c
>> +++ b/drivers/input/rmi4/rmi_bus.c
>> @@ -275,12 +275,11 @@ void rmi_unregister_function(struct rmi_function *fn)
>> rmi_dbg(RMI_DEBUG_CORE, &fn->dev, "Unregistering F%02X.\n",
>> fn->fd.function_number);
>>
>> - device_del(&fn->dev);
>> - of_node_put(fn->dev.of_node);
>> -
>> for (i = 0; i < fn->num_of_irqs; i++)
>> irq_dispose_mapping(fn->irq[i]);
>>
>> + device_del(&fn->dev);
>> + of_node_put(fn->dev.of_node);
>> put_device(&fn->dev);
>> }
>>
>
> Acked-by: Mathias Krause <minipli@grsecurity.net>
So, forget about that one, sorry.
^ permalink raw reply
* [PATCH] Input: synaptics-rmi4 - fix UAF of IRQ domain on driver removal
From: Mathias Krause @ 2024-02-22 14:26 UTC (permalink / raw)
To: Dmitry Torokhov, Torsten Hilbrich
Cc: Dan Carpenter, Nick Dyer, Brad Spengler, Sasha Levin,
Jiapeng Chong, linux-input, linux-kernel, Mathias Krause
Calling irq_domain_remove() will lead to freeing the IRQ domain
prematurely. The domain is still referenced and will be attempted to get
used via rmi_free_function_list() -> rmi_unregister_function() ->
irq_dispose_mapping() -> irq_get_irq_data()'s ->domain pointer.
With PaX's MEMORY_SANITIZE this will lead to an access fault when
attempting to dereference embedded pointers, as in Torsten's report that
was faulting on the 'domain->ops->unmap' test.
Fix this by releasing the IRQ domain only after all related IRQs have
been deactivated.
Fixes: 24d28e4f1271 ("Input: synaptics-rmi4 - convert irq distribution to irq_domain")
Reported-by: Torsten Hilbrich <torsten.hilbrich@secunet.com>
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
Torsten, could you please confirm (again) that this patch, instead of
[1], fixes the bug for you?
[1] https://lore.kernel.org/lkml/e77756aa-c0f1-44a8-bcd6-a46fe954b195@moroto.mountain/
drivers/input/rmi4/rmi_driver.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/input/rmi4/rmi_driver.c b/drivers/input/rmi4/rmi_driver.c
index 258d5fe3d395..42eaebb3bf5c 100644
--- a/drivers/input/rmi4/rmi_driver.c
+++ b/drivers/input/rmi4/rmi_driver.c
@@ -978,12 +978,12 @@ static int rmi_driver_remove(struct device *dev)
rmi_disable_irq(rmi_dev, false);
+ rmi_f34_remove_sysfs(rmi_dev);
+ rmi_free_function_list(rmi_dev);
+
irq_domain_remove(data->irqdomain);
data->irqdomain = NULL;
- rmi_f34_remove_sysfs(rmi_dev);
- rmi_free_function_list(rmi_dev);
-
return 0;
}
--
2.30.2
^ permalink raw reply related
* Re: Future handling of complex RGB devices on Linux v2
From: Pavel Machek @ 2024-02-22 17:23 UTC (permalink / raw)
To: Hans de Goede
Cc: Werner Sembach, Lee Jones, jikos, linux-kernel, Jelle van der Waa,
Miguel Ojeda, dri-devel@lists.freedesktop.org, linux-input, ojeda,
linux-leds
In-Reply-To: <b6d79727-ae94-44b1-aa88-069416435c14@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 922 bytes --]
Hi!
> > Yeah, so ... this is not a interface. This is a backdoor to pass
> > arbitrary data. That's not going to fly.
>
> Pavel, Note the data will be interpreted by a kernel driver and
> not passed directly to the hw.
Yes, still not flying :-).
> With that said I tend to agree that this seems to be a bit too
> generic.
Exactly.
> Given that these devices are all different in various ways
> and that we only want this for devices which cannot be accessed
> from userspace directly (so a limit set of devices) I really
> think that just doing custom ioctls per vendor is best.
I don't think that's good idea in the long term. Kernel should provide
hardware abstraction, so that userspace does not need to know about
hardware. Obviously there are exceptions, but this should not be one
of those.
BR,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Pavel Machek @ 2024-02-22 17:38 UTC (permalink / raw)
To: Pekka Paalanen
Cc: Werner Sembach, Hans de Goede, Lee Jones, jikos, linux-kernel,
Jelle van der Waa, Miguel Ojeda, dri-devel@lists.freedesktop.org,
linux-input, ojeda, linux-leds
In-Reply-To: <20240222110457.71618f27@eldfell>
[-- Attachment #1: Type: text/plain, Size: 4397 bytes --]
Hi!
> > > so after more feedback from the OpenRGB maintainers I came up with an even
> > > more generic proposal:
> > > https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
> >
> > > >evaluate-set-command ioctl taking:
> > > >{
> > > > enum command /* one of supported_commands */
> > > > union data
> > > > {
> > > > char raw[3072],
> > > > {
> > > > <input struct for command 0>
> > > > },
> >
> > Yeah, so ... this is not a interface. This is a backdoor to pass
> > arbitrary data. That's not going to fly.
> >
> > For keyboards, we don't need complete new interface; we reasonable
> > extensions over existing display APIs -- keyboards are clearly 2D.
>
> I suppose they could be seen as *a* display, but if you are referring
> to DRM KMS UAPI, then no, I don't see that fitting at all:
So -- we already have very similar displays in
drivers/auxdisplay. drivers/auxdisplay/cfag12864b.c is 128x64 display,
1-bit display for example.
> - the "pixel grid" is not orthogonal, it's not a rectangle, and it
> might not be a grid at all
It is quite close to orthogonal. I'd suggest simply pretending it is
orthogonal grid with some pixels missing :-). We already have
cellphone displays with rounded corners and holes in them, so I
suspect handling of missing pixels will be neccessary anyway.
> - Timings and video modes? DRM KMS has always been somewhat awkward for
> display devices that do not have an inherent scanout cycle and timings
> totally depend on the amount of pixels updated at a time
> (FB_DAMAGE_CLIPS), e.g. USB displays (not USB-C DP alt mode).
> They do work, but they are very different from the usual hardware
> involved with KMS, require special consideration in userspace, and
> they still are actual displays while what we're talking about here
> are not.
As you say, there are other displays with similar problems.
> - KMS has no concept of programmed autonomous animations, and likely
> never will. They are not useful with actual displays.
Yep. We need some kind of extension here, and this is likely be quite
vendor-specific, as animations will differ between vendors. I guess
"please play pattern xyzzy with parametrs 3 and 5" might be enough for this.
> - Userspace will try to light up KMS outputs automatically and extend
> the traditional desktop there. This was already a problem for
> head-mounted displays (HMD) where it made no sense. That was worked
> around with an in-kernel list of HMDs and some KMS property
> quirking.
This will need fixing for cfag12864b.c, no? Perhaps userspace should
simply ignore anything smaller than 240x160 or something...
> Modern KMS UAPI very much aims to be a generic UAPI that abstracts
> display devices. It already breaks down a little for things like USB
> displays and virtual machines (e.g. qemu, vmware, especially with
> remote viewers), which I find unfortunate. With HMDs the genericity
> breaks down in other ways, but I'd claim HMDs are a better fit still
> than full-featured VM virtual displays (cursor plane hijacking). With
> non-displays like keyboards the genericity would be completely lost, as
> they won't work at all the same way as displays. You cannot even show
> proper images there, only coarse light patterns *IF* you actually know
> the pixel layout. But the pixel layout is(?) hardware-specific which is
> the opposite of generic.
>
> While you could dress keyboard lights etc. up with DRM KMS UAPI, the
> userspace would have to be written from scratch for them, and you
> somehow need to make existing KMS userspace to never touch those
> devices. What's the point of using DRM KMS UAPI in the first place,
> then?
Well, at least we have good UAPI to work with. Other options were 100
files in /sys/class/leds, pretending it is a linear array of leds,
just passing raw data around, and pretending it is grid of RGB888
data.
Anyway, there are devices such as these: (8x8 LED display).
https://www.laskakit.cz/8x8-led-matice-s-max7219-3mm-cervena/
We should think about what interface we want for these, and then I
believe we should make RGB keyboards use something similar.
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Pavel Machek @ 2024-02-22 17:42 UTC (permalink / raw)
To: Hans de Goede
Cc: Gregor Riepl, Werner Sembach, Lee Jones, jikos, linux-kernel,
Jelle van der Waa, Miguel Ojeda, dri-devel@lists.freedesktop.org,
linux-input, ojeda, linux-leds
In-Reply-To: <825129ea-d389-4c6c-8a23-39f05572e4b4@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 1721 bytes --]
Hi!
> > To be honest, I think the kernel shouldn't include too much high-level complexity. If there is a desire to implement a generic display device on top of the RGB device, this should be a configurable service running in user space. The kernel should provide an interface to expose this emulated display as a "real" display to applications - unless this can also be done entirely in user space in a generic way.
>
> We really need to stop seeing per key addressable RGB keyboards as displays:
>
> 1. Some "pixels" are non square
> 2. Not all "pixels" have the same width-height ratio
They are quite close to square usually.
> 3. Not all rows have the same amount of pixels
True for cellphone displays, too. Rounded corners.
> 4. There are holes in the rows like between the enter key and then numpad
True for cellphone displays, too. Hole for camera.
> 5. Some "pixels" have multiple LEDs beneath them. These might be addressable
> per LEDs are the sub-pixels ? What about a 2 key wide backspace key vs
> the 1 key wide + another key (some non US layouts) in place of the backspace?
> This will be "2 pixels" in some layout and 1 pixel with maybe / maybe-not
> 2 subpixels where the sub-pixels may/may not be individually addressable ?
Treat those "sub pixels" as pixels. They will be in same matrix as the rest.
> For all these reasons the display analogy really is a bit fit for these keyboards
> we tried to come up with a universal coordinate system for these at the beginning
> of the thread and we failed ...
I'd suggest trying harder this time :-).
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Pavel Machek @ 2024-02-22 17:52 UTC (permalink / raw)
To: Hans de Goede
Cc: Gregor Riepl, Werner Sembach, Lee Jones, jikos, linux-kernel,
Jelle van der Waa, Miguel Ojeda, dri-devel@lists.freedesktop.org,
linux-input, ojeda, linux-leds
In-Reply-To: <825129ea-d389-4c6c-8a23-39f05572e4b4@redhat.com>
[-- Attachment #1: Type: text/plain, Size: 755 bytes --]
Hi!
> For all these reasons the display analogy really is a bit fit for these keyboards
> we tried to come up with a universal coordinate system for these at the beginning
> of the thread and we failed ...
I quite liked the coordinate system proposal. I can propose this:
Vendor maps the keyboard lights to a grid. That would be something
16x8 for thinkpad X220. Then we provide functionality to query "is a
working pixel there" and "what kind of key is at this pixel" -- I
guess we can use input keycodes for that. Multiple pixels can map to
one keycode.
(And then we make best effort to map normal keyboards into similar
grids).
Best regards,
Pavel
--
People of Russia, stop Putin before his war on Ukraine escalates.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 09/16] HID: bpf/dispatch: regroup kfuncs definitions
From: Eduard Zingerman @ 2024-02-22 20:17 UTC (permalink / raw)
To: Benjamin Tissoires, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest
In-Reply-To: <20240221-hid-bpf-sleepable-v3-9-1fb378ca6301@kernel.org>
On Wed, 2024-02-21 at 17:25 +0100, Benjamin Tissoires wrote:
[...]
> diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
> index e630caf644e8..52abb27426f4 100644
> --- a/drivers/hid/bpf/hid_bpf_dispatch.c
> +++ b/drivers/hid/bpf/hid_bpf_dispatch.c
> @@ -143,48 +143,6 @@ u8 *call_hid_bpf_rdesc_fixup(struct hid_device *hdev, u8 *rdesc, unsigned int *s
> }
> EXPORT_SYMBOL_GPL(call_hid_bpf_rdesc_fixup);
>
> -/* Disables missing prototype warnings */
> -__bpf_kfunc_start_defs();
Note:
this patch does not apply on top of current bpf-next [0] because
__bpf_kfunc_start_defs and __bpf_kfunc are not present in [0].
[0] commit 58fd62e0aa50 ("bpf: Clarify batch lookup/lookup_and_delete semantics")
> -
> -/**
> - * hid_bpf_get_data - Get the kernel memory pointer associated with the context @ctx
> - *
> - * @ctx: The HID-BPF context
> - * @offset: The offset within the memory
> - * @rdwr_buf_size: the const size of the buffer
> - *
> - * @returns %NULL on error, an %__u8 memory pointer on success
> - */
> -__bpf_kfunc __u8 *
> -hid_bpf_get_data(struct hid_bpf_ctx *ctx, unsigned int offset, const size_t rdwr_buf_size)
> -{
> - struct hid_bpf_ctx_kern *ctx_kern;
> -
> - if (!ctx)
> - return NULL;
[...]
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 04/16] bpf/helpers: introduce sleepable bpf_timers
From: Alexei Starovoitov @ 2024-02-22 20:47 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan, bpf,
LKML, open list:HID CORE LAYER, open list:DOCUMENTATION,
open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <20240221-hid-bpf-sleepable-v3-4-1fb378ca6301@kernel.org>
On Wed, Feb 21, 2024 at 8:25 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> /* the actual struct hidden inside uapi struct bpf_timer */
> @@ -1113,6 +1120,55 @@ struct bpf_timer_kern {
> struct bpf_spin_lock lock;
> } __attribute__((aligned(8)));
>
> +static u32 __bpf_timer_compute_key(struct bpf_hrtimer *timer)
> +{
> + struct bpf_map *map = timer->map;
> + void *value = timer->value;
> +
> + if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> +
> + /* compute the key */
> + return ((char *)value - array->value) / array->elem_size;
> + }
> +
> + /* hash or lru */
> + return *(u32 *)(value - round_up(map->key_size, 8));
> +}
> +
> +static void bpf_timer_work_cb(struct work_struct *work)
> +{
> + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
> + struct bpf_map *map = t->map;
> + void *value = t->value;
> + bpf_callback_t callback_fn;
> + u32 key;
> +
> + BTF_TYPE_EMIT(struct bpf_timer);
> +
> + down(&t->sleepable_lock);
> +
> + callback_fn = READ_ONCE(t->callback_fn);
> + if (!callback_fn) {
> + up(&t->sleepable_lock);
> + return;
> + }
> +
> + key = __bpf_timer_compute_key(t);
> +
> +
> + callback_fn((u64)(long)map, (u64)(long)&key, (u64)(long)value, 0, 0);
> + /* The verifier checked that return value is zero. */
> +
> + bpf_prog_put(t->prog);
> +}
> +
> static DEFINE_PER_CPU(struct bpf_hrtimer *, hrtimer_running);
>
> static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
> @@ -1121,8 +1177,7 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
> struct bpf_map *map = t->map;
> void *value = t->value;
> bpf_callback_t callback_fn;
> - void *key;
> - u32 idx;
> + u32 key;
>
> BTF_TYPE_EMIT(struct bpf_timer);
> callback_fn = rcu_dereference_check(t->callback_fn, rcu_read_lock_bh_held());
> @@ -1136,17 +1191,9 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
> * bpf_map_delete_elem() on the same timer.
> */
> this_cpu_write(hrtimer_running, t);
> - if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> - struct bpf_array *array = container_of(map, struct bpf_array, map);
> -
> - /* compute the key */
> - idx = ((char *)value - array->value) / array->elem_size;
> - key = &idx;
> - } else { /* hash or lru */
> - key = value - round_up(map->key_size, 8);
> - }
> + key = __bpf_timer_compute_key(t);
Please don't mix such "cleanup" with main changes.
It's buggy for a hash map.
Instead of passing a pointer to the real key into bpf prog
you're reading the first 4 bytes from the key. Copying it into a temp var
and passing an address to that.
It would have been very painful to debug such a bug if it slipped through,
since bpf prog would sort-of work for 4-byte keys.
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 06/16] bpf/helpers: introduce bpf_timer_set_sleepable_cb() kfunc
From: Alexei Starovoitov @ 2024-02-22 20:53 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan, bpf,
LKML, open list:HID CORE LAYER, open list:DOCUMENTATION,
open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <20240221-hid-bpf-sleepable-v3-6-1fb378ca6301@kernel.org>
On Wed, Feb 21, 2024 at 8:25 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> In this patch, bpf_timer_set_sleepable_cb() is functionally equivalent
> to bpf_timer_set_callback(), to the exception that it enforces
> the timer to be started with BPF_F_TIMER_SLEEPABLE.
>
> But given that bpf_timer_set_callback() is a helper when
> bpf_timer_set_sleepable_cb() is a kfunc, we need to teach the verifier
> about its attached callback.
> Marking that callback as sleepable will be done in a separate patch
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> new in v3 (split from v2 02/10)
> ---
> kernel/bpf/helpers.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++--
> kernel/bpf/verifier.c | 31 +++++++++++++++++++++++++++++--
> 2 files changed, 75 insertions(+), 4 deletions(-)
>
> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> index f9add0abe40a..2c6dc3d0ffff 100644
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
> @@ -1108,6 +1108,7 @@ struct bpf_hrtimer {
> void __rcu *callback_fn;
> void *value;
> struct semaphore sleepable_lock;
> + bool is_sleepable;
> };
>
> /* the actual struct hidden inside uapi struct bpf_timer */
> @@ -1270,8 +1271,8 @@ static const struct bpf_func_proto bpf_timer_init_proto = {
> .arg3_type = ARG_ANYTHING,
> };
>
> -BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
> - struct bpf_prog_aux *, aux)
> +static int __bpf_timer_set_callback(struct bpf_timer_kern *timer, void *callback_fn,
> + struct bpf_prog_aux *aux, bool is_sleepable)
> {
> struct bpf_prog *prev, *prog = aux->prog;
> struct bpf_hrtimer *t;
> @@ -1311,12 +1312,19 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callb
> t->prog = prog;
> }
> rcu_assign_pointer(t->callback_fn, callback_fn);
> + t->is_sleepable = is_sleepable;
> up(&t->sleepable_lock);
> out:
> __bpf_spin_unlock_irqrestore(&timer->lock);
> return ret;
> }
>
> +BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callback_fn,
> + struct bpf_prog_aux *, aux)
> +{
> + return __bpf_timer_set_callback(timer, callback_fn, aux, false);
> +}
> +
> static const struct bpf_func_proto bpf_timer_set_callback_proto = {
> .func = bpf_timer_set_callback,
> .gpl_only = true,
> @@ -1342,6 +1350,11 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
> goto out;
> }
>
> + if (t->is_sleepable && !(flags & BPF_F_TIMER_SLEEPABLE)) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> if (flags & BPF_F_TIMER_ABS)
> mode = HRTIMER_MODE_ABS_SOFT;
> else
> @@ -2606,6 +2619,36 @@ __bpf_kfunc void bpf_throw(u64 cookie)
> WARN(1, "A call to BPF exception callback should never return\n");
> }
>
> +/**
> + * bpf_timer_set_sleepable_cb() - Configure the timer to call %callback_fn
> + * static function in a sleepable context.
> + * @timer: The bpf_timer that needs to be configured
> + * @callback_fn: a static bpf function
> + *
> + * @returns %0 on success. %-EINVAL if %timer was not initialized with
> + * bpf_timer_init() earlier. %-EPERM if %timer is in a map that doesn't
> + * have any user references.
> + * The user space should either hold a file descriptor to a map with timers
> + * or pin such map in bpffs. When map is unpinned or file descriptor is
> + * closed all timers in the map will be cancelled and freed.
> + *
> + * This kfunc is equivalent to %bpf_timer_set_callback except that it tells
> + * the verifier that the target callback is run in a sleepable context.
> + */
> +__bpf_kfunc int bpf_timer_set_sleepable_cb(struct bpf_timer_kern *timer,
> + int (callback_fn)(void *map, int *key, struct bpf_timer *timer))
> +{
> + struct bpf_throw_ctx ctx = {};
> +
> + arch_bpf_stack_walk(bpf_stack_walker, &ctx);
> + WARN_ON_ONCE(!ctx.aux);
Sorry. Why such complexity?
Please see how do_misc_fixups() handles BPF_FUNC_timer_set_callback.
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 04/16] bpf/helpers: introduce sleepable bpf_timers
From: Eduard Zingerman @ 2024-02-22 22:40 UTC (permalink / raw)
To: Benjamin Tissoires, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest
In-Reply-To: <20240221-hid-bpf-sleepable-v3-4-1fb378ca6301@kernel.org>
On Wed, 2024-02-21 at 17:25 +0100, Benjamin Tissoires wrote:
[...]
> @@ -1282,7 +1333,7 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
>
> if (in_nmi())
> return -EOPNOTSUPP;
> - if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN))
> + if (flags & ~(BPF_F_TIMER_ABS | BPF_F_TIMER_CPU_PIN | BPF_F_TIMER_SLEEPABLE))
> return -EINVAL;
> __bpf_spin_lock_irqsave(&timer->lock);
> t = timer->timer;
> @@ -1299,7 +1350,10 @@ BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, fla
> if (flags & BPF_F_TIMER_CPU_PIN)
> mode |= HRTIMER_MODE_PINNED;
>
> - hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
> + if (flags & BPF_F_TIMER_SLEEPABLE)
> + schedule_work(&t->work);
> + else
> + hrtimer_start(&t->timer, ns_to_ktime(nsecs), mode);
It looks like nsecs is simply ignored for sleepable timers.
Should this be hrtimer_start() that waits nsecs and schedules work,
or schedule_delayed_work()? (but it takes delay in jiffies, which is
probably too coarse). Sorry if I miss something.
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 05/16] bpf/verifier: add bpf_timer as a kfunc capable type
From: Eduard Zingerman @ 2024-02-23 0:22 UTC (permalink / raw)
To: Benjamin Tissoires, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest
In-Reply-To: <20240221-hid-bpf-sleepable-v3-5-1fb378ca6301@kernel.org>
On Wed, 2024-02-21 at 17:25 +0100, Benjamin Tissoires wrote:
[...]
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index f81c799b2c80..2b11687063ff 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -5444,6 +5444,26 @@ static int check_map_access(struct bpf_verifier_env *env, u32 regno,
> return -EACCES;
> }
> break;
> + case BPF_TIMER:
> + /* FIXME: kptr does the above, should we use the same? */
I don't think so.
Basically this allows double word reads / writes from timer address,
which probably should not be allowed.
The ACCESS_DIRECT is passed to check_map_access() from
check_mem_access() and I don't see points where check_mem_access()
call would be triggered for pointer parameter of kfunc
(unless it is accompanied by a size parameter).
I tried the following simple program and it verifies fine:
struct elem {
struct bpf_timer t;
};
struct {
__uint(type, BPF_MAP_TYPE_ARRAY);
__uint(max_entries, 2);
__type(key, int);
__type(value, struct elem);
} array SEC(".maps");
int bpf_timer_set_sleepable_cb
(struct bpf_timer *timer,
int (callback_fn)(void *map, int *key, struct bpf_timer *timer))
__ksym __weak;
static int cb_sleepable(void *map, int *key, struct bpf_timer *timer)
{
return 0;
}
SEC("fentry/bpf_fentry_test5")
int BPF_PROG2(test_sleepable, int, a)
{
struct bpf_timer *arr_timer;
int array_key = ARRAY;
arr_timer = bpf_map_lookup_elem(&array, &array_key);
if (!arr_timer)
return 0;
bpf_timer_init(arr_timer, &array, CLOCK_MONOTONIC);
bpf_timer_set_sleepable_cb(arr_timer, cb_sleepable);
bpf_timer_start(arr_timer, 0, 0);
return 0;
}
(in general, it would be easier to review if there were some test
cases to play with).
> + if (src != ACCESS_DIRECT) {
> + verbose(env, "bpf_timer cannot be accessed indirectly by helper\n");
> + return -EACCES;
> + }
> + if (!tnum_is_const(reg->var_off)) {
> + verbose(env, "bpf_timer access cannot have variable offset\n");
> + return -EACCES;
> + }
> + if (p != off + reg->var_off.value) {
> + verbose(env, "bpf_timer access misaligned expected=%u off=%llu\n",
> + p, off + reg->var_off.value);
> + return -EACCES;
> + }
> + if (size != bpf_size_to_bytes(BPF_DW)) {
> + verbose(env, "bpf_timer access size must be BPF_DW\n");
> + return -EACCES;
> + }
> + break;
> default:
> verbose(env, "%s cannot be accessed directly by load/store\n",
> btf_field_type_name(field->type));
[...]
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 05/16] bpf/verifier: add bpf_timer as a kfunc capable type
From: Eduard Zingerman @ 2024-02-23 0:26 UTC (permalink / raw)
To: Benjamin Tissoires, Alexei Starovoitov, Daniel Borkmann,
John Fastabend, Andrii Nakryiko, Martin KaFai Lau, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan
Cc: bpf, linux-kernel, linux-input, linux-doc, linux-kselftest
In-Reply-To: <dfbb1464e99e057d77f78395d985208d6510040d.camel@gmail.com>
On Fri, 2024-02-23 at 02:22 +0200, Eduard Zingerman wrote:
[...]
> > + case BPF_TIMER:
> > + /* FIXME: kptr does the above, should we use the same? */
[...]
> I tried the following simple program and it verifies fine:
Sorry, I meant that I tried it with the above check removed.
^ permalink raw reply
* Re: [PATCH RFC bpf-next v3 02/16] bpf/verifier: introduce in_sleepable() helper
From: Alexei Starovoitov @ 2024-02-23 1:56 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Daniel Borkmann, John Fastabend,
Andrii Nakryiko, Martin KaFai Lau, Eduard Zingerman, Song Liu,
Yonghong Song, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Jiri Kosina, Benjamin Tissoires, Jonathan Corbet, Shuah Khan, bpf,
LKML, open list:HID CORE LAYER, open list:DOCUMENTATION,
open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <20240221-hid-bpf-sleepable-v3-2-1fb378ca6301@kernel.org>
On Wed, Feb 21, 2024 at 8:25 AM Benjamin Tissoires <bentiss@kernel.org> wrote:
> @@ -18193,7 +18198,7 @@ static int resolve_pseudo_ldimm64(struct bpf_verifier_env *env)
> return -E2BIG;
> }
>
> - if (env->prog->aux->sleepable)
> + if (in_sleepable(env))
> atomic64_inc(&map->sleepable_refcnt);
this one doesn't look correct.
The verifier didn't start its main loop when resolve_pseudo_ldimm64()
is called.
It also loses symmetry with other sleepable_refcnt operations
in syscall.c and core.c
I reverted this hunk and applied patches 1,2,3
with minor edits, like removing unnecessary parens in patch 3,
and patch subject rewords.
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Werner Sembach @ 2024-02-23 8:33 UTC (permalink / raw)
To: Pavel Machek
Cc: Hans de Goede, Lee Jones, jikos, linux-kernel, Jelle van der Waa,
Miguel Ojeda, dri-devel@lists.freedesktop.org, linux-input, ojeda,
linux-leds
In-Reply-To: <ZdZ2kMASawJ9wdZj@duo.ucw.cz>
Hi,
Am 21.02.24 um 23:17 schrieb Pavel Machek:
> Hi!
>
>> so after more feedback from the OpenRGB maintainers I came up with an even
>> more generic proposal:
>> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>>> evaluate-set-command ioctl taking:
>>> {
>>> enum command /* one of supported_commands */
>>> union data
>>> {
>>> char raw[3072],
>>> {
>>> <input struct for command 0>
>>> },
> Yeah, so ... this is not a interface. This is a backdoor to pass
> arbitrary data. That's not going to fly.
>
> For keyboards, we don't need complete new interface; we reasonable
> extensions over existing display APIs -- keyboards are clearly 2D.
Maybe we should look on this from a users perspective: Running custom Animation
(i.e. where treating it as a display would be helpfull) is only >one< of the
ways a user might want to use the keyboard backlight.
Equally viable are for example:
- Having it mono colored.
- Playing a hardware effect
- Playing a hardware effect on one side of the keyboard while having the other
side of the keyboard playing a custom animation (As I learned from the OpenRGB
maintainers: There are keyboards which allow this)
There is no reason to define a custom animation as the default and then just
bolt the other options on top as it might not even be possible for some devices
where the firmware is plainly to slow for custom animations.
Also I still think a 2*2, 1*3 or even 1*1 matrix is not a display, coming back
aground to the earlier point where I want to implement this for keyboard first,
but this discussion is also thought to find a way that works for all complex RGB
devices. And I don't think it is a good idea find a way that works for keyboards
and then introduce again something completely new for other device categories.
>
> Best regards,
> Pavel
Best regards,
Werner
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Pekka Paalanen @ 2024-02-23 8:53 UTC (permalink / raw)
To: Pavel Machek
Cc: Werner Sembach, Hans de Goede, Lee Jones, jikos, linux-kernel,
Jelle van der Waa, Miguel Ojeda, dri-devel@lists.freedesktop.org,
linux-input, ojeda, linux-leds
In-Reply-To: <ZdeGiMf2npmzJidU@duo.ucw.cz>
[-- Attachment #1: Type: text/plain, Size: 6142 bytes --]
On Thu, 22 Feb 2024 18:38:16 +0100
Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
>
> > > > so after more feedback from the OpenRGB maintainers I came up with an even
> > > > more generic proposal:
> > > > https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
> > >
> > > > >evaluate-set-command ioctl taking:
> > > > >{
> > > > > enum command /* one of supported_commands */
> > > > > union data
> > > > > {
> > > > > char raw[3072],
> > > > > {
> > > > > <input struct for command 0>
> > > > > },
> > >
> > > Yeah, so ... this is not a interface. This is a backdoor to pass
> > > arbitrary data. That's not going to fly.
> > >
> > > For keyboards, we don't need complete new interface; we reasonable
> > > extensions over existing display APIs -- keyboards are clearly 2D.
> >
> > I suppose they could be seen as *a* display, but if you are referring
> > to DRM KMS UAPI, then no, I don't see that fitting at all:
>
> So -- we already have very similar displays in
> drivers/auxdisplay. drivers/auxdisplay/cfag12864b.c is 128x64 display,
> 1-bit display for example.
Auxdisplay are not exposed as DRM KMS device, or are they?
I don't see cfag12864b.c using any DRM calls.
DRM drivers are under drivers/gpu/drm.
I know nothing about auxdisplay. If it's fbdev UAPI, then I don't know -
people have been trying to kill it for years, and basing anything on it
seems like a dead idea. Or maybe it's ok for extremely small displays
where there is no display controller to speak of, and definitely no use
ever for dmabuf? Where CPU banging bits of raw pixels is enough, and no
desktop would ever want to touch them.
But I'm not talking about fbdev either, I'm talking about DRM KMS.
>
> > - the "pixel grid" is not orthogonal, it's not a rectangle, and it
> > might not be a grid at all
>
> It is quite close to orthogonal. I'd suggest simply pretending it is
> orthogonal grid with some pixels missing :-). We already have
> cellphone displays with rounded corners and holes in them, so I
> suspect handling of missing pixels will be neccessary anyway.
Yes, but I do not agree on the similarity at all, nor that it would be
"close to orthogonal" by simply looking at my keyboard. Hans de Goede
iterated on this much better than I.
> > - Timings and video modes? DRM KMS has always been somewhat awkward for
> > display devices that do not have an inherent scanout cycle and timings
> > totally depend on the amount of pixels updated at a time
> > (FB_DAMAGE_CLIPS), e.g. USB displays (not USB-C DP alt mode).
> > They do work, but they are very different from the usual hardware
> > involved with KMS, require special consideration in userspace, and
> > they still are actual displays while what we're talking about here
> > are not.
>
> As you say, there are other displays with similar problems.
That's no justification to add even more problems.
> > - KMS has no concept of programmed autonomous animations, and likely
> > never will. They are not useful with actual displays.
>
> Yep. We need some kind of extension here, and this is likely be quite
> vendor-specific, as animations will differ between vendors. I guess
> "please play pattern xyzzy with parametrs 3 and 5" might be enough for this.
Right. I very much believe that is not going to fly in DRM KMS.
> > - Userspace will try to light up KMS outputs automatically and extend
> > the traditional desktop there. This was already a problem for
> > head-mounted displays (HMD) where it made no sense. That was worked
> > around with an in-kernel list of HMDs and some KMS property
> > quirking.
>
> This will need fixing for cfag12864b.c, no? Perhaps userspace should
> simply ignore anything smaller than 240x160 or something...
Yes, a size limit would make sense in desktop usage.
> > Modern KMS UAPI very much aims to be a generic UAPI that abstracts
> > display devices. It already breaks down a little for things like USB
> > displays and virtual machines (e.g. qemu, vmware, especially with
> > remote viewers), which I find unfortunate. With HMDs the genericity
> > breaks down in other ways, but I'd claim HMDs are a better fit still
> > than full-featured VM virtual displays (cursor plane hijacking). With
> > non-displays like keyboards the genericity would be completely lost, as
> > they won't work at all the same way as displays. You cannot even show
> > proper images there, only coarse light patterns *IF* you actually know
> > the pixel layout. But the pixel layout is(?) hardware-specific which is
> > the opposite of generic.
> >
> > While you could dress keyboard lights etc. up with DRM KMS UAPI, the
> > userspace would have to be written from scratch for them, and you
> > somehow need to make existing KMS userspace to never touch those
> > devices. What's the point of using DRM KMS UAPI in the first place,
> > then?
>
> Well, at least we have good UAPI to work with.
..Where the great majority of that UAPI is ill-fitting for the device,
and requires userspace to use existing UAPI in completely new ways. KMS
UAPI is also very complex to use, because the devices it is meant for
are complex, timing sensitive, and capable of image processing.
> Other options were 100
> files in /sys/class/leds, pretending it is a linear array of leds,
> just passing raw data around, and pretending it is grid of RGB888
> data.
>
> Anyway, there are devices such as these: (8x8 LED display).
>
> https://www.laskakit.cz/8x8-led-matice-s-max7219-3mm-cervena/
>
> We should think about what interface we want for these, and then I
> believe we should make RGB keyboards use something similar.
Unfortunately I cannot say anything about any other UAPI options. I know
nothing of any of them. My comments come from being a Weston developer,
a Wayland compositor for both desktop'ish and embedded use cases that
uses DRM KMS.
Thanks,
pq
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: Future handling of complex RGB devices on Linux v2
From: Thomas Zimmermann @ 2024-02-23 9:21 UTC (permalink / raw)
To: Pavel Machek, Pekka Paalanen
Cc: Werner Sembach, Hans de Goede, Lee Jones, jikos, linux-kernel,
Jelle van der Waa, Miguel Ojeda, dri-devel@lists.freedesktop.org,
linux-input, ojeda, linux-leds
In-Reply-To: <ZdeGiMf2npmzJidU@duo.ucw.cz>
Hi
Am 22.02.24 um 18:38 schrieb Pavel Machek:
> Hi!
>
>>>> so after more feedback from the OpenRGB maintainers I came up with an even
>>>> more generic proposal:
>>>> https://gitlab.com/CalcProgrammer1/OpenRGB/-/issues/3916#note_1753072869
>>>>> evaluate-set-command ioctl taking:
>>>>> {
>>>>> enum command /* one of supported_commands */
>>>>> union data
>>>>> {
>>>>> char raw[3072],
>>>>> {
>>>>> <input struct for command 0>
>>>>> },
>>> Yeah, so ... this is not a interface. This is a backdoor to pass
>>> arbitrary data. That's not going to fly.
>>>
>>> For keyboards, we don't need complete new interface; we reasonable
>>> extensions over existing display APIs -- keyboards are clearly 2D.
>> I suppose they could be seen as *a* display, but if you are referring
>> to DRM KMS UAPI, then no, I don't see that fitting at all:
> So -- we already have very similar displays in
> drivers/auxdisplay. drivers/auxdisplay/cfag12864b.c is 128x64 display,
> 1-bit display for example.
Auxdisplay can be anything for everyone. It appears to be the rest that
didn't clearly fit elsewhere. The core interface seems to be a custom
char device. The fbdev code in cfag12864b is not representative.
>
>> - the "pixel grid" is not orthogonal, it's not a rectangle, and it
>> might not be a grid at all
> It is quite close to orthogonal. I'd suggest simply pretending it is
> orthogonal grid with some pixels missing :-). We already have
> cellphone displays with rounded corners and holes in them, so I
> suspect handling of missing pixels will be neccessary anyway.
>
>> - Timings and video modes? DRM KMS has always been somewhat awkward for
>> display devices that do not have an inherent scanout cycle and timings
>> totally depend on the amount of pixels updated at a time
>> (FB_DAMAGE_CLIPS), e.g. USB displays (not USB-C DP alt mode).
>> They do work, but they are very different from the usual hardware
>> involved with KMS, require special consideration in userspace, and
>> they still are actual displays while what we're talking about here
>> are not.
> As you say, there are other displays with similar problems.
>
>> - KMS has no concept of programmed autonomous animations, and likely
>> never will. They are not useful with actual displays.
> Yep. We need some kind of extension here, and this is likely be quite
> vendor-specific, as animations will differ between vendors. I guess
> "please play pattern xyzzy with parametrs 3 and 5" might be enough for this.
The litmus test for DRM and fbdev is something like "would the user run
the console, desktop, or any other meaningful output in this display".
That is also what userspace (e.g., X, Wayland, gfx terminals) expects: a
display to show the user's main output. Keyboard LEDs don't fit here.
Best regards
Thomas
>
>> - Userspace will try to light up KMS outputs automatically and extend
>> the traditional desktop there. This was already a problem for
>> head-mounted displays (HMD) where it made no sense. That was worked
>> around with an in-kernel list of HMDs and some KMS property
>> quirking.
> This will need fixing for cfag12864b.c, no? Perhaps userspace should
> simply ignore anything smaller than 240x160 or something...
>
>> Modern KMS UAPI very much aims to be a generic UAPI that abstracts
>> display devices. It already breaks down a little for things like USB
>> displays and virtual machines (e.g. qemu, vmware, especially with
>> remote viewers), which I find unfortunate. With HMDs the genericity
>> breaks down in other ways, but I'd claim HMDs are a better fit still
>> than full-featured VM virtual displays (cursor plane hijacking). With
>> non-displays like keyboards the genericity would be completely lost, as
>> they won't work at all the same way as displays. You cannot even show
>> proper images there, only coarse light patterns *IF* you actually know
>> the pixel layout. But the pixel layout is(?) hardware-specific which is
>> the opposite of generic.
>>
>> While you could dress keyboard lights etc. up with DRM KMS UAPI, the
>> userspace would have to be written from scratch for them, and you
>> somehow need to make existing KMS userspace to never touch those
>> devices. What's the point of using DRM KMS UAPI in the first place,
>> then?
> Well, at least we have good UAPI to work with. Other options were 100
> files in /sys/class/leds, pretending it is a linear array of leds,
> just passing raw data around, and pretending it is grid of RGB888
> data.
>
> Anyway, there are devices such as these: (8x8 LED display).
>
> https://www.laskakit.cz/8x8-led-matice-s-max7219-3mm-cervena/
>
> We should think about what interface we want for these, and then I
> believe we should make RGB keyboards use something similar.
>
> Best regards,
> Pavel
--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
^ permalink raw reply
* Re: [PATCH v2 01/10] backlight: Match backlight device against struct fb_info.bl_dev
From: Lee Jones @ 2024-02-23 10:53 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: daniel.thompson, jingoohan1, deller, andy, robin, javierm,
dri-devel, linux-fbdev, linux-input, linux-pwm
In-Reply-To: <20240221094324.27436-2-tzimmermann@suse.de>
On Wed, 21 Feb 2024, Thomas Zimmermann wrote:
> Framebuffer drivers for devices with dedicated backlight are supposed
> to set struct fb_info.bl_dev to the backlight's respective device. Use
> the value to match backlight and framebuffer in the backlight core code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
> drivers/video/backlight/backlight.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/backlight/backlight.c b/drivers/video/backlight/backlight.c
> index 86e1cdc8e3697..48844a4f28ad3 100644
> --- a/drivers/video/backlight/backlight.c
> +++ b/drivers/video/backlight/backlight.c
> @@ -98,7 +98,8 @@ static int fb_notifier_callback(struct notifier_block *self,
> {
> struct backlight_device *bd;
> struct fb_event *evdata = data;
> - int node = evdata->info->node;
> + struct fb_info *info = evdata->info;
> + int node = info->node;
> int fb_blank = 0;
>
> /* If we aren't interested in this event, skip it immediately ... */
> @@ -110,8 +111,12 @@ static int fb_notifier_callback(struct notifier_block *self,
>
> if (!bd->ops)
> goto out;
> - if (bd->ops->check_fb && !bd->ops->check_fb(bd, evdata->info))
> + else if (bd->ops->check_fb && !bd->ops->check_fb(bd, info))
> goto out;
> +#if IS_ENABLED(CONFIG_FB_BACKLIGHT)
I don't want #ifery in C files.
Please find another way.
> + else if (info->bl_dev && info->bl_dev != bd)
> + goto out;
> +#endif
>
> fb_blank = *(int *)evdata->data;
> if (fb_blank == FB_BLANK_UNBLANK && !bd->fb_bl_on[node]) {
> --
> 2.43.0
>
--
Lee Jones [李琼斯]
^ permalink raw reply
* Re: [PATCH v1 00/19] hid-ft260: Fixes for serial driver patch v4
From: Christina Quast @ 2024-02-23 13:07 UTC (permalink / raw)
To: Jiri Kosina, Michael Zaidman
Cc: daniel.beer, linux-kernel, linux-input, linux-serial,
ilpo.jarvinen, johan, gregkh, equinox
In-Reply-To: <nycvar.YFH.7.76.2402131120070.21798@cbobk.fhfr.pm>
Hi Jiri!
I send Patch v5, feel free to have a look!
Best regards,
Christina
On 2/13/24 11:20, Jiri Kosina wrote:
> On Sat, 10 Feb 2024, Michael Zaidman wrote:
>
>> Modifications on top of "[PATCH v4 RESEND] hid-ft260: Add serial driver"
>> https://lore.kernel.org/all/20231218093153.192268-1-contact@christina-quast.de/
>>
>> They are mostly the fixes to the original v4 patch and should be melded into
>> the v5 patch rather than upstreamed as separate patches.
> Agreed; I am not acting upon this series now then, and will wait for v5
> with these folded in.
>
> Thanks,
>
^ 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