Re: [PATCH linux-next 3/3] powerpc/kdump: Split KEXEC_CORE and CRASH_DUMP dependency
Hello Hari,
Build failure detected.
On 13/02/24 17:01, Hari Bathini wrote:
Remove CONFIG_CRASH_DUMP dependency on CONFIG_KEXEC. CONFIG_KEXEC_CORE
was used at places where CONFIG_CRASH_DUMP or CONFIG_CRASH_RESERVE was
appropriate. Replace with appropriate #ifdefs to support CONFIG_KEXEC
and !CONFIG_CRASH_DUMP configuration option. Also, make CONFIG_FA_DUMP
dependent on CONFIG_CRASH_DUMP to avoid unmet dependencies for FA_DUMP
with !CONFIG_KEXEC_CORE configuration option.
Signed-off-by: Hari Bathini
---
arch/powerpc/Kconfig | 9 +--
arch/powerpc/include/asm/kexec.h | 98 +++---
arch/powerpc/kernel/prom.c | 2 +-
arch/powerpc/kernel/setup-common.c | 2 +-
arch/powerpc/kernel/smp.c | 4 +-
arch/powerpc/kexec/Makefile| 3 +-
arch/powerpc/kexec/core.c | 4 ++
7 files changed, 60 insertions(+), 62 deletions(-)
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 5cf8ad8d7e8e..e377deefa2dc 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -607,11 +607,6 @@ config PPC64_SUPPORTS_MEMORY_FAILURE
config ARCH_SUPPORTS_KEXEC
def_bool PPC_BOOK3S || PPC_E500 || (44x && !SMP)
-config ARCH_SELECTS_KEXEC
- def_bool y
- depends on KEXEC
- select CRASH_DUMP
-
config ARCH_SUPPORTS_KEXEC_FILE
def_bool PPC64
@@ -622,7 +617,6 @@ config ARCH_SELECTS_KEXEC_FILE
def_bool y
depends on KEXEC_FILE
select KEXEC_ELF
- select CRASH_DUMP
select HAVE_IMA_KEXEC if IMA
config PPC64_BIG_ENDIAN_ELF_ABI_V2
@@ -694,8 +688,7 @@ config ARCH_SELECTS_CRASH_DUMP
config FA_DUMP
bool "Firmware-assisted dump"
- depends on PPC64 && (PPC_RTAS || PPC_POWERNV)
- select CRASH_DUMP
+ depends on CRASH_DUMP && PPC64 && (PPC_RTAS || PPC_POWERNV)
help
A robust mechanism to get reliable kernel crash dump with
assistance from firmware. This approach does not use kexec,
diff --git a/arch/powerpc/include/asm/kexec.h b/arch/powerpc/include/asm/kexec.h
index e1b43aa12175..fdb90e24dc74 100644
--- a/arch/powerpc/include/asm/kexec.h
+++ b/arch/powerpc/include/asm/kexec.h
@@ -55,59 +55,18 @@
typedef void (*crash_shutdown_t)(void);
#ifdef CONFIG_KEXEC_CORE
-
-/*
- * This function is responsible for capturing register states if coming
- * via panic or invoking dump using sysrq-trigger.
- */
-static inline void crash_setup_regs(struct pt_regs *newregs,
- struct pt_regs *oldregs)
-{
- if (oldregs)
- memcpy(newregs, oldregs, sizeof(*newregs));
- else
- ppc_save_regs(newregs);
-}
+struct kimage;
+struct pt_regs;
extern void kexec_smp_wait(void); /* get and clear naca physid, wait for
master to copy new code to 0 */
-extern int crashing_cpu;
-extern void crash_send_ipi(void (*crash_ipi_callback)(struct pt_regs *));
-extern void crash_ipi_callback(struct pt_regs *);
-extern int crash_wake_offline;
-
-struct kimage;
-struct pt_regs;
extern void default_machine_kexec(struct kimage *image);
-extern void default_machine_crash_shutdown(struct pt_regs *regs);
-extern int crash_shutdown_register(crash_shutdown_t handler);
-extern int crash_shutdown_unregister(crash_shutdown_t handler);
-
-extern void crash_kexec_prepare(void);
-extern void crash_kexec_secondary(struct pt_regs *regs);
-int __init overlaps_crashkernel(unsigned long start, unsigned long size);
-extern void reserve_crashkernel(void);
extern void machine_kexec_mask_interrupts(void);
-static inline bool kdump_in_progress(void)
-{
- return crashing_cpu >= 0;
-}
-
void relocate_new_kernel(unsigned long indirection_page, unsigned long
reboot_code_buffer,
unsigned long start_address) __noreturn;
-
void kexec_copy_flush(struct kimage *image);
-#if defined(CONFIG_CRASH_DUMP)
-bool is_kdump_kernel(void);
-#define is_kdump_kernelis_kdump_kernel
-#if defined(CONFIG_PPC_RTAS)
-void crash_free_reserved_phys_range(unsigned long begin, unsigned long end);
-#define crash_free_reserved_phys_range crash_free_reserved_phys_range
-#endif /* CONFIG_PPC_RTAS */
-#endif /* CONFIG_CRASH_DUMP */
-
#ifdef CONFIG_KEXEC_FILE
extern const struct kexec_file_ops kexec_elf64_ops;
@@ -152,15 +111,56 @@ int setup_new_fdt_ppc64(const struct kimage *image, void *fdt,
#endif /* CONFIG_KEXEC_FILE */
-#else /* !CONFIG_KEXEC_CORE */
-static inline void crash_kexec_secondary(struct pt_regs *regs) { }
+#endif /* CONFIG_KEXEC_CORE */
+
+#ifdef CONFIG_CRASH_RESERVE
+int __init overlaps_crashkernel(unsigned long start, unsigned long size);
+extern void reserve_crashkernel(void);
+#else
+static inline void reserve_crashkernel(void) {}
+static inline int overlaps_crashkernel(unsigned long start, unsigned long
size) { return 0; }
+#endif
-static inline int overlaps_crashkernel(unsigned
Re: [PATCHv9 2/3] irq: use a struct for the kstat_irqs in the interrupt descriptor
On Fri, Feb 23 2024 at 15:18, Bitao Hu wrote:
> On 2024/2/22 21:22, Thomas Gleixner wrote:
>>> - if (desc->kstat_irqs) {
>>> - for_each_online_cpu(j)
>>> - any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs,
>>> j));
>>> - }
>>> + if (desc->kstat_irqs)
>>> + any_count = data_race(desc->tot_count);
>>
>> This is an unrelated change and needs to be split out into a separate
>> patch with a proper changelog which explains why this is equivalent.
>>
>
> Alright, I will remove this change witch is not related to the purpose
> of this patch.
>
> I guess that the purpose of suggesting this change in your V1 response
> was to speedup the 'show_interrupts'. However, after reviewing the
> usage of 'desc->tot_count' in 'unsigned int kstat_irqs(unsigned int
> irq)', I think the change might be as follows:
>
> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
> index 623b8136e9af..53b8d6edd7ac 100644
> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -489,8 +489,13 @@ int show_interrupts(struct seq_file *p, void *v)
> goto outsparse;
>
> if (desc->kstat_irqs) {
> - for_each_online_cpu(j)
> - any_count |=
> data_race(per_cpu(desc->kstat_irqs->cnt, j));
> + if (!irq_settings_is_per_cpu_devid(desc) &&
> + !irq_settings_is_per_cpu(desc) &&
> + !irq_is_nmi(desc))
> + any_count = data_race(desc->tot_count);
> + else
> + for_each_online_cpu(j)
> + any_count |=
> data_race(per_cpu(desc->kstat_irqs->cnt, j));
> }
>
> if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
>
> Is my idea correct?
Yes.
Re: [PATCHv9 2/3] irq: use a struct for the kstat_irqs in the interrupt descriptor
Hi,
On 2024/2/22 21:22, Thomas Gleixner wrote:
On Thu, Feb 22 2024 at 17:34, Bitao Hu wrote:
First of all the subsystem prefix is 'genirq:'. 'git log kernel/irq/'
gives you a pretty good hint. It's documented
Secondly the subject line does not match what this patch is about. It's
not about using a struct, it's about providing a snapshot mechanism, no?
The current implementation uses an int for the kstat_irqs in the
interrupt descriptor.
However, we need to know the number of interrupts which happened
since softlockup detection took a snapshot in order to analyze
the problem caused by an interrupt storm.
Replacing an int with a struct and providing sensible interfaces
for the watchdog code can keep it self contained to the interrupt
core code.
So something like this makes a useful change log for this:
Subject: genirq: Provide a snapshot mechanism for interrupt statistics
The soft lockup detector lacks a mechanism to identify interrupt storms
as root cause of a lockup. To enable this the detector needs a
mechanism to snapshot the interrupt count statistics on a CPU when the
detector observes a potential lockup scenario and compare that against
the interrupt count when it warns about the lockup later on. The number
of interrupts in that period give a hint whether the lockup might be
caused by an interrupt storm.
Instead of having extra storage in the lockup detector and accessing
the internals of the interrupt descriptor directly, convert the per CPU
irq_desc::kstat_irq member to a data structure which contains the
counter plus a snapshot member and provide interfaces to take a
snapshot of all interrupts on the current CPU and to retrieve the delta
of a specific interrupt later on.
Thanks, the changelog you wrote very clearly articulates the purpose of
this patch.
Hmm?
Signed-off-by: Bitao Hu
Interesting. You fully authored the patch?
That's not how it works. You cannot take work from others and claim that
it is yours. The minimal courtesy is to add a 'Originally-by:' tag.
I'm very sorry, the majority of this patch is your work, I will add an
'Originally-by:' tag.
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 623b8136e9af..3ad40cf30c66 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -488,18 +488,15 @@ int show_interrupts(struct seq_file *p, void *v)
if (!desc || irq_settings_is_hidden(desc))
goto outsparse;
- if (desc->kstat_irqs) {
- for_each_online_cpu(j)
- any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs,
j));
- }
+ if (desc->kstat_irqs)
+ any_count = data_race(desc->tot_count);
This is an unrelated change and needs to be split out into a separate
patch with a proper changelog which explains why this is equivalent.
Alright, I will remove this change witch is not related to the purpose
of this patch.
I guess that the purpose of suggesting this change in your V1 response
was to speedup the 'show_interrupts'. However, after reviewing the
usage of 'desc->tot_count' in 'unsigned int kstat_irqs(unsigned int
irq)', I think the change might be as follows:
diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
index 623b8136e9af..53b8d6edd7ac 100644
--- a/kernel/irq/proc.c
+++ b/kernel/irq/proc.c
@@ -489,8 +489,13 @@ int show_interrupts(struct seq_file *p, void *v)
goto outsparse;
if (desc->kstat_irqs) {
- for_each_online_cpu(j)
- any_count |=
data_race(per_cpu(desc->kstat_irqs->cnt, j));
+ if (!irq_settings_is_per_cpu_devid(desc) &&
+ !irq_settings_is_per_cpu(desc) &&
+ !irq_is_nmi(desc))
+ any_count = data_race(desc->tot_count);
+ else
+ for_each_online_cpu(j)
+ any_count |=
data_race(per_cpu(desc->kstat_irqs->cnt, j));
}
if ((!desc->action || irq_desc_is_chained(desc)) && !any_count)
Is my idea correct?
Best Regards,
Bitao
Re: [kvm-unit-tests PATCH v5 0/8] Multi-migration support
On 21/02/2024 04.27, Nicholas Piggin wrote: Now that strange arm64 hang is found to be QEMU bug, I'll repost. Since arm64 requires Thomas's uart patch and it is worse affected by the QEMU bug, I will just not build it on arm. The QEMU bug still affects powerpc (and presumably s390x) but it's not causing so much trouble for this test case. I have another test case that can hit it reliably and doesn't cause crashes but that takes some harness and common lib work so I'll send that another time. Since v4: - Don't build selftest-migration on arm. - Reduce selftest-migration iterations from 100 to 30 to make the test run faster (it's ~0.5s per migration). Thanks, I think the series is ready to go now ... we just have to wait for your QEMU TCG migration fix to get merged first. Or should we maybe mark the selftest-migration with "accel = kvm" for now and remove that line later once QEMU has been fixed? Thomas
Re: "test_ip_fast_csum: ASSERTION FAILED at lib/checksum_kunit.c:589" at boot with CONFIG_CHECKSUM_KUNIT=y enabled on a Talos II, kernel 6.8-rc5
Le 23/02/2024 à 07:12, Charlie Jenkins a écrit :
> On Fri, Feb 23, 2024 at 05:59:07AM +, Christophe Leroy wrote:
>> Hi Erhard, hi Charlie,
>>
>> Le 23/02/2024 à 02:26, Erhard Furtner a écrit :
>>> Greetings!
>>>
>>> Looks like my Talos II (running a BE kernel+system) fails some of the
>>> kernels internal unit tests. One of the failing tests is checksum_kunit,
>>> enabled via CONFIG_CHECKSUM_KUNIT=y:
>>>
>>> [...]
>>> KTAP version 1
>>> # Subtest: checksum
>>> # module: checksum_kunit
>>> 1..5
>>> entry-flush: disabled on command line.
>>> ok 1 test_csum_fixed_random_inputs
>>> ok 2 test_csum_all_carry_inputs
>>> ok 3 test_csum_no_carry_inputs
>>> # test_ip_fast_csum: ASSERTION FAILED at lib/checksum_kunit.c:589
>>> Expected ( u64)expected == ( u64)csum_result, but
>>> ( u64)expected == 55939 (0xda83)
>>> ( u64)csum_result == 33754 (0x83da)
>>> not ok 4 test_ip_fast_csum
>>> # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:617
>>> Expected ( u64)expected_csum_ipv6_magic[i] == (
>>> u64)csum_ipv6_magic(saddr, daddr, len, proto, csum), but
>>> ( u64)expected_csum_ipv6_magic[i] == 6356 (0x18d4)
>>> ( u64)csum_ipv6_magic(saddr, daddr, len, proto, csum) == 43586
>>> (0xaa42)
>>> not ok 5 test_csum_ipv6_magic
>>> # checksum: pass:3 fail:2 skip:0 total:5
>>> # Totals: pass:3 fail:2 skip:0 total:5
>>> not ok 4 checksum
>>> [...]
>>>
>>> Full dmesg + kernel .config attached.
>>
>> Looks like the same problem as the one I fixed with commit b38460bc463c
>> ("kunit: Fix checksum tests on big endian CPUs")
>>
>> The new tests implemented through commit 6f4c45cbcb00 ("kunit: Add tests
>> for csum_ipv6_magic and ip_fast_csum") create a lot of type issues as
>> reported by sparse when built with C=2 (see below).
>>
>> Once those issues are fixed, it should work.
>>
>> Charlie, can you provide a fix ?
>>
>> Thanks,
>> Christophe
>
> The "lib: checksum: Fix issues with checksum tests" patch should fix all of
> these issues [1].
>
> [1]
> https://lore.kernel.org/all/20240221-fix_sparse_errors_checksum_tests-v9-1-bff4d73ab...@rivosinc.com/T/#m189783a9b2a7d12e3c34c4a412e65408658db2c9
It doesn't fix the issues, I still get the following with your patch 1/2
applied:
[6.893141] KTAP version 1
[6.896118] 1..1
[6.897764] KTAP version 1
[6.900800] # Subtest: checksum
[6.904518] # module: checksum_kunit
[6.904601] 1..5
[7.139784] ok 1 test_csum_fixed_random_inputs
[7.590056] ok 2 test_csum_all_carry_inputs
[8.064415] ok 3 test_csum_no_carry_inputs
[8.070065] # test_ip_fast_csum: ASSERTION FAILED at
lib/checksum_kunit.c:589
[8.070065] Expected ( u64)expected == ( u64)csum_result, but
[8.070065] ( u64)expected == 55939 (0xda83)
[8.070065] ( u64)csum_result == 33754 (0x83da)
[8.075836] not ok 4 test_ip_fast_csum
[8.101039] # test_csum_ipv6_magic: ASSERTION FAILED at
lib/checksum_kunit.c:617
[8.101039] Expected ( u64)( __sum16)expected_csum_ipv6_magic[i]
== ( u64)csum_ipv6_magic(saddr, daddr, len, proto, ( __wsum)csum), but
[8.101039] ( u64)( __sum16)expected_csum_ipv6_magic[i] ==
6356 (0x18d4)
[8.101039] ( u64)csum_ipv6_magic(saddr, daddr, len, proto, (
__wsum)csum) == 43586 (0xaa42)
[8.106446] not ok 5 test_csum_ipv6_magic
[8.143829] # checksum: pass:3 fail:2 skip:0 total:5
[8.148334] # Totals: pass:3 fail:2 skip:0 total:5
[8.153173] not ok 1 checksum
All your patch does is to hide the sparse warnings. But forcing a cast
doesn't fix byte orders.
Please have a look at commit b38460bc463c ("kunit: Fix checksum tests on
big endian CPUs"), there are helpers to put checksums in the correct
byte order.
Christophe
Re: [PATCH 1/2] powerpc: Refactor __kernel_map_pages()
Christophe Leroy writes:
> Le 22/02/2024 à 06:32, Michael Ellerman a écrit :
>> Christophe Leroy writes:
>>> __kernel_map_pages() is almost identical for PPC32 and RADIX.
>>>
>>> Refactor it.
>>>
>>> On PPC32 it is not needed for KFENCE, but to keep it simple
>>> just make it similar to PPC64.
>>>
>>> Signed-off-by: Christophe Leroy
>>> ---
>>> arch/powerpc/include/asm/book3s/64/pgtable.h | 10 --
>>> arch/powerpc/include/asm/book3s/64/radix.h | 2 --
>>> arch/powerpc/mm/book3s64/radix_pgtable.c | 14 --
>>> arch/powerpc/mm/pageattr.c | 19 +++
>>> arch/powerpc/mm/pgtable_32.c | 15 ---
>>> 5 files changed, 19 insertions(+), 41 deletions(-)
>>>
>>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>>> index 421db7c4f2a4..16b8d20d6ca8 100644
>>> --- a/arch/powerpc/mm/pageattr.c
>>> +++ b/arch/powerpc/mm/pageattr.c
>>> @@ -101,3 +101,22 @@ int change_memory_attr(unsigned long addr, int
>>> numpages, long action)
>>> return apply_to_existing_page_range(_mm, start, size,
>>> change_page_attr, (void *)action);
>>> }
>>> +
>>> +#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
>>> +#ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>>> +void __kernel_map_pages(struct page *page, int numpages, int enable)
>>> +{
>>> + unsigned long addr = (unsigned long)page_address(page);
>>> +
>>> + if (PageHighMem(page))
>>> + return;
>>> +
>>> + if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>>> + hash__kernel_map_pages(page, numpages, enable);
>>> + else if (enable)
>>> + set_memory_p(addr, numpages);
>>> + else
>>> + set_memory_np(addr, numpages);
>>> +}
>>
>> This doesn't build on 32-bit, eg. ppc32_allmodconfig:
>>
>> ../arch/powerpc/mm/pageattr.c: In function '__kernel_map_pages':
>> ../arch/powerpc/mm/pageattr.c:116:23: error: implicit declaration of
>> function 'hash__kernel_map_pages' [-Werror=implicit-function-declaration]
>>116 | err = hash__kernel_map_pages(page, numpages,
>> enable);
>>| ^~
>>
>> I couldn't see a nice way to get around it, so ended up with:
>>
>> void __kernel_map_pages(struct page *page, int numpages, int enable)
>> {
>> int err;
>> unsigned long addr = (unsigned long)page_address(page);
>>
>> if (PageHighMem(page))
>> return;
>>
>> #ifdef CONFIG_PPC_BOOK3S_64
>> if (!radix_enabled())
>> err = hash__kernel_map_pages(page, numpages, enable);
>> else
>> #endif
>> if (enable)
>> err = set_memory_p(addr, numpages);
>> else
>> err = set_memory_np(addr, numpages);
>>
>
>
> I missed something it seems. Not good to leave something unterminated
> when you leave for vacation and think it was finished when you come back.
>
> The best solution I see is to move hash__kernel_map_pages() prototype
> somewhere else.
> $ git grep -e hash__ -e radix__ -- arch/powerpc/include/asm/*.h
> arch/powerpc/include/asm/bug.h:void hash__do_page_fault(struct pt_regs *);
> arch/powerpc/include/asm/mmu.h:extern void radix__mmu_cleanup_all(void);
> arch/powerpc/include/asm/mmu_context.h:extern void
> radix__switch_mmu_context(struct mm_struct *prev,
> arch/powerpc/include/asm/mmu_context.h: return
> radix__switch_mmu_context(prev, next);
> arch/powerpc/include/asm/mmu_context.h:extern int
> hash__alloc_context_id(void);
> arch/powerpc/include/asm/mmu_context.h:void __init
> hash__reserve_context_id(int id);
> arch/powerpc/include/asm/mmu_context.h: context_id = hash__alloc_context_id();
> arch/powerpc/include/asm/mmu_context.h: * radix__flush_all_mm() to determine
> the scope (local/global)
> arch/powerpc/include/asm/mmu_context.h: radix__flush_all_mm(mm);
If anything I'd prefer to move those out of there into the book3s/64/
headers :)
> Maybe asm/mmu.h ?
>
> Or mm/mmu_decl.h ?
Yeah I'll do that. It's a bit of a dumping ground, but at least it's
internal to arch code, not exported to the rest of the kernel.
cheers
Re: [PATCH] powerpc/rtas: use correct function name for resetting TCE tables
Nathan Lynch via B4 Relay
writes:
> From: Nathan Lynch
>
> The PAPR spec spells the function name as
>
> "ibm,reset-pe-dma-windows"
>
> but in practice firmware uses the singular form:
Just to be clear, you're talking about IBM firmware on PowerVM machines.
> "ibm,reset-pe-dma-window"
>
> in the device tree. Since we have the wrong spelling in the RTAS
> function table, reverse lookups (token -> name) fail and warn:
>
> unexpected failed lookup for token 86
> WARNING: CPU: 1 PID: 545 at arch/powerpc/kernel/rtas.c:659
> __do_enter_rtas_trace+0x2a4/0x2b4
...
> diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
> index 7e793b503e29..8064d9c3de86 100644
> --- a/arch/powerpc/kernel/rtas.c
> +++ b/arch/powerpc/kernel/rtas.c
> @@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[]
> __ro_after_init = {
> [RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = {
> .name = "ibm,remove-pe-dma-window",
> },
> - [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = {
> - .name = "ibm,reset-pe-dma-windows",
> + [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = {
> + /*
> + * Note: PAPR+ v2.13 7.3.31.4.1 spells this as
> + * "ibm,reset-pe-dma-windows" (plural), but RTAS
> + * implementations use the singular form in practice.
> + */
> + .name = "ibm,reset-pe-dma-window",
Qemu also spells it that way:
$ grep -C 12 ibm,reset-pe-dma-window hw/ppc/spapr_rtas_ddw.c
static void spapr_rtas_ddw_init(void)
{
spapr_rtas_register(RTAS_IBM_QUERY_PE_DMA_WINDOW,
"ibm,query-pe-dma-window",
rtas_ibm_query_pe_dma_window);
spapr_rtas_register(RTAS_IBM_CREATE_PE_DMA_WINDOW,
"ibm,create-pe-dma-window",
rtas_ibm_create_pe_dma_window);
spapr_rtas_register(RTAS_IBM_REMOVE_PE_DMA_WINDOW,
"ibm,remove-pe-dma-window",
rtas_ibm_remove_pe_dma_window);
spapr_rtas_register(RTAS_IBM_RESET_PE_DMA_WINDOW,
"ibm,reset-pe-dma-window",
rtas_ibm_reset_pe_dma_window);
}
There's no version in SLOF, it delegates to Qemu.
The old platforms that use RTAS won't implement this call at all, so
there's no issue with the naming there.
So LGTM.
cheers
Re: "test_ip_fast_csum: ASSERTION FAILED at lib/checksum_kunit.c:589" at boot with CONFIG_CHECKSUM_KUNIT=y enabled on a Talos II, kernel 6.8-rc5
Hi Erhard, hi Charlie,
Le 23/02/2024 à 02:26, Erhard Furtner a écrit :
> Greetings!
>
> Looks like my Talos II (running a BE kernel+system) fails some of the kernels
> internal unit tests. One of the failing tests is checksum_kunit, enabled via
> CONFIG_CHECKSUM_KUNIT=y:
>
> [...]
> KTAP version 1
> # Subtest: checksum
> # module: checksum_kunit
> 1..5
> entry-flush: disabled on command line.
> ok 1 test_csum_fixed_random_inputs
> ok 2 test_csum_all_carry_inputs
> ok 3 test_csum_no_carry_inputs
> # test_ip_fast_csum: ASSERTION FAILED at lib/checksum_kunit.c:589
> Expected ( u64)expected == ( u64)csum_result, but
> ( u64)expected == 55939 (0xda83)
> ( u64)csum_result == 33754 (0x83da)
> not ok 4 test_ip_fast_csum
> # test_csum_ipv6_magic: ASSERTION FAILED at lib/checksum_kunit.c:617
> Expected ( u64)expected_csum_ipv6_magic[i] == (
> u64)csum_ipv6_magic(saddr, daddr, len, proto, csum), but
> ( u64)expected_csum_ipv6_magic[i] == 6356 (0x18d4)
> ( u64)csum_ipv6_magic(saddr, daddr, len, proto, csum) == 43586
> (0xaa42)
> not ok 5 test_csum_ipv6_magic
> # checksum: pass:3 fail:2 skip:0 total:5
> # Totals: pass:3 fail:2 skip:0 total:5
> not ok 4 checksum
> [...]
>
> Full dmesg + kernel .config attached.
Looks like the same problem as the one I fixed with commit b38460bc463c
("kunit: Fix checksum tests on big endian CPUs")
The new tests implemented through commit 6f4c45cbcb00 ("kunit: Add tests
for csum_ipv6_magic and ip_fast_csum") create a lot of type issues as
reported by sparse when built with C=2 (see below).
Once those issues are fixed, it should work.
Charlie, can you provide a fix ?
Thanks,
Christophe
CC lib/checksum_kunit.o
CHECK lib/checksum_kunit.c
lib/checksum_kunit.c:219:9: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:9:expected restricted __sum16
lib/checksum_kunit.c:219:9:got int
lib/checksum_kunit.c:219:17: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:17:expected restricted __sum16
lib/checksum_kunit.c:219:17:got int
lib/checksum_kunit.c:219:25: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:25:expected restricted __sum16
lib/checksum_kunit.c:219:25:got int
lib/checksum_kunit.c:219:33: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:33:expected restricted __sum16
lib/checksum_kunit.c:219:33:got int
lib/checksum_kunit.c:219:41: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:41:expected restricted __sum16
lib/checksum_kunit.c:219:41:got int
lib/checksum_kunit.c:219:49: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:49:expected restricted __sum16
lib/checksum_kunit.c:219:49:got int
lib/checksum_kunit.c:219:57: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:57:expected restricted __sum16
lib/checksum_kunit.c:219:57:got int
lib/checksum_kunit.c:219:65: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:65:expected restricted __sum16
lib/checksum_kunit.c:219:65:got int
lib/checksum_kunit.c:219:73: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:219:73:expected restricted __sum16
lib/checksum_kunit.c:219:73:got int
lib/checksum_kunit.c:220:9: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:9:expected restricted __sum16
lib/checksum_kunit.c:220:9:got int
lib/checksum_kunit.c:220:17: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:17:expected restricted __sum16
lib/checksum_kunit.c:220:17:got int
lib/checksum_kunit.c:220:25: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:25:expected restricted __sum16
lib/checksum_kunit.c:220:25:got int
lib/checksum_kunit.c:220:33: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:33:expected restricted __sum16
lib/checksum_kunit.c:220:33:got int
lib/checksum_kunit.c:220:41: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:41:expected restricted __sum16
lib/checksum_kunit.c:220:41:got int
lib/checksum_kunit.c:220:49: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:49:expected restricted __sum16
lib/checksum_kunit.c:220:49:got int
lib/checksum_kunit.c:220:57: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:57:expected restricted __sum16
lib/checksum_kunit.c:220:57:got int
lib/checksum_kunit.c:220:65: warning: incorrect type in initializer
(different base types)
lib/checksum_kunit.c:220:65:
Re: [PATCH v2 00/14] Split crash out from kexec and clean up related config items
On 23/02/24 2:59 am, Andrew Morton wrote: On Thu, 22 Feb 2024 10:47:29 +0530 Hari Bathini wrote: On 22/02/24 2:27 am, Andrew Morton wrote: On Wed, 21 Feb 2024 11:15:00 +0530 Hari Bathini wrote: On 04/02/24 8:56 am, Baoquan He wrote: Hope Hari and Pingfan can help have a look, see if it's doable. Now, I make it either have both kexec and crash enabled, or disable both of them altogether. Sure. I will take a closer look... Thanks a lot. Please feel free to post patches to make that, or I can do it with your support or suggestion. Tested your changes and on top of these changes, came up with the below changes to get it working for powerpc: https://lore.kernel.org/all/20240213113150.1148276-1-hbath...@linux.ibm.com/ So can we take it that you're OK with Baoquan's series as-is? Hi Andrew, If you mean v3 (https://lore.kernel.org/all/20240124051254.67105-1-...@redhat.com/) + follow-up from Baoquan (https://lore.kernel.org/all/Zb8D1ASrgX0qVm9z@MiWiFi-R3L-srv/) Yes. Can I add your Acked-by: and/or Tested-by: to the patches in this series? Sure, Andrew. Acked-by: Hari Bathini for.. Patches 1-5 & 8 in: https://lore.kernel.org/all/20240124051254.67105-1-...@redhat.com/ and this follow-up patch: https://lore.kernel.org/all/Zb8D1ASrgX0qVm9z@MiWiFi-R3L-srv/ Thanks Hari
Re: [RESEND2 PATCH net v4 2/2] soc: fsl: qbman: Use raw spinlock for cgr_lock
Le 22/02/2024 à 18:07, Sean Anderson a écrit :
> [Vous ne recevez pas souvent de courriers de sean.ander...@linux.dev.
> Découvrez pourquoi ceci est important à
> https://aka.ms/LearnAboutSenderIdentification ]
>
> cgr_lock may be locked with interrupts already disabled by
> smp_call_function_single. As such, we must use a raw spinlock to avoid
> problems on PREEMPT_RT kernels. Although this bug has existed for a
> while, it was not apparent until commit ef2a8d5478b9 ("net: dpaa: Adjust
> queue depth on rate change") which invokes smp_call_function_single via
> qman_update_cgr_safe every time a link goes up or down.
Why a raw spinlock to avoid problems on PREEMPT_RT, can you elaborate ?
If the problem is that interrupts are already disabled, shouldn't you
just change the spin_lock_irq() by spin_lock_irqsave() ?
Christophe
>
> Fixes: 96f413f47677 ("soc/fsl/qbman: fix issue in qman_delete_cgr_safe()")
> CC: sta...@vger.kernel.org
> Reported-by: Vladimir Oltean
> Closes: https://lore.kernel.org/all/20230323153935.nofnjucqjqnz34ej@skbuf/
> Reported-by: Steffen Trumtrar
> Closes:
> https://lore.kernel.org/linux-arm-kernel/87wmsyvclu@pengutronix.de/
> Signed-off-by: Sean Anderson
> Reviewed-by: Camelia Groza
> Tested-by: Vladimir Oltean
>
> ---
>
> Changes in v4:
> - Add a note about how raw spinlocks aren't quite right
>
> Changes in v3:
> - Change blamed commit to something more appropriate
>
> drivers/soc/fsl/qbman/qman.c | 25 ++---
> 1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
> index 1bf1f1ea67f0..7e9074519ad2 100644
> --- a/drivers/soc/fsl/qbman/qman.c
> +++ b/drivers/soc/fsl/qbman/qman.c
> @@ -991,7 +991,7 @@ struct qman_portal {
> /* linked-list of CSCN handlers. */
> struct list_head cgr_cbs;
> /* list lock */
> - spinlock_t cgr_lock;
> + raw_spinlock_t cgr_lock;
> struct work_struct congestion_work;
> struct work_struct mr_work;
> char irqname[MAX_IRQNAME];
> @@ -1281,7 +1281,7 @@ static int qman_create_portal(struct qman_portal
> *portal,
> /* if the given mask is NULL, assume all CGRs can be seen */
> qman_cgrs_fill(>cgrs[0]);
> INIT_LIST_HEAD(>cgr_cbs);
> - spin_lock_init(>cgr_lock);
> + raw_spin_lock_init(>cgr_lock);
> INIT_WORK(>congestion_work, qm_congestion_task);
> INIT_WORK(>mr_work, qm_mr_process_task);
> portal->bits = 0;
> @@ -1456,11 +1456,14 @@ static void qm_congestion_task(struct work_struct
> *work)
> union qm_mc_result *mcr;
> struct qman_cgr *cgr;
>
> - spin_lock_irq(>cgr_lock);
> + /*
> +* FIXME: QM_MCR_TIMEOUT is 10ms, which is too long for a raw
> spinlock!
> +*/
> + raw_spin_lock_irq(>cgr_lock);
> qm_mc_start(>p);
> qm_mc_commit(>p, QM_MCC_VERB_QUERYCONGESTION);
> if (!qm_mc_result_timeout(>p, )) {
> - spin_unlock_irq(>cgr_lock);
> + raw_spin_unlock_irq(>cgr_lock);
> dev_crit(p->config->dev, "QUERYCONGESTION timeout\n");
> qman_p_irqsource_add(p, QM_PIRQ_CSCI);
> return;
> @@ -1476,7 +1479,7 @@ static void qm_congestion_task(struct work_struct *work)
> list_for_each_entry(cgr, >cgr_cbs, node)
> if (cgr->cb && qman_cgrs_get(, cgr->cgrid))
> cgr->cb(p, cgr, qman_cgrs_get(, cgr->cgrid));
> - spin_unlock_irq(>cgr_lock);
> + raw_spin_unlock_irq(>cgr_lock);
> qman_p_irqsource_add(p, QM_PIRQ_CSCI);
> }
>
> @@ -2440,7 +2443,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
> preempt_enable();
>
> cgr->chan = p->config->channel;
> - spin_lock_irq(>cgr_lock);
> + raw_spin_lock_irq(>cgr_lock);
>
> if (opts) {
> struct qm_mcc_initcgr local_opts = *opts;
> @@ -2477,7 +2480,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
> qman_cgrs_get(>cgrs[1], cgr->cgrid))
> cgr->cb(p, cgr, 1);
> out:
> - spin_unlock_irq(>cgr_lock);
> + raw_spin_unlock_irq(>cgr_lock);
> put_affine_portal();
> return ret;
> }
> @@ -2512,7 +2515,7 @@ int qman_delete_cgr(struct qman_cgr *cgr)
> return -EINVAL;
>
> memset(_opts, 0, sizeof(struct qm_mcc_initcgr));
> - spin_lock_irqsave(>cgr_lock, irqflags);
> + raw_spin_lock_irqsave(>cgr_lock, irqflags);
> list_del(>node);
> /*
> * If there are no other CGR objects for this CGRID in the list,
> @@ -2537,7 +2540,7 @@ int qman_delete_cgr(struct qman_cgr *cgr)
> /* add back to the list */
> list_add(>node, >cgr_cbs);
> release_lock:
> - spin_unlock_irqrestore(>cgr_lock, irqflags);
> + raw_spin_unlock_irqrestore(>cgr_lock,
Re: BUG: KASAN: vmalloc-out-of-bounds in memset32 (bpf_prog_pack_free)
On Wed, 2024-01-31 at 11:46 +, Christophe Leroy wrote: > Hi, > > Got the following BUG while loading module test_bpf.ko > > No time to investigate for now. > > root@vgoip:~# insmod test_bpf.ko > [ 263.409030] > == > [ 263.416415] BUG: KASAN: vmalloc-out-of-bounds in > memset32+0x5c/0xa0 > [ 263.422952] Write of size 4 at addr c9000e40 by task kworker/0:0/7 > [ 263.429322] > [ 263.430816] CPU: 0 PID: 7 Comm: kworker/0:0 Not tainted > 6.8.0-rc1-s3k-dev-02364-gc626219462a6-dirty #606 > [ 263.440580] Hardware name: MIAE 8xx 0x50 CMPC885 > [ 263.445658] Workqueue: events bpf_prog_free_deferred > [ 263.450973] Call Trace: > [ 263.453411] [c905bd00] [c0c114e8] dump_stack_lvl+0x34/0x50 > (unreliable) > [ 263.460744] [c905bd20] [c026b9d4] print_report+0x174/0x608 > [ 263.466853] [c905bd70] [c026c01c] kasan_report+0xc0/0x130 > [ 263.472788] [c905bdd0] [c0c43cb0] memset32+0x5c/0xa0 > [ 263.478198] [c905bdf0] [c0030690] patch_instructions+0x70/0x17c > [ 263.484656] [c905be30] [c00389b0] > bpf_arch_text_invalidate+0xa8/0x120 > [ 263.491638] [c905be90] [c018e63c] bpf_prog_pack_free+0xec/0x24c > [ 263.498096] [c905bec0] [c018ea34] > bpf_jit_binary_pack_free+0x3c/0x80 > [ 263.504991] [c905bee0] [c0038ae8] bpf_jit_free+0xc0/0x128 > [ 263.510925] [c905bf00] [c00790f8] process_one_work+0x310/0x6e8 > [ 263.517209] [c905bf50] [c0079b2c] worker_thread+0x65c/0x868 > [ 263.523232] [c905bfc0] [c0084b78] kthread+0x17c/0x1ac > [ 263.528817] [c905bff0] [c00181fc] start_kernel_thread+0x10/0x14 > [ 263.535279] > [ 263.536782] The buggy address belongs to the virtual mapping at > [ 263.536782] [c900, c9008000) created by: > [ 263.536782] text_area_cpu_up+0x28/0x1d4 > [ 263.551418] > [ 263.552902] The buggy address belongs to the physical page: > [ 263.560228] > [ 263.561713] Memory state around the buggy address: > [ 263.566585] c9000d00: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > f8 f8 > [ 263.573307] c9000d80: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > f8 f8 > [ 263.580027] >c9000e00: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > f8 f8 > [ 263.586677] ^ > [ 263.591370] c9000e80: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > f8 f8 > [ 263.598093] c9000f00: f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 f8 > f8 f8 > [ 263.604743] > == > > Christophe > Looks like a false positive. It's clearly in range of the poking allocation after all. KASAN apparently leaves VM_ALLOC areas as poisoned, expecting some kind of subsequent allocator to come and unpoison specific parts. Except we call map_patch_area() instead of whatever path generic code was expecting, so we never unpoison the range. The issue would be pre-existing from the beginning, and gone unnoticed because the original code path that does the patching (i.e., actually interacts with the poking page) isn't instrumented.
Re: [PATCH 2/3] arch: Remove struct fb_info from video helpers
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/x86/core]
[also build test ERROR on deller-parisc/for-next arnd-asm-generic/master
linus/master v6.8-rc5]
[cannot apply to next-20240222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/arch-Select-fbdev-helpers-with-CONFIG_VIDEO/20240222-001622
base: tip/x86/core
patch link:
https://lore.kernel.org/r/20240221161431.8245-3-tzimmermann%40suse.de
patch subject: [PATCH 2/3] arch: Remove struct fb_info from video helpers
config: x86_64-rhel-8.3
(https://download.01.org/0day-ci/archive/20240223/202402230941.jzdvhhex-...@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240223/202402230941.jzdvhhex-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202402230941.jzdvhhex-...@intel.com/
All errors (new ones prefixed by >>):
ld: vmlinux.o: in function `fbcon_select_primary':
>> drivers/video/fbdev/core/fbcon.c:2944: undefined reference to
>> `video_is_primary_device'
ld: vmlinux.o: in function `fb_io_mmap':
drivers/video/fbdev/core/fb_io_fops.c:164: undefined reference to
`pgprot_framebuffer'
vim +2944 drivers/video/fbdev/core/fbcon.c
2939
2940 #ifdef CONFIG_FRAMEBUFFER_CONSOLE_DETECT_PRIMARY
2941 static void fbcon_select_primary(struct fb_info *info)
2942 {
2943 if (!map_override && primary_device == -1 &&
> 2944 video_is_primary_device(info->device)) {
2945 int i;
2946
2947 printk(KERN_INFO "fbcon: %s (fb%i) is primary device\n",
2948 info->fix.id, info->node);
2949 primary_device = info->node;
2950
2951 for (i = first_fb_vc; i <= last_fb_vc; i++)
2952 con2fb_map_boot[i] = primary_device;
2953
2954 if (con_is_bound(_con)) {
2955 printk(KERN_INFO "fbcon: Remapping primary
device, "
2956 "fb%i, to tty %i-%i\n", info->node,
2957 first_fb_vc + 1, last_fb_vc + 1);
2958 info_idx = primary_device;
2959 }
2960 }
2961
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Re: [PATCH 3/3] arch: Rename fbdev header and source files
Hi Thomas,
kernel test robot noticed the following build errors:
[auto build test ERROR on tip/x86/core]
[also build test ERROR on deller-parisc/for-next arnd-asm-generic/master
linus/master v6.8-rc5]
[cannot apply to next-20240222]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url:
https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/arch-Select-fbdev-helpers-with-CONFIG_VIDEO/20240222-001622
base: tip/x86/core
patch link:
https://lore.kernel.org/r/20240221161431.8245-4-tzimmermann%40suse.de
patch subject: [PATCH 3/3] arch: Rename fbdev header and source files
config: um-randconfig-002-20240222
(https://download.01.org/0day-ci/archive/20240223/202402230737.e7gwpgup-...@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project
edd4aee4dd9b5b98b2576a6f783e4086173d902a)
reproduce (this is a W=1 build):
(https://download.01.org/0day-ci/archive/20240223/202402230737.e7gwpgup-...@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot
| Closes:
https://lore.kernel.org/oe-kbuild-all/202402230737.e7gwpgup-...@intel.com/
All errors (new ones prefixed by >>):
/usr/bin/ld: warning: .tmp_vmlinux.kallsyms1 has a LOAD segment with RWX
permissions
/usr/bin/ld: drivers/video/fbdev/core/fb_io_fops.o: in function `fb_io_mmap':
>> drivers/video/fbdev/core/fb_io_fops.c:164: undefined reference to
>> `pgprot_framebuffer'
clang: error: linker command failed with exit code 1 (use -v to see
invocation)
vim +164 drivers/video/fbdev/core/fb_io_fops.c
6b180f66c0dd62 Thomas Zimmermann 2023-09-27 140
33253d9e01d405 Thomas Zimmermann 2023-11-27 141 int fb_io_mmap(struct fb_info
*info, struct vm_area_struct *vma)
33253d9e01d405 Thomas Zimmermann 2023-11-27 142 {
33253d9e01d405 Thomas Zimmermann 2023-11-27 143unsigned long start =
info->fix.smem_start;
33253d9e01d405 Thomas Zimmermann 2023-11-27 144u32 len =
info->fix.smem_len;
33253d9e01d405 Thomas Zimmermann 2023-11-27 145unsigned long
mmio_pgoff = PAGE_ALIGN((start & ~PAGE_MASK) + len) >> PAGE_SHIFT;
33253d9e01d405 Thomas Zimmermann 2023-11-27 146
b3e8813773c568 Thomas Zimmermann 2023-11-27 147if (info->flags &
FBINFO_VIRTFB)
b3e8813773c568 Thomas Zimmermann 2023-11-27 148
fb_warn_once(info, "Framebuffer is not in I/O address space.");
b3e8813773c568 Thomas Zimmermann 2023-11-27 149
33253d9e01d405 Thomas Zimmermann 2023-11-27 150/*
33253d9e01d405 Thomas Zimmermann 2023-11-27 151 * This can be either
the framebuffer mapping, or if pgoff points
33253d9e01d405 Thomas Zimmermann 2023-11-27 152 * past it, the mmio
mapping.
33253d9e01d405 Thomas Zimmermann 2023-11-27 153 */
33253d9e01d405 Thomas Zimmermann 2023-11-27 154if (vma->vm_pgoff >=
mmio_pgoff) {
33253d9e01d405 Thomas Zimmermann 2023-11-27 155if
(info->var.accel_flags)
33253d9e01d405 Thomas Zimmermann 2023-11-27 156return
-EINVAL;
33253d9e01d405 Thomas Zimmermann 2023-11-27 157
33253d9e01d405 Thomas Zimmermann 2023-11-27 158vma->vm_pgoff
-= mmio_pgoff;
33253d9e01d405 Thomas Zimmermann 2023-11-27 159start =
info->fix.mmio_start;
33253d9e01d405 Thomas Zimmermann 2023-11-27 160len =
info->fix.mmio_len;
33253d9e01d405 Thomas Zimmermann 2023-11-27 161}
33253d9e01d405 Thomas Zimmermann 2023-11-27 162
33253d9e01d405 Thomas Zimmermann 2023-11-27 163vma->vm_page_prot =
vm_get_page_prot(vma->vm_flags);
33253d9e01d405 Thomas Zimmermann 2023-11-27 @164vma->vm_page_prot =
pgprot_framebuffer(vma->vm_page_prot, vma->vm_start,
33253d9e01d405 Thomas Zimmermann 2023-11-27 165
vma->vm_end, start);
33253d9e01d405 Thomas Zimmermann 2023-11-27 166
33253d9e01d405 Thomas Zimmermann 2023-11-27 167return
vm_iomap_memory(vma, start, len);
33253d9e01d405 Thomas Zimmermann 2023-11-27 168 }
33253d9e01d405 Thomas Zimmermann 2023-11-27 169 EXPORT_SYMBOL(fb_io_mmap);
33253d9e01d405 Thomas Zimmermann 2023-11-27 170
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
[PATCH] powerpc/rtas: use correct function name for resetting TCE tables
From: Nathan Lynch
The PAPR spec spells the function name as
"ibm,reset-pe-dma-windows"
but in practice firmware uses the singular form:
"ibm,reset-pe-dma-window"
in the device tree. Since we have the wrong spelling in the RTAS
function table, reverse lookups (token -> name) fail and warn:
unexpected failed lookup for token 86
WARNING: CPU: 1 PID: 545 at arch/powerpc/kernel/rtas.c:659
__do_enter_rtas_trace+0x2a4/0x2b4
CPU: 1 PID: 545 Comm: systemd-udevd Not tainted 6.8.0-rc4 #30
Hardware name: IBM,9105-22A POWER10 (raw) 0x800200 0xf06 of:IBM,FW1060.00
(NL1060_028) hv:phyp pSeries
NIP [c00417f0] __do_enter_rtas_trace+0x2a4/0x2b4
LR [c00417ec] __do_enter_rtas_trace+0x2a0/0x2b4
Call Trace:
__do_enter_rtas_trace+0x2a0/0x2b4 (unreliable)
rtas_call+0x1f8/0x3e0
enable_ddw.constprop.0+0x4d0/0xc84
dma_iommu_dma_supported+0xe8/0x24c
dma_set_mask+0x5c/0xd8
mlx5_pci_init.constprop.0+0xf0/0x46c [mlx5_core]
probe_one+0xfc/0x32c [mlx5_core]
local_pci_probe+0x68/0x12c
pci_call_probe+0x68/0x1ec
pci_device_probe+0xbc/0x1a8
really_probe+0x104/0x570
__driver_probe_device+0xb8/0x224
driver_probe_device+0x54/0x130
__driver_attach+0x158/0x2b0
bus_for_each_dev+0xa8/0x120
driver_attach+0x34/0x48
bus_add_driver+0x174/0x304
driver_register+0x8c/0x1c4
__pci_register_driver+0x68/0x7c
mlx5_init+0xb8/0x118 [mlx5_core]
do_one_initcall+0x60/0x388
do_init_module+0x7c/0x2a4
init_module_from_file+0xb4/0x108
idempotent_init_module+0x184/0x34c
sys_finit_module+0x90/0x114
And oopses are possible when lockdep is enabled or the RTAS
tracepoints are active, since those paths dereference the result of
the lookup.
Use the correct spelling to match firmware's behavior, adjusting the
related constants to match.
Signed-off-by: Nathan Lynch
Reported-by: Gaurav Batra
Fixes: 8252b88294d2 ("powerpc/rtas: improve function information lookups")
---
arch/powerpc/include/asm/rtas.h | 4 ++--
arch/powerpc/kernel/rtas.c | 9 +++--
2 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/include/asm/rtas.h b/arch/powerpc/include/asm/rtas.h
index 9bb2210c8d44..065ffd1b2f8a 100644
--- a/arch/powerpc/include/asm/rtas.h
+++ b/arch/powerpc/include/asm/rtas.h
@@ -69,7 +69,7 @@ enum rtas_function_index {
RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE,
RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE2,
RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW,
- RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS,
+ RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW,
RTAS_FNIDX__IBM_SCAN_LOG_DUMP,
RTAS_FNIDX__IBM_SET_DYNAMIC_INDICATOR,
RTAS_FNIDX__IBM_SET_EEH_OPTION,
@@ -164,7 +164,7 @@ typedef struct {
#define RTAS_FN_IBM_READ_SLOT_RESET_STATE
rtas_fn_handle(RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE)
#define RTAS_FN_IBM_READ_SLOT_RESET_STATE2
rtas_fn_handle(RTAS_FNIDX__IBM_READ_SLOT_RESET_STATE2)
#define RTAS_FN_IBM_REMOVE_PE_DMA_WINDOW
rtas_fn_handle(RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW)
-#define RTAS_FN_IBM_RESET_PE_DMA_WINDOWS
rtas_fn_handle(RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS)
+#define RTAS_FN_IBM_RESET_PE_DMA_WINDOW
rtas_fn_handle(RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW)
#define RTAS_FN_IBM_SCAN_LOG_DUMP
rtas_fn_handle(RTAS_FNIDX__IBM_SCAN_LOG_DUMP)
#define RTAS_FN_IBM_SET_DYNAMIC_INDICATOR
rtas_fn_handle(RTAS_FNIDX__IBM_SET_DYNAMIC_INDICATOR)
#define RTAS_FN_IBM_SET_EEH_OPTION
rtas_fn_handle(RTAS_FNIDX__IBM_SET_EEH_OPTION)
diff --git a/arch/powerpc/kernel/rtas.c b/arch/powerpc/kernel/rtas.c
index 7e793b503e29..8064d9c3de86 100644
--- a/arch/powerpc/kernel/rtas.c
+++ b/arch/powerpc/kernel/rtas.c
@@ -375,8 +375,13 @@ static struct rtas_function rtas_function_table[]
__ro_after_init = {
[RTAS_FNIDX__IBM_REMOVE_PE_DMA_WINDOW] = {
.name = "ibm,remove-pe-dma-window",
},
- [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOWS] = {
- .name = "ibm,reset-pe-dma-windows",
+ [RTAS_FNIDX__IBM_RESET_PE_DMA_WINDOW] = {
+ /*
+* Note: PAPR+ v2.13 7.3.31.4.1 spells this as
+* "ibm,reset-pe-dma-windows" (plural), but RTAS
+* implementations use the singular form in practice.
+*/
+ .name = "ibm,reset-pe-dma-window",
},
[RTAS_FNIDX__IBM_SCAN_LOG_DUMP] = {
.name = "ibm,scan-log-dump",
---
base-commit: b22ea627225b53ec7ce25c19d6df9fa8217d1643
change-id: 20240222-rtas-fix-ibm-reset-pe-dma-window-745dd1824011
Best regards,
--
Nathan Lynch
Re: [PATCH v4 3/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers
On Thu, Feb 22, 2024 at 05:49:59PM +0100, Herve Codina wrote: > Hi Andy, Yury, > > On Thu, 22 Feb 2024 17:39:27 +0200 > Andy Shevchenko wrote: > > ... > > > + * bitmap_scatter() for the bitmap scatter detailed operations). > > > > > + * Suppose scattered computed using bitmap_scatter(scattered, src, mask, > > > n). > > > + * The operation bitmap_gather(result, scattered, mask, n) leads to a > > > result > > > + * equal or equivalent to src. > > > > This paragraph... > > > > > + * The result can be 'equivalent' because bitmap_scatter() and > > > bitmap_gather() > > > + * are not bijective. > > > > > > > + * The result and src values are equivalent in that sense that a call to > > > + * bitmap_scatter(res, src, mask, n) and a call to bitmap_scatter(res, > > > result, > > > + * mask, n) will lead to the same res value. > > > > ...seems duplicating this one. > > > > I would drop the latter one. > > I would like to give details about the 'equivalent' in this scatter/gather > case. If you would like - please do! :) > If Yury is ok, I can drop this last paragraph. The original bitmap_onto() description is 3 times longer, and barely that descriptive. I'm OK with your working and especially pictures. Thanks, Yury
Re: [PATCH v2 RESEND 2/5] sched/vtime: get rid of generic vtime_task_switch() implementation
Alexander Gordeev writes:
> The generic vtime_task_switch() implementation gets built only
> if __ARCH_HAS_VTIME_TASK_SWITCH is not defined, but requires an
> architecture to implement arch_vtime_task_switch() callback at
> the same time, which is confusing.
>
> Further, arch_vtime_task_switch() is implemented for 32-bit PowerPC
> architecture only and vtime_task_switch() generic variant is rather
> superfluous.
>
> Simplify the whole vtime_task_switch() wiring by moving the existing
> generic implementation to PowerPC.
>
> Reviewed-by: Frederic Weisbecker
> Reviewed-by: Nicholas Piggin
> Signed-off-by: Alexander Gordeev
> ---
> arch/powerpc/include/asm/cputime.h | 13 -
> arch/powerpc/kernel/time.c | 22 ++
> kernel/sched/cputime.c | 13 -
> 3 files changed, 22 insertions(+), 26 deletions(-)
Acked-by: Michael Ellerman (powerpc)
cheers
> diff --git a/arch/powerpc/include/asm/cputime.h
> b/arch/powerpc/include/asm/cputime.h
> index 4961fb38e438..aff858ca99c0 100644
> --- a/arch/powerpc/include/asm/cputime.h
> +++ b/arch/powerpc/include/asm/cputime.h
> @@ -32,23 +32,10 @@
> #ifdef CONFIG_PPC64
> #define get_accounting(tsk) (_paca()->accounting)
> #define raw_get_accounting(tsk) (_paca->accounting)
> -static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
>
> #else
> #define get_accounting(tsk) (_thread_info(tsk)->accounting)
> #define raw_get_accounting(tsk) get_accounting(tsk)
> -/*
> - * Called from the context switch with interrupts disabled, to charge all
> - * accumulated times to the current process, and to prepare accounting on
> - * the next process.
> - */
> -static inline void arch_vtime_task_switch(struct task_struct *prev)
> -{
> - struct cpu_accounting_data *acct = get_accounting(current);
> - struct cpu_accounting_data *acct0 = get_accounting(prev);
> -
> - acct->starttime = acct0->starttime;
> -}
> #endif
>
> /*
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index df20cf201f74..c0fdc6d94fee 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -354,6 +354,28 @@ void vtime_flush(struct task_struct *tsk)
> acct->hardirq_time = 0;
> acct->softirq_time = 0;
> }
> +
> +/*
> + * Called from the context switch with interrupts disabled, to charge all
> + * accumulated times to the current process, and to prepare accounting on
> + * the next process.
> + */
> +void vtime_task_switch(struct task_struct *prev)
> +{
> + if (is_idle_task(prev))
> + vtime_account_idle(prev);
> + else
> + vtime_account_kernel(prev);
> +
> + vtime_flush(prev);
> +
> + if (!IS_ENABLED(CONFIG_PPC64)) {
> + struct cpu_accounting_data *acct = get_accounting(current);
> + struct cpu_accounting_data *acct0 = get_accounting(prev);
> +
> + acct->starttime = acct0->starttime;
> + }
> +}
> #endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
>
> void __no_kcsan __delay(unsigned long loops)
> diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
> index af7952f12e6c..aa48b2ec879d 100644
> --- a/kernel/sched/cputime.c
> +++ b/kernel/sched/cputime.c
> @@ -424,19 +424,6 @@ static inline void irqtime_account_process_tick(struct
> task_struct *p, int user_
> */
> #ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
>
> -# ifndef __ARCH_HAS_VTIME_TASK_SWITCH
> -void vtime_task_switch(struct task_struct *prev)
> -{
> - if (is_idle_task(prev))
> - vtime_account_idle(prev);
> - else
> - vtime_account_kernel(prev);
> -
> - vtime_flush(prev);
> - arch_vtime_task_switch(prev);
> -}
> -# endif
> -
> void vtime_account_irq(struct task_struct *tsk, unsigned int offset)
> {
> unsigned int pc = irq_count() - offset;
> --
> 2.40.1
[RESEND2 PATCH net v4 2/2] soc: fsl: qbman: Use raw spinlock for cgr_lock
cgr_lock may be locked with interrupts already disabled by
smp_call_function_single. As such, we must use a raw spinlock to avoid
problems on PREEMPT_RT kernels. Although this bug has existed for a
while, it was not apparent until commit ef2a8d5478b9 ("net: dpaa: Adjust
queue depth on rate change") which invokes smp_call_function_single via
qman_update_cgr_safe every time a link goes up or down.
Fixes: 96f413f47677 ("soc/fsl/qbman: fix issue in qman_delete_cgr_safe()")
CC: sta...@vger.kernel.org
Reported-by: Vladimir Oltean
Closes: https://lore.kernel.org/all/20230323153935.nofnjucqjqnz34ej@skbuf/
Reported-by: Steffen Trumtrar
Closes: https://lore.kernel.org/linux-arm-kernel/87wmsyvclu@pengutronix.de/
Signed-off-by: Sean Anderson
Reviewed-by: Camelia Groza
Tested-by: Vladimir Oltean
---
Changes in v4:
- Add a note about how raw spinlocks aren't quite right
Changes in v3:
- Change blamed commit to something more appropriate
drivers/soc/fsl/qbman/qman.c | 25 ++---
1 file changed, 14 insertions(+), 11 deletions(-)
diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 1bf1f1ea67f0..7e9074519ad2 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -991,7 +991,7 @@ struct qman_portal {
/* linked-list of CSCN handlers. */
struct list_head cgr_cbs;
/* list lock */
- spinlock_t cgr_lock;
+ raw_spinlock_t cgr_lock;
struct work_struct congestion_work;
struct work_struct mr_work;
char irqname[MAX_IRQNAME];
@@ -1281,7 +1281,7 @@ static int qman_create_portal(struct qman_portal *portal,
/* if the given mask is NULL, assume all CGRs can be seen */
qman_cgrs_fill(>cgrs[0]);
INIT_LIST_HEAD(>cgr_cbs);
- spin_lock_init(>cgr_lock);
+ raw_spin_lock_init(>cgr_lock);
INIT_WORK(>congestion_work, qm_congestion_task);
INIT_WORK(>mr_work, qm_mr_process_task);
portal->bits = 0;
@@ -1456,11 +1456,14 @@ static void qm_congestion_task(struct work_struct *work)
union qm_mc_result *mcr;
struct qman_cgr *cgr;
- spin_lock_irq(>cgr_lock);
+ /*
+* FIXME: QM_MCR_TIMEOUT is 10ms, which is too long for a raw spinlock!
+*/
+ raw_spin_lock_irq(>cgr_lock);
qm_mc_start(>p);
qm_mc_commit(>p, QM_MCC_VERB_QUERYCONGESTION);
if (!qm_mc_result_timeout(>p, )) {
- spin_unlock_irq(>cgr_lock);
+ raw_spin_unlock_irq(>cgr_lock);
dev_crit(p->config->dev, "QUERYCONGESTION timeout\n");
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
return;
@@ -1476,7 +1479,7 @@ static void qm_congestion_task(struct work_struct *work)
list_for_each_entry(cgr, >cgr_cbs, node)
if (cgr->cb && qman_cgrs_get(, cgr->cgrid))
cgr->cb(p, cgr, qman_cgrs_get(, cgr->cgrid));
- spin_unlock_irq(>cgr_lock);
+ raw_spin_unlock_irq(>cgr_lock);
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
}
@@ -2440,7 +2443,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
preempt_enable();
cgr->chan = p->config->channel;
- spin_lock_irq(>cgr_lock);
+ raw_spin_lock_irq(>cgr_lock);
if (opts) {
struct qm_mcc_initcgr local_opts = *opts;
@@ -2477,7 +2480,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
qman_cgrs_get(>cgrs[1], cgr->cgrid))
cgr->cb(p, cgr, 1);
out:
- spin_unlock_irq(>cgr_lock);
+ raw_spin_unlock_irq(>cgr_lock);
put_affine_portal();
return ret;
}
@@ -2512,7 +2515,7 @@ int qman_delete_cgr(struct qman_cgr *cgr)
return -EINVAL;
memset(_opts, 0, sizeof(struct qm_mcc_initcgr));
- spin_lock_irqsave(>cgr_lock, irqflags);
+ raw_spin_lock_irqsave(>cgr_lock, irqflags);
list_del(>node);
/*
* If there are no other CGR objects for this CGRID in the list,
@@ -2537,7 +2540,7 @@ int qman_delete_cgr(struct qman_cgr *cgr)
/* add back to the list */
list_add(>node, >cgr_cbs);
release_lock:
- spin_unlock_irqrestore(>cgr_lock, irqflags);
+ raw_spin_unlock_irqrestore(>cgr_lock, irqflags);
put_affine_portal();
return ret;
}
@@ -2577,9 +2580,9 @@ static int qman_update_cgr(struct qman_cgr *cgr, struct
qm_mcc_initcgr *opts)
if (!p)
return -EINVAL;
- spin_lock_irqsave(>cgr_lock, irqflags);
+ raw_spin_lock_irqsave(>cgr_lock, irqflags);
ret = qm_modify_cgr(cgr, 0, opts);
- spin_unlock_irqrestore(>cgr_lock, irqflags);
+ raw_spin_unlock_irqrestore(>cgr_lock, irqflags);
put_affine_portal();
return ret;
}
--
2.35.1.1320.gc452695387.dirty
[RESEND2 PATCH net v4 1/2] soc: fsl: qbman: Always disable interrupts when taking cgr_lock
smp_call_function_single disables IRQs when executing the callback. To
prevent deadlocks, we must disable IRQs when taking cgr_lock elsewhere.
This is already done by qman_update_cgr and qman_delete_cgr; fix the
other lockers.
Fixes: 96f413f47677 ("soc/fsl/qbman: fix issue in qman_delete_cgr_safe()")
CC: sta...@vger.kernel.org
Signed-off-by: Sean Anderson
Reviewed-by: Camelia Groza
Tested-by: Vladimir Oltean
---
Resent from a non-mangling email.
(no changes since v3)
Changes in v3:
- Change blamed commit to something more appropriate
Changes in v2:
- Fix one additional call to spin_unlock
drivers/soc/fsl/qbman/qman.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c
index 739e4eee6b75..1bf1f1ea67f0 100644
--- a/drivers/soc/fsl/qbman/qman.c
+++ b/drivers/soc/fsl/qbman/qman.c
@@ -1456,11 +1456,11 @@ static void qm_congestion_task(struct work_struct *work)
union qm_mc_result *mcr;
struct qman_cgr *cgr;
- spin_lock(>cgr_lock);
+ spin_lock_irq(>cgr_lock);
qm_mc_start(>p);
qm_mc_commit(>p, QM_MCC_VERB_QUERYCONGESTION);
if (!qm_mc_result_timeout(>p, )) {
- spin_unlock(>cgr_lock);
+ spin_unlock_irq(>cgr_lock);
dev_crit(p->config->dev, "QUERYCONGESTION timeout\n");
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
return;
@@ -1476,7 +1476,7 @@ static void qm_congestion_task(struct work_struct *work)
list_for_each_entry(cgr, >cgr_cbs, node)
if (cgr->cb && qman_cgrs_get(, cgr->cgrid))
cgr->cb(p, cgr, qman_cgrs_get(, cgr->cgrid));
- spin_unlock(>cgr_lock);
+ spin_unlock_irq(>cgr_lock);
qman_p_irqsource_add(p, QM_PIRQ_CSCI);
}
@@ -2440,7 +2440,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
preempt_enable();
cgr->chan = p->config->channel;
- spin_lock(>cgr_lock);
+ spin_lock_irq(>cgr_lock);
if (opts) {
struct qm_mcc_initcgr local_opts = *opts;
@@ -2477,7 +2477,7 @@ int qman_create_cgr(struct qman_cgr *cgr, u32 flags,
qman_cgrs_get(>cgrs[1], cgr->cgrid))
cgr->cb(p, cgr, 1);
out:
- spin_unlock(>cgr_lock);
+ spin_unlock_irq(>cgr_lock);
put_affine_portal();
return ret;
}
--
2.35.1.1320.gc452695387.dirty
Re: [PATCH v2 00/14] Split crash out from kexec and clean up related config items
On Thu, 22 Feb 2024 10:47:29 +0530 Hari Bathini wrote: > > > On 22/02/24 2:27 am, Andrew Morton wrote: > > On Wed, 21 Feb 2024 11:15:00 +0530 Hari Bathini > > wrote: > > > >> On 04/02/24 8:56 am, Baoquan He wrote: > > Hope Hari and Pingfan can help have a look, see if > > it's doable. Now, I make it either have both kexec and crash enabled, or > > disable both of them altogether. > > Sure. I will take a closer look... > >>> Thanks a lot. Please feel free to post patches to make that, or I can do > >>> it with your support or suggestion. > >> > >> Tested your changes and on top of these changes, came up with the below > >> changes to get it working for powerpc: > >> > >> > >> https://lore.kernel.org/all/20240213113150.1148276-1-hbath...@linux.ibm.com/ > > > > So can we take it that you're OK with Baoquan's series as-is? > > Hi Andrew, > > If you mean > > v3 (https://lore.kernel.org/all/20240124051254.67105-1-...@redhat.com/) > + > follow-up from Baoquan > (https://lore.kernel.org/all/Zb8D1ASrgX0qVm9z@MiWiFi-R3L-srv/) > > Yes. > Can I add your Acked-by: and/or Tested-by: to the patches in this series?
[PATCH v5 15/20] EDAC/mc: Re-use generic unique MC index allocation procedure
The EDAC drivers locally maintaining a statically defined
memory-controllers counter don't care much about the MC index assigned as
long as it's unique so the EDAC core perceives it. Convert these drivers
to be using the generic MC index allocation procedure recently added to
the EDAC core.
Signed-off-by: Serge Semin
---
Changelog v4:
- Initial patch introduction.
---
drivers/edac/dmc520_edac.c | 4 +---
drivers/edac/pasemi_edac.c | 5 +
drivers/edac/ppc4xx_edac.c | 5 +
3 files changed, 3 insertions(+), 11 deletions(-)
diff --git a/drivers/edac/dmc520_edac.c b/drivers/edac/dmc520_edac.c
index 4e30b989a1a4..93734a97a67b 100644
--- a/drivers/edac/dmc520_edac.c
+++ b/drivers/edac/dmc520_edac.c
@@ -173,8 +173,6 @@ struct dmc520_edac {
int masks[NUMBER_OF_IRQS];
};
-static int dmc520_mc_idx;
-
static u32 dmc520_read_reg(struct dmc520_edac *pvt, u32 offset)
{
return readl(pvt->reg_base + offset);
@@ -517,7 +515,7 @@ static int dmc520_edac_probe(struct platform_device *pdev)
layers[0].size = dmc520_get_rank_count(reg_base);
layers[0].is_virt_csrow = true;
- mci = edac_mc_alloc(dmc520_mc_idx++, ARRAY_SIZE(layers), layers,
sizeof(*pvt));
+ mci = edac_mc_alloc(EDAC_AUTO_MC_NUM, ARRAY_SIZE(layers), layers,
sizeof(*pvt));
if (!mci) {
edac_printk(KERN_ERR, EDAC_MOD_NAME,
"Failed to allocate memory for mc instance\n");
diff --git a/drivers/edac/pasemi_edac.c b/drivers/edac/pasemi_edac.c
index 1a1c3296ccc8..afebfbda1ea0 100644
--- a/drivers/edac/pasemi_edac.c
+++ b/drivers/edac/pasemi_edac.c
@@ -57,8 +57,6 @@
#define PASEMI_EDAC_ERROR_GRAIN64
static int last_page_in_mmc;
-static int system_mmc_id;
-
static u32 pasemi_edac_get_error_info(struct mem_ctl_info *mci)
{
@@ -203,8 +201,7 @@ static int pasemi_edac_probe(struct pci_dev *pdev,
layers[1].type = EDAC_MC_LAYER_CHANNEL;
layers[1].size = PASEMI_EDAC_NR_CHANS;
layers[1].is_virt_csrow = false;
- mci = edac_mc_alloc(system_mmc_id++, ARRAY_SIZE(layers), layers,
- 0);
+ mci = edac_mc_alloc(EDAC_AUTO_MC_NUM, ARRAY_SIZE(layers), layers, 0);
if (mci == NULL)
return -ENOMEM;
diff --git a/drivers/edac/ppc4xx_edac.c b/drivers/edac/ppc4xx_edac.c
index 1eea3341a916..06d267d40a6a 100644
--- a/drivers/edac/ppc4xx_edac.c
+++ b/drivers/edac/ppc4xx_edac.c
@@ -1214,7 +1214,6 @@ static int ppc4xx_edac_probe(struct platform_device *op)
const struct device_node *np = op->dev.of_node;
struct mem_ctl_info *mci = NULL;
struct edac_mc_layer layers[2];
- static int ppc4xx_edac_instance;
/*
* At this point, we only support the controller realized on
@@ -1265,7 +1264,7 @@ static int ppc4xx_edac_probe(struct platform_device *op)
layers[1].type = EDAC_MC_LAYER_CHANNEL;
layers[1].size = ppc4xx_edac_nr_chans;
layers[1].is_virt_csrow = false;
- mci = edac_mc_alloc(ppc4xx_edac_instance, ARRAY_SIZE(layers), layers,
+ mci = edac_mc_alloc(EDAC_AUTO_MC_NUM, ARRAY_SIZE(layers), layers,
sizeof(struct ppc4xx_edac_pdata));
if (mci == NULL) {
ppc4xx_edac_printk(KERN_ERR, "%pOF: "
@@ -1303,8 +1302,6 @@ static int ppc4xx_edac_probe(struct platform_device *op)
goto fail1;
}
- ppc4xx_edac_instance++;
-
return 0;
fail1:
--
2.43.0
[PATCH RESEND v10 0/1] dt-bindings: usb: Harmonize xHCI/EHCI/OHCI/DWC3 nodes name
As the subject states this series is an attempt to harmonize the xHCI,
EHCI, OHCI and DWC USB3 DT nodes with the DT schema introduced in the
framework of the patchset [1].
Firstly as Krzysztof suggested we've deprecated a support of DWC USB3
controllers with "synopsys,"-vendor prefix compatible string in favor of
the ones with valid "snps,"-prefix. It's done in all the DTS files,
which have been unfortunate to define such nodes.
Secondly we suggest to fix the snps,quirk-frame-length-adjustment property
declaration in the Amlogic meson-g12-common.dtsi DTS file, since it has
been erroneously declared as boolean while having uint32 type. Neil said
it was ok to init that property with 0x20 value.
Thirdly the main part of the patchset concern fixing the xHCI, EHCI/OHCI
and DWC USB3 DT nodes name as in accordance with their DT schema the
corresponding node name is suppose to comply with the Generic USB HCD DT
schema, which requires the USB nodes to have the name acceptable by the
regexp: "^usb(@.*)?". Such requirement had been applicable even before we
introduced the new DT schema in [1], but as we can see it hasn't been
strictly implemented for a lot the DTS files. Since DT schema is now
available the automated DTS validation shall make sure that the rule isn't
violated.
Note most of these patches have been a part of the last three patches of
[1]. But since there is no way to have them merged in in a combined
manner, I had to move them to the dedicated series and split them up so to
be accepted by the corresponding subsystem maintainers one-by-one.
[1] Link:
https://lore.kernel.org/linux-usb/20201014101402.18271-1-sergey.se...@baikalelectronics.ru
Changelog v1:
- As Krzysztof suggested I've created a script which checked whether the
node names had been also updated in all the depended dts files. As a
result I found two more files which should have been also modified:
arch/arc/boot/dts/{axc003.dtsi,axc003_idu.dtsi}
- Correct the USB DWC3 nodes name found in
arch/arm64/boot/dts/apm/{apm-storm.dtsi,apm-shadowcat.dtsi} too.
Link:
https://lore.kernel.org/linux-usb/20201020115959.2658-1-sergey.se...@baikalelectronics.ru
Changelog v2:
- Drop the patch:
[PATCH 01/29] usb: dwc3: Discard synopsys,dwc3 compatibility string
and get back the one which marks the "synopsys,dwc3" compatible string
as deprecated into the DT schema related series.
- Drop the patches:
[PATCH 03/29] arm: dts: am437x: Correct DWC USB3 compatible string
[PATCH 04/29] arm: dts: exynos: Correct DWC USB3 compatible string
[PATCH 07/29] arm: dts: bcm53x: Harmonize EHCI/OHCI DT nodes name
[PATCH 08/29] arm: dts: stm32: Harmonize EHCI/OHCI DT nodes name
[PATCH 16/29] arm: dts: bcm5301x: Harmonize xHCI DT nodes name
[PATCH 19/29] arm: dts: exynos: Harmonize DWC USB3 DT nodes name
[PATCH 21/29] arm: dts: ls1021a: Harmonize DWC USB3 DT nodes name
[PATCH 22/29] arm: dts: omap5: Harmonize DWC USB3 DT nodes name
[PATCH 24/29] arm64: dts: allwinner: h6: Harmonize DWC USB3 DT nodes name
[PATCH 26/29] arm64: dts: exynos: Harmonize DWC USB3 DT nodes name
[PATCH 27/29] arm64: dts: layerscape: Harmonize DWC USB3 DT nodes name
since they have been applied to the corresponding maintainers repos.
- Fix drivers/usb/dwc3/dwc3-qcom.c to be looking for the "usb@"-prefixed
sub-node and falling back to the "dwc3@"-prefixed one on failure.
Link:
https://lore.kernel.org/linux-usb/2020091552.15593-1-sergey.se...@baikalelectronics.ru
Changelog v3:
- Drop the patches:
[PATCH v2 04/18] arm: dts: hisi-x5hd2: Harmonize EHCI/OHCI DT nodes name
[PATCH v2 06/18] arm64: dts: hisi: Harmonize EHCI/OHCI DT nodes name
[PATCH v2 07/18] mips: dts: jz47x: Harmonize EHCI/OHCI DT nodes name
[PATCH v2 08/18] mips: dts: sead3: Harmonize EHCI/OHCI DT nodes name
[PATCH v2 09/18] mips: dts: ralink: mt7628a: Harmonize EHCI/OHCI DT nodes name
[PATCH v2 11/18] arm64: dts: marvell: cp11x: Harmonize xHCI DT nodes name
[PATCH v2 12/18] arm: dts: marvell: armada-375: Harmonize DWC USB3 DT nodes
name
[PATCH v2 16/18] arm64: dts: hi3660: Harmonize DWC USB3 DT nodes name
since they have been applied to the corresponding maintainers repos.
Link:
https://lore.kernel.org/linux-usb/20201205155621.3045-1-sergey.se...@baikalelectronics.ru
Changelog v4:
- Just resend.
Link:
https://lore.kernel.org/linux-usb/20201210091756.18057-1-sergey.se...@baikalelectronics.ru
Changelog v5:
- Drop the patch:
[PATCH v4 02/10] arm64: dts: amlogic: meson-g12: Set FL-adj property value
since it has been applied to the corresponding maintainers repos.
- Get back the patch:
[PATCH 21/29] arm: dts: ls1021a: Harmonize DWC USB3 DT nodes name
as it has been missing in the kernel 5.11-rc7
- Rebase onto the kernel 5.11-rc7.
Link:
https://lore.kernel.org/lkml/20210208135154.6645-1-sergey.se...@baikalelectronics.ru
Changelog v6:
- Just resend and add linux-usb.vger.kernel.org to the list of Ccecipients.
Link:
[PATCH RESEND v10 1/1] powerpc: dts: akebono: Harmonize EHCI/OHCI DT nodes name
In accordance with the Generic EHCI/OHCI bindings the corresponding node
name is suppose to comply with the Generic USB HCD DT schema, which
requires the USB nodes to have the name acceptable by the regexp:
"^usb(@.*)?" . Make sure the "generic-ehci" and "generic-ohci"-compatible
nodes are correctly named.
Signed-off-by: Serge Semin
Acked-by: Krzysztof Kozlowski
---
arch/powerpc/boot/dts/akebono.dts | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/powerpc/boot/dts/akebono.dts
b/arch/powerpc/boot/dts/akebono.dts
index df18f8dc4642..343326c30380 100644
--- a/arch/powerpc/boot/dts/akebono.dts
+++ b/arch/powerpc/boot/dts/akebono.dts
@@ -126,7 +126,7 @@ SATA0: sata@301 {
interrupts = <93 2>;
};
- EHCI0: ehci@3001000 {
+ EHCI0: usb@3001000 {
compatible = "ibm,476gtr-ehci", "generic-ehci";
reg = <0x300 0x1000 0x0 0x1>;
interrupt-parent = <>;
@@ -140,14 +140,14 @@ SD0: sd@300 {
interrupt-parent = <>;
};
- OHCI0: ohci@3001001 {
+ OHCI0: usb@3001001 {
compatible = "ibm,476gtr-ohci", "generic-ohci";
reg = <0x300 0x1001 0x0 0x1>;
interrupt-parent = <>;
interrupts = <89 1>;
};
- OHCI1: ohci@3001002 {
+ OHCI1: usb@3001002 {
compatible = "ibm,476gtr-ohci", "generic-ohci";
reg = <0x300 0x1002 0x0 0x1>;
interrupt-parent = <>;
--
2.43.0
Re: [PATCH v4 1/5] net: wan: Add support for QMC HDLC
Andy, Jakub,
On Thu, 22 Feb 2024 18:56:26 +0200
Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 05:45:01PM +0100, Herve Codina wrote:
> > On Thu, 22 Feb 2024 17:29:05 +0200
> > Andy Shevchenko wrote:
> > > On Thu, Feb 22, 2024 at 03:22:14PM +0100, Herve Codina wrote:
>
> ...
>
> > > > + spin_lock_irqsave(_hdlc->tx_lock, flags);
> > >
> > > Why not using cleanup.h from day 1?
> >
> > I don't know about cleanup.h.
> > Can you tell me more ?
> > How should I use it ?
> >
> > > > +end:
> > >
> > > This label, in particular, will not be needed with above in place.
> > >
> > > > + spin_unlock_irqrestore(_hdlc->tx_lock, flags);
> > > > + return ret;
> > > > +}
>
> Here are the examples:
> 6191e49de389 ("pinctrl: baytrail: Simplify code with cleanup helpers")
> 1d1b4770d4b6 ("platform/x86/intel/vsec: Use cleanup.h")
> e2eeddefb046 ("pstore: inode: Convert mutex usage to guard(mutex)")
>
> Some advanced stuff:
> ced085ef369a ("PCI: Introduce cleanup helpers for device reference counts and
> locks")
>
> Hope this helps.
Sure, thanks for the pointer.
Jakub,
nothing in drivers/net seems to use the guard() (from cleanup.h) family
macro.
Are you ok with having this HDLC driver that uses guard() macros ?
Best regards,
Hervé
--
Hervé Codina, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v4 1/5] net: wan: Add support for QMC HDLC
On Thu, Feb 22, 2024 at 05:45:01PM +0100, Herve Codina wrote:
> On Thu, 22 Feb 2024 17:29:05 +0200
> Andy Shevchenko wrote:
> > On Thu, Feb 22, 2024 at 03:22:14PM +0100, Herve Codina wrote:
...
> > > + spin_lock_irqsave(_hdlc->tx_lock, flags);
> >
> > Why not using cleanup.h from day 1?
>
> I don't know about cleanup.h.
> Can you tell me more ?
> How should I use it ?
>
> > > +end:
> >
> > This label, in particular, will not be needed with above in place.
> >
> > > + spin_unlock_irqrestore(_hdlc->tx_lock, flags);
> > > + return ret;
> > > +}
Here are the examples:
6191e49de389 ("pinctrl: baytrail: Simplify code with cleanup helpers")
1d1b4770d4b6 ("platform/x86/intel/vsec: Use cleanup.h")
e2eeddefb046 ("pstore: inode: Convert mutex usage to guard(mutex)")
Some advanced stuff:
ced085ef369a ("PCI: Introduce cleanup helpers for device reference counts and
locks")
Hope this helps.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 3/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers
Hi Andy, Yury, On Thu, 22 Feb 2024 17:39:27 +0200 Andy Shevchenko wrote: ... > > + * bitmap_scatter() for the bitmap scatter detailed operations). > > > + * Suppose scattered computed using bitmap_scatter(scattered, src, mask, > > n). > > + * The operation bitmap_gather(result, scattered, mask, n) leads to a > > result > > + * equal or equivalent to src. > > This paragraph... > > > + * The result can be 'equivalent' because bitmap_scatter() and > > bitmap_gather() > > + * are not bijective. > > > > + * The result and src values are equivalent in that sense that a call to > > + * bitmap_scatter(res, src, mask, n) and a call to bitmap_scatter(res, > > result, > > + * mask, n) will lead to the same res value. > > ...seems duplicating this one. > > I would drop the latter one. I would like to give details about the 'equivalent' in this scatter/gather case. If Yury is ok, I can drop this last paragraph. Thanks for the review, Regards, Hervé
Re: [PATCH v4 1/5] net: wan: Add support for QMC HDLC
On Thu, 22 Feb 2024 17:29:05 +0200
Andy Shevchenko wrote:
> On Thu, Feb 22, 2024 at 03:22:14PM +0100, Herve Codina wrote:
> > The QMC HDLC driver provides support for HDLC using the QMC (QUICC
> > Multichannel Controller) to transfer the HDLC data.
>
> ...
>
> > +struct qmc_hdlc {
> > + struct device *dev;
> > + struct qmc_chan *qmc_chan;
> > + struct net_device *netdev;
> > + bool is_crc32;
> > + spinlock_t tx_lock; /* Protect tx descriptors */
>
> Just wondering if above tx/rx descriptors should be aligned on a cacheline
> for DMA?
These descriptors are not used by DMA.
Not sure that aligning them to a cacheline is really needed.
>
> > + struct qmc_hdlc_desc tx_descs[8];
> > + unsigned int tx_out;
> > + struct qmc_hdlc_desc rx_descs[4];
> > +};
>
> ...
>
> > +#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
> > +QMC_RX_FLAG_HDLC_UNA | \
> > +QMC_RX_FLAG_HDLC_ABORT | \
> > +QMC_RX_FLAG_HDLC_CRC)
>
> Wouldn't be slightly better to have it as
>
> #define QMC_HDLC_RX_ERROR_FLAGS \
> (QMC_RX_FLAG_HDLC_OVF | QMC_RX_FLAG_HDLC_UNA | \
>QMC_RX_FLAG_HDLC_CRC | QMC_RX_FLAG_HDLC_ABORT)
>
> ?
Will be done in the next iteration.
>
> ...
>
> > + ret = qmc_chan_write_submit(qmc_hdlc->qmc_chan, desc->dma_addr,
> > desc->dma_size,
> > + qmc_hdlc_xmit_complete, desc);
> > + if (ret) {
>
> > + dev_err(qmc_hdlc->dev, "qmc chan write returns %d\n", ret);
> > + dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size,
> > DMA_TO_DEVICE);
> > + return ret;
>
> I would do other way around, i.e. release resource followed up by printing
> a message. Printing a message is a slow operation and may prevent the (soon
> freed) resources to be re-used earlier.
Will do that in the next iteration.
>
> > + }
>
> ...
>
> > + spin_lock_irqsave(_hdlc->tx_lock, flags);
>
> Why not using cleanup.h from day 1?
I don't know about cleanup.h.
Can you tell me more ?
How should I use it ?
>
> > +end:
>
> This label, in particular, will not be needed with above in place.
>
> > + spin_unlock_irqrestore(_hdlc->tx_lock, flags);
> > + return ret;
> > +}
>
> ...
>
> > + /* Queue as many recv descriptors as possible */
> > + for (i = 0; i < ARRAY_SIZE(qmc_hdlc->rx_descs); i++) {
> > + desc = _hdlc->rx_descs[i];
> > +
> > + desc->netdev = netdev;
> > + ret = qmc_hdlc_recv_queue(qmc_hdlc, desc,
> > chan_param.hdlc.max_rx_buf_size);
>
> > + if (ret) {
> > + if (ret == -EBUSY && i != 0)
> > + break; /* We use all the QMC chan capability */
> > + goto free_desc;
> > + }
>
> Can be unfolded to
>
> if (ret == -EBUSY && i)
> break; /* We use all the QMC chan capability */
> if (ret)
> goto free_desc;
>
> Easy to read and understand.
Sure, will be changed.
>
> > + }
>
> ...
>
> > +static int qmc_hdlc_probe(struct platform_device *pdev)
> > +{
>
> With
>
> struct device *dev = >dev;
>
> the below code will be neater (see other comments for the examples).
Will use that.
>
> > + struct device_node *np = pdev->dev.of_node;
>
> It is used only once, drop it (see below).
Ok.
>
> > + struct qmc_hdlc *qmc_hdlc;
> > + struct qmc_chan_info info;
> > + hdlc_device *hdlc;
> > + int ret;
> > +
> > + qmc_hdlc = devm_kzalloc(>dev, sizeof(*qmc_hdlc), GFP_KERNEL);
> > + if (!qmc_hdlc)
> > + return -ENOMEM;
> > +
> > + qmc_hdlc->dev = >dev;
> > + spin_lock_init(_hdlc->tx_lock);
> > +
> > + qmc_hdlc->qmc_chan = devm_qmc_chan_get_bychild(qmc_hdlc->dev, np);
>
> qmc_hdlc->qmc_chan = devm_qmc_chan_get_bychild(dev, dev->of_node);
>
> > + if (IS_ERR(qmc_hdlc->qmc_chan)) {
> > + ret = PTR_ERR(qmc_hdlc->qmc_chan);
> > + return dev_err_probe(qmc_hdlc->dev, ret, "get QMC channel
> > failed\n");
>
> return dev_err_probe(dev, PTR_ERR(qmc_hdlc->qmc_chan), "get QMC
> channel failed\n");
>
> > + }
> > +
> > + ret = qmc_chan_get_info(qmc_hdlc->qmc_chan, );
> > + if (ret) {
>
> > + dev_err(qmc_hdlc->dev, "get QMC channel info failed %d\n", ret);
> > + return ret;
>
> Why not using same message pattern everywhere, i.e. dev_err_probe()?
>
> return dev_err_probe(dev, ret, "get QMC channel info failed\n");
>
> (and so on...)
No reason. Just because I missed them.
Will be updated using dev_err_probe().
>
> > + }
> > +
> > + if (info.mode != QMC_HDLC) {
> > + dev_err(qmc_hdlc->dev, "QMC chan mode %d is not QMC_HDLC\n",
> > + info.mode);
> > + return -EINVAL;
> > + }
> > +
> > + qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
> > +
Re: [PATCH 3/3] arch: Rename fbdev header and source files
Hi Thomas, kernel test robot noticed the following build errors: [auto build test ERROR on tip/x86/core] [also build test ERROR on deller-parisc/for-next arnd-asm-generic/master linus/master v6.8-rc5 next-20240221] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/arch-Select-fbdev-helpers-with-CONFIG_VIDEO/20240222-001622 base: tip/x86/core patch link: https://lore.kernel.org/r/20240221161431.8245-4-tzimmermann%40suse.de patch subject: [PATCH 3/3] arch: Rename fbdev header and source files config: um-randconfig-r052-20240222 (https://download.01.org/0day-ci/archive/20240223/202402230023.xa2jjwui-...@intel.com/config) compiler: gcc-12 (Debian 12.2.0-14) 12.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240223/202402230023.xa2jjwui-...@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot | Closes: https://lore.kernel.org/oe-kbuild-all/202402230023.xa2jjwui-...@intel.com/ All errors (new ones prefixed by >>): /usr/bin/ld: drivers/video/fbdev/core/fb_io_fops.o: in function `fb_io_mmap': >> fb_io_fops.c:(.text+0x591): undefined reference to `pgprot_framebuffer' collect2: error: ld returned 1 exit status -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki
Re: [PATCH v4 0/5] Add support for QMC HDLC
On Thu, Feb 22, 2024 at 03:22:13PM +0100, Herve Codina wrote: > Hi, > > This series introduces the QMC HDLC support. > > Patches were previously sent as part of a full feature series and were > previously reviewed in that context: > "Add support for QMC HDLC, framer infrastructure and PEF2256 framer" [1] > > In order to ease the merge, the full feature series has been split and > needed parts were merged in v6.8-rc1: > - "Prepare the PowerQUICC QMC and TSA for the HDLC QMC driver" [2] > - "Add support for framer infrastructure and PEF2256 framer" [3] > > This series contains patches related to the QMC HDLC part (QMC HDLC > driver): > - Introduce the QMC HDLC driver (patches 1 and 2) > - Add timeslots change support in QMC HDLC (patch 3) > - Add framer support as a framer consumer in QMC HDLC (patch 4) > > Compare to the original full feature series, a modification was done on > patch 3 in order to use a coherent prefix in the commit title. > > I kept the patches unsquashed as they were previously sent and reviewed. > Of course, I can squash them if needed. > > Compared to the previous iteration: > > https://lore.kernel.org/linux-kernel/20240212075646.19114-1-herve.cod...@bootlin.com/ > this v4 series mainly: >From my point of view after addressing the few non-critical issues the v4 will be final. Thank you! -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 5/5] net: wan: fsl_qmc_hdlc: Add framer support
On Thu, Feb 22, 2024 at 03:22:18PM +0100, Herve Codina wrote:
> Add framer support in the fsl_qmc_hdlc driver in order to be able to
> signal carrier changes to the network stack based on the framer status
> Also use this framer to provide information related to the E1/T1 line
> interface on IF_GET_IFACE and configure the line interface according to
> IF_IFACE_{E1,T1} information.
...
> +static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
> +{
> + struct framer_status framer_status;
> + unsigned long flags;
> + int ret;
> +
> + if (!qmc_hdlc->framer)
> + return 0;
> + spin_lock_irqsave(_hdlc->carrier_lock, flags);
cleanup.h ?
> + ret = framer_get_status(qmc_hdlc->framer, _status);
> + if (ret) {
> + dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
> + goto end;
> + }
> + if (framer_status.link_is_on)
> + netif_carrier_on(qmc_hdlc->netdev);
> + else
> + netif_carrier_off(qmc_hdlc->netdev);
> +
> +end:
> + spin_unlock_irqrestore(_hdlc->carrier_lock, flags);
> + return ret;
> +}
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 4/5] net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support
On Thu, Feb 22, 2024 at 03:22:17PM +0100, Herve Codina wrote:
> QMC channels support runtime timeslots changes but nothing is done at
> the QMC HDLC driver to handle these changes.
>
> Use existing IFACE ioctl in order to configure the timeslots to use.
...
> +static int qmc_hdlc_xlate_slot_map(struct qmc_hdlc *qmc_hdlc,
> +u32 slot_map, struct qmc_chan_ts_info
> *ts_info)
> +{
> + DECLARE_BITMAP(ts_mask_avail, 64);
> + DECLARE_BITMAP(ts_mask, 64);
> + DECLARE_BITMAP(map, 64);
Perhaps more 1:1 naming?
DECLARE_BITMAP(rx_ts_mask_avail, 64);
DECLARE_BITMAP(tx_ts_mask, 64);
DECLARE_BITMAP(slot_map, 64);
> + /* Tx and Rx available masks must be identical */
> + if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
> + dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch
> (0x%llx, 0x%llx)\n",
> + ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
> + return -EINVAL;
> + }
> +
> + bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
> + bitmap_from_u64(map, slot_map);
> + bitmap_scatter(ts_mask, map, ts_mask_avail, 64);
> +
> + if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
> + dev_err(qmc_hdlc->dev, "Cannot translate timeslots %*pb ->
> (%*pb, %*pb)\n",
> + 64, map, 64, ts_mask_avail, 64, ts_mask);
You can save a bit of code and stack:
dev_err(qmc_hdlc->dev, "Cannot translate timeslots %64pb ->
(%64pb, %64pb)\n",
slot_map, rx_ts_mask_avail, tx_ts_mask);
> + return -EINVAL;
> + }
> +
> + bitmap_to_arr64(_info->tx_ts_mask, ts_mask, 64);
> + ts_info->rx_ts_mask = ts_info->tx_ts_mask;
> + return 0;
> +}
...
> +static int qmc_hdlc_xlate_ts_info(struct qmc_hdlc *qmc_hdlc,
> + const struct qmc_chan_ts_info *ts_info, u32
> *slot_map)
Similar comments apply as per above function.
...
> + ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, _info);
> + if (ret) {
> + dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n",
> ret);
> + return ret;
return dev_err_probe(...);
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 3/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers
On Thu, Feb 22, 2024 at 05:39:27PM +0200, Andy Shevchenko wrote: > On Thu, Feb 22, 2024 at 03:22:16PM +0100, Herve Codina wrote: > > From: Andy Shevchenko > > The original work was done by Andy Shevchenko. > > Mine SoB is enough for a credit, but thank you :-) That said, you forgot to add your Co-developed-by. -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 3/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers
On Thu, Feb 22, 2024 at 03:22:16PM +0100, Herve Codina wrote: > From: Andy Shevchenko > > These helpers scatters or gathers a bitmap with the help of the mask > position bits parameter. > > bitmap_scatter() does the following: > src: 01011010 > || >+--+| >| ++ >| |++||| >| || +-+|| >| || | || > mask: ...v..vv...v..vv > ...0..11...0..10 > dst: 00110010 > > and bitmap_gather() performs this one: >mask: ...v..vv...v..vv >src: 00110010 > ^ ^^ ^ 0 > | || | 10 > | || > 010 > | |+--> 1010 > | +--> 11010 > +> 011010 >dst: 00011010 > > bitmap_gather() can the seen as the reverse bitmap_scatter() operation. > The original work was done by Andy Shevchenko. Mine SoB is enough for a credit, but thank you :-) ... > +/** > + * bitmap_gather - Gather a bitmap according to given mask > + * @dst: gathered bitmap > + * @src: scattered bitmap > + * @mask: mask representing bits to extract from in the scattered bitmap > + * @nbits: number of bits in each of these bitmaps > + * > + * Gathers bitmap with sparse bits according to the given @mask. > + * > + * Example: > + * If @src bitmap = 0x0302, with @mask = 0x1313, @dst will be 0x001a. > + * > + * Or in binary form > + * @src @mask @dst > + * 00110010 000100110001001100011010 > + * > + * (Bits 0, 1, 4, 8, 9, 12 are copied to the bits 0, 1, 2, 3, 4, 5) > + * > + * A more 'visual' description of the operation: > + * mask: ...v..vv...v..vv > + * src: 00110010 > + * ^ ^^ ^ 0 > + * | || | 10 > + * | || > 010 > + * | |+--> 1010 > + * | +--> 11010 > + * +> 011010 > + * dst: 00011010 Cool! > + * A relationship exists between bitmap_gather() and bitmap_scatter() (See Either '... (see' or '(). See' > + * bitmap_scatter() for the bitmap scatter detailed operations). > + * Suppose scattered computed using bitmap_scatter(scattered, src, mask, n). > + * The operation bitmap_gather(result, scattered, mask, n) leads to a result > + * equal or equivalent to src. This paragraph... > + * The result can be 'equivalent' because bitmap_scatter() and > bitmap_gather() > + * are not bijective. > + * The result and src values are equivalent in that sense that a call to > + * bitmap_scatter(res, src, mask, n) and a call to bitmap_scatter(res, > result, > + * mask, n) will lead to the same res value. ...seems duplicating this one. I would drop the latter one. > + */ -- With Best Regards, Andy Shevchenko
Re: [PATCH v4 1/5] net: wan: Add support for QMC HDLC
On Thu, Feb 22, 2024 at 03:22:14PM +0100, Herve Codina wrote:
> The QMC HDLC driver provides support for HDLC using the QMC (QUICC
> Multichannel Controller) to transfer the HDLC data.
...
> +struct qmc_hdlc {
> + struct device *dev;
> + struct qmc_chan *qmc_chan;
> + struct net_device *netdev;
> + bool is_crc32;
> + spinlock_t tx_lock; /* Protect tx descriptors */
Just wondering if above tx/rx descriptors should be aligned on a cacheline
for DMA?
> + struct qmc_hdlc_desc tx_descs[8];
> + unsigned int tx_out;
> + struct qmc_hdlc_desc rx_descs[4];
> +};
...
> +#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
> + QMC_RX_FLAG_HDLC_UNA | \
> + QMC_RX_FLAG_HDLC_ABORT | \
> + QMC_RX_FLAG_HDLC_CRC)
Wouldn't be slightly better to have it as
#define QMC_HDLC_RX_ERROR_FLAGS \
(QMC_RX_FLAG_HDLC_OVF | QMC_RX_FLAG_HDLC_UNA | \
QMC_RX_FLAG_HDLC_CRC | QMC_RX_FLAG_HDLC_ABORT)
?
...
> + ret = qmc_chan_write_submit(qmc_hdlc->qmc_chan, desc->dma_addr,
> desc->dma_size,
> + qmc_hdlc_xmit_complete, desc);
> + if (ret) {
> + dev_err(qmc_hdlc->dev, "qmc chan write returns %d\n", ret);
> + dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size,
> DMA_TO_DEVICE);
> + return ret;
I would do other way around, i.e. release resource followed up by printing
a message. Printing a message is a slow operation and may prevent the (soon
freed) resources to be re-used earlier.
> + }
...
> + spin_lock_irqsave(_hdlc->tx_lock, flags);
Why not using cleanup.h from day 1?
> +end:
This label, in particular, will not be needed with above in place.
> + spin_unlock_irqrestore(_hdlc->tx_lock, flags);
> + return ret;
> +}
...
> + /* Queue as many recv descriptors as possible */
> + for (i = 0; i < ARRAY_SIZE(qmc_hdlc->rx_descs); i++) {
> + desc = _hdlc->rx_descs[i];
> +
> + desc->netdev = netdev;
> + ret = qmc_hdlc_recv_queue(qmc_hdlc, desc,
> chan_param.hdlc.max_rx_buf_size);
> + if (ret) {
> + if (ret == -EBUSY && i != 0)
> + break; /* We use all the QMC chan capability */
> + goto free_desc;
> + }
Can be unfolded to
if (ret == -EBUSY && i)
break; /* We use all the QMC chan capability */
if (ret)
goto free_desc;
Easy to read and understand.
> + }
...
> +static int qmc_hdlc_probe(struct platform_device *pdev)
> +{
With
struct device *dev = >dev;
the below code will be neater (see other comments for the examples).
> + struct device_node *np = pdev->dev.of_node;
It is used only once, drop it (see below).
> + struct qmc_hdlc *qmc_hdlc;
> + struct qmc_chan_info info;
> + hdlc_device *hdlc;
> + int ret;
> +
> + qmc_hdlc = devm_kzalloc(>dev, sizeof(*qmc_hdlc), GFP_KERNEL);
> + if (!qmc_hdlc)
> + return -ENOMEM;
> +
> + qmc_hdlc->dev = >dev;
> + spin_lock_init(_hdlc->tx_lock);
> +
> + qmc_hdlc->qmc_chan = devm_qmc_chan_get_bychild(qmc_hdlc->dev, np);
qmc_hdlc->qmc_chan = devm_qmc_chan_get_bychild(dev, dev->of_node);
> + if (IS_ERR(qmc_hdlc->qmc_chan)) {
> + ret = PTR_ERR(qmc_hdlc->qmc_chan);
> + return dev_err_probe(qmc_hdlc->dev, ret, "get QMC channel
> failed\n");
return dev_err_probe(dev, PTR_ERR(qmc_hdlc->qmc_chan), "get QMC
channel failed\n");
> + }
> +
> + ret = qmc_chan_get_info(qmc_hdlc->qmc_chan, );
> + if (ret) {
> + dev_err(qmc_hdlc->dev, "get QMC channel info failed %d\n", ret);
> + return ret;
Why not using same message pattern everywhere, i.e. dev_err_probe()?
return dev_err_probe(dev, ret, "get QMC channel info failed\n");
(and so on...)
> + }
> +
> + if (info.mode != QMC_HDLC) {
> + dev_err(qmc_hdlc->dev, "QMC chan mode %d is not QMC_HDLC\n",
> + info.mode);
> + return -EINVAL;
> + }
> +
> + qmc_hdlc->netdev = alloc_hdlcdev(qmc_hdlc);
> + if (!qmc_hdlc->netdev) {
> + dev_err(qmc_hdlc->dev, "failed to alloc hdlc dev\n");
> + return -ENOMEM;
We do not issue a message for -ENOMEM.
> + }
> +
> + hdlc = dev_to_hdlc(qmc_hdlc->netdev);
> + hdlc->attach = qmc_hdlc_attach;
> + hdlc->xmit = qmc_hdlc_xmit;
> + SET_NETDEV_DEV(qmc_hdlc->netdev, qmc_hdlc->dev);
> + qmc_hdlc->netdev->tx_queue_len = ARRAY_SIZE(qmc_hdlc->tx_descs);
> + qmc_hdlc->netdev->netdev_ops = _hdlc_netdev_ops;
> + ret = register_hdlc_device(qmc_hdlc->netdev);
> + if (ret) {
> + dev_err(qmc_hdlc->dev, "failed to register hdlc device (%d)\n",
> ret);
> +
[PATCH v2 RESEND 4/5] s390/irq,nmi: include header directly
update_timer_sys() and update_timer_mcck() are inlines used for
CPU time accounting from the interrupt and machine-check handlers.
These routines are specific to s390 architecture, but included
via header implicitly. Avoid the extra loop and
include header directly.
Acked-by: Heiko Carstens
Signed-off-by: Alexander Gordeev
---
arch/s390/kernel/irq.c | 1 +
arch/s390/kernel/nmi.c | 1 +
2 files changed, 2 insertions(+)
diff --git a/arch/s390/kernel/irq.c b/arch/s390/kernel/irq.c
index 6f71b0ce1068..259496fe0ef9 100644
--- a/arch/s390/kernel/irq.c
+++ b/arch/s390/kernel/irq.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include "entry.h"
DEFINE_PER_CPU_SHARED_ALIGNED(struct irq_stat, irq_stat);
diff --git a/arch/s390/kernel/nmi.c b/arch/s390/kernel/nmi.c
index 9ad44c26d1a2..4422a27faace 100644
--- a/arch/s390/kernel/nmi.c
+++ b/arch/s390/kernel/nmi.c
@@ -32,6 +32,7 @@
#include
#include
#include
+#include
#include
struct mcck_struct {
--
2.40.1
[PATCH v2 RESEND 3/5] s390/vtime: remove unused __ARCH_HAS_VTIME_TASK_SWITCH leftover
__ARCH_HAS_VTIME_TASK_SWITCH macro is not used anymore.
Reviewed-by: Frederic Weisbecker
Acked-by: Heiko Carstens
Acked-by: Nicholas Piggin
Signed-off-by: Alexander Gordeev
---
arch/s390/include/asm/vtime.h | 2 --
1 file changed, 2 deletions(-)
diff --git a/arch/s390/include/asm/vtime.h b/arch/s390/include/asm/vtime.h
index fe17e448c0c5..561c91c1a87c 100644
--- a/arch/s390/include/asm/vtime.h
+++ b/arch/s390/include/asm/vtime.h
@@ -2,8 +2,6 @@
#ifndef _S390_VTIME_H
#define _S390_VTIME_H
-#define __ARCH_HAS_VTIME_TASK_SWITCH
-
static inline void update_timer_sys(void)
{
S390_lowcore.system_timer += S390_lowcore.last_update_timer -
S390_lowcore.exit_timer;
--
2.40.1
[PATCH v2 RESEND 2/5] sched/vtime: get rid of generic vtime_task_switch() implementation
The generic vtime_task_switch() implementation gets built only
if __ARCH_HAS_VTIME_TASK_SWITCH is not defined, but requires an
architecture to implement arch_vtime_task_switch() callback at
the same time, which is confusing.
Further, arch_vtime_task_switch() is implemented for 32-bit PowerPC
architecture only and vtime_task_switch() generic variant is rather
superfluous.
Simplify the whole vtime_task_switch() wiring by moving the existing
generic implementation to PowerPC.
Reviewed-by: Frederic Weisbecker
Reviewed-by: Nicholas Piggin
Signed-off-by: Alexander Gordeev
---
arch/powerpc/include/asm/cputime.h | 13 -
arch/powerpc/kernel/time.c | 22 ++
kernel/sched/cputime.c | 13 -
3 files changed, 22 insertions(+), 26 deletions(-)
diff --git a/arch/powerpc/include/asm/cputime.h
b/arch/powerpc/include/asm/cputime.h
index 4961fb38e438..aff858ca99c0 100644
--- a/arch/powerpc/include/asm/cputime.h
+++ b/arch/powerpc/include/asm/cputime.h
@@ -32,23 +32,10 @@
#ifdef CONFIG_PPC64
#define get_accounting(tsk)(_paca()->accounting)
#define raw_get_accounting(tsk)(_paca->accounting)
-static inline void arch_vtime_task_switch(struct task_struct *tsk) { }
#else
#define get_accounting(tsk)(_thread_info(tsk)->accounting)
#define raw_get_accounting(tsk)get_accounting(tsk)
-/*
- * Called from the context switch with interrupts disabled, to charge all
- * accumulated times to the current process, and to prepare accounting on
- * the next process.
- */
-static inline void arch_vtime_task_switch(struct task_struct *prev)
-{
- struct cpu_accounting_data *acct = get_accounting(current);
- struct cpu_accounting_data *acct0 = get_accounting(prev);
-
- acct->starttime = acct0->starttime;
-}
#endif
/*
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index df20cf201f74..c0fdc6d94fee 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -354,6 +354,28 @@ void vtime_flush(struct task_struct *tsk)
acct->hardirq_time = 0;
acct->softirq_time = 0;
}
+
+/*
+ * Called from the context switch with interrupts disabled, to charge all
+ * accumulated times to the current process, and to prepare accounting on
+ * the next process.
+ */
+void vtime_task_switch(struct task_struct *prev)
+{
+ if (is_idle_task(prev))
+ vtime_account_idle(prev);
+ else
+ vtime_account_kernel(prev);
+
+ vtime_flush(prev);
+
+ if (!IS_ENABLED(CONFIG_PPC64)) {
+ struct cpu_accounting_data *acct = get_accounting(current);
+ struct cpu_accounting_data *acct0 = get_accounting(prev);
+
+ acct->starttime = acct0->starttime;
+ }
+}
#endif /* CONFIG_VIRT_CPU_ACCOUNTING_NATIVE */
void __no_kcsan __delay(unsigned long loops)
diff --git a/kernel/sched/cputime.c b/kernel/sched/cputime.c
index af7952f12e6c..aa48b2ec879d 100644
--- a/kernel/sched/cputime.c
+++ b/kernel/sched/cputime.c
@@ -424,19 +424,6 @@ static inline void irqtime_account_process_tick(struct
task_struct *p, int user_
*/
#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE
-# ifndef __ARCH_HAS_VTIME_TASK_SWITCH
-void vtime_task_switch(struct task_struct *prev)
-{
- if (is_idle_task(prev))
- vtime_account_idle(prev);
- else
- vtime_account_kernel(prev);
-
- vtime_flush(prev);
- arch_vtime_task_switch(prev);
-}
-# endif
-
void vtime_account_irq(struct task_struct *tsk, unsigned int offset)
{
unsigned int pc = irq_count() - offset;
--
2.40.1
[PATCH v2 RESEND 1/5] sched/vtime: remove confusing arch_vtime_task_switch() declaration
Callback arch_vtime_task_switch() is only defined when CONFIG_VIRT_CPU_ACCOUNTING_NATIVE is selected. Yet, the function prototype forward declaration is present for CONFIG_VIRT_CPU_ACCOUNTING_GEN variant. Remove it. Reviewed-by: Frederic Weisbecker Reviewed-by: Nicholas Piggin Signed-off-by: Alexander Gordeev --- include/linux/vtime.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/linux/vtime.h b/include/linux/vtime.h index 3684487d01e1..593466ceebed 100644 --- a/include/linux/vtime.h +++ b/include/linux/vtime.h @@ -18,7 +18,6 @@ extern void vtime_account_idle(struct task_struct *tsk); #endif /* !CONFIG_VIRT_CPU_ACCOUNTING */ #ifdef CONFIG_VIRT_CPU_ACCOUNTING_GEN -extern void arch_vtime_task_switch(struct task_struct *tsk); extern void vtime_user_enter(struct task_struct *tsk); extern void vtime_user_exit(struct task_struct *tsk); extern void vtime_guest_enter(struct task_struct *tsk); -- 2.40.1
[PATCH v2 RESEND 5/5] sched/vtime: do not include header
There is no architecture-specific code or data left that generic needs to know about. Thus, avoid the inclusion of header. Reviewed-by: Frederic Weisbecker Acked-by: Nicholas Piggin Signed-off-by: Alexander Gordeev --- arch/powerpc/include/asm/Kbuild | 1 - include/asm-generic/vtime.h | 1 - include/linux/vtime.h | 4 3 files changed, 6 deletions(-) delete mode 100644 include/asm-generic/vtime.h diff --git a/arch/powerpc/include/asm/Kbuild b/arch/powerpc/include/asm/Kbuild index 61a8dcd7..e5fdc336c9b2 100644 --- a/arch/powerpc/include/asm/Kbuild +++ b/arch/powerpc/include/asm/Kbuild @@ -6,5 +6,4 @@ generic-y += agp.h generic-y += kvm_types.h generic-y += mcs_spinlock.h generic-y += qrwlock.h -generic-y += vtime.h generic-y += early_ioremap.h diff --git a/include/asm-generic/vtime.h b/include/asm-generic/vtime.h deleted file mode 100644 index b1a49677fe25.. --- a/include/asm-generic/vtime.h +++ /dev/null @@ -1 +0,0 @@ -/* no content, but patch(1) dislikes empty files */ diff --git a/include/linux/vtime.h b/include/linux/vtime.h index 593466ceebed..29dd5b91dd7d 100644 --- a/include/linux/vtime.h +++ b/include/linux/vtime.h @@ -5,10 +5,6 @@ #include #include -#ifdef CONFIG_VIRT_CPU_ACCOUNTING_NATIVE -#include -#endif - /* * Common vtime APIs */ -- 2.40.1
[PATCH v2 RESEND 0/5] sched/vtime: vtime.h headers cleanup
Hi All, There are no changes since the last post, just a re-send. v2: - patch 4: commit message reworded (Heiko) - patch 5: vtime.h is removed from Kbuild scripts (PowerPC only) (Heiko) v1: Please find a small cleanup to vtime_task_switch() wiring. I split it into smaller patches to allow separate PowerPC vs s390 reviews. Otherwise patches 2+3 and 4+5 could have been merged. I tested it on s390 and compile-tested it on 32- and 64-bit PowerPC and few other major architectures only, but it is only of concern for CONFIG_VIRT_CPU_ACCOUNTING_NATIVE-capable ones (AFAICT). Thanks! Alexander Gordeev (5): sched/vtime: remove confusing arch_vtime_task_switch() declaration sched/vtime: get rid of generic vtime_task_switch() implementation s390/vtime: remove unused __ARCH_HAS_VTIME_TASK_SWITCH leftover s390/irq,nmi: include header directly sched/vtime: do not include header arch/powerpc/include/asm/Kbuild| 1 - arch/powerpc/include/asm/cputime.h | 13 - arch/powerpc/kernel/time.c | 22 ++ arch/s390/include/asm/vtime.h | 2 -- arch/s390/kernel/irq.c | 1 + arch/s390/kernel/nmi.c | 1 + include/asm-generic/vtime.h| 1 - include/linux/vtime.h | 5 - kernel/sched/cputime.c | 13 - 9 files changed, 24 insertions(+), 35 deletions(-) delete mode 100644 include/asm-generic/vtime.h -- 2.40.1
[PATCH v4 5/5] net: wan: fsl_qmc_hdlc: Add framer support
Add framer support in the fsl_qmc_hdlc driver in order to be able to
signal carrier changes to the network stack based on the framer status
Also use this framer to provide information related to the E1/T1 line
interface on IF_GET_IFACE and configure the line interface according to
IF_IFACE_{E1,T1} information.
Signed-off-by: Herve Codina
Reviewed-by: Christophe Leroy
---
drivers/net/wan/fsl_qmc_hdlc.c | 239 -
1 file changed, 235 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index 1b7f1d5af273..5e80d3ce4e51 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -9,6 +9,7 @@
#include
#include
+#include
#include
#include
#include
@@ -28,6 +29,9 @@ struct qmc_hdlc {
struct device *dev;
struct qmc_chan *qmc_chan;
struct net_device *netdev;
+ struct framer *framer;
+ spinlock_t carrier_lock; /* Protect carrier detection */
+ struct notifier_block nb;
bool is_crc32;
spinlock_t tx_lock; /* Protect tx descriptors */
struct qmc_hdlc_desc tx_descs[8];
@@ -41,6 +45,195 @@ static struct qmc_hdlc *netdev_to_qmc_hdlc(struct
net_device *netdev)
return dev_to_hdlc(netdev)->priv;
}
+static int qmc_hdlc_framer_set_carrier(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ unsigned long flags;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ spin_lock_irqsave(_hdlc->carrier_lock, flags);
+
+ ret = framer_get_status(qmc_hdlc->framer, _status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto end;
+ }
+ if (framer_status.link_is_on)
+ netif_carrier_on(qmc_hdlc->netdev);
+ else
+ netif_carrier_off(qmc_hdlc->netdev);
+
+end:
+ spin_unlock_irqrestore(_hdlc->carrier_lock, flags);
+ return ret;
+}
+
+static int qmc_hdlc_framer_notifier(struct notifier_block *nb, unsigned long
action,
+ void *data)
+{
+ struct qmc_hdlc *qmc_hdlc = container_of(nb, struct qmc_hdlc, nb);
+ int ret;
+
+ if (action != FRAMER_EVENT_STATUS)
+ return NOTIFY_DONE;
+
+ ret = qmc_hdlc_framer_set_carrier(qmc_hdlc);
+ return ret ? NOTIFY_DONE : NOTIFY_OK;
+}
+
+static int qmc_hdlc_framer_start(struct qmc_hdlc *qmc_hdlc)
+{
+ struct framer_status framer_status;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_power_on(qmc_hdlc->framer);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer power-on failed (%d)\n", ret);
+ return ret;
+ }
+
+ /* Be sure that get_status is supported */
+ ret = framer_get_status(qmc_hdlc->framer, _status);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get framer status failed (%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ qmc_hdlc->nb.notifier_call = qmc_hdlc_framer_notifier;
+ ret = framer_notifier_register(qmc_hdlc->framer, _hdlc->nb);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "framer notifier register failed
(%d)\n", ret);
+ goto framer_power_off;
+ }
+
+ return 0;
+
+framer_power_off:
+ framer_power_off(qmc_hdlc->framer);
+ return ret;
+}
+
+static void qmc_hdlc_framer_stop(struct qmc_hdlc *qmc_hdlc)
+{
+ if (!qmc_hdlc->framer)
+ return;
+
+ framer_notifier_unregister(qmc_hdlc->framer, _hdlc->nb);
+ framer_power_off(qmc_hdlc->framer);
+}
+
+static int qmc_hdlc_framer_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface,
+const te1_settings *te1)
+{
+ struct framer_config config;
+ int ret;
+
+ if (!qmc_hdlc->framer)
+ return 0;
+
+ ret = framer_get_config(qmc_hdlc->framer, );
+ if (ret)
+ return ret;
+
+ switch (if_iface) {
+ case IF_IFACE_E1:
+ config.iface = FRAMER_IFACE_E1;
+ break;
+ case IF_IFACE_T1:
+ config.iface = FRAMER_IFACE_T1;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ switch (te1->clock_type) {
+ case CLOCK_DEFAULT:
+ /* Keep current value */
+ break;
+ case CLOCK_EXT:
+ config.clock_type = FRAMER_CLOCK_EXT;
+ break;
+ case CLOCK_INT:
+ config.clock_type = FRAMER_CLOCK_INT;
+ break;
+ default:
+ return -EINVAL;
+ }
+ config.line_clock_rate = te1->clock_rate;
+
+ return framer_set_config(qmc_hdlc->framer, );
+}
+
+static int qmc_hdlc_framer_get_iface(struct qmc_hdlc *qmc_hdlc, int *if_iface,
te1_settings *te1)
+{
+ struct framer_config config;
+ int
[PATCH v4 4/5] net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support
QMC channels support runtime timeslots changes but nothing is done at
the QMC HDLC driver to handle these changes.
Use existing IFACE ioctl in order to configure the timeslots to use.
Signed-off-by: Herve Codina
Reviewed-by: Christophe Leroy
Acked-by: Jakub Kicinski
---
drivers/net/wan/fsl_qmc_hdlc.c | 152 -
1 file changed, 151 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
index ec08ab217a72..1b7f1d5af273 100644
--- a/drivers/net/wan/fsl_qmc_hdlc.c
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -7,6 +7,7 @@
* Author: Herve Codina
*/
+#include
#include
#include
#include
@@ -32,6 +33,7 @@ struct qmc_hdlc {
struct qmc_hdlc_desc tx_descs[8];
unsigned int tx_out;
struct qmc_hdlc_desc rx_descs[4];
+ u32 slot_map;
};
static struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
@@ -206,6 +208,144 @@ static netdev_tx_t qmc_hdlc_xmit(struct sk_buff *skb,
struct net_device *netdev)
return ret;
}
+static int qmc_hdlc_xlate_slot_map(struct qmc_hdlc *qmc_hdlc,
+ u32 slot_map, struct qmc_chan_ts_info
*ts_info)
+{
+ DECLARE_BITMAP(ts_mask_avail, 64);
+ DECLARE_BITMAP(ts_mask, 64);
+ DECLARE_BITMAP(map, 64);
+
+ /* Tx and Rx available masks must be identical */
+ if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+ dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch
(0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+ return -EINVAL;
+ }
+
+ bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+ bitmap_from_u64(map, slot_map);
+ bitmap_scatter(ts_mask, map, ts_mask_avail, 64);
+
+ if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+ dev_err(qmc_hdlc->dev, "Cannot translate timeslots %*pb ->
(%*pb, %*pb)\n",
+ 64, map, 64, ts_mask_avail, 64, ts_mask);
+ return -EINVAL;
+ }
+
+ bitmap_to_arr64(_info->tx_ts_mask, ts_mask, 64);
+ ts_info->rx_ts_mask = ts_info->tx_ts_mask;
+ return 0;
+}
+
+static int qmc_hdlc_xlate_ts_info(struct qmc_hdlc *qmc_hdlc,
+ const struct qmc_chan_ts_info *ts_info, u32
*slot_map)
+{
+ DECLARE_BITMAP(ts_mask_avail, 64);
+ DECLARE_BITMAP(ts_mask, 64);
+ DECLARE_BITMAP(map, 64);
+ u32 array32[2];
+
+ /* Tx and Rx masks and available masks must be identical */
+ if (ts_info->rx_ts_mask_avail != ts_info->tx_ts_mask_avail) {
+ dev_err(qmc_hdlc->dev, "tx and rx available timeslots mismatch
(0x%llx, 0x%llx)\n",
+ ts_info->rx_ts_mask_avail, ts_info->tx_ts_mask_avail);
+ return -EINVAL;
+ }
+ if (ts_info->rx_ts_mask != ts_info->tx_ts_mask) {
+ dev_err(qmc_hdlc->dev, "tx and rx timeslots mismatch (0x%llx,
0x%llx)\n",
+ ts_info->rx_ts_mask, ts_info->tx_ts_mask);
+ return -EINVAL;
+ }
+
+ bitmap_from_u64(ts_mask_avail, ts_info->rx_ts_mask_avail);
+ bitmap_from_u64(ts_mask, ts_info->rx_ts_mask);
+ bitmap_gather(map, ts_mask, ts_mask_avail, 64);
+
+ if (bitmap_weight(ts_mask, 64) != bitmap_weight(map, 64)) {
+ dev_err(qmc_hdlc->dev, "Cannot translate timeslots (%*pb, %*pb)
-> %*pb\n",
+ 64, ts_mask_avail, 64, ts_mask, 64, map);
+ return -EINVAL;
+ }
+
+ bitmap_to_arr32(array32, map, 64);
+ if (array32[1]) {
+ dev_err(qmc_hdlc->dev, "Slot map out of 32bit (%*pb, %*pb) ->
%*pb\n",
+ 64, ts_mask_avail, 64, ts_mask, 64, map);
+ return -EINVAL;
+ }
+
+ *slot_map = array32[0];
+ return 0;
+}
+
+static int qmc_hdlc_set_iface(struct qmc_hdlc *qmc_hdlc, int if_iface, const
te1_settings *te1)
+{
+ struct qmc_chan_ts_info ts_info;
+ int ret;
+
+ ret = qmc_chan_get_ts_info(qmc_hdlc->qmc_chan, _info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "get QMC channel ts info failed %d\n",
ret);
+ return ret;
+ }
+ ret = qmc_hdlc_xlate_slot_map(qmc_hdlc, te1->slot_map, _info);
+ if (ret)
+ return ret;
+
+ ret = qmc_chan_set_ts_info(qmc_hdlc->qmc_chan, _info);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "set QMC channel ts info failed %d\n",
ret);
+ return ret;
+ }
+
+ qmc_hdlc->slot_map = te1->slot_map;
+
+ return 0;
+}
+
+static int qmc_hdlc_ioctl(struct net_device *netdev, struct if_settings *ifs)
+{
+ struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+ te1_settings te1;
+
+ switch (ifs->type) {
+ case IF_GET_IFACE:
+ ifs->type = IF_IFACE_E1;
+ if (ifs->size <
[PATCH v4 3/5] lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers
From: Andy Shevchenko
These helpers scatters or gathers a bitmap with the help of the mask
position bits parameter.
bitmap_scatter() does the following:
src: 01011010
||
+--+|
| ++
| |++|||
| || +-+||
| || | ||
mask: ...v..vv...v..vv
...0..11...0..10
dst: 00110010
and bitmap_gather() performs this one:
mask: ...v..vv...v..vv
src: 00110010
^ ^^ ^ 0
| || | 10
| || > 010
| |+--> 1010
| +--> 11010
+> 011010
dst: 00011010
bitmap_gather() can the seen as the reverse bitmap_scatter() operation.
The original work was done by Andy Shevchenko.
Signed-off-by: Andy Shevchenko
Link:
https://lore.kernel.org/lkml/20230926052007.3917389-3-andriy.shevche...@linux.intel.com/
Signed-off-by: Herve Codina
---
Yury, Andy, I hope I correctly took into accounts the comments
reveived on Andy's v1 series and found good compromise to satisfy
your different point of view.
include/linux/bitmap.h | 101 +
lib/test_bitmap.c | 42 +
2 files changed, 143 insertions(+)
diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
index 99451431e4d6..a171030ff71c 100644
--- a/include/linux/bitmap.h
+++ b/include/linux/bitmap.h
@@ -62,6 +62,8 @@ struct device;
* bitmap_shift_left(dst, src, n, nbits) *dst = *src << n
* bitmap_cut(dst, src, first, n, nbits) Cut n bits from first, copy
rest
* bitmap_replace(dst, old, new, mask, nbits) *dst = (*old & ~(*mask)) |
(*new & *mask)
+ * bitmap_scatter(dst, src, mask, nbits) *dst = map(dense, sparse)(src)
+ * bitmap_gather(dst, src, mask, nbits) *dst = map(sparse, dense)(src)
* bitmap_remap(dst, src, old, new, nbits) *dst = map(old, new)(src)
* bitmap_bitremap(oldbit, old, new, nbits)newbit = map(old, new)(oldbit)
* bitmap_onto(dst, orig, relmap, nbits) *dst = orig relative to relmap
@@ -487,6 +489,105 @@ static inline void bitmap_replace(unsigned long *dst,
__bitmap_replace(dst, old, new, mask, nbits);
}
+/**
+ * bitmap_scatter - Scatter a bitmap according to the given mask
+ * @dst: scattered bitmap
+ * @src: gathered bitmap
+ * @mask: mask representing bits to assign to in the scattered bitmap
+ * @nbits: number of bits in each of these bitmaps
+ *
+ * Scatters bitmap with sequential bits according to the given @mask.
+ *
+ * Example:
+ * If @src bitmap = 0x005a, with @mask = 0x1313, @dst will be 0x0302.
+ *
+ * Or in binary form
+ * @src@mask @dst
+ * 01011010000100110001001100110010
+ *
+ * (Bits 0, 1, 2, 3, 4, 5 are copied to the bits 0, 1, 4, 8, 9, 12)
+ *
+ * A more 'visual' description of the operation:
+ * src: 01011010
+ * ||
+ * +--+|
+ * | ++
+ * | |++|||
+ * | || +-+||
+ * | || | ||
+ * mask: ...v..vv...v..vv
+ * ...0..11...0..10
+ * dst: 00110010
+ *
+ * A relationship exists between bitmap_scatter() and bitmap_gather().
+ * bitmap_gather() can be seen as the 'reverse' bitmap_scatter() operation.
+ * See bitmap_scatter() for details related to this relationship.
+ */
+static inline void bitmap_scatter(unsigned long *dst, const unsigned long *src,
+ const unsigned long *mask, unsigned int nbits)
+{
+ unsigned int n = 0;
+ unsigned int bit;
+
+ bitmap_zero(dst, nbits);
+
+ for_each_set_bit(bit, mask, nbits)
+ __assign_bit(bit, dst, test_bit(n++, src));
+}
+
+/**
+ * bitmap_gather - Gather a bitmap according to given mask
+ * @dst: gathered bitmap
+ * @src: scattered bitmap
+ * @mask: mask representing bits to extract from in the scattered bitmap
+ * @nbits: number of bits in each of these bitmaps
+ *
+ * Gathers bitmap with sparse bits according to the given @mask.
+ *
+ * Example:
+ * If @src bitmap = 0x0302, with @mask = 0x1313, @dst will be 0x001a.
+ *
+ * Or in binary form
+ * @src@mask @dst
+ * 00110010000100110001001100011010
+ *
+ * (Bits 0, 1, 4, 8, 9, 12 are copied to the bits 0, 1, 2, 3, 4, 5)
+ *
+ * A more 'visual' description of the operation:
+ * mask: ...v..vv...v..vv
+ * src: 00110010
+ * ^ ^^ ^ 0
+ * | || | 10
+ * | || > 010
+ * | |+--> 1010
+ * | +--> 11010
+ * +> 011010
+ * dst: 00011010
+ *
+ * A relationship exists between bitmap_gather() and bitmap_scatter() (See
+ * bitmap_scatter() for the bitmap scatter detailed operations).
+ * Suppose scattered computed using bitmap_scatter(scattered, src, mask, n).
+ * The operation bitmap_gather(result, scattered,
[PATCH v4 2/5] MAINTAINERS: Add the Freescale QMC HDLC driver entry
After contributing the driver, add myself as the maintainer for the Freescale QMC HDLC driver. Signed-off-by: Herve Codina --- MAINTAINERS | 7 +++ 1 file changed, 7 insertions(+) diff --git a/MAINTAINERS b/MAINTAINERS index 8d1052fa6a69..15cd3a8e5866 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -8584,6 +8584,13 @@ F: Documentation/devicetree/bindings/soc/fsl/cpm_qe/fsl,cpm1-scc-qmc.yaml F: drivers/soc/fsl/qe/qmc.c F: include/soc/fsl/qe/qmc.h +FREESCALE QUICC ENGINE QMC HDLC DRIVER +M: Herve Codina +L: net...@vger.kernel.org +L: linuxppc-dev@lists.ozlabs.org +S: Maintained +F: drivers/net/wan/fsl_qmc_hdlc.c + FREESCALE QUICC ENGINE TSA DRIVER M: Herve Codina L: linuxppc-dev@lists.ozlabs.org -- 2.43.0
[PATCH v4 0/5] Add support for QMC HDLC
Hi,
This series introduces the QMC HDLC support.
Patches were previously sent as part of a full feature series and were
previously reviewed in that context:
"Add support for QMC HDLC, framer infrastructure and PEF2256 framer" [1]
In order to ease the merge, the full feature series has been split and
needed parts were merged in v6.8-rc1:
- "Prepare the PowerQUICC QMC and TSA for the HDLC QMC driver" [2]
- "Add support for framer infrastructure and PEF2256 framer" [3]
This series contains patches related to the QMC HDLC part (QMC HDLC
driver):
- Introduce the QMC HDLC driver (patches 1 and 2)
- Add timeslots change support in QMC HDLC (patch 3)
- Add framer support as a framer consumer in QMC HDLC (patch 4)
Compare to the original full feature series, a modification was done on
patch 3 in order to use a coherent prefix in the commit title.
I kept the patches unsquashed as they were previously sent and reviewed.
Of course, I can squash them if needed.
Compared to the previous iteration:
https://lore.kernel.org/linux-kernel/20240212075646.19114-1-herve.cod...@bootlin.com/
this v4 series mainly:
- Introduces and use bitmap_{scatter,gather}().
Best regards,
Hervé
[1]:
https://lore.kernel.org/linux-kernel/20231115144007.478111-1-herve.cod...@bootlin.com/
[2]:
https://lore.kernel.org/linux-kernel/20231205152116.122512-1-herve.cod...@bootlin.com/
[3]:
https://lore.kernel.org/linux-kernel/20231128132534.258459-1-herve.cod...@bootlin.com/
Changes v3 -> v4
- Patch 1
Remove of.h and of_platform.h includes, add mod_devicetable.h.
Add a blank line in the includes list.
- Path 2
No changes.
- v3 patches 3 and 4 removed
- Patch 3 (new patch in v4)
Introduce bitmap_{scatter,gather}() based on the original patch done
by Andy Shevchenko.
Address comments already done on the original patch:
https://lore.kernel.org/lkml/20230926052007.3917389-3-andriy.shevche...@linux.intel.com/
- Removed the returned values.
- Used 'unsigned int' for all indexes.
- Added a 'visual' description of the operations in kernel-doc.
- Described the relationship between bitmap_scatter() and
bitmap_gather().
- Moved bitmap_{scatter,gather}() to the bitmap.h file.
- Improved bitmap_{scatter,gather}() test.
- Reworked the commit log.
- Patch 4 (v3 patch 5)
Use bitmap_{scatter,gather}()
- Patches 5 (v3 patch 6)
No changes.
Changes v2 -> v3
- Patch 1
Remove 'inline' function specifier from .c file.
Fix a bug introduced when added WARN_ONCE(). The warn condition must
be desc->skb (descriptor used) instead of !desc->skb.
Remove a lock/unlock section locking the entire qmc_hdlc_xmit()
function.
- Patch 5
Use bitmap_from_u64() everywhere instead of bitmap_from_arr32() and
bitmap_from_arr64().
Changes v1 -> v2
- Patch 1
Use the same qmc_hdlc initialisation in qmc_hcld_recv_complete()
than the one present in qmc_hcld_xmit_complete().
Use WARN_ONCE()
- Patch 3 (new patch in v2)
Make bitmap_onto() available to users
- Patch 4 (new patch in v2)
Introduce bitmap_off()
- Patch 5 (patch 3 in v1)
Use bitmap_*() functions
- Patch 6 (patch 4 in v1)
No changes
Changes compare to the full feature series:
- Patch 3
Use 'net: wan: fsl_qmc_hdlc:' as commit title prefix
Patches extracted:
- Patch 1 : full feature series patch 7
- Patch 2 : full feature series patch 8
- Patch 3 : full feature series patch 20
- Patch 4 : full feature series patch 27
Andy Shevchenko (1):
lib/bitmap: Introduce bitmap_scatter() and bitmap_gather() helpers
Herve Codina (4):
net: wan: Add support for QMC HDLC
MAINTAINERS: Add the Freescale QMC HDLC driver entry
net: wan: fsl_qmc_hdlc: Add runtime timeslots changes support
net: wan: fsl_qmc_hdlc: Add framer support
MAINTAINERS| 7 +
drivers/net/wan/Kconfig| 12 +
drivers/net/wan/Makefile | 1 +
drivers/net/wan/fsl_qmc_hdlc.c | 807 +
include/linux/bitmap.h | 101 +
lib/test_bitmap.c | 42 ++
6 files changed, 970 insertions(+)
create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
--
2.43.0
[PATCH v4 1/5] net: wan: Add support for QMC HDLC
The QMC HDLC driver provides support for HDLC using the QMC (QUICC
Multichannel Controller) to transfer the HDLC data.
Signed-off-by: Herve Codina
Reviewed-by: Christophe Leroy
Acked-by: Jakub Kicinski
---
drivers/net/wan/Kconfig| 12 +
drivers/net/wan/Makefile | 1 +
drivers/net/wan/fsl_qmc_hdlc.c | 426 +
3 files changed, 439 insertions(+)
create mode 100644 drivers/net/wan/fsl_qmc_hdlc.c
diff --git a/drivers/net/wan/Kconfig b/drivers/net/wan/Kconfig
index 7dda87756d3f..31ab2136cdf1 100644
--- a/drivers/net/wan/Kconfig
+++ b/drivers/net/wan/Kconfig
@@ -197,6 +197,18 @@ config FARSYNC
To compile this driver as a module, choose M here: the
module will be called farsync.
+config FSL_QMC_HDLC
+ tristate "Freescale QMC HDLC support"
+ depends on HDLC
+ depends on CPM_QMC
+ help
+ HDLC support using the Freescale QUICC Multichannel Controller (QMC).
+
+ To compile this driver as a module, choose M here: the
+ module will be called fsl_qmc_hdlc.
+
+ If unsure, say N.
+
config FSL_UCC_HDLC
tristate "Freescale QUICC Engine HDLC support"
depends on HDLC
diff --git a/drivers/net/wan/Makefile b/drivers/net/wan/Makefile
index 8119b49d1da9..00e9b7ee1e01 100644
--- a/drivers/net/wan/Makefile
+++ b/drivers/net/wan/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_WANXL) += wanxl.o
obj-$(CONFIG_PCI200SYN)+= pci200syn.o
obj-$(CONFIG_PC300TOO) += pc300too.o
obj-$(CONFIG_IXP4XX_HSS) += ixp4xx_hss.o
+obj-$(CONFIG_FSL_QMC_HDLC) += fsl_qmc_hdlc.o
obj-$(CONFIG_FSL_UCC_HDLC) += fsl_ucc_hdlc.o
obj-$(CONFIG_SLIC_DS26522) += slic_ds26522.o
diff --git a/drivers/net/wan/fsl_qmc_hdlc.c b/drivers/net/wan/fsl_qmc_hdlc.c
new file mode 100644
index ..ec08ab217a72
--- /dev/null
+++ b/drivers/net/wan/fsl_qmc_hdlc.c
@@ -0,0 +1,426 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Freescale QMC HDLC Device Driver
+ *
+ * Copyright 2023 CS GROUP France
+ *
+ * Author: Herve Codina
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include
+
+struct qmc_hdlc_desc {
+ struct net_device *netdev;
+ struct sk_buff *skb; /* NULL if the descriptor is not in use */
+ dma_addr_t dma_addr;
+ size_t dma_size;
+};
+
+struct qmc_hdlc {
+ struct device *dev;
+ struct qmc_chan *qmc_chan;
+ struct net_device *netdev;
+ bool is_crc32;
+ spinlock_t tx_lock; /* Protect tx descriptors */
+ struct qmc_hdlc_desc tx_descs[8];
+ unsigned int tx_out;
+ struct qmc_hdlc_desc rx_descs[4];
+};
+
+static struct qmc_hdlc *netdev_to_qmc_hdlc(struct net_device *netdev)
+{
+ return dev_to_hdlc(netdev)->priv;
+}
+
+static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc
*desc, size_t size);
+
+#define QMC_HDLC_RX_ERROR_FLAGS (QMC_RX_FLAG_HDLC_OVF | \
+QMC_RX_FLAG_HDLC_UNA | \
+QMC_RX_FLAG_HDLC_ABORT | \
+QMC_RX_FLAG_HDLC_CRC)
+
+static void qmc_hcld_recv_complete(void *context, size_t length, unsigned int
flags)
+{
+ struct qmc_hdlc_desc *desc = context;
+ struct net_device *netdev = desc->netdev;
+ struct qmc_hdlc *qmc_hdlc = netdev_to_qmc_hdlc(netdev);
+ int ret;
+
+ dma_unmap_single(qmc_hdlc->dev, desc->dma_addr, desc->dma_size,
DMA_FROM_DEVICE);
+
+ if (flags & QMC_HDLC_RX_ERROR_FLAGS) {
+ netdev->stats.rx_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_OVF) /* Data overflow */
+ netdev->stats.rx_over_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_UNA) /* bits received not multiple
of 8 */
+ netdev->stats.rx_frame_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_ABORT) /* Received an abort
sequence */
+ netdev->stats.rx_frame_errors++;
+ if (flags & QMC_RX_FLAG_HDLC_CRC) /* CRC error */
+ netdev->stats.rx_crc_errors++;
+ kfree_skb(desc->skb);
+ } else {
+ netdev->stats.rx_packets++;
+ netdev->stats.rx_bytes += length;
+
+ skb_put(desc->skb, length);
+ desc->skb->protocol = hdlc_type_trans(desc->skb, netdev);
+ netif_rx(desc->skb);
+ }
+
+ /* Re-queue a transfer using the same descriptor */
+ ret = qmc_hdlc_recv_queue(qmc_hdlc, desc, desc->dma_size);
+ if (ret) {
+ dev_err(qmc_hdlc->dev, "queue recv desc failed (%d)\n", ret);
+ netdev->stats.rx_errors++;
+ }
+}
+
+static int qmc_hdlc_recv_queue(struct qmc_hdlc *qmc_hdlc, struct qmc_hdlc_desc
*desc, size_t size)
+{
+ int ret;
+
+ desc->skb = dev_alloc_skb(size);
+ if (!desc->skb)
+ return -ENOMEM;
+
+ desc->dma_size = size;
+
Re: [PATCHv9 2/3] irq: use a struct for the kstat_irqs in the interrupt descriptor
On Thu, Feb 22 2024 at 17:34, Bitao Hu wrote:
First of all the subsystem prefix is 'genirq:'. 'git log kernel/irq/'
gives you a pretty good hint. It's documented
Secondly the subject line does not match what this patch is about. It's
not about using a struct, it's about providing a snapshot mechanism, no?
> The current implementation uses an int for the kstat_irqs in the
> interrupt descriptor.
>
> However, we need to know the number of interrupts which happened
> since softlockup detection took a snapshot in order to analyze
> the problem caused by an interrupt storm.
>
> Replacing an int with a struct and providing sensible interfaces
> for the watchdog code can keep it self contained to the interrupt
> core code.
So something like this makes a useful change log for this:
Subject: genirq: Provide a snapshot mechanism for interrupt statistics
The soft lockup detector lacks a mechanism to identify interrupt storms
as root cause of a lockup. To enable this the detector needs a
mechanism to snapshot the interrupt count statistics on a CPU when the
detector observes a potential lockup scenario and compare that against
the interrupt count when it warns about the lockup later on. The number
of interrupts in that period give a hint whether the lockup might be
caused by an interrupt storm.
Instead of having extra storage in the lockup detector and accessing
the internals of the interrupt descriptor directly, convert the per CPU
irq_desc::kstat_irq member to a data structure which contains the
counter plus a snapshot member and provide interfaces to take a
snapshot of all interrupts on the current CPU and to retrieve the delta
of a specific interrupt later on.
Hmm?
> Signed-off-by: Bitao Hu
Interesting. You fully authored the patch?
That's not how it works. You cannot take work from others and claim that
it is yours. The minimal courtesy is to add a 'Originally-by:' tag.
> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
> index 623b8136e9af..3ad40cf30c66 100644
> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -488,18 +488,15 @@ int show_interrupts(struct seq_file *p, void *v)
> if (!desc || irq_settings_is_hidden(desc))
> goto outsparse;
>
> - if (desc->kstat_irqs) {
> - for_each_online_cpu(j)
> - any_count |= data_race(*per_cpu_ptr(desc->kstat_irqs,
> j));
> - }
> + if (desc->kstat_irqs)
> + any_count = data_race(desc->tot_count);
This is an unrelated change and needs to be split out into a separate
patch with a proper changelog which explains why this is equivalent.
Thanks,
tglx
Re: [PATCH v3 RESEND 1/6] net: wan: Add support for QMC HDLC
On Thu, 22 Feb 2024 15:19:11 +0200 Andy Shevchenko wrote: > On Thu, Feb 22, 2024 at 01:05:16PM +0100, Herve Codina wrote: > > On Mon, 12 Feb 2024 14:22:56 +0200 > > Andy Shevchenko wrote: > > ... > > > > > +#include > > > > +#include > > > > +#include > > > > > > > +#include > > > > +#include > > > > > > I do not see how these are being used, am I right? > > > What's is missing OTOH is the mod_devicetable.h. > > > > Agree for removing of.h and of_platform.h. > > > > Why do I need mod_devicetable.h ? > > Isn't including module.h enough ? > > In that header the definitions of many of ID table data structures are > located. > You are using that in the code. > Ok, thanks. Hervé
Re: [PATCH v3 RESEND 1/6] net: wan: Add support for QMC HDLC
On Thu, Feb 22, 2024 at 01:05:16PM +0100, Herve Codina wrote: > On Mon, 12 Feb 2024 14:22:56 +0200 > Andy Shevchenko wrote: ... > > > +#include > > > +#include > > > +#include > > > > > +#include > > > +#include > > > > I do not see how these are being used, am I right? > > What's is missing OTOH is the mod_devicetable.h. > > Agree for removing of.h and of_platform.h. > > Why do I need mod_devicetable.h ? > Isn't including module.h enough ? In that header the definitions of many of ID table data structures are located. You are using that in the code. -- With Best Regards, Andy Shevchenko
Re: [PATCH v3 RESEND 1/6] net: wan: Add support for QMC HDLC
Hi Andy, On Mon, 12 Feb 2024 14:22:56 +0200 Andy Shevchenko wrote: ... > > > +#include > > +#include > > +#include > > > +#include > > +#include > > I do not see how these are being used, am I right? > What's is missing OTOH is the mod_devicetable.h. Agree for removing of.h and of_platform.h. Why do I need mod_devicetable.h ? Isn't including module.h enough ? Best regards, Hervé
[PATCHv9 3/3] watchdog/softlockup: report the most frequent interrupts
When the watchdog determines that the current soft lockup is due
to an interrupt storm based on CPU utilization, reporting the
most frequent interrupts could be good enough for further
troubleshooting.
Below is an example of interrupt storm. The call tree does not
provide useful information, but we can analyze which interrupt
caused the soft lockup by comparing the counts of interrupts.
[ 638.870231] watchdog: BUG: soft lockup - CPU#9 stuck for 26s! [swapper/9:0]
[ 638.870825] CPU#9 Utilization every 4s during lockup:
[ 638.871194] #1: 0% system, 0% softirq, 100% hardirq, 0%
idle
[ 638.871652] #2: 0% system, 0% softirq, 100% hardirq, 0%
idle
[ 638.872107] #3: 0% system, 0% softirq, 100% hardirq, 0%
idle
[ 638.872563] #4: 0% system, 0% softirq, 100% hardirq, 0%
idle
[ 638.873018] #5: 0% system, 0% softirq, 100% hardirq, 0%
idle
[ 638.873494] CPU#9 Detect HardIRQ Time exceeds 50%. Most frequent HardIRQs:
[ 638.873994] #1: 330945 irq#7
[ 638.874236] #2: 31 irq#82
[ 638.874493] #3: 10 irq#10
[ 638.874744] #4: 2 irq#89
[ 638.874992] #5: 1 irq#102
...
[ 638.875313] Call trace:
[ 638.875315] __do_softirq+0xa8/0x364
Signed-off-by: Bitao Hu
---
kernel/watchdog.c | 115 --
1 file changed, 111 insertions(+), 4 deletions(-)
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 69e72d7e461d..c9d49ae8d045 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -12,22 +12,25 @@
#define pr_fmt(fmt) "watchdog: " fmt
-#include
#include
-#include
#include
+#include
+#include
#include
+#include
#include
+#include
#include
+#include
+#include
#include
#include
+
#include
#include
#include
-#include
#include
-#include
static DEFINE_MUTEX(watchdog_mutex);
@@ -417,13 +420,104 @@ static void print_cpustat(void)
}
}
+#define HARDIRQ_PERCENT_THRESH 50
+#define NUM_HARDIRQ_REPORT 5
+struct irq_counts {
+ int irq;
+ u32 counts;
+};
+
+static DEFINE_PER_CPU(bool, snapshot_taken);
+
+/* Tabulate the most frequent interrupts. */
+static void tabulate_irq_count(struct irq_counts *irq_counts, int irq, u32
counts, int rank)
+{
+ int i;
+ struct irq_counts new_count = {irq, counts};
+
+ for (i = 0; i < rank; i++) {
+ if (counts > irq_counts[i].counts)
+ swap(new_count, irq_counts[i]);
+ }
+}
+
+/*
+ * If the hardirq time exceeds HARDIRQ_PERCENT_THRESH% of the sample_period,
+ * then the cause of softlockup might be interrupt storm. In this case, it
+ * would be useful to start interrupt counting.
+ */
+static bool need_counting_irqs(void)
+{
+ u8 util;
+ int tail = __this_cpu_read(cpustat_tail);
+
+ tail = (tail + NUM_HARDIRQ_REPORT - 1) % NUM_HARDIRQ_REPORT;
+ util = __this_cpu_read(cpustat_util[tail][STATS_HARDIRQ]);
+ return util > HARDIRQ_PERCENT_THRESH;
+}
+
+static void start_counting_irqs(void)
+{
+ if (!__this_cpu_read(snapshot_taken)) {
+ kstat_snapshot_irqs();
+ __this_cpu_write(snapshot_taken, true);
+ }
+}
+
+static void stop_counting_irqs(void)
+{
+ __this_cpu_write(snapshot_taken, false);
+}
+
+static void print_irq_counts(void)
+{
+ unsigned int i, count;
+ struct irq_counts irq_counts_sorted[NUM_HARDIRQ_REPORT] = {
+ {-1, 0}, {-1, 0}, {-1, 0}, {-1, 0}, {-1, 0}
+ };
+
+ if (__this_cpu_read(snapshot_taken)) {
+ for_each_active_irq(i) {
+ count = kstat_get_irq_since_snapshot(i);
+ tabulate_irq_count(irq_counts_sorted, i, count,
NUM_HARDIRQ_REPORT);
+ }
+
+ /*
+* We do not want the "watchdog: " prefix on every line,
+* hence we use "printk" instead of "pr_crit".
+*/
+ printk(KERN_CRIT "CPU#%d Detect HardIRQ Time exceeds %d%%. Most
frequent HardIRQs:\n",
+ smp_processor_id(), HARDIRQ_PERCENT_THRESH);
+
+ for (i = 0; i < NUM_HARDIRQ_REPORT; i++) {
+ if (irq_counts_sorted[i].irq == -1)
+ break;
+
+ printk(KERN_CRIT "\t#%u: %-10u\tirq#%d\n",
+ i + 1, irq_counts_sorted[i].counts,
+ irq_counts_sorted[i].irq);
+ }
+
+ /*
+* If the hardirq time is less than HARDIRQ_PERCENT_THRESH% in
the last
+* sample_period, then we suspect the interrupt storm might be
subsiding.
+*/
+ if (!need_counting_irqs())
+ stop_counting_irqs();
+ }
+}
+
static void report_cpu_status(void)
{
print_cpustat();
+ print_irq_counts();
}
#else
static
[PATCHv9 2/3] irq: use a struct for the kstat_irqs in the interrupt descriptor
The current implementation uses an int for the kstat_irqs in the
interrupt descriptor.
However, we need to know the number of interrupts which happened
since softlockup detection took a snapshot in order to analyze
the problem caused by an interrupt storm.
Replacing an int with a struct and providing sensible interfaces
for the watchdog code can keep it self contained to the interrupt
core code.
Signed-off-by: Bitao Hu
---
arch/mips/dec/setup.c| 2 +-
arch/parisc/kernel/smp.c | 2 +-
arch/powerpc/kvm/book3s_hv_rm_xics.c | 2 +-
include/linux/irqdesc.h | 9 ++--
include/linux/kernel_stat.h | 3 +++
kernel/irq/internals.h | 2 +-
kernel/irq/irqdesc.c | 34 ++--
kernel/irq/proc.c| 9 +++-
scripts/gdb/linux/interrupts.py | 6 ++---
9 files changed, 47 insertions(+), 22 deletions(-)
diff --git a/arch/mips/dec/setup.c b/arch/mips/dec/setup.c
index 6c3704f51d0d..87f0a1436bf9 100644
--- a/arch/mips/dec/setup.c
+++ b/arch/mips/dec/setup.c
@@ -756,7 +756,7 @@ void __init arch_init_irq(void)
NULL))
pr_err("Failed to register fpu interrupt\n");
desc_fpu = irq_to_desc(irq_fpu);
- fpu_kstat_irq = this_cpu_ptr(desc_fpu->kstat_irqs);
+ fpu_kstat_irq = this_cpu_ptr(_fpu->kstat_irqs->cnt);
}
if (dec_interrupt[DEC_IRQ_CASCADE] >= 0) {
if (request_irq(dec_interrupt[DEC_IRQ_CASCADE], no_action,
diff --git a/arch/parisc/kernel/smp.c b/arch/parisc/kernel/smp.c
index 444154271f23..800eb64e91ad 100644
--- a/arch/parisc/kernel/smp.c
+++ b/arch/parisc/kernel/smp.c
@@ -344,7 +344,7 @@ static int smp_boot_one_cpu(int cpuid, struct task_struct
*idle)
struct irq_desc *desc = irq_to_desc(i);
if (desc && desc->kstat_irqs)
- *per_cpu_ptr(desc->kstat_irqs, cpuid) = 0;
+ *per_cpu_ptr(desc->kstat_irqs, cpuid) = (struct
irqstat) { };
}
#endif
diff --git a/arch/powerpc/kvm/book3s_hv_rm_xics.c
b/arch/powerpc/kvm/book3s_hv_rm_xics.c
index e42984878503..f2636414d82a 100644
--- a/arch/powerpc/kvm/book3s_hv_rm_xics.c
+++ b/arch/powerpc/kvm/book3s_hv_rm_xics.c
@@ -837,7 +837,7 @@ static inline void this_cpu_inc_rm(unsigned int __percpu
*addr)
*/
static void kvmppc_rm_handle_irq_desc(struct irq_desc *desc)
{
- this_cpu_inc_rm(desc->kstat_irqs);
+ this_cpu_inc_rm(>kstat_irqs->cnt);
__this_cpu_inc(kstat.irqs_sum);
}
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index d9451d456a73..2912b1998670 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -17,6 +17,11 @@ struct irq_desc;
struct irq_domain;
struct pt_regs;
+struct irqstat {
+ unsigned intcnt;
+ unsigned intref;
+};
+
/**
* struct irq_desc - interrupt descriptor
* @irq_common_data: per irq and chip data passed down to chip functions
@@ -55,7 +60,7 @@ struct pt_regs;
struct irq_desc {
struct irq_common_data irq_common_data;
struct irq_data irq_data;
- unsigned int __percpu *kstat_irqs;
+ struct irqstat __percpu *kstat_irqs;
irq_flow_handler_t handle_irq;
struct irqaction*action;/* IRQ action list */
unsigned intstatus_use_accessors;
@@ -119,7 +124,7 @@ extern struct irq_desc irq_desc[NR_IRQS];
static inline unsigned int irq_desc_kstat_cpu(struct irq_desc *desc,
unsigned int cpu)
{
- return desc->kstat_irqs ? *per_cpu_ptr(desc->kstat_irqs, cpu) : 0;
+ return desc->kstat_irqs ? per_cpu(desc->kstat_irqs->cnt, cpu) : 0;
}
static inline struct irq_desc *irq_data_to_desc(struct irq_data *data)
diff --git a/include/linux/kernel_stat.h b/include/linux/kernel_stat.h
index 9935f7ecbfb9..98b3043ea5e6 100644
--- a/include/linux/kernel_stat.h
+++ b/include/linux/kernel_stat.h
@@ -79,6 +79,9 @@ static inline unsigned int kstat_cpu_softirqs_sum(int cpu)
return sum;
}
+extern void kstat_snapshot_irqs(void);
+extern unsigned int kstat_get_irq_since_snapshot(unsigned int irq);
+
/*
* Number of interrupts per specific IRQ source, since bootup
*/
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index bcc7f21db9ee..1d92532c2aae 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -258,7 +258,7 @@ static inline void irq_state_set_masked(struct irq_desc
*desc)
static inline void __kstat_incr_irqs_this_cpu(struct irq_desc *desc)
{
- __this_cpu_inc(*desc->kstat_irqs);
+ __this_cpu_inc(desc->kstat_irqs->cnt);
__this_cpu_inc(kstat.irqs_sum);
}
diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c
index 27ca1c866f29..9cd17080b2d8 100644
--- a/kernel/irq/irqdesc.c
+++ b/kernel/irq/irqdesc.c
@@ -122,7 +122,7 @@ static void
[PATCHv9 1/3] watchdog/softlockup: low-overhead detection of interrupt storm
The following softlockup is caused by interrupt storm, but it cannot be
identified from the call tree. Because the call tree is just a snapshot
and doesn't fully capture the behavior of the CPU during the soft lockup.
watchdog: BUG: soft lockup - CPU#28 stuck for 23s! [fio:83921]
...
Call trace:
__do_softirq+0xa0/0x37c
__irq_exit_rcu+0x108/0x140
irq_exit+0x14/0x20
__handle_domain_irq+0x84/0xe0
gic_handle_irq+0x80/0x108
el0_irq_naked+0x50/0x58
Therefore，I think it is necessary to report CPU utilization during the
softlockup_thresh period (report once every sample_period, for a total
of 5 reportings), like this:
watchdog: BUG: soft lockup - CPU#28 stuck for 23s! [fio:83921]
CPU#28 Utilization every 4s during lockup:
#1: 0% system, 0% softirq, 100% hardirq, 0% idle
#2: 0% system, 0% softirq, 100% hardirq, 0% idle
#3: 0% system, 0% softirq, 100% hardirq, 0% idle
#4: 0% system, 0% softirq, 100% hardirq, 0% idle
#5: 0% system, 0% softirq, 100% hardirq, 0% idle
...
This would be helpful in determining whether an interrupt storm has
occurred or in identifying the cause of the softlockup. The criteria for
determination are as follows:
a. If the hardirq utilization is high, then interrupt storm should be
considered and the root cause cannot be determined from the call tree.
b. If the softirq utilization is high, then we could analyze the call
tree but it may cannot reflect the root cause.
c. If the system utilization is high, then we could analyze the root
cause from the call tree.
The mechanism requires a considerable amount of global storage space
when configured for the maximum number of CPUs. Therefore, adding a
SOFTLOCKUP_DETECTOR_INTR_STORM Kconfig knob that defaults to "yes"
if the max number of CPUs is <= 128.
Signed-off-by: Bitao Hu
Reviewed-by: Douglas Anderson
Reviewed-by: Liu Song
---
kernel/watchdog.c | 98 ++-
lib/Kconfig.debug | 13 +++
2 files changed, 110 insertions(+), 1 deletion(-)
diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 81a8862295d6..69e72d7e461d 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -16,6 +16,8 @@
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -35,6 +37,8 @@ static DEFINE_MUTEX(watchdog_mutex);
# define WATCHDOG_HARDLOCKUP_DEFAULT 0
#endif
+#define NUM_SAMPLE_PERIODS 5
+
unsigned long __read_mostly watchdog_enabled;
int __read_mostly watchdog_user_enabled = 1;
static int __read_mostly watchdog_hardlockup_user_enabled =
WATCHDOG_HARDLOCKUP_DEFAULT;
@@ -333,6 +337,95 @@ __setup("watchdog_thresh=", watchdog_thresh_setup);
static void __lockup_detector_cleanup(void);
+#ifdef CONFIG_SOFTLOCKUP_DETECTOR_INTR_STORM
+enum stats_per_group {
+ STATS_SYSTEM,
+ STATS_SOFTIRQ,
+ STATS_HARDIRQ,
+ STATS_IDLE,
+ NUM_STATS_PER_GROUP,
+};
+
+static const enum cpu_usage_stat tracked_stats[NUM_STATS_PER_GROUP] = {
+ CPUTIME_SYSTEM,
+ CPUTIME_SOFTIRQ,
+ CPUTIME_IRQ,
+ CPUTIME_IDLE,
+};
+
+static DEFINE_PER_CPU(u16, cpustat_old[NUM_STATS_PER_GROUP]);
+static DEFINE_PER_CPU(u8,
cpustat_util[NUM_SAMPLE_PERIODS][NUM_STATS_PER_GROUP]);
+static DEFINE_PER_CPU(u8, cpustat_tail);
+
+/*
+ * We don't need nanosecond resolution. A granularity of 16ms is
+ * sufficient for our precision, allowing us to use u16 to store
+ * cpustats, which will roll over roughly every ~1000 seconds.
+ * 2^24 ~= 16 * 10^6
+ */
+static u16 get_16bit_precision(u64 data_ns)
+{
+ return data_ns >> 24LL; /* 2^24ns ~= 16.8ms */
+}
+
+static void update_cpustat(void)
+{
+ int i;
+ u8 util;
+ u16 old_stat, new_stat;
+ struct kernel_cpustat kcpustat;
+ u64 *cpustat = kcpustat.cpustat;
+ u8 tail = __this_cpu_read(cpustat_tail);
+ u16 sample_period_16 = get_16bit_precision(sample_period);
+
+ kcpustat_cpu_fetch(, smp_processor_id());
+
+ for (i = 0; i < NUM_STATS_PER_GROUP; i++) {
+ old_stat = __this_cpu_read(cpustat_old[i]);
+ new_stat = get_16bit_precision(cpustat[tracked_stats[i]]);
+ util = DIV_ROUND_UP(100 * (new_stat - old_stat),
sample_period_16);
+ __this_cpu_write(cpustat_util[tail][i], util);
+ __this_cpu_write(cpustat_old[i], new_stat);
+ }
+
+ __this_cpu_write(cpustat_tail, (tail + 1) % NUM_SAMPLE_PERIODS);
+}
+
+static void print_cpustat(void)
+{
+ int i, group;
+ u8 tail = __this_cpu_read(cpustat_tail);
+ u64 sample_period_second = sample_period;
+
+ do_div(sample_period_second, NSEC_PER_SEC);
+
+ /*
+* We do not want the "watchdog: " prefix on every line,
+* hence we use "printk" instead of "pr_crit".
+*/
+ printk(KERN_CRIT "CPU#%d Utilization every %llus during lockup:\n",
+ smp_processor_id(), sample_period_second);
+
+ for (i = 0; i <
[PATCHv9 0/3] *** Detect interrupt storm in softlockup ***
Hi, guys. I have implemented a low-overhead method for detecting interrupt storm in softlockup. Please review it, all comments are welcome. Changes from v8 to v9: - Patch #1 remains unchanged. - From Thomas Gleixner, split patch #2 into two patches. Interrupt infrastructure first and then the actual usage site in the watchdog code. Changes from v7 to v8: - From Thomas Gleixner, implement statistics within the interrupt core code and provide sensible interfaces for the watchdog code. - Patch #1 remains unchanged. Patch #2 has significant changes based on Thomas's suggestions, which is why I have removed Liu Song and Douglas's Reviewed-by from patch #2. Please review it again, and all comments are welcome. Changes from v6 to v7: - Remove "READ_ONCE" in "start_counting_irqs" - Replace the hard-coded 5 with "NUM_SAMPLE_PERIODS" macro in "set_sample_period". - Add empty lines to help with reading the code. - Remove the branch that processes IRQs where "counts_diff = 0". - Add the Reviewed-by of Liu Song and Douglas. Changes from v5 to v6: - Use "./scripts/checkpatch.pl --strict" to get a few extra style nits and fix them. - Squash patch #3 into patch #1, and wrapp the help text to 80 columns. - Sort existing headers alphabetically in watchdog.c - Drop "softlockup_hardirq_cpus", just read "hardirq_counts" and see if it's non-NULL. - Store "nr_irqs" in a local variable. - Simplify the calculation of "cpu_diff". Changes from v4 to v5: - Rearranging variable placement to make code look neater. Changes from v3 to v4: - Renaming some variable and function names to make the code logic more readable. - Change the code location to avoid predeclaring. - Just swap rather than a double loop in tabulate_irq_count. - Since nr_irqs has the potential to grow at runtime, bounds-check logic has been implemented. - Add SOFTLOCKUP_DETECTOR_INTR_STORM Kconfig knob. Changes from v2 to v3: - From Liu Song, using enum instead of macro for cpu_stats, shortening the name 'idx_to_stat' to 'stats', adding 'get_16bit_precesion' instead of using right shift operations, and using 'struct irq_counts'. - From kernel robot test, using '__this_cpu_read' and '__this_cpu_write' instead of accessing to an per-cpu array directly, in order to avoid this warning. 'sparse: incorrect type in initializer (different modifiers)' Changes from v1 to v2: - From Douglas, optimize the memory of cpustats. With the maximum number of CPUs, that's now this. 2 * 8192 * 4 + 1 * 8192 * 5 * 4 + 1 * 8192 = 237,568 bytes. - From Liu Song, refactor the code format and add necessary comments. - From Douglas, use interrupt counts instead of interrupt time to determine the cause of softlockup. - Remove the cmdline parameter added in PATCHv1. Bitao Hu (3): watchdog/softlockup: low-overhead detection of interrupt storm irq: use a struct for the kstat_irqs in the interrupt descriptor watchdog/softlockup: report the most frequent interrupts arch/mips/dec/setup.c| 2 +- arch/parisc/kernel/smp.c | 2 +- arch/powerpc/kvm/book3s_hv_rm_xics.c | 2 +- include/linux/irqdesc.h | 9 +- include/linux/kernel_stat.h | 3 + kernel/irq/internals.h | 2 +- kernel/irq/irqdesc.c | 34 - kernel/irq/proc.c| 9 +- kernel/watchdog.c| 213 ++- lib/Kconfig.debug| 13 ++ scripts/gdb/linux/interrupts.py | 6 +- 11 files changed, 268 insertions(+), 27 deletions(-) -- 2.37.1 (Apple Git-137.1)
Re: [PATCH RFC net] ps3/gelic: Fix possible NULL pointer dereference
Hi Simon,
On Wed, Feb 21, 2024 at 5:57 PM Simon Horman wrote:
> Fix possible NULL pointer dereference in gelic_card_release_tx_chain()
>
> The cited commit introduced a netdev variable to
> gelic_card_release_tx_chain() which is set unconditionally on each
> iteration of a for loop.
>
> It is set to the value of tx_chain->tail->skb->dev. However, in some
> cases it is assumed that tx_chain->tail->skb may be NULL. And if that
> occurs, setting netdev will cause a NULl pointer dereference.
Thanks for your patch!
> Given the age of this code I do wonder if this can occur in practice.
> But to be on the safe side this patch assumes that it can and aims to
> avoid the dereference in the case where tx_chain->tail->skb may be NULL.
The compiler may also lazy-load netdev until it's actually used,
avoiding the crash?
> Fixes: 589866f9f1cb ("PS3: gelic: Add support for dual network interface")
> Signed-off-by: Simon Horman
Reviewed-by: Geert Uytterhoeven
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [PATCH] powerpc/hv-gpci: Fix the hcall return value checks in single_gpci_request function
On 2/20/24 18:08, Michael Ellerman wrote:
> Kajol Jain writes:
>> Running event
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
>> in one of the system throws below error:
>>
>> ---Logs---
>> # perf list | grep
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles
>>
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=?/[Kernel
>> PMU event]
>>
>>
>> # perf stat -v -e
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
>> sleep 2
>> Using CPUID 00800200
>> Control descriptor is not initialized
>> Warning:
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
>> event is not supported by the kernel.
>> failed to read counter
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
>>
>> Performance counter stats for 'system wide':
>>
>>
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
>>
>>2.000700771 seconds time elapsed
>>
>> The above error is because of the hcall failure as required
>> permission "Enable Performance Information Collection" is not set.
>> Based on current code, single_gpci_request function did not check the
>> error type incase hcall fails and by default returns EINVAL. But we can
>> have other reasons for hcall failures like H_AUTHORITY/H_PARAMETER for which
>> we need to act accordingly.
>> Fix this issue by adding new checks in the single_gpci_request function.
>>
>> Result after fix patch changes:
>>
>> # perf stat -e
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
>> sleep 2
>> Error:
>> No permission to enable
>> hv_gpci/dispatch_timebase_by_processor_processor_time_in_timebase_cycles,phys_processor_idx=0/
>> event.
>>
>> Fixes: 220a0c609ad1 ("powerpc/perf: Add support for the hv gpci (get
>> performance counter info) interface")
>> Reported-by: Akanksha J N
>> Signed-off-by: Kajol Jain
>> ---
>> arch/powerpc/perf/hv-gpci.c | 29 +
>> 1 file changed, 25 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/powerpc/perf/hv-gpci.c b/arch/powerpc/perf/hv-gpci.c
>> index 27f18119fda1..101060facd81 100644
>> --- a/arch/powerpc/perf/hv-gpci.c
>> +++ b/arch/powerpc/perf/hv-gpci.c
>> @@ -695,7 +695,17 @@ static unsigned long single_gpci_request(u32 req, u32
>> starting_index,
>>
>> ret = plpar_hcall_norets(H_GET_PERF_COUNTER_INFO,
>> virt_to_phys(arg), HGPCI_REQ_BUFFER_SIZE);
>> -if (ret) {
>> +
>> +/*
>> + * ret value as 'H_PARAMETER' corresponds to 'GEN_BUF_TOO_SMALL',
>
> Don't we expect H_PARAMETER if any parameter value is incorrect?
>
>> + * which means that the current buffer size cannot accommodate
>> + * all the information and a partial buffer returned.
>
> I don't see how we can infer that H_PARAMETER means the buffer is too
> small and accessing the first entry is OK?
Hi Michael,
Based on getCounterInfo documentation and the name convention it uses,
we actually used H_PARAMETER to specify the buffer issue incase buffer
cannot accommodate all the data.
Hence we are using that return value in the check.
Since based on hv-gpci event counter we only want data for specific
starting index and the hv-gpci hcall actually store data starting from
given starting index in the result buffer. We can ensure that accessing
first entry will be enough.
Thanks,
Kajol Jain
>
> cheers
>
>> + * Since in this function we are only accessing data for a given
>> starting index,
>> + * we don't need to accommodate whole data and can get required count by
>> + * accessing very first entry.
>> + * Hence hcall fails only incase the ret value is other than H_SUCCESS
>> or H_PARAMETER.
>> + */
>> +if (ret && (ret != H_PARAMETER)) {
>> pr_devel("hcall failed: 0x%lx\n", ret);
>> goto out;
>> }
Re: [PATCH v2 03/13] mm: Provide generic pmd_thp_or_huge()
On Wed, Feb 21, 2024 at 08:57:53AM -0400, Jason Gunthorpe wrote: > On Wed, Feb 21, 2024 at 05:37:37PM +0800, Peter Xu wrote: > > On Mon, Jan 15, 2024 at 01:55:51PM -0400, Jason Gunthorpe wrote: > > > On Wed, Jan 03, 2024 at 05:14:13PM +0800, pet...@redhat.com wrote: > > > > From: Peter Xu > > > > > > > > ARM defines pmd_thp_or_huge(), detecting either a THP or a huge PMD. It > > > > can be a helpful helper if we want to merge more THP and hugetlb code > > > > paths. Make it a generic default implementation, only exist when > > > > CONFIG_MMU. Arch can overwrite it by defining its own version. > > > > > > > > For example, ARM's pgtable-2level.h defines it to always return false. > > > > > > > > Keep the macro declared with all config, it should be optimized to a > > > > false > > > > anyway if !THP && !HUGETLB. > > > > > > > > Signed-off-by: Peter Xu > > > > --- > > > > include/linux/pgtable.h | 4 > > > > mm/gup.c| 3 +-- > > > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/linux/pgtable.h b/include/linux/pgtable.h > > > > index 466cf477551a..2b42e95a4e3a 100644 > > > > --- a/include/linux/pgtable.h > > > > +++ b/include/linux/pgtable.h > > > > @@ -1362,6 +1362,10 @@ static inline int pmd_write(pmd_t pmd) > > > > #endif /* pmd_write */ > > > > #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ > > > > > > > > +#ifndef pmd_thp_or_huge > > > > +#define pmd_thp_or_huge(pmd) (pmd_huge(pmd) || pmd_trans_huge(pmd)) > > > > +#endif > > > > > > Why not just use pmd_leaf() ? > > > > > > This GUP case seems to me exactly like what pmd_leaf() should really > > > do and be used for.. > > > > I think I mostly agree with you, and these APIs are indeed confusing. IMHO > > the challenge is about the risk of breaking others on small changes in the > > details where evil resides. > > These APIs are super confusing, which is why I brought it up.. Adding > even more subtly different variations is not helping. > > I think pmd_leaf means the entry is present and refers to a physical > page not another radix level. > > > > eg x86 does: > > > > > > #define pmd_leaf pmd_large > > > static inline int pmd_large(pmd_t pte) > > > return pmd_flags(pte) & _PAGE_PSE; > > > > > > static inline int pmd_trans_huge(pmd_t pmd) > > > return (pmd_val(pmd) & (_PAGE_PSE|_PAGE_DEVMAP)) == _PAGE_PSE; > > > > > > int pmd_huge(pmd_t pmd) > > > return !pmd_none(pmd) && > > > (pmd_val(pmd) & (_PAGE_PRESENT|_PAGE_PSE)) != > > > _PAGE_PRESENT; > > > > For example, here I don't think it's strictly pmd_leaf()? As pmd_huge() > > will return true if PRESENT=0 && PSE=0 (as long as none pte ruled out > > first), while pmd_leaf() will return false; I think that came from > > cbef8478bee5. > > Yikes, but do you even want to handle non-present entries in GUP > world? Isn't everything gated by !present in the first place? I am as confused indeed. > > > Besides that, there're also other cases where it's not clear of such direct > > replacement, not until further investigated. E.g., arm-3level has: > > > > #define pmd_leaf(pmd) pmd_sect(pmd) > > #define pmd_sect(pmd) ((pmd_val(pmd) & PMD_TYPE_MASK) == \ > > PMD_TYPE_SECT) > > #define PMD_TYPE_SECT (_AT(pmdval_t, 1) << 0) > > > > While pmd_huge() there relies on PMD_TABLE_BIT () > > I looked at tht, it looked OK.. > > #define PMD_TYPE_MASK (_AT(pmdval_t, 3) << 0) > #define PMD_TABLE_BIT (_AT(pmdval_t, 1) << 1) > > It is the same stuff, just a little confusingly written True, my eyes decided to skip all the shifts. :-( Ok then, let me see whether I can give it a stab on the pXd_huge() mess. Thanks, -- Peter Xu
Re: [PATCH 1/2] powerpc: Refactor __kernel_map_pages()
Le 22/02/2024 à 06:32, Michael Ellerman a écrit :
> Christophe Leroy writes:
>> __kernel_map_pages() is almost identical for PPC32 and RADIX.
>>
>> Refactor it.
>>
>> On PPC32 it is not needed for KFENCE, but to keep it simple
>> just make it similar to PPC64.
>>
>> Signed-off-by: Christophe Leroy
>> ---
>> arch/powerpc/include/asm/book3s/64/pgtable.h | 10 --
>> arch/powerpc/include/asm/book3s/64/radix.h | 2 --
>> arch/powerpc/mm/book3s64/radix_pgtable.c | 14 --
>> arch/powerpc/mm/pageattr.c | 19 +++
>> arch/powerpc/mm/pgtable_32.c | 15 ---
>> 5 files changed, 19 insertions(+), 41 deletions(-)
>>
>> diff --git a/arch/powerpc/mm/pageattr.c b/arch/powerpc/mm/pageattr.c
>> index 421db7c4f2a4..16b8d20d6ca8 100644
>> --- a/arch/powerpc/mm/pageattr.c
>> +++ b/arch/powerpc/mm/pageattr.c
>> @@ -101,3 +101,22 @@ int change_memory_attr(unsigned long addr, int
>> numpages, long action)
>> return apply_to_existing_page_range(_mm, start, size,
>> change_page_attr, (void *)action);
>> }
>> +
>> +#if defined(CONFIG_DEBUG_PAGEALLOC) || defined(CONFIG_KFENCE)
>> +#ifdef CONFIG_ARCH_SUPPORTS_DEBUG_PAGEALLOC
>> +void __kernel_map_pages(struct page *page, int numpages, int enable)
>> +{
>> +unsigned long addr = (unsigned long)page_address(page);
>> +
>> +if (PageHighMem(page))
>> +return;
>> +
>> +if (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && !radix_enabled())
>> +hash__kernel_map_pages(page, numpages, enable);
>> +else if (enable)
>> +set_memory_p(addr, numpages);
>> +else
>> +set_memory_np(addr, numpages);
>> +}
>
> This doesn't build on 32-bit, eg. ppc32_allmodconfig:
>
> ../arch/powerpc/mm/pageattr.c: In function '__kernel_map_pages':
> ../arch/powerpc/mm/pageattr.c:116:23: error: implicit declaration of function
> 'hash__kernel_map_pages' [-Werror=implicit-function-declaration]
>116 | err = hash__kernel_map_pages(page, numpages, enable);
>| ^~
>
> I couldn't see a nice way to get around it, so ended up with:
>
> void __kernel_map_pages(struct page *page, int numpages, int enable)
> {
> int err;
> unsigned long addr = (unsigned long)page_address(page);
>
> if (PageHighMem(page))
> return;
>
> #ifdef CONFIG_PPC_BOOK3S_64
> if (!radix_enabled())
> err = hash__kernel_map_pages(page, numpages, enable);
> else
> #endif
> if (enable)
> err = set_memory_p(addr, numpages);
> else
> err = set_memory_np(addr, numpages);
>
I missed something it seems. Not good to leave something unterminated
when you leave for vacation and think it was finished when you come back.
The best solution I see is to move hash__kernel_map_pages() prototype
somewhere else.
$ git grep -e hash__ -e radix__ -- arch/powerpc/include/asm/*.h
arch/powerpc/include/asm/bug.h:void hash__do_page_fault(struct pt_regs *);
arch/powerpc/include/asm/mmu.h:extern void radix__mmu_cleanup_all(void);
arch/powerpc/include/asm/mmu_context.h:extern void
radix__switch_mmu_context(struct mm_struct *prev,
arch/powerpc/include/asm/mmu_context.h: return
radix__switch_mmu_context(prev, next);
arch/powerpc/include/asm/mmu_context.h:extern int
hash__alloc_context_id(void);
arch/powerpc/include/asm/mmu_context.h:void __init
hash__reserve_context_id(int id);
arch/powerpc/include/asm/mmu_context.h: context_id =
hash__alloc_context_id();
arch/powerpc/include/asm/mmu_context.h: * radix__flush_all_mm() to
determine the scope (local/global)
arch/powerpc/include/asm/mmu_context.h: radix__flush_all_mm(mm);
Maybe asm/mmu.h ?
Or mm/mmu_decl.h ?
Christophe
