* Re: [PATCH riscv64] kobject: fix WARNING in input_register_device
From: Greg KH @ 2024-02-13 7:20 UTC (permalink / raw)
To: Edward Adam Davis
Cc: linux-input, linux-kernel, linux-usb, rafael,
syzbot+8e41bb0c055b209ebbf4, syzkaller-bugs
In-Reply-To: <tencent_DDCFB377C3642974A3A3A44D176B776DA605@qq.com>
On Tue, Feb 13, 2024 at 08:43:26AM +0800, Edward Adam Davis wrote:
> On Thu, 8 Feb 2024 12:25:10 +0000, Greg KH wrote:
> > On Thu, Feb 08, 2024 at 07:37:56PM +0800, Edward Adam Davis wrote:
> > > On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> > > > > The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> > > > > of data to env, which will result in insufficient memory allocated to the buf
> > > > > members of env.
> > > >
> > > > What is "env"? And can you wrap your lines at 72 columns please?
> > > env is an instance of struct kobj_uevent_env.
> >
> > Ok, be specific please in your changelog text, otherwise we can't really
> > understand what is happening.
> >
> > > > > Reported-and-tested-by: syzbot+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > > ---
> > > > > include/linux/kobject.h | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> > > > > index c30affcc43b4..74b37b6459cd 100644
> > > > > --- a/include/linux/kobject.h
> > > > > +++ b/include/linux/kobject.h
> > > > > @@ -30,7 +30,7 @@
> > > > >
> > > > > #define UEVENT_HELPER_PATH_LEN 256
> > > > > #define UEVENT_NUM_ENVP 64 /* number of env pointers */
> > > > > -#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
> > > > > +#define UEVENT_BUFFER_SIZE 2560 /* buffer for the variables */
> > > >
> > > > That's an odd number, why that? Why not just a page? What happens if
> > > > some other path wants more?
> > > An increase of 512 bytes is sufficient for the current issue. Do not consider
> > > the problem of hypothetical existence.
> >
> > Why is this 512 bytes sufficient now? What changed to cause this?
> There is the following code in input_print_modalias():
>
> drivers/input/input.c
> 1 len += input_print_modalias_bits(buf + len, size - len,
> 1403 'k', id->keybit, KEY_MIN_INTERESTING, KEY_MAX);
> This code will add up to 2608 bytes of data to env at most.
> (KEY_MAX - KEY_MIN_INTERESTING) * 4 = (256 * 3 - 1 - 113 ) * 4 = (765 - 113) * 4 = 652 * 4 = 2608 bytes。
> Note: In the expression, 4 represents 3 bytes of hexadecimal data and 1 byte of comma.
So your change above is wrong and will not work for the max size?
Why not restrict the modalias here to fit instead of overflowing? Odds
are we should be checking this properly no matter what the value is
changed to, right?
> include/uapi/linux/input-event-codes.h
> 188 #define KEY_MUTE 113
> 807 #define KEY_MIN_INTERESTING KEY_MUTE
> 808 #define KEY_MAX 0x2ff
> During my actual testing process, I found that a total of 1684 bytes were
> contributed in input_print_modalias().
> >
> > And how can we detect this automatically in the future? Shouldn't we
> > just be truncating the buffer instead of having an overflow?
> >
> > > > And what's causing the input stack to have so many variables all of a
> > > > sudden, what changed to cause this? Is this a bugfix for a specific
> > > > commit that needs to be backported to older kernels? Why did this
> > > > buffer size all of a sudden be too small?
> > > The result of my analysis is that several members of struct input_dev are too
> > > large, such as its member keybit.
> >
> > And when did that change? What commit id? What prevents it from
> > growing again and us needing to change this again?
> The code that caused this issue has been introduced for a long time, and it is
> speculated that it was due to the fact that the warning in add_uevent_var() was
> returned directly to ENOMEM without being taken seriously.
>
> lib/kobject_uevent.c
> 2 if (len >= (sizeof(env->buf) - env->buflen)) {
> 1 WARN(1, KERN_ERR "add_uevent_var: buffer size too small\n");
> 672 return -ENOMEM;
Odd line numbers?
Anyway, we should get rid of the WARN() as that will cause crashes, and
just handle it properly there.
> 1 }
>
> I believe that this issue was introduced by:
> 7eff2e7a8b65 - Driver core: change add_ueventvar to use a struct.
In 2007? And never been actually hit since then? So is this a real
issue? :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH riscv64] kobject: fix WARNING in input_register_device
From: Greg KH @ 2024-02-13 7:21 UTC (permalink / raw)
To: Edward Adam Davis
Cc: linux-input, linux-kernel, linux-usb, rafael,
syzbot+8e41bb0c055b209ebbf4, syzkaller-bugs
In-Reply-To: <tencent_9D9878866FF42C756D2C94DCEA36EC26CE0A@qq.com>
On Tue, Feb 13, 2024 at 08:50:01AM +0800, Edward Adam Davis wrote:
> On Thu, 8 Feb 2024 12:25:35 +0000, Greg KH wrote:
> > On Thu, Feb 08, 2024 at 07:37:56PM +0800, Edward Adam Davis wrote:
> > > On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> > > > > The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> > > > > of data to env, which will result in insufficient memory allocated to the buf
> > > > > members of env.
> > > >
> > > > What is "env"? And can you wrap your lines at 72 columns please?
> > > env is an instance of struct kobj_uevent_env.
> >
> > Also, why is "risc64" in the subject line?
> Because when syzbot reported this issue, it wrote "userspace arch: riscv64".
> However, I actually tested it on the master branch of upstream.
Then of course, this was not correct in the subject line.
thanks,
greg k-h
^ permalink raw reply
* [PATCH 0/4] leds: trigger: Improve handling of led_trigger_event() and simplify mute audio trigger
From: Heiner Kallweit @ 2024-02-13 7:30 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer
Cc: linux-leds@vger.kernel.org, linux-sound, open list:HID CORE LAYER,
linux-mips
If a simple trigger is assigned to a LED, then the LED may be off until
the next led_trigger_event() call. This may be an issue for simple
triggers with rare led_trigger_event() calls, e.g. power supply
charging indicators (drivers/power/supply/power_supply_leds.c).
Therefore persist the brightness value of the last led_trigger_event()
call and use this value if the trigger is assigned to a LED.
This change allows to use simple triggers in more cases.
As a first use case simplify handling of the mute audio trigger.
This series touches few subsystems. I'd propose to handle it via
the LED subsystem.
Heiner Kallweit (4):
leds: trigger: Store brightness set by led_trigger_event()
ALSA: control-led: Integrate mute led trigger
Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER
leds: trigger: audio: Remove this trigger
arch/mips/configs/ci20_defconfig | 1 -
drivers/input/input-leds.c | 8 +---
drivers/leds/led-triggers.c | 6 ++-
drivers/leds/trigger/Kconfig | 7 ---
drivers/leds/trigger/Makefile | 1 -
drivers/leds/trigger/ledtrig-audio.c | 67 ----------------------------
include/linux/leds.h | 29 ++++++------
sound/core/Kconfig | 1 -
sound/core/control_led.c | 20 +++++++--
9 files changed, 37 insertions(+), 103 deletions(-)
delete mode 100644 drivers/leds/trigger/ledtrig-audio.c
--
2.43.1
^ permalink raw reply
* [PATCH 1/4] leds: trigger: Store brightness set by led_trigger_event()
From: Heiner Kallweit @ 2024-02-13 7:31 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer
Cc: linux-leds@vger.kernel.org, linux-sound, open list:HID CORE LAYER,
linux-mips
In-Reply-To: <30d49088-283c-40f3-b97b-fd5f5174a467@gmail.com>
If a simple trigger is assigned to a LED, then the LED may be off until
the next led_trigger_event() call. This may be an issue for simple
triggers with rare led_trigger_event() calls, e.g. power supply
charging indicators (drivers/power/supply/power_supply_leds.c).
Therefore persist the brightness value of the last led_trigger_event()
call and use this value if the trigger is assigned to a LED.
In addition add a getter for the trigger brightness value.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/leds/led-triggers.c | 6 ++++--
include/linux/leds.h | 15 +++++++++++++++
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/drivers/leds/led-triggers.c b/drivers/leds/led-triggers.c
index 0f5ac3005..b1b323b19 100644
--- a/drivers/leds/led-triggers.c
+++ b/drivers/leds/led-triggers.c
@@ -194,11 +194,11 @@ int led_trigger_set(struct led_classdev *led_cdev, struct led_trigger *trig)
spin_unlock(&trig->leddev_list_lock);
led_cdev->trigger = trig;
+ ret = 0;
if (trig->activate)
ret = trig->activate(led_cdev);
else
- ret = 0;
-
+ led_set_brightness(led_cdev, trig->brightness);
if (ret)
goto err_activate;
@@ -387,6 +387,8 @@ void led_trigger_event(struct led_trigger *trig,
if (!trig)
return;
+ trig->brightness = brightness;
+
rcu_read_lock();
list_for_each_entry_rcu(led_cdev, &trig->led_cdevs, trig_list)
led_set_brightness(led_cdev, brightness);
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 7598d4729..48fff5980 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -455,6 +455,9 @@ struct led_trigger {
int (*activate)(struct led_classdev *led_cdev);
void (*deactivate)(struct led_classdev *led_cdev);
+ /* Brightness set by led_trigger_event */
+ enum led_brightness brightness;
+
/* LED-private triggers have this set */
struct led_hw_trigger_type *trigger_type;
@@ -508,6 +511,12 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
return led_cdev->trigger_data;
}
+static inline enum led_brightness
+led_trigger_get_brightness(const struct led_trigger *trigger)
+{
+ return trigger ? trigger->brightness : LED_OFF;
+}
+
#define module_led_trigger(__led_trigger) \
module_driver(__led_trigger, led_trigger_register, \
led_trigger_unregister)
@@ -544,6 +553,12 @@ static inline void *led_get_trigger_data(struct led_classdev *led_cdev)
return NULL;
}
+static inline enum led_brightness
+led_trigger_get_brightness(const struct led_trigger *trigger)
+{
+ return LED_OFF;
+}
+
#endif /* CONFIG_LEDS_TRIGGERS */
/* Trigger specific enum */
--
2.43.1
^ permalink raw reply related
* [PATCH 2/4] ALSA: control-led: Integrate mute led trigger
From: Heiner Kallweit @ 2024-02-13 7:32 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer
Cc: linux-leds@vger.kernel.org, linux-sound, open list:HID CORE LAYER,
linux-mips
In-Reply-To: <30d49088-283c-40f3-b97b-fd5f5174a467@gmail.com>
This driver is the only one calling ledtrig_audio_set(), therefore
the LED audio trigger isn't usable standalone. So it makes sense
to fully integrate LED audio triger handling here.
The module aliases ensure that the driver is auto-loaded (if built
as module) if a LED device has one of the two audio triggers as
default trigger.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
sound/core/control_led.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/sound/core/control_led.c b/sound/core/control_led.c
index a78eb4892..b9d650380 100644
--- a/sound/core/control_led.c
+++ b/sound/core/control_led.c
@@ -53,6 +53,7 @@ struct snd_ctl_led_ctl {
static DEFINE_MUTEX(snd_ctl_led_mutex);
static bool snd_ctl_led_card_valid[SNDRV_CARDS];
+static struct led_trigger *snd_ctl_ledtrig_audio[NUM_AUDIO_LEDS];
static struct snd_ctl_led snd_ctl_leds[MAX_LED] = {
{
.name = "speaker",
@@ -176,8 +177,11 @@ static void snd_ctl_led_set_state(struct snd_card *card, unsigned int access,
case MODE_FOLLOW_ROUTE: if (route >= 0) route ^= 1; break;
case MODE_FOLLOW_MUTE: /* noop */ break;
}
- if (route >= 0)
- ledtrig_audio_set(led->trigger_type, route ? LED_OFF : LED_ON);
+ if (route >= 0) {
+ struct led_trigger *trig = snd_ctl_ledtrig_audio[led->trigger_type];
+
+ led_trigger_event(trig, route ? LED_OFF : LED_ON);
+ }
}
static struct snd_ctl_led_ctl *snd_ctl_led_find(struct snd_kcontrol *kctl, unsigned int ioff)
@@ -442,8 +446,9 @@ static ssize_t brightness_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
struct snd_ctl_led *led = container_of(dev, struct snd_ctl_led, dev);
+ struct led_trigger *trig = snd_ctl_ledtrig_audio[led->trigger_type];
- return sysfs_emit(buf, "%u\n", ledtrig_audio_get(led->trigger_type));
+ return sysfs_emit(buf, "%u\n", led_trigger_get_brightness(trig));
}
static DEVICE_ATTR_RW(mode);
@@ -736,6 +741,9 @@ static int __init snd_ctl_led_init(void)
struct snd_ctl_led *led;
unsigned int group;
+ led_trigger_register_simple("audio-mute", &snd_ctl_ledtrig_audio[LED_AUDIO_MUTE]);
+ led_trigger_register_simple("audio-micmute", &snd_ctl_ledtrig_audio[LED_AUDIO_MICMUTE]);
+
device_initialize(&snd_ctl_led_dev);
snd_ctl_led_dev.class = &sound_class;
snd_ctl_led_dev.release = snd_ctl_led_dev_release;
@@ -788,7 +796,13 @@ static void __exit snd_ctl_led_exit(void)
}
device_unregister(&snd_ctl_led_dev);
snd_ctl_led_clean(NULL);
+
+ led_trigger_unregister_simple(snd_ctl_ledtrig_audio[LED_AUDIO_MUTE]);
+ led_trigger_unregister_simple(snd_ctl_ledtrig_audio[LED_AUDIO_MICMUTE]);
}
module_init(snd_ctl_led_init)
module_exit(snd_ctl_led_exit)
+
+MODULE_ALIAS("ledtrig:audio-mute");
+MODULE_ALIAS("ledtrig:audio-micmute");
--
2.43.1
^ permalink raw reply related
* [PATCH 3/4] Input: leds: Prepare for removal of config option LEDS_AUDIO_TRIGGER
From: Heiner Kallweit @ 2024-02-13 7:33 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer
Cc: linux-leds@vger.kernel.org, linux-sound, open list:HID CORE LAYER,
linux-mips
In-Reply-To: <30d49088-283c-40f3-b97b-fd5f5174a467@gmail.com>
In a follow-up patch handling of the LED audio trigger will be changed,
including removal of config symbol LEDS_AUDIO_TRIGGER. Therefore set
the default trigger unconditionally to "audio-mute". It does no harm
if a default trigger doesn't exist.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/input/input-leds.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/drivers/input/input-leds.c b/drivers/input/input-leds.c
index b16fc8194..176f1da7f 100644
--- a/drivers/input/input-leds.c
+++ b/drivers/input/input-leds.c
@@ -18,12 +18,6 @@
#define VT_TRIGGER(_name) .trigger = NULL
#endif
-#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
-#define AUDIO_TRIGGER(_name) .trigger = _name
-#else
-#define AUDIO_TRIGGER(_name) .trigger = NULL
-#endif
-
static const struct {
const char *name;
const char *trigger;
@@ -35,7 +29,7 @@ static const struct {
[LED_KANA] = { "kana", VT_TRIGGER("kbd-kanalock") },
[LED_SLEEP] = { "sleep" } ,
[LED_SUSPEND] = { "suspend" },
- [LED_MUTE] = { "mute", AUDIO_TRIGGER("audio-mute") },
+ [LED_MUTE] = { "mute", "audio-mute" },
[LED_MISC] = { "misc" },
[LED_MAIL] = { "mail" },
[LED_CHARGING] = { "charging" },
--
2.43.1
^ permalink raw reply related
* [PATCH 4/4] leds: trigger: audio: Remove this trigger
From: Heiner Kallweit @ 2024-02-13 7:34 UTC (permalink / raw)
To: Pavel Machek, Lee Jones, Jaroslav Kysela, Takashi Iwai,
Dmitry Torokhov, Thomas Bogendoerfer
Cc: linux-leds@vger.kernel.org, linux-sound, open list:HID CORE LAYER,
linux-mips
In-Reply-To: <30d49088-283c-40f3-b97b-fd5f5174a467@gmail.com>
Now that the audio trigger is fully integrated in
sound/core/control_led.c, we can remove it here.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
arch/mips/configs/ci20_defconfig | 1 -
drivers/leds/trigger/Kconfig | 7 ---
drivers/leds/trigger/Makefile | 1 -
drivers/leds/trigger/ledtrig-audio.c | 67 ----------------------------
include/linux/leds.h | 14 ------
sound/core/Kconfig | 1 -
6 files changed, 91 deletions(-)
delete mode 100644 drivers/leds/trigger/ledtrig-audio.c
diff --git a/arch/mips/configs/ci20_defconfig b/arch/mips/configs/ci20_defconfig
index cdf2a782d..7827b2b39 100644
--- a/arch/mips/configs/ci20_defconfig
+++ b/arch/mips/configs/ci20_defconfig
@@ -152,7 +152,6 @@ CONFIG_LEDS_TRIGGER_CAMERA=m
CONFIG_LEDS_TRIGGER_PANIC=y
CONFIG_LEDS_TRIGGER_NETDEV=y
CONFIG_LEDS_TRIGGER_PATTERN=y
-CONFIG_LEDS_TRIGGER_AUDIO=y
CONFIG_RTC_CLASS=y
CONFIG_RTC_DRV_JZ4740=y
CONFIG_DMADEVICES=y
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index d11d80176..31576952e 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -136,13 +136,6 @@ config LEDS_TRIGGER_PATTERN
which is a series of tuples, of brightness and duration (ms).
If unsure, say N
-config LEDS_TRIGGER_AUDIO
- tristate "Audio Mute LED Trigger"
- help
- This allows LEDs to be controlled by audio drivers for following
- the audio mute and mic-mute changes.
- If unsure, say N
-
config LEDS_TRIGGER_TTY
tristate "LED Trigger for TTY devices"
depends on TTY
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 25c4db97c..242f6c4e3 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -14,5 +14,4 @@ obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o
obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
obj-$(CONFIG_LEDS_TRIGGER_NETDEV) += ledtrig-netdev.o
obj-$(CONFIG_LEDS_TRIGGER_PATTERN) += ledtrig-pattern.o
-obj-$(CONFIG_LEDS_TRIGGER_AUDIO) += ledtrig-audio.o
obj-$(CONFIG_LEDS_TRIGGER_TTY) += ledtrig-tty.o
diff --git a/drivers/leds/trigger/ledtrig-audio.c b/drivers/leds/trigger/ledtrig-audio.c
deleted file mode 100644
index 2ecd4b760..000000000
--- a/drivers/leds/trigger/ledtrig-audio.c
+++ /dev/null
@@ -1,67 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-//
-// Audio Mute LED trigger
-//
-
-#include <linux/kernel.h>
-#include <linux/leds.h>
-#include <linux/module.h>
-#include "../leds.h"
-
-static enum led_brightness audio_state[NUM_AUDIO_LEDS];
-
-static int ledtrig_audio_mute_activate(struct led_classdev *led_cdev)
-{
- led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MUTE]);
- return 0;
-}
-
-static int ledtrig_audio_micmute_activate(struct led_classdev *led_cdev)
-{
- led_set_brightness_nosleep(led_cdev, audio_state[LED_AUDIO_MICMUTE]);
- return 0;
-}
-
-static struct led_trigger ledtrig_audio[NUM_AUDIO_LEDS] = {
- [LED_AUDIO_MUTE] = {
- .name = "audio-mute",
- .activate = ledtrig_audio_mute_activate,
- },
- [LED_AUDIO_MICMUTE] = {
- .name = "audio-micmute",
- .activate = ledtrig_audio_micmute_activate,
- },
-};
-
-enum led_brightness ledtrig_audio_get(enum led_audio type)
-{
- return audio_state[type];
-}
-EXPORT_SYMBOL_GPL(ledtrig_audio_get);
-
-void ledtrig_audio_set(enum led_audio type, enum led_brightness state)
-{
- audio_state[type] = state;
- led_trigger_event(&ledtrig_audio[type], state);
-}
-EXPORT_SYMBOL_GPL(ledtrig_audio_set);
-
-static int __init ledtrig_audio_init(void)
-{
- led_trigger_register(&ledtrig_audio[LED_AUDIO_MUTE]);
- led_trigger_register(&ledtrig_audio[LED_AUDIO_MICMUTE]);
- return 0;
-}
-module_init(ledtrig_audio_init);
-
-static void __exit ledtrig_audio_exit(void)
-{
- led_trigger_unregister(&ledtrig_audio[LED_AUDIO_MUTE]);
- led_trigger_unregister(&ledtrig_audio[LED_AUDIO_MICMUTE]);
-}
-module_exit(ledtrig_audio_exit);
-
-MODULE_DESCRIPTION("LED trigger for audio mute control");
-MODULE_LICENSE("GPL v2");
-MODULE_ALIAS("ledtrig:audio-mute");
-MODULE_ALIAS("ledtrig:audio-micmute");
diff --git a/include/linux/leds.h b/include/linux/leds.h
index 48fff5980..d2668b427 100644
--- a/include/linux/leds.h
+++ b/include/linux/leds.h
@@ -705,18 +705,4 @@ enum led_audio {
NUM_AUDIO_LEDS
};
-#if IS_ENABLED(CONFIG_LEDS_TRIGGER_AUDIO)
-enum led_brightness ledtrig_audio_get(enum led_audio type);
-void ledtrig_audio_set(enum led_audio type, enum led_brightness state);
-#else
-static inline enum led_brightness ledtrig_audio_get(enum led_audio type)
-{
- return LED_OFF;
-}
-static inline void ledtrig_audio_set(enum led_audio type,
- enum led_brightness state)
-{
-}
-#endif
-
#endif /* __LINUX_LEDS_H_INCLUDED */
diff --git a/sound/core/Kconfig b/sound/core/Kconfig
index 8077f481d..b970a1734 100644
--- a/sound/core/Kconfig
+++ b/sound/core/Kconfig
@@ -262,6 +262,5 @@ config SND_CTL_LED
tristate
select NEW_LEDS if SND_CTL_LED
select LEDS_TRIGGERS if SND_CTL_LED
- select LEDS_TRIGGER_AUDIO if SND_CTL_LED
source "sound/core/seq/Kconfig"
--
2.43.1
^ permalink raw reply related
* Re: [PATCH v7 00/39] ep93xx device tree conversion
From: Nikita Shubin @ 2024-02-13 9:48 UTC (permalink / raw)
To: andy.shevchenko, Vinod Koul, Stephen Boyd
Cc: Hartley Sweeten, Alexander Sverdlin, Russell King,
Lukasz Majewski, Linus Walleij, Bartosz Golaszewski,
Andy Shevchenko, Michael Turquette, Stephen Boyd,
Sebastian Reichel, Rob Herring, Krzysztof Kozlowski, Conor Dooley,
Vinod Koul, Wim Van Sebroeck, Guenter Roeck, Thierry Reding,
Uwe Kleine-König, Mark Brown, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Miquel Raynal, Richard Weinberger,
Vignesh Raghavendra, Damien Le Moal, Sergey Shtylyov,
Dmitry Torokhov, Liam Girdwood, Jaroslav Kysela, Takashi Iwai,
Ralf Baechle, Wu, Aaron, Lee Jones, Olof Johansson, Niklas Cassel,
linux-arm-kernel, linux-kernel, linux-gpio, linux-clk, linux-pm,
devicetree, dmaengine, linux-watchdog, linux-pwm, linux-spi,
netdev, linux-mtd, linux-ide, linux-input, linux-sound,
Arnd Bergmann, Andy Shevchenko, Bartosz Golaszewski,
Krzysztof Kozlowski, Andy Shevchenko, Andrew Lunn
In-Reply-To: <Zb_DfISgoNyTKWMp@surfacebook.localdomain>
On Sun, 2024-02-04 at 19:03 +0200, andy.shevchenko@gmail.com wrote:
> Thu, Jan 18, 2024 at 11:20:43AM +0300, Nikita Shubin kirjoitti:
> > The goal is to recieve ACKs for all patches in series to merge it
> > via Arnd branch.
> >
> > No major changes since last version (v6) all changes are cometic.
> >
> > Following patches require attention from Stephen Boyd, as they were
> > converted to aux_dev as suggested:
> >
> > - ARM: ep93xx: add regmap aux_dev
> > - clk: ep93xx: add DT support for Cirrus EP93xx
> >
> > Following patches require attention from Vinod Koul:
> >
> > - dma: cirrus: Convert to DT for Cirrus EP93xx
> > - dma: cirrus: remove platform code
> >
> > Following patches are dropped:
> > - dt-bindings: wdt: Add ts72xx (pulled requested by Wim Van
> > Sebroeck)
> >
> > Big Thanks to Andy Shevchenko once again.
>
> You're welcome!
>
Thank you Andy!
> I have a few minor comments, I believe if you send a new version it
> will be
> final (at least from my p.o.v.).
>
Still waiting for some comment from Stephen Boyd and Vinod Koul on:
- ARM: ep93xx: add regmap aux_dev
- clk: ep93xx: add DT support for Cirrus EP93xx
- dma: cirrus: Convert to DT for Cirrus EP93xx
- dma: cirrus: remove platform code
^ permalink raw reply
* Re: [PATCH v1 00/19] hid-ft260: Fixes for serial driver patch v4
From: Jiri Kosina @ 2024-02-13 10:20 UTC (permalink / raw)
To: Michael Zaidman
Cc: chrysh, daniel.beer, linux-kernel, linux-input, linux-serial,
ilpo.jarvinen, johan, gregkh, equinox
In-Reply-To: <20240210215147.77629-1-michael.zaidman@gmail.com>
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,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: multitouch: Add required quirk for Synaptics 0xcddc device
From: Jiri Kosina @ 2024-02-13 10:28 UTC (permalink / raw)
To: Manuel Fombuena; +Cc: Benjamin Tissoires, linux-input, linux-kernel
In-Reply-To: <CWLP123MB547355BA62AD835719F60865C5492@CWLP123MB5473.GBRP123.PROD.OUTLOOK.COM>
On Sun, 11 Feb 2024, Manuel Fombuena wrote:
> Add support for the pointing stick (Accupoint) and 2 mouse buttons.
>
> Present on some Toshiba/dynabook Portege X30 and X40 laptops.
>
> It should close https://bugzilla.kernel.org/show_bug.cgi?id=205817
>
> Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
> ---
> drivers/hid/hid-multitouch.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index fd5b0637dad6..3e91e4d6ba6f 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -2151,6 +2151,10 @@ static const struct hid_device_id mt_devices[] = {
> HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> USB_VENDOR_ID_SYNAPTICS, 0xcd7e) },
>
> + { .driver_data = MT_CLS_WIN_8_FORCE_MULTI_INPUT,
> + HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> + USB_VENDOR_ID_SYNAPTICS, 0xcddc) },
> +
> { .driver_data = MT_CLS_WIN_8_FORCE_MULTI_INPUT,
> HID_DEVICE(BUS_I2C, HID_GROUP_MULTITOUCH_WIN_8,
> USB_VENDOR_ID_SYNAPTICS, 0xce08) },
Applied, thanks.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: Intel-ish-hid: Ishtp: Fix sensor reads after ACPI S3 suspend
From: Jiri Kosina @ 2024-02-13 10:31 UTC (permalink / raw)
To: srinivas pandruvada
Cc: Even Xu, benjamin.tissoires, linux-input,
linux-kernel@vger.kernel.org
In-Reply-To: <64ee580b9969335d60966e23e9bd859e8f075953.camel@linux.intel.com>
On Fri, 9 Feb 2024, srinivas pandruvada wrote:
> > Fixes: f645a90e8ff7 ("HID: intel-ish-hid: ishtp-hid-client: use
> > helper functions for connection")
> > Closes: https://bugzilla.kernel.org/show_bug.cgi?id=218442
> > Signed-off-by: Even Xu <even.xu@intel.com>
> Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
>
> Hi Jiri,
>
> This regression is introduced with 6.8-rc1, so need a pull request for
> this rc cycle.
Right; now queued in hid.git#for-6.8/upstream-fixes. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH] Input: synaptics - enable InterTouch for TOS0213
From: Manuel Fombuena @ 2024-02-13 10:27 UTC (permalink / raw)
To: Dmitry Torokhov, Raul Rangel, linux-input, linux-kernel; +Cc: Manuel Fombuena
[ 5.989588] psmouse serio1: synaptics: Your touchpad (PNP: TOS0213 PNP0f03) says it can support a different bus. If i2c-hid and hid-rmi are not used, you might want to try setting psmouse.synaptics_intertouch to 1 and report this to linux-input@vger.kernel.org.
[ 6.039923] psmouse serio1: synaptics: Touchpad model: 1, fw: 9.32, id: 0x1e2a1, caps: 0xf00223/0x840300/0x12e800/0x52d884, board id: 3322, fw id: 2658004
The board is labelled TM3322.
Present on the Toshiba / Dynabook Portege X30-D and possibly others.
Confirmed working well with psmouse.synaptics_intertouch=1 and local build.
Signed-off-by: Manuel Fombuena <fombuena@outlook.com>
---
drivers/input/mouse/synaptics.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/input/mouse/synaptics.c b/drivers/input/mouse/synaptics.c
index ada299ec5bba..a6601e007082 100644
--- a/drivers/input/mouse/synaptics.c
+++ b/drivers/input/mouse/synaptics.c
@@ -192,6 +192,7 @@ static const char * const smbus_pnp_ids[] = {
"SYN3221", /* HP 15-ay000 */
"SYN323d", /* HP Spectre X360 13-w013dx */
"SYN3257", /* HP Envy 13-ad105ng */
+ "TOS0213", /* Dynabook Portege X30-D */
NULL
};
--
2.43.0
^ permalink raw reply related
* Re: [PATCH 1/2] HID: wacom: generic: Avoid reporting a serial of '0' to userspace
From: Jiri Kosina @ 2024-02-13 10:40 UTC (permalink / raw)
To: Tobita, Tatsunosuke
Cc: linux-input, Benjamin Tissoires, Ping Cheng, Jason Gerecke,
Aaron Armstrong Skomra, Joshua Dickens, Tatsunosuke Tobita,
Jason Gerecke, stable
In-Reply-To: <20240201044055.23367-1-tatsunosuke.wacom@gmail.com>
On Thu, 1 Feb 2024, Tobita, Tatsunosuke wrote:
> From: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>
>
> The xf86-input-wacom driver does not treat '0' as a valid serial
> number and will drop any input report which contains an
> MSC_SERIAL = 0 event. The kernel driver already takes care to
> avoid sending any MSC_SERIAL event if the value of serial[0] == 0
> (which is the case for devices that don't actually report a
> serial number), but this is not quite sufficient.
> Only the lower 32 bits of the serial get reported to userspace,
> so if this portion of the serial is zero then there can still
> be problems.
>
> This commit allows the driver to report either the lower 32 bits
> if they are non-zero or the upper 32 bits otherwise.
>
> Signed-off-by: Jason Gerecke <jason.gerecke@wacom.com>
> Signed-off-by: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>
> Fixes: f85c9dc678a5 ("HID: wacom: generic: Support tool ID and additional tool types")
> CC: stable@vger.kernel.org # v4.10
Applied to hid.git#for-6.8/upstream-fixes.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 2/2] HID: wacom: Clean up use of struct->wacom_wac
From: Jiri Kosina @ 2024-02-13 10:42 UTC (permalink / raw)
To: Tobita, Tatsunosuke
Cc: linux-input, Benjamin Tissoires, Ping Cheng, Jason Gerecke,
Aaron Armstrong Skomra, Joshua Dickens, Tatsunosuke Tobita
In-Reply-To: <20240201044346.23476-1-tatsunosuke.wacom@gmail.com>
On Thu, 1 Feb 2024, Tobita, Tatsunosuke wrote:
> From: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>
>
> Replace the indirect accesses to struct->wacom_wac from struct->wacom
> to the direct access in order for better code reading.
>
> Signed-off-by: Tatsunosuke Tobita <tatsunosuke.tobita@wacom.com>
Applied to hid.git#for-6.9/wacom.
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 0/7] HID: unused struct members cleanup
From: Jiri Kosina @ 2024-02-13 10:44 UTC (permalink / raw)
To: Jiri Slaby (SUSE); +Cc: linux-input, linux-kernel
In-Reply-To: <20240201115320.684-1-jirislaby@kernel.org>
On Thu, 1 Feb 2024, Jiri Slaby (SUSE) wrote:
> Hi,
>
> the patches deal with unused members of structures as found by
> clang-struct.
>
> Sometimes even whole structures are removed.
Good stuff, thanks. Applied to hid.git#for-6.9/unused-struct-removal.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH riscv64] kobject: fix WARNING in input_register_device
From: Edward Adam Davis @ 2024-02-13 13:10 UTC (permalink / raw)
To: gregkh
Cc: eadavis, linux-input, linux-kernel, linux-usb, rafael,
syzbot+8e41bb0c055b209ebbf4, syzkaller-bugs
In-Reply-To: <2024021316-nuclei-botany-5446@gregkh>
On Tue, 13 Feb 2024 08:20:52 +0100, Greg KH wrote:
> On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> On Tue, Feb 13, 2024 at 08:43:26AM +0800, Edward Adam Davis wrote:
> > On Thu, 8 Feb 2024 12:25:10 +0000, Greg KH wrote:
> > > On Thu, Feb 08, 2024 at 07:37:56PM +0800, Edward Adam Davis wrote:
> > > > On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> > > > > > The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> > > > > > of data to env, which will result in insufficient memory allocated to the buf
> > > > > > members of env.
> > > > >
> > > > > What is "env"? And can you wrap your lines at 72 columns please?
> > > > env is an instance of struct kobj_uevent_env.
> > >
> > > Ok, be specific please in your changelog text, otherwise we can't really
> > > understand what is happening.
> > >
> > > > > > Reported-and-tested-by: syzbot+8e41bb0c055b209ebbf4@syzkaller.appspotmail.com
> > > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > > > ---
> > > > > > include/linux/kobject.h | 2 +-
> > > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/linux/kobject.h b/include/linux/kobject.h
> > > > > > index c30affcc43b4..74b37b6459cd 100644
> > > > > > --- a/include/linux/kobject.h
> > > > > > +++ b/include/linux/kobject.h
> > > > > > @@ -30,7 +30,7 @@
> > > > > >
> > > > > > #define UEVENT_HELPER_PATH_LEN 256
> > > > > > #define UEVENT_NUM_ENVP 64 /* number of env pointers */
> > > > > > -#define UEVENT_BUFFER_SIZE 2048 /* buffer for the variables */
> > > > > > +#define UEVENT_BUFFER_SIZE 2560 /* buffer for the variables */
> > > > >
> > > > > That's an odd number, why that? Why not just a page? What happens if
> > > > > some other path wants more?
> > > > An increase of 512 bytes is sufficient for the current issue. Do not consider
> > > > the problem of hypothetical existence.
> > >
> > > Why is this 512 bytes sufficient now? What changed to cause this?
> > There is the following code in input_print_modalias():
> >
> > drivers/input/input.c
> > 1 len += input_print_modalias_bits(buf + len, size - len,
> > 1403 'k', id->keybit, KEY_MIN_INTERESTING, KEY_MAX);
> > This code will add up to 2608 bytes of data to env at most.
> > (KEY_MAX - KEY_MIN_INTERESTING) * 4 = (256 * 3 - 1 - 113 ) * 4 = (765 - 113) * 4 = 652 * 4 = 2608 bytes。
> > Note: In the expression, 4 represents 3 bytes of hexadecimal data and 1 byte of comma.
>
> So your change above is wrong and will not work for the max size?
Yes.
>
> Why not restrict the modalias here to fit instead of overflowing? Odds
> are we should be checking this properly no matter what the value is
> changed to, right?
Right.
It may be necessary to deepen our understanding of this piece of code before
fixing this issue internally.
>
> > include/uapi/linux/input-event-codes.h
> > 188 #define KEY_MUTE 113
> > 807 #define KEY_MIN_INTERESTING KEY_MUTE
> > 808 #define KEY_MAX 0x2ff
> > During my actual testing process, I found that a total of 1684 bytes were
> > contributed in input_print_modalias().
> > >
> > > And how can we detect this automatically in the future? Shouldn't we
> > > just be truncating the buffer instead of having an overflow?
> > >
> > > > > And what's causing the input stack to have so many variables all of a
> > > > > sudden, what changed to cause this? Is this a bugfix for a specific
> > > > > commit that needs to be backported to older kernels? Why did this
> > > > > buffer size all of a sudden be too small?
> > > > The result of my analysis is that several members of struct input_dev are too
> > > > large, such as its member keybit.
> > >
> > > And when did that change? What commit id? What prevents it from
> > > growing again and us needing to change this again?
> > The code that caused this issue has been introduced for a long time, and it is
> > speculated that it was due to the fact that the warning in add_uevent_var() was
> > returned directly to ENOMEM without being taken seriously.
> >
> > lib/kobject_uevent.c
> > 2 if (len >= (sizeof(env->buf) - env->buflen)) {
> > 1 WARN(1, KERN_ERR "add_uevent_var: buffer size too small\n");
> > 672 return -ENOMEM;
>
> Odd line numbers?
>
> Anyway, we should get rid of the WARN() as that will cause crashes, and
> just handle it properly there.
>
>
> > 1 }
> >
> > I believe that this issue was introduced by:
> > 7eff2e7a8b65 - Driver core: change add_ueventvar to use a struct.
>
> In 2007? And never been actually hit since then? So is this a real
> issue? :)
Yes. But as I mentioned earlier, in add_uevent_var(), it will exit directly after
a warning, so this issue has not been given enough attention, perhaps it has
happened many times.
thanks,
edward.
^ permalink raw reply
* Re: [PATCH 02/10] auxdisplay/ht16k33: Remove struct backlight_ops.check_fb
From: Robin van der Gracht @ 2024-02-13 13:17 UTC (permalink / raw)
To: Thomas Zimmermann
Cc: lee, daniel.thompson, jingoohan1, deller, javierm, dri-devel,
linux-fbdev, linux-input, linux-pwm
In-Reply-To: <20240212162645.5661-3-tzimmermann@suse.de>
On Mon, 12 Feb 2024 17:16:35 +0100
Thomas Zimmermann <tzimmermann@suse.de> wrote:
> The driver sets struct fb_info.bl_dev to the correct backlight
> device. Thus rely on the backlight core code to match backlight
> and framebuffer devices, and remove the extra check_fb function
> from struct backlight_ops.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: Robin van der Gracht <robin@protonic.nl>
> ---
> drivers/auxdisplay/ht16k33.c | 8 --------
> 1 file changed, 8 deletions(-)
>
> diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
> index a90430b7d07ba..0a858db32486b 100644
> --- a/drivers/auxdisplay/ht16k33.c
> +++ b/drivers/auxdisplay/ht16k33.c
> @@ -325,16 +325,8 @@ static int ht16k33_bl_update_status(struct backlight_device *bl)
> return ht16k33_brightness_set(priv, brightness);
> }
>
> -static int ht16k33_bl_check_fb(struct backlight_device *bl, struct fb_info *fi)
> -{
> - struct ht16k33_priv *priv = bl_get_data(bl);
> -
> - return (fi == NULL) || (fi->par == priv);
> -}
> -
> static const struct backlight_ops ht16k33_bl_ops = {
> .update_status = ht16k33_bl_update_status,
> - .check_fb = ht16k33_bl_check_fb,
> };
When combined with the previous patch:
[01/10] backlight: Match backlight device against struct fb_info.bl_dev
(I wasn't in the CC)
Acked-By: Robin van der Gracht <robin@protonic.nl>
^ permalink raw reply
* Re: [PATCH riscv64] kobject: fix WARNING in input_register_device
From: Edward Adam Davis @ 2024-02-13 13:13 UTC (permalink / raw)
To: gregkh
Cc: eadavis, linux-input, linux-kernel, linux-usb, rafael,
syzbot+8e41bb0c055b209ebbf4, syzkaller-bugs
In-Reply-To: <2024021356-unveiling-falcon-54db@gregkh>
On Tue, 13 Feb 2024 08:21:18 +0100, Greg KH wrote:
> On Tue, Feb 13, 2024 at 08:50:01AM +0800, Edward Adam Davis wrote:
> > On Thu, 8 Feb 2024 12:25:35 +0000, Greg KH wrote:
> > > On Thu, Feb 08, 2024 at 07:37:56PM +0800, Edward Adam Davis wrote:
> > > > On Thu, 8 Feb 2024 10:56:00, Greg KH wrote:
> > > > > > The input_add_uevent_modalias_var()->input_print_modalias() will add 1684 bytes
> > > > > > of data to env, which will result in insufficient memory allocated to the buf
> > > > > > members of env.
> > > > >
> > > > > What is "env"? And can you wrap your lines at 72 columns please?
> > > > env is an instance of struct kobj_uevent_env.
> > >
> > > Also, why is "risc64" in the subject line?
> > Because when syzbot reported this issue, it wrote "userspace arch: riscv64".
> > However, I actually tested it on the master branch of upstream.
>
> Then of course, this was not correct in the subject line.
Got it.
thanks,
edward.
^ permalink raw reply
* Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
From: kernel test robot @ 2024-02-13 14:53 UTC (permalink / raw)
To: Thomas Zimmermann, lee, daniel.thompson, jingoohan1, deller,
javierm
Cc: llvm, oe-kbuild-all, dri-devel, linux-fbdev, linux-input,
linux-pwm, Thomas Zimmermann, Bruno Prémont
In-Reply-To: <20240212162645.5661-5-tzimmermann@suse.de>
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on lee-backlight/for-backlight-next]
[also build test ERROR on lee-backlight/for-backlight-fixes hid/for-next lee-leds/for-leds-next linus/master v6.8-rc4 next-20240213]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/backlight-Match-backlight-device-against-struct-fb_info-bl_dev/20240213-002853
base: https://git.kernel.org/pub/scm/linux/kernel/git/lee/backlight.git for-backlight-next
patch link: https://lore.kernel.org/r/20240212162645.5661-5-tzimmermann%40suse.de
patch subject: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240213/202402132248.A5ky78Hx-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240213/202402132248.A5ky78Hx-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202402132248.A5ky78Hx-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/hid/hid-picolcd_fb.c:497:8: error: no member named 'bl_dev' in 'struct fb_info'
497 | info->bl_dev = data->backlight;
| ~~~~ ^
1 error generated.
vim +497 drivers/hid/hid-picolcd_fb.c
459
460 static DEVICE_ATTR(fb_update_rate, 0664, picolcd_fb_update_rate_show,
461 picolcd_fb_update_rate_store);
462
463 /* initialize Framebuffer device */
464 int picolcd_init_framebuffer(struct picolcd_data *data)
465 {
466 struct device *dev = &data->hdev->dev;
467 struct fb_info *info = NULL;
468 struct picolcd_fb_data *fbdata = NULL;
469 int i, error = -ENOMEM;
470 u32 *palette;
471
472 /* The extra memory is:
473 * - 256*u32 for pseudo_palette
474 * - struct fb_deferred_io
475 */
476 info = framebuffer_alloc(256 * sizeof(u32) +
477 sizeof(struct fb_deferred_io) +
478 sizeof(struct picolcd_fb_data) +
479 PICOLCDFB_SIZE, dev);
480 if (!info)
481 goto err_nomem;
482
483 info->fbdefio = info->par;
484 *info->fbdefio = picolcd_fb_defio;
485 info->par += sizeof(struct fb_deferred_io);
486 palette = info->par;
487 info->par += 256 * sizeof(u32);
488 for (i = 0; i < 256; i++)
489 palette[i] = i > 0 && i < 16 ? 0xff : 0;
490 info->pseudo_palette = palette;
491 info->fbops = &picolcdfb_ops;
492 info->var = picolcdfb_var;
493 info->fix = picolcdfb_fix;
494 info->fix.smem_len = PICOLCDFB_SIZE*8;
495
496 #ifdef CONFIG_HID_PICOLCD_BACKLIGHT
> 497 info->bl_dev = data->backlight;
498 #endif
499
500 fbdata = info->par;
501 spin_lock_init(&fbdata->lock);
502 fbdata->picolcd = data;
503 fbdata->update_rate = PICOLCDFB_UPDATE_RATE_DEFAULT;
504 fbdata->bpp = picolcdfb_var.bits_per_pixel;
505 fbdata->force = 1;
506 fbdata->vbitmap = info->par + sizeof(struct picolcd_fb_data);
507 fbdata->bitmap = vmalloc(PICOLCDFB_SIZE*8);
508 if (fbdata->bitmap == NULL) {
509 dev_err(dev, "can't get a free page for framebuffer\n");
510 goto err_nomem;
511 }
512 info->flags |= FBINFO_VIRTFB;
513 info->screen_buffer = fbdata->bitmap;
514 info->fix.smem_start = (unsigned long)fbdata->bitmap;
515 memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
516 data->fb_info = info;
517
518 error = picolcd_fb_reset(data, 1);
519 if (error) {
520 dev_err(dev, "failed to configure display\n");
521 goto err_cleanup;
522 }
523
524 error = device_create_file(dev, &dev_attr_fb_update_rate);
525 if (error) {
526 dev_err(dev, "failed to create sysfs attributes\n");
527 goto err_cleanup;
528 }
529
530 fb_deferred_io_init(info);
531 error = register_framebuffer(info);
532 if (error) {
533 dev_err(dev, "failed to register framebuffer\n");
534 goto err_sysfs;
535 }
536 return 0;
537
538 err_sysfs:
539 device_remove_file(dev, &dev_attr_fb_update_rate);
540 fb_deferred_io_cleanup(info);
541 err_cleanup:
542 data->fb_info = NULL;
543
544 err_nomem:
545 if (fbdata)
546 vfree(fbdata->bitmap);
547 framebuffer_release(info);
548 return error;
549 }
550
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply
* Re: [PATCH] HID: nintendo: Remove some unused functions
From: Jiri Kosina @ 2024-02-13 15:25 UTC (permalink / raw)
To: Jiapeng Chong
Cc: djogorchock, benjamin.tissoires, linux-input, linux-kernel,
Abaci Robot, Ryan McClelland
In-Reply-To: <20240126075445.15495-1-jiapeng.chong@linux.alibaba.com>
On Fri, 26 Jan 2024, Jiapeng Chong wrote:
> These functions are defined in the hid-nintendo.c file, but not called
> elsewhere, so delete these unused functions.
>
> drivers/hid/hid-nintendo.c:757:20: warning: unused function 'joycon_type_has_left_controls'.
> drivers/hid/hid-nintendo.c:763:20: warning: unused function 'joycon_type_has_right_controls'.
>
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Closes: https://bugzilla.openanolis.cn/show_bug.cgi?id=8060
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
No idea why those were added, CCing Ryan, who added those in 94f18bb1994.
I've added a Fixes: tag and applied, thanks.
> ---
> drivers/hid/hid-nintendo.c | 12 ------------
> 1 file changed, 12 deletions(-)
>
> diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
> index 7ce6be0a8dee..ab5953fc2436 100644
> --- a/drivers/hid/hid-nintendo.c
> +++ b/drivers/hid/hid-nintendo.c
> @@ -754,18 +754,6 @@ static inline bool joycon_type_is_right_nescon(struct joycon_ctlr *ctlr)
> return ctlr->ctlr_type == JOYCON_CTLR_TYPE_NESR;
> }
>
> -static inline bool joycon_type_has_left_controls(struct joycon_ctlr *ctlr)
> -{
> - return joycon_type_is_left_joycon(ctlr) ||
> - joycon_type_is_procon(ctlr);
> -}
> -
> -static inline bool joycon_type_has_right_controls(struct joycon_ctlr *ctlr)
> -{
> - return joycon_type_is_right_joycon(ctlr) ||
> - joycon_type_is_procon(ctlr);
> -}
> -
> static inline bool joycon_type_is_any_joycon(struct joycon_ctlr *ctlr)
> {
> return joycon_type_is_left_joycon(ctlr) ||
> --
> 2.20.1.7.g153144c
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v10 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus Bridge
From: Rob Herring @ 2024-02-13 15:28 UTC (permalink / raw)
To: Danny Kaehn
Cc: krzysztof.kozlowski+dt, andriy.shevchenko, bentiss, jikos,
bartosz.golaszewski, niyas.sait, dmitry.torokhov, devicetree,
linux-input, ethan.twardy
In-Reply-To: <20240205170920.93499-2-danny.kaehn@plexus.com>
On Mon, Feb 05, 2024 at 11:09:20AM -0600, Danny Kaehn wrote:
> This is a USB HID device which includes an I2C controller and 8 GPIO pins.
>
> The binding allows describing the chip's gpio and i2c controller in DT
> using the subnodes named "gpio" and "i2c", respectively. This is
> intended to be used in configurations where the CP2112 is permanently
> connected in hardware.
>
> Signed-off-by: Danny Kaehn <danny.kaehn@plexus.com>
> ---
>
> Note -- Reviewed-By tags have been removed as suggested by Benjamin, since
> 1. It has been 6+ months since this binding was reviewed, and a lot can
> change upstream in that time
> 2. There has been some contention between using named child nodes to
> identify i2c and gpio nodes, and also making the driver implementing this
> binding compatible with ACPI, since names aren't significant for ACPI
> nodes, and ACPI names are always automatically uppercased. It has been
> suggested that perhaps the DT binding should use child nodes with
> addressable `reg` properties to identify the child nodes, instead of by
> name [1].
'reg' only makes sense if there are values which relate to the h/w. If
your addresses are indices, that will be suspect.
There's documented nodenames for specific device classes in DT. You have
to use those whether there's 'reg' and a unit-address or not. I'm not
really clear what the problem is.
>
> Of course, I acknowledge that other firmware languages and kernel details
> shouldn't impact DT bindings, but it also seems that there should
> be some consistent way to specify sub-functions like this accross DT
> and ACPI. Some additional commentary / requests for comment about the
> seemingly missing glue here can be found in [2].
I have little interest in worrying about ACPI as I have limited
knowledge in ACPI requirements, what I do know is the model for bindings
are fundamentally differ, and no one has stepped up to maintain bindings
from an ACPI perspective.
> Any comments from Rob/Krzysztof/other DT folks would be greatly appreciated
>
> [1] https://lore.kernel.org/all/ZBhoHzTr5l38u%2FkX@smile.fi.intel.com/
> [2] https://lore.kernel.org/all/CAP+ZCCd0cD+q7=ngyEzScAte2VT9R00mqCQxB3K2TMbeg8UAfA@mail.gmail.com/
>
> .../bindings/i2c/silabs,cp2112.yaml | 113 ++++++++++++++++++
> 1 file changed, 113 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
>
> diff --git a/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
> new file mode 100644
> index 000000000000..a27509627804
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/silabs,cp2112.yaml
> @@ -0,0 +1,113 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/i2c/silabs,cp2112.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: CP2112 HID USB to SMBus/I2C Bridge
> +
> +maintainers:
> + - Danny Kaehn <kaehndan@gmail.com>
> +
> +description:
> + The CP2112 is a USB HID device which includes an integrated I2C controller
> + and 8 GPIO pins. Its GPIO pins can each be configured as inputs, open-drain
> + outputs, or push-pull outputs.
> +
> +properties:
> + compatible:
> + const: usb10c4,ea90
> +
> + reg:
> + maxItems: 1
> + description: The USB port number on the host controller
> +
> + i2c:
> + description: The SMBus/I2C controller node for the CP2112
> + $ref: /schemas/i2c/i2c-controller.yaml#
> + unevaluatedProperties: false
> +
> + properties:
> + sda-gpios:
> + maxItems: 1
> +
> + scl-gpios:
> + maxItems: 1
Why do you have GPIOs if this is a proper controller?
> +
> + clock-frequency:
> + minimum: 10000
> + default: 100000
> + maximum: 400000
> +
> + gpio:
> + description: The GPIO controller node for the CP2112
There's no need for a child node here. All these properties can be part
of the parent.
> + type: object
> + unevaluatedProperties: false
> +
> + properties:
> + interrupt-controller: true
> + "#interrupt-cells":
> + const: 2
> +
> + gpio-controller: true
> + "#gpio-cells":
> + const: 2
> +
> + gpio-line-names:
> + minItems: 1
> + maxItems: 8
> +
> + patternProperties:
> + "-hog(-[0-9]+)?$":
> + type: object
> +
> + required:
> + - gpio-hog
> +
> +required:
> + - compatible
> + - reg
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + #include <dt-bindings/interrupt-controller/irq.h>
> + #include <dt-bindings/gpio/gpio.h>
> +
> + usb {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + device@1 {
> + compatible = "usb10c4,ea90";
> + reg = <1>;
> +
> + i2c {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + sda-gpios = <&cp2112_gpio 0 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> + scl-gpios = <&cp2112_gpio 1 (GPIO_ACTIVE_HIGH | GPIO_OPEN_DRAIN)>;
> +
> + temp@48 {
> + compatible = "national,lm75";
> + reg = <0x48>;
> + };
> + };
> +
> + cp2112_gpio: gpio {
> + gpio-controller;
> + interrupt-controller;
> + #gpio-cells = <2>;
> + gpio-line-names = "CP2112_SDA", "CP2112_SCL", "TEST2",
> + "TEST3","TEST4", "TEST5", "TEST6";
> +
> + fan-rst-hog {
> + gpio-hog;
> + gpios = <7 GPIO_ACTIVE_HIGH>;
> + output-high;
> + line-name = "FAN_RST";
> + };
> + };
> + };
> + };
> --
> 2.25.1
>
^ permalink raw reply
* Different uhid_event size on different archs.
From: Ajrat Makhmutov @ 2024-02-13 17:38 UTC (permalink / raw)
To: linux-input
https://github.com/torvalds/linux/blob/1f719a2f3fa67665578c759ac34fd3d3690c1a20/include/uapi/linux/uhid.h#L180
Union in struct uhid_event has no __attribute__((__packed__)), so
alignment from 4372 to 4376 bytes is performed on a 64-bit system. On a
32-bit system, the size remains 4372. Is this a bug?_
_
^ permalink raw reply
* Re: [PATCH RFC bpf-next 0/9] allow HID-BPF to do device IOs
From: Benjamin Tissoires @ 2024-02-13 17:46 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Benjamin Tissoires, Toke Høiland-Jørgensen,
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, Jonathan Corbet, Shuah Khan, bpf, LKML,
open list:HID CORE LAYER, open list:DOCUMENTATION,
open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <CAADnVQKt7zu2OY0xHCkTb=KSXO33Xj8H4vVYMqP51ZJ_Kj1sZA@mail.gmail.com>
On Feb 12 2024, Alexei Starovoitov wrote:
> On Mon, Feb 12, 2024 at 10:21 AM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > On Mon, Feb 12, 2024 at 6:46 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > >
> > > Benjamin Tissoires <benjamin.tissoires@redhat.com> writes:
> > >
[...]
> I agree that workqueue delegation fits into the bpf_timer concept and
> a lot of code can and should be shared.
Thanks Alexei for the detailed answer. I've given it an attempt but still can not
figure it out entirely.
> All the lessons(bugs) learned with bpf_timer don't need to be re-discovered :)
> Too bad, bpf_timer_set_callback() doesn't have a flag argument,
> so we need a new kfunc to set a sleepable callback.
> Maybe
> bpf_timer_set_sleepable_cb() ?
OK. So I guess I should drop Toke's suggestion with the bpf_timer_ini() flag?
> The verifier will set is_async_cb = true for it (like it does for regular cb-s).
> And since prog->aux->sleepable is kinda "global" we need another
> per subprog flag:
> bool is_sleepable: 1;
done (in push_callback_call())
>
> We can factor out a check "if (prog->aux->sleepable)" into a helper
> that will check that "global" flag and another env->cur_state->in_sleepable
> flag that will work similar to active_rcu_lock.
done (I think), cf patch 2 below
> Once the verifier starts processing subprog->is_sleepable
> it will set cur_state->in_sleepable = true;
> to make all subprogs called from that cb to be recognized as sleepable too.
That's the point I don't know where to put the new code.
It seems the best place would be in do_check(), but I am under the impression
that the code of the callback is added at the end of the instruction list, meaning
that I do not know where it starts, and which subprog index it corresponds to.
>
> A bit of a challenge is what to do with global subprogs,
> since they're verified lazily. They can be called from
> sleepable and non-sleepable contex. Should be solvable.
I must confess this is way over me (and given that I didn't even managed to make
the "easy" case working, that might explain things a little :-P )
>
> Overall I think this feature is needed urgently,
> so if you don't have cycles to work on this soon,
> I can prioritize it right after bpf_arena work.
I can try to spare a few cycles on it. Even if your instructions were on
spot, I still can't make the subprogs recognized as sleepable.
For reference, this is where I am (probably bogus, but seems to be
working when timer_set_sleepable_cb() is called from a sleepable context
as mentioned by Toke):
---
From d4aa3d969fa9a89c6447d843dad338fde2ac0155 Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <bentiss@kernel.org>
Date: Tue, 13 Feb 2024 18:40:01 +0100
Subject: [PATCH RFC bpf-next v2 01/11] Sleepable timers
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Message-Id: <20240213-hid-bpf-sleepable-v2-1-6c2d6b49865c@kernel.org>
---
include/linux/bpf_verifier.h | 2 +
include/uapi/linux/bpf.h | 13 ++++++
kernel/bpf/helpers.c | 91 +++++++++++++++++++++++++++++++++++++++---
kernel/bpf/verifier.c | 20 ++++++++--
tools/include/uapi/linux/bpf.h | 13 ++++++
5 files changed, 130 insertions(+), 9 deletions(-)
diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h
index 84365e6dd85d..789ef5fec547 100644
--- a/include/linux/bpf_verifier.h
+++ b/include/linux/bpf_verifier.h
@@ -426,6 +426,7 @@ struct bpf_verifier_state {
* while they are still in use.
*/
bool used_as_loop_entry;
+ bool in_sleepable;
/* first and last insn idx of this verifier state */
u32 first_insn_idx;
@@ -626,6 +627,7 @@ struct bpf_subprog_info {
bool is_async_cb: 1;
bool is_exception_cb: 1;
bool args_cached: 1;
+ bool is_sleepable: 1;
u8 arg_cnt;
struct bpf_subprog_arg_info args[MAX_BPF_FUNC_REG_ARGS];
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index d96708380e52..ef1f2be4cfef 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -5742,6 +5742,18 @@ union bpf_attr {
* 0 on success.
*
* **-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * long bpf_timer_set_sleepable_cb(struct bpf_timer *timer, void *callback_fn)
+ * Description
+ * Configure the timer to call *callback_fn* static function in a
+ * sleepable context.
+ * Return
+ * 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.
*/
#define ___BPF_FUNC_MAPPER(FN, ctx...) \
FN(unspec, 0, ##ctx) \
@@ -5956,6 +5968,7 @@ union bpf_attr {
FN(user_ringbuf_drain, 209, ##ctx) \
FN(cgrp_storage_get, 210, ##ctx) \
FN(cgrp_storage_delete, 211, ##ctx) \
+ FN(timer_set_sleepable_cb, 212, ##ctx) \
/* */
/* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 4db1c658254c..e3b83d27b1b6 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1097,9 +1097,11 @@ const struct bpf_func_proto bpf_snprintf_proto = {
*/
struct bpf_hrtimer {
struct hrtimer timer;
+ struct work_struct work;
struct bpf_map *map;
struct bpf_prog *prog;
void __rcu *callback_fn;
+ void __rcu *sleepable_cb_fn;
void *value;
};
@@ -1113,18 +1115,64 @@ struct bpf_timer_kern {
struct bpf_spin_lock lock;
} __attribute__((aligned(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;
+ void *key;
+ u32 idx;
+
+ BTF_TYPE_EMIT(struct bpf_timer);
+
+ rcu_read_lock();
+ callback_fn = rcu_dereference(t->sleepable_cb_fn);
+ rcu_read_unlock();
+ if (!callback_fn)
+ return;
+
+ // /* bpf_timer_work_cb() runs in hrtimer_run_softirq. It doesn't migrate and
+ // * cannot be preempted by another bpf_timer_cb() on the same cpu.
+ // * Remember the timer this callback is servicing to prevent
+ // * deadlock if callback_fn() calls bpf_timer_cancel() or
+ // * 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);
+ }
+
+ callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0);
+ /* The verifier checked that return value is zero. */
+
+ // this_cpu_write(hrtimer_running, NULL);
+}
+
static DEFINE_PER_CPU(struct bpf_hrtimer *, hrtimer_running);
static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
{
struct bpf_hrtimer *t = container_of(hrtimer, struct bpf_hrtimer, timer);
+ bpf_callback_t callback_fn, sleepable_cb_fn;
struct bpf_map *map = t->map;
void *value = t->value;
- bpf_callback_t callback_fn;
void *key;
u32 idx;
BTF_TYPE_EMIT(struct bpf_timer);
+ sleepable_cb_fn = rcu_dereference_check(t->sleepable_cb_fn, rcu_read_lock_bh_held());
+ if (sleepable_cb_fn) {
+ schedule_work(&t->work);
+ goto out;
+ }
+
callback_fn = rcu_dereference_check(t->callback_fn, rcu_read_lock_bh_held());
if (!callback_fn)
goto out;
@@ -1190,7 +1238,9 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
t->map = map;
t->prog = NULL;
rcu_assign_pointer(t->callback_fn, NULL);
+ rcu_assign_pointer(t->sleepable_cb_fn, NULL);
hrtimer_init(&t->timer, clockid, HRTIMER_MODE_REL_SOFT);
+ INIT_WORK(&t->work, bpf_timer_work_cb);
t->timer.function = bpf_timer_cb;
WRITE_ONCE(timer->timer, t);
/* Guarantee the order between timer->timer and map->usercnt. So
@@ -1221,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;
@@ -1260,12 +1310,24 @@ BPF_CALL_3(bpf_timer_set_callback, struct bpf_timer_kern *, timer, void *, callb
bpf_prog_put(prev);
t->prog = prog;
}
- rcu_assign_pointer(t->callback_fn, callback_fn);
+ if (is_sleepable) {
+ rcu_assign_pointer(t->sleepable_cb_fn, callback_fn);
+ rcu_assign_pointer(t->callback_fn, NULL);
+ } else {
+ rcu_assign_pointer(t->callback_fn, callback_fn);
+ rcu_assign_pointer(t->sleepable_cb_fn, NULL);
+ }
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,
@@ -1274,6 +1336,20 @@ static const struct bpf_func_proto bpf_timer_set_callback_proto = {
.arg2_type = ARG_PTR_TO_FUNC,
};
+BPF_CALL_3(bpf_timer_set_sleepable_cb, struct bpf_timer_kern *, timer, void *, callback_fn,
+ struct bpf_prog_aux *, aux)
+{
+ return __bpf_timer_set_callback(timer, callback_fn, aux, true);
+}
+
+static const struct bpf_func_proto bpf_timer_set_sleepable_cb_proto = {
+ .func = bpf_timer_set_sleepable_cb,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_TIMER,
+ .arg2_type = ARG_PTR_TO_FUNC,
+};
+
BPF_CALL_3(bpf_timer_start, struct bpf_timer_kern *, timer, u64, nsecs, u64, flags)
{
struct bpf_hrtimer *t;
@@ -1353,6 +1429,7 @@ BPF_CALL_1(bpf_timer_cancel, struct bpf_timer_kern *, timer)
* if it was running.
*/
ret = ret ?: hrtimer_cancel(&t->timer);
+ ret = ret ?: cancel_work_sync(&t->work);
return ret;
}
@@ -1405,8 +1482,10 @@ void bpf_timer_cancel_and_free(void *val)
* effectively cancelled because bpf_timer_cb() will return
* HRTIMER_NORESTART.
*/
- if (this_cpu_read(hrtimer_running) != t)
+ if (this_cpu_read(hrtimer_running) != t) {
hrtimer_cancel(&t->timer);
+ }
+ cancel_work_sync(&t->work);
kfree(t);
}
@@ -1749,6 +1828,8 @@ bpf_base_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_timer_init_proto;
case BPF_FUNC_timer_set_callback:
return &bpf_timer_set_callback_proto;
+ case BPF_FUNC_timer_set_sleepable_cb:
+ return &bpf_timer_set_sleepable_cb_proto;
case BPF_FUNC_timer_start:
return &bpf_timer_start_proto;
case BPF_FUNC_timer_cancel:
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 64fa188d00ad..400c625efe22 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -513,7 +513,8 @@ static bool is_sync_callback_calling_function(enum bpf_func_id func_id)
static bool is_async_callback_calling_function(enum bpf_func_id func_id)
{
- return func_id == BPF_FUNC_timer_set_callback;
+ return func_id == BPF_FUNC_timer_set_callback ||
+ func_id == BPF_FUNC_timer_set_sleepable_cb;
}
static bool is_callback_calling_function(enum bpf_func_id func_id)
@@ -1414,6 +1415,7 @@ static int copy_verifier_state(struct bpf_verifier_state *dst_state,
}
dst_state->speculative = src->speculative;
dst_state->active_rcu_lock = src->active_rcu_lock;
+ dst_state->in_sleepable = src->in_sleepable;
dst_state->curframe = src->curframe;
dst_state->active_lock.ptr = src->active_lock.ptr;
dst_state->active_lock.id = src->active_lock.id;
@@ -9434,11 +9436,13 @@ static int push_callback_call(struct bpf_verifier_env *env, struct bpf_insn *ins
if (insn->code == (BPF_JMP | BPF_CALL) &&
insn->src_reg == 0 &&
- insn->imm == BPF_FUNC_timer_set_callback) {
+ (insn->imm == BPF_FUNC_timer_set_callback ||
+ insn->imm == BPF_FUNC_timer_set_sleepable_cb)) {
struct bpf_verifier_state *async_cb;
/* there is no real recursion here. timer callbacks are async */
env->subprog_info[subprog].is_async_cb = true;
+ env->subprog_info[subprog].is_sleepable = insn->imm == BPF_FUNC_timer_set_sleepable_cb;
async_cb = push_async_cb(env, env->subprog_info[subprog].start,
insn_idx, subprog);
if (!async_cb)
@@ -10280,6 +10284,8 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
set_map_elem_callback_state);
break;
case BPF_FUNC_timer_set_callback:
+ fallthrough;
+ case BPF_FUNC_timer_set_sleepable_cb:
err = push_callback_call(env, insn, insn_idx, meta.subprogno,
set_timer_callback_state);
break;
@@ -15586,7 +15592,9 @@ static int visit_insn(int t, struct bpf_verifier_env *env)
return DONE_EXPLORING;
case BPF_CALL:
- if (insn->src_reg == 0 && insn->imm == BPF_FUNC_timer_set_callback)
+ if (insn->src_reg == 0 &&
+ (insn->imm == BPF_FUNC_timer_set_callback ||
+ insn->imm == BPF_FUNC_timer_set_sleepable_cb))
/* Mark this call insn as a prune point to trigger
* is_state_visited() check before call itself is
* processed by __check_func_call(). Otherwise new
@@ -16762,6 +16770,9 @@ static bool states_equal(struct bpf_verifier_env *env,
if (old->active_rcu_lock != cur->active_rcu_lock)
return false;
+ if (old->in_sleepable != cur->in_sleepable)
+ return false;
+
/* for states to be equal callsites have to be the same
* and all frame states need to be equivalent
*/
@@ -19639,7 +19650,8 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
continue;
}
- if (insn->imm == BPF_FUNC_timer_set_callback) {
+ if (insn->imm == BPF_FUNC_timer_set_callback ||
+ insn->imm == BPF_FUNC_timer_set_sleepable_cb) {
/* The verifier will process callback_fn as many times as necessary
* with different maps and the register states prepared by
* set_timer_callback_state will be accurate.
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index d96708380e52..ef1f2be4cfef 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -5742,6 +5742,18 @@ union bpf_attr {
* 0 on success.
*
* **-ENOENT** if the bpf_local_storage cannot be found.
+ *
+ * long bpf_timer_set_sleepable_cb(struct bpf_timer *timer, void *callback_fn)
+ * Description
+ * Configure the timer to call *callback_fn* static function in a
+ * sleepable context.
+ * Return
+ * 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.
*/
#define ___BPF_FUNC_MAPPER(FN, ctx...) \
FN(unspec, 0, ##ctx) \
@@ -5956,6 +5968,7 @@ union bpf_attr {
FN(user_ringbuf_drain, 209, ##ctx) \
FN(cgrp_storage_get, 210, ##ctx) \
FN(cgrp_storage_delete, 211, ##ctx) \
+ FN(timer_set_sleepable_cb, 212, ##ctx) \
/* */
/* backwards-compatibility macros for users of __BPF_FUNC_MAPPER that don't
--
2.43.0
---
From 6c654010a4660fd26ffce44406dba308ded3b465 Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <bentiss@kernel.org>
Date: Tue, 13 Feb 2024 18:40:02 +0100
Subject: [PATCH RFC bpf-next v2 02/11] bpf/verifier: introduce in_sleepable() helper
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Message-Id: <20240213-hid-bpf-sleepable-v2-2-6c2d6b49865c@kernel.org>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
kernel/bpf/verifier.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 400c625efe22..8c3707d27c02 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5257,6 +5257,12 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
return -EINVAL;
}
+static bool in_sleepable(struct bpf_verifier_env *env)
+{
+ return env->prog->aux->sleepable ||
+ (env->cur_state && env->cur_state->in_sleepable);
+}
+
/* The non-sleepable programs and sleepable programs with explicit bpf_rcu_read_lock()
* can dereference RCU protected pointers and result is PTR_TRUSTED.
*/
@@ -5264,7 +5270,7 @@ static bool in_rcu_cs(struct bpf_verifier_env *env)
{
return env->cur_state->active_rcu_lock ||
env->cur_state->active_lock.ptr ||
- !env->prog->aux->sleepable;
+ !in_sleepable(env);
}
/* Once GCC supports btf_type_tag the following mechanism will be replaced with tag check */
@@ -10153,7 +10159,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
return -EINVAL;
}
- if (!env->prog->aux->sleepable && fn->might_sleep) {
+ if (!in_sleepable(env) && fn->might_sleep) {
verbose(env, "helper call might sleep in a non-sleepable prog\n");
return -EINVAL;
}
@@ -10183,7 +10189,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
return -EINVAL;
}
- if (env->prog->aux->sleepable && is_storage_get_function(func_id))
+ if (in_sleepable(env) && is_storage_get_function(func_id))
env->insn_aux_data[insn_idx].storage_get_func_atomic = true;
}
@@ -11544,7 +11550,7 @@ static bool check_css_task_iter_allowlist(struct bpf_verifier_env *env)
return true;
fallthrough;
default:
- return env->prog->aux->sleepable;
+ return in_sleepable(env);
}
}
@@ -12065,7 +12071,7 @@ static int check_kfunc_call(struct bpf_verifier_env *env, struct bpf_insn *insn,
}
sleepable = is_kfunc_sleepable(&meta);
- if (sleepable && !env->prog->aux->sleepable) {
+ if (sleepable && !in_sleepable(env)) {
verbose(env, "program must be sleepable to call sleepable kfunc %s\n", func_name);
return -EACCES;
}
@@ -18208,7 +18214,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);
/* hold the map. If the program is rejected by verifier,
* the map will be released by release_maps() or it
@@ -19685,7 +19691,7 @@ static int do_misc_fixups(struct bpf_verifier_env *env)
}
if (is_storage_get_function(insn->imm)) {
- if (!env->prog->aux->sleepable ||
+ if (!in_sleepable(env) ||
env->insn_aux_data[i + delta].storage_get_func_atomic)
insn_buf[0] = BPF_MOV64_IMM(BPF_REG_5, (__force __s32)GFP_ATOMIC);
else
--
2.43.0
---
Cheers,
Benjamin
^ permalink raw reply related
* Re: [PATCH RFC bpf-next 0/9] allow HID-BPF to do device IOs
From: Kumar Kartikeya Dwivedi @ 2024-02-13 19:23 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Benjamin Tissoires,
Toke Høiland-Jørgensen, 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,
Jonathan Corbet, Shuah Khan, bpf, LKML, open list:HID CORE LAYER,
open list:DOCUMENTATION, open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <zybv26nmqtmyghakbebwxanzgzsfm6brvi7qw3ljoh4dijbjki@ub7atnumzuhy>
On Tue, 13 Feb 2024 at 18:46, Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> On Feb 12 2024, Alexei Starovoitov wrote:
> > On Mon, Feb 12, 2024 at 10:21 AM Benjamin Tissoires
> > <benjamin.tissoires@redhat.com> wrote:
> > >
> > > On Mon, Feb 12, 2024 at 6:46 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> > > >
> > > > Benjamin Tissoires <benjamin.tissoires@redhat.com> writes:
> > > >
> [...]
> > I agree that workqueue delegation fits into the bpf_timer concept and
> > a lot of code can and should be shared.
>
> Thanks Alexei for the detailed answer. I've given it an attempt but still can not
> figure it out entirely.
>
> > All the lessons(bugs) learned with bpf_timer don't need to be re-discovered :)
> > Too bad, bpf_timer_set_callback() doesn't have a flag argument,
> > so we need a new kfunc to set a sleepable callback.
> > Maybe
> > bpf_timer_set_sleepable_cb() ?
>
> OK. So I guess I should drop Toke's suggestion with the bpf_timer_ini() flag?
>
> > The verifier will set is_async_cb = true for it (like it does for regular cb-s).
> > And since prog->aux->sleepable is kinda "global" we need another
> > per subprog flag:
> > bool is_sleepable: 1;
>
> done (in push_callback_call())
>
> >
> > We can factor out a check "if (prog->aux->sleepable)" into a helper
> > that will check that "global" flag and another env->cur_state->in_sleepable
> > flag that will work similar to active_rcu_lock.
>
> done (I think), cf patch 2 below
>
> > Once the verifier starts processing subprog->is_sleepable
> > it will set cur_state->in_sleepable = true;
> > to make all subprogs called from that cb to be recognized as sleepable too.
>
> That's the point I don't know where to put the new code.
>
I think that would go in the already existing special case for
push_async_cb where you get the verifier state of the async callback.
You can make setting the boolean in that verifier state conditional on
whether it's your kfunc/helper you're processing taking a sleepable
callback.
> It seems the best place would be in do_check(), but I am under the impression
> that the code of the callback is added at the end of the instruction list, meaning
> that I do not know where it starts, and which subprog index it corresponds to.
>
> >
> > A bit of a challenge is what to do with global subprogs,
> > since they're verified lazily. They can be called from
> > sleepable and non-sleepable contex. Should be solvable.
>
> I must confess this is way over me (and given that I didn't even managed to make
> the "easy" case working, that might explain things a little :-P )
>
I think it will be solvable but made somewhat difficult by the fact
that even if we mark subprog_info of some global_func A as
in_sleepable, so that we explore it as sleepable during its
verification, we might encounter later another global_func that calls
a global func, already explored as non-sleepable, in sleepable
context. In this case I think we need to redo the verification of that
global func as sleepable once again. It could be that it is called
from both non-sleepable and sleepable contexts, so both paths
(in_sleepable = true, and in_sleepable = false) need to be explored,
or we could reject such cases, but it might be a little restrictive.
Some common helper global func unrelated to caller context doing some
auxiliary work, called from sleepable timer callback and normal main
subprog might be an example where rejection will be prohibitive.
An approach might be to explore main and global subprogs once as we do
now, and then keep a list of global subprogs that need to be revisited
as in_sleepable (due to being called from a sleepable context) and
trigger do_check_common for them again, this might have to be repeated
as the list grows on each iteration, but eventually we will have
explored all of them as in_sleepable if need be, and the loop will
end. Surely, this trades off logical simplicity of verifier code with
redoing verification of global subprogs again.
To add items to such a list, for each global subprog we encounter that
needs to be analyzed as in_sleepable, we will also collect all its
callee global subprogs by walking its instructions (a bit like
check_max_stack_depth does).
> >
> > Overall I think this feature is needed urgently,
> > so if you don't have cycles to work on this soon,
> > I can prioritize it right after bpf_arena work.
>
> I can try to spare a few cycles on it. Even if your instructions were on
> spot, I still can't make the subprogs recognized as sleepable.
>
> For reference, this is where I am (probably bogus, but seems to be
> working when timer_set_sleepable_cb() is called from a sleepable context
> as mentioned by Toke):
>
I just skimmed the patch but I think it's already 90% there. The only
other change I would suggest is switching from helper to kfunc, as
originally proposed by Alexei.
> [...]
^ permalink raw reply
* Re: [PATCH RFC bpf-next 0/9] allow HID-BPF to do device IOs
From: Toke Høiland-Jørgensen @ 2024-02-13 19:51 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi, Benjamin Tissoires
Cc: Alexei Starovoitov, 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,
Jonathan Corbet, Shuah Khan, bpf, LKML, open list:HID CORE LAYER,
open list:DOCUMENTATION, open list:KERNEL SELFTEST FRAMEWORK
In-Reply-To: <CAP01T75Giw_5j0RXaaxX0rDzCcXXZgmHrw7QZ_Ayib8rHgunBQ@mail.gmail.com>
Kumar Kartikeya Dwivedi <memxor@gmail.com> writes:
> On Tue, 13 Feb 2024 at 18:46, Benjamin Tissoires <bentiss@kernel.org> wrote:
>>
>> On Feb 12 2024, Alexei Starovoitov wrote:
>> > On Mon, Feb 12, 2024 at 10:21 AM Benjamin Tissoires
>> > <benjamin.tissoires@redhat.com> wrote:
>> > >
>> > > On Mon, Feb 12, 2024 at 6:46 PM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
>> > > >
>> > > > Benjamin Tissoires <benjamin.tissoires@redhat.com> writes:
>> > > >
>> [...]
>> > I agree that workqueue delegation fits into the bpf_timer concept and
>> > a lot of code can and should be shared.
>>
>> Thanks Alexei for the detailed answer. I've given it an attempt but still can not
>> figure it out entirely.
>>
>> > All the lessons(bugs) learned with bpf_timer don't need to be re-discovered :)
>> > Too bad, bpf_timer_set_callback() doesn't have a flag argument,
>> > so we need a new kfunc to set a sleepable callback.
>> > Maybe
>> > bpf_timer_set_sleepable_cb() ?
>>
>> OK. So I guess I should drop Toke's suggestion with the bpf_timer_ini() flag?
>>
>> > The verifier will set is_async_cb = true for it (like it does for regular cb-s).
>> > And since prog->aux->sleepable is kinda "global" we need another
>> > per subprog flag:
>> > bool is_sleepable: 1;
>>
>> done (in push_callback_call())
>>
>> >
>> > We can factor out a check "if (prog->aux->sleepable)" into a helper
>> > that will check that "global" flag and another env->cur_state->in_sleepable
>> > flag that will work similar to active_rcu_lock.
>>
>> done (I think), cf patch 2 below
>>
>> > Once the verifier starts processing subprog->is_sleepable
>> > it will set cur_state->in_sleepable = true;
>> > to make all subprogs called from that cb to be recognized as sleepable too.
>>
>> That's the point I don't know where to put the new code.
>>
>
> I think that would go in the already existing special case for
> push_async_cb where you get the verifier state of the async callback.
> You can make setting the boolean in that verifier state conditional on
> whether it's your kfunc/helper you're processing taking a sleepable
> callback.
>
>> It seems the best place would be in do_check(), but I am under the impression
>> that the code of the callback is added at the end of the instruction list, meaning
>> that I do not know where it starts, and which subprog index it corresponds to.
>>
>> >
>> > A bit of a challenge is what to do with global subprogs,
>> > since they're verified lazily. They can be called from
>> > sleepable and non-sleepable contex. Should be solvable.
>>
>> I must confess this is way over me (and given that I didn't even managed to make
>> the "easy" case working, that might explain things a little :-P )
>>
>
> I think it will be solvable but made somewhat difficult by the fact
> that even if we mark subprog_info of some global_func A as
> in_sleepable, so that we explore it as sleepable during its
> verification, we might encounter later another global_func that calls
> a global func, already explored as non-sleepable, in sleepable
> context. In this case I think we need to redo the verification of that
> global func as sleepable once again. It could be that it is called
> from both non-sleepable and sleepable contexts, so both paths
> (in_sleepable = true, and in_sleepable = false) need to be explored,
> or we could reject such cases, but it might be a little restrictive.
>
> Some common helper global func unrelated to caller context doing some
> auxiliary work, called from sleepable timer callback and normal main
> subprog might be an example where rejection will be prohibitive.
>
> An approach might be to explore main and global subprogs once as we do
> now, and then keep a list of global subprogs that need to be revisited
> as in_sleepable (due to being called from a sleepable context) and
> trigger do_check_common for them again, this might have to be repeated
> as the list grows on each iteration, but eventually we will have
> explored all of them as in_sleepable if need be, and the loop will
> end. Surely, this trades off logical simplicity of verifier code with
> redoing verification of global subprogs again.
>
> To add items to such a list, for each global subprog we encounter that
> needs to be analyzed as in_sleepable, we will also collect all its
> callee global subprogs by walking its instructions (a bit like
> check_max_stack_depth does).
Sorry if I'm being dense, but why is all this needed if it's already
possible to just define the timer callback from a program type that
allows sleeping, and then set the actual timeout from a different
program that is not sleepable? Isn't the set_sleepable_cb() kfunc just a
convenience then? Or did I misunderstand and it's not actually possible
to mix callback/timer arming from different program types?
-Toke
^ 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