Re: [PATCH v8 8/8] clk: Add KUnit tests for clks registered with struct clk_parent_data
Quoting Guenter Roeck (2024-10-08 16:27:37) > On 10/8/24 16:12, Stephen Boyd wrote: > > > > The best I can come up with then is to test for a NULL of_root when > > CONFIG_ARM64 and CONFIG_ACPI are enabled, because the tests > > intentionally don't work when both those configs are enabled and the > > 'of_root' isn't populated. In all other cases the 'of_root' missing is a > > bug. I'll probably make this into some sort of kunit helper function in > > of_private.h and send it to DT maintainers. > > Sounds good. Thanks a lot for tracking this down. > > That makes me wonder though why only arm64 has that restriction. Both > riscv and loongarch have ACPI enabled in their defconfig files but call > unflatten_device_tree() unconditionally. > > Oh well ... Some of the reason is described in the thread I linked earlier. In particular, this email from Mark[1]. There's also more comments from Mark on an earlier patchset[2]. Maybe arm64 will allow it later, and then we'll be able to revert this skip patch. [1] https://lore.kernel.org/all/zd4dqpho7em1j...@fvff77s0q05n.cambridge.arm.com/ [2] https://lore.kernel.org/all/ZaZtbU9hre3YhZam@FVFF77S0Q05N/
Re: [PATCH v8 8/8] clk: Add KUnit tests for clks registered with struct clk_parent_data
Quoting Guenter Roeck (2024-10-03 21:52:09)
> On 10/3/24 17:42, Stephen Boyd wrote:
> >
> > Can you please describe how you run the kunit test? And provide the qemu
> > command you run to boot arm64 with acpi?
> >
>
> Example command line:
>
> qemu-system-aarch64 -M virt -m 512 \
> -kernel arch/arm64/boot/Image -no-reboot -nographic \
> -snapshot \
> -bios /opt/buildbot/rootfs/arm64/../firmware/QEMU_EFI-aarch64.fd \
> -device virtio-blk-device,drive=d0 \
> -drive file=rootfs.ext2,if=none,id=d0,format=raw \
> -cpu cortex-a57 -serial stdio -monitor none -no-reboot \
> --append "kunit.stats_enabled=2 kunit.filter=speed>slow root=/dev/vda
> rootwait earlycon=pl011,0x900 console=ttyAMA0"
>
> That works fine for me. Configuration is arm64 defconfig plus various
> debug and kunit options. I built the efi image myself from sources.
> The root file system is from buildroot with modified init script.
> kunit tests are all built into the kernel and run during boot.
Thanks. I figured out that I was missing enabling CONFIG_ACPI. Here's my
commandline
./tools/testing/kunit/kunit.py run --arch=arm64 \
--kunitconfig=drivers/of \
--qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd -smp 2" \
--kconfig_add="CONFIG_ACPI=y" \
--kernel_args="earlycon=pl011,0x900"
Now I can boot and reproduce the failure, but there's another problem.
ACPI disables itself when it fails to find tables.
ACPI: Unable to load the System Description Tables
This calls disable_acpi() which sets acpi_disabled to 1. This happens
before the unit test runs, meaning we can't reliably use 'acpi_disabled'
as a method to skip.
The best I can come up with then is to test for a NULL of_root when
CONFIG_ARM64 and CONFIG_ACPI are enabled, because the tests
intentionally don't work when both those configs are enabled and the
'of_root' isn't populated. In all other cases the 'of_root' missing is a
bug. I'll probably make this into some sort of kunit helper function in
of_private.h and send it to DT maintainers.
---8<
diff --git a/drivers/of/of_kunit_helpers.c b/drivers/of/of_kunit_helpers.c
index 287d6c91bb37..a1330e183230 100644
--- a/drivers/of/of_kunit_helpers.c
+++ b/drivers/of/of_kunit_helpers.c
@@ -36,6 +36,9 @@ int of_overlay_fdt_apply_kunit(struct kunit *test, void
*overlay_fdt,
int ret;
int *copy_id;
+ if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ACPI) && !of_root)
+ kunit_skip(test, "arm64+acpi rejects overlays");
+
copy_id = kunit_kmalloc(test, sizeof(*copy_id), GFP_KERNEL);
if (!copy_id)
return -ENOMEM;
diff --git a/drivers/of/of_test.c b/drivers/of/of_test.c
index c85a258bc6ae..6cca43bf8029 100644
--- a/drivers/of/of_test.c
+++ b/drivers/of/of_test.c
@@ -38,6 +38,8 @@ static int of_dtb_test_init(struct kunit *test)
{
if (!IS_ENABLED(CONFIG_OF_EARLY_FLATTREE))
kunit_skip(test, "requires CONFIG_OF_EARLY_FLATTREE");
+ if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ACPI) && !of_root)
+ kunit_skip(test, "arm64+acpi doesn't populate a root node on
ACPI systems");
return 0;
}
diff --git a/drivers/of/overlay_test.c b/drivers/of/overlay_test.c
index 19a292cdeee3..3e7ac97a6796 100644
--- a/drivers/of/overlay_test.c
+++ b/drivers/of/overlay_test.c
@@ -64,6 +64,8 @@ static void of_overlay_apply_kunit_cleanup(struct kunit *test)
if (!IS_ENABLED(CONFIG_OF_EARLY_FLATTREE))
kunit_skip(test, "requires CONFIG_OF_EARLY_FLATTREE for root
node");
+ if (IS_ENABLED(CONFIG_ARM64) && IS_ENABLED(CONFIG_ACPI) && !of_root)
+ kunit_skip(test, "arm64+acpi rejects overlays");
kunit_init_test(&fake, "fake test", NULL);
KUNIT_ASSERT_EQ(test, fake.status, KUNIT_SUCCESS);
Re: [PATCH v8 8/8] clk: Add KUnit tests for clks registered with struct clk_parent_data
Quoting Guenter Roeck (2024-10-03 17:25:37) > On 10/3/24 16:46, Stephen Boyd wrote: > [ ... ] > > That DT test has been there for a few releases. Is this the first time > > those tests have been run on arm64+acpi? I didn't try after sending the > > patches and forgot that the patch was dropped. > > > > Previously I had the affected tests disabled and never tracked down the > problem. > Since the problem is now spreading to additional tests, I finally tracked it > down, > that is all. Ok great. Good to know this isn't a new problem. Thanks for tracking it down. > > > How are you running kunit tests? I installed the qemu-efi-aarch64 debian > > package to get QEMU_EFI.fd but passing that to the kunit.py run command > > with --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd" didn't > > get me beyond the point that the EFI stub boots linux. I think the > > serial console must not be working and thus the kunit wrapper waits for > > something to show up but nothing ever does. I haven't dug any further > > though, so maybe you have a working command. > > > > I run all tests during boot, not from the command line. I also use the -bios > command but don't recall any issues with the console. I specify the > console on the qemu command line; depending on the qemu machine it is either > ttyS0 or ttyAMA0. The init script then finds and selects the active console. Can you please describe how you run the kunit test? And provide the qemu command you run to boot arm64 with acpi? > > > Here's my command that isn't working: > > > > ./tools/testing/kunit/kunit.py run --arch=arm64 --kunitconfig=drivers/of > > --qemu_args="-bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd" > > > > I can't really see what that command is actually doing ;-). It eventually runs this qemu command qemu-system-aarch64 -nodefaults -m 1024 -kernel .kunit/arch/arm64/boot/Image.gz -append 'kunit.enable=1 console=ttyAMA0 kunit_shutdown=reboot' -no-reboot -nographic -serial stdio -machine virt -cpu max,pauth-impdef=on -bios /usr/share/qemu-efi-aarch64/QEMU_EFI.fd I see that it fails because the architected timer isn't there after I add an earlycon=pl011,0x900 to the kernel commandline. I suspect passing a bios like this is incorrect, but I rarely run qemu manually so I don't know what I'm doing wrong. NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0 rcu: srcu_init: Setting srcu_struct sizes based on contention. timer_probe: no matching timers found Kernel panic - not syncing: Unable to initialise architected timer. CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc1-00261-g497f7c30f184 #203 Call trace: dump_backtrace+0x94/0xec show_stack+0x18/0x24 dump_stack_lvl+0x38/0x90 dump_stack+0x18/0x24 panic+0x36c/0x380 early_brk64+0x0/0xa8 start_kernel+0x27c/0x558 __primary_switched+0x80/0x88 ---[ end Kernel panic - not syncing: Unable to initialise architected timer. ]--- > > I'll just keep the affected tests disabled on arm64 for the time being. We should skip the tests on arm64+acpi, which is similar to disabling but not exactly the same. There will likely be more DT overlay usage in kunit and so that will lead to more test disabling. Skipping properly is the better solution.
Re: [PATCH v8 8/8] clk: Add KUnit tests for clks registered with struct clk_parent_data
Quoting Guenter Roeck (2024-09-28 14:32:35) > On 9/28/24 12:27, Shuah Khan wrote: > > On 9/28/24 11:54, Shuah Khan wrote: > >> On 9/28/24 11:31, Guenter Roeck wrote: > >>> On 9/27/24 17:08, Guenter Roeck wrote: > >>>> On 9/27/24 13:45, Shuah Khan wrote: > >>>>> On 9/27/24 10:19, Guenter Roeck wrote: > >>>>>> Copying devicetree maintainers. > >>>>>> > >>>>>> On Thu, Sep 26, 2024 at 09:39:38PM -0700, Guenter Roeck wrote: > >>>>>>> On Thu, Sep 26, 2024 at 09:14:11PM -0700, Guenter Roeck wrote: > >>>>>>>> Hi Stephen, > >>>>>>>> > >>>>>>>> On Thu, Jul 18, 2024 at 02:05:07PM -0700, Stephen Boyd wrote: > >>>>>>>>> Test that clks registered with 'struct clk_parent_data' work as > >>>>>>>>> intended and can find their parents. > >>>>>>>>> > >>>>>>>> > >>>>>>>> When testing this on arm64, I see the error below. The error is only > >>>>>>>> seen if I boot through efi, i.e., with "-bios QEMU_EFI-aarch64.fd" > >>>>>>>> qemu parameter. > >>>>>>>> > >>>>>>>> Any idea what might cause the problem ? > >>>>>>>> > >>>>>>> I noticed that the new overlay tests fail as well, also with "path > >>>>>>> '/' not > >>>>>>> found". > >>>>>>> > >>>>>>> [Maybe] answering my own question: I think the problem may be that > >>>>>>> there > >>>>>>> is no devicetree file and thus no devicetree root when booting through > >>>>>>> efi (in other words, of_root is NULL). Would it make sense to skip the > >>>>>>> tests in that case ? > >>>>>>> > >>>>>> > >>>>>> The problem is that of_root is not initialized in arm64 boots if ACPI > >>>>>> is enabled. > >>>>>> > >>>>>> From arch/arm64/kernel/setup.c:setup_arch(): > >>>>>> > >>>>>> if (acpi_disabled) > >>>>>> unflatten_device_tree(); // initializes of_root Oof I forgot that Rob didn't apply the patch that let an empty root live on ARM64 ACPI systems. See this thread[1] for all the details. > >>>>>> > >>>>>> ACPI is enabled if the system boots from EFI. This also affects > >>>>>> CONFIG_OF_KUNIT_TEST, which explicitly checks if of_root exists and > >>>>>> fails the test if it doesn't. > >>>>>> > >>>>>> I think those tests need to add a check for this condition, or affected > >>>>>> machines won't be able to run those unit tests. The obvious solution > >>>>>> would > >>>>>> be to check if of_root is set, but then the associated test case in > >>>>>> CONFIG_OF_KUNIT_TEST would not make sense. > >>>>>> > >>>>>> Any suggestions ? > >>>>>> I think that's the best we can do for now. Basically add a check like if (IS_ENABLED(CONFIG_ARM64) && acpi_disabled) kunit_skip(test, "ARM64 + ACPI rejects DT overlays"); to the overlay application function and the DT test. > >>>>> > >>>>> Would it work if these tests check if acpi_disabled and skip if it isn't > >>>>> disabled? It might be low overhead condition to check from these tests. > >>>>> > >>>>> acpi_disabled is exported: > >>>>> > >>>>> arch/arm64/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled); > >>>>> arch/loongarch/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled); > >>>>> arch/riscv/kernel/acpi.c:EXPORT_SYMBOL(acpi_disabled); > >>>>> arch/x86/kernel/acpi/boot.c:EXPORT_SYMBOL(acpi_disabled); > >>>>> > >>>> > >>>> I don't think that would work. Looking through the use of acpi_init, > >>>> I don't think that of_root is always NULL when acpi_init is false; that > >>>> just happens to be the case on arm64 when booting through efi. > >>>> However, even arm64 has the following code. > &
Re: [PATCH v2 2/5] dt-bindings: soc: qcom: smd-rpm: add generic compatibles
Quoting Dmitry Baryshkov (2024-07-29 12:52:15) > Add two generic compatibles to all smd-rpm devices, they follow the same > RPMSG protocol and are either accessed through the smd-edge or through > the glink-edge. > > Signed-off-by: Dmitry Baryshkov > --- Acked-by: Stephen Boyd
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-20 06:32:56) > On 4/20/24 00:24, Stephen Boyd wrote: > > Quoting Duje Mihanović (2024-04-19 07:31:14) > >> On Friday, April 12, 2024 4:57:09 AM GMT+2 Stephen Boyd wrote: > >>> Quoting Duje Mihanović (2024-04-11 03:15:34) > >>> > >>>> On 4/11/2024 10:00 AM, Stephen Boyd wrote: > >>>>> Is there a reason this file can't be a platform driver? > >>>> > >>>> Not that I know of, I did it like this only because the other in-tree > >>>> MMP clk drivers do so. I guess the initialization should look like any > >>>> of the qcom GCC drivers then? > >>> > >>> Yes. > >> > >> With the entire clock driver code in one file this is quite messy as I also > >> needed to add module_init and module_exit functions to (un)register each > >> platform driver, presumably because the module_platform_driver macro > >> doesn't > >> work with multiple platform drivers in one module. If I split up the driver > >> code for each clock controller block into its own file (such as clk-of- > >> pxa1908-apbc.c) as I believe is the best option, should the commits be > >> split > >> up accordingly as well? > > > > Sure. Why is 'of' in the name? Maybe that is unnecessary? > > That seems to be a historical leftover from when Marvell was just adding > DT support to the ARM32 MMP SoCs which Rob followed along with in the > PXA1928 clk driver and so have I. Should I drop it then as Marvell has > in the PXA1908 vendor kernel? > Sounds good to me.
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-19 07:31:14) > On Friday, April 12, 2024 4:57:09 AM GMT+2 Stephen Boyd wrote: > > Quoting Duje Mihanović (2024-04-11 03:15:34) > > > > > On 4/11/2024 10:00 AM, Stephen Boyd wrote: > > > > Is there a reason this file can't be a platform driver? > > > > > > Not that I know of, I did it like this only because the other in-tree > > > MMP clk drivers do so. I guess the initialization should look like any > > > of the qcom GCC drivers then? > > > > Yes. > > With the entire clock driver code in one file this is quite messy as I also > needed to add module_init and module_exit functions to (un)register each > platform driver, presumably because the module_platform_driver macro doesn't > work with multiple platform drivers in one module. If I split up the driver > code for each clock controller block into its own file (such as clk-of- > pxa1908-apbc.c) as I believe is the best option, should the commits be split > up accordingly as well? Sure. Why is 'of' in the name? Maybe that is unnecessary? > > > > While at it, do you think the other MMP clk drivers could use a > conversion? > > > > I'm a little wary if the conversion cannot be tested though. > > I'd rather leave it to someone with the hardware then, especially since the > only reason I found out about the above is that the board I'm working on > failed to boot completely without the module_init function. > Ok, sounds fine.
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-11 03:15:34) > On 4/11/2024 10:00 AM, Stephen Boyd wrote: > > > > Is there a reason this file can't be a platform driver? > > Not that I know of, I did it like this only because the other in-tree > MMP clk drivers do so. I guess the initialization should look like any > of the qcom GCC drivers then? Yes. > > While at it, do you think the other MMP clk drivers could use a conversion? > Sure, go for it. I'm a little wary if the conversion cannot be tested though. It doesn't hurt that other drivers haven't been converted.
Re: [PATCH v9 5/9] clk: mmp: Add Marvell PXA1908 clock driver
Quoting Duje Mihanović (2024-04-02 13:55:41)
> diff --git a/drivers/clk/mmp/clk-of-pxa1908.c
> b/drivers/clk/mmp/clk-of-pxa1908.c
> new file mode 100644
> index ..6f1f6e25a718
> --- /dev/null
> +++ b/drivers/clk/mmp/clk-of-pxa1908.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0-only
[...]
> +static void __init pxa1908_apbc_clk_init(struct device_node *np)
> +{
> + struct pxa1908_clk_unit *pxa_unit;
> +
> + pxa_unit = kzalloc(sizeof(*pxa_unit), GFP_KERNEL);
> + if (!pxa_unit)
> + return;
> +
> + pxa_unit->apbc_base = of_iomap(np, 0);
> + if (!pxa_unit->apbc_base) {
> + pr_err("failed to map apbc registers\n");
> + kfree(pxa_unit);
> + return;
> + }
> +
> + mmp_clk_init(np, &pxa_unit->unit, APBC_NR_CLKS);
> +
> + pxa1908_apb_periph_clk_init(pxa_unit);
> +}
> +CLK_OF_DECLARE(pxa1908_apbc, "marvell,pxa1908-apbc", pxa1908_apbc_clk_init);
Is there a reason this file can't be a platform driver?
Re: [PATCH v9 4/9] dt-bindings: clock: Add Marvell PXA1908 clock bindings
Quoting Duje Mihanović (2024-04-02 13:55:40) > Add dt bindings and documentation for the Marvell PXA1908 clock > controller. > > Reviewed-by: Conor Dooley > Signed-off-by: Duje Mihanović > --- Reviewed-by: Stephen Boyd
Re: [PATCH v9 1/9] clk: mmp: Switch to use struct u32_fract instead of custom one
Quoting Duje Mihanović (2024-04-02 13:55:37) > From: Andy Shevchenko > > The struct mmp_clk_factor_tbl repeats the generic struct u32_fract. > Kill the custom one and use the generic one instead. > > Signed-off-by: Andy Shevchenko > Tested-by: Duje Mihanović > Reviewed-by: Linus Walleij > Signed-off-by: Duje Mihanović > --- Reviewed-by: Stephen Boyd > > diff --git a/drivers/clk/mmp/clk.h b/drivers/clk/mmp/clk.h > index 55ac05379781..c83cec169ddc 100644 > --- a/drivers/clk/mmp/clk.h > +++ b/drivers/clk/mmp/clk.h > @@ -3,6 +3,7 @@ > #define __MACH_MMP_CLK_H > > #include > +#include > #include > #include > This clkdev include should be dropped in another patch.
Re: [PATCH 1/3] dt-bindings: clock: keystone: remove unstable remark
Quoting Krzysztof Kozlowski (2024-02-24 01:12:34)
> Keystone clock controller bindings were marked as work-in-progress /
> unstable in 2013 in commit b9e0d40c0d83 ("clk: keystone: add Keystone
> PLL clock driver") and commit 7affe5685c96 ("clk: keystone: Add gate
> control clock driver") Almost eleven years is enough, so drop the
> "unstable" remark and expect usual ABI rules.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
Acked-by: Stephen Boyd
Re: [PATCH 2/3] dt-bindings: clock: ti: remove unstable remark
+Tony
Quoting Krzysztof Kozlowski (2024-02-24 01:12:35)
> Several TI SoC clock bindings were marked as work-in-progress / unstable
> between 2013-2016, for example in commit f60b1ea5ea7a ("CLK: TI: add
> support for gate clock"). It was enough of time to consider them stable
> and expect usual ABI rules.
>
> Signed-off-by: Krzysztof Kozlowski
> ---
Acked-by: Stephen Boyd
> Documentation/devicetree/bindings/clock/ti/adpll.txt| 2 --
> Documentation/devicetree/bindings/clock/ti/apll.txt | 2 --
> Documentation/devicetree/bindings/clock/ti/autoidle.txt | 2 --
> Documentation/devicetree/bindings/clock/ti/clockdomain.txt | 2 --
> Documentation/devicetree/bindings/clock/ti/composite.txt| 2 --
> Documentation/devicetree/bindings/clock/ti/divider.txt | 2 --
> Documentation/devicetree/bindings/clock/ti/dpll.txt | 2 --
> Documentation/devicetree/bindings/clock/ti/fapll.txt| 2 --
> .../devicetree/bindings/clock/ti/fixed-factor-clock.txt | 2 --
> Documentation/devicetree/bindings/clock/ti/gate.txt | 2 --
> Documentation/devicetree/bindings/clock/ti/interface.txt| 2 --
> Documentation/devicetree/bindings/clock/ti/mux.txt | 2 --
> 12 files changed, 24 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/clock/ti/adpll.txt
> b/Documentation/devicetree/bindings/clock/ti/adpll.txt
> index 4c8a2ce2cd70..3122360adcf3 100644
> --- a/Documentation/devicetree/bindings/clock/ti/adpll.txt
> +++ b/Documentation/devicetree/bindings/clock/ti/adpll.txt
> @@ -1,7 +1,5 @@
> Binding for Texas Instruments ADPLL clock.
>
> -Binding status: Unstable - ABI compatibility may be broken in the future
> -
> This binding uses the common clock binding[1]. It assumes a
> register-mapped ADPLL with two to three selectable input clocks
> and three to four children.
> diff --git a/Documentation/devicetree/bindings/clock/ti/apll.txt
> b/Documentation/devicetree/bindings/clock/ti/apll.txt
> index ade4dd4c30f0..bbd505c1199d 100644
> --- a/Documentation/devicetree/bindings/clock/ti/apll.txt
> +++ b/Documentation/devicetree/bindings/clock/ti/apll.txt
> @@ -1,7 +1,5 @@
> Binding for Texas Instruments APLL clock.
>
> -Binding status: Unstable - ABI compatibility may be broken in the future
> -
> This binding uses the common clock binding[1]. It assumes a
> register-mapped APLL with usually two selectable input clocks
> (reference clock and bypass clock), with analog phase locked
> diff --git a/Documentation/devicetree/bindings/clock/ti/autoidle.txt
> b/Documentation/devicetree/bindings/clock/ti/autoidle.txt
> index 7c735dde9fe9..05645a10a9e3 100644
> --- a/Documentation/devicetree/bindings/clock/ti/autoidle.txt
> +++ b/Documentation/devicetree/bindings/clock/ti/autoidle.txt
> @@ -1,7 +1,5 @@
> Binding for Texas Instruments autoidle clock.
>
> -Binding status: Unstable - ABI compatibility may be broken in the future
> -
> This binding uses the common clock binding[1]. It assumes a register mapped
> clock which can be put to idle automatically by hardware based on the usage
> and a configuration bit setting. Autoidle clock is never an individual
> diff --git a/Documentation/devicetree/bindings/clock/ti/clockdomain.txt
> b/Documentation/devicetree/bindings/clock/ti/clockdomain.txt
> index 9c6199249ce5..edf0b5d42768 100644
> --- a/Documentation/devicetree/bindings/clock/ti/clockdomain.txt
> +++ b/Documentation/devicetree/bindings/clock/ti/clockdomain.txt
> @@ -1,7 +1,5 @@
> Binding for Texas Instruments clockdomain.
>
> -Binding status: Unstable - ABI compatibility may be broken in the future
> -
> This binding uses the common clock binding[1] in consumer role.
> Every clock on TI SoC belongs to one clockdomain, but software
> only needs this information for specific clocks which require
> diff --git a/Documentation/devicetree/bindings/clock/ti/composite.txt
> b/Documentation/devicetree/bindings/clock/ti/composite.txt
> index 33ac7c9ad053..6f7e1331b546 100644
> --- a/Documentation/devicetree/bindings/clock/ti/composite.txt
> +++ b/Documentation/devicetree/bindings/clock/ti/composite.txt
> @@ -1,7 +1,5 @@
> Binding for TI composite clock.
>
> -Binding status: Unstable - ABI compatibility may be broken in the future
> -
> This binding uses the common clock binding[1]. It assumes a
> register-mapped composite clock with multiple different sub-types;
>
> diff --git a/Documentation/devicetree/bindings/clock/ti/divider.txt
> b/Documentation/devicetree/bindings/clock/ti/divider.txt
> index 9b13b32974f9..4d7c76f0b356 100644
> --- a/Documentation/devicetree/bindings/clock/ti/divider.txt
> +++ b/Documentat
Re: [PATCH] dt-bindings: correct white-spaces in examples
Quoting Krzysztof Kozlowski (2023-11-24 01:21:21) > Use only one and exactly one space around '=' in DTS example. > > Signed-off-by: Krzysztof Kozlowski > > --- Acked-by: Stephen Boyd
Re: [PATCH v3 3/3] drm/msm/dp: check main link status before start aux read
Quoting Kuogee Hsieh (2021-04-16 10:38:51)
> Maybe when the cable is disconnected the DP phy should be shutdown and
> some bit in the phy could effectively "cut off" the aux channel and then
> NAKs would start coming through here in the DP controller I/O register
> space. This patch have DP aux channel read/write to return NAK immediately
> if DP controller connection status is in unplugged state.
>
> Changes in V3:
> -- check core_initialized before handle irq_hpd
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 5 +
> drivers/gpu/drm/msm/dp/dp_display.c | 14 ++
> drivers/gpu/drm/msm/dp/dp_link.c| 20 +++-
> 3 files changed, 30 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 7c22bfe..fae3806 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>
> mutex_lock(&aux->mutex);
>
> + if (!dp_catalog_link_is_connected(aux->catalog)) {
> + ret = -ETIMEDOUT;
> + goto unlock_exit;
> + }
> +
This still makes me concerned. Any possibility to not do this and have
the phy cut the connection off and have this transfer timeout
immediately?
> aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
> DP_AUX_NATIVE_READ);
>
> /* Ignore address only message */
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 1784e11..db3f45e 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -571,7 +571,7 @@ static int dp_hpd_plug_handle(struct dp_display_private
> *dp, u32 data)
> dp->hpd_state = ST_DISCONNECTED;
>
> if (ret == -ECONNRESET) { /* cable unplugged */
> - dp->core_initialized = false;
> + DRM_ERROR("dongle unplugged = %d\n", ret);
Is this a debug message?
> }
>
> } else {
> @@ -711,9 +711,15 @@ static int dp_irq_hpd_handle(struct dp_display_private
> *dp, u32 data)
> return 0;
> }
>
> - ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> - if (ret == -ECONNRESET) { /* cable unplugged */
> - dp->core_initialized = false;
> + /*
> +* dp core (ahb/aux clks) must be initialized before
> +* irq_hpd be handled
> +*/
> + if (dp->core_initialized) {
> + ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> + if (ret == -ECONNRESET) { /* cable unplugged */
> + DRM_ERROR("dongle unplugged = %d\n", ret);
Another debug message?
> + }
> }
>
> mutex_unlock(&dp->event_mutex);
> diff --git a/drivers/gpu/drm/msm/dp/dp_link.c
> b/drivers/gpu/drm/msm/dp/dp_link.c
> index be986da..53ecae6 100644
> --- a/drivers/gpu/drm/msm/dp/dp_link.c
> +++ b/drivers/gpu/drm/msm/dp/dp_link.c
> @@ -737,18 +737,25 @@ static int dp_link_parse_sink_count(struct dp_link
> *dp_link)
> return 0;
> }
>
> -static void dp_link_parse_sink_status_field(struct dp_link_private *link)
> +static int dp_link_parse_sink_status_field(struct dp_link_private *link)
> {
> int len = 0;
>
> link->prev_sink_count = link->dp_link.sink_count;
> - dp_link_parse_sink_count(&link->dp_link);
> + len = dp_link_parse_sink_count(&link->dp_link);
> + if (len < 0) {
> + DRM_ERROR("DP parse sink count failed\n");
> + return len;
> + }
>
> len = drm_dp_dpcd_read_link_status(link->aux,
> link->link_status);
> - if (len < DP_LINK_STATUS_SIZE)
> + if (len < DP_LINK_STATUS_SIZE) {
> DRM_ERROR("DP link status read failed\n");
> - dp_link_parse_request(link);
> + return len;
> + }
> +
> + return dp_link_parse_request(link);
> }
>
> /**
> @@ -1032,7 +1039,10 @@ int dp_link_process_request(struct dp_link *dp_link)
>
> dp_link_reset_data(link);
>
> - dp_link_parse_sink_status_field(link);
> + ret = dp_link_parse_sink_status_field(link);
> + if (ret) {
> + return ret;
> + }
>
> if (link->request.test_requested == DP_TEST_LINK_EDID_READ) {
> dp_link->sink_request |= DP_TEST_LINK_EDID_READ;
> --
Can you split this part off into another patch? It seems to stand on its
own as it makes the code more robust to transfer errors in the sink
parsing code.
Re: [PATCH 1/2] drm/msm/dp: service only one irq_hpd if there are multiple irq_hpd pending
Quoting Kuogee Hsieh (2021-04-16 13:27:57)
> Some dongle may generate more than one irq_hpd events in a short period of
> time. This patch will treat those irq_hpd events as single one and service
> only one irq_hpd event.
Why is it bad to get multiple irq_hpd events in a short period of time?
Please tell us here in the commit text.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 9 +
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5a39da6..0a7d383 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -707,6 +707,9 @@ static int dp_irq_hpd_handle(struct dp_display_private
> *dp, u32 data)
> return 0;
> }
>
> + /* only handle first irq_hpd in case of multiple irs_hpd pending */
> + dp_del_event(dp, EV_IRQ_HPD_INT);
> +
> ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> if (ret == -ECONNRESET) { /* cable unplugged */
> dp->core_initialized = false;
> @@ -1300,6 +1303,9 @@ static int dp_pm_suspend(struct device *dev)
> /* host_init will be called at pm_resume */
> dp->core_initialized = false;
>
> + /* system suspended, delete pending irq_hdps */
> + dp_del_event(dp, EV_IRQ_HPD_INT);
What happens if I suspend my device and when this function is running I
toggle my monitor to use the HDMI input that is connected instead of some
other input, maybe the second HDMI input? Wouldn't that generate an HPD
interrupt to grab the attention of this device?
> +
> mutex_unlock(&dp->event_mutex);
>
> return 0;
> @@ -1496,6 +1502,9 @@ int msm_dp_display_disable(struct msm_dp *dp, struct
> drm_encoder *encoder)
> /* stop sentinel checking */
> dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
>
> + /* link is down, delete pending irq_hdps */
> + dp_del_event(dp_display, EV_IRQ_HPD_INT);
> +
I'm becoming convinced that the whole kthread design and event queue is
broken. These sorts of patches are working around the larger problem
that the kthread is running independently of the driver and irqs can
come in at any time but the event queue is not checked from the irq
handler to debounce the irq event. Is the event queue necessary at all?
I wonder if it would be simpler to just use an irq thread and process
the hpd signal from there. Then we're guaranteed to not get an irq again
until the irq thread is done processing the event. This would naturally
debounce the irq hpd event that way.
> dp_display_disable(dp_display, 0);
>
> rc = dp_display_unprepare(dp);
[PATCH v5 13/13] kdump: Use vmlinux_build_id to simplify
We can use the vmlinux_build_id array here now instead of open coding
it. This mostly consolidates code.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Dave Young
Cc: Baoquan He
Cc: Vivek Goyal
Cc:
Signed-off-by: Stephen Boyd
---
include/linux/crash_core.h | 12 -
kernel/crash_core.c| 50 ++
2 files changed, 8 insertions(+), 54 deletions(-)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 206bde8308b2..de62a722431e 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -38,8 +38,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
#define VMCOREINFO_OSRELEASE(value) \
vmcoreinfo_append_str("OSRELEASE=%s\n", value)
-#define VMCOREINFO_BUILD_ID(value) \
- vmcoreinfo_append_str("BUILD-ID=%s\n", value)
+#define VMCOREINFO_BUILD_ID() \
+ ({ \
+ static_assert(sizeof(vmlinux_build_id) == 20); \
+ vmcoreinfo_append_str("BUILD-ID=%20phN\n", vmlinux_build_id); \
+ })
+
#define VMCOREINFO_PAGESIZE(value) \
vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
#define VMCOREINFO_SYMBOL(name) \
@@ -69,10 +73,6 @@ extern unsigned char *vmcoreinfo_data;
extern size_t vmcoreinfo_size;
extern u32 *vmcoreinfo_note;
-/* raw contents of kernel .notes section */
-extern const void __start_notes __weak;
-extern const void __stop_notes __weak;
-
Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
void *data, size_t data_len);
void final_note(Elf_Word *buf);
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 825284baaf46..29cc15398ee4 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -4,6 +4,7 @@
* Copyright (C) 2002-2004 Eric Biederman
*/
+#include
#include
#include
#include
@@ -378,53 +379,6 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
}
EXPORT_SYMBOL(paddr_vmcoreinfo_note);
-#define NOTES_SIZE (&__stop_notes - &__start_notes)
-#define BUILD_ID_MAX SHA1_DIGEST_SIZE
-#define NT_GNU_BUILD_ID 3
-
-struct elf_note_section {
- struct elf_note n_hdr;
- u8 n_data[];
-};
-
-/*
- * Add build ID from .notes section as generated by the GNU ld(1)
- * or LLVM lld(1) --build-id option.
- */
-static void add_build_id_vmcoreinfo(void)
-{
- char build_id[BUILD_ID_MAX * 2 + 1];
- int n_remain = NOTES_SIZE;
-
- while (n_remain >= sizeof(struct elf_note)) {
- const struct elf_note_section *note_sec =
- &__start_notes + NOTES_SIZE - n_remain;
- const u32 n_namesz = note_sec->n_hdr.n_namesz;
-
- if (note_sec->n_hdr.n_type == NT_GNU_BUILD_ID &&
- n_namesz != 0 &&
- !strcmp((char *)¬e_sec->n_data[0], "GNU")) {
- if (note_sec->n_hdr.n_descsz <= BUILD_ID_MAX) {
- const u32 n_descsz = note_sec->n_hdr.n_descsz;
- const u8 *s = ¬e_sec->n_data[n_namesz];
-
- s = PTR_ALIGN(s, 4);
- bin2hex(build_id, s, n_descsz);
- build_id[2 * n_descsz] = '\0';
- VMCOREINFO_BUILD_ID(build_id);
- return;
- }
- pr_warn("Build ID is too large to include in
vmcoreinfo: %u > %u\n",
- note_sec->n_hdr.n_descsz,
- BUILD_ID_MAX);
- return;
- }
- n_remain -= sizeof(struct elf_note) +
- ALIGN(note_sec->n_hdr.n_namesz, 4) +
- ALIGN(note_sec->n_hdr.n_descsz, 4);
- }
-}
-
static int __init crash_save_vmcoreinfo_init(void)
{
vmcoreinfo_data = (unsigned char *)get_zeroed_page(GFP_KERNEL);
@@ -443,7 +397,7 @@ static int __init crash_save_vmcoreinfo_init(void)
}
VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
- add_build_id_vmcoreinfo();
+ VMCOREINFO_BUILD_ID();
VMCOREINFO_PAGESIZE(PAGE_SIZE);
VMCOREINFO_SYMBOL(init_uts_ns);
--
https://chromeos.dev
[PATCH v5 09/13] scripts/decode_stacktrace.sh: Silence stderr messages from addr2line/nm
Sometimes if you're using tools that have linked things improperly or
have new features/sections that older tools don't expect you'll see
warnings printed to stderr. We don't really care about these warnings,
so let's just silence these messages to cleanup output of this script.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Konstantin Khlebnikov
Cc: Sasha Levin
Signed-off-by: Stephen Boyd
---
scripts/decode_stacktrace.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index ca21f8bdf5f2..20b5af1ebe5e 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -74,7 +74,7 @@ find_module() {
find_module && return
if [[ $release == "" ]] ; then
- release=$(gdb -ex 'print init_uts_ns.name.release' -ex 'quit'
-quiet -batch "$vmlinux" | sed -n 's/\$1 = "\(.*\)".*/\1/p')
+ release=$(gdb -ex 'print init_uts_ns.name.release' -ex 'quit'
-quiet -batch "$vmlinux" 2>/dev/null | sed -n 's/\$1 = "\(.*\)".*/\1/p')
fi
for dn in {/usr/lib/debug,}/lib/modules/$release ; do
@@ -128,7 +128,7 @@ parse_symbol() {
if [[ "${cache[$module,$name]+isset}" == "isset" ]]; then
local base_addr=${cache[$module,$name]}
else
- local base_addr=$(nm "$objfile" | awk '$3 == "'$name'" && ($2
== "t" || $2 == "T") {print $1; exit}')
+ local base_addr=$(nm "$objfile" 2>/dev/null | awk '$3 ==
"'$name'" && ($2 == "t" || $2 == "T") {print $1; exit}')
if [[ $base_addr == "" ]] ; then
# address not found
return
@@ -152,7 +152,7 @@ parse_symbol() {
if [[ "${cache[$module,$address]+isset}" == "isset" ]]; then
local code=${cache[$module,$address]}
else
- local code=$(${CROSS_COMPILE}addr2line -i -e "$objfile"
"$address")
+ local code=$(${CROSS_COMPILE}addr2line -i -e "$objfile"
"$address" 2>/dev/null)
cache[$module,$address]=$code
fi
--
https://chromeos.dev
[PATCH v5 12/13] buildid: Fix kernel-doc notation
Kernel doc should use "Return:" instead of "Returns" to properly reflect the return values. Cc: Jiri Olsa Cc: Alexei Starovoitov Cc: Jessica Yu Cc: Evan Green Cc: Hsin-Yi Wang Signed-off-by: Stephen Boyd --- lib/buildid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/buildid.c b/lib/buildid.c index a7edab4914e6..dfc62625cae4 100644 --- a/lib/buildid.c +++ b/lib/buildid.c @@ -121,7 +121,7 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id, * @build_id: buffer to store build id, at least BUILD_ID_SIZE long * @size: returns actual build id size in case of success * - * Returns 0 on success, otherwise error (< 0). + * Return: 0 on success, -EINVAL otherwise */ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size) -- https://chromeos.dev
[PATCH v5 10/13] scripts/decode_stacktrace.sh: Indicate 'auto' can be used for base path
Add "auto" to the usage message so that it's a little clearer that you
can pass "auto" as the second argument. When passing "auto" the script
tries to find the base path automatically instead of requiring it be
passed on the commandline. Also use [] to indicate the
variable argument and that it is optional so that we can differentiate
from the literal "auto" that should be passed.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Konstantin Khlebnikov
Cc: Sasha Levin
Signed-off-by: Stephen Boyd
---
scripts/decode_stacktrace.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index 20b5af1ebe5e..5fbad61fe490 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -5,7 +5,7 @@
usage() {
echo "Usage:"
- echo " $0 -r | [base path] [modules path]"
+ echo " $0 -r | [|auto] []"
}
if [[ $1 == "-r" ]] ; then
--
https://chromeos.dev
[PATCH v5 08/13] scripts/decode_stacktrace.sh: Support debuginfod
Now that stacktraces contain the build ID information we can update this
script to use debuginfod-find to locate the debuginfo for the vmlinux
and modules automatically. This can replace the existing code that
requires specifying a path to vmlinux or tries to find the vmlinux and
modules automatically by using the release number. Work it into the
script as a fallback option if the vmlinux isn't specified on the
commandline.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Konstantin Khlebnikov
Cc: Sasha Levin
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Signed-off-by: Stephen Boyd
---
scripts/decode_stacktrace.sh | 81 +++-
1 file changed, 70 insertions(+), 11 deletions(-)
diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index 90398347e366..ca21f8bdf5f2 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -3,11 +3,10 @@
# (c) 2014, Sasha Levin
#set -x
-if [[ $# < 1 ]]; then
+usage() {
echo "Usage:"
echo " $0 -r | [base path] [modules path]"
- exit 1
-fi
+}
if [[ $1 == "-r" ]] ; then
vmlinux=""
@@ -24,6 +23,7 @@ if [[ $1 == "-r" ]] ; then
if [[ $vmlinux == "" ]] ; then
echo "ERROR! vmlinux image for release $release is not found"
>&2
+ usage
exit 2
fi
else
@@ -31,12 +31,35 @@ else
basepath=${2-auto}
modpath=$3
release=""
+ debuginfod=
+
+ # Can we use debuginfod-find?
+ if type debuginfod-find >/dev/null 2>&1 ; then
+ debuginfod=${1-only}
+ fi
+
+ if [[ $vmlinux == "" && -z $debuginfod ]] ; then
+ echo "ERROR! vmlinux image must be specified" >&2
+ usage
+ exit 1
+ fi
fi
declare -A cache
declare -A modcache
find_module() {
+ if [[ -n $debuginfod ]] ; then
+ if [[ -n $modbuildid ]] ; then
+ debuginfod-find debuginfo $modbuildid && return
+ fi
+
+ # Only using debuginfod so don't try to find vmlinux module path
+ if [[ $debuginfod == "only" ]] ; then
+ return
+ fi
+ fi
+
if [[ "$modpath" != "" ]] ; then
for fn in $(find "$modpath" -name "${module//_/[-_]}.ko*") ; do
if readelf -WS "$fn" | grep -qwF .debug_line ; then
@@ -150,6 +173,27 @@ parse_symbol() {
symbol="$segment$name ($code)"
}
+debuginfod_get_vmlinux() {
+ local vmlinux_buildid=${1##* }
+
+ if [[ $vmlinux != "" ]]; then
+ return
+ fi
+
+ if [[ $vmlinux_buildid =~ ^[0-9a-f]+ ]]; then
+ vmlinux=$(debuginfod-find debuginfo $vmlinux_buildid)
+ if [[ $? -ne 0 ]] ; then
+ echo "ERROR! vmlinux image not found via
debuginfod-find" >&2
+ usage
+ exit 2
+ fi
+ return
+ fi
+ echo "ERROR! Build ID for vmlinux not found. Try passing -r or
specifying vmlinux" >&2
+ usage
+ exit 2
+}
+
decode_code() {
local scripts=`dirname "${BASH_SOURCE[0]}"`
@@ -157,6 +201,14 @@ decode_code() {
}
handle_line() {
+ if [[ $basepath == "auto" && $vmlinux != "" ]] ; then
+ module=""
+ symbol="kernel_init+0x0/0x0"
+ parse_symbol
+ basepath=${symbol#kernel_init (}
+ basepath=${basepath%/init/main.c:*)}
+ fi
+
local words
# Tokenize
@@ -182,16 +234,28 @@ handle_line() {
fi
done
+ if [[ ${words[$last]} =~ ^[0-9a-f]+\] ]]; then
+ words[$last-1]="${words[$last-1]} ${words[$last]}"
+ unset words[$last]
+ last=$(( $last - 1 ))
+ fi
+
if [[ ${words[$last]} =~ \[([^]]+)\] ]]; then
module=${words[$last]}
module=${module#\[}
module=${module%\]}
+ modbuildid=${module#* }
+ module=${module% *}
+ if [[ $modbuildid == $module ]]; then
+ modbuildid=
+ fi
symbol=${words[$last-1]}
unset words[$last-1]
else
# The symbol is the last element, process it
symbol=${words[$last]}
module=
+ modbuildid=
fi
unset words[$last]
@@ -201,14 +265,6 @@ handle_line() {
echo &
[PATCH v5 11/13] buildid: Mark some arguments const
These arguments are never modified so they can be marked const to
indicate as such.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Signed-off-by: Stephen Boyd
---
lib/buildid.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/buildid.c b/lib/buildid.c
index 530dbd1f6bbe..a7edab4914e6 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -48,10 +48,10 @@ static int parse_build_id_buf(unsigned char *build_id,
return -EINVAL;
}
-static inline int parse_build_id(void *page_addr,
+static inline int parse_build_id(const void *page_addr,
unsigned char *build_id,
__u32 *size,
-void *note_start,
+const void *note_start,
Elf32_Word note_size)
{
/* check for overflow */
@@ -66,7 +66,7 @@ static inline int parse_build_id(void *page_addr,
}
/* Parse build ID from 32-bit ELF */
-static int get_build_id_32(void *page_addr, unsigned char *build_id,
+static int get_build_id_32(const void *page_addr, unsigned char *build_id,
__u32 *size)
{
Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
@@ -91,7 +91,7 @@ static int get_build_id_32(void *page_addr, unsigned char
*build_id,
}
/* Parse build ID from 64-bit ELF */
-static int get_build_id_64(void *page_addr, unsigned char *build_id,
+static int get_build_id_64(const void *page_addr, unsigned char *build_id,
__u32 *size)
{
Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
--
https://chromeos.dev
[PATCH v5 07/13] x86/dumpstack: Use %pSb/%pBb for backtrace printing
Let's use the new printk formats to print the stacktrace entries when
printing a backtrace to the kernel logs. This will include any module's
build ID[1] in it so that offline/crash debugging can easily locate the
debuginfo for a module via something like debuginfod[2].
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc:
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1]
Link: https://sourceware.org/elfutils/Debuginfod.html [2]
Signed-off-by: Stephen Boyd
---
arch/x86/kernel/dumpstack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 299c20f0a38b..ea4fe192189d 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -69,7 +69,7 @@ static void printk_stack_address(unsigned long address, int
reliable,
const char *log_lvl)
{
touch_nmi_watchdog();
- printk("%s %s%pB\n", log_lvl, reliable ? "" : "? ", (void *)address);
+ printk("%s %s%pBb\n", log_lvl, reliable ? "" : "? ", (void *)address);
}
static int copy_code(struct pt_regs *regs, u8 *buf, unsigned long src,
--
https://chromeos.dev
[PATCH v5 05/13] module: Add printk formats to add module build ID to stacktraces
Let's make kernel stacktraces easier to identify by including the build ID[1] of a module if the stacktrace is printing a symbol from a module. This makes it simpler for developers to locate a kernel module's full debuginfo for a particular stacktrace. Combined with scripts/decode_stracktrace.sh, a developer can download the matching debuginfo from a debuginfod[2] server and find the exact file and line number for the functions plus offsets in a stacktrace that match the module. This is especially useful for pstore crash debugging where the kernel crashes are recorded in something like console-ramoops and the recovery kernel/modules are different or the debuginfo doesn't exist on the device due to space concerns (the debuginfo can be too large for space limited devices). Originally, I put this on the %pS format, but that was quickly rejected given that %pS is used in other places such as ftrace where build IDs aren't meaningful. There was some discussions on the list to put every module build ID into the "Modules linked in:" section of the stacktrace message but that quickly becomes very hard to read once you have more than three or four modules linked in. It also provides too much information when we don't expect each module to be traversed in a stacktrace. Having the build ID for modules that aren't important just makes things messy. Splitting it to multiple lines for each module quickly explodes the number of lines printed in an oops too, possibly wrapping the warning off the console. And finally, trying to stash away each module used in a callstack to provide the ID of each symbol printed is cumbersome and would require changes to each architecture to stash away modules and return their build IDs once unwinding has completed. Instead, we opt for the simpler approach of introducing new printk formats '%pS[R]b' for "pointer symbolic backtrace with module build ID" and '%pBb' for "pointer backtrace with module build ID" and then updating the few places in the architecture layer where the stacktrace is printed to use this new format. Example: WARNING: CPU: 3 PID: 3373 at drivers/misc/lkdtm/bugs.c:83 lkdtm_WARNING+0x28/0x30 [lkdtm] Modules linked in: lkdtm rfcomm algif_hash algif_skcipher af_alg xt_cgroup uinput xt_MASQUERADE hci_uart CPU: 3 PID: 3373 Comm: bash Not tainted 5.11 #12 a8c0d47f7051f3e6670ceaea724af66a39c6cec8 Hardware name: Google Lazor (rev3+) with KB Backlight (DT) pstate: 0049 (nzcv daif +PAN -UAO -TCO BTYPE=--) pc : lkdtm_WARNING+0x28/0x30 [lkdtm] lr : lkdtm_do_action+0x24/0x40 [lkdtm] sp : ffc013febca0 x29: ffc013febca0 x28: ff88d9438040 x27: x26: x25: x24: ffdd0e9772c0 x23: 0020 x22: ffdd0e975366 x21: ffdd0e9772e0 x20: ffc013febde0 x19: 0008 x18: x17: x16: 0037 x15: ffdd102ab174 x14: 0003 x13: 0004 x12: x11: x10: x9 : 0001 x8 : ffdd0e979000 x7 : x6 : ffdd10ff6b54 x5 : x4 : x3 : ffc013feb938 x2 : ff89fef05a70 x1 : ff89feef5788 x0 : ffdd0e9772e0 Call trace: lkdtm_WARNING+0x28/0x30 [lkdtm 6c2215028606bda50de823490723dc4bc5bf46f9] direct_entry+0x16c/0x1b4 [lkdtm 6c2215028606bda50de823490723dc4bc5bf46f9] full_proxy_write+0x74/0xa4 vfs_write+0xec/0x2e8 ksys_write+0x84/0xf0 __arm64_sys_write+0x24/0x30 el0_svc_common+0xf4/0x1c0 do_el0_svc_compat+0x28/0x3c el0_svc_compat+0x10/0x1c el0_sync_compat_handler+0xa8/0xcc el0_sync_compat+0x178/0x180 ---[ end trace f89bc7f5417cbcc6 ]--- Cc: Jiri Olsa Cc: Alexei Starovoitov Cc: Jessica Yu Cc: Evan Green Cc: Hsin-Yi Wang Cc: Petr Mladek Cc: Steven Rostedt Cc: Sergey Senozhatsky Cc: Andy Shevchenko Cc: Rasmus Villemoes Cc: Cc: Matthew Wilcox Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1] Link: https://sourceware.org/elfutils/Debuginfod.html [2] Signed-off-by: Stephen Boyd --- Documentation/core-api/printk-formats.rst | 11 +++ include/linux/kallsyms.h | 20 - include/linux/module.h| 8 +- kernel/kallsyms.c | 101 +- kernel/module.c | 31 ++- lib/vsprintf.c| 8 +- 6 files changed, 154 insertions(+), 25 deletions(-) diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 160e710d992f..5f60533f2a56 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -114,6 +114,17 @@ used when printing stack backtraces. The specifier takes into consideration the effect of compiler optimisations which may occur when tail-calls are used and m
[PATCH v5 06/13] arm64: stacktrace: Use %pSb for backtrace printing
Let's use the new printk format to print the stacktrace entry when
printing a backtrace to the kernel logs. This will include any module's
build ID[1] in it so that offline/crash debugging can easily locate the
debuginfo for a module via something like debuginfod[2].
Cc: Catalin Marinas
Cc: Will Deacon
Cc:
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1]
Link: https://sourceware.org/elfutils/Debuginfod.html [2]
Signed-off-by: Stephen Boyd
---
arch/arm64/kernel/stacktrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ad20981dfda4..9d38da01ff98 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -129,7 +129,7 @@ NOKPROBE_SYMBOL(walk_stackframe);
static void dump_backtrace_entry(unsigned long where, const char *loglvl)
{
- printk("%s %pS\n", loglvl, (void *)where);
+ printk("%s %pSb\n", loglvl, (void *)where);
}
void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
--
https://chromeos.dev
[PATCH v5 04/13] dump_stack: Add vmlinux build ID to stack traces
Add the running kernel's build ID[1] to the stacktrace information
header. This makes it simpler for developers to locate the vmlinux with
full debuginfo for a particular kernel stacktrace. Combined with
scripts/decode_stracktrace.sh, a developer can download the correct
vmlinux from a debuginfod[2] server and find the exact file and line
number for the functions plus offsets in a stacktrace.
This is especially useful for pstore crash debugging where the kernel
crashes are recorded in the pstore logs and the recovery kernel is
different or the debuginfo doesn't exist on the device due to space
concerns (the data can be large and a security concern). The stacktrace
can be analyzed after the crash by using the build ID to find the
matching vmlinux and understand where in the function something went
wrong.
Example stacktrace from lkdtm:
WARNING: CPU: 4 PID: 3255 at drivers/misc/lkdtm/bugs.c:83
lkdtm_WARNING+0x28/0x30 [lkdtm]
Modules linked in: lkdtm rfcomm algif_hash algif_skcipher af_alg xt_cgroup
uinput xt_MASQUERADE
CPU: 4 PID: 3255 Comm: bash Not tainted 5.11 #3
aa23f7a1231c229de205662d5a9e0d4c580f19a1
Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
pstate: 0049 (nzcv daif +PAN -UAO -TCO BTYPE=--)
pc : lkdtm_WARNING+0x28/0x30 [lkdtm]
The hex string aa23f7a1231c229de205662d5a9e0d4c580f19a1 is the build ID,
following the kernel version number. Put it all behind a config option,
STACKTRACE_BUILD_ID, so that kernel developers can remove this
information if they decide it is too much.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1]
Link: https://sourceware.org/elfutils/Debuginfod.html [2]
Signed-off-by: Stephen Boyd
---
include/linux/buildid.h | 4
lib/Kconfig.debug | 11 +++
lib/buildid.c | 2 ++
lib/dump_stack.c| 13 +++--
4 files changed, 28 insertions(+), 2 deletions(-)
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index f375900cf9ed..3b7a0ff4642f 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -10,7 +10,11 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char
*build_id,
__u32 *size);
int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
+#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) || IS_ENABLED(CONFIG_CRASH_CORE)
extern unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX];
void init_vmlinux_build_id(void);
+#else
+static inline void init_vmlinux_build_id(void) { }
+#endif
#endif
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2779c29d9981..5f883e50f406 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -35,6 +35,17 @@ config PRINTK_CALLER
no option to enable/disable at the kernel command line parameter or
sysfs interface.
+config STACKTRACE_BUILD_ID
+ bool "Show build ID information in stacktraces"
+ depends on PRINTK
+ help
+ Selecting this option adds build ID information for symbols in
+ stacktraces printed with the printk format '%p[SR]b'.
+
+ This option is intended for distros where debuginfo is not easily
+ accessible but can be downloaded given the build ID of the vmlinux or
+ kernel module where the function is located.
+
config CONSOLE_LOGLEVEL_DEFAULT
int "Default console loglevel (1-15)"
range 1 15
diff --git a/lib/buildid.c b/lib/buildid.c
index 1103ed46214f..530dbd1f6bbe 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -174,6 +174,7 @@ int build_id_parse_buf(const void *buf, unsigned char
*build_id, u32 buf_size)
return parse_build_id_buf(build_id, NULL, buf, buf_size);
}
+#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) || IS_ENABLED(CONFIG_CRASH_CORE)
unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX] __ro_after_init;
/**
@@ -187,3 +188,4 @@ void __init init_vmlinux_build_id(void)
build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
}
+#endif
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index f5a33b6f773f..d685331b065f 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -5,6 +5,7 @@
*/
#include
+#include
#include
#include
#include
@@ -36,6 +37,14 @@ void __init dump_stack_set_arch_desc(const char *fmt, ...)
va_end(args);
}
+#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
+#define BUILD_ID_FMT " %20phN"
+#define BUILD_ID_VAL vmlinux_build_id
+#else
+#define BUILD_ID_FMT "%s"
+#define BUILD_ID_VAL ""
+#endif
+
/**
* dump_stack_print_info - print generic debug info for dump_stack()
* @log_lvl: log level
@@ -45,13 +54,13 @@ void __init dump_stack_set_arch_desc(const char *fmt, ...)
*/
void dump_stack_print_info(const char *log_lvl)
{
- printk("%sCPU: %d PID: %d Comm: %.
[PATCH v5 02/13] buildid: Add API to parse build ID out of buffer
Add an API that can parse the build ID out of a buffer, instead of a
vma, to support printing a kernel module's build ID for stack traces.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Signed-off-by: Stephen Boyd
---
include/linux/buildid.h | 1 +
lib/buildid.c | 50 ++---
2 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index 40232f90db6e..ebce93f26d06 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -8,5 +8,6 @@
int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
__u32 *size);
+int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
#endif
diff --git a/lib/buildid.c b/lib/buildid.c
index e014636ec3eb..6aea1c4e5e85 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -2,30 +2,23 @@
#include
#include
+#include
#include
#define BUILD_ID 3
+
/*
* Parse build id from the note segment. This logic can be shared between
* 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are
* identical.
*/
-static inline int parse_build_id(void *page_addr,
-unsigned char *build_id,
-__u32 *size,
-void *note_start,
-Elf32_Word note_size)
+static int parse_build_id_buf(unsigned char *build_id,
+ __u32 *size,
+ const void *note_start,
+ Elf32_Word note_size)
{
Elf32_Word note_offs = 0, new_offs;
- /* check for overflow */
- if (note_start < page_addr || note_start + note_size < note_start)
- return -EINVAL;
-
- /* only supports note that fits in the first page */
- if (note_start + note_size > page_addr + PAGE_SIZE)
- return -EINVAL;
-
while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
@@ -50,9 +43,27 @@ static inline int parse_build_id(void *page_addr,
break;
note_offs = new_offs;
}
+
return -EINVAL;
}
+static inline int parse_build_id(void *page_addr,
+unsigned char *build_id,
+__u32 *size,
+void *note_start,
+Elf32_Word note_size)
+{
+ /* check for overflow */
+ if (note_start < page_addr || note_start + note_size < note_start)
+ return -EINVAL;
+
+ /* only supports note that fits in the first page */
+ if (note_start + note_size > page_addr + PAGE_SIZE)
+ return -EINVAL;
+
+ return parse_build_id_buf(build_id, size, note_start, note_size);
+}
+
/* Parse build ID from 32-bit ELF */
static int get_build_id_32(void *page_addr, unsigned char *build_id,
__u32 *size)
@@ -148,3 +159,16 @@ int build_id_parse(struct vm_area_struct *vma, unsigned
char *build_id,
put_page(page);
return ret;
}
+
+/**
+ * build_id_parse_buf - Get build ID from a buffer
+ * @buf: Elf note section(s) to parse
+ * @buf_size: Size of @buf in bytes
+ * @build_id: Build ID parsed from @buf, at least BUILD_ID_SIZE_MAX long
+ *
+ * Return: 0 on success, -EINVAL otherwise
+ */
+int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size)
+{
+ return parse_build_id_buf(build_id, NULL, buf, buf_size);
+}
--
https://chromeos.dev
[PATCH v5 03/13] buildid: Stash away kernels build ID on init
Parse the kernel's build ID at initialization so that other code can
print a hex format string representation of the running kernel's build
ID. This will be used in the kdump and dump_stack code so that
developers can easily locate the vmlinux debug symbols for a
crash/stacktrace.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Dave Young
Cc: Baoquan He
Cc: Vivek Goyal
Cc:
Signed-off-by: Stephen Boyd
---
include/linux/buildid.h | 3 +++
init/main.c | 1 +
lib/buildid.c | 15 +++
3 files changed, 19 insertions(+)
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index ebce93f26d06..f375900cf9ed 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -10,4 +10,7 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char
*build_id,
__u32 *size);
int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
+extern unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX];
+void init_vmlinux_build_id(void);
+
#endif
diff --git a/init/main.c b/init/main.c
index 53b278845b88..eaede2f41327 100644
--- a/init/main.c
+++ b/init/main.c
@@ -857,6 +857,7 @@ asmlinkage __visible void __init __no_sanitize_address
start_kernel(void)
set_task_stack_end_magic(&init_task);
smp_setup_processor_id();
debug_objects_early_init();
+ init_vmlinux_build_id();
cgroup_init_early();
diff --git a/lib/buildid.c b/lib/buildid.c
index 6aea1c4e5e85..1103ed46214f 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
#include
+#include
#include
#include
#include
@@ -172,3 +173,17 @@ int build_id_parse_buf(const void *buf, unsigned char
*build_id, u32 buf_size)
{
return parse_build_id_buf(build_id, NULL, buf, buf_size);
}
+
+unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX] __ro_after_init;
+
+/**
+ * init_vmlinux_build_id - Compute and stash the running kernel's build ID
+ */
+void __init init_vmlinux_build_id(void)
+{
+ extern const void __start_notes __weak;
+ extern const void __stop_notes __weak;
+ unsigned int size = &__stop_notes - &__start_notes;
+
+ build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
+}
--
https://chromeos.dev
[PATCH v5 01/13] buildid: Only consider GNU notes for build ID parsing
Some kernel elf files have various notes that also happen to have an elf
note type of '3', which matches NT_GNU_BUILD_ID but the note name isn't
"GNU". For example, this note trips up the existing logic:
Owner Data size Description
Xen0x0008 Unknown note type: (0x0003) description data: 00 00 00
ff80
Let's make sure that it is a GNU note when parsing the build ID so that
we can use this function to parse a vmlinux's build ID too.
Reported-by: Petr Mladek
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Fixes: bd7525dacd7e ("bpf: Move stack_map_get_build_id into lib")
Tested-by: Petr Mladek
Signed-off-by: Stephen Boyd
---
lib/buildid.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/buildid.c b/lib/buildid.c
index 6156997c3895..e014636ec3eb 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -31,6 +31,7 @@ static inline int parse_build_id(void *page_addr,
if (nhdr->n_type == BUILD_ID &&
nhdr->n_namesz == sizeof("GNU") &&
+ !strcmp((char *)(nhdr + 1), "GNU") &&
nhdr->n_descsz > 0 &&
nhdr->n_descsz <= BUILD_ID_SIZE_MAX) {
memcpy(build_id,
--
https://chromeos.dev
[PATCH v5 00/13] Add build ID to stacktraces
This series adds the kernel's build ID[1] to the stacktrace header printed in oops messages, warnings, etc. and the build ID for any module that appears in the stacktrace after the module name. The goal is to make the stacktrace more self-contained and descriptive by including the relevant build IDs in the kernel logs when something goes wrong. This can be used by post processing tools like script/decode_stacktrace.sh and kernel developers to easily locate the debug info associated with a kernel crash and line up what line and file things started falling apart at. To show how this can be used I've included a patch to decode_stacktrace.sh that downloads the debuginfo from a debuginfod server. This also includes some patches to make the buildid.c file use more const arguments and consolidate logic into buildid.c from kdump. These are left to the end as they were mostly cleanup patches. I don't know who exactly maintains this so I guess Andrew is the best option to merge all this code. Here's an example lkdtm stacktrace on arm64. WARNING: CPU: 4 PID: 3255 at drivers/misc/lkdtm/bugs.c:83 lkdtm_WARNING+0x28/0x30 [lkdtm] Modules linked in: lkdtm rfcomm algif_hash algif_skcipher af_alg xt_cgroup uinput xt_MASQUERADE CPU: 4 PID: 3255 Comm: bash Not tainted 5.11 #3 aa23f7a1231c229de205662d5a9e0d4c580f19a1 Hardware name: Google Lazor (rev3+) with KB Backlight (DT) pstate: 0049 (nzcv daif +PAN -UAO -TCO BTYPE=--) pc : lkdtm_WARNING+0x28/0x30 [lkdtm] lr : lkdtm_do_action+0x24/0x40 [lkdtm] sp : ffc0134fbca0 x29: ffc0134fbca0 x28: ff92d53ba240 x27: x26: x25: x24: ffe3622352c0 x23: 0020 x22: ffe362233366 x21: ffe3622352e0 x20: ffc0134fbde0 x19: 0008 x18: x17: ff929b6536fc x16: x15: x14: 0012 x13: ffe380ed892c x12: ffe381d05068 x11: x10: x9 : 0001 x8 : ffe362237000 x7 : x6 : x5 : x4 : 0001 x3 : 0008 x2 : ff93fef25a70 x1 : ff93fef15788 x0 : ffe3622352e0 Call trace: lkdtm_WARNING+0x28/0x30 [lkdtm ed5019fdf5e53be37cb1ba7899292d7e143b259e] direct_entry+0x16c/0x1b4 [lkdtm ed5019fdf5e53be37cb1ba7899292d7e143b259e] full_proxy_write+0x74/0xa4 vfs_write+0xec/0x2e8 ksys_write+0x84/0xf0 __arm64_sys_write+0x24/0x30 el0_svc_common+0xf4/0x1c0 do_el0_svc_compat+0x28/0x3c el0_svc_compat+0x10/0x1c el0_sync_compat_handler+0xa8/0xcc el0_sync_compat+0x178/0x180 ---[ end trace 3d95032303e59e68 ]--- Changes from v4 (https://lore.kernel.org/r/20210410015300.3764485-1-swb...@chromium.org): * Stubbed out more code when CONFIG_STACKTRACE_BUILD_ID=n * Use static_assert instead of BUILD_BUG_ON() * Dropped bad printk change to IP on x86 Changes from v3 (https://lore.kernel.org/r/20210331030520.3816265-1-swb...@chromium.org): * Fixed compilation warnings due to config changes * Fixed kernel-doc on init_vmlinx_build_id() * Totally removed add_build_id_vmcoreinfo() * Added another printk format %pBb to help x86 print backtraces * Some BUILD_BUG_ON() checks to make sure the buildid doesn't get bigger or smaller Changes from v2 (https://lore.kernel.org/r/20210324020443.1815557-1-swb...@chromium.org): * Renamed symbol printing function to indicate build IDness * Put build ID information behind Kconfig knob * Build ID for vmlinux is calculated in early init instead of on demand * printk format is %pS[R]b Changes from v1 (https://lore.kernel.org/r/20210301174749.1269154-1-swb...@chromium.org): * New printk format %pSb and %pSr * Return binary format instead of hex format string from build ID APIs * Some new patches to cleanup buildid/decode_stacktrace.sh * A new patch to decode_stacktrace.sh to parse output [1] https://fedoraproject.org/wiki/Releases/FeatureBuildId Cc: Alexei Starovoitov Cc: Andy Shevchenko Cc: Baoquan He Cc: Borislav Petkov Cc: Catalin Marinas Cc: Dave Young Cc: Evan Green Cc: Hsin-Yi Wang Cc: Ingo Molnar Cc: Jessica Yu Cc: Jiri Olsa Cc: Cc: Konstantin Khlebnikov Cc: Cc: Cc: Cc: Matthew Wilcox Cc: Petr Mladek Cc: Rasmus Villemoes Cc: Sasha Levin Cc: Sergey Senozhatsky Cc: Steven Rostedt Cc: Thomas Gleixner Cc: Vivek Goyal Cc: Will Deacon Cc: Cc: Christoph Hellwig Cc: peter enderborg https://chromeos.dev
Re: [PATCH v3] arm64: vdso32: drop -no-integrated-as flag
Quoting Nick Desaulniers (2021-04-20 10:44:25) > Clang can assemble these files just fine; this is a relic from the top > level Makefile conditionally adding this. We no longer need --prefix, > --gcc-toolchain, or -Qunused-arguments flags either with this change, so > remove those too. > > To test building: > $ ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- \ > CROSS_COMPILE_COMPAT=arm-linux-gnueabi- make LLVM=1 LLVM_IAS=1 \ > defconfig arch/arm64/kernel/vdso32/ > > Suggested-by: Nathan Chancellor > Signed-off-by: Nick Desaulniers > Reviewed-by: Nathan Chancellor > Reviewed-by: Vincenzo Frascino > --- It boots for me and compat vDSO seems to work properly per tools/testing/selftests/vDSO/, userspace programs aren't crashing right and left, must be good: Tested-by: Stephen Boyd
Re: [PATCH 12/12] dt-bindings: soc: qcom: aoss: Delete unused power-domain definitions
Quoting Sibi Sankar (2021-04-16 05:03:58) > Delete unused power-domain definitions exposed by AOSS QMP. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 10/12] arm64: dts: qcom: sm8250: Use QMP binding to control load state
Quoting Sibi Sankar (2021-04-16 05:03:56) > Use the Qualcomm Mailbox Protocol (QMP) binding to control the load > state resources on SM8250 SoCs and drop deprecated power-domains exposed > by AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 11/12] arm64: dts: qcom: sm8350: Use QMP binding to control load state
Quoting Sibi Sankar (2021-04-16 05:03:57) > Use the Qualcomm Mailbox Protocol (QMP) binding to control the load > state resources on SM8350 SoCs and drop deprecated power-domains exposed > by AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 09/12] arm64: dts: qcom: sm8150: Use QMP binding to control load state
Quoting Sibi Sankar (2021-04-16 05:03:55) > Use the Qualcomm Mailbox Protocol (QMP) binding to control the load > state resources on SM8150 SoCs and drop deprecated power-domains exposed > by AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 08/12] arm64: dts: qcom: sdm845: Use QMP binding to control load state
Quoting Sibi Sankar (2021-04-16 05:03:54) > Use the Qualcomm Mailbox Protocol (QMP) binding to control the load > state resources on SDM845 SoCs and drop deprecated power-domains exposed > by AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 07/12] arm64: dts: qcom: sc7280: Use QMP binding to control load state
Quoting Sibi Sankar (2021-04-16 05:03:53) > Use the Qualcomm Mailbox Protocol (QMP) binding to control the load > state resources on SC7280 SoCs and drop deprecated power-domains exposed > by AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 06/12] arm64: dts: qcom: sc7180: Use QMP binding to control load state
Quoting Sibi Sankar (2021-04-16 05:03:52) > Use the Qualcomm Mailbox Protocol (QMP) binding to control the load > state resources on SC7180 SoCs and drop deprecated power-domains exposed > by AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 02/12] soc: qcom: aoss: Drop power domain support
Quoting Sibi Sankar (2021-04-16 05:03:48) > The load state resources are expected to follow the life cycle of the > remote processor it tracks. However, modeling load state resources as > power-domains result in them getting turned off during system suspend > and thereby falling out of sync with the remote processors that are still > on. Fix this by replacing load state resource control through the generic > qmp message send interface instead. > > Signed-off-by: Sibi Sankar > --- Is it possible to keep this code around for a cycle so that there isn't the chance that someone is using the deprecated DT bindings with a new kernel? I worry that ripping the code out will cause them angst. Certainly we have to keep the code in place until DT is updated, so this patch should come last?
Re: [PATCH 03/12] dt-bindings: remoteproc: qcom: pas: Add QMP bindings
Quoting Sibi Sankar (2021-04-16 05:03:49) > Add Qualcomm Mailbox Protocol (QMP) binding to replace the power domains > exposed by the AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 04/12] dt-bindings: remoteproc: qcom: Add QMP bindings
Quoting Sibi Sankar (2021-04-16 05:03:50) > Add Qualcomm Mailbox Protocol (QMP) binding to replace the power domains > exposed by the AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH 01/12] dt-bindings: soc: qcom: aoss: Drop power-domain bindings
Quoting Sibi Sankar (2021-04-16 05:03:47) > Drop power-domain bindings exposed by AOSS QMP node. > > Signed-off-by: Sibi Sankar > --- Reviewed-by: Stephen Boyd
Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces
Quoting Jessica Yu (2021-04-15 06:04:35)
> +++ Stephen Boyd [09/04/21 18:52 -0700]:
> >diff --git a/include/linux/module.h b/include/linux/module.h
> >index 59f094fa6f74..4bf869f6c944 100644
> >--- a/include/linux/module.h
> >+++ b/include/linux/module.h
> >@@ -11,6 +11,7 @@
> >
> > #include
> > #include
> >+#include
> > #include
> > #include
> > #include
> >@@ -367,6 +368,9 @@ struct module {
> > /* Unique handle for this module */
> > char name[MODULE_NAME_LEN];
> >
> >+ /* Module build ID */
> >+ unsigned char build_id[BUILD_ID_SIZE_MAX];
>
> Hi Stephen,
>
> Since this field is not used when !CONFIG_STACKTRACE_BUILD_ID, I
> would prefer to wrap this in an ifdef, similar to the other
> CONFIG-dependent fields in struct module. This makes it explicit under
> what conditions (i.e. config) the field is meant to be used.
Ok will do.
> >diff --git a/kernel/module.c b/kernel/module.c
> >index 30479355ab85..6f5bc1b046a5 100644
> >--- a/kernel/module.c
> >+++ b/kernel/module.c
> >@@ -2770,6 +2771,20 @@ static void add_kallsyms(struct module *mod, const
> >struct load_info *info)
> > }
> > mod->core_kallsyms.num_symtab = ndst;
> > }
> >+
> >+static void init_build_id(struct module *mod, const struct load_info *info)
> >+{
> >+ const Elf_Shdr *sechdr;
> >+ unsigned int i;
> >+
> >+ for (i = 0; i < info->hdr->e_shnum; i++) {
> >+ sechdr = &info->sechdrs[i];
> >+ if (!sect_empty(sechdr) && sechdr->sh_type == SHT_NOTE &&
> >+ !build_id_parse_buf((void *)sechdr->sh_addr,
> >mod->build_id,
> >+ sechdr->sh_size))
> >+ break;
> >+ }
>
> If mod->build_id is not used when !CONFIG_STACKTRACE_BUILD_ID, then we
> don't need to look for it. I would be fine with wrapping the function
> body in an ifdef (similar to what we currently do in
> del_usage_links() and do_mod_ctors()).
Ok, done.
>
> >+}
> > #else
> > static inline void layout_symtab(struct module *mod, struct load_info *info)
> > {
> >@@ -2778,6 +2793,10 @@ static inline void layout_symtab(struct module *mod,
> >struct load_info *info)
> > static void add_kallsyms(struct module *mod, const struct load_info *info)
> > {
> > }
> >+
> >+static void init_build_id(struct module *mod, const struct load_info *info)
> >+{
> >+}
> > #endif /* CONFIG_KALLSYMS */
> >
> > static void dynamic_debug_setup(struct module *mod, struct _ddebug *debug,
> > unsigned int num)
> >@@ -4004,6 +4023,7 @@ static int load_module(struct load_info *info, const
> >char __user *uargs,
> > goto free_arch_cleanup;
> > }
> >
> >+ init_build_id(mod, info);
> > dynamic_debug_setup(mod, info->debug, info->num_debug);
> >
> > /* Ftrace init must be called in the MODULE_STATE_UNFORMED state */
> >@@ -4235,7 +4255,7 @@ void * __weak
> >dereference_module_function_descriptor(struct module *mod,
> > const char *module_address_lookup(unsigned long addr,
> > unsigned long *size,
> > unsigned long *offset,
> >- char **modname,
> >+ char **modname, const unsigned char **modbuildid,
> > char *namebuf)
> > {
> > const char *ret = NULL;
> >@@ -4246,6 +4266,8 @@ const char *module_address_lookup(unsigned long addr,
> > if (mod) {
> > if (modname)
> > *modname = mod->name;
> >+ if (modbuildid)
> >+ *modbuildid = mod->build_id;
>
> Then maybe we can set *modbuildid = NULL in the case of
> !CONFIG_STACKTRACE_BUILD_ID, similar to the kernel case in
> kallsyms_lookup_buildid().
>
Sounds good. It means that some more ifdefs are probably required vs.
making the array size be 0 when the config is disabled but that isn't a
big problem for me. I'm reworking the code now and will test and then
send v5 shortly. Thanks!
Re: [PATCH v2 1/7] dt-bindings: clock: Add MT8192 APU clock bindings
Quoting Flora Fu (2021-04-14 22:52:34) > Add clock bindings for APU on MT8192. > > Signed-off-by: Flora Fu > Acked-by: Rob Herring > --- Acked-by: Stephen Boyd
Re: [PATCH v2 2/7] clk: mediatek: mt8192: Add APU clocks support
Quoting Flora Fu (2021-04-14 22:52:35) > Add APU clocks support on MT8192. > > Signed-off-by: Flora Fu > --- Acked-by: Stephen Boyd
Re: [PATCH 2/4] dt-bindings: clock: Convert ti,sci-clk to json schema
Quoting Nishanth Menon (2021-04-15 23:37:19)
> diff --git a/Documentation/devicetree/bindings/clock/ti,sci-clk.yaml
> b/Documentation/devicetree/bindings/clock/ti,sci-clk.yaml
> new file mode 100644
> index ..72633651f0c7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/ti,sci-clk.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only or BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/ti,sci-clk.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: TI-SCI clock controller node bindings
> +
> +maintainers:
> + - Nishanth Menon
> +
> +allOf:
> + - $ref: /schemas/clock/clock.yaml#
Is this needed?
> +
> +description: |
> + Some TI SoCs contain a system controller (like the Power Management Micro
> + Controller (PMMC) on Keystone 66AK2G SoC) that are responsible for
> controlling
> + the state of the various hardware modules present on the SoC. Communication
> + between the host processor running an OS and the system controller happens
> + through a protocol called TI System Control Interface (TI-SCI protocol).
> +
> + This clock controller node uses the TI SCI protocol to perform various
> clock
> + management of various hardware modules (devices) present on the SoC. This
> + node must be a child node of the associated TI-SCI system controller node.
> +
> +properties:
> + $nodename:
> +pattern: "^clock-controller$"
Is this nodename pattern check required?
> +
> + compatible:
> +const: ti,k2g-sci-clk
I thought most things keyed off the compatible string.
> +
> + "#clock-cells":
> +const: 2
> +description:
> + The two cells represent values that the TI-SCI controller defines.
> +
> + The first cell should contain the device ID.
> +
> + The second cell should contain the clock ID.
> +
> + Please see http://processors.wiki.ti.com/index.php/TISCI for
> + protocol documentation for the values to be used for different devices.
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> +k3_clks: clock-controller {
> +compatible = "ti,k2g-sci-clk";
> +#clock-cells = <2>;
> +};
Re: [PATCH] mmc: core: Don't allocate IDA for OF aliases
Quoting Ulf Hansson (2021-04-16 00:17:10) > On Thu, 15 Apr 2021 at 21:29, Stephen Boyd wrote: > > > > > > Don't think so. The device (with the kobject inside) is removed, and > > thus the mmc1 device will be removed, but the kobject's release function > > is delayed due to the config. This means that > > mmc_host_classdev_release() is called at a later time. The only thing > > inside that function is the IDA removal and the kfree of the container > > object. Given that nothing else is in that release function I believe it > > is safe to skip IDA allocation as it won't be blocking anything in the > > reserved alias case. > > > > Furthermore, when the device is deleted in mmc_remove_host() there could > > be other users of the device that haven't called put_device() yet. > > Either way, those other users are keeping the device memory alive, but > > otherwise device_del() has unlinked it from the various driver core > > lists and sysfs has removed it too so it's in a state where code may be > > referencing it but it's on the way out so users of the device will not > > be able to do much with it during this time. > > Right, but see more below. > > > > > This sort of problem (if it exists which I don't think it does) would > > have been there all along and can't be fixed at this level. When a > > device that has an alias calls the mmc_alloc_host() function twice it > > gets two different device structures created so there are two distinct > > kobjects that will need to be released at some point. The index is > > usually different for those two kobjects, but with aliases it turns out > > it is the same. When it comes to registering that device with the same > > name the second one will fail because a device with that name already > > exists on the bus. This would be really hard to do given that it would > > need to be the same aliased device in DT calling the mmc_add_host() > > function without calling mmc_remove_host() for the first one it added in > > between. > > In fact, we have a few rare corner cases that can trigger KASAN splats > when mmc_remove_host() gets executed. Similar splats can be triggered > by just doing a sudden card removal. It's especially related to the > cases, where a thread holds a reference to the card/host object when > it's being removed. I am working on patches to fix these cases, but > haven't yet decided on the best solution. Ok. Can you share the KASAN reports? The memory allocated for this class object and associated index from the IDA will be unique for each call the mmc_alloc_host() so I don't see how KASAN could be noticing something unless a reference to the host is leaking out without the proper get_device() call being made to keep the underlying memory from being freed. > > That's the reason why I was thinking that maybe returning > -EPROBE_DEFER could be an option, but certainly I need to think more > about this. I was hoping that you wouldn't need to think more about returning -EPROBE_DEFER after my email. :( Returning EPROBE_DEFER looks like it's a bandaid for bigger problems with reference counting the pointer allocated in mmc_alloc_host(). I hope I convinced you that there isn't any danger in reusing the IDA in the case of an alias because the only way that is a problem is if the same device calls mmc_alloc_host() twice without calling mmc_remove_host() in between. That should be a pretty obvious problem in driver code? The check to see if that same device has tried to alloc a host twice would still effectively happen after this patch because when mmc_add_host() tries to add that second device to the driver core it will complain about duplicate device names and fail. Will you apply this patch?
Re: [PATCH v2 3/3] drm/msm/dp: check main link status before start aux read
Quoting khs...@codeaurora.org (2021-04-15 15:02:40) > On 2021-04-15 13:06, Stephen Boyd wrote: > > > > Is it really necessary to have this patch at all? I think there are > > bigger problems with suspend/resume of the DP driver in relation to the > > kthread stopping. I hope that the aux channel would start NAKing > > transfers once the cable is disconnected too, so that we don't need to > > do an extra check for each aux transfer. > > I am working on duplicate this problem, but it is not happen on me yet > so far. > From kernel dump, i can see it crash at dp_irq_hdp_handle() after > suspended. > dp_irq_hpd_handle and dp_pm_suspend() are serialized by event_mutex. > > After suspend, ahb clock is disabled. > Hence next dp_catalog_link_is_connected() crash at acess dp ctrl > registers. > > > aux channel does not do NAKing immediately if unplugged. Therefore > aux_transfer will wait until timeout (HZ/4). > worst, drm_dp_dpcd_access() will retry 32 times before return dpcd > read/write failed. > This patch try to eliminate the time spinning on waiting for timeout 32 > times. > Would be useful to have that level of detail in the commit text. Maybe when the cable is disconnected the DP phy should be shutdown and some bit in the phy could effectively "cut off" the aux channel and then NAKs would start coming through here in the DP controller I/O register space?
Re: [PATCH v2 3/3] drm/msm/dp: check main link status before start aux read
Quoting khs...@codeaurora.org (2021-04-15 10:37:29)
> On 2021-04-14 14:09, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2021-04-13 16:11:44)
> >> Make sure main link is in connection state before start aux
> >> read/write operation to avoid unnecessary long delay due to
> >> main link had been unplugged.
> >>
> >> Signed-off-by: Kuogee Hsieh
> >> ---
> >> drivers/gpu/drm/msm/dp/dp_aux.c | 5 +
> >> drivers/gpu/drm/msm/dp/dp_link.c | 20 +++-
> >> 2 files changed, 20 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> index 7c22bfe..fae3806 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> >> @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> >> *dp_aux,
> >>
> >> mutex_lock(&aux->mutex);
> >>
> >> + if (!dp_catalog_link_is_connected(aux->catalog)) {
> >> + ret = -ETIMEDOUT;
> >> + goto unlock_exit;
> >> + }
> >
> > I get a crash here sometimes when plugging and unplugging an apple HDMI
> > dongle during suspend/resume. I guess device power management
> > (dp_pm_suspend()) is happening in parallel with aux transfers from the
> > kthread. Why doesn't the aux communication start reporting NAKs once
> > the
> > cable is disconnected?
> >
> > [ 366.210058] hdmi-audio-codec hdmi-audio-codec.15.auto: calling
> > platform_pm_suspend+0x0/0x60 @ 7175, parent:
> > ae9.displayport-controller
> > [ 366.222825] hdmi-audio-codec hdmi-audio-codec.15.auto:
> > platform_pm_suspend+0x0/0x60 returned 0 after 1 usecs
> > [ 366.232939] msm-dp-display ae9.displayport-controller: calling
> > dp_pm_suspend+0x0/0x80 @ 7175, parent: ae0.mdss
> > [ 366.244006] msm-dp-display ae9.displayport-controller:
> > dp_pm_suspend+0x0/0x80 returned 0 after 79 usecs
> > [ 366.254025] msm_dsi ae94000.dsi: calling
> > pm_runtime_force_suspend+0x0/0xb4 @ 7175, parent: ae0.mdss
> > [ 366.263669] msm_dsi ae94000.dsi: pm_runtime_force_suspend+0x0/0xb4
> > returned 0 after 0 usecs
> > [ 366.272290] panel-simple panel: calling
> > platform_pm_suspend+0x0/0x60 @ 7175, parent: platform
> > [ 366.272501] ti_sn65dsi86 2-002d: calling
> > pm_runtime_force_suspend+0x0/0xb4 @ 176, parent: i2c-2
> > [ 366.281055] panel-simple panel: platform_pm_suspend+0x0/0x60
> > returned 0 after 0 usecs
> > [ 366.281081] leds mmc1::: calling led_suspend+0x0/0x4c @ 7175,
> > parent: 7c4000.sdhci
> > [ 366.290065] ti_sn65dsi86 2-002d: pm_runtime_force_suspend+0x0/0xb4
> > returned 0 after 0 usecs
> > [ 366.298046] leds mmc1::: led_suspend+0x0/0x4c returned 0 after 1
> > usecs
> > [ 366.302994] Internal error: synchronous external abort: 9610
> > [#1] PREEMPT SMP
> > [ 366.303006] Modules linked in: vhost_vsock
> > vmw_vsock_virtio_transport_common vsock vhost rfcomm algif_hash
> > algif_skcipher af_alg xt_cgroup uinput xt_MASQUERADE venus_enc
> > hci_uart venus_dec btqca cros_ec_typec typec bluetooth qcom_spmi_adc5
> > snd_soc_sc7180 qcom_spmi_temp_alarm ecdh_generic qcom_spmi_adc_tm5
> > qcom_vadc_common snd_soc_qcom_common ecc snd_soc_rt5682_i2c
> > snd_soc_rt5682 snd_soc_rl6231 venus_core v4l2_mem2mem
> > snd_soc_lpass_sc7180 snd_soc_lpass_hdmi snd_soc_lpass_cpu
> > snd_soc_lpass_platform snd_soc_max98357a fuse iio_trig_sysfs
> > cros_ec_sensors cros_ec_sensors_ring cros_ec_lid_angle
> > cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf
> > cros_ec_sensorhub lzo_rle lzo_compress zram ath10k_snoc ath10k_core
> > ath mac80211 cfg80211 cdc_ether usbnet r8152 mii uvcvideo
> > videobuf2_vmalloc joydev
> > [ 366.303211] CPU: 0 PID: 224 Comm: dp_hpd_handler Not tainted 5.4.109
> > #27
> > [ 366.303216] Hardware name: Google Lazor (rev3+) with KB Backlight
> > (DT)
> > [ 366.303225] pstate: 60c9 (nZCv daif +PAN +UAO)
> > [ 366.303234] pc : dp_catalog_link_is_connected+0x20/0x34
> > [ 366.303244] lr : dp_aux_transfer+0x44/0x284
> > [ 366.303250] sp : ffc011bfbbe0
> > [ 366.303254] x29: ffc011bfbbe0 x28:
> > [ 366.303262] x27: 000c x26: ff896f8212bc
> > [ 366.303269] x25: 0001 x24: 0001
> > [ 366.303275] x23: 0020 x22: ff896ff82118
> > [ 366.303282] x21: ffc011bfbc50 x20: ff896ff82090
> > [ 366.
Re: [PATCH] mmc: core: Don't allocate IDA for OF aliases
Quoting Ulf Hansson (2021-04-15 01:56:12)
> On Tue, 13 Apr 2021 at 02:36, Stephen Boyd wrote:
> >
> > - err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
> > - if (err < 0) {
> > - kfree(host);
> > - return NULL;
> > + index = ida_simple_get(&mmc_host_ida, min_idx, max_idx,
> > GFP_KERNEL);
> > + if (index < 0) {
> > + kfree(host);
> > + return NULL;
> > + }
>
> This means that a DTB that is screwed up in a way that it has two mmc
> aliases with the same index, would be allowed to use the same index.
>
> What will happen when we continue the probe sequence in such a case?
Yeah I thought about this after sending the patch. So the problem would
be like this right?
aliases {
mmc1 = &sdhci0;
mmc1 = &sdhci1;
};
I have good news! DT won't compile it because it saw the same alias
assigned to twice. I tried it on my sc7180 board.
arch/arm64/boot/dts/qcom/sc7180.dtsi:35.3-18:
ERROR (duplicate_property_names): /aliases:mmc1: Duplicate property name
ERROR: Input tree has errors, aborting (use -f to force output)
arch/arm64/boot/dts/qcom/sc7180-idp.dtb] Error 2
I suppose if someone forced the compilation it may be bad, but do we
really care?
TL;DR: this seems like it isn't a problem.
>
> > }
> >
> > - host->index = err;
> > + host->index = index;
> >
> > dev_set_name(&host->class_dev, "mmc%d", host->index);
> > host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
>
> Another concern that could potentially be a problem, is that the
> "thread" that holds the reference that prevents ida from being
> removed, how would that react to a new mmc device to become
> re-registered with the same index?
>
> I wonder if we perhaps should return -EPROBE_DEFER instead, when
> ida_simple_get() fails?
>
Don't think so. The device (with the kobject inside) is removed, and
thus the mmc1 device will be removed, but the kobject's release function
is delayed due to the config. This means that
mmc_host_classdev_release() is called at a later time. The only thing
inside that function is the IDA removal and the kfree of the container
object. Given that nothing else is in that release function I believe it
is safe to skip IDA allocation as it won't be blocking anything in the
reserved alias case.
Furthermore, when the device is deleted in mmc_remove_host() there could
be other users of the device that haven't called put_device() yet.
Either way, those other users are keeping the device memory alive, but
otherwise device_del() has unlinked it from the various driver core
lists and sysfs has removed it too so it's in a state where code may be
referencing it but it's on the way out so users of the device will not
be able to do much with it during this time.
This sort of problem (if it exists which I don't think it does) would
have been there all along and can't be fixed at this level. When a
device that has an alias calls the mmc_alloc_host() function twice it
gets two different device structures created so there are two distinct
kobjects that will need to be released at some point. The index is
usually different for those two kobjects, but with aliases it turns out
it is the same. When it comes to registering that device with the same
name the second one will fail because a device with that name already
exists on the bus. This would be really hard to do given that it would
need to be the same aliased device in DT calling the mmc_add_host()
function without calling mmc_remove_host() for the first one it added in
between.
(Sorry if that is long. I'm sort of stream of conciousness writing it to
you here and not rewriting it to be more concise)
Re: [PATCH v2 2/3] drm/msm/dp: initialize audio_comp when audio starts
Quoting Kuogee Hsieh (2021-04-14 14:02:50)
> Initialize audio_comp when audio starts and wait for audio_comp at
> dp_display_disable(). This will take care of both dongle unplugged
> and display off (suspend) cases.
>
> Changes in v2:
> -- add dp_display_start_audio()
>
> Signed-off-by: Kuogee Hsieh
Looking better. Thanks!
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 0ba71c7..8a69bcd 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -177,6 +177,14 @@ static int dp_del_event(struct dp_display_private
> *dp_priv, u32 event)
>
> return 0;
> }
> +void dp_display_start_audio(struct msm_dp *dp_display)
Please unstick this from previous function by adding a newline above.
> +{
> + struct dp_display_private *dp;
> +
> + dp = container_of(dp_display, struct dp_display_private, dp_display);
> +
> + reinit_completion(&dp->audio_comp);
> +}
>
> void dp_display_signal_audio_complete(struct msm_dp *dp_display)
> {
> @@ -648,10 +656,6 @@ static int dp_hpd_unplug_handle(struct
> dp_display_private *dp, u32 data)
> /* start sentinel checking in case of missing uevent */
> dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0,
> DP_TIMEOUT_5_SECOND);
>
> - /* signal the disconnect event early to ensure proper teardown */
This doesn't need to be done early anymore? Please mention why in the
commit text.
> - reinit_completion(&dp->audio_comp);
> - dp_display_handle_plugged_change(g_dp_display, false);
> -
> dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK |
> DP_DP_IRQ_HPD_INT_MASK, true);
>
> @@ -894,7 +898,6 @@ static int dp_display_disable(struct dp_display_private
> *dp, u32 data)
> /* wait only if audio was enabled */
> if (dp_display->audio_enabled) {
> /* signal the disconnect event */
> - reinit_completion(&dp->audio_comp);
> dp_display_handle_plugged_change(dp_display, false);
> if (!wait_for_completion_timeout(&dp->audio_comp,
> HZ * 5))
Re: [PATCH v2 1/3] drm/msm/dp: check sink_count before update is_connected status
Quoting Kuogee Hsieh (2021-04-14 14:02:34)
> Link status is different from display connected status in the case
> of something like an Apple dongle where the type-c plug can be
> connected, and therefore the link is connected, but no sink is
> connected until an HDMI cable is plugged into the dongle.
> The sink_count of DPCD of dongle will increase to 1 once an HDMI
> cable is plugged into the dongle so that display connected status
> will become true. This checking also apply at pm_resume.
>
> Fixes: 94e58e2d06e3 ("drm/msm/dp: reset dp controller only at boot up and
> pm_resume")
> Reported-by: Stephen Boyd
> Reviewed-by: Stephen Boyd
> Tested-by: Stephen Boyd
> Signed-off-by: Kuogee Hsieh
> ---
Can you please thread your emailed patches? I see them all as toplevel
messages in my inbox :(
> drivers/gpu/drm/msm/dp/dp_display.c | 15 ---
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5a39da6..0ba71c7 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -586,10 +586,8 @@ static int dp_connect_pending_timeout(struct
> dp_display_private *dp, u32 data)
> mutex_lock(&dp->event_mutex);
>
> state = dp->hpd_state;
> - if (state == ST_CONNECT_PENDING) {
> - dp_display_enable(dp, 0);
> + if (state == ST_CONNECT_PENDING)
> dp->hpd_state = ST_CONNECTED;
> - }
>
> mutex_unlock(&dp->event_mutex);
>
> @@ -669,10 +667,8 @@ static int dp_disconnect_pending_timeout(struct
> dp_display_private *dp, u32 data
> mutex_lock(&dp->event_mutex);
>
> state = dp->hpd_state;
> - if (state == ST_DISCONNECT_PENDING) {
> - dp_display_disable(dp, 0);
> + if (state == ST_DISCONNECT_PENDING)
> dp->hpd_state = ST_DISCONNECTED;
> - }
>
> mutex_unlock(&dp->event_mutex);
>
> @@ -1272,7 +1268,12 @@ static int dp_pm_resume(struct device *dev)
>
> status = dp_catalog_link_is_connected(dp->catalog);
>
> - if (status)
> + /*
> +* can not declared display is connected unless
> +* HDMI cable is plugged in and sink_count of
> +* dongle become 1
> +*/
> + if (status && dp->link->sink_count)
> dp->dp_display.is_connected = true;
> else
> dp->dp_display.is_connected = false;
With this patch applied things still go wrong for me sometimes. I can
connect the apple dongle and then disconnect the apple dongle, instead
of connect and disconnect the HDMI cable, and sometimes the external
display doesn't come on. I'm still investigating but just wanted to let
you know.
Re: [PATCH v2 3/3] drm/msm/dp: check main link status before start aux read
Quoting Kuogee Hsieh (2021-04-13 16:11:44)
> Make sure main link is in connection state before start aux
> read/write operation to avoid unnecessary long delay due to
> main link had been unplugged.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 5 +
> drivers/gpu/drm/msm/dp/dp_link.c | 20 +++-
> 2 files changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 7c22bfe..fae3806 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -343,6 +343,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>
> mutex_lock(&aux->mutex);
>
> + if (!dp_catalog_link_is_connected(aux->catalog)) {
> + ret = -ETIMEDOUT;
> + goto unlock_exit;
> + }
I get a crash here sometimes when plugging and unplugging an apple HDMI
dongle during suspend/resume. I guess device power management
(dp_pm_suspend()) is happening in parallel with aux transfers from the
kthread. Why doesn't the aux communication start reporting NAKs once the
cable is disconnected?
[ 366.210058] hdmi-audio-codec hdmi-audio-codec.15.auto: calling
platform_pm_suspend+0x0/0x60 @ 7175, parent: ae9.displayport-controller
[ 366.222825] hdmi-audio-codec hdmi-audio-codec.15.auto:
platform_pm_suspend+0x0/0x60 returned 0 after 1 usecs
[ 366.232939] msm-dp-display ae9.displayport-controller: calling
dp_pm_suspend+0x0/0x80 @ 7175, parent: ae0.mdss
[ 366.244006] msm-dp-display ae9.displayport-controller:
dp_pm_suspend+0x0/0x80 returned 0 after 79 usecs
[ 366.254025] msm_dsi ae94000.dsi: calling pm_runtime_force_suspend+0x0/0xb4 @
7175, parent: ae0.mdss
[ 366.263669] msm_dsi ae94000.dsi: pm_runtime_force_suspend+0x0/0xb4 returned
0 after 0 usecs
[ 366.272290] panel-simple panel: calling platform_pm_suspend+0x0/0x60 @ 7175,
parent: platform
[ 366.272501] ti_sn65dsi86 2-002d: calling pm_runtime_force_suspend+0x0/0xb4 @
176, parent: i2c-2
[ 366.281055] panel-simple panel: platform_pm_suspend+0x0/0x60 returned 0
after 0 usecs
[ 366.281081] leds mmc1::: calling led_suspend+0x0/0x4c @ 7175, parent:
7c4000.sdhci
[ 366.290065] ti_sn65dsi86 2-002d: pm_runtime_force_suspend+0x0/0xb4 returned
0 after 0 usecs
[ 366.298046] leds mmc1::: led_suspend+0x0/0x4c returned 0 after 1 usecs
[ 366.302994] Internal error: synchronous external abort: 9610 [#1]
PREEMPT SMP
[ 366.303006] Modules linked in: vhost_vsock vmw_vsock_virtio_transport_common
vsock vhost rfcomm algif_hash algif_skcipher af_alg xt_cgroup uinput
xt_MASQUERADE venus_enc hci_uart venus_dec btqca cros_ec_typec typec bluetooth
qcom_spmi_adc5 snd_soc_sc7180 qcom_spmi_temp_alarm ecdh_generic
qcom_spmi_adc_tm5 qcom_vadc_common snd_soc_qcom_common ecc snd_soc_rt5682_i2c
snd_soc_rt5682 snd_soc_rl6231 venus_core v4l2_mem2mem snd_soc_lpass_sc7180
snd_soc_lpass_hdmi snd_soc_lpass_cpu snd_soc_lpass_platform snd_soc_max98357a
fuse iio_trig_sysfs cros_ec_sensors cros_ec_sensors_ring cros_ec_lid_angle
cros_ec_sensors_core industrialio_triggered_buffer kfifo_buf cros_ec_sensorhub
lzo_rle lzo_compress zram ath10k_snoc ath10k_core ath mac80211 cfg80211
cdc_ether usbnet r8152 mii uvcvideo videobuf2_vmalloc joydev
[ 366.303211] CPU: 0 PID: 224 Comm: dp_hpd_handler Not tainted 5.4.109 #27
[ 366.303216] Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
[ 366.303225] pstate: 60c9 (nZCv daif +PAN +UAO)
[ 366.303234] pc : dp_catalog_link_is_connected+0x20/0x34
[ 366.303244] lr : dp_aux_transfer+0x44/0x284
[ 366.303250] sp : ffc011bfbbe0
[ 366.303254] x29: ffc011bfbbe0 x28:
[ 366.303262] x27: 000c x26: ff896f8212bc
[ 366.303269] x25: 0001 x24: 0001
[ 366.303275] x23: 0020 x22: ff896ff82118
[ 366.303282] x21: ffc011bfbc50 x20: ff896ff82090
[ 366.303289] x19: ff896ffc3598 x18:
[ 366.303295] x17: x16: 0001
[ 366.303303] x15: x14: 02ef
[ 366.303311] x13: 0400 x12: ffeb896ea060
[ 366.303317] x11: x10:
[ 366.303324] x9 : ff896f061100 x8 : ffc010582204
[ 366.303331] x7 : 00b2b5593519 x6 : 003033ff
[ 366.303338] x5 : x4 : 0001
[ 366.303345] x3 : ff896ff432a1 x2 :
[ 366.303352] x1 : 0119 x0 : ff896ffc3598
[ 366.303360] Call trace:
[ 366.303367] dp_catalog_link_is_connected+0x20/0x34
[ 366.303374] dp_aux_transfer+0x44/0x284
[ 366.303387] drm_dp_dpcd_access+0x8c/0x11c
[ 366.303393] drm_dp_dpcd_read+0x64/0x10c
[ 366.303399] dp_link_process_request+0x94/0xaf8
[ 366.303405] dp_display_usbpd_attention_cb+0x38/0x140
[ 366.303411] hpd_event_thread+0x3f0/0x48c
[ 366.303426] kthread+0x140/0x17c
[ 366.303439] ret_from_fork+0x1
Re: [PATCH v2] usb: dwc3: core: Add shutdown callback for dwc3
Quoting Sandeep Maheswaram (2021-04-13 23:03:29) > This patch adds a shutdown callback to USB DWC core driver to ensure that > it is properly shutdown in reboot/shutdown path. This is required > where SMMU address translation is enabled like on SC7180 > SoC and few others. If the hardware is still accessing memory after > SMMU translation is disabled as part of SMMU shutdown callback in > system reboot or shutdown path, then IOVAs(I/O virtual address) > which it was using will go on the bus as the physical addresses which > might result in unknown crashes (NoC/interconnect errors). > > Signed-off-by: Sandeep Maheswaram > --- Reviewed-by: Stephen Boyd
Re: [PATCH v2 2/3] drm/msm/dp: do not re initialize of audio_comp at display_disable()
Quoting Kuogee Hsieh (2021-04-13 16:11:30)
> At dongle unplug, dp initializes audio_comp followed by sending disconnect
> event notification to audio and to make sure audio had shutdown completely
> by wait for audio completion notification at display_disable(). This patch
Is this dp_display_disable()? Doubtful that display_disable() is the
function we're talking about.
> will not re initialize audio_comp at display_disable() if audio shutdown
> is triggered by dongle unplugged.
This commit text seems to say the why before the what, where why is "dp
initializes audio_comp followed by sending disconnect.." and the what is
"this patch will no re-initialized audio_comp...". Can you reorder this
so the what comes before the why?
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 0ba71c7..1d71c95 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -894,8 +894,10 @@ static int dp_display_disable(struct dp_display_private
> *dp, u32 data)
> /* wait only if audio was enabled */
> if (dp_display->audio_enabled) {
> /* signal the disconnect event */
> - reinit_completion(&dp->audio_comp);
> - dp_display_handle_plugged_change(dp_display, false);
> + if (dp->hpd_state != ST_DISCONNECT_PENDING) {
> + reinit_completion(&dp->audio_comp);
Why is this reinitialized here at all? Wouldn't it make more sense to
initialize the completion once at cable plug in and then not initialize
the completion anywhere else? Or initialize the completion whenever
dp_display->audio_enabled is set to true and then only wait for the
completion here if that boolean is true? Or initialize the completion
when dp_display_handle_plugged_change() is passed true for the 'plugged'
argument?
I started reading the code and quickly got lost figuring out how
dp_display_handle_plugged_change() worked and the interaction between
the dp display code and the audio codec embedded in here. There seem to
be a couple of conditions that cut off things early, like
dp_display->audio_enabled and audio->engine_on. Why? Why does
dp_display_signal_audio_complete() call complete_all() vs. just
complete()? Please help! :(
> + dp_display_handle_plugged_change(dp_display, false);
I think it's this way because dp_hpd_unplug_handle() is the function
that sets the hpd_state to ST_DISCONNECT_PENDING and then reinitializes
the completion (why?) and calls dp_display_handle_plugged_change(). So
the commit text could say that reinitializing the completion again here
at dp_display_disable() is racing with the audio code in the case that
dp_hpd_unplug_handle() already called
dp_display_handle_plugged_change() and it would make more sense. But the
question still stands why that race even exists in the first place vs.
initializing the completion variable in only one place unconditionally
when the cable is connected, in dp_hpd_plug_handle() or
dp_display_post_enable().
> + }
> if (!wait_for_completion_timeout(&dp->audio_comp,
> HZ * 5))
> DRM_ERROR("audio comp timeout\n");
Re: [PATCH v2 1/3] drm/msm/dp: check sink_count before update is_connected status
Quoting Kuogee Hsieh (2021-04-13 16:11:10)
> Link status is different from display connected status in the case
> of something like an Apple dongle where the type-c plug can be
> connected, and therefore the link is connected, but no sink is
> connected until an HDMI cable is plugged into the dongle.
> The sink_count of DPCD of dongle will increase to 1 once an HDMI
> cable is plugged into the dongle so that display connected status
> will become true. This checking also apply at pm_resume.
>
> Fixes: 94e58e2d06e3 ("drm/msm/dp: reset dp controller only at boot up and
> pm_resume")
> Reported-by: Stephen Boyd
> Signed-off-by: Kuogee Hsieh
> ---
Reviewed-by: Stephen Boyd
Tested-by: Stephen Boyd
Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces
Quoting Petr Mladek (2021-04-13 08:01:14)
> On Fri 2021-04-09 18:52:52, Stephen Boyd wrote:
> > Let's make kernel stacktraces easier to identify by including the build
> > ID[1] of a module if the stacktrace is printing a symbol from a module.
> > This makes it simpler for developers to locate a kernel module's full
> > debuginfo for a particular stacktrace. Combined with
> > scripts/decode_stracktrace.sh, a developer can download the matching
> > debuginfo from a debuginfod[2] server and find the exact file and line
> > number for the functions plus offsets in a stacktrace that match the
> > module. This is especially useful for pstore crash debugging where the
> > kernel crashes are recorded in something like console-ramoops and the
> > recovery kernel/modules are different or the debuginfo doesn't exist on
> > the device due to space concerns (the debuginfo can be too large for
> > space limited devices).
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index 59f094fa6f74..4bf869f6c944 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -11,6 +11,7 @@
> >
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -367,6 +368,9 @@ struct module {
> > /* Unique handle for this module */
> > char name[MODULE_NAME_LEN];
> >
> > + /* Module build ID */
> > + unsigned char build_id[BUILD_ID_SIZE_MAX];
>
> Do we want to initialize/store the ID even when
> CONFIG_STACKTRACE_BUILD_ID is disabled and nobody would
> use it?
>
> Most struct module members are added only when the related feature
> is enabled.
>
> I am not sure how it would complicate the code. It is possible
> that it is not worth it. Well, I could imagine that the API
> will always pass the buildid parameter and
> module_address_lookup() might do something like
>
> #ifndef CONFIG_STACKTRACE_BUILD_ID
> static char empty_build_id[BUILD_ID_SIZE_MAX];
> #endif
>
> if (modbuildid) {
> if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID))
> *modbuildid = mod->build_id;
> else
> *modbuildid = empty_build_id;
>
> IMHO, this is primary a call for Jessica as the module code maintainer.
>
> Otherwise, I am fine with this patch. And it works as expected.
>
Does declaring mod->build_id as zero length work well enough?
8<
diff --git a/include/linux/module.h b/include/linux/module.h
index 4bf869f6c944..03b2f6af093a 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -359,6 +359,12 @@ struct klp_modinfo {
};
#endif
+#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
+#define MODULE_BUILD_ID_LEN BUILD_ID_SIZE_MAX
+#else
+#define MODULE_BUILD_ID_LEN 0
+#endif
+
struct module {
enum module_state state;
@@ -369,7 +375,7 @@ struct module {
char name[MODULE_NAME_LEN];
/* Module build ID */
- unsigned char build_id[BUILD_ID_SIZE_MAX];
+ unsigned char build_id[MODULE_BUILD_ID_LEN];
/* Sysfs stuff. */
struct module_kobject mkobj;
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index b835992e76c2..ebd5b30c3039 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -25,7 +25,10 @@
#include
#include
#include
+#include
#include
+#include
+#include
/*
* These will be re-linked against their real values
@@ -393,10 +396,13 @@ static int __sprint_symbol(char *buffer, unsigned long
address,
if (modname) {
len += sprintf(buffer + len, " [%s", modname);
- /* build ID should match length of sprintf below */
- BUILD_BUG_ON(BUILD_ID_SIZE_MAX != 20);
- if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) && add_buildid &&
buildid)
+#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
+ if (add_buildid && buildid) {
+ /* build ID should match length of sprintf */
+ static_assert(MODULE_BUILD_ID_LEN == 20);
len += sprintf(buffer + len, " %20phN", buildid);
+ }
+#endif
len += sprintf(buffer + len, "]");
}
diff --git a/kernel/module.c b/kernel/module.c
index 6f5bc1b046a5..a0d222fbd281 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -2771,7 +2771,17 @@ static void add_kallsyms(struct module *mod, const
struct load_info *info)
}
mod->core_kallsyms.num_symtab = ndst;
}
+#else
+static inline void layout_symtab(struct module *mod, struct load_info *info)
+{
+}
+
+static void add_kallsyms(struct module *mod, con
Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces
Quoting Stephen Boyd (2021-04-13 13:10:05)
> Quoting Petr Mladek (2021-04-13 08:16:20)
> > On Tue 2021-04-13 13:56:31, Andy Shevchenko wrote:
> > > On Mon, Apr 12, 2021 at 12:29:05PM -0700, Stephen Boyd wrote:
> > > > Quoting Andy Shevchenko (2021-04-12 04:58:02)
> > > > >
> > > > > First of all, why not static_assert() defined near to the actual
> > > > > macro?
> > > >
> > > > Which macro? BUILD_ID_SIZE_MAX?
> > >
> > > Yes.
> > >
> > > > I tried static_assert() and it didn't
> > > > work for me but maybe I missed something.
> >
> > I guess that you wanted to use it inside macro definition:
> >
> > #define VMCOREINFO_BUILD_ID(value) \
> > static_assert(ARRAY_SIZE(value) == BUILD_ID_SIZE_MAX); \
> > vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
> >
> > Instead, you should do it outside the macro:
> >
> > static_assert(ARRAY_SIZE(value) == BUILD_ID_SIZE_MAX);
> > #define VMCOREINFO_BUILD_ID(value) \
> > vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
>
> In this example "value" is not defined because it's an argument to the
> macro. How can this work?
>
> From what I can tell static_assert() is for the case that you want to
> assert something at the global scope level. BUILD_BUG_ON() can't be used
> at global scope. I see the usage is usually to assert struct members and
> alignment of those members. In turn, static_assert() can't be used at
> function level scope. Each has a use and in this case I want to assert
> at function level scope to be as close as possible to the place that
> would need to change.
>
Good news. I can do this to force a basic block and then GCC doesn't complain.
---8<---
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 2174dab16ba9..de62a722431e 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -38,9 +38,12 @@ phys_addr_t paddr_vmcoreinfo_note(void);
#define VMCOREINFO_OSRELEASE(value) \
vmcoreinfo_append_str("OSRELEASE=%s\n", value)
-#define VMCOREINFO_BUILD_ID(value) \
- BUILD_BUG_ON(ARRAY_SIZE(value) != BUILD_ID_SIZE_MAX); \
- vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
+#define VMCOREINFO_BUILD_ID() \
+ ({ \
+ static_assert(sizeof(vmlinux_build_id) == 20); \
+ vmcoreinfo_append_str("BUILD-ID=%20phN\n", vmlinux_build_id); \
+ })
+
#define VMCOREINFO_PAGESIZE(value) \
vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
Re: [PATCH v4 04/13] dump_stack: Add vmlinux build ID to stack traces
Quoting Petr Mladek (2021-04-13 07:41:11) > > diff --git a/lib/dump_stack.c b/lib/dump_stack.c > > index f5a33b6f773f..d685331b065f 100644 > > --- a/lib/dump_stack.c > > +++ b/lib/dump_stack.c > > @@ -5,6 +5,7 @@ > > */ > > > > #include > > +#include > > #include > > #include > > #include > > @@ -36,6 +37,14 @@ void __init dump_stack_set_arch_desc(const char *fmt, > > ...) > > va_end(args); > > } > > > > +#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) > > +#define BUILD_ID_FMT " %20phN" > > +#define BUILD_ID_VAL vmlinux_build_id > > +#else > > +#define BUILD_ID_FMT "%s" > > +#define BUILD_ID_VAL "" > > +#endif > > 3rd patch always defines and initializes vmlinux_build_id. But it is > used only when CONFIG_STACKTRACE_BUILD_ID is enabled. It is also used for crash code. > Is it intentional, please? Yes, mostly for simplicity with the other user. > > It is not a big deal for vmlinux_build_id. But it is more questionable > for the per-module id. I am going to open this question for 5th patch > as well. > Right, for the vmlinux_build_id symbol it is not exported, and the whole buildid.c file is part of lib-y, so if the symbol isn't used the linker should drop it during link phase. I can drop the early init call if the config is disabled and crash kernel code isn't enabled, and then rely on the linker to drop the vmlinux_build_id symbol. Let me see if that can work so that we don't have to parse it at boot if it is never used.
Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces
Quoting Petr Mladek (2021-04-13 08:16:20)
> On Tue 2021-04-13 13:56:31, Andy Shevchenko wrote:
> > On Mon, Apr 12, 2021 at 12:29:05PM -0700, Stephen Boyd wrote:
> > > Quoting Andy Shevchenko (2021-04-12 04:58:02)
> > > >
> > > > First of all, why not static_assert() defined near to the actual macro?
> > >
> > > Which macro? BUILD_ID_SIZE_MAX?
> >
> > Yes.
> >
> > > I tried static_assert() and it didn't
> > > work for me but maybe I missed something.
>
> I guess that you wanted to use it inside macro definition:
>
> #define VMCOREINFO_BUILD_ID(value) \
> static_assert(ARRAY_SIZE(value) == BUILD_ID_SIZE_MAX); \
> vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
>
> Instead, you should do it outside the macro:
>
> static_assert(ARRAY_SIZE(value) == BUILD_ID_SIZE_MAX);
> #define VMCOREINFO_BUILD_ID(value) \
> vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
In this example "value" is not defined because it's an argument to the
macro. How can this work?
>From what I can tell static_assert() is for the case that you want to
assert something at the global scope level. BUILD_BUG_ON() can't be used
at global scope. I see the usage is usually to assert struct members and
alignment of those members. In turn, static_assert() can't be used at
function level scope. Each has a use and in this case I want to assert
at function level scope to be as close as possible to the place that
would need to change.
>
> > Sounds weird. static_assert() is a good one. Check, for example,
> > lib/vsprintf.c
> > on how to use it.
> >
> > > Why is static_assert()
> > > preferred?
>
> I guess that it is because it is enough and more efficient for
> checks of constant values (no computation of the value).
>
> > Because it's cleaner way to achieve it and as a bonus it can be put outside
> > of
> > the functions (be in the header or so).
Ok, but I'm still not sure what it would be enforcing. In this case we
need to have it near the sprintf line so that we know to fix the 20 in
there should it ever change to be larger. If it's defined next to the
BUILD_ID_SIZE_MAX macro then it does practically nothing to help future
developers know what to change.
> >
> > > > > + if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) &&
> > > > > add_buildid && buildid)
> > > > > + len += sprintf(buffer + len, " %20phN",
> > > > > buildid);
> > > >
> > > > len += sprintf(buffer + len, " %*phN",
> > > > BUILD_ID_SIZE_MAX, buildid);
> > > >
> > >
> > > Are you suggesting to use sprintf format here so that the size is part
> > > of the printf? Sounds good to me. Thanks.
> >
> > I prefer %20phN when the size is carved in stone (for example by
> > specification), but if you are really expecting that it may be
> > changed in the future, use variadic approach as I showed above.
>
> I would consider this written in stone (last famous words ;-) and use
> %20phN with the static_assert().
>
Yes it is pretty much written in stone. The build ID can be an md5sum
instead of SHA1, and thus 16 bytes instead of 20 bytes for the 160-bit
SHA1 form. This is rare, and the code in buildid.c is padding it out
with zeroes in the case that the note is smaller than 20 bytes in
length. Within the kernel we can always assume the buffer is
BUILD_ID_SIZE_MAX. How about this patch?
8<-
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 2174dab16ba9..042c9c034fba 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -39,7 +39,7 @@ phys_addr_t paddr_vmcoreinfo_note(void);
#define VMCOREINFO_OSRELEASE(value) \
vmcoreinfo_append_str("OSRELEASE=%s\n", value)
#define VMCOREINFO_BUILD_ID(value) \
- BUILD_BUG_ON(ARRAY_SIZE(value) != BUILD_ID_SIZE_MAX); \
+ BUILD_BUG_ON(ARRAY_SIZE(value) != 20); \
vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
#define VMCOREINFO_PAGESIZE(value) \
vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
diff --git a/kernel/kallsyms.c b/kernel/kallsyms.c
index b835992e76c2..5d9c7ac80633 100644
--- a/kernel/kallsyms.c
+++ b/kernel/kallsyms.c
@@ -25,7 +25,10 @@
#include
#include
#include
+#include
#include
+#include
+#include
/*
* These will be re-linked against their real values
@@ -394,7 +397,7 @@ static int __sprint_symbol(char *buffer, unsigned long
address,
if (modname) {
len += sprintf(buffer + len, " [%s", modname);
/* build ID should match length of sprintf below */
- BUILD_BUG_ON(BUILD_ID_SIZE_MAX != 20);
+ BUILD_BUG_ON(sizeof(typeof_member(struct module, build_id)) !=
20);
if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) && add_buildid &&
buildid)
len += sprintf(buffer + len, " %20phN", buildid);
len += sprintf(buffer + len, "]");
Re: [PATCH v13 4/4] MAINTAINERS: add MT7621 CLOCK maintainer
Quoting Sergio Paracuellos (2021-04-09 22:50:59) > Adding myself as maintainer for mt7621 clock driver. > > Signed-off-by: Sergio Paracuellos > --- Applied to clk-next
Re: [PATCH v13 2/4] staging: mt7621-dts: make use of new 'mt7621-clk'
Quoting Sergio Paracuellos (2021-04-09 22:50:57) > Clocks for SoC mt7621 have been properly integrated so there is > no need to declare fixed clocks at all in the device tree. Remove > all of them, add new device tree nodes for mt7621-clk and update > the rest of the nodes to use them. > > Acked-by: Greg Kroah-Hartman > Signed-off-by: Sergio Paracuellos > --- Applied to clk-next
Re: [PATCH v13 3/4] staging: mt7621-dts: use valid vendor 'mediatek' instead of invalid 'mtk'
Quoting Sergio Paracuellos (2021-04-09 22:50:58) > Vendor listed for mediatek in kernel vendor file 'vendor-prefixes.yaml' > contains 'mediatek' as a valid vendor string. Some nodes in the device > tree are using an invalid vendor string vfor 'mtk' instead. Fix all of > them in dts file. Update also ralink mt7621 related code to properly > match new strings. Even there are used in the device tree there are > some strings that are not referred anywhere but have been also updated > with new vendor name. These are 'mtk,mt7621-wdt', 'mtk,mt7621-nand', > 'mtk,mt7621-mc', and 'mtk,mt7621-cpc'. > > Acked-by: Greg Kroah-Hartman > Acked-by: Thomas Bogendoerfer > Signed-off-by: Sergio Paracuellos > --- Applied to clk-next
Re: [PATCH v13 1/4] clk: ralink: add clock driver for mt7621 SoC
Quoting Sergio Paracuellos (2021-04-09 22:50:56) > The documentation for this SOC only talks about two > registers regarding to the clocks: > * SYSC_REG_CPLL_CLKCFG0 - provides some information about > boostrapped refclock. PLL and dividers used for CPU and some > sort of BUS. > * SYSC_REG_CPLL_CLKCFG1 - a banch of gates to enable/disable > clocks for all or some ip cores. > > Looking into driver code, and some openWRT patched there are > another frequencies which are used in some drivers (uart, sd...). > According to all of this information the clock plan for this > SoC is set as follows: > - Main top clock "xtal" from where all the rest of the world is > derived. > - CPU clock "cpu" derived from "xtal" frequencies and a bunch of > register reads and predividers. > - BUS clock "bus" derived from "cpu" and with (cpu / 4) MHz. > - Fixed clocks from "xtal": > * "50m": 50 MHz. > * "125m": 125 MHz. > * "150m": 150 MHz. > * "250m": 250 MHz. > * "270m": 270 MHz. > > We also have a buch of gate clocks with their parents: > * "hsdma": "150m" > * "fe": "250m" > * "sp_divtx": "270m" > * "timer": "50m" > * "pcm": "270m" > * "pio": "50m" > * "gdma": "bus" > * "nand": "125m" > * "i2c": "50m" > * "i2s": "270m" > * "spi": "bus" > * "uart1": "50m" > * "uart2": "50m" > * "uart3": "50m" > * "eth": "50m" > * "pcie0": "125m" > * "pcie1": "125m" > * "pcie2": "125m" > * "crypto": "250m" > * "shxc": "50m" > > With this information the clk driver will provide clock and gates > functionality from a a set of hardcoded clocks allowing to define > a nice device tree without fixed clocks. > > Signed-off-by: Sergio Paracuellos > --- Applied to clk-next
Re: [PATCH 2/2] drm/msm/dp: do not re initialize of audio_comp
Quoting Kuogee Hsieh (2021-04-12 10:03:23)
> At dp_display_disable(), do not re initialize audio_comp if
> hdp_state == ST_DISCONNECT_PENDING (unplug event) to avoid
> race condition which cause 5 second timeout expired.
More details please.
> Also
> add abort mechanism to reduce time spinning at dp_aux_transfer()
> during dpcd read if type-c connection had been broken.
Please split this to a different patch.
>
> Signed-off-by: Kuogee Hsieh
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 18 ++
> drivers/gpu/drm/msm/dp/dp_aux.h | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 16
> drivers/gpu/drm/msm/dp/dp_link.c| 20 +++-
> 4 files changed, 46 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index 7c22bfe..e5ece8c 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -28,6 +28,7 @@ struct dp_aux_private {
> u32 offset;
> u32 segment;
> u32 isr;
> + atomic_t aborted;
Why is it an atomic?
>
> struct drm_dp_aux dp_aux;
> };
> @@ -343,6 +344,11 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
>
> mutex_lock(&aux->mutex);
>
> + if (atomic_read(&aux->aborted)) {
> + ret = -ETIMEDOUT;
> + goto unlock_exit;
> + }
> +
Cool, it's checked inside a mutex.
> aux->native = msg->request & (DP_AUX_NATIVE_WRITE &
> DP_AUX_NATIVE_READ);
>
> /* Ignore address only message */
> @@ -533,3 +539,15 @@ void dp_aux_put(struct drm_dp_aux *dp_aux)
>
> devm_kfree(aux->dev, aux);
> }
> +
> +void dp_aux_abort(struct drm_dp_aux *dp_aux, bool abort)
> +{
> + struct dp_aux_private *aux;
> +
> + if (!dp_aux)
> + return;
> +
> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +
> + atomic_set(&aux->aborted, abort);
> +}
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 4992a049..8960333 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -898,8 +898,10 @@ static int dp_display_disable(struct dp_display_private
> *dp, u32 data)
> /* wait only if audio was enabled */
> if (dp_display->audio_enabled) {
> /* signal the disconnect event */
> - reinit_completion(&dp->audio_comp);
> - dp_display_handle_plugged_change(dp_display, false);
> + if (dp->hpd_state != ST_DISCONNECT_PENDING) {
> + reinit_completion(&dp->audio_comp);
> + dp_display_handle_plugged_change(dp_display, false);
> + }
> if (!wait_for_completion_timeout(&dp->audio_comp,
> HZ * 5))
> DRM_ERROR("audio comp timeout\n");
This hunk is the first part of the patch and should be split away to one
for itself, with appropriate Fixes tag and a proper explanation.
> @@ -1137,20 +1139,26 @@ static irqreturn_t dp_display_irq_handler(int irq,
> void *dev_id)
> /* hpd related interrupts */
> if (hpd_isr_status & DP_DP_HPD_PLUG_INT_MASK ||
> hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
> + dp_aux_abort(dp->aux, false);
> dp_add_event(dp, EV_HPD_PLUG_INT, 0, 0);
> }
>
> if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
> /* stop sentinel connect pending checking */
> + dp_aux_abort(dp->aux, false);
> dp_del_event(dp, EV_CONNECT_PENDING_TIMEOUT);
> dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
> }
>
> - if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK)
> + if (hpd_isr_status & DP_DP_HPD_REPLUG_INT_MASK) {
> + dp_aux_abort(dp->aux, false);
> dp_add_event(dp, EV_HPD_REPLUG_INT, 0, 0);
> + }
>
> - if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK)
> + if (hpd_isr_status & DP_DP_HPD_UNPLUG_INT_MASK) {
> + dp_aux_abort(dp->aux, true);
Ok, so it seems that we want to stop trying aux transfers if the unplug
irq comes in? That's a pretty big sledge hammer to stop a transfer in
the middle of progress. Why doesn't the hardware timeout and stop or the
dpcd reads in this DP driver fail and start bailing out when the cable
is disconnected? Having to inject that synthetically is not great. Is
there some sort of AUX channel "status" bit that can be read from the
aux registers in the DP hardware to see if the connection was lost?
> dp_add_event(dp, EV_HPD_UNPLUG_INT, 0, 0);
> + }
> }
>
> /* DP controller isr */
> diff --git a/drivers/gpu/
Re: [PATCH 1/2] drm/msm/dp: check sink_count before update is_connected status
Quoting Kuogee Hsieh (2021-04-12 10:02:51) > At pm_resume check link sisnk_count before update is_connected status > base on HPD real time link status. Also print out error message only > when either EV_CONNECT_PENDING_TIMEOUT or EV_DISCONNECT_PENDING_TIMEOUT > happen. > > Signed-off-by: Kuogee Hsieh > --- Also please include Reported-by: Stephen Boyd in the next post.
Re: [PATCH 1/2] drm/msm/dp: check sink_count before update is_connected status
Quoting Kuogee Hsieh (2021-04-12 10:02:51)
> At pm_resume check link sisnk_count before update is_connected status
s/sisnk_count/sink_count/
> base on HPD real time link status. Also print out error message only
> when either EV_CONNECT_PENDING_TIMEOUT or EV_DISCONNECT_PENDING_TIMEOUT
> happen.
Can you add "why"? I think the why is something like "link status is
different from display connected status in the case of something like an
Apple dongle where the type-c plug can be connected, and therefore the
link is connected, but no sink is connected until an HDMI cable is
plugged into the dongle". This still doesn't explain why it's important
to check at resume time though.
>
> Signed-off-by: Kuogee Hsieh
> ---
Any Fixes tag?
> drivers/gpu/drm/msm/dp/dp_display.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 5a39da6..4992a049 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -587,7 +587,7 @@ static int dp_connect_pending_timeout(struct
> dp_display_private *dp, u32 data)
>
> state = dp->hpd_state;
> if (state == ST_CONNECT_PENDING) {
> - dp_display_enable(dp, 0);
> + DRM_ERROR("EV_CONNECT_PENDING_TIMEOUT error\n");
Can we get rid of these messages?
> dp->hpd_state = ST_CONNECTED;
> }
>
> @@ -670,7 +670,7 @@ static int dp_disconnect_pending_timeout(struct
> dp_display_private *dp, u32 data
>
> state = dp->hpd_state;
> if (state == ST_DISCONNECT_PENDING) {
> - dp_display_disable(dp, 0);
> + DRM_ERROR("EV_DISCONNECT_PENDING_TIMEOUT error\n");
And this one? If it happens it will just sit in the logs when probably
the user can't do anything about it. Timeouts are just a fact of life.
> dp->hpd_state = ST_DISCONNECTED;
> }
>
> @@ -1272,7 +1272,7 @@ static int dp_pm_resume(struct device *dev)
>
> status = dp_catalog_link_is_connected(dp->catalog);
>
> - if (status)
> + if (status && dp->link->sink_count)
Can we add a comment above this if? Otherwise it doesn't make much
sense why sink_count is important.
/*
* Only consider the display as connected, and send a connected
* notification to userspace in
* dp_display_send_hpd_notification(), if there's actually a
* sink connected. Otherwise, the link could be up/connected or
* in the process of being established, but there isn't actually
* anything to display to on the other side yet.
*/
> dp->dp_display.is_connected = true;
> else
> dp->dp_display.is_connected = false;
Re: [PATCH][V2] clk: uniphier: Fix potential infinite loop
Quoting Colin King (2021-04-09 02:01:04)
> From: Colin Ian King
>
> The for-loop iterates with a u8 loop counter i and compares this
> with the loop upper limit of num_parents that is an int type.
> There is a potential infinite loop if num_parents is larger than
> the u8 loop counter. Fix this by making the loop counter the same
> type as num_parents. Also make num_parents an unsigned int to
> match the return type of the call to clk_hw_get_num_parents.
>
> Addresses-Coverity: ("Infinite loop")
> Fixes: 734d82f4a678 ("clk: uniphier: add core support code for UniPhier clock
> driver")
>
> Signed-off-by: Colin Ian King
> ---
Applied to clk-next
Re: [PATCH] clk: qcom: rpmh: add support for SDX55 rpmh IPA clock
Quoting Alex Elder (2021-04-09 06:44:07) > The IPA core clock is required for SDX55. Define it. > > Signed-off-by: Alex Elder > --- Applied to clk-next
[PATCH] mmc: core: Don't allocate IDA for OF aliases
There's a chance that the IDA allocated in mmc_alloc_host() is not freed
for some time because it's freed as part of a class' release function
(see mmc_host_classdev_release() where the IDA is freed). If another
thread is holding a reference to the class, then only once all balancing
device_put() calls (in turn calling kobject_put()) have been made will
the IDA be released and usable again.
Normally this isn't a problem because the kobject is released before
anything else that may want to use the same number tries to again, but
with CONFIG_DEBUG_KOBJECT_RELEASE=y and OF aliases it becomes pretty
easy to try to allocate an alias from the IDA twice while the first time
it was allocated is still pending a call to ida_simple_remove(). It's
also possible to trigger it by using CONFIG_DEBUG_KOBJECT_RELEASE and
probe defering a driver at boot that calls mmc_alloc_host() before
trying to get resources that may defer likes clks or regulators.
Instead of allocating from the IDA in this scenario, let's just skip it
if we know this is an OF alias. The number is already "claimed" and
devices that aren't using OF aliases won't try to use the claimed
numbers anyway (see mmc_first_nonreserved_index()). This should avoid
any issues with mmc_alloc_host() returning failures from the
ida_simple_get() in the case that we're using an OF alias.
Cc: Matthias Schiffer
Cc: Sujit Kautkar
Reported-by: Zubin Mithra
Fixes: fa2d0aa96941 ("mmc: core: Allow setting slot index via device tree
alias")
Signed-off-by: Stephen Boyd
---
drivers/mmc/core/host.c | 20 ++--
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/core/host.c b/drivers/mmc/core/host.c
index 9b89a91b6b47..137b4a769f62 100644
--- a/drivers/mmc/core/host.c
+++ b/drivers/mmc/core/host.c
@@ -39,7 +39,8 @@ static void mmc_host_classdev_release(struct device *dev)
{
struct mmc_host *host = cls_dev_to_mmc_host(dev);
wakeup_source_unregister(host->ws);
- ida_simple_remove(&mmc_host_ida, host->index);
+ if (of_alias_get_id(host->parent->of_node, "mmc") < 0)
+ ida_simple_remove(&mmc_host_ida, host->index);
kfree(host);
}
@@ -444,7 +445,7 @@ static int mmc_first_nonreserved_index(void)
*/
struct mmc_host *mmc_alloc_host(int extra, struct device *dev)
{
- int err;
+ int index;
struct mmc_host *host;
int alias_id, min_idx, max_idx;
@@ -457,20 +458,19 @@ struct mmc_host *mmc_alloc_host(int extra, struct device
*dev)
alias_id = of_alias_get_id(dev->of_node, "mmc");
if (alias_id >= 0) {
- min_idx = alias_id;
- max_idx = alias_id + 1;
+ index = alias_id;
} else {
min_idx = mmc_first_nonreserved_index();
max_idx = 0;
- }
- err = ida_simple_get(&mmc_host_ida, min_idx, max_idx, GFP_KERNEL);
- if (err < 0) {
- kfree(host);
- return NULL;
+ index = ida_simple_get(&mmc_host_ida, min_idx, max_idx,
GFP_KERNEL);
+ if (index < 0) {
+ kfree(host);
+ return NULL;
+ }
}
- host->index = err;
+ host->index = index;
dev_set_name(&host->class_dev, "mmc%d", host->index);
host->ws = wakeup_source_register(NULL, dev_name(&host->class_dev));
--
https://chromeos.dev
Re: [PATCH 3/5] ASoC: rt5682: clock driver must use the clock provider API
Quoting Jerome Brunet (2021-04-10 04:13:54) > Clock drivers ops should not the clk API but the clock provider (clk_hw) > instead. > > Signed-off-by: Jerome Brunet > --- > sound/soc/codecs/rt5682.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/sound/soc/codecs/rt5682.c b/sound/soc/codecs/rt5682.c > index 0e2a10ed11da..2eee02ac8d49 100644 > --- a/sound/soc/codecs/rt5682.c > +++ b/sound/soc/codecs/rt5682.c > @@ -2634,7 +2634,7 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, > unsigned long rate, > container_of(hw, struct rt5682_priv, > dai_clks_hw[RT5682_DAI_WCLK_IDX]); > struct snd_soc_component *component = rt5682->component; > - struct clk *parent_clk; > + struct clk_hw *parent_hw; > const char * const clk_name = clk_hw_get_name(hw); > int pre_div; > unsigned int clk_pll2_out; > @@ -2649,8 +2649,8 @@ static int rt5682_wclk_set_rate(struct clk_hw *hw, > unsigned long rate, > * > * It will set the codec anyway by assuming mclk is 48MHz. > */ > - parent_clk = clk_get_parent(hw->clk); > - if (!parent_clk) > + parent_hw = clk_hw_get_parent(hw); > + if (!parent_hw) > dev_warn(component->dev, > "Parent mclk of wclk not acquired in driver. Please > ensure mclk was provided as %d Hz.\n", > CLK_PLL2_FIN); Can this code be removed? I don't know why we care to check if the clk has a parent or not.
Re: [PATCH 5/5] ASoC: da7219: properly get clk from the provider
Quoting Jerome Brunet (2021-04-10 04:13:56) > Instead of using the clk embedded in the clk_hw (which is meant to go > away), a clock provider which need to interact with its own clock should > request clk reference through the clock provider API. > > Signed-off-by: Jerome Brunet > --- Reviewed-by: Stephen Boyd
Re: [PATCH 2/5] ASoC: wcd934x: use the clock provider API
Quoting Jerome Brunet (2021-04-10 04:13:53) > Clock providers should use the clk_hw API It sort of already is :) > > Signed-off-by: Jerome Brunet > --- Reviewed-by: Stephen Boyd
Re: [PATCH 1/5] ASoC: stm32: properly get clk from the provider
Quoting Jerome Brunet (2021-04-10 04:13:52) > Instead of using the clk embedded in the clk_hw (which is meant to go > away), a clock provider which need to interact with its own clock should > request clk reference through the clock provider API. > > Signed-off-by: Jerome Brunet > --- Reviewed-by: Stephen Boyd
Re: [PATCH] [v2] clk: renesas: rcar-usb2-clock-sel: Fix error handling in rcar_usb2_clock_sel_probe
Quoting Dinghao Liu (2021-04-12 04:26:01)
> diff --git a/drivers/clk/renesas/rcar-usb2-clock-sel.c
> b/drivers/clk/renesas/rcar-usb2-clock-sel.c
> index 3abafd78f7c8..3b0e33e0bf7b 100644
> --- a/drivers/clk/renesas/rcar-usb2-clock-sel.c
> +++ b/drivers/clk/renesas/rcar-usb2-clock-sel.c
> @@ -193,11 +191,22 @@ static int rcar_usb2_clock_sel_probe(struct
> platform_device *pdev)
> init.num_parents = 0;
> priv->hw.init = &init;
>
> - clk = clk_register(NULL, &priv->hw);
> - if (IS_ERR(clk))
> - return PTR_ERR(clk);
> + clk = devm_clk_register(NULL, &priv->hw);
Please use devm_clk_hw_register() unless that can't be done for some
reason?
> + if (IS_ERR(clk)) {
> + ret = PTR_ERR(clk);
> + goto pm_put;
> + }
> +
Re: [PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces
Quoting Andy Shevchenko (2021-04-12 04:58:02)
> On Fri, Apr 09, 2021 at 06:52:52PM -0700, Stephen Boyd wrote:
> > Let's make kernel stacktraces easier to identify by including the build
> > ID[1] of a module if the stacktrace is printing a symbol from a module.
> > This makes it simpler for developers to locate a kernel module's full
> > debuginfo for a particular stacktrace. Combined with
> > scripts/decode_stracktrace.sh, a developer can download the matching
> > debuginfo from a debuginfod[2] server and find the exact file and line
> > number for the functions plus offsets in a stacktrace that match the
> > module. This is especially useful for pstore crash debugging where the
> > kernel crashes are recorded in something like console-ramoops and the
> > recovery kernel/modules are different or the debuginfo doesn't exist on
> > the device due to space concerns (the debuginfo can be too large for
> > space limited devices).
> >
> > Originally, I put this on the %pS format, but that was quickly rejected
> > given that %pS is used in other places such as ftrace where build IDs
> > aren't meaningful. There was some discussions on the list to put every
> > module build ID into the "Modules linked in:" section of the stacktrace
> > message but that quickly becomes very hard to read once you have more
> > than three or four modules linked in. It also provides too much
> > information when we don't expect each module to be traversed in a
> > stacktrace. Having the build ID for modules that aren't important just
> > makes things messy. Splitting it to multiple lines for each module
> > quickly explodes the number of lines printed in an oops too, possibly
> > wrapping the warning off the console. And finally, trying to stash away
> > each module used in a callstack to provide the ID of each symbol printed
> > is cumbersome and would require changes to each architecture to stash
> > away modules and return their build IDs once unwinding has completed.
> >
> > Instead, we opt for the simpler approach of introducing new printk
> > formats '%pS[R]b' for "pointer symbolic backtrace with module build ID"
> > and '%pBb' for "pointer backtrace with module build ID" and then
> > updating the few places in the architecture layer where the stacktrace
> > is printed to use this new format.
> >
> > Example:
>
> Can you trim a bit the example, so we will see only important lines.
> In such case you may provide "before" and "after" variants.
>
> ...
>
> > - if (modname)
> > - len += sprintf(buffer + len, " [%s]", modname);
> > + if (modname) {
> > + len += sprintf(buffer + len, " [%s", modname);
>
> > + /* build ID should match length of sprintf below */
> > + BUILD_BUG_ON(BUILD_ID_SIZE_MAX != 20);
>
> First of all, why not static_assert() defined near to the actual macro?
Which macro? BUILD_ID_SIZE_MAX? I tried static_assert() and it didn't
work for me but maybe I missed something. Why is static_assert()
preferred?
>
> > + if (IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID) && add_buildid &&
> > buildid)
> > + len += sprintf(buffer + len, " %20phN", buildid);
>
> len += sprintf(buffer + len, " %*phN",
> BUILD_ID_SIZE_MAX, buildid);
>
Are you suggesting to use sprintf format here so that the size is part
of the printf? Sounds good to me. Thanks.
Re: [PATCH v2] arm64: dts: qcom: Update iommu property for simultaneous playback
Quoting Srinivasa Rao Mandadapu (2021-04-09 22:17:07)
> Hi Stephen.
>
> Thanks for your time!!!
>
>
> On 4/9/2021 10:31 PM, Stephen Boyd wrote:
> > Quoting Srinivasa Rao Mandadapu (2021-04-06 09:33:30)
> >> From: V Sujith Kumar Reddy
> >>
> >> Update iommu property in lpass cpu node for supporting
> >> simultaneous playback on headset and speaker.
> >>
> >> Signed-off-by: V Sujith Kumar Reddy
> >> Signed-off-by: Srinivasa Rao Mandadapu
> >> ---
> >> Changes since v1:
> >> -- Commit messge header change
> >>
> >> arch/arm64/boot/dts/qcom/sc7180.dtsi | 3 ++-
> >> 1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> >> b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> >> index a6da78d31fdd..6228ba2d8513 100644
> >> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> >> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> >> @@ -3566,7 +3566,8 @@ lpass_cpu: lpass@62f0 {
> >> reg = <0 0x62f0 0 0x29000>;
> >> reg-names = "lpass-lpaif";
> >>
> >> - iommus = <&apps_smmu 0x1020 0>;
> >> + iommus = <&apps_smmu 0x1020 0>,
> >> + <&apps_smmu 0x1021 0>;
> > The stream ID 0x1032 was also dropped in this version but there's no
> > mention of that in the changelog. Why?
> That is ID is for HDMI Stream, so as part of DP patches that will be added.
Ok, got it.
Re: [PATCH v2] arm64: dts: qcom: Update iommu property for simultaneous playback
Quoting Srinivasa Rao Mandadapu (2021-04-06 09:33:30) > From: V Sujith Kumar Reddy > > Update iommu property in lpass cpu node for supporting > simultaneous playback on headset and speaker. > > Signed-off-by: V Sujith Kumar Reddy > Signed-off-by: Srinivasa Rao Mandadapu > --- Reviewed-by: Stephen Boyd
Re: [PATCH v1 1/2] arm64: dts: qcom: sc7280: Add cpufreq hw node
Quoting Taniya Das (2021-04-09 19:04:39)
> @@ -1116,6 +1124,17 @@
> #clock-cells = <1>;
> };
> };
> +
> + cpufreq_hw: cpufreq@18591000 {
> + compatible = "qcom,cpufreq-epss";
> + reg = <0 0x18591000 0 0x1000>,
> + <0 0x18592000 0 0x1000>,
> + <0 0x18593000 0 0x1000>;
> + reg-names = "freq-domain0", "freq-domain1",
> "freq-domain2";
The reg-names provides practically no value. Can you drop it?
> + clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GCC_GPLL0>;
> + clock-names = "xo", "alternate";
> + #freq-domain-cells = <1>;
> + };
> };
>
Re: [PATCH v2] platform/chrome: Don't populate lightbar device if it isn't there
Quoting Stephen Boyd (2020-09-10 17:53:07)
> Quoting Enric Balletbo i Serra (2020-09-10 08:49:42)
> > On 10/9/20 16:52, Guenter Roeck wrote:
> > > On Thu, Sep 10, 2020 at 7:32 AM Enric Balletbo i Serra
> > > wrote:
> > >> On 10/9/20 16:18, Guenter Roeck wrote:
> > >>> On Wed, Sep 9, 2020 at 3:42 PM Stephen Boyd wrote:
> > >>>> @@ -206,6 +209,17 @@ static int ec_device_probe(struct platform_device
> > >>>> *pdev)
> > >>>> }
> > >>>> }
> > >>>>
> > >>>> + if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> > >>>> + !cros_ec_get_lightbar_version(ec, NULL, NULL)) {
> > >>>
> > >>> Any idea why the lightbar code doesn't use cros_ec_check_features() ?
> > >>> There is a definition for EC_FEATURE_LIGHTBAR, but it doesn't seem to
> > >>> be used. It would be much more convenient if that feature check could
> > >>> be used instead of moving the get_lightbar_version command and its
> > >>> helper function around.
> > >>>
> > >>
> > >> IIRC it was to support a very old device, the Pixel Chromebook (Link).
> > >> This flag
> > >> is not set in this device but has a lightbar, hence we had this 'weird'
> > >> way to
> > >> detect the lightbar.
> > >>
> > >
> > > If that is the only reason, wouldn't it be better to use something
> > > else (eg dmi_match) to determine if the system in question is a Pixel
> > > Chromebook (Link) ?
> > >
> > > if (!strcmp(ec_platform->ec_name, CROS_EC_DEV_NAME) &&
> > > (cros_ec_check_features(ec, EC_FEATURE_LIGHTBAR) ||
> > > dmi_match(DMI_PRODUCT_NAME, "Link")) {
> > >
> >
> > That looks a better solution, indeed. And definetely I'd prefer use the
> > check
> > features way.
> >
> > Gwendal, can you confirm that the Pixel Chromebook (Link) is the _only_ one
> > affected? This one is the only that comes to my mind but I might miss
> > others.
> >
> > I think that Samus has this flag (I can double check) and this was discussed
> > with you (long, long time ago :-) )
> >
>
> Sounds fine by me. I'll wait for Gwendal to inform us.
Anything come of this? I haven't seen any updates.
[PATCH v4 11/13] buildid: Mark some arguments const
These arguments are never modified so they can be marked const to
indicate as such.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Signed-off-by: Stephen Boyd
---
lib/buildid.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/buildid.c b/lib/buildid.c
index 1103ed46214f..df42282b36ff 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -48,10 +48,10 @@ static int parse_build_id_buf(unsigned char *build_id,
return -EINVAL;
}
-static inline int parse_build_id(void *page_addr,
+static inline int parse_build_id(const void *page_addr,
unsigned char *build_id,
__u32 *size,
-void *note_start,
+const void *note_start,
Elf32_Word note_size)
{
/* check for overflow */
@@ -66,7 +66,7 @@ static inline int parse_build_id(void *page_addr,
}
/* Parse build ID from 32-bit ELF */
-static int get_build_id_32(void *page_addr, unsigned char *build_id,
+static int get_build_id_32(const void *page_addr, unsigned char *build_id,
__u32 *size)
{
Elf32_Ehdr *ehdr = (Elf32_Ehdr *)page_addr;
@@ -91,7 +91,7 @@ static int get_build_id_32(void *page_addr, unsigned char
*build_id,
}
/* Parse build ID from 64-bit ELF */
-static int get_build_id_64(void *page_addr, unsigned char *build_id,
+static int get_build_id_64(const void *page_addr, unsigned char *build_id,
__u32 *size)
{
Elf64_Ehdr *ehdr = (Elf64_Ehdr *)page_addr;
--
https://chromeos.dev
[PATCH v4 10/13] scripts/decode_stacktrace.sh: Indicate 'auto' can be used for base path
Add "auto" to the usage message so that it's a little clearer that you
can pass "auto" as the second argument. When passing "auto" the script
tries to find the base path automatically instead of requiring it be
passed on the commandline. Also use [] to indicate the
variable argument and that it is optional so that we can differentiate
from the literal "auto" that should be passed.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Konstantin Khlebnikov
Cc: Sasha Levin
Signed-off-by: Stephen Boyd
---
scripts/decode_stacktrace.sh | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index 20b5af1ebe5e..5fbad61fe490 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -5,7 +5,7 @@
usage() {
echo "Usage:"
- echo " $0 -r | [base path] [modules path]"
+ echo " $0 -r | [|auto] []"
}
if [[ $1 == "-r" ]] ; then
--
https://chromeos.dev
[PATCH v4 13/13] kdump: Use vmlinux_build_id to simplify
We can use the vmlinux_build_id array here now instead of open coding
it. This mostly consolidates code.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Dave Young
Cc: Baoquan He
Cc: Vivek Goyal
Cc:
Signed-off-by: Stephen Boyd
---
include/linux/crash_core.h | 7 ++
kernel/crash_core.c| 50 ++
2 files changed, 4 insertions(+), 53 deletions(-)
diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
index 206bde8308b2..2174dab16ba9 100644
--- a/include/linux/crash_core.h
+++ b/include/linux/crash_core.h
@@ -39,7 +39,8 @@ phys_addr_t paddr_vmcoreinfo_note(void);
#define VMCOREINFO_OSRELEASE(value) \
vmcoreinfo_append_str("OSRELEASE=%s\n", value)
#define VMCOREINFO_BUILD_ID(value) \
- vmcoreinfo_append_str("BUILD-ID=%s\n", value)
+ BUILD_BUG_ON(ARRAY_SIZE(value) != BUILD_ID_SIZE_MAX); \
+ vmcoreinfo_append_str("BUILD-ID=%20phN\n", value)
#define VMCOREINFO_PAGESIZE(value) \
vmcoreinfo_append_str("PAGESIZE=%ld\n", value)
#define VMCOREINFO_SYMBOL(name) \
@@ -69,10 +70,6 @@ extern unsigned char *vmcoreinfo_data;
extern size_t vmcoreinfo_size;
extern u32 *vmcoreinfo_note;
-/* raw contents of kernel .notes section */
-extern const void __start_notes __weak;
-extern const void __stop_notes __weak;
-
Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
void *data, size_t data_len);
void final_note(Elf_Word *buf);
diff --git a/kernel/crash_core.c b/kernel/crash_core.c
index 825284baaf46..f7eb752560f1 100644
--- a/kernel/crash_core.c
+++ b/kernel/crash_core.c
@@ -4,6 +4,7 @@
* Copyright (C) 2002-2004 Eric Biederman
*/
+#include
#include
#include
#include
@@ -378,53 +379,6 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void)
}
EXPORT_SYMBOL(paddr_vmcoreinfo_note);
-#define NOTES_SIZE (&__stop_notes - &__start_notes)
-#define BUILD_ID_MAX SHA1_DIGEST_SIZE
-#define NT_GNU_BUILD_ID 3
-
-struct elf_note_section {
- struct elf_note n_hdr;
- u8 n_data[];
-};
-
-/*
- * Add build ID from .notes section as generated by the GNU ld(1)
- * or LLVM lld(1) --build-id option.
- */
-static void add_build_id_vmcoreinfo(void)
-{
- char build_id[BUILD_ID_MAX * 2 + 1];
- int n_remain = NOTES_SIZE;
-
- while (n_remain >= sizeof(struct elf_note)) {
- const struct elf_note_section *note_sec =
- &__start_notes + NOTES_SIZE - n_remain;
- const u32 n_namesz = note_sec->n_hdr.n_namesz;
-
- if (note_sec->n_hdr.n_type == NT_GNU_BUILD_ID &&
- n_namesz != 0 &&
- !strcmp((char *)¬e_sec->n_data[0], "GNU")) {
- if (note_sec->n_hdr.n_descsz <= BUILD_ID_MAX) {
- const u32 n_descsz = note_sec->n_hdr.n_descsz;
- const u8 *s = ¬e_sec->n_data[n_namesz];
-
- s = PTR_ALIGN(s, 4);
- bin2hex(build_id, s, n_descsz);
- build_id[2 * n_descsz] = '\0';
- VMCOREINFO_BUILD_ID(build_id);
- return;
- }
- pr_warn("Build ID is too large to include in
vmcoreinfo: %u > %u\n",
- note_sec->n_hdr.n_descsz,
- BUILD_ID_MAX);
- return;
- }
- n_remain -= sizeof(struct elf_note) +
- ALIGN(note_sec->n_hdr.n_namesz, 4) +
- ALIGN(note_sec->n_hdr.n_descsz, 4);
- }
-}
-
static int __init crash_save_vmcoreinfo_init(void)
{
vmcoreinfo_data = (unsigned char *)get_zeroed_page(GFP_KERNEL);
@@ -443,7 +397,7 @@ static int __init crash_save_vmcoreinfo_init(void)
}
VMCOREINFO_OSRELEASE(init_uts_ns.name.release);
- add_build_id_vmcoreinfo();
+ VMCOREINFO_BUILD_ID(vmlinux_build_id);
VMCOREINFO_PAGESIZE(PAGE_SIZE);
VMCOREINFO_SYMBOL(init_uts_ns);
--
https://chromeos.dev
[PATCH v4 09/13] scripts/decode_stacktrace.sh: Silence stderr messages from addr2line/nm
Sometimes if you're using tools that have linked things improperly or
have new features/sections that older tools don't expect you'll see
warnings printed to stderr. We don't really care about these warnings,
so let's just silence these messages to cleanup output of this script.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Konstantin Khlebnikov
Cc: Sasha Levin
Signed-off-by: Stephen Boyd
---
scripts/decode_stacktrace.sh | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index ca21f8bdf5f2..20b5af1ebe5e 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -74,7 +74,7 @@ find_module() {
find_module && return
if [[ $release == "" ]] ; then
- release=$(gdb -ex 'print init_uts_ns.name.release' -ex 'quit'
-quiet -batch "$vmlinux" | sed -n 's/\$1 = "\(.*\)".*/\1/p')
+ release=$(gdb -ex 'print init_uts_ns.name.release' -ex 'quit'
-quiet -batch "$vmlinux" 2>/dev/null | sed -n 's/\$1 = "\(.*\)".*/\1/p')
fi
for dn in {/usr/lib/debug,}/lib/modules/$release ; do
@@ -128,7 +128,7 @@ parse_symbol() {
if [[ "${cache[$module,$name]+isset}" == "isset" ]]; then
local base_addr=${cache[$module,$name]}
else
- local base_addr=$(nm "$objfile" | awk '$3 == "'$name'" && ($2
== "t" || $2 == "T") {print $1; exit}')
+ local base_addr=$(nm "$objfile" 2>/dev/null | awk '$3 ==
"'$name'" && ($2 == "t" || $2 == "T") {print $1; exit}')
if [[ $base_addr == "" ]] ; then
# address not found
return
@@ -152,7 +152,7 @@ parse_symbol() {
if [[ "${cache[$module,$address]+isset}" == "isset" ]]; then
local code=${cache[$module,$address]}
else
- local code=$(${CROSS_COMPILE}addr2line -i -e "$objfile"
"$address")
+ local code=$(${CROSS_COMPILE}addr2line -i -e "$objfile"
"$address" 2>/dev/null)
cache[$module,$address]=$code
fi
--
https://chromeos.dev
[PATCH v4 12/13] buildid: Fix kernel-doc notation
Kernel doc should use "Return:" instead of "Returns" to properly reflect the return values. Cc: Jiri Olsa Cc: Alexei Starovoitov Cc: Jessica Yu Cc: Evan Green Cc: Hsin-Yi Wang Signed-off-by: Stephen Boyd --- lib/buildid.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/buildid.c b/lib/buildid.c index df42282b36ff..ce88133f8dc4 100644 --- a/lib/buildid.c +++ b/lib/buildid.c @@ -121,7 +121,7 @@ static int get_build_id_64(const void *page_addr, unsigned char *build_id, * @build_id: buffer to store build id, at least BUILD_ID_SIZE long * @size: returns actual build id size in case of success * - * Returns 0 on success, otherwise error (< 0). + * Return: 0 on success, -EINVAL otherwise */ int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size) -- https://chromeos.dev
[PATCH v4 08/13] scripts/decode_stacktrace.sh: Support debuginfod
Now that stacktraces contain the build ID information we can update this
script to use debuginfod-find to locate the debuginfo for the vmlinux
and modules automatically. This can replace the existing code that
requires specifying a path to vmlinux or tries to find the vmlinux and
modules automatically by using the release number. Work it into the
script as a fallback option if the vmlinux isn't specified on the
commandline.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Konstantin Khlebnikov
Cc: Sasha Levin
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Signed-off-by: Stephen Boyd
---
scripts/decode_stacktrace.sh | 81 +++-
1 file changed, 70 insertions(+), 11 deletions(-)
diff --git a/scripts/decode_stacktrace.sh b/scripts/decode_stacktrace.sh
index 90398347e366..ca21f8bdf5f2 100755
--- a/scripts/decode_stacktrace.sh
+++ b/scripts/decode_stacktrace.sh
@@ -3,11 +3,10 @@
# (c) 2014, Sasha Levin
#set -x
-if [[ $# < 1 ]]; then
+usage() {
echo "Usage:"
echo " $0 -r | [base path] [modules path]"
- exit 1
-fi
+}
if [[ $1 == "-r" ]] ; then
vmlinux=""
@@ -24,6 +23,7 @@ if [[ $1 == "-r" ]] ; then
if [[ $vmlinux == "" ]] ; then
echo "ERROR! vmlinux image for release $release is not found"
>&2
+ usage
exit 2
fi
else
@@ -31,12 +31,35 @@ else
basepath=${2-auto}
modpath=$3
release=""
+ debuginfod=
+
+ # Can we use debuginfod-find?
+ if type debuginfod-find >/dev/null 2>&1 ; then
+ debuginfod=${1-only}
+ fi
+
+ if [[ $vmlinux == "" && -z $debuginfod ]] ; then
+ echo "ERROR! vmlinux image must be specified" >&2
+ usage
+ exit 1
+ fi
fi
declare -A cache
declare -A modcache
find_module() {
+ if [[ -n $debuginfod ]] ; then
+ if [[ -n $modbuildid ]] ; then
+ debuginfod-find debuginfo $modbuildid && return
+ fi
+
+ # Only using debuginfod so don't try to find vmlinux module path
+ if [[ $debuginfod == "only" ]] ; then
+ return
+ fi
+ fi
+
if [[ "$modpath" != "" ]] ; then
for fn in $(find "$modpath" -name "${module//_/[-_]}.ko*") ; do
if readelf -WS "$fn" | grep -qwF .debug_line ; then
@@ -150,6 +173,27 @@ parse_symbol() {
symbol="$segment$name ($code)"
}
+debuginfod_get_vmlinux() {
+ local vmlinux_buildid=${1##* }
+
+ if [[ $vmlinux != "" ]]; then
+ return
+ fi
+
+ if [[ $vmlinux_buildid =~ ^[0-9a-f]+ ]]; then
+ vmlinux=$(debuginfod-find debuginfo $vmlinux_buildid)
+ if [[ $? -ne 0 ]] ; then
+ echo "ERROR! vmlinux image not found via
debuginfod-find" >&2
+ usage
+ exit 2
+ fi
+ return
+ fi
+ echo "ERROR! Build ID for vmlinux not found. Try passing -r or
specifying vmlinux" >&2
+ usage
+ exit 2
+}
+
decode_code() {
local scripts=`dirname "${BASH_SOURCE[0]}"`
@@ -157,6 +201,14 @@ decode_code() {
}
handle_line() {
+ if [[ $basepath == "auto" && $vmlinux != "" ]] ; then
+ module=""
+ symbol="kernel_init+0x0/0x0"
+ parse_symbol
+ basepath=${symbol#kernel_init (}
+ basepath=${basepath%/init/main.c:*)}
+ fi
+
local words
# Tokenize
@@ -182,16 +234,28 @@ handle_line() {
fi
done
+ if [[ ${words[$last]} =~ ^[0-9a-f]+\] ]]; then
+ words[$last-1]="${words[$last-1]} ${words[$last]}"
+ unset words[$last]
+ last=$(( $last - 1 ))
+ fi
+
if [[ ${words[$last]} =~ \[([^]]+)\] ]]; then
module=${words[$last]}
module=${module#\[}
module=${module%\]}
+ modbuildid=${module#* }
+ module=${module% *}
+ if [[ $modbuildid == $module ]]; then
+ modbuildid=
+ fi
symbol=${words[$last-1]}
unset words[$last-1]
else
# The symbol is the last element, process it
symbol=${words[$last]}
module=
+ modbuildid=
fi
unset words[$last]
@@ -201,14 +265,6 @@ handle_line() {
echo &
[PATCH v4 07/13] x86/dumpstack: Use %pSb/%pBb for backtrace printing
Let's use the new printk formats to print the stacktrace entries when
printing a backtrace to the kernel logs. This will include any module's
build ID[1] in it so that offline/crash debugging can easily locate the
debuginfo for a module via something like debuginfod[2].
Cc: Thomas Gleixner
Cc: Ingo Molnar
Cc: Borislav Petkov
Cc:
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1]
Link: https://sourceware.org/elfutils/Debuginfod.html [2]
Signed-off-by: Stephen Boyd
---
arch/x86/kernel/dumpstack.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 299c20f0a38b..be2de39bf16f 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -69,7 +69,7 @@ static void printk_stack_address(unsigned long address, int
reliable,
const char *log_lvl)
{
touch_nmi_watchdog();
- printk("%s %s%pB\n", log_lvl, reliable ? "" : "? ", (void *)address);
+ printk("%s %s%pBb\n", log_lvl, reliable ? "" : "? ", (void *)address);
}
static int copy_code(struct pt_regs *regs, u8 *buf, unsigned long src,
@@ -143,9 +143,9 @@ void show_opcodes(struct pt_regs *regs, const char *loglvl)
void show_ip(struct pt_regs *regs, const char *loglvl)
{
#ifdef CONFIG_X86_32
- printk("%sEIP: %pS\n", loglvl, (void *)regs->ip);
+ printk("%sEIP: %pSb\n", loglvl, (void *)regs->ip);
#else
- printk("%sRIP: %04x:%pS\n", loglvl, (int)regs->cs, (void *)regs->ip);
+ printk("%sRIP: %04x:%pSb\n", loglvl, (int)regs->cs, (void *)regs->ip);
#endif
show_opcodes(regs, loglvl);
}
--
https://chromeos.dev
[PATCH v4 05/13] module: Add printk formats to add module build ID to stacktraces
Let's make kernel stacktraces easier to identify by including the build ID[1] of a module if the stacktrace is printing a symbol from a module. This makes it simpler for developers to locate a kernel module's full debuginfo for a particular stacktrace. Combined with scripts/decode_stracktrace.sh, a developer can download the matching debuginfo from a debuginfod[2] server and find the exact file and line number for the functions plus offsets in a stacktrace that match the module. This is especially useful for pstore crash debugging where the kernel crashes are recorded in something like console-ramoops and the recovery kernel/modules are different or the debuginfo doesn't exist on the device due to space concerns (the debuginfo can be too large for space limited devices). Originally, I put this on the %pS format, but that was quickly rejected given that %pS is used in other places such as ftrace where build IDs aren't meaningful. There was some discussions on the list to put every module build ID into the "Modules linked in:" section of the stacktrace message but that quickly becomes very hard to read once you have more than three or four modules linked in. It also provides too much information when we don't expect each module to be traversed in a stacktrace. Having the build ID for modules that aren't important just makes things messy. Splitting it to multiple lines for each module quickly explodes the number of lines printed in an oops too, possibly wrapping the warning off the console. And finally, trying to stash away each module used in a callstack to provide the ID of each symbol printed is cumbersome and would require changes to each architecture to stash away modules and return their build IDs once unwinding has completed. Instead, we opt for the simpler approach of introducing new printk formats '%pS[R]b' for "pointer symbolic backtrace with module build ID" and '%pBb' for "pointer backtrace with module build ID" and then updating the few places in the architecture layer where the stacktrace is printed to use this new format. Example: WARNING: CPU: 3 PID: 3373 at drivers/misc/lkdtm/bugs.c:83 lkdtm_WARNING+0x28/0x30 [lkdtm] Modules linked in: lkdtm rfcomm algif_hash algif_skcipher af_alg xt_cgroup uinput xt_MASQUERADE hci_uart CPU: 3 PID: 3373 Comm: bash Not tainted 5.11 #12 a8c0d47f7051f3e6670ceaea724af66a39c6cec8 Hardware name: Google Lazor (rev3+) with KB Backlight (DT) pstate: 0049 (nzcv daif +PAN -UAO -TCO BTYPE=--) pc : lkdtm_WARNING+0x28/0x30 [lkdtm] lr : lkdtm_do_action+0x24/0x40 [lkdtm] sp : ffc013febca0 x29: ffc013febca0 x28: ff88d9438040 x27: x26: x25: x24: ffdd0e9772c0 x23: 0020 x22: ffdd0e975366 x21: ffdd0e9772e0 x20: ffc013febde0 x19: 0008 x18: x17: x16: 0037 x15: ffdd102ab174 x14: 0003 x13: 0004 x12: x11: x10: x9 : 0001 x8 : ffdd0e979000 x7 : x6 : ffdd10ff6b54 x5 : x4 : x3 : ffc013feb938 x2 : ff89fef05a70 x1 : ff89feef5788 x0 : ffdd0e9772e0 Call trace: lkdtm_WARNING+0x28/0x30 [lkdtm 6c2215028606bda50de823490723dc4bc5bf46f9] direct_entry+0x16c/0x1b4 [lkdtm 6c2215028606bda50de823490723dc4bc5bf46f9] full_proxy_write+0x74/0xa4 vfs_write+0xec/0x2e8 ksys_write+0x84/0xf0 __arm64_sys_write+0x24/0x30 el0_svc_common+0xf4/0x1c0 do_el0_svc_compat+0x28/0x3c el0_svc_compat+0x10/0x1c el0_sync_compat_handler+0xa8/0xcc el0_sync_compat+0x178/0x180 ---[ end trace f89bc7f5417cbcc6 ]--- Cc: Jiri Olsa Cc: Alexei Starovoitov Cc: Jessica Yu Cc: Evan Green Cc: Hsin-Yi Wang Cc: Petr Mladek Cc: Steven Rostedt Cc: Sergey Senozhatsky Cc: Andy Shevchenko Cc: Rasmus Villemoes Cc: Cc: Matthew Wilcox Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1] Link: https://sourceware.org/elfutils/Debuginfod.html [2] Signed-off-by: Stephen Boyd --- Documentation/core-api/printk-formats.rst | 11 +++ include/linux/kallsyms.h | 20 - include/linux/module.h| 6 +- kernel/kallsyms.c | 95 ++- kernel/module.c | 24 +- lib/vsprintf.c| 8 +- 6 files changed, 139 insertions(+), 25 deletions(-) diff --git a/Documentation/core-api/printk-formats.rst b/Documentation/core-api/printk-formats.rst index 160e710d992f..5f60533f2a56 100644 --- a/Documentation/core-api/printk-formats.rst +++ b/Documentation/core-api/printk-formats.rst @@ -114,6 +114,17 @@ used when printing stack backtraces. The specifier takes into consideration the effect of compiler optimisations which may occur when tail-calls are used and marked with t
[PATCH v4 06/13] arm64: stacktrace: Use %pSb for backtrace printing
Let's use the new printk format to print the stacktrace entry when
printing a backtrace to the kernel logs. This will include any module's
build ID[1] in it so that offline/crash debugging can easily locate the
debuginfo for a module via something like debuginfod[2].
Cc: Catalin Marinas
Cc: Will Deacon
Cc:
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1]
Link: https://sourceware.org/elfutils/Debuginfod.html [2]
Signed-off-by: Stephen Boyd
---
arch/arm64/kernel/stacktrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/arm64/kernel/stacktrace.c b/arch/arm64/kernel/stacktrace.c
index ad20981dfda4..9d38da01ff98 100644
--- a/arch/arm64/kernel/stacktrace.c
+++ b/arch/arm64/kernel/stacktrace.c
@@ -129,7 +129,7 @@ NOKPROBE_SYMBOL(walk_stackframe);
static void dump_backtrace_entry(unsigned long where, const char *loglvl)
{
- printk("%s %pS\n", loglvl, (void *)where);
+ printk("%s %pSb\n", loglvl, (void *)where);
}
void dump_backtrace(struct pt_regs *regs, struct task_struct *tsk,
--
https://chromeos.dev
[PATCH v4 03/13] buildid: Stash away kernels build ID on init
Parse the kernel's build ID at initialization so that other code can
print a hex format string representation of the running kernel's build
ID. This will be used in the kdump and dump_stack code so that
developers can easily locate the vmlinux debug symbols for a
crash/stacktrace.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Dave Young
Cc: Baoquan He
Cc: Vivek Goyal
Cc:
Signed-off-by: Stephen Boyd
---
include/linux/buildid.h | 3 +++
init/main.c | 1 +
lib/buildid.c | 15 +++
3 files changed, 19 insertions(+)
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index ebce93f26d06..f375900cf9ed 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -10,4 +10,7 @@ int build_id_parse(struct vm_area_struct *vma, unsigned char
*build_id,
__u32 *size);
int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
+extern unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX];
+void init_vmlinux_build_id(void);
+
#endif
diff --git a/init/main.c b/init/main.c
index 53b278845b88..eaede2f41327 100644
--- a/init/main.c
+++ b/init/main.c
@@ -857,6 +857,7 @@ asmlinkage __visible void __init __no_sanitize_address
start_kernel(void)
set_task_stack_end_magic(&init_task);
smp_setup_processor_id();
debug_objects_early_init();
+ init_vmlinux_build_id();
cgroup_init_early();
diff --git a/lib/buildid.c b/lib/buildid.c
index 6aea1c4e5e85..1103ed46214f 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
#include
+#include
#include
#include
#include
@@ -172,3 +173,17 @@ int build_id_parse_buf(const void *buf, unsigned char
*build_id, u32 buf_size)
{
return parse_build_id_buf(build_id, NULL, buf, buf_size);
}
+
+unsigned char vmlinux_build_id[BUILD_ID_SIZE_MAX] __ro_after_init;
+
+/**
+ * init_vmlinux_build_id - Compute and stash the running kernel's build ID
+ */
+void __init init_vmlinux_build_id(void)
+{
+ extern const void __start_notes __weak;
+ extern const void __stop_notes __weak;
+ unsigned int size = &__stop_notes - &__start_notes;
+
+ build_id_parse_buf(&__start_notes, vmlinux_build_id, size);
+}
--
https://chromeos.dev
[PATCH v4 04/13] dump_stack: Add vmlinux build ID to stack traces
Add the running kernel's build ID[1] to the stacktrace information
header. This makes it simpler for developers to locate the vmlinux with
full debuginfo for a particular kernel stacktrace. Combined with
scripts/decode_stracktrace.sh, a developer can download the correct
vmlinux from a debuginfod[2] server and find the exact file and line
number for the functions plus offsets in a stacktrace.
This is especially useful for pstore crash debugging where the kernel
crashes are recorded in the pstore logs and the recovery kernel is
different or the debuginfo doesn't exist on the device due to space
concerns (the data can be large and a security concern). The stacktrace
can be analyzed after the crash by using the build ID to find the
matching vmlinux and understand where in the function something went
wrong.
Example stacktrace from lkdtm:
WARNING: CPU: 4 PID: 3255 at drivers/misc/lkdtm/bugs.c:83
lkdtm_WARNING+0x28/0x30 [lkdtm]
Modules linked in: lkdtm rfcomm algif_hash algif_skcipher af_alg xt_cgroup
uinput xt_MASQUERADE
CPU: 4 PID: 3255 Comm: bash Not tainted 5.11 #3
aa23f7a1231c229de205662d5a9e0d4c580f19a1
Hardware name: Google Lazor (rev3+) with KB Backlight (DT)
pstate: 0049 (nzcv daif +PAN -UAO -TCO BTYPE=--)
pc : lkdtm_WARNING+0x28/0x30 [lkdtm]
The hex string aa23f7a1231c229de205662d5a9e0d4c580f19a1 is the build ID,
following the kernel version number. Put it all behind a config option,
STACKTRACE_BUILD_ID, so that kernel developers can remove this
information if they decide it is too much.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Cc: Petr Mladek
Cc: Steven Rostedt
Cc: Andy Shevchenko
Cc: Matthew Wilcox
Link: https://fedoraproject.org/wiki/Releases/FeatureBuildId [1]
Link: https://sourceware.org/elfutils/Debuginfod.html [2]
Signed-off-by: Stephen Boyd
---
lib/Kconfig.debug | 11 +++
lib/dump_stack.c | 13 +++--
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index 2779c29d9981..5f883e50f406 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -35,6 +35,17 @@ config PRINTK_CALLER
no option to enable/disable at the kernel command line parameter or
sysfs interface.
+config STACKTRACE_BUILD_ID
+ bool "Show build ID information in stacktraces"
+ depends on PRINTK
+ help
+ Selecting this option adds build ID information for symbols in
+ stacktraces printed with the printk format '%p[SR]b'.
+
+ This option is intended for distros where debuginfo is not easily
+ accessible but can be downloaded given the build ID of the vmlinux or
+ kernel module where the function is located.
+
config CONSOLE_LOGLEVEL_DEFAULT
int "Default console loglevel (1-15)"
range 1 15
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index f5a33b6f773f..d685331b065f 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -5,6 +5,7 @@
*/
#include
+#include
#include
#include
#include
@@ -36,6 +37,14 @@ void __init dump_stack_set_arch_desc(const char *fmt, ...)
va_end(args);
}
+#if IS_ENABLED(CONFIG_STACKTRACE_BUILD_ID)
+#define BUILD_ID_FMT " %20phN"
+#define BUILD_ID_VAL vmlinux_build_id
+#else
+#define BUILD_ID_FMT "%s"
+#define BUILD_ID_VAL ""
+#endif
+
/**
* dump_stack_print_info - print generic debug info for dump_stack()
* @log_lvl: log level
@@ -45,13 +54,13 @@ void __init dump_stack_set_arch_desc(const char *fmt, ...)
*/
void dump_stack_print_info(const char *log_lvl)
{
- printk("%sCPU: %d PID: %d Comm: %.20s %s%s %s %.*s\n",
+ printk("%sCPU: %d PID: %d Comm: %.20s %s%s %s %.*s" BUILD_ID_FMT "\n",
log_lvl, raw_smp_processor_id(), current->pid, current->comm,
kexec_crash_loaded() ? "Kdump: loaded " : "",
print_tainted(),
init_utsname()->release,
(int)strcspn(init_utsname()->version, " "),
- init_utsname()->version);
+ init_utsname()->version, BUILD_ID_VAL);
if (dump_stack_arch_desc_str[0] != '\0')
printk("%sHardware name: %s\n",
--
https://chromeos.dev
[PATCH v4 02/13] buildid: Add API to parse build ID out of buffer
Add an API that can parse the build ID out of a buffer, instead of a
vma, to support printing a kernel module's build ID for stack traces.
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Signed-off-by: Stephen Boyd
---
include/linux/buildid.h | 1 +
lib/buildid.c | 50 ++---
2 files changed, 38 insertions(+), 13 deletions(-)
diff --git a/include/linux/buildid.h b/include/linux/buildid.h
index 40232f90db6e..ebce93f26d06 100644
--- a/include/linux/buildid.h
+++ b/include/linux/buildid.h
@@ -8,5 +8,6 @@
int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
__u32 *size);
+int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size);
#endif
diff --git a/lib/buildid.c b/lib/buildid.c
index e014636ec3eb..6aea1c4e5e85 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -2,30 +2,23 @@
#include
#include
+#include
#include
#define BUILD_ID 3
+
/*
* Parse build id from the note segment. This logic can be shared between
* 32-bit and 64-bit system, because Elf32_Nhdr and Elf64_Nhdr are
* identical.
*/
-static inline int parse_build_id(void *page_addr,
-unsigned char *build_id,
-__u32 *size,
-void *note_start,
-Elf32_Word note_size)
+static int parse_build_id_buf(unsigned char *build_id,
+ __u32 *size,
+ const void *note_start,
+ Elf32_Word note_size)
{
Elf32_Word note_offs = 0, new_offs;
- /* check for overflow */
- if (note_start < page_addr || note_start + note_size < note_start)
- return -EINVAL;
-
- /* only supports note that fits in the first page */
- if (note_start + note_size > page_addr + PAGE_SIZE)
- return -EINVAL;
-
while (note_offs + sizeof(Elf32_Nhdr) < note_size) {
Elf32_Nhdr *nhdr = (Elf32_Nhdr *)(note_start + note_offs);
@@ -50,9 +43,27 @@ static inline int parse_build_id(void *page_addr,
break;
note_offs = new_offs;
}
+
return -EINVAL;
}
+static inline int parse_build_id(void *page_addr,
+unsigned char *build_id,
+__u32 *size,
+void *note_start,
+Elf32_Word note_size)
+{
+ /* check for overflow */
+ if (note_start < page_addr || note_start + note_size < note_start)
+ return -EINVAL;
+
+ /* only supports note that fits in the first page */
+ if (note_start + note_size > page_addr + PAGE_SIZE)
+ return -EINVAL;
+
+ return parse_build_id_buf(build_id, size, note_start, note_size);
+}
+
/* Parse build ID from 32-bit ELF */
static int get_build_id_32(void *page_addr, unsigned char *build_id,
__u32 *size)
@@ -148,3 +159,16 @@ int build_id_parse(struct vm_area_struct *vma, unsigned
char *build_id,
put_page(page);
return ret;
}
+
+/**
+ * build_id_parse_buf - Get build ID from a buffer
+ * @buf: Elf note section(s) to parse
+ * @buf_size: Size of @buf in bytes
+ * @build_id: Build ID parsed from @buf, at least BUILD_ID_SIZE_MAX long
+ *
+ * Return: 0 on success, -EINVAL otherwise
+ */
+int build_id_parse_buf(const void *buf, unsigned char *build_id, u32 buf_size)
+{
+ return parse_build_id_buf(build_id, NULL, buf, buf_size);
+}
--
https://chromeos.dev
[PATCH v4 01/13] buildid: Only consider GNU notes for build ID parsing
Some kernel elf files have various notes that also happen to have an elf
note type of '3', which matches NT_GNU_BUILD_ID but the note name isn't
"GNU". For example, this note trips up the existing logic:
Owner Data size Description
Xen0x0008 Unknown note type: (0x0003) description data: 00 00 00
ff80
Let's make sure that it is a GNU note when parsing the build ID so that
we can use this function to parse a vmlinux's build ID too.
Reported-by: Petr Mladek
Cc: Jiri Olsa
Cc: Alexei Starovoitov
Cc: Jessica Yu
Cc: Evan Green
Cc: Hsin-Yi Wang
Fixes: bd7525dacd7e ("bpf: Move stack_map_get_build_id into lib")
Signed-off-by: Stephen Boyd
---
lib/buildid.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/lib/buildid.c b/lib/buildid.c
index 6156997c3895..e014636ec3eb 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -31,6 +31,7 @@ static inline int parse_build_id(void *page_addr,
if (nhdr->n_type == BUILD_ID &&
nhdr->n_namesz == sizeof("GNU") &&
+ !strcmp((char *)(nhdr + 1), "GNU") &&
nhdr->n_descsz > 0 &&
nhdr->n_descsz <= BUILD_ID_SIZE_MAX) {
memcpy(build_id,
--
https://chromeos.dev
[PATCH v4 00/13] Add build ID to stacktraces
This series adds the kernel's build ID[1] to the stacktrace header printed in oops messages, warnings, etc. and the build ID for any module that appears in the stacktrace after the module name. The goal is to make the stacktrace more self-contained and descriptive by including the relevant build IDs in the kernel logs when something goes wrong. This can be used by post processing tools like script/decode_stacktrace.sh and kernel developers to easily locate the debug info associated with a kernel crash and line up what line and file things started falling apart at. To show how this can be used I've included a patch to decode_stacktrace.sh that downloads the debuginfo from a debuginfod server. This also includes some patches to make the buildid.c file use more const arguments and consolidate logic into buildid.c from kdump. These are left to the end as they were mostly cleanup patches. I don't know who exactly maintains this so I guess Andrew is the best option to merge all this code. Here's an example lkdtm stacktrace on arm64. WARNING: CPU: 4 PID: 3255 at drivers/misc/lkdtm/bugs.c:83 lkdtm_WARNING+0x28/0x30 [lkdtm] Modules linked in: lkdtm rfcomm algif_hash algif_skcipher af_alg xt_cgroup uinput xt_MASQUERADE CPU: 4 PID: 3255 Comm: bash Not tainted 5.11 #3 aa23f7a1231c229de205662d5a9e0d4c580f19a1 Hardware name: Google Lazor (rev3+) with KB Backlight (DT) pstate: 0049 (nzcv daif +PAN -UAO -TCO BTYPE=--) pc : lkdtm_WARNING+0x28/0x30 [lkdtm] lr : lkdtm_do_action+0x24/0x40 [lkdtm] sp : ffc0134fbca0 x29: ffc0134fbca0 x28: ff92d53ba240 x27: x26: x25: x24: ffe3622352c0 x23: 0020 x22: ffe362233366 x21: ffe3622352e0 x20: ffc0134fbde0 x19: 0008 x18: x17: ff929b6536fc x16: x15: x14: 0012 x13: ffe380ed892c x12: ffe381d05068 x11: x10: x9 : 0001 x8 : ffe362237000 x7 : x6 : x5 : x4 : 0001 x3 : 0008 x2 : ff93fef25a70 x1 : ff93fef15788 x0 : ffe3622352e0 Call trace: lkdtm_WARNING+0x28/0x30 [lkdtm ed5019fdf5e53be37cb1ba7899292d7e143b259e] direct_entry+0x16c/0x1b4 [lkdtm ed5019fdf5e53be37cb1ba7899292d7e143b259e] full_proxy_write+0x74/0xa4 vfs_write+0xec/0x2e8 ksys_write+0x84/0xf0 __arm64_sys_write+0x24/0x30 el0_svc_common+0xf4/0x1c0 do_el0_svc_compat+0x28/0x3c el0_svc_compat+0x10/0x1c el0_sync_compat_handler+0xa8/0xcc el0_sync_compat+0x178/0x180 ---[ end trace 3d95032303e59e68 ]--- Changes from v3 (https://lore.kernel.org/r/20210331030520.3816265-1-swb...@chromium.org): * Fixed compilation warnings due to config changes * Fixed kernel-doc on init_vmlinx_build_id() * Totally removed add_build_id_vmcoreinfo() * Added another printk format %pBb to help x86 print backtraces * Some BUILD_BUG_ON() checks to make sure the buildid doesn't get bigger or smaller Changes from v2 (https://lore.kernel.org/r/20210324020443.1815557-1-swb...@chromium.org): * Renamed symbol printing function to indicate build IDness * Put build ID information behind Kconfig knob * Build ID for vmlinux is calculated in early init instead of on demand * printk format is %pS[R]b Changes from v1 (https://lore.kernel.org/r/20210301174749.1269154-1-swb...@chromium.org): * New printk format %pSb and %pSr * Return binary format instead of hex format string from build ID APIs * Some new patches to cleanup buildid/decode_stacktrace.sh * A new patch to decode_stacktrace.sh to parse output [1] https://fedoraproject.org/wiki/Releases/FeatureBuildId Cc: Alexei Starovoitov Cc: Andy Shevchenko Cc: Baoquan He Cc: Borislav Petkov Cc: Catalin Marinas Cc: Dave Young Cc: Evan Green Cc: Hsin-Yi Wang Cc: Ingo Molnar Cc: Jessica Yu Cc: Jiri Olsa Cc: Cc: Konstantin Khlebnikov Cc: Cc: Cc: Cc: Matthew Wilcox Cc: Petr Mladek Cc: Rasmus Villemoes Cc: Sasha Levin Cc: Sergey Senozhatsky Cc: Steven Rostedt Cc: Thomas Gleixner Cc: Vivek Goyal Cc: Will Deacon Cc: Cc: Christoph Hellwig Cc: peter enderborg Stephen Boyd (13): buildid: Only consider GNU notes for build ID parsing buildid: Add API to parse build ID out of buffer buildid: Stash away kernels build ID on init dump_stack: Add vmlinux build ID to stack traces module: Add printk formats to add module build ID to stacktraces arm64: stacktrace: Use %pSb for backtrace printing x86/dumpstack: Use %pSb/%pBb for backtrace printing scripts/decode_stacktrace.sh: Support debuginfod scripts/decode_stacktrace.sh: Silence stderr messages from addr2line/nm scripts/decode_stacktrace.sh: Indicate 'auto' can be used for base path buildid: Mark some arguments const buildid: Fix kernel-doc notation kdump: Use vmlinux_build_id to simplify Documentation/core-api/prin
[GIT PULL] clk fixes for v5.12-rc6
The following changes since commit 148ddaa89d4a0a927c4353398096cc33687755c1:
clk: qcom: gcc-sc7180: Use floor ops for the correct sdcc1 clk (2021-03-13
13:00:05 -0800)
are available in the Git repository at:
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git
tags/clk-fixes-for-linus
for you to fetch changes up to 50ce6826a48f119baf2794fa384a64efe9bd84a5:
clk: fixed: fix double free in resource managed fixed-factor clock
(2021-04-07 16:01:25 -0700)
Here's the latest pile of clk driver and clk framework fixes for this
release.
- Two clk framework fixes for a long standing issue in
clk_notifier_{register,unregister}() where we used a
pointer that was for a struct containing a list head
when there was no container struct
- A compile warning fix for socfpga that's good to have
- A double free problem with devm registered fixed factor clks
- One last fix to the Qualcomm camera clk driver to use the
right clk ops so clks don't get stuck and stop working
because the firmware takes them for a ride.
Dmitry Baryshkov (1):
clk: fixed: fix double free in resource managed fixed-factor clock
Krzysztof Kozlowski (1):
clk: socfpga: fix iomem pointer cast on 64-bit
Lukasz Bartosik (2):
clk: fix invalid usage of list cursor in register
clk: fix invalid usage of list cursor in unregister
Taniya Das (1):
clk: qcom: camcc: Update the clock ops for the SC7180
drivers/clk/clk-fixed-factor.c | 9 +++-
drivers/clk/clk.c | 47 +-
drivers/clk/qcom/camcc-sc7180.c | 50 -
drivers/clk/socfpga/clk-gate.c | 2 +-
4 files changed, 55 insertions(+), 53 deletions(-)
--
https://git.kernel.org/pub/scm/linux/kernel/git/clk/linux.git/
https://git.kernel.org/pub/scm/linux/kernel/git/sboyd/spmi.git
Re: [PATCH v11 0/6] MIPS: ralink: add CPU clock detection and clock driver for MT7621
Quoting Sergio Paracuellos (2021-04-09 11:25:24) > Hi Stephen, > > On Fri, Apr 9, 2021 at 8:17 PM Stephen Boyd wrote: > > > > Quoting Sergio Paracuellos (2021-03-23 01:13:22) > > > On Tue, Mar 9, 2021 at 6:22 AM Sergio Paracuellos > > > wrote: > > > > > > > > Changes in v11: > > > > - Collect Rob's Reviewed-by in bindings documentation patch. > > > > - Fix MAINTAINERS patch using file 'mediatek,mt7621-sysc.yaml' > > > >for documentation bindings. > > > > > > Something still missing or something that is needed to be fixed to get > > > this series applied through your tree? > > > > > > Thanks in advance for your time. > > > > > > > Sorry I missed this series. I thought it was going through another tree. > > It can merge through clk tree. Just a few nits on the clk driver patch > > but otherwise I've merged the first two patches. If you resend in the > > next few days it would be great. Thanks. > > I will hopefully do during this weekend. Since you already merge the > first two patches, the remaining four should be sent as v12, right? Yes. I'll push it out to kernel.org shortly.
Re: [PATCH v2 -next 3/3] clk: qcom: apss-ipq-pll: Add missing MODULE_DEVICE_TABLE
Quoting Chen Hui (2021-04-09 01:23:52)
> CONFIG_IPQ_APSS_PLL is tristate option and therefore this driver can
> be compiled as a module. This patch adds missing MODULE_DEVICE_TABLE
> definition which generates correct modalias for automatic loading of
> this driver when it is built as an external module.
>
> Fixes: ecd2bacfbbc4 ("clk: qcom: Add ipq apss pll driver")
> Signed-off-by: Chen Hui
> Reviewed-by: Manivannan Sadhasivam
> ---
Applied to clk-next
Re: [PATCH v2 -next 2/3] clk: qcom: a53-pll: Add missing MODULE_DEVICE_TABLE
Quoting Chen Hui (2021-04-09 01:23:51)
> CONFIG_QCOM_A53PLL is tristate option and therefore this driver can be
> compiled as a module. This patch adds missing MODULE_DEVICE_TABLE
> definition which generates correct modalias for automatic loading of
> this driver when it is built as an external module.
>
> Fixes: 0c6ab1b8f894 ("clk: qcom: Add A53 PLL support")
> Signed-off-by: Chen Hui
> Reviewed-by: Manivannan Sadhasivam
> ---
Applied to clk-next
Re: [PATCH v2 -next 1/3] clk: qcom: a7-pll: Add missing MODULE_DEVICE_TABLE
Quoting Chen Hui (2021-04-09 01:23:50)
> CONFIG_QCOM_A7PLL is tristate option and therefore this driver can be
> compiled as a module. This patch adds missing MODULE_DEVICE_TABLE
> definition which generates correct modalias for automatic loading of
> this driver when it is built as an external module.
>
> Fixes: 5a5223ffd7ef ("clk: qcom: Add A7 PLL support")
> Signed-off-by: Chen Hui
> Reviewed-by: Manivannan Sadhasivam
> ---
Applied to clk-next
Re: [PATCH v11 2/6] dt: bindings: add mt7621-sysc device tree binding documentation
Quoting Sergio Paracuellos (2021-03-08 21:22:22) > Adds device tree binding documentation for clocks in the > MT7621 SOC. > > Reviewed-by: Rob Herring > Signed-off-by: Sergio Paracuellos > --- Applied to clk-next
