Re: [Bisected] PowerMac G5 fails booting kernel 6.6-rc3 (BUG: Unable to handle kernel data access at 0xfeffbb62ffec65fe)
Erhard Furtner writes: > On Thu, 12 Oct 2023 22:41:56 +1100 > Michael Ellerman wrote: > >> Can you checkout the exact commit that crash is from and do: >> >> $ make arch/powerpc/mm/book3s64/hash_utils.lst >> >> And paste/attach the content of that file. >> >> cheers > > Ok, attached the output from: > > git checkout 9fee28baa601f4dbf869b1373183b312d2d5ef3d > make vmlinux -j16 > make arch/powerpc/mm/book3s64/hash_utils.lst > > Commit 9fee28baa601f4dbf869b1373183b312d2d5ef3d is the 1st bad commit of my > bisect. Thanks. I think I've reproduced the crash on my Quad G5 by using your config with some things tweaked, but I don't get any output on the screen :/ Do you mind booting the commit above and taking a photo of the oops and attach it here. The oops you transcribed didn't entirely make sense, probably due to a typo here or there, so a photo would be best. cheers
[PATCHv9 2/2] powerpc/setup: Loosen the mapping between cpu logical id and its seq in dt
*** Idea ***
For kexec -p, the boot cpu can be not the cpu0, this causes the problem
of allocating memory for paca_ptrs[]. However, in theory, there is no
requirement to assign cpu's logical id as its present sequence in the
device tree. But there is something like cpu_first_thread_sibling(),
which makes assumption on the mapping inside a core. Hence partially
loosening the mapping, i.e. unbind the mapping of core while keep the
mapping inside a core.
*** Implement ***
At this early stage, there are plenty of memory to utilize. Hence, this
patch allocates interim memory to link the cpu info on a list, then
reorder cpus by changing the list head. As a result, there is a rotate
shift between the sequence number in dt and the cpu logical number.
*** Result ***
After this patch, a boot-cpu's logical id will always be mapped into the
range [0,threads_per_core).
Besides this, at this phase, all threads in the boot core are forced to
be onlined. This restriction will be lifted in a later patch with
extra effort.
Signed-off-by: Pingfan Liu
Cc: Michael Ellerman
Cc: Nicholas Piggin
Cc: Christophe Leroy
Cc: Mahesh Salgaonkar
Cc: Wen Xiong
Cc: Baoquan He
Cc: Ming Lei
Cc: Sourabh Jain
Cc: Hari Bathini
Cc: ke...@lists.infradead.org
To: linuxppc-dev@lists.ozlabs.org
---
arch/powerpc/kernel/prom.c | 25 +
arch/powerpc/kernel/setup-common.c | 84 +++---
2 files changed, 82 insertions(+), 27 deletions(-)
diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c
index ec82f5bda908..7ed9034912ca 100644
--- a/arch/powerpc/kernel/prom.c
+++ b/arch/powerpc/kernel/prom.c
@@ -76,7 +76,9 @@ u64 ppc64_rma_size;
unsigned int boot_cpu_node_count __ro_after_init;
#endif
static phys_addr_t first_memblock_size;
+#ifdef CONFIG_SMP
static int __initdata boot_cpu_count;
+#endif
static int __init early_parse_mem(char *p)
{
@@ -331,8 +333,7 @@ static int __init early_init_dt_scan_cpus(unsigned long
node,
const __be32 *intserv;
int i, nthreads;
int len;
- int found = -1;
- int found_thread = 0;
+ bool found = false;
/* We are scanning "cpu" nodes only */
if (type == NULL || strcmp(type, "cpu") != 0)
@@ -355,8 +356,15 @@ static int __init early_init_dt_scan_cpus(unsigned long
node,
for (i = 0; i < nthreads; i++) {
if (be32_to_cpu(intserv[i]) ==
fdt_boot_cpuid_phys(initial_boot_params)) {
- found = boot_cpu_count;
- found_thread = i;
+ /*
+* always map the boot-cpu logical id into the
+* range of [0, thread_per_core)
+*/
+ boot_cpuid = i;
+ found = true;
+ /* This forces all threads in a core to be online */
+ if (nr_cpu_ids % nthreads != 0)
+ set_nr_cpu_ids(ALIGN(nr_cpu_ids, nthreads));
}
#ifdef CONFIG_SMP
/* logical cpu id is always 0 on UP kernels */
@@ -365,14 +373,13 @@ static int __init early_init_dt_scan_cpus(unsigned long
node,
}
/* Not the boot CPU */
- if (found < 0)
+ if (!found)
return 0;
- DBG("boot cpu: logical %d physical %d\n", found,
- be32_to_cpu(intserv[found_thread]));
- boot_cpuid = found;
+ DBG("boot cpu: logical %d physical %d\n", boot_cpuid,
+ be32_to_cpu(intserv[boot_cpuid]));
- boot_cpu_hwid = be32_to_cpu(intserv[found_thread]);
+ boot_cpu_hwid = be32_to_cpu(intserv[boot_cpuid]);
/*
* PAPR defines "logical" PVR values for cpus that
diff --git a/arch/powerpc/kernel/setup-common.c
b/arch/powerpc/kernel/setup-common.c
index 707f0490639d..9802c7e5ee2f 100644
--- a/arch/powerpc/kernel/setup-common.c
+++ b/arch/powerpc/kernel/setup-common.c
@@ -36,6 +36,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -425,6 +426,13 @@ static void __init cpu_init_thread_core_maps(int tpc)
u32 *cpu_to_phys_id = NULL;
+struct interrupt_server_node {
+ struct list_head node;
+ boolavail;
+ int len;
+ __be32 intserv[];
+};
+
/**
* setup_cpu_maps - initialize the following cpu maps:
* cpu_possible_mask
@@ -446,11 +454,16 @@ u32 *cpu_to_phys_id = NULL;
void __init smp_setup_cpu_maps(void)
{
struct device_node *dn;
- int cpu = 0;
- int nthreads = 1;
+ int shift = 0, cpu = 0;
+ int j, nthreads = 1;
+ int len;
+ struct interrupt_server_node *intserv_node, *n;
+ struct list_head *bt_node, head;
+ bool avail, found_boot_cpu = false;
DBG("smp_setup_cpu_maps()\n");
+ INIT_LIST_HEAD();
cpu_to_phys_id = memblock_alloc(nr_cpu_ids * sizeof(u32),
[PATCHv9 1/2] powerpc/setup : Enable boot_cpu_hwid for PPC32
In order to identify the boot cpu, its intserv[] should be recorded and checked in smp_setup_cpu_maps(). smp_setup_cpu_maps() is shared between PPC64 and PPC32. Since PPC64 has already used boot_cpu_hwid to carry that information, enabling this variable on PPC32 so later it can also be used to carry that information for PPC32 in the coming patch. Signed-off-by: Pingfan Liu Cc: Michael Ellerman Cc: Nicholas Piggin Cc: Christophe Leroy Cc: Mahesh Salgaonkar Cc: Wen Xiong Cc: Baoquan He Cc: Ming Lei Cc: Sourabh Jain Cc: Hari Bathini Cc: ke...@lists.infradead.org To: linuxppc-dev@lists.ozlabs.org --- arch/powerpc/include/asm/smp.h | 2 +- arch/powerpc/kernel/prom.c | 3 +-- arch/powerpc/kernel/setup-common.c | 2 -- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/include/asm/smp.h b/arch/powerpc/include/asm/smp.h index 576d0e15..5db9178cc800 100644 --- a/arch/powerpc/include/asm/smp.h +++ b/arch/powerpc/include/asm/smp.h @@ -26,7 +26,7 @@ #include extern int boot_cpuid; -extern int boot_cpu_hwid; /* PPC64 only */ +extern int boot_cpu_hwid; extern int spinning_secondaries; extern u32 *cpu_to_phys_id; extern bool coregroup_enabled; diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index 0b5878c3125b..ec82f5bda908 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -372,8 +372,7 @@ static int __init early_init_dt_scan_cpus(unsigned long node, be32_to_cpu(intserv[found_thread])); boot_cpuid = found; - if (IS_ENABLED(CONFIG_PPC64)) - boot_cpu_hwid = be32_to_cpu(intserv[found_thread]); + boot_cpu_hwid = be32_to_cpu(intserv[found_thread]); /* * PAPR defines "logical" PVR values for cpus that diff --git a/arch/powerpc/kernel/setup-common.c b/arch/powerpc/kernel/setup-common.c index 2f1026fba00d..707f0490639d 100644 --- a/arch/powerpc/kernel/setup-common.c +++ b/arch/powerpc/kernel/setup-common.c @@ -87,9 +87,7 @@ EXPORT_SYMBOL(machine_id); int boot_cpuid = -1; EXPORT_SYMBOL_GPL(boot_cpuid); -#ifdef CONFIG_PPC64 int boot_cpu_hwid = -1; -#endif /* * These are used in binfmt_elf.c to put aux entries on the stack -- 2.31.1
[PATCHv9 0/2] enable nr_cpus for powerpc
From: Pingfan Liu Since my last v4 [1], the code has undergone great changes. The paca[] array has been reorganized and indexed by paca_ptrs[], which dramatically decreases the memory consumption even if there are many unpresent cpus in the middle. However, reordering the logical cpu numbers can further decrease the size of paca_ptrs[] in the kdump case. These two patches rotate-shifts the cpu's sequence number in the device tree to obtain the logical cpu id. [1]: https://lore.kernel.org/linuxppc-dev/1520829790-14029-1-git-send-email-kernelf...@gmail.com/ --- v8 -> v9 put aside [3-5/5] in v8 for the time being, which complicates the code. optimize out some unnecessary initialization according to Hari's suggestion Cc: Michael Ellerman Cc: Nicholas Piggin Cc: Christophe Leroy Cc: Mahesh Salgaonkar Cc: Wen Xiong Cc: Baoquan He Cc: Ming Lei Cc: Sourabh Jain Cc: Hari Bathini Cc: ke...@lists.infradead.org To: linuxppc-dev@lists.ozlabs.org Pingfan Liu (2): powerpc/setup : Enable boot_cpu_hwid for PPC32 powerpc/setup: Loosen the mapping between cpu logical id and its seq in dt arch/powerpc/include/asm/smp.h | 2 +- arch/powerpc/kernel/prom.c | 26 + arch/powerpc/kernel/setup-common.c | 86 +++--- 3 files changed, 83 insertions(+), 31 deletions(-) -- 2.31.1
Re: [PATCHv8 1/5] powerpc/setup : Enable boot_cpu_hwid for PPC32
On Mon, Oct 16, 2023 at 12:13:53PM +0530, Sourabh Jain wrote: > Hello Pingfan, > > > > > > > With this patch series applied, the kdump kernel fails to boot on > > > > > > powerpc with nr_cpus=1. > > > > > > > > > > > > Console logs: > > > > > > --- > > > > > > [root]# echo c > /proc/sysrq-trigger > > > > > > [ 74.783235] sysrq: Trigger a crash > > > > > > [ 74.783244] Kernel panic - not syncing: sysrq triggered crash > > > > > > [ 74.783252] CPU: 58 PID: 3838 Comm: bash Kdump: loaded Not > > > > > > tainted > > > > > > 6.6.0-rc5pf-nr-cpus+ #3 > > > > > > [ 74.783259] Hardware name: POWER10 (raw) phyp pSeries > > > > > > [ 74.783275] Call Trace: > > > > > > [ 74.783280] [c0020f4ebac0] [c0ed9f38] > > > > > > dump_stack_lvl+0x6c/0x9c (unreliable) > > > > > > [ 74.783291] [c0020f4ebaf0] [c0150300] > > > > > > panic+0x178/0x438 > > > > > > [ 74.783298] [c0020f4ebb90] [c0936d48] > > > > > > sysrq_handle_crash+0x28/0x30 > > > > > > [ 74.783304] [c0020f4ebbf0] [c093773c] > > > > > > __handle_sysrq+0x10c/0x250 > > > > > > [ 74.783309] [c0020f4ebc90] [c0937fa8] > > > > > > write_sysrq_trigger+0xc8/0x168 > > > > > > [ 74.783314] [c0020f4ebcd0] [c0665d8c] > > > > > > proc_reg_write+0x10c/0x1b0 > > > > > > [ 74.783321] [c0020f4ebd00] [c058da54] > > > > > > vfs_write+0x104/0x4b0 > > > > > > [ 74.783326] [c0020f4ebdc0] [c058dfdc] > > > > > > ksys_write+0x7c/0x140 > > > > > > [ 74.783331] [c0020f4ebe10] [c0033a64] > > > > > > system_call_exception+0x144/0x3a0 > > > > > > [ 74.783337] [c0020f4ebe50] [c000c554] > > > > > > system_call_common+0xf4/0x258 > > > > > > [ 74.783343] --- interrupt: c00 at 0x7fffa0721594 > > > > > > [ 74.783352] NIP: 7fffa0721594 LR: 7fffa0697bf4 CTR: > > > > > > > > > > > > [ 74.783364] REGS: c0020f4ebe80 TRAP: 0c00 Not tainted > > > > > > (6.6.0-rc5pf-nr-cpus+) > > > > > > [ 74.783376] MSR: 8280f033 > > > > > > CR: 2802 XER: > > > > > > [ 74.783394] IRQMASK: 0 > > > > > > [ 74.783394] GPR00: 0004 7c4b6800 > > > > > > 7fffa0807300 > > > > > > 0001 > > > > > > [ 74.783394] GPR04: 00013549ea60 0002 > > > > > > 0010 > > > > > > > > > > > > [ 74.783394] GPR08: > > > > > > > > > > > > > > > > > > [ 74.783394] GPR12: 7fffa0abaf70 > > > > > > 4000 > > > > > > 00011a0f9798 > > > > > > [ 74.783394] GPR16: 00011a0f9724 00011a097688 > > > > > > 00011a02ff70 > > > > > > 00011a0fd568 > > > > > > [ 74.783394] GPR20: 000135554bf0 0001 > > > > > > 00011a0aa478 > > > > > > 7c4b6a24 > > > > > > [ 74.783394] GPR24: 7c4b6a20 00011a0faf94 > > > > > > 0002 > > > > > > 00013549ea60 > > > > > > [ 74.783394] GPR28: 0002 7fffa08017a0 > > > > > > 00013549ea60 > > > > > > 0002 > > > > > > [ 74.783440] NIP [7fffa0721594] 0x7fffa0721594 > > > > > > [ 74.783443] LR [7fffa0697bf4] 0x7fffa0697bf4 > > > > > > [ 74.783447] --- interrupt: c00 > > > > > > I'm in purgatory > > > > > > [0.00] radix-mmu: Page sizes from device-tree: > > > > > > [0.00] radix-mmu: Page size shift = 12 AP=0x0 > > > > > > [0.00] radix-mmu: Page size shift = 16 AP=0x5 > > > > > > [0.00] radix-mmu: Page size shift = 21 AP=0x1 > > > > > > [0.00] radix-mmu: Page size shift = 30 AP=0x2 > > > > > > [0.00] Activating Kernel Userspace Access Prevention > > > > > > [0.00] Activating Kernel Userspace Execution Prevention > > > > > > [0.00] radix-mmu: Mapped > > > > > > 0x-0x0001 > > > > > > with 64.0 KiB pages (exec) > > > > > > [0.00] radix-mmu: Mapped > > > > > > 0x0001-0x0020 > > > > > > with 64.0 KiB pages > > > > > > [0.00] radix-mmu: Mapped > > > > > > 0x0020-0x2000 > > > > > > with 2.00 MiB pages > > > > > > [0.00] radix-mmu: Mapped > > > > > > 0x2000-0x2260 > > > > > > with 2.00 MiB pages (exec) > > > > > > [0.00] radix-mmu: Mapped > > > > > > 0x2260-0x4000 > > > > > > with 2.00 MiB pages > > > > > > [0.00] radix-mmu: Mapped > > > > > > 0x4000-0x00018000 > > > > > > with 1.00 GiB pages > > > > > > [0.00] radix-mmu: Mapped > > > > > > 0x00018000-0x0001a000 > > > > > > with 2.00 MiB pages > > > > > > [0.00] lpar: Using radix MMU under hypervisor > > > > > > [0.00] Linux version 6.6.0-rc5pf-nr-cpus+ > > > > > > (r...@ltcever7x0-lp1.aus.stglabs.ibm.com) (gcc (GCC) 8.5.0 20210514 > > > > > > (Red > > > > > > Hat
Re: [PATCH] scsi: ibmvfc: Use 'unsigned int' for single-bit bitfields in 'struct ibmvfc_host'
On Tue, 10 Oct 2023 13:32:37 -0700, Nathan Chancellor wrote: > Clang warns (or errors with CONFIG_WERROR=y) several times along the > lines of: > > drivers/scsi/ibmvscsi/ibmvfc.c:650:17: warning: implicit truncation from > 'int' to a one-bit wide bit-field changes value from 1 to -1 > [-Wsingle-bit-bitfield-constant-conversion] > 650 | vhost->reinit = 1; > | ^ ~ > > [...] Applied to 6.7/scsi-queue, thanks! [1/1] scsi: ibmvfc: Use 'unsigned int' for single-bit bitfields in 'struct ibmvfc_host' https://git.kernel.org/mkp/scsi/c/78882c7657bb -- Martin K. Petersen Oracle Linux Engineering
Re: [PATCH V2 0/3] Fix for shellcheck issues with latest scripts in tests/shell
Hello, On Fri, Oct 13, 2023 at 12:31 AM Athira Rajeev wrote: > > shellcheck was run on perf tool shell scripts as a pre-requisite > to include a build option for shellcheck discussed here: > https://www.spinics.net/lists/linux-perf-users/msg25553.html > > And fixes were added for the coding/formatting issues in > two patchsets: > https://lore.kernel.org/linux-perf-users/20230613164145.50488-1-atraj...@linux.vnet.ibm.com/ > https://lore.kernel.org/linux-perf-users/20230709182800.53002-1-atraj...@linux.vnet.ibm.com/ > > Three additional issues were observed and fixes are part of: > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git > > With recent commits in perf, other three issues are observed. > shellcheck version: 0.6.0 > The changes are with recent commits ( which is mentioned in each patch) > for lock_contention, record_sideband and stat_all_metricgroups test. > Patch series fixes these testcases and patches are on top of: > git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git > > The version 1 patchset had fix patch for test_arm_coresight.sh. > Dropping that in V2 based on discussion here: > https://lore.kernel.org/linux-perf-users/f265857d-0d37-4878-908c-d20732f21...@linux.vnet.ibm.com/T/#u > > Athira Rajeev (3): > tools/perf/tests Ignore the shellcheck SC2046 warning in > lock_contention > tools/perf/tests: Fix shellcheck warning in record_sideband.sh test > tools/perf/tests/shell: Fix shellcheck warning SC2112 with > stat_all_metricgroups For the series, Acked-by: Namhyung Kim Thanks, Namhyung
Re: [PATCH] powerpc/pseries/vas: Migration suspend waits for no in-progress open windows
On 10/16/23 1:30 PM, Nathan Lynch wrote:
Nathan Lynch writes:
Haren Myneni writes:
Haren Myneni writes:
The hypervisor returns migration failure if all VAS windows are not
closed. During pre-migration stage, vas_migration_handler() sets
migration_in_progress flag and closes all windows from the list.
The allocate VAS window routine checks the migration flag, setup
the window and then add it to the list. So there is possibility of
the migration handler missing the window that is still in the
process of setup.
t1: Allocate and open VAS t2: Migration event
window
lock vas_pseries_mutex
If migration_in_progress set
unlock vas_pseries_mutex
return
open window HCALL
unlock vas_pseries_mutex
Modify window HCALL lock vas_pseries_mutex
setup windowmigration_in_progress=true
Closes all windows from
the list
unlock vas_pseries_mutex
lock vas_pseries_mutex return
if nr_closed_windows == 0
// No DLPAR CPU or migration
add to the list
unlock vas_pseries_mutex
return
unlock vas_pseries_mutex
Close VAS window
// due to DLPAR CPU or migration
return -EBUSY
Could the the path t1 takes simply hold the mutex for the duration of
its execution instead of dropping and reacquiring it in the middle?
Here's the relevant code from vas_allocate_window():
mutex_lock(_pseries_mutex);
if (migration_in_progress)
rc = -EBUSY;
else
rc = allocate_setup_window(txwin, (u64 *)[0],
cop_feat_caps->win_type);
mutex_unlock(_pseries_mutex);
if (rc)
goto out;
rc = h_modify_vas_window(txwin);
if (!rc)
rc = get_vas_user_win_ref(>vas_win.task_ref);
if (rc)
goto out_free;
txwin->win_type = cop_feat_caps->win_type;
mutex_lock(_pseries_mutex);
if (!caps->nr_close_wins) {
list_add(>win_list, >list);
caps->nr_open_windows++;
mutex_unlock(_pseries_mutex);
vas_user_win_add_mm_context(>vas_win.task_ref);
return >vas_win;
}
mutex_unlock(_pseries_mutex);
Is there something about h_modify_vas_window() or get_vas_user_win_ref()
that requires temporarily dropping the lock?
Thanks Nathan for your comments.
vas_pseries_mutex protects window ID and IRQ allocation between alloc
and free window HCALLs, and window list. Generally try to not using
mutex in HCALLs, but we need this mutex with these HCALLs.
We can add h_modify_vas_window() or get_vas_user_win_ref() with in the
mutex context, but not needed.
Hmm. I contend that it would fix your bug in a simpler way that
eliminates the race instead of coping with it by adding more state and
complicating the locking model. With your change, readers of the
migration_in_progress flag check it under the mutex, but the writer
updates it outside of the mutex, which seems strange and unlikely to be
correct.
Expanding on this, with your change, migration_in_progress becomes a
boolean atomic_t flag accessed only with atomic_set() and
atomic_read(). These are non-RMW operations. Documentation/atomic_t.txt
says:
Non-RMW ops:
The non-RMW ops are (typically) regular LOADs and STOREs and are
canonically implemented using READ_ONCE(), WRITE_ONCE(),
smp_load_acquire() and smp_store_release() respectively. Therefore, if
you find yourself only using the Non-RMW operations of atomic_t, you
do not in fact need atomic_t at all and are doing it wrong.
So making migration_in_progress an atomic_t does not confer any
advantageous properties to it that it lacks as a plain boolean.
Considering also (from the same document):
- non-RMW operations are unordered;
- RMW operations that have no return value are unordered;
I am concerned that threads executing these segments of code will not
always observe each others' effects in the intended order:
// vas_allocate_window()
mutex_lock(_pseries_mutex);
if (!caps->nr_close_wins && !atomic_read(_in_progress)) {
list_add(>win_list, >list);
caps->nr_open_windows++;
atomic_dec(>nr_open_wins_progress);
mutex_unlock(_pseries_mutex);
vas_user_win_add_mm_context(>vas_win.task_ref);
return >vas_win;
}
mutex_unlock(_pseries_mutex);
...
atomic_dec(>nr_open_wins_progress);
wake_up(_win_progress_wq);
// vas_migration_handler()
atomic_set(_in_progress, 1);
...
mutex_lock(_pseries_mutex);
rc = reconfig_close_windows(vcaps,
vcaps->nr_open_windows,
true);
Re: [PATCH] powerpc/pseries/vas: Migration suspend waits for no in-progress open windows
Nathan Lynch writes:
> Haren Myneni writes:
>>> Haren Myneni writes:
The hypervisor returns migration failure if all VAS windows are not
closed. During pre-migration stage, vas_migration_handler() sets
migration_in_progress flag and closes all windows from the list.
The allocate VAS window routine checks the migration flag, setup
the window and then add it to the list. So there is possibility of
the migration handler missing the window that is still in the
process of setup.
t1: Allocate and open VAS t2: Migration event
window
lock vas_pseries_mutex
If migration_in_progress set
unlock vas_pseries_mutex
return
open window HCALL
unlock vas_pseries_mutex
Modify window HCALLlock vas_pseries_mutex
setup window migration_in_progress=true
Closes all windows from
the list
unlock vas_pseries_mutex
lock vas_pseries_mutex return
if nr_closed_windows == 0
// No DLPAR CPU or migration
add to the list
unlock vas_pseries_mutex
return
unlock vas_pseries_mutex
Close VAS window
// due to DLPAR CPU or migration
return -EBUSY
>>>
>>> Could the the path t1 takes simply hold the mutex for the duration of
>>> its execution instead of dropping and reacquiring it in the middle?
>>>
>>> Here's the relevant code from vas_allocate_window():
>>>
>>> mutex_lock(_pseries_mutex);
>>> if (migration_in_progress)
>>> rc = -EBUSY;
>>> else
>>> rc = allocate_setup_window(txwin, (u64 *)[0],
>>>cop_feat_caps->win_type);
>>> mutex_unlock(_pseries_mutex);
>>> if (rc)
>>> goto out;
>>>
>>> rc = h_modify_vas_window(txwin);
>>> if (!rc)
>>> rc = get_vas_user_win_ref(>vas_win.task_ref);
>>> if (rc)
>>> goto out_free;
>>>
>>> txwin->win_type = cop_feat_caps->win_type;
>>> mutex_lock(_pseries_mutex);
>>> if (!caps->nr_close_wins) {
>>> list_add(>win_list, >list);
>>> caps->nr_open_windows++;
>>> mutex_unlock(_pseries_mutex);
>>> vas_user_win_add_mm_context(>vas_win.task_ref);
>>> return >vas_win;
>>> }
>>> mutex_unlock(_pseries_mutex);
>>>
>>> Is there something about h_modify_vas_window() or get_vas_user_win_ref()
>>> that requires temporarily dropping the lock?
>>>
>>
>> Thanks Nathan for your comments.
>>
>> vas_pseries_mutex protects window ID and IRQ allocation between alloc
>> and free window HCALLs, and window list. Generally try to not using
>> mutex in HCALLs, but we need this mutex with these HCALLs.
>>
>> We can add h_modify_vas_window() or get_vas_user_win_ref() with in the
>> mutex context, but not needed.
>
> Hmm. I contend that it would fix your bug in a simpler way that
> eliminates the race instead of coping with it by adding more state and
> complicating the locking model. With your change, readers of the
> migration_in_progress flag check it under the mutex, but the writer
> updates it outside of the mutex, which seems strange and unlikely to be
> correct.
Expanding on this, with your change, migration_in_progress becomes a
boolean atomic_t flag accessed only with atomic_set() and
atomic_read(). These are non-RMW operations. Documentation/atomic_t.txt
says:
Non-RMW ops:
The non-RMW ops are (typically) regular LOADs and STOREs and are
canonically implemented using READ_ONCE(), WRITE_ONCE(),
smp_load_acquire() and smp_store_release() respectively. Therefore, if
you find yourself only using the Non-RMW operations of atomic_t, you
do not in fact need atomic_t at all and are doing it wrong.
So making migration_in_progress an atomic_t does not confer any
advantageous properties to it that it lacks as a plain boolean.
Considering also (from the same document):
- non-RMW operations are unordered;
- RMW operations that have no return value are unordered;
I am concerned that threads executing these segments of code will not
always observe each others' effects in the intended order:
// vas_allocate_window()
mutex_lock(_pseries_mutex);
if (!caps->nr_close_wins && !atomic_read(_in_progress)) {
list_add(>win_list, >list);
caps->nr_open_windows++;
atomic_dec(>nr_open_wins_progress);
mutex_unlock(_pseries_mutex);
vas_user_win_add_mm_context(>vas_win.task_ref);
return >vas_win;
}
mutex_unlock(_pseries_mutex);
...
atomic_dec(>nr_open_wins_progress);
wake_up(_win_progress_wq);
// vas_migration_handler()
atomic_set(_in_progress, 1);
...
mutex_lock(_pseries_mutex);
rc =
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Mon, Oct 16, 2023 at 10:28:24PM +0530, Manivannan Sadhasivam wrote:
> On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> > ls1021a add suspend/resume support.
> >
>
> Please add what the driver is doing during suspend/resume.
>
> > Signed-off-by: Frank Li
> > ---
> > drivers/pci/controller/dwc/pci-layerscape.c | 88 -
> > 1 file changed, 87 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> > b/drivers/pci/controller/dwc/pci-layerscape.c
> > index 20c48c06e2248..bc5a8ff1a26ce 100644
> > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > @@ -35,6 +35,12 @@
> > #define PF_MCR_PTOMR BIT(0)
> > #define PF_MCR_EXL2S BIT(1)
> >
> > +/* LS1021A PEXn PM Write Control Register */
> > +#define SCFG_PEXPMWRCR(idx)(0x5c + (idx) * 0x64)
> > +#define PMXMTTURNOFF BIT(31)
> > +#define SCFG_PEXSFTRSTCR 0x190
> > +#define PEXSR(idx) BIT(idx)
> > +
> > #define PCIE_IATU_NUM 6
> >
> > struct ls_pcie_drvdata {
> > @@ -48,6 +54,8 @@ struct ls_pcie {
> > struct dw_pcie *pci;
> > const struct ls_pcie_drvdata *drvdata;
> > void __iomem *pf_base;
> > + struct regmap *scfg;
> > + int index;
> > bool big_endian;
> > };
> >
> > @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> > return 0;
> > }
> >
> > +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > + u32 val;
> > +
> > + if (!pcie->scfg) {
>
> Can this ever happen?
>
> > + dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > + return;
> > + }
> > +
> > + /* Send Turn_off message */
>
> "Send PME_Turn_Off message"
>
> > + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
> > + val |= PMXMTTURNOFF;
> > + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +
> > + /* There are not register to check ACK, so wait
> > PCIE_PME_TO_L2_TIMEOUT_US */
>
> "There is no specific register to check for PME_To_Ack from endpoint. So on
> the
> safe side, wait for PCIE_PME_TO_L2_TIMEOUT_US."
>
> > + mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> > +
> > + /* Clear Turn_off message */
>
> "PME_Turn_off". But I'm not sure if this is really required. Are you doing it
> because the layerspace hw implements the PME_Turn_Off bit as "level
> triggered"?
I am not sure how hardware implement this. But reference manual said:
PMXMTTURNOFF:
Generate PM turnoff message for power management of PCI Express controllers.
This bit should be cleared by software.
0 Clear PM turnoff (default)
1 Trigger PM turnoff
Frank
>
> > + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
> > + val &= ~PMXMTTURNOFF;
> > + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > +}
> > +
> > +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > + u32 val;
> > +
>
> A comment here would be good.
>
> > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > + val |= PEXSR(pcie->index);
> > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +
> > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > + val &= ~PEXSR(pcie->index);
> > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > +}
> > +
> > +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> > +{
> > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > + struct device *dev = pcie->pci->dev;
> > + u32 index[2];
> > + int ret;
> > +
> > + ret = ls_pcie_host_init(pp);
> > + if (ret)
> > + return ret;
> > +
> > + pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > "fsl,pcie-scfg");
> > + if (IS_ERR(pcie->scfg)) {
> > + ret = PTR_ERR(pcie->scfg);
> > + dev_err(dev, "No syscfg phandle specified\n");
> > + pcie->scfg = NULL;
> > + return ret;
> > + }
> > +
> > + ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index,
> > 2);
> > + if (ret) {
> > + pcie->scfg = NULL;
> > + return ret;
> > + }
> > +
> > + pcie->index = index[1];
> > +
>
> The above syscon parsing could be done conditionally during probe itself.
> There
> is no need to do it during host_init() time.
>
> - Mani
>
> > + return ret;
> > +}
> > +
> > static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> > .host_init = ls_pcie_host_init,
> > .pme_turn_off = ls_pcie_send_turnoff_msg,
> > };
> >
> > +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> > + .host_init = ls1021a_pcie_host_init,
> > + .pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> > +};
> > +
> > static const struct ls_pcie_drvdata
Re: [PATCH 3/3] PCI: layerscape: add suspend/resume for ls1043a
On Fri, Sep 15, 2023 at 02:43:06PM -0400, Frank Li wrote:
> ls1043a add suspend/resume support.
>
Same comment as previous patch for patch description.
> Signed-off-by: Frank Li
> ---
> drivers/pci/controller/dwc/pci-layerscape.c | 91 -
> 1 file changed, 90 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> b/drivers/pci/controller/dwc/pci-layerscape.c
> index bc5a8ff1a26ce..debabb9bb41f4 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -41,10 +41,20 @@
> #define SCFG_PEXSFTRSTCR 0x190
> #define PEXSR(idx) BIT(idx)
>
> +/* LS1043A PEX PME control register */
> +#define SCFG_PEXPMECR0x144
> +#define PEXPME(idx) BIT(31 - (idx) * 4)
> +
> +/* LS1043A PEX LUT debug register */
> +#define LS_PCIE_LDBG 0x7fc
> +#define LDBG_SR BIT(30)
> +#define LDBG_WE BIT(31)
> +
> #define PCIE_IATU_NUM6
>
> struct ls_pcie_drvdata {
> const u32 pf_off;
> + const u32 lut_off;
> const struct dw_pcie_host_ops *ops;
> void (*exit_from_l2)(struct dw_pcie_rp *pp);
> bool pm_support;
> @@ -54,6 +64,7 @@ struct ls_pcie {
> struct dw_pcie *pci;
> const struct ls_pcie_drvdata *drvdata;
> void __iomem *pf_base;
> + void __iomem *lut_base;
> struct regmap *scfg;
> int index;
> bool big_endian;
> @@ -116,6 +127,23 @@ static void ls_pcie_pf_writel(struct ls_pcie *pcie, u32
> off, u32 val)
> iowrite32(val, pcie->pf_base + off);
> }
>
> +static u32 ls_pcie_lut_readl(struct ls_pcie *pcie, u32 off)
> +{
Looking at ls_pcie_pf_{readl/writel} you can use a common function that does the
read/write and pass the relevant base/offset. This will avoid code duplication.
> + if (pcie->big_endian)
> + return ioread32be(pcie->lut_base + off);
> +
> + return ioread32(pcie->lut_base + off);
> +}
> +
> +static void ls_pcie_lut_writel(struct ls_pcie *pcie, u32 off, u32 val)
> +{
> + if (pcie->big_endian)
> + iowrite32be(val, pcie->lut_base + off);
> + else
> + iowrite32(val, pcie->lut_base + off);
> +}
> +
Remove extra newline
> +
> static void ls_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> {
> struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> @@ -249,6 +277,54 @@ static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> return ret;
> }
>
> +static void ls1043a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct ls_pcie *pcie = to_ls_pcie(pci);
> + u32 val;
> +
> + if (!pcie->scfg) {
> + dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> + return;
> + }
> +
> + /* Send Turn_off message */
> + regmap_read(pcie->scfg, SCFG_PEXPMECR, );
If the register offset is the only difference, then you could pass the register
offset via drvdata and use the same functions.
> + val |= PEXPME(pcie->index);
> + regmap_write(pcie->scfg, SCFG_PEXPMECR, val);
> +
> + /* There are not register to check ACK, so wait
> PCIE_PME_TO_L2_TIMEOUT_US */
> + mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> +
> + /* Clear Turn_off message */
> + regmap_read(pcie->scfg, SCFG_PEXPMECR, );
> + val &= ~PEXPME(pcie->index);
> + regmap_write(pcie->scfg, SCFG_PEXPMECR, val);
> +}
> +
> +static void ls1043a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct ls_pcie *pcie = to_ls_pcie(pci);
> + u32 val;
> +
Again, a comment here would be useful.
- Mani
> + val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
> + val |= LDBG_WE;
> + ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
> +
> + val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
> + val |= LDBG_SR;
> + ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
> +
> + val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
> + val &= ~LDBG_SR;
> + ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
> +
> + val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
> + val &= ~LDBG_WE;
> + ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
> +}
> +
> static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> .host_init = ls_pcie_host_init,
> .pme_turn_off = ls_pcie_send_turnoff_msg,
> @@ -265,6 +341,18 @@ static const struct ls_pcie_drvdata ls1021a_drvdata = {
> .exit_from_l2 = ls1021a_pcie_exit_from_l2,
> };
>
> +static const struct dw_pcie_host_ops ls1043a_pcie_host_ops = {
> + .host_init = ls1021a_pcie_host_init, /* the same as ls1021 */
> + .pme_turn_off = ls1043a_pcie_send_turnoff_msg,
> +};
> +
> +static const struct ls_pcie_drvdata ls1043a_drvdata = {
> + .lut_off = 0x1,
> + .pm_support = true,
> + .ops = _pcie_host_ops,
> + .exit_from_l2 = ls1043a_pcie_exit_from_l2,
> +};
> +
> static const struct ls_pcie_drvdata
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Mon, Oct 16, 2023 at 11:25:12AM -0500, Bjorn Helgaas wrote: > On Mon, Oct 16, 2023 at 12:11:04PM -0400, Frank Li wrote: > > On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote: > > > > Obviously Lorenzo *could* edit all your subject lines on your behalf, > > > but it makes everybody's life easier if people look at the existing > > > code and follow the style when making changes. > > > > Understand, but simple mark 'a' and 'A' to me. I will update patches and > > take care for next time instead search whole docuemnt to guess which one > > violated. I know I make some mistakes at here. But I am working on many > > difference kernel subsystems, some require upper case, some require low > > case, someone doesn't care. > > Right, that's why I always suggest following the example of the > surrounding code and history. English is the only language I know, > but I speculate that this typographical detail probably doesn't make > sense in languages that don't have a similar upper/lowercase > distinction. If everyone thinks it is important. I suggest put it in checkpatch.pl script. The only script check can prevent to human make mistakes. I asked the same question at: https://lore.kernel.org/imx/ZSV1sINV%2F2GrAYFr@lizhi-Precision-Tower-5810/T/#t It lets teach kid mulitplication, kid did 20 questions. only 1 failure. The good teacher should tell which one is wrong and grade as 19/20 instead of just grade 19/20 without any comments. We are using email communication instead of face to face. The efficient of communication is important. We have differece background, difference native languadge, live on difference area in world and do the same jobs to make linux kernel better. The simple and straight forward's feedback can save both our time and efforts. Frank Li > > Thanks for persevering; we'd be having a lot more trouble if I tried > to send emails in your native language ;) > > Bjorn
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> ls1021a add suspend/resume support.
>
Please add what the driver is doing during suspend/resume.
> Signed-off-by: Frank Li
> ---
> drivers/pci/controller/dwc/pci-layerscape.c | 88 -
> 1 file changed, 87 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> b/drivers/pci/controller/dwc/pci-layerscape.c
> index 20c48c06e2248..bc5a8ff1a26ce 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -35,6 +35,12 @@
> #define PF_MCR_PTOMR BIT(0)
> #define PF_MCR_EXL2S BIT(1)
>
> +/* LS1021A PEXn PM Write Control Register */
> +#define SCFG_PEXPMWRCR(idx) (0x5c + (idx) * 0x64)
> +#define PMXMTTURNOFF BIT(31)
> +#define SCFG_PEXSFTRSTCR 0x190
> +#define PEXSR(idx) BIT(idx)
> +
> #define PCIE_IATU_NUM6
>
> struct ls_pcie_drvdata {
> @@ -48,6 +54,8 @@ struct ls_pcie {
> struct dw_pcie *pci;
> const struct ls_pcie_drvdata *drvdata;
> void __iomem *pf_base;
> + struct regmap *scfg;
> + int index;
> bool big_endian;
> };
>
> @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
> return 0;
> }
>
> +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct ls_pcie *pcie = to_ls_pcie(pci);
> + u32 val;
> +
> + if (!pcie->scfg) {
Can this ever happen?
> + dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> + return;
> + }
> +
> + /* Send Turn_off message */
"Send PME_Turn_Off message"
> + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
> + val |= PMXMTTURNOFF;
> + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> +
> + /* There are not register to check ACK, so wait
> PCIE_PME_TO_L2_TIMEOUT_US */
"There is no specific register to check for PME_To_Ack from endpoint. So on the
safe side, wait for PCIE_PME_TO_L2_TIMEOUT_US."
> + mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> +
> + /* Clear Turn_off message */
"PME_Turn_off". But I'm not sure if this is really required. Are you doing it
because the layerspace hw implements the PME_Turn_Off bit as "level triggered"?
> + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
> + val &= ~PMXMTTURNOFF;
> + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> +}
> +
> +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct ls_pcie *pcie = to_ls_pcie(pci);
> + u32 val;
> +
A comment here would be good.
> + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> + val |= PEXSR(pcie->index);
> + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +
> + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> + val &= ~PEXSR(pcie->index);
> + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> +}
> +
> +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> +{
> + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> + struct ls_pcie *pcie = to_ls_pcie(pci);
> + struct device *dev = pcie->pci->dev;
> + u32 index[2];
> + int ret;
> +
> + ret = ls_pcie_host_init(pp);
> + if (ret)
> + return ret;
> +
> + pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node,
> "fsl,pcie-scfg");
> + if (IS_ERR(pcie->scfg)) {
> + ret = PTR_ERR(pcie->scfg);
> + dev_err(dev, "No syscfg phandle specified\n");
> + pcie->scfg = NULL;
> + return ret;
> + }
> +
> + ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index,
> 2);
> + if (ret) {
> + pcie->scfg = NULL;
> + return ret;
> + }
> +
> + pcie->index = index[1];
> +
The above syscon parsing could be done conditionally during probe itself. There
is no need to do it during host_init() time.
- Mani
> + return ret;
> +}
> +
> static const struct dw_pcie_host_ops ls_pcie_host_ops = {
> .host_init = ls_pcie_host_init,
> .pme_turn_off = ls_pcie_send_turnoff_msg,
> };
>
> +static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
> + .host_init = ls1021a_pcie_host_init,
> + .pme_turn_off = ls1021a_pcie_send_turnoff_msg,
> +};
> +
> static const struct ls_pcie_drvdata ls1021a_drvdata = {
> - .pm_support = false,
> + .pm_support = true,
> + .ops = _pcie_host_ops,
> + .exit_from_l2 = ls1021a_pcie_exit_from_l2,
> };
>
> static const struct ls_pcie_drvdata layerscape_drvdata = {
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
Re: [PATCH 1/3] PCI: layerscape: add function pointer for exit_from_l2()
On Fri, Sep 15, 2023 at 02:43:04PM -0400, Frank Li wrote:
> Difference layerscape chip have not difference exit_from_l2() method.
> Using function pointer for ls1028. It prepare for other layerscape
> suspend/resume support.
>
How about:
Since difference SoCs require different sequence for exiting L2, let's add a
separate "exit_from_l2()" callback. This callback can be used to execute SoC
specific sequence.
> Signed-off-by: Frank Li
> ---
> drivers/pci/controller/dwc/pci-layerscape.c | 7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> b/drivers/pci/controller/dwc/pci-layerscape.c
> index b931d597656f6..20c48c06e2248 100644
> --- a/drivers/pci/controller/dwc/pci-layerscape.c
> +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> @@ -39,6 +39,8 @@
>
> struct ls_pcie_drvdata {
> const u32 pf_off;
> + const struct dw_pcie_host_ops *ops;
Where is this ops used? If this is added as a preparatory for next patches, I'd
suggest you to move it to the respective one instead to avoid confusion.
> + void (*exit_from_l2)(struct dw_pcie_rp *pp);
> bool pm_support;
> };
>
> @@ -180,6 +182,7 @@ static const struct ls_pcie_drvdata ls1021a_drvdata = {
> static const struct ls_pcie_drvdata layerscape_drvdata = {
> .pf_off = 0xc,
> .pm_support = true,
> + .exit_from_l2 = ls_pcie_exit_from_l2,
> };
>
> static const struct of_device_id ls_pcie_of_match[] = {
> @@ -213,7 +216,7 @@ static int ls_pcie_probe(struct platform_device *pdev)
> pcie->drvdata = of_device_get_match_data(dev);
>
> pci->dev = dev;
> - pci->pp.ops = _pcie_host_ops;
> + pci->pp.ops = pcie->drvdata->ops ? pcie->drvdata->ops :
> _pcie_host_ops;
This one also.
>
> pcie->pci = pci;
>
> @@ -251,7 +254,7 @@ static int ls_pcie_resume_noirq(struct device *dev)
> if (!pcie->drvdata->pm_support)
> return 0;
>
> - ls_pcie_exit_from_l2(>pci->pp);
> + pcie->drvdata->exit_from_l2(>pci->pp);
You should always check for the existence of the callback first before invoking
it.
- Mani
>
> return dw_pcie_resume_noirq(pcie->pci);
> }
> --
> 2.34.1
>
--
மணிவண்ணன் சதாசிவம்
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Mon, Oct 16, 2023 at 12:11:04PM -0400, Frank Li wrote: > On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote: > > On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote: > > > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote: > > > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote: > > > > > > > Ping > > > > > > > > Read and follow please (and then ping us): > > > > https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com > > > > > > Could you please help point which specic one was not follow aboved guide? > > > Then I can update my code. I think that's efficial communication method. I > > > think I have read it serial times. But not sure which one violate the > > > guide? > > > > > > @Bjorn Helgaas. How do you think so? > > > > Since Lorenzo didn't point out anything specific in the patch itself, > > I think he was probably referring to the subject line and this advice: > > > > - Follow the existing convention, i.e., run "git log --oneline > > " and make yours match in format, capitalization, and > > sentence structure. For example, native host bridge driver patch > > titles look like this: > > > > PCI: altera: Fix platform_get_irq() error handling > > PCI: vmd: Remove IRQ affinity so we can allocate more IRQs > > PCI: mediatek: Add MSI support for MT2712 and MT7622 > > PCI: rockchip: Remove IRQ domain if probe fails > > > > In this case, your subject line was: > > > > PCI: layerscape: add suspend/resume for ls1021a > > > > The advice was to run this: > > > > $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c > > 83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals > > 9fda4d09905d PCI: layerscape: Add power management support for ls1028a > > 277004d7a4a3 PCI: Remove unnecessary includes > > 60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp > > d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check > > function > > 7007b745a508 PCI: layerscape: Convert to builtin_platform_driver() > > 60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around > > dw_pcie_host_init() > > b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code > > f78f02638af5 PCI: dwc: Rework MSI initialization > > > > Note that these summaries are all complete sentences that start with a > > capital letter: > > > > Use PCI_HEADER_TYPE_* instead of literals > > Add power management support for ls1028a > > Remove unnecessary includes > > ... > > > > So yours could be this: > > > > PCI: layerscape: Add suspend/resume for ls1021a > >^ > > > > This is trivial, obviously. But the uppercase/lowercase distinction > > carries information, and it's an unnecessary distraction to notice > > that "oh, this is different from the rest; is the difference > > important or should I ignore it?" > > Thanks. Not everyone think it is trivial. Especially for the one, who > English are not native language. > > > > > Obviously Lorenzo *could* edit all your subject lines on your behalf, > > but it makes everybody's life easier if people look at the existing > > code and follow the style when making changes. > > Understand, but simple mark 'a' and 'A' to me. I will update patches and > take care for next time instead search whole docuemnt to guess which one > violated. I know I make some mistakes at here. But I am working on many > difference kernel subsystems, some require upper case, some require low > case, someone doesn't care. > > I respected all reviewer's and maintaner's time, but I hope respected > the contributor's time also. I do respect your time, please respect ours by following the guidelines I provided you with multiple times already. Thank you for resending the series. Lorenzo
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Mon, Oct 16, 2023 at 12:11:04PM -0400, Frank Li wrote: > On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote: > > Obviously Lorenzo *could* edit all your subject lines on your behalf, > > but it makes everybody's life easier if people look at the existing > > code and follow the style when making changes. > > Understand, but simple mark 'a' and 'A' to me. I will update patches and > take care for next time instead search whole docuemnt to guess which one > violated. I know I make some mistakes at here. But I am working on many > difference kernel subsystems, some require upper case, some require low > case, someone doesn't care. Right, that's why I always suggest following the example of the surrounding code and history. English is the only language I know, but I speculate that this typographical detail probably doesn't make sense in languages that don't have a similar upper/lowercase distinction. Thanks for persevering; we'd be having a lot more trouble if I tried to send emails in your native language ;) Bjorn
[PATCH v2 3/3] PCI: layerscape: Add suspend/resume for ls1043a
ls1043a add suspend/resume support.
Signed-off-by: Frank Li
---
Notes:
Change from v1 to v2
- Change subject 'a' to 'A'
drivers/pci/controller/dwc/pci-layerscape.c | 91 -
1 file changed, 90 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
b/drivers/pci/controller/dwc/pci-layerscape.c
index bc5a8ff1a26c..debabb9bb41f 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -41,10 +41,20 @@
#define SCFG_PEXSFTRSTCR 0x190
#define PEXSR(idx) BIT(idx)
+/* LS1043A PEX PME control register */
+#define SCFG_PEXPMECR 0x144
+#define PEXPME(idx)BIT(31 - (idx) * 4)
+
+/* LS1043A PEX LUT debug register */
+#define LS_PCIE_LDBG 0x7fc
+#define LDBG_SRBIT(30)
+#define LDBG_WEBIT(31)
+
#define PCIE_IATU_NUM 6
struct ls_pcie_drvdata {
const u32 pf_off;
+ const u32 lut_off;
const struct dw_pcie_host_ops *ops;
void (*exit_from_l2)(struct dw_pcie_rp *pp);
bool pm_support;
@@ -54,6 +64,7 @@ struct ls_pcie {
struct dw_pcie *pci;
const struct ls_pcie_drvdata *drvdata;
void __iomem *pf_base;
+ void __iomem *lut_base;
struct regmap *scfg;
int index;
bool big_endian;
@@ -116,6 +127,23 @@ static void ls_pcie_pf_writel(struct ls_pcie *pcie, u32
off, u32 val)
iowrite32(val, pcie->pf_base + off);
}
+static u32 ls_pcie_lut_readl(struct ls_pcie *pcie, u32 off)
+{
+ if (pcie->big_endian)
+ return ioread32be(pcie->lut_base + off);
+
+ return ioread32(pcie->lut_base + off);
+}
+
+static void ls_pcie_lut_writel(struct ls_pcie *pcie, u32 off, u32 val)
+{
+ if (pcie->big_endian)
+ iowrite32be(val, pcie->lut_base + off);
+ else
+ iowrite32(val, pcie->lut_base + off);
+}
+
+
static void ls_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
{
struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
@@ -249,6 +277,54 @@ static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
return ret;
}
+static void ls1043a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct ls_pcie *pcie = to_ls_pcie(pci);
+ u32 val;
+
+ if (!pcie->scfg) {
+ dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
+ return;
+ }
+
+ /* Send Turn_off message */
+ regmap_read(pcie->scfg, SCFG_PEXPMECR, );
+ val |= PEXPME(pcie->index);
+ regmap_write(pcie->scfg, SCFG_PEXPMECR, val);
+
+ /* There are not register to check ACK, so wait
PCIE_PME_TO_L2_TIMEOUT_US */
+ mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
+
+ /* Clear Turn_off message */
+ regmap_read(pcie->scfg, SCFG_PEXPMECR, );
+ val &= ~PEXPME(pcie->index);
+ regmap_write(pcie->scfg, SCFG_PEXPMECR, val);
+}
+
+static void ls1043a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct ls_pcie *pcie = to_ls_pcie(pci);
+ u32 val;
+
+ val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
+ val |= LDBG_WE;
+ ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
+
+ val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
+ val |= LDBG_SR;
+ ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
+
+ val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
+ val &= ~LDBG_SR;
+ ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
+
+ val = ls_pcie_lut_readl(pcie, LS_PCIE_LDBG);
+ val &= ~LDBG_WE;
+ ls_pcie_lut_writel(pcie, LS_PCIE_LDBG, val);
+}
+
static const struct dw_pcie_host_ops ls_pcie_host_ops = {
.host_init = ls_pcie_host_init,
.pme_turn_off = ls_pcie_send_turnoff_msg,
@@ -265,6 +341,18 @@ static const struct ls_pcie_drvdata ls1021a_drvdata = {
.exit_from_l2 = ls1021a_pcie_exit_from_l2,
};
+static const struct dw_pcie_host_ops ls1043a_pcie_host_ops = {
+ .host_init = ls1021a_pcie_host_init, /* the same as ls1021 */
+ .pme_turn_off = ls1043a_pcie_send_turnoff_msg,
+};
+
+static const struct ls_pcie_drvdata ls1043a_drvdata = {
+ .lut_off = 0x1,
+ .pm_support = true,
+ .ops = _pcie_host_ops,
+ .exit_from_l2 = ls1043a_pcie_exit_from_l2,
+};
+
static const struct ls_pcie_drvdata layerscape_drvdata = {
.pf_off = 0xc,
.pm_support = true,
@@ -275,7 +363,7 @@ static const struct of_device_id ls_pcie_of_match[] = {
{ .compatible = "fsl,ls1012a-pcie", .data = _drvdata },
{ .compatible = "fsl,ls1021a-pcie", .data = _drvdata },
{ .compatible = "fsl,ls1028a-pcie", .data = _drvdata },
- { .compatible = "fsl,ls1043a-pcie", .data = _drvdata },
+ { .compatible = "fsl,ls1043a-pcie", .data = _drvdata },
{ .compatible = "fsl,ls1046a-pcie", .data = _drvdata },
{ .compatible =
[PATCH v2 2/3] PCI: layerscape: Add suspend/resume for ls1021a
ls1021a add suspend/resume support.
Signed-off-by: Frank Li
---
Notes:
change from v1 to v2
- change subject 'a' to 'A'
drivers/pci/controller/dwc/pci-layerscape.c | 88 -
1 file changed, 87 insertions(+), 1 deletion(-)
diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
b/drivers/pci/controller/dwc/pci-layerscape.c
index 20c48c06e224..bc5a8ff1a26c 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -35,6 +35,12 @@
#define PF_MCR_PTOMR BIT(0)
#define PF_MCR_EXL2S BIT(1)
+/* LS1021A PEXn PM Write Control Register */
+#define SCFG_PEXPMWRCR(idx)(0x5c + (idx) * 0x64)
+#define PMXMTTURNOFF BIT(31)
+#define SCFG_PEXSFTRSTCR 0x190
+#define PEXSR(idx) BIT(idx)
+
#define PCIE_IATU_NUM 6
struct ls_pcie_drvdata {
@@ -48,6 +54,8 @@ struct ls_pcie {
struct dw_pcie *pci;
const struct ls_pcie_drvdata *drvdata;
void __iomem *pf_base;
+ struct regmap *scfg;
+ int index;
bool big_endian;
};
@@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp *pp)
return 0;
}
+static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct ls_pcie *pcie = to_ls_pcie(pci);
+ u32 val;
+
+ if (!pcie->scfg) {
+ dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
+ return;
+ }
+
+ /* Send Turn_off message */
+ regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
+ val |= PMXMTTURNOFF;
+ regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
+
+ /* There are not register to check ACK, so wait
PCIE_PME_TO_L2_TIMEOUT_US */
+ mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
+
+ /* Clear Turn_off message */
+ regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
+ val &= ~PMXMTTURNOFF;
+ regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
+}
+
+static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct ls_pcie *pcie = to_ls_pcie(pci);
+ u32 val;
+
+ regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
+ val |= PEXSR(pcie->index);
+ regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
+
+ regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
+ val &= ~PEXSR(pcie->index);
+ regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
+}
+
+static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
+{
+ struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
+ struct ls_pcie *pcie = to_ls_pcie(pci);
+ struct device *dev = pcie->pci->dev;
+ u32 index[2];
+ int ret;
+
+ ret = ls_pcie_host_init(pp);
+ if (ret)
+ return ret;
+
+ pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node,
"fsl,pcie-scfg");
+ if (IS_ERR(pcie->scfg)) {
+ ret = PTR_ERR(pcie->scfg);
+ dev_err(dev, "No syscfg phandle specified\n");
+ pcie->scfg = NULL;
+ return ret;
+ }
+
+ ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg", index,
2);
+ if (ret) {
+ pcie->scfg = NULL;
+ return ret;
+ }
+
+ pcie->index = index[1];
+
+ return ret;
+}
+
static const struct dw_pcie_host_ops ls_pcie_host_ops = {
.host_init = ls_pcie_host_init,
.pme_turn_off = ls_pcie_send_turnoff_msg,
};
+static const struct dw_pcie_host_ops ls1021a_pcie_host_ops = {
+ .host_init = ls1021a_pcie_host_init,
+ .pme_turn_off = ls1021a_pcie_send_turnoff_msg,
+};
+
static const struct ls_pcie_drvdata ls1021a_drvdata = {
- .pm_support = false,
+ .pm_support = true,
+ .ops = _pcie_host_ops,
+ .exit_from_l2 = ls1021a_pcie_exit_from_l2,
};
static const struct ls_pcie_drvdata layerscape_drvdata = {
--
2.34.1
[PATCH v2 1/3] PCI: layerscape: Add function pointer for exit_from_l2()
Difference layerscape chip have not difference exit_from_l2() method.
Using function pointer for ls1028. It prepare for other layerscape
suspend/resume support.
Signed-off-by: Frank Li
---
Notes:
Change from v1 to v2
- change subject 'a' to 'A'
drivers/pci/controller/dwc/pci-layerscape.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
b/drivers/pci/controller/dwc/pci-layerscape.c
index b931d597656f..20c48c06e224 100644
--- a/drivers/pci/controller/dwc/pci-layerscape.c
+++ b/drivers/pci/controller/dwc/pci-layerscape.c
@@ -39,6 +39,8 @@
struct ls_pcie_drvdata {
const u32 pf_off;
+ const struct dw_pcie_host_ops *ops;
+ void (*exit_from_l2)(struct dw_pcie_rp *pp);
bool pm_support;
};
@@ -180,6 +182,7 @@ static const struct ls_pcie_drvdata ls1021a_drvdata = {
static const struct ls_pcie_drvdata layerscape_drvdata = {
.pf_off = 0xc,
.pm_support = true,
+ .exit_from_l2 = ls_pcie_exit_from_l2,
};
static const struct of_device_id ls_pcie_of_match[] = {
@@ -213,7 +216,7 @@ static int ls_pcie_probe(struct platform_device *pdev)
pcie->drvdata = of_device_get_match_data(dev);
pci->dev = dev;
- pci->pp.ops = _pcie_host_ops;
+ pci->pp.ops = pcie->drvdata->ops ? pcie->drvdata->ops :
_pcie_host_ops;
pcie->pci = pci;
@@ -251,7 +254,7 @@ static int ls_pcie_resume_noirq(struct device *dev)
if (!pcie->drvdata->pm_support)
return 0;
- ls_pcie_exit_from_l2(>pci->pp);
+ pcie->drvdata->exit_from_l2(>pci->pp);
return dw_pcie_resume_noirq(pcie->pci);
}
--
2.34.1
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote: > On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote: > > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote: > > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote: > > > > > Ping > > > > > > Read and follow please (and then ping us): > > > https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com > > > > Could you please help point which specic one was not follow aboved guide? > > Then I can update my code. I think that's efficial communication method. I > > think I have read it serial times. But not sure which one violate the > > guide? > > > > @Bjorn Helgaas. How do you think so? > > Since Lorenzo didn't point out anything specific in the patch itself, > I think he was probably referring to the subject line and this advice: > > - Follow the existing convention, i.e., run "git log --oneline > " and make yours match in format, capitalization, and > sentence structure. For example, native host bridge driver patch > titles look like this: > > PCI: altera: Fix platform_get_irq() error handling > PCI: vmd: Remove IRQ affinity so we can allocate more IRQs > PCI: mediatek: Add MSI support for MT2712 and MT7622 > PCI: rockchip: Remove IRQ domain if probe fails > > In this case, your subject line was: > > PCI: layerscape: add suspend/resume for ls1021a > > The advice was to run this: > > $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c > 83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals > 9fda4d09905d PCI: layerscape: Add power management support for ls1028a > 277004d7a4a3 PCI: Remove unnecessary includes > 60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp > d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check > function > 7007b745a508 PCI: layerscape: Convert to builtin_platform_driver() > 60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around > dw_pcie_host_init() > b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code > f78f02638af5 PCI: dwc: Rework MSI initialization > > Note that these summaries are all complete sentences that start with a > capital letter: > > Use PCI_HEADER_TYPE_* instead of literals > Add power management support for ls1028a > Remove unnecessary includes > ... > > So yours could be this: > > PCI: layerscape: Add suspend/resume for ls1021a >^ > > This is trivial, obviously. But the uppercase/lowercase distinction > carries information, and it's an unnecessary distraction to notice > that "oh, this is different from the rest; is the difference > important or should I ignore it?" > > Obviously Lorenzo *could* edit all your subject lines on your behalf, > but it makes everybody's life easier if people look at the existing > code and follow the style when making changes. > > E.g., write subject lines that are similar in style to previous ones, > name local variables similarly to other functions, use line lengths > consistent with the rest of the file, etc. After applying a change, > the file should look like a coherent whole; we should not be able to > tell that this hunk was added later by somebody else. This all helps > make the code (and the git history) more readable and maintainable. Thank you very much Bjorn. Frank, now please resend your patches and make sure that you follow these guidelines, they must be clear by now. Lorenzo
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Mon, Oct 16, 2023 at 10:22:11AM -0500, Bjorn Helgaas wrote: > On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote: > > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote: > > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote: > > > > > Ping > > > > > > Read and follow please (and then ping us): > > > https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com > > > > Could you please help point which specic one was not follow aboved guide? > > Then I can update my code. I think that's efficial communication method. I > > think I have read it serial times. But not sure which one violate the > > guide? > > > > @Bjorn Helgaas. How do you think so? > > Since Lorenzo didn't point out anything specific in the patch itself, > I think he was probably referring to the subject line and this advice: > > - Follow the existing convention, i.e., run "git log --oneline > " and make yours match in format, capitalization, and > sentence structure. For example, native host bridge driver patch > titles look like this: > > PCI: altera: Fix platform_get_irq() error handling > PCI: vmd: Remove IRQ affinity so we can allocate more IRQs > PCI: mediatek: Add MSI support for MT2712 and MT7622 > PCI: rockchip: Remove IRQ domain if probe fails > > In this case, your subject line was: > > PCI: layerscape: add suspend/resume for ls1021a > > The advice was to run this: > > $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c > 83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals > 9fda4d09905d PCI: layerscape: Add power management support for ls1028a > 277004d7a4a3 PCI: Remove unnecessary includes > 60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp > d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check > function > 7007b745a508 PCI: layerscape: Convert to builtin_platform_driver() > 60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around > dw_pcie_host_init() > b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code > f78f02638af5 PCI: dwc: Rework MSI initialization > > Note that these summaries are all complete sentences that start with a > capital letter: > > Use PCI_HEADER_TYPE_* instead of literals > Add power management support for ls1028a > Remove unnecessary includes > ... > > So yours could be this: > > PCI: layerscape: Add suspend/resume for ls1021a >^ > > This is trivial, obviously. But the uppercase/lowercase distinction > carries information, and it's an unnecessary distraction to notice > that "oh, this is different from the rest; is the difference > important or should I ignore it?" Thanks. Not everyone think it is trivial. Especially for the one, who English are not native language. > > Obviously Lorenzo *could* edit all your subject lines on your behalf, > but it makes everybody's life easier if people look at the existing > code and follow the style when making changes. Understand, but simple mark 'a' and 'A' to me. I will update patches and take care for next time instead search whole docuemnt to guess which one violated. I know I make some mistakes at here. But I am working on many difference kernel subsystems, some require upper case, some require low case, someone doesn't care. I respected all reviewer's and maintaner's time, but I hope respected the contributor's time also. Just simple words like , 'a' to 'A' or run git log --oneline to check subject. I will know what exact means. Do you think it will better than below words "Read and follow please (and then ping us): https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com; Frank > > E.g., write subject lines that are similar in style to previous ones, > name local variables similarly to other functions, use line lengths > consistent with the rest of the file, etc. After applying a change, > the file should look like a coherent whole; we should not be able to > tell that this hunk was added later by somebody else. This all helps > make the code (and the git history) more readable and maintainable. > > Bjorn
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Mon, Oct 16, 2023 at 10:45:25AM -0400, Frank Li wrote: > On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote: > > On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote: > > > Ping > > > > Read and follow please (and then ping us): > > https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com > > Could you please help point which specic one was not follow aboved guide? > Then I can update my code. I think that's efficial communication method. I > think I have read it serial times. But not sure which one violate the > guide? > > @Bjorn Helgaas. How do you think so? Since Lorenzo didn't point out anything specific in the patch itself, I think he was probably referring to the subject line and this advice: - Follow the existing convention, i.e., run "git log --oneline " and make yours match in format, capitalization, and sentence structure. For example, native host bridge driver patch titles look like this: PCI: altera: Fix platform_get_irq() error handling PCI: vmd: Remove IRQ affinity so we can allocate more IRQs PCI: mediatek: Add MSI support for MT2712 and MT7622 PCI: rockchip: Remove IRQ domain if probe fails In this case, your subject line was: PCI: layerscape: add suspend/resume for ls1021a The advice was to run this: $ git log --oneline drivers/pci/controller/dwc/pci-layerscape.c 83c088148c8e PCI: Use PCI_HEADER_TYPE_* instead of literals 9fda4d09905d PCI: layerscape: Add power management support for ls1028a 277004d7a4a3 PCI: Remove unnecessary includes 60b3c27fb9b9 PCI: dwc: Rename struct pcie_port to dw_pcie_rp d23f0c11aca2 PCI: layerscape: Change to use the DWC common link-up check function 7007b745a508 PCI: layerscape: Convert to builtin_platform_driver() 60f5b73fa0f2 PCI: dwc: Remove unnecessary wrappers around dw_pcie_host_init() b9ac0f9dc8ea PCI: dwc: Move dw_pcie_setup_rc() to DWC common code f78f02638af5 PCI: dwc: Rework MSI initialization Note that these summaries are all complete sentences that start with a capital letter: Use PCI_HEADER_TYPE_* instead of literals Add power management support for ls1028a Remove unnecessary includes ... So yours could be this: PCI: layerscape: Add suspend/resume for ls1021a ^ This is trivial, obviously. But the uppercase/lowercase distinction carries information, and it's an unnecessary distraction to notice that "oh, this is different from the rest; is the difference important or should I ignore it?" Obviously Lorenzo *could* edit all your subject lines on your behalf, but it makes everybody's life easier if people look at the existing code and follow the style when making changes. E.g., write subject lines that are similar in style to previous ones, name local variables similarly to other functions, use line lengths consistent with the rest of the file, etc. After applying a change, the file should look like a coherent whole; we should not be able to tell that this hunk was added later by somebody else. This all helps make the code (and the git history) more readable and maintainable. Bjorn
Re: [PATCH 2/3] PCI: layerscape: add suspend/resume for ls1021a
On Tue, Oct 10, 2023 at 06:02:36PM +0200, Lorenzo Pieralisi wrote:
> On Tue, Oct 10, 2023 at 10:20:12AM -0400, Frank Li wrote:
> > On Wed, Oct 04, 2023 at 10:23:51AM -0400, Frank Li wrote:
> > > On Fri, Sep 15, 2023 at 02:43:05PM -0400, Frank Li wrote:
> > > > ls1021a add suspend/resume support.
> > > >
> > > > Signed-off-by: Frank Li
> > > > ---
> > >
> > > ping
> > >
> > > Frank
> >
> > Ping
>
> Read and follow please (and then ping us):
> https://lore.kernel.org/linux-pci/20171026223701.ga25...@bhelgaas-glaptop.roam.corp.google.com
Could you please help point which specic one was not follow aboved guide?
Then I can update my code. I think that's efficial communication method. I
think I have read it serial times. But not sure which one violate the
guide?
@Bjorn Helgaas. How do you think so?
best regards
Frank Li
>
> > Frank
> >
> > >
> > > > drivers/pci/controller/dwc/pci-layerscape.c | 88 -
> > > > 1 file changed, 87 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/pci/controller/dwc/pci-layerscape.c
> > > > b/drivers/pci/controller/dwc/pci-layerscape.c
> > > > index 20c48c06e2248..bc5a8ff1a26ce 100644
> > > > --- a/drivers/pci/controller/dwc/pci-layerscape.c
> > > > +++ b/drivers/pci/controller/dwc/pci-layerscape.c
> > > > @@ -35,6 +35,12 @@
> > > > #define PF_MCR_PTOMR BIT(0)
> > > > #define PF_MCR_EXL2S BIT(1)
> > > >
> > > > +/* LS1021A PEXn PM Write Control Register */
> > > > +#define SCFG_PEXPMWRCR(idx)(0x5c + (idx) * 0x64)
> > > > +#define PMXMTTURNOFF BIT(31)
> > > > +#define SCFG_PEXSFTRSTCR 0x190
> > > > +#define PEXSR(idx) BIT(idx)
> > > > +
> > > > #define PCIE_IATU_NUM 6
> > > >
> > > > struct ls_pcie_drvdata {
> > > > @@ -48,6 +54,8 @@ struct ls_pcie {
> > > > struct dw_pcie *pci;
> > > > const struct ls_pcie_drvdata *drvdata;
> > > > void __iomem *pf_base;
> > > > + struct regmap *scfg;
> > > > + int index;
> > > > bool big_endian;
> > > > };
> > > >
> > > > @@ -170,13 +178,91 @@ static int ls_pcie_host_init(struct dw_pcie_rp
> > > > *pp)
> > > > return 0;
> > > > }
> > > >
> > > > +static void ls1021a_pcie_send_turnoff_msg(struct dw_pcie_rp *pp)
> > > > +{
> > > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > > > + u32 val;
> > > > +
> > > > + if (!pcie->scfg) {
> > > > + dev_dbg(pcie->pci->dev, "SYSCFG is NULL\n");
> > > > + return;
> > > > + }
> > > > +
> > > > + /* Send Turn_off message */
> > > > + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
> > > > + val |= PMXMTTURNOFF;
> > > > + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > > +
> > > > + /* There are not register to check ACK, so wait
> > > > PCIE_PME_TO_L2_TIMEOUT_US */
> > > > + mdelay(PCIE_PME_TO_L2_TIMEOUT_US/1000);
> > > > +
> > > > + /* Clear Turn_off message */
> > > > + regmap_read(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), );
> > > > + val &= ~PMXMTTURNOFF;
> > > > + regmap_write(pcie->scfg, SCFG_PEXPMWRCR(pcie->index), val);
> > > > +}
> > > > +
> > > > +static void ls1021a_pcie_exit_from_l2(struct dw_pcie_rp *pp)
> > > > +{
> > > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > > > + u32 val;
> > > > +
> > > > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > > > + val |= PEXSR(pcie->index);
> > > > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > > +
> > > > + regmap_read(pcie->scfg, SCFG_PEXSFTRSTCR, );
> > > > + val &= ~PEXSR(pcie->index);
> > > > + regmap_write(pcie->scfg, SCFG_PEXSFTRSTCR, val);
> > > > +}
> > > > +
> > > > +static int ls1021a_pcie_host_init(struct dw_pcie_rp *pp)
> > > > +{
> > > > + struct dw_pcie *pci = to_dw_pcie_from_pp(pp);
> > > > + struct ls_pcie *pcie = to_ls_pcie(pci);
> > > > + struct device *dev = pcie->pci->dev;
> > > > + u32 index[2];
> > > > + int ret;
> > > > +
> > > > + ret = ls_pcie_host_init(pp);
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + pcie->scfg = syscon_regmap_lookup_by_phandle(dev->of_node,
> > > > "fsl,pcie-scfg");
> > > > + if (IS_ERR(pcie->scfg)) {
> > > > + ret = PTR_ERR(pcie->scfg);
> > > > + dev_err(dev, "No syscfg phandle specified\n");
> > > > + pcie->scfg = NULL;
> > > > + return ret;
> > > > + }
> > > > +
> > > > + ret = of_property_read_u32_array(dev->of_node, "fsl,pcie-scfg",
> > > > index, 2);
> > > > + if (ret) {
> > > > + pcie->scfg = NULL;
> > > > + return ret;
> > > > + }
> > > > +
> > > > + pcie->index = index[1];
> > > > +
> > > > + return ret;
> > > >
Re: [RFC PATCH v6 11/11] media: viaudm2m: add virtual driver for audio memory to memory
On 13/10/2023 10:31, Shengjiu Wang wrote:
> Audio memory to memory virtual driver use video memory to memory
> virtual driver vim2m.c as example. The main difference is
> device type is VFL_TYPE_AUDIO and device cap type is V4L2_CAP_AUDIO_M2M.
>
> The device_run function is a dummy function, which is simply
> copy the data from input buffer to output buffer.
>
> Signed-off-by: Shengjiu Wang
> ---
> drivers/media/test-drivers/Kconfig| 9 +
> drivers/media/test-drivers/Makefile | 1 +
> drivers/media/test-drivers/viaudm2m.c | 707 ++
> 3 files changed, 717 insertions(+)
> create mode 100644 drivers/media/test-drivers/viaudm2m.c
>
> diff --git a/drivers/media/test-drivers/Kconfig
> b/drivers/media/test-drivers/Kconfig
> index 459b433e9fae..143d829e1b67 100644
> --- a/drivers/media/test-drivers/Kconfig
> +++ b/drivers/media/test-drivers/Kconfig
> @@ -17,6 +17,15 @@ config VIDEO_VIM2M
> This is a virtual test device for the memory-to-memory driver
> framework.
>
> +config VIDEO_VIAUDM2M
> + tristate "Virtual Memory-to-Memory Driver For Audio"
I think this is better:
VIDEO_VIM2M_AUDIO
and name the source: vim2m_audio.c.
> + depends on VIDEO_DEV
> + select VIDEOBUF2_VMALLOC
> + select V4L2_MEM2MEM_DEV
> + help
> + This is a virtual audio test device for the memory-to-memory driver
> + framework.
> +
> source "drivers/media/test-drivers/vicodec/Kconfig"
> source "drivers/media/test-drivers/vimc/Kconfig"
> source "drivers/media/test-drivers/vivid/Kconfig"
> diff --git a/drivers/media/test-drivers/Makefile
> b/drivers/media/test-drivers/Makefile
> index 740714a4584d..a39d27eb53fa 100644
> --- a/drivers/media/test-drivers/Makefile
> +++ b/drivers/media/test-drivers/Makefile
> @@ -10,6 +10,7 @@ obj-$(CONFIG_DVB_VIDTV) += vidtv/
>
> obj-$(CONFIG_VIDEO_VICODEC) += vicodec/
> obj-$(CONFIG_VIDEO_VIM2M) += vim2m.o
> +obj-$(CONFIG_VIDEO_VIAUDM2M) += viaudm2m.o
> obj-$(CONFIG_VIDEO_VIMC) += vimc/
> obj-$(CONFIG_VIDEO_VIVID) += vivid/
> obj-$(CONFIG_VIDEO_VISL) += visl/
> diff --git a/drivers/media/test-drivers/viaudm2m.c
> b/drivers/media/test-drivers/viaudm2m.c
> new file mode 100644
> index ..5fefd616b6c8
> --- /dev/null
> +++ b/drivers/media/test-drivers/viaudm2m.c
> @@ -0,0 +1,707 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * A virtual v4l2-mem2mem example for audio device.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +MODULE_DESCRIPTION("Virtual device for audio mem2mem testing");
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION("0.1");
Drop this, rarely needed.
> +MODULE_ALIAS("audio_mem2mem_testdev");
Drop this as well. I'm not sure why it is in the original vim2m.c source.
> +
> +static unsigned int debug;
> +module_param(debug, uint, 0644);
> +MODULE_PARM_DESC(debug, "debug level");
> +
> +#define MEM2MEM_NAME "viaudm2m"
"vim2m-audio"
> +
> +#define dprintk(dev, lvl, fmt, arg...) \
> + v4l2_dbg(lvl, debug, &(dev)->v4l2_dev, "%s: " fmt, __func__, ## arg)
> +
> +#define SAMPLE_NUM 4096
> +
> +static void audm2m_dev_release(struct device *dev)
> +{}
> +
> +static struct platform_device audm2m_pdev = {
> + .name = MEM2MEM_NAME,
> + .dev.release= audm2m_dev_release,
> +};
> +
> +struct audm2m_fmt {
> + u32 fourcc;
> + snd_pcm_format_tformat;
Same as for the previous driver: I think you can just drop this field and thus
the whole struct.
> +};
> +
> +static struct audm2m_fmt formats[] = {
And this is just a u32 array.
> + {
> + .fourcc = V4L2_AUDIO_FMT_S8,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S16_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U16_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S24_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S24_3LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U24_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U24_3LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S32_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U32_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S20_3LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U20_3LE,
> + },
> +};
> +
> +#define NUM_FORMATS ARRAY_SIZE(formats)
> +
> +/* Per-queue, driver-specific private data */
> +struct audm2m_q_data {
> + unsigned intrate;
> + unsigned intchannels;
> + unsigned intbuffersize;
> + struct audm2m_fmt *fmt;
> +};
> +
> +enum {
> + V4L2_M2M_SRC = 0,
> + V4L2_M2M_DST = 1,
> +};
> +
> +static struct audm2m_fmt *find_format(u32 fourcc)
> +{
> + struct audm2m_fmt *fmt;
> + unsigned int k;
> +
> + for (k = 0; k < NUM_FORMATS; k++) {
>
Re: [RFC PATCH v6 10/11] media: imx-asrc: Add memory to memory driver
On 13/10/2023 10:31, Shengjiu Wang wrote:
> Implement the ASRC memory to memory function using
> the v4l2 framework, user can use this function with
> v4l2 ioctl interface.
>
> User send the output and capture buffer to driver and
> driver store the converted data to the capture buffer.
>
> This feature can be shared by ASRC and EASRC drivers
>
> Signed-off-by: Shengjiu Wang
> ---
> drivers/media/platform/nxp/Kconfig| 12 +
> drivers/media/platform/nxp/Makefile |1 +
> drivers/media/platform/nxp/imx-asrc.c | 1248 +
> 3 files changed, 1261 insertions(+)
> create mode 100644 drivers/media/platform/nxp/imx-asrc.c
>
> diff --git a/drivers/media/platform/nxp/Kconfig
> b/drivers/media/platform/nxp/Kconfig
> index 40e3436669e2..8234644ee341 100644
> --- a/drivers/media/platform/nxp/Kconfig
> +++ b/drivers/media/platform/nxp/Kconfig
> @@ -67,3 +67,15 @@ config VIDEO_MX2_EMMAPRP
>
> source "drivers/media/platform/nxp/dw100/Kconfig"
> source "drivers/media/platform/nxp/imx-jpeg/Kconfig"
> +
> +config VIDEO_IMX_ASRC
> + tristate "NXP i.MX ASRC M2M support"
> + depends on V4L_MEM2MEM_DRIVERS
> + depends on MEDIA_SUPPORT
> + select VIDEOBUF2_DMA_CONTIG
> + select V4L2_MEM2MEM_DEV
> + help
> + Say Y if you want to add ASRC M2M support for NXP CPUs.
> + It is a complement for ASRC M2P and ASRC P2M features.
> + This option is only useful for out-of-tree drivers since
> + in-tree drivers select it automatically.
> diff --git a/drivers/media/platform/nxp/Makefile
> b/drivers/media/platform/nxp/Makefile
> index 4d90eb713652..1325675e34f5 100644
> --- a/drivers/media/platform/nxp/Makefile
> +++ b/drivers/media/platform/nxp/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_VIDEO_IMX8MQ_MIPI_CSI2) += imx8mq-mipi-csi2.o
> obj-$(CONFIG_VIDEO_IMX_MIPI_CSIS) += imx-mipi-csis.o
> obj-$(CONFIG_VIDEO_IMX_PXP) += imx-pxp.o
> obj-$(CONFIG_VIDEO_MX2_EMMAPRP) += mx2_emmaprp.o
> +obj-$(CONFIG_VIDEO_IMX_ASRC) += imx-asrc.o
> diff --git a/drivers/media/platform/nxp/imx-asrc.c
> b/drivers/media/platform/nxp/imx-asrc.c
> new file mode 100644
> index ..373ca2b5ec90
> --- /dev/null
> +++ b/drivers/media/platform/nxp/imx-asrc.c
> @@ -0,0 +1,1248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +//
> +// Copyright (C) 2014-2016 Freescale Semiconductor, Inc.
> +// Copyright (C) 2019-2023 NXP
> +//
> +// Freescale ASRC Memory to Memory (M2M) driver
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define V4L_CAP OUT
> +#define V4L_OUT IN
> +
> +#define ASRC_xPUT_DMA_CALLBACK(dir) \
> + (((dir) == V4L_OUT) ? asrc_input_dma_callback \
> + : asrc_output_dma_callback)
> +
> +#define DIR_STR(dir) (dir) == V4L_OUT ? "out" : "cap"
> +
> +#define ASRC_M2M_BUFFER_SIZE (512 * 1024)
> +#define ASRC_M2M_PERIOD_SIZE (48 * 1024)
> +#define ASRC_M2M_SG_NUM (20)
Where do all these values come from? How do they relate?
Some comments would be welcome.
Esp. ASRC_M2M_SG_NUM is a bit odd.
> +
> +struct asrc_fmt {
> + u32 fourcc;
> + snd_pcm_format_t format;
Do you need this field? If not, then you can drop the whole
struct and just use u32 fourcc in the formats[] array.
> +};
> +
> +struct asrc_pair_m2m {
> + struct fsl_asrc_pair *pair;
> + struct asrc_m2m *m2m;
> + struct v4l2_fh fh;
> + struct v4l2_ctrl_handler ctrl_handler;
> + int channels[2];
> + struct v4l2_ctrl_fixed_point src_rate;
> + struct v4l2_ctrl_fixed_point dst_rate;
> +
> +};
> +
> +struct asrc_m2m {
> + struct fsl_asrc_m2m_pdata pdata;
> + struct v4l2_device v4l2_dev;
> + struct v4l2_m2m_dev *m2m_dev;
> + struct video_device *dec_vdev;
> + struct mutex mlock; /* v4l2 ioctls serialization */
> + struct platform_device *pdev;
> +};
> +
> +static struct asrc_fmt formats[] = {
> + {
> + .fourcc = V4L2_AUDIO_FMT_S8,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S16_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U16_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S24_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S24_3LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U24_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U24_3LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S32_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U32_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_S20_3LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_U20_3LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_FLOAT_LE,
> + },
> + {
> + .fourcc = V4L2_AUDIO_FMT_IEC958_SUBFRAME_LE,
> + },
> +};
> +
> +#define NUM_FORMATS ARRAY_SIZE(formats)
> +
> +static snd_pcm_format_t convert_fourcc(u32 fourcc) {
> +
> + return
Re: Re: [PATCH v3 1/2] ASoC: dt-bindings: sound-card-common: List DAPM endpoints ignoring system suspend
On Mon, Oct 16, 2023 at 12:08:56PM +, Chancel Liu wrote: > Thanks Mark and Rob for your advice. In fact, it's common use case. We can see > many drivers set widgets ignoring suspend. I will remove the linux specifics > and focus on the key concept. How about the modification on the property name > and description as following: > ignore-suspend-widgets: > description: | > A list of audio sound widgets which are marked ignoring system suspend. > Paths between these endpoints are still active over suspend of the > main > application processor that the current operating system is running. That's probably fine from my point of view. There's likely a better way of saying system suspend but I'm not immediately seeing it and it could always be improved later. signature.asc Description: PGP signature
Re: [RFC PATCH v6 09/11] media: uapi: Add audio rate controls support
Hi Shengjiu,
On 13/10/2023 10:31, Shengjiu Wang wrote:
> Fixed point controls are used by the user to configure
> the audio sample rate to driver.
>
> Add V4L2_CID_ASRC_SOURCE_RATE and V4L2_CID_ASRC_DEST_RATE
> new IDs for ASRC rate control.
>
> Signed-off-by: Shengjiu Wang
> ---
> .../userspace-api/media/v4l/common.rst| 1 +
> .../media/v4l/ext-ctrls-fixed-point.rst | 36 +++
> .../media/v4l/vidioc-g-ext-ctrls.rst | 4 +++
> .../media/v4l/vidioc-queryctrl.rst| 7
> .../media/videodev2.h.rst.exceptions | 1 +
> drivers/media/v4l2-core/v4l2-ctrls-core.c | 5 +++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 +++
> include/media/v4l2-ctrls.h| 2 ++
> include/uapi/linux/v4l2-controls.h| 13 +++
> include/uapi/linux/videodev2.h| 3 ++
> 10 files changed, 76 insertions(+)
> create mode 100644
> Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
>
> diff --git a/Documentation/userspace-api/media/v4l/common.rst
> b/Documentation/userspace-api/media/v4l/common.rst
> index ea0435182e44..35707edffb13 100644
> --- a/Documentation/userspace-api/media/v4l/common.rst
> +++ b/Documentation/userspace-api/media/v4l/common.rst
> @@ -52,6 +52,7 @@ applicable to all devices.
> ext-ctrls-fm-rx
> ext-ctrls-detect
> ext-ctrls-colorimetry
> +ext-ctrls-fixed-point
Rename this to ext-ctrls-audio-m2m.
> fourcc
> format
> planar-apis
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> new file mode 100644
> index ..2ef6e250580c
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-fixed-point.rst
> @@ -0,0 +1,36 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +
> +.. _fixed-point-controls:
> +
> +***
> +Fixed Point Control Reference
This is for audio controls. "Fixed Point" is just the type, and it doesn't make
sense to group fixed point controls. But it does make sense to group the audio
controls.
V4L2 controls can be grouped into classes. Basically it is a way to put controls
into categories, and for each category there is also a control that gives a
description of the class (see 2.15.15 in
https://linuxtv.org/downloads/v4l-dvb-apis-new/driver-api/v4l2-controls.html#introduction)
If you use e.g. 'v4l2-ctl -l' to list all the controls, then you will see that
they are grouped based on what class of control they are.
So I think it would be a good idea to create a new control class for M2M audio
controls,
instead of just adding them to the catch-all 'User Controls' class.
Search e.g. for V4L2_CTRL_CLASS_COLORIMETRY and V4L2_CID_COLORIMETRY_CLASS to
see how
it is done.
M2M_AUDIO would probably be a good name for the class.
> +***
> +
> +These controls are intended to support an asynchronous sample
> +rate converter.
Add ' (ASRC).' at the end to indicate the common abbreviation for
that.
> +
> +.. _v4l2-audio-asrc:
> +
> +``V4L2_CID_ASRC_SOURCE_RATE``
> +sets the resampler source rate.
> +
> +``V4L2_CID_ASRC_DEST_RATE``
> +sets the resampler destination rate.
Document the unit (Hz) for these two controls.
> +
> +.. c:type:: v4l2_ctrl_fixed_point
> +
> +.. cssclass:: longtable
> +
> +.. tabularcolumns:: |p{1.5cm}|p{5.8cm}|p{10.0cm}|
> +
> +.. flat-table:: struct v4l2_ctrl_fixed_point
> +:header-rows: 0
> +:stub-columns: 0
> +:widths: 1 1 2
> +
> +* - __u32
Hmm, shouldn't this be __s32?
> + - ``integer``
> + - integer part of fixed point value.
> +* - __s32
and this __u32?
You want to be able to use this generic type as a signed value.
> + - ``fractional``
> + - fractional part of fixed point value, which is Q31.
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> index f9f73530a6be..1811dabf5c74 100644
> --- a/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> +++ b/Documentation/userspace-api/media/v4l/vidioc-g-ext-ctrls.rst
> @@ -295,6 +295,10 @@ still cause this situation.
>- ``p_av1_film_grain``
>- A pointer to a struct :c:type:`v4l2_ctrl_av1_film_grain`. Valid if
> this control is
> of type ``V4L2_CTRL_TYPE_AV1_FILM_GRAIN``.
> +* - struct :c:type:`v4l2_ctrl_fixed_point` *
> + - ``p_fixed_point``
> + - A pointer to a struct :c:type:`v4l2_ctrl_fixed_point`. Valid if this
> control is
> +of type ``V4L2_CTRL_TYPE_FIXED_POINT``.
> * - void *
>- ``ptr``
>- A pointer to a compound type which can be an N-dimensional array
> diff --git a/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> b/Documentation/userspace-api/media/v4l/vidioc-queryctrl.rst
> index 4d38acafe8e1..9285f4f39eed 100644
> ---
Re: [PATCH v2 3/7] powerpc/rtas: serialize ibm,get-vpd service with papr-vpd sequences
> Take the papr-vpd driver's internal mutex when sys_rtas performs
> ibm,get-vpd calls. This prevents sys_rtas(ibm,get-vpd) calls from
> interleaving with sequences performed by the driver, ensuring that
> such sequences are not disrupted.
> @@ -1861,6 +1862,28 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
> goto copy_return;
> }
>
> + if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD)) {
> + /*
> + * ibm,get-vpd potentially needs to be invoked
> + * multiple times to obtain complete results.
> + * Interleaved ibm,get-vpd sequences disrupt each
> + * other.
> + *
> + * /dev/papr-vpd doesn't have this problem and users
> + * do not need to be aware of each other to use it
> + * safely.
> + *
> + * We can prevent this call from disrupting a
> + * /dev/papr-vpd-initiated sequence in progress by
> + * reaching into the driver to take its internal
> + * lock. Unfortunately there is no way to prevent
> + * interference in the other direction without
> + * resorting to even worse hacks.
> + */
> + pr_notice_once("Calling ibm,get-vpd via sys_rtas is allowed but
> deprecated. Use /dev/papr-vpd instead.\n");
> + papr_vpd_mutex_lock();
> + }
> +
> buff_copy = get_errorlog_buffer();
>
> raw_spin_lock_irqsave(_lock, flags);
> @@ -1870,6 +1893,9 @@ SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
> do_enter_rtas(_args);
> args = rtas_args;
>
> + if (token == rtas_function_token(RTAS_FN_IBM_GET_VPD))
> + papr_vpd_mutex_unlock();
> +
The mutex ought to nest entirely outside rtas_lock, this releases it
too early.
Anyway, I'm considering a different way to get the synchronization right
between drivers like papr-vpd and sys_rtas. Instead of having sys_rtas
acquire the driver's internal lock, rtas.c should provide a way for code
like papr-vpd to temporarily lock the syscall path.
Something like this:
// rtas.c
+ static DEFINE_MUTEX(rtas_syscall_lock);
+ void rtas_syscall_lock(void) { mutex_lock(_syscall_lock); }
+ void rtas_syscall_unlock(void) { mutex_unlock(_syscall_lock); }
SYSCALL_DEFINE1(rtas, struct rtas_args __user *, uargs)
{
+rtas_syscall_lock();
...
do_enter_rtas(_args);
...
+rtas_syscall_unlock();
return 0;
}
// papr-vpd.c
static void vpd_sequence_begin(struct vpd_sequence *seq,
const struct papr_location_code *loc_code)
{
static struct papr_location_code static_loc_code;
papr_vpd_mutex_lock();
+ rtas_syscall_lock();
static_loc_code = *loc_code;
*seq = (struct vpd_sequence) {
.params = {
.work_area = rtas_work_area_alloc(SZ_4K),
.loc_code = _loc_code,
.sequence = 1,
},
};
}
/**
* vpd_sequence_end() - Finalize a VPD retrieval sequence.
* @seq: Sequence state.
*
* Releases resources obtained by vpd_sequence_begin().
*/
static void vpd_sequence_end(struct vpd_sequence *seq)
{
rtas_work_area_free(seq->params.work_area);
+ rtas_syscall_unlock();
papr_vpd_mutex_unlock();
}
This is a sketch to communicate the idea. The locking in the real code
could be finer, perhaps a mutex per RTAS function.
Re: [PATCH] arch: powerpc: net: bpf_jit_comp32.c: Fixed 'instead' typo
On 10/13/23 7:31 AM, Muhammad Muzammil wrote: Fixed 'instead' typo Signed-off-by: Muhammad Muzammil Michael, I presume you'll pick it up? Thanks, Daniel --- arch/powerpc/net/bpf_jit_comp32.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/powerpc/net/bpf_jit_comp32.c b/arch/powerpc/net/bpf_jit_comp32.c index 7f91ea064c08..bc7f92ec7f2d 100644 --- a/arch/powerpc/net/bpf_jit_comp32.c +++ b/arch/powerpc/net/bpf_jit_comp32.c @@ -940,7 +940,7 @@ int bpf_jit_build_body(struct bpf_prog *fp, u32 *image, struct codegen_context * * !fp->aux->verifier_zext. Emit NOP otherwise. * * Note that "li reg_h,0" is emitted for BPF_B/H/W case, -* if necessary. So, jump there insted of emitting an +* if necessary. So, jump there instead of emitting an * additional "li reg_h,0" instruction. */ if (size == BPF_DW && !fp->aux->verifier_zext)
[PATCH 6/6] powerpc/qspinlock: Rename yield_propagate_owner tunable
Rename yield_propagate_owner to yield_sleepy_owner, which better
describes what it does (what, not how).
Signed-off-by: Nicholas Piggin
---
arch/powerpc/lib/qspinlock.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index c68c2bd7b853..5de4dd549f6e 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -44,7 +44,7 @@ static bool pv_sleepy_lock_sticky __read_mostly = false;
static u64 pv_sleepy_lock_interval_ns __read_mostly = 0;
static int pv_sleepy_lock_factor __read_mostly = 256;
static bool pv_yield_prev __read_mostly = true;
-static bool pv_yield_propagate_owner __read_mostly = true;
+static bool pv_yield_sleepy_owner __read_mostly = true;
static bool pv_prod_head __read_mostly = false;
static DEFINE_PER_CPU_ALIGNED(struct qnodes, qnodes);
@@ -357,7 +357,7 @@ static __always_inline void propagate_sleepy(struct qnode
*node, u32 val, bool p
if (!paravirt)
return;
- if (!pv_yield_propagate_owner)
+ if (!pv_yield_sleepy_owner)
return;
next = READ_ONCE(node->next);
@@ -381,7 +381,7 @@ static __always_inline bool yield_to_prev(struct qspinlock
*lock, struct qnode *
if (!paravirt)
goto relax;
- if (!pv_yield_propagate_owner)
+ if (!pv_yield_sleepy_owner)
goto yield_prev;
/*
@@ -934,21 +934,21 @@ static int pv_yield_prev_get(void *data, u64 *val)
DEFINE_SIMPLE_ATTRIBUTE(fops_pv_yield_prev, pv_yield_prev_get,
pv_yield_prev_set, "%llu\n");
-static int pv_yield_propagate_owner_set(void *data, u64 val)
+static int pv_yield_sleepy_owner_set(void *data, u64 val)
{
- pv_yield_propagate_owner = !!val;
+ pv_yield_sleepy_owner = !!val;
return 0;
}
-static int pv_yield_propagate_owner_get(void *data, u64 *val)
+static int pv_yield_sleepy_owner_get(void *data, u64 *val)
{
- *val = pv_yield_propagate_owner;
+ *val = pv_yield_sleepy_owner;
return 0;
}
-DEFINE_SIMPLE_ATTRIBUTE(fops_pv_yield_propagate_owner,
pv_yield_propagate_owner_get, pv_yield_propagate_owner_set, "%llu\n");
+DEFINE_SIMPLE_ATTRIBUTE(fops_pv_yield_sleepy_owner, pv_yield_sleepy_owner_get,
pv_yield_sleepy_owner_set, "%llu\n");
static int pv_prod_head_set(void *data, u64 val)
{
@@ -980,7 +980,7 @@ static __init int spinlock_debugfs_init(void)
debugfs_create_file("qspl_pv_sleepy_lock_interval_ns", 0600,
arch_debugfs_dir, NULL, _pv_sleepy_lock_interval_ns);
debugfs_create_file("qspl_pv_sleepy_lock_factor", 0600,
arch_debugfs_dir, NULL, _pv_sleepy_lock_factor);
debugfs_create_file("qspl_pv_yield_prev", 0600,
arch_debugfs_dir, NULL, _pv_yield_prev);
- debugfs_create_file("qspl_pv_yield_propagate_owner", 0600,
arch_debugfs_dir, NULL, _pv_yield_propagate_owner);
+ debugfs_create_file("qspl_pv_yield_sleepy_owner", 0600,
arch_debugfs_dir, NULL, _pv_yield_sleepy_owner);
debugfs_create_file("qspl_pv_prod_head", 0600,
arch_debugfs_dir, NULL, _pv_prod_head);
}
--
2.42.0
[PATCH 5/6] powerpc/qspinlock: Propagate sleepy if previous waiter is preempted
The sleepy (aka lock-owner-is-preempted) condition is propagated down
the queue by each waiter. If a waiter becomes preempted, it can no
longer propagate sleepy. To allow subsequent waiters to yield to the
lock owner, also check the lock owner in this case.
Signed-off-by: Nicholas Piggin
---
arch/powerpc/lib/qspinlock.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index 6bb627e90a32..c68c2bd7b853 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -384,7 +384,11 @@ static __always_inline bool yield_to_prev(struct qspinlock
*lock, struct qnode *
if (!pv_yield_propagate_owner)
goto yield_prev;
- if (node->sleepy) {
+ /*
+* If the previous waiter was preempted it might not be able to
+* propagate sleepy to us, so check the lock in that case too.
+*/
+ if (node->sleepy || vcpu_is_preempted(prev_cpu)) {
u32 val = READ_ONCE(lock->val);
if (val & _Q_LOCKED_VAL) {
--
2.42.0
[PATCH 4/6] powerpc/qspinlock: don't propagate the not-sleepy state
To simplify things, don't propagate the not-sleepy condition back down
the queue. Instead, have the waiters clear their own node->sleepy when
finding the lock owner is not preempted.
Signed-off-by: Nicholas Piggin
---
arch/powerpc/lib/qspinlock.c | 26 --
1 file changed, 8 insertions(+), 18 deletions(-)
diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index 0932d24a6b07..6bb627e90a32 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -350,7 +350,7 @@ static __always_inline bool
yield_head_to_locked_owner(struct qspinlock *lock, u
return __yield_to_locked_owner(lock, val, paravirt, mustq);
}
-static __always_inline void propagate_sleepy(struct qnode *node, u32 val, bool
*set_sleepy, bool paravirt)
+static __always_inline void propagate_sleepy(struct qnode *node, u32 val, bool
paravirt)
{
struct qnode *next;
int owner;
@@ -359,18 +359,17 @@ static __always_inline void propagate_sleepy(struct qnode
*node, u32 val, bool *
return;
if (!pv_yield_propagate_owner)
return;
- if (*set_sleepy)
- return;
next = READ_ONCE(node->next);
if (!next)
return;
+ if (next->sleepy)
+ return;
+
owner = get_owner_cpu(val);
- if (vcpu_is_preempted(owner)) {
+ if (vcpu_is_preempted(owner))
next->sleepy = 1;
- *set_sleepy = true;
- }
}
/* Called inside spin_begin() */
@@ -385,12 +384,7 @@ static __always_inline bool yield_to_prev(struct qspinlock
*lock, struct qnode *
if (!pv_yield_propagate_owner)
goto yield_prev;
- if (!READ_ONCE(node->sleepy)) {
- /* Propagate back sleepy==false */
- if (node->next && node->next->sleepy)
- node->next->sleepy = 0;
- goto yield_prev;
- } else {
+ if (node->sleepy) {
u32 val = READ_ONCE(lock->val);
if (val & _Q_LOCKED_VAL) {
@@ -410,6 +404,7 @@ static __always_inline bool yield_to_prev(struct qspinlock
*lock, struct qnode *
if (preempted)
return preempted;
}
+ node->sleepy = false;
}
yield_prev:
@@ -533,7 +528,6 @@ static __always_inline void
queued_spin_lock_mcs_queue(struct qspinlock *lock, b
bool sleepy = false;
bool mustq = false;
int idx;
- bool set_sleepy = false;
int iters = 0;
BUILD_BUG_ON(CONFIG_NR_CPUS >= (1U << _Q_TAIL_CPU_BITS));
@@ -591,10 +585,6 @@ static __always_inline void
queued_spin_lock_mcs_queue(struct qspinlock *lock, b
spec_barrier();
spin_end();
- /* Clear out stale propagated sleepy */
- if (paravirt && pv_yield_propagate_owner && node->sleepy)
- node->sleepy = 0;
-
smp_rmb(); /* acquire barrier for the mcs lock */
/*
@@ -636,7 +626,7 @@ static __always_inline void
queued_spin_lock_mcs_queue(struct qspinlock *lock, b
}
}
- propagate_sleepy(node, val, _sleepy, paravirt);
+ propagate_sleepy(node, val, paravirt);
preempted = yield_head_to_locked_owner(lock, val, paravirt);
if (!maybe_stealers)
continue;
--
2.42.0
[PATCH 3/6] powerpc/qspinlock: propagate owner preemptedness rather than CPU number
Rather than propagating the CPU number of the preempted lock owner,
just propagate whether the owner was preempted. Waiters must read the
lock value when yielding to it to prevent races anyway, so might as
well always load the owner CPU from the lock.
To further simplify the code, also don't propagate the -1 (or
sleepy=false in the new scheme) down the queue. Instead, have the
waiters clear it themselves when finding the lock owner is not
preempted.
Signed-off-by: Nicholas Piggin
---
arch/powerpc/lib/qspinlock.c | 80
1 file changed, 36 insertions(+), 44 deletions(-)
diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index 75608ced14c2..0932d24a6b07 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -16,7 +16,8 @@ struct qnode {
struct qnode*next;
struct qspinlock *lock;
int cpu;
- int yield_cpu;
+ u8 sleepy; /* 1 if the previous vCPU was preempted or
+* if the previous node was sleepy */
u8 locked; /* 1 if lock acquired */
};
@@ -349,7 +350,7 @@ static __always_inline bool
yield_head_to_locked_owner(struct qspinlock *lock, u
return __yield_to_locked_owner(lock, val, paravirt, mustq);
}
-static __always_inline void propagate_yield_cpu(struct qnode *node, u32 val,
int *set_yield_cpu, bool paravirt)
+static __always_inline void propagate_sleepy(struct qnode *node, u32 val, bool
*set_sleepy, bool paravirt)
{
struct qnode *next;
int owner;
@@ -358,21 +359,17 @@ static __always_inline void propagate_yield_cpu(struct
qnode *node, u32 val, int
return;
if (!pv_yield_propagate_owner)
return;
-
- owner = get_owner_cpu(val);
- if (*set_yield_cpu == owner)
+ if (*set_sleepy)
return;
next = READ_ONCE(node->next);
if (!next)
return;
+ owner = get_owner_cpu(val);
if (vcpu_is_preempted(owner)) {
- next->yield_cpu = owner;
- *set_yield_cpu = owner;
- } else if (*set_yield_cpu != -1) {
- next->yield_cpu = owner;
- *set_yield_cpu = owner;
+ next->sleepy = 1;
+ *set_sleepy = true;
}
}
@@ -380,7 +377,6 @@ static __always_inline void propagate_yield_cpu(struct
qnode *node, u32 val, int
static __always_inline bool yield_to_prev(struct qspinlock *lock, struct qnode
*node, int prev_cpu, bool paravirt)
{
u32 yield_count;
- int yield_cpu;
bool preempted = false;
if (!paravirt)
@@ -389,36 +385,32 @@ static __always_inline bool yield_to_prev(struct
qspinlock *lock, struct qnode *
if (!pv_yield_propagate_owner)
goto yield_prev;
- yield_cpu = READ_ONCE(node->yield_cpu);
- if (yield_cpu == -1) {
- /* Propagate back the -1 CPU */
- if (node->next && node->next->yield_cpu != -1)
- node->next->yield_cpu = yield_cpu;
+ if (!READ_ONCE(node->sleepy)) {
+ /* Propagate back sleepy==false */
+ if (node->next && node->next->sleepy)
+ node->next->sleepy = 0;
goto yield_prev;
- }
-
- yield_count = yield_count_of(yield_cpu);
- if ((yield_count & 1) == 0)
- goto yield_prev; /* owner vcpu is running */
-
- if (get_owner_cpu(READ_ONCE(lock->val)) != yield_cpu)
- goto yield_prev; /* re-sample lock owner */
-
- spin_end();
-
- preempted = true;
- seen_sleepy_node();
-
- smp_rmb();
+ } else {
+ u32 val = READ_ONCE(lock->val);
+
+ if (val & _Q_LOCKED_VAL) {
+ if (node->next && !node->next->sleepy) {
+ /*
+* Propagate sleepy to next waiter. Only if
+* owner is preempted, which allows the queue
+* to become "non-sleepy" if vCPU preemption
+* ceases to occur, even if the lock remains
+* highly contended.
+*/
+ if (vcpu_is_preempted(get_owner_cpu(val)))
+ node->next->sleepy = 1;
+ }
- if (yield_cpu == node->yield_cpu) {
- if (node->next && node->next->yield_cpu != yield_cpu)
- node->next->yield_cpu = yield_cpu;
- yield_to_preempted(yield_cpu, yield_count);
- spin_begin();
- return preempted;
+ preempted = yield_to_locked_owner(lock, val, paravirt);
+ if (preempted)
+ return preempted;
+
[PATCH 2/6] powerpc/qspinlock: stop queued waiters trying to set lock sleepy
If a queued waiter notices the lock owner or the previous waiter has
been preempted, it attempts to mark the lock sleepy, but it does this
as a try-set operation using the original lock value it got when
queueing, which will become stale as the queue progresses, and the
try-set will fail. Drop this and just set the sleepy seen clock.
Signed-off-by: Nicholas Piggin
---
arch/powerpc/lib/qspinlock.c | 24 ++--
1 file changed, 10 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index 6dd2f46bd3ef..75608ced14c2 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -247,22 +247,18 @@ static __always_inline void seen_sleepy_lock(void)
this_cpu_write(sleepy_lock_seen_clock, sched_clock());
}
-static __always_inline void seen_sleepy_node(struct qspinlock *lock, u32 val)
+static __always_inline void seen_sleepy_node(void)
{
if (pv_sleepy_lock) {
if (pv_sleepy_lock_interval_ns)
this_cpu_write(sleepy_lock_seen_clock, sched_clock());
- if (val & _Q_LOCKED_VAL) {
- if (!(val & _Q_SLEEPY_VAL))
- try_set_sleepy(lock, val);
- }
+ /* Don't set sleepy because we likely have a stale val */
}
}
-static struct qnode *get_tail_qnode(struct qspinlock *lock, u32 val)
+static struct qnode *get_tail_qnode(struct qspinlock *lock, int prev_cpu)
{
- int cpu = decode_tail_cpu(val);
- struct qnodes *qnodesp = per_cpu_ptr(, cpu);
+ struct qnodes *qnodesp = per_cpu_ptr(, prev_cpu);
int idx;
/*
@@ -381,9 +377,8 @@ static __always_inline void propagate_yield_cpu(struct
qnode *node, u32 val, int
}
/* Called inside spin_begin() */
-static __always_inline bool yield_to_prev(struct qspinlock *lock, struct qnode
*node, u32 val, bool paravirt)
+static __always_inline bool yield_to_prev(struct qspinlock *lock, struct qnode
*node, int prev_cpu, bool paravirt)
{
- int prev_cpu = decode_tail_cpu(val);
u32 yield_count;
int yield_cpu;
bool preempted = false;
@@ -412,7 +407,7 @@ static __always_inline bool yield_to_prev(struct qspinlock
*lock, struct qnode *
spin_end();
preempted = true;
- seen_sleepy_node(lock, val);
+ seen_sleepy_node();
smp_rmb();
@@ -436,7 +431,7 @@ static __always_inline bool yield_to_prev(struct qspinlock
*lock, struct qnode *
spin_end();
preempted = true;
- seen_sleepy_node(lock, val);
+ seen_sleepy_node();
smp_rmb(); /* See __yield_to_locked_owner comment */
@@ -587,7 +582,8 @@ static __always_inline void
queued_spin_lock_mcs_queue(struct qspinlock *lock, b
* head of the waitqueue.
*/
if (old & _Q_TAIL_CPU_MASK) {
- struct qnode *prev = get_tail_qnode(lock, old);
+ int prev_cpu = decode_tail_cpu(old);
+ struct qnode *prev = get_tail_qnode(lock, prev_cpu);
/* Link @node into the waitqueue. */
WRITE_ONCE(prev->next, node);
@@ -597,7 +593,7 @@ static __always_inline void
queued_spin_lock_mcs_queue(struct qspinlock *lock, b
while (!READ_ONCE(node->locked)) {
spec_barrier();
- if (yield_to_prev(lock, node, old, paravirt))
+ if (yield_to_prev(lock, node, prev_cpu, paravirt))
seen_preempted = true;
}
spec_barrier();
--
2.42.0
[PATCH 1/6] powerpc/qspinlock: Fix stale propagated yield_cpu
yield_cpu is a sample of a preempted lock holder that gets propagated
back through the queue. Queued waiters use this to yield to the
preempted lock holder without continually sampling the lock word (which
would defeat the purpose of MCS queueing by bouncing the cache line).
The problem is that yield_cpu can become stale. It can take some time to
be passed down the chain, and if any queued waiter gets preempted then
it will cease to propagate the yield_cpu to later waiters.
This can result in yielding to a CPU that no longer holds the lock,
which is bad, but particularly if it is currently in H_CEDE (idle),
then it appears to be preempted and some hypervisors (PowerVM) can
cause very long H_CONFER latencies waiting for H_CEDE wakeup. This
results in latency spikes and hard lockups on oversubscribed
partitions with lock contention.
This is a minimal fix. Before yielding to yield_cpu, sample the lock
word to confirm yield_cpu is still the owner, and bail out of it is not.
Thanks to a bunch of people who reported this and tracked down the
exact problem using tracepoints and dispatch trace logs.
Fixes: 28db61e207ea ("powerpc/qspinlock: allow propagation of yield CPU down
the queue")
Reported-by: Srikar Dronamraju
Reported-by: Laurent Dufour
Reported-by: Shrikanth Hegde
Debugged-by: Nysal Jan K.A
Signed-off-by: Nicholas Piggin
---
arch/powerpc/lib/qspinlock.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/powerpc/lib/qspinlock.c b/arch/powerpc/lib/qspinlock.c
index 253620979d0c..6dd2f46bd3ef 100644
--- a/arch/powerpc/lib/qspinlock.c
+++ b/arch/powerpc/lib/qspinlock.c
@@ -406,6 +406,9 @@ static __always_inline bool yield_to_prev(struct qspinlock
*lock, struct qnode *
if ((yield_count & 1) == 0)
goto yield_prev; /* owner vcpu is running */
+ if (get_owner_cpu(READ_ONCE(lock->val)) != yield_cpu)
+ goto yield_prev; /* re-sample lock owner */
+
spin_end();
preempted = true;
--
2.42.0
[PATCH 0/6] powerpc/qspinlock: Fix yield latency bug and other
This fixes a long-standing latency bug in the powerpc qspinlock implementation that quite a few people have reported and helped out with debugging. The first patch is a minimal fix that avoids the problem. The other patches are streamlining and improvements after the fix. Thanks, Nick Nicholas Piggin (6): powerpc/qspinlock: Fix stale propagated yield_cpu powerpc/qspinlock: stop queued waiters trying to set lock sleepy powerpc/qspinlock: propagate owner preemptedness rather than CPU number powerpc/qspinlock: don't propagate the not-sleepy state powerpc/qspinlock: Propagate sleepy if previous waiter is preempted powerpc/qspinlock: Rename yield_propagate_owner tunable arch/powerpc/lib/qspinlock.c | 119 +++ 1 file changed, 52 insertions(+), 67 deletions(-) -- 2.42.0
Re: [RFC PATCH v6 08/11] media: uapi: define audio sample format fourcc type
On 13/10/2023 10:31, Shengjiu Wang wrote:
> The audio sample format definition is from alsa,
> the header file is include/uapi/sound/asound.h, but
> don't include this header file directly, because in
> user space, there is another copy in alsa-lib.
> There will be conflict in userspace for include
> videodev2.h & asound.h and asoundlib.h
>
> Here still use the fourcc format.
>
> Signed-off-by: Shengjiu Wang
> ---
> .../userspace-api/media/v4l/pixfmt-audio.rst | 202 ++
> .../userspace-api/media/v4l/pixfmt.rst| 1 +
> drivers/media/v4l2-core/v4l2-ioctl.c | 36
> include/uapi/linux/videodev2.h| 48 +
> 4 files changed, 287 insertions(+)
> create mode 100644 Documentation/userspace-api/media/v4l/pixfmt-audio.rst
>
> diff --git a/Documentation/userspace-api/media/v4l/pixfmt-audio.rst
> b/Documentation/userspace-api/media/v4l/pixfmt-audio.rst
> new file mode 100644
> index ..ac89b2c4b594
> --- /dev/null
> +++ b/Documentation/userspace-api/media/v4l/pixfmt-audio.rst
> @@ -0,0 +1,202 @@
> +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later
> +
> +.. _pixfmt-audio:
> +
> +*
> +Audio Formats
> +*
> +
> +These formats are used for :ref:`audiomem2mem` interface only.
> +
> +.. tabularcolumns:: |p{5.8cm}|p{1.2cm}|p{10.3cm}|
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: Audio Format
> +:header-rows: 1
> +:stub-columns: 0
> +:widths: 3 1 4
> +
> +* - Identifier
> + - Code
> + - Details
> +* .. _V4L2-AUDIO-FMT-S8:
> +
> + - ``V4L2_AUDIO_FMT_S8``
> + - 'S8'
> + - Corresponds to SNDRV_PCM_FORMAT_S8 in ALSA
> +* .. _V4L2-AUDIO-FMT-U8:
> +
> + - ``V4L2_AUDIO_FMT_U8``
> + - 'U8'
> + - Corresponds to SNDRV_PCM_FORMAT_U8 in ALSA
> +* .. _V4L2-AUDIO-FMT-S16-LE:
> +
> + - ``V4L2_AUDIO_FMT_S16_LE``
> + - 'S16_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_S16_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-S16-BE:
> +
> + - ``V4L2_AUDIO_FMT_S16_BE``
> + - 'S16_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_S16_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-U16-LE:
> +
> + - ``V4L2_AUDIO_FMT_U16_LE``
> + - 'U16_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_U16_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-U16-BE:
> +
> + - ``V4L2_AUDIO_FMT_U16_BE``
> + - 'U16_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_U16_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-S24-LE:
> +
> + - ``V4L2_AUDIO_FMT_S24_LE``
> + - 'S24_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_S24_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-S24-BE:
> +
> + - ``V4L2_AUDIO_FMT_S24_BE``
> + - 'S24_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_S24_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-U24-LE:
> +
> + - ``V4L2_AUDIO_FMT_U24_LE``
> + - 'U24_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_U24_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-U24-BE:
> +
> + - ``V4L2_AUDIO_FMT_U24_BE``
> + - 'U24_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_U24_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-S32-LE:
> +
> + - ``V4L2_AUDIO_FMT_S32_LE``
> + - 'S32_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_S32_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-S32-BE:
> +
> + - ``V4L2_AUDIO_FMT_S32_BE``
> + - 'S32_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_S32_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-U32-LE:
> +
> + - ``V4L2_AUDIO_FMT_U32_LE``
> + - 'U32_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_U32_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-U32-BE:
> +
> + - ``V4L2_AUDIO_FMT_U32_BE``
> + - 'U32_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_U32_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-FLOAT-LE:
> +
> + - ``V4L2_AUDIO_FMT_FLOAT_LE``
> + - 'FLOAT_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_FLOAT_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-FLOAT-BE:
> +
> + - ``V4L2_AUDIO_FMT_FLOAT_BE``
> + - 'FLOAT_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_FLOAT_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-FLOAT64-LE:
> +
> + - ``V4L2_AUDIO_FMT_FLOAT64_LE``
> + - 'FLOAT64_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_FLOAT64_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-FLOAT64-BE:
> +
> + - ``V4L2_AUDIO_FMT_FLOAT64_BE``
> + - 'FLOAT64_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_FLOAT64_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-IEC958-SUBFRAME-LE:
> +
> + - ``V4L2_AUDIO_FMT_IEC958_SUBFRAME_LE``
> + - 'IEC958_SUBFRAME_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_IEC958_SUBFRAME_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-IEC958-SUBFRAME-BE:
> +
> + - ``V4L2_AUDIO_FMT_IEC958_SUBFRAME_BE``
> + - 'IEC958_SUBFRAME_BE'
> + - Corresponds to SNDRV_PCM_FORMAT_IEC958_SUBFRAME_BE in ALSA
> +* .. _V4L2-AUDIO-FMT-S20-LE:
> +
> + - ``V4L2_AUDIO_FMT_S20_LE``
> + - 'S20_LE'
> + - Corresponds to SNDRV_PCM_FORMAT_S20_LE in ALSA
> +* .. _V4L2-AUDIO-FMT-S20-BE:
> +
> + - ``V4L2_AUDIO_FMT_S20_BE``
>
Re: [PATCH v6 0/5] powerpc/bpf: use BPF prog pack allocator
On 10/12/23 10:03 PM, Hari Bathini wrote: Most BPF programs are small, but they consume a page each. For systems with busy traffic and many BPF programs, this may also add significant pressure on instruction TLB. High iTLB pressure usually slows down the whole system causing visible performance degradation for production workloads. bpf_prog_pack, a customized allocator that packs multiple bpf programs into preallocated memory chunks, was proposed [1] to address it. This series extends this support on powerpc. Both bpf_arch_text_copy() & bpf_arch_text_invalidate() functions, needed for this support depend on instruction patching in text area. Currently, patch_instruction() supports patching only one instruction at a time. The first patch introduces patch_instructions() function to enable patching more than one instruction at a time. This helps in avoiding performance degradation while JITing bpf programs. Patches 2 & 3 implement the above mentioned arch specific functions using patch_instructions(). Patch 4 fixes a misnomer in bpf JITing code. The last patch enables the use of BPF prog pack allocator on powerpc and also, ensures cleanup is handled gracefully. [1] https://lore.kernel.org/bpf/20220204185742.271030-1-s...@kernel.org/ Changes in v6: * No changes in patches 2-5/5 except addition of Acked-by tags from Song. * Skipped merging code path of patch_instruction() & patch_instructions() to avoid performance overhead observed on ppc32 with that. I presume this will be routed via Michael? Thanks, Daniel
Re: [RFC PATCH v6 07/11] media: v4l2: Add audio capture and output support
On 13/10/2023 10:31, Shengjiu Wang wrote: > Audio signal processing has the requirement for memory to > memory similar as Video. > > This patch is to add this support in v4l2 framework, defined > new buffer type V4L2_BUF_TYPE_AUDIO_CAPTURE and > V4L2_BUF_TYPE_AUDIO_OUTPUT, defined new format v4l2_audio_format > for audio case usage. > > The created audio device is named "/dev/v4l-audioX". > > Signed-off-by: Shengjiu Wang > --- > .../userspace-api/media/v4l/buffer.rst| 6 ++ > .../media/v4l/dev-audio-mem2mem.rst | 71 +++ > .../userspace-api/media/v4l/devices.rst | 1 + > .../media/v4l/vidioc-enum-fmt.rst | 2 + > .../userspace-api/media/v4l/vidioc-g-fmt.rst | 4 ++ > .../media/videodev2.h.rst.exceptions | 2 + > .../media/common/videobuf2/videobuf2-v4l2.c | 4 ++ > drivers/media/v4l2-core/v4l2-dev.c| 17 + > drivers/media/v4l2-core/v4l2-ioctl.c | 53 ++ > include/media/v4l2-dev.h | 2 + > include/media/v4l2-ioctl.h| 34 + > include/uapi/linux/videodev2.h| 17 + > 12 files changed, 213 insertions(+) > create mode 100644 > Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst > > diff --git a/Documentation/userspace-api/media/v4l/buffer.rst > b/Documentation/userspace-api/media/v4l/buffer.rst > index 04dec3e570ed..80cf2cb20dfe 100644 > --- a/Documentation/userspace-api/media/v4l/buffer.rst > +++ b/Documentation/userspace-api/media/v4l/buffer.rst > @@ -438,6 +438,12 @@ enum v4l2_buf_type > * - ``V4L2_BUF_TYPE_META_OUTPUT`` >- 14 >- Buffer for metadata output, see :ref:`metadata`. > +* - ``V4L2_BUF_TYPE_AUDIO_CAPTURE`` > + - 15 > + - Buffer for audio capture, see :ref:`audio`. > +* - ``V4L2_BUF_TYPE_AUDIO_OUTPUT`` > + - 16 > + - Buffer for audio output, see :ref:`audio`. > > > .. _buffer-flags: > diff --git a/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst > b/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst > new file mode 100644 > index ..2ea493d0a73b > --- /dev/null > +++ b/Documentation/userspace-api/media/v4l/dev-audio-mem2mem.rst > @@ -0,0 +1,71 @@ > +.. SPDX-License-Identifier: GFDL-1.1-no-invariants-or-later > + > +.. _audiomem2mem: > + > + > +Audio Memory-To-Memory Interface > + > + > +A audio memory-to-memory device can compress, decompress, transform, or A -> An > +otherwise convert audio data from one format into another format, in memory. > +Such memory-to-memory devices set the ``V4L2_CAP_AUDIO_M2M`` capability. > +Examples of memory-to-memory devices are codecs, audio preprocessing, codecs -> audio codecs Reason: within V4L2 'codec' refers to a video codec by default, so for audio codecs it is better to be explicit and mention 'audio'. > +audio postprocessing. > + > +A memory-to-memory audio node supports both output (sending frames from I think it is better to write 'audio frames' instead of just 'frames', for the same reason as why I prefer 'audio codec'. It makes it explicit that we are dealing with audio. > +memory to the hardware) and capture (receiving the processed frames audio frames > +from the hardware into memory) stream I/O. An application will have to > +setup the stream I/O for both sides and finally call > +:ref:`VIDIOC_STREAMON ` for both capture and output to > +start the hardware. > + > +Memory-to-memory devices function as a shared resource: you can > +open the audio node multiple times, each application setting up their > +own properties that are local to the file handle, and each can use > +it independently from the others. The driver will arbitrate access to > +the hardware and reprogram it whenever another file handler gets access. > + > +Audio memory-to-memory devices are accessed through character device > +special files named ``/dev/v4l-audio`` > + > +Querying Capabilities > += > + > +Device nodes supporting the audio memory-to-memory interface set the > +``V4L2_CAP_AUDIO_M2M`` flag in the ``device_caps`` field of the > +:c:type:`v4l2_capability` structure returned by the :c:func:`VIDIOC_QUERYCAP` > +ioctl. > + > +Data Format Negotiation > +=== > + > +The audio device uses the :ref:`format` ioctls to select the capture format. > +The audio buffer content format is bound to that selected format. In addition > +to the basic :ref:`format` ioctls, the :c:func:`VIDIOC_ENUM_FMT` ioctl must > be > +supported as well. > + > +To use the :ref:`format` ioctls applications set the ``type`` field of the > +:c:type:`v4l2_format` structure to ``V4L2_BUF_TYPE_AUDIO_CAPTURE`` or to > +``V4L2_BUF_TYPE_AUDIO_OUTPUT``. Both drivers and applications must set the > +remainder of the :c:type:`v4l2_format` structure to 0. > + > +.. c:type:: v4l2_audio_format > + > +.. tabularcolumns::
RE: Re: [PATCH v3 1/2] ASoC: dt-bindings: sound-card-common: List DAPM endpoints ignoring system suspend
> > I think the key thing > > here is that these are endpoints that can be active over suspend of > > the main application processor that the current operating system is > > running (system DT stuff is an interesting corner case here...), and > > the example is probably a bit specific. Other bindings use "audio sound > widgets" > > rather than "DAPM widgets". > > > > We also shouldn't see that these endpoints "should not be disabled" > > since that implies that they should be left on even if they aren't > > active which isn't quite the case, instead it's that we can continue > > playing an audio stream through them in suspend. > > This seems like one of those things that everyone has/does, and everyone > handles it a bit differently. I applaud trying to do something common, but it > isn't really common until we have multiple users. > > Rob Thanks Mark and Rob for your advice. In fact, it's common use case. We can see many drivers set widgets ignoring suspend. I will remove the linux specifics and focus on the key concept. How about the modification on the property name and description as following: ignore-suspend-widgets: description: | A list of audio sound widgets which are marked ignoring system suspend. Paths between these endpoints are still active over suspend of the main application processor that the current operating system is running. Regards, Chancel Liu
Re: [PATCH v2 9/9] iommu/dart: Remove the force_bypass variable
Hej,
On Thu, Sep 28, 2023, at 01:47, Jason Gunthorpe wrote:
> This flag just caches if the IO page size is larger than the CPU
> PAGE_SIZE. This only needs to be checked in two places so remove the
> confusingly named cache.
>
> dart would like to not support paging domains at all if the IO page size
> is larger than the CPU page size. In this case we should ideally fail
> domain_alloc_paging(), as there is no point in creating a domain that can
> never be attached. Move the test into apple_dart_finalize_domain().
>
> The check in apple_dart_mod_streams() will prevent the domain from being
> attached to the wrong dart
>
> There is no HW limitation that prevents BLOCKED domains from working,
> remove that test.
>
> The check in apple_dart_of_xlate() is redundant since immediately after
> the pgsize is checked. Remove it.
>
> Remove the variable.
>
> Suggested-by: Janne Grunau
> Signed-off-by: Jason Gunthorpe
> ---
> drivers/iommu/apple-dart.c | 20 ++--
> 1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iommu/apple-dart.c b/drivers/iommu/apple-dart.c
> index 126da0d89f0dd4..821b4a3465dfb9 100644
> --- a/drivers/iommu/apple-dart.c
> +++ b/drivers/iommu/apple-dart.c
> @@ -196,7 +196,6 @@ struct apple_dart_hw {
> * @lock: lock for hardware operations involving this dart
> * @pgsize: pagesize supported by this DART
> * @supports_bypass: indicates if this DART supports bypass mode
> - * @force_bypass: force bypass mode due to pagesize mismatch?
> * @sid2group: maps stream ids to iommu_groups
> * @iommu: iommu core device
> */
> @@ -217,7 +216,6 @@ struct apple_dart {
> u32 pgsize;
> u32 num_streams;
> u32 supports_bypass : 1;
> - u32 force_bypass : 1;
>
> struct iommu_group *sid2group[DART_MAX_STREAMS];
> struct iommu_device iommu;
> @@ -576,6 +574,9 @@ static int apple_dart_finalize_domain(struct
> apple_dart_domain *dart_domain,
> int ret = 0;
> int i, j;
>
> + if (dart->pgsize > PAGE_SIZE)
> + return -EINVAL;
> +
> mutex_lock(_domain->init_lock);
>
> if (dart_domain->finalized)
> @@ -659,9 +660,6 @@ static int apple_dart_attach_dev_paging(struct
> iommu_domain *domain,
> struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
> struct apple_dart_domain *dart_domain = to_dart_domain(domain);
>
> - if (cfg->stream_maps[0].dart->force_bypass)
> - return -EINVAL;
> -
> ret = apple_dart_finalize_domain(dart_domain, cfg);
> if (ret)
> return ret;
> @@ -706,9 +704,6 @@ static int apple_dart_attach_dev_blocked(struct
> iommu_domain *domain,
> struct apple_dart_stream_map *stream_map;
> int i;
>
> - if (cfg->stream_maps[0].dart->force_bypass)
> - return -EINVAL;
> -
> for_each_stream_map(i, cfg, stream_map)
> apple_dart_hw_disable_dma(stream_map);
> return 0;
> @@ -803,8 +798,6 @@ static int apple_dart_of_xlate(struct device *dev,
> struct of_phandle_args *args)
> if (cfg_dart) {
> if (cfg_dart->supports_bypass != dart->supports_bypass)
> return -EINVAL;
> - if (cfg_dart->force_bypass != dart->force_bypass)
> - return -EINVAL;
> if (cfg_dart->pgsize != dart->pgsize)
> return -EINVAL;
> }
> @@ -946,7 +939,7 @@ static int apple_dart_def_domain_type(struct device
> *dev)
> {
> struct apple_dart_master_cfg *cfg = dev_iommu_priv_get(dev);
>
> - if (cfg->stream_maps[0].dart->force_bypass)
> + if (cfg->stream_maps[0].dart->pgsize > PAGE_SIZE)
> return IOMMU_DOMAIN_IDENTITY;
> if (!cfg->stream_maps[0].dart->supports_bypass)
> return IOMMU_DOMAIN_DMA;
> @@ -1146,8 +1139,6 @@ static int apple_dart_probe(struct platform_device
> *pdev)
> goto err_clk_disable;
> }
>
> - dart->force_bypass = dart->pgsize > PAGE_SIZE;
> -
> ret = apple_dart_hw_reset(dart);
> if (ret)
> goto err_clk_disable;
> @@ -1171,7 +1162,8 @@ static int apple_dart_probe(struct
> platform_device *pdev)
> dev_info(
> >dev,
> "DART [pagesize %x, %d streams, bypass support: %d, bypass
> forced:
> %d] initialized\n",
> - dart->pgsize, dart->num_streams, dart->supports_bypass,
> dart->force_bypass);
> + dart->pgsize, dart->num_streams, dart->supports_bypass,
> + dart->pgsize > PAGE_SIZE);
> return 0;
>
> err_sysfs_remove:
> --
Reviewed-by: Janne Grunau
thanks,
Janne
Re: [PATCH 06/20] mtd: powernv_flash: Convert to platform remove callback returning void
On Sun, 2023-10-08 at 20:01:29 UTC, =?utf-8?q?Uwe_Kleine-K=C3=B6nig?= wrote: > The .remove() callback for a platform driver returns an int which makes > many driver authors wrongly assume it's possible to do error handling by > returning an error code. However the value returned is ignored (apart > from emitting a warning) and this typically results in resource leaks. > > To improve here there is a quest to make the remove callback return > void. In the first step of this quest all drivers are converted to > .remove_new(), which already returns void. Eventually after all drivers > are converted, .remove_new() will be renamed to .remove(). > > Trivially convert this driver from always returning zero in the remove > callback to the void returning variant. > > Signed-off-by: Uwe Kleine-König Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git mtd/next, thanks. Miquel
Re: [PATCHv8 1/5] powerpc/setup : Enable boot_cpu_hwid for PPC32
Hello Pingfan, With this patch series applied, the kdump kernel fails to boot on powerpc with nr_cpus=1. Console logs: --- [root]# echo c > /proc/sysrq-trigger [ 74.783235] sysrq: Trigger a crash [ 74.783244] Kernel panic - not syncing: sysrq triggered crash [ 74.783252] CPU: 58 PID: 3838 Comm: bash Kdump: loaded Not tainted 6.6.0-rc5pf-nr-cpus+ #3 [ 74.783259] Hardware name: POWER10 (raw) phyp pSeries [ 74.783275] Call Trace: [ 74.783280] [c0020f4ebac0] [c0ed9f38] dump_stack_lvl+0x6c/0x9c (unreliable) [ 74.783291] [c0020f4ebaf0] [c0150300] panic+0x178/0x438 [ 74.783298] [c0020f4ebb90] [c0936d48] sysrq_handle_crash+0x28/0x30 [ 74.783304] [c0020f4ebbf0] [c093773c] __handle_sysrq+0x10c/0x250 [ 74.783309] [c0020f4ebc90] [c0937fa8] write_sysrq_trigger+0xc8/0x168 [ 74.783314] [c0020f4ebcd0] [c0665d8c] proc_reg_write+0x10c/0x1b0 [ 74.783321] [c0020f4ebd00] [c058da54] vfs_write+0x104/0x4b0 [ 74.783326] [c0020f4ebdc0] [c058dfdc] ksys_write+0x7c/0x140 [ 74.783331] [c0020f4ebe10] [c0033a64] system_call_exception+0x144/0x3a0 [ 74.783337] [c0020f4ebe50] [c000c554] system_call_common+0xf4/0x258 [ 74.783343] --- interrupt: c00 at 0x7fffa0721594 [ 74.783352] NIP: 7fffa0721594 LR: 7fffa0697bf4 CTR: [ 74.783364] REGS: c0020f4ebe80 TRAP: 0c00 Not tainted (6.6.0-rc5pf-nr-cpus+) [ 74.783376] MSR: 8280f033 CR: 2802 XER: [ 74.783394] IRQMASK: 0 [ 74.783394] GPR00: 0004 7c4b6800 7fffa0807300 0001 [ 74.783394] GPR04: 00013549ea60 0002 0010 [ 74.783394] GPR08: [ 74.783394] GPR12: 7fffa0abaf70 4000 00011a0f9798 [ 74.783394] GPR16: 00011a0f9724 00011a097688 00011a02ff70 00011a0fd568 [ 74.783394] GPR20: 000135554bf0 0001 00011a0aa478 7c4b6a24 [ 74.783394] GPR24: 7c4b6a20 00011a0faf94 0002 00013549ea60 [ 74.783394] GPR28: 0002 7fffa08017a0 00013549ea60 0002 [ 74.783440] NIP [7fffa0721594] 0x7fffa0721594 [ 74.783443] LR [7fffa0697bf4] 0x7fffa0697bf4 [ 74.783447] --- interrupt: c00 I'm in purgatory [0.00] radix-mmu: Page sizes from device-tree: [0.00] radix-mmu: Page size shift = 12 AP=0x0 [0.00] radix-mmu: Page size shift = 16 AP=0x5 [0.00] radix-mmu: Page size shift = 21 AP=0x1 [0.00] radix-mmu: Page size shift = 30 AP=0x2 [0.00] Activating Kernel Userspace Access Prevention [0.00] Activating Kernel Userspace Execution Prevention [0.00] radix-mmu: Mapped 0x-0x0001 with 64.0 KiB pages (exec) [0.00] radix-mmu: Mapped 0x0001-0x0020 with 64.0 KiB pages [0.00] radix-mmu: Mapped 0x0020-0x2000 with 2.00 MiB pages [0.00] radix-mmu: Mapped 0x2000-0x2260 with 2.00 MiB pages (exec) [0.00] radix-mmu: Mapped 0x2260-0x4000 with 2.00 MiB pages [0.00] radix-mmu: Mapped 0x4000-0x00018000 with 1.00 GiB pages [0.00] radix-mmu: Mapped 0x00018000-0x0001a000 with 2.00 MiB pages [0.00] lpar: Using radix MMU under hypervisor [0.00] Linux version 6.6.0-rc5pf-nr-cpus+ (r...@ltcever7x0-lp1.aus.stglabs.ibm.com) (gcc (GCC) 8.5.0 20210514 (Red Hat 8.5.0-20), GNU ld version 2.30-123.el8) #3 SMP Mon Oct 9 11:07: 41 CDT 2023 [0.00] Found initrd at 0xc00022e6:0xc000248f08d8 [0.00] Hardware name: IBM,9043-MRX POWER10 (raw) 0x800200 0xf06 of:IBM,FW1060.00 (NM1060_016) hv:phyp pSeries [0.00] printk: bootconsole [udbg0] enabled [0.00] the round shift between dt seq and the cpu logic number: 56 [0.00] BUG: Unable to handle kernel data access on write at 0xc001a000 [0.00] Faulting instruction address: 0xc00022009c64 [0.00] Oops: Kernel access of bad area, sig: 11 [#1] [0.00] LE PAGE_SIZE=64K MMU=Radix SMP NR_CPUS=2048 NUMA pSeries [0.00] Modules linked in: [0.00] CPU: 2 PID: 0 Comm: swapper Not tainted 6.6.0-rc5pf-nr-cpus+ #3 [0.00] Hardware name: POWER10 (raw) hv:phyp pSeries [0.00] NIP: c00022009c64 LR: c00022009c54 CTR: c000201ff348 [0.00] REGS: c00022aebb00 TRAP: 0300 Not tainted (6.6.0-rc5pf-nr-cpus+) [0.00] MSR: 80001033 CR: 28222824 XER: 0001 [0.00] CFAR: c00020031574 DAR: c001a000 DSISR: 4200 IRQMASK: 1 [0.00] GPR00: c00022009ba0 c00022aebda0 c000213d1300 0004 [0.00] GPR04: 0001
