[PATCH v3 2/3] powerpc/fadump: Use the correct VMCOREINFO_NOTE_SIZE for phdr
vmcoreinfo_max_size stands for the vmcoreinfo_data, the correct one we should use is vmcoreinfo_note whose total size is VMCOREINFO_NOTE_SIZE. Like explained in commit 77019967f06b ("kdump: fix exported size of vmcoreinfo note"), it does not affect the actual function, we better fix it, also this change should be safe and backward compatible. After this, we can get rid of variable vmcoreinfo_max_size, let's use the macro VMCOREINFO_BYTES instead, fewer variables means more safety for vmcoreinfo operation. Signed-off-by: Xunlei Pang--- arch/powerpc/kernel/fadump.c | 3 +-- include/linux/kexec.h| 1 - kernel/kexec_core.c | 3 +-- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index 8ff0dd4..b8e15cf 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -906,8 +906,7 @@ static int fadump_create_elfcore_headers(char *bufp) phdr->p_paddr = fadump_relocate(paddr_vmcoreinfo_note()); phdr->p_offset = phdr->p_paddr; - phdr->p_memsz = vmcoreinfo_max_size; - phdr->p_filesz = vmcoreinfo_max_size; + phdr->p_memsz = phdr->p_filesz = VMCOREINFO_NOTE_SIZE; /* Increment number of program headers. */ (elf->e_phnum)++; diff --git a/include/linux/kexec.h b/include/linux/kexec.h index f1c601b..6918fda 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -319,7 +319,6 @@ extern void *kexec_purgatory_get_symbol_addr(struct kimage *image, extern note_buf_t __percpu *crash_notes; extern u32 *vmcoreinfo_note; extern size_t vmcoreinfo_size; -extern size_t vmcoreinfo_max_size; /* flag to track if kexec reboot is in progress */ extern bool kexec_in_progress; diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c index e3a4bda..e503b48 100644 --- a/kernel/kexec_core.c +++ b/kernel/kexec_core.c @@ -54,7 +54,6 @@ /* vmcoreinfo stuff */ static unsigned char *vmcoreinfo_data; size_t vmcoreinfo_size; -size_t vmcoreinfo_max_size = VMCOREINFO_BYTES; u32 *vmcoreinfo_note; /* Flag to indicate we are going to kexec a new kernel */ @@ -1386,7 +1385,7 @@ void vmcoreinfo_append_str(const char *fmt, ...) r = vscnprintf(buf, sizeof(buf), fmt, args); va_end(args); - r = min(r, vmcoreinfo_max_size - vmcoreinfo_size); + r = min(r, VMCOREINFO_BYTES - vmcoreinfo_size); memcpy(_data[vmcoreinfo_size], buf, r); -- 1.8.3.1
[PATCH v3 2/3] powerpc/fadump: Use the correct VMCOREINFO_NOTE_SIZE for phdr
vmcoreinfo_max_size stands for the vmcoreinfo_data, the correct one we should use is vmcoreinfo_note whose total size is VMCOREINFO_NOTE_SIZE. Like explained in commit 77019967f06b ("kdump: fix exported size of vmcoreinfo note"), it does not affect the actual function, we better fix it, also this change should be safe and backward compatible. After this, we can get rid of variable vmcoreinfo_max_size, let's use the macro VMCOREINFO_BYTES instead, fewer variables means more safety for vmcoreinfo operation. Signed-off-by: Xunlei Pang --- arch/powerpc/kernel/fadump.c | 3 +-- include/linux/kexec.h| 1 - kernel/kexec_core.c | 3 +-- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/powerpc/kernel/fadump.c b/arch/powerpc/kernel/fadump.c index 8ff0dd4..b8e15cf 100644 --- a/arch/powerpc/kernel/fadump.c +++ b/arch/powerpc/kernel/fadump.c @@ -906,8 +906,7 @@ static int fadump_create_elfcore_headers(char *bufp) phdr->p_paddr = fadump_relocate(paddr_vmcoreinfo_note()); phdr->p_offset = phdr->p_paddr; - phdr->p_memsz = vmcoreinfo_max_size; - phdr->p_filesz = vmcoreinfo_max_size; + phdr->p_memsz = phdr->p_filesz = VMCOREINFO_NOTE_SIZE; /* Increment number of program headers. */ (elf->e_phnum)++; diff --git a/include/linux/kexec.h b/include/linux/kexec.h index f1c601b..6918fda 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -319,7 +319,6 @@ extern void *kexec_purgatory_get_symbol_addr(struct kimage *image, extern note_buf_t __percpu *crash_notes; extern u32 *vmcoreinfo_note; extern size_t vmcoreinfo_size; -extern size_t vmcoreinfo_max_size; /* flag to track if kexec reboot is in progress */ extern bool kexec_in_progress; diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c index e3a4bda..e503b48 100644 --- a/kernel/kexec_core.c +++ b/kernel/kexec_core.c @@ -54,7 +54,6 @@ /* vmcoreinfo stuff */ static unsigned char *vmcoreinfo_data; size_t vmcoreinfo_size; -size_t vmcoreinfo_max_size = VMCOREINFO_BYTES; u32 *vmcoreinfo_note; /* Flag to indicate we are going to kexec a new kernel */ @@ -1386,7 +1385,7 @@ void vmcoreinfo_append_str(const char *fmt, ...) r = vscnprintf(buf, sizeof(buf), fmt, args); va_end(args); - r = min(r, vmcoreinfo_max_size - vmcoreinfo_size); + r = min(r, VMCOREINFO_BYTES - vmcoreinfo_size); memcpy(_data[vmcoreinfo_size], buf, r); -- 1.8.3.1
[PATCH v3 3/3] kdump: Relocate vmcoreinfo to the crash memory range
Currently vmcoreinfo data is updated at boot time subsys_initcall(), it has the risk of being modified by some wrong code during system is running. As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, when using "crash", "makedumpfile", etc utility to parse this vmcore, we probably will get "Segmentation fault" or other unexpected errors. E.g. 1) wrong code overwrites vmcoreinfo_data; 2) further crashes the system; 3) trigger kdump, then we obviously will fail to recognize the crash context correctly due to the corrupted vmcoreinfo. Now except for vmcoreinfo, all the crash data is well protected(including the cpu note which is fully updated in the crash path, thus its correctness is guaranteed). Given that vmcoreinfo data is a large chunk, we better protect it as well. To solve this, we relocate and copy vmcoreinfo_data to the crash memory when kdump is loading via kexec syscalls. Because the whole crash memory will be protected by existing arch_kexec_protect_crashkres() mechanism, we naturally protect vmcoreinfo_data from write(even read) access under kernel direct mapping after kdump is loaded. Since kdump is usually loaded at the very early stage after boot, we can trust the correctness of the vmcoreinfo data copied. On the other hand, we still need to operate the vmcoreinfo safe copy when crash happens to generate vmcoreinfo_note again, we rely on vmap() to map out a new kernel virtual address and update to use this new one instead in the following crash_save_vmcoreinfo(). BTW, we do not touch vmcoreinfo_note, because it will be fully updated using the protected vmcoreinfo_data after crash which is surely correct just like the cpu crash note. Signed-off-by: Xunlei Pang--- include/linux/kexec.h | 3 +++ kernel/kexec.c| 3 +++ kernel/kexec_core.c | 52 +++ kernel/kexec_file.c | 3 +++ 4 files changed, 61 insertions(+) diff --git a/include/linux/kexec.h b/include/linux/kexec.h index 6918fda..fae2fc6 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -187,6 +187,8 @@ struct kimage { unsigned long start; struct page *control_code_page; struct page *swap_page; + void *vmcoreinfo_data_copy; /* locates in the crash memory */ + size_t vmcoreinfo_size_copy; unsigned long nr_segments; struct kexec_segment segment[KEXEC_SEGMENT_MAX]; @@ -243,6 +245,7 @@ extern asmlinkage long sys_kexec_load(unsigned long entry, extern int kernel_kexec(void); extern struct page *kimage_alloc_control_pages(struct kimage *image, unsigned int order); +extern int kimage_crash_copy_vmcoreinfo(struct kimage *image); extern int kexec_load_purgatory(struct kimage *image, unsigned long min, unsigned long max, int top_down, unsigned long *load_addr); diff --git a/kernel/kexec.c b/kernel/kexec.c index 980936a..e0c4dea 100644 --- a/kernel/kexec.c +++ b/kernel/kexec.c @@ -93,6 +93,9 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, pr_err("Could not allocate swap buffer\n"); goto out_free_control_pages; } + } else { + if (kimage_crash_copy_vmcoreinfo(image) < 0) + goto out_free_image; } *rimage = image; diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c index e503b48..7fad9f6 100644 --- a/kernel/kexec_core.c +++ b/kernel/kexec_core.c @@ -486,6 +486,45 @@ struct page *kimage_alloc_control_pages(struct kimage *image, return pages; } +int kimage_crash_copy_vmcoreinfo(struct kimage *image) +{ + struct page *vmcoreinfo_page; + void *safecopy; + + WARN_ON(image->type != KEXEC_TYPE_CRASH); + + if (!vmcoreinfo_size) { + pr_err("empty vmcoreinfo data\n"); + return -ENOMEM; + } + + /* +* For kdump, allocate one vmcoreinfo safe copy from the +* crash memory. as we have arch_kexec_protect_crashkres() +* after kexec syscall, we naturally protect it from write +* (even read) access under kernel direct mapping. But on +* the other hand, we still need to operate it when crash +* happens to generate vmcoreinfo note, hereby we rely on +* vmap for this purpose. +*/ + vmcoreinfo_page = kimage_alloc_control_pages(image, 0); + if (!vmcoreinfo_page) { + pr_err("could not allocate vmcoreinfo buffer\n"); + return -ENOMEM; + } + safecopy = vmap(_page, 1, VM_MAP, PAGE_KERNEL); + if (!safecopy) { + pr_err("cound not vmap vmcoreinfo buffer\n"); + return -ENOMEM; + } + + memcpy(safecopy, vmcoreinfo_data, vmcoreinfo_size); + image->vmcoreinfo_data_copy = safecopy; +
[PATCH v3 3/3] kdump: Relocate vmcoreinfo to the crash memory range
Currently vmcoreinfo data is updated at boot time subsys_initcall(), it has the risk of being modified by some wrong code during system is running. As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, when using "crash", "makedumpfile", etc utility to parse this vmcore, we probably will get "Segmentation fault" or other unexpected errors. E.g. 1) wrong code overwrites vmcoreinfo_data; 2) further crashes the system; 3) trigger kdump, then we obviously will fail to recognize the crash context correctly due to the corrupted vmcoreinfo. Now except for vmcoreinfo, all the crash data is well protected(including the cpu note which is fully updated in the crash path, thus its correctness is guaranteed). Given that vmcoreinfo data is a large chunk, we better protect it as well. To solve this, we relocate and copy vmcoreinfo_data to the crash memory when kdump is loading via kexec syscalls. Because the whole crash memory will be protected by existing arch_kexec_protect_crashkres() mechanism, we naturally protect vmcoreinfo_data from write(even read) access under kernel direct mapping after kdump is loaded. Since kdump is usually loaded at the very early stage after boot, we can trust the correctness of the vmcoreinfo data copied. On the other hand, we still need to operate the vmcoreinfo safe copy when crash happens to generate vmcoreinfo_note again, we rely on vmap() to map out a new kernel virtual address and update to use this new one instead in the following crash_save_vmcoreinfo(). BTW, we do not touch vmcoreinfo_note, because it will be fully updated using the protected vmcoreinfo_data after crash which is surely correct just like the cpu crash note. Signed-off-by: Xunlei Pang --- include/linux/kexec.h | 3 +++ kernel/kexec.c| 3 +++ kernel/kexec_core.c | 52 +++ kernel/kexec_file.c | 3 +++ 4 files changed, 61 insertions(+) diff --git a/include/linux/kexec.h b/include/linux/kexec.h index 6918fda..fae2fc6 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -187,6 +187,8 @@ struct kimage { unsigned long start; struct page *control_code_page; struct page *swap_page; + void *vmcoreinfo_data_copy; /* locates in the crash memory */ + size_t vmcoreinfo_size_copy; unsigned long nr_segments; struct kexec_segment segment[KEXEC_SEGMENT_MAX]; @@ -243,6 +245,7 @@ extern asmlinkage long sys_kexec_load(unsigned long entry, extern int kernel_kexec(void); extern struct page *kimage_alloc_control_pages(struct kimage *image, unsigned int order); +extern int kimage_crash_copy_vmcoreinfo(struct kimage *image); extern int kexec_load_purgatory(struct kimage *image, unsigned long min, unsigned long max, int top_down, unsigned long *load_addr); diff --git a/kernel/kexec.c b/kernel/kexec.c index 980936a..e0c4dea 100644 --- a/kernel/kexec.c +++ b/kernel/kexec.c @@ -93,6 +93,9 @@ static int kimage_alloc_init(struct kimage **rimage, unsigned long entry, pr_err("Could not allocate swap buffer\n"); goto out_free_control_pages; } + } else { + if (kimage_crash_copy_vmcoreinfo(image) < 0) + goto out_free_image; } *rimage = image; diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c index e503b48..7fad9f6 100644 --- a/kernel/kexec_core.c +++ b/kernel/kexec_core.c @@ -486,6 +486,45 @@ struct page *kimage_alloc_control_pages(struct kimage *image, return pages; } +int kimage_crash_copy_vmcoreinfo(struct kimage *image) +{ + struct page *vmcoreinfo_page; + void *safecopy; + + WARN_ON(image->type != KEXEC_TYPE_CRASH); + + if (!vmcoreinfo_size) { + pr_err("empty vmcoreinfo data\n"); + return -ENOMEM; + } + + /* +* For kdump, allocate one vmcoreinfo safe copy from the +* crash memory. as we have arch_kexec_protect_crashkres() +* after kexec syscall, we naturally protect it from write +* (even read) access under kernel direct mapping. But on +* the other hand, we still need to operate it when crash +* happens to generate vmcoreinfo note, hereby we rely on +* vmap for this purpose. +*/ + vmcoreinfo_page = kimage_alloc_control_pages(image, 0); + if (!vmcoreinfo_page) { + pr_err("could not allocate vmcoreinfo buffer\n"); + return -ENOMEM; + } + safecopy = vmap(_page, 1, VM_MAP, PAGE_KERNEL); + if (!safecopy) { + pr_err("cound not vmap vmcoreinfo buffer\n"); + return -ENOMEM; + } + + memcpy(safecopy, vmcoreinfo_data, vmcoreinfo_size); + image->vmcoreinfo_data_copy = safecopy; + image->vmcoreinfo_size_copy =
[PATCH v3 1/3] kexec: Move vmcoreinfo out of the kernel's .bss section
As Eric said, "what we need to do is move the variable vmcoreinfo_note out of the kernel's .bss section. And modify the code to regenerate and keep this information in something like the control page. Definitely something like this needs a page all to itself, and ideally far away from any other kernel data structures. I clearly was not watching closely the data someone decided to keep this silly thing in the kernel's .bss section." This patch allocates extra pages for these vmcoreinfo_XXX variables, one advantage is that it enhances some safety of vmcoreinfo, because vmcoreinfo now is kept far away from other kernel data structures. Suggested-by: Eric BiedermanSigned-off-by: Xunlei Pang --- arch/ia64/kernel/machine_kexec.c | 5 - arch/x86/kernel/crash.c | 2 +- include/linux/kexec.h| 2 +- kernel/kexec_core.c | 29 - kernel/ksysfs.c | 2 +- 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/arch/ia64/kernel/machine_kexec.c b/arch/ia64/kernel/machine_kexec.c index 599507b..c14815d 100644 --- a/arch/ia64/kernel/machine_kexec.c +++ b/arch/ia64/kernel/machine_kexec.c @@ -163,8 +163,3 @@ void arch_crash_save_vmcoreinfo(void) #endif } -phys_addr_t paddr_vmcoreinfo_note(void) -{ - return ia64_tpa((unsigned long)(char *)_note); -} - diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c index 3741461..4d35fbb 100644 --- a/arch/x86/kernel/crash.c +++ b/arch/x86/kernel/crash.c @@ -456,7 +456,7 @@ static int prepare_elf64_headers(struct crash_elf_data *ced, bufp += sizeof(Elf64_Phdr); phdr->p_type = PT_NOTE; phdr->p_offset = phdr->p_paddr = paddr_vmcoreinfo_note(); - phdr->p_filesz = phdr->p_memsz = sizeof(vmcoreinfo_note); + phdr->p_filesz = phdr->p_memsz = VMCOREINFO_NOTE_SIZE; (ehdr->e_phnum)++; #ifdef CONFIG_X86_64 diff --git a/include/linux/kexec.h b/include/linux/kexec.h index e98e546..f1c601b 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -317,7 +317,7 @@ extern void *kexec_purgatory_get_symbol_addr(struct kimage *image, extern struct resource crashk_low_res; typedef u32 note_buf_t[KEXEC_NOTE_BYTES/4]; extern note_buf_t __percpu *crash_notes; -extern u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; +extern u32 *vmcoreinfo_note; extern size_t vmcoreinfo_size; extern size_t vmcoreinfo_max_size; diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c index bfe62d5..e3a4bda 100644 --- a/kernel/kexec_core.c +++ b/kernel/kexec_core.c @@ -52,10 +52,10 @@ note_buf_t __percpu *crash_notes; /* vmcoreinfo stuff */ -static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; +static unsigned char *vmcoreinfo_data; size_t vmcoreinfo_size; -size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); +size_t vmcoreinfo_max_size = VMCOREINFO_BYTES; +u32 *vmcoreinfo_note; /* Flag to indicate we are going to kexec a new kernel */ bool kexec_in_progress = false; @@ -1369,6 +1369,9 @@ static void update_vmcoreinfo_note(void) void crash_save_vmcoreinfo(void) { + if (!vmcoreinfo_note) + return; + vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds()); update_vmcoreinfo_note(); } @@ -1397,13 +1400,29 @@ void vmcoreinfo_append_str(const char *fmt, ...) void __weak arch_crash_save_vmcoreinfo(void) {} -phys_addr_t __weak paddr_vmcoreinfo_note(void) +phys_addr_t paddr_vmcoreinfo_note(void) { - return __pa_symbol((unsigned long)(char *)_note); + return __pa(vmcoreinfo_note); } static int __init crash_save_vmcoreinfo_init(void) { + /* One page should be enough for VMCOREINFO_BYTES under all archs */ + vmcoreinfo_data = (unsigned char *)get_zeroed_page(GFP_KERNEL); + if (!vmcoreinfo_data) { + pr_warn("Memory allocation for vmcoreinfo_data failed\n"); + return -ENOMEM; + } + + vmcoreinfo_note = alloc_pages_exact(VMCOREINFO_NOTE_SIZE, + GFP_KERNEL | __GFP_ZERO); + if (!vmcoreinfo_note) { + free_page((unsigned long)vmcoreinfo_data); + vmcoreinfo_data = NULL; + pr_warn("Memory allocation for vmcoreinfo_note failed\n"); + return -ENOMEM; + } + VMCOREINFO_OSRELEASE(init_uts_ns.name.release); VMCOREINFO_PAGESIZE(PAGE_SIZE); diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c index ee1bc1b..9de6fcc 100644 --- a/kernel/ksysfs.c +++ b/kernel/ksysfs.c @@ -130,7 +130,7 @@ static ssize_t vmcoreinfo_show(struct kobject *kobj, { phys_addr_t vmcore_base = paddr_vmcoreinfo_note(); return sprintf(buf, "%pa %x\n", _base, - (unsigned int)sizeof(vmcoreinfo_note)); + (unsigned int)VMCOREINFO_NOTE_SIZE); } KERNEL_ATTR_RO(vmcoreinfo); -- 1.8.3.1
[PATCH v3 1/3] kexec: Move vmcoreinfo out of the kernel's .bss section
As Eric said, "what we need to do is move the variable vmcoreinfo_note out of the kernel's .bss section. And modify the code to regenerate and keep this information in something like the control page. Definitely something like this needs a page all to itself, and ideally far away from any other kernel data structures. I clearly was not watching closely the data someone decided to keep this silly thing in the kernel's .bss section." This patch allocates extra pages for these vmcoreinfo_XXX variables, one advantage is that it enhances some safety of vmcoreinfo, because vmcoreinfo now is kept far away from other kernel data structures. Suggested-by: Eric Biederman Signed-off-by: Xunlei Pang --- arch/ia64/kernel/machine_kexec.c | 5 - arch/x86/kernel/crash.c | 2 +- include/linux/kexec.h| 2 +- kernel/kexec_core.c | 29 - kernel/ksysfs.c | 2 +- 5 files changed, 27 insertions(+), 13 deletions(-) diff --git a/arch/ia64/kernel/machine_kexec.c b/arch/ia64/kernel/machine_kexec.c index 599507b..c14815d 100644 --- a/arch/ia64/kernel/machine_kexec.c +++ b/arch/ia64/kernel/machine_kexec.c @@ -163,8 +163,3 @@ void arch_crash_save_vmcoreinfo(void) #endif } -phys_addr_t paddr_vmcoreinfo_note(void) -{ - return ia64_tpa((unsigned long)(char *)_note); -} - diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c index 3741461..4d35fbb 100644 --- a/arch/x86/kernel/crash.c +++ b/arch/x86/kernel/crash.c @@ -456,7 +456,7 @@ static int prepare_elf64_headers(struct crash_elf_data *ced, bufp += sizeof(Elf64_Phdr); phdr->p_type = PT_NOTE; phdr->p_offset = phdr->p_paddr = paddr_vmcoreinfo_note(); - phdr->p_filesz = phdr->p_memsz = sizeof(vmcoreinfo_note); + phdr->p_filesz = phdr->p_memsz = VMCOREINFO_NOTE_SIZE; (ehdr->e_phnum)++; #ifdef CONFIG_X86_64 diff --git a/include/linux/kexec.h b/include/linux/kexec.h index e98e546..f1c601b 100644 --- a/include/linux/kexec.h +++ b/include/linux/kexec.h @@ -317,7 +317,7 @@ extern void *kexec_purgatory_get_symbol_addr(struct kimage *image, extern struct resource crashk_low_res; typedef u32 note_buf_t[KEXEC_NOTE_BYTES/4]; extern note_buf_t __percpu *crash_notes; -extern u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; +extern u32 *vmcoreinfo_note; extern size_t vmcoreinfo_size; extern size_t vmcoreinfo_max_size; diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c index bfe62d5..e3a4bda 100644 --- a/kernel/kexec_core.c +++ b/kernel/kexec_core.c @@ -52,10 +52,10 @@ note_buf_t __percpu *crash_notes; /* vmcoreinfo stuff */ -static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; +static unsigned char *vmcoreinfo_data; size_t vmcoreinfo_size; -size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); +size_t vmcoreinfo_max_size = VMCOREINFO_BYTES; +u32 *vmcoreinfo_note; /* Flag to indicate we are going to kexec a new kernel */ bool kexec_in_progress = false; @@ -1369,6 +1369,9 @@ static void update_vmcoreinfo_note(void) void crash_save_vmcoreinfo(void) { + if (!vmcoreinfo_note) + return; + vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds()); update_vmcoreinfo_note(); } @@ -1397,13 +1400,29 @@ void vmcoreinfo_append_str(const char *fmt, ...) void __weak arch_crash_save_vmcoreinfo(void) {} -phys_addr_t __weak paddr_vmcoreinfo_note(void) +phys_addr_t paddr_vmcoreinfo_note(void) { - return __pa_symbol((unsigned long)(char *)_note); + return __pa(vmcoreinfo_note); } static int __init crash_save_vmcoreinfo_init(void) { + /* One page should be enough for VMCOREINFO_BYTES under all archs */ + vmcoreinfo_data = (unsigned char *)get_zeroed_page(GFP_KERNEL); + if (!vmcoreinfo_data) { + pr_warn("Memory allocation for vmcoreinfo_data failed\n"); + return -ENOMEM; + } + + vmcoreinfo_note = alloc_pages_exact(VMCOREINFO_NOTE_SIZE, + GFP_KERNEL | __GFP_ZERO); + if (!vmcoreinfo_note) { + free_page((unsigned long)vmcoreinfo_data); + vmcoreinfo_data = NULL; + pr_warn("Memory allocation for vmcoreinfo_note failed\n"); + return -ENOMEM; + } + VMCOREINFO_OSRELEASE(init_uts_ns.name.release); VMCOREINFO_PAGESIZE(PAGE_SIZE); diff --git a/kernel/ksysfs.c b/kernel/ksysfs.c index ee1bc1b..9de6fcc 100644 --- a/kernel/ksysfs.c +++ b/kernel/ksysfs.c @@ -130,7 +130,7 @@ static ssize_t vmcoreinfo_show(struct kobject *kobj, { phys_addr_t vmcore_base = paddr_vmcoreinfo_note(); return sprintf(buf, "%pa %x\n", _base, - (unsigned int)sizeof(vmcoreinfo_note)); + (unsigned int)VMCOREINFO_NOTE_SIZE); } KERNEL_ATTR_RO(vmcoreinfo); -- 1.8.3.1
linux-next: build failure after merge of the akpm tree
Hi Andrew, After merging the akpm tree, today's linux-next build (x86_64 allmodconfig) failed like this: fs/f2fs/node.c: In function 'init_free_nid_cache': fs/f2fs/node.c:2634:25: error: implicit declaration of function 'f2fs_kvzalloc' [-Werror=implicit-function-declaration] nm_i->free_nid_count = f2fs_kvzalloc(nm_i->nat_blocks * ^ fs/f2fs/node.c:2634:23: warning: assignment makes pointer from integer without a cast [-Wint-conversion] nm_i->free_nid_count = f2fs_kvzalloc(nm_i->nat_blocks * ^ Caused by commit 69e5e80117a3 ("mm: introduce kv[mz]alloc helpers") interacting with commit c48b15867a2f ("f2fs: skip scanning free nid bitmap of full NAT blocks") from the f2fs tree. I applied the following fix patch: From: Stephen RothwellDate: Mon, 20 Mar 2017 16:28:21 +1100 Subject: [PATCH] mm: introduce kv[mz]alloc helpers - f2fs fix up Signed-off-by: Stephen Rothwell --- fs/f2fs/node.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index cdbb05745895..0ea1dca8a0e2 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -2631,7 +2631,7 @@ static int init_free_nid_cache(struct f2fs_sb_info *sbi) if (!nm_i->nat_block_bitmap) return -ENOMEM; - nm_i->free_nid_count = f2fs_kvzalloc(nm_i->nat_blocks * + nm_i->free_nid_count = kvzalloc(nm_i->nat_blocks * sizeof(unsigned short), GFP_KERNEL); if (!nm_i->free_nid_count) return -ENOMEM; -- 2.11.0 -- Cheers, Stephen Rothwell
linux-next: build failure after merge of the akpm tree
Hi Andrew, After merging the akpm tree, today's linux-next build (x86_64 allmodconfig) failed like this: fs/f2fs/node.c: In function 'init_free_nid_cache': fs/f2fs/node.c:2634:25: error: implicit declaration of function 'f2fs_kvzalloc' [-Werror=implicit-function-declaration] nm_i->free_nid_count = f2fs_kvzalloc(nm_i->nat_blocks * ^ fs/f2fs/node.c:2634:23: warning: assignment makes pointer from integer without a cast [-Wint-conversion] nm_i->free_nid_count = f2fs_kvzalloc(nm_i->nat_blocks * ^ Caused by commit 69e5e80117a3 ("mm: introduce kv[mz]alloc helpers") interacting with commit c48b15867a2f ("f2fs: skip scanning free nid bitmap of full NAT blocks") from the f2fs tree. I applied the following fix patch: From: Stephen Rothwell Date: Mon, 20 Mar 2017 16:28:21 +1100 Subject: [PATCH] mm: introduce kv[mz]alloc helpers - f2fs fix up Signed-off-by: Stephen Rothwell --- fs/f2fs/node.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index cdbb05745895..0ea1dca8a0e2 100644 --- a/fs/f2fs/node.c +++ b/fs/f2fs/node.c @@ -2631,7 +2631,7 @@ static int init_free_nid_cache(struct f2fs_sb_info *sbi) if (!nm_i->nat_block_bitmap) return -ENOMEM; - nm_i->free_nid_count = f2fs_kvzalloc(nm_i->nat_blocks * + nm_i->free_nid_count = kvzalloc(nm_i->nat_blocks * sizeof(unsigned short), GFP_KERNEL); if (!nm_i->free_nid_count) return -ENOMEM; -- 2.11.0 -- Cheers, Stephen Rothwell
[PATCH v2] xfs: Honor FALLOC_FL_KEEP_SIZE when punching ends of files
When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will round the file size up to the nearest multiple of PAGE_SIZE: calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1 calvinow@vm-disks/generic-xfs-1 ~$ stat test Size: 2048Blocks: 8 IO Block: 4096 regular file calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test calvinow@vm-disks/generic-xfs-1 ~$ stat test Size: 4096Blocks: 8 IO Block: 4096 regular file Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers don't enforce that [pos,offset) lies strictly on [0,i_size) when being called from xfs_free_file_space(), so by "leaking" these ranges into xfs_zero_range() we get this buggy behavior. Fix this by reintroducing the checks xfs_zero_remaining_bytes() did against i_size at the bottom of xfs_free_file_space(). Reported-by: Aaron GaoFixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") Cc: Christoph Hellwig Cc: # 4.8+ Signed-off-by: Calvin Owens --- fs/xfs/xfs_bmap_util.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index 8b75dce..0796ebc 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -1309,6 +1309,17 @@ xfs_free_file_space( } /* +* Avoid doing I/O beyond eof - it's not necessary +* since nothing can read beyond eof. The space will +* be zeroed when the file is extended anyway. +*/ + if (offset >= XFS_ISIZE(ip)) + return 0; + + if ((offset + len) >= XFS_ISIZE(ip)) + len = XFS_ISIZE(ip) - offset - 1; + + /* * Now that we've unmap all full blocks we'll have to zero out any * partial block at the beginning and/or end. xfs_zero_range is * smart enough to skip any holes, including those we just created. -- 2.9.3
[PATCH v2] xfs: Honor FALLOC_FL_KEEP_SIZE when punching ends of files
When punching past EOF on XFS, fallocate(mode=PUNCH_HOLE|KEEP_SIZE) will round the file size up to the nearest multiple of PAGE_SIZE: calvinow@vm-disks/generic-xfs-1 ~$ dd if=/dev/urandom of=test bs=2048 count=1 calvinow@vm-disks/generic-xfs-1 ~$ stat test Size: 2048Blocks: 8 IO Block: 4096 regular file calvinow@vm-disks/generic-xfs-1 ~$ fallocate -n -l 2048 -o 2048 -p test calvinow@vm-disks/generic-xfs-1 ~$ stat test Size: 4096Blocks: 8 IO Block: 4096 regular file Commit 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") replaced xfs_zero_remaining_bytes() with calls to iomap helpers. The new helpers don't enforce that [pos,offset) lies strictly on [0,i_size) when being called from xfs_free_file_space(), so by "leaking" these ranges into xfs_zero_range() we get this buggy behavior. Fix this by reintroducing the checks xfs_zero_remaining_bytes() did against i_size at the bottom of xfs_free_file_space(). Reported-by: Aaron Gao Fixes: 3c2bdc912a1cc050 ("xfs: kill xfs_zero_remaining_bytes") Cc: Christoph Hellwig Cc: # 4.8+ Signed-off-by: Calvin Owens --- fs/xfs/xfs_bmap_util.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c index 8b75dce..0796ebc 100644 --- a/fs/xfs/xfs_bmap_util.c +++ b/fs/xfs/xfs_bmap_util.c @@ -1309,6 +1309,17 @@ xfs_free_file_space( } /* +* Avoid doing I/O beyond eof - it's not necessary +* since nothing can read beyond eof. The space will +* be zeroed when the file is extended anyway. +*/ + if (offset >= XFS_ISIZE(ip)) + return 0; + + if ((offset + len) >= XFS_ISIZE(ip)) + len = XFS_ISIZE(ip) - offset - 1; + + /* * Now that we've unmap all full blocks we'll have to zero out any * partial block at the beginning and/or end. xfs_zero_range is * smart enough to skip any holes, including those we just created. -- 2.9.3
Re: [Patch v2 02/11] s5p-mfc: Adding initial support for MFC v10.10
On Wed, 2017-03-15 at 14:52 -0500, Rob Herring wrote: > On Fri, Mar 03, 2017 at 02:37:07PM +0530, Smitha T Murthy wrote: > > Adding the support for MFC v10.10, with new register file and > > necessary hw control, decoder, encoder and structural changes. > > > > Signed-off-by: Smitha T Murthy> > CC: Rob Herring > > CC: devicet...@vger.kernel.org > > --- > > .../devicetree/bindings/media/s5p-mfc.txt |1 + > > Acked-by: Rob Herring > Thank you for the Acked-by. Regards, Smitha T Murthy > > drivers/media/platform/s5p-mfc/regs-mfc-v10.h | 36 > > drivers/media/platform/s5p-mfc/s5p_mfc.c | 30 + > > drivers/media/platform/s5p-mfc/s5p_mfc_common.h|4 +- > > drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c |4 ++ > > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 44 > > +++- > > drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 21 + > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c|9 +++- > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h|2 + > > 9 files changed, 118 insertions(+), 33 deletions(-) > > create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v10.h > >
Re: [Patch v2 02/11] s5p-mfc: Adding initial support for MFC v10.10
On Wed, 2017-03-15 at 14:52 -0500, Rob Herring wrote: > On Fri, Mar 03, 2017 at 02:37:07PM +0530, Smitha T Murthy wrote: > > Adding the support for MFC v10.10, with new register file and > > necessary hw control, decoder, encoder and structural changes. > > > > Signed-off-by: Smitha T Murthy > > CC: Rob Herring > > CC: devicet...@vger.kernel.org > > --- > > .../devicetree/bindings/media/s5p-mfc.txt |1 + > > Acked-by: Rob Herring > Thank you for the Acked-by. Regards, Smitha T Murthy > > drivers/media/platform/s5p-mfc/regs-mfc-v10.h | 36 > > drivers/media/platform/s5p-mfc/s5p_mfc.c | 30 + > > drivers/media/platform/s5p-mfc/s5p_mfc_common.h|4 +- > > drivers/media/platform/s5p-mfc/s5p_mfc_ctrl.c |4 ++ > > drivers/media/platform/s5p-mfc/s5p_mfc_dec.c | 44 > > +++- > > drivers/media/platform/s5p-mfc/s5p_mfc_enc.c | 21 + > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c|9 +++- > > drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.h|2 + > > 9 files changed, 118 insertions(+), 33 deletions(-) > > create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v10.h > >
linux-next: build warnings after merge of the akpm-current tree
Hi Andrew, After merging the akpm-current tree, today's linux-next build (x86_64 allmodconfig) produced these warnings: drivers/crypto/cavium/zip/zip_main.c: In function 'zip_show_stats': drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'long long int' [-Wformat=] seq_printf(s, "ZIP Device %d Stats\n" ^ drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 6 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 7 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 10 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 11 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 12 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 13 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 14 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 15 has type 'long long int' [-Wformat=] Introduced by commit 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics") from the crypto tree interacting with commit 3f4ca3d25e1a ("asm-generic, x86: wrap atomic operations") from the akpm-current tree. This latter commit changed atomic64read() from "long" to "long long" on x86_64. -- Cheers, Stephen Rothwell
linux-next: build warnings after merge of the akpm-current tree
Hi Andrew, After merging the akpm-current tree, today's linux-next build (x86_64 allmodconfig) produced these warnings: drivers/crypto/cavium/zip/zip_main.c: In function 'zip_show_stats': drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 4 has type 'long long int' [-Wformat=] seq_printf(s, "ZIP Device %d Stats\n" ^ drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 5 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 6 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 7 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 10 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 11 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 12 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 13 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 14 has type 'long long int' [-Wformat=] drivers/crypto/cavium/zip/zip_main.c:489:18: warning: format '%ld' expects argument of type 'long int', but argument 15 has type 'long long int' [-Wformat=] Introduced by commit 09ae5d37e093 ("crypto: zip - Add Compression/Decompression statistics") from the crypto tree interacting with commit 3f4ca3d25e1a ("asm-generic, x86: wrap atomic operations") from the akpm-current tree. This latter commit changed atomic64read() from "long" to "long long" on x86_64. -- Cheers, Stephen Rothwell
Re: [PATCH 09/15] power: supply: bq24190_charger: Add voltage_max_design prop to battery
Hi, On Fri, Mar 17, 2017 at 10:55:21AM +0100, Hans de Goede wrote: > When combined with an external fuel-gauge, upower needs voltage_max_design > as it internally does all its calculations in Watts and converts the > charge_foo properties from A to Watts by using voltage_max_design. > > Signed-off-by: Hans de GoedeAssuming, that my comment on patch 14 works POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN is required in the fuel-gauge driver instead. As far as I understand it the charger voltage was too high for your device anyways, so that you capped it at 4.3V. So I assume, that you can just provide it via device properties. -- Sebastian > drivers/power/supply/bq24190_charger.c | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/supply/bq24190_charger.c > b/drivers/power/supply/bq24190_charger.c > index 9fe69a5..82cb33d 100644 > --- a/drivers/power/supply/bq24190_charger.c > +++ b/drivers/power/supply/bq24190_charger.c > @@ -1103,6 +1103,13 @@ static int bq24190_battery_get_property(struct > power_supply *psy, > val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN; > ret = 0; > break; > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + /* > + * Report charger configured voltage as max design voltage, > + * not entirely correct, but userspace needs something here. > + */ > + ret = bq24190_charger_get_voltage(bdi, val); > + break; > case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > ret = bq24190_battery_get_temp_alert_max(bdi, val); > break; > @@ -1169,6 +1176,7 @@ static enum power_supply_property > bq24190_battery_properties[] = { > POWER_SUPPLY_PROP_HEALTH, > POWER_SUPPLY_PROP_ONLINE, > POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > POWER_SUPPLY_PROP_TEMP_ALERT_MAX, > POWER_SUPPLY_PROP_SCOPE, > /* Begin of extended battery properties */ > @@ -1186,7 +1194,7 @@ static const struct power_supply_desc > bq24190_battery_desc = { > .name = "bq24190-battery", > .type = POWER_SUPPLY_TYPE_BATTERY, > .properties = bq24190_battery_properties, > - .num_properties = 6, > + .num_properties = 7, > .get_property = bq24190_battery_get_property, > .set_property = bq24190_battery_set_property, > .property_is_writeable = bq24190_battery_property_is_writeable, > -- > 2.9.3 > signature.asc Description: PGP signature
Re: [PATCH 09/15] power: supply: bq24190_charger: Add voltage_max_design prop to battery
Hi, On Fri, Mar 17, 2017 at 10:55:21AM +0100, Hans de Goede wrote: > When combined with an external fuel-gauge, upower needs voltage_max_design > as it internally does all its calculations in Watts and converts the > charge_foo properties from A to Watts by using voltage_max_design. > > Signed-off-by: Hans de Goede Assuming, that my comment on patch 14 works POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN is required in the fuel-gauge driver instead. As far as I understand it the charger voltage was too high for your device anyways, so that you capped it at 4.3V. So I assume, that you can just provide it via device properties. -- Sebastian > drivers/power/supply/bq24190_charger.c | 10 +- > 1 file changed, 9 insertions(+), 1 deletion(-) > > diff --git a/drivers/power/supply/bq24190_charger.c > b/drivers/power/supply/bq24190_charger.c > index 9fe69a5..82cb33d 100644 > --- a/drivers/power/supply/bq24190_charger.c > +++ b/drivers/power/supply/bq24190_charger.c > @@ -1103,6 +1103,13 @@ static int bq24190_battery_get_property(struct > power_supply *psy, > val->intval = POWER_SUPPLY_TECHNOLOGY_UNKNOWN; > ret = 0; > break; > + case POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN: > + /* > + * Report charger configured voltage as max design voltage, > + * not entirely correct, but userspace needs something here. > + */ > + ret = bq24190_charger_get_voltage(bdi, val); > + break; > case POWER_SUPPLY_PROP_TEMP_ALERT_MAX: > ret = bq24190_battery_get_temp_alert_max(bdi, val); > break; > @@ -1169,6 +1176,7 @@ static enum power_supply_property > bq24190_battery_properties[] = { > POWER_SUPPLY_PROP_HEALTH, > POWER_SUPPLY_PROP_ONLINE, > POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > POWER_SUPPLY_PROP_TEMP_ALERT_MAX, > POWER_SUPPLY_PROP_SCOPE, > /* Begin of extended battery properties */ > @@ -1186,7 +1194,7 @@ static const struct power_supply_desc > bq24190_battery_desc = { > .name = "bq24190-battery", > .type = POWER_SUPPLY_TYPE_BATTERY, > .properties = bq24190_battery_properties, > - .num_properties = 6, > + .num_properties = 7, > .get_property = bq24190_battery_get_property, > .set_property = bq24190_battery_set_property, > .property_is_writeable = bq24190_battery_property_is_writeable, > -- > 2.9.3 > signature.asc Description: PGP signature
linux-next: manual merge of the akpm tree with the net-next tree
Hi Andrew, Today's linux-next merge of the akpm tree got a conflict in: Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt between commit: 0ce5aa1d6c97 ("dt-bindings: net: update bcmgenet binding for GENETv5") from the net-next tree and patch: "scripts/spelling.txt: Add regsiter -> register spelling mistake" from the akpm tree. I fixed it up (the former also fixed the typo from the latter, so I just used that) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell
linux-next: manual merge of the akpm tree with the net-next tree
Hi Andrew, Today's linux-next merge of the akpm tree got a conflict in: Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt between commit: 0ce5aa1d6c97 ("dt-bindings: net: update bcmgenet binding for GENETv5") from the net-next tree and patch: "scripts/spelling.txt: Add regsiter -> register spelling mistake" from the akpm tree. I fixed it up (the former also fixed the typo from the latter, so I just used that) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell
Re: [PATCH 26/26] x86/mm: allow to have userspace mappings above 47-bits
"Kirill A. Shutemov"writes: @@ -168,6 +182,10 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0, > unsigned long addr = addr0; > struct vm_unmapped_area_info info; > > + addr = mpx_unmapped_area_check(addr, len, flags); > + if (IS_ERR_VALUE(addr)) > + return addr; > + > /* requested length too big for entire address space */ > if (len > TASK_SIZE) > return -ENOMEM; > @@ -192,6 +210,14 @@ arch_get_unmapped_area_topdown(struct file *filp, const > unsigned long addr0, > info.length = len; > info.low_limit = PAGE_SIZE; > info.high_limit = mm->mmap_base; > + > + /* > + * If hint address is above DEFAULT_MAP_WINDOW, look for unmapped area > + * in the full address space. > + */ > + if (addr > DEFAULT_MAP_WINDOW) > + info.high_limit += TASK_SIZE - DEFAULT_MAP_WINDOW; > + Is this ok for 32 bit application ? > info.align_mask = 0; > info.align_offset = pgoff << PAGE_SHIFT; > if (filp) { -aneesh
Re: [PATCH 26/26] x86/mm: allow to have userspace mappings above 47-bits
"Kirill A. Shutemov" writes: @@ -168,6 +182,10 @@ arch_get_unmapped_area_topdown(struct file *filp, const unsigned long addr0, > unsigned long addr = addr0; > struct vm_unmapped_area_info info; > > + addr = mpx_unmapped_area_check(addr, len, flags); > + if (IS_ERR_VALUE(addr)) > + return addr; > + > /* requested length too big for entire address space */ > if (len > TASK_SIZE) > return -ENOMEM; > @@ -192,6 +210,14 @@ arch_get_unmapped_area_topdown(struct file *filp, const > unsigned long addr0, > info.length = len; > info.low_limit = PAGE_SIZE; > info.high_limit = mm->mmap_base; > + > + /* > + * If hint address is above DEFAULT_MAP_WINDOW, look for unmapped area > + * in the full address space. > + */ > + if (addr > DEFAULT_MAP_WINDOW) > + info.high_limit += TASK_SIZE - DEFAULT_MAP_WINDOW; > + Is this ok for 32 bit application ? > info.align_mask = 0; > info.align_offset = pgoff << PAGE_SHIFT; > if (filp) { -aneesh
Re: [PATCH v21 00/13] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer
Hi Mark, On 18 March 2017 at 04:03, Mark Rutlandwrote: > On Thu, Mar 09, 2017 at 11:47:16PM +0100, Fu Wei wrote: >> Hi Mark, Marc, > > Hi, > >> I have tried to rebase all the 19(6+13) patches on 4.11-rc1, >> all the patchse can directly apply on 4.11-rc1, >> >> Could you help to review the patches, and see if there is anywhere I >> can improve ? > > I'm sorry I have taken so long to review this posting. > > This is generally looking much better. I'm not sure about a few details > relating to the watchdog, but I'm largely happy to pick up the rest of > the series, if you can address my comments. Great thanks for your review, I will post a new v22 ASAP(before your Tuesday). > > Thanks, > Mark. -- Best regards, Fu Wei Software Engineer Red Hat
Re: [PATCH v21 00/13] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer
Hi Mark, On 18 March 2017 at 04:03, Mark Rutland wrote: > On Thu, Mar 09, 2017 at 11:47:16PM +0100, Fu Wei wrote: >> Hi Mark, Marc, > > Hi, > >> I have tried to rebase all the 19(6+13) patches on 4.11-rc1, >> all the patchse can directly apply on 4.11-rc1, >> >> Could you help to review the patches, and see if there is anywhere I >> can improve ? > > I'm sorry I have taken so long to review this posting. > > This is generally looking much better. I'm not sure about a few details > relating to the watchdog, but I'm largely happy to pick up the rest of > the series, if you can address my comments. Great thanks for your review, I will post a new v22 ASAP(before your Tuesday). > > Thanks, > Mark. -- Best regards, Fu Wei Software Engineer Red Hat
Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge
Hi, On Fri, Mar 17, 2017 at 10:55:26AM +0100, Hans de Goede wrote: > Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge, note > the Cherry Trail Whiskey Cove PMIC Fuel Gauge block is purely a fuel gauge > and not a full battery controller. As such it offers a platform_data > callback for extra power_supply properties for the actual external-charger > ic driver and does not register a power_supply itself. > > Signed-off-by: Hans de GoedeI think this should become a normal battery-type power-supply driver. bq24190_charger driver should only expose a charger-type power-supply device on your system (and probably most others, its very rare, that systems have a programmable charger and no fuel-gauge). -- Sebastian > --- > drivers/power/supply/Kconfig | 9 ++ > drivers/power/supply/Makefile| 1 + > drivers/power/supply/cht_wc_fuel_gauge.c | 209 > +++ > include/linux/power/cht_wc_fuel_gauge.h | 21 > 4 files changed, 240 insertions(+) > create mode 100644 drivers/power/supply/cht_wc_fuel_gauge.c > create mode 100644 include/linux/power/cht_wc_fuel_gauge.h > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index fd93110..34ebfca 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -538,4 +538,13 @@ config AXP20X_POWER > This driver provides support for the power supply features of > AXP20x PMIC. > > +config CHT_WC_FUEL_GAUGE > + tristate "Intel Cherry Trail Whiskey Cove PMIC Fuel Gauge" > + depends on INTEL_SOC_PMIC_CHTWC > + help > + This adds support for the battery fuel gauge found in the Intel > + Cherry Trail Whiskey Cove PMIC. This driver allows monitoring > + of the charge level of the battery on Intel Cherry Trail systems > + with a Whiskey Cove PMIC. > + > endif # POWER_SUPPLY > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index 3789a2c..702e28a 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o > obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o > obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o > obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o > +obj-$(CONFIG_CHT_WC_FUEL_GAUGE) += cht_wc_fuel_gauge.o > diff --git a/drivers/power/supply/cht_wc_fuel_gauge.c > b/drivers/power/supply/cht_wc_fuel_gauge.c > new file mode 100644 > index 000..56f6e5a > --- /dev/null > +++ b/drivers/power/supply/cht_wc_fuel_gauge.c > @@ -0,0 +1,209 @@ > +/* > + * Intel CHT Whiskey Cove Fuel Gauge driver > + * Copyright (C) 2017 Hans de Goede > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Cherrytrail Whiskey Cove devices have 2 functional blocks which interact > + * with the battery. > + * > + * 1) The fuel-gauge which is build into the Whiskey Cove PMIC, but has its > + * own i2c bus and i2c client addresses separately from the rest of the PMIC. > + * That block is what this driver is for. > + * > + * 2) An external charger IC, which is connected to the SMBUS controller > + * which is part of the rest of the Whiskey Cove PMIC, mfd/intel_cht_wc.c > + * registers a platform device for the SMBUS controller and > + * i2c/busses/i2c-cht-wc.c contains the i2c-adapter driver for this. > + * > + * However we want to present this as a single power_supply device to > + * userspace. So this driver offers a callback to get the fuel-gauge > + * power_supply properties, which gets passed to the external charger > + * driver via i2c_board_info when i2c-cht-wc.c calls i2c_new_device(). > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define REG_CHARGE_NOW 0x05 > +#define REG_VOLTAGE_NOW 0x09 > +#define REG_CURRENT_NOW 0x0a > +#define REG_CURRENT_AVG 0x0b > +#define REG_CHARGE_FULL 0x10 > +#define REG_CHARGE_DESIGN0x18 > +#define REG_VOLTAGE_AVG 0x19 > +#define REG_VOLTAGE_OCV 0x1b /* Only updated during charging */ > + > +#define CHT_WC_FG_PTYPE 4 > + > +struct cht_wc_fg_data { > + struct device *dev; > + struct i2c_client *client; > +}; > + > +static DEFINE_MUTEX(cht_wc_fg_mutex); > +static struct cht_wc_fg_data *cht_wc_fg; > + > +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg, > +
Re: [PATCH 14/15] power: supply: Add driver for Cherry Trail Whiskey Cove PMIC Fuel Gauge
Hi, On Fri, Mar 17, 2017 at 10:55:26AM +0100, Hans de Goede wrote: > Add a driver for the Cherry Trail Whiskey Cove PMIC Fuel Gauge, note > the Cherry Trail Whiskey Cove PMIC Fuel Gauge block is purely a fuel gauge > and not a full battery controller. As such it offers a platform_data > callback for extra power_supply properties for the actual external-charger > ic driver and does not register a power_supply itself. > > Signed-off-by: Hans de Goede I think this should become a normal battery-type power-supply driver. bq24190_charger driver should only expose a charger-type power-supply device on your system (and probably most others, its very rare, that systems have a programmable charger and no fuel-gauge). -- Sebastian > --- > drivers/power/supply/Kconfig | 9 ++ > drivers/power/supply/Makefile| 1 + > drivers/power/supply/cht_wc_fuel_gauge.c | 209 > +++ > include/linux/power/cht_wc_fuel_gauge.h | 21 > 4 files changed, 240 insertions(+) > create mode 100644 drivers/power/supply/cht_wc_fuel_gauge.c > create mode 100644 include/linux/power/cht_wc_fuel_gauge.h > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index fd93110..34ebfca 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -538,4 +538,13 @@ config AXP20X_POWER > This driver provides support for the power supply features of > AXP20x PMIC. > > +config CHT_WC_FUEL_GAUGE > + tristate "Intel Cherry Trail Whiskey Cove PMIC Fuel Gauge" > + depends on INTEL_SOC_PMIC_CHTWC > + help > + This adds support for the battery fuel gauge found in the Intel > + Cherry Trail Whiskey Cove PMIC. This driver allows monitoring > + of the charge level of the battery on Intel Cherry Trail systems > + with a Whiskey Cove PMIC. > + > endif # POWER_SUPPLY > diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile > index 3789a2c..702e28a 100644 > --- a/drivers/power/supply/Makefile > +++ b/drivers/power/supply/Makefile > @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65090) += tps65090-charger.o > obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o > obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o > obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o > +obj-$(CONFIG_CHT_WC_FUEL_GAUGE) += cht_wc_fuel_gauge.o > diff --git a/drivers/power/supply/cht_wc_fuel_gauge.c > b/drivers/power/supply/cht_wc_fuel_gauge.c > new file mode 100644 > index 000..56f6e5a > --- /dev/null > +++ b/drivers/power/supply/cht_wc_fuel_gauge.c > @@ -0,0 +1,209 @@ > +/* > + * Intel CHT Whiskey Cove Fuel Gauge driver > + * Copyright (C) 2017 Hans de Goede > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License version > + * 2 as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * Cherrytrail Whiskey Cove devices have 2 functional blocks which interact > + * with the battery. > + * > + * 1) The fuel-gauge which is build into the Whiskey Cove PMIC, but has its > + * own i2c bus and i2c client addresses separately from the rest of the PMIC. > + * That block is what this driver is for. > + * > + * 2) An external charger IC, which is connected to the SMBUS controller > + * which is part of the rest of the Whiskey Cove PMIC, mfd/intel_cht_wc.c > + * registers a platform device for the SMBUS controller and > + * i2c/busses/i2c-cht-wc.c contains the i2c-adapter driver for this. > + * > + * However we want to present this as a single power_supply device to > + * userspace. So this driver offers a callback to get the fuel-gauge > + * power_supply properties, which gets passed to the external charger > + * driver via i2c_board_info when i2c-cht-wc.c calls i2c_new_device(). > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define REG_CHARGE_NOW 0x05 > +#define REG_VOLTAGE_NOW 0x09 > +#define REG_CURRENT_NOW 0x0a > +#define REG_CURRENT_AVG 0x0b > +#define REG_CHARGE_FULL 0x10 > +#define REG_CHARGE_DESIGN0x18 > +#define REG_VOLTAGE_AVG 0x19 > +#define REG_VOLTAGE_OCV 0x1b /* Only updated during charging */ > + > +#define CHT_WC_FG_PTYPE 4 > + > +struct cht_wc_fg_data { > + struct device *dev; > + struct i2c_client *client; > +}; > + > +static DEFINE_MUTEX(cht_wc_fg_mutex); > +static struct cht_wc_fg_data *cht_wc_fg; > + > +static int cht_wc_fg_read(struct cht_wc_fg_data *fg, u8 reg, > + union
Re: [PATCH] Input: synaptics-rmi4 - Report slot as inactive when contact is a palm
On Thu, Mar 16, 2017 at 05:52:07PM -0700, Andrew Duggan wrote: > On 03/16/2017 05:04 PM, Dmitry Torokhov wrote: > > On Thu, Mar 16, 2017 at 04:56:31PM -0700, Andrew Duggan wrote: > > > When the firmware identifies a contact as a palm the driver sets the tool > > > type to MT_TOOL_PALM, but sets the slot state as active. Reporting the > > > palm as active results in userspace input libraries considering the palm > > > as a valid contact. Touchpads which previously were using hid-multitouch > > > are now not suppressing palms when switching to the RMI4 driver. This > > > change fixes palm rejection when using the RMI4 driver. > > > > > > Signed-off-by: Andrew Duggan> > > Tested-by: Cameron Gutman > > > --- > > > drivers/input/rmi4/rmi_2d_sensor.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/input/rmi4/rmi_2d_sensor.c > > > b/drivers/input/rmi4/rmi_2d_sensor.c > > > index 8bb866c..8d1f295 100644 > > > --- a/drivers/input/rmi4/rmi_2d_sensor.c > > > +++ b/drivers/input/rmi4/rmi_2d_sensor.c > > > @@ -80,7 +80,8 @@ void rmi_2d_sensor_abs_report(struct rmi_2d_sensor > > > *sensor, > > > input_mt_slot(input, slot); > > > input_mt_report_slot_state(input, obj->mt_tool, > > > -obj->type != RMI_2D_OBJECT_NONE); > > > +(obj->type != RMI_2D_OBJECT_NONE) > > > +&& (obj->type != RMI_2D_OBJECT_PALM)); > > > if (obj->type != RMI_2D_OBJECT_NONE) { > > > obj->x = sensor->tracking_pos[slot].x; > > If we are relying on hardware to do palm rejection, then we should not > > be reporting the rest of the events for palm either (i.e. the condition > > in the if statement above should also be updated). > > > > But I do not understand why userspace doe snot do the right thing? Yes, > > the slot is active, but reported contact type is MT_TOOL_PALM, so it > > knows what it deals with. > > > > Thanks. > > > My intent is to notify userspace that there is a palm present. But, if > userspace does not have code which explicitly handles the MT_TOOL_PALM type > it won't be considered a finger. I think it is only recently that drivers > have started reporting MT_TOOL_PALM to userspace so I'm not sure if > libraries like libinput make use of it yet. > > Starting with v4.11 some touchpads will be switching from hid-multitouch to > the RMI4 driver and reporting palms as active results in unsuppressed palms. > I want to avoid users from upgrading and experiencing a degradation in > usability. In which case I can update the if statement and resubmit. This is > basically how hid-multitouch is handling it. Maybe in the future we can add > a parameter to enable reporting palms to userspace. Can you send me an evemu record of that happening please? I need to see the exact instance, e.g. when it's set, what's set before, etc. Ideally just attach to bug 100243, thanks. Cheers, Peter
Re: [PATCH] Input: synaptics-rmi4 - Report slot as inactive when contact is a palm
On Thu, Mar 16, 2017 at 05:52:07PM -0700, Andrew Duggan wrote: > On 03/16/2017 05:04 PM, Dmitry Torokhov wrote: > > On Thu, Mar 16, 2017 at 04:56:31PM -0700, Andrew Duggan wrote: > > > When the firmware identifies a contact as a palm the driver sets the tool > > > type to MT_TOOL_PALM, but sets the slot state as active. Reporting the > > > palm as active results in userspace input libraries considering the palm > > > as a valid contact. Touchpads which previously were using hid-multitouch > > > are now not suppressing palms when switching to the RMI4 driver. This > > > change fixes palm rejection when using the RMI4 driver. > > > > > > Signed-off-by: Andrew Duggan > > > Tested-by: Cameron Gutman > > > --- > > > drivers/input/rmi4/rmi_2d_sensor.c | 3 ++- > > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/input/rmi4/rmi_2d_sensor.c > > > b/drivers/input/rmi4/rmi_2d_sensor.c > > > index 8bb866c..8d1f295 100644 > > > --- a/drivers/input/rmi4/rmi_2d_sensor.c > > > +++ b/drivers/input/rmi4/rmi_2d_sensor.c > > > @@ -80,7 +80,8 @@ void rmi_2d_sensor_abs_report(struct rmi_2d_sensor > > > *sensor, > > > input_mt_slot(input, slot); > > > input_mt_report_slot_state(input, obj->mt_tool, > > > -obj->type != RMI_2D_OBJECT_NONE); > > > +(obj->type != RMI_2D_OBJECT_NONE) > > > +&& (obj->type != RMI_2D_OBJECT_PALM)); > > > if (obj->type != RMI_2D_OBJECT_NONE) { > > > obj->x = sensor->tracking_pos[slot].x; > > If we are relying on hardware to do palm rejection, then we should not > > be reporting the rest of the events for palm either (i.e. the condition > > in the if statement above should also be updated). > > > > But I do not understand why userspace doe snot do the right thing? Yes, > > the slot is active, but reported contact type is MT_TOOL_PALM, so it > > knows what it deals with. > > > > Thanks. > > > My intent is to notify userspace that there is a palm present. But, if > userspace does not have code which explicitly handles the MT_TOOL_PALM type > it won't be considered a finger. I think it is only recently that drivers > have started reporting MT_TOOL_PALM to userspace so I'm not sure if > libraries like libinput make use of it yet. > > Starting with v4.11 some touchpads will be switching from hid-multitouch to > the RMI4 driver and reporting palms as active results in unsuppressed palms. > I want to avoid users from upgrading and experiencing a degradation in > usability. In which case I can update the if statement and resubmit. This is > basically how hid-multitouch is handling it. Maybe in the future we can add > a parameter to enable reporting palms to userspace. Can you send me an evemu record of that happening please? I need to see the exact instance, e.g. when it's set, what's set before, etc. Ideally just attach to bug 100243, thanks. Cheers, Peter
Re: Synaptics RMI4 touchpad regression in 4.11-rc1
On Fri, Mar 17, 2017 at 12:23:36PM -0700, Andrew Duggan wrote: > On 03/17/2017 09:57 AM, Benjamin Tissoires wrote: > > On Wed, Mar 15, 2017 at 2:19 AM, Andrew Duggan> > wrote: > > > On 03/13/2017 10:10 PM, Cameron Gutman wrote: > > > > > > > > On 03/13/2017 06:35 PM, Andrew Duggan wrote: > > > > > > > > > > On 03/13/2017 06:15 AM, Benjamin Tissoires wrote: > > > > > > [Resending, forgot to add Jiri in CC] > > > > > > > > > > > > On Mar 13 2017 or thereabouts, Benjamin Tissoires wrote: > > > > > > > On Mar 13 2017 or thereabouts, Thorsten Leemhuis wrote: > > > > > > > > Lo! On 12.03.2017 02:55, Cameron Gutman wrote: > > > > > > > > > Beginning in 4.11-rc1, it looks like RMI4 is binding to my > > > > > > > > > XPS 13 > > > > > > > > > 9343's > > > > > > > > > Synaptics touchpad and dropping some errors into dmesg. Here > > > > > > > > > are the > > > > > > > > > messages that seem RMI-related: > > > > > > > > > > > > > > > > > > rmi4_f34 rmi4-00.fn34: rmi_f34v7_probe: Unrecognized > > > > > > > > > bootloader > > > > > > > > > version > > > > > > > > > rmi4_f34: probe of rmi4-00.fn34 failed with error -22 > > > > > > > > > rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: > > > > > > > > > Synaptics, > > > > > > > > > product: TM3038-001, fw id: 1832324 > > > > > > > > > input: Synaptics TM3038-001 as > > > > > > > > > /devices/pci:00/INT3433:00/i2c-7/i2c-DLL0665:01/0018:06CB:76AD.0001/input/input19 > > > > > > > > > hid-rmi 0018:06CB:76AD.0001: input,hidraw0: I2C HID v1.00 > > > > > > > > > Mouse > > > > > > > > > [DLL0665:01 06CB:76AD] on i2c-DLL0665:01 > > > > > > > > FWIW, I get this on my XPS 13 DE (9360) with 4.11-rc1: > > > > > > > > > > > > > > > > input: SynPS/2 Synaptics TouchPad as > > > > > > > > /devices/platform/i8042/serio1/input/input6 > > > > > > > > rmi4_f34 rmi4-00.fn34: rmi_f34v7_probe: Unrecognized bootloader > > > > > > > > version > > > > > > > > rmi4_f34: probe of rmi4-00.fn34 failed with error -22 > > > > > > > > rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: > > > > > > > > Synaptics, > > > > > > > > product: TM3038-003, fw id: 2375007 > > > > > > > > input: Synaptics TM3038-003 as > > > > > > > > > > > > > > > > /devices/pci:00/:00:15.1/i2c_designware.1/i2c-8/i2c-DLL075B:01/0018:06CB:76AF.0001/input/input20 > > > > > > > > hid-rmi 0018:06CB:76AF.0001: input,hidraw0: I2C HID v1.00 Mouse > > > > > > > > [DLL075B:01 06CB:76AF] on i2c-DLL075B:01 > > > > > > > > > > > > > > > > > […] > > > > > > > > > Compared to hid-multitouch, the RMI stack seems to have > > > > > > > > > completely > > > > > > > > > broken > > > > > > > > > palm rejection and introduced some random jumpiness during > > > > > > > > > fine > > > > > > > > > pointing > > > > > > > > > motions. I don't know if these issues are caused by the above > > > > > > > > > errors > > > > > > > > > or > > > > > > > > > are a separate issue. > > > > > The error about the bootloader version not being recognized just means > > > > > that updating the firmware is not supported on this touchpad. It is > > > > > only the > > > > > F34 firmware update functionality which is failing to load. The palm > > > > > rejection and jumps are not related to this error. > > > > > > > > > Maybe that code path should be changed to not make as much noise when it > > > > runs > > > > on known unsupported hardware. Something like the attached patch? > > > > > > > > > Looking at how hid-multitouch handles palms it looks like palms should > > > > > not be reported as active when calling input_mt_report_slot_state(). > > > > > I'm > > > > > setting the tool type to MT_TOOL_PALM when the firmware determines > > > > > that a > > > > > contact is a palm. But, that does not seem to be sufficient enough to > > > > > have > > > > > the palms filtered out. After some quick testing it looks like the > > > > > change > > > > > below will restore palm rejection similar to that provided by > > > > > hid-multitouch. > > > > > > > > > It looks like your email client ate the tabs, but if I apply the change > > > > myself it seems to fix the palm rejection regression for me. > > > > > > > > Tested-by: Cameron Gutman > > > > > > Sorry, I was short on time and just copied the diff into the email. I'll > > > submit a proper patch soon with your tested-by included. Thanks for > > > testing. > > > > > > > > I just pointed out this patch (well the actual submission) to Jason > > (Cc-ed). Given that there is no proper handling of MT_TOOL_PALM yet in > > userspace, I thought it was the easiest way. > > However, it seems that this doesn't enhance the jumps and just make it > > worse. > > I was assuming that the jumps and palm rejection where two separate issues. > But, the palm rejection patch makes things worse? > > > Is there anything we can do to fix it (besides temporary disabling the > > automatic loading of hid-rmi)? > > I do not have a fix for the jumps yet. My next
Re: Synaptics RMI4 touchpad regression in 4.11-rc1
On Fri, Mar 17, 2017 at 12:23:36PM -0700, Andrew Duggan wrote: > On 03/17/2017 09:57 AM, Benjamin Tissoires wrote: > > On Wed, Mar 15, 2017 at 2:19 AM, Andrew Duggan > > wrote: > > > On 03/13/2017 10:10 PM, Cameron Gutman wrote: > > > > > > > > On 03/13/2017 06:35 PM, Andrew Duggan wrote: > > > > > > > > > > On 03/13/2017 06:15 AM, Benjamin Tissoires wrote: > > > > > > [Resending, forgot to add Jiri in CC] > > > > > > > > > > > > On Mar 13 2017 or thereabouts, Benjamin Tissoires wrote: > > > > > > > On Mar 13 2017 or thereabouts, Thorsten Leemhuis wrote: > > > > > > > > Lo! On 12.03.2017 02:55, Cameron Gutman wrote: > > > > > > > > > Beginning in 4.11-rc1, it looks like RMI4 is binding to my > > > > > > > > > XPS 13 > > > > > > > > > 9343's > > > > > > > > > Synaptics touchpad and dropping some errors into dmesg. Here > > > > > > > > > are the > > > > > > > > > messages that seem RMI-related: > > > > > > > > > > > > > > > > > > rmi4_f34 rmi4-00.fn34: rmi_f34v7_probe: Unrecognized > > > > > > > > > bootloader > > > > > > > > > version > > > > > > > > > rmi4_f34: probe of rmi4-00.fn34 failed with error -22 > > > > > > > > > rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: > > > > > > > > > Synaptics, > > > > > > > > > product: TM3038-001, fw id: 1832324 > > > > > > > > > input: Synaptics TM3038-001 as > > > > > > > > > /devices/pci:00/INT3433:00/i2c-7/i2c-DLL0665:01/0018:06CB:76AD.0001/input/input19 > > > > > > > > > hid-rmi 0018:06CB:76AD.0001: input,hidraw0: I2C HID v1.00 > > > > > > > > > Mouse > > > > > > > > > [DLL0665:01 06CB:76AD] on i2c-DLL0665:01 > > > > > > > > FWIW, I get this on my XPS 13 DE (9360) with 4.11-rc1: > > > > > > > > > > > > > > > > input: SynPS/2 Synaptics TouchPad as > > > > > > > > /devices/platform/i8042/serio1/input/input6 > > > > > > > > rmi4_f34 rmi4-00.fn34: rmi_f34v7_probe: Unrecognized bootloader > > > > > > > > version > > > > > > > > rmi4_f34: probe of rmi4-00.fn34 failed with error -22 > > > > > > > > rmi4_f01 rmi4-00.fn01: found RMI device, manufacturer: > > > > > > > > Synaptics, > > > > > > > > product: TM3038-003, fw id: 2375007 > > > > > > > > input: Synaptics TM3038-003 as > > > > > > > > > > > > > > > > /devices/pci:00/:00:15.1/i2c_designware.1/i2c-8/i2c-DLL075B:01/0018:06CB:76AF.0001/input/input20 > > > > > > > > hid-rmi 0018:06CB:76AF.0001: input,hidraw0: I2C HID v1.00 Mouse > > > > > > > > [DLL075B:01 06CB:76AF] on i2c-DLL075B:01 > > > > > > > > > > > > > > > > > […] > > > > > > > > > Compared to hid-multitouch, the RMI stack seems to have > > > > > > > > > completely > > > > > > > > > broken > > > > > > > > > palm rejection and introduced some random jumpiness during > > > > > > > > > fine > > > > > > > > > pointing > > > > > > > > > motions. I don't know if these issues are caused by the above > > > > > > > > > errors > > > > > > > > > or > > > > > > > > > are a separate issue. > > > > > The error about the bootloader version not being recognized just means > > > > > that updating the firmware is not supported on this touchpad. It is > > > > > only the > > > > > F34 firmware update functionality which is failing to load. The palm > > > > > rejection and jumps are not related to this error. > > > > > > > > > Maybe that code path should be changed to not make as much noise when it > > > > runs > > > > on known unsupported hardware. Something like the attached patch? > > > > > > > > > Looking at how hid-multitouch handles palms it looks like palms should > > > > > not be reported as active when calling input_mt_report_slot_state(). > > > > > I'm > > > > > setting the tool type to MT_TOOL_PALM when the firmware determines > > > > > that a > > > > > contact is a palm. But, that does not seem to be sufficient enough to > > > > > have > > > > > the palms filtered out. After some quick testing it looks like the > > > > > change > > > > > below will restore palm rejection similar to that provided by > > > > > hid-multitouch. > > > > > > > > > It looks like your email client ate the tabs, but if I apply the change > > > > myself it seems to fix the palm rejection regression for me. > > > > > > > > Tested-by: Cameron Gutman > > > > > > Sorry, I was short on time and just copied the diff into the email. I'll > > > submit a proper patch soon with your tested-by included. Thanks for > > > testing. > > > > > > > > I just pointed out this patch (well the actual submission) to Jason > > (Cc-ed). Given that there is no proper handling of MT_TOOL_PALM yet in > > userspace, I thought it was the easiest way. > > However, it seems that this doesn't enhance the jumps and just make it > > worse. > > I was assuming that the jumps and palm rejection where two separate issues. > But, the palm rejection patch makes things worse? > > > Is there anything we can do to fix it (besides temporary disabling the > > automatic loading of hid-rmi)? > > I do not have a fix for the jumps yet. My next step is to file a bug against > libinput or
Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost
Hi, On Fri, Mar 17, 2017 at 10:55:22AM +0100, Hans de Goede wrote: > Add support for monitoring an extcon device with SDP/CDP/DCP and HOST > cables and adjust ilimit and enable/disable the 5v boost converter > accordingly. This is necessary on systems where the PSEL pin is hardwired > high and ILIM needs to be set by software based on the detected charger > type. > > Signed-off-by: Hans de GoedeAcked-By: Sebastian Reichel -- Sebastian > --- > drivers/power/supply/Kconfig | 1 + > drivers/power/supply/bq24190_charger.c | 85 > ++ > include/linux/power/bq24190_charger.h | 1 + > 3 files changed, 87 insertions(+) > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index f8b6e64..fd93110 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -442,6 +442,7 @@ config CHARGER_BQ2415X > config CHARGER_BQ24190 > tristate "TI BQ24190 battery charger driver" > depends on I2C > + depends on EXTCON > depends on GPIOLIB || COMPILE_TEST > help > Say Y to enable support for the TI BQ24190 battery charger. > diff --git a/drivers/power/supply/bq24190_charger.c > b/drivers/power/supply/bq24190_charger.c > index 82cb33d..03990e2 100644 > --- a/drivers/power/supply/bq24190_charger.c > +++ b/drivers/power/supply/bq24190_charger.c > @@ -11,10 +11,12 @@ > #include > #include > #include > +#include > #include > #include > #include > #include > +#include > #include > #include > > @@ -39,6 +41,8 @@ > #define BQ24190_REG_POC_WDT_RESET_SHIFT 6 > #define BQ24190_REG_POC_CHG_CONFIG_MASK (BIT(5) | BIT(4)) > #define BQ24190_REG_POC_CHG_CONFIG_SHIFT 4 > +#define BQ24190_REG_POC_CHG_CONFIG_CHARGE1 > +#define BQ24190_REG_POC_CHG_CONFIG_OTG 2 > #define BQ24190_REG_POC_SYS_MIN_MASK (BIT(3) | BIT(2) | BIT(1)) > #define BQ24190_REG_POC_SYS_MIN_SHIFT1 > #define BQ24190_REG_POC_BOOST_LIM_MASK BIT(0) > @@ -152,6 +156,9 @@ struct bq24190_dev_info { > struct power_supply *charger; > struct power_supply *battery; > struct bq24190_platform_data*pdata; > + struct extcon_dev *extcon; > + struct notifier_block extcon_nb; > + struct work_struct extcon_work; > charmodel_name[I2C_NAME_SIZE]; > kernel_ulong_t model; > struct mutexf_reg_lock; > @@ -167,6 +174,11 @@ struct bq24190_dev_info { > * number at that index in the array is the real-world value that it > * represents. > */ > + > +/* REG00[2:0] (IINLIM) in uAh */ > +static const int bq24190_iinlim_values[] = { > + 10, 15, 50, 90, 120, 150, 200, 300 }; > + > /* REG02[7:2] (ICHG) in uAh */ > static const int bq24190_ccc_ichg_values[] = { >512000, 576000, 64, 704000, 768000, 832000, 896000, 96, > @@ -1290,6 +1302,61 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, > void *data) > return IRQ_HANDLED; > } > > +static void bq24190_extcon_work(struct work_struct *work) > +{ > + struct bq24190_dev_info *bdi = > + container_of(work, struct bq24190_dev_info, extcon_work); > + int ret, iinlim = 0; > + > + if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1) > + iinlim = 50; > + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 || > + extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1) > + iinlim = 150; > + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1) > + iinlim = 200; > + > + if (iinlim) { > + ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC, > + BQ24190_REG_ISC_IINLIM_MASK, > + BQ24190_REG_ISC_IINLIM_SHIFT, > + bq24190_iinlim_values, > + ARRAY_SIZE(bq24190_iinlim_values), > + iinlim); > + if (ret) > + dev_err(bdi->dev, "Can't set IINLIM: %d\n", ret); > + } > + > + /* > + * If no charger has been detected and host mode is requested, activate > + * the 5V boost converter, otherwise deactivate it. > + */ > + if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) { > + ret = bq24190_write_mask(bdi, BQ24190_REG_POC, > + BQ24190_REG_POC_CHG_CONFIG_MASK, > + BQ24190_REG_POC_CHG_CONFIG_SHIFT, > + BQ24190_REG_POC_CHG_CONFIG_OTG); > + } else { > + ret = bq24190_write_mask(bdi, BQ24190_REG_POC, > + BQ24190_REG_POC_CHG_CONFIG_MASK, > +
Re: [PATCH 10/15] power: supply: bq24190_charger: Use extcon to determine ilimit, 5v boost
Hi, On Fri, Mar 17, 2017 at 10:55:22AM +0100, Hans de Goede wrote: > Add support for monitoring an extcon device with SDP/CDP/DCP and HOST > cables and adjust ilimit and enable/disable the 5v boost converter > accordingly. This is necessary on systems where the PSEL pin is hardwired > high and ILIM needs to be set by software based on the detected charger > type. > > Signed-off-by: Hans de Goede Acked-By: Sebastian Reichel -- Sebastian > --- > drivers/power/supply/Kconfig | 1 + > drivers/power/supply/bq24190_charger.c | 85 > ++ > include/linux/power/bq24190_charger.h | 1 + > 3 files changed, 87 insertions(+) > > diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig > index f8b6e64..fd93110 100644 > --- a/drivers/power/supply/Kconfig > +++ b/drivers/power/supply/Kconfig > @@ -442,6 +442,7 @@ config CHARGER_BQ2415X > config CHARGER_BQ24190 > tristate "TI BQ24190 battery charger driver" > depends on I2C > + depends on EXTCON > depends on GPIOLIB || COMPILE_TEST > help > Say Y to enable support for the TI BQ24190 battery charger. > diff --git a/drivers/power/supply/bq24190_charger.c > b/drivers/power/supply/bq24190_charger.c > index 82cb33d..03990e2 100644 > --- a/drivers/power/supply/bq24190_charger.c > +++ b/drivers/power/supply/bq24190_charger.c > @@ -11,10 +11,12 @@ > #include > #include > #include > +#include > #include > #include > #include > #include > +#include > #include > #include > > @@ -39,6 +41,8 @@ > #define BQ24190_REG_POC_WDT_RESET_SHIFT 6 > #define BQ24190_REG_POC_CHG_CONFIG_MASK (BIT(5) | BIT(4)) > #define BQ24190_REG_POC_CHG_CONFIG_SHIFT 4 > +#define BQ24190_REG_POC_CHG_CONFIG_CHARGE1 > +#define BQ24190_REG_POC_CHG_CONFIG_OTG 2 > #define BQ24190_REG_POC_SYS_MIN_MASK (BIT(3) | BIT(2) | BIT(1)) > #define BQ24190_REG_POC_SYS_MIN_SHIFT1 > #define BQ24190_REG_POC_BOOST_LIM_MASK BIT(0) > @@ -152,6 +156,9 @@ struct bq24190_dev_info { > struct power_supply *charger; > struct power_supply *battery; > struct bq24190_platform_data*pdata; > + struct extcon_dev *extcon; > + struct notifier_block extcon_nb; > + struct work_struct extcon_work; > charmodel_name[I2C_NAME_SIZE]; > kernel_ulong_t model; > struct mutexf_reg_lock; > @@ -167,6 +174,11 @@ struct bq24190_dev_info { > * number at that index in the array is the real-world value that it > * represents. > */ > + > +/* REG00[2:0] (IINLIM) in uAh */ > +static const int bq24190_iinlim_values[] = { > + 10, 15, 50, 90, 120, 150, 200, 300 }; > + > /* REG02[7:2] (ICHG) in uAh */ > static const int bq24190_ccc_ichg_values[] = { >512000, 576000, 64, 704000, 768000, 832000, 896000, 96, > @@ -1290,6 +1302,61 @@ static irqreturn_t bq24190_irq_handler_thread(int irq, > void *data) > return IRQ_HANDLED; > } > > +static void bq24190_extcon_work(struct work_struct *work) > +{ > + struct bq24190_dev_info *bdi = > + container_of(work, struct bq24190_dev_info, extcon_work); > + int ret, iinlim = 0; > + > + if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_SDP) == 1) > + iinlim = 50; > + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_CDP) == 1 || > + extcon_get_state(bdi->extcon, EXTCON_CHG_USB_ACA) == 1) > + iinlim = 150; > + else if (extcon_get_state(bdi->extcon, EXTCON_CHG_USB_DCP) == 1) > + iinlim = 200; > + > + if (iinlim) { > + ret = bq24190_set_field_val(bdi, BQ24190_REG_ISC, > + BQ24190_REG_ISC_IINLIM_MASK, > + BQ24190_REG_ISC_IINLIM_SHIFT, > + bq24190_iinlim_values, > + ARRAY_SIZE(bq24190_iinlim_values), > + iinlim); > + if (ret) > + dev_err(bdi->dev, "Can't set IINLIM: %d\n", ret); > + } > + > + /* > + * If no charger has been detected and host mode is requested, activate > + * the 5V boost converter, otherwise deactivate it. > + */ > + if (!iinlim && extcon_get_state(bdi->extcon, EXTCON_USB_HOST) == 1) { > + ret = bq24190_write_mask(bdi, BQ24190_REG_POC, > + BQ24190_REG_POC_CHG_CONFIG_MASK, > + BQ24190_REG_POC_CHG_CONFIG_SHIFT, > + BQ24190_REG_POC_CHG_CONFIG_OTG); > + } else { > + ret = bq24190_write_mask(bdi, BQ24190_REG_POC, > + BQ24190_REG_POC_CHG_CONFIG_MASK, > +
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/20/2017 at 11:55 AM, Baoquan He wrote: > On 03/20/17 at 10:39am, Xunlei Pang wrote: >> On 03/20/2017 at 10:13 AM, Baoquan He wrote: >>> On 03/17/17 at 12:22pm, Eric W. Biederman wrote: Xunlei Pangwrites: > Currently vmcoreinfo data is updated at boot time subsys_initcall(), > it has the risk of being modified by some wrong code during system > is running. > > As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, > when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, > we probably will get "Segmentation fault" or other unexpected/confusing > errors. If this is a real concern and the previous discussion sounds like it is part of what we need to do is move the variable vmcoreinfo_note out of the kernel's .bss section. And modify the code to regenerate and keep this information in something like the control page. >>> I guess this is not from a real issue, just from Xunlei's worry. But >>> Xunlei didn't give a direct answer to this, and Petr's question. Not >> It's easy to reproduce: write a kernel module to modify part content of >> vmcoreinfo_data (we surely have many ways to acquire its VA). If it does >> exist in theory, we will met it sooner or later in real world due to billions >> of applications. >> >> Also there are bugs like this one >> https://bugzilla.redhat.com/show_bug.cgi?id=1287097 >> Not sure if it is makedumpfile issue or this one, maybe we can't know >> forever. > Well, kdump is not all-purpose. If you write code in module to stomp > page init_level4_pgt is pointing at, you won't get a vmcore. vmcoreinfo is a large data chunk prepared for kdump not a normal-sized variable, we better protect it. > And you are saying vmcoreinfo_data, it's a intermediate page, should be > vmcoreinfo_note. If the wrong code you mentioned didn't change > vmcoreinfo_note, but other kernel data which need be saved into > vmcoreinfo_note, crash_save_vmcoreinfo_init is doing better than you > re-saved one. I am not going to touch vmcoreinfo_note, just trying to relocate vmcoreinfo_data into the crash memory, then use it to update vmcoreinfo_note which is fully overwritten when crash happens just like the cpu crash note. Anyway I will send v3 soon, let further discuss it there, thanks! Regards, Xunlei > >> Regards, >> Xunlei >> >>> very sure if this will impact other implementation. fadump will be >>> impacted by this or other dump? Maybe yet or maybe not. >>> >>> I don't object this strongly, but please at least add code comment to >>> explain why vmcoreinfo need be saved twice because it does look weird. >>> Definitely something like this needs a page all to itself, and ideally far away from any other kernel data structures. I clearly was not watching closely the data someone decided to keep this silly thing in the kernel's .bss section. > As vmcoreinfo is the most fundamental information for vmcore, we better > double check its correctness. Here we generate a signature(using crc32) > after it is saved, then verify it in crash_save_vmcoreinfo() to see if > the signature was broken, if so we have to re-save the vmcoreinfo data > to get the correct vmcoreinfo for kdump as possible as we can. Sigh. We already have a sha256 that is supposed to cover this sort of thing. The bug rather is that apparently it isn't covering this data. That sounds like what we should be fixing. Please let's not invent new mechanisms we have to maintain. Let's reorganize this so this static data is protected like all other static data in the kexec-on-panic path. We have good mechanims and good strategies for avoiding and detecting corruption we just need to use them. Eric > Signed-off-by: Xunlei Pang > --- > v1->v2: > - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" > uses the information. > - Add crc32 verification for vmcoreinfo, re-save when failure. > > arch/Kconfig| 1 + > kernel/kexec_core.c | 43 +++ > 2 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index c4d6833..66eb296 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -4,6 +4,7 @@ > > config KEXEC_CORE > bool > + select CRC32 > > config HAVE_IMA_KEXEC > bool > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > index bfe62d5..012acbe 100644 > --- a/kernel/kexec_core.c > +++ b/kernel/kexec_core.c > @@ -38,6 +38,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -53,9 +54,10 @@ > > /* vmcoreinfo stuff */ > static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; > -u32
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/20/2017 at 11:55 AM, Baoquan He wrote: > On 03/20/17 at 10:39am, Xunlei Pang wrote: >> On 03/20/2017 at 10:13 AM, Baoquan He wrote: >>> On 03/17/17 at 12:22pm, Eric W. Biederman wrote: Xunlei Pang writes: > Currently vmcoreinfo data is updated at boot time subsys_initcall(), > it has the risk of being modified by some wrong code during system > is running. > > As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, > when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, > we probably will get "Segmentation fault" or other unexpected/confusing > errors. If this is a real concern and the previous discussion sounds like it is part of what we need to do is move the variable vmcoreinfo_note out of the kernel's .bss section. And modify the code to regenerate and keep this information in something like the control page. >>> I guess this is not from a real issue, just from Xunlei's worry. But >>> Xunlei didn't give a direct answer to this, and Petr's question. Not >> It's easy to reproduce: write a kernel module to modify part content of >> vmcoreinfo_data (we surely have many ways to acquire its VA). If it does >> exist in theory, we will met it sooner or later in real world due to billions >> of applications. >> >> Also there are bugs like this one >> https://bugzilla.redhat.com/show_bug.cgi?id=1287097 >> Not sure if it is makedumpfile issue or this one, maybe we can't know >> forever. > Well, kdump is not all-purpose. If you write code in module to stomp > page init_level4_pgt is pointing at, you won't get a vmcore. vmcoreinfo is a large data chunk prepared for kdump not a normal-sized variable, we better protect it. > And you are saying vmcoreinfo_data, it's a intermediate page, should be > vmcoreinfo_note. If the wrong code you mentioned didn't change > vmcoreinfo_note, but other kernel data which need be saved into > vmcoreinfo_note, crash_save_vmcoreinfo_init is doing better than you > re-saved one. I am not going to touch vmcoreinfo_note, just trying to relocate vmcoreinfo_data into the crash memory, then use it to update vmcoreinfo_note which is fully overwritten when crash happens just like the cpu crash note. Anyway I will send v3 soon, let further discuss it there, thanks! Regards, Xunlei > >> Regards, >> Xunlei >> >>> very sure if this will impact other implementation. fadump will be >>> impacted by this or other dump? Maybe yet or maybe not. >>> >>> I don't object this strongly, but please at least add code comment to >>> explain why vmcoreinfo need be saved twice because it does look weird. >>> Definitely something like this needs a page all to itself, and ideally far away from any other kernel data structures. I clearly was not watching closely the data someone decided to keep this silly thing in the kernel's .bss section. > As vmcoreinfo is the most fundamental information for vmcore, we better > double check its correctness. Here we generate a signature(using crc32) > after it is saved, then verify it in crash_save_vmcoreinfo() to see if > the signature was broken, if so we have to re-save the vmcoreinfo data > to get the correct vmcoreinfo for kdump as possible as we can. Sigh. We already have a sha256 that is supposed to cover this sort of thing. The bug rather is that apparently it isn't covering this data. That sounds like what we should be fixing. Please let's not invent new mechanisms we have to maintain. Let's reorganize this so this static data is protected like all other static data in the kexec-on-panic path. We have good mechanims and good strategies for avoiding and detecting corruption we just need to use them. Eric > Signed-off-by: Xunlei Pang > --- > v1->v2: > - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" > uses the information. > - Add crc32 verification for vmcoreinfo, re-save when failure. > > arch/Kconfig| 1 + > kernel/kexec_core.c | 43 +++ > 2 files changed, 36 insertions(+), 8 deletions(-) > > diff --git a/arch/Kconfig b/arch/Kconfig > index c4d6833..66eb296 100644 > --- a/arch/Kconfig > +++ b/arch/Kconfig > @@ -4,6 +4,7 @@ > > config KEXEC_CORE > bool > + select CRC32 > > config HAVE_IMA_KEXEC > bool > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > index bfe62d5..012acbe 100644 > --- a/kernel/kexec_core.c > +++ b/kernel/kexec_core.c > @@ -38,6 +38,7 @@ > #include > #include > #include > +#include > > #include > #include > @@ -53,9 +54,10 @@ > > /* vmcoreinfo stuff */ > static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; > -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4];
Re: [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core irq-mapping code
Hi, On Fri, Mar 17, 2017 at 07:24:14PM +0200, Andy Shevchenko wrote: > On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: > > The i2c-core already maps of irqs before calling the driver's probe > > function and there are no in tree users of > > bq24190_platform_data->gpio_int. > > > > Remove the redundant custom irq-mapping code and just use client->irq. > > > > Nice! I did similar clean up to some other driver, so, > > Reviewed-by: Andy Shevchenko> > P.S. This should be first in the series against bq24190. Right. Acked-by: Sebastian Reichel -- Sebastian > > Signed-off-by: Hans de Goede > > --- > >  drivers/power/supply/bq24190_charger.c | 61 ++--- > > - > >  include/linux/power/bq24190_charger.h  |  1 - > >  2 files changed, 2 insertions(+), 60 deletions(-) > > > > diff --git a/drivers/power/supply/bq24190_charger.c > > b/drivers/power/supply/bq24190_charger.c > > index 7bca8d0..9c4b171 100644 > > --- a/drivers/power/supply/bq24190_charger.c > > +++ b/drivers/power/supply/bq24190_charger.c > > @@ -154,8 +154,6 @@ struct bq24190_dev_info { > >  struct bq24190_platform_data*pdata; > >  charmodel_name[I2C_NAME_SIZE] > > ; > >  kernel_ulong_t model; > > - unsigned intgpio_int; > > - unsigned intirq; > >  struct mutexf_reg_lock; > >  u8 f_reg; > >  u8 ss_reg; > > @@ -1296,56 +1294,11 @@ static int bq24190_hw_init(struct > > bq24190_dev_info *bdi) > >  return ret; > >  } > >  > > -#ifdef CONFIG_OF > > -static int bq24190_setup_dt(struct bq24190_dev_info *bdi) > > -{ > > - bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0); > > - if (bdi->irq <= 0) > > - return -1; > > - > > - return 0; > > -} > > -#else > > -static int bq24190_setup_dt(struct bq24190_dev_info *bdi) > > -{ > > - return -1; > > -} > > -#endif > > - > > -static int bq24190_setup_pdata(struct bq24190_dev_info *bdi, > > - struct bq24190_platform_data *pdata) > > -{ > > - int ret; > > - > > - if (!gpio_is_valid(pdata->gpio_int)) > > - return -1; > > - > > - ret = gpio_request(pdata->gpio_int, dev_name(bdi->dev)); > > - if (ret < 0) > > - return -1; > > - > > - ret = gpio_direction_input(pdata->gpio_int); > > - if (ret < 0) > > - goto out; > > - > > - bdi->irq = gpio_to_irq(pdata->gpio_int); > > - if (!bdi->irq) > > - goto out; > > - > > - bdi->gpio_int = pdata->gpio_int; > > - return 0; > > - > > -out: > > - gpio_free(pdata->gpio_int); > > - return -1; > > -} > > - > >  static int bq24190_probe(struct i2c_client *client, > >  const struct i2c_device_id *id) > >  { > >  struct i2c_adapter *adapter = to_i2c_adapter(client- > > >dev.parent); > >  struct device *dev = >dev; > > - struct bq24190_platform_data *pdata = client- > > >dev.platform_data; > >  struct power_supply_config charger_cfg = {}, battery_cfg = > > {}; > >  struct bq24190_dev_info *bdi; > >  int ret; > > @@ -1372,12 +1325,7 @@ static int bq24190_probe(struct i2c_client > > *client, > >  > >  i2c_set_clientdata(client, bdi); > >  > > - if (dev->of_node) > > - ret = bq24190_setup_dt(bdi); > > - else > > - ret = bq24190_setup_pdata(bdi, pdata); > > - > > - if (ret) { > > + if (!client->irq) { > >  dev_err(dev, "Can't get irq info\n"); > >  return -EINVAL; > >  } > > @@ -1417,7 +1365,7 @@ static int bq24190_probe(struct i2c_client > > *client, > >  goto out3; > >  } > >  > > - ret = devm_request_threaded_irq(dev, bdi->irq, NULL, > > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > >  bq24190_irq_handler_thread, > >  IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >  "bq24190-charger", bdi); > > @@ -1439,8 +1387,6 @@ static int bq24190_probe(struct i2c_client > > *client, > >  > >  out1: > >  pm_runtime_disable(dev); > > - if (bdi->gpio_int) > > - gpio_free(bdi->gpio_int); > >  return ret; > >  } > >  > > @@ -1457,9 +1403,6 @@ static int bq24190_remove(struct i2c_client > > *client) > >  power_supply_unregister(bdi->charger); > >  pm_runtime_disable(bdi->dev); > >  > > - if (bdi->gpio_int) > > - gpio_free(bdi->gpio_int); > > - > >  return 0; > >  } > >  > > diff --git a/include/linux/power/bq24190_charger.h > > b/include/linux/power/bq24190_charger.h > > index cb49717..8d918cb 100644 > > --- a/include/linux/power/bq24190_charger.h > > +++ b/include/linux/power/bq24190_charger.h > > @@ -10,7 +10,6 @@ > >  #define _BQ24190_CHARGER_H_ > >  > >  struct bq24190_platform_data { > > - unsigned intgpio_int; /* GPIO pin that's > > connected to INT# */ > >  bool no_register_reset;
Re: [PATCH 06/15] power: supply: bq24190_charger: Use i2c-core irq-mapping code
Hi, On Fri, Mar 17, 2017 at 07:24:14PM +0200, Andy Shevchenko wrote: > On Fri, 2017-03-17 at 10:55 +0100, Hans de Goede wrote: > > The i2c-core already maps of irqs before calling the driver's probe > > function and there are no in tree users of > > bq24190_platform_data->gpio_int. > > > > Remove the redundant custom irq-mapping code and just use client->irq. > > > > Nice! I did similar clean up to some other driver, so, > > Reviewed-by: Andy Shevchenko > > P.S. This should be first in the series against bq24190. Right. Acked-by: Sebastian Reichel -- Sebastian > > Signed-off-by: Hans de Goede > > --- > >  drivers/power/supply/bq24190_charger.c | 61 ++--- > > - > >  include/linux/power/bq24190_charger.h  |  1 - > >  2 files changed, 2 insertions(+), 60 deletions(-) > > > > diff --git a/drivers/power/supply/bq24190_charger.c > > b/drivers/power/supply/bq24190_charger.c > > index 7bca8d0..9c4b171 100644 > > --- a/drivers/power/supply/bq24190_charger.c > > +++ b/drivers/power/supply/bq24190_charger.c > > @@ -154,8 +154,6 @@ struct bq24190_dev_info { > >  struct bq24190_platform_data*pdata; > >  charmodel_name[I2C_NAME_SIZE] > > ; > >  kernel_ulong_t model; > > - unsigned intgpio_int; > > - unsigned intirq; > >  struct mutexf_reg_lock; > >  u8 f_reg; > >  u8 ss_reg; > > @@ -1296,56 +1294,11 @@ static int bq24190_hw_init(struct > > bq24190_dev_info *bdi) > >  return ret; > >  } > >  > > -#ifdef CONFIG_OF > > -static int bq24190_setup_dt(struct bq24190_dev_info *bdi) > > -{ > > - bdi->irq = irq_of_parse_and_map(bdi->dev->of_node, 0); > > - if (bdi->irq <= 0) > > - return -1; > > - > > - return 0; > > -} > > -#else > > -static int bq24190_setup_dt(struct bq24190_dev_info *bdi) > > -{ > > - return -1; > > -} > > -#endif > > - > > -static int bq24190_setup_pdata(struct bq24190_dev_info *bdi, > > - struct bq24190_platform_data *pdata) > > -{ > > - int ret; > > - > > - if (!gpio_is_valid(pdata->gpio_int)) > > - return -1; > > - > > - ret = gpio_request(pdata->gpio_int, dev_name(bdi->dev)); > > - if (ret < 0) > > - return -1; > > - > > - ret = gpio_direction_input(pdata->gpio_int); > > - if (ret < 0) > > - goto out; > > - > > - bdi->irq = gpio_to_irq(pdata->gpio_int); > > - if (!bdi->irq) > > - goto out; > > - > > - bdi->gpio_int = pdata->gpio_int; > > - return 0; > > - > > -out: > > - gpio_free(pdata->gpio_int); > > - return -1; > > -} > > - > >  static int bq24190_probe(struct i2c_client *client, > >  const struct i2c_device_id *id) > >  { > >  struct i2c_adapter *adapter = to_i2c_adapter(client- > > >dev.parent); > >  struct device *dev = >dev; > > - struct bq24190_platform_data *pdata = client- > > >dev.platform_data; > >  struct power_supply_config charger_cfg = {}, battery_cfg = > > {}; > >  struct bq24190_dev_info *bdi; > >  int ret; > > @@ -1372,12 +1325,7 @@ static int bq24190_probe(struct i2c_client > > *client, > >  > >  i2c_set_clientdata(client, bdi); > >  > > - if (dev->of_node) > > - ret = bq24190_setup_dt(bdi); > > - else > > - ret = bq24190_setup_pdata(bdi, pdata); > > - > > - if (ret) { > > + if (!client->irq) { > >  dev_err(dev, "Can't get irq info\n"); > >  return -EINVAL; > >  } > > @@ -1417,7 +1365,7 @@ static int bq24190_probe(struct i2c_client > > *client, > >  goto out3; > >  } > >  > > - ret = devm_request_threaded_irq(dev, bdi->irq, NULL, > > + ret = devm_request_threaded_irq(dev, client->irq, NULL, > >  bq24190_irq_handler_thread, > >  IRQF_TRIGGER_FALLING | IRQF_ONESHOT, > >  "bq24190-charger", bdi); > > @@ -1439,8 +1387,6 @@ static int bq24190_probe(struct i2c_client > > *client, > >  > >  out1: > >  pm_runtime_disable(dev); > > - if (bdi->gpio_int) > > - gpio_free(bdi->gpio_int); > >  return ret; > >  } > >  > > @@ -1457,9 +1403,6 @@ static int bq24190_remove(struct i2c_client > > *client) > >  power_supply_unregister(bdi->charger); > >  pm_runtime_disable(bdi->dev); > >  > > - if (bdi->gpio_int) > > - gpio_free(bdi->gpio_int); > > - > >  return 0; > >  } > >  > > diff --git a/include/linux/power/bq24190_charger.h > > b/include/linux/power/bq24190_charger.h > > index cb49717..8d918cb 100644 > > --- a/include/linux/power/bq24190_charger.h > > +++ b/include/linux/power/bq24190_charger.h > > @@ -10,7 +10,6 @@ > >  #define _BQ24190_CHARGER_H_ > >  > >  struct bq24190_platform_data { > > - unsigned intgpio_int; /* GPIO pin that's > > connected to INT# */ > >  bool no_register_reset; > >  }; > >  > > -- > Andy Shevchenko > Intel Finland Oy
linux-next: manual merge of the gpio tree with the input tree
Hi Linus, Today's linux-next merge of the gpio tree got a conflict in: drivers/input/misc/soc_button_array.c between commit: a01cd17000a4 ("Input: soc_button_array - use NULL for GPIO connection ID") from the input tree and commit: c5097538c86a ("Input: soc_button_array - Propagate error from gpiod_count()") from the gpio tree. I fixed it up (see below) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell diff --cc drivers/input/misc/soc_button_array.c index 95b787a63560,c3b8e1fb4699.. --- a/drivers/input/misc/soc_button_array.c +++ b/drivers/input/misc/soc_button_array.c @@@ -312,17 -167,12 +312,18 @@@ static int soc_button_probe(struct plat if (!id) return -ENODEV; - button_info = (struct soc_button_info *)id->driver_data; + if (!id->driver_data) { + button_info = soc_button_get_button_info(dev); + if (IS_ERR(button_info)) + return PTR_ERR(button_info); + } else { + button_info = (struct soc_button_info *)id->driver_data; + } - if (gpiod_count(dev, NULL) <= 0) { + error = gpiod_count(dev, NULL); + if (error < 0) { dev_dbg(dev, "no GPIO attached, ignoring...\n"); - return -ENODEV; + return error; } priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
linux-next: manual merge of the gpio tree with the input tree
Hi Linus, Today's linux-next merge of the gpio tree got a conflict in: drivers/input/misc/soc_button_array.c between commit: a01cd17000a4 ("Input: soc_button_array - use NULL for GPIO connection ID") from the input tree and commit: c5097538c86a ("Input: soc_button_array - Propagate error from gpiod_count()") from the gpio tree. I fixed it up (see below) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell diff --cc drivers/input/misc/soc_button_array.c index 95b787a63560,c3b8e1fb4699.. --- a/drivers/input/misc/soc_button_array.c +++ b/drivers/input/misc/soc_button_array.c @@@ -312,17 -167,12 +312,18 @@@ static int soc_button_probe(struct plat if (!id) return -ENODEV; - button_info = (struct soc_button_info *)id->driver_data; + if (!id->driver_data) { + button_info = soc_button_get_button_info(dev); + if (IS_ERR(button_info)) + return PTR_ERR(button_info); + } else { + button_info = (struct soc_button_info *)id->driver_data; + } - if (gpiod_count(dev, NULL) <= 0) { + error = gpiod_count(dev, NULL); + if (error < 0) { dev_dbg(dev, "no GPIO attached, ignoring...\n"); - return -ENODEV; + return error; } priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/20/17 at 10:39am, Xunlei Pang wrote: > On 03/20/2017 at 10:13 AM, Baoquan He wrote: > > On 03/17/17 at 12:22pm, Eric W. Biederman wrote: > >> Xunlei Pangwrites: > >> > >>> Currently vmcoreinfo data is updated at boot time subsys_initcall(), > >>> it has the risk of being modified by some wrong code during system > >>> is running. > >>> > >>> As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, > >>> when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, > >>> we probably will get "Segmentation fault" or other unexpected/confusing > >>> errors. > >> If this is a real concern and the previous discussion sounds like it is > >> part of what we need to do is move the variable vmcoreinfo_note out > >> of the kernel's .bss section. And modify the code to regenerate > >> and keep this information in something like the control page. > > I guess this is not from a real issue, just from Xunlei's worry. But > > Xunlei didn't give a direct answer to this, and Petr's question. Not > > It's easy to reproduce: write a kernel module to modify part content of > vmcoreinfo_data (we surely have many ways to acquire its VA). If it does > exist in theory, we will met it sooner or later in real world due to billions > of applications. > > Also there are bugs like this one > https://bugzilla.redhat.com/show_bug.cgi?id=1287097 > Not sure if it is makedumpfile issue or this one, maybe we can't know forever. Well, kdump is not all-purpose. If you write code in module to stomp page init_level4_pgt is pointing at, you won't get a vmcore. And you are saying vmcoreinfo_data, it's a intermediate page, should be vmcoreinfo_note. If the wrong code you mentioned didn't change vmcoreinfo_note, but other kernel data which need be saved into vmcoreinfo_note, crash_save_vmcoreinfo_init is doing better than you re-saved one. > > Regards, > Xunlei > > > very sure if this will impact other implementation. fadump will be > > impacted by this or other dump? Maybe yet or maybe not. > > > > I don't object this strongly, but please at least add code comment to > > explain why vmcoreinfo need be saved twice because it does look weird. > > > >> Definitely something like this needs a page all to itself, and ideally > >> far away from any other kernel data structures. I clearly was not > >> watching closely the data someone decided to keep this silly thing > >> in the kernel's .bss section. > >> > >>> As vmcoreinfo is the most fundamental information for vmcore, we better > >>> double check its correctness. Here we generate a signature(using crc32) > >>> after it is saved, then verify it in crash_save_vmcoreinfo() to see if > >>> the signature was broken, if so we have to re-save the vmcoreinfo data > >>> to get the correct vmcoreinfo for kdump as possible as we can. > >> Sigh. We already have a sha256 that is supposed to cover this sort of > >> thing. The bug rather is that apparently it isn't covering this data. > >> That sounds like what we should be fixing. > >> > >> Please let's not invent new mechanisms we have to maintain. Let's > >> reorganize this so this static data is protected like all other static > >> data in the kexec-on-panic path. We have good mechanims and good > >> strategies for avoiding and detecting corruption we just need to use > >> them. > >> > >> Eric > >> > >> > >> > >>> Signed-off-by: Xunlei Pang > >>> --- > >>> v1->v2: > >>> - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" > >>> uses the information. > >>> - Add crc32 verification for vmcoreinfo, re-save when failure. > >>> > >>> arch/Kconfig| 1 + > >>> kernel/kexec_core.c | 43 +++ > >>> 2 files changed, 36 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/arch/Kconfig b/arch/Kconfig > >>> index c4d6833..66eb296 100644 > >>> --- a/arch/Kconfig > >>> +++ b/arch/Kconfig > >>> @@ -4,6 +4,7 @@ > >>> > >>> config KEXEC_CORE > >>> bool > >>> + select CRC32 > >>> > >>> config HAVE_IMA_KEXEC > >>> bool > >>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > >>> index bfe62d5..012acbe 100644 > >>> --- a/kernel/kexec_core.c > >>> +++ b/kernel/kexec_core.c > >>> @@ -38,6 +38,7 @@ > >>> #include > >>> #include > >>> #include > >>> +#include > >>> > >>> #include > >>> #include > >>> @@ -53,9 +54,10 @@ > >>> > >>> /* vmcoreinfo stuff */ > >>> static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; > >>> -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > >>> +static u32 vmcoreinfo_sig; > >>> size_t vmcoreinfo_size; > >>> size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); > >>> +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > >>> > >>> /* Flag to indicate we are going to kexec a new kernel */ > >>> bool kexec_in_progress = false; > >>> @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void) > >>> final_note(buf); > >>> } > >>> > >>> -void crash_save_vmcoreinfo(void) >
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/20/17 at 10:39am, Xunlei Pang wrote: > On 03/20/2017 at 10:13 AM, Baoquan He wrote: > > On 03/17/17 at 12:22pm, Eric W. Biederman wrote: > >> Xunlei Pang writes: > >> > >>> Currently vmcoreinfo data is updated at boot time subsys_initcall(), > >>> it has the risk of being modified by some wrong code during system > >>> is running. > >>> > >>> As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, > >>> when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, > >>> we probably will get "Segmentation fault" or other unexpected/confusing > >>> errors. > >> If this is a real concern and the previous discussion sounds like it is > >> part of what we need to do is move the variable vmcoreinfo_note out > >> of the kernel's .bss section. And modify the code to regenerate > >> and keep this information in something like the control page. > > I guess this is not from a real issue, just from Xunlei's worry. But > > Xunlei didn't give a direct answer to this, and Petr's question. Not > > It's easy to reproduce: write a kernel module to modify part content of > vmcoreinfo_data (we surely have many ways to acquire its VA). If it does > exist in theory, we will met it sooner or later in real world due to billions > of applications. > > Also there are bugs like this one > https://bugzilla.redhat.com/show_bug.cgi?id=1287097 > Not sure if it is makedumpfile issue or this one, maybe we can't know forever. Well, kdump is not all-purpose. If you write code in module to stomp page init_level4_pgt is pointing at, you won't get a vmcore. And you are saying vmcoreinfo_data, it's a intermediate page, should be vmcoreinfo_note. If the wrong code you mentioned didn't change vmcoreinfo_note, but other kernel data which need be saved into vmcoreinfo_note, crash_save_vmcoreinfo_init is doing better than you re-saved one. > > Regards, > Xunlei > > > very sure if this will impact other implementation. fadump will be > > impacted by this or other dump? Maybe yet or maybe not. > > > > I don't object this strongly, but please at least add code comment to > > explain why vmcoreinfo need be saved twice because it does look weird. > > > >> Definitely something like this needs a page all to itself, and ideally > >> far away from any other kernel data structures. I clearly was not > >> watching closely the data someone decided to keep this silly thing > >> in the kernel's .bss section. > >> > >>> As vmcoreinfo is the most fundamental information for vmcore, we better > >>> double check its correctness. Here we generate a signature(using crc32) > >>> after it is saved, then verify it in crash_save_vmcoreinfo() to see if > >>> the signature was broken, if so we have to re-save the vmcoreinfo data > >>> to get the correct vmcoreinfo for kdump as possible as we can. > >> Sigh. We already have a sha256 that is supposed to cover this sort of > >> thing. The bug rather is that apparently it isn't covering this data. > >> That sounds like what we should be fixing. > >> > >> Please let's not invent new mechanisms we have to maintain. Let's > >> reorganize this so this static data is protected like all other static > >> data in the kexec-on-panic path. We have good mechanims and good > >> strategies for avoiding and detecting corruption we just need to use > >> them. > >> > >> Eric > >> > >> > >> > >>> Signed-off-by: Xunlei Pang > >>> --- > >>> v1->v2: > >>> - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" > >>> uses the information. > >>> - Add crc32 verification for vmcoreinfo, re-save when failure. > >>> > >>> arch/Kconfig| 1 + > >>> kernel/kexec_core.c | 43 +++ > >>> 2 files changed, 36 insertions(+), 8 deletions(-) > >>> > >>> diff --git a/arch/Kconfig b/arch/Kconfig > >>> index c4d6833..66eb296 100644 > >>> --- a/arch/Kconfig > >>> +++ b/arch/Kconfig > >>> @@ -4,6 +4,7 @@ > >>> > >>> config KEXEC_CORE > >>> bool > >>> + select CRC32 > >>> > >>> config HAVE_IMA_KEXEC > >>> bool > >>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > >>> index bfe62d5..012acbe 100644 > >>> --- a/kernel/kexec_core.c > >>> +++ b/kernel/kexec_core.c > >>> @@ -38,6 +38,7 @@ > >>> #include > >>> #include > >>> #include > >>> +#include > >>> > >>> #include > >>> #include > >>> @@ -53,9 +54,10 @@ > >>> > >>> /* vmcoreinfo stuff */ > >>> static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; > >>> -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > >>> +static u32 vmcoreinfo_sig; > >>> size_t vmcoreinfo_size; > >>> size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); > >>> +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > >>> > >>> /* Flag to indicate we are going to kexec a new kernel */ > >>> bool kexec_in_progress = false; > >>> @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void) > >>> final_note(buf); > >>> } > >>> > >>> -void crash_save_vmcoreinfo(void) > >>> -{ > >>> -
[PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode
HDA mode fixed the issue by these two commits: '9476d369d7b3 ALSA: hda - Mute headphone pin on suspend on XPS13 9333' '3e1b0c4a9d56 ALSA: hda - Fix click noise at start on Dell XPS13' Apply the same workarounds to rt286 can solve the issue. When jack is plugged, it rapidly generates I2C interrupts, which triggers rt286_irq() and rt286_jack_detect(), which produces the click noise. alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop the frantic interrupts, hence avoids the click noise. When rt286 is under powersaving state, play a sound with headphone or plug a headphone in will produce a loud crack sound. Set AMP_OUT_MUTE before power events can make the noise less noticeable. Unmute the AMP when the power is up. v3: Implicit conversion instead of tenary operator. v2: Use 'HP Power' instead of individual power events. Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611 Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434 Signed-off-by: Kai-Heng Feng--- sound/soc/codecs/rt286.c | 48 +--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c index 9c365a7f758d..97b52697f974 100644 --- a/sound/soc/codecs/rt286.c +++ b/sound/soc/codecs/rt286.c @@ -36,6 +36,9 @@ #define RT286_VENDOR_ID 0x10ec0286 #define RT288_VENDOR_ID 0x10ec0288 +#define AMP_OUT_MUTE0xb080 +#define AMP_OUT_UNMUTE 0xb000 + struct rt286_priv { struct reg_default *index_cache; int index_cache_size; @@ -47,6 +50,7 @@ struct rt286_priv { struct delayed_work jack_detect_work; int sys_clk; int clk_id; + bool is_dell_dino; }; static const struct reg_default rt286_index_def[] = { @@ -472,6 +476,32 @@ static int rt286_set_dmic1_event(struct snd_soc_dapm_widget *w, return 0; } +/* Power event function to workaround headphone crack noise */ +static int rt286_hp_power_event(struct snd_soc_dapm_widget *w, +struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm); + struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec); + + if (!rt286->is_dell_dino) + return 0; + + switch (event) { + case SND_SOC_DAPM_PRE_PMD: + case SND_SOC_DAPM_POST_PMD: + case SND_SOC_DAPM_POST_PMU: + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); + break; + case SND_SOC_DAPM_PRE_PMU: + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_UNMUTE); + break; + default: + return 0; + } + + return 0; +} + static int rt286_ldo2_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { @@ -578,7 +608,9 @@ static const struct snd_soc_dapm_widget rt286_dapm_widgets[] = { SND_SOC_DAPM_MUX("HPO Mux", SND_SOC_NOPM, 0, 0, _hpo_mux), SND_SOC_DAPM_SUPPLY("HP Power", RT286_SET_PIN_HPO, - RT286_SET_PIN_SFT, 0, NULL, 0), + RT286_SET_PIN_SFT, 0, rt286_hp_power_event, + SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), /* Output Mixer */ SND_SOC_DAPM_MIXER("Front", RT286_SET_POWER(RT286_DAC_OUT1), 0, 1, @@ -1175,8 +1207,10 @@ static int rt286_i2c_probe(struct i2c_client *i2c, if (pdata) rt286->pdata = *pdata; + rt286->is_dell_dino = dmi_check_system(dmi_dell_dino); + if (dmi_check_system(force_combo_jack_table) || - dmi_check_system(dmi_dell_dino)) + rt286->is_dell_dino) rt286->pdata.cbj_en = true; regmap_write(rt286->regmap, RT286_SET_AUDIO_POWER, AC_PWRST_D3); @@ -1192,6 +1226,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c, regmap_update_bits(rt286->regmap, RT286_CBJ_CTRL1, 0xf000, 0xb000); } else { + /* Fix headphone click noise */ + if (rt286->is_dell_dino) + regmap_write(rt286->regmap, + RT286_MIC1_DET_CTRL, 0x0020); + regmap_update_bits(rt286->regmap, RT286_CBJ_CTRL1, 0xf000, 0x5000); } @@ -1215,7 +1254,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c, regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL3, 0xf777, 0x4737); regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL4, 0x00ff, 0x003f); - if (dmi_check_system(dmi_dell_dino)) { + if (rt286->is_dell_dino) { regmap_update_bits(rt286->regmap, RT286_SET_GPIO_MASK, 0x40, 0x40); regmap_update_bits(rt286->regmap, @@ -1224,6 +1263,9 @@ static int rt286_i2c_probe(struct i2c_client
[PATCH v3] ASoC: rt286: fix headphone click/crack noise on Dell XPS 9343 I2S mode
HDA mode fixed the issue by these two commits: '9476d369d7b3 ALSA: hda - Mute headphone pin on suspend on XPS13 9333' '3e1b0c4a9d56 ALSA: hda - Fix click noise at start on Dell XPS13' Apply the same workarounds to rt286 can solve the issue. When jack is plugged, it rapidly generates I2C interrupts, which triggers rt286_irq() and rt286_jack_detect(), which produces the click noise. alc_fixup_dell_xps13() in patch_realtek.c sets up a pin that can stop the frantic interrupts, hence avoids the click noise. When rt286 is under powersaving state, play a sound with headphone or plug a headphone in will produce a loud crack sound. Set AMP_OUT_MUTE before power events can make the noise less noticeable. Unmute the AMP when the power is up. v3: Implicit conversion instead of tenary operator. v2: Use 'HP Power' instead of individual power events. Link: https://bugzilla.kernel.org/show_bug.cgi?id=112611 Link: https://bugzilla.redhat.com/show_bug.cgi?id=1313434 Signed-off-by: Kai-Heng Feng --- sound/soc/codecs/rt286.c | 48 +--- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/sound/soc/codecs/rt286.c b/sound/soc/codecs/rt286.c index 9c365a7f758d..97b52697f974 100644 --- a/sound/soc/codecs/rt286.c +++ b/sound/soc/codecs/rt286.c @@ -36,6 +36,9 @@ #define RT286_VENDOR_ID 0x10ec0286 #define RT288_VENDOR_ID 0x10ec0288 +#define AMP_OUT_MUTE0xb080 +#define AMP_OUT_UNMUTE 0xb000 + struct rt286_priv { struct reg_default *index_cache; int index_cache_size; @@ -47,6 +50,7 @@ struct rt286_priv { struct delayed_work jack_detect_work; int sys_clk; int clk_id; + bool is_dell_dino; }; static const struct reg_default rt286_index_def[] = { @@ -472,6 +476,32 @@ static int rt286_set_dmic1_event(struct snd_soc_dapm_widget *w, return 0; } +/* Power event function to workaround headphone crack noise */ +static int rt286_hp_power_event(struct snd_soc_dapm_widget *w, +struct snd_kcontrol *kcontrol, int event) +{ + struct snd_soc_codec *codec = snd_soc_dapm_to_codec(w->dapm); + struct rt286_priv *rt286 = snd_soc_codec_get_drvdata(codec); + + if (!rt286->is_dell_dino) + return 0; + + switch (event) { + case SND_SOC_DAPM_PRE_PMD: + case SND_SOC_DAPM_POST_PMD: + case SND_SOC_DAPM_POST_PMU: + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_MUTE); + break; + case SND_SOC_DAPM_PRE_PMU: + snd_soc_write(codec, RT286_SET_AMP_GAIN_HPO, AMP_OUT_UNMUTE); + break; + default: + return 0; + } + + return 0; +} + static int rt286_ldo2_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event) { @@ -578,7 +608,9 @@ static const struct snd_soc_dapm_widget rt286_dapm_widgets[] = { SND_SOC_DAPM_MUX("HPO Mux", SND_SOC_NOPM, 0, 0, _hpo_mux), SND_SOC_DAPM_SUPPLY("HP Power", RT286_SET_PIN_HPO, - RT286_SET_PIN_SFT, 0, NULL, 0), + RT286_SET_PIN_SFT, 0, rt286_hp_power_event, + SND_SOC_DAPM_PRE_PMD | SND_SOC_DAPM_PRE_PMU | + SND_SOC_DAPM_POST_PMU | SND_SOC_DAPM_POST_PMD), /* Output Mixer */ SND_SOC_DAPM_MIXER("Front", RT286_SET_POWER(RT286_DAC_OUT1), 0, 1, @@ -1175,8 +1207,10 @@ static int rt286_i2c_probe(struct i2c_client *i2c, if (pdata) rt286->pdata = *pdata; + rt286->is_dell_dino = dmi_check_system(dmi_dell_dino); + if (dmi_check_system(force_combo_jack_table) || - dmi_check_system(dmi_dell_dino)) + rt286->is_dell_dino) rt286->pdata.cbj_en = true; regmap_write(rt286->regmap, RT286_SET_AUDIO_POWER, AC_PWRST_D3); @@ -1192,6 +1226,11 @@ static int rt286_i2c_probe(struct i2c_client *i2c, regmap_update_bits(rt286->regmap, RT286_CBJ_CTRL1, 0xf000, 0xb000); } else { + /* Fix headphone click noise */ + if (rt286->is_dell_dino) + regmap_write(rt286->regmap, + RT286_MIC1_DET_CTRL, 0x0020); + regmap_update_bits(rt286->regmap, RT286_CBJ_CTRL1, 0xf000, 0x5000); } @@ -1215,7 +1254,7 @@ static int rt286_i2c_probe(struct i2c_client *i2c, regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL3, 0xf777, 0x4737); regmap_update_bits(rt286->regmap, RT286_DEPOP_CTRL4, 0x00ff, 0x003f); - if (dmi_check_system(dmi_dell_dino)) { + if (rt286->is_dell_dino) { regmap_update_bits(rt286->regmap, RT286_SET_GPIO_MASK, 0x40, 0x40); regmap_update_bits(rt286->regmap, @@ -1224,6 +1263,9 @@ static int rt286_i2c_probe(struct i2c_client *i2c,
Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs
On 19-03-17, 14:34, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki> > The PELT metric used by the schedutil governor underestimates the > CPU utilization in some cases. The reason for that may be time spent > in interrupt handlers and similar which is not accounted for by PELT. > > That can be easily demonstrated by running kernel compilation on > a Sandy Bridge Intel processor, running turbostat in parallel with > it and looking at the values written to the MSR_IA32_PERF_CTL > register. Namely, the expected result would be that when all CPUs > were 100% busy, all of them would be requested to run in the maximum > P-state, but observation shows that this clearly isn't the case. > The CPUs run in the maximum P-state for a while and then are > requested to run slower and go back to the maximum P-state after > a while again. That causes the actual frequency of the processor to > visibly oscillate below the sustainable maximum in a jittery fashion > which clearly is not desirable. > > To work around this issue use the observation that, from the > schedutil governor's perspective, CPUs that are never idle should > always run at the maximum frequency and make that happen. > > To that end, add a counter of idle calls to struct sugov_cpu and > modify cpuidle_idle_call() to increment that counter every time it > is about to put the given CPU into an idle state. Next, make the > schedutil governor look at that counter for the current CPU every > time before it is about to start heavy computations. If the counter > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms), > the CPU has not been idle for at least that long and the governor > will choose the maximum frequency for it without looking at the PELT > metric at all. Looks like we are fixing a PELT problem with a schedutil Hack :) > Signed-off-by: Rafael J. Wysocki > --- > include/linux/sched/cpufreq.h|6 ++ > kernel/sched/cpufreq_schedutil.c | 38 > -- > kernel/sched/idle.c |3 +++ > 3 files changed, 45 insertions(+), 2 deletions(-) > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c > === > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c > +++ linux-pm/kernel/sched/cpufreq_schedutil.c > @@ -20,6 +20,7 @@ > #include "sched.h" > > #define SUGOV_KTHREAD_PRIORITY 50 > +#define SUGOV_BUSY_THRESHOLD (50 * NSEC_PER_MSEC) > > struct sugov_tunables { > struct gov_attr_set attr_set; > @@ -55,6 +56,9 @@ struct sugov_cpu { > > unsigned long iowait_boost; > unsigned long iowait_boost_max; > + unsigned long idle_calls; > + unsigned long saved_idle_calls; > + u64 busy_start; > u64 last_update; > > /* The fields below are only needed when sharing a policy. */ > @@ -192,6 +196,34 @@ static void sugov_iowait_boost(struct su > sg_cpu->iowait_boost >>= 1; > } > > +void cpufreq_schedutil_idle_call(void) > +{ > + struct sugov_cpu *sg_cpu = this_cpu_ptr(_cpu); > + > + sg_cpu->idle_calls++; > +} > + > +static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) > +{ > + if (sg_cpu->idle_calls != sg_cpu->saved_idle_calls) { > + sg_cpu->busy_start = 0; > + return false; > + } > + > + if (!sg_cpu->busy_start) { > + sg_cpu->busy_start = sg_cpu->last_update; > + return false; > + } > + > + return sg_cpu->last_update - sg_cpu->busy_start > SUGOV_BUSY_THRESHOLD; > +} > + > +static void sugov_save_idle_calls(struct sugov_cpu *sg_cpu) > +{ > + if (!sg_cpu->busy_start) > + sg_cpu->saved_idle_calls = sg_cpu->idle_calls; Why aren't we doing this in sugov_cpu_is_busy() itself ? And isn't it possible for idle_calls to get incremented by this time? > +} > + > static void sugov_update_single(struct update_util_data *hook, u64 time, > unsigned int flags) > { > @@ -207,7 +239,7 @@ static void sugov_update_single(struct u > if (!sugov_should_update_freq(sg_policy, time)) > return; > > - if (flags & SCHED_CPUFREQ_RT_DL) { > + if ((flags & SCHED_CPUFREQ_RT_DL) || sugov_cpu_is_busy(sg_cpu)) { > next_f = policy->cpuinfo.max_freq; > } else { > sugov_get_util(, ); > @@ -215,6 +247,7 @@ static void sugov_update_single(struct u > next_f = get_next_freq(sg_policy, util, max); > } > sugov_update_commit(sg_policy, time, next_f); > + sugov_save_idle_calls(sg_cpu); > } > > static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu) > @@ -278,12 +311,13 @@ static void sugov_update_shared(struct u > sg_cpu->last_update = time; > > if (sugov_should_update_freq(sg_policy, time)) { > - if (flags & SCHED_CPUFREQ_RT_DL) > + if ((flags & SCHED_CPUFREQ_RT_DL) ||
Re: [RFC][PATCH 2/2] cpufreq: schedutil: Force max frequency on busy CPUs
On 19-03-17, 14:34, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > The PELT metric used by the schedutil governor underestimates the > CPU utilization in some cases. The reason for that may be time spent > in interrupt handlers and similar which is not accounted for by PELT. > > That can be easily demonstrated by running kernel compilation on > a Sandy Bridge Intel processor, running turbostat in parallel with > it and looking at the values written to the MSR_IA32_PERF_CTL > register. Namely, the expected result would be that when all CPUs > were 100% busy, all of them would be requested to run in the maximum > P-state, but observation shows that this clearly isn't the case. > The CPUs run in the maximum P-state for a while and then are > requested to run slower and go back to the maximum P-state after > a while again. That causes the actual frequency of the processor to > visibly oscillate below the sustainable maximum in a jittery fashion > which clearly is not desirable. > > To work around this issue use the observation that, from the > schedutil governor's perspective, CPUs that are never idle should > always run at the maximum frequency and make that happen. > > To that end, add a counter of idle calls to struct sugov_cpu and > modify cpuidle_idle_call() to increment that counter every time it > is about to put the given CPU into an idle state. Next, make the > schedutil governor look at that counter for the current CPU every > time before it is about to start heavy computations. If the counter > has not changed for over SUGOV_BUSY_THRESHOLD time (equal to 50 ms), > the CPU has not been idle for at least that long and the governor > will choose the maximum frequency for it without looking at the PELT > metric at all. Looks like we are fixing a PELT problem with a schedutil Hack :) > Signed-off-by: Rafael J. Wysocki > --- > include/linux/sched/cpufreq.h|6 ++ > kernel/sched/cpufreq_schedutil.c | 38 > -- > kernel/sched/idle.c |3 +++ > 3 files changed, 45 insertions(+), 2 deletions(-) > > Index: linux-pm/kernel/sched/cpufreq_schedutil.c > === > --- linux-pm.orig/kernel/sched/cpufreq_schedutil.c > +++ linux-pm/kernel/sched/cpufreq_schedutil.c > @@ -20,6 +20,7 @@ > #include "sched.h" > > #define SUGOV_KTHREAD_PRIORITY 50 > +#define SUGOV_BUSY_THRESHOLD (50 * NSEC_PER_MSEC) > > struct sugov_tunables { > struct gov_attr_set attr_set; > @@ -55,6 +56,9 @@ struct sugov_cpu { > > unsigned long iowait_boost; > unsigned long iowait_boost_max; > + unsigned long idle_calls; > + unsigned long saved_idle_calls; > + u64 busy_start; > u64 last_update; > > /* The fields below are only needed when sharing a policy. */ > @@ -192,6 +196,34 @@ static void sugov_iowait_boost(struct su > sg_cpu->iowait_boost >>= 1; > } > > +void cpufreq_schedutil_idle_call(void) > +{ > + struct sugov_cpu *sg_cpu = this_cpu_ptr(_cpu); > + > + sg_cpu->idle_calls++; > +} > + > +static bool sugov_cpu_is_busy(struct sugov_cpu *sg_cpu) > +{ > + if (sg_cpu->idle_calls != sg_cpu->saved_idle_calls) { > + sg_cpu->busy_start = 0; > + return false; > + } > + > + if (!sg_cpu->busy_start) { > + sg_cpu->busy_start = sg_cpu->last_update; > + return false; > + } > + > + return sg_cpu->last_update - sg_cpu->busy_start > SUGOV_BUSY_THRESHOLD; > +} > + > +static void sugov_save_idle_calls(struct sugov_cpu *sg_cpu) > +{ > + if (!sg_cpu->busy_start) > + sg_cpu->saved_idle_calls = sg_cpu->idle_calls; Why aren't we doing this in sugov_cpu_is_busy() itself ? And isn't it possible for idle_calls to get incremented by this time? > +} > + > static void sugov_update_single(struct update_util_data *hook, u64 time, > unsigned int flags) > { > @@ -207,7 +239,7 @@ static void sugov_update_single(struct u > if (!sugov_should_update_freq(sg_policy, time)) > return; > > - if (flags & SCHED_CPUFREQ_RT_DL) { > + if ((flags & SCHED_CPUFREQ_RT_DL) || sugov_cpu_is_busy(sg_cpu)) { > next_f = policy->cpuinfo.max_freq; > } else { > sugov_get_util(, ); > @@ -215,6 +247,7 @@ static void sugov_update_single(struct u > next_f = get_next_freq(sg_policy, util, max); > } > sugov_update_commit(sg_policy, time, next_f); > + sugov_save_idle_calls(sg_cpu); > } > > static unsigned int sugov_next_freq_shared(struct sugov_cpu *sg_cpu) > @@ -278,12 +311,13 @@ static void sugov_update_shared(struct u > sg_cpu->last_update = time; > > if (sugov_should_update_freq(sg_policy, time)) { > - if (flags & SCHED_CPUFREQ_RT_DL) > + if ((flags & SCHED_CPUFREQ_RT_DL) || sugov_cpu_is_busy(sg_cpu)) What about others CPUs in this policy?
Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'
Thanks Masami for detailed review. Please see my comments below. On Friday 17 March 2017 02:35 PM, Masami Hiramatsu wrote: > Hi Ravi, > > (I avoided to review parser part since it may go to yacc in next version) > > On Tue, 14 Mar 2017 20:36:54 +0530 > Ravi Bangoriawrote: > > [SNIP] >> @@ -1516,9 +1534,10 @@ static bool dry_run; >> * using pipes, etc. >> */ >> static struct option __record_options[] = { >> -OPT_CALLBACK('e', "event", , "event", >> - "event selector. use 'perf list' to list available events", >> - parse_events_option), >> +OPT_CALLBACK_ARG('e', "event", , >> + _event_list, "event", >> + "event selector. use 'perf list' to list available >> events", >> + record__parse_events_option), > Does --event option NOT requires argument without this patch? > If it should be changed to use OPT_CALLBACK_ARG(), would it be > better merge this part to previous patch? Ok. Yes, it does. I think macro name is confusing. This new macro allows passing of extra data from builtin-*.c to libelf. One such macro already exists (OPT_CALLBACK_OPTARG), but the argument is optional for it and thus it ignores the argument. I need a macro in which argument is necessary and it also allows to pass extra data. Hence, I introduced this macro. Will change macro to OPT_CALLBACK_ARGDATA. Please suggest if you have better name. > [SNIP] >> +/* >> + * Delete the SDT events from uprobe_events file that >> + * were created initially. >> + */ >> +void remove_sdt_event_list(struct list_head *sdt_events) >> +{ >> +struct sdt_event_list *sdt_event; >> +struct strfilter *filter = NULL; >> +const char *err = NULL; >> + >> +if (list_empty(sdt_events)) >> +return; >> + >> +list_for_each_entry(sdt_event, sdt_events, list) { >> +if (!filter) { >> +filter = strfilter__new(sdt_event->name, ); >> +if (!filter) >> +goto free_list; > Don't we need to return error code for this case? > >> +} else { >> +strfilter__or(filter, sdt_event->name, ); > strfilter__or() can fail here. > >> +} >> +} >> + >> +del_perf_probe_events(filter); > Here too, if it is ignored silently by design, please comment it here. Sure. Will think about handling errors in this function. > >> + >> +free_list: >> +free_sdt_list(sdt_events); >> +} >> + >> +static int get_sdt_events_from_cache(struct perf_probe_event *pev) >> +{ >> +int ret = 0; >> + >> +pev->ntevs = find_cached_events_all(pev, >tevs); >> + >> +if (pev->ntevs < 0) { >> +pr_err("Error: Cache lookup failed (code: %d)\n", pev->ntevs); >> +ret = pev->ntevs; >> +} else if (!pev->ntevs) { >> +pr_err("Error: %s:%s not found in the cache\n", >> +pev->group, pev->event); >> +ret = -EINVAL; >> +} else if (pev->ntevs > 1) { >> +pr_warning("Warning : Recording on %d occurences of %s:%s\n", >> + pev->ntevs, pev->group, pev->event); >> +} >> + >> +return ret; >> +} >> + >> +static int add_event_to_sdt_evlist(struct probe_trace_event *tev, >> + struct list_head *sdt_evlist) >> +{ >> +struct sdt_event_list *tmp; > Well, strbuf can make this simpler as below ;-) > > struct strbuf buf = STRBUF_INIT; Sure, will do it. Thanks :) Ravi >> + >> +tmp = zalloc(sizeof(*tmp)); >> +if (!tmp) >> +return -ENOMEM; >> + >> +INIT_LIST_HEAD(>list); > if (strbuf_addf(, "%s:%s", tev->group, tev->event)) > goto error; > > tmp->name = strbuf_detach(); > >> +list_add(>list, sdt_evlist); >> + >> +return 0; > error: > free(tmp); > > return -ENOMEM; >> +} >> + >> +static int add_events_to_sdt_evlist(struct perf_probe_event *pev, >> +struct list_head *sdt_evlist) >> +{ >> +int i, ret; >> + >> +for (i = 0; i < pev->ntevs; i++) { >> +ret = add_event_to_sdt_evlist(>tevs[i], sdt_evlist); >> + >> +if (ret < 0) >> +return ret; >> +} >> +return 0; >> +} > > Thanks, >
Re: [PATCH v5 3/7] perf/sdt: Directly record SDT events with 'perf record'
Thanks Masami for detailed review. Please see my comments below. On Friday 17 March 2017 02:35 PM, Masami Hiramatsu wrote: > Hi Ravi, > > (I avoided to review parser part since it may go to yacc in next version) > > On Tue, 14 Mar 2017 20:36:54 +0530 > Ravi Bangoria wrote: > > [SNIP] >> @@ -1516,9 +1534,10 @@ static bool dry_run; >> * using pipes, etc. >> */ >> static struct option __record_options[] = { >> -OPT_CALLBACK('e', "event", , "event", >> - "event selector. use 'perf list' to list available events", >> - parse_events_option), >> +OPT_CALLBACK_ARG('e', "event", , >> + _event_list, "event", >> + "event selector. use 'perf list' to list available >> events", >> + record__parse_events_option), > Does --event option NOT requires argument without this patch? > If it should be changed to use OPT_CALLBACK_ARG(), would it be > better merge this part to previous patch? Ok. Yes, it does. I think macro name is confusing. This new macro allows passing of extra data from builtin-*.c to libelf. One such macro already exists (OPT_CALLBACK_OPTARG), but the argument is optional for it and thus it ignores the argument. I need a macro in which argument is necessary and it also allows to pass extra data. Hence, I introduced this macro. Will change macro to OPT_CALLBACK_ARGDATA. Please suggest if you have better name. > [SNIP] >> +/* >> + * Delete the SDT events from uprobe_events file that >> + * were created initially. >> + */ >> +void remove_sdt_event_list(struct list_head *sdt_events) >> +{ >> +struct sdt_event_list *sdt_event; >> +struct strfilter *filter = NULL; >> +const char *err = NULL; >> + >> +if (list_empty(sdt_events)) >> +return; >> + >> +list_for_each_entry(sdt_event, sdt_events, list) { >> +if (!filter) { >> +filter = strfilter__new(sdt_event->name, ); >> +if (!filter) >> +goto free_list; > Don't we need to return error code for this case? > >> +} else { >> +strfilter__or(filter, sdt_event->name, ); > strfilter__or() can fail here. > >> +} >> +} >> + >> +del_perf_probe_events(filter); > Here too, if it is ignored silently by design, please comment it here. Sure. Will think about handling errors in this function. > >> + >> +free_list: >> +free_sdt_list(sdt_events); >> +} >> + >> +static int get_sdt_events_from_cache(struct perf_probe_event *pev) >> +{ >> +int ret = 0; >> + >> +pev->ntevs = find_cached_events_all(pev, >tevs); >> + >> +if (pev->ntevs < 0) { >> +pr_err("Error: Cache lookup failed (code: %d)\n", pev->ntevs); >> +ret = pev->ntevs; >> +} else if (!pev->ntevs) { >> +pr_err("Error: %s:%s not found in the cache\n", >> +pev->group, pev->event); >> +ret = -EINVAL; >> +} else if (pev->ntevs > 1) { >> +pr_warning("Warning : Recording on %d occurences of %s:%s\n", >> + pev->ntevs, pev->group, pev->event); >> +} >> + >> +return ret; >> +} >> + >> +static int add_event_to_sdt_evlist(struct probe_trace_event *tev, >> + struct list_head *sdt_evlist) >> +{ >> +struct sdt_event_list *tmp; > Well, strbuf can make this simpler as below ;-) > > struct strbuf buf = STRBUF_INIT; Sure, will do it. Thanks :) Ravi >> + >> +tmp = zalloc(sizeof(*tmp)); >> +if (!tmp) >> +return -ENOMEM; >> + >> +INIT_LIST_HEAD(>list); > if (strbuf_addf(, "%s:%s", tev->group, tev->event)) > goto error; > > tmp->name = strbuf_detach(); > >> +list_add(>list, sdt_evlist); >> + >> +return 0; > error: > free(tmp); > > return -ENOMEM; >> +} >> + >> +static int add_events_to_sdt_evlist(struct perf_probe_event *pev, >> +struct list_head *sdt_evlist) >> +{ >> +int i, ret; >> + >> +for (i = 0; i < pev->ntevs; i++) { >> +ret = add_event_to_sdt_evlist(>tevs[i], sdt_evlist); >> + >> +if (ret < 0) >> +return ret; >> +} >> +return 0; >> +} > > Thanks, >
[PATCH v6] drm/rockchip: Refactor the component match logic.
Currently we are adding all components from the dts, if one of their drivers been disabled, we would not be able to bring up others. Refactor component match logic, follow exynos drm. Signed-off-by: Jeffy ChenReviewed-by: Andrzej Hajda Acked-by: Mark Yao --- Changes in v6: Add Mark Yao 's Acked-by. Changes in v5: Fix compile error reported by Heiko Stuebner . Changes in v4: Use platform_driver helpers to register/unregister drivers. Fix null pointer error reported by Heiko Stuebner . Changes in v3: Address Andrzej Hajda 's comments. Changes in v2: Address Sean Paul 's comments. drivers/gpu/drm/rockchip/Kconfig| 10 +- drivers/gpu/drm/rockchip/Makefile | 16 +-- drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 9 +- drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 8 +- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 10 +- drivers/gpu/drm/rockchip/inno_hdmi.c| 10 +- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 138 +++- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 6 ++ drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 8 +- 10 files changed, 115 insertions(+), 108 deletions(-) diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig index 0e4eb84..50c41c0 100644 --- a/drivers/gpu/drm/rockchip/Kconfig +++ b/drivers/gpu/drm/rockchip/Kconfig @@ -13,7 +13,7 @@ config DRM_ROCKCHIP IP found on the SoC. config ROCKCHIP_ANALOGIX_DP - tristate "Rockchip specific extensions for Analogix DP driver" + bool "Rockchip specific extensions for Analogix DP driver" depends on DRM_ROCKCHIP select DRM_ANALOGIX_DP help @@ -22,7 +22,7 @@ config ROCKCHIP_ANALOGIX_DP on RK3288 based SoC, you should selet this option. config ROCKCHIP_CDN_DP -tristate "Rockchip cdn DP" +bool "Rockchip cdn DP" depends on DRM_ROCKCHIP depends on EXTCON select SND_SOC_HDMI_CODEC if SND_SOC @@ -33,7 +33,7 @@ config ROCKCHIP_CDN_DP option. config ROCKCHIP_DW_HDMI -tristate "Rockchip specific extensions for Synopsys DW HDMI" +bool "Rockchip specific extensions for Synopsys DW HDMI" depends on DRM_ROCKCHIP select DRM_DW_HDMI help @@ -43,7 +43,7 @@ config ROCKCHIP_DW_HDMI option. config ROCKCHIP_DW_MIPI_DSI - tristate "Rockchip specific extensions for Synopsys DW MIPI DSI" + bool "Rockchip specific extensions for Synopsys DW MIPI DSI" depends on DRM_ROCKCHIP select DRM_MIPI_DSI help @@ -53,7 +53,7 @@ config ROCKCHIP_DW_MIPI_DSI option. config ROCKCHIP_INNO_HDMI - tristate "Rockchip specific extensions for Innosilicon HDMI" + bool "Rockchip specific extensions for Innosilicon HDMI" depends on DRM_ROCKCHIP help This selects support for Rockchip SoC specific extensions diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile index c931e2a..fa8dc9d 100644 --- a/drivers/gpu/drm/rockchip/Makefile +++ b/drivers/gpu/drm/rockchip/Makefile @@ -3,14 +3,14 @@ # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \ - rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o + rockchip_drm_gem.o rockchip_drm_psr.o \ + rockchip_drm_vop.o rockchip_vop_reg.o rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o -obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o -obj-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp.o -cdn-dp-objs := cdn-dp-core.o cdn-dp-reg.o -obj-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o -obj-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o -obj-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o +rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o +rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o +rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o +rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o +rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o -obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o rockchip_vop_reg.o +obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index 8548e82..91ebe5c 100644 --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -507,7 +507,7 @@ static const struct of_device_id rockchip_dp_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids); -static struct platform_driver rockchip_dp_driver = { +struct platform_driver rockchip_dp_driver = {
[PATCH v6] drm/rockchip: Refactor the component match logic.
Currently we are adding all components from the dts, if one of their drivers been disabled, we would not be able to bring up others. Refactor component match logic, follow exynos drm. Signed-off-by: Jeffy Chen Reviewed-by: Andrzej Hajda Acked-by: Mark Yao --- Changes in v6: Add Mark Yao 's Acked-by. Changes in v5: Fix compile error reported by Heiko Stuebner . Changes in v4: Use platform_driver helpers to register/unregister drivers. Fix null pointer error reported by Heiko Stuebner . Changes in v3: Address Andrzej Hajda 's comments. Changes in v2: Address Sean Paul 's comments. drivers/gpu/drm/rockchip/Kconfig| 10 +- drivers/gpu/drm/rockchip/Makefile | 16 +-- drivers/gpu/drm/rockchip/analogix_dp-rockchip.c | 9 +- drivers/gpu/drm/rockchip/cdn-dp-core.c | 8 +- drivers/gpu/drm/rockchip/dw-mipi-dsi.c | 8 +- drivers/gpu/drm/rockchip/dw_hdmi-rockchip.c | 10 +- drivers/gpu/drm/rockchip/inno_hdmi.c| 10 +- drivers/gpu/drm/rockchip/rockchip_drm_drv.c | 138 +++- drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 6 ++ drivers/gpu/drm/rockchip/rockchip_vop_reg.c | 8 +- 10 files changed, 115 insertions(+), 108 deletions(-) diff --git a/drivers/gpu/drm/rockchip/Kconfig b/drivers/gpu/drm/rockchip/Kconfig index 0e4eb84..50c41c0 100644 --- a/drivers/gpu/drm/rockchip/Kconfig +++ b/drivers/gpu/drm/rockchip/Kconfig @@ -13,7 +13,7 @@ config DRM_ROCKCHIP IP found on the SoC. config ROCKCHIP_ANALOGIX_DP - tristate "Rockchip specific extensions for Analogix DP driver" + bool "Rockchip specific extensions for Analogix DP driver" depends on DRM_ROCKCHIP select DRM_ANALOGIX_DP help @@ -22,7 +22,7 @@ config ROCKCHIP_ANALOGIX_DP on RK3288 based SoC, you should selet this option. config ROCKCHIP_CDN_DP -tristate "Rockchip cdn DP" +bool "Rockchip cdn DP" depends on DRM_ROCKCHIP depends on EXTCON select SND_SOC_HDMI_CODEC if SND_SOC @@ -33,7 +33,7 @@ config ROCKCHIP_CDN_DP option. config ROCKCHIP_DW_HDMI -tristate "Rockchip specific extensions for Synopsys DW HDMI" +bool "Rockchip specific extensions for Synopsys DW HDMI" depends on DRM_ROCKCHIP select DRM_DW_HDMI help @@ -43,7 +43,7 @@ config ROCKCHIP_DW_HDMI option. config ROCKCHIP_DW_MIPI_DSI - tristate "Rockchip specific extensions for Synopsys DW MIPI DSI" + bool "Rockchip specific extensions for Synopsys DW MIPI DSI" depends on DRM_ROCKCHIP select DRM_MIPI_DSI help @@ -53,7 +53,7 @@ config ROCKCHIP_DW_MIPI_DSI option. config ROCKCHIP_INNO_HDMI - tristate "Rockchip specific extensions for Innosilicon HDMI" + bool "Rockchip specific extensions for Innosilicon HDMI" depends on DRM_ROCKCHIP help This selects support for Rockchip SoC specific extensions diff --git a/drivers/gpu/drm/rockchip/Makefile b/drivers/gpu/drm/rockchip/Makefile index c931e2a..fa8dc9d 100644 --- a/drivers/gpu/drm/rockchip/Makefile +++ b/drivers/gpu/drm/rockchip/Makefile @@ -3,14 +3,14 @@ # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher. rockchipdrm-y := rockchip_drm_drv.o rockchip_drm_fb.o \ - rockchip_drm_gem.o rockchip_drm_psr.o rockchip_drm_vop.o + rockchip_drm_gem.o rockchip_drm_psr.o \ + rockchip_drm_vop.o rockchip_vop_reg.o rockchipdrm-$(CONFIG_DRM_FBDEV_EMULATION) += rockchip_drm_fbdev.o -obj-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o -obj-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp.o -cdn-dp-objs := cdn-dp-core.o cdn-dp-reg.o -obj-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o -obj-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o -obj-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o +rockchipdrm-$(CONFIG_ROCKCHIP_ANALOGIX_DP) += analogix_dp-rockchip.o +rockchipdrm-$(CONFIG_ROCKCHIP_CDN_DP) += cdn-dp-core.o cdn-dp-reg.o +rockchipdrm-$(CONFIG_ROCKCHIP_DW_HDMI) += dw_hdmi-rockchip.o +rockchipdrm-$(CONFIG_ROCKCHIP_DW_MIPI_DSI) += dw-mipi-dsi.o +rockchipdrm-$(CONFIG_ROCKCHIP_INNO_HDMI) += inno_hdmi.o -obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o rockchip_vop_reg.o +obj-$(CONFIG_DRM_ROCKCHIP) += rockchipdrm.o diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c index 8548e82..91ebe5c 100644 --- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c +++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c @@ -507,7 +507,7 @@ static const struct of_device_id rockchip_dp_dt_ids[] = { }; MODULE_DEVICE_TABLE(of, rockchip_dp_dt_ids); -static struct platform_driver rockchip_dp_driver = { +struct platform_driver rockchip_dp_driver = { .probe = rockchip_dp_probe, .remove = rockchip_dp_remove, .driver = { @@ -516,10 +516,3 @@ static struct platform_driver rockchip_dp_driver = {
Re: [PATCH 1/2] cpufreq: schedutil: Fix per-CPU structure initialization in sugov_start()
On 19-03-17, 14:30, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki> > sugov_start() only initializes struct sugov_cpu per-CPU structures > for shared policies, but it should do that for single-CPU policies too. > > That in particular makes the IO-wait boost mechanism work in the > cases when cpufreq policies correspond to individual CPUs. > > Fixes: 21ca6d2c52f8 (cpufreq: schedutil: Add iowait boosting) > Signed-off-by: Rafael J. Wysocki Cc: 4.9+ # 4.9+ Acked-by: Viresh Kumar -- viresh
Re: [PATCH 1/2] cpufreq: schedutil: Fix per-CPU structure initialization in sugov_start()
On 19-03-17, 14:30, Rafael J. Wysocki wrote: > From: Rafael J. Wysocki > > sugov_start() only initializes struct sugov_cpu per-CPU structures > for shared policies, but it should do that for single-CPU policies too. > > That in particular makes the IO-wait boost mechanism work in the > cases when cpufreq policies correspond to individual CPUs. > > Fixes: 21ca6d2c52f8 (cpufreq: schedutil: Add iowait boosting) > Signed-off-by: Rafael J. Wysocki Cc: 4.9+ # 4.9+ Acked-by: Viresh Kumar -- viresh
Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online
On 17-03-17, 18:40, Rafael J. Wysocki wrote: > On Fri, Mar 17, 2017 at 5:43 PM, Viresh Kumarwrote: > > On 17 March 2017 at 22:01, Rafael J. Wysocki wrote: > > > >> IMO if we are not going to restore the governor, we also should not > >> restore the limits as those things are related. Now, the governor can > >> be unloaded while the CPU is offline. > > > > I thought about it earlier but then governor and policy min/max > > looked independent to me. Why do you think they are related? > > They are parts of one set of settings. > > If the governor is not restored, the policy starts with the default > one, so why would it not start with the default limits then? Do we reset the limits when we change governor's normally? No. Then why should we consider suspend/resume special in that sense? These are completely different and independent settings which user has done and we don't really need to relate them. > My opinion is that either we restore everything the way it was, or we > start afresh entirely. What about fields like: policy->user_policy.*? They aren't reset for existing policies if the last governor isn't found. And there are drivers which call cpufreq_update_policy(), and that would mean that the CPU will come back to user defined policies before system suspended. And that kind of defeats whatever you were trying to do in this patch. Isn't it? -- viresh
Re: [PATCH] cpufreq: Restore policy min/max limits on CPU online
On 17-03-17, 18:40, Rafael J. Wysocki wrote: > On Fri, Mar 17, 2017 at 5:43 PM, Viresh Kumar wrote: > > On 17 March 2017 at 22:01, Rafael J. Wysocki wrote: > > > >> IMO if we are not going to restore the governor, we also should not > >> restore the limits as those things are related. Now, the governor can > >> be unloaded while the CPU is offline. > > > > I thought about it earlier but then governor and policy min/max > > looked independent to me. Why do you think they are related? > > They are parts of one set of settings. > > If the governor is not restored, the policy starts with the default > one, so why would it not start with the default limits then? Do we reset the limits when we change governor's normally? No. Then why should we consider suspend/resume special in that sense? These are completely different and independent settings which user has done and we don't really need to relate them. > My opinion is that either we restore everything the way it was, or we > start afresh entirely. What about fields like: policy->user_policy.*? They aren't reset for existing policies if the last governor isn't found. And there are drivers which call cpufreq_update_policy(), and that would mean that the CPU will come back to user defined policies before system suspended. And that kind of defeats whatever you were trying to do in this patch. Isn't it? -- viresh
Re: Still OOM problems with 4.9er/4.10er kernels
On Sun, 2017-03-19 at 17:02 +0100, Gerhard Wiesinger wrote: > mount | grep cgroup Just because controllers are mounted doesn't mean they're populated. To check that, you want to look for directories under the mount points with a non-empty 'tasks'. You will find some, but memory cgroup assignments would likely be most interesting for this thread. You can eliminate any diddling there by booting with cgroup_disable=memory. -Mike
Re: Still OOM problems with 4.9er/4.10er kernels
On Sun, 2017-03-19 at 17:02 +0100, Gerhard Wiesinger wrote: > mount | grep cgroup Just because controllers are mounted doesn't mean they're populated. To check that, you want to look for directories under the mount points with a non-empty 'tasks'. You will find some, but memory cgroup assignments would likely be most interesting for this thread. You can eliminate any diddling there by booting with cgroup_disable=memory. -Mike
[PATCH v2] drm/bridge: dw_hdmi: support i2c extended read mode
"I2C Master Interface Extended Read Mode" implements a segment pointer-based read operation using the Special Register configuration. This patch fix https://patchwork.kernel.org/patch/7098101/ mentioned "The current implementation does not support "I2C Master Interface Extended Read Mode" to read data addressed by non-zero segment pointer, this means that if EDID has more than 1 extension blocks" With this patch,dw-hdmi can read EDID data with 1/2/4 blocks. Signed-off-by: Nickey YangReviewed-by: Douglas Anderson --- drivers/gpu/drm/bridge/dw-hdmi.c | 38 -- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 9a9ec27..3b93655 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -32,6 +32,7 @@ #include "dw-hdmi.h" #include "dw-hdmi-audio.h" +#define DDC_SEGMENT_ADDR 0x30 #define HDMI_EDID_LEN 512 #define RGB0 @@ -111,6 +112,7 @@ struct dw_hdmi_i2c { u8 slave_reg; boolis_regaddr; + boolis_segment; }; struct dw_hdmi_phy_data { @@ -258,8 +260,12 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi, reinit_completion(>cmp); hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS); - hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ, - HDMI_I2CM_OPERATION); + if (i2c->is_segment) + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ_EXT, + HDMI_I2CM_OPERATION); + else + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ, + HDMI_I2CM_OPERATION); stat = wait_for_completion_timeout(>cmp, HZ / 10); if (!stat) @@ -271,6 +277,7 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi, *buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI); } + i2c->is_segment = false; return 0; } @@ -320,12 +327,6 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr); for (i = 0; i < num; i++) { - if (msgs[i].addr != addr) { - dev_warn(hdmi->dev, -"unsupported transfer, changed slave address\n"); - return -EOPNOTSUPP; - } - if (msgs[i].len == 0) { dev_dbg(hdmi->dev, "unsupported transfer %d/%d, no data\n", @@ -345,15 +346,24 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, /* Set slave device register address on transfer */ i2c->is_regaddr = false; + /* Set segment pointer for I2C extended read mode operation */ + i2c->is_segment = false; + for (i = 0; i < num; i++) { dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: %#x\n", i + 1, num, msgs[i].len, msgs[i].flags); - - if (msgs[i].flags & I2C_M_RD) - ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len); - else - ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len); - + if (msgs[i].addr == DDC_SEGMENT_ADDR && msgs[i].len == 1) { + i2c->is_segment = true; + hdmi_writeb(hdmi, DDC_SEGMENT_ADDR, HDMI_I2CM_SEGADDR); + hdmi_writeb(hdmi, *msgs[i].buf, HDMI_I2CM_SEGPTR); + } else { + if (msgs[i].flags & I2C_M_RD) + ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, + msgs[i].len); + else + ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, + msgs[i].len); + } if (ret < 0) break; } -- 1.9.1
[PATCH v2] drm/bridge: dw_hdmi: support i2c extended read mode
"I2C Master Interface Extended Read Mode" implements a segment pointer-based read operation using the Special Register configuration. This patch fix https://patchwork.kernel.org/patch/7098101/ mentioned "The current implementation does not support "I2C Master Interface Extended Read Mode" to read data addressed by non-zero segment pointer, this means that if EDID has more than 1 extension blocks" With this patch,dw-hdmi can read EDID data with 1/2/4 blocks. Signed-off-by: Nickey Yang Reviewed-by: Douglas Anderson --- drivers/gpu/drm/bridge/dw-hdmi.c | 38 -- 1 file changed, 24 insertions(+), 14 deletions(-) diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 9a9ec27..3b93655 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -32,6 +32,7 @@ #include "dw-hdmi.h" #include "dw-hdmi-audio.h" +#define DDC_SEGMENT_ADDR 0x30 #define HDMI_EDID_LEN 512 #define RGB0 @@ -111,6 +112,7 @@ struct dw_hdmi_i2c { u8 slave_reg; boolis_regaddr; + boolis_segment; }; struct dw_hdmi_phy_data { @@ -258,8 +260,12 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi, reinit_completion(>cmp); hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS); - hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ, - HDMI_I2CM_OPERATION); + if (i2c->is_segment) + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ_EXT, + HDMI_I2CM_OPERATION); + else + hdmi_writeb(hdmi, HDMI_I2CM_OPERATION_READ, + HDMI_I2CM_OPERATION); stat = wait_for_completion_timeout(>cmp, HZ / 10); if (!stat) @@ -271,6 +277,7 @@ static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi, *buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI); } + i2c->is_segment = false; return 0; } @@ -320,12 +327,6 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, dev_dbg(hdmi->dev, "xfer: num: %d, addr: %#x\n", num, addr); for (i = 0; i < num; i++) { - if (msgs[i].addr != addr) { - dev_warn(hdmi->dev, -"unsupported transfer, changed slave address\n"); - return -EOPNOTSUPP; - } - if (msgs[i].len == 0) { dev_dbg(hdmi->dev, "unsupported transfer %d/%d, no data\n", @@ -345,15 +346,24 @@ static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap, /* Set slave device register address on transfer */ i2c->is_regaddr = false; + /* Set segment pointer for I2C extended read mode operation */ + i2c->is_segment = false; + for (i = 0; i < num; i++) { dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: %#x\n", i + 1, num, msgs[i].len, msgs[i].flags); - - if (msgs[i].flags & I2C_M_RD) - ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len); - else - ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len); - + if (msgs[i].addr == DDC_SEGMENT_ADDR && msgs[i].len == 1) { + i2c->is_segment = true; + hdmi_writeb(hdmi, DDC_SEGMENT_ADDR, HDMI_I2CM_SEGADDR); + hdmi_writeb(hdmi, *msgs[i].buf, HDMI_I2CM_SEGPTR); + } else { + if (msgs[i].flags & I2C_M_RD) + ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, + msgs[i].len); + else + ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, + msgs[i].len); + } if (ret < 0) break; } -- 1.9.1
[PATCH 2/4] perf annotate: Avoid division by zero when calculating percent
Currently perf-annotate with --print-line can print -nan(0x8) because of division by zero when calculating percent. So if a sum of samples is zero, skip calculating percent. Before: $ perf annotate --stdio -l Sorted summary for file /home/taeung/workspace/a.out -- 32.89-nan7.04 a.c:38 25.14-nan0.00 a.c:34 16.26-nan 56.34 a.c:31 15.88-nan1.41 a.c:37 5.67-nan0.00 a.c:39 1.13-nan 35.21 a.c:26 0.95-nan0.00 a.c:44 0.57-nan0.00 a.c:32 Percent | Source code & Disassembly of a.out for cycles (529 samples) - : ... a.c:260.57-nan4.23 : 40081a: mov%edi,-0x24(%rbp) a.c:260.00-nan9.86 : 40081d: mov%rsi,-0x30(%rbp) ... After: $ perf annotate --stdio -l Sorted summary for file /home/taeung/workspace/a.out -- 32.890.007.04 a.c:38 25.140.000.00 a.c:34 16.260.00 56.34 a.c:31 15.880.001.41 a.c:37 5.670.000.00 a.c:39 1.130.00 35.21 a.c:26 0.950.000.00 a.c:44 0.570.000.00 a.c:32 Percent | Source code & Disassembly of old for cycles (529 samples) - : ... a.c:260.570.004.23 : 40081a: mov%edi,-0x24(%rbp) a.c:260.000.009.86 : 40081d: mov%rsi,-0x30(%rbp) ... Cc: Namhyung KimCc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index fc91c6b..9bb43cd 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1665,11 +1665,15 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map, src_line->nr_pcnt = nr_pcnt; for (k = 0; k < nr_pcnt; k++) { + double percent = 0.0; + h = annotation__histogram(notes, evidx + k); - src_line->samples[k].percent = 100.0 * h->addr[i] / h->sum; + if (h->sum) + percent = 100.0 * h->addr[i] / h->sum; - if (src_line->samples[k].percent > percent_max) - percent_max = src_line->samples[k].percent; + if (percent > percent_max) + percent_max = percent; + src_line->samples[k].percent = percent; } if (percent_max <= 0.5) -- 2.7.4
[PATCH 2/4] perf annotate: Avoid division by zero when calculating percent
Currently perf-annotate with --print-line can print -nan(0x8) because of division by zero when calculating percent. So if a sum of samples is zero, skip calculating percent. Before: $ perf annotate --stdio -l Sorted summary for file /home/taeung/workspace/a.out -- 32.89-nan7.04 a.c:38 25.14-nan0.00 a.c:34 16.26-nan 56.34 a.c:31 15.88-nan1.41 a.c:37 5.67-nan0.00 a.c:39 1.13-nan 35.21 a.c:26 0.95-nan0.00 a.c:44 0.57-nan0.00 a.c:32 Percent | Source code & Disassembly of a.out for cycles (529 samples) - : ... a.c:260.57-nan4.23 : 40081a: mov%edi,-0x24(%rbp) a.c:260.00-nan9.86 : 40081d: mov%rsi,-0x30(%rbp) ... After: $ perf annotate --stdio -l Sorted summary for file /home/taeung/workspace/a.out -- 32.890.007.04 a.c:38 25.140.000.00 a.c:34 16.260.00 56.34 a.c:31 15.880.001.41 a.c:37 5.670.000.00 a.c:39 1.130.00 35.21 a.c:26 0.950.000.00 a.c:44 0.570.000.00 a.c:32 Percent | Source code & Disassembly of old for cycles (529 samples) - : ... a.c:260.570.004.23 : 40081a: mov%edi,-0x24(%rbp) a.c:260.000.009.86 : 40081d: mov%rsi,-0x30(%rbp) ... Cc: Namhyung Kim Cc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 10 +++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index fc91c6b..9bb43cd 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1665,11 +1665,15 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map, src_line->nr_pcnt = nr_pcnt; for (k = 0; k < nr_pcnt; k++) { + double percent = 0.0; + h = annotation__histogram(notes, evidx + k); - src_line->samples[k].percent = 100.0 * h->addr[i] / h->sum; + if (h->sum) + percent = 100.0 * h->addr[i] / h->sum; - if (src_line->samples[k].percent > percent_max) - percent_max = src_line->samples[k].percent; + if (percent > percent_max) + percent_max = percent; + src_line->samples[k].percent = percent; } if (percent_max <= 0.5) -- 2.7.4
[PATCH 3/4] perf annotate: Fix missing setting nr samples on source_line
If using --show-total-period with -l, disasm__calc_percent() use a 'samples' array of source_line. But samples[evidx].nr is always zero so print 0 values as below. Before: $ perf annotate --stdio -l --show-total-period ... 0 :400816: push %rbp 0 :400817: mov%rsp,%rbp 0 :40081a: mov%edi,-0x24(%rbp) 0 :40081d: mov%rsi,-0x30(%rbp) 0 :400821: mov-0x24(%rbp),%eax 0 :400824: mov-0x30(%rbp),%rdx 0 :400828: mov(%rdx),%esi 0 :40082a: mov$0x0,%edx ... The reason is that in symbol__get_source_line() did't set number of samples into a memeber variable 'nr' of samples of source_line. So fix it for correct number of samples when using --show-total-period option as below. After: $ perf annotate --stdio -l --show-total-period ... 3 :400816: push %rbp 4 :400817: mov%rsp,%rbp 0 :40081a: mov%edi,-0x24(%rbp) 0 :40081d: mov%rsi,-0x30(%rbp) 1 :400821: mov-0x24(%rbp),%eax 2 :400824: mov-0x30(%rbp),%rdx 0 :400828: mov(%rdx),%esi 1 :40082a: mov$0x0,%edx ... Cc: Namhyung KimCc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 6 -- tools/perf/util/annotate.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 9bb43cd..63130ec 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1659,7 +1659,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map, start = map__rip_2objdump(map, sym->start); for (i = 0; i < len; i++) { - u64 offset; + u64 offset, nr_samples; double percent_max = 0.0; src_line->nr_pcnt = nr_pcnt; @@ -1668,12 +1668,14 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map, double percent = 0.0; h = annotation__histogram(notes, evidx + k); + nr_samples = h->addr[i]; if (h->sum) - percent = 100.0 * h->addr[i] / h->sum; + percent = 100.0 * nr_samples / h->sum; if (percent > percent_max) percent_max = percent; src_line->samples[k].percent = percent; + src_line->samples[k].nr = nr_samples; } if (percent_max <= 0.5) diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h index 09776b5..948aa8e 100644 --- a/tools/perf/util/annotate.h +++ b/tools/perf/util/annotate.h @@ -98,7 +98,7 @@ struct cyc_hist { struct source_line_samples { double percent; double percent_sum; - double nr; + u64 nr; }; struct source_line { -- 2.7.4
[PATCH 1/4] perf annotate: Use build-id dir when reading link name
In dso__disassemble_filename() when reading link name from a build-id file, it is failed each time because a build-id file gotten from dso__build_id_filename() is not symbolic link. So use build-id directory path instead of a build-id file name. For example, if build-id file name gotten from dso__build_id_filename() is as below, /root/.debug/.build-id/4f/75c7d197c951659d1c1b8b5fd49bcdf8f3f8b1/elf instead of the above build-id file name, use the build-id dir path that is a symbolic link as below. /root/.debug/.build-id/4f/75c7d197c951659d1c1b8b5fd49bcdf8f3f8b1 Cc: Namhyung KimCc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 273f21f..fc91c6b 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1307,6 +1307,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil { char linkname[PATH_MAX]; char *build_id_filename; + char *build_id_path = NULL; if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS && !dso__is_kcore(dso)) @@ -1322,8 +1323,14 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil goto fallback; } + build_id_path = strdup(filename); + if (!build_id_path) + return -1; + + dirname(build_id_path); + if (dso__is_kcore(dso) || - readlink(filename, linkname, sizeof(linkname)) < 0 || + readlink(build_id_path, linkname, sizeof(linkname)) < 0 || strstr(linkname, DSO__NAME_KALLSYMS) || access(filename, R_OK)) { fallback: @@ -1335,6 +1342,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil __symbol__join_symfs(filename, filename_size, dso->long_name); } + free(build_id_path); return 0; } -- 2.7.4
[PATCH 3/4] perf annotate: Fix missing setting nr samples on source_line
If using --show-total-period with -l, disasm__calc_percent() use a 'samples' array of source_line. But samples[evidx].nr is always zero so print 0 values as below. Before: $ perf annotate --stdio -l --show-total-period ... 0 :400816: push %rbp 0 :400817: mov%rsp,%rbp 0 :40081a: mov%edi,-0x24(%rbp) 0 :40081d: mov%rsi,-0x30(%rbp) 0 :400821: mov-0x24(%rbp),%eax 0 :400824: mov-0x30(%rbp),%rdx 0 :400828: mov(%rdx),%esi 0 :40082a: mov$0x0,%edx ... The reason is that in symbol__get_source_line() did't set number of samples into a memeber variable 'nr' of samples of source_line. So fix it for correct number of samples when using --show-total-period option as below. After: $ perf annotate --stdio -l --show-total-period ... 3 :400816: push %rbp 4 :400817: mov%rsp,%rbp 0 :40081a: mov%edi,-0x24(%rbp) 0 :40081d: mov%rsi,-0x30(%rbp) 1 :400821: mov-0x24(%rbp),%eax 2 :400824: mov-0x30(%rbp),%rdx 0 :400828: mov(%rdx),%esi 1 :40082a: mov$0x0,%edx ... Cc: Namhyung Kim Cc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 6 -- tools/perf/util/annotate.h | 2 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 9bb43cd..63130ec 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1659,7 +1659,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map, start = map__rip_2objdump(map, sym->start); for (i = 0; i < len; i++) { - u64 offset; + u64 offset, nr_samples; double percent_max = 0.0; src_line->nr_pcnt = nr_pcnt; @@ -1668,12 +1668,14 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map, double percent = 0.0; h = annotation__histogram(notes, evidx + k); + nr_samples = h->addr[i]; if (h->sum) - percent = 100.0 * h->addr[i] / h->sum; + percent = 100.0 * nr_samples / h->sum; if (percent > percent_max) percent_max = percent; src_line->samples[k].percent = percent; + src_line->samples[k].nr = nr_samples; } if (percent_max <= 0.5) diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h index 09776b5..948aa8e 100644 --- a/tools/perf/util/annotate.h +++ b/tools/perf/util/annotate.h @@ -98,7 +98,7 @@ struct cyc_hist { struct source_line_samples { double percent; double percent_sum; - double nr; + u64 nr; }; struct source_line { -- 2.7.4
[PATCH 1/4] perf annotate: Use build-id dir when reading link name
In dso__disassemble_filename() when reading link name from a build-id file, it is failed each time because a build-id file gotten from dso__build_id_filename() is not symbolic link. So use build-id directory path instead of a build-id file name. For example, if build-id file name gotten from dso__build_id_filename() is as below, /root/.debug/.build-id/4f/75c7d197c951659d1c1b8b5fd49bcdf8f3f8b1/elf instead of the above build-id file name, use the build-id dir path that is a symbolic link as below. /root/.debug/.build-id/4f/75c7d197c951659d1c1b8b5fd49bcdf8f3f8b1 Cc: Namhyung Kim Cc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 10 +- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 273f21f..fc91c6b 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1307,6 +1307,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil { char linkname[PATH_MAX]; char *build_id_filename; + char *build_id_path = NULL; if (dso->symtab_type == DSO_BINARY_TYPE__KALLSYMS && !dso__is_kcore(dso)) @@ -1322,8 +1323,14 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil goto fallback; } + build_id_path = strdup(filename); + if (!build_id_path) + return -1; + + dirname(build_id_path); + if (dso__is_kcore(dso) || - readlink(filename, linkname, sizeof(linkname)) < 0 || + readlink(build_id_path, linkname, sizeof(linkname)) < 0 || strstr(linkname, DSO__NAME_KALLSYMS) || access(filename, R_OK)) { fallback: @@ -1335,6 +1342,7 @@ static int dso__disassemble_filename(struct dso *dso, char *filename, size_t fil __symbol__join_symfs(filename, filename_size, dso->long_name); } + free(build_id_path); return 0; } -- 2.7.4
[PATCH 0/4] perf annotate: Bugfixes
Hi, perf-annotate has little bugs so I fix them. I'd appreciate some feedback on this patchset. :) Thanks, Taeung Taeung Song (4): perf annotate: Use build-id dir when reading link name perf annotate: Avoid division by zero when calculating percent perf annotate: Fix missing setting nr samples on source_line perf annotate: More exactly grep -v of the objdump command tools/perf/util/annotate.c | 26 -- tools/perf/util/annotate.h | 2 +- 2 files changed, 21 insertions(+), 7 deletions(-) -- 2.7.4
[PATCH 0/4] perf annotate: Bugfixes
Hi, perf-annotate has little bugs so I fix them. I'd appreciate some feedback on this patchset. :) Thanks, Taeung Taeung Song (4): perf annotate: Use build-id dir when reading link name perf annotate: Avoid division by zero when calculating percent perf annotate: Fix missing setting nr samples on source_line perf annotate: More exactly grep -v of the objdump command tools/perf/util/annotate.c | 26 -- tools/perf/util/annotate.h | 2 +- 2 files changed, 21 insertions(+), 7 deletions(-) -- 2.7.4
[PATCH 4/4] perf annotate: More exactly grep -v of the objdump command
grep -v "file name" in the objdump command cause a side effect eliminating filename:linenr of output of 'objdump -l' if the object file name and source file name are about the same so fix it. The objdump command in symbol__disassemble() can be as below $ objdump -l -d -S -C /home/taeung/hello --start-address=... /home/taeung/hello: file format elf64-x86-64 Disassembly of section .text: 00400526 : main(): /home/taeung/hello.c:4 void main() { 400526: 55 push %rbp 400527: 48 89 e5mov%rsp,%rbp /home/taeung/hello.c:5 ... But currently it use grep -v "file name" e.g. "/home/taeung/hello" in the objdump command to remove the first line containing file name and file format such as, Before: $ objdump -l -d -S -C /home/taeung/hello | grep /home/taeung/hello But it cause a side effect removing filename:linenr because the object file and source file has same name e.g. "/home/taueng/hello", "/home/taeung/hello.c" So more exactly grep -v as below to correctly remove the one line e.g. "/home/taeung/hello: file format elf64-x86-64" After: $ objdump -l -d -S -C /home/taeung/hello | grep /home/taeung/hello: Cc: Namhyung KimCc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 63130ec..e49eb7e 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1443,7 +1443,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na snprintf(command, sizeof(command), "%s %s%s --start-address=0x%016" PRIx64 " --stop-address=0x%016" PRIx64 -" -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand", +" -l -d %s %s -C %s 2>/dev/null|grep -v %s: |expand", objdump_path ? objdump_path : "objdump", disassembler_style ? "-M " : "", disassembler_style ? disassembler_style : "", -- 2.7.4
[PATCH 4/4] perf annotate: More exactly grep -v of the objdump command
grep -v "file name" in the objdump command cause a side effect eliminating filename:linenr of output of 'objdump -l' if the object file name and source file name are about the same so fix it. The objdump command in symbol__disassemble() can be as below $ objdump -l -d -S -C /home/taeung/hello --start-address=... /home/taeung/hello: file format elf64-x86-64 Disassembly of section .text: 00400526 : main(): /home/taeung/hello.c:4 void main() { 400526: 55 push %rbp 400527: 48 89 e5mov%rsp,%rbp /home/taeung/hello.c:5 ... But currently it use grep -v "file name" e.g. "/home/taeung/hello" in the objdump command to remove the first line containing file name and file format such as, Before: $ objdump -l -d -S -C /home/taeung/hello | grep /home/taeung/hello But it cause a side effect removing filename:linenr because the object file and source file has same name e.g. "/home/taueng/hello", "/home/taeung/hello.c" So more exactly grep -v as below to correctly remove the one line e.g. "/home/taeung/hello: file format elf64-x86-64" After: $ objdump -l -d -S -C /home/taeung/hello | grep /home/taeung/hello: Cc: Namhyung Kim Cc: Jiri Olsa Signed-off-by: Taeung Song --- tools/perf/util/annotate.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c index 63130ec..e49eb7e 100644 --- a/tools/perf/util/annotate.c +++ b/tools/perf/util/annotate.c @@ -1443,7 +1443,7 @@ int symbol__disassemble(struct symbol *sym, struct map *map, const char *arch_na snprintf(command, sizeof(command), "%s %s%s --start-address=0x%016" PRIx64 " --stop-address=0x%016" PRIx64 -" -l -d %s %s -C %s 2>/dev/null|grep -v %s|expand", +" -l -d %s %s -C %s 2>/dev/null|grep -v %s: |expand", objdump_path ? objdump_path : "objdump", disassembler_style ? "-M " : "", disassembler_style ? disassembler_style : "", -- 2.7.4
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure
Hi Kuppuswamy, [auto build test ERROR on linus/master] [also build test ERROR on v4.11-rc3 next-20170310] [cannot apply to platform-drivers-x86/for-next tip/x86/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Kuppuswamy-Sathyanarayanan/platform-x86-intel_pmc_ipc-fix-gcr-offset/20170320-032638 config: x86_64-randconfig-h0-03200816 (attached as .config) compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/built-in.o: In function `iTCO_wdt_unset_NO_REBOOT_bit': >> iTCO_wdt.c:(.text+0x3b6f0f): undefined reference to `intel_pmc_gcr_read' >> iTCO_wdt.c:(.text+0x3b6f24): undefined reference to `intel_pmc_gcr_write' iTCO_wdt.c:(.text+0x3b6f2e): undefined reference to `intel_pmc_gcr_read' drivers/built-in.o: In function `iTCO_wdt_set_NO_REBOOT_bit': iTCO_wdt.c:(.text+0x3b729e): undefined reference to `intel_pmc_gcr_read' iTCO_wdt.c:(.text+0x3b72d1): undefined reference to `intel_pmc_gcr_write' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH v2 3/4] watchdog: iTCO_wdt: Fix PMC GCR memory mapping failure
Hi Kuppuswamy, [auto build test ERROR on linus/master] [also build test ERROR on v4.11-rc3 next-20170310] [cannot apply to platform-drivers-x86/for-next tip/x86/core] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Kuppuswamy-Sathyanarayanan/platform-x86-intel_pmc_ipc-fix-gcr-offset/20170320-032638 config: x86_64-randconfig-h0-03200816 (attached as .config) compiler: gcc-4.9 (Debian 4.9.4-2) 4.9.4 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/built-in.o: In function `iTCO_wdt_unset_NO_REBOOT_bit': >> iTCO_wdt.c:(.text+0x3b6f0f): undefined reference to `intel_pmc_gcr_read' >> iTCO_wdt.c:(.text+0x3b6f24): undefined reference to `intel_pmc_gcr_write' iTCO_wdt.c:(.text+0x3b6f2e): undefined reference to `intel_pmc_gcr_read' drivers/built-in.o: In function `iTCO_wdt_set_NO_REBOOT_bit': iTCO_wdt.c:(.text+0x3b729e): undefined reference to `intel_pmc_gcr_read' iTCO_wdt.c:(.text+0x3b72d1): undefined reference to `intel_pmc_gcr_write' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH 2/2 v2] clk: qoriq: Separate root input clock for core PLLs on ls1012a
From: Scott Woodls1012a has separate input root clocks for core PLLs versus the platform PLL, with the latter described as sysclk in the hw docs. If a second input clock, named "coreclk", is present, this clock will be used for the core PLLs. Signed-off-by: Scott Wood Signed-off-by: Tang Yuantian Acked-by: Rob Herring --- v2: -- change the author to Scott. drivers/clk/clk-qoriq.c | 91 + 1 file changed, 77 insertions(+), 14 deletions(-) diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c index d0bf8b1..f3931e3 100644 --- a/drivers/clk/clk-qoriq.c +++ b/drivers/clk/clk-qoriq.c @@ -87,7 +87,7 @@ struct clockgen { struct device_node *node; void __iomem *regs; struct clockgen_chipinfo info; /* mutable copy */ - struct clk *sysclk; + struct clk *sysclk, *coreclk; struct clockgen_pll pll[6]; struct clk *cmux[NUM_CMUX]; struct clk *hwaccel[NUM_HWACCEL]; @@ -904,7 +904,12 @@ static void __init create_muxes(struct clockgen *cg) static void __init clockgen_init(struct device_node *np); -/* Legacy nodes may get probed before the parent clockgen node */ +/* + * Legacy nodes may get probed before the parent clockgen node. + * It is assumed that device trees with legacy nodes will not + * contain a "clocks" property -- otherwise the input clocks may + * not be initialized at this point. + */ static void __init legacy_init_clockgen(struct device_node *np) { if (!clockgen.node) @@ -945,18 +950,13 @@ static struct clk __init return clk_register_fixed_rate(NULL, name, NULL, 0, rate); } -static struct clk *sysclk_from_parent(const char *name) +static struct clk __init *input_clock(const char *name, struct clk *clk) { - struct clk *clk; - const char *parent_name; - - clk = of_clk_get(clockgen.node, 0); - if (IS_ERR(clk)) - return clk; + const char *input_name; /* Register the input clock under the desired name. */ - parent_name = __clk_get_name(clk); - clk = clk_register_fixed_factor(NULL, name, parent_name, + input_name = __clk_get_name(clk); + clk = clk_register_fixed_factor(NULL, name, input_name, 0, 1, 1); if (IS_ERR(clk)) pr_err("%s: Couldn't register %s: %ld\n", __func__, name, @@ -965,6 +965,29 @@ static struct clk *sysclk_from_parent(const char *name) return clk; } +static struct clk __init *input_clock_by_name(const char *name, + const char *dtname) +{ + struct clk *clk; + + clk = of_clk_get_by_name(clockgen.node, dtname); + if (IS_ERR(clk)) + return clk; + + return input_clock(name, clk); +} + +static struct clk __init *input_clock_by_index(const char *name, int idx) +{ + struct clk *clk; + + clk = of_clk_get(clockgen.node, 0); + if (IS_ERR(clk)) + return clk; + + return input_clock(name, clk); +} + static struct clk * __init create_sysclk(const char *name) { struct device_node *sysclk; @@ -974,7 +997,11 @@ static struct clk * __init create_sysclk(const char *name) if (!IS_ERR(clk)) return clk; - clk = sysclk_from_parent(name); + clk = input_clock_by_name(name, "sysclk"); + if (!IS_ERR(clk)) + return clk; + + clk = input_clock_by_index(name, 0); if (!IS_ERR(clk)) return clk; @@ -985,7 +1012,27 @@ static struct clk * __init create_sysclk(const char *name) return clk; } - pr_err("%s: No input clock\n", __func__); + pr_err("%s: No input sysclk\n", __func__); + return NULL; +} + +static struct clk * __init create_coreclk(const char *name) +{ + struct clk *clk; + + clk = input_clock_by_name(name, "coreclk"); + if (!IS_ERR(clk)) + return clk; + + /* +* This indicates a mix of legacy nodes with the new coreclk +* mechanism, which should never happen. If this error occurs, +* don't use the wrong input clock just because coreclk isn't +* ready yet. +*/ + if (WARN_ON(PTR_ERR(clk) == -EPROBE_DEFER)) + return clk; + return NULL; } @@ -1008,11 +1055,19 @@ static void __init create_one_pll(struct clockgen *cg, int idx) u32 __iomem *reg; u32 mult; struct clockgen_pll *pll = >pll[idx]; + const char *input = "cg-sysclk"; int i; if (!(cg->info.pll_mask & (1 << idx))) return; + if (cg->coreclk && idx != PLATFORM_PLL) { + if (IS_ERR(cg->coreclk)) + return; + + input = "cg-coreclk"; + } + if (cg->info.flags & CG_VER3) { switch
[PATCH 2/2 v2] clk: qoriq: Separate root input clock for core PLLs on ls1012a
From: Scott Wood ls1012a has separate input root clocks for core PLLs versus the platform PLL, with the latter described as sysclk in the hw docs. If a second input clock, named "coreclk", is present, this clock will be used for the core PLLs. Signed-off-by: Scott Wood Signed-off-by: Tang Yuantian Acked-by: Rob Herring --- v2: -- change the author to Scott. drivers/clk/clk-qoriq.c | 91 + 1 file changed, 77 insertions(+), 14 deletions(-) diff --git a/drivers/clk/clk-qoriq.c b/drivers/clk/clk-qoriq.c index d0bf8b1..f3931e3 100644 --- a/drivers/clk/clk-qoriq.c +++ b/drivers/clk/clk-qoriq.c @@ -87,7 +87,7 @@ struct clockgen { struct device_node *node; void __iomem *regs; struct clockgen_chipinfo info; /* mutable copy */ - struct clk *sysclk; + struct clk *sysclk, *coreclk; struct clockgen_pll pll[6]; struct clk *cmux[NUM_CMUX]; struct clk *hwaccel[NUM_HWACCEL]; @@ -904,7 +904,12 @@ static void __init create_muxes(struct clockgen *cg) static void __init clockgen_init(struct device_node *np); -/* Legacy nodes may get probed before the parent clockgen node */ +/* + * Legacy nodes may get probed before the parent clockgen node. + * It is assumed that device trees with legacy nodes will not + * contain a "clocks" property -- otherwise the input clocks may + * not be initialized at this point. + */ static void __init legacy_init_clockgen(struct device_node *np) { if (!clockgen.node) @@ -945,18 +950,13 @@ static struct clk __init return clk_register_fixed_rate(NULL, name, NULL, 0, rate); } -static struct clk *sysclk_from_parent(const char *name) +static struct clk __init *input_clock(const char *name, struct clk *clk) { - struct clk *clk; - const char *parent_name; - - clk = of_clk_get(clockgen.node, 0); - if (IS_ERR(clk)) - return clk; + const char *input_name; /* Register the input clock under the desired name. */ - parent_name = __clk_get_name(clk); - clk = clk_register_fixed_factor(NULL, name, parent_name, + input_name = __clk_get_name(clk); + clk = clk_register_fixed_factor(NULL, name, input_name, 0, 1, 1); if (IS_ERR(clk)) pr_err("%s: Couldn't register %s: %ld\n", __func__, name, @@ -965,6 +965,29 @@ static struct clk *sysclk_from_parent(const char *name) return clk; } +static struct clk __init *input_clock_by_name(const char *name, + const char *dtname) +{ + struct clk *clk; + + clk = of_clk_get_by_name(clockgen.node, dtname); + if (IS_ERR(clk)) + return clk; + + return input_clock(name, clk); +} + +static struct clk __init *input_clock_by_index(const char *name, int idx) +{ + struct clk *clk; + + clk = of_clk_get(clockgen.node, 0); + if (IS_ERR(clk)) + return clk; + + return input_clock(name, clk); +} + static struct clk * __init create_sysclk(const char *name) { struct device_node *sysclk; @@ -974,7 +997,11 @@ static struct clk * __init create_sysclk(const char *name) if (!IS_ERR(clk)) return clk; - clk = sysclk_from_parent(name); + clk = input_clock_by_name(name, "sysclk"); + if (!IS_ERR(clk)) + return clk; + + clk = input_clock_by_index(name, 0); if (!IS_ERR(clk)) return clk; @@ -985,7 +1012,27 @@ static struct clk * __init create_sysclk(const char *name) return clk; } - pr_err("%s: No input clock\n", __func__); + pr_err("%s: No input sysclk\n", __func__); + return NULL; +} + +static struct clk * __init create_coreclk(const char *name) +{ + struct clk *clk; + + clk = input_clock_by_name(name, "coreclk"); + if (!IS_ERR(clk)) + return clk; + + /* +* This indicates a mix of legacy nodes with the new coreclk +* mechanism, which should never happen. If this error occurs, +* don't use the wrong input clock just because coreclk isn't +* ready yet. +*/ + if (WARN_ON(PTR_ERR(clk) == -EPROBE_DEFER)) + return clk; + return NULL; } @@ -1008,11 +1055,19 @@ static void __init create_one_pll(struct clockgen *cg, int idx) u32 __iomem *reg; u32 mult; struct clockgen_pll *pll = >pll[idx]; + const char *input = "cg-sysclk"; int i; if (!(cg->info.pll_mask & (1 << idx))) return; + if (cg->coreclk && idx != PLATFORM_PLL) { + if (IS_ERR(cg->coreclk)) + return; + + input = "cg-coreclk"; + } + if (cg->info.flags & CG_VER3) { switch (idx) { case PLATFORM_PLL: @@ -1063,7 +1118,7 @@ static
[PATCH 1/2 v2] dt-bindings: qoriq-clock: Add coreclk
From: Scott Woodls1012a has separate input root clocks for core PLLs versus the platform PLL, with the latter described as sysclk in the hw docs. Update the qoriq-clock binding to allow a second input clock, named "coreclk". If present, this clock will be used for the core PLLs. Signed-off-by: Scott Wood Signed-off-by: Tang Yuantian Acked-by: Rob Herring --- v2: -- change the author to Scott Documentation/devicetree/bindings/clock/qoriq-clock.txt | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/clock/qoriq-clock.txt b/Documentation/devicetree/bindings/clock/qoriq-clock.txt index aa3526f..119cafd 100644 --- a/Documentation/devicetree/bindings/clock/qoriq-clock.txt +++ b/Documentation/devicetree/bindings/clock/qoriq-clock.txt @@ -56,6 +56,11 @@ Optional properties: - clocks: If clock-frequency is not specified, sysclk may be provided as an input clock. Either clock-frequency or clocks must be provided. + A second input clock, called "coreclk", may be provided if + core PLLs are based on a different input clock from the + platform PLL. +- clock-names: Required if a coreclk is present. Valid names are + "sysclk" and "coreclk". 2. Clock Provider @@ -72,6 +77,7 @@ second cell is the clock index for the specified type. 2 hwaccel index (n in CLKCGnHWACSR) 3 fman0 for fm1, 1 for fm2 4 platform pll0=pll, 1=pll/2, 2=pll/3, 3=pll/4 + 5 coreclk must be 0 3. Example -- 2.1.0.27.g96db324
[PATCH 1/2 v2] dt-bindings: qoriq-clock: Add coreclk
From: Scott Wood ls1012a has separate input root clocks for core PLLs versus the platform PLL, with the latter described as sysclk in the hw docs. Update the qoriq-clock binding to allow a second input clock, named "coreclk". If present, this clock will be used for the core PLLs. Signed-off-by: Scott Wood Signed-off-by: Tang Yuantian Acked-by: Rob Herring --- v2: -- change the author to Scott Documentation/devicetree/bindings/clock/qoriq-clock.txt | 6 ++ 1 file changed, 6 insertions(+) diff --git a/Documentation/devicetree/bindings/clock/qoriq-clock.txt b/Documentation/devicetree/bindings/clock/qoriq-clock.txt index aa3526f..119cafd 100644 --- a/Documentation/devicetree/bindings/clock/qoriq-clock.txt +++ b/Documentation/devicetree/bindings/clock/qoriq-clock.txt @@ -56,6 +56,11 @@ Optional properties: - clocks: If clock-frequency is not specified, sysclk may be provided as an input clock. Either clock-frequency or clocks must be provided. + A second input clock, called "coreclk", may be provided if + core PLLs are based on a different input clock from the + platform PLL. +- clock-names: Required if a coreclk is present. Valid names are + "sysclk" and "coreclk". 2. Clock Provider @@ -72,6 +77,7 @@ second cell is the clock index for the specified type. 2 hwaccel index (n in CLKCGnHWACSR) 3 fman0 for fm1, 1 for fm2 4 platform pll0=pll, 1=pll/2, 2=pll/3, 3=pll/4 + 5 coreclk must be 0 3. Example -- 2.1.0.27.g96db324
RE: [PATCH] HV: properly delay KVP packets when negotiation is in progress
> -Original Message- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: Friday, March 17, 2017 9:16 AM > To: Long Li> Cc: KY Srinivasan ; Haiyang Zhang > ; Stephen Hemminger > ; de...@linuxdriverproject.org; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH] HV: properly delay KVP packets when negotiation is in > progress > > Long Li writes: > > > The host may send multiple KVP packets before the negotiation with > > daemon is finished. We need to keep those packets in ring buffer until > > the daemon is negotiated and connected. > > The patch looks OK but previously we always presumed that this can't > happen for util drivers and host will never send a new request before we > answer to the previous one. If this is not true we may have more issues > which need fixing as all three drivers we have are written in a 'transaction' > fashion. > > So my question would be: can the host send multiple (KVP) packets _after_ > the negotiation with daemon is finished? Thanks Vitaly. I'm checking with Windows guys and will update soon. > > > > > > This patch is based on the work of Nick Meier > > > > > > Signed-off-by: Long Li > > --- > > drivers/hv/hv_kvp.c | 9 + > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index > > de26371..b9f928d 100644 > > --- a/drivers/hv/hv_kvp.c > > +++ b/drivers/hv/hv_kvp.c > > @@ -628,16 +628,17 @@ void hv_kvp_onchannelcallback(void *context) > > NEGO_IN_PROGRESS, > > NEGO_FINISHED} host_negotiatied = > NEGO_NOT_STARTED; > > > > - if (host_negotiatied == NEGO_NOT_STARTED && > > - kvp_transaction.state < HVUTIL_READY) { > > + if (kvp_transaction.state < HVUTIL_READY) { > > /* > > * If userspace daemon is not connected and host is asking > > * us to negotiate we need to delay to not lose messages. > > * This is important for Failover IP setting. > > */ > > - host_negotiatied = NEGO_IN_PROGRESS; > > - schedule_delayed_work(_host_handshake_work, > > + if (host_negotiatied == NEGO_NOT_STARTED) { > > + host_negotiatied = NEGO_IN_PROGRESS; > > + > schedule_delayed_work(_host_handshake_work, > > HV_UTIL_NEGO_TIMEOUT * HZ); > > + } > > return; > > } > > if (kvp_transaction.state > HVUTIL_READY) > > -- > Vitaly
RE: [PATCH] HV: properly delay KVP packets when negotiation is in progress
> -Original Message- > From: Vitaly Kuznetsov [mailto:vkuzn...@redhat.com] > Sent: Friday, March 17, 2017 9:16 AM > To: Long Li > Cc: KY Srinivasan ; Haiyang Zhang > ; Stephen Hemminger > ; de...@linuxdriverproject.org; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH] HV: properly delay KVP packets when negotiation is in > progress > > Long Li writes: > > > The host may send multiple KVP packets before the negotiation with > > daemon is finished. We need to keep those packets in ring buffer until > > the daemon is negotiated and connected. > > The patch looks OK but previously we always presumed that this can't > happen for util drivers and host will never send a new request before we > answer to the previous one. If this is not true we may have more issues > which need fixing as all three drivers we have are written in a 'transaction' > fashion. > > So my question would be: can the host send multiple (KVP) packets _after_ > the negotiation with daemon is finished? Thanks Vitaly. I'm checking with Windows guys and will update soon. > > > > > > This patch is based on the work of Nick Meier > > > > > > Signed-off-by: Long Li > > --- > > drivers/hv/hv_kvp.c | 9 + > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c index > > de26371..b9f928d 100644 > > --- a/drivers/hv/hv_kvp.c > > +++ b/drivers/hv/hv_kvp.c > > @@ -628,16 +628,17 @@ void hv_kvp_onchannelcallback(void *context) > > NEGO_IN_PROGRESS, > > NEGO_FINISHED} host_negotiatied = > NEGO_NOT_STARTED; > > > > - if (host_negotiatied == NEGO_NOT_STARTED && > > - kvp_transaction.state < HVUTIL_READY) { > > + if (kvp_transaction.state < HVUTIL_READY) { > > /* > > * If userspace daemon is not connected and host is asking > > * us to negotiate we need to delay to not lose messages. > > * This is important for Failover IP setting. > > */ > > - host_negotiatied = NEGO_IN_PROGRESS; > > - schedule_delayed_work(_host_handshake_work, > > + if (host_negotiatied == NEGO_NOT_STARTED) { > > + host_negotiatied = NEGO_IN_PROGRESS; > > + > schedule_delayed_work(_host_handshake_work, > > HV_UTIL_NEGO_TIMEOUT * HZ); > > + } > > return; > > } > > if (kvp_transaction.state > HVUTIL_READY) > > -- > Vitaly
linux-next: build failure after merge of the char-misc tree
Hi all, After merging the char-misc tree, today's linux-next build (x86_64 allmodconfig) failed like this: drivers/misc/aspeed-lpc-ctrl.c: In function 'aspeed_lpc_ctrl_mmap': drivers/misc/aspeed-lpc-ctrl.c:51:9: error: implicit declaration of function 'pgprot_dmacoherent' [-Werror=implicit-function-declaration] prot = pgprot_dmacoherent(prot); ^ drivers/misc/aspeed-lpc-ctrl.c:51:7: error: incompatible types when assigning to type 'pgprot_t {aka struct pgprot}' from type 'int' prot = pgprot_dmacoherent(prot); ^ In file included from include/linux/miscdevice.h:6:0, from drivers/misc/aspeed-lpc-ctrl.c:11: drivers/misc/aspeed-lpc-ctrl.c: In function 'aspeed_lpc_ctrl_probe': drivers/misc/aspeed-lpc-ctrl.c:232:17: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'phys_addr_t {aka long long unsigned int}' [-Wformat=] dev_info(dev, "Loaded at 0x%08x (0x%08x)\n", ^ include/linux/device.h:1317:51: note: in definition of macro 'dev_info' #define dev_info(dev, fmt, arg...) _dev_info(dev, fmt, ##arg) ^ drivers/misc/aspeed-lpc-ctrl.c:232:17: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=] dev_info(dev, "Loaded at 0x%08x (0x%08x)\n", ^ include/linux/device.h:1317:51: note: in definition of macro 'dev_info' #define dev_info(dev, fmt, arg...) _dev_info(dev, fmt, ##arg) ^ Caused by commit 6c4e97678501 ("drivers/misc: Add Aspeed LPC control driver") Clearly this will only build on arm :-( You can only use COMPILE_TEST if you can reasonably expect the build to work on all architectures and platforms. I have added the following patch for today (the warnings should be fixed as well): From: Stephen RothwellDate: Mon, 20 Mar 2017 13:38:10 +1100 Subject: [PATCH] drivers/misc: Aspeed LPC control driver will only build on arm Signed-off-by: Stephen Rothwell --- drivers/misc/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index fb933b0b9297..52a46b129214 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -773,7 +773,7 @@ config PANEL_BOOT_MESSAGE endif # PANEL config ASPEED_LPC_CTRL - depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON + depends on ARCH_ASPEED && REGMAP && MFD_SYSCON tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge control" ---help--- Control Aspeed ast2400/2500 HOST LPC to BMC mappings through -- 2.11.0 -- Cheers, Stephen Rothwell
linux-next: build failure after merge of the char-misc tree
Hi all, After merging the char-misc tree, today's linux-next build (x86_64 allmodconfig) failed like this: drivers/misc/aspeed-lpc-ctrl.c: In function 'aspeed_lpc_ctrl_mmap': drivers/misc/aspeed-lpc-ctrl.c:51:9: error: implicit declaration of function 'pgprot_dmacoherent' [-Werror=implicit-function-declaration] prot = pgprot_dmacoherent(prot); ^ drivers/misc/aspeed-lpc-ctrl.c:51:7: error: incompatible types when assigning to type 'pgprot_t {aka struct pgprot}' from type 'int' prot = pgprot_dmacoherent(prot); ^ In file included from include/linux/miscdevice.h:6:0, from drivers/misc/aspeed-lpc-ctrl.c:11: drivers/misc/aspeed-lpc-ctrl.c: In function 'aspeed_lpc_ctrl_probe': drivers/misc/aspeed-lpc-ctrl.c:232:17: warning: format '%x' expects argument of type 'unsigned int', but argument 3 has type 'phys_addr_t {aka long long unsigned int}' [-Wformat=] dev_info(dev, "Loaded at 0x%08x (0x%08x)\n", ^ include/linux/device.h:1317:51: note: in definition of macro 'dev_info' #define dev_info(dev, fmt, arg...) _dev_info(dev, fmt, ##arg) ^ drivers/misc/aspeed-lpc-ctrl.c:232:17: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=] dev_info(dev, "Loaded at 0x%08x (0x%08x)\n", ^ include/linux/device.h:1317:51: note: in definition of macro 'dev_info' #define dev_info(dev, fmt, arg...) _dev_info(dev, fmt, ##arg) ^ Caused by commit 6c4e97678501 ("drivers/misc: Add Aspeed LPC control driver") Clearly this will only build on arm :-( You can only use COMPILE_TEST if you can reasonably expect the build to work on all architectures and platforms. I have added the following patch for today (the warnings should be fixed as well): From: Stephen Rothwell Date: Mon, 20 Mar 2017 13:38:10 +1100 Subject: [PATCH] drivers/misc: Aspeed LPC control driver will only build on arm Signed-off-by: Stephen Rothwell --- drivers/misc/Kconfig | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index fb933b0b9297..52a46b129214 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -773,7 +773,7 @@ config PANEL_BOOT_MESSAGE endif # PANEL config ASPEED_LPC_CTRL - depends on (ARCH_ASPEED || COMPILE_TEST) && REGMAP && MFD_SYSCON + depends on ARCH_ASPEED && REGMAP && MFD_SYSCON tristate "Aspeed ast2400/2500 HOST LPC to BMC bridge control" ---help--- Control Aspeed ast2400/2500 HOST LPC to BMC mappings through -- 2.11.0 -- Cheers, Stephen Rothwell
[PATCH] kvmgt: Hold struct kvm reference
The kvmgt code keeps a pointer to the struct kvm associated with the device, but doesn't actually hold a reference to it. If we do unclean shutdown testing (ie. killing the user process), then we can see the kvm association to the device unset, which causes kvmgt to trigger a device release via a work queue. Naturally we cannot guarantee that the cached struct kvm pointer is still valid at this point without holding a reference. The observed failure in this case is a stuck cpu trying to acquire the spinlock from the invalid reference, but other failure modes are clearly possible. Hold a reference to avoid this. Signed-off-by: Alex WilliamsonCc: sta...@vger.kernel.org #v4.10 Cc: Jike Song Cc: Paolo Bonzini Cc: Zhenyu Wang Cc: Zhi Wang --- drivers/gpu/drm/i915/gvt/kvmgt.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 84d801638ede..142b8bd4ba6b 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1324,6 +1324,7 @@ static int kvmgt_guest_init(struct mdev_device *mdev) vgpu->handle = (unsigned long)info; info->vgpu = vgpu; info->kvm = kvm; + kvm_get_kvm(info->kvm); kvmgt_protect_table_init(info); gvt_cache_init(vgpu); @@ -1343,6 +1344,7 @@ static bool kvmgt_guest_exit(struct kvmgt_guest_info *info) } kvm_page_track_unregister_notifier(info->kvm, >track_node); + kvm_put_kvm(info->kvm); kvmgt_protect_table_destroy(info); gvt_cache_destroy(info->vgpu); vfree(info);
[PATCH] kvmgt: Hold struct kvm reference
The kvmgt code keeps a pointer to the struct kvm associated with the device, but doesn't actually hold a reference to it. If we do unclean shutdown testing (ie. killing the user process), then we can see the kvm association to the device unset, which causes kvmgt to trigger a device release via a work queue. Naturally we cannot guarantee that the cached struct kvm pointer is still valid at this point without holding a reference. The observed failure in this case is a stuck cpu trying to acquire the spinlock from the invalid reference, but other failure modes are clearly possible. Hold a reference to avoid this. Signed-off-by: Alex Williamson Cc: sta...@vger.kernel.org #v4.10 Cc: Jike Song Cc: Paolo Bonzini Cc: Zhenyu Wang Cc: Zhi Wang --- drivers/gpu/drm/i915/gvt/kvmgt.c |2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index 84d801638ede..142b8bd4ba6b 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -1324,6 +1324,7 @@ static int kvmgt_guest_init(struct mdev_device *mdev) vgpu->handle = (unsigned long)info; info->vgpu = vgpu; info->kvm = kvm; + kvm_get_kvm(info->kvm); kvmgt_protect_table_init(info); gvt_cache_init(vgpu); @@ -1343,6 +1344,7 @@ static bool kvmgt_guest_exit(struct kvmgt_guest_info *info) } kvm_page_track_unregister_notifier(info->kvm, >track_node); + kvm_put_kvm(info->kvm); kvmgt_protect_table_destroy(info); gvt_cache_destroy(info->vgpu); vfree(info);
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/20/2017 at 10:13 AM, Baoquan He wrote: > On 03/17/17 at 12:22pm, Eric W. Biederman wrote: >> Xunlei Pangwrites: >> >>> Currently vmcoreinfo data is updated at boot time subsys_initcall(), >>> it has the risk of being modified by some wrong code during system >>> is running. >>> >>> As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, >>> when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, >>> we probably will get "Segmentation fault" or other unexpected/confusing >>> errors. >> If this is a real concern and the previous discussion sounds like it is >> part of what we need to do is move the variable vmcoreinfo_note out >> of the kernel's .bss section. And modify the code to regenerate >> and keep this information in something like the control page. > I guess this is not from a real issue, just from Xunlei's worry. But > Xunlei didn't give a direct answer to this, and Petr's question. Not It's easy to reproduce: write a kernel module to modify part content of vmcoreinfo_data (we surely have many ways to acquire its VA). If it does exist in theory, we will met it sooner or later in real world due to billions of applications. Also there are bugs like this one https://bugzilla.redhat.com/show_bug.cgi?id=1287097 Not sure if it is makedumpfile issue or this one, maybe we can't know forever. Regards, Xunlei > very sure if this will impact other implementation. fadump will be > impacted by this or other dump? Maybe yet or maybe not. > > I don't object this strongly, but please at least add code comment to > explain why vmcoreinfo need be saved twice because it does look weird. > >> Definitely something like this needs a page all to itself, and ideally >> far away from any other kernel data structures. I clearly was not >> watching closely the data someone decided to keep this silly thing >> in the kernel's .bss section. >> >>> As vmcoreinfo is the most fundamental information for vmcore, we better >>> double check its correctness. Here we generate a signature(using crc32) >>> after it is saved, then verify it in crash_save_vmcoreinfo() to see if >>> the signature was broken, if so we have to re-save the vmcoreinfo data >>> to get the correct vmcoreinfo for kdump as possible as we can. >> Sigh. We already have a sha256 that is supposed to cover this sort of >> thing. The bug rather is that apparently it isn't covering this data. >> That sounds like what we should be fixing. >> >> Please let's not invent new mechanisms we have to maintain. Let's >> reorganize this so this static data is protected like all other static >> data in the kexec-on-panic path. We have good mechanims and good >> strategies for avoiding and detecting corruption we just need to use >> them. >> >> Eric >> >> >> >>> Signed-off-by: Xunlei Pang >>> --- >>> v1->v2: >>> - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" >>> uses the information. >>> - Add crc32 verification for vmcoreinfo, re-save when failure. >>> >>> arch/Kconfig| 1 + >>> kernel/kexec_core.c | 43 +++ >>> 2 files changed, 36 insertions(+), 8 deletions(-) >>> >>> diff --git a/arch/Kconfig b/arch/Kconfig >>> index c4d6833..66eb296 100644 >>> --- a/arch/Kconfig >>> +++ b/arch/Kconfig >>> @@ -4,6 +4,7 @@ >>> >>> config KEXEC_CORE >>> bool >>> + select CRC32 >>> >>> config HAVE_IMA_KEXEC >>> bool >>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c >>> index bfe62d5..012acbe 100644 >>> --- a/kernel/kexec_core.c >>> +++ b/kernel/kexec_core.c >>> @@ -38,6 +38,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -53,9 +54,10 @@ >>> >>> /* vmcoreinfo stuff */ >>> static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; >>> -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; >>> +static u32 vmcoreinfo_sig; >>> size_t vmcoreinfo_size; >>> size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); >>> +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; >>> >>> /* Flag to indicate we are going to kexec a new kernel */ >>> bool kexec_in_progress = false; >>> @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void) >>> final_note(buf); >>> } >>> >>> -void crash_save_vmcoreinfo(void) >>> -{ >>> - vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds()); >>> - update_vmcoreinfo_note(); >>> -} >>> - >>> void vmcoreinfo_append_str(const char *fmt, ...) >>> { >>> va_list args; >>> @@ -1402,7 +1398,7 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void) >>> return __pa_symbol((unsigned long)(char *)_note); >>> } >>> >>> -static int __init crash_save_vmcoreinfo_init(void) >>> +static void do_crash_save_vmcoreinfo_init(void) >>> { >>> VMCOREINFO_OSRELEASE(init_uts_ns.name.release); >>> VMCOREINFO_PAGESIZE(PAGE_SIZE); >>> @@ -1474,6 +1470,37 @@ static int __init crash_save_vmcoreinfo_init(void) >>> #endif >>> >>>
[PATCH] vfio: Rework group release notifier warning
The intent of the original warning is make sure that the mdev vendor driver has removed any group notifiers at the point where the group is closed by the user. Theoretically this would be through an orderly shutdown where any devices are release prior to the group release. We can't always count on an orderly shutdown, the user can close the group before the notifier can be removed or the user task might be killed. We'd like to add this sanity test when the group is idle and the only references are from the devices within the group themselves, but we don't have a good way to do that. Instead check both when the group itself is removed and when the group is opened. A bit later than we'd prefer, but better than the current over aggressive approach. Signed-off-by: Alex WilliamsonCc: Jike Song --- drivers/vfio/vfio.c |8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 609f4f982c74..561084ab387f 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -403,6 +403,7 @@ static void vfio_group_release(struct kref *kref) struct iommu_group *iommu_group = group->iommu_group; WARN_ON(!list_empty(>device_list)); + WARN_ON(group->notifier.head); list_for_each_entry_safe(unbound, tmp, >unbound_list, unbound_next) { @@ -1573,6 +1574,10 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep) return -EBUSY; } + /* Warn if previous user didn't cleanup and re-init to drop them */ + if (WARN_ON(group->notifier.head)) + BLOCKING_INIT_NOTIFIER_HEAD(>notifier); + filep->private_data = group; return 0; @@ -1584,9 +1589,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) filep->private_data = NULL; - /* Any user didn't unregister? */ - WARN_ON(group->notifier.head); - vfio_group_try_dissolve_container(group); atomic_dec(>opened);
Re: [PATCH v6 5/8] dt-bindings: input: touchscreen: Add max11801-ts binding
On Thu, Mar 16, 2017 at 12:35:38PM +0530, Jagan Teki wrote: > From: Jagan Teki> > Add missing documentation of max11801-ts dt-binding details. > > Cc: Mark Rutland > Cc: Rob Herring > Cc: Shawn Guo > Cc: Michael Trimarchi > Signed-off-by: Jagan Teki > --- > Changes for v6: > - Replace the lable and name of the node >ts: max11801 => max11801: touchscreen@48 > Changes for v5: > - Newly added patch > > .../bindings/input/touchscreen/max11801-ts.txt | 18 > ++ > 1 file changed, 18 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > > diff --git > a/Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > b/Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > new file mode 100644 > index 000..4afccbe > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > @@ -0,0 +1,18 @@ > +* MAXI MAX11801 Resistive touch screen controller with i2c interface > + > +Required properties: > +- compatible: must be "max11801" Shouldn't it have a vendor prefix? Also I will need Rob's ACK to get this go via IMX tree. Shawn > +- reg: i2c slave address > +- interrupt-parent: the phandle for the interrupt controller > +- interrupts: touch controller interrupt > + > +Example: > + > + { > + max11801: touchscreen@48 { > + compatible = "max11801"; > + reg = <0x48>; > + interrupt-parent = <>; > + interrupts = <31 2>; > + }; > +}; > -- > 1.9.1 >
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/20/2017 at 10:13 AM, Baoquan He wrote: > On 03/17/17 at 12:22pm, Eric W. Biederman wrote: >> Xunlei Pang writes: >> >>> Currently vmcoreinfo data is updated at boot time subsys_initcall(), >>> it has the risk of being modified by some wrong code during system >>> is running. >>> >>> As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, >>> when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, >>> we probably will get "Segmentation fault" or other unexpected/confusing >>> errors. >> If this is a real concern and the previous discussion sounds like it is >> part of what we need to do is move the variable vmcoreinfo_note out >> of the kernel's .bss section. And modify the code to regenerate >> and keep this information in something like the control page. > I guess this is not from a real issue, just from Xunlei's worry. But > Xunlei didn't give a direct answer to this, and Petr's question. Not It's easy to reproduce: write a kernel module to modify part content of vmcoreinfo_data (we surely have many ways to acquire its VA). If it does exist in theory, we will met it sooner or later in real world due to billions of applications. Also there are bugs like this one https://bugzilla.redhat.com/show_bug.cgi?id=1287097 Not sure if it is makedumpfile issue or this one, maybe we can't know forever. Regards, Xunlei > very sure if this will impact other implementation. fadump will be > impacted by this or other dump? Maybe yet or maybe not. > > I don't object this strongly, but please at least add code comment to > explain why vmcoreinfo need be saved twice because it does look weird. > >> Definitely something like this needs a page all to itself, and ideally >> far away from any other kernel data structures. I clearly was not >> watching closely the data someone decided to keep this silly thing >> in the kernel's .bss section. >> >>> As vmcoreinfo is the most fundamental information for vmcore, we better >>> double check its correctness. Here we generate a signature(using crc32) >>> after it is saved, then verify it in crash_save_vmcoreinfo() to see if >>> the signature was broken, if so we have to re-save the vmcoreinfo data >>> to get the correct vmcoreinfo for kdump as possible as we can. >> Sigh. We already have a sha256 that is supposed to cover this sort of >> thing. The bug rather is that apparently it isn't covering this data. >> That sounds like what we should be fixing. >> >> Please let's not invent new mechanisms we have to maintain. Let's >> reorganize this so this static data is protected like all other static >> data in the kexec-on-panic path. We have good mechanims and good >> strategies for avoiding and detecting corruption we just need to use >> them. >> >> Eric >> >> >> >>> Signed-off-by: Xunlei Pang >>> --- >>> v1->v2: >>> - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" >>> uses the information. >>> - Add crc32 verification for vmcoreinfo, re-save when failure. >>> >>> arch/Kconfig| 1 + >>> kernel/kexec_core.c | 43 +++ >>> 2 files changed, 36 insertions(+), 8 deletions(-) >>> >>> diff --git a/arch/Kconfig b/arch/Kconfig >>> index c4d6833..66eb296 100644 >>> --- a/arch/Kconfig >>> +++ b/arch/Kconfig >>> @@ -4,6 +4,7 @@ >>> >>> config KEXEC_CORE >>> bool >>> + select CRC32 >>> >>> config HAVE_IMA_KEXEC >>> bool >>> diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c >>> index bfe62d5..012acbe 100644 >>> --- a/kernel/kexec_core.c >>> +++ b/kernel/kexec_core.c >>> @@ -38,6 +38,7 @@ >>> #include >>> #include >>> #include >>> +#include >>> >>> #include >>> #include >>> @@ -53,9 +54,10 @@ >>> >>> /* vmcoreinfo stuff */ >>> static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; >>> -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; >>> +static u32 vmcoreinfo_sig; >>> size_t vmcoreinfo_size; >>> size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); >>> +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; >>> >>> /* Flag to indicate we are going to kexec a new kernel */ >>> bool kexec_in_progress = false; >>> @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void) >>> final_note(buf); >>> } >>> >>> -void crash_save_vmcoreinfo(void) >>> -{ >>> - vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds()); >>> - update_vmcoreinfo_note(); >>> -} >>> - >>> void vmcoreinfo_append_str(const char *fmt, ...) >>> { >>> va_list args; >>> @@ -1402,7 +1398,7 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void) >>> return __pa_symbol((unsigned long)(char *)_note); >>> } >>> >>> -static int __init crash_save_vmcoreinfo_init(void) >>> +static void do_crash_save_vmcoreinfo_init(void) >>> { >>> VMCOREINFO_OSRELEASE(init_uts_ns.name.release); >>> VMCOREINFO_PAGESIZE(PAGE_SIZE); >>> @@ -1474,6 +1470,37 @@ static int __init crash_save_vmcoreinfo_init(void) >>> #endif >>> >>> arch_crash_save_vmcoreinfo(); >>> +} >>> + >>> +static
[PATCH] vfio: Rework group release notifier warning
The intent of the original warning is make sure that the mdev vendor driver has removed any group notifiers at the point where the group is closed by the user. Theoretically this would be through an orderly shutdown where any devices are release prior to the group release. We can't always count on an orderly shutdown, the user can close the group before the notifier can be removed or the user task might be killed. We'd like to add this sanity test when the group is idle and the only references are from the devices within the group themselves, but we don't have a good way to do that. Instead check both when the group itself is removed and when the group is opened. A bit later than we'd prefer, but better than the current over aggressive approach. Signed-off-by: Alex Williamson Cc: Jike Song --- drivers/vfio/vfio.c |8 +--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/vfio/vfio.c b/drivers/vfio/vfio.c index 609f4f982c74..561084ab387f 100644 --- a/drivers/vfio/vfio.c +++ b/drivers/vfio/vfio.c @@ -403,6 +403,7 @@ static void vfio_group_release(struct kref *kref) struct iommu_group *iommu_group = group->iommu_group; WARN_ON(!list_empty(>device_list)); + WARN_ON(group->notifier.head); list_for_each_entry_safe(unbound, tmp, >unbound_list, unbound_next) { @@ -1573,6 +1574,10 @@ static int vfio_group_fops_open(struct inode *inode, struct file *filep) return -EBUSY; } + /* Warn if previous user didn't cleanup and re-init to drop them */ + if (WARN_ON(group->notifier.head)) + BLOCKING_INIT_NOTIFIER_HEAD(>notifier); + filep->private_data = group; return 0; @@ -1584,9 +1589,6 @@ static int vfio_group_fops_release(struct inode *inode, struct file *filep) filep->private_data = NULL; - /* Any user didn't unregister? */ - WARN_ON(group->notifier.head); - vfio_group_try_dissolve_container(group); atomic_dec(>opened);
Re: [PATCH v6 5/8] dt-bindings: input: touchscreen: Add max11801-ts binding
On Thu, Mar 16, 2017 at 12:35:38PM +0530, Jagan Teki wrote: > From: Jagan Teki > > Add missing documentation of max11801-ts dt-binding details. > > Cc: Mark Rutland > Cc: Rob Herring > Cc: Shawn Guo > Cc: Michael Trimarchi > Signed-off-by: Jagan Teki > --- > Changes for v6: > - Replace the lable and name of the node >ts: max11801 => max11801: touchscreen@48 > Changes for v5: > - Newly added patch > > .../bindings/input/touchscreen/max11801-ts.txt | 18 > ++ > 1 file changed, 18 insertions(+) > create mode 100644 > Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > > diff --git > a/Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > b/Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > new file mode 100644 > index 000..4afccbe > --- /dev/null > +++ b/Documentation/devicetree/bindings/input/touchscreen/max11801-ts.txt > @@ -0,0 +1,18 @@ > +* MAXI MAX11801 Resistive touch screen controller with i2c interface > + > +Required properties: > +- compatible: must be "max11801" Shouldn't it have a vendor prefix? Also I will need Rob's ACK to get this go via IMX tree. Shawn > +- reg: i2c slave address > +- interrupt-parent: the phandle for the interrupt controller > +- interrupts: touch controller interrupt > + > +Example: > + > + { > + max11801: touchscreen@48 { > + compatible = "max11801"; > + reg = <0x48>; > + interrupt-parent = <>; > + interrupts = <31 2>; > + }; > +}; > -- > 1.9.1 >
Linux 4.11-rc3
Another week, another rc. As is our usual pattern after the merge window, rc3 is larger than rc2, but this is hopefully the point where things start to shrink and calm down. We had a late typo in rc2 that affected arm and powerpc (the prep code for the 5-level page tables), and hopefully there are no similar brown-paper-bugs now in rc3. On the whole rc3 looks pretty normal, with two thirds being driver updates (late qla2xxx scsi driver updates stand out, but ethernet drivers for broadcom and cavium aren't that far behind, and there are updates for gpu, md, cpufreq, x86 platform drivers etc). Outside of drivers, the rest is a mix of arch updates (parisc, powerpc, x86), filesystems (afs, nfs, xfs) and "misc" (mainly core kernel and general networking updates). Shortlog appended for those who want to see some overview of the details, but what we really want is testing. Please. Linus --- Alan Jenkins (9): platform/x86: fujitsu-laptop: clearly denote backlight-related symbols platform/x86: fujitsu-laptop: replace "hotkey" with "laptop" in symbol names platform/x86: fujitsu-laptop: make platform-related variables match naming convention platform/x86: fujitsu-laptop: rename FUNC_RFKILL to FUNC_FLAGS platform/x86: fujitsu-laptop: replace numeric values with constants platform/x86: fujitsu-laptop: remove redundant forward declarations platform/x86: fujitsu-laptop: simplify acpi_bus_register_driver() error handling platform/x86: fujitsu-laptop: autodetect LCD interface on all models platform/x86: fujitsu-laptop: remove redundant MODULE_ALIAS entries Alex Deucher (2): drm/radeon/si: add dpm quirk for Oland drm/amdgpu/si: add dpm quirk for Oland Alexander Potapenko (1): net: initialize msg.msg_flags in recvfrom Alexei Starovoitov (4): bpf: add get_next_key callback to LPM map bpf: fix struct htab_elem layout bpf: convert htab map to hlist_nulls selftests/bpf: fix broken build Alexey Kardashevskiy (2): powerpc/powernv/ioda2: Gracefully fail if too many TCE levels requested powerpc/powernv/ioda2: Update iommu table base on ownership change Alexey Khoroshilov (1): net/sched: act_skbmod: remove unneeded rcu_read_unlock in tcf_skbmod_dump Alexey Kodanev (1): udp: avoid ufo handling on IP payload compression packets Ander Conselvan de Oliveira (1): drm/i915/glk: Fix watermark computations for third sprite plane Andreas Gruenbacher (1): gfs2: Avoid alignment hole in struct lm_lockname Andreea-Cristina Bernat (2): afs: inode: Replace rcu_assign_pointer() with RCU_INIT_POINTER() afs: security: Replace rcu_assign_pointer() with RCU_INIT_POINTER() Andrew Lunn (1): net: phy: marvell: Fix double free of hwmon device Andrey Ryabinin (1): x86/kasan: Fix boot with KASAN=y and PROFILE_ANNOTATED_BRANCHES=y Andrey Vagin (1): net: use net->count to check whether a netns is alive or not Andy Lutomirski (2): x86/perf: Fix CR4.PCE propagation to use active_mm instead of mm x86/perf: Clarify why x86_pmu_event_mapped() isn't racy Andy Shevchenko (3): x86/platform/intel-mid: Correct MSI IRQ line for watchdog device x86/platform/intel-mid: Use common power off sequence x86/platform/intel-mid: Add power button support for Merrifield Anil Gurumurthy (1): qla2xxx: Export DIF stats via debugfs Anton Blanchard (1): scsi: lpfc: Add shutdown method for kexec Arnd Bergmann (5): scsi: qedi: fix build error without DEBUG_FS net/mlx5e: add IPV6 dependency mm, x86: Fix native_pud_clear build error drm: amd: remove broken include path mm, x86: fix native_pud_clear build error Arvind Yadav (1): parisc: perf: Fix potential NULL pointer dereference Bart Van Assche (1): scsi: mpt3sas: Avoid sleeping in interrupt context Blomme, Maarten (2): spi_ks8995: fix "BUG: key accdaa28 not in .data!" spi_ks8995: regs_size incorrect for some devices Chandan Rajendra (1): powerpc: Wire up statx() syscall Chris Leech (1): scsi: libiscsi: add lock around task lists to fix list corruption regression Chris Wilson (6): drm/i915: Squelch any ktime/jiffie rounding errors for wait-ioctl drm/i915/fbdev: Stop repeating tile configuration on stagnation drm/i915: Remove the vma from the drm_mm if binding fails drm/i915: Store a permanent error in obj->mm.pages drm/i915: Use pagecache write to prepopulate shmemfs from pwrite-ioctl drm/i915: Drain the freed state from the tail of the next commit Christian Lamparter (2): dt: emac: document device-tree based phy discovery and setup net: ibm: emac: fix regression caused by emac_dt_phy_probe() Christoph Hellwig (1): scsi: vmw_pvscsi: handle the return value from pci_alloc_irq_vectors correctly Chuck Lever (1): xprtrdma: Squelch kbuild sparse complaint Colin Ian
Linux 4.11-rc3
Another week, another rc. As is our usual pattern after the merge window, rc3 is larger than rc2, but this is hopefully the point where things start to shrink and calm down. We had a late typo in rc2 that affected arm and powerpc (the prep code for the 5-level page tables), and hopefully there are no similar brown-paper-bugs now in rc3. On the whole rc3 looks pretty normal, with two thirds being driver updates (late qla2xxx scsi driver updates stand out, but ethernet drivers for broadcom and cavium aren't that far behind, and there are updates for gpu, md, cpufreq, x86 platform drivers etc). Outside of drivers, the rest is a mix of arch updates (parisc, powerpc, x86), filesystems (afs, nfs, xfs) and "misc" (mainly core kernel and general networking updates). Shortlog appended for those who want to see some overview of the details, but what we really want is testing. Please. Linus --- Alan Jenkins (9): platform/x86: fujitsu-laptop: clearly denote backlight-related symbols platform/x86: fujitsu-laptop: replace "hotkey" with "laptop" in symbol names platform/x86: fujitsu-laptop: make platform-related variables match naming convention platform/x86: fujitsu-laptop: rename FUNC_RFKILL to FUNC_FLAGS platform/x86: fujitsu-laptop: replace numeric values with constants platform/x86: fujitsu-laptop: remove redundant forward declarations platform/x86: fujitsu-laptop: simplify acpi_bus_register_driver() error handling platform/x86: fujitsu-laptop: autodetect LCD interface on all models platform/x86: fujitsu-laptop: remove redundant MODULE_ALIAS entries Alex Deucher (2): drm/radeon/si: add dpm quirk for Oland drm/amdgpu/si: add dpm quirk for Oland Alexander Potapenko (1): net: initialize msg.msg_flags in recvfrom Alexei Starovoitov (4): bpf: add get_next_key callback to LPM map bpf: fix struct htab_elem layout bpf: convert htab map to hlist_nulls selftests/bpf: fix broken build Alexey Kardashevskiy (2): powerpc/powernv/ioda2: Gracefully fail if too many TCE levels requested powerpc/powernv/ioda2: Update iommu table base on ownership change Alexey Khoroshilov (1): net/sched: act_skbmod: remove unneeded rcu_read_unlock in tcf_skbmod_dump Alexey Kodanev (1): udp: avoid ufo handling on IP payload compression packets Ander Conselvan de Oliveira (1): drm/i915/glk: Fix watermark computations for third sprite plane Andreas Gruenbacher (1): gfs2: Avoid alignment hole in struct lm_lockname Andreea-Cristina Bernat (2): afs: inode: Replace rcu_assign_pointer() with RCU_INIT_POINTER() afs: security: Replace rcu_assign_pointer() with RCU_INIT_POINTER() Andrew Lunn (1): net: phy: marvell: Fix double free of hwmon device Andrey Ryabinin (1): x86/kasan: Fix boot with KASAN=y and PROFILE_ANNOTATED_BRANCHES=y Andrey Vagin (1): net: use net->count to check whether a netns is alive or not Andy Lutomirski (2): x86/perf: Fix CR4.PCE propagation to use active_mm instead of mm x86/perf: Clarify why x86_pmu_event_mapped() isn't racy Andy Shevchenko (3): x86/platform/intel-mid: Correct MSI IRQ line for watchdog device x86/platform/intel-mid: Use common power off sequence x86/platform/intel-mid: Add power button support for Merrifield Anil Gurumurthy (1): qla2xxx: Export DIF stats via debugfs Anton Blanchard (1): scsi: lpfc: Add shutdown method for kexec Arnd Bergmann (5): scsi: qedi: fix build error without DEBUG_FS net/mlx5e: add IPV6 dependency mm, x86: Fix native_pud_clear build error drm: amd: remove broken include path mm, x86: fix native_pud_clear build error Arvind Yadav (1): parisc: perf: Fix potential NULL pointer dereference Bart Van Assche (1): scsi: mpt3sas: Avoid sleeping in interrupt context Blomme, Maarten (2): spi_ks8995: fix "BUG: key accdaa28 not in .data!" spi_ks8995: regs_size incorrect for some devices Chandan Rajendra (1): powerpc: Wire up statx() syscall Chris Leech (1): scsi: libiscsi: add lock around task lists to fix list corruption regression Chris Wilson (6): drm/i915: Squelch any ktime/jiffie rounding errors for wait-ioctl drm/i915/fbdev: Stop repeating tile configuration on stagnation drm/i915: Remove the vma from the drm_mm if binding fails drm/i915: Store a permanent error in obj->mm.pages drm/i915: Use pagecache write to prepopulate shmemfs from pwrite-ioctl drm/i915: Drain the freed state from the tail of the next commit Christian Lamparter (2): dt: emac: document device-tree based phy discovery and setup net: ibm: emac: fix regression caused by emac_dt_phy_probe() Christoph Hellwig (1): scsi: vmw_pvscsi: handle the return value from pci_alloc_irq_vectors correctly Chuck Lever (1): xprtrdma: Squelch kbuild sparse complaint Colin Ian
linux-next: manual merge of the tty tree with the tty.current tree
Hi Greg, Today's linux-next merge of the tty tree got a conflict in: drivers/tty/tty_ldisc.c between commit: 5362544bebe8 ("tty: don't panic on OOM in tty_set_ldisc()") from the tty.current tree and commit: 71472fa9c52b ("tty: Fix ldisc crash on reopened tty") from the tty tree. I fixed it up (see below) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell diff --cc drivers/tty/tty_ldisc.c index b0500a0a87b8,4ee7742dced3.. --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@@ -621,14 -669,17 +621,15 @@@ int tty_ldisc_reinit(struct tty_struct tty_ldisc_put(tty->ldisc); } - /* switch the line discipline */ - tty->ldisc = ld; tty_set_termios_ldisc(tty, disc); - retval = tty_ldisc_open(tty, tty->ldisc); + retval = tty_ldisc_open(tty, ld); if (retval) { - tty_ldisc_put(tty->ldisc); - tty->ldisc = NULL; - if (!WARN_ON(disc == N_TTY)) { - tty_ldisc_put(ld); - ld = NULL; - } ++ tty_ldisc_put(ld); ++ ld = NULL; } + + /* switch the line discipline */ + smp_store_release(>ldisc, ld); return retval; }
linux-next: manual merge of the tty tree with the tty.current tree
Hi Greg, Today's linux-next merge of the tty tree got a conflict in: drivers/tty/tty_ldisc.c between commit: 5362544bebe8 ("tty: don't panic on OOM in tty_set_ldisc()") from the tty.current tree and commit: 71472fa9c52b ("tty: Fix ldisc crash on reopened tty") from the tty tree. I fixed it up (see below) and can carry the fix as necessary. This is now fixed as far as linux-next is concerned, but any non trivial conflicts should be mentioned to your upstream maintainer when your tree is submitted for merging. You may also want to consider cooperating with the maintainer of the conflicting tree to minimise any particularly complex conflicts. -- Cheers, Stephen Rothwell diff --cc drivers/tty/tty_ldisc.c index b0500a0a87b8,4ee7742dced3.. --- a/drivers/tty/tty_ldisc.c +++ b/drivers/tty/tty_ldisc.c @@@ -621,14 -669,17 +621,15 @@@ int tty_ldisc_reinit(struct tty_struct tty_ldisc_put(tty->ldisc); } - /* switch the line discipline */ - tty->ldisc = ld; tty_set_termios_ldisc(tty, disc); - retval = tty_ldisc_open(tty, tty->ldisc); + retval = tty_ldisc_open(tty, ld); if (retval) { - tty_ldisc_put(tty->ldisc); - tty->ldisc = NULL; - if (!WARN_ON(disc == N_TTY)) { - tty_ldisc_put(ld); - ld = NULL; - } ++ tty_ldisc_put(ld); ++ ld = NULL; } + + /* switch the line discipline */ + smp_store_release(>ldisc, ld); return retval; }
Re: [PATCH] x86/mm: trivial code cleanup for memory_map_top_doown()
Hi, Borislav Do you still have some concern on this change? On Tue, Mar 14, 2017 at 11:56:39AM +0800, Wei Yang wrote: >On Mon, Mar 13, 2017 at 07:50:21PM +0100, Borislav Petkov wrote: >>On Fri, Feb 17, 2017 at 10:30:33PM +0800, Wei Yang wrote: >>> In case (last_start <= step_size), start is for sure to be 0. So, it is >> > >Hmm, I may write it more specific: > >"start" is for sure to be set to 0 with round_down(last_start - 1, step_size). > >>Well, lemme see: >> >>[0.00] memory_map_top_down: entry, [0x10:0x7ffdf000) >>[0.00] memory_map_top_down: addr: 0x7fc0, real_end: 0x7fe0 >>[0.00] memory_map_top_down: last_start: 0x4000 <= step_size: >>0x20, start: 0x4000 >> >> ^^ >>It doesn't look like 0 to me. >> >>--- >>diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c >>index 2193799ca800..d3b02a416df3 100644 >>--- a/arch/x86/mm/init.c >>+++ b/arch/x86/mm/init.c >>@@ -527,8 +531,13 @@ static void __init memory_map_top_down(unsigned long >>map_start, >> start = round_down(last_start - 1, step_size); >> if (start < map_start) >> start = map_start; >>- } else >>+ } else { >>+ pr_info("%s: last_start: 0x%lx <= step_size: 0x%lx, >>start: 0x%lx\n", >>+ __func__, last_start, step_size, start); >>+ > >If you change this log with the following > > pr_err("%s: last_start: 0x%lx <= step_size: 0x%lx, > start: 0x%lx\n", > __func__, last_start, step_size, > round_down(last_start - 1, step_size)); > >You could see after calculation, start is 0 when (last_start <= step_size). > >-- >Wei Yang >Help you, Help me -- Wei Yang Help you, Help me signature.asc Description: PGP signature
Re: [PATCH] x86/mm: trivial code cleanup for memory_map_top_doown()
Hi, Borislav Do you still have some concern on this change? On Tue, Mar 14, 2017 at 11:56:39AM +0800, Wei Yang wrote: >On Mon, Mar 13, 2017 at 07:50:21PM +0100, Borislav Petkov wrote: >>On Fri, Feb 17, 2017 at 10:30:33PM +0800, Wei Yang wrote: >>> In case (last_start <= step_size), start is for sure to be 0. So, it is >> > >Hmm, I may write it more specific: > >"start" is for sure to be set to 0 with round_down(last_start - 1, step_size). > >>Well, lemme see: >> >>[0.00] memory_map_top_down: entry, [0x10:0x7ffdf000) >>[0.00] memory_map_top_down: addr: 0x7fc0, real_end: 0x7fe0 >>[0.00] memory_map_top_down: last_start: 0x4000 <= step_size: >>0x20, start: 0x4000 >> >> ^^ >>It doesn't look like 0 to me. >> >>--- >>diff --git a/arch/x86/mm/init.c b/arch/x86/mm/init.c >>index 2193799ca800..d3b02a416df3 100644 >>--- a/arch/x86/mm/init.c >>+++ b/arch/x86/mm/init.c >>@@ -527,8 +531,13 @@ static void __init memory_map_top_down(unsigned long >>map_start, >> start = round_down(last_start - 1, step_size); >> if (start < map_start) >> start = map_start; >>- } else >>+ } else { >>+ pr_info("%s: last_start: 0x%lx <= step_size: 0x%lx, >>start: 0x%lx\n", >>+ __func__, last_start, step_size, start); >>+ > >If you change this log with the following > > pr_err("%s: last_start: 0x%lx <= step_size: 0x%lx, > start: 0x%lx\n", > __func__, last_start, step_size, > round_down(last_start - 1, step_size)); > >You could see after calculation, start is 0 when (last_start <= step_size). > >-- >Wei Yang >Help you, Help me -- Wei Yang Help you, Help me signature.asc Description: PGP signature
[PATCH] backlight: Add TI LMU backlight driver
This is consolidated driver which supports backlight devices below. LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697. Structure - It consists of two parts - core and data. Core part supports features below. - Backlight subsystem control - Channel configuration from DT properties - Light dimming effect control: ramp up and down. - LMU fault monitor notifier handling - PWM brightness control Data part describes device specific data. - Register value configuration for each LMU device : initialization, channel configuration, control mode, enable and brightness. - PWM action configuration - Light dimming effect table - Option for LMU fault monitor support Macros for register data All LMU devices have 8-bit based registers. LMU_BL_REG() creates 24-bit register value in data part. It consists of address, mask and value. On the other hand, register value should be parsed when the driver reads/writes data from/to I2C registers. Driver uses LMU_BL_GET_ADDR(), LMU_BL_GET_MASK() and LMU_BL_GET_VAL() for this purpose. Data structure -- ti_lmu_bl: Backlight output channel data ti_lmu_bl_chip:Backlight device data. One device can have multiple backlight channel data. ti_lmu_bl_reg: Backlight device register data ti_lmu_bl_cfg: Backlight configuration data for each LMU device Cc: Rob HerringCc: Sebastian Reichel Cc: Tony Lindgren Signed-off-by: Milo Kim --- .../bindings/leds/backlight/ti-lmu-backlight.txt | 65 ++ drivers/video/backlight/Kconfig| 7 + drivers/video/backlight/Makefile | 3 + drivers/video/backlight/ti-lmu-backlight-core.c| 655 + drivers/video/backlight/ti-lmu-backlight-data.c| 287 + include/linux/mfd/ti-lmu-backlight.h | 290 + 6 files changed, 1307 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt create mode 100644 drivers/video/backlight/ti-lmu-backlight-core.c create mode 100644 drivers/video/backlight/ti-lmu-backlight-data.c create mode 100644 include/linux/mfd/ti-lmu-backlight.h diff --git a/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt new file mode 100644 index ..c2c35b293716 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt @@ -0,0 +1,65 @@ +TI LMU backlight device tree bindings + +Required property: + - compatible: Should be one of: +"ti,lm3532-backlight" +"ti,lm3631-backlight" +"ti,lm3632-backlight" +"ti,lm3633-backlight" +"ti,lm3695-backlight" +"ti,lm3697-backlight" + +Optional properties: + There are two backlight control mode. One is I2C, the other is PWM mode. + Following properties are only specified in PWM mode. + Please note that LMU backlight device can have only one PWM channel. + + - pwms: OF device-tree PWM specification. + - pwm-names: a list of names for the PWM devices specified in the "pwms" + property. + + For the PWM user nodes, please refer to [1]. + +Child nodes: + LMU backlight is represented as sub-nodes of the TI LMU device [2]. + So, LMU backlight should have more than one backlight child node. + Each node exactly matches with backlight control bank configuration. + Maximum numbers of child nodes depend on the device. + 1 = LM3631, LM3632, LM3695 + 2 = LM3633, LM3697 + 3 = LM3532 + + Required property of a child node: + - led-sources: List of enabled channels from 0 to 2. + Please refer to LED binding [3]. + For output channels, please refer to the datasheets [4]. + + Optional properties of a child node: + - label: Backlight channel identification. + Please refer to LED binding [3]. + - default-brightness-level: Backlight initial brightness value. + Type is . It is set as soon as backlight + device is created. + 0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and + LM3697 + 0 ~ 255 = LM3532 + - ramp-up-msec, ramp-down-msec: Light dimming effect properties. + Type is . Unit is millisecond. + 0 ~ 65 msec= LM3532 + 0 ~ 4000 msec = LM3631 + 0 ~ 16000 msec = LM3633 and LM3697 + - pwm-period: PWM period. Only valid in PWM brightness mode. +Type is . If this property is missing, then control +mode is set to I2C by default. + +Examples: Please
[PATCH] backlight: Add TI LMU backlight driver
This is consolidated driver which supports backlight devices below. LM3532, LM3631, LM3632, LM3633, LM3695 and LM3697. Structure - It consists of two parts - core and data. Core part supports features below. - Backlight subsystem control - Channel configuration from DT properties - Light dimming effect control: ramp up and down. - LMU fault monitor notifier handling - PWM brightness control Data part describes device specific data. - Register value configuration for each LMU device : initialization, channel configuration, control mode, enable and brightness. - PWM action configuration - Light dimming effect table - Option for LMU fault monitor support Macros for register data All LMU devices have 8-bit based registers. LMU_BL_REG() creates 24-bit register value in data part. It consists of address, mask and value. On the other hand, register value should be parsed when the driver reads/writes data from/to I2C registers. Driver uses LMU_BL_GET_ADDR(), LMU_BL_GET_MASK() and LMU_BL_GET_VAL() for this purpose. Data structure -- ti_lmu_bl: Backlight output channel data ti_lmu_bl_chip:Backlight device data. One device can have multiple backlight channel data. ti_lmu_bl_reg: Backlight device register data ti_lmu_bl_cfg: Backlight configuration data for each LMU device Cc: Rob Herring Cc: Sebastian Reichel Cc: Tony Lindgren Signed-off-by: Milo Kim --- .../bindings/leds/backlight/ti-lmu-backlight.txt | 65 ++ drivers/video/backlight/Kconfig| 7 + drivers/video/backlight/Makefile | 3 + drivers/video/backlight/ti-lmu-backlight-core.c| 655 + drivers/video/backlight/ti-lmu-backlight-data.c| 287 + include/linux/mfd/ti-lmu-backlight.h | 290 + 6 files changed, 1307 insertions(+) create mode 100644 Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt create mode 100644 drivers/video/backlight/ti-lmu-backlight-core.c create mode 100644 drivers/video/backlight/ti-lmu-backlight-data.c create mode 100644 include/linux/mfd/ti-lmu-backlight.h diff --git a/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt new file mode 100644 index ..c2c35b293716 --- /dev/null +++ b/Documentation/devicetree/bindings/leds/backlight/ti-lmu-backlight.txt @@ -0,0 +1,65 @@ +TI LMU backlight device tree bindings + +Required property: + - compatible: Should be one of: +"ti,lm3532-backlight" +"ti,lm3631-backlight" +"ti,lm3632-backlight" +"ti,lm3633-backlight" +"ti,lm3695-backlight" +"ti,lm3697-backlight" + +Optional properties: + There are two backlight control mode. One is I2C, the other is PWM mode. + Following properties are only specified in PWM mode. + Please note that LMU backlight device can have only one PWM channel. + + - pwms: OF device-tree PWM specification. + - pwm-names: a list of names for the PWM devices specified in the "pwms" + property. + + For the PWM user nodes, please refer to [1]. + +Child nodes: + LMU backlight is represented as sub-nodes of the TI LMU device [2]. + So, LMU backlight should have more than one backlight child node. + Each node exactly matches with backlight control bank configuration. + Maximum numbers of child nodes depend on the device. + 1 = LM3631, LM3632, LM3695 + 2 = LM3633, LM3697 + 3 = LM3532 + + Required property of a child node: + - led-sources: List of enabled channels from 0 to 2. + Please refer to LED binding [3]. + For output channels, please refer to the datasheets [4]. + + Optional properties of a child node: + - label: Backlight channel identification. + Please refer to LED binding [3]. + - default-brightness-level: Backlight initial brightness value. + Type is . It is set as soon as backlight + device is created. + 0 ~ 2047 = LM3631, LM3632, LM3633, LM3695 and + LM3697 + 0 ~ 255 = LM3532 + - ramp-up-msec, ramp-down-msec: Light dimming effect properties. + Type is . Unit is millisecond. + 0 ~ 65 msec= LM3532 + 0 ~ 4000 msec = LM3631 + 0 ~ 16000 msec = LM3633 and LM3697 + - pwm-period: PWM period. Only valid in PWM brightness mode. +Type is . If this property is missing, then control +mode is set to I2C by default. + +Examples: Please refer to ti-lmu dt-bindings. [2]. + +[1] ../pwm/pwm.txt +[2]
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/17/17 at 12:22pm, Eric W. Biederman wrote: > Xunlei Pangwrites: > > > Currently vmcoreinfo data is updated at boot time subsys_initcall(), > > it has the risk of being modified by some wrong code during system > > is running. > > > > As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, > > when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, > > we probably will get "Segmentation fault" or other unexpected/confusing > > errors. > > If this is a real concern and the previous discussion sounds like it is > part of what we need to do is move the variable vmcoreinfo_note out > of the kernel's .bss section. And modify the code to regenerate > and keep this information in something like the control page. I guess this is not from a real issue, just from Xunlei's worry. But Xunlei didn't give a direct answer to this, and Petr's question. Not very sure if this will impact other implementation. fadump will be impacted by this or other dump? Maybe yet or maybe not. I don't object this strongly, but please at least add code comment to explain why vmcoreinfo need be saved twice because it does look weird. > > Definitely something like this needs a page all to itself, and ideally > far away from any other kernel data structures. I clearly was not > watching closely the data someone decided to keep this silly thing > in the kernel's .bss section. > > > As vmcoreinfo is the most fundamental information for vmcore, we better > > double check its correctness. Here we generate a signature(using crc32) > > after it is saved, then verify it in crash_save_vmcoreinfo() to see if > > the signature was broken, if so we have to re-save the vmcoreinfo data > > to get the correct vmcoreinfo for kdump as possible as we can. > > Sigh. We already have a sha256 that is supposed to cover this sort of > thing. The bug rather is that apparently it isn't covering this data. > That sounds like what we should be fixing. > > Please let's not invent new mechanisms we have to maintain. Let's > reorganize this so this static data is protected like all other static > data in the kexec-on-panic path. We have good mechanims and good > strategies for avoiding and detecting corruption we just need to use > them. > > Eric > > > > > Signed-off-by: Xunlei Pang > > --- > > v1->v2: > > - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" > > uses the information. > > - Add crc32 verification for vmcoreinfo, re-save when failure. > > > > arch/Kconfig| 1 + > > kernel/kexec_core.c | 43 +++ > > 2 files changed, 36 insertions(+), 8 deletions(-) > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index c4d6833..66eb296 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -4,6 +4,7 @@ > > > > config KEXEC_CORE > > bool > > + select CRC32 > > > > config HAVE_IMA_KEXEC > > bool > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > index bfe62d5..012acbe 100644 > > --- a/kernel/kexec_core.c > > +++ b/kernel/kexec_core.c > > @@ -38,6 +38,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -53,9 +54,10 @@ > > > > /* vmcoreinfo stuff */ > > static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; > > -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > > +static u32 vmcoreinfo_sig; > > size_t vmcoreinfo_size; > > size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); > > +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > > > > /* Flag to indicate we are going to kexec a new kernel */ > > bool kexec_in_progress = false; > > @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void) > > final_note(buf); > > } > > > > -void crash_save_vmcoreinfo(void) > > -{ > > - vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds()); > > - update_vmcoreinfo_note(); > > -} > > - > > void vmcoreinfo_append_str(const char *fmt, ...) > > { > > va_list args; > > @@ -1402,7 +1398,7 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void) > > return __pa_symbol((unsigned long)(char *)_note); > > } > > > > -static int __init crash_save_vmcoreinfo_init(void) > > +static void do_crash_save_vmcoreinfo_init(void) > > { > > VMCOREINFO_OSRELEASE(init_uts_ns.name.release); > > VMCOREINFO_PAGESIZE(PAGE_SIZE); > > @@ -1474,6 +1470,37 @@ static int __init crash_save_vmcoreinfo_init(void) > > #endif > > > > arch_crash_save_vmcoreinfo(); > > +} > > + > > +static u32 crash_calc_vmcoreinfo_sig(void) > > +{ > > + return crc32(~0, vmcoreinfo_data, vmcoreinfo_size); > > +} > > + > > +static bool crash_verify_vmcoreinfo(void) > > +{ > > + if (crash_calc_vmcoreinfo_sig() == vmcoreinfo_sig) > > + return true; > > + > > + return false; > > +} > > + > > +void crash_save_vmcoreinfo(void) > > +{ > > + /* Re-save if verification fails */ > > + if (!crash_verify_vmcoreinfo()) { > > +
Re: [PATCH v2] kexec: Introduce vmcoreinfo signature verification
On 03/17/17 at 12:22pm, Eric W. Biederman wrote: > Xunlei Pang writes: > > > Currently vmcoreinfo data is updated at boot time subsys_initcall(), > > it has the risk of being modified by some wrong code during system > > is running. > > > > As a result, vmcore dumped may contain the wrong vmcoreinfo. Later on, > > when using "crash" or "makedumpfile"(etc) utility to parse this vmcore, > > we probably will get "Segmentation fault" or other unexpected/confusing > > errors. > > If this is a real concern and the previous discussion sounds like it is > part of what we need to do is move the variable vmcoreinfo_note out > of the kernel's .bss section. And modify the code to regenerate > and keep this information in something like the control page. I guess this is not from a real issue, just from Xunlei's worry. But Xunlei didn't give a direct answer to this, and Petr's question. Not very sure if this will impact other implementation. fadump will be impacted by this or other dump? Maybe yet or maybe not. I don't object this strongly, but please at least add code comment to explain why vmcoreinfo need be saved twice because it does look weird. > > Definitely something like this needs a page all to itself, and ideally > far away from any other kernel data structures. I clearly was not > watching closely the data someone decided to keep this silly thing > in the kernel's .bss section. > > > As vmcoreinfo is the most fundamental information for vmcore, we better > > double check its correctness. Here we generate a signature(using crc32) > > after it is saved, then verify it in crash_save_vmcoreinfo() to see if > > the signature was broken, if so we have to re-save the vmcoreinfo data > > to get the correct vmcoreinfo for kdump as possible as we can. > > Sigh. We already have a sha256 that is supposed to cover this sort of > thing. The bug rather is that apparently it isn't covering this data. > That sounds like what we should be fixing. > > Please let's not invent new mechanisms we have to maintain. Let's > reorganize this so this static data is protected like all other static > data in the kexec-on-panic path. We have good mechanims and good > strategies for avoiding and detecting corruption we just need to use > them. > > Eric > > > > > Signed-off-by: Xunlei Pang > > --- > > v1->v2: > > - Keep crash_save_vmcoreinfo_init() because "makedumpfile --mem-usage" > > uses the information. > > - Add crc32 verification for vmcoreinfo, re-save when failure. > > > > arch/Kconfig| 1 + > > kernel/kexec_core.c | 43 +++ > > 2 files changed, 36 insertions(+), 8 deletions(-) > > > > diff --git a/arch/Kconfig b/arch/Kconfig > > index c4d6833..66eb296 100644 > > --- a/arch/Kconfig > > +++ b/arch/Kconfig > > @@ -4,6 +4,7 @@ > > > > config KEXEC_CORE > > bool > > + select CRC32 > > > > config HAVE_IMA_KEXEC > > bool > > diff --git a/kernel/kexec_core.c b/kernel/kexec_core.c > > index bfe62d5..012acbe 100644 > > --- a/kernel/kexec_core.c > > +++ b/kernel/kexec_core.c > > @@ -38,6 +38,7 @@ > > #include > > #include > > #include > > +#include > > > > #include > > #include > > @@ -53,9 +54,10 @@ > > > > /* vmcoreinfo stuff */ > > static unsigned char vmcoreinfo_data[VMCOREINFO_BYTES]; > > -u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > > +static u32 vmcoreinfo_sig; > > size_t vmcoreinfo_size; > > size_t vmcoreinfo_max_size = sizeof(vmcoreinfo_data); > > +u32 vmcoreinfo_note[VMCOREINFO_NOTE_SIZE/4]; > > > > /* Flag to indicate we are going to kexec a new kernel */ > > bool kexec_in_progress = false; > > @@ -1367,12 +1369,6 @@ static void update_vmcoreinfo_note(void) > > final_note(buf); > > } > > > > -void crash_save_vmcoreinfo(void) > > -{ > > - vmcoreinfo_append_str("CRASHTIME=%ld\n", get_seconds()); > > - update_vmcoreinfo_note(); > > -} > > - > > void vmcoreinfo_append_str(const char *fmt, ...) > > { > > va_list args; > > @@ -1402,7 +1398,7 @@ phys_addr_t __weak paddr_vmcoreinfo_note(void) > > return __pa_symbol((unsigned long)(char *)_note); > > } > > > > -static int __init crash_save_vmcoreinfo_init(void) > > +static void do_crash_save_vmcoreinfo_init(void) > > { > > VMCOREINFO_OSRELEASE(init_uts_ns.name.release); > > VMCOREINFO_PAGESIZE(PAGE_SIZE); > > @@ -1474,6 +1470,37 @@ static int __init crash_save_vmcoreinfo_init(void) > > #endif > > > > arch_crash_save_vmcoreinfo(); > > +} > > + > > +static u32 crash_calc_vmcoreinfo_sig(void) > > +{ > > + return crc32(~0, vmcoreinfo_data, vmcoreinfo_size); > > +} > > + > > +static bool crash_verify_vmcoreinfo(void) > > +{ > > + if (crash_calc_vmcoreinfo_sig() == vmcoreinfo_sig) > > + return true; > > + > > + return false; > > +} > > + > > +void crash_save_vmcoreinfo(void) > > +{ > > + /* Re-save if verification fails */ > > + if (!crash_verify_vmcoreinfo()) { > > + vmcoreinfo_size = 0; > > +
Re: [PATCH] x86_64, kexec: Avoid unnecessary identity mappings for kdump
On 03/18/2017 at 01:38 AM, Eric W. Biederman wrote: > Xunlei Pangwrites: > >> kexec setups identity mappings for all the memory mapped in 1st kernel, >> this is not necessary for the kdump case. Actually it can cause extra >> memory consumption for paging structures, which is quite considerable >> on modern machines with huge memory. >> >> E.g. On our 24TB machine, it will waste around 96MB (around 4MB/TB) >> from the reserved memory range if setting all the identity mappings. >> >> It also causes some trouble for distributions that use an intelligent >> policy to evaluate the proper "crashkernel=X" for users. >> >> To solve it, in case of kdump, we only setup identity mappings for the >> crash memory and the ISA memory(may be needed by purgatory/kdump >> boot). > How about instead we detect the presence of 1GiB pages and use them > if they are available. We already use 2MiB pages. If we can do that > we will only need about 192K for page tables in the case you have > described and this all becomes a non-issue. > > I strongly suspect that the presence of 24TiB of memory in an x86 system > strongly correlates to the presence of 1GiB pages. > > In principle we certainly can use a less extensive mapping but that > should not be something that differs between the two kexec cases. Ok, will try gbpages for the identity mapping. Regards, Xunlei > I can see forcing the low 1MiB range in. But calling it ISA range is > very wrong and misleading. The reasons that range are special during > boot-up have nothing to do with ISA. But have everything to do with > where legacy page tables are mapped, and where we need identity pages to > start other cpus. I think the only user that actually cares is > purgatory where it plays swapping games with the low 1MiB because we > can't preload what we need to down there or it would mess up the running > kernel. So saying anything about the old ISA bus is wrong and > misleading. At the very very least we need accurate comments. > > Eric > > >> Signed-off-by: Xunlei Pang >> --- >> arch/x86/kernel/machine_kexec_64.c | 34 ++ >> 1 file changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kernel/machine_kexec_64.c >> b/arch/x86/kernel/machine_kexec_64.c >> index 857cdbd..db77a76 100644 >> --- a/arch/x86/kernel/machine_kexec_64.c >> +++ b/arch/x86/kernel/machine_kexec_64.c >> @@ -112,14 +112,40 @@ static int init_pgtable(struct kimage *image, unsigned >> long start_pgtable) >> >> level4p = (pgd_t *)__va(start_pgtable); >> clear_page(level4p); >> -for (i = 0; i < nr_pfn_mapped; i++) { >> -mstart = pfn_mapped[i].start << PAGE_SHIFT; >> -mend = pfn_mapped[i].end << PAGE_SHIFT; >> >> +if (image->type == KEXEC_TYPE_CRASH) { >> +/* Always map the ISA range */ >> result = kernel_ident_mapping_init(, >> - level4p, mstart, mend); >> +level4p, 0, ISA_END_ADDRESS); >> if (result) >> return result; >> + >> +/* crashk_low_res may not be initialized when reaching here */ >> +if (crashk_low_res.end) { >> +mstart = crashk_low_res.start; >> +mend = crashk_low_res.end + 1; >> +result = kernel_ident_mapping_init(, >> +level4p, mstart, mend); >> +if (result) >> +return result; >> +} >> + >> +mstart = crashk_res.start; >> +mend = crashk_res.end + 1; >> +result = kernel_ident_mapping_init(, >> +level4p, mstart, mend); >> +if (result) >> +return result; >> +} else { >> +for (i = 0; i < nr_pfn_mapped; i++) { >> +mstart = pfn_mapped[i].start << PAGE_SHIFT; >> +mend = pfn_mapped[i].end << PAGE_SHIFT; >> + >> +result = kernel_ident_mapping_init(, >> + level4p, mstart, mend); >> +if (result) >> +return result; >> +} >> } >> >> /*
Re: [PATCH] x86_64, kexec: Avoid unnecessary identity mappings for kdump
On 03/18/2017 at 01:38 AM, Eric W. Biederman wrote: > Xunlei Pang writes: > >> kexec setups identity mappings for all the memory mapped in 1st kernel, >> this is not necessary for the kdump case. Actually it can cause extra >> memory consumption for paging structures, which is quite considerable >> on modern machines with huge memory. >> >> E.g. On our 24TB machine, it will waste around 96MB (around 4MB/TB) >> from the reserved memory range if setting all the identity mappings. >> >> It also causes some trouble for distributions that use an intelligent >> policy to evaluate the proper "crashkernel=X" for users. >> >> To solve it, in case of kdump, we only setup identity mappings for the >> crash memory and the ISA memory(may be needed by purgatory/kdump >> boot). > How about instead we detect the presence of 1GiB pages and use them > if they are available. We already use 2MiB pages. If we can do that > we will only need about 192K for page tables in the case you have > described and this all becomes a non-issue. > > I strongly suspect that the presence of 24TiB of memory in an x86 system > strongly correlates to the presence of 1GiB pages. > > In principle we certainly can use a less extensive mapping but that > should not be something that differs between the two kexec cases. Ok, will try gbpages for the identity mapping. Regards, Xunlei > I can see forcing the low 1MiB range in. But calling it ISA range is > very wrong and misleading. The reasons that range are special during > boot-up have nothing to do with ISA. But have everything to do with > where legacy page tables are mapped, and where we need identity pages to > start other cpus. I think the only user that actually cares is > purgatory where it plays swapping games with the low 1MiB because we > can't preload what we need to down there or it would mess up the running > kernel. So saying anything about the old ISA bus is wrong and > misleading. At the very very least we need accurate comments. > > Eric > > >> Signed-off-by: Xunlei Pang >> --- >> arch/x86/kernel/machine_kexec_64.c | 34 ++ >> 1 file changed, 30 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kernel/machine_kexec_64.c >> b/arch/x86/kernel/machine_kexec_64.c >> index 857cdbd..db77a76 100644 >> --- a/arch/x86/kernel/machine_kexec_64.c >> +++ b/arch/x86/kernel/machine_kexec_64.c >> @@ -112,14 +112,40 @@ static int init_pgtable(struct kimage *image, unsigned >> long start_pgtable) >> >> level4p = (pgd_t *)__va(start_pgtable); >> clear_page(level4p); >> -for (i = 0; i < nr_pfn_mapped; i++) { >> -mstart = pfn_mapped[i].start << PAGE_SHIFT; >> -mend = pfn_mapped[i].end << PAGE_SHIFT; >> >> +if (image->type == KEXEC_TYPE_CRASH) { >> +/* Always map the ISA range */ >> result = kernel_ident_mapping_init(, >> - level4p, mstart, mend); >> +level4p, 0, ISA_END_ADDRESS); >> if (result) >> return result; >> + >> +/* crashk_low_res may not be initialized when reaching here */ >> +if (crashk_low_res.end) { >> +mstart = crashk_low_res.start; >> +mend = crashk_low_res.end + 1; >> +result = kernel_ident_mapping_init(, >> +level4p, mstart, mend); >> +if (result) >> +return result; >> +} >> + >> +mstart = crashk_res.start; >> +mend = crashk_res.end + 1; >> +result = kernel_ident_mapping_init(, >> +level4p, mstart, mend); >> +if (result) >> +return result; >> +} else { >> +for (i = 0; i < nr_pfn_mapped; i++) { >> +mstart = pfn_mapped[i].start << PAGE_SHIFT; >> +mend = pfn_mapped[i].end << PAGE_SHIFT; >> + >> +result = kernel_ident_mapping_init(, >> + level4p, mstart, mend); >> +if (result) >> +return result; >> +} >> } >> >> /*
Re: [PATCH] kexec: Update vmcoreinfo after crash happened
On 03/19/2017 at 02:23 AM, Petr Tesarik wrote: > On Thu, 16 Mar 2017 21:40:58 +0800 > Xunlei Pangwrote: > >> On 03/16/2017 at 09:18 PM, Baoquan He wrote: >>> On 03/16/17 at 08:36pm, Xunlei Pang wrote: On 03/16/2017 at 08:27 PM, Baoquan He wrote: > Hi Xunlei, > > Did you really see this ever happened? Because the vmcore size estimate > feature, namely --mem-usage option of makedumpfile, depends on the > vmcoreinfo in 1st kernel, your change will break it. Hi Baoquan, I can reproduce it using a kernel module which modifies the vmcoreinfo, so it's a problem can actually happen. > If not, it could be not good to change that. That's a good point, then I guess we can keep the crash_save_vmcoreinfo_init(), and store again all the vmcoreinfo after crash. What do you think? >>> Well, then it will make makedumpfile segfault happen too when execute >>> below command in 1st kernel if it existed: >>> makedumpfile --mem-usage /proc/kcore >> Yes, if the initial vmcoreinfo data was modified before "makedumpfile >> --mem-usage", it might happen, >> after all the system is going something wrong. And that's why we deploy >> kdump service at the very >> beginning when the system has a low possibility of going wrong. >> >> But we have to guarantee kdump vmcore can be generated correctly as possible >> as it can. >> >>> So we still need to face that problem and need fix it. vmcoreinfo_note >>> is in kernel data area, how does module intrude into this area? And can >>> we fix the module code? >>> >> Bugs always exist in products, we can't know what will happen and fix all >> the errors, >> that's why we need kdump. >> >> I think the following update should guarantee the correct vmcoreinfo for >> kdump. > I'm still not convinced. I would probably have more trust in a clean > kernel (after boot) than a kernel that has already crashed (presumably > because of a serious bug). How can be reliability improved by running > more code in unsafe environment? Correct, I realized that, so used crc32 to protect the original data, but since Eric left a more reasonable idea, I will try that later. > > If some code overwrites reserved areas (such as vmcoreinfo), then it's > seriously buggy. And in my opinion, it is more difficult to identify > such bugs if they are masked by re-initializing vmcoreinfo after crash. > In fact, if makedumpfile in the kexec'ed kernel complains that it > didn't find valid VMCOREINFO content, that's already a hint. > > As a side note, if you're debugging a vmcoreinfo corruption, it's > possible to use a standalone VMCOREINFO file with makedumpfile, so you > can pre-generate it and save it in the kdump initrd. > > In short, I don't see a compelling case for this change. E.g. 1) wrong code overwrites vmcoreinfo_data; 2) further crashes the system; 3) trigger kdump, then we obviously will fail to recognize the crash context correctly due to the corrupted vmcoreinfo. Everyone will get confused if met such unfortunate customer-side issue. Although it's corner case, if it's easy to fix, then I think we better do it. Now except for vmcoreinfo, all the crash data is well protected (including cpu note which is fully updated in the crash path, thus its correctness is guaranteed). Regards, Xunlei
Re: [PATCH] kexec: Update vmcoreinfo after crash happened
On 03/19/2017 at 02:23 AM, Petr Tesarik wrote: > On Thu, 16 Mar 2017 21:40:58 +0800 > Xunlei Pang wrote: > >> On 03/16/2017 at 09:18 PM, Baoquan He wrote: >>> On 03/16/17 at 08:36pm, Xunlei Pang wrote: On 03/16/2017 at 08:27 PM, Baoquan He wrote: > Hi Xunlei, > > Did you really see this ever happened? Because the vmcore size estimate > feature, namely --mem-usage option of makedumpfile, depends on the > vmcoreinfo in 1st kernel, your change will break it. Hi Baoquan, I can reproduce it using a kernel module which modifies the vmcoreinfo, so it's a problem can actually happen. > If not, it could be not good to change that. That's a good point, then I guess we can keep the crash_save_vmcoreinfo_init(), and store again all the vmcoreinfo after crash. What do you think? >>> Well, then it will make makedumpfile segfault happen too when execute >>> below command in 1st kernel if it existed: >>> makedumpfile --mem-usage /proc/kcore >> Yes, if the initial vmcoreinfo data was modified before "makedumpfile >> --mem-usage", it might happen, >> after all the system is going something wrong. And that's why we deploy >> kdump service at the very >> beginning when the system has a low possibility of going wrong. >> >> But we have to guarantee kdump vmcore can be generated correctly as possible >> as it can. >> >>> So we still need to face that problem and need fix it. vmcoreinfo_note >>> is in kernel data area, how does module intrude into this area? And can >>> we fix the module code? >>> >> Bugs always exist in products, we can't know what will happen and fix all >> the errors, >> that's why we need kdump. >> >> I think the following update should guarantee the correct vmcoreinfo for >> kdump. > I'm still not convinced. I would probably have more trust in a clean > kernel (after boot) than a kernel that has already crashed (presumably > because of a serious bug). How can be reliability improved by running > more code in unsafe environment? Correct, I realized that, so used crc32 to protect the original data, but since Eric left a more reasonable idea, I will try that later. > > If some code overwrites reserved areas (such as vmcoreinfo), then it's > seriously buggy. And in my opinion, it is more difficult to identify > such bugs if they are masked by re-initializing vmcoreinfo after crash. > In fact, if makedumpfile in the kexec'ed kernel complains that it > didn't find valid VMCOREINFO content, that's already a hint. > > As a side note, if you're debugging a vmcoreinfo corruption, it's > possible to use a standalone VMCOREINFO file with makedumpfile, so you > can pre-generate it and save it in the kdump initrd. > > In short, I don't see a compelling case for this change. E.g. 1) wrong code overwrites vmcoreinfo_data; 2) further crashes the system; 3) trigger kdump, then we obviously will fail to recognize the crash context correctly due to the corrupted vmcoreinfo. Everyone will get confused if met such unfortunate customer-side issue. Although it's corner case, if it's easy to fix, then I think we better do it. Now except for vmcoreinfo, all the crash data is well protected (including cpu note which is fully updated in the crash path, thus its correctness is guaranteed). Regards, Xunlei
Re: kexec regression since 4.9 caused by efi
On 03/17/17 at 01:32pm, Matt Fleming wrote: > On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote: > > > > Matt, I think it should be fine although I think the md type checking in > > efi_mem_desc_lookup() is causing confusion and not easy to understand.. > > Could you make that a separate patch if you think of improvements > there? Duplicate the lookup function is indeed a little ugly, will do it when I have a better idea, we can leave it as is since it works. > > > How about move the if chunk early like below because it seems no need > > to sanity check the addr + size any more if the md is still RUNTIME? > > My original version did as you suggest, but I changed it because we > *really* want to know if someone tries to reserve a range that spans > regions. That would be totally unexpected and a warning about a > potential bug/issue. Matt, I'm fine if you prefer to capture the range checking errors. Would you like me to post it or just you send it out? Thanks Dave
Re: kexec regression since 4.9 caused by efi
On 03/17/17 at 01:32pm, Matt Fleming wrote: > On Fri, 17 Mar, at 10:09:51AM, Dave Young wrote: > > > > Matt, I think it should be fine although I think the md type checking in > > efi_mem_desc_lookup() is causing confusion and not easy to understand.. > > Could you make that a separate patch if you think of improvements > there? Duplicate the lookup function is indeed a little ugly, will do it when I have a better idea, we can leave it as is since it works. > > > How about move the if chunk early like below because it seems no need > > to sanity check the addr + size any more if the md is still RUNTIME? > > My original version did as you suggest, but I changed it because we > *really* want to know if someone tries to reserve a range that spans > regions. That would be totally unexpected and a warning about a > potential bug/issue. Matt, I'm fine if you prefer to capture the range checking errors. Would you like me to post it or just you send it out? Thanks Dave
Re: [PATCH 1/5] mm, swap: Fix comment in __read_swap_cache_async
Hi, Rafeal, Rafael Aquiniwrites: > On Fri, Mar 17, 2017 at 02:46:19PM +0800, Huang, Ying wrote: >> From: Huang Ying >> >> The commit cbab0e4eec29 ("swap: avoid read_swap_cache_async() race to >> deadlock while waiting on discard I/O completion") fixed a deadlock in >> read_swap_cache_async(). Because at that time, in swap allocation >> path, a swap entry may be set as SWAP_HAS_CACHE, then wait for >> discarding to complete before the page for the swap entry is added to >> the swap cache. But in the commit 815c2c543d3a ("swap: make swap >> discard async"), the discarding for swap become asynchronous, waiting >> for discarding to complete will be done before the swap entry is set >> as SWAP_HAS_CACHE. So the comments in code is incorrect now. This >> patch fixes the comments. >> >> The cond_resched() added in the commit cbab0e4eec29 is not necessary >> now too. But if we added some sleep in swap allocation path in the >> future, there may be some hard to debug/reproduce deadlock bug. So it >> is kept. >> > > ^ this is a rather disconcerting way to describe why you left that part > behind, and I recollect telling you about it in a private discussion. > > The fact is that __read_swap_cache_async() still races against get_swap_page() > with a way narrower window due to the async fashioned SSD wear leveling > done for swap nowadays and other changes made within > __read_swap_cache_async()'s > while loop thus making that old deadlock scenario very improbable to strike > again. Thanks for your comments! Could you tell me which kind of race remaining? > All seems legit, apart from that last paragraph in the commit log > message > > > Acked-by: Rafael Aquini Thanks! Best Regards, Huang, Ying >> Cc: Shaohua Li >> Cc: Rafael Aquini >> Signed-off-by: "Huang, Ying" >> --- >> mm/swap_state.c | 12 +--- >> 1 file changed, 1 insertion(+), 11 deletions(-) >> >> diff --git a/mm/swap_state.c b/mm/swap_state.c >> index 473b71e052a8..7bfb9bd1ca21 100644 >> --- a/mm/swap_state.c >> +++ b/mm/swap_state.c >> @@ -360,17 +360,7 @@ struct page *__read_swap_cache_async(swp_entry_t entry, >> gfp_t gfp_mask, >> /* >> * We might race against get_swap_page() and stumble >> * across a SWAP_HAS_CACHE swap_map entry whose page >> - * has not been brought into the swapcache yet, while >> - * the other end is scheduled away waiting on discard >> - * I/O completion at scan_swap_map(). >> - * >> - * In order to avoid turning this transitory state >> - * into a permanent loop around this -EEXIST case >> - * if !CONFIG_PREEMPT and the I/O completion happens >> - * to be waiting on the CPU waitqueue where we are now >> - * busy looping, we just conditionally invoke the >> - * scheduler here, if there are some more important >> - * tasks to run. >> + * has not been brought into the swapcache yet. >> */ >> cond_resched(); >> continue; >> -- >> 2.11.0 >>
Re: [PATCH 1/5] mm, swap: Fix comment in __read_swap_cache_async
Hi, Rafeal, Rafael Aquini writes: > On Fri, Mar 17, 2017 at 02:46:19PM +0800, Huang, Ying wrote: >> From: Huang Ying >> >> The commit cbab0e4eec29 ("swap: avoid read_swap_cache_async() race to >> deadlock while waiting on discard I/O completion") fixed a deadlock in >> read_swap_cache_async(). Because at that time, in swap allocation >> path, a swap entry may be set as SWAP_HAS_CACHE, then wait for >> discarding to complete before the page for the swap entry is added to >> the swap cache. But in the commit 815c2c543d3a ("swap: make swap >> discard async"), the discarding for swap become asynchronous, waiting >> for discarding to complete will be done before the swap entry is set >> as SWAP_HAS_CACHE. So the comments in code is incorrect now. This >> patch fixes the comments. >> >> The cond_resched() added in the commit cbab0e4eec29 is not necessary >> now too. But if we added some sleep in swap allocation path in the >> future, there may be some hard to debug/reproduce deadlock bug. So it >> is kept. >> > > ^ this is a rather disconcerting way to describe why you left that part > behind, and I recollect telling you about it in a private discussion. > > The fact is that __read_swap_cache_async() still races against get_swap_page() > with a way narrower window due to the async fashioned SSD wear leveling > done for swap nowadays and other changes made within > __read_swap_cache_async()'s > while loop thus making that old deadlock scenario very improbable to strike > again. Thanks for your comments! Could you tell me which kind of race remaining? > All seems legit, apart from that last paragraph in the commit log > message > > > Acked-by: Rafael Aquini Thanks! Best Regards, Huang, Ying >> Cc: Shaohua Li >> Cc: Rafael Aquini >> Signed-off-by: "Huang, Ying" >> --- >> mm/swap_state.c | 12 +--- >> 1 file changed, 1 insertion(+), 11 deletions(-) >> >> diff --git a/mm/swap_state.c b/mm/swap_state.c >> index 473b71e052a8..7bfb9bd1ca21 100644 >> --- a/mm/swap_state.c >> +++ b/mm/swap_state.c >> @@ -360,17 +360,7 @@ struct page *__read_swap_cache_async(swp_entry_t entry, >> gfp_t gfp_mask, >> /* >> * We might race against get_swap_page() and stumble >> * across a SWAP_HAS_CACHE swap_map entry whose page >> - * has not been brought into the swapcache yet, while >> - * the other end is scheduled away waiting on discard >> - * I/O completion at scan_swap_map(). >> - * >> - * In order to avoid turning this transitory state >> - * into a permanent loop around this -EEXIST case >> - * if !CONFIG_PREEMPT and the I/O completion happens >> - * to be waiting on the CPU waitqueue where we are now >> - * busy looping, we just conditionally invoke the >> - * scheduler here, if there are some more important >> - * tasks to run. >> + * has not been brought into the swapcache yet. >> */ >> cond_resched(); >> continue; >> -- >> 2.11.0 >>