* Re: [PATCH v14 2/4] dt-bindings: remoteproc: add Tightly Coupled Memory (TCM) bindings
From: Krzysztof Kozlowski @ 2024-04-09 7:29 UTC (permalink / raw)
To: Tanmay Shah, andersson, mathieu.poirier, robh,
krzysztof.kozlowski+dt, conor+dt, michal.simek, ben.levinsky
Cc: linux-remoteproc, devicetree, linux-arm-kernel, linux-kernel,
Radhey Shyam Pandey
In-Reply-To: <20240408205313.3552165-3-tanmay.shah@amd.com>
On 08/04/2024 22:53, Tanmay Shah wrote:
> From: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
>
> Introduce bindings for TCM memory address space on AMD-xilinx Zynq
> UltraScale+ platform. It will help in defining TCM in device-tree
> and make it's access platform agnostic and data-driven.
>
> Tightly-coupled memories(TCMs) are low-latency memory that provides
> predictable instruction execution and predictable data load/store
> timing. Each Cortex-R5F processor contains two 64-bit wide 64 KB memory
> banks on the ATCM and BTCM ports, for a total of 128 KB of memory.
>
> The TCM resources(reg, reg-names and power-domain) are documented for
> each TCM in the R5 node. The reg and reg-names are made as required
> properties as we don't want to hardcode TCM addresses for future
> platforms and for zu+ legacy implementation will ensure that the
> old dts w/o reg/reg-names works and stable ABI is maintained.
>
> It also extends the examples for TCM split and lockstep modes.
>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>
> Signed-off-by: Tanmay Shah <tanmay.shah@amd.com>
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 1/9] dt-bindings: rtc: orion-rtc: move to trivial-rtc
From: Krzysztof Kozlowski @ 2024-04-09 7:31 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-1-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> This RTC requires a compatible, a reg and a single interrupt,
> which makes it suitable for a direct conversion into trivial-rtc.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> Documentation/devicetree/bindings/rtc/orion-rtc.txt | 18 ------------------
> Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 2 ++
> 2 files changed, 2 insertions(+), 18 deletions(-)
I assume you checked if any entry in MAINTAINERS needs updating.
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 2/9] dt-bindings: rtc: google,goldfish-rtc: move to trivial-rtc
From: Krzysztof Kozlowski @ 2024-04-09 7:32 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-2-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> This RTC requires a compatible, a reg and a single interrupt,
> which makes it suitable for a direct conversion into trivial-rtc.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH] arm64: dts: renesas: s4sk: Fix ethernet0 alias
From: Geert Uytterhoeven @ 2024-04-09 7:33 UTC (permalink / raw)
To: Kuninori Morimoto, Magnus Damm
Cc: linux-renesas-soc, linux-arm-kernel, Geert Uytterhoeven
U-Boot uses "ethernet0", not "eth0".
While at it, fix nearby whitespace errors (TAB instead of space before
equal sign).
Fixes: 93be50c7ff8e8087 ("arm64: dts: renesas: Add R-Car S4 Starter Kit support")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
To be queued in renesas-devel for v6.10.
---
arch/arm64/boot/dts/renesas/r8a779f4-s4sk.dts | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/arm64/boot/dts/renesas/r8a779f4-s4sk.dts b/arch/arm64/boot/dts/renesas/r8a779f4-s4sk.dts
index abfda5c6ca16922e..bc65a7b4d999740c 100644
--- a/arch/arm64/boot/dts/renesas/r8a779f4-s4sk.dts
+++ b/arch/arm64/boot/dts/renesas/r8a779f4-s4sk.dts
@@ -14,9 +14,9 @@ / {
compatible = "renesas,s4sk", "renesas,r8a779f4", "renesas,r8a779f0";
aliases {
- serial0 = &hscif0;
- serial1 = &hscif1;
- eth0 = &rswitch;
+ serial0 = &hscif0;
+ serial1 = &hscif1;
+ ethernet0 = &rswitch;
};
chosen {
--
2.34.1
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH 3/9] dt-bindings: rtc: lpc32xx-rtc: move to trivial-rtc
From: Krzysztof Kozlowski @ 2024-04-09 7:34 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-3-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> This RTC requires a compatible, a reg and a single interrupt,
> which makes it suitable for a direct conversion into trivial-rtc.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> Documentation/devicetree/bindings/rtc/lpc32xx-rtc.txt | 15 ---------------
> Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 2 ++
> 2 files changed, 2 insertions(+), 15 deletions(-)
This one no... and if you tested DTS you would see errors, although you
need to test specific lpc config, not multi_v7.
It does not look like you tested the DTS against bindings. Please run
`make dtbs_check W=1` (see
Documentation/devicetree/bindings/writing-schema.rst or
https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
for instructions).
Anyway, you *must* check all DTS before moving anything to trivial.
Does it mean all other bindings were not checked against DTS at all?
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 4/9] dt-bindings: rtc: maxim,ds1742: move to trivial-rtc
From: Krzysztof Kozlowski @ 2024-04-09 7:35 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-4-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> This RTC only requires the compatible a reg and properties,
> which makes it suitable for a direct conversion into trivial-rtc.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 5/9] dt-bindings: rtc: rtc-aspeed: move to trivial-rtc
From: Krzysztof Kozlowski @ 2024-04-09 7:36 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-5-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> The RTCs documented in this binding require a compatible, a reg
> and a single interrupt, which make them suitable for a direct
> conversion into trivial-rtc.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 6/9] dt-bindings: rtc: pxa-rtc: convert to dtschema
From: Krzysztof Kozlowski @ 2024-04-09 7:37 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-6-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> Convert existing binding to dtschema to support validation.
>
> The missing 'reg' and 'interrupts' properties have been added, taking
> the 2 supported interrupts into account to fix the example.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 1/2] ACPI/AEST: Initial AEST driver
From: Ruidong Tian @ 2024-04-09 7:37 UTC (permalink / raw)
To: Baolin Wang, catalin.marinas, will, lpieralisi, guohanjun,
sudeep.holla, xueshuai, linux-kernel, linux-acpi,
linux-arm-kernel, rafael, lenb, tony.luck, bp, linux-edac
Cc: tianruidong, Tyler Baicar
In-Reply-To: <0f0e88a3-a617-408f-8f1a-3af7a0f99337@linux.alibaba.com>
在 2024/3/21 11:52, Baolin Wang 写道:
>
>
> On 2024/3/21 10:53, Ruidong Tian wrote:
>> From: Tyler Baicar <baicar@os.amperecomputing.com>
>>
>> Add support for parsing the ARM Error Source Table and basic handling of
>> errors reported through both memory mapped and system register
>> interfaces.
>>
>> Assume system register interfaces are only registered with private
>> peripheral interrupts (PPIs); otherwise there is no guarantee the
>> core handling the error is the core which took the error and has the
>> syndrome info in its system registers.
>>
>> In kernel-first mode, all configuration is controlled by kernel, include
>> CE ce_threshold and interrupt enable/disable.
>>
>> All detected errors will be processed as follow:
>> - CE, DE: use a workqueue to log this hardware errors.
>> - UER, UEO: log it and call memory_failure in workquee.
>> - UC, UEU: panic in irq context.
>>
>> Signed-off-by: Tyler Baicar <baicar@os.amperecomputing.com>
>> Signed-off-by: Ruidong Tian <tianruidong@linux.alibaba.com>
>> ---
>> MAINTAINERS | 11 +
>> arch/arm64/include/asm/ras.h | 71 +++
>> drivers/acpi/arm64/Kconfig | 10 +
>> drivers/acpi/arm64/Makefile | 1 +
>> drivers/acpi/arm64/aest.c | 834 +++++++++++++++++++++++++++++++++++
>> include/linux/acpi_aest.h | 92 ++++
>> include/linux/cpuhotplug.h | 1 +
>> 7 files changed, 1020 insertions(+)
>> create mode 100644 arch/arm64/include/asm/ras.h
>> create mode 100644 drivers/acpi/arm64/aest.c
>> create mode 100644 include/linux/acpi_aest.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index dd5de540ec0b..34900d4bb677 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -330,6 +330,17 @@ L: linux-arm-kernel@lists.infradead.org
>> (moderated for non-subscribers)
>> S: Maintained
>> F: drivers/acpi/arm64
>> +ACPI AEST
>> +M: Tyler Baicar <baicar@os.amperecomputing.com>
>> +M: Ruidong Tian <tianruidond@linux.alibaba.com>
>> +L: linux-acpi@vger.kernel.org
>> +L: linux-arm-kernel@lists.infradead.org
>> +S: Supported
>> +F: arch/arm64/include/asm/ras.h
>> +F: drivers/acpi/arm64/aest.c
>> +F: include/linux/acpi_aest.h
>> +
>> +
>> ACPI FOR RISC-V (ACPI/riscv)
>> M: Sunil V L <sunilvl@ventanamicro.com>
>> L: linux-acpi@vger.kernel.org
>> diff --git a/arch/arm64/include/asm/ras.h b/arch/arm64/include/asm/ras.h
>> new file mode 100644
>> index 000000000000..04667f0de30f
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/ras.h
>> @@ -0,0 +1,71 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __ASM_RAS_H
>> +#define __ASM_RAS_H
>> +
>> +#include <linux/types.h>
>> +#include <linux/bits.h>
>> +
>> +/* ERR<n>FR */
>> +#define ERR_FR_RP BIT(15)
>> +#define ERR_FR_CEC GENMASK_ULL(14, 12)
>> +
>> +#define ERR_FR_RP_SINGLE_COUNTER 0
>> +#define ERR_FR_RP_DOUBLE_COUNTER 1
>> +
>> +#define ERR_FR_CEC_0B_COUNTER 0
>> +#define ERR_FR_CEC_8B_COUNTER BIT(1)
>> +#define ERR_FR_CEC_16B_COUNTER BIT(2)
>> +
>> +/* ERR<n>STATUS */
>> +#define ERR_STATUS_AV BIT(31)
>> +#define ERR_STATUS_V BIT(30)
>> +#define ERR_STATUS_UE BIT(29)
>> +#define ERR_STATUS_ER BIT(28)
>> +#define ERR_STATUS_OF BIT(27)
>> +#define ERR_STATUS_MV BIT(26)
>> +#define ERR_STATUS_CE (BIT(25) | BIT(24))
>> +#define ERR_STATUS_DE BIT(23)
>> +#define ERR_STATUS_PN BIT(22)
>> +#define ERR_STATUS_UET (BIT(21) | BIT(20))
>> +#define ERR_STATUS_CI BIT(19)
>> +#define ERR_STATUS_IERR GENMASK_ULL(15, 8)
>> +#define ERR_STATUS_SERR GENMASK_ULL(7, 0)
>> +
>> +/* These bit is write-one-to-clear */
>> +#define ERR_STATUS_W1TC (ERR_STATUS_AV | ERR_STATUS_V |
>> ERR_STATUS_UE | \
>> + ERR_STATUS_ER | ERR_STATUS_OF | ERR_STATUS_MV | \
>> + ERR_STATUS_CE | ERR_STATUS_DE | ERR_STATUS_PN | \
>> + ERR_STATUS_UET | ERR_STATUS_CI)
>> +
>> +#define ERR_STATUS_UET_UC 0
>> +#define ERR_STATUS_UET_UEU 1
>> +#define ERR_STATUS_UET_UER 2
>> +#define ERR_STATUS_UET_UEO 3
>> +
>> +/* ERR<n>CTLR */
>> +#define ERR_CTLR_FI BIT(3)
>> +#define ERR_CTLR_UI BIT(2)
>> +
>> +/* ERR<n>ADDR */
>> +#define ERR_ADDR_AI BIT(61)
>> +#define ERR_ADDR_PADDR GENMASK_ULL(55, 0)
>> +
>> +/* ERR<n>MISC0 */
>> +
>> +/* ERR<n>FR.CEC == 0b010, ERR<n>FR.RP == 0 */
>> +#define ERR_MISC0_8B_OF BIT(39)
>> +#define ERR_MISC0_8B_CEC GENMASK_ULL(38, 32)
>> +
>> +/* ERR<n>FR.CEC == 0b100, ERR<n>FR.RP == 0 */
>> +#define ERR_MISC0_16B_OF BIT(47)
>> +#define ERR_MISC0_16B_CEC GENMASK_ULL(46, 32)
>> +
>> +struct ras_ext_regs {
>> + u64 err_fr;
>> + u64 err_ctlr;
>> + u64 err_status;
>> + u64 err_addr;
>> + u64 err_misc[4];
>> +};
>> +
>> +#endif /* __ASM_RAS_H */
>> diff --git a/drivers/acpi/arm64/Kconfig b/drivers/acpi/arm64/Kconfig
>> index b3ed6212244c..639db671c5cf 100644
>> --- a/drivers/acpi/arm64/Kconfig
>> +++ b/drivers/acpi/arm64/Kconfig
>> @@ -21,3 +21,13 @@ config ACPI_AGDI
>> config ACPI_APMT
>> bool
>> +
>> +config ACPI_AEST
>> + bool "ARM Error Source Table Support"
>> +
>> + help
>> + The Arm Error Source Table (AEST) provides details on ACPI
>> + extensions that enable kernel-first handling of errors in a
>> + system that supports the Armv8 RAS extensions.
>> +
>> + If set, the kernel will report and log hardware errors.
>> diff --git a/drivers/acpi/arm64/Makefile b/drivers/acpi/arm64/Makefile
>> index 143debc1ba4a..b5b740058c46 100644
>> --- a/drivers/acpi/arm64/Makefile
>> +++ b/drivers/acpi/arm64/Makefile
>> @@ -5,3 +5,4 @@ obj-$(CONFIG_ACPI_GTDT) += gtdt.o
>> obj-$(CONFIG_ACPI_APMT) += apmt.o
>> obj-$(CONFIG_ARM_AMBA) += amba.o
>> obj-y += dma.o init.o
>> +obj-$(CONFIG_ACPI_AEST) += aest.o
>> diff --git a/drivers/acpi/arm64/aest.c b/drivers/acpi/arm64/aest.c
>> new file mode 100644
>> index 000000000000..ab17aa5f5997
>> --- /dev/null
>> +++ b/drivers/acpi/arm64/aest.c
>> @@ -0,0 +1,834 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * ARM Error Source Table Support
>> + *
>> + * Copyright (c) 2021, Ampere Computing LLC
>> + * Copyright (c) 2021-2024, Alibaba Group.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include <linux/acpi_aest.h>
>> +#include <linux/cpuhotplug.h>
>> +#include <linux/kernel.h>
>> +#include <linux/genalloc.h>
>> +#include <linux/llist.h>
>> +#include <acpi/actbl.h>
>> +#include <asm/ras.h>
>> +
>> +#undef pr_fmt
>> +#define pr_fmt(fmt) "ACPI AEST: " fmt
>> +
>> +#define CASE_READ(res, x) \
>> + case (x): { \
>> + res = read_sysreg_s(SYS_##x##_EL1); \
>> + break; \
>> + }
>> +
>> +#define CASE_WRITE(val, x) \
>> + case (x): { \
>> + write_sysreg_s((val), SYS_##x##_EL1); \
>> + break; \
>> + }
>> +
>> +#define for_each_implemented_record(index, node) \
>> + for ((index) = node->interface.record_start; \
>> + (index) < node->interface.record_end; \
>> + (index)++)
>> +
>> +#define AEST_LOG_PREFIX_BUFFER 64
>> +
>> +/*
>> + * This memory pool is only to be used to save AEST node in AEST irq
>> context.
>> + * There can be 500 AEST node at most.
>> + */
>> +#define AEST_NODE_ALLOCED_MAX 500
>> +
>> +static struct acpi_table_header *aest_table;
>> +
>> +static struct aest_node __percpu **aest_ppi_data;
>> +
>> +static int *ppi_irqs;
>> +static u8 num_ppi;
>> +static u8 ppi_idx;
>> +
>> +static struct work_struct aest_work;
>> +
>> +static struct gen_pool *aest_node_pool;
>> +static struct llist_head aest_node_llist;
>> +
>> +static u64 aest_sysreg_read(u64 __unused, u32 offset)
>> +{
>> + u64 res;
>> +
>> + switch (offset) {
>> + CASE_READ(res, ERXFR)
>> + CASE_READ(res, ERXCTLR)
>> + CASE_READ(res, ERXSTATUS)
>> + CASE_READ(res, ERXADDR)
>> + CASE_READ(res, ERXMISC0)
>> + CASE_READ(res, ERXMISC1)
>> + CASE_READ(res, ERXMISC2)
>> + CASE_READ(res, ERXMISC3)
>> + default :
>> + res = 0;
>> + }
>> + return res;
>> +}
>> +
>> +static void aest_sysreg_write(u64 base, u32 offset, u64 val)
>> +{
>> + switch (offset) {
>> + CASE_WRITE(val, ERXFR)
>> + CASE_WRITE(val, ERXCTLR)
>> + CASE_WRITE(val, ERXSTATUS)
>> + CASE_WRITE(val, ERXADDR)
>> + CASE_WRITE(val, ERXMISC0)
>> + CASE_WRITE(val, ERXMISC1)
>> + CASE_WRITE(val, ERXMISC2)
>> + CASE_WRITE(val, ERXMISC3)
>> + default :
>> + return;
>> + }
>> +}
>> +
>> +static u64 aest_iomem_read(u64 base, u32 offset)
>> +{
>> + return readq_relaxed((void *)(base + offset));
>> +}
>> +
>> +static void aest_iomem_write(u64 base, u32 offset, u64 val)
>> +{
>> + writeq_relaxed(val, (void *)(base + offset));
>> +}
>> +
>> +static void aest_print(struct aest_node_llist *lnode)
>> +{
>> + static atomic_t seqno = { 0 };
>> + unsigned int curr_seqno;
>> + char pfx_seq[AEST_LOG_PREFIX_BUFFER];
>> + int index;
>> + struct ras_ext_regs *regs;
>> +
>> + curr_seqno = atomic_inc_return(&seqno);
>> + snprintf(pfx_seq, sizeof(pfx_seq), "{%u}" HW_ERR, curr_seqno);
>> + pr_info("%sHardware error from %s\n", pfx_seq, lnode->node_name);
>> +
>> + switch (lnode->type) {
>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
>> + pr_err("%s Error from CPU%d\n", pfx_seq, lnode->id0);
>> + break;
>> + case ACPI_AEST_MEMORY_ERROR_NODE:
>> + pr_err("%s Error from memory at SRAT proximity domain 0x%x\n",
>> + pfx_seq, lnode->id0);
>> + break;
>> + case ACPI_AEST_SMMU_ERROR_NODE:
>> + pr_err("%s Error from SMMU IORT node 0x%x subcomponent 0x%x\n",
>> + pfx_seq, lnode->id0, lnode->id1);
>> + break;
>> + case ACPI_AEST_VENDOR_ERROR_NODE:
>> + pr_err("%s Error from vendor hid 0x%x uid 0x%x\n",
>> + pfx_seq, lnode->id0, lnode->id1);
>> + break;
>> + case ACPI_AEST_GIC_ERROR_NODE:
>> + pr_err("%s Error from GIC type 0x%x instance 0x%x\n",
>> + pfx_seq, lnode->id0, lnode->id1);
>> + break;
>> + default:
>> + pr_err("%s Unknown AEST node type\n", pfx_seq);
>> + return;
>> + }
>> +
>> + index = lnode->index;
>> + regs = lnode->regs;
>> +
>> + pr_err("%s ERR%uFR: 0x%llx\n", pfx_seq, index, regs->err_fr);
>> + pr_err("%s ERR%uCTRL: 0x%llx\n", pfx_seq, index, regs->err_ctlr);
>> + pr_err("%s ERR%uSTATUS: 0x%llx\n", pfx_seq, index,
>> regs->err_status);
>> + if (regs->err_status & ERR_STATUS_AV)
>> + pr_err("%s ERR%uADDR: 0x%llx\n", pfx_seq, index,
>> regs->err_addr);
>> +
>> + if (regs->err_status & ERR_STATUS_MV) {
>> + pr_err("%s ERR%uMISC0: 0x%llx\n", pfx_seq, index,
>> regs->err_misc[0]);
>> + pr_err("%s ERR%uMISC1: 0x%llx\n", pfx_seq, index,
>> regs->err_misc[1]);
>> + pr_err("%s ERR%uMISC2: 0x%llx\n", pfx_seq, index,
>> regs->err_misc[2]);
>> + pr_err("%s ERR%uMISC3: 0x%llx\n", pfx_seq, index,
>> regs->err_misc[3]);
>> + }
>> +}
>> +
>> +static void aest_handle_memory_failure(struct aest_node_llist *lnode)
>> +{
>> + unsigned long pfn;
>> + u64 addr;
>> +
>> + if (test_bit(lnode->index, &lnode->addressing_mode) ||
>> + (lnode->regs->err_addr & ERR_ADDR_AI))
>> + return;
>> +
>> + addr = lnode->regs->err_addr & (1UL << CONFIG_ARM64_PA_BITS);
>> + pfn = PHYS_PFN(addr);
>> +
>> + if (!pfn_valid(pfn)) {
>> + pr_warn(HW_ERR "Invalid physical address: %#llx\n", addr);
>> + return;
>> + }
>> +
>> + memory_failure(pfn, 0);
>> +}
>> +
>> +static void aest_node_pool_process(struct work_struct *__unused)
>> +{
>> + struct llist_node *head;
>> + struct aest_node_llist *lnode, *tmp;
>> + u64 status;
>> +
>> + head = llist_del_all(&aest_node_llist);
>> + if (!head)
>> + return;
>> +
>> + head = llist_reverse_order(head);
>> + llist_for_each_entry_safe(lnode, tmp, head, llnode) {
>> + aest_print(lnode);
>> +
>> + status = lnode->regs->err_status;
>> + if ((status & ERR_STATUS_UE) &&
>> + (status & ERR_STATUS_UET) > ERR_STATUS_UET_UEU)
>> + aest_handle_memory_failure(lnode);
>> + gen_pool_free(aest_node_pool, (unsigned long)lnode,
>> + sizeof(*lnode));
>> + }
>> +}
>> +
>> +static int aest_node_gen_pool_add(struct aest_node *node, int index,
>> + struct ras_ext_regs *regs)
>> +{
>> + struct aest_node_llist *list;
>> +
>> + if (!aest_node_pool)
>> + return -EINVAL;
>> +
>> + list = (void *)gen_pool_alloc(aest_node_pool, sizeof(*list));
>> + if (!list)
>> + return -ENOMEM;
>> +
>> + list->type = node->type;
>> + list->node_name = node->name;
>> + switch (node->type) {
>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
>> + list->id0 = node->spec.processor.processor_id;
>> + if (node->spec.processor.flags & (ACPI_AEST_PROC_FLAG_SHARED |
>> + ACPI_AEST_PROC_FLAG_GLOBAL))
>> + list->id0 = smp_processor_id();
>> +
>> + list->id1 = node->spec.processor.resource_type;
>> + break;
>> + case ACPI_AEST_MEMORY_ERROR_NODE:
>> + list->id0 = node->spec.memory.srat_proximity_domain;
>> + break;
>> + case ACPI_AEST_SMMU_ERROR_NODE:
>> + list->id0 = node->spec.smmu.iort_node_reference;
>> + list->id1 = node->spec.smmu.subcomponent_reference;
>> + break;
>> + case ACPI_AEST_VENDOR_ERROR_NODE:
>> + list->id0 = node->spec.vendor.acpi_hid;
>> + list->id1 = node->spec.vendor.acpi_uid;
>> + break;
>> + case ACPI_AEST_GIC_ERROR_NODE:
>> + list->id0 = node->spec.gic.interface_type;
>> + list->id1 = node->spec.gic.instance_id;
>> + break;
>> + default:
>> + list->id0 = 0;
>> + list->id1 = 0;
>> + }
>> +
>> + list->regs = regs;
>> + list->index = index;
>> + list->addressing_mode = node->interface.addressing_mode;
>> + llist_add(&list->llnode, &aest_node_llist);
>> +
>> + return 0;
>> +}
>> +
>> +static int aest_node_pool_init(void)
>> +{
>> + unsigned long addr, size;
>> + int rc;
>> +
>> + if (aest_node_pool)
>> + return 0;
>> +
>> + size = ilog2(sizeof(struct aest_node_llist));
>> + aest_node_pool = gen_pool_create(size, -1);
>> + if (!aest_node_pool)
>> + return -ENOMEM;
>> +
>> + addr = (unsigned long)vmalloc(PAGE_ALIGN(size *
>> AEST_NODE_ALLOCED_MAX));
>> + if (!addr)
>> + goto err_pool_alloc;
>> +
>> + rc = gen_pool_add(aest_node_pool, addr, size, -1);
>> + if (rc)
>> + goto err_pool_add;
>> +
>> + return 0;
>> +
>> +err_pool_add:
>> + vfree((void *)addr);
>> +
>> +err_pool_alloc:
>> + gen_pool_destroy(aest_node_pool);
>> +
>> + return -ENOMEM;
>> +}
>> +
>> +static void aest_log(struct aest_node *node, int index, struct
>> ras_ext_regs *regs)
>> +{
>> + if (!aest_node_gen_pool_add(node, index, regs))
>> + schedule_work(&aest_work);
>> +}
>> +
>> +/*
>> + * Each PE may has multi error record, you must selects an error
>> record to
>> + * be accessed through the Error Record System registers.
>> + */
>> +static inline void aest_select_record(struct aest_node *node, int i)
>> +{
>> + if (node->interface.type == ACPI_AEST_NODE_SYSTEM_REGISTER) {
>> + write_sysreg_s(i, SYS_ERRSELR_EL1);
>> + isb();
>> + }
>> +}
>> +
>> +/* Ensure all writes has taken effect. */
>> +static inline void aest_sync(struct aest_node *node)
>> +{
>> + if (node->interface.type == ACPI_AEST_NODE_SYSTEM_REGISTER)
>> + isb();
>> +}
>> +
>> +static int aest_proc(struct aest_node *node)
>> +{
>> + struct ras_ext_regs regs = {0};
>> + struct aest_access *access;
>> + int i, count = 0;
>> + u64 regs_p;
>> +
>> + for_each_implemented_record(i, node) {
>> +
>> + /* 1b: Error record at i index is not implemented */
>> + if (test_bit(i, &node->interface.record_implemented))
>> + continue;
>> +
>> + aest_select_record(node, i);
>> +
>> + access = node->access;
>> + regs_p = (u64)&node->interface.regs[i];
>> +
>> + regs.err_status = access->read(regs_p, ERXSTATUS);
>> + if (!(regs.err_status & ERR_STATUS_V))
>> + continue;
>> +
>> + count++;
>> +
>> + if (regs.err_status & ERR_STATUS_AV)
>> + regs.err_addr = access->read(regs_p, ERXADDR);
>> +
>> + regs.err_fr = access->read(regs_p, ERXFR);
>> + regs.err_ctlr = access->read(regs_p, ERXCTLR);
>> +
>> + if (regs.err_status & ERR_STATUS_MV) {
>> + regs.err_misc[0] = access->read(regs_p, ERXMISC0);
>> + regs.err_misc[1] = access->read(regs_p, ERXMISC1);
>> + regs.err_misc[2] = access->read(regs_p, ERXMISC2);
>> + regs.err_misc[3] = access->read(regs_p, ERXMISC3);
>> + }
>> +
>> + if (node->interface.flags & ACPI_AEST_INTERFACE_CLEAR_MISC) {
>> + access->write(regs_p, ERXMISC0, 0);
>> + access->write(regs_p, ERXMISC1, 0);
>> + access->write(regs_p, ERXMISC2, 0);
>> + access->write(regs_p, ERXMISC3, 0);
>> + } else
>> + access->write(regs_p, ERXMISC0,
>> + node->interface.ce_threshold[i]);
>> +
>> + aest_log(node, i, ®s);
>> +
>> + /* panic if unrecoverable and uncontainable error encountered */
>> + if ((regs.err_status & ERR_STATUS_UE) &&
>> + (regs.err_status & ERR_STATUS_UET) < ERR_STATUS_UET_UER)
>> + panic("AEST: unrecoverable error encountered");
>> +
>> + /* Write-one-to-clear the bits we've seen */
>> + regs.err_status &= ERR_STATUS_W1TC;
>> +
>> + /* Multi bit filed need to write all-ones to clear. */
>> + if (regs.err_status & ERR_STATUS_CE)
>> + regs.err_status |= ERR_STATUS_CE;
>> +
>> + /* Multi bit filed need to write all-ones to clear. */
>> + if (regs.err_status & ERR_STATUS_UET)
>> + regs.err_status |= ERR_STATUS_UET;
>> +
>> + access->write(regs_p, ERXSTATUS, regs.err_status);
>> +
>> + aest_sync(node);
>> + }
>> +
>> + return count;
>> +}
>> +
>> +static irqreturn_t aest_irq_func(int irq, void *input)
>> +{
>> + struct aest_node *node = input;
>> +
>> + if (aest_proc(node))
>> + return IRQ_HANDLED;
>> +
>> + return IRQ_NONE;
>> +}
>> +
>> +static int __init aest_register_gsi(u32 gsi, int trigger, void *data,
>> + irq_handler_t aest_irq_func)
>> +{
>> + int cpu, irq;
>> +
>> + irq = acpi_register_gsi(NULL, gsi, trigger, ACPI_ACTIVE_HIGH);
>> +
>> + if (irq == -EINVAL) {
>> + pr_err("failed to map AEST GSI %d\n", gsi);
>> + return -EINVAL;
>> + }
>
> IMO, should be:
> if (irq < 0) {
> pr_err("failed to map AEST GSI %d\n", gsi);
> return irq;
> }
>
>> +
>> + if (irq_is_percpu_devid(irq)) {
>> + ppi_irqs[ppi_idx] = irq;
>> + for_each_possible_cpu(cpu) {
>> + memcpy(per_cpu_ptr(aest_ppi_data[ppi_idx], cpu), data,
>> + sizeof(struct aest_node));
>> + }
>> + if (request_percpu_irq(irq, aest_irq_func, "AEST",
>> + aest_ppi_data[ppi_idx++])) {
>> + pr_err("failed to register AEST IRQ %d\n", irq);
>> + return -EINVAL;
>
> Do not override the error number.
>
>> + }
>> + } else {
>> + if (request_irq(irq, aest_irq_func, IRQF_SHARED, "AEST",
>> + data)) {
>> + pr_err("failed to register AEST IRQ %d\n", irq);
>> + return -EINVAL;
>
> ditto.
>
>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int __init aest_init_interrupts(struct acpi_aest_hdr *hdr,
>> + struct aest_node *node)
>> +{
>> + struct acpi_aest_node_interrupt *interrupt;
>> + int i, trigger, ret = 0, err_ctlr, regs_p;
>> +
>> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, hdr,
>> + hdr->node_interrupt_offset);
>> +
>> + for (i = 0; i < hdr->node_interrupt_count; i++, interrupt++) {
>> + trigger = (interrupt->flags & AEST_INTERRUPT_MODE) ?
>> + ACPI_LEVEL_SENSITIVE : ACPI_EDGE_SENSITIVE;
>> + if (aest_register_gsi(interrupt->gsiv, trigger, node,
>> + aest_irq_func))
>> + ret = -EINVAL;
>
> Do not override the error number.
>
>> + }
>> +
>> + /* Ensure RAS interrupt is enabled */
>> + for_each_implemented_record(i, node) {
>> + /* 1b: Error record at i index is not implemented */
>> + if (test_bit(i, &node->interface.record_implemented))
>> + continue;
>> +
>> + aest_select_record(node, i);
>> +
>> + regs_p = (u64)&node->interface.regs[i];
>> +
>> + err_ctlr = node->access->read(regs_p, ERXCTLR);
>> +
>> + if (interrupt->type == ACPI_AEST_NODE_FAULT_HANDLING)
>> + err_ctlr |= ERR_CTLR_FI;
>> + if (interrupt->type == ACPI_AEST_NODE_ERROR_RECOVERY)
>> + err_ctlr |= ERR_CTLR_UI;
>> +
>> + node->access->write(regs_p, ERXCTLR, err_ctlr);
>> +
>> + aest_sync(node);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> +static void __init set_aest_node_name(struct aest_node *node)
>> +{
>> + switch (node->type) {
>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
>> + node->name = kasprintf(GFP_KERNEL, "AEST-CPU%d",
>> + node->spec.processor.processor_id);
>> + break;
>> + case ACPI_AEST_MEMORY_ERROR_NODE:
>> + case ACPI_AEST_SMMU_ERROR_NODE:
>> + case ACPI_AEST_VENDOR_ERROR_NODE:
>> + case ACPI_AEST_GIC_ERROR_NODE:
>> + node->name = kasprintf(GFP_KERNEL, "AEST-%llx",
>> + node->interface.phy_addr);
>> + break;
>> + default:
>> + node->name = kasprintf(GFP_KERNEL, "AEST-Unkown-Node");
>
> IMO, better to check the return value for memory allocation.
>
>> + }
>> +}
>> +
>> +/* access type is decided by AEST interface type. */
>> +static struct aest_access aest_access[] = {
>> + [ACPI_AEST_NODE_SYSTEM_REGISTER] = {
>> + .read = aest_sysreg_read,
>> + .write = aest_sysreg_write,
>> + },
>> +
>> + [ACPI_AEST_NODE_MEMORY_MAPPED] = {
>> + .read = aest_iomem_read,
>> + .write = aest_iomem_write,
>> + },
>> + { }
>> +};
>> +
>> +/* In kernel-first mode, kernel will report every CE by default. */
>> +static void __init aest_set_ce_threshold(struct aest_node *node)
>> +{
>> + u64 regs_p, err_fr, err_fr_cec, err_fr_rp, err_misc0, ce_threshold;
>> + int i;
>> +
>> + for_each_implemented_record(i, node) {
>> + /* 1b: Error record at i index is not implemented */
>> + if (test_bit(i, &node->interface.record_implemented))
>> + continue;
>> +
>> + aest_select_record(node, i);
>> + regs_p = (u64)&node->interface.regs[i];
>> +
>> + err_fr = node->access->read(regs_p, ERXFR);
>> + err_fr_cec = FIELD_GET(ERR_FR_CEC, err_fr);
>> + err_fr_rp = FIELD_GET(ERR_FR_RP, err_fr);
>> + err_misc0 = node->access->read(regs_p, ERXMISC0);
>> +
>> + if (err_fr_cec == ERR_FR_CEC_0B_COUNTER)
>> + pr_debug("%s-%d do not support CE threshold!\n",
>> + node->name, i);
>> + else if (err_fr_cec == ERR_FR_CEC_8B_COUNTER &&
>> + err_fr_rp == ERR_FR_RP_SINGLE_COUNTER) {
>> + pr_debug("%s-%d support 8 bit CE threshold!\n",
>> + node->name, i);
>> + ce_threshold = err_misc0 | ERR_MISC0_8B_CEC;
>> + } else if (err_fr_cec == ERR_FR_CEC_16B_COUNTER &&
>> + err_fr_rp == ERR_FR_RP_SINGLE_COUNTER) {
>> + pr_debug("%s-%d support 16 bit CE threshold!\n",
>> + node->name, i);
>> + ce_threshold = err_misc0 | ERR_MISC0_16B_CEC;
>> + } else
>> + pr_debug("%s-%d do not support double counter yet!\n",
>> + node->name, i);
>
> Change to 'switch' statement will be more readable.
>
>> +
>> + node->access->write(regs_p, ERXMISC0, ce_threshold);
>> + node->interface.ce_threshold[i] = ce_threshold;
>> +
>> + aest_sync(node);
>> + }
>> +}
>> +
>> +static int __init aest_init_interface(struct acpi_aest_hdr *hdr,
>> + struct aest_node *node)
>> +{
>> + struct acpi_aest_node_interface *interface;
>> + struct resource *res;
>> + int size;
>> +
>> + interface = ACPI_ADD_PTR(struct acpi_aest_node_interface, hdr,
>> + hdr->node_interface_offset);
>> +
>> + if (interface->type >= ACPI_AEST_XFACE_RESERVED) {
>> + pr_err("invalid interface type: %d\n", interface->type);
>> + return -EINVAL;
>> + }
>> +
>> + node->interface.type = interface->type;
>> + node->interface.phy_addr = interface->address;
>> + node->interface.record_start = interface->error_record_index;
>> + node->interface.record_end = interface->error_record_index +
>> + interface->error_record_count;
>> + node->interface.flags = interface->flags;
>> + node->interface.record_implemented =
>> interface->error_record_implemented;
>> + node->interface.status_reporting =
>> interface->error_status_reporting;
>> + node->interface.addressing_mode = interface->addressing_mode;
>> + node->access = &aest_access[interface->type];
>> +
>> + /*
>> + * Currently SR based handling is done through the architected
>> + * discovery exposed through SRs. That may change in the future
>> + * if there is supplemental information in the AEST that is
>> + * needed.
>> + */
>> + if (interface->type == ACPI_AEST_NODE_SYSTEM_REGISTER)
>> + return 0;
>> +
>> + res = kzalloc(sizeof(struct resource), GFP_KERNEL);
>> + if (!res)
>> + return -ENOMEM;
>> +
>> + size = interface->error_record_count * sizeof(struct ras_ext_regs);
>> + res->name = "AEST";
>> + res->start = interface->address;
>> + res->end = res->start + size;
>> + res->flags = IORESOURCE_MEM;
>> +
>> + if (insert_resource(&iomem_resource, res)) {
>> + pr_notice("request region conflict with %s\n",
>> + res->name);
>> + }
>> +
>> + node->interface.regs = ioremap(res->start, size);
>> + if (!node->interface.regs) {
>> + pr_err("Ioremap for %s failed!\n", node->name);
>> + kfree(res);
>> + return -EINVAL;
>
> return -ENOMEM;
>
>> + }
>> +
>> + node->interface.ce_threshold = kzalloc(sizeof(u64) *
>> + interface->error_record_count, GFP_KERNEL);
>> + if (!node->interface.ce_threshold)
>
> kfree(res) and iounmap()
>
>> + return -ENOMEM;
>> +
>> + aest_set_ce_threshold(node);
>> +
>> + return 0;
>> +}
>> +
>> +static int __init aest_init_common(struct acpi_aest_hdr *hdr,
>> + struct aest_node *node)
>> +{
>> + int ret;
>> +
>> + set_aest_node_name(node);
>> +
>> + ret = aest_init_interface(hdr, node);
>> + if (ret) {
>> + pr_err("failed to init interface\n");
>> + return ret;
>
> I did not see you free the node->name before returning an error.
>
>> + }
>> +
>> + return aest_init_interrupts(hdr, node);
>> +}
>> +
>> +static int __init aest_init_node_default(struct acpi_aest_hdr *hdr)
>> +{
>> + struct aest_node *node;
>> + union aest_node_spec *node_spec;
>> + int ret;
>> +
>> + node = kzalloc(sizeof(struct aest_node), GFP_KERNEL);
>> + if (!node)
>> + return -ENOMEM;
>> +
>> + node->type = hdr->type;
>> + node_spec = ACPI_ADD_PTR(union aest_node_spec, hdr,
>> + hdr->node_specific_offset);
>> +
>> + memcpy(&node->spec, node_spec,
>> + hdr->node_interface_offset - hdr->node_specific_offset);
>> +
>> + ret = aest_init_common(hdr, node);
>> + if (ret)
>> + kfree(node);
>> +
>> + return ret;
>> +}
>> +
>> +static int __init aest_init_processor_node(struct acpi_aest_hdr *hdr)
>> +{
>> + struct aest_node *node;
>> + union aest_node_spec *node_spec;
>> + union aest_node_processor *proc;
>> + int ret;
>> +
>> + node = kzalloc(sizeof(struct aest_node), GFP_KERNEL);
>> + if (!node)
>> + return -ENOMEM;
>> +
>> + node->type = hdr->type;
>> + node_spec = ACPI_ADD_PTR(union aest_node_spec, hdr,
>> + hdr->node_specific_offset);
>> +
>> + memcpy(&node->spec, node_spec,
>> + hdr->node_interface_offset - hdr->node_specific_offset);
>> +
>> + proc = ACPI_ADD_PTR(union aest_node_processor, node_spec,
>> + sizeof(acpi_aest_processor));
>> +
>> + switch (node->spec.processor.resource_type) {
>> + case ACPI_AEST_CACHE_RESOURCE:
>> + memcpy(&node->proc, proc,
>> + sizeof(struct acpi_aest_processor_cache));
>> + break;
>> + case ACPI_AEST_TLB_RESOURCE:
>> + memcpy(&node->proc, proc,
>> + sizeof(struct acpi_aest_processor_tlb));
>> + break;
>> + case ACPI_AEST_GENERIC_RESOURCE:
>> + memcpy(&node->proc, proc,
>> + sizeof(struct acpi_aest_processor_generic));
>> + break;
>> + }
>> +
>> + ret = aest_init_common(hdr, node);
>> + if (ret)
>> + kfree(node);
>> +
>> + return ret;
>> +}
>> +
>> +static int __init aest_init_node(struct acpi_aest_hdr *node)
>> +{
>> + switch (node->type) {
>> + case ACPI_AEST_PROCESSOR_ERROR_NODE:
>> + return aest_init_processor_node(node);
>> + case ACPI_AEST_MEMORY_ERROR_NODE:
>> + case ACPI_AEST_VENDOR_ERROR_NODE:
>> + case ACPI_AEST_SMMU_ERROR_NODE:
>> + case ACPI_AEST_GIC_ERROR_NODE:
>> + return aest_init_node_default(node);
>> + default:
>> + return -EINVAL;
>> + }
>> +}
>> +
>> +static void __init aest_count_ppi(struct acpi_aest_hdr *header)
>> +{
>> + struct acpi_aest_node_interrupt *interrupt;
>> + int i;
>> +
>> + interrupt = ACPI_ADD_PTR(struct acpi_aest_node_interrupt, header,
>> + header->node_interrupt_offset);
>> +
>> + for (i = 0; i < header->node_interrupt_count; i++, interrupt++) {
>> + if (interrupt->gsiv >= 16 && interrupt->gsiv < 32)
>> + num_ppi++;
>> + }
>> +}
>> +
>> +static int aest_starting_cpu(unsigned int cpu)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < num_ppi; i++)
>> + enable_percpu_irq(ppi_irqs[i], IRQ_TYPE_NONE);
>> +
>> + return 0;
>> +}
>> +
>> +static int aest_dying_cpu(unsigned int cpu)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < num_ppi; i++)
>> + disable_percpu_irq(ppi_irqs[i]);
>> +
>> + return 0;
>> +}
>> +
>> +int __init acpi_aest_init(void)
>
> Should be 'static'.
>
>> +{
>> + struct acpi_aest_hdr *aest_node, *aest_end;
>> + struct acpi_table_aest *aest;
>> + int i, ret = 0;
>> +
>> + if (acpi_disabled)
>> + return 0;
>> +
>> + if (!IS_ENABLED(CONFIG_ARM64_RAS_EXTN))
>> + return 0;
>
> I think you can move this into Kconfig file, that makes ACPI_AEST
> dependent on this CONFIG_ARM64_RAS_EXTN?
>
>> +
>> + if (ACPI_FAILURE(acpi_get_table(ACPI_SIG_AEST, 0, &aest_table)))
>> + return -EINVAL;
>> +
>> + ret = aest_node_pool_init();
>> + if (ret) {
>> + pr_err("Failed init aest node pool.\n");
>> + goto fail;
>
> Just return ret;
>
>> + }
>> +
>> + INIT_WORK(&aest_work, aest_node_pool_process);
>> +
>> + aest = (struct acpi_table_aest *)aest_table;
>> +
>> + /* Get the first AEST node */
>> + aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest,
>> + sizeof(struct acpi_table_header));
>> + /* Pointer to the end of the AEST table */
>> + aest_end = ACPI_ADD_PTR(struct acpi_aest_hdr, aest,
>> + aest_table->length);
>> +
>> + while (aest_node < aest_end) {
>> + if (((u64)aest_node + aest_node->length) > (u64)aest_end) {
>> + pr_err("AEST node pointer overflow, bad table.\n");
>> + return -EINVAL;
>
> You should destroy the node pool before returning errors.
>
>> + }
>> +
>> + aest_count_ppi(aest_node);
>> +
>> + aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest_node,
>> + aest_node->length);
>> + }
>> +
>> + aest_ppi_data = kcalloc(num_ppi, sizeof(struct aest_node_data *),
>> + GFP_KERNEL);
>> + if (!aest_ppi_data) {
>> + ret = -ENOMEM;
>> + goto fail;
>> + }
>> +
>> + ppi_irqs = kcalloc(num_ppi, sizeof(int), GFP_KERNEL);
>> + if (!ppi_irqs) {
>> + ret = -ENOMEM;
>> + goto fail;
>> + }
>> +
>> + for (i = 0; i < num_ppi; i++) {
>> + aest_ppi_data[i] = alloc_percpu(struct aest_node);
>> + if (!aest_ppi_data[i]) {
>> + pr_err("Failed percpu allocation.\n");
>> + ret = -ENOMEM;
>> + goto fail;
>> + }
>> + }
>> +
>> + aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest,
>> + sizeof(struct acpi_table_header));
>> +
>> + while (aest_node < aest_end) {
>> + ret = aest_init_node(aest_node);
>> + if (ret) {
>> + pr_err("failed to init node: %d", ret);
>> + goto fail;
>> + }
>> +
>> + aest_node = ACPI_ADD_PTR(struct acpi_aest_hdr, aest_node,
>> + aest_node->length);
>> + }
>> +
>> +
>> +
>> + return cpuhp_setup_state(CPUHP_AP_ARM_AEST_STARTING,
>> + "drivers/acpi/arm64/aest:starting",
>> + aest_starting_cpu, aest_dying_cpu);
>
> Need free the resources you requested if an error occurs.
>
>> +
>> +fail:
>> + for (i = 0; i < num_ppi; i++)
>> + free_percpu(aest_ppi_data[i]);
>> + kfree(aest_ppi_data);
>> + return ret;
>> +}
>> +subsys_initcall(acpi_aest_init);
>> diff --git a/include/linux/acpi_aest.h b/include/linux/acpi_aest.h
>> new file mode 100644
>> index 000000000000..679187505dc6
>> --- /dev/null
>> +++ b/include/linux/acpi_aest.h
>> @@ -0,0 +1,92 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef AEST_H
>> +#define AEST_H
>> +
>> +#include <acpi/actbl.h>
>> +#include <asm/ras.h>
>> +
>> +#define AEST_INTERRUPT_MODE BIT(0)
>> +
>> +#define ACPI_AEST_PROC_FLAG_GLOBAL (1<<0)
>> +#define ACPI_AEST_PROC_FLAG_SHARED (1<<1)
>> +
>> +#define ACPI_AEST_INTERFACE_CLEAR_MISC (1<<0)
>> +
>> +#define ERXFR 0x0
>> +#define ERXCTLR 0x8
>> +#define ERXSTATUS 0x10
>> +#define ERXADDR 0x18
>> +#define ERXMISC0 0x20
>> +#define ERXMISC1 0x28
>> +#define ERXMISC2 0x30
>> +#define ERXMISC3 0x38
>> +
>> +struct aest_node_interface {
>> + u8 type;
>> + u64 phy_addr;
>> + u16 record_start;
>> + u16 record_end;
>> + u32 flags;
>> + unsigned long record_implemented;
>> + unsigned long status_reporting;
>> + unsigned long addressing_mode;
>> + struct ras_ext_regs *regs;
>> + u64 *ce_threshold;
>> +};
>> +
>> +union aest_node_processor {
>> + struct acpi_aest_processor_cache cache_data;
>> + struct acpi_aest_processor_tlb tlb_data;
>> + struct acpi_aest_processor_generic generic_data;
>> +};
>> +
>> +union aest_node_spec {
>> + struct acpi_aest_processor processor;
>> + struct acpi_aest_memory memory;
>> + struct acpi_aest_smmu smmu;
>> + struct acpi_aest_vendor vendor;
>> + struct acpi_aest_gic gic;
>> +};
>> +
>> +struct aest_access {
>> + u64 (*read)(u64 base, u32 offset);
>> + void (*write)(u64 base, u32 offset, u64 val);
>> +};
>> +
>> +struct aest_node {
>> + char *name;
>> + u8 type;
>> + struct aest_node_interface interface;
>> + union aest_node_spec spec;
>> + union aest_node_processor proc;
>> + struct aest_access *access;
>> +};
>> +
>> +struct aest_node_llist {
>> + struct llist_node llnode;
>> + char *node_name;
>> + int type;
>> + /*
>> + * Different nodes have different meanings:
>> + * - Processor node : processor number.
>> + * - Memory node : SRAT proximity domain.
>> + * - SMMU node : IORT proximity domain.
>> + * - Vendor node : hardware ID.
>> + * - GIC node : interface type.
>> + */
>> + u32 id0;
>> + /*
>> + * Different nodes have different meanings:
>> + * - Processor node : processor resource type.
>> + * - Memory node : Non.
>> + * - SMMU node : subcomponent reference.
>> + * - Vendor node : Unique ID.
>> + * - GIC node : instance identifier.
>> + */
>> + u32 id1;
>> + int index;
>> + unsigned long addressing_mode;
>> + struct ras_ext_regs *regs;
>> +};
>
> These are only aest-related structures? If so, I think they should be in
> aest.c file.
>
>> +
>> +#endif /* AEST_H */
>> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
>> index 624d4a38c358..f0dda08dbad2 100644
>> --- a/include/linux/cpuhotplug.h
>> +++ b/include/linux/cpuhotplug.h
>> @@ -186,6 +186,7 @@ enum cpuhp_state {
>> CPUHP_AP_CSKY_TIMER_STARTING,
>> CPUHP_AP_TI_GP_TIMER_STARTING,
>> CPUHP_AP_HYPERV_TIMER_STARTING,
>> + CPUHP_AP_ARM_AEST_STARTING,
>> /* Must be the last timer callback */
>> CPUHP_AP_DUMMY_TIMER_STARTING,
>> CPUHP_AP_ARM_XEN_STARTING,
All accept, thanks for reviewing.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 7/9] dt-bindings: rtc: spear-rtc: move to trivial-rtc
From: Krzysztof Kozlowski @ 2024-04-09 7:37 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-7-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> The RTC documented in this binding requires a compatible, a reg
> and a single interrupt, which makes it suitable for a direct
> conversion into trivial-rtc.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> Documentation/devicetree/bindings/rtc/spear-rtc.txt | 15 ---------------
> Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 2 ++
> 2 files changed, 2 insertions(+), 15 deletions(-)
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 8/9] dt-bindings: rtc: stmp3xxx-rtc: convert to dtschema
From: Krzysztof Kozlowski @ 2024-04-09 7:40 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-8-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> Convert existing binding to dtschema to support validation.
>
> The 'fsl,imx28-rtc' compatible is currently not supported, and it is
> only referenced in this binding and in nxp/mxs/imx28.dtsi. Therefore,
> that compatible has been dropped, which triggers a warning when testing
> the DT against the new binding.
Instead document missing compatibles and mention this in commit msg.
>
> There is another reference to fsl,stmp3xxx-rtc in nxp/mxs/imx23.dtsi,
> where another unsupported compatible 'fsl,imx23-rtc' is used, and the
> same problem would arise when testing the file against the new binding.
Please write concise messages... you have to paragraphs about the same?
What is the difference here?
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
> .../devicetree/bindings/rtc/fsl,stmp3xxx-rtc.yaml | 45 ++++++++++++++++++++++
> .../devicetree/bindings/rtc/stmp3xxx-rtc.txt | 21 ----------
> 2 files changed, 45 insertions(+), 21 deletions(-)
>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 9/9] dt-bindings: rtc: via,vt8500-rtc: move to trivial-rtc
From: Krzysztof Kozlowski @ 2024-04-09 7:40 UTC (permalink / raw)
To: Javier Carrasco, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <20240408-rtc_dtschema-v1-9-c447542fc362@gmail.com>
On 08/04/2024 17:53, Javier Carrasco wrote:
> The RTC documented in this binding requires a compatible, a reg
> and a single interrupt, which makes it suitable for a direct
> conversion into trivial-rtc.
>
> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
> ---
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: mailbox: arm,mhuv3: Add bindings
From: Krzysztof Kozlowski @ 2024-04-09 7:44 UTC (permalink / raw)
To: Cristian Marussi, linux-kernel, linux-arm-kernel, devicetree
Cc: sudeep.holla, jassisinghbrar, robh+dt, krzysztof.kozlowski+dt,
conor+dt
In-Reply-To: <20240404062347.3219795-2-cristian.marussi@arm.com>
On 04/04/2024 08:23, Cristian Marussi wrote:
> Add bindings for the ARM MHUv3 Mailbox controller.
>
> Signed-off-by: Cristian Marussi <cristian.marussi@arm.com>
...
> + mailbox@2aaa0000 {
> + compatible = "arm,mhuv3";
> + #mbox-cells = <3>;
> + reg = <0 0x2aaa0000 0 0x10000>;
> + clocks = <&clock 0>;
> + interrupt-names = "combined", "pbx-dbch-xfer-ack-1",
> + "ffch-high-tide-0";
> + interrupts = <0 36 4>, <0 37 4>;
> + };
> +
> + mailbox@2ab00000 {
> + compatible = "arm,mhuv3";
> + #mbox-cells = <3>;
> + reg = <0 0x2aab0000 0 0x10000>;
> + clocks = <&clock 0>;
> + interrupt-names = "combined", "mbx-dbch-xfer-1", "ffch-low-tide-0";
> + interrupts = <0 35 4>, <0 38 4>, <0 39 4>;
Use defines for GIC and level flags.
With this fixed:
Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
Best regards,
Krzysztof
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [syzbot] [mm?] BUG: unable to handle kernel paging request in copy_from_kernel_nofault (2)
From: Puranjay Mohan @ 2024-04-09 7:45 UTC (permalink / raw)
To: Russell King (Oracle), Andrii Nakryiko
Cc: Alexei Starovoitov, Mark Rutland, Andrew Morton, linux-arm-kernel,
syzbot, LKML, linux-mm, syzkaller-bugs, bpf
In-Reply-To: <ZhBAnvLRfj/JW5bZ@shell.armlinux.org.uk>
"Russell King (Oracle)" <linux@armlinux.org.uk> writes:
> On Fri, Apr 05, 2024 at 10:50:30AM -0700, Andrii Nakryiko wrote:
>> On Fri, Apr 5, 2024 at 9:30 AM Alexei Starovoitov
>> <alexei.starovoitov@gmail.com> wrote:
>> >
>> > On Fri, Apr 5, 2024 at 4:36 AM Russell King (Oracle)
>> > <linux@armlinux.org.uk> wrote:
>> > >
>> > > On Fri, Apr 05, 2024 at 12:02:36PM +0100, Mark Rutland wrote:
>> > > > On Thu, Apr 04, 2024 at 03:57:04PM -0700, Alexei Starovoitov wrote:
>> > > > > On Wed, Apr 3, 2024 at 6:56 PM Andrew Morton <akpm@linux-foundationorg> wrote:
>> > > > > >
>> > > > > > On Mon, 01 Apr 2024 22:19:25 -0700 syzbot <syzbot+186522670e6722692d86@syzkaller.appspotmail.com> wrote:
>> > > > > >
>> > > > > > > Hello,
>> > > > > >
>> > > > > > Thanks. Cc: bpf@vger.kernel.org
>> > > > >
>> > > > > I suspect the issue is not on bpf side.
>> > > > > Looks like the bug is somewhere in arm32 bits.
>> > > > > copy_from_kernel_nofault() is called from lots of places.
>> > > > > bpf is just one user that is easy for syzbot to fuzz.
>> > > > > Interestingly arm defines copy_from_kernel_nofault_allowed()
>> > > > > that should have filtered out user addresses.
>> > > > > In this case ffffffe9 is probably a kernel address?
>> > > >
>> > > > It's at the end of the kernel range, and it's ERR_PTR(-EINVAL).
>> > > >
>> > > > 0xffffffe9 is -0x16, which is -22, which is -EINVAL.
>> > > >
>> > > > > But the kernel is doing a write?
>> > > > > Which makes no sense, since copy_from_kernel_nofault is probe reading.
>> > > >
>> > > > It makes perfect sense; the read from 'src' happened, then the kernel tries to
>> > > > write the result to 'dst', and that aligns with the disassembly in the report
>> > > > below, which I beleive is:
>> > > >
>> > > > 8: e4942000 ldr r2, [r4], #0 <-- Read of 'src', fault fixup is elsewhere
>> > > > c: e3530000 cmp r3, #0
>> > > > * 10: e5852000 str r2, [r5] <-- Write to 'dst'
>> > > >
>> > > > As above, it looks like 'dst' is ERR_PTR(-EINVAL).
>> > > >
>> > > > Are you certain that BPF is passing a sane value for 'dst'? Where does that
>> > > > come from in the first place?
>> > >
>> > > It looks to me like it gets passed in from the BPF program, and the
>> > > "type" for the argument is set to ARG_PTR_TO_UNINIT_MEM. What that
>> > > means for validation purposes, I've no idea, I'm not a BPF hacker.
>> > >
>> > > Obviously, if BPF is allowing copy_from_kernel_nofault() to be passed
>> > > an arbitary destination address, that would be a huge security hole.
>> >
>> > If that's the case that's indeed a giant security hole,
>> > but I doubt it. We would be crashing other archs as well.
>> > I cannot really tell whether arm32 JIT is on.
>> > If it is, it's likely a bug there.
>> > Puranjay,
>> > could you please take a look.
>> >
>>
>> I dumped the BPF program that repro.c is loading, it works on x86-64
>> and there is nothing special there. We are probe-reading 5 bytes from
>> somewhere into the stack. Everything is unaligned here, but stays
>> within a well-defined memory slot.
>>
>> Note the r3 = (s8)r1, that's a new-ish thing, maybe bug is somewhere
>> there (but then it would be JIT, not verifier itself)
>>
>> 0: (7a) *(u64 *)(r10 -8) = 896542069
>> 1: (bf) r1 = r10
>> 2: (07) r1 += -7
>> 3: (b7) r2 = 5
>> 4: (bf) r3 = (s8)r1
>> 5: (85) call bpf_probe_read_kernel#-72390
>
I have started looking into this, the issue only reproduces when the JIT
is enabled. With the interpreter, it works fine.
I used GDB to dump the JITed BPF program:
0xbf00012c: push {r4, r5, r6, r7, r8, r9, r11, lr}
0xbf000130: mov r11, sp
0xbf000134: mov r3, #0
0xbf000138: sub r2, sp, #80 @ 0x50
0xbf00013c: sub sp, sp, #88 @ 0x58
0xbf000140: strd r2, [r11, #-64] @ 0xffffffc0
0xbf000144: mov r2, #0
0xbf000148: strd r2, [r11, #-72] @ 0xffffffb8
0xbf00014c: mov r2, r0
0xbf000150: movw r8, #9589 @ 0x2575
0xbf000154: movt r8, #13680 @ 0x3570
0xbf000158: mov r9, #0
0xbf00015c: ldr r6, [r11, #-64] @ 0xffffffc0
0xbf000160: str r8, [r6, #-8]
0xbf000164: str r9, [r6, #-4]
0xbf000168: ldrd r2, [r11, #-64] @ 0xffffffc0
0xbf00016c: movw r8, #65529 @ 0xfff9
0xbf000170: movt r8, #65535 @ 0xffff
0xbf000174: movw r9, #65535 @ 0xffff
0xbf000178: movt r9, #65535 @ 0xffff
0xbf00017c: adds r2, r2, r8
0xbf000180: adc r3, r3, r9
0xbf000184: mov r6, #5
0xbf000188: mov r7, #0
0xbf00018c: strd r6, [r11, #-8]
0xbf000190: ldrd r6, [r11, #-16]
0xbf000194: lsl r2, r2, #24
0xbf000198: asr r2, r2, #24
0xbf00019c: str r2, [r11, #-16]
0xbf0001a0: asr r7, r6, #31
0xbf0001a4: mov r1, r3
0xbf0001a8: mov r0, r2
0xbf0001ac: ldrd r2, [r11, #-8]
0xbf0001b0: ldrd r8, [r11, #-32] @ 0xffffffe0
0xbf0001b4: push {r8, r9}
0xbf0001b8: ldrd r8, [r11, #-24] @ 0xffffffe8
0xbf0001bc: push {r8, r9}
0xbf0001c0: ldrd r8, [r11, #-16]
0xbf0001c4: push {r8, r9}
0xbf0001c8: movw r6, #40536 @ 0x9e58
0xbf0001cc: movt r6, #49223 @ 0xc047
0xbf0001d0: blx r6
0xbf0001d4: add sp, sp, #24
0xbf0001d8: mov r0, #0
0xbf0001dc: mov r1, #0
0xbf0001e0: mov sp, r11
0xbf0001e4: pop {r4, r5, r6, r7, r8, r9, r11, pc}
Thanks,
Puranjay
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v1 00/12] First try to replace page_frag with page_frag_cache
From: Yunsheng Lin @ 2024-04-09 7:59 UTC (permalink / raw)
To: Alexander Duyck
Cc: davem, kuba, pabeni, netdev, linux-kernel, Matthias Brugger,
AngeloGioacchino Del Regno, Alexei Starovoitov, Daniel Borkmann,
Jesper Dangaard Brouer, John Fastabend, linux-arm-kernel,
linux-mediatek, bpf
In-Reply-To: <CAKgT0UdjBXguCudxM9-tzKx2qWYg18xp5cG2xaeY893rVbw5qQ@mail.gmail.com>
On 2024/4/8 23:09, Alexander Duyck wrote:
> On Mon, Apr 8, 2024 at 6:38 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>
>> On 2024/4/8 1:02, Alexander Duyck wrote:
>>> On Sun, Apr 7, 2024 at 6:10 AM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>>>>
>>>> After [1], Only there are two implementations for page frag:
>>>>
>>>> 1. mm/page_alloc.c: net stack seems to be using it in the
>>>> rx part with 'struct page_frag_cache' and the main API
>>>> being page_frag_alloc_align().
>>>> 2. net/core/sock.c: net stack seems to be using it in the
>>>> tx part with 'struct page_frag' and the main API being
>>>> skb_page_frag_refill().
>>>>
>>>> This patchset tries to unfiy the page frag implementation
>>>> by replacing page_frag with page_frag_cache for sk_page_frag()
>>>> first. net_high_order_alloc_disable_key for the implementation
>>>> in net/core/sock.c doesn't seems matter that much now have
>>>> have pcp support for high-order pages in commit 44042b449872
>>>> ("mm/page_alloc: allow high-order pages to be stored on the
>>>> per-cpu lists").
>>>>
>>>> As the related change is mostly related to networking, so
>>>> targeting the net-next. And will try to replace the rest
>>>> of page_frag in the follow patchset.
>>>>
>>>> After this patchset, we are not only able to unify the page
>>>> frag implementation a little, but seems able to have about
>>>> 0.5+% performance boost testing by using the vhost_net_test
>>>> introduced in [1] and page_frag_test.ko introduced in this
>>>> patch.
>>>
>>> One question that jumps out at me for this is "why?". No offense but
>>> this is a pretty massive set of changes with over 1400 additions and
>>> 500+ deletions and I can't help but ask why, and this cover page
>>> doesn't give me any good reason to think about accepting this set.
>>
>> There are 375 + 256 additions for testing module and the documentation
>> update in the last two patches, and there is 198 additions and 176
>> deletions for moving the page fragment allocator from page_alloc into
>> its own file in patch 1.
>> Without above number, there are above 600+ additions and 300+ deletions,
>> deos that seems reasonable considering 140+ additions are needed to for
>> the new API, 300+ additions and deletions for updating the users to use
>> the new API as there are many users using the old API?
>
> Maybe it would make more sense to break this into 2 sets. The first
> one adding your testing, and the second one consolidating the API.
> With that we would have a clearly defined test infrastructure in place
> for the second set which is making significant changes to the API. In
> addition it would provide the opportunity for others to point out any
> other test that they might want pulled in since this is likely to have
> impact outside of just the tests you have proposed.
Do you have someone might want pulled in some test in mind, if yes, then
it might make sense to work together to minimise some possible duplicated
work. If no, it does not make much sense to break this into 2 sets just to
introduce a testing in the first set.
If it helps you or someone to do the comparing test before and after patchset
easier, I would reorder the patch adding the micro-benchmark ko to the first
patch.
>
>>> What is meant to be the benefit to the community for adding this? All
>>> I am seeing is a ton of extra code to have to review as this
>>> unification is adding an additional 1000+ lines without a good
>>> explanation as to why they are needed.
>>
>> Some benefits I see for now:
>> 1. Improve the maintainability of page frag's implementation:
>> (1) future bugfix and performance can be done in one place.
>> For example, we may able to save some space for the
>> 'page_frag_cache' API user, and avoid 'get_page()' for
>> the old 'page_frag' API user.
>
> The problem as I see it is it is consolidating all the consumers down
> to the least common denominator in terms of performance. You have
> already demonstrated that with patch 2 which enforces that all drivers
> have to work from the bottom up instead of being able to work top down
> in the page.
I am agreed that consolidating 'the least common denominator' is what we
do when we design a subsystem/libary and sometimes we may need to have a
trade off between maintainability and perfromance.
But your argument 'having to load two registers with the values and then
compare them which saves us a few cycles' in [1] does not seems to justify
that we need to have it's own implementation of page_frag, not to mention
the 'work top down' way has its own disadvantages as mentioned in patch 2.
Also, in patch 5 & 6, we need to load 'size' to a register anyway so that we
can remove 'pagecnt_bias' and 'pfmemalloc' from 'struct page_frag_cache', it
would be better you can work through the whole patchset to get a bigger picture.
1. https://lore.kernel.org/all/f4abe71b3439b39d17a6fb2d410180f367cadf5c.camel@gmail.com/
>
> This eventually leads you down the path where every time somebody has
> a use case for it that may not be optimal for others it is going to be
> a fight to see if the new use case can degrade the performance of the
> other use cases.
I think it is always better to have a disscusion[or 'fight'] about how to
support a new use case:
1. refoctor the existing implementation to support the new use case, and
introduce a new API for it if have to.
2. if the above does not work, and the use case is important enough that
we might create/design a subsystem/libary for it.
But from updating page_frag API, I do not see that we need the second
option yet.
>
>> (2) Provide a proper API so that caller does not need to access
>> internal data field. Exposing the internal data field may
>> enable the caller to do some unexpcted implementation of
>> its own like below, after this patchset the API user is not
>> supposed to do access the data field of 'page_frag_cache'
>> directly[Currently it is still acessable from API caller if
>> the caller is not following the rule, I am not sure how to
>> limit the access without any performance impact yet].
>> https://elixir.bootlin.com/linux/v6.9-rc3/source/drivers/net/ethernet/chelsio/inline_crypto/chtls/chtls_io.c#L1141
>
> This just makes the issue I point out in 1 even worse. The problem is
> this code has to be used at the very lowest of levels and is as
> tightly optimized as it is since it is called at least once per packet
> in the case of networking. Networking that is still getting faster
> mind you and demanding even fewer cycles per packet to try and keep
> up. I just see this change as taking us in the wrong direction.
Yes, I am agreed with your point about 'demanding even fewer cycles per
packet', but not so with 'tightly optimized'.
'tightly optimized' may mean everybody inventing their own wheels.
>
>> 2. page_frag API may provide a central point for netwroking to allocate
>> memory instead of calling page allocator directly in the future, so
>> that we can decouple 'struct page' from networking.
>
> I hope not. The fact is the page allocator serves a very specific
> purpose, and the page frag API was meant to serve a different one and
> not be a replacement for it. One thing that has really irked me is the
> fact that I have seen it abused as much as it has been where people
> seem to think it is just a page allocator when it was really meant to
> just provide a way to shard order 0 pages into sizes that are half a
> page or less in size. I really meant for it to be a quick-n-dirty slab
> allocator for sizes 2K or less where ideally we are working with
> powers of 2.
>
> It concerns me that you are talking about taking this down a path that
> will likely lead to further misuse of the code as a backdoor way to
> allocate order 0 pages using this instead of just using the page
> allocator.
Let's not get to a conclusion here and wait to see how thing evolve
in the future.
>
>>>
>>> Also I wouldn't bother mentioning the 0.5+% performance gain as a
>>> "bonus". Changes of that amount usually mean it is within the margin
>>> of error. At best it likely means you haven't introduced a noticeable
>>> regression.
>>
>> For micro-benchmark ko added in this patchset, performance gain seems quit
>> stable from testing in system without any other load.
>
> Again, that doesn't mean anything. It could just be that the code
> shifted somewhere due to all the code moved so a loop got more aligned
> than it was before. To give you an idea I have seen performance gains
> in the past from turning off Rx checksum for some workloads and that
> was simply due to the fact that the CPUs were staying awake longer
> instead of going into deep sleep states as such we could handle more
> packets per second even though we were using more cycles. Without
> significantly more context it is hard to say that the gain is anything
> real at all and a 0.5% gain is well within that margin of error.
As vhost_net_test added in [2] is heavily invovled with tun and virtio
handling, the 0.5% gain does seems within that margin of error, there is
why I added a micro-benchmark specificly for page_frag in this patchset.
It is tested five times, three times with this patchset and two times without
this patchset, the complete log is as below, even there is some noise, all
the result with this patchset is better than the result without this patchset:
with this patchset:
Performance counter stats for 'insmod ./page_frag_test.ko nr_test=99999999' (30 runs):
40.09 msec task-clock # 0.001 CPUs utilized ( +- 4.60% )
5 context-switches # 124.722 /sec ( +- 3.45% )
1 cpu-migrations # 24.944 /sec ( +- 12.62% )
197 page-faults # 4.914 K/sec ( +- 0.11% )
10221721 cycles # 0.255 GHz ( +- 9.05% ) (27.73%)
2459009 stalled-cycles-frontend # 24.06% frontend cycles idle ( +- 10.80% ) (29.05%)
5148423 stalled-cycles-backend # 50.37% backend cycles idle ( +- 7.30% ) (82.47%)
5889929 instructions # 0.58 insn per cycle
# 0.87 stalled cycles per insn ( +- 11.85% ) (87.75%)
1276667 branches # 31.846 M/sec ( +- 11.48% ) (89.80%)
50631 branch-misses # 3.97% of all branches ( +- 8.72% ) (83.20%)
29.341 +- 0.300 seconds time elapsed ( +- 1.02% )
Performance counter stats for 'insmod ./page_frag_test.ko nr_test=99999999' (30 runs):
36.56 msec task-clock # 0.001 CPUs utilized ( +- 4.29% )
6 context-switches # 164.130 /sec ( +- 2.65% )
1 cpu-migrations # 27.355 /sec ( +- 15.67% )
197 page-faults # 5.389 K/sec ( +- 0.12% )
10006308 cycles # 0.274 GHz ( +- 8.36% ) (81.62%)
2928275 stalled-cycles-frontend # 29.26% frontend cycles idle ( +- 11.50% ) (82.62%)
5321882 stalled-cycles-backend # 53.19% backend cycles idle ( +- 8.39% ) (32.25%)
6653737 instructions # 0.66 insn per cycle
# 0.80 stalled cycles per insn ( +- 14.95% ) (37.23%)
1301600 branches # 35.605 M/sec ( +- 14.24% ) (86.14%)
47880 branch-misses # 3.68% of all branches ( +- 10.70% ) (80.16%)
28.683 +- 0.253 seconds time elapsed ( +- 0.88% )
Performance counter stats for 'insmod ./page_frag_test.ko nr_test=99999999' (30 runs):
39.02 msec task-clock # 0.001 CPUs utilized ( +- 4.13% )
6 context-switches # 153.753 /sec ( +- 2.98% )
1 cpu-migrations # 25.626 /sec ( +- 14.50% )
197 page-faults # 5.048 K/sec ( +- 0.08% )
10184452 cycles # 0.261 GHz ( +- 8.30% ) (40.64%)
2756400 stalled-cycles-frontend # 27.06% frontend cycles idle ( +- 10.82% ) (71.70%)
5127852 stalled-cycles-backend # 50.35% backend cycles idle ( +- 8.95% ) (78.94%)
6353385 instructions # 0.62 insn per cycle
# 0.81 stalled cycles per insn ( +- 18.79% ) (84.34%)
1409873 branches # 36.129 M/sec ( +- 23.85% ) (80.42%)
52044 branch-misses # 3.69% of all branches ( +- 10.68% ) (43.96%)
28.730 +- 0.201 seconds time elapsed ( +- 0.70% )
-----------------------------------------------------------------------------------------------------------
without this patchset:
Performance counter stats for 'insmod ./page_frag_test.ko nr_test=99999999' (30 runs):
39.12 msec task-clock # 0.001 CPUs utilized ( +- 4.51% )
5 context-switches # 127.805 /sec ( +- 3.76% )
1 cpu-migrations # 25.561 /sec ( +- 15.52% )
197 page-faults # 5.035 K/sec ( +- 0.10% )
10689913 cycles # 0.273 GHz ( +- 9.46% ) (72.72%)
2821237 stalled-cycles-frontend # 26.39% frontend cycles idle ( +- 12.04% ) (76.23%)
5035549 stalled-cycles-backend # 47.11% backend cycles idle ( +- 9.69% ) (49.40%)
5439395 instructions # 0.51 insn per cycle
# 0.93 stalled cycles per insn ( +- 11.58% ) (51.45%)
1274419 branches # 32.575 M/sec ( +- 12.69% ) (77.88%)
49562 branch-misses # 3.89% of all branches ( +- 9.91% ) (72.32%)
30.309 +- 0.305 seconds time elapsed ( +- 1.01% )
Performance counter stats for 'insmod ./page_frag_test.ko nr_test=99999999' (30 runs):
37.40 msec task-clock # 0.001 CPUs utilized ( +- 4.72% )
5 context-switches # 133.691 /sec ( +- 3.65% )
1 cpu-migrations # 26.738 /sec ( +- 14.13% )
197 page-faults # 5.267 K/sec ( +- 0.12% )
10196250 cycles # 0.273 GHz ( +- 9.37% ) (79.84%)
2579562 stalled-cycles-frontend # 25.30% frontend cycles idle ( +- 13.05% ) (48.29%)
4833236 stalled-cycles-backend # 47.40% backend cycles idle ( +- 9.84% ) (45.64%)
5992762 instructions # 0.59 insn per cycle
# 0.81 stalled cycles per insn ( +- 11.01% ) (76.56%)
1274592 branches # 34.080 M/sec ( +- 12.88% ) (74.52%)
51015 branch-misses # 4.00% of all branches ( +- 10.60% ) (75.15%)
29.958 +- 0.314 seconds time elapsed ( +- 1.05% )
2. https://lore.kernel.org/all/20240228093013.8263-6-linyunsheng@huawei.com/
> .
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: dts: meson: fix S4 power-controller node
From: Xianwei Zhao @ 2024-04-09 8:15 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: Rob Herring, Krzysztof Kozlowski, Conor Dooley, Neil Armstrong,
Kevin Hilman, Jerome Brunet, devicetree, linux-arm-kernel,
linux-amlogic, linux-kernel
In-Reply-To: <CAFBinCD=xWuhoX9cWcKU3bSGcsDShKbxnMVTdyfD84AFZQn8aw@mail.gmail.com>
Hi Martin,
Thanks for your review.
On 2024/4/9 01:27, Martin Blumenstingl wrote:
> [ EXTERNAL EMAIL ]
>
> On Mon, Apr 8, 2024 at 5:26 AM Xianwei Zhao via B4 Relay
> <devnull+xianwei.zhao.amlogic.com@kernel.org> wrote:
>>
>> From: Xianwei Zhao <xianwei.zhao@amlogic.com>
>>
>> The power-controller module works well by adding its parent
>> node secure-monitor.
>>
>
> Please add a Fixes tag here with the original commit where the
> incorrectly placed node was added.
Will add Fixes tag.
>> Signed-off-by: Xianwei Zhao <xianwei.zhao@amlogic.com>
>> ---
>> arch/arm64/boot/dts/amlogic/meson-s4.dtsi | 11 +++++++----
>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
>> index ce90b35686a2..24d00dce4969 100644
>> --- a/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
>> +++ b/arch/arm64/boot/dts/amlogic/meson-s4.dtsi
>> @@ -65,10 +65,13 @@ xtal: xtal-clk {
>> #clock-cells = <0>;
>> };
>>
>> - pwrc: power-controller {
>> - compatible = "amlogic,meson-s4-pwrc";
>> - #power-domain-cells = <1>;
>> - status = "okay";
>> + sm: secure-monitor {
>> + compatible = "amlogic,meson-gxbb-sm";
>> +
>> + pwrc: power-controller {
>> + compatible = "amlogic,meson-s4-pwrc";
>> + #power-domain-cells = <1>;
>> + };
> In Documentation/devicetree/bindings/firmware/amlogic,meson-gxbb-sm.yaml
> the hierarchy is:
> firmware {
> secure-monitor {
> power-controller {
> ...
> }
> }
> }
>
> Is this patch correct (and the documentation needs to be adapted) or
> is the documentation correct (and this patch has to be adapted)?
Will add firmware node to adapt documentation.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [syzbot] [mm?] BUG: unable to handle kernel paging request in copy_from_kernel_nofault (2)
From: Russell King (Oracle) @ 2024-04-09 8:15 UTC (permalink / raw)
To: Puranjay Mohan
Cc: Andrii Nakryiko, Alexei Starovoitov, Mark Rutland, Andrew Morton,
linux-arm-kernel, syzbot, LKML, linux-mm, syzkaller-bugs, bpf
In-Reply-To: <mb61pcyqzx9l9.fsf@gmail.com>
On Tue, Apr 09, 2024 at 07:45:54AM +0000, Puranjay Mohan wrote:
> "Russell King (Oracle)" <linux@armlinux.org.uk> writes:
>
> > On Fri, Apr 05, 2024 at 10:50:30AM -0700, Andrii Nakryiko wrote:
> >> On Fri, Apr 5, 2024 at 9:30 AM Alexei Starovoitov
> >> <alexei.starovoitov@gmail.com> wrote:
> >> >
> >> > On Fri, Apr 5, 2024 at 4:36 AM Russell King (Oracle)
> >> > <linux@armlinux.org.uk> wrote:
> >> > >
> >> > > On Fri, Apr 05, 2024 at 12:02:36PM +0100, Mark Rutland wrote:
> >> > > > On Thu, Apr 04, 2024 at 03:57:04PM -0700, Alexei Starovoitov wrote:
> >> > > > > On Wed, Apr 3, 2024 at 6:56 PM Andrew Morton <akpm@linux-foundationorg> wrote:
> >> > > > > >
> >> > > > > > On Mon, 01 Apr 2024 22:19:25 -0700 syzbot <syzbot+186522670e6722692d86@syzkaller.appspotmail.com> wrote:
> >> > > > > >
> >> > > > > > > Hello,
> >> > > > > >
> >> > > > > > Thanks. Cc: bpf@vger.kernel.org
> >> > > > >
> >> > > > > I suspect the issue is not on bpf side.
> >> > > > > Looks like the bug is somewhere in arm32 bits.
> >> > > > > copy_from_kernel_nofault() is called from lots of places.
> >> > > > > bpf is just one user that is easy for syzbot to fuzz.
> >> > > > > Interestingly arm defines copy_from_kernel_nofault_allowed()
> >> > > > > that should have filtered out user addresses.
> >> > > > > In this case ffffffe9 is probably a kernel address?
> >> > > >
> >> > > > It's at the end of the kernel range, and it's ERR_PTR(-EINVAL).
> >> > > >
> >> > > > 0xffffffe9 is -0x16, which is -22, which is -EINVAL.
> >> > > >
> >> > > > > But the kernel is doing a write?
> >> > > > > Which makes no sense, since copy_from_kernel_nofault is probe reading.
> >> > > >
> >> > > > It makes perfect sense; the read from 'src' happened, then the kernel tries to
> >> > > > write the result to 'dst', and that aligns with the disassembly in the report
> >> > > > below, which I beleive is:
> >> > > >
> >> > > > 8: e4942000 ldr r2, [r4], #0 <-- Read of 'src', fault fixup is elsewhere
> >> > > > c: e3530000 cmp r3, #0
> >> > > > * 10: e5852000 str r2, [r5] <-- Write to 'dst'
> >> > > >
> >> > > > As above, it looks like 'dst' is ERR_PTR(-EINVAL).
> >> > > >
> >> > > > Are you certain that BPF is passing a sane value for 'dst'? Where does that
> >> > > > come from in the first place?
> >> > >
> >> > > It looks to me like it gets passed in from the BPF program, and the
> >> > > "type" for the argument is set to ARG_PTR_TO_UNINIT_MEM. What that
> >> > > means for validation purposes, I've no idea, I'm not a BPF hacker.
> >> > >
> >> > > Obviously, if BPF is allowing copy_from_kernel_nofault() to be passed
> >> > > an arbitary destination address, that would be a huge security hole.
> >> >
> >> > If that's the case that's indeed a giant security hole,
> >> > but I doubt it. We would be crashing other archs as well.
> >> > I cannot really tell whether arm32 JIT is on.
> >> > If it is, it's likely a bug there.
> >> > Puranjay,
> >> > could you please take a look.
> >> >
> >>
> >> I dumped the BPF program that repro.c is loading, it works on x86-64
> >> and there is nothing special there. We are probe-reading 5 bytes from
> >> somewhere into the stack. Everything is unaligned here, but stays
> >> within a well-defined memory slot.
> >>
> >> Note the r3 = (s8)r1, that's a new-ish thing, maybe bug is somewhere
> >> there (but then it would be JIT, not verifier itself)
> >>
> >> 0: (7a) *(u64 *)(r10 -8) = 896542069
> >> 1: (bf) r1 = r10
> >> 2: (07) r1 += -7
> >> 3: (b7) r2 = 5
> >> 4: (bf) r3 = (s8)r1
> >> 5: (85) call bpf_probe_read_kernel#-72390
> >
>
> I have started looking into this, the issue only reproduces when the JIT
> is enabled. With the interpreter, it works fine.
>
> I used GDB to dump the JITed BPF program:
>
> 0xbf00012c: push {r4, r5, r6, r7, r8, r9, r11, lr}
> 0xbf000130: mov r11, sp
> 0xbf000134: mov r3, #0
> 0xbf000138: sub r2, sp, #80 @ 0x50
> 0xbf00013c: sub sp, sp, #88 @ 0x58
> 0xbf000140: strd r2, [r11, #-64] @ 0xffffffc0
> 0xbf000144: mov r2, #0
> 0xbf000148: strd r2, [r11, #-72] @ 0xffffffb8
> 0xbf00014c: mov r2, r0
> 0xbf000150: movw r8, #9589 @ 0x2575
> 0xbf000154: movt r8, #13680 @ 0x3570
> 0xbf000158: mov r9, #0
> 0xbf00015c: ldr r6, [r11, #-64] @ 0xffffffc0
> 0xbf000160: str r8, [r6, #-8]
> 0xbf000164: str r9, [r6, #-4]
> 0xbf000168: ldrd r2, [r11, #-64] @ 0xffffffc0
> 0xbf00016c: movw r8, #65529 @ 0xfff9
> 0xbf000170: movt r8, #65535 @ 0xffff
> 0xbf000174: movw r9, #65535 @ 0xffff
> 0xbf000178: movt r9, #65535 @ 0xffff
> 0xbf00017c: adds r2, r2, r8
> 0xbf000180: adc r3, r3, r9
> 0xbf000184: mov r6, #5
> 0xbf000188: mov r7, #0
> 0xbf00018c: strd r6, [r11, #-8]
> 0xbf000190: ldrd r6, [r11, #-16]
Up to this point, it looks correct. r2/r3 contain the stack pointer
which corresponds to the instruction at "2:"
> 0xbf000194: lsl r2, r2, #24
> 0xbf000198: asr r2, r2, #24
> 0xbf00019c: str r2, [r11, #-16]
This then narrows the 64-bit pointer down to just 8!!! bits, but this
is what the instruction at "4:" is asking for. However, it looks like
it's happening to BPF's "r1" rather than "r3" and this is probably
where the problem lies.
I haven't got time to analyse this further this morning - I'm only
around sporadically today. I'll try to look deeper at this later on.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Zapytanie ofertowe
From: Patryk Wysocki @ 2024-04-09 7:35 UTC (permalink / raw)
To: linux-arm-kernel
Dzień dobry,
Pozwoliłem sobie na kontakt, ponieważ jestem zainteresowany weryfikacją możliwości nawiązania współpracy.
Wspieramy firmy w pozyskiwaniu nowych klientów biznesowych.
Czy możemy porozmawiać w celu przedstawienia szczegółowych informacji?
Pozdrawiam
Patryk Wysocki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 01/15] bug/kunit: Core support for suppressing warning backtraces
From: David Gow @ 2024-04-09 8:29 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kselftest, David Airlie, Arnd Bergmann, Maíra Canal,
Dan Carpenter, Kees Cook, Daniel Diaz, Arthur Grillo,
Brendan Higgins, Naresh Kamboju, Maarten Lankhorst, Andrew Morton,
Maxime Ripard, Ville Syrjälä, Daniel Vetter,
Thomas Zimmermann, dri-devel, kunit-dev, linux-arch,
linux-arm-kernel, linux-doc, linux-kernel, linux-parisc,
linuxppc-dev, linux-riscv, linux-s390, linux-sh, loongarch,
netdev, x86, Linux Kernel Functional Testing
In-Reply-To: <20240403131936.787234-2-linux@roeck-us.net>
[-- Attachment #1.1: Type: text/plain, Size: 18263 bytes --]
On Wed, 3 Apr 2024 at 21:19, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Some unit tests intentionally trigger warning backtraces by passing
> bad parameters to API functions. Such unit tests typically check the
> return value from those calls, not the existence of the warning backtrace.
>
> Such intentionally generated warning backtraces are neither desirable
> nor useful for a number of reasons.
> - They can result in overlooked real problems.
> - A warning that suddenly starts to show up in unit tests needs to be
> investigated and has to be marked to be ignored, for example by
> adjusting filter scripts. Such filters are ad-hoc because there is
> no real standard format for warnings. On top of that, such filter
> scripts would require constant maintenance.
>
> One option to address problem would be to add messages such as "expected
> warning backtraces start / end here" to the kernel log. However, that
> would again require filter scripts, it might result in missing real
> problematic warning backtraces triggered while the test is running, and
> the irrelevant backtrace(s) would still clog the kernel log.
>
> Solve the problem by providing a means to identify and suppress specific
> warning backtraces while executing test code. Since the new functionality
> results in an image size increase of about 1% if CONFIG_KUNIT is enabled,
> provide configuration option KUNIT_SUPPRESS_BACKTRACE to be able to disable
> the new functionality. This option is by default enabled since almost all
> systems with CONFIG_KUNIT enabled will want to benefit from it.
>
> Cc: Dan Carpenter <dan.carpenter@linaro.org>
> Cc: Daniel Diaz <daniel.diaz@linaro.org>
> Cc: Naresh Kamboju <naresh.kamboju@linaro.org>
> Cc: Kees Cook <keescook@chromium.org>
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Acked-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
Sorry it took so long to get to this.
I love the idea, we've needed this for a while.
There are some downsides to this being entirely based on the name of
the function which contains WARN(). Partly because there could be
several WARN()s within a function, and there'd be overlap, and partly
because the function name is never actually printed during a warning
(it may come from the stack trace, but that can be misleading with
inlined functions). I don't think either of these are showstoppers,
though, but it'd be nice to extend this in the future with (a) other
ways of identifying warnings, such as the format string, and (b) print
the function name in the report, if it's present. The function name is
probably a good middle ground, complexity-wise, though, so I'm happy
to have it thus far.
I also think we're missing some opportunities to integrate this
better with existing KUnit infrastructure, like the
action/resource/cleanup system. In particular, it'd be nice to have a
way of ensuring that suppressions won't get leaked if the test aborts
between START_SUPPRESSED_WARNING() and END_SUPPRESSED_WARNING(). It's
not difficult to use this as-is, but it'd be nice to have some
helpers, rather than having to, for instance:
KUNIT_DEFINE_ACTION_WRAPPER(kunit_stop_suppressing_warning,
__end_suppress_warning, struct __suppressed_warning *);
DEFINE_SUPPRESSED_WARNING(vfree);
START_SUPPRESSED_WARNING(vfree);
kunit_add_action(test, kunit_stop_suppressing_warning, (void
*)&__kunit_suppress_vfree);
(With the note that the DEFINE_SUPPRESSED_WARNING() will have to be
global, or put on the heap, lest it become a dangling pointer by the
time the suppression has stopped.)
Equally, do we want to make the
__{start,end,is}_suppress[ed]_warning() functions KUnit 'hooks'? This
would allow them to be used in modules which don't depend directly on
KUnit. I suspect it's not important in this case: but worth keeping in
mind in case we find a situation where we'd need to suppress a warning
elsewhere.
These are all things which could be added/changed in follow-up
patches, though, so I don't think they're blockers. Otherwise, this
looks good: perhaps the naming could be a bit more consistent with
other KUnit things, but that depends on how much we want this to be 'a
part of KUnit' versus an independent bit of functionality.
> v2:
> - Rebased to v6.9-rc1
> - Added Tested-by:, Acked-by:, and Reviewed-by: tags
> - Added CONFIG_KUNIT_SUPPRESS_BACKTRACE configuration option,
> enabled by default
> v3:
> - Rebased to v6.9-rc2
>
> include/asm-generic/bug.h | 16 +++++++++---
> include/kunit/bug.h | 51 +++++++++++++++++++++++++++++++++++++++
> include/kunit/test.h | 1 +
> include/linux/bug.h | 13 ++++++++++
> lib/bug.c | 51 ++++++++++++++++++++++++++++++++++++---
> lib/kunit/Kconfig | 9 +++++++
> lib/kunit/Makefile | 6 +++--
> lib/kunit/bug.c | 40 ++++++++++++++++++++++++++++++
> 8 files changed, 178 insertions(+), 9 deletions(-)
> create mode 100644 include/kunit/bug.h
> create mode 100644 lib/kunit/bug.c
>
> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 6e794420bd39..c170b6477689 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -18,6 +18,7 @@
> #endif
>
> #ifndef __ASSEMBLY__
> +#include <kunit/bug.h>
> #include <linux/panic.h>
> #include <linux/printk.h>
>
> @@ -39,8 +40,14 @@ struct bug_entry {
> #ifdef CONFIG_DEBUG_BUGVERBOSE
> #ifndef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
> const char *file;
> +#ifdef HAVE_BUG_FUNCTION
> + const char *function;
> +#endif
> #else
> signed int file_disp;
> +#ifdef HAVE_BUG_FUNCTION
> + signed int function_disp;
> +#endif
> #endif
> unsigned short line;
> #endif
> @@ -96,15 +103,18 @@ extern __printf(1, 2) void __warn_printk(const char *fmt, ...);
> #define __WARN() __WARN_printf(TAINT_WARN, NULL)
> #define __WARN_printf(taint, arg...) do { \
> instrumentation_begin(); \
> - warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
> + if (!IS_SUPPRESSED_WARNING(__func__)) \
> + warn_slowpath_fmt(__FILE__, __LINE__, taint, arg);\
> instrumentation_end(); \
> } while (0)
> #else
> #define __WARN() __WARN_FLAGS(BUGFLAG_TAINT(TAINT_WARN))
> #define __WARN_printf(taint, arg...) do { \
> instrumentation_begin(); \
> - __warn_printk(arg); \
> - __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\
> + if (!IS_SUPPRESSED_WARNING(__func__)) { \
> + __warn_printk(arg); \
> + __WARN_FLAGS(BUGFLAG_NO_CUT_HERE | BUGFLAG_TAINT(taint));\
> + } \
> instrumentation_end(); \
> } while (0)
> #define WARN_ON_ONCE(condition) ({ \
> diff --git a/include/kunit/bug.h b/include/kunit/bug.h
> new file mode 100644
> index 000000000000..bd0fe047572b
> --- /dev/null
> +++ b/include/kunit/bug.h
> @@ -0,0 +1,51 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * KUnit helpers for backtrace suppression
> + *
> + * Copyright (c) 2024 Guenter Roeck <linux@roeck-us.net>
> + */
> +
> +#ifndef _KUNIT_BUG_H
> +#define _KUNIT_BUG_H
> +
> +#ifndef __ASSEMBLY__
> +
> +#include <linux/kconfig.h>
> +
> +#ifdef CONFIG_KUNIT_SUPPRESS_BACKTRACE
> +
> +#include <linux/stringify.h>
> +#include <linux/types.h>
> +
> +struct __suppressed_warning {
> + struct list_head node;
> + const char *function;
> +};
> +
> +void __start_suppress_warning(struct __suppressed_warning *warning);
> +void __end_suppress_warning(struct __suppressed_warning *warning);
> +bool __is_suppressed_warning(const char *function);
Do we want to call these '__kunit_start_suppress_warning', etc., to
match other similar functions exported by KUnit to be used in macros,
et al.
> +
> +#define DEFINE_SUPPRESSED_WARNING(func) \
> + struct __suppressed_warning __kunit_suppress_##func = \
We use the __kunit_ prefix here...
> + { .function = __stringify(func) }
> +
> +#define START_SUPPRESSED_WARNING(func) \
> + __start_suppress_warning(&__kunit_suppress_##func)
> +
> +#define END_SUPPRESSED_WARNING(func) \
> + __end_suppress_warning(&__kunit_suppress_##func)
> +
> +#define IS_SUPPRESSED_WARNING(func) \
> + __is_suppressed_warning(func)
> +
Similarly, do we want to give these KUNIT_ prefixes to match other KUnit macros.
One possibility would be to have both KUNIT_- and non-KUNIT_-
variants, the latter of which accepts a struct kunit*, and registers
the suppression with the test for automated cleanup.
> +#else /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
> +
> +#define DEFINE_SUPPRESSED_WARNING(func)
> +#define START_SUPPRESSED_WARNING(func)
> +#define END_SUPPRESSED_WARNING(func)
> +#define IS_SUPPRESSED_WARNING(func) (false)
> +
> +#endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
> +#endif /* __ASSEMBLY__ */
> +#endif /* _KUNIT_BUG_H */
> diff --git a/include/kunit/test.h b/include/kunit/test.h
> index 61637ef32302..d0c44594d34c 100644
> --- a/include/kunit/test.h
> +++ b/include/kunit/test.h
> @@ -10,6 +10,7 @@
> #define _KUNIT_TEST_H
>
> #include <kunit/assert.h>
> +#include <kunit/bug.h>
> #include <kunit/try-catch.h>
>
> #include <linux/args.h>
> diff --git a/include/linux/bug.h b/include/linux/bug.h
> index 348acf2558f3..c668762dc76a 100644
> --- a/include/linux/bug.h
> +++ b/include/linux/bug.h
> @@ -36,6 +36,9 @@ static inline int is_warning_bug(const struct bug_entry *bug)
> return bug->flags & BUGFLAG_WARNING;
> }
>
> +void bug_get_file_function_line(struct bug_entry *bug, const char **file,
> + const char **function, unsigned int *line);
> +
> void bug_get_file_line(struct bug_entry *bug, const char **file,
> unsigned int *line);
>
> @@ -62,6 +65,16 @@ static inline enum bug_trap_type report_bug(unsigned long bug_addr,
> }
>
> struct bug_entry;
> +static inline void bug_get_file_function_line(struct bug_entry *bug,
> + const char **file,
> + const char **function,
> + unsigned int *line)
> +{
> + *file = NULL;
> + *function = NULL;
> + *line = 0;
> +}
> +
> static inline void bug_get_file_line(struct bug_entry *bug, const char **file,
> unsigned int *line)
> {
> diff --git a/lib/bug.c b/lib/bug.c
> index e0ff21989990..aa8bb12b9809 100644
> --- a/lib/bug.c
> +++ b/lib/bug.c
> @@ -26,6 +26,14 @@
> when CONFIG_DEBUG_BUGVERBOSE is not enabled, so you must generate
> the values accordingly.
>
> + 2a.Optionally implement support for the "function" entry in struct
> + bug_entry. This entry must point to the name of the function triggering
> + the warning or bug trap (normally __func__). This is only needed if
> + both CONFIG_DEBUG_BUGVERBOSE and CONFIG_KUNIT_SUPPRESS_BACKTRACE are
> + enabled and if the architecture wants to implement support for suppressing
> + warning backtraces. The architecture must define HAVE_BUG_FUNCTION if it
> + adds pointers to function names to struct bug_entry.
> +
> 3. Implement the trap
> - In the illegal instruction trap handler (typically), verify
> that the fault was in kernel mode, and call report_bug()
> @@ -127,14 +135,21 @@ static inline struct bug_entry *module_find_bug(unsigned long bugaddr)
> }
> #endif
>
> -void bug_get_file_line(struct bug_entry *bug, const char **file,
> - unsigned int *line)
> +void bug_get_file_function_line(struct bug_entry *bug, const char **file,
> + const char **function, unsigned int *line)
> {
> + *function = NULL;
> #ifdef CONFIG_DEBUG_BUGVERBOSE
> #ifdef CONFIG_GENERIC_BUG_RELATIVE_POINTERS
> *file = (const char *)&bug->file_disp + bug->file_disp;
> +#ifdef HAVE_BUG_FUNCTION
> + *function = (const char *)&bug->function_disp + bug->function_disp;
> +#endif
> #else
> *file = bug->file;
> +#ifdef HAVE_BUG_FUNCTION
> + *function = bug->function;
> +#endif
> #endif
> *line = bug->line;
> #else
> @@ -143,6 +158,13 @@ void bug_get_file_line(struct bug_entry *bug, const char **file,
> #endif
> }
>
> +void bug_get_file_line(struct bug_entry *bug, const char **file, unsigned int *line)
> +{
> + const char *function;
> +
> + bug_get_file_function_line(bug, file, &function, line);
> +}
> +
> struct bug_entry *find_bug(unsigned long bugaddr)
> {
> struct bug_entry *bug;
> @@ -157,8 +179,9 @@ struct bug_entry *find_bug(unsigned long bugaddr)
> static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *regs)
> {
> struct bug_entry *bug;
> - const char *file;
> + const char *file, *function;
As mentioned, I'd love to see the function plumbed through and
reported some day, both to make it easier to know what to suppress,
and also because it's possibly more reliable even outside the
suppression use-case. Could be a follow-up patch later, though.
> unsigned line, warning, once, done;
> + char __maybe_unused sym[KSYM_SYMBOL_LEN];
>
> if (!is_valid_bugaddr(bugaddr))
> return BUG_TRAP_TYPE_NONE;
> @@ -169,12 +192,32 @@ static enum bug_trap_type __report_bug(unsigned long bugaddr, struct pt_regs *re
>
> disable_trace_on_warning();
>
> - bug_get_file_line(bug, &file, &line);
> + bug_get_file_function_line(bug, &file, &function, &line);
> +#if defined(CONFIG_KUNIT_SUPPRESS_BACKTRACE) && defined(CONFIG_KALLSYMS)
> + if (!function) {
> + /*
> + * This will be seen if report_bug is called on an architecture
> + * with no architecture-specific support for suppressing warning
> + * backtraces, if CONFIG_DEBUG_BUGVERBOSE is not enabled, or if
> + * the calling code is from assembler which does not record a
> + * function name. Extracting the function name from the bug
> + * address is less than perfect since compiler optimization may
> + * result in 'bugaddr' pointing to a function which does not
> + * actually trigger the warning, but it is better than no
> + * suppression at all.
> + */
> + sprint_symbol_no_offset(sym, bugaddr);
> + function = sym;
> + }
> +#endif /* defined(CONFIG_KUNIT_SUPPRESS_BACKTRACE) && defined(CONFIG_KALLSYMS) */
>
> warning = (bug->flags & BUGFLAG_WARNING) != 0;
> once = (bug->flags & BUGFLAG_ONCE) != 0;
> done = (bug->flags & BUGFLAG_DONE) != 0;
>
> + if (warning && IS_SUPPRESSED_WARNING(function))
> + return BUG_TRAP_TYPE_WARN;
> +
> if (warning && once) {
> if (done)
> return BUG_TRAP_TYPE_WARN;
> diff --git a/lib/kunit/Kconfig b/lib/kunit/Kconfig
> index 68a6daec0aef..b1b899265acc 100644
> --- a/lib/kunit/Kconfig
> +++ b/lib/kunit/Kconfig
> @@ -15,6 +15,15 @@ menuconfig KUNIT
>
> if KUNIT
>
> +config KUNIT_SUPPRESS_BACKTRACE
> + bool "KUnit - Enable backtrace suppression"
> + default y
> + help
> + Enable backtrace suppression for KUnit. If enabled, backtraces
> + generated intentionally by KUnit tests are suppressed. Disable
> + to reduce kernel image size if image size is more important than
> + suppression of backtraces generated by KUnit tests.
> +
> config KUNIT_DEBUGFS
> bool "KUnit - Enable /sys/kernel/debug/kunit debugfs representation" if !KUNIT_ALL_TESTS
> default KUNIT_ALL_TESTS
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index 309659a32a78..545b57c3be48 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -14,8 +14,10 @@ ifeq ($(CONFIG_KUNIT_DEBUGFS),y)
> kunit-objs += debugfs.o
> endif
>
> -# KUnit 'hooks' are built-in even when KUnit is built as a module.
> -obj-y += hooks.o
> +# KUnit 'hooks' and bug handling are built-in even when KUnit is built
> +# as a module.
> +obj-y += hooks.o \
> + bug.o
>
> obj-$(CONFIG_KUNIT_TEST) += kunit-test.o
>
> diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c
> new file mode 100644
> index 000000000000..f93544d7a9d1
> --- /dev/null
> +++ b/lib/kunit/bug.c
> @@ -0,0 +1,40 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit helpers for backtrace suppression
> + *
> + * Copyright (c) 2024 Guenter Roeck <linux@roeck-us.net>
> + */
> +
> +#include <kunit/bug.h>
> +#include <linux/export.h>
> +#include <linux/list.h>
> +#include <linux/string.h>
> +
> +static LIST_HEAD(suppressed_warnings);
> +
> +void __start_suppress_warning(struct __suppressed_warning *warning)
> +{
> + list_add(&warning->node, &suppressed_warnings);
> +}
> +EXPORT_SYMBOL_GPL(__start_suppress_warning);
> +
> +void __end_suppress_warning(struct __suppressed_warning *warning)
> +{
> + list_del(&warning->node);
> +}
> +EXPORT_SYMBOL_GPL(__end_suppress_warning);
> +
> +bool __is_suppressed_warning(const char *function)
> +{
> + struct __suppressed_warning *warning;
> +
> + if (!function)
> + return false;
> +
> + list_for_each_entry(warning, &suppressed_warnings, node) {
> + if (!strcmp(function, warning->function))
> + return true;
> + }
> + return false;
> +}
> +EXPORT_SYMBOL_GPL(__is_suppressed_warning);
> --
> 2.39.2
>
Thanks,
-- David
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 02/15] kunit: bug: Count suppressed warning backtraces
From: David Gow @ 2024-04-09 8:29 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kselftest, David Airlie, Arnd Bergmann, Maíra Canal,
Dan Carpenter, Kees Cook, Daniel Diaz, Arthur Grillo,
Brendan Higgins, Naresh Kamboju, Maarten Lankhorst, Andrew Morton,
Maxime Ripard, Ville Syrjälä, Daniel Vetter,
Thomas Zimmermann, dri-devel, kunit-dev, linux-arch,
linux-arm-kernel, linux-doc, linux-kernel, linux-parisc,
linuxppc-dev, linux-riscv, linux-s390, linux-sh, loongarch,
netdev, x86, Linux Kernel Functional Testing
In-Reply-To: <20240403131936.787234-3-linux@roeck-us.net>
[-- Attachment #1.1: Type: text/plain, Size: 3349 bytes --]
On Wed, 3 Apr 2024 at 21:19, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Count suppressed warning backtraces to enable code which suppresses
> warning backtraces to check if the expected backtrace(s) have been
> observed.
>
> Using atomics for the backtrace count resulted in build errors on some
> architectures due to include file recursion, so use a plain integer
> for now.
>
> Acked-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
Looks good to me, thanks.
Reviewed-by: David Gow <davidgow@google.com>
Cheers,
-- David
> v2:
> - Rebased to v6.9-rc1
> - Added Tested-by:, Acked-by:, and Reviewed-by: tags
> - Introduced KUNIT_SUPPRESS_BACKTRACE configuration option
> v3:
> - Rebased to v6.9-rc2
>
> include/kunit/bug.h | 7 ++++++-
> lib/kunit/bug.c | 4 +++-
> 2 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/include/kunit/bug.h b/include/kunit/bug.h
> index bd0fe047572b..72e9fb23bbd5 100644
> --- a/include/kunit/bug.h
> +++ b/include/kunit/bug.h
> @@ -20,6 +20,7 @@
> struct __suppressed_warning {
> struct list_head node;
> const char *function;
> + int counter;
> };
>
> void __start_suppress_warning(struct __suppressed_warning *warning);
> @@ -28,7 +29,7 @@ bool __is_suppressed_warning(const char *function);
>
> #define DEFINE_SUPPRESSED_WARNING(func) \
> struct __suppressed_warning __kunit_suppress_##func = \
> - { .function = __stringify(func) }
> + { .function = __stringify(func), .counter = 0 }
>
> #define START_SUPPRESSED_WARNING(func) \
> __start_suppress_warning(&__kunit_suppress_##func)
> @@ -39,12 +40,16 @@ bool __is_suppressed_warning(const char *function);
> #define IS_SUPPRESSED_WARNING(func) \
> __is_suppressed_warning(func)
>
> +#define SUPPRESSED_WARNING_COUNT(func) \
> + (__kunit_suppress_##func.counter)
> +
> #else /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
>
> #define DEFINE_SUPPRESSED_WARNING(func)
> #define START_SUPPRESSED_WARNING(func)
> #define END_SUPPRESSED_WARNING(func)
> #define IS_SUPPRESSED_WARNING(func) (false)
> +#define SUPPRESSED_WARNING_COUNT(func) (0)
>
> #endif /* CONFIG_KUNIT_SUPPRESS_BACKTRACE */
> #endif /* __ASSEMBLY__ */
> diff --git a/lib/kunit/bug.c b/lib/kunit/bug.c
> index f93544d7a9d1..13b3d896c114 100644
> --- a/lib/kunit/bug.c
> +++ b/lib/kunit/bug.c
> @@ -32,8 +32,10 @@ bool __is_suppressed_warning(const char *function)
> return false;
>
> list_for_each_entry(warning, &suppressed_warnings, node) {
> - if (!strcmp(function, warning->function))
> + if (!strcmp(function, warning->function)) {
> + warning->counter++;
> return true;
> + }
> }
> return false;
> }
> --
> 2.39.2
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20240403131936.787234-3-linux%40roeck-us.net.
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 03/15] kunit: Add test cases for backtrace warning suppression
From: David Gow @ 2024-04-09 8:29 UTC (permalink / raw)
To: Guenter Roeck
Cc: linux-kselftest, David Airlie, Arnd Bergmann, Maíra Canal,
Dan Carpenter, Kees Cook, Daniel Diaz, Arthur Grillo,
Brendan Higgins, Naresh Kamboju, Maarten Lankhorst, Andrew Morton,
Maxime Ripard, Ville Syrjälä, Daniel Vetter,
Thomas Zimmermann, dri-devel, kunit-dev, linux-arch,
linux-arm-kernel, linux-doc, linux-kernel, linux-parisc,
linuxppc-dev, linux-riscv, linux-s390, linux-sh, loongarch,
netdev, x86, Linux Kernel Functional Testing
In-Reply-To: <20240403131936.787234-4-linux@roeck-us.net>
[-- Attachment #1.1: Type: text/plain, Size: 6122 bytes --]
On Wed, 3 Apr 2024 at 21:19, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Add unit tests to verify that warning backtrace suppression works.
>
> If backtrace suppression does _not_ work, the unit tests will likely
> trigger unsuppressed backtraces, which should actually help to get
> the affected architectures / platforms fixed.
>
> Tested-by: Linux Kernel Functional Testing <lkft@linaro.org>
> Acked-by: Dan Carpenter <dan.carpenter@linaro.org>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
There's a typo in the Makefile, which stops this from being built at
all. Otherwise, it seems good to me.
-- David
> v2:
> - Rebased to v6.9-rc1
> - Added Tested-by:, Acked-by:, and Reviewed-by: tags
> - Introduced KUNIT_SUPPRESS_BACKTRACE configuration option
> v3:
> - Rebased to v6.9-rc2
>
> lib/kunit/Makefile | 7 +-
> lib/kunit/backtrace-suppression-test.c | 104 +++++++++++++++++++++++++
> 2 files changed, 109 insertions(+), 2 deletions(-)
> create mode 100644 lib/kunit/backtrace-suppression-test.c
>
> diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
> index 545b57c3be48..3eee1bd0ce5e 100644
> --- a/lib/kunit/Makefile
> +++ b/lib/kunit/Makefile
> @@ -16,10 +16,13 @@ endif
>
> # KUnit 'hooks' and bug handling are built-in even when KUnit is built
> # as a module.
> -obj-y += hooks.o \
> - bug.o
> +obj-y += hooks.o
> +obj-$(CONFIG_KUNIT_SUPPRESS_BACKTRACE) += bug.o
>
> obj-$(CONFIG_KUNIT_TEST) += kunit-test.o
> +ifeq ($(CCONFIG_KUNIT_SUPPRESS_BACKTRACE),y)
s/CCONFIG_/CONFIG_/ ?
> +obj-$(CONFIG_KUNIT_TEST) += backtrace-suppression-test.o
> +endif
>
> # string-stream-test compiles built-in only.
> ifeq ($(CONFIG_KUNIT_TEST),y)
> diff --git a/lib/kunit/backtrace-suppression-test.c b/lib/kunit/backtrace-suppression-test.c
> new file mode 100644
> index 000000000000..47c619283802
> --- /dev/null
> +++ b/lib/kunit/backtrace-suppression-test.c
> @@ -0,0 +1,104 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * KUnit test for suppressing warning tracebacks
> + *
> + * Copyright (C) 2024, Guenter Roeck
> + * Author: Guenter Roeck <linux@roeck-us.net>
> + */
> +
> +#include <kunit/test.h>
> +#include <linux/bug.h>
> +
> +static void backtrace_suppression_test_warn_direct(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);
> +
> + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);
> + WARN(1, "This backtrace should be suppressed");
> + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_direct);
> +
> + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_direct), 1);
> +}
> +
> +static void trigger_backtrace_warn(void)
> +{
> + WARN(1, "This backtrace should be suppressed");
> +}
> +
> +static void backtrace_suppression_test_warn_indirect(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn);
> +
> + START_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + trigger_backtrace_warn();
> + END_SUPPRESSED_WARNING(trigger_backtrace_warn);
> +
> + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1);
> +}
> +
> +static void backtrace_suppression_test_warn_multi(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);
> +
> + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);
> + START_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + WARN(1, "This backtrace should be suppressed");
> + trigger_backtrace_warn();
> + END_SUPPRESSED_WARNING(trigger_backtrace_warn);
> + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_multi);
> +
> + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_multi), 1);
> + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn), 1);
> +}
> +
> +static void backtrace_suppression_test_warn_on_direct(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);
> +
> + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE) && !IS_ENABLED(CONFIG_KALLSYMS))
> + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE or CONFIG_KALLSYMS");
> +
> + START_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);
> + WARN_ON(1);
> + END_SUPPRESSED_WARNING(backtrace_suppression_test_warn_on_direct);
> +
> + KUNIT_EXPECT_EQ(test,
> + SUPPRESSED_WARNING_COUNT(backtrace_suppression_test_warn_on_direct), 1);
> +}
> +
> +static void trigger_backtrace_warn_on(void)
> +{
> + WARN_ON(1);
> +}
> +
> +static void backtrace_suppression_test_warn_on_indirect(struct kunit *test)
> +{
> + DEFINE_SUPPRESSED_WARNING(trigger_backtrace_warn_on);
> +
> + if (!IS_ENABLED(CONFIG_DEBUG_BUGVERBOSE))
> + kunit_skip(test, "requires CONFIG_DEBUG_BUGVERBOSE");
> +
> + START_SUPPRESSED_WARNING(trigger_backtrace_warn_on);
> + trigger_backtrace_warn_on();
> + END_SUPPRESSED_WARNING(trigger_backtrace_warn_on);
> +
> + KUNIT_EXPECT_EQ(test, SUPPRESSED_WARNING_COUNT(trigger_backtrace_warn_on), 1);
> +}
> +
> +static struct kunit_case backtrace_suppression_test_cases[] = {
> + KUNIT_CASE(backtrace_suppression_test_warn_direct),
> + KUNIT_CASE(backtrace_suppression_test_warn_indirect),
> + KUNIT_CASE(backtrace_suppression_test_warn_multi),
> + KUNIT_CASE(backtrace_suppression_test_warn_on_direct),
> + KUNIT_CASE(backtrace_suppression_test_warn_on_indirect),
> + {}
> +};
> +
> +static struct kunit_suite backtrace_suppression_test_suite = {
> + .name = "backtrace-suppression-test",
> + .test_cases = backtrace_suppression_test_cases,
> +};
> +kunit_test_suites(&backtrace_suppression_test_suite);
> +
> +MODULE_LICENSE("GPL");
> --
> 2.39.2
>
[-- Attachment #1.2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4014 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] Add support for flower actions mirred and redirect
From: patchwork-bot+netdevbpf @ 2024-04-09 8:50 UTC (permalink / raw)
To: Daniel Machon
Cc: lars.povlsen, Steen.Hegelund, UNGLinuxDriver, davem, edumazet,
kuba, pabeni, rkannoth, linux-arm-kernel, netdev, linux-kernel
In-Reply-To: <20240405-mirror-redirect-actions-v2-0-875d4c1927c8@microchip.com>
Hello:
This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Fri, 5 Apr 2024 09:44:48 +0200 you wrote:
> ================================================================================
> Add support for tc flower actions mirred and redirect.
> ================================================================================
>
> This series adds support for the two tc flower actions mirred and
> redirect. Both actions are implemented by means of a port mask and a
> mask mode. The mask mode controls how the mask is applied, and together
> they are used by the switch to make a forwarding decision. Both actions
> are configurable via the IS0 or IS2 VCAP's (ingress stage 0 and 2,
> respectively).
>
> [...]
Here is the summary with links:
- [net-next,v2,1/2] net: sparx5: add support for tc flower mirred action.
https://git.kernel.org/netdev/net-next/c/48ba00da2eb4
- [net-next,v2,2/2] net: sparx5: add support for tc flower redirect action
https://git.kernel.org/netdev/net-next/c/1164b8e0b108
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH net-next v6 00/10] Support ICSSG-based Ethernet on AM65x SR1.0 devices
From: patchwork-bot+netdevbpf @ 2024-04-09 8:50 UTC (permalink / raw)
To: Diogo Ivo
Cc: danishanwar, rogerq, davem, edumazet, kuba, pabeni, andrew,
dan.carpenter, jacob.e.keller, robh, robh+dt,
krzysztof.kozlowski+dt, conor+dt, vigneshr, wsa+renesas,
hkallweit1, arnd, vladimir.oltean, linux-arm-kernel, netdev,
devicetree, jan.kiszka
In-Reply-To: <20240403104821.283832-1-diogo.ivo@siemens.com>
Hello:
This series was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:
On Wed, 3 Apr 2024 11:48:10 +0100 you wrote:
> Hello,
>
> This series extends the current ICSSG-based Ethernet driver to support
> AM65x Silicon Revision 1.0 devices.
>
> Notable differences between the Silicon Revisions are that there is
> no TX core in SR1.0 with this being handled by the firmware, requiring
> extra DMA channels to manage communication with the firmware (with the
> firmware being different as well) and in the packet classifier.
>
> [...]
Here is the summary with links:
- [net-next,v6,01/10] dt-bindings: net: Add support for AM65x SR1.0 in ICSSG
https://git.kernel.org/netdev/net-next/c/dc073430db8d
- [net-next,v6,02/10] eth: Move IPv4/IPv6 multicast address bases to their own symbols
https://git.kernel.org/netdev/net-next/c/e1900d7ba9c9
- [net-next,v6,03/10] net: ti: icssg-prueth: Move common functions into a separate file
https://git.kernel.org/netdev/net-next/c/e2dc7bfd677f
- [net-next,v6,04/10] net: ti: icssg-prueth: Add SR1.0-specific configuration bits
https://git.kernel.org/netdev/net-next/c/6d6a5751cd8e
- [net-next,v6,05/10] net: ti: icssg-prueth: Add SR1.0-specific description bits
https://git.kernel.org/netdev/net-next/c/8623dea207a7
- [net-next,v6,06/10] net: ti: icssg-prueth: Adjust IPG configuration for SR1.0
https://git.kernel.org/netdev/net-next/c/95c2e689331e
- [net-next,v6,07/10] net: ti: icssg-prueth: Adjust the number of TX channels for SR1.0
https://git.kernel.org/netdev/net-next/c/604e603d73ec
- [net-next,v6,08/10] net: ti: icssg-prueth: Add functions to configure SR1.0 packet classifier
https://git.kernel.org/netdev/net-next/c/0a74a9de79c1
- [net-next,v6,09/10] net: ti: icssg-prueth: Modify common functions for SR1.0
https://git.kernel.org/netdev/net-next/c/ce95cb4c8d26
- [net-next,v6,10/10] net: ti: icssg-prueth: Add ICSSG Ethernet driver for AM65x SR1.0 platforms
https://git.kernel.org/netdev/net-next/c/e654b85a693e
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 3/9] dt-bindings: rtc: lpc32xx-rtc: move to trivial-rtc
From: Javier Carrasco @ 2024-04-09 8:52 UTC (permalink / raw)
To: Krzysztof Kozlowski, Alexandre Belloni, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Jiaxun Yang,
Vladimir Zapolskiy, Joel Stanley, Andrew Jeffery, Maxime Coquelin,
Alexandre Torgue
Cc: linux-rtc, devicetree, linux-kernel, linux-arm-kernel,
linux-aspeed, linux-stm32
In-Reply-To: <dd5e9837-0dcf-4b0e-8d11-f8bed868cdf2@linaro.org>
On 4/9/24 09:34, Krzysztof Kozlowski wrote:
> On 08/04/2024 17:53, Javier Carrasco wrote:
>> This RTC requires a compatible, a reg and a single interrupt,
>> which makes it suitable for a direct conversion into trivial-rtc.
>>
>> Signed-off-by: Javier Carrasco <javier.carrasco.cruz@gmail.com>
>> ---
>> Documentation/devicetree/bindings/rtc/lpc32xx-rtc.txt | 15 ---------------
>> Documentation/devicetree/bindings/rtc/trivial-rtc.yaml | 2 ++
>> 2 files changed, 2 insertions(+), 15 deletions(-)
>
> This one no... and if you tested DTS you would see errors, although you
> need to test specific lpc config, not multi_v7.
>
> It does not look like you tested the DTS against bindings. Please run
> `make dtbs_check W=1` (see
> Documentation/devicetree/bindings/writing-schema.rst or
> https://www.linaro.org/blog/tips-and-tricks-for-validating-devicetree-sources-with-the-devicetree-schema/
> for instructions).
>
> Anyway, you *must* check all DTS before moving anything to trivial.
>
> Does it mean all other bindings were not checked against DTS at all?
>
> Best regards,
> Krzysztof
>
Hi,
I did check the conversion against nxp/lpc/lpc3250-phy3250.dts, which
throws a message about the 'clocks' property.
That property is not documented in the original binding, and even though
it could be missing, I could not find any function to get a clock (i.e.
any form of clk_get()) in rtc-lpc32xx.c, which is the only file where
the compatible can be found.
Is therefore the property not useless in the dts? My apologies if I am
missing something here.
Thanks and best regards,
Javier Carrasco
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2 4/7] powerpc: mm: accelerate pagefault when badaccess
From: Michael Ellerman @ 2024-04-09 8:56 UTC (permalink / raw)
To: Kefeng Wang, akpm
Cc: Russell King, Catalin Marinas, Will Deacon, Nicholas Piggin,
Christophe Leroy, Paul Walmsley, Palmer Dabbelt, Albert Ou,
Alexander Gordeev, Gerald Schaefer, Dave Hansen, Andy Lutomirski,
Peter Zijlstra, x86, linux-arm-kernel, linuxppc-dev, linux-riscv,
linux-s390, surenb, linux-mm, Kefeng Wang
In-Reply-To: <20240403083805.1818160-5-wangkefeng.wang@huawei.com>
Kefeng Wang <wangkefeng.wang@huawei.com> writes:
> The access_[pkey]_error() of vma already checked under per-VMA lock, if
> it is a bad access, directly handle error, no need to retry with mmap_lock
> again. In order to release the correct lock, pass the mm_struct into
> bad_access_pkey()/bad_access(), if mm is NULL, release vma lock, or
> release mmap_lock. Since the page faut is handled under per-VMA lock,
> count it as a vma lock event with VMA_LOCK_SUCCESS.
>
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> arch/powerpc/mm/fault.c | 33 ++++++++++++++++++++-------------
> 1 file changed, 20 insertions(+), 13 deletions(-)
I thought there might be a nicer way to do this, plumbing the mm and vma
down through all those levels is a bit of a pain (vma->vm_mm exists
after all).
But I couldn't come up with anything obviously better, without doing
lots of refactoring first, which would be a pain to integrate into this
series.
So anyway, if the series goes ahead:
Acked-by: Michael Ellerman <mpe@ellerman.id.au> (powerpc)
cheers
> diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
> index 53335ae21a40..215690452495 100644
> --- a/arch/powerpc/mm/fault.c
> +++ b/arch/powerpc/mm/fault.c
> @@ -71,23 +71,26 @@ static noinline int bad_area_nosemaphore(struct pt_regs *regs, unsigned long add
> return __bad_area_nosemaphore(regs, address, SEGV_MAPERR);
> }
>
> -static int __bad_area(struct pt_regs *regs, unsigned long address, int si_code)
> +static int __bad_area(struct pt_regs *regs, unsigned long address, int si_code,
> + struct mm_struct *mm, struct vm_area_struct *vma)
> {
> - struct mm_struct *mm = current->mm;
>
> /*
> * Something tried to access memory that isn't in our memory map..
> * Fix it, but check if it's kernel or user first..
> */
> - mmap_read_unlock(mm);
> + if (mm)
> + mmap_read_unlock(mm);
> + else
> + vma_end_read(vma);
>
> return __bad_area_nosemaphore(regs, address, si_code);
> }
>
> static noinline int bad_access_pkey(struct pt_regs *regs, unsigned long address,
> + struct mm_struct *mm,
> struct vm_area_struct *vma)
> {
> - struct mm_struct *mm = current->mm;
> int pkey;
>
> /*
> @@ -109,7 +112,10 @@ static noinline int bad_access_pkey(struct pt_regs *regs, unsigned long address,
> */
> pkey = vma_pkey(vma);
>
> - mmap_read_unlock(mm);
> + if (mm)
> + mmap_read_unlock(mm);
> + else
> + vma_end_read(vma);
>
> /*
> * If we are in kernel mode, bail out with a SEGV, this will
> @@ -124,9 +130,10 @@ static noinline int bad_access_pkey(struct pt_regs *regs, unsigned long address,
> return 0;
> }
>
> -static noinline int bad_access(struct pt_regs *regs, unsigned long address)
> +static noinline int bad_access(struct pt_regs *regs, unsigned long address,
> + struct mm_struct *mm, struct vm_area_struct *vma)
> {
> - return __bad_area(regs, address, SEGV_ACCERR);
> + return __bad_area(regs, address, SEGV_ACCERR, mm, vma);
> }
>
> static int do_sigbus(struct pt_regs *regs, unsigned long address,
> @@ -479,13 +486,13 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>
> if (unlikely(access_pkey_error(is_write, is_exec,
> (error_code & DSISR_KEYFAULT), vma))) {
> - vma_end_read(vma);
> - goto lock_mmap;
> + count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
> + return bad_access_pkey(regs, address, NULL, vma);
> }
>
> if (unlikely(access_error(is_write, is_exec, vma))) {
> - vma_end_read(vma);
> - goto lock_mmap;
> + count_vm_vma_lock_event(VMA_LOCK_SUCCESS);
> + return bad_access(regs, address, NULL, vma);
> }
>
> fault = handle_mm_fault(vma, address, flags | FAULT_FLAG_VMA_LOCK, regs);
> @@ -521,10 +528,10 @@ static int ___do_page_fault(struct pt_regs *regs, unsigned long address,
>
> if (unlikely(access_pkey_error(is_write, is_exec,
> (error_code & DSISR_KEYFAULT), vma)))
> - return bad_access_pkey(regs, address, vma);
> + return bad_access_pkey(regs, address, mm, vma);
>
> if (unlikely(access_error(is_write, is_exec, vma)))
> - return bad_access(regs, address);
> + return bad_access(regs, address, mm, vma);
>
> /*
> * If for any reason at all we couldn't handle the fault,
> --
> 2.27.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox