* 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
* Re: [PATCH v7 5/5] ASoC: cs40l50: Support I2S streaming to CS40L50
From: Mark Brown @ 2024-02-13 19:55 UTC (permalink / raw)
To: James Ogletree
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
jeff, patches, linux-sound, linux-input, devicetree
In-Reply-To: <20240212173111.771107-6-jogletre@opensource.cirrus.com>
[-- Attachment #1: Type: text/plain, Size: 2198 bytes --]
On Mon, Feb 12, 2024 at 05:31:11PM +0000, James Ogletree wrote:
> @@ -0,0 +1,311 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * CS40L50 Advanced Haptic Driver with waveform memory,
Please use C++ style for the whole comment to make things look more
intentional.
> +#define CS40L50_PLL_CLK_FRQ_32768 32768
> +#define CS40L50_PLL_CLK_FRQ_1536000 1536000
> +#define CS40L50_PLL_CLK_FRQ_3072000 3072000
> +#define CS40L50_PLL_CLK_FRQ_6144000 6144000
> +#define CS40L50_PLL_CLK_FRQ_9600000 9600000
> +#define CS40L50_PLL_CLK_FRQ_12288000 12288000
I'm not sure these defines add greatly to legibility, indeed they make
me wonder where the translation function is when we take a directly
specified clock value in...
> + switch (clk_src) {
> + case CS40L50_PLL_REFCLK_BCLK:
> + ret = cs40l50_get_clk_config(codec->sysclk_rate, &clk_cfg);
> + if (ret)
> + return ret;
> + break;
We appear to have a set_sysclk() operation but this is saying the sysclk
is BCLK? Should the driver be using the bclk_ratio() interface rather
than set_sysclk(), especially given that the device only appears to
support either 32.768kHz with no audio or 48kHz and a rather restrictive
set of multiples of that for the clock?
> + case CS40L50_PLL_REFCLK_MCLK:
> + clk_cfg = CS40L50_PLL_CLK_CFG_32768;
> + break;
MCLK is always 32.768kHz?
> +static int cs40l50_set_dai_fmt(struct snd_soc_dai *codec_dai, unsigned int fmt)
> +{
> + struct cs40l50_codec *codec = snd_soc_component_get_drvdata(codec_dai->component);
> +
> + if ((fmt & SND_SOC_DAIFMT_MASTER_MASK) != SND_SOC_DAIFMT_CBS_CFS)
> + return -EINVAL;
Please use the modern names, the device is a clock consumer (it would be
nice if someone from Cirrus could update your drivers...).
> +static struct platform_driver cs40l50_codec_driver = {
> + .probe = cs40l50_codec_driver_probe,
> + .driver = {
> + .name = "cs40l50-codec",
> + },
> +};
> +module_platform_driver(cs40l50_codec_driver);
> +
> +MODULE_DESCRIPTION("ASoC CS40L50 driver");
> +MODULE_AUTHOR("James Ogletree <james.ogletree@cirrus.com>");
> +MODULE_LICENSE("GPL");
There's nothing here that ensures the driver autoloads, you need to
either a MODULE_DEVICE_TABLE or a MODULE_ALIAS.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH RFC bpf-next 0/9] allow HID-BPF to do device IOs
From: Alexei Starovoitov @ 2024-02-13 20:48 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
Cc: Benjamin Tissoires, 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: <CAP01T75Giw_5j0RXaaxX0rDzCcXXZgmHrw7QZ_Ayib8rHgunBQ@mail.gmail.com>
On Tue, Feb 13, 2024 at 08:23:17PM +0100, Kumar Kartikeya Dwivedi wrote:
> 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?
I think the flag for bpf_timer_init() is still needed.
Since we probably shouldn't combine hrtimer with workqueue.
bpf_timer_start() should do schedule_work() directly.
The latency matters in some cases.
We will use flags to specify which workqueue strategy to use.
Fingers crossed, WQ_BH will be merged in the next merge window and we'd need to expose it
as an option to bpf progs.
> >
> > > 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).
imo that would be a serious increase in the verifier complexity.
It's not clear whether this is needed at this point.
For now I would drop cur_state->in_sleepable to false before verifying global subprogs.
> > >
> > > 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.
+1. I quickly looked through the patches and it does look like 90% there.
^ permalink raw reply
* Re: [PATCH RFC bpf-next 0/9] allow HID-BPF to do device IOs
From: Alexei Starovoitov @ 2024-02-13 20:52 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: Kumar Kartikeya Dwivedi, Benjamin Tissoires, 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: <877cj8f8ht.fsf@toke.dk>
On Tue, Feb 13, 2024 at 08:51:26PM +0100, Toke Høiland-Jørgensen wrote:
> 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?
More than just convience.
bpf_set_sleepable_cb() might need to be called from non-sleepable and
there could be no way to hack it around with fake sleepable entry.
bpf_timer_cancel() clears callback_fn.
So if prog wants to bpf_timer_start() and later bpf_timer_cancel()
it would need to bpf_set_sleepable_cb() every time before bpf_timer_start().
And at that time it might be in non-sleepable ctx.
^ permalink raw reply
* Re: [PATCH 04/10] hid/hid-picolcd: Remove struct backlight_ops.check_fb
From: kernel test robot @ 2024-02-13 21:57 UTC (permalink / raw)
To: Thomas Zimmermann, lee, daniel.thompson, jingoohan1, deller,
javierm
Cc: 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 (https://download.01.org/0day-ci/archive/20240214/202402140514.sb1rerJx-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240214/202402140514.sb1rerJx-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/202402140514.sb1rerJx-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/hid/hid-picolcd_fb.c: In function 'picolcd_init_framebuffer':
>> drivers/hid/hid-picolcd_fb.c:497:13: error: 'struct fb_info' has no member named 'bl_dev'
497 | info->bl_dev = data->backlight;
| ^~
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
* [PATCH v2 RESEND] HID: nintendo: use ida for LED player id
From: Martino Fontana @ 2024-02-14 9:55 UTC (permalink / raw)
To: djogorchock, jikos, benjamin.tissoires, linux-input, linux-kernel
Cc: Martino Fontana, Ryan McClelland
Previously, the leds pattern would just increment with every controller
connected. This wouldn't take into consideration when controllers are
disconnected. The same controller could be connected and disconnected
with the pattern increasing player count each time.
This patch changes it by using an ID allocator in order to assign the
player id, the same way hid-playstation does.
Signed-off-by: Martino Fontana <tinozzo123@gmail.com>
Signed-off-by: Ryan McClelland <rymcclel@gmail.com>
---
Changes for v2:
ida_free now frees the correct id, instead of an id that got moduloed.
drivers/hid/hid-nintendo.c | 27 +++++++++++++++------------
1 file changed, 15 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-nintendo.c b/drivers/hid/hid-nintendo.c
index ccc4032fb2b0..6ab4c2ec4a5d 100644
--- a/drivers/hid/hid-nintendo.c
+++ b/drivers/hid/hid-nintendo.c
@@ -34,6 +34,7 @@
#include <linux/device.h>
#include <linux/kernel.h>
#include <linux/hid.h>
+#include <linux/idr.h>
#include <linux/input.h>
#include <linux/jiffies.h>
#include <linux/leds.h>
@@ -569,6 +570,7 @@ static const enum led_brightness joycon_player_led_patterns[JC_NUM_LED_PATTERNS]
struct joycon_ctlr {
struct hid_device *hdev;
struct input_dev *input;
+ u32 player_id;
struct led_classdev leds[JC_NUM_LEDS]; /* player leds */
struct led_classdev home_led;
enum joycon_ctlr_state ctlr_state;
@@ -2283,7 +2285,8 @@ static int joycon_home_led_brightness_set(struct led_classdev *led,
return ret;
}
-static DEFINE_SPINLOCK(joycon_input_num_spinlock);
+static DEFINE_IDA(nintendo_player_id_allocator);
+
static int joycon_leds_create(struct joycon_ctlr *ctlr)
{
struct hid_device *hdev = ctlr->hdev;
@@ -2294,20 +2297,19 @@ static int joycon_leds_create(struct joycon_ctlr *ctlr)
char *name;
int ret;
int i;
- unsigned long flags;
int player_led_pattern;
- static int input_num;
-
- /*
- * Set the player leds based on controller number
- * Because there is no standard concept of "player number", the pattern
- * number will simply increase by 1 every time a controller is connected.
- */
- spin_lock_irqsave(&joycon_input_num_spinlock, flags);
- player_led_pattern = input_num++ % JC_NUM_LED_PATTERNS;
- spin_unlock_irqrestore(&joycon_input_num_spinlock, flags);
/* configure the player LEDs */
+ ctlr->player_id = U32_MAX;
+ ret = ida_alloc(&nintendo_player_id_allocator, GFP_KERNEL);
+ if (ret < 0) {
+ hid_warn(hdev, "Failed to allocate player ID, skipping; ret=%d\n", ret);
+ goto home_led;
+ }
+ ctlr->player_id = ret;
+ player_led_pattern = ret % JC_NUM_LED_PATTERNS;
+ hid_info(ctlr->hdev, "assigned player %d led pattern", player_led_pattern + 1);
+
for (i = 0; i < JC_NUM_LEDS; i++) {
name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s:%s",
d_name,
@@ -2789,6 +2791,7 @@ static void nintendo_hid_remove(struct hid_device *hdev)
spin_unlock_irqrestore(&ctlr->lock, flags);
destroy_workqueue(ctlr->rumble_queue);
+ ida_free(&nintendo_player_id_allocator, ctlr->player_id);
hid_hw_close(hdev);
hid_hw_stop(hdev);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v7 1/5] firmware: cs_dsp: Add write sequencer interface
From: Charles Keepax @ 2024-02-14 10:24 UTC (permalink / raw)
To: James Ogletree
Cc: dmitry.torokhov, robh+dt, krzysztof.kozlowski+dt, conor+dt, lee,
broonie, jeff, patches, linux-sound, linux-input, devicetree
In-Reply-To: <20240212173111.771107-2-jogletre@opensource.cirrus.com>
On Mon, Feb 12, 2024 at 05:31:07PM +0000, James Ogletree wrote:
> A write sequencer is a sequence of register addresses
> and values executed by some Cirrus DSPs following
> power state transitions.
>
> Add support for Cirrus drivers to update or add to a
> write sequencer present in firmware.
>
> Signed-off-by: James Ogletree <jogletre@opensource.cirrus.com>
> ---
> + if (!update) {
> + if (wseq->ctl->len - op_end->offset < new_op_size) {
> + cs_dsp_err(dsp, "Not enough memory in write sequence for entry\n");
> + ret = -ENOMEM;
> + goto op_new_free;
> + }
> +
> + op_end->offset += new_op_size;
> +
> + ret = cs_dsp_coeff_write_ctrl(wseq->ctl, op_end->offset / sizeof(u32),
> + &op_end->data, sizeof(u32));
> + if (ret)
> + goto op_new_free;
> +
> + list_add_tail(&op_new->list, &wseq->ops);
This means the new write will be after the terminator in the
list. Whilst I think the code will work this way it would be
much cleaner if we kept the list ordered to match the writes,
by inserting the new element just before op_end.
Thanks,
Charles
^ permalink raw reply
* Re: [PATCH 1/7] HID: apple: remove unused members from struct apple_sc_backlight
From: Aditya Garg @ 2024-02-14 12:54 UTC (permalink / raw)
To: Jiri Slaby (SUSE)
Cc: jikos@kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Paul Pawlowski, Aun-Ali Zaidi,
Benjamin Tissoires
In-Reply-To: <20240201115320.684-2-jirislaby@kernel.org>
Hi Jiri
I got this patch tested by a user and no regression was reported.
Thanks
> On 01-Feb-2024, at 5:23 PM, Jiri Slaby (SUSE) <jirislaby@kernel.org> wrote:
>
> The backlight members in struct apple_sc_backlight are unused. They were
> added in commit 9018eacbe623 (HID: apple: Add support for keyboard
> backlight on certain T2 Macs.), but never used.
>
> As this is not a struct to communicate with the HW, remove these.
>
> Found by https://github.com/jirislaby/clang-struct.
>
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@kernel.org>
> Cc: Paul Pawlowski <paul@mrarm.io>
> Cc: Aun-Ali Zaidi <admin@kodeit.net>
> Cc: Aditya Garg <gargaditya08@live.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> ---
> drivers/hid/hid-apple.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/hid/hid-apple.c b/drivers/hid/hid-apple.c
> index b9c7c0ed7bcc..bd022e004356 100644
> --- a/drivers/hid/hid-apple.c
> +++ b/drivers/hid/hid-apple.c
> @@ -79,7 +79,6 @@ struct apple_non_apple_keyboard {
> struct apple_sc_backlight {
> struct led_classdev cdev;
> struct hid_device *hdev;
> - unsigned short backlight_off, backlight_on_min, backlight_on_max;
> };
>
> struct apple_sc {
> --
> 2.43.0
>
^ 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-14 12:56 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Kumar Kartikeya Dwivedi, Benjamin Tissoires, 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: <b2k6rlzu5vgpouedwjbsigoteo43nwfk6qeeb2pc7c3r4ejnm6@nml66ds6wbeo>
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> On Tue, Feb 13, 2024 at 08:51:26PM +0100, Toke Høiland-Jørgensen wrote:
>> 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?
>
> More than just convience.
> bpf_set_sleepable_cb() might need to be called from non-sleepable and
> there could be no way to hack it around with fake sleepable entry.
> bpf_timer_cancel() clears callback_fn.
> So if prog wants to bpf_timer_start() and later bpf_timer_cancel()
> it would need to bpf_set_sleepable_cb() every time before bpf_timer_start().
> And at that time it might be in non-sleepable ctx.
Ah, right, makes sense; didn't think about bpf_timer_cancel(). Thanks
for the explanation :)
-Toke
^ permalink raw reply
* [PATCH 0/5] Fixes and updates to amd-sfh
From: Basavaraj Natikar @ 2024-02-14 14:41 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input
Cc: akshata.mukundshetty, Basavaraj Natikar
This patch series include changes for:
- Increase sensor command timeout.
- Update HPD sensor structure elements.
- Avoid disabling the interrupt.
- Improve boot time.
- Extend MP2 register access.
Basavaraj Natikar (5):
HID: amd_sfh: Increase sensor command timeout
HID: amd_sfh: Update HPD sensor structure elements
HID: amd_sfh: Avoid disabling the interrupt
HID: amd_sfh: Improve boot time when SFH is available
HID: amd_sfh: Extend MP2 register access to SFH
drivers/hid/amd-sfh-hid/amd_sfh_common.h | 16 +++
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 118 ++++++++++++++----
drivers/hid/amd-sfh-hid/amd_sfh_pcie.h | 6 +-
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 2 +-
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 4 +-
.../amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 10 +-
6 files changed, 124 insertions(+), 32 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH 1/5] HID: amd_sfh: Increase sensor command timeout
From: Basavaraj Natikar @ 2024-02-14 14:41 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input
Cc: akshata.mukundshetty, Basavaraj Natikar
In-Reply-To: <20240214144144.682750-1-Basavaraj.Natikar@amd.com>
During the initialization sensors may take some time to respond. Hence,
increase the sensor command timeouts in order to obtain status responses
within a maximum timeout.
Co-developed-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 2530fa98b568..8a4cd793d021 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -39,11 +39,11 @@ static int amd_sfh_wait_response_v2(struct amd_mp2_dev *mp2, u8 sid, u32 sensor_
{
union cmd_response cmd_resp;
- /* Get response with status within a max of 1600 ms timeout */
+ /* Get response with status within a max of 10 seconds timeout */
if (!readl_poll_timeout(mp2->mmio + AMD_P2C_MSG(0), cmd_resp.resp,
(cmd_resp.response_v2.response == sensor_sts &&
cmd_resp.response_v2.status == 0 && (sid == 0xff ||
- cmd_resp.response_v2.sensor_id == sid)), 500, 1600000))
+ cmd_resp.response_v2.sensor_id == sid)), 500, 10000000))
return cmd_resp.response_v2.response;
return SENSOR_DISABLED;
--
2.25.1
^ permalink raw reply related
* [PATCH 2/5] HID: amd_sfh: Update HPD sensor structure elements
From: Basavaraj Natikar @ 2024-02-14 14:41 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input
Cc: akshata.mukundshetty, Basavaraj Natikar
In-Reply-To: <20240214144144.682750-1-Basavaraj.Natikar@amd.com>
HPD sensor data is not populating properly because of wrong order of HPD
sensor structure elements. So update the order of structure elements to
match the HPD sensor data received from the firmware.
Fixes: 24a31ea94922 ("HID: amd_sfh: Add initial support for HPD sensor")
Co-developed-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_pcie.h | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
index 70add75fc506..05e400a4a83e 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.h
@@ -90,10 +90,10 @@ enum mem_use_type {
struct hpd_status {
union {
struct {
- u32 human_presence_report : 4;
- u32 human_presence_actual : 4;
- u32 probablity : 8;
u32 object_distance : 16;
+ u32 probablity : 8;
+ u32 human_presence_actual : 4;
+ u32 human_presence_report : 4;
} shpd;
u32 val;
};
--
2.25.1
^ permalink raw reply related
* [PATCH 3/5] HID: amd_sfh: Avoid disabling the interrupt
From: Basavaraj Natikar @ 2024-02-14 14:41 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input
Cc: akshata.mukundshetty, Basavaraj Natikar
In-Reply-To: <20240214144144.682750-1-Basavaraj.Natikar@amd.com>
HP ProBook x360 435 G7 using older version of firmware which doesn't
support disabling the interrupt for all commands. Hence avoid disabling
the interrupt for that particular model.
Link: https://bugzilla.kernel.org/show_bug.cgi?id=218104
Fixes: b300667b33b2 ("HID: amd_sfh: Disable the interrupt for all command")
Co-developed-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Akshata MukundShetty <akshata.mukundshetty@amd.com>
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 30 +++++++++++++++++++++++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 8a4cd793d021..8c1809c7d1f7 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -35,6 +35,8 @@ static int sensor_mask_override = -1;
module_param_named(sensor_mask, sensor_mask_override, int, 0444);
MODULE_PARM_DESC(sensor_mask, "override the detected sensors mask");
+static bool intr_disable = true;
+
static int amd_sfh_wait_response_v2(struct amd_mp2_dev *mp2, u8 sid, u32 sensor_sts)
{
union cmd_response cmd_resp;
@@ -55,7 +57,7 @@ static void amd_start_sensor_v2(struct amd_mp2_dev *privdata, struct amd_mp2_sen
cmd_base.ul = 0;
cmd_base.cmd_v2.cmd_id = ENABLE_SENSOR;
- cmd_base.cmd_v2.intr_disable = 1;
+ cmd_base.cmd_v2.intr_disable = intr_disable;
cmd_base.cmd_v2.period = info.period;
cmd_base.cmd_v2.sensor_id = info.sensor_idx;
cmd_base.cmd_v2.length = 16;
@@ -73,7 +75,7 @@ static void amd_stop_sensor_v2(struct amd_mp2_dev *privdata, u16 sensor_idx)
cmd_base.ul = 0;
cmd_base.cmd_v2.cmd_id = DISABLE_SENSOR;
- cmd_base.cmd_v2.intr_disable = 1;
+ cmd_base.cmd_v2.intr_disable = intr_disable;
cmd_base.cmd_v2.period = 0;
cmd_base.cmd_v2.sensor_id = sensor_idx;
cmd_base.cmd_v2.length = 16;
@@ -87,7 +89,7 @@ static void amd_stop_all_sensor_v2(struct amd_mp2_dev *privdata)
union sfh_cmd_base cmd_base;
cmd_base.cmd_v2.cmd_id = STOP_ALL_SENSORS;
- cmd_base.cmd_v2.intr_disable = 1;
+ cmd_base.cmd_v2.intr_disable = intr_disable;
cmd_base.cmd_v2.period = 0;
cmd_base.cmd_v2.sensor_id = 0;
@@ -292,6 +294,26 @@ int amd_sfh_irq_init(struct amd_mp2_dev *privdata)
return 0;
}
+static int mp2_disable_intr(const struct dmi_system_id *id)
+{
+ intr_disable = false;
+ return 0;
+}
+
+static const struct dmi_system_id dmi_sfh_table[] = {
+ {
+ /*
+ * https://bugzilla.kernel.org/show_bug.cgi?id=218104
+ */
+ .callback = mp2_disable_intr,
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "HP"),
+ DMI_MATCH(DMI_PRODUCT_NAME, "HP ProBook x360 435 G7"),
+ },
+ },
+ {}
+};
+
static const struct dmi_system_id dmi_nodevs[] = {
{
/*
@@ -315,6 +337,8 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
if (dmi_first_match(dmi_nodevs))
return -ENODEV;
+ dmi_check_system(dmi_sfh_table);
+
privdata = devm_kzalloc(&pdev->dev, sizeof(*privdata), GFP_KERNEL);
if (!privdata)
return -ENOMEM;
--
2.25.1
^ permalink raw reply related
* [PATCH 5/5] HID: amd_sfh: Extend MP2 register access to SFH
From: Basavaraj Natikar @ 2024-02-14 14:41 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input
Cc: akshata.mukundshetty, Basavaraj Natikar
In-Reply-To: <20240214144144.682750-1-Basavaraj.Natikar@amd.com>
Various MP2 register sets are supported by newer processors. Therefore,
extend MP2 register access to SFH.
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_common.h | 14 ++++++++++++++
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 9 ++++++---
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c | 2 +-
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c | 4 ++--
drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c | 10 +++++-----
5 files changed, 28 insertions(+), 11 deletions(-)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
index ef5551c1eec5..e5620d7db569 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
@@ -19,6 +19,9 @@
#define AMD_C2P_MSG(regno) (0x10500 + ((regno) * 4))
#define AMD_P2C_MSG(regno) (0x10680 + ((regno) * 4))
+#define AMD_C2P_MSG_V1(regno) (0x10900 + ((regno) * 4))
+#define AMD_P2C_MSG_V1(regno) (0x10500 + ((regno) * 4))
+
#define SENSOR_ENABLED 4
#define SENSOR_DISABLED 5
@@ -55,6 +58,7 @@ struct amd_mp2_dev {
struct sfh_dev_status dev_en;
struct work_struct work;
u8 init_done;
+ u8 rver;
};
struct amd_mp2_ops {
@@ -81,4 +85,14 @@ void amd_sfh_clear_intr_v2(struct amd_mp2_dev *privdata);
int amd_sfh_irq_init_v2(struct amd_mp2_dev *privdata);
void amd_sfh_clear_intr(struct amd_mp2_dev *privdata);
int amd_sfh_irq_init(struct amd_mp2_dev *privdata);
+
+static inline u64 amd_get_c2p_val(struct amd_mp2_dev *mp2, u32 idx)
+{
+ return mp2->rver == 1 ? AMD_C2P_MSG_V1(idx) : AMD_C2P_MSG(idx);
+}
+
+static inline u64 amd_get_p2c_val(struct amd_mp2_dev *mp2, u32 idx)
+{
+ return mp2->rver == 1 ? AMD_P2C_MSG_V1(idx) : AMD_P2C_MSG(idx);
+}
#endif
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 495ec1179ee5..9e97c26c4482 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -99,9 +99,9 @@ static void amd_stop_all_sensor_v2(struct amd_mp2_dev *privdata)
void amd_sfh_clear_intr_v2(struct amd_mp2_dev *privdata)
{
- if (readl(privdata->mmio + AMD_P2C_MSG(4))) {
- writel(0, privdata->mmio + AMD_P2C_MSG(4));
- writel(0xf, privdata->mmio + AMD_P2C_MSG(5));
+ if (readl(privdata->mmio + amd_get_p2c_val(privdata, 4))) {
+ writel(0, privdata->mmio + amd_get_p2c_val(privdata, 4));
+ writel(0xf, privdata->mmio + amd_get_p2c_val(privdata, 5));
}
}
@@ -410,6 +410,9 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
privdata->sfh1_1_ops = (const struct amd_sfh1_1_ops *)id->driver_data;
if (privdata->sfh1_1_ops) {
+ if (boot_cpu_data.x86 >= 0x1A)
+ privdata->rver = 1;
+
rc = devm_work_autocancel(&pdev->dev, &privdata->work, sfh1_1_init_work);
if (rc)
return rc;
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
index 33fbdde8aff0..c8916afefa62 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_desc.c
@@ -251,7 +251,7 @@ static u8 get_input_rep(u8 current_index, int sensor_idx, int report_id,
break;
case HPD_IDX:
get_common_inputs(&hpd_input.common_property, report_id);
- hpdstatus.val = readl(mp2->mmio + AMD_C2P_MSG(4));
+ hpdstatus.val = readl(mp2->mmio + amd_get_c2p_val(mp2, 4));
hpd_input.human_presence = hpdstatus.shpd.presence;
report_size = sizeof(hpd_input);
memcpy(input_report, &hpd_input, sizeof(hpd_input));
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
index 9dbe6f4cb294..5b24d5f63701 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_init.c
@@ -172,7 +172,7 @@ static int amd_sfh1_1_hid_client_init(struct amd_mp2_dev *privdata)
if (rc)
goto cleanup;
- writel(0, privdata->mmio + AMD_P2C_MSG(0));
+ writel(0, privdata->mmio + amd_get_p2c_val(privdata, 0));
mp2_ops->start(privdata, info);
status = amd_sfh_wait_for_response
(privdata, cl_data->sensor_idx[i], ENABLE_SENSOR);
@@ -298,7 +298,7 @@ static void amd_sfh_set_ops(struct amd_mp2_dev *mp2)
int amd_sfh1_1_init(struct amd_mp2_dev *mp2)
{
- u32 phy_base = readl(mp2->mmio + AMD_C2P_MSG(22));
+ u32 phy_base = readl(mp2->mmio + amd_get_c2p_val(mp2, 22));
struct device *dev = &mp2->pdev->dev;
struct sfh_base_info binfo;
int rc;
diff --git a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
index ae36312bc236..2de2668a0277 100644
--- a/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
+++ b/drivers/hid/amd-sfh-hid/sfh1_1/amd_sfh_interface.c
@@ -20,7 +20,7 @@ static int amd_sfh_wait_response(struct amd_mp2_dev *mp2, u8 sid, u32 cmd_id)
struct sfh_cmd_response cmd_resp;
/* Get response with status within a max of 10000 ms timeout */
- if (!readl_poll_timeout(mp2->mmio + AMD_P2C_MSG(0), cmd_resp.resp,
+ if (!readl_poll_timeout(mp2->mmio + amd_get_p2c_val(mp2, 0), cmd_resp.resp,
(cmd_resp.response.response == 0 &&
cmd_resp.response.cmd_id == cmd_id && (sid == 0xff ||
cmd_resp.response.sensor_id == sid)), 500, 10000000))
@@ -39,7 +39,7 @@ static void amd_start_sensor(struct amd_mp2_dev *privdata, struct amd_mp2_sensor
cmd_base.cmd.sub_cmd_value = 1;
cmd_base.cmd.sensor_id = info.sensor_idx;
- writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG(0));
+ writel(cmd_base.ul, privdata->mmio + amd_get_c2p_val(privdata, 0));
}
static void amd_stop_sensor(struct amd_mp2_dev *privdata, u16 sensor_idx)
@@ -52,8 +52,8 @@ static void amd_stop_sensor(struct amd_mp2_dev *privdata, u16 sensor_idx)
cmd_base.cmd.sub_cmd_value = 1;
cmd_base.cmd.sensor_id = sensor_idx;
- writeq(0x0, privdata->mmio + AMD_C2P_MSG(1));
- writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG(0));
+ writeq(0x0, privdata->mmio + amd_get_c2p_val(privdata, 1));
+ writel(cmd_base.ul, privdata->mmio + amd_get_c2p_val(privdata, 0));
}
static void amd_stop_all_sensor(struct amd_mp2_dev *privdata)
@@ -66,7 +66,7 @@ static void amd_stop_all_sensor(struct amd_mp2_dev *privdata)
/* 0xf indicates all sensors */
cmd_base.cmd.sensor_id = 0xf;
- writel(cmd_base.ul, privdata->mmio + AMD_C2P_MSG(0));
+ writel(cmd_base.ul, privdata->mmio + amd_get_c2p_val(privdata, 0));
}
static struct amd_mp2_ops amd_sfh_ops = {
--
2.25.1
^ permalink raw reply related
* [PATCH 4/5] HID: amd_sfh: Improve boot time when SFH is available
From: Basavaraj Natikar @ 2024-02-14 14:41 UTC (permalink / raw)
To: jikos, benjamin.tissoires, linux-input
Cc: akshata.mukundshetty, Basavaraj Natikar
In-Reply-To: <20240214144144.682750-1-Basavaraj.Natikar@amd.com>
AMD SFH load takes longer time in initialization. Hence split and defer
initialization code to improve SFH module load time and boot time of the
system when SFH is available.
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
drivers/hid/amd-sfh-hid/amd_sfh_common.h | 2 +
drivers/hid/amd-sfh-hid/amd_sfh_pcie.c | 75 ++++++++++++++++++++----
2 files changed, 64 insertions(+), 13 deletions(-)
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_common.h b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
index a1950bc6e6ce..ef5551c1eec5 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_common.h
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_common.h
@@ -53,6 +53,8 @@ struct amd_mp2_dev {
/* mp2 active control status */
u32 mp2_acs;
struct sfh_dev_status dev_en;
+ struct work_struct work;
+ u8 init_done;
};
struct amd_mp2_ops {
diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
index 8c1809c7d1f7..495ec1179ee5 100644
--- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
+++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
@@ -10,6 +10,7 @@
#include <linux/bitops.h>
#include <linux/delay.h>
+#include <linux/devm-helpers.h>
#include <linux/dma-mapping.h>
#include <linux/dmi.h>
#include <linux/interrupt.h>
@@ -329,6 +330,48 @@ static const struct dmi_system_id dmi_nodevs[] = {
{ }
};
+static void sfh1_1_init_work(struct work_struct *work)
+{
+ struct amd_mp2_dev *mp2 = container_of(work, struct amd_mp2_dev, work);
+ struct pci_dev *pdev = mp2->pdev;
+ int rc;
+
+ rc = mp2->sfh1_1_ops->init(mp2);
+ if (rc) {
+ dev_err(&pdev->dev, "sfh1_1_init failed err %d\n", rc);
+ return;
+ }
+
+ amd_sfh_clear_intr(mp2);
+ mp2->init_done = 1;
+}
+
+static void sfh_init_work(struct work_struct *work)
+{
+ struct amd_mp2_dev *mp2 = container_of(work, struct amd_mp2_dev, work);
+ struct pci_dev *pdev = mp2->pdev;
+ int rc;
+
+ rc = amd_sfh_hid_client_init(mp2);
+ if (rc) {
+ amd_sfh_clear_intr(mp2);
+ dev_err(&pdev->dev, "amd_sfh_hid_client_init failed err %d\n", rc);
+ return;
+ }
+
+ amd_sfh_clear_intr(mp2);
+ mp2->init_done = 1;
+}
+
+static void amd_sfh_remove(struct pci_dev *pdev)
+{
+ struct amd_mp2_dev *mp2 = pci_get_drvdata(pdev);
+
+ flush_work(&mp2->work);
+ if (mp2->init_done)
+ mp2->mp2_ops->remove(mp2);
+}
+
static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id)
{
struct amd_mp2_dev *privdata;
@@ -367,10 +410,12 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
privdata->sfh1_1_ops = (const struct amd_sfh1_1_ops *)id->driver_data;
if (privdata->sfh1_1_ops) {
- rc = privdata->sfh1_1_ops->init(privdata);
+ rc = devm_work_autocancel(&pdev->dev, &privdata->work, sfh1_1_init_work);
if (rc)
return rc;
- goto init_done;
+
+ schedule_work(&privdata->work);
+ return 0;
}
mp2_select_ops(privdata);
@@ -381,33 +426,34 @@ static int amd_mp2_pci_probe(struct pci_dev *pdev, const struct pci_device_id *i
return rc;
}
- rc = amd_sfh_hid_client_init(privdata);
+ rc = devm_work_autocancel(&pdev->dev, &privdata->work, sfh_init_work);
if (rc) {
amd_sfh_clear_intr(privdata);
- if (rc != -EOPNOTSUPP)
- dev_err(&pdev->dev, "amd_sfh_hid_client_init failed\n");
return rc;
}
-init_done:
- amd_sfh_clear_intr(privdata);
-
- return devm_add_action_or_reset(&pdev->dev, privdata->mp2_ops->remove, privdata);
+ schedule_work(&privdata->work);
+ return 0;
}
static void amd_sfh_shutdown(struct pci_dev *pdev)
{
struct amd_mp2_dev *mp2 = pci_get_drvdata(pdev);
- if (mp2 && mp2->mp2_ops)
- mp2->mp2_ops->stop_all(mp2);
+ if (mp2) {
+ flush_work(&mp2->work);
+ if (mp2->init_done)
+ mp2->mp2_ops->stop_all(mp2);
+ }
}
static int __maybe_unused amd_mp2_pci_resume(struct device *dev)
{
struct amd_mp2_dev *mp2 = dev_get_drvdata(dev);
- mp2->mp2_ops->resume(mp2);
+ flush_work(&mp2->work);
+ if (mp2->init_done)
+ mp2->mp2_ops->resume(mp2);
return 0;
}
@@ -416,7 +462,9 @@ static int __maybe_unused amd_mp2_pci_suspend(struct device *dev)
{
struct amd_mp2_dev *mp2 = dev_get_drvdata(dev);
- mp2->mp2_ops->suspend(mp2);
+ flush_work(&mp2->work);
+ if (mp2->init_done)
+ mp2->mp2_ops->suspend(mp2);
return 0;
}
@@ -438,6 +486,7 @@ static struct pci_driver amd_mp2_pci_driver = {
.probe = amd_mp2_pci_probe,
.driver.pm = &amd_mp2_pm_ops,
.shutdown = amd_sfh_shutdown,
+ .remove = amd_sfh_remove,
};
module_pci_driver(amd_mp2_pci_driver);
--
2.25.1
^ permalink raw reply related
* Re: [PATCH v10 1/3] dt-bindings: i2c: Add CP2112 HID USB to SMBus
From: Danny Kaehn @ 2024-02-14 16:06 UTC (permalink / raw)
To: robh
Cc: andriy.shevchenko, bartosz.golaszewski, bentiss, danny.kaehn,
devicetree, dmitry.torokhov, ethan.twardy, jikos,
krzysztof.kozlowski+dt, linux-input, niyas.sait
In-Reply-To: <20240213152825.GA1223720-robh@kernel.org>
Thanks for taking a look Rob.
On Tue, 2024-02-13 at 09:28 -0600, Rob Herring wrote:
> 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.
...
> > 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.
>
Ack. Mostly just forwarding on Andy Shevchenko's suggestion for making a more
consistent interface across ACPI and DT since ACPI doesn't support identifying
children by named nodes.
> >
> > 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.
>
Fair enough.
> > 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?
This is exclusively for bus recovery (not implemented in the driver patch
sent here). I believe there's precedent for this in bindings like i2c-imx.yaml?
(skip if the above was all you needed):
Hopefully without going into more details than you're interested in, the
CP2112 hardware doesn't implement any runtime bus recovery algorithms.
Even just by bridging two of the CP2112's GPIOs with SCL and SDA,
I was able to use the generic GPIO bus recovery routine to recover a stuck bus.
This was especially important since v2 of the CP2112 hardware has an errata
which can cause uncompleted I2C transactions on a semi-regular basis.
>
> > +
> > + 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.
I had gone back and forth on this for quite some time. Would you suggest this
just because it _can_ be combined, due to the naming constraint on the "hog"
nodes? (as opposed to the i2c not being able to be combined, due to
unconstrained names of child nodes?).
I had initially thought this approach would scale better -- say there was a
similar chip with I2C, GPIO, SPI, and UART interfaces -- would GPIO still
share a node with the parent? And is there a reason that gpio is
special, or just it _can_ be combined due to the naming restrictions? Looking
at some of the bindings under mfd/ I see the GPIO controller broken into a
named child node, although I see they also have their own compatible strings...
>
>
> > + 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
> >
Thanks,
Danny Kaehn
^ permalink raw reply
* Re: [PATCH RFC bpf-next 0/9] allow HID-BPF to do device IOs
From: Benjamin Tissoires @ 2024-02-14 17:10 UTC (permalink / raw)
To: Kumar Kartikeya Dwivedi
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: <CAP01T75Giw_5j0RXaaxX0rDzCcXXZgmHrw7QZ_Ayib8rHgunBQ@mail.gmail.com>
On Feb 13 2024, Kumar Kartikeya Dwivedi wrote:
> 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.
Hehe, thanks a lot. Indeed, it was a simple fix. I tried to put this
everywhere but here, and with your help got it working in 2 mins :)
>
> > 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).
FWIW, this (or Alexei's suggestion) is still not implemented in v2
>
> > >
> > > 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.
the kfunc was a rabbit hole:
- I needed to teach the verifier about BPF_TIMER in kfunc
- I needed to teach the verifier about the kfunc itself
- I'm failing at calling the callback :(
Anyway, I'm about to send a second RFC so we can discuss on the code and
see where my monkey patching capabilities are reaching their limits.
Cheers,
Benjamin
^ permalink raw reply
* [PATCH RFC bpf-next v2 00/10] allow HID-BPF to do device IOs
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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
[Still a RFC: there are a lot of FIXMEs in the code, and
calling the sleepable timer cb actually crashes.]
[Also using bpf-next as the base tree as there will be conflicting
changes otherwise]
This is crashing, and I have a few questions in the code (look for all
of the FIXMEs), so sending this now before I become insane :)
For reference, the use cases I have in mind:
---
Basically, I need to be able to defer a HID-BPF program for the
following reasons (from the aforementioned patch):
1. defer an event:
Sometimes we receive an out of proximity event, but the device can not
be trusted enough, and we need to ensure that we won't receive another
one in the following n milliseconds. So we need to wait those n
milliseconds, and eventually re-inject that event in the stack.
2. inject new events in reaction to one given event:
We might want to transform one given event into several. This is the
case for macro keys where a single key press is supposed to send
a sequence of key presses. But this could also be used to patch a
faulty behavior, if a device forgets to send a release event.
3. communicate with the device in reaction to one event:
We might want to communicate back to the device after a given event.
For example a device might send us an event saying that it came back
from sleeping state and needs to be re-initialized.
Currently we can achieve that by keeping a userspace program around,
raise a bpf event, and let that userspace program inject the events and
commands.
However, we are just keeping that program alive as a daemon for just
scheduling commands. There is no logic in it, so it doesn't really justify
an actual userspace wakeup. So a kernel workqueue seems simpler to handle.
The other part I'm not sure is whether we can say that BPF maps of type
queue/stack can be used in sleepable context.
I don't see any warning when running the test programs, but that's probably
not a guarantee I'm doing the things properly :)
Cheers,
Benjamin
To: Alexei Starovoitov <ast@kernel.org>
To: Daniel Borkmann <daniel@iogearbox.net>
To: John Fastabend <john.fastabend@gmail.com>
To: Andrii Nakryiko <andrii@kernel.org>
To: Martin KaFai Lau <martin.lau@linux.dev>
To: Eduard Zingerman <eddyz87@gmail.com>
To: Song Liu <song@kernel.org>
To: Yonghong Song <yonghong.song@linux.dev>
To: KP Singh <kpsingh@kernel.org>
To: Stanislav Fomichev <sdf@google.com>
To: Hao Luo <haoluo@google.com>
To: Jiri Olsa <jolsa@kernel.org>
To: Jiri Kosina <jikos@kernel.org>
To: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Jonathan Corbet <corbet@lwn.net>
To: Shuah Khan <shuah@kernel.org>
Cc: <bpf@vger.kernel.org>
Cc: <linux-kernel@vger.kernel.org>
Cc: <linux-input@vger.kernel.org>
Cc: <linux-doc@vger.kernel.org>
Cc: <linux-kselftest@vger.kernel.org>
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Changes in v2:
- make use of bpf_timer (and dropped the custom HID handling)
- implemented bpf_timer_set_sleepable_cb as a kfunc
- still not implemented global subprogs
- no sleepable bpf_timer selftests yet
- Link to v1: https://lore.kernel.org/r/20240209-hid-bpf-sleepable-v1-0-4cc895b5adbd@kernel.org
---
Benjamin Tissoires (10):
bpf/verifier: introduce in_sleepable() helper
bpf/helpers: introduce sleepable timers
bpf/verifier: allow more maps in sleepable bpf programs
HID: bpf/dispatch: regroup kfuncs definitions
HID: bpf: export hid_hw_output_report as a BPF kfunc
selftests/hid: Add test for hid_bpf_hw_output_report
HID: bpf: allow to inject HID event from BPF
selftests/hid: add tests for hid_bpf_input_report
HID: bpf: allow to use bpf_timer_set_sleepable_cb() in tracing callbacks.
selftests/hid: add test for bpf_timer
Documentation/hid/hid-bpf.rst | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 232 ++++++++++++++-------
drivers/hid/hid-core.c | 2 +
include/linux/bpf_verifier.h | 2 +
include/linux/hid_bpf.h | 3 +
include/uapi/linux/bpf.h | 12 ++
kernel/bpf/helpers.c | 105 +++++++++-
kernel/bpf/verifier.c | 91 +++++++-
tools/testing/selftests/hid/hid_bpf.c | 195 ++++++++++++++++-
tools/testing/selftests/hid/progs/hid.c | 198 ++++++++++++++++++
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 8 +
11 files changed, 756 insertions(+), 94 deletions(-)
---
base-commit: 4f7a05917237b006ceae760507b3d15305769ade
change-id: 20240205-hid-bpf-sleepable-c01260fd91c4
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* [PATCH RFC bpf-next v2 01/10] bpf/verifier: introduce in_sleepable() helper
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
No code change, but it'll allow to have only one place to change
everything when we add in_sleepable in cur_state.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
changes in v2 (compared to the one attaches to v1 0/9):
- dropped the cur_state flag, so it can be put first
---
kernel/bpf/verifier.c | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 64fa188d00ad..7831adba9abf 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -5255,6 +5255,11 @@ 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;
+}
+
/* The non-sleepable programs and sleepable programs with explicit bpf_rcu_read_lock()
* can dereference RCU protected pointers and result is PTR_TRUSTED.
*/
@@ -5262,7 +5267,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 */
@@ -10149,7 +10154,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;
}
@@ -10179,7 +10184,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;
}
@@ -11538,7 +11543,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);
}
}
@@ -12059,7 +12064,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;
}
@@ -18197,7 +18202,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
@@ -19673,7 +19678,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
^ permalink raw reply related
* [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
They are implemented as a kfunc, which means a little bit of tweaks in
the verifier.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
changes in v2 (compared to the one attaches to v1 0/9):
- make use of a kfunc
- add a (non-used) BPF_F_TIMER_SLEEPABLE
- the callback is *not* called, it makes the kernel crashes
---
include/linux/bpf_verifier.h | 2 +
include/uapi/linux/bpf.h | 12 +++++
kernel/bpf/helpers.c | 105 ++++++++++++++++++++++++++++++++++++++++---
kernel/bpf/verifier.c | 72 ++++++++++++++++++++++++++---
4 files changed, 180 insertions(+), 11 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..0838597028a9 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -7427,6 +7427,18 @@ enum {
BPF_F_TIMER_CPU_PIN = (1ULL << 1),
};
+/* Extra flags to control bpf_timer_init() behaviour, in addition to CLOCK_*.
+ * - BPF_F_TIMER_SLEEPABLE: Timer will run in a workqueue in a sleepable
+ * context.
+ */
+enum {
+ /* CLOCK_* are using bits 0 to 3 */
+ BPF_F_TIMER_SLEEPABLE = (1ULL << 4),
+ __MAX_BPF_F_TIMER_INIT,
+};
+
+#define MAX_BPF_F_TIMER_INIT __MAX_BPF_F_TIMER_INIT
+
/* BPF numbers iterator state */
struct bpf_iter_num {
/* opaque iterator state; having __u64 here allows to preserve correct
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index 4db1c658254c..2dbc09ce841a 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,59 @@ 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;
+
+ /* FIXME: do we need any locking? */
+ 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);
+ }
+
+ /* FIXME: this crashes the system with
+ * BUG: kernel NULL pointer dereference, address: 000000000000000b
+ */
+ /* callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0); */
+ /* The verifier checked that return value is zero. */
+}
+
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;
@@ -1154,10 +1197,14 @@ static enum hrtimer_restart bpf_timer_cb(struct hrtimer *hrtimer)
return HRTIMER_NORESTART;
}
+#define BPF_TIMER_CLOCK_MASK (MAX_CLOCKS - 1) /* 0xf */
+#define BPF_TIMER_FLAGS_MASK GENMASK_ULL(63, 4)
+
BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map,
u64, flags)
{
- clockid_t clockid = flags & (MAX_CLOCKS - 1);
+ clockid_t clockid = flags & BPF_TIMER_CLOCK_MASK;
+ u64 bpf_timer_flags = flags & BPF_TIMER_FLAGS_MASK;
struct bpf_hrtimer *t;
int ret = 0;
@@ -1168,7 +1215,7 @@ BPF_CALL_3(bpf_timer_init, struct bpf_timer_kern *, timer, struct bpf_map *, map
if (in_nmi())
return -EOPNOTSUPP;
- if (flags >= MAX_CLOCKS ||
+ if (bpf_timer_flags & ~(BPF_F_TIMER_SLEEPABLE) ||
/* similar to timerfd except _ALARM variants are not supported */
(clockid != CLOCK_MONOTONIC &&
clockid != CLOCK_REALTIME &&
@@ -1190,7 +1237,10 @@ 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);
+ /* FIXME: probably do something about the SLEEPABLE flag */
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,
@@ -1353,6 +1415,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;
}
@@ -1407,6 +1470,8 @@ void bpf_timer_cancel_and_free(void *val)
*/
if (this_cpu_read(hrtimer_running) != t)
hrtimer_cancel(&t->timer);
+
+ cancel_work_sync(&t->work);
kfree(t);
}
@@ -2542,6 +2607,33 @@ __bpf_kfunc void bpf_throw(u64 cookie)
WARN(1, "A call to BPF exception callback should never return\n");
}
+/* FIXME: use kernel doc style */
+/* 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.
+ */
+__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 = {};
+
+ /* FIXME: definietely not sure this is OK */
+ arch_bpf_stack_walk(bpf_stack_walker, &ctx);
+ WARN_ON_ONCE(!ctx.aux);
+
+ if (!ctx.aux)
+ return -EINVAL;
+
+ return __bpf_timer_set_callback(timer, (void *)callback_fn, ctx.aux, true);
+}
+
__bpf_kfunc_end_defs();
BTF_KFUNCS_START(generic_btf_ids)
@@ -2573,6 +2665,7 @@ BTF_ID_FLAGS(func, bpf_task_get_cgroup1, KF_ACQUIRE | KF_RCU | KF_RET_NULL)
#endif
BTF_ID_FLAGS(func, bpf_task_from_pid, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, bpf_throw)
+BTF_ID_FLAGS(func, bpf_timer_set_sleepable_cb)
BTF_KFUNCS_END(generic_btf_ids)
static const struct btf_kfunc_id_set generic_kfunc_set = {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 7831adba9abf..67da3f7bddb5 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -503,6 +503,8 @@ static bool is_dynptr_ref_function(enum bpf_func_id func_id)
static bool is_sync_callback_calling_kfunc(u32 btf_id);
static bool is_bpf_throw_kfunc(struct bpf_insn *insn);
+static bool is_bpf_timer_set_sleepable_cb_kfunc(u32 btf_id);
+
static bool is_sync_callback_calling_function(enum bpf_func_id func_id)
{
return func_id == BPF_FUNC_for_each_map_elem ||
@@ -513,7 +515,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 ||
+ is_bpf_timer_set_sleepable_cb_kfunc(func_id);
}
static bool is_callback_calling_function(enum bpf_func_id func_id)
@@ -1414,6 +1417,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;
@@ -2413,6 +2417,7 @@ static struct bpf_verifier_state *push_async_cb(struct bpf_verifier_env *env,
* Initialize it similar to do_check_common().
*/
elem->st.branches = 1;
+ elem->st.in_sleepable = env->subprog_info[subprog].is_sleepable;
frame = kzalloc(sizeof(*frame), GFP_KERNEL);
if (!frame)
goto err;
@@ -5257,7 +5262,8 @@ static int map_kptr_match_type(struct bpf_verifier_env *env,
static bool in_sleepable(struct bpf_verifier_env *env)
{
- return env->prog->aux->sleepable;
+ 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()
@@ -5439,6 +5445,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? */
+ 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));
@@ -9439,11 +9465,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 ||
+ is_bpf_timer_set_sleepable_cb_kfunc(insn->imm))) {
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 = is_bpf_timer_set_sleepable_cb_kfunc(insn->imm);
async_cb = push_async_cb(env, env->subprog_info[subprog].start,
insn_idx, subprog);
if (!async_cb)
@@ -10412,6 +10440,10 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn
break;
}
+ if (is_bpf_timer_set_sleepable_cb_kfunc(func_id))
+ err = push_callback_call(env, insn, insn_idx, meta.subprogno,
+ set_timer_callback_state);
+
if (err)
return err;
@@ -10789,6 +10821,7 @@ enum {
KF_ARG_LIST_NODE_ID,
KF_ARG_RB_ROOT_ID,
KF_ARG_RB_NODE_ID,
+ KF_ARG_TIMER_ID,
};
BTF_ID_LIST(kf_arg_btf_ids)
@@ -10797,6 +10830,7 @@ BTF_ID(struct, bpf_list_head)
BTF_ID(struct, bpf_list_node)
BTF_ID(struct, bpf_rb_root)
BTF_ID(struct, bpf_rb_node)
+BTF_ID(struct, bpf_timer_kern)
static bool __is_kfunc_ptr_arg_type(const struct btf *btf,
const struct btf_param *arg, int type)
@@ -10840,6 +10874,12 @@ static bool is_kfunc_arg_rbtree_node(const struct btf *btf, const struct btf_par
return __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_RB_NODE_ID);
}
+static bool is_kfunc_arg_timer(const struct btf *btf, const struct btf_param *arg)
+{
+ bool ret = __is_kfunc_ptr_arg_type(btf, arg, KF_ARG_TIMER_ID);
+ return ret;
+}
+
static bool is_kfunc_arg_callback(struct bpf_verifier_env *env, const struct btf *btf,
const struct btf_param *arg)
{
@@ -10908,6 +10948,7 @@ enum kfunc_ptr_arg_type {
KF_ARG_PTR_TO_RB_NODE,
KF_ARG_PTR_TO_NULL,
KF_ARG_PTR_TO_CONST_STR,
+ KF_ARG_PTR_TO_TIMER,
};
enum special_kfunc_type {
@@ -10934,6 +10975,7 @@ enum special_kfunc_type {
KF_bpf_percpu_obj_drop_impl,
KF_bpf_throw,
KF_bpf_iter_css_task_new,
+ KF_bpf_timer_set_sleepable_cb,
};
BTF_SET_START(special_kfunc_set)
@@ -10960,6 +11002,7 @@ BTF_ID(func, bpf_throw)
#ifdef CONFIG_CGROUPS
BTF_ID(func, bpf_iter_css_task_new)
#endif
+BTF_ID(func, bpf_timer_set_sleepable_cb)
BTF_SET_END(special_kfunc_set)
BTF_ID_LIST(special_kfunc_list)
@@ -10990,6 +11033,7 @@ BTF_ID(func, bpf_iter_css_task_new)
#else
BTF_ID_UNUSED
#endif
+BTF_ID(func, bpf_timer_set_sleepable_cb)
static bool is_kfunc_ret_null(struct bpf_kfunc_call_arg_meta *meta)
{
@@ -11061,6 +11105,9 @@ get_kfunc_ptr_arg_type(struct bpf_verifier_env *env,
if (is_kfunc_arg_const_str(meta->btf, &args[argno]))
return KF_ARG_PTR_TO_CONST_STR;
+ if (is_kfunc_arg_timer(meta->btf, &args[argno]))
+ return KF_ARG_PTR_TO_TIMER;
+
if ((base_type(reg->type) == PTR_TO_BTF_ID || reg2btf_ids[base_type(reg->type)])) {
if (!btf_type_is_struct(ref_t)) {
verbose(env, "kernel function %s args#%d pointer type %s %s is not supported\n",
@@ -11318,6 +11365,11 @@ static bool is_bpf_throw_kfunc(struct bpf_insn *insn)
insn->imm == special_kfunc_list[KF_bpf_throw];
}
+static bool is_bpf_timer_set_sleepable_cb_kfunc(u32 btf_id)
+{
+ return btf_id == special_kfunc_list[KF_bpf_timer_set_sleepable_cb];
+}
+
static bool is_rbtree_lock_required_kfunc(u32 btf_id)
{
return is_bpf_rbtree_api_kfunc(btf_id);
@@ -11693,6 +11745,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
case KF_ARG_PTR_TO_CALLBACK:
case KF_ARG_PTR_TO_REFCOUNTED_KPTR:
case KF_ARG_PTR_TO_CONST_STR:
+ case KF_ARG_PTR_TO_TIMER:
/* Trusted by default */
break;
default:
@@ -11973,6 +12026,9 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
if (ret)
return ret;
break;
+ case KF_ARG_PTR_TO_TIMER:
+ /* FIXME: should we do anything here? */
+ break;
}
}
@@ -15591,7 +15647,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 ||
+ is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)))
/* 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
@@ -16767,6 +16825,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
*/
@@ -19644,7 +19705,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 ||
+ is_bpf_timer_set_sleepable_cb_kfunc(insn->imm)) {
/* 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.
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 03/10] bpf/verifier: allow more maps in sleepable bpf programs
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
These 2 maps types are required for HID-BPF when a user wants to do
IO with a device from a sleepable tracing point.
Allowing BPF_MAP_TYPE_QUEUE (and therefore BPF_MAP_TYPE_STACK) allows
for a BPF program to prepare from an IRQ the list of HID commands to send
back to the device and then these commands can be retrieved from the
sleepable trace point.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
changes in v2:
- dropped BPF_MAP_TYPE_PROG_ARRAY from the list
---
kernel/bpf/verifier.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index 67da3f7bddb5..cb1266566b69 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -18094,6 +18094,8 @@ static int check_map_prog_compatibility(struct bpf_verifier_env *env,
case BPF_MAP_TYPE_SK_STORAGE:
case BPF_MAP_TYPE_TASK_STORAGE:
case BPF_MAP_TYPE_CGRP_STORAGE:
+ case BPF_MAP_TYPE_QUEUE:
+ case BPF_MAP_TYPE_STACK:
break;
default:
verbose(env,
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 04/10] HID: bpf/dispatch: regroup kfuncs definitions
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
No code change, just move down the hid_bpf_get_data() kfunc definition
so we have only one block of __bpf_kfunc_start/end_defs()
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
drivers/hid/bpf/hid_bpf_dispatch.c | 80 ++++++++++++++++++--------------------
1 file changed, 38 insertions(+), 42 deletions(-)
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();
-
-/**
- * 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;
-
- ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
-
- if (rdwr_buf_size + offset > ctx->allocated_size)
- return NULL;
-
- return ctx_kern->data + offset;
-}
-__bpf_kfunc_end_defs();
-
-/*
- * The following set contains all functions we agree BPF programs
- * can use.
- */
-BTF_KFUNCS_START(hid_bpf_kfunc_ids)
-BTF_ID_FLAGS(func, hid_bpf_get_data, KF_RET_NULL)
-BTF_KFUNCS_END(hid_bpf_kfunc_ids)
-
-static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
- .owner = THIS_MODULE,
- .set = &hid_bpf_kfunc_ids,
-};
-
static int device_match_id(struct device *dev, const void *id)
{
struct hid_device *hdev = to_hid_device(dev);
@@ -281,6 +239,31 @@ static int do_hid_bpf_attach_prog(struct hid_device *hdev, int prog_fd, struct b
/* Disables missing prototype warnings */
__bpf_kfunc_start_defs();
+/**
+ * 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;
+
+ ctx_kern = container_of(ctx, struct hid_bpf_ctx_kern, ctx);
+
+ if (rdwr_buf_size + offset > ctx->allocated_size)
+ return NULL;
+
+ return ctx_kern->data + offset;
+}
+
/**
* hid_bpf_attach_prog - Attach the given @prog_fd to the given HID device
*
@@ -474,6 +457,19 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
}
__bpf_kfunc_end_defs();
+/*
+ * The following set contains all functions we agree BPF programs
+ * can use.
+ */
+BTF_KFUNCS_START(hid_bpf_kfunc_ids)
+BTF_ID_FLAGS(func, hid_bpf_get_data, KF_RET_NULL)
+BTF_KFUNCS_END(hid_bpf_kfunc_ids)
+
+static const struct btf_kfunc_id_set hid_bpf_kfunc_set = {
+ .owner = THIS_MODULE,
+ .set = &hid_bpf_kfunc_ids,
+};
+
/* our HID-BPF entrypoints */
BTF_SET8_START(hid_bpf_fmodret_ids)
BTF_ID_FLAGS(func, hid_bpf_device_event)
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 05/10] HID: bpf: export hid_hw_output_report as a BPF kfunc
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
We currently only export hid_hw_raw_request() as a BPF kfunc.
However, some devices require an explicit write on the Output Report
instead of the use of the control channel.
So also export hid_hw_output_report to BPF
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
Documentation/hid/hid-bpf.rst | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 112 +++++++++++++++++++++++++++----------
drivers/hid/hid-core.c | 1 +
include/linux/hid_bpf.h | 1 +
4 files changed, 86 insertions(+), 30 deletions(-)
diff --git a/Documentation/hid/hid-bpf.rst b/Documentation/hid/hid-bpf.rst
index 4fad83a6ebc3..a575004d9025 100644
--- a/Documentation/hid/hid-bpf.rst
+++ b/Documentation/hid/hid-bpf.rst
@@ -179,7 +179,7 @@ Available API that can be used in syscall HID-BPF programs:
-----------------------------------------------------------
.. kernel-doc:: drivers/hid/bpf/hid_bpf_dispatch.c
- :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_allocate_context hid_bpf_release_context
+ :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_allocate_context hid_bpf_release_context
General overview of a HID-BPF program
=====================================
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index 52abb27426f4..a5b88b491b80 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -376,6 +376,46 @@ hid_bpf_release_context(struct hid_bpf_ctx *ctx)
put_device(&hid->dev);
}
+static int
+__hid_bpf_hw_check_params(struct hid_bpf_ctx *ctx, __u8 *buf, size_t *buf__sz,
+ enum hid_report_type rtype)
+{
+ struct hid_report_enum *report_enum;
+ struct hid_report *report;
+ struct hid_device *hdev;
+ u32 report_len;
+
+ /* check arguments */
+ if (!ctx || !hid_bpf_ops || !buf)
+ return -EINVAL;
+
+ switch (rtype) {
+ case HID_INPUT_REPORT:
+ case HID_OUTPUT_REPORT:
+ case HID_FEATURE_REPORT:
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (*buf__sz < 1)
+ return -EINVAL;
+
+ hdev = (struct hid_device *)ctx->hid; /* discard const */
+
+ report_enum = hdev->report_enum + rtype;
+ report = hid_bpf_ops->hid_get_report(report_enum, buf);
+ if (!report)
+ return -EINVAL;
+
+ report_len = hid_report_len(report);
+
+ if (*buf__sz > report_len)
+ *buf__sz = report_len;
+
+ return 0;
+}
+
/**
* hid_bpf_hw_request - Communicate with a HID device
*
@@ -392,24 +432,14 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
enum hid_report_type rtype, enum hid_class_request reqtype)
{
struct hid_device *hdev;
- struct hid_report *report;
- struct hid_report_enum *report_enum;
+ size_t size = buf__sz;
u8 *dma_data;
- u32 report_len;
int ret;
/* check arguments */
- if (!ctx || !hid_bpf_ops || !buf)
- return -EINVAL;
-
- switch (rtype) {
- case HID_INPUT_REPORT:
- case HID_OUTPUT_REPORT:
- case HID_FEATURE_REPORT:
- break;
- default:
- return -EINVAL;
- }
+ ret = __hid_bpf_hw_check_params(ctx, buf, &size, rtype);
+ if (ret)
+ return ret;
switch (reqtype) {
case HID_REQ_GET_REPORT:
@@ -423,29 +453,16 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
return -EINVAL;
}
- if (buf__sz < 1)
- return -EINVAL;
-
hdev = (struct hid_device *)ctx->hid; /* discard const */
- report_enum = hdev->report_enum + rtype;
- report = hid_bpf_ops->hid_get_report(report_enum, buf);
- if (!report)
- return -EINVAL;
-
- report_len = hid_report_len(report);
-
- if (buf__sz > report_len)
- buf__sz = report_len;
-
- dma_data = kmemdup(buf, buf__sz, GFP_KERNEL);
+ dma_data = kmemdup(buf, size, GFP_KERNEL);
if (!dma_data)
return -ENOMEM;
ret = hid_bpf_ops->hid_hw_raw_request(hdev,
dma_data[0],
dma_data,
- buf__sz,
+ size,
rtype,
reqtype);
@@ -455,6 +472,42 @@ hid_bpf_hw_request(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz,
kfree(dma_data);
return ret;
}
+
+/**
+ * hid_bpf_hw_output_report - Send an output report to a HID device
+ *
+ * @ctx: the HID-BPF context previously allocated in hid_bpf_allocate_context()
+ * @buf: a %PTR_TO_MEM buffer
+ * @buf__sz: the size of the data to transfer
+ *
+ * @returns the number of bytes transferred on success, a negative error code otherwise.
+ */
+__bpf_kfunc int
+hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
+{
+ struct hid_device *hdev;
+ size_t size = buf__sz;
+ u8 *dma_data;
+ int ret;
+
+ /* check arguments */
+ ret = __hid_bpf_hw_check_params(ctx, buf, &size, HID_OUTPUT_REPORT);
+ if (ret)
+ return ret;
+
+ hdev = (struct hid_device *)ctx->hid; /* discard const */
+
+ dma_data = kmemdup(buf, size, GFP_KERNEL);
+ if (!dma_data)
+ return -ENOMEM;
+
+ ret = hid_bpf_ops->hid_hw_output_report(hdev,
+ dma_data,
+ size);
+
+ kfree(dma_data);
+ return ret;
+}
__bpf_kfunc_end_defs();
/*
@@ -488,6 +541,7 @@ BTF_ID_FLAGS(func, hid_bpf_attach_prog)
BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE)
BTF_ID_FLAGS(func, hid_bpf_hw_request)
+BTF_ID_FLAGS(func, hid_bpf_hw_output_report)
BTF_KFUNCS_END(hid_bpf_syscall_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index de7a477d6665..1243595890ba 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2974,6 +2974,7 @@ EXPORT_SYMBOL_GPL(hid_check_keys_pressed);
static struct hid_bpf_ops hid_ops = {
.hid_get_report = hid_get_report,
.hid_hw_raw_request = hid_hw_raw_request,
+ .hid_hw_output_report = hid_hw_output_report,
.owner = THIS_MODULE,
.bus_type = &hid_bus_type,
};
diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
index 7118ac28d468..5c7ff93dc73e 100644
--- a/include/linux/hid_bpf.h
+++ b/include/linux/hid_bpf.h
@@ -103,6 +103,7 @@ struct hid_bpf_ops {
unsigned char reportnum, __u8 *buf,
size_t len, enum hid_report_type rtype,
enum hid_class_request reqtype);
+ int (*hid_hw_output_report)(struct hid_device *hdev, __u8 *buf, size_t len);
struct module *owner;
const struct bus_type *bus_type;
};
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 06/10] selftests/hid: Add test for hid_bpf_hw_output_report
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
This time we need to ensure uhid receives it, thus the new mutex and
condition.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
tools/testing/selftests/hid/hid_bpf.c | 63 ++++++++++++++++++++++
tools/testing/selftests/hid/progs/hid.c | 24 +++++++++
.../testing/selftests/hid/progs/hid_bpf_helpers.h | 2 +
3 files changed, 89 insertions(+)
diff --git a/tools/testing/selftests/hid/hid_bpf.c b/tools/testing/selftests/hid/hid_bpf.c
index 2cf96f818f25..8332014838b0 100644
--- a/tools/testing/selftests/hid/hid_bpf.c
+++ b/tools/testing/selftests/hid/hid_bpf.c
@@ -16,6 +16,11 @@
#define SHOW_UHID_DEBUG 0
+#define min(a, b) \
+ ({ __typeof__(a) _a = (a); \
+ __typeof__(b) _b = (b); \
+ _a < _b ? _a : _b; })
+
static unsigned char rdesc[] = {
0x06, 0x00, 0xff, /* Usage Page (Vendor Defined Page 1) */
0x09, 0x21, /* Usage (Vendor Usage 0x21) */
@@ -111,6 +116,10 @@ struct hid_hw_request_syscall_args {
static pthread_mutex_t uhid_started_mtx = PTHREAD_MUTEX_INITIALIZER;
static pthread_cond_t uhid_started = PTHREAD_COND_INITIALIZER;
+static pthread_mutex_t uhid_output_mtx = PTHREAD_MUTEX_INITIALIZER;
+static pthread_cond_t uhid_output_cond = PTHREAD_COND_INITIALIZER;
+static unsigned char output_report[10];
+
/* no need to protect uhid_stopped, only one thread accesses it */
static bool uhid_stopped;
@@ -205,6 +214,13 @@ static int uhid_event(struct __test_metadata *_metadata, int fd)
break;
case UHID_OUTPUT:
UHID_LOG("UHID_OUTPUT from uhid-dev");
+
+ pthread_mutex_lock(&uhid_output_mtx);
+ memcpy(output_report,
+ ev.u.output.data,
+ min(ev.u.output.size, sizeof(output_report)));
+ pthread_cond_signal(&uhid_output_cond);
+ pthread_mutex_unlock(&uhid_output_mtx);
break;
case UHID_GET_REPORT:
UHID_LOG("UHID_GET_REPORT from uhid-dev");
@@ -733,6 +749,53 @@ TEST_F(hid_bpf, test_hid_change_report)
ASSERT_EQ(buf[2], 0) TH_LOG("leftovers_from_previous_test");
}
+/*
+ * Call hid_bpf_hw_output_report against the given uhid device,
+ * check that the program is called and does the expected.
+ */
+TEST_F(hid_bpf, test_hid_user_output_report_call)
+{
+ struct hid_hw_request_syscall_args args = {
+ .retval = -1,
+ .size = 10,
+ };
+ DECLARE_LIBBPF_OPTS(bpf_test_run_opts, tattrs,
+ .ctx_in = &args,
+ .ctx_size_in = sizeof(args),
+ );
+ int err, cond_err, prog_fd;
+ struct timespec time_to_wait;
+
+ LOAD_BPF;
+
+ args.hid = self->hid_id;
+ args.data[0] = 1; /* report ID */
+ args.data[1] = 2; /* report ID */
+ args.data[2] = 42; /* report ID */
+
+ prog_fd = bpf_program__fd(self->skel->progs.hid_user_output_report);
+
+ pthread_mutex_lock(&uhid_output_mtx);
+
+ memset(output_report, 0, sizeof(output_report));
+ clock_gettime(CLOCK_REALTIME, &time_to_wait);
+ time_to_wait.tv_sec += 2;
+
+ err = bpf_prog_test_run_opts(prog_fd, &tattrs);
+ cond_err = pthread_cond_timedwait(&uhid_output_cond, &uhid_output_mtx, &time_to_wait);
+
+ ASSERT_OK(err) TH_LOG("error while calling bpf_prog_test_run_opts");
+ ASSERT_OK(cond_err) TH_LOG("error while calling waiting for the condition");
+
+ ASSERT_EQ(args.retval, 3);
+
+ ASSERT_EQ(output_report[0], 1);
+ ASSERT_EQ(output_report[1], 2);
+ ASSERT_EQ(output_report[2], 42);
+
+ pthread_mutex_unlock(&uhid_output_mtx);
+}
+
/*
* Attach hid_user_raw_request to the given uhid device,
* call the bpf program from userspace
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 1e558826b809..2c2b679a83b1 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -101,6 +101,30 @@ int hid_user_raw_request(struct hid_hw_request_syscall_args *args)
return 0;
}
+SEC("syscall")
+int hid_user_output_report(struct hid_hw_request_syscall_args *args)
+{
+ struct hid_bpf_ctx *ctx;
+ const size_t size = args->size;
+ int i, ret = 0;
+
+ if (size > sizeof(args->data))
+ return -7; /* -E2BIG */
+
+ ctx = hid_bpf_allocate_context(args->hid);
+ if (!ctx)
+ return -1; /* EPERM check */
+
+ ret = hid_bpf_hw_output_report(ctx,
+ args->data,
+ size);
+ args->retval = ret;
+
+ hid_bpf_release_context(ctx);
+
+ return 0;
+}
+
static const __u8 rdesc[] = {
0x05, 0x01, /* USAGE_PAGE (Generic Desktop) */
0x09, 0x32, /* USAGE (Z) */
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 65e657ac1198..50c6a0d5765e 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -94,5 +94,7 @@ extern int hid_bpf_hw_request(struct hid_bpf_ctx *ctx,
size_t buf__sz,
enum hid_report_type type,
enum hid_class_request reqtype) __ksym;
+extern int hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx,
+ __u8 *buf, size_t buf__sz) __ksym;
#endif /* __HID_BPF_HELPERS_H */
--
2.43.0
^ permalink raw reply related
* [PATCH RFC bpf-next v2 07/10] HID: bpf: allow to inject HID event from BPF
From: Benjamin Tissoires @ 2024-02-14 17:18 UTC (permalink / raw)
To: 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: <20240214-hid-bpf-sleepable-v2-0-5756b054724d@kernel.org>
It can be interesting to inject events from BPF as if the event were
to come from the device.
For example, some multitouch devices do not all the time send a proximity
out event, and we might want to send it for the physical device.
Compared to uhid, we can now inject events on any physical device, not
just uhid virtual ones.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v2
---
Documentation/hid/hid-bpf.rst | 2 +-
drivers/hid/bpf/hid_bpf_dispatch.c | 29 +++++++++++++++++++++++++++++
drivers/hid/hid-core.c | 1 +
include/linux/hid_bpf.h | 2 ++
4 files changed, 33 insertions(+), 1 deletion(-)
diff --git a/Documentation/hid/hid-bpf.rst b/Documentation/hid/hid-bpf.rst
index a575004d9025..0765b3298ecf 100644
--- a/Documentation/hid/hid-bpf.rst
+++ b/Documentation/hid/hid-bpf.rst
@@ -179,7 +179,7 @@ Available API that can be used in syscall HID-BPF programs:
-----------------------------------------------------------
.. kernel-doc:: drivers/hid/bpf/hid_bpf_dispatch.c
- :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_allocate_context hid_bpf_release_context
+ :functions: hid_bpf_attach_prog hid_bpf_hw_request hid_bpf_hw_output_report hid_bpf_input_report hid_bpf_allocate_context hid_bpf_release_context
General overview of a HID-BPF program
=====================================
diff --git a/drivers/hid/bpf/hid_bpf_dispatch.c b/drivers/hid/bpf/hid_bpf_dispatch.c
index a5b88b491b80..e1a650f4a626 100644
--- a/drivers/hid/bpf/hid_bpf_dispatch.c
+++ b/drivers/hid/bpf/hid_bpf_dispatch.c
@@ -508,6 +508,34 @@ hid_bpf_hw_output_report(struct hid_bpf_ctx *ctx, __u8 *buf, size_t buf__sz)
kfree(dma_data);
return ret;
}
+
+/**
+ * hid_bpf_input_report - Inject a HID report in the kernel from a HID device
+ *
+ * @ctx: the HID-BPF context previously allocated in hid_bpf_allocate_context()
+ * @type: the type of the report (%HID_INPUT_REPORT, %HID_FEATURE_REPORT, %HID_OUTPUT_REPORT)
+ * @buf: a %PTR_TO_MEM buffer
+ * @buf__sz: the size of the data to transfer
+ *
+ * @returns %0 on success, a negative error code otherwise.
+ */
+__bpf_kfunc int
+hid_bpf_input_report(struct hid_bpf_ctx *ctx, enum hid_report_type type, u8 *buf,
+ const size_t buf__sz)
+{
+ struct hid_device *hdev;
+ size_t size = buf__sz;
+ int ret;
+
+ /* check arguments */
+ ret = __hid_bpf_hw_check_params(ctx, buf, &size, type);
+ if (ret)
+ return ret;
+
+ hdev = (struct hid_device *)ctx->hid; /* discard const */
+
+ return hid_input_report(hdev, type, buf, size, 0);
+}
__bpf_kfunc_end_defs();
/*
@@ -542,6 +570,7 @@ BTF_ID_FLAGS(func, hid_bpf_allocate_context, KF_ACQUIRE | KF_RET_NULL)
BTF_ID_FLAGS(func, hid_bpf_release_context, KF_RELEASE)
BTF_ID_FLAGS(func, hid_bpf_hw_request)
BTF_ID_FLAGS(func, hid_bpf_hw_output_report)
+BTF_ID_FLAGS(func, hid_bpf_input_report)
BTF_KFUNCS_END(hid_bpf_syscall_kfunc_ids)
static const struct btf_kfunc_id_set hid_bpf_syscall_kfunc_set = {
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 1243595890ba..b1fa0378e8f4 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2975,6 +2975,7 @@ static struct hid_bpf_ops hid_ops = {
.hid_get_report = hid_get_report,
.hid_hw_raw_request = hid_hw_raw_request,
.hid_hw_output_report = hid_hw_output_report,
+ .hid_input_report = hid_input_report,
.owner = THIS_MODULE,
.bus_type = &hid_bus_type,
};
diff --git a/include/linux/hid_bpf.h b/include/linux/hid_bpf.h
index 5c7ff93dc73e..17b08f500098 100644
--- a/include/linux/hid_bpf.h
+++ b/include/linux/hid_bpf.h
@@ -104,6 +104,8 @@ struct hid_bpf_ops {
size_t len, enum hid_report_type rtype,
enum hid_class_request reqtype);
int (*hid_hw_output_report)(struct hid_device *hdev, __u8 *buf, size_t len);
+ int (*hid_input_report)(struct hid_device *hid, enum hid_report_type type,
+ u8 *data, u32 size, int interrupt);
struct module *owner;
const struct bus_type *bus_type;
};
--
2.43.0
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox