* Re: [PATCH v8 2/5] arm64: kdump: reserve crashkenel above 4G for crash dump kernel
From: Baoquan He @ 2020-05-26 0:59 UTC (permalink / raw)
To: Chen Zhou
Cc: horms, John.p.donnelly, arnd, will, devicetree, catalin.marinas,
linux-doc, kexec, linux-kernel, robh+dt, mingo, guohanjun, tglx,
pkushwaha, dyoung, linux-arm-kernel
In-Reply-To: <20200521093805.64398-3-chenzhou10@huawei.com>
On 05/21/20 at 05:38pm, Chen Zhou wrote:
> Crashkernel=X tries to reserve memory for the crash dump kernel under
> 4G. If crashkernel=X,low is specified simultaneously, reserve spcified
> size low memory for crash kdump kernel devices firstly and then reserve
> memory above 4G.
Wondering why crashkernel=,high is not introduced to arm64 to be
consistent with x86_64, to make the behaviour be the same on all
architecutres.
>
> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> Tested-by: John Donnelly <John.p.donnelly@oracle.com>
> Tested-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
> ---
> arch/arm64/kernel/setup.c | 8 +++++++-
> arch/arm64/mm/init.c | 31 +++++++++++++++++++++++++++++--
> 2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
> index 3fd2c11c09fc..a8487e4d3e5a 100644
> --- a/arch/arm64/kernel/setup.c
> +++ b/arch/arm64/kernel/setup.c
> @@ -238,7 +238,13 @@ static void __init request_standard_resources(void)
> kernel_data.end <= res->end)
> request_resource(res, &kernel_data);
> #ifdef CONFIG_KEXEC_CORE
> - /* Userspace will find "Crash kernel" region in /proc/iomem. */
> + /*
> + * Userspace will find "Crash kernel" region in /proc/iomem.
> + * Note: the low region is renamed as Crash kernel (low).
> + */
> + if (crashk_low_res.end && crashk_low_res.start >= res->start &&
> + crashk_low_res.end <= res->end)
> + request_resource(res, &crashk_low_res);
> if (crashk_res.end && crashk_res.start >= res->start &&
> crashk_res.end <= res->end)
> request_resource(res, &crashk_res);
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index e42727e3568e..71498acf0cd8 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -81,6 +81,7 @@ static void __init reserve_crashkernel(void)
> {
> unsigned long long crash_base, crash_size;
> int ret;
> + phys_addr_t crash_max = arm64_dma32_phys_limit;
>
> ret = parse_crashkernel(boot_command_line, memblock_phys_mem_size(),
> &crash_size, &crash_base);
> @@ -88,12 +89,38 @@ static void __init reserve_crashkernel(void)
> if (ret || !crash_size)
> return;
>
> + ret = reserve_crashkernel_low();
> + if (!ret && crashk_low_res.end) {
> + /*
> + * If crashkernel=X,low specified, there may be two regions,
> + * we need to make some changes as follows:
> + *
> + * 1. rename the low region as "Crash kernel (low)"
> + * In order to distinct from the high region and make no effect
> + * to the use of existing kexec-tools, rename the low region as
> + * "Crash kernel (low)".
> + *
> + * 2. change the upper bound for crash memory
> + * Set MEMBLOCK_ALLOC_ACCESSIBLE upper bound for crash memory.
> + *
> + * 3. mark the low region as "nomap"
> + * The low region is intended to be used for crash dump kernel
> + * devices, just mark the low region as "nomap" simply.
> + */
> + const char *rename = "Crash kernel (low)";
> +
> + crashk_low_res.name = rename;
> + crash_max = MEMBLOCK_ALLOC_ACCESSIBLE;
> + memblock_mark_nomap(crashk_low_res.start,
> + resource_size(&crashk_low_res));
> + }
> +
> crash_size = PAGE_ALIGN(crash_size);
>
> if (crash_base == 0) {
> /* Current arm64 boot protocol requires 2MB alignment */
> - crash_base = memblock_find_in_range(0, arm64_dma32_phys_limit,
> - crash_size, SZ_2M);
> + crash_base = memblock_find_in_range(0, crash_max, crash_size,
> + SZ_2M);
> if (crash_base == 0) {
> pr_warn("cannot allocate crashkernel (size:0x%llx)\n",
> crash_size);
> --
> 2.20.1
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v8 1/5] x86: kdump: move reserve_crashkernel_low() into crash_core.c
From: Baoquan He @ 2020-05-26 0:56 UTC (permalink / raw)
To: Chen Zhou
Cc: horms, John.p.donnelly, arnd, will, devicetree, catalin.marinas,
linux-doc, kexec, linux-kernel, robh+dt, mingo, guohanjun, tglx,
pkushwaha, dyoung, linux-arm-kernel
In-Reply-To: <20200521093805.64398-2-chenzhou10@huawei.com>
On 05/21/20 at 05:38pm, Chen Zhou wrote:
> In preparation for supporting reserve_crashkernel_low in arm64 as
> x86_64 does, move reserve_crashkernel_low() into kernel/crash_core.c.
> BTW, move x86 CRASH_ALIGN to 2M.
The reason is?
>
> Note, in arm64, we reserve low memory if and only if crashkernel=X,low
> is specified. Different with x86_64, don't set low memory automatically.
>
> Reported-by: kbuild test robot <lkp@intel.com>
> Signed-off-by: Chen Zhou <chenzhou10@huawei.com>
> Tested-by: John Donnelly <John.p.donnelly@oracle.com>
> Tested-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
> ---
> arch/x86/kernel/setup.c | 66 ++++-------------------------
> include/linux/crash_core.h | 3 ++
> include/linux/kexec.h | 2 -
> kernel/crash_core.c | 85 ++++++++++++++++++++++++++++++++++++++
> kernel/kexec_core.c | 17 --------
> 5 files changed, 96 insertions(+), 77 deletions(-)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 4b3fa6cd3106..de75fec73d47 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -395,8 +395,8 @@ static void __init memblock_x86_reserve_range_setup_data(void)
>
> #ifdef CONFIG_KEXEC_CORE
>
> -/* 16M alignment for crash kernel regions */
> -#define CRASH_ALIGN SZ_16M
> +/* 2M alignment for crash kernel regions */
> +#define CRASH_ALIGN SZ_2M
>
> /*
> * Keep the crash kernel below this limit.
> @@ -419,59 +419,6 @@ static void __init memblock_x86_reserve_range_setup_data(void)
> # define CRASH_ADDR_HIGH_MAX SZ_64T
> #endif
>
> -static int __init reserve_crashkernel_low(void)
> -{
> -#ifdef CONFIG_X86_64
> - unsigned long long base, low_base = 0, low_size = 0;
> - unsigned long total_low_mem;
> - int ret;
> -
> - total_low_mem = memblock_mem_size(1UL << (32 - PAGE_SHIFT));
> -
> - /* crashkernel=Y,low */
> - ret = parse_crashkernel_low(boot_command_line, total_low_mem, &low_size, &base);
> - if (ret) {
> - /*
> - * two parts from kernel/dma/swiotlb.c:
> - * -swiotlb size: user-specified with swiotlb= or default.
> - *
> - * -swiotlb overflow buffer: now hardcoded to 32k. We round it
> - * to 8M for other buffers that may need to stay low too. Also
> - * make sure we allocate enough extra low memory so that we
> - * don't run out of DMA buffers for 32-bit devices.
> - */
> - low_size = max(swiotlb_size_or_default() + (8UL << 20), 256UL << 20);
> - } else {
> - /* passed with crashkernel=0,low ? */
> - if (!low_size)
> - return 0;
> - }
> -
> - low_base = memblock_find_in_range(0, 1ULL << 32, low_size, CRASH_ALIGN);
> - if (!low_base) {
> - pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
> - (unsigned long)(low_size >> 20));
> - return -ENOMEM;
> - }
> -
> - ret = memblock_reserve(low_base, low_size);
> - if (ret) {
> - pr_err("%s: Error reserving crashkernel low memblock.\n", __func__);
> - return ret;
> - }
> -
> - pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
> - (unsigned long)(low_size >> 20),
> - (unsigned long)(low_base >> 20),
> - (unsigned long)(total_low_mem >> 20));
> -
> - crashk_low_res.start = low_base;
> - crashk_low_res.end = low_base + low_size - 1;
> - insert_resource(&iomem_resource, &crashk_low_res);
> -#endif
> - return 0;
> -}
> -
> static void __init reserve_crashkernel(void)
> {
> unsigned long long crash_size, crash_base, total_mem;
> @@ -535,9 +482,12 @@ static void __init reserve_crashkernel(void)
> return;
> }
>
> - if (crash_base >= (1ULL << 32) && reserve_crashkernel_low()) {
> - memblock_free(crash_base, crash_size);
> - return;
> + if (crash_base >= (1ULL << 32)) {
> + if (reserve_crashkernel_low()) {
> + memblock_free(crash_base, crash_size);
> + return;
> + }
> + insert_resource(&iomem_resource, &crashk_low_res);
> }
>
> pr_info("Reserving %ldMB of memory at %ldMB for crashkernel (System RAM: %ldMB)\n",
> diff --git a/include/linux/crash_core.h b/include/linux/crash_core.h
> index 525510a9f965..4df8c0bff03e 100644
> --- a/include/linux/crash_core.h
> +++ b/include/linux/crash_core.h
> @@ -63,6 +63,8 @@ phys_addr_t paddr_vmcoreinfo_note(void);
> extern unsigned char *vmcoreinfo_data;
> extern size_t vmcoreinfo_size;
> extern u32 *vmcoreinfo_note;
> +extern struct resource crashk_res;
> +extern struct resource crashk_low_res;
>
> Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
> void *data, size_t data_len);
> @@ -74,5 +76,6 @@ int parse_crashkernel_high(char *cmdline, unsigned long long system_ram,
> unsigned long long *crash_size, unsigned long long *crash_base);
> int parse_crashkernel_low(char *cmdline, unsigned long long system_ram,
> unsigned long long *crash_size, unsigned long long *crash_base);
> +int __init reserve_crashkernel_low(void);
>
> #endif /* LINUX_CRASH_CORE_H */
> diff --git a/include/linux/kexec.h b/include/linux/kexec.h
> index 1776eb2e43a4..5d5d9635b18d 100644
> --- a/include/linux/kexec.h
> +++ b/include/linux/kexec.h
> @@ -330,8 +330,6 @@ extern int kexec_load_disabled;
>
> /* Location of a reserved region to hold the crash kernel.
> */
> -extern struct resource crashk_res;
> -extern struct resource crashk_low_res;
> extern note_buf_t __percpu *crash_notes;
>
> /* flag to track if kexec reboot is in progress */
> diff --git a/kernel/crash_core.c b/kernel/crash_core.c
> index 9f1557b98468..a7580d291c37 100644
> --- a/kernel/crash_core.c
> +++ b/kernel/crash_core.c
> @@ -7,6 +7,8 @@
> #include <linux/crash_core.h>
> #include <linux/utsname.h>
> #include <linux/vmalloc.h>
> +#include <linux/memblock.h>
> +#include <linux/swiotlb.h>
>
> #include <asm/page.h>
> #include <asm/sections.h>
> @@ -19,6 +21,22 @@ u32 *vmcoreinfo_note;
> /* trusted vmcoreinfo, e.g. we can make a copy in the crash memory */
> static unsigned char *vmcoreinfo_data_safecopy;
>
> +/* Location of the reserved area for the crash kernel */
> +struct resource crashk_res = {
> + .name = "Crash kernel",
> + .start = 0,
> + .end = 0,
> + .flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> + .desc = IORES_DESC_CRASH_KERNEL
> +};
> +struct resource crashk_low_res = {
> + .name = "Crash kernel",
> + .start = 0,
> + .end = 0,
> + .flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> + .desc = IORES_DESC_CRASH_KERNEL
> +};
> +
> /*
> * parsing the "crashkernel" commandline
> *
> @@ -292,6 +310,73 @@ int __init parse_crashkernel_low(char *cmdline,
> "crashkernel=", suffix_tbl[SUFFIX_LOW]);
> }
>
> +#if defined(CONFIG_X86_64) || defined(CONFIG_ARM64)
> +#define CRASH_ALIGN SZ_2M
> +#endif
> +
> +int __init reserve_crashkernel_low(void)
> +{
> +#if defined(CONFIG_X86_64) || defined(CONFIG_ARM64)
> + unsigned long long base, low_base = 0, low_size = 0;
> + unsigned long total_low_mem;
> + int ret;
> +
> + total_low_mem = memblock_mem_size(1UL << (32 - PAGE_SHIFT));
> +
> + /* crashkernel=Y,low */
> + ret = parse_crashkernel_low(boot_command_line, total_low_mem, &low_size,
> + &base);
> + if (ret) {
> +#ifdef CONFIG_X86_64
> + /*
> + * two parts from lib/swiotlb.c:
> + * -swiotlb size: user-specified with swiotlb= or default.
> + *
> + * -swiotlb overflow buffer: now hardcoded to 32k. We round it
> + * to 8M for other buffers that may need to stay low too. Also
> + * make sure we allocate enough extra low memory so that we
> + * don't run out of DMA buffers for 32-bit devices.
> + */
> + low_size = max(swiotlb_size_or_default() + (8UL << 20),
> + 256UL << 20);
> +#else
> + /*
> + * in arm64, reserve low memory if and only if crashkernel=X,low
> + * specified.
> + */
> + return -EINVAL;
> +#endif
> + } else {
> + /* passed with crashkernel=0,low ? */
> + if (!low_size)
> + return 0;
> + }
> +
> + low_base = memblock_find_in_range(0, 1ULL << 32, low_size, CRASH_ALIGN);
> + if (!low_base) {
> + pr_err("Cannot reserve %ldMB crashkernel low memory, please try smaller size.\n",
> + (unsigned long)(low_size >> 20));
> + return -ENOMEM;
> + }
> +
> + ret = memblock_reserve(low_base, low_size);
> + if (ret) {
> + pr_err("%s: Error reserving crashkernel low memblock.\n",
> + __func__);
> + return ret;
> + }
> +
> + pr_info("Reserving %ldMB of low memory at %ldMB for crashkernel (System low RAM: %ldMB)\n",
> + (unsigned long)(low_size >> 20),
> + (unsigned long)(low_base >> 20),
> + (unsigned long)(total_low_mem >> 20));
> +
> + crashk_low_res.start = low_base;
> + crashk_low_res.end = low_base + low_size - 1;
> +#endif
> + return 0;
> +}
> +
> Elf_Word *append_elf_note(Elf_Word *buf, char *name, unsigned int type,
> void *data, size_t data_len)
> {
> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c
> index c19c0dad1ebe..db66bbabfff3 100644
> --- a/kernel/kexec_core.c
> +++ b/kernel/kexec_core.c
> @@ -53,23 +53,6 @@ note_buf_t __percpu *crash_notes;
> /* Flag to indicate we are going to kexec a new kernel */
> bool kexec_in_progress = false;
>
> -
> -/* Location of the reserved area for the crash kernel */
> -struct resource crashk_res = {
> - .name = "Crash kernel",
> - .start = 0,
> - .end = 0,
> - .flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> - .desc = IORES_DESC_CRASH_KERNEL
> -};
> -struct resource crashk_low_res = {
> - .name = "Crash kernel",
> - .start = 0,
> - .end = 0,
> - .flags = IORESOURCE_BUSY | IORESOURCE_SYSTEM_RAM,
> - .desc = IORES_DESC_CRASH_KERNEL
> -};
> -
> int kexec_should_crash(struct task_struct *p)
> {
> /*
> --
> 2.20.1
>
>
> _______________________________________________
> kexec mailing list
> kexec@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH RFCv2 0/9] kvm/arm64: Support Async Page Fault
From: Gavin Shan @ 2020-05-25 23:39 UTC (permalink / raw)
To: kvmarm
Cc: mark.rutland, aarcange, drjones, suzuki.poulose, maz,
linux-kernel, eric.auger, james.morse, shan.gavin,
catalin.marinas, will, linux-arm-kernel
In-Reply-To: <20200508032919.52147-1-gshan@redhat.com>
On 5/8/20 1:29 PM, Gavin Shan wrote:
> There are two stages of page faults and the stage one page fault is
> handled by guest itself. The guest is trapped to host when the page
> fault is caused by stage 2 page table, for example missing. The guest
> is suspended until the requested page is populated. There might be
> IO activities involved for host to populate the requested page. For
> instance, the requested page has been swapped out previously. In this
> case, the guest (vCPU) has to suspend for a few of milliseconds, which
> depends on the swapping media, regardless of the overall system load.
>
> The series adds asychornous page fault to improve the situation. A
> signal (PAGE_NOT_PRESENT) is sent from host to the guest if the requested
> page isn't absent immediately. In the mean while, a worker is started
> to populate the requested page in background. Guest either picks another
> available process to run or puts current (faulting) process to power
> saving mode when receiving the (PAGE_NOT_PRESENT) signal. After the
> requested page is populated by the worker, another signal (PAGE_READY)
> is sent from host to guest. Guest wakes up the (faulting) process when
> receiving the (PAGE_READY) signal.
>
> The signals are conveyed through control block. The control block physical
> address is passed from guest to host through dedicated KVM vendor specific
> hypercall. The control block is visible and accessible by host and guest
> in the mean while. The hypercall is also used to enable, disable, configure
> the functionality. Notifications, by injected abort data exception, are
> fired when there are pending signals. The exception handler will be invoked
> in guest kernel.
>
> Testing
> =======
> The tests are carried on the following machine. A guest with single vCPU
> and 4GB memory is started. Also, the QEMU process is put into memory cgroup
> (v1) whose memory limit is set to 2GB. In the guest, there are two threads,
> which are memory bound and CPU bound separately. The memory bound thread
> allocates all available memory, accesses and them free them. The CPU bound
> thread simply executes block of "nop". The test is carried out for 5 time
> continuously and the average number (per minute) of executed blocks in the
> CPU bound thread is taken as indicator of improvement.
>
> Vendor: GIGABYTE CPU: 224 x Cavium ThunderX2(R) CPU CN9975 v2.2 @ 2.0GHz
> Memory: 32GB Disk: Fusion-MPT SAS-3 (PCIe3.0 x8)
>
> Without-APF: 7029030180/minute = avg(7559625120 5962155840 7823208540
> 7629633480 6170527920)
> With-APF: 8286827472/minute = avg(8464584540 8177073360 8262723180
> 8095084020 8434672260)
> Outcome: +17.8%
>
> Another test case is to measure the time consumed by the application, but
> with the CPU-bound thread disabled.
>
> Without-APF: 40.3s = avg(40.6 39.3 39.2 41.6 41.2)
> With-APF: 40.8s = avg(40.6 41.1 40.9 41.0 40.7)
> Outcome: +1.2%
>
> I also have some code in the host to capture the number of async page faults,
> time used to do swapin and its maximal/minimal values when async page fault
> is enabled. During the test, the CPU-bound thread is disabled. There is about
> 30% of the time used to do swapin.
>
> Number of async page fault: 7555 times
> Total time used by application: 42.2 seconds
> Total time used by swapin: 12.7 seconds (30%)
> Minimal swapin time: 36.2 us
> Maximal swapin time: 55.7 ms
>
A kindly ping... Marc/Mark/Will, please let me know your comments
on this. thanks in advance!
> Changelog
> =========
> RFCv1 -> RFCv2
> * Rebase to 5.7.rc3
> * Performance data (Marc Zyngier)
> * Replace IMPDEF system register with KVM vendor specific hypercall (Mark Rutland)
> * Based on Will's KVM vendor hypercall probe mechanism (Will Deacon)
> * Don't use IMPDEF DFSC (0x43). Async page fault reason is conveyed
> by the control block (Mark Rutland)
> * Delayed wakeup mechanism in guest kernel (Gavin Shan)
> * Stability improvement in the guest kernel: delayed wakeup mechanism,
> external abort disallowed region, lazily clear async page fault,
> disabled interrupt on acquiring the head's lock and so on (Gavin Shan)
> * Stability improvement in the host kernel: serialized async page
> faults etc. (Gavin Shan)
> * Performance improvement in guest kernel: percpu sleeper head (Gavin Shan)
>
> Gavin Shan (7):
> kvm/arm64: Rename kvm_vcpu_get_hsr() to kvm_vcpu_get_esr()
> kvm/arm64: Detach ESR operator from vCPU struct
> kvm/arm64: Replace hsr with esr
> kvm/arm64: Export kvm_handle_user_mem_abort() with prefault mode
> kvm/arm64: Support async page fault
> kernel/sched: Add cpu_rq_is_locked()
> arm64: Support async page fault
>
> Will Deacon (2):
> arm64: Probe for the presence of KVM hypervisor services during boot
> arm/arm64: KVM: Advertise KVM UID to guests via SMCCC
>
> arch/arm64/Kconfig | 11 +
> arch/arm64/include/asm/exception.h | 3 +
> arch/arm64/include/asm/hypervisor.h | 11 +
> arch/arm64/include/asm/kvm_emulate.h | 83 +++--
> arch/arm64/include/asm/kvm_host.h | 47 +++
> arch/arm64/include/asm/kvm_para.h | 40 +++
> arch/arm64/include/uapi/asm/Kbuild | 2 -
> arch/arm64/include/uapi/asm/kvm_para.h | 22 ++
> arch/arm64/kernel/entry.S | 33 ++
> arch/arm64/kernel/process.c | 4 +
> arch/arm64/kernel/setup.c | 35 ++
> arch/arm64/kvm/Kconfig | 1 +
> arch/arm64/kvm/Makefile | 2 +
> arch/arm64/kvm/handle_exit.c | 48 +--
> arch/arm64/kvm/hyp/switch.c | 33 +-
> arch/arm64/kvm/hyp/vgic-v2-cpuif-proxy.c | 7 +-
> arch/arm64/kvm/inject_fault.c | 4 +-
> arch/arm64/kvm/sys_regs.c | 38 +-
> arch/arm64/mm/fault.c | 434 +++++++++++++++++++++++
> include/linux/arm-smccc.h | 32 ++
> include/linux/sched.h | 1 +
> kernel/sched/core.c | 8 +
> virt/kvm/arm/arm.c | 40 ++-
> virt/kvm/arm/async_pf.c | 335 +++++++++++++++++
> virt/kvm/arm/hyp/aarch32.c | 4 +-
> virt/kvm/arm/hyp/vgic-v3-sr.c | 7 +-
> virt/kvm/arm/hypercalls.c | 37 +-
> virt/kvm/arm/mmio.c | 27 +-
> virt/kvm/arm/mmu.c | 69 +++-
> 29 files changed, 1264 insertions(+), 154 deletions(-)
> create mode 100644 arch/arm64/include/asm/kvm_para.h
> create mode 100644 arch/arm64/include/uapi/asm/kvm_para.h
> create mode 100644 virt/kvm/arm/async_pf.c
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC v3 1/2] thermal: core: Let thermal zone device's mode be stored in its struct
From: Daniel Lezcano @ 2020-05-25 22:08 UTC (permalink / raw)
To: Andrzej Pietrasiewicz, linux-pm
Cc: Rafael J . Wysocki, platform-driver-x86, Shawn Guo, kernel,
Fabio Estevam, Amit Kucheria, linux-acpi, NXP Linux Team,
Darren Hart, Zhang Rui, Gayatri Kammela, Len Brown,
Barlomiej Zolnierkiewicz, Sascha Hauer, Ido Schimmel, Jiri Pirko,
Thomas Gleixner, Allison Randal, linux-arm-kernel,
Support Opensource, netdev, Peter Kaestle,
Pengutronix Kernel Team, Enrico Weigelt, David S . Miller,
Andy Shevchenko
In-Reply-To: <802b4bd5-07c9-de3a-2ac6-5905b12d6adc@collabora.com>
On 25/05/2020 21:35, Andrzej Pietrasiewicz wrote:
> Hi Daniel,
>
> W dniu 23.05.2020 o 23:24, Daniel Lezcano pisze:
>> Hi Andrzej,
>>
>> On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote:
>>> Thermal zone devices' mode is stored in individual drivers. This patch
>>> changes it so that mode is stored in struct thermal_zone_device instead.
>>>
>>> As a result all driver-specific variables storing the mode are not
>>> needed
>>> and are removed. Consequently, the get_mode() implementations have
>>> nothing
>>> to operate on and need to be removed, too.
>>>
>>> Some thermal framework specific functions are introduced:
>>>
>>> thermal_zone_device_get_mode()
>>> thermal_zone_device_set_mode()
>>> thermal_zone_device_enable()
>>> thermal_zone_device_disable()
>>>
>>> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock
>>> and the "set" calls driver's set_mode() if provided, so the latter must
>>> not take this lock again. At the end of the "set"
>>> thermal_zone_device_update() is called so drivers don't need to
>>> repeat this
>>> invocation in their specific set_mode() implementations.
>>>
>>> The scope of the above 4 functions is purposedly limited to the thermal
>>> framework and drivers are not supposed to call them. This encapsulation
>>> does not fully work at the moment for some drivers, though:
>>>
>>> - platform/x86/acerhdf.c
>>> - drivers/thermal/imx_thermal.c
>>> - drivers/thermal/intel/intel_quark_dts_thermal.c
>>> - drivers/thermal/of-thermal.c
>>>
>>> and they manipulate struct thermal_zone_device's members directly.
>>>
>>> struct thermal_zone_params gains a new member called initial_mode, which
>>> is used to set tzd's mode at registration time.
>>>
>>> The sysfs "mode" attribute is always exposed from now on, because all
>>> thermal zone devices now have their get_mode() implemented at the
>>> generic
>>> level and it is always available. Exposing "mode" doesn't hurt the
>>> drivers
>>> which don't provide their own set_mode(), because writing to "mode" will
>>> result in -EPERM, as expected.
>>
>> The result is great, that is a nice cleanup of the thermal framework.
>>
>> After review it appears there are still problems IMO, especially with
>> the suspend / resume path. The patch is big, it is a bit complex to
>> comment. I suggest to re-org the changes as following:
>>
>> - patch 1 : Add the four functions:
>>
>> * thermal_zone_device_set_mode()
>> * thermal_zone_device_enable()
>> * thermal_zone_device_disable()
>> * thermal_zone_device_is_enabled()
>>
>> *but* do not export thermal_zone_device_set_mode(), it must stay private
>> to the thermal framework ATM.
>
> Not exporting thermal_zone_device_set_mode() implies not exporting
> thermal_zone_device_enable()/thermal_zone_device_disable() because they
> are implemented in terms of the former. Or do you have a different idea?
I meant no inline for them but as below:
in .h
extern int thermal_zone_device_enable();
extern int thermal_zone_device_disable();
extern int thermal_zone_device_is_enabled();
in .c
static int thermal_zone_device_set_mode()
{
...
}
int thermal_zone_device_enable()
{
thermal_zone_device_set_mode();
}
EXPORT_SYMBOL_GPL(thermal_zone_device_enable);
>> - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED
>>
>> In the thermal_pm_notify() in the:
>>
>> - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if
>> the mode is THERMAL_DEVICE_ENABLED
>>
>> - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the
>> mode is THERMAL_DEVICE_SUSPENDED
>>
>> - patch 3 : Change the monitor function
>>
>> Change monitor_thermal_zone() function to set the polling to zero if the
>> mode is THERMAL_DEVICE_DISABLED
>
> So we assume this: if a driver creates a tz which is initially ENABLED,
> it will be polled. If a driver creates a tz which is initially DISABLED
> (which is what you suggest the drivers should be doing, but not all of them
> do), it won't be polled unless the driver explicitly enables its tz.
Yes.
> Am I concluding right that a suspended device will remain polled? Is it ok?
Actually it is not ok but AFAICT, it is the current behavior. The
polling do not stop but the 'in_suspend' prevent an update. I thought we
can post-pone the suspend case later when the ENABLED/DISABLED changes
are consolidated, so SUSPENDED will act as DISABLED.
>> - patch 4 : Do the changes to remove get_mode() ops
>>
>> Make sure there is no access to tz->mode from the drivers anymore but
>> use of the functions of patch 1. IMO, this is the tricky part because a
>> part of the drivers are not calling the update after setting the mode
>> while the function thermal_zone_device_enable()/disable() call update
>> via the thermal_zone_device_set_mode(), so we must be sure to not break
>> anything.
>
> Ah, I guess now is the time to make the functions from patch 1 exported?
Yes :)
> Ensuring no driver accesses tz->mode directly might be tricky, indeed.
> If it can be shown that calling the update doesn't hurt a particular
> driver,
> it can be converted to use the helpers instead of manipulating tz->mode
> directly. If, however, it does make a difference then it all depends and
> getting rid of accessing tz->mode directly might require help from the
> respective maintainers.
Agree.
> This also calls for storing tz's mode in struct thermal_zone_device
> rather than in individual drivers. In fact it seems the purpose
> of ->get_mode() is to produce the state stored internally in drivers.
> Removing ->get_mode() requires changing the place where the state is
> stored. struct thermal_zone_device seems most appropriate. So this patch
> is not going to be small.
Yes, the patch can be big. It is fine if the changes are only to replace
tz->mode by their respective disable/enable/is_enabled functions. More
complex changes can be separate.
> Once we start storing tz's state in struct thermal_zone_device the
> ->set_mode() implementations need to be changed, too. To me it seems
> awkward to split this change in two patches: if we keep the changes
> split then in patch 4 we need to introduce code which implements
> ->set_mode() in terms of the new state location, only to remove it
> in the very next patch.
Yes, it is a valid point. May be you can do the changes in two patches
to see the results in terms of complexity for the review process, then
decide if it is worth to merge them before sending.
> While we are at it some drivers, namely acpi/thermal and int3400 store
> their mode in an int rather than enum thermal_device_mode. So maybe
> changing this should go even before patch 4?
I agree.
> acerhdf does not store
> its mode at all and on top of it it wants to manipulate the polling
> delay directly and it has a module parameter which specifies it.
>> - patch 5 : Do the changes to remove set_mode() ops users
>>
>> As the patch 3 sets the polling to zero, the routine in the driver
>> setting the polling to zero is no longer needed (eg. in the mellanox
>> driver). I expect int300 to be the last user of this ops, hopefully we
>> can find a way to get rid of the specific call done inside and then
>> remove the ops.
>
> acerhdf wants ->set_mode() desperately.
Yes, there is a couple of drivers which requires for the moment to keep
the ops->set_mode to be called: int3400 and acerhdf. Both of them will
be greatly simplified with the DISABLED / ENABLED changes.
>> The initial_mode approach looks hackish, I suggest to make the default
>> the thermal zone disabled after creating and then explicitly enable it.
>
> Is it needed in drivers which create their thermal zone enabled?
IMO, yes. We are doing changes with a code prone to issues, so making
the steps: creation + enable will make things more clear. For instance,
the clk framework do the same.
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [GIT PULL 07/11] memory: tegra: Changes for v5.8-rc1
From: Arnd Bergmann @ 2020-05-25 21:52 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Boyd, Michael Turquette, Jon Hunter, SoC Team, arm-soc,
open list:TEGRA ARCHITECTURE SUPPORT, linux-clk, Linux ARM
In-Reply-To: <20200515145311.1580134-8-thierry.reding@gmail.com>
On Fri, May 15, 2020 at 4:53 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> ----------------------------------------------------------------
> memory: tegra: Changes for v5.8-rc1
>
> Contains a few cleanup patches and an implementation to scale the EMC
> frequency on Tegra210 systems.
I don't mind taking the memory driver patches, but it seems odd that this
pull request has so many drivers/clk changes but does not mention that
in the pull request, and does not Cc the clk maintainers or include Acks
from them.
I would assume that the reason for this is that you have based
the memory controller changes on a branch that was already
accepted by the clk maintainers in to their tree, but when you do that
please be more explicit so I know what is going on.
Waiting for clarification before I can pull this.
Arnd
> Dmitry Osipenko (9):
> dt-bindings: cpufreq: Add binding for NVIDIA Tegra20/30
> clk: tegra: Add custom CCLK implementation
> clk: tegra: pll: Add pre/post rate-change hooks
> clk: tegra: cclk: Add helpers for handling PLLX rate changes
> clk: tegra20: Use custom CCLK implementation
> clk: tegra30: Use custom CCLK implementation
> Joseph Lo (7):
> dt-bindings: memory: tegra: Add external memory controller binding for Tegra210
> clk: tegra: Add PLLP_UD and PLLMB_UD for Tegra210
> clk: tegra: Export functions for EMC clock scaling
> clk: tegra: Implement Tegra210 EMC clock
> clk: tegra: Remove the old emc_mux clock for Tegra210
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 0/2] drivers/iommu: Constify structs
From: Rikard Falkeborn @ 2020-05-25 21:49 UTC (permalink / raw)
To: Joerg Roedel
Cc: Wei Liu, Stephen Hemminger, linux-hyperv, Haiyang Zhang,
linux-kernel, Rikard Falkeborn, Chen-Yu Tsai, iommu,
Maxime Ripard, K. Y. Srinivasan, linux-arm-kernel
Constify some structs with function pointers to allow the compiler to
put them in read-only memory. There is not dependency between the
patches.
Rikard Falkeborn (2):
iommu/hyper-v: Constify hyperv_ir_domain_ops
iommu/sun50i: Constify sun50i_iommu_ops
drivers/iommu/hyperv-iommu.c | 2 +-
drivers/iommu/sun50i-iommu.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* [PATCH 2/2] iommu/sun50i: Constify sun50i_iommu_ops
From: Rikard Falkeborn @ 2020-05-25 21:49 UTC (permalink / raw)
To: Joerg Roedel
Cc: Chen-Yu Tsai, Maxime Ripard, linux-kernel, iommu,
Rikard Falkeborn, linux-arm-kernel
In-Reply-To: <20200525214958.30015-1-rikard.falkeborn@gmail.com>
The struct sun50i_iommu_ops is not modified and can be made const to
allow the compiler to put it in read-only memory.
Before:
text data bss dec hex filename
14358 2501 64 16923 421b drivers/iommu/sun50i-iommu.o
After:
text data bss dec hex filename
14726 2117 64 16907 420b drivers/iommu/sun50i-iommu.o
Signed-off-by: Rikard Falkeborn <rikard.falkeborn@gmail.com>
---
drivers/iommu/sun50i-iommu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/iommu/sun50i-iommu.c b/drivers/iommu/sun50i-iommu.c
index 1fa09ddcebd4..fce605e96aa2 100644
--- a/drivers/iommu/sun50i-iommu.c
+++ b/drivers/iommu/sun50i-iommu.c
@@ -771,7 +771,7 @@ static int sun50i_iommu_of_xlate(struct device *dev,
return iommu_fwspec_add_ids(dev, &id, 1);
}
-static struct iommu_ops sun50i_iommu_ops = {
+static const struct iommu_ops sun50i_iommu_ops = {
.pgsize_bitmap = SZ_4K,
.attach_dev = sun50i_iommu_attach_device,
.detach_dev = sun50i_iommu_detach_device,
--
2.26.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related
* Re: [PATCH v3 1/8] dt-bindings: net: meson-dwmac: Add the amlogic,rx-delay-ns property
From: Pavel Machek @ 2020-05-25 20:17 UTC (permalink / raw)
To: Andrew Lunn
Cc: devicetree, Florian Fainelli, jianxin.pan, Martin Blumenstingl,
netdev, linux-kernel, robh+dt, linux-amlogic, davem,
linux-arm-kernel
In-Reply-To: <20200525135728.GE752669@lunn.ch>
[-- Attachment #1.1: Type: text/plain, Size: 858 bytes --]
On Mon 2020-05-25 15:57:28, Andrew Lunn wrote:
> > > standardizing on rx-delay-ps and tx-delay-ps would make sense since that
> > > is the lowest resolution and the property would be correctly named with
> > > an unit in the name.
> >
> > Seems like similar patch is already being reviewed from Dan Murphy (?)
> > from TI.
>
> Dan is working on the PHY side. But there is probably code which can
> be shared.
>
> One question to consider, do we want the same properties names for MAC
> and PHY, or do we want to make them different, to avoid confusion?
We have same properties accross different hardware (compatible, reg),
so same property between MAC and PHY seems to make sense.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] staging: vchiq_arm: cast with __force as needed
From: Mitchell Tasman @ 2020-05-25 19:50 UTC (permalink / raw)
To: Dan Carpenter
Cc: devel, Greg Kroah-Hartman, Marcelo Diop-Gonzalez, linux-kernel,
bcm-kernel-feedback-list, linux-rpi-kernel, Nishka Dasgupta,
Jamal Shareef, Nicolas Saenz Julienne, linux-arm-kernel
In-Reply-To: <20200522110623.GL30374@kadam>
On 5/22/20 7:06 AM, Dan Carpenter wrote:
> On Mon, May 18, 2020 at 08:45:31PM -0400, Mitchell Tasman wrote:
>> In several cases where a pointer marked as __user is
>> (intentionally) assigned or passed to a non-marked target,
>> cast to the target pointer type with a __force directive
>> to quiet warnings from sparse.
>>
>> Signed-off-by: Mitchell Tasman <tasman@leaflabs.com>
>> ---
>> .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 7 ++++---
>> .../staging/vc04_services/interface/vchiq_arm/vchiq_arm.c | 4 +++-
>> 2 files changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>> index c18c6ca0b6c0..38a13e4618a8 100644
>> --- a/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>> +++ b/drivers/staging/vc04_services/interface/vchiq_arm/vchiq_2835_arm.c
>> @@ -371,14 +371,15 @@ create_pagelist(char __user *buf, size_t count, unsigned short type)
>> pagelistinfo->scatterlist = scatterlist;
>> pagelistinfo->scatterlist_mapped = 0;
>>
>> - if (is_vmalloc_addr(buf)) {
>> + if (is_vmalloc_addr((void __force *)buf)) {
>
> Am I reading this correctly???
>
> This is actually a user controlled pointer that comes from the
> vchiq_ioctl() when we do VCHIQ_IOC_QUEUE_BULK_TRANSMIT/RECEIVE. So we
> take random pointer from user space and if it happens to point to kernel
> space then we trust it and presumably start BULK_TRANSMITing data to
> it???
>
> LOL.... This doesn't seem safe at all.
Is additional validation of buf and its extent necessary and sufficient, e.g. perhaps access_ok(buf, count * PAGE_SIZE) somewhere along the call chain? Or does vhciq_arm need to take a different approach in the area that Dan Carpenter flagged?
Thank you.
>
> regards,
> dan carpenter
>
Regards,
Mitch
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC v3 1/2] thermal: core: Let thermal zone device's mode be stored in its struct
From: Andrzej Pietrasiewicz @ 2020-05-25 19:35 UTC (permalink / raw)
To: Daniel Lezcano, linux-pm
Cc: Rafael J . Wysocki, platform-driver-x86, Shawn Guo, kernel,
Fabio Estevam, Amit Kucheria, linux-acpi, NXP Linux Team,
Darren Hart, Zhang Rui, Gayatri Kammela, Len Brown,
Barlomiej Zolnierkiewicz, Sascha Hauer, Ido Schimmel, Jiri Pirko,
Thomas Gleixner, Allison Randal, linux-arm-kernel,
Support Opensource, netdev, Peter Kaestle,
Pengutronix Kernel Team, Enrico Weigelt, David S . Miller,
Andy Shevchenko
In-Reply-To: <f39c5ca6-5efa-889c-21f5-632dfd24715e@linaro.org>
Hi Daniel,
W dniu 23.05.2020 o 23:24, Daniel Lezcano pisze:
> Hi Andrzej,
>
> On 17/04/2020 18:20, Andrzej Pietrasiewicz wrote:
>> Thermal zone devices' mode is stored in individual drivers. This patch
>> changes it so that mode is stored in struct thermal_zone_device instead.
>>
>> As a result all driver-specific variables storing the mode are not needed
>> and are removed. Consequently, the get_mode() implementations have nothing
>> to operate on and need to be removed, too.
>>
>> Some thermal framework specific functions are introduced:
>>
>> thermal_zone_device_get_mode()
>> thermal_zone_device_set_mode()
>> thermal_zone_device_enable()
>> thermal_zone_device_disable()
>>
>> thermal_zone_device_get_mode() and its "set" counterpart take tzd's lock
>> and the "set" calls driver's set_mode() if provided, so the latter must
>> not take this lock again. At the end of the "set"
>> thermal_zone_device_update() is called so drivers don't need to repeat this
>> invocation in their specific set_mode() implementations.
>>
>> The scope of the above 4 functions is purposedly limited to the thermal
>> framework and drivers are not supposed to call them. This encapsulation
>> does not fully work at the moment for some drivers, though:
>>
>> - platform/x86/acerhdf.c
>> - drivers/thermal/imx_thermal.c
>> - drivers/thermal/intel/intel_quark_dts_thermal.c
>> - drivers/thermal/of-thermal.c
>>
>> and they manipulate struct thermal_zone_device's members directly.
>>
>> struct thermal_zone_params gains a new member called initial_mode, which
>> is used to set tzd's mode at registration time.
>>
>> The sysfs "mode" attribute is always exposed from now on, because all
>> thermal zone devices now have their get_mode() implemented at the generic
>> level and it is always available. Exposing "mode" doesn't hurt the drivers
>> which don't provide their own set_mode(), because writing to "mode" will
>> result in -EPERM, as expected.
>
> The result is great, that is a nice cleanup of the thermal framework.
>
> After review it appears there are still problems IMO, especially with
> the suspend / resume path. The patch is big, it is a bit complex to
> comment. I suggest to re-org the changes as following:
>
> - patch 1 : Add the four functions:
>
> * thermal_zone_device_set_mode()
> * thermal_zone_device_enable()
> * thermal_zone_device_disable()
> * thermal_zone_device_is_enabled()
>
> *but* do not export thermal_zone_device_set_mode(), it must stay private
> to the thermal framework ATM.
Not exporting thermal_zone_device_set_mode() implies not exporting
thermal_zone_device_enable()/thermal_zone_device_disable() because they
are implemented in terms of the former. Or do you have a different idea?
>
> - patch 2 : Add the mode THERMAL_DEVICE_SUSPENDED
>
> In the thermal_pm_notify() in the:
>
> - PM_SUSPEND_PREPARE case, set the mode to THERMAL_DEVICE_SUSPENDED if
> the mode is THERMAL_DEVICE_ENABLED
>
> - PM_POST_SUSPEND case, set the mode to THERMAL_DEVICE_ENABLED, if the
> mode is THERMAL_DEVICE_SUSPENDED
>
> - patch 3 : Change the monitor function
>
> Change monitor_thermal_zone() function to set the polling to zero if the
> mode is THERMAL_DEVICE_DISABLED
So we assume this: if a driver creates a tz which is initially ENABLED,
it will be polled. If a driver creates a tz which is initially DISABLED
(which is what you suggest the drivers should be doing, but not all of them
do), it won't be polled unless the driver explicitly enables its tz.
Am I concluding right that a suspended device will remain polled? Is it ok?
>
> - patch 4 : Do the changes to remove get_mode() ops
>
> Make sure there is no access to tz->mode from the drivers anymore but
> use of the functions of patch 1. IMO, this is the tricky part because a
> part of the drivers are not calling the update after setting the mode
> while the function thermal_zone_device_enable()/disable() call update
> via the thermal_zone_device_set_mode(), so we must be sure to not break
> anything.
Ah, I guess now is the time to make the functions from patch 1 exported?
Ensuring no driver accesses tz->mode directly might be tricky, indeed.
If it can be shown that calling the update doesn't hurt a particular driver,
it can be converted to use the helpers instead of manipulating tz->mode
directly. If, however, it does make a difference then it all depends and
getting rid of accessing tz->mode directly might require help from the
respective maintainers.
This also calls for storing tz's mode in struct thermal_zone_device
rather than in individual drivers. In fact it seems the purpose
of ->get_mode() is to produce the state stored internally in drivers.
Removing ->get_mode() requires changing the place where the state is
stored. struct thermal_zone_device seems most appropriate. So this patch
is not going to be small.
Once we start storing tz's state in struct thermal_zone_device the
->set_mode() implementations need to be changed, too. To me it seems
awkward to split this change in two patches: if we keep the changes
split then in patch 4 we need to introduce code which implements
->set_mode() in terms of the new state location, only to remove it
in the very next patch.
While we are at it some drivers, namely acpi/thermal and int3400 store
their mode in an int rather than enum thermal_device_mode. So maybe
changing this should go even before patch 4? acerhdf does not store
its mode at all and on top of it it wants to manipulate the polling
delay directly and it has a module parameter which specifies it.
>
> - patch 5 : Do the changes to remove set_mode() ops users
>
> As the patch 3 sets the polling to zero, the routine in the driver
> setting the polling to zero is no longer needed (eg. in the mellanox
> driver). I expect int300 to be the last user of this ops, hopefully we
> can find a way to get rid of the specific call done inside and then
> remove the ops.
acerhdf wants ->set_mode() desperately.
>
> The initial_mode approach looks hackish, I suggest to make the default
> the thermal zone disabled after creating and then explicitly enable it.
Is it needed in drivers which create their thermal zone enabled?
Regards,
Andrzej
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v6 0/4] kasan: memorize and print call_rcu stack
From: Andrey Konovalov @ 2020-05-25 17:36 UTC (permalink / raw)
To: Walter Wu
Cc: wsd_upstream, Paul E . McKenney, Linux Memory Management List,
Lai Jiangshan, Josh Triplett, kasan-dev, LKML, Joel Fernandes,
linux-mediatek, Alexander Potapenko, Linux ARM, Matthias Brugger,
Andrey Ryabinin, Andrew Morton, Dmitry Vyukov, Mathieu Desnoyers
In-Reply-To: <20200522015757.22267-1-walter-zh.wu@mediatek.com>
On Fri, May 22, 2020 at 3:58 AM Walter Wu <walter-zh.wu@mediatek.com> wrote:
>
> This patchset improves KASAN reports by making them to have
> call_rcu() call stack information. It is useful for programmers
> to solve use-after-free or double-free memory issue.
>
> The KASAN report was as follows(cleaned up slightly):
>
> BUG: KASAN: use-after-free in kasan_rcu_reclaim+0x58/0x60
>
> Freed by task 0:
> kasan_save_stack+0x24/0x50
> kasan_set_track+0x24/0x38
> kasan_set_free_info+0x18/0x20
> __kasan_slab_free+0x10c/0x170
> kasan_slab_free+0x10/0x18
> kfree+0x98/0x270
> kasan_rcu_reclaim+0x1c/0x60
>
> Last call_rcu():
> kasan_save_stack+0x24/0x50
> kasan_record_aux_stack+0xbc/0xd0
> call_rcu+0x8c/0x580
> kasan_rcu_uaf+0xf4/0xf8
>
> Generic KASAN will record the last two call_rcu() call stacks and
> print up to 2 call_rcu() call stacks in KASAN report. it is only
> suitable for generic KASAN.
>
> This feature considers the size of struct kasan_alloc_meta and
> kasan_free_meta, we try to optimize the structure layout and size
> , let it get better memory consumption.
>
> [1]https://bugzilla.kernel.org/show_bug.cgi?id=198437
> [2]https://groups.google.com/forum/#!searchin/kasan-dev/better$20stack$20traces$20for$20rcu%7Csort:date/kasan-dev/KQsjT_88hDE/7rNUZprRBgAJ
Reviewed-by: Andrey Konovalov <andreyknvl@google.com>
for the series.
Thanks!
>
> Changes since v2:
> - remove new config option, default enable it in generic KASAN
> - test this feature in SLAB/SLUB, it is pass.
> - modify macro to be more clearly
> - modify documentation
>
> Changes since v3:
> - change recording from first/last to the last two call stacks
> - move free track into kasan free meta
> - init slab_free_meta on object slot creation
> - modify documentation
>
> Changes since v4:
> - change variable name to be more clearly
> - remove the redundant condition
> - remove init free meta-data and increasing object condition
>
> Changes since v5:
> - add a macro KASAN_KMALLOC_FREETRACK in order to check whether
> print free stack
> - change printing message
> - remove descriptions in Kocong.kasan
>
> Changes since v6:
> - reuse print_stack() in print_track()
>
> Walter Wu (4):
> rcu/kasan: record and print call_rcu() call stack
> kasan: record and print the free track
> kasan: add tests for call_rcu stack recording
> kasan: update documentation for generic kasan
>
> Documentation/dev-tools/kasan.rst | 3 +++
> include/linux/kasan.h | 2 ++
> kernel/rcu/tree.c | 2 ++
> lib/test_kasan.c | 30 ++++++++++++++++++++++++++++++
> mm/kasan/common.c | 26 ++++----------------------
> mm/kasan/generic.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> mm/kasan/generic_report.c | 1 +
> mm/kasan/kasan.h | 23 +++++++++++++++++++++--
> mm/kasan/quarantine.c | 1 +
> mm/kasan/report.c | 54 +++++++++++++++++++++++++++---------------------------
> mm/kasan/tags.c | 37 +++++++++++++++++++++++++++++++++++++
> 11 files changed, 171 insertions(+), 51 deletions(-)
>
> --
> You received this message because you are subscribed to the Google Groups "kasan-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kasan-dev+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kasan-dev/20200522015757.22267-1-walter-zh.wu%40mediatek.com.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v3 03/16] mfd: mfd-core: match device tree node against reg property
From: Michael Walle @ 2020-05-25 17:36 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm, Linus Walleij, Thierry Reding, linux-watchdog,
Andy Shevchenko, Marc Zyngier, Bartosz Golaszewski,
Uwe Kleine-König, Guenter Roeck, devicetree, Jean Delvare,
Jason Cooper, linux-gpio, Rob Herring, Thomas Gleixner,
Wim Van Sebroeck, linux-arm-kernel, linux-hwmon,
Greg Kroah-Hartman, linux-kernel, Li Yang, Mark Brown, Shawn Guo
In-Reply-To: <20200515102848.GH271301@dell>
Am 2020-05-15 12:28, schrieb Lee Jones:
> On Thu, 30 Apr 2020, Michael Walle wrote:
>
>> Hi Lee,
>>
>> Am 2020-04-23 19:45, schrieb Michael Walle:
>> > There might be multiple children with the device tree compatible, for
>> > example if a MFD has multiple instances of the same function. In this
>> > case only the first is matched and the other children get a wrong
>> > of_node reference.
>> > Add a new option to match also against the unit address of the child
>> > node. Additonally, a new helper OF_MFD_CELL_REG is added.
>>
>>
>> Do you think this is feasible? I guess this is the biggest uncertainty
>> for me at the moment in this patch series.
>
> I think it sounds fine in principle. So long as it doesn't change the
> existing behaviour when of_reg isn't set.
>
>> > Signed-off-by: Michael Walle <michael@walle.cc>
>> > ---
>> > drivers/mfd/mfd-core.c | 29 ++++++++++++++++++++---------
>> > include/linux/mfd/core.h | 26 ++++++++++++++++++++------
>> > 2 files changed, 40 insertions(+), 15 deletions(-)
>> >
>> > diff --git a/drivers/mfd/mfd-core.c b/drivers/mfd/mfd-core.c
>> > index e735565969b3..4ecb376338f7 100644
>> > --- a/drivers/mfd/mfd-core.c
>> > +++ b/drivers/mfd/mfd-core.c
>> > @@ -117,6 +117,7 @@ static int mfd_add_device(struct device *parent, int
>> > id,
>> > struct device_node *np = NULL;
>> > int ret = -ENOMEM;
>> > int platform_id;
>> > + u32 of_reg;
>> > int r;
>> >
>> > if (id == PLATFORM_DEVID_AUTO)
>> > @@ -151,16 +152,26 @@ static int mfd_add_device(struct device *parent,
>> > int id,
>> >
>> > if (parent->of_node && cell->of_compatible) {
>> > for_each_child_of_node(parent->of_node, np) {
>> > - if (of_device_is_compatible(np, cell->of_compatible)) {
>> > - if (!of_device_is_available(np)) {
>> > - /* Ignore disabled devices error free */
>> > - ret = 0;
>> > - goto fail_alias;
>> > - }
>> > - pdev->dev.of_node = np;
>> > - pdev->dev.fwnode = &np->fwnode;
>> > - break;
>> > + if (!of_device_is_compatible(np, cell->of_compatible))
>> > + continue;
>> > +
>> > + /* also match the unit address if set */
>
> Please use correct grammar in comments (leaving off the full-stop).
>
>> > + if (cell->of_reg & MFD_OF_REG_VALID) {
>> > + if (of_property_read_u32(np, "reg", &of_reg))
>> > + continue;
>> > + if ((cell->of_reg & MFD_OF_REG_MASK) != of_reg)
>> > + continue;
>> > }
>> > +
>> > + if (!of_device_is_available(np)) {
>> > + /* Ignore disabled devices error free */
>> > + ret = 0;
>> > + goto fail_alias;
>> > + }
>> > +
>> > + pdev->dev.of_node = np;
>> > + pdev->dev.fwnode = &np->fwnode;
>> > + break;
>> > }
>> > }
>> >
>> > diff --git a/include/linux/mfd/core.h b/include/linux/mfd/core.h
>> > index d01d1299e49d..c2c0ad6b14f3 100644
>> > --- a/include/linux/mfd/core.h
>> > +++ b/include/linux/mfd/core.h
>> > @@ -13,8 +13,11 @@
>> > #include <linux/platform_device.h>
>> >
>> > #define MFD_RES_SIZE(arr) (sizeof(arr) / sizeof(struct resource))
>> > +#define MFD_OF_REG_VALID BIT(31)
>
> What about 64bit platforms?
The idea was to have this as a logical number. I.e. for now you may only
have one subdevice per unique compatible string. In fact, if you have a
look at the ab8500.c, there are multiple "stericsson,ab8500-pwm"
subdevices. But there is only one DT node for all three of it. I guess
this works as long as you don't use phandles to reference the pwm node
in the device tree. Or you don't want to use device tree properties
per subdevice (for example the "timeout-sec" of a watchdog device).
So to circumvent this, I thought of having the unit-address (and thus
the "reg" property) to differentiate between multiple subdevices. Now
there is one special case for me: this board management controller
might be upgradable and it might change internally. Thus I came up
with that logical numbering of subdevices. Rob doesn't seem to be a
fan of that, though. Therefore, having bit 31 as a valid indicator
leaves you with 2^31 logical devices, which should be enough ;)
Rob proposed to have the internal offset as the unit-address. But
in that case I can also use devm_of_platform_populate() and don't
need the OF_MFD_CELL_REG; I'd just parse the reg offset in each
individual subdevice driver. But like I said, I wanted to keep the
internal offsets out of the device tree.
-michael
>
>> > +#define MFD_OF_REG_MASK GENMASK(30, 0)
>> >
>> > -#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat,
>> > _match)\
>> > +#define MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \
>> > + _of_reg, _match) \
>> > { \
>> > .name = (_name), \
>> > .resources = (_res), \
>> > @@ -22,24 +25,32 @@
>> > .platform_data = (_pdata), \
>> > .pdata_size = (_pdsize), \
>> > .of_compatible = (_compat), \
>> > + .of_reg = (_of_reg), \
>> > .acpi_match = (_match), \
>> > .id = (_id), \
>> > }
>> >
>> > +#define OF_MFD_CELL_REG(_name, _res, _pdata, _pdsize, _id, _compat, \
>> > + _of_reg) \
>> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \
>> > + ((_of_reg) | MFD_OF_REG_VALID), NULL) \
>> > +
>> > #define OF_MFD_CELL(_name, _res, _pdata, _pdsize,_id, _compat) \
>> > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, NULL) \
>> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, _compat, \
>> > + 0, NULL) \
>> >
>> > #define ACPI_MFD_CELL(_name, _res, _pdata, _pdsize, _id, _match) \
>> > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, _match) \
>> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, \
>> > + _match) \
>> >
>> > #define MFD_CELL_BASIC(_name, _res, _pdata, _pdsize, _id) \
>> > - MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, NULL) \
>> > + MFD_CELL_ALL(_name, _res, _pdata, _pdsize, _id, NULL, 0, NULL) \
>> >
>> > #define MFD_CELL_RES(_name, _res) \
>> > - MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, NULL) \
>> > + MFD_CELL_ALL(_name, _res, NULL, 0, 0, NULL, 0, NULL) \
>> >
>> > #define MFD_CELL_NAME(_name) \
>> > - MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, NULL) \
>> > + MFD_CELL_ALL(_name, NULL, NULL, 0, 0, NULL, 0, NULL) \
>> >
>> > struct irq_domain;
>> > struct property_entry;
>> > @@ -78,6 +89,9 @@ struct mfd_cell {
>> > */
>> > const char *of_compatible;
>> >
>> > + /* matching the reg property if set */
>
> Proper grammar please.
>
> "OF unit address for device matching"
>
>> > + unsigned int of_reg;
>> > +
>> > /* Matches ACPI */
>> > const struct mfd_cell_acpi_match *acpi_match;
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 07/15] PCI: brcmstb: Add control of rescal reset
From: Florian Fainelli @ 2020-05-25 16:58 UTC (permalink / raw)
To: Jim Quinlan, Philipp Zabel
Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Rob Herring, Lorenzo Pieralisi,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS, open list,
maintainer:BROADCOM BCM7XXX ARM ARCHITECTURE,
moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
Bjorn Helgaas, Nicolas Saenz Julienne
In-Reply-To: <CA+-6iNxtW66QLrb5BaOOCPJwG-1fShdfZqiLSkKfi6Y669dn5w@mail.gmail.com>
On 5/21/2020 2:48 PM, Jim Quinlan wrote:
> On Wed, May 20, 2020 at 3:27 AM Philipp Zabel <pza@pengutronix.de> wrote:
>>
>> Hi Jim,
>>
>> On Tue, May 19, 2020 at 04:34:05PM -0400, Jim Quinlan wrote:
>>> From: Jim Quinlan <jquinlan@broadcom.com>
>>>
>>> Some STB chips have a special purpose reset controller named
>>> RESCAL (reset calibration). This commit adds the control
>>> of RESCAL as well as the ability to start and stop its
>>> operation for PCIe HW.
>>>
>>> Signed-off-by: Jim Quinlan <jquinlan@broadcom.com>
>>> ---
>>> drivers/pci/controller/pcie-brcmstb.c | 81 ++++++++++++++++++++++++++-
>>> 1 file changed, 80 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
>>> index 2c470104ba38..0787e8f6f7e5 100644
>>> --- a/drivers/pci/controller/pcie-brcmstb.c
>>> +++ b/drivers/pci/controller/pcie-brcmstb.c
>> [...]
>>> @@ -1100,6 +1164,21 @@ static int brcm_pcie_probe(struct platform_device *pdev)
>>> dev_err(&pdev->dev, "could not enable clock\n");
>>> return ret;
>>> }
>>> + pcie->rescal = devm_reset_control_get_shared(&pdev->dev, "rescal");
>>> + if (IS_ERR(pcie->rescal)) {
>>> + if (PTR_ERR(pcie->rescal) == -EPROBE_DEFER)
>>> + return -EPROBE_DEFER;
>>> + pcie->rescal = NULL;
>>
>> This is effectively an optional reset control, so it is better to use:
>> ↵
>> pcie->rescal = devm_reset_control_get_optional_shared(&pdev->dev,
>> "rescal");↵
>> if (IS_ERR(pcie->rescal))
>> return PTR_ERR(pcie->rescal);
>>
>>> + } else {
>>> + ret = reset_control_deassert(pcie->rescal);
>>> + if (ret)
>>> + dev_err(&pdev->dev, "failed to deassert 'rescal'\n");
>>> + }
>>
>> reset_control_* can handle rstc == NULL parameters for optional reset
>> controls, so this can be done unconditionally:
>>
>> ret = reset_control_deassert(pcie->rescal);↵
>> if (ret)↵
>> dev_err(&pdev->dev, "failed to deassert 'rescal'\n");↵
>>
>> Is rescal handled by the reset-brcmstb-rescal driver? Since that only
>> implements the .reset op, I would expect reset_control_reset() here.
> Where exactly? "reset.h" says that "Calling reset_control_rese()t is
> not allowed on a shared reset control." so I'm not sure why you would
> want me to invoke it.
Yes this is handled by drivers/reset/reset-brcmstb-rescal.c which only
implements a .reset() callback, what would be the appropriate API usage
here given that this is a shared reset between AHCI and PCIe, should
drivers/reset/reset-brcmstb-rescal.c not implement a .reset() callback
and .assert() callback instead?
>> Otherwise this looks like it'd be missing a reset_control_assert in
>> remove.
> I can add this.
> Thanks,
> Jim
>>
>> regards
>> Philipp
--
Florian
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH v2] iio: stm32-adc: remove usage of iio_priv_to_dev() helper
From: Fabrice Gasnier @ 2020-05-25 16:27 UTC (permalink / raw)
To: Alexandru Ardelean, linux-iio, linux-stm32, linux-arm-kernel,
linux-kernel
Cc: olivier.moysan, mcoquelin.stm32, jic23, alexandre.torgue
In-Reply-To: <20200525090720.72696-1-alexandru.ardelean@analog.com>
On 5/25/20 11:07 AM, Alexandru Ardelean wrote:
> We may want to get rid of the iio_priv_to_dev() helper. The reason is that
> we will hide some of the members of the iio_dev structure (to prevent
> drivers from accessing them directly), and that will also mean hiding the
> implementation of the iio_priv_to_dev() helper inside the IIO core.
>
> Hiding the implementation of iio_priv_to_dev() implies that some fast-paths
> may not be fast anymore, so a general idea is to try to get rid of the
> iio_priv_to_dev() altogether.
> The iio_priv() helper won't be affected by the rework, as the iio_dev
> struct will keep a reference to the private information.
>
> For this driver, not using iio_priv_to_dev(), means reworking some paths to
> pass the iio device and using iio_priv() to access the private information.
>
> Signed-off-by: Alexandru Ardelean <alexandru.ardelean@analog.com>
> ---
> drivers/iio/adc/stm32-adc.c | 108 +++++++++++++++++++-----------------
> 1 file changed, 58 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/iio/adc/stm32-adc.c b/drivers/iio/adc/stm32-adc.c
> index ae622ee6d08c..9428c5c22712 100644
> --- a/drivers/iio/adc/stm32-adc.c
> +++ b/drivers/iio/adc/stm32-adc.c
> @@ -162,10 +162,10 @@ struct stm32_adc_cfg {
> struct stm32_adc_trig_info *trigs;
> bool clk_required;
> bool has_vregready;
> - int (*prepare)(struct stm32_adc *);
> - void (*start_conv)(struct stm32_adc *, bool dma);
> - void (*stop_conv)(struct stm32_adc *);
> - void (*unprepare)(struct stm32_adc *);
> + int (*prepare)(struct iio_dev *);
> + void (*start_conv)(struct iio_dev *, bool dma);
> + void (*stop_conv)(struct iio_dev *);
> + void (*unprepare)(struct iio_dev *);
> const unsigned int *smp_cycles;
> };
>
> @@ -538,10 +538,11 @@ static void stm32_adc_set_res(struct stm32_adc *adc)
>
> static int stm32_adc_hw_stop(struct device *dev)
> {
> - struct stm32_adc *adc = dev_get_drvdata(dev);
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct stm32_adc *adc = iio_priv(indio_dev);
>
> if (adc->cfg->unprepare)
> - adc->cfg->unprepare(adc);
> + adc->cfg->unprepare(indio_dev);
>
> if (adc->clk)
> clk_disable_unprepare(adc->clk);
> @@ -551,7 +552,8 @@ static int stm32_adc_hw_stop(struct device *dev)
>
> static int stm32_adc_hw_start(struct device *dev)
> {
> - struct stm32_adc *adc = dev_get_drvdata(dev);
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int ret;
>
> if (adc->clk) {
> @@ -563,7 +565,7 @@ static int stm32_adc_hw_start(struct device *dev)
> stm32_adc_set_res(adc);
>
> if (adc->cfg->prepare) {
> - ret = adc->cfg->prepare(adc);
> + ret = adc->cfg->prepare(indio_dev);
> if (ret)
> goto err_clk_dis;
> }
> @@ -587,8 +589,10 @@ static int stm32_adc_hw_start(struct device *dev)
> * conversions, in IIO buffer modes. Otherwise, use ADC interrupt with direct
> * DR read instead (e.g. read_raw, or triggered buffer mode without DMA).
> */
> -static void stm32f4_adc_start_conv(struct stm32_adc *adc, bool dma)
> +static void stm32f4_adc_start_conv(struct iio_dev *indio_dev, bool dma)
Hi Alexandru,
I've tested your patch. I've no objection, but found few build warnings
(some of these routines have kernel-doc style).
Building with W=1 makes warnings appear, like:
drivers/iio/adc/stm32-adc.c:593: warning: Function parameter or member
'indio_dev' not described in 'stm32f4_adc_start_conv'
drivers/iio/adc/stm32-adc.c:593: warning: Excess function parameter
'adc' description in 'stm32f4_adc_start_conv'
...
Could you update routine's doc as well ?
e.g. something like:
- * @adc: stm32 adc instance
+ * @indio_dev: IIO device
> {
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> stm32_adc_set_bits(adc, STM32F4_ADC_CR1, STM32F4_SCAN);
>
> if (dma)
> @@ -605,8 +609,10 @@ static void stm32f4_adc_start_conv(struct stm32_adc *adc, bool dma)
> stm32_adc_set_bits(adc, STM32F4_ADC_CR2, STM32F4_SWSTART);
> }
>
> -static void stm32f4_adc_stop_conv(struct stm32_adc *adc)
> +static void stm32f4_adc_stop_conv(struct iio_dev *indio_dev)
> {
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> stm32_adc_clr_bits(adc, STM32F4_ADC_CR2, STM32F4_EXTEN_MASK);
> stm32_adc_clr_bits(adc, STM32F4_ADC_SR, STM32F4_STRT);
>
> @@ -615,8 +621,9 @@ static void stm32f4_adc_stop_conv(struct stm32_adc *adc)
> STM32F4_ADON | STM32F4_DMA | STM32F4_DDS);
> }
>
> -static void stm32h7_adc_start_conv(struct stm32_adc *adc, bool dma)
> +static void stm32h7_adc_start_conv(struct iio_dev *indio_dev, bool dma)
> {
> + struct stm32_adc *adc = iio_priv(indio_dev);
> enum stm32h7_adc_dmngt dmngt;
> unsigned long flags;
> u32 val;
> @@ -635,9 +642,9 @@ static void stm32h7_adc_start_conv(struct stm32_adc *adc, bool dma)
> stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_ADSTART);
> }
>
> -static void stm32h7_adc_stop_conv(struct stm32_adc *adc)
> +static void stm32h7_adc_stop_conv(struct iio_dev *indio_dev)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int ret;
> u32 val;
>
> @@ -652,9 +659,9 @@ static void stm32h7_adc_stop_conv(struct stm32_adc *adc)
> stm32_adc_clr_bits(adc, STM32H7_ADC_CFGR, STM32H7_DMNGT_MASK);
> }
>
> -static int stm32h7_adc_exit_pwr_down(struct stm32_adc *adc)
> +static int stm32h7_adc_exit_pwr_down(struct iio_dev *indio_dev)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int ret;
> u32 val;
>
> @@ -690,9 +697,9 @@ static void stm32h7_adc_enter_pwr_down(struct stm32_adc *adc)
> stm32_adc_set_bits(adc, STM32H7_ADC_CR, STM32H7_DEEPPWD);
> }
>
> -static int stm32h7_adc_enable(struct stm32_adc *adc)
> +static int stm32h7_adc_enable(struct iio_dev *indio_dev)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int ret;
> u32 val;
>
> @@ -713,9 +720,9 @@ static int stm32h7_adc_enable(struct stm32_adc *adc)
> return ret;
> }
>
> -static void stm32h7_adc_disable(struct stm32_adc *adc)
> +static void stm32h7_adc_disable(struct iio_dev *indio_dev)
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int ret;
> u32 val;
>
> @@ -733,9 +740,9 @@ static void stm32h7_adc_disable(struct stm32_adc *adc)
> * @adc: stm32 adc instance
> * Note: Must be called once ADC is enabled, so LINCALRDYW[1..6] are writable
> */
> -static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> +static int stm32h7_adc_read_selfcalib(struct iio_dev *indio_dev)
Same here.
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int i, ret;
> u32 lincalrdyw_mask, val;
>
> @@ -777,9 +784,9 @@ static int stm32h7_adc_read_selfcalib(struct stm32_adc *adc)
> * @adc: stm32 adc instance
> * Note: ADC must be enabled, with no on-going conversions.
> */
> -static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
> +static int stm32h7_adc_restore_selfcalib(struct iio_dev *indio_dev)
Same here.
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int i, ret;
> u32 lincalrdyw_mask, val;
>
> @@ -850,9 +857,9 @@ static int stm32h7_adc_restore_selfcalib(struct stm32_adc *adc)
> * @adc: stm32 adc instance
> * Note: Must be called once ADC is out of power down.
> */
> -static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> +static int stm32h7_adc_selfcalib(struct iio_dev *indio_dev)
Same here
> {
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int ret;
> u32 val;
>
> @@ -912,30 +919,31 @@ static int stm32h7_adc_selfcalib(struct stm32_adc *adc)
> * - Only one input is selected for single ended (e.g. 'vinp')
> * - Two inputs are selected for differential channels (e.g. 'vinp' & 'vinn')
> */
> -static int stm32h7_adc_prepare(struct stm32_adc *adc)
> +static int stm32h7_adc_prepare(struct iio_dev *indio_dev)
Same here.
With the comments updated, you can add my:
Acked-by: Fabrice Gasnier <fabrice.gasnier@st.com>
Thanks for the patch,
Fabrice
> {
> + struct stm32_adc *adc = iio_priv(indio_dev);
> int calib, ret;
>
> - ret = stm32h7_adc_exit_pwr_down(adc);
> + ret = stm32h7_adc_exit_pwr_down(indio_dev);
> if (ret)
> return ret;
>
> - ret = stm32h7_adc_selfcalib(adc);
> + ret = stm32h7_adc_selfcalib(indio_dev);
> if (ret < 0)
> goto pwr_dwn;
> calib = ret;
>
> stm32_adc_writel(adc, STM32H7_ADC_DIFSEL, adc->difsel);
>
> - ret = stm32h7_adc_enable(adc);
> + ret = stm32h7_adc_enable(indio_dev);
> if (ret)
> goto pwr_dwn;
>
> /* Either restore or read calibration result for future reference */
> if (calib)
> - ret = stm32h7_adc_restore_selfcalib(adc);
> + ret = stm32h7_adc_restore_selfcalib(indio_dev);
> else
> - ret = stm32h7_adc_read_selfcalib(adc);
> + ret = stm32h7_adc_read_selfcalib(indio_dev);
> if (ret)
> goto disable;
>
> @@ -944,16 +952,18 @@ static int stm32h7_adc_prepare(struct stm32_adc *adc)
> return 0;
>
> disable:
> - stm32h7_adc_disable(adc);
> + stm32h7_adc_disable(indio_dev);
> pwr_dwn:
> stm32h7_adc_enter_pwr_down(adc);
>
> return ret;
> }
>
> -static void stm32h7_adc_unprepare(struct stm32_adc *adc)
> +static void stm32h7_adc_unprepare(struct iio_dev *indio_dev)
> {
> - stm32h7_adc_disable(adc);
> + struct stm32_adc *adc = iio_priv(indio_dev);
> +
> + stm32h7_adc_disable(indio_dev);
> stm32h7_adc_enter_pwr_down(adc);
> }
>
> @@ -1160,7 +1170,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
>
> stm32_adc_conv_irq_enable(adc);
>
> - adc->cfg->start_conv(adc, false);
> + adc->cfg->start_conv(indio_dev, false);
>
> timeout = wait_for_completion_interruptible_timeout(
> &adc->completion, STM32_ADC_TIMEOUT);
> @@ -1173,7 +1183,7 @@ static int stm32_adc_single_conv(struct iio_dev *indio_dev,
> ret = IIO_VAL_INT;
> }
>
> - adc->cfg->stop_conv(adc);
> + adc->cfg->stop_conv(indio_dev);
>
> stm32_adc_conv_irq_disable(adc);
>
> @@ -1227,8 +1237,8 @@ static int stm32_adc_read_raw(struct iio_dev *indio_dev,
>
> static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
> {
> - struct stm32_adc *adc = data;
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = data;
> + struct stm32_adc *adc = iio_priv(indio_dev);
> const struct stm32_adc_regspec *regs = adc->cfg->regs;
> u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
>
> @@ -1240,8 +1250,8 @@ static irqreturn_t stm32_adc_threaded_isr(int irq, void *data)
>
> static irqreturn_t stm32_adc_isr(int irq, void *data)
> {
> - struct stm32_adc *adc = data;
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = data;
> + struct stm32_adc *adc = iio_priv(indio_dev);
> const struct stm32_adc_regspec *regs = adc->cfg->regs;
> u32 status = stm32_adc_readl(adc, regs->isr_eoc.reg);
>
> @@ -1514,7 +1524,7 @@ static int __stm32_adc_buffer_postenable(struct iio_dev *indio_dev)
> if (!adc->dma_chan)
> stm32_adc_conv_irq_enable(adc);
>
> - adc->cfg->start_conv(adc, !!adc->dma_chan);
> + adc->cfg->start_conv(indio_dev, !!adc->dma_chan);
>
> return 0;
>
> @@ -1547,7 +1557,7 @@ static void __stm32_adc_buffer_predisable(struct iio_dev *indio_dev)
> struct stm32_adc *adc = iio_priv(indio_dev);
> struct device *dev = indio_dev->dev.parent;
>
> - adc->cfg->stop_conv(adc);
> + adc->cfg->stop_conv(indio_dev);
> if (!adc->dma_chan)
> stm32_adc_conv_irq_disable(adc);
>
> @@ -1891,7 +1901,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
> indio_dev->info = &stm32_adc_iio_info;
> indio_dev->modes = INDIO_DIRECT_MODE | INDIO_HARDWARE_TRIGGERED;
>
> - platform_set_drvdata(pdev, adc);
> + platform_set_drvdata(pdev, indio_dev);
>
> ret = of_property_read_u32(pdev->dev.of_node, "reg", &adc->offset);
> if (ret != 0) {
> @@ -1905,7 +1915,7 @@ static int stm32_adc_probe(struct platform_device *pdev)
>
> ret = devm_request_threaded_irq(&pdev->dev, adc->irq, stm32_adc_isr,
> stm32_adc_threaded_isr,
> - 0, pdev->name, adc);
> + 0, pdev->name, indio_dev);
> if (ret) {
> dev_err(&pdev->dev, "failed to request IRQ\n");
> return ret;
> @@ -1989,8 +1999,8 @@ static int stm32_adc_probe(struct platform_device *pdev)
>
> static int stm32_adc_remove(struct platform_device *pdev)
> {
> - struct stm32_adc *adc = platform_get_drvdata(pdev);
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = platform_get_drvdata(pdev);
> + struct stm32_adc *adc = iio_priv(indio_dev);
>
> pm_runtime_get_sync(&pdev->dev);
> iio_device_unregister(indio_dev);
> @@ -2012,8 +2022,7 @@ static int stm32_adc_remove(struct platform_device *pdev)
> #if defined(CONFIG_PM_SLEEP)
> static int stm32_adc_suspend(struct device *dev)
> {
> - struct stm32_adc *adc = dev_get_drvdata(dev);
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
>
> if (iio_buffer_enabled(indio_dev))
> __stm32_adc_buffer_predisable(indio_dev);
> @@ -2023,8 +2032,7 @@ static int stm32_adc_suspend(struct device *dev)
>
> static int stm32_adc_resume(struct device *dev)
> {
> - struct stm32_adc *adc = dev_get_drvdata(dev);
> - struct iio_dev *indio_dev = iio_priv_to_dev(adc);
> + struct iio_dev *indio_dev = dev_get_drvdata(dev);
> int ret;
>
> ret = pm_runtime_force_resume(dev);
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCHv3 0/2] Add support for replicators which loses context on clock removal
From: Mathieu Poirier @ 2020-05-25 16:15 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: devicetree, Suzuki K Poulose, linux-arm-msm, linux-kernel,
Stephen Boyd, Rob Herring, linux-arm-kernel, Mike Leach
In-Reply-To: <cover.1590171891.git.saiprakash.ranjan@codeaurora.org>
Hi Sai,
On Sat, May 23, 2020 at 12:06:50AM +0530, Sai Prakash Ranjan wrote:
> This series is mainly to add support for replicators
> which lose context on removing AMBA clock like on SC7180
> SoC where replicator in AOSS domain loses context.
>
I am good with this set but need a reviewed-by on the DT binding before I can
add it to my tree. The same goes for your other set[1].
Thanks,
Mathieu
[1]. "coresight: etm4x: Add support to skip trace unit power up"
> v2 - https://lore.kernel.org/patchwork/cover/1244340/
> More discussion is found here - https://lore.kernel.org/patchwork/patch/1231182/
>
> Changes since v2:
> * Added DT maintainers which I missed in v2
> * Added proper kernel-doc and header as per Mathieu
>
> Sai Prakash Ranjan (2):
> coresight: replicator: Reset replicator if context is lost
> dt-bindings: arm: coresight: Add optional property to replicators
>
> .../devicetree/bindings/arm/coresight.txt | 6 ++
> .../coresight/coresight-replicator.c | 55 +++++++++++++------
> 2 files changed, 44 insertions(+), 17 deletions(-)
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [EXT] Re: [PATCH] stmmac: platform: add "snps,dwmac-5.10a" IP compatible string
From: Andy Duan @ 2020-05-25 16:13 UTC (permalink / raw)
To: Andrew Lunn
Cc: p.zabel@pengutronix.de, alexandre.torgue@st.com,
netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
joabreu@synopsys.com, mcoquelin.stm32@gmail.com, kuba@kernel.org,
peppe.cavallaro@st.com, davem@davemloft.net,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20200525160537.GD762220@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch> Sent: Tuesday, May 26, 2020 12:06 AM
> On Mon, May 25, 2020 at 04:00:29PM +0000, Andy Duan wrote:
> > From: Andrew Lunn <andrew@lunn.ch> Sent: Monday, May 25, 2020 10:11
> PM
> > > On Mon, May 25, 2020 at 04:22:25PM +0800, Fugang Duan wrote:
> > > > Add "snps,dwmac-5.10a" compatible string for 5.10a version that
> > > > can avoid to define some plat data in glue layer.
> > >
> > > Documentation/devicetree/bindings/net/snps,dwmac.yaml ?
> > >
> > > Andrew
> >
> > Here, we don't want to use generic driver "dwmac-generic.c" for 5.10a
> > version since it requires platform specific code to be functional,
> > like the we implement glue layer driver "dwmac-imx.c" to support 5.10a on
> i.MX platform.
> >
> > So I think it doesn't require to add the compatible string into dwmac.yaml.
>
> Hi Andy
>
> It needs to be documented somewhere. If not
> Documentation/devicetree/bindings/net/snps,dwmac.yaml it needs to be in
> an NXP specific document.
>
> Andrew
Yes, it can be added into NXP binding document.
I wait other's comment for dwmac-imx.c driver review, then will add it together
in next version.
Thanks for your comments.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [EXT] Re: [PATCH] stmmac: platform: add "snps,dwmac-5.10a" IP compatible string
From: Andrew Lunn @ 2020-05-25 16:05 UTC (permalink / raw)
To: Andy Duan
Cc: p.zabel@pengutronix.de, alexandre.torgue@st.com,
netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
joabreu@synopsys.com, mcoquelin.stm32@gmail.com, kuba@kernel.org,
peppe.cavallaro@st.com, davem@davemloft.net,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <AM6PR0402MB3607312E97B14B09C398B586FFB30@AM6PR0402MB3607.eurprd04.prod.outlook.com>
On Mon, May 25, 2020 at 04:00:29PM +0000, Andy Duan wrote:
> From: Andrew Lunn <andrew@lunn.ch> Sent: Monday, May 25, 2020 10:11 PM
> > On Mon, May 25, 2020 at 04:22:25PM +0800, Fugang Duan wrote:
> > > Add "snps,dwmac-5.10a" compatible string for 5.10a version that can
> > > avoid to define some plat data in glue layer.
> >
> > Documentation/devicetree/bindings/net/snps,dwmac.yaml ?
> >
> > Andrew
>
> Here, we don't want to use generic driver "dwmac-generic.c" for 5.10a version
> since it requires platform specific code to be functional, like the we implement
> glue layer driver "dwmac-imx.c" to support 5.10a on i.MX platform.
>
> So I think it doesn't require to add the compatible string into dwmac.yaml.
Hi Andy
It needs to be documented somewhere. If not
Documentation/devicetree/bindings/net/snps,dwmac.yaml it needs to be
in an NXP specific document.
Andrew
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCHv3 2/2] dt-bindings: arm: coresight: Add optional property to replicators
From: Mathieu Poirier @ 2020-05-25 16:01 UTC (permalink / raw)
To: Sai Prakash Ranjan
Cc: devicetree, Suzuki K Poulose, linux-arm-msm,
Linux Kernel Mailing List, Stephen Boyd, Rob Herring,
linux-arm-kernel, Mike Leach
In-Reply-To: <ccfe8a5ede0523436508e31085322ccdab8f972a.1590171891.git.saiprakash.ranjan@codeaurora.org>
On Fri, 22 May 2020 at 12:37, Sai Prakash Ranjan
<saiprakash.ranjan@codeaurora.org> wrote:
>
> Add an optional boolean property "qcom,replicator-loses-context" to
> identify replicators which loses context when AMBA clocks are removed
> in certain configurable replicator designs.
>
> Reviewed-by: Mike Leach <mike.leach@linaro.org>
> Signed-off-by: Sai Prakash Ranjan <saiprakash.ranjan@codeaurora.org>
> ---
> Documentation/devicetree/bindings/arm/coresight.txt | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/arm/coresight.txt b/Documentation/devicetree/bindings/arm/coresight.txt
> index 846f6daae71b..b598a5f0037d 100644
> --- a/Documentation/devicetree/bindings/arm/coresight.txt
> +++ b/Documentation/devicetree/bindings/arm/coresight.txt
> @@ -121,6 +121,12 @@ its hardware characteristcs.
> * interrupts : Exactly one SPI may be listed for reporting the address
> error
>
> +* Optional property for configurable replicators:
> +
> + * qcom,replicator-loses-context: boolean. Indicates that the replicator
> + will lose register context when AMBA clock is removed which is observed
> + in some replicator designs.
Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> +
> Graph bindings for Coresight
> -------------------------------
>
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [EXT] Re: [PATCH] stmmac: platform: add "snps,dwmac-5.10a" IP compatible string
From: Andy Duan @ 2020-05-25 16:00 UTC (permalink / raw)
To: Andrew Lunn
Cc: p.zabel@pengutronix.de, alexandre.torgue@st.com,
netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
joabreu@synopsys.com, mcoquelin.stm32@gmail.com, kuba@kernel.org,
peppe.cavallaro@st.com, davem@davemloft.net,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <20200525141048.GF752669@lunn.ch>
From: Andrew Lunn <andrew@lunn.ch> Sent: Monday, May 25, 2020 10:11 PM
> On Mon, May 25, 2020 at 04:22:25PM +0800, Fugang Duan wrote:
> > Add "snps,dwmac-5.10a" compatible string for 5.10a version that can
> > avoid to define some plat data in glue layer.
>
> Documentation/devicetree/bindings/net/snps,dwmac.yaml ?
>
> Andrew
Here, we don't want to use generic driver "dwmac-generic.c" for 5.10a version
since it requires platform specific code to be functional, like the we implement
glue layer driver "dwmac-imx.c" to support 5.10a on i.MX platform.
So I think it doesn't require to add the compatible string into dwmac.yaml.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: kvm: Fix incorrect comment on kvm_get_hyp_vector()
From: Marc Zyngier @ 2020-05-25 15:48 UTC (permalink / raw)
To: David Brazdil; +Cc: kvmarm, linux-arm-kernel, linux-kernel
In-Reply-To: <20200515152550.83810-1-dbrazdil@google.com>
On Fri, 15 May 2020 16:25:50 +0100, David Brazdil wrote:
> The comment used to say that kvm_get_hyp_vector is only called on VHE systems.
> In fact, it is also called from the nVHE init function cpu_init_hyp_mode().
> Fix the comment to stop confusing devs.
>
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/20200515152550.83810-1-dbrazdil@google.com
>
> [...]
Applied to kvmarm-master/next, thanks!
[1/1] KVM: arm64: Fix incorrect comment on kvm_get_hyp_vector()
commit: 438f711ce1d889632467be80779c8f5762b107d7
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH] arm64: kvm: Clean up cpu_init_hyp_mode()
From: Marc Zyngier @ 2020-05-25 15:48 UTC (permalink / raw)
To: David Brazdil; +Cc: kvmarm, linux-arm-kernel, linux-kernel
In-Reply-To: <20200515152056.83158-1-dbrazdil@google.com>
On Fri, 15 May 2020 16:20:56 +0100, David Brazdil wrote:
> Pull bits of code to the only place where it is used. Remove empty function
> __cpu_init_stage2(). Remove redundant has_vhe() check since this function is
> nVHE-only. No functional changes intended.
>
> Signed-off-by: David Brazdil <dbrazdil@google.com>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> Link: https://lore.kernel.org/r/20200515152056.83158-1-dbrazdil@google.com
>
> [...]
Applied to kvmarm-master/next, thanks!
[1/1] KVM: arm64: Clean up cpu_init_hyp_mode()
commit: 71b3ec5f221b8b3ff545639be83ddfcd5d7c9800
Cheers,
M.
--
Without deviation from the norm, progress is not possible.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [PATCH 15/21] drm/rcar-du: Use GEM CMA object functions
From: Kieran Bingham @ 2020-05-25 15:38 UTC (permalink / raw)
To: Thomas Zimmermann, Laurent Pinchart
Cc: alexandre.belloni, linux-aspeed, narmstrong, airlied, liviu.dudau,
dri-devel, paul, mihail.atanassov, sam, marex, khilman, abrodkin,
kong.kongxinwei, xinliang.liu, ludovic.desroches, tomi.valkeinen,
james.qian.wang, joel, linux-imx, alexandre.torgue, puck.chen,
s.hauer, alison.wang, jsarha, wens, vincent.abriou,
linux-arm-kernel, mcoquelin.stm32, bbrezillon, andrew,
philippe.cornu, yannick.fertre, kernel, zourongrong, shawnguo
In-Reply-To: <816a8a0e-bb98-ea6c-5016-94b18e045fb5@suse.de>
On 25/05/2020 13:49, Thomas Zimmermann wrote:
> Hi
>
> Am 22.05.20 um 22:12 schrieb Laurent Pinchart:
>> Hi Thomas,
>>
>> Thank you for the patch.
>>
>> On Fri, May 22, 2020 at 03:52:40PM +0200, Thomas Zimmermann wrote:
>>> The rcar-du driver uses the default implementation for CMA functions;
>>> except for the .dumb_create callback. The __DRM_GEM_CMA_DRIVER_OPS macro
>>> now sets these defaults and .dumb_create in struct drm_driver. All
>>> remaining operations are provided by CMA GEM object functions.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>> drivers/gpu/drm/rcar-du/rcar_du_drv.c | 11 +----------
>>> 1 file changed, 1 insertion(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.c b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>>> index 3e67cf70f0402..3728038cec1d1 100644
>>> --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>>> +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.c
>>> @@ -476,16 +476,7 @@ DEFINE_DRM_GEM_CMA_FOPS(rcar_du_fops);
>>>
>>> static struct drm_driver rcar_du_driver = {
>>> .driver_features = DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC,
>>> - .gem_free_object_unlocked = drm_gem_cma_free_object,
>>> - .gem_vm_ops = &drm_gem_cma_vm_ops,
>>> - .prime_handle_to_fd = drm_gem_prime_handle_to_fd,
>>> - .prime_fd_to_handle = drm_gem_prime_fd_to_handle,
>>> - .gem_prime_get_sg_table = drm_gem_cma_prime_get_sg_table,
>>> - .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table,
>>> - .gem_prime_vmap = drm_gem_cma_prime_vmap,
>>> - .gem_prime_vunmap = drm_gem_cma_prime_vunmap,
>>> - .gem_prime_mmap = drm_gem_cma_prime_mmap,
>>> - .dumb_create = rcar_du_dumb_create,
>>> + __DRM_GEM_CMA_DRIVER_OPS(rcar_du_dumb_create),
>>
>> Your __DRM_GEM_CMA_DRIVER_OPS is defined as
>>
>> #define __DRM_GEM_CMA_DRIVER_OPS(__dumb_create) \
>> .gem_create_object = drm_cma_gem_create_object_default_funcs, \
>> .dumb_create = (__dumb_create), \
>> .prime_handle_to_fd = drm_gem_prime_handle_to_fd, \
>> .prime_fd_to_handle = drm_gem_prime_fd_to_handle, \
>> .gem_prime_import_sg_table = drm_gem_cma_prime_import_sg_table_vmap, \
>> .gem_prime_mmap = drm_gem_prime_mmap
>>
>> The patch thus introduces several changes:
>>
>> - drm_gem_cma_prime_import_sg_table_vmap() is used instead of
>> drm_gem_cma_prime_import_sg_table() combined with .gem_prime_vmap()
>> and .gem_prime_vunmap(). I believe that's fine, but splitting that
>> change in a separate commit, or at the very least explaining it in
>> details in the commit message, would make review easier.
>>
>> - .gem_create_object() is now set. That seems to be OK, but I'm not sure
>> to grasp all the implications. This should also be explained in the
>> commit message, and ideally split to a separate patch.
>
> That's relevant during object creation and sets the object functions.
> See one of my other replies for how this can go away after all CMA
> drivers have been updated to GEM object functions.
>
>
>>
>> - drm_gem_cma_prime_mmap() is replaced with drm_gem_prime_mmap(). Same
>> comments :-)
>
> I relied on the aspeed driver to be correct. After Sam's comment on
> that, I read the code once again several times. The original
> implementation clears VM_PFNMAP. And I cannot find that code any longer.
> Going back to the original function might be better.
>
>
>>
>> This patch hides way too many changes in what is documented as just
>> innocent refactoring. It seems other drivers are affected too.
>
> Could you test the patchset? I don't have the HW.
Digging out the branch you provided elsewhere in this thread:
>>> Could you boot-test with the patchset applied?
>>
>> Yes, if you have a git branch I can just build and boot I can
>> do it quickly!
>
> Fantastic! It's the cma-objfuncs branch of
>
> https://gitlab.freedesktop.org/tzimmermann/linux.git
I have successfully run our display tests with your patches here on an
R-Car H3 Salvator-XS(-es2).
Tested-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> Best regards
> Thomas
>
>>
>>> .fops = &rcar_du_fops,
>>> .name = "rcar-du",
>>> .desc = "Renesas R-Car Display Unit",
>>
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH 0/7] kvm: arm64: Support stage2 hardware DBM
From: Marc Zyngier @ 2020-05-25 15:44 UTC (permalink / raw)
To: Keqian Zhu
Cc: Andrew Morton, kvm, Suzuki K Poulose, Catalin Marinas,
linux-kernel, Sean Christopherson, Alexios Zavras, zhengxiang9,
Mark Brown, James Morse, Julien Thierry, wanghaibin.wang,
Thomas Gleixner, Will Deacon, kvmarm, linux-arm-kernel
In-Reply-To: <20200525112406.28224-1-zhukeqian1@huawei.com>
On 2020-05-25 12:23, Keqian Zhu wrote:
> This patch series add support for stage2 hardware DBM, and it is only
> used for dirty log for now.
>
> It works well under some migration test cases, including VM with 4K
> pages or 2M THP. I checked the SHA256 hash digest of all memory and
> they keep same for source VM and destination VM, which means no dirty
> pages is missed under hardware DBM.
>
> However, there are some known issues not solved.
>
> 1. Some mechanisms that rely on "write permission fault" become
> invalid,
> such as kvm_set_pfn_dirty and "mmap page sharing".
>
> kvm_set_pfn_dirty is called in user_mem_abort when guest issues
> write
> fault. This guarantees physical page will not be dropped directly
> when
> host kernel recycle memory. After using hardware dirty management,
> we
> have no chance to call kvm_set_pfn_dirty.
Then you will end-up with memory corruption under memory pressure.
This also breaks things like CoW, which we depend on.
>
> For "mmap page sharing" mechanism, host kernel will allocate a new
> physical page when guest writes a page that is shared with other
> page
> table entries. After using hardware dirty management, we have no
> chance
> to do this too.
>
> I need to do some survey on how stage1 hardware DBM solve these
> problems.
> It helps if anyone can figure it out.
>
> 2. Page Table Modification Races: Though I have found and solved some
> data
> races when kernel changes page table entries, I still doubt that
> there
> are data races I am not aware of. It's great if anyone can figure
> them out.
>
> 3. Performance: Under Kunpeng 920 platform, for every 64GB memory, KVM
> consumes about 40ms to traverse all PTEs to collect dirty log. It
> will
> cause unbearable downtime for migration if memory size is too big. I
> will
> try to solve this problem in Patch v1.
This, in my opinion, is why Stage-2 DBM is fairly useless.
From a performance perspective, this is the worse possible
situation. You end up continuously scanning page tables, at
an arbitrary rate, without a way to evaluate the fault rate.
One thing S2-DBM would be useful for is SVA, where a device
write would mark the S2 PTs dirty as they are shared between
CPU and SMMU. Another thing is SPE, which is essentially a DMA
agent using the CPU's PTs.
But on its own, and just to log the dirty pages, S2-DBM is
pretty rubbish. I wish arm64 had something like Intel's PML,
which looks far more interesting for the purpose of tracking
accesses.
Thanks,
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: [RFC PATCH v12 10/11] arm64: add mechanism to let user choose which counter to return
From: Marc Zyngier @ 2020-05-25 15:28 UTC (permalink / raw)
To: Jianyong Wu
Cc: Mark Rutland, Justin He, Wei Chen, kvm, Suzuki Poulose, netdev,
Richard Cochran, Steve Capper, linux-kernel,
sean.j.christopherson, Steven Price, Kaly Xin, john.stultz,
yangbo.lu, pbonzini, tglx, nd, will, kvmarm, linux-arm-kernel
In-Reply-To: <HE1PR0802MB2555E64BD5C076E5AF08E644F4B30@HE1PR0802MB2555.eurprd08.prod.outlook.com>
On 2020-05-25 15:18, Jianyong Wu wrote:
> Hi Marc,
>
>> -----Original Message-----
>> From: Marc Zyngier <maz@kernel.org>
>> Sent: Monday, May 25, 2020 5:17 PM
>> To: Richard Cochran <richardcochran@gmail.com>; Jianyong Wu
>> <Jianyong.Wu@arm.com>
>> Cc: netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
>> tglx@linutronix.de; pbonzini@redhat.com;
>> sean.j.christopherson@intel.com;
>> Mark Rutland <Mark.Rutland@arm.com>; will@kernel.org; Suzuki Poulose
>> <Suzuki.Poulose@arm.com>; Steven Price <Steven.Price@arm.com>; linux-
>> kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
>> kvmarm@lists.cs.columbia.edu; kvm@vger.kernel.org; Steve Capper
>> <Steve.Capper@arm.com>; Kaly Xin <Kaly.Xin@arm.com>; Justin He
>> <Justin.He@arm.com>; Wei Chen <Wei.Chen@arm.com>; nd <nd@arm.com>
>> Subject: Re: [RFC PATCH v12 10/11] arm64: add mechanism to let user
>> choose which counter to return
>>
>> On 2020-05-24 03:11, Richard Cochran wrote:
>> > On Fri, May 22, 2020 at 04:37:23PM +0800, Jianyong Wu wrote:
>> >> In general, vm inside will use virtual counter compered with host use
>> >> phyical counter. But in some special scenarios, like nested
>> >> virtualization, phyical counter maybe used by vm. A interface added
>> >> in ptp_kvm driver to offer a mechanism to let user choose which
>> >> counter should be return from host.
>> >
>> > Sounds like you have two time sources, one for normal guest, and one
>> > for nested. Why not simply offer the correct one to user space
>> > automatically? If that cannot be done, then just offer two PHC
>> > devices with descriptive names.
>>
>> There is no such thing as a distinction between nested or non-nested.
>> Both counters are available to the guest at all times, and said guest
>> can
>> choose whichever it wants to use. So the hypervisor (KVM) has to
>> support
>> both counters as a reference.
>>
> It's great that we can decide which counter to return in guest kernel.
> So we can abandon these code, including patch 9/11 and 10/11, that
> expose the interface to userspace to do the decision.
>
>> For a Linux guest, we always know which reference we're using (the
>> virtual
>> counter). So it is pointless to expose the choice to userspace at all.
>>
> So, we should throw these code of deciding counter type in linux
> driver away and just keep the hypercall service of providing both
> virtual counter and physical counter in linux to server non-linux
> guest.
> Am I right?
Exactly. We control Linux, and so far nothing is using the physical
counter directly. It is only using the virtual counter.
On the other side, this is *only* Linux. Other operating systems
will need to pick the reference clock that matches their own.
If one day we change Linux to use the physical counter, we'll
have to do the same thing.
M.
--
Jazz is not dead. It just smells funny...
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* RE: [PATCH] thermal: imx8mm: Add get_trend ops
From: Anson Huang @ 2020-05-25 15:05 UTC (permalink / raw)
To: Daniel Lezcano, rui.zhang@intel.com, amit.kucheria@verdurent.com,
shawnguo@kernel.org, s.hauer@pengutronix.de,
kernel@pengutronix.de, festevam@gmail.com,
linux-pm@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
In-Reply-To: <dadf94db-8aa5-d1a7-5818-c56032a44ea4@linaro.org>
Hi, Daniel
> Subject: Re: [PATCH] thermal: imx8mm: Add get_trend ops
>
> On 25/05/2020 04:46, Anson Huang wrote:
> > Hi, Daniel
>
> [ ... ]
>
> > I tried modifying the min/max to '2' in cooling map, it works that
> > whenever cooling action is needed, the max cooling action will be
> > applied. But I also noticed some behaviors which NOT as expected:
> >
> > 1. to easy the test, I enable the " CONFIG_THERMAL_WRITABLE_TRIPS",
> > and just modify the passive trip threshold to trigger the cooling
> > action, this is much more easy then putting the board into an oven to
> > increase the SoC temperature or running many high loading test to
> > increase the temperature, but when I modify the passive trip threshold
> > to be lower than current temperature, the cooling action is NOT
> > triggered immediately, it is because the default step_wise governor
> > will NOT trigger the cooling action when the trend is
> > THERMAL_TREND_STABLE. But what expected is, when the temperature is
> > exceed the passive trip threshold, the cooling action can be triggered
> > immediately no matter the trend is stable or raising.
>
> You are right, what is expected is, when the temperature exceeds the passive
> trip threshold, a cooling action happens, the trend is raising in this case.
>
> But in your test, it is not what is happening: the trip point is changing, not the
> temperature.
>
> Probably, the cpufreq driver is at its lowest OPP, so there is no room for more
> cooling effect when changing the trip point.
>
> IMO, the test is not right as the trip point is decreased to a temperature where
> actually the SoC is not hot.
>
> If you want to test it easily, I recommend to use dhrystone, something like:
>
> dhrystone -t 6 -l 10000
>
> That will make your board to heat immediately.
Thanks, I understand. To aligned with the formal test method, I will inform our test
team to update the test case to meet the requirement.
>
> > That
> > means we have to implement our own .get_trend callback?
>
> From my POV it must disappear, because it has little meaning. The governor is
> the one which should be dealing with that and call the corresponding cooling
> index.
OK, I will use common .get_trend() implementation.
>
> > 2. No margin for releasing the cooling action, for example, if cooling
> > action is triggered, when the temperature drops below the passive trip
> > threshold, the cooling action will be cancelled immediately, if SoC
> > keeps running at full performance, the temperature will increase very
> > soon, which may cause the SoC keep triggering/cancelling the cooling
> > action around the passive trip threshold. If there is a margin, the
> > situation will be much better.
> >
> > Do you have any idea/comment about them?
>
> Yes, that is a good point. The hysteresis is supposed to do that. There is a work
> done by Andrzej Pietrasiewicz to disable / enable the thermal zones [1]. I think
> we should be able to fix that after the changes are done.
OK, then I will wait for this change. So to apply MAX cooling action immediately,
all expected changes for i.MX platforms are to assign min/max cooling index in
DT cooling map, I will summit a patch set then.
Thanks,
Anson.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox