* Re: [PATCH v3 16/32] ARM: dts: qcom: mdm9615: split PMIC to separate dtsi files
From: Dmitry Baryshkov @ 2023-08-26 14:03 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
linux-arm-msm, devicetree, linux-input, Neil Armstrong
In-Reply-To: <aff8e42f-4861-4953-966a-c6ac735404dd@linaro.org>
On Sat, 26 Aug 2023 at 16:48, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 26.08.2023 15:43, Dmitry Baryshkov wrote:
> > On Sat, 26 Aug 2023 at 15:08, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
> >>
> >> On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> >>> The PMIC is not a part of the SoC, so move PMIC to a separate file and
> >>> include it from the board files.
> >>>
> >>> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> >>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> >>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >>> ---
> >> [...]
> >>
> >>> + pmic {
> >> Are you leaving an empty subnode here?
> >
> > No. It contains 'interrupts' property (which is specific to the SoC).
> Meh, that's bad design.. should have been in the controller node!
It will not be logical either. The interrupt line comes from the PMIC.
Wait. Maybe we should do it other way around: move IRQ to the _board_
file, since it is just a GPIO line. Then we don't have to leave this
band-aid in place.
> But noboyd thought about this 10y+ ago so here we are
>
> Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>
> Konrad
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v3 17/32] ARM: dts: qcom: msm8660: split PMIC to separate dtsi files
From: Konrad Dybcio @ 2023-08-26 13:49 UTC (permalink / raw)
To: Dmitry Baryshkov, Andy Gross, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input
In-Reply-To: <20230822001349.899298-18-dmitry.baryshkov@linaro.org>
On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> The PMIC is not a part of the SoC, so move PMIC to a separate file and
> include it from the board files.
>
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
^ permalink raw reply
* Re: [PATCH v3 16/32] ARM: dts: qcom: mdm9615: split PMIC to separate dtsi files
From: Konrad Dybcio @ 2023-08-26 13:48 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
linux-arm-msm, devicetree, linux-input, Neil Armstrong
In-Reply-To: <CAA8EJpoB6JYrFPZ7PMrVYvuwxgu6SH1zuPWG3q8Xy1J2YcCPcA@mail.gmail.com>
On 26.08.2023 15:43, Dmitry Baryshkov wrote:
> On Sat, 26 Aug 2023 at 15:08, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>>
>> On 22.08.2023 02:13, Dmitry Baryshkov wrote:
>>> The PMIC is not a part of the SoC, so move PMIC to a separate file and
>>> include it from the board files.
>>>
>>> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
>>> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>> ---
>> [...]
>>
>>> + pmic {
>> Are you leaving an empty subnode here?
>
> No. It contains 'interrupts' property (which is specific to the SoC).
Meh, that's bad design.. should have been in the controller node!
But noboyd thought about this 10y+ ago so here we are
Acked-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
^ permalink raw reply
* Re: [PATCH v3 16/32] ARM: dts: qcom: mdm9615: split PMIC to separate dtsi files
From: Dmitry Baryshkov @ 2023-08-26 13:43 UTC (permalink / raw)
To: Konrad Dybcio
Cc: Andy Gross, Bjorn Andersson, Rob Herring, Krzysztof Kozlowski,
linux-arm-msm, devicetree, linux-input, Neil Armstrong
In-Reply-To: <2dea943a-7a9e-4963-8ae5-6b126c750f80@linaro.org>
On Sat, 26 Aug 2023 at 15:08, Konrad Dybcio <konrad.dybcio@linaro.org> wrote:
>
> On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> > The PMIC is not a part of the SoC, so move PMIC to a separate file and
> > include it from the board files.
> >
> > Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> > Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> [...]
>
> > + pmic {
> Are you leaving an empty subnode here?
No. It contains 'interrupts' property (which is specific to the SoC).
--
With best wishes
Dmitry
^ permalink raw reply
* Re: [PATCH v3 17/32] ARM: dts: qcom: msm8660: split PMIC to separate dtsi files
From: Konrad Dybcio @ 2023-08-26 12:08 UTC (permalink / raw)
To: Dmitry Baryshkov, Andy Gross, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input
In-Reply-To: <20230822001349.899298-18-dmitry.baryshkov@linaro.org>
On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> The PMIC is not a part of the SoC, so move PMIC to a separate file and
> include it from the board files.
>
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
[...]
> + pmic {
empty sub?
Konrad
^ permalink raw reply
* Re: [PATCH v3 16/32] ARM: dts: qcom: mdm9615: split PMIC to separate dtsi files
From: Konrad Dybcio @ 2023-08-26 12:08 UTC (permalink / raw)
To: Dmitry Baryshkov, Andy Gross, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input, Neil Armstrong
In-Reply-To: <20230822001349.899298-17-dmitry.baryshkov@linaro.org>
On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> The PMIC is not a part of the SoC, so move PMIC to a separate file and
> include it from the board files.
>
> Suggested-by: Konrad Dybcio <konrad.dybcio@linaro.org>
> Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
[...]
> + pmic {
Are you leaving an empty subnode here?
Konrad
^ permalink raw reply
* Re: [PATCH v3 19/32] ARM: dts: qcom: pm8921: reorder nodes
From: Konrad Dybcio @ 2023-08-26 10:28 UTC (permalink / raw)
To: Dmitry Baryshkov, Andy Gross, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input
In-Reply-To: <20230822001349.899298-20-dmitry.baryshkov@linaro.org>
On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> Move pm8921 device nodes to follow the alphanumberic sorting order.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
^ permalink raw reply
* Re: [PATCH v3 31/32] ARM: dts: qcom: pm8921: Disable keypad by default
From: Konrad Dybcio @ 2023-08-26 10:27 UTC (permalink / raw)
To: Dmitry Baryshkov, Andy Gross, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input
In-Reply-To: <20230822001349.899298-32-dmitry.baryshkov@linaro.org>
On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> Since keypad is used only by some devices, disable it by default and enable explicitly.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
^ permalink raw reply
* Re: [PATCH v3 32/32] ARM: dts: qcom: apq8060-dragonboard: rename mpp ADC channels to adc-channel
From: Konrad Dybcio @ 2023-08-26 10:25 UTC (permalink / raw)
To: Dmitry Baryshkov, Andy Gross, Bjorn Andersson, Rob Herring,
Krzysztof Kozlowski
Cc: linux-arm-msm, devicetree, linux-input
In-Reply-To: <20230822001349.899298-33-dmitry.baryshkov@linaro.org>
On 22.08.2023 02:13, Dmitry Baryshkov wrote:
> Use generic `adc-channel@N' node names for board-specific ADC channels
> (routed to MPP pins) to follow the schema.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org>
Konrad
^ permalink raw reply
* Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
From: Thomas Weißschuh @ 2023-08-26 10:15 UTC (permalink / raw)
To: Julius Zint
Cc: Lee Jones, Daniel Thompson, Jingoo Han, Jiri Kosina,
Benjamin Tissoires, Helge Deller, linux-kernel, dri-devel,
linux-input, linux-fbdev
In-Reply-To: <20230820094118.20521-2-julius@zint.sh>
On 2023-08-20 11:41:18+0200, Julius Zint wrote:
> [..]
> diff --git a/drivers/video/backlight/Kconfig b/drivers/video/backlight/Kconfig
> index 51387b1ef012..b964a820956d 100644
> --- a/drivers/video/backlight/Kconfig
> +++ b/drivers/video/backlight/Kconfig
> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
> If you have a LCD backlight adjustable by LED class driver, say Y
> to enable this driver.
>
> +config BACKLIGHT_HID
> + tristate "VESA VCP HID Backlight Driver"
> + depends on HID
> + help
> + If you have an external display with VESA compliant HID brightness
> + controls then say Y to enable this backlight driver. Currently the
> + only supported device is the Apple Studio Display.
Is the last sentence needed?
It will go out of date soon, requiring updates to the Kconfig.
> +
> endif # BACKLIGHT_CLASS_DEVICE
>
> endmenu
> [..]
> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
> new file mode 100644
> index 000000000000..b40f8f412ee2
> --- /dev/null
> +++ b/drivers/video/backlight/hid_bl.c
> @@ -0,0 +1,269 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <linux/device.h>
> +#include <linux/hid.h>
> +#include <linux/module.h>
> +#include <linux/backlight.h>
> +
> +#define APPLE_STUDIO_DISPLAY_VENDOR_ID 0x05ac
> +#define APPLE_STUDIO_DISPLAY_PRODUCT_ID 0x1114
Use hid-ids.h. The vendor ID already has an entry.
> +
> +#define HID_USAGE_MONITOR_CTRL 0x800001
> +#define HID_USAGE_VESA_VCP_BRIGHTNESS 0x820010
> [..]
> +static int hid_bl_probe(struct hid_device *hdev, const struct hid_device_id *id)
> +{
> [..]
> +
> + memset(&props, 0, sizeof(props));
> + props.type = BACKLIGHT_RAW;
Wouldn't this be more a BACKLIGHT_FIRMWARE?
> + props.max_brightness = data->max_brightness - data->min_brightness;
> +
> + bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",
It's non-obvious that the "vesa_vcp" backlight comes from the
"hid_backlight" driver. Maybe align the names.
What happens when multiple compatible devices are used?
That seems to be possible with external monitors.
Can existing userspace figure out which display the backlight device
belongs to?
(I don't know either)
> + &hdev->dev, data,
> + &hid_bl_ops,
> + &props);
> [..]
^ permalink raw reply
* Re: [PATCH 4/3] selftests/hid: more fixes to build with older kernel
From: Eduard Zingerman @ 2023-08-25 18:53 UTC (permalink / raw)
To: Justin Stitt, Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Nick Desaulniers,
linux-input, linux-kselftest, linux-kernel, bpf
In-Reply-To: <20230825182316.m2ksjoxe4s7dsapn@google.com>
On Fri, 2023-08-25 at 18:23 +0000, Justin Stitt wrote:
> On Fri, Aug 25, 2023 at 10:36:30AM +0200, Benjamin Tissoires wrote:
> > These fixes have been triggered by [0]:
> > basically, if you do not recompile the kernel first, and are
> > running on an old kernel, vmlinux.h doesn't have the required
> > symbols and the compilation fails.
> >
> > The tests will fail if you run them on that very same machine,
> > of course, but the binary should compile.
> >
> > And while I was sorting out why it was failing, I realized I
> > could do a couple of improvements on the Makefile.
> >
> > [0] https://lore.kernel.org/linux-input/56ba8125-2c6f-a9c9-d498-0ca1c153dcb2@redhat.com/T/#t
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> > Benjamin Tissoires (3):
> > selftests/hid: ensure we can compile the tests on kernels pre-6.3
> > selftests/hid: do not manually call headers_install
> > selftests/hid: force using our compiled libbpf headers
> >
> > tools/testing/selftests/hid/Makefile | 10 ++++------
> > tools/testing/selftests/hid/progs/hid.c | 3 ---
> > tools/testing/selftests/hid/progs/hid_bpf_helpers.h | 20 ++++++++++++++++++++
> > 3 files changed, 24 insertions(+), 9 deletions(-)
> > ---
> > base-commit: 1d7546042f8fdc4bc39ab91ec966203e2d64f8bd
> > change-id: 20230825-wip-selftests-9a7502b56542
> >
> > Best regards,
> > --
> > Benjamin Tissoires <bentiss@kernel.org>
> >
>
> Benjamin, thanks for the work here. Your series fixed up _some_ of the
> errors I had while building on my 6.3.11 kernel. I'm proposing a single
> patch that should be applied on top of your series that fully fixes
> _all_ of the build errors I'm experiencing.
>
> Can you let me know if it works and potentially formulate a new series
> so that `b4 shazam` applies it cleanly?
>
> PATCH BELOW
> ---
> From 5378d70e1b3f7f75656332f9bff65a37122bb288 Mon Sep 17 00:00:00 2001
> From: Justin Stitt <justinstitt@google.com>
> Date: Fri, 25 Aug 2023 18:10:33 +0000
> Subject: [PATCH 4/3] selftests/hid: more fixes to build with older kernel
>
> I had to use the clever trick [1] on some other symbols to get my builds
> working.
>
> Apply this patch on top of Benjamin's series [2].
>
> This is now a n=4 patch series which has fixed my builds when running:
> > $ make LLVM=1 -j128 ARCH=x86_64 mrproper headers
> > $ make LLVM=1 -j128 ARCH=x86_64 -C tools/testing/selftests TARGETS=hid
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/bpf/progs/bpf_iter.h#n3
> [2]: https://lore.kernel.org/all/20230825-wip-selftests-v1-0-c862769020a8@kernel.org/
> Signed-off-by: Justin Stitt <justinstitt@google.com>
> ---
> .../selftests/hid/progs/hid_bpf_helpers.h | 29 +++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> index 749097f8f4d9..e2eace2c0029 100644
> --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> @@ -7,12 +7,26 @@
>
> /* "undefine" structs in vmlinux.h, because we "override" them below */
> #define hid_bpf_ctx hid_bpf_ctx___not_used
> +#define hid_report_type hid_report_type___not_used
> +#define hid_class_request hid_class_request___not_used
> +#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
> #include "vmlinux.h"
> #undef hid_bpf_ctx
> +#undef hid_report_type
> +#undef hid_class_request
> +#undef hid_bpf_attach_flags
>
> #include <bpf/bpf_helpers.h>
> #include <bpf/bpf_tracing.h>
> +#include <linux/const.h>
>
> +enum hid_report_type {
> + HID_INPUT_REPORT = 0,
> + HID_OUTPUT_REPORT = 1,
> + HID_FEATURE_REPORT = 2,
> +
> + HID_REPORT_TYPES,
> +};
>
> struct hid_bpf_ctx {
> __u32 index;
> @@ -25,6 +39,21 @@ struct hid_bpf_ctx {
> };
> };
Note, vmlinux.h has the following preamble/postamble:
#ifndef BPF_NO_PRESERVE_ACCESS_INDEX
#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)
#endif
...
#ifndef BPF_NO_PRESERVE_ACCESS_INDEX
#pragma clang attribute pop
#endif
You might want to use it or add __attribute__((preserve_access_index))
to structure definitions, depending on whether or not you need CO-RE
functionality for these tests.
>
> +enum hid_class_request {
> + HID_REQ_GET_REPORT = 0x01,
> + HID_REQ_GET_IDLE = 0x02,
> + HID_REQ_GET_PROTOCOL = 0x03,
> + HID_REQ_SET_REPORT = 0x09,
> + HID_REQ_SET_IDLE = 0x0A,
> + HID_REQ_SET_PROTOCOL = 0x0B,
> +};
> +
> +enum hid_bpf_attach_flags {
> + HID_BPF_FLAG_NONE = 0,
> + HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
> + HID_BPF_FLAG_MAX,
> +};
> +
> /* following are kfuncs exported by HID for HID-BPF */
> extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> unsigned int offset,
> --
> 2.42.0.rc1.204.g551eb34607-goog
>
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Eduard Zingerman @ 2023-08-25 18:46 UTC (permalink / raw)
To: Justin Stitt
Cc: Benjamin Tissoires, Nick Desaulniers, linux-kselftest, bpf,
linux-input, Linux Kernel Mailing List, Nathan Chancellor,
Kees Cook
In-Reply-To: <CAFhGd8ob_qet6ODduHz2=sjGXkHaFMzrtu1FFkN0eUWQvpyPrQ@mail.gmail.com>
On Fri, 2023-08-25 at 11:36 -0700, Justin Stitt wrote:
> On Fri, Aug 25, 2023 at 6:01 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
> >
> > On Fri, 2023-08-25 at 10:08 +0200, Benjamin Tissoires wrote:
> > >
> > > On Tue, Aug 22, 2023 at 11:42 PM Justin Stitt <justinstitt@google.com> wrote:
> > > > > > > > Which kernel are you trying to test?
> > > > > > > > I tested your 2 commands on v6.5-rc7 and it just works.
> > > > > > >
> > > > > > > I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
> > > > > > >
> > > > > > > I ran these exact commands:
> > > > > > > > $ make mrproper
> > > > > > > > $ make LLVM=1 ARCH=x86_64 headers
> > > > > > > > $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> > > > > > > TARGETS=hid &> out
> > > > > > >
> > > > > > > and here's the contents of `out` (still warnings/errors):
> > > > > > > https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
> > > > > > >
> > > > > > > I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
> > > > > >
> > > > > > Sigh... there is a high chance my Makefile is not correct and uses the
> > > > > > installed headers (I was running the exact same commands, but on a
> > > > > > v6.4-rc7+ kernel).
> > > > > >
> > > > > > But sorry, it will have to wait for tomorrow if you want me to have a
> > > > > > look at it. It's 11:35 PM here, and I need to go to bed
> > > > > Take it easy. Thanks for the prompt responses here! I'd like to get
> > > > > the entire kselftest make target building with Clang so that we can
> > > > > close [1].
> > >
> > > Sorry I got urgent matters to tackle yesterday.
> > >
> > > It took me a while to understand what was going on, and I finally found
> > > it.
> > >
> > > struct hid_bpf_ctx is internal to the kernel, and so is declared in
> > > vmlinux.h, that we generate through BTF. But to generate the vmlinux.h
> > > with the correct symbols, these need to be present in the running
> > > kernel.
> > > And that's where we had a fundamental difference: I was running my
> > > compilations on a kernel v6.3+ (6.4.11) with that symbol available, and
> > > you are probably not.
> > >
> > > The bpf folks are using a clever trick to force the compilation[2]. And
> > > I think the following patch would work for you:
> >
> > Hi Benjamin, Justin,
> >
> > You might want to take a look at these two links:
> > [1] https://nakryiko.com/posts/bpf-core-reference-guide/#handling-incompatible-field-and-type-changes
> > [2] https://facebookmicrosites.github.io/bpf/blog/2020/02/19/bpf-portability-and-co-re.html#dealing-with-kernel-version-and-configuration-differences
> >
> > The short version is: CO-RE relocation handling logic in libbpf
> > ignores suffixes of form '___something' for type and field names.
> >
> > So, the following should accomplish the same as the trick with
> > #define/#undef:
> >
> > #include "vmlinux.h"
> > ...
> > struct hid_bpf_ctx___local {
> > __u32 index;
> > const struct hid_device *hid;
> > __u32 allocated_size;
> > enum hid_report_type report_type;
> > union {
> > __s32 retval;
> > __s32 size;
> > };
> >
> > };
> > ...
> > extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
> > unsigned int offset, ...)
> >
> > However, if the kernel does not have `hid_bpf_ctx` definition would
> > the test `progs/hid.c` still make sense?
> >
> > When I tried to build hid tests locally I run into similar errors:
> >
> > ...
> > CLNG-BPF hid.bpf.o
> > In file included from progs/hid.c:6:
> > progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct hid_bpf_ctx' \
> > will not be visible outside of this function [-Werror,-Wvisibility]
> > extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> > ...
> >
> > And there is indeed no `hid_bpf_ctx` in my vmlinux.h.
> > However, after enabling CONFIG_HID_BPF in kernel config the
> > `hid_bpf_ctx` appears in vmlinux.h, and I can compile HID selftests
> > w/o issues.
>
> Even with enabling this configuration option I was unable to get clean
> builds of the HID selftests. I proposed a 4th patch on top of
> Benjamin's n=3 patch series here [1] using the #def/#undef pattern.
What are the remaining errors?
Could you please share your .config (e.g. as a gist).
As I said, when your kernel does not have `struct hid_bpf_ctx`,
sure you can define these data structures in the test itself,
but the test would loose it's meaning. If kernel is built
w/o HID BPF support there is no sense in compiling this test.
>
> >
> > >
> > > ---
> > > From bb9eccb7a896ba4b3a35ed12a248e6d6cfed2df6 Mon Sep 17 00:00:00 2001
> > > From: Benjamin Tissoires <bentiss@kernel.org>
> > > Date: Fri, 25 Aug 2023 10:02:32 +0200
> > > Subject: [PATCH] selftests/hid: ensure we can compile the tests on kernels
> > > pre-6.3
> > >
> > > For the hid-bpf tests to compile, we need to have the definition of
> > > struct hid_bpf_ctx. This definition is an internal one from the kernel
> > > and it is supposed to be defined in the generated vmlinux.h.
> > >
> > > This vmlinux.h header is generated based on the currently running kernel
> > > or if the kernel was already compiled in the tree. If you just compile
> > > the selftests without compiling the kernel beforehand and you are running
> > > on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> > > definition.
> > >
> > > Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> > > to force the definition of that symbol in case we don't find it in the
> > > BTF.
> > >
> > > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > > ---
> > > tools/testing/selftests/hid/progs/hid.c | 3 ---
> > > .../selftests/hid/progs/hid_bpf_helpers.h | 20 +++++++++++++++++++
> > > 2 files changed, 20 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> > > index 88c593f753b5..1e558826b809 100644
> > > --- a/tools/testing/selftests/hid/progs/hid.c
> > > +++ b/tools/testing/selftests/hid/progs/hid.c
> > > @@ -1,8 +1,5 @@
> > > // SPDX-License-Identifier: GPL-2.0
> > > /* Copyright (c) 2022 Red hat */
> > > -#include "vmlinux.h"
> > > -#include <bpf/bpf_helpers.h>
> > > -#include <bpf/bpf_tracing.h>
> > > #include "hid_bpf_helpers.h"
> > >
> > > char _license[] SEC("license") = "GPL";
> > > diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > > index 4fff31dbe0e7..749097f8f4d9 100644
> > > --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > > +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > > @@ -5,6 +5,26 @@
> > > #ifndef __HID_BPF_HELPERS_H
> > > #define __HID_BPF_HELPERS_H
> > >
> > > +/* "undefine" structs in vmlinux.h, because we "override" them below */
> > > +#define hid_bpf_ctx hid_bpf_ctx___not_used
> > > +#include "vmlinux.h"
> > > +#undef hid_bpf_ctx
> > > +
> > > +#include <bpf/bpf_helpers.h>
> > > +#include <bpf/bpf_tracing.h>
> > > +
> > > +
> > > +struct hid_bpf_ctx {
> > > + __u32 index;
> > > + const struct hid_device *hid;
> > > + __u32 allocated_size;
> > > + enum hid_report_type report_type;
> > > + union {
> > > + __s32 retval;
> > > + __s32 size;
> > > + };
> > > +};
> > > +
> > > /* following are kfuncs exported by HID for HID-BPF */
> > > extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> > > unsigned int offset,
> >
>
> [1]: https://lore.kernel.org/all/20230825182316.m2ksjoxe4s7dsapn@google.com/
>
> Thanks
> Justin
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Justin Stitt @ 2023-08-25 18:36 UTC (permalink / raw)
To: Eduard Zingerman
Cc: Benjamin Tissoires, Nick Desaulniers, linux-kselftest, bpf,
linux-input, Linux Kernel Mailing List, Nathan Chancellor,
Kees Cook
In-Reply-To: <e99b4226bd450fedfebd4eb5c37054f032432b4f.camel@gmail.com>
On Fri, Aug 25, 2023 at 6:01 AM Eduard Zingerman <eddyz87@gmail.com> wrote:
>
> On Fri, 2023-08-25 at 10:08 +0200, Benjamin Tissoires wrote:
> >
> > On Tue, Aug 22, 2023 at 11:42 PM Justin Stitt <justinstitt@google.com> wrote:
> > > > > > > Which kernel are you trying to test?
> > > > > > > I tested your 2 commands on v6.5-rc7 and it just works.
> > > > > >
> > > > > > I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
> > > > > >
> > > > > > I ran these exact commands:
> > > > > > > $ make mrproper
> > > > > > > $ make LLVM=1 ARCH=x86_64 headers
> > > > > > > $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> > > > > > TARGETS=hid &> out
> > > > > >
> > > > > > and here's the contents of `out` (still warnings/errors):
> > > > > > https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
> > > > > >
> > > > > > I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
> > > > >
> > > > > Sigh... there is a high chance my Makefile is not correct and uses the
> > > > > installed headers (I was running the exact same commands, but on a
> > > > > v6.4-rc7+ kernel).
> > > > >
> > > > > But sorry, it will have to wait for tomorrow if you want me to have a
> > > > > look at it. It's 11:35 PM here, and I need to go to bed
> > > > Take it easy. Thanks for the prompt responses here! I'd like to get
> > > > the entire kselftest make target building with Clang so that we can
> > > > close [1].
> >
> > Sorry I got urgent matters to tackle yesterday.
> >
> > It took me a while to understand what was going on, and I finally found
> > it.
> >
> > struct hid_bpf_ctx is internal to the kernel, and so is declared in
> > vmlinux.h, that we generate through BTF. But to generate the vmlinux.h
> > with the correct symbols, these need to be present in the running
> > kernel.
> > And that's where we had a fundamental difference: I was running my
> > compilations on a kernel v6.3+ (6.4.11) with that symbol available, and
> > you are probably not.
> >
> > The bpf folks are using a clever trick to force the compilation[2]. And
> > I think the following patch would work for you:
>
> Hi Benjamin, Justin,
>
> You might want to take a look at these two links:
> [1] https://nakryiko.com/posts/bpf-core-reference-guide/#handling-incompatible-field-and-type-changes
> [2] https://facebookmicrosites.github.io/bpf/blog/2020/02/19/bpf-portability-and-co-re.html#dealing-with-kernel-version-and-configuration-differences
>
> The short version is: CO-RE relocation handling logic in libbpf
> ignores suffixes of form '___something' for type and field names.
>
> So, the following should accomplish the same as the trick with
> #define/#undef:
>
> #include "vmlinux.h"
> ...
> struct hid_bpf_ctx___local {
> __u32 index;
> const struct hid_device *hid;
> __u32 allocated_size;
> enum hid_report_type report_type;
> union {
> __s32 retval;
> __s32 size;
> };
>
> };
> ...
> extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
> unsigned int offset, ...)
>
> However, if the kernel does not have `hid_bpf_ctx` definition would
> the test `progs/hid.c` still make sense?
>
> When I tried to build hid tests locally I run into similar errors:
>
> ...
> CLNG-BPF hid.bpf.o
> In file included from progs/hid.c:6:
> progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct hid_bpf_ctx' \
> will not be visible outside of this function [-Werror,-Wvisibility]
> extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> ...
>
> And there is indeed no `hid_bpf_ctx` in my vmlinux.h.
> However, after enabling CONFIG_HID_BPF in kernel config the
> `hid_bpf_ctx` appears in vmlinux.h, and I can compile HID selftests
> w/o issues.
Even with enabling this configuration option I was unable to get clean
builds of the HID selftests. I proposed a 4th patch on top of
Benjamin's n=3 patch series here [1] using the #def/#undef pattern.
>
> >
> > ---
> > From bb9eccb7a896ba4b3a35ed12a248e6d6cfed2df6 Mon Sep 17 00:00:00 2001
> > From: Benjamin Tissoires <bentiss@kernel.org>
> > Date: Fri, 25 Aug 2023 10:02:32 +0200
> > Subject: [PATCH] selftests/hid: ensure we can compile the tests on kernels
> > pre-6.3
> >
> > For the hid-bpf tests to compile, we need to have the definition of
> > struct hid_bpf_ctx. This definition is an internal one from the kernel
> > and it is supposed to be defined in the generated vmlinux.h.
> >
> > This vmlinux.h header is generated based on the currently running kernel
> > or if the kernel was already compiled in the tree. If you just compile
> > the selftests without compiling the kernel beforehand and you are running
> > on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> > definition.
> >
> > Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> > to force the definition of that symbol in case we don't find it in the
> > BTF.
> >
> > Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> > ---
> > tools/testing/selftests/hid/progs/hid.c | 3 ---
> > .../selftests/hid/progs/hid_bpf_helpers.h | 20 +++++++++++++++++++
> > 2 files changed, 20 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> > index 88c593f753b5..1e558826b809 100644
> > --- a/tools/testing/selftests/hid/progs/hid.c
> > +++ b/tools/testing/selftests/hid/progs/hid.c
> > @@ -1,8 +1,5 @@
> > // SPDX-License-Identifier: GPL-2.0
> > /* Copyright (c) 2022 Red hat */
> > -#include "vmlinux.h"
> > -#include <bpf/bpf_helpers.h>
> > -#include <bpf/bpf_tracing.h>
> > #include "hid_bpf_helpers.h"
> >
> > char _license[] SEC("license") = "GPL";
> > diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > index 4fff31dbe0e7..749097f8f4d9 100644
> > --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> > @@ -5,6 +5,26 @@
> > #ifndef __HID_BPF_HELPERS_H
> > #define __HID_BPF_HELPERS_H
> >
> > +/* "undefine" structs in vmlinux.h, because we "override" them below */
> > +#define hid_bpf_ctx hid_bpf_ctx___not_used
> > +#include "vmlinux.h"
> > +#undef hid_bpf_ctx
> > +
> > +#include <bpf/bpf_helpers.h>
> > +#include <bpf/bpf_tracing.h>
> > +
> > +
> > +struct hid_bpf_ctx {
> > + __u32 index;
> > + const struct hid_device *hid;
> > + __u32 allocated_size;
> > + enum hid_report_type report_type;
> > + union {
> > + __s32 retval;
> > + __s32 size;
> > + };
> > +};
> > +
> > /* following are kfuncs exported by HID for HID-BPF */
> > extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> > unsigned int offset,
>
[1]: https://lore.kernel.org/all/20230825182316.m2ksjoxe4s7dsapn@google.com/
Thanks
Justin
^ permalink raw reply
* [PATCH 4/3] selftests/hid: more fixes to build with older kernel
From: Justin Stitt @ 2023-08-25 18:23 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Nick Desaulniers,
linux-input, linux-kselftest, linux-kernel, bpf
In-Reply-To: <20230825-wip-selftests-v1-0-c862769020a8@kernel.org>
On Fri, Aug 25, 2023 at 10:36:30AM +0200, Benjamin Tissoires wrote:
> These fixes have been triggered by [0]:
> basically, if you do not recompile the kernel first, and are
> running on an old kernel, vmlinux.h doesn't have the required
> symbols and the compilation fails.
>
> The tests will fail if you run them on that very same machine,
> of course, but the binary should compile.
>
> And while I was sorting out why it was failing, I realized I
> could do a couple of improvements on the Makefile.
>
> [0] https://lore.kernel.org/linux-input/56ba8125-2c6f-a9c9-d498-0ca1c153dcb2@redhat.com/T/#t
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> Benjamin Tissoires (3):
> selftests/hid: ensure we can compile the tests on kernels pre-6.3
> selftests/hid: do not manually call headers_install
> selftests/hid: force using our compiled libbpf headers
>
> tools/testing/selftests/hid/Makefile | 10 ++++------
> tools/testing/selftests/hid/progs/hid.c | 3 ---
> tools/testing/selftests/hid/progs/hid_bpf_helpers.h | 20 ++++++++++++++++++++
> 3 files changed, 24 insertions(+), 9 deletions(-)
> ---
> base-commit: 1d7546042f8fdc4bc39ab91ec966203e2d64f8bd
> change-id: 20230825-wip-selftests-9a7502b56542
>
> Best regards,
> --
> Benjamin Tissoires <bentiss@kernel.org>
>
Benjamin, thanks for the work here. Your series fixed up _some_ of the
errors I had while building on my 6.3.11 kernel. I'm proposing a single
patch that should be applied on top of your series that fully fixes
_all_ of the build errors I'm experiencing.
Can you let me know if it works and potentially formulate a new series
so that `b4 shazam` applies it cleanly?
PATCH BELOW
---
From 5378d70e1b3f7f75656332f9bff65a37122bb288 Mon Sep 17 00:00:00 2001
From: Justin Stitt <justinstitt@google.com>
Date: Fri, 25 Aug 2023 18:10:33 +0000
Subject: [PATCH 4/3] selftests/hid: more fixes to build with older kernel
I had to use the clever trick [1] on some other symbols to get my builds
working.
Apply this patch on top of Benjamin's series [2].
This is now a n=4 patch series which has fixed my builds when running:
| $ make LLVM=1 -j128 ARCH=x86_64 mrproper headers
| $ make LLVM=1 -j128 ARCH=x86_64 -C tools/testing/selftests TARGETS=hid
[1]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/bpf/progs/bpf_iter.h#n3
[2]: https://lore.kernel.org/all/20230825-wip-selftests-v1-0-c862769020a8@kernel.org/
Signed-off-by: Justin Stitt <justinstitt@google.com>
---
.../selftests/hid/progs/hid_bpf_helpers.h | 29 +++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 749097f8f4d9..e2eace2c0029 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -7,12 +7,26 @@
/* "undefine" structs in vmlinux.h, because we "override" them below */
#define hid_bpf_ctx hid_bpf_ctx___not_used
+#define hid_report_type hid_report_type___not_used
+#define hid_class_request hid_class_request___not_used
+#define hid_bpf_attach_flags hid_bpf_attach_flags___not_used
#include "vmlinux.h"
#undef hid_bpf_ctx
+#undef hid_report_type
+#undef hid_class_request
+#undef hid_bpf_attach_flags
#include <bpf/bpf_helpers.h>
#include <bpf/bpf_tracing.h>
+#include <linux/const.h>
+enum hid_report_type {
+ HID_INPUT_REPORT = 0,
+ HID_OUTPUT_REPORT = 1,
+ HID_FEATURE_REPORT = 2,
+
+ HID_REPORT_TYPES,
+};
struct hid_bpf_ctx {
__u32 index;
@@ -25,6 +39,21 @@ struct hid_bpf_ctx {
};
};
+enum hid_class_request {
+ HID_REQ_GET_REPORT = 0x01,
+ HID_REQ_GET_IDLE = 0x02,
+ HID_REQ_GET_PROTOCOL = 0x03,
+ HID_REQ_SET_REPORT = 0x09,
+ HID_REQ_SET_IDLE = 0x0A,
+ HID_REQ_SET_PROTOCOL = 0x0B,
+};
+
+enum hid_bpf_attach_flags {
+ HID_BPF_FLAG_NONE = 0,
+ HID_BPF_FLAG_INSERT_HEAD = _BITUL(0),
+ HID_BPF_FLAG_MAX,
+};
+
/* following are kfuncs exported by HID for HID-BPF */
extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
unsigned int offset,
--
2.42.0.rc1.204.g551eb34607-goog
^ permalink raw reply related
* Re: selftests: hid: trouble building with clang due to missing header
From: Eduard Zingerman @ 2023-08-25 13:01 UTC (permalink / raw)
To: Benjamin Tissoires, Justin Stitt
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <56ba8125-2c6f-a9c9-d498-0ca1c153dcb2@redhat.com>
On Fri, 2023-08-25 at 10:08 +0200, Benjamin Tissoires wrote:
>
> On Tue, Aug 22, 2023 at 11:42 PM Justin Stitt <justinstitt@google.com> wrote:
> > > > > > Which kernel are you trying to test?
> > > > > > I tested your 2 commands on v6.5-rc7 and it just works.
> > > > >
> > > > > I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
> > > > >
> > > > > I ran these exact commands:
> > > > > > $ make mrproper
> > > > > > $ make LLVM=1 ARCH=x86_64 headers
> > > > > > $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> > > > > TARGETS=hid &> out
> > > > >
> > > > > and here's the contents of `out` (still warnings/errors):
> > > > > https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
> > > > >
> > > > > I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
> > > >
> > > > Sigh... there is a high chance my Makefile is not correct and uses the
> > > > installed headers (I was running the exact same commands, but on a
> > > > v6.4-rc7+ kernel).
> > > >
> > > > But sorry, it will have to wait for tomorrow if you want me to have a
> > > > look at it. It's 11:35 PM here, and I need to go to bed
> > > Take it easy. Thanks for the prompt responses here! I'd like to get
> > > the entire kselftest make target building with Clang so that we can
> > > close [1].
>
> Sorry I got urgent matters to tackle yesterday.
>
> It took me a while to understand what was going on, and I finally found
> it.
>
> struct hid_bpf_ctx is internal to the kernel, and so is declared in
> vmlinux.h, that we generate through BTF. But to generate the vmlinux.h
> with the correct symbols, these need to be present in the running
> kernel.
> And that's where we had a fundamental difference: I was running my
> compilations on a kernel v6.3+ (6.4.11) with that symbol available, and
> you are probably not.
>
> The bpf folks are using a clever trick to force the compilation[2]. And
> I think the following patch would work for you:
Hi Benjamin, Justin,
You might want to take a look at these two links:
[1] https://nakryiko.com/posts/bpf-core-reference-guide/#handling-incompatible-field-and-type-changes
[2] https://facebookmicrosites.github.io/bpf/blog/2020/02/19/bpf-portability-and-co-re.html#dealing-with-kernel-version-and-configuration-differences
The short version is: CO-RE relocation handling logic in libbpf
ignores suffixes of form '___something' for type and field names.
So, the following should accomplish the same as the trick with
#define/#undef:
#include "vmlinux.h"
...
struct hid_bpf_ctx___local {
__u32 index;
const struct hid_device *hid;
__u32 allocated_size;
enum hid_report_type report_type;
union {
__s32 retval;
__s32 size;
};
};
...
extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx___local *ctx,
unsigned int offset, ...)
However, if the kernel does not have `hid_bpf_ctx` definition would
the test `progs/hid.c` still make sense?
When I tried to build hid tests locally I run into similar errors:
...
CLNG-BPF hid.bpf.o
In file included from progs/hid.c:6:
progs/hid_bpf_helpers.h:9:38: error: declaration of 'struct hid_bpf_ctx' \
will not be visible outside of this function [-Werror,-Wvisibility]
extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
...
And there is indeed no `hid_bpf_ctx` in my vmlinux.h.
However, after enabling CONFIG_HID_BPF in kernel config the
`hid_bpf_ctx` appears in vmlinux.h, and I can compile HID selftests
w/o issues.
>
> ---
> From bb9eccb7a896ba4b3a35ed12a248e6d6cfed2df6 Mon Sep 17 00:00:00 2001
> From: Benjamin Tissoires <bentiss@kernel.org>
> Date: Fri, 25 Aug 2023 10:02:32 +0200
> Subject: [PATCH] selftests/hid: ensure we can compile the tests on kernels
> pre-6.3
>
> For the hid-bpf tests to compile, we need to have the definition of
> struct hid_bpf_ctx. This definition is an internal one from the kernel
> and it is supposed to be defined in the generated vmlinux.h.
>
> This vmlinux.h header is generated based on the currently running kernel
> or if the kernel was already compiled in the tree. If you just compile
> the selftests without compiling the kernel beforehand and you are running
> on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
> definition.
>
> Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
> to force the definition of that symbol in case we don't find it in the
> BTF.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> tools/testing/selftests/hid/progs/hid.c | 3 ---
> .../selftests/hid/progs/hid_bpf_helpers.h | 20 +++++++++++++++++++
> 2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
> index 88c593f753b5..1e558826b809 100644
> --- a/tools/testing/selftests/hid/progs/hid.c
> +++ b/tools/testing/selftests/hid/progs/hid.c
> @@ -1,8 +1,5 @@
> // SPDX-License-Identifier: GPL-2.0
> /* Copyright (c) 2022 Red hat */
> -#include "vmlinux.h"
> -#include <bpf/bpf_helpers.h>
> -#include <bpf/bpf_tracing.h>
> #include "hid_bpf_helpers.h"
>
> char _license[] SEC("license") = "GPL";
> diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> index 4fff31dbe0e7..749097f8f4d9 100644
> --- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> +++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
> @@ -5,6 +5,26 @@
> #ifndef __HID_BPF_HELPERS_H
> #define __HID_BPF_HELPERS_H
>
> +/* "undefine" structs in vmlinux.h, because we "override" them below */
> +#define hid_bpf_ctx hid_bpf_ctx___not_used
> +#include "vmlinux.h"
> +#undef hid_bpf_ctx
> +
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +
> +
> +struct hid_bpf_ctx {
> + __u32 index;
> + const struct hid_device *hid;
> + __u32 allocated_size;
> + enum hid_report_type report_type;
> + union {
> + __s32 retval;
> + __s32 size;
> + };
> +};
> +
> /* following are kfuncs exported by HID for HID-BPF */
> extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
> unsigned int offset,
^ permalink raw reply
* Re: [PATCH v2] HID: logitech-hidpp: rework one more time the retries attempts
From: Benjamin Tissoires @ 2023-08-25 9:45 UTC (permalink / raw)
To: Linux regressions mailing list
Cc: bentiss, Filipe Laíns, Bastien Nocera, Jiri Kosina, Greg KH,
linux-input, linux-kernel, stable
In-Reply-To: <b68cf309-fca4-7ae0-b42f-90d5f338acdd@leemhuis.info>
Hi Thorsten,
On Fri, Aug 4, 2023 at 11:14 AM Linux regression tracking (Thorsten
Leemhuis) <regressions@leemhuis.info> wrote:
>
> Hi Benjamin, /me again! :-D
>
> On 12.07.23 17:02, bentiss@kernel.org wrote:
> > From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> >
> > Extract the internal code inside a helper function, fix the
> > initialization of the parameters used in the helper function
> > (`hidpp->answer_available` was not reset and `*response` wasn't either),
> > and use a `do {...} while();` loop.
> >
> > Fixes: 586e8fede795 ("HID: logitech-hidpp: Retry commands when device is busy")
> > Cc: stable@vger.kernel.org
>
> From what I understood there was hope that this would cure the last
> remains (occasional init problems iirc) of the recent regressions with
> this driver and their fixes. But things look stalled. Is there a reason?
> Lack of reviews? Is there nevertheless hope that this will make it at
> least into 6.6?
Well, mostly lack of testing. But given that nothing happened, I just
sneaked it in the 6.6 work. I guess bots are happy enough by now.
Cheers,
Benjamin
>
> Ciao, Thorsten
>
> > Reviewed-by: Bastien Nocera <hadess@hadess.net>
> > Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > ---
> > as requested by https://lore.kernel.org/all/CAHk-=wiMbF38KCNhPFiargenpSBoecSXTLQACKS2UMyo_Vu2ww@mail.gmail.com/
> > This is a rewrite of that particular piece of code.
> > ---
> > Changes in v2:
> > - added __must_hold() for KASAN
> > - Reworked the comment describing the functions and their return values
> > - Link to v1: https://lore.kernel.org/r/20230621-logitech-fixes-v1-1-32e70933c0b0@redhat.com
> > ---
> > drivers/hid/hid-logitech-hidpp.c | 115 +++++++++++++++++++++++++--------------
> > 1 file changed, 75 insertions(+), 40 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 129b01be488d..09ba2086c95c 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -275,21 +275,22 @@ static int __hidpp_send_report(struct hid_device *hdev,
> > }
> >
> > /*
> > - * hidpp_send_message_sync() returns 0 in case of success, and something else
> > - * in case of a failure.
> > - * - If ' something else' is positive, that means that an error has been raised
> > - * by the protocol itself.
> > - * - If ' something else' is negative, that means that we had a classic error
> > - * (-ENOMEM, -EPIPE, etc...)
> > + * Effectively send the message to the device, waiting for its answer.
> > + *
> > + * Must be called with hidpp->send_mutex locked
> > + *
> > + * Same return protocol than hidpp_send_message_sync():
> > + * - success on 0
> > + * - negative error means transport error
> > + * - positive value means protocol error
> > */
> > -static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > +static int __do_hidpp_send_message_sync(struct hidpp_device *hidpp,
> > struct hidpp_report *message,
> > struct hidpp_report *response)
> > {
> > - int ret = -1;
> > - int max_retries = 3;
> > + int ret;
> >
> > - mutex_lock(&hidpp->send_mutex);
> > + __must_hold(&hidpp->send_mutex);
> >
> > hidpp->send_receive_buf = response;
> > hidpp->answer_available = false;
> > @@ -300,47 +301,74 @@ static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > */
> > *response = *message;
> >
> > - for (; max_retries != 0 && ret; max_retries--) {
> > - ret = __hidpp_send_report(hidpp->hid_dev, message);
> > + ret = __hidpp_send_report(hidpp->hid_dev, message);
> > + if (ret) {
> > + dbg_hid("__hidpp_send_report returned err: %d\n", ret);
> > + memset(response, 0, sizeof(struct hidpp_report));
> > + return ret;
> > + }
> >
> > - if (ret) {
> > - dbg_hid("__hidpp_send_report returned err: %d\n", ret);
> > - memset(response, 0, sizeof(struct hidpp_report));
> > - break;
> > - }
> > + if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
> > + 5*HZ)) {
> > + dbg_hid("%s:timeout waiting for response\n", __func__);
> > + memset(response, 0, sizeof(struct hidpp_report));
> > + return -ETIMEDOUT;
> > + }
> >
> > - if (!wait_event_timeout(hidpp->wait, hidpp->answer_available,
> > - 5*HZ)) {
> > - dbg_hid("%s:timeout waiting for response\n", __func__);
> > - memset(response, 0, sizeof(struct hidpp_report));
> > - ret = -ETIMEDOUT;
> > - break;
> > - }
> > + if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > + response->rap.sub_id == HIDPP_ERROR) {
> > + ret = response->rap.params[1];
> > + dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> > + return ret;
> > + }
> >
> > - if (response->report_id == REPORT_ID_HIDPP_SHORT &&
> > - response->rap.sub_id == HIDPP_ERROR) {
> > - ret = response->rap.params[1];
> > - dbg_hid("%s:got hidpp error %02X\n", __func__, ret);
> > + if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> > + response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
> > + response->fap.feature_index == HIDPP20_ERROR) {
> > + ret = response->fap.params[1];
> > + dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +/*
> > + * hidpp_send_message_sync() returns 0 in case of success, and something else
> > + * in case of a failure.
> > + *
> > + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
> > + * value.
> > + */
> > +static int hidpp_send_message_sync(struct hidpp_device *hidpp,
> > + struct hidpp_report *message,
> > + struct hidpp_report *response)
> > +{
> > + int ret;
> > + int max_retries = 3;
> > +
> > + mutex_lock(&hidpp->send_mutex);
> > +
> > + do {
> > + ret = __do_hidpp_send_message_sync(hidpp, message, response);
> > + if (ret != HIDPP20_ERROR_BUSY)
> > break;
> > - }
> >
> > - if ((response->report_id == REPORT_ID_HIDPP_LONG ||
> > - response->report_id == REPORT_ID_HIDPP_VERY_LONG) &&
> > - response->fap.feature_index == HIDPP20_ERROR) {
> > - ret = response->fap.params[1];
> > - if (ret != HIDPP20_ERROR_BUSY) {
> > - dbg_hid("%s:got hidpp 2.0 error %02X\n", __func__, ret);
> > - break;
> > - }
> > - dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
> > - }
> > - }
> > + dbg_hid("%s:got busy hidpp 2.0 error %02X, retrying\n", __func__, ret);
> > + } while (--max_retries);
> >
> > mutex_unlock(&hidpp->send_mutex);
> > return ret;
> >
> > }
> >
> > +/*
> > + * hidpp_send_fap_command_sync() returns 0 in case of success, and something else
> > + * in case of a failure.
> > + *
> > + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
> > + * value.
> > + */
> > static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
> > u8 feat_index, u8 funcindex_clientid, u8 *params, int param_count,
> > struct hidpp_report *response)
> > @@ -373,6 +401,13 @@ static int hidpp_send_fap_command_sync(struct hidpp_device *hidpp,
> > return ret;
> > }
> >
> > +/*
> > + * hidpp_send_rap_command_sync() returns 0 in case of success, and something else
> > + * in case of a failure.
> > + *
> > + * See __do_hidpp_send_message_sync() for a detailed explanation of the returned
> > + * value.
> > + */
> > static int hidpp_send_rap_command_sync(struct hidpp_device *hidpp_dev,
> > u8 report_id, u8 sub_id, u8 reg_address, u8 *params, int param_count,
> > struct hidpp_report *response)
> >
> > ---
> > base-commit: 87854366176403438d01f368b09de3ec2234e0f5
> > change-id: 20230621-logitech-fixes-a4c0e66ea2ad
> >
> > Best regards,
>
^ permalink raw reply
* Re: [PATCH v2] HID: logitech-hidpp: rework one more time the retries attempts
From: Benjamin Tissoires @ 2023-08-25 9:44 UTC (permalink / raw)
To: Filipe Laíns, Bastien Nocera, Jiri Kosina, Greg KH, bentiss
Cc: linux-input, linux-kernel, Benjamin Tissoires, stable
In-Reply-To: <20230621-logitech-fixes-v2-1-3635f7f9c8af@kernel.org>
On Wed, 12 Jul 2023 17:02:34 +0200, bentiss@kernel.org wrote:
> Extract the internal code inside a helper function, fix the
> initialization of the parameters used in the helper function
> (`hidpp->answer_available` was not reset and `*response` wasn't either),
> and use a `do {...} while();` loop.
>
>
Applied to hid/hid.git (for-6.6/logitech), thanks!
[1/1] HID: logitech-hidpp: rework one more time the retries attempts
https://git.kernel.org/hid/hid/c/60165ab774cb
Cheers,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* [PATCH 2/3] selftests/hid: do not manually call headers_install
From: Benjamin Tissoires @ 2023-08-25 8:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Justin Stitt,
Nick Desaulniers
Cc: linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires
In-Reply-To: <20230825-wip-selftests-v1-0-c862769020a8@kernel.org>
"make headers" is a requirement before calling make on the selftests
dir, so we should not have to manually install those headers
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/Makefile | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index 01c0491d64da..c5522088ece4 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -21,7 +21,7 @@ CXX ?= $(CROSS_COMPILE)g++
HOSTPKG_CONFIG := pkg-config
-CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(KHDR_INCLUDES) -I$(OUTPUT)
+CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT)
LDLIBS += -lelf -lz -lrt -lpthread
# Silence some warnings when compiled with clang
@@ -65,7 +65,6 @@ BPFTOOLDIR := $(TOOLSDIR)/bpf/bpftool
SCRATCH_DIR := $(OUTPUT)/tools
BUILD_DIR := $(SCRATCH_DIR)/build
INCLUDE_DIR := $(SCRATCH_DIR)/include
-KHDR_INCLUDES := $(SCRATCH_DIR)/uapi/include
BPFOBJ := $(BUILD_DIR)/libbpf/libbpf.a
ifneq ($(CROSS_COMPILE),)
HOST_BUILD_DIR := $(BUILD_DIR)/host
@@ -151,9 +150,6 @@ else
$(Q)cp "$(VMLINUX_H)" $@
endif
-$(KHDR_INCLUDES)/linux/hid.h: $(top_srcdir)/include/uapi/linux/hid.h
- $(MAKE) -C $(top_srcdir) INSTALL_HDR_PATH=$(SCRATCH_DIR)/uapi headers_install
-
$(RESOLVE_BTFIDS): $(HOST_BPFOBJ) | $(HOST_BUILD_DIR)/resolve_btfids \
$(TOOLSDIR)/bpf/resolve_btfids/main.c \
$(TOOLSDIR)/lib/rbtree.c \
@@ -231,7 +227,7 @@ $(BPF_SKELS): %.skel.h: %.bpf.o $(BPFTOOL) | $(OUTPUT)
$(Q)$(BPFTOOL) gen object $(<:.o=.linked1.o) $<
$(Q)$(BPFTOOL) gen skeleton $(<:.o=.linked1.o) name $(notdir $(<:.bpf.o=)) > $@
-$(OUTPUT)/%.o: %.c $(BPF_SKELS) $(KHDR_INCLUDES)/linux/hid.h
+$(OUTPUT)/%.o: %.c $(BPF_SKELS)
$(call msg,CC,,$@)
$(Q)$(CC) $(CFLAGS) -c $(filter %.c,$^) $(LDLIBS) -o $@
--
2.39.1
^ permalink raw reply related
* [PATCH 3/3] selftests/hid: force using our compiled libbpf headers
From: Benjamin Tissoires @ 2023-08-25 8:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Justin Stitt,
Nick Desaulniers
Cc: linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires
In-Reply-To: <20230825-wip-selftests-v1-0-c862769020a8@kernel.org>
Turns out that we were relying on the globally installed headers, not
the ones we freshly compiled.
Add a manual include in CFLAGS to sort this out.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/Makefile | 2 ++
1 file changed, 2 insertions(+)
diff --git a/tools/testing/selftests/hid/Makefile b/tools/testing/selftests/hid/Makefile
index c5522088ece4..b01c14077c5d 100644
--- a/tools/testing/selftests/hid/Makefile
+++ b/tools/testing/selftests/hid/Makefile
@@ -22,6 +22,8 @@ CXX ?= $(CROSS_COMPILE)g++
HOSTPKG_CONFIG := pkg-config
CFLAGS += -g -O0 -rdynamic -Wall -Werror -I$(OUTPUT)
+CFLAGS += -I$(OUTPUT)/tools/include
+
LDLIBS += -lelf -lz -lrt -lpthread
# Silence some warnings when compiled with clang
--
2.39.1
^ permalink raw reply related
* [PATCH 1/3] selftests/hid: ensure we can compile the tests on kernels pre-6.3
From: Benjamin Tissoires @ 2023-08-25 8:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Justin Stitt,
Nick Desaulniers
Cc: linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires
In-Reply-To: <20230825-wip-selftests-v1-0-c862769020a8@kernel.org>
For the hid-bpf tests to compile, we need to have the definition of
struct hid_bpf_ctx. This definition is an internal one from the kernel
and it is supposed to be defined in the generated vmlinux.h.
This vmlinux.h header is generated based on the currently running kernel
or if the kernel was already compiled in the tree. If you just compile
the selftests without compiling the kernel beforehand and you are running
on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
definition.
Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
to force the definition of that symbol in case we don't find it in the
BTF.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/progs/hid.c | 3 ---
tools/testing/selftests/hid/progs/hid_bpf_helpers.h | 20 ++++++++++++++++++++
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 88c593f753b5..1e558826b809 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -1,8 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2022 Red hat */
-#include "vmlinux.h"
-#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include "hid_bpf_helpers.h"
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 4fff31dbe0e7..749097f8f4d9 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -5,6 +5,26 @@
#ifndef __HID_BPF_HELPERS_H
#define __HID_BPF_HELPERS_H
+/* "undefine" structs in vmlinux.h, because we "override" them below */
+#define hid_bpf_ctx hid_bpf_ctx___not_used
+#include "vmlinux.h"
+#undef hid_bpf_ctx
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+
+struct hid_bpf_ctx {
+ __u32 index;
+ const struct hid_device *hid;
+ __u32 allocated_size;
+ enum hid_report_type report_type;
+ union {
+ __s32 retval;
+ __s32 size;
+ };
+};
+
/* following are kfuncs exported by HID for HID-BPF */
extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
unsigned int offset,
--
2.39.1
^ permalink raw reply related
* [PATCH 0/3] selftests/hid: assorted fixes
From: Benjamin Tissoires @ 2023-08-25 8:36 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires, Shuah Khan, Justin Stitt,
Nick Desaulniers
Cc: linux-input, linux-kselftest, linux-kernel, bpf,
Benjamin Tissoires
These fixes have been triggered by [0]:
basically, if you do not recompile the kernel first, and are
running on an old kernel, vmlinux.h doesn't have the required
symbols and the compilation fails.
The tests will fail if you run them on that very same machine,
of course, but the binary should compile.
And while I was sorting out why it was failing, I realized I
could do a couple of improvements on the Makefile.
[0] https://lore.kernel.org/linux-input/56ba8125-2c6f-a9c9-d498-0ca1c153dcb2@redhat.com/T/#t
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Benjamin Tissoires (3):
selftests/hid: ensure we can compile the tests on kernels pre-6.3
selftests/hid: do not manually call headers_install
selftests/hid: force using our compiled libbpf headers
tools/testing/selftests/hid/Makefile | 10 ++++------
tools/testing/selftests/hid/progs/hid.c | 3 ---
tools/testing/selftests/hid/progs/hid_bpf_helpers.h | 20 ++++++++++++++++++++
3 files changed, 24 insertions(+), 9 deletions(-)
---
base-commit: 1d7546042f8fdc4bc39ab91ec966203e2d64f8bd
change-id: 20230825-wip-selftests-9a7502b56542
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply
* Re: selftests: hid: trouble building with clang due to missing header
From: Benjamin Tissoires @ 2023-08-25 8:08 UTC (permalink / raw)
To: Justin Stitt
Cc: Nick Desaulniers, linux-kselftest, bpf, linux-input,
Linux Kernel Mailing List, Nathan Chancellor, Kees Cook
In-Reply-To: <20230822214220.jjx3srik4mteeond@google.com>
On Tue, Aug 22, 2023 at 11:42 PM Justin Stitt <justinstitt@google.com> wrote:
> > > > > Which kernel are you trying to test?
> > > > > I tested your 2 commands on v6.5-rc7 and it just works.
> > > >
> > > > I'm also on v6.5-rc7 (706a741595047797872e669b3101429ab8d378ef)
> > > >
> > > > I ran these exact commands:
> > > > | $ make mrproper
> > > > | $ make LLVM=1 ARCH=x86_64 headers
> > > > | $ make LLVM=1 ARCH=x86_64 -j128 -C tools/testing/selftests
> > > > TARGETS=hid &> out
> > > >
> > > > and here's the contents of `out` (still warnings/errors):
> > > > https://gist.github.com/JustinStitt/d0c30180a2a2e046c32d5f0ce5f59c6d
> > > >
> > > > I have a feeling I'm doing something fundamentally incorrectly. Any ideas?
> > >
> > > Sigh... there is a high chance my Makefile is not correct and uses the
> > > installed headers (I was running the exact same commands, but on a
> > > v6.4-rc7+ kernel).
> > >
> > > But sorry, it will have to wait for tomorrow if you want me to have a
> > > look at it. It's 11:35 PM here, and I need to go to bed
> > Take it easy. Thanks for the prompt responses here! I'd like to get
> > the entire kselftest make target building with Clang so that we can
> > close [1].
Sorry I got urgent matters to tackle yesterday.
It took me a while to understand what was going on, and I finally found
it.
struct hid_bpf_ctx is internal to the kernel, and so is declared in
vmlinux.h, that we generate through BTF. But to generate the vmlinux.h
with the correct symbols, these need to be present in the running
kernel.
And that's where we had a fundamental difference: I was running my
compilations on a kernel v6.3+ (6.4.11) with that symbol available, and
you are probably not.
The bpf folks are using a clever trick to force the compilation[2]. And
I think the following patch would work for you:
---
From bb9eccb7a896ba4b3a35ed12a248e6d6cfed2df6 Mon Sep 17 00:00:00 2001
From: Benjamin Tissoires <bentiss@kernel.org>
Date: Fri, 25 Aug 2023 10:02:32 +0200
Subject: [PATCH] selftests/hid: ensure we can compile the tests on kernels
pre-6.3
For the hid-bpf tests to compile, we need to have the definition of
struct hid_bpf_ctx. This definition is an internal one from the kernel
and it is supposed to be defined in the generated vmlinux.h.
This vmlinux.h header is generated based on the currently running kernel
or if the kernel was already compiled in the tree. If you just compile
the selftests without compiling the kernel beforehand and you are running
on a 6.2 kernel, you'll end up with a vmlinux.h without the hid_bpf_ctx
definition.
Use the clever trick from tools/testing/selftests/bpf/progs/bpf_iter.h
to force the definition of that symbol in case we don't find it in the
BTF.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
tools/testing/selftests/hid/progs/hid.c | 3 ---
.../selftests/hid/progs/hid_bpf_helpers.h | 20 +++++++++++++++++++
2 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/hid/progs/hid.c b/tools/testing/selftests/hid/progs/hid.c
index 88c593f753b5..1e558826b809 100644
--- a/tools/testing/selftests/hid/progs/hid.c
+++ b/tools/testing/selftests/hid/progs/hid.c
@@ -1,8 +1,5 @@
// SPDX-License-Identifier: GPL-2.0
/* Copyright (c) 2022 Red hat */
-#include "vmlinux.h"
-#include <bpf/bpf_helpers.h>
-#include <bpf/bpf_tracing.h>
#include "hid_bpf_helpers.h"
char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
index 4fff31dbe0e7..749097f8f4d9 100644
--- a/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
+++ b/tools/testing/selftests/hid/progs/hid_bpf_helpers.h
@@ -5,6 +5,26 @@
#ifndef __HID_BPF_HELPERS_H
#define __HID_BPF_HELPERS_H
+/* "undefine" structs in vmlinux.h, because we "override" them below */
+#define hid_bpf_ctx hid_bpf_ctx___not_used
+#include "vmlinux.h"
+#undef hid_bpf_ctx
+
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+
+
+struct hid_bpf_ctx {
+ __u32 index;
+ const struct hid_device *hid;
+ __u32 allocated_size;
+ enum hid_report_type report_type;
+ union {
+ __s32 retval;
+ __s32 size;
+ };
+};
+
/* following are kfuncs exported by HID for HID-BPF */
extern __u8 *hid_bpf_get_data(struct hid_bpf_ctx *ctx,
unsigned int offset,
--
2.41.0
---
Would you mind testing it?
Cheers,
Benjamin
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/testing/selftests/bpf/progs/bpf_iter.h#n3
^ permalink raw reply related
* Słowa kluczowe do wypozycjonowania
From: Adam Charachuta @ 2023-08-25 7:36 UTC (permalink / raw)
To: linux-input
Dzień dobry,
zapoznałem się z Państwa ofertą i z przyjemnością przyznaję, że przyciąga uwagę i zachęca do dalszych rozmów.
Pomyślałem, że może mógłbym mieć swój wkład w Państwa rozwój i pomóc dotrzeć z tą ofertą do większego grona odbiorców. Pozycjonuję strony www, dzięki czemu generują świetny ruch w sieci.
Możemy porozmawiać w najbliższym czasie?
Pozdrawiam serdecznie
Adam Charachuta
^ permalink raw reply
* Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
From: Julius Zint @ 2023-08-24 17:52 UTC (permalink / raw)
To: Christophe JAILLET
Cc: Helge Deller, Thomas Weißschuh, linux-kernel, dri-devel,
linux-input, linux-fbdev, Benjamin Tissoires, Jiri Kosina,
Jingoo Han, Daniel Thompson, Lee Jones
In-Reply-To: <2b7cf5af-1d96-8650-ac93-b1243bab5d7a@wanadoo.fr>
On 20.08.23 12:06, Christophe JAILLET wrote:
> [...]
>
>> +static void hid_bl_remove(struct hid_device *hdev)
>> +{
>> + struct backlight_device *bl;
>> + struct hid_bl_data *data;
>> +
>> + hid_dbg(hdev, "remove\n");
>> + bl = hid_get_drvdata(hdev);
>> + data = bl_get_data(bl);
>> +
>> + devm_backlight_device_unregister(&hdev->dev, bl);
>
> Hi,
>
> If this need to be called here, before the hid_() calls below, there
> seems to be no real point in using devm_backlight_device_register() /
> devm_backlight_device_unregister().
>
> The non-devm_ version should be enough.
The non-devm_ version is marked deprecated. As for the order, I am not
really sure. I am
concerned about someone updating the brightness while its being removed.
So the HID device
would already have been stopped and then I would issue a request and
wait for completion.
If hid_hw_request and hid_hw_wait can handle this situation we are fine.
>
>> + hid_hw_close(hdev);
>> + hid_hw_stop(hdev);
>> + hid_set_drvdata(hdev, NULL);
>> + devm_kfree(&hdev->dev, data);
>
> 'data' is devm_kzalloc()'ed.
> It is likely that this explicit devm_kfree() could be saved. It will
> be freed by the framework.
>
>> +}
>
> [...]
>
>> + if (input_field->logical_maximum !=
>> feature_field->logical_maximum) {
>> + hid_warn(hdev, "maximums do not match: %d / %d\n",
>> + input_field->logical_maximum,
>> + feature_field->logical_maximum);
>> + ret = -ENODEV;
>> + goto exit_stop;
>> + }
>> +
>> + hid_dbg(hdev, "Monitor VESA VCP with brightness control\n");
>> +
>> + ret = hid_hw_open(hdev);
>> + if (ret) {
>> + hid_err(hdev, "hw open failed: %d\n", ret);
>> + goto exit_stop;
>> + }
>> +
>> + data = devm_kzalloc(&hdev->dev, sizeof(*data), GFP_KERNEL);
>> + if (data == NULL) {
>> + ret = -ENOMEM;
>> + goto exit_stop;
>
> exit_free?
> in order to undo the hid_hw_open() just above.
Yes, my mistake. This does not call hid_hw_close(). I will fix it in v4.
>
>> + }
>> + data->hdev = hdev;
>> + data->min_brightness = input_field->logical_minimum;
>> + data->max_brightness = input_field->logical_maximum;
>> + data->input_field = input_field;
>> + data->feature_field = feature_field;
>> +
>> + memset(&props, 0, sizeof(props));
>> + props.type = BACKLIGHT_RAW;
>> + props.max_brightness = data->max_brightness - data->min_brightness;
>> +
>> + bl = devm_backlight_device_register(&hdev->dev, "vesa_vcp",
>> + &hdev->dev, data,
>> + &hid_bl_ops,
>> + &props);
>> + if (IS_ERR(bl)) {
>> + ret = PTR_ERR(bl);
>> + hid_err(hdev, "failed to register backlight: %d\n", ret);
>> + goto exit_free;
>> + }
>> +
>> + hid_set_drvdata(hdev, bl);
>> +
>> + return 0;
>> +
>> +exit_free:
>> + hid_hw_close(hdev);
>> + devm_kfree(&hdev->dev, data);
>
> 'data' is devm_kzalloc()'ed.
> It is likely that this explicit devm_kfree() could be saved. It will
> be freed by the framework.
>
>> +
>> +exit_stop:
>> + hid_hw_stop(hdev);
>> + return ret;
>> +}
>
> [...]
I will remove all of the explicit calls to devm_kfree (and others) in v4
(and test it thoroughly).
Thank you for the helpful review.
Julius
^ permalink raw reply
* Re: [PATCH v3 1/1] backlight: hid_bl: Add VESA VCP HID backlight driver
From: Julius Zint @ 2023-08-24 17:35 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Jingoo Han, Jiri Kosina, Benjamin Tissoires,
Helge Deller, Thomas Weißschuh, linux-kernel, dri-devel,
linux-input, linux-fbdev
In-Reply-To: <20230821163631.GA214013@aspen.lan>
On 21.08.23 18:36, Daniel Thompson wrote:
>> @@ -472,6 +472,14 @@ config BACKLIGHT_LED
>> If you have a LCD backlight adjustable by LED class driver, say Y
>> to enable this driver.
>>
>> +config BACKLIGHT_HID
>> + tristate "VESA VCP HID Backlight Driver"
>> + depends on HID
>> + help
>> + If you have an external display with VESA compliant HID brightness
>> + controls then say Y to enable this backlight driver. Currently the
>> + only supported device is the Apple Studio Display.
> This contradicts the description which says you write the driver to the
> standard but only tested on Apple Studio Display. There is no need to
> spell what has been tested in the Kconfig text. Remove the final
> sentence!
Will remove it in v4.
>> diff --git a/drivers/video/backlight/hid_bl.c b/drivers/video/backlight/hid_bl.c
>> new file mode 100644
>> index 000000000000..b40f8f412ee2
>> --- /dev/null
>> +++ b/drivers/video/backlight/hid_bl.c
>> <snip>
>> +static void hid_bl_remove(struct hid_device *hdev)
>> +{
>> + struct backlight_device *bl;
>> + struct hid_bl_data *data;
>> +
>> + hid_dbg(hdev, "remove\n");
> This message probably should be removed (if you want to know if a function was
> executed use ftrace).
>
>
>> + bl = hid_get_drvdata(hdev);
>> + data = bl_get_data(bl);
>> +
>> + devm_backlight_device_unregister(&hdev->dev, bl);
>> + hid_hw_close(hdev);
>> + hid_hw_stop(hdev);
>> + hid_set_drvdata(hdev, NULL);
>> + devm_kfree(&hdev->dev, data);
>> +}
>> +
>> +static int hid_bl_get_brightness_raw(struct hid_bl_data *data)
>> +{
>> + struct hid_field *field;
>> + int result;
>> +
>> + field = data->input_field;
>> + hid_hw_request(data->hdev, field->report, HID_REQ_GET_REPORT);
>> + hid_hw_wait(data->hdev);
>> + result = *field->new_value;
>> + hid_dbg(data->hdev, "get brightness: %d\n", result);
> To be honest I'm a little dubious about *all* the hid_dbg() calls. They
> add very little value (e.g. they are useful to get the driver working
> but not that important to keeping it working). As such I don't think
> they are worth the clutter in a CONFIG_DYNAMIC_DEBUG kernel.
>
> Note this is strictly for the hid_dbg() stuff... the hid_err() stuff in
> the probe error paths are much more useful!
You are right, I will remove all hid_dbg calls in v4.
Thank you very much for the review.
Julius
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox