[PATCH v2 6/6] kselftest/arm64: Check mte tagged user address in kernel
Add a testcase to check that user address with valid/invalid
mte tag works in kernel mode. This test verifies that the kernel
API's __arch_copy_from_user/__arch_copy_to_user works by considering
if the user pointer has valid/invalid allocation tags.
In MTE sync mode, file memory read/write and other similar interfaces
fails if a user memory with invalid tag is accessed in kernel. In async
mode no such failure occurs.
Cc: Shuah Khan
Cc: Catalin Marinas
Cc: Will Deacon
Signed-off-by: Amit Daniel Kachhap
---
Changes in v2:
* Updated the test to handle the error properly in case of failure
in accessing memory with invalid tag in kernel. errno check is not
done now and read length checks are sufficient.
tools/testing/selftests/arm64/mte/.gitignore | 1 +
.../selftests/arm64/mte/check_user_mem.c | 111 ++
.../selftests/arm64/mte/mte_common_util.h | 1 +
.../testing/selftests/arm64/mte/mte_helper.S | 14 +++
4 files changed, 127 insertions(+)
create mode 100644 tools/testing/selftests/arm64/mte/check_user_mem.c
diff --git a/tools/testing/selftests/arm64/mte/.gitignore
b/tools/testing/selftests/arm64/mte/.gitignore
index 44e9bfdaeca6..bc3ac63f3314 100644
--- a/tools/testing/selftests/arm64/mte/.gitignore
+++ b/tools/testing/selftests/arm64/mte/.gitignore
@@ -3,3 +3,4 @@ check_tags_inclusion
check_child_memory
check_mmap_options
check_ksm_options
+check_user_mem
diff --git a/tools/testing/selftests/arm64/mte/check_user_mem.c
b/tools/testing/selftests/arm64/mte/check_user_mem.c
new file mode 100644
index ..594e98e76880
--- /dev/null
+++ b/tools/testing/selftests/arm64/mte/check_user_mem.c
@@ -0,0 +1,111 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#define _GNU_SOURCE
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "kselftest.h"
+#include "mte_common_util.h"
+#include "mte_def.h"
+
+static size_t page_sz;
+
+static int check_usermem_access_fault(int mem_type, int mode, int mapping)
+{
+ int fd, i, err;
+ char val = 'A';
+ size_t len, read_len;
+ void *ptr, *ptr_next;
+
+ err = KSFT_FAIL;
+ len = 2 * page_sz;
+ mte_switch_mode(mode, MTE_ALLOW_NON_ZERO_TAG);
+ fd = create_temp_file();
+ if (fd == -1)
+ return KSFT_FAIL;
+ for (i = 0; i < len; i++)
+ write(fd, , sizeof(val));
+ lseek(fd, 0, 0);
+ ptr = mte_allocate_memory(len, mem_type, mapping, true);
+ if (check_allocated_memory(ptr, len, mem_type, true) != KSFT_PASS) {
+ close(fd);
+ return KSFT_FAIL;
+ }
+ mte_initialize_current_context(mode, (uintptr_t)ptr, len);
+ /* Copy from file into buffer with valid tag */
+ read_len = read(fd, ptr, len);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid || read_len < len)
+ goto usermem_acc_err;
+ /* Verify same pattern is read */
+ for (i = 0; i < len; i++)
+ if (*(char *)(ptr + i) != val)
+ break;
+ if (i < len)
+ goto usermem_acc_err;
+
+ /* Tag the next half of memory with different value */
+ ptr_next = (void *)((unsigned long)ptr + page_sz);
+ ptr_next = mte_insert_new_tag(ptr_next);
+ mte_set_tag_address_range(ptr_next, page_sz);
+
+ lseek(fd, 0, 0);
+ /* Copy from file into buffer with invalid tag */
+ read_len = read(fd, ptr, len);
+ mte_wait_after_trig();
+ /*
+* Accessing user memory in kernel with invalid tag should fail in sync
+* mode without fault but may not fail in async mode as per the
+* implemented MTE userspace support in Arm64 kernel.
+*/
+ if (mode == MTE_SYNC_ERR &&
+ !cur_mte_cxt.fault_valid && read_len < len) {
+ err = KSFT_PASS;
+ } else if (mode == MTE_ASYNC_ERR &&
+ !cur_mte_cxt.fault_valid && read_len == len) {
+ err = KSFT_PASS;
+ }
+usermem_acc_err:
+ mte_free_memory((void *)ptr, len, mem_type, true);
+ close(fd);
+ return err;
+}
+
+int main(int argc, char *argv[])
+{
+ int err;
+
+ page_sz = getpagesize();
+ if (!page_sz) {
+ ksft_print_msg("ERR: Unable to get page size\n");
+ return KSFT_FAIL;
+ }
+ err = mte_default_setup();
+ if (err)
+ return err;
+ /* Register signal handlers */
+ mte_register_signal(SIGSEGV, mte_default_handler);
+
+ evaluate_test(check_usermem_access_fault(USE_MMAP, MTE_SYNC_ERR,
MAP_PRIVATE),
+ "Check memory access from kernel in sync mode, private mapping
and mmap memory\n");
+ evaluate_test(check_usermem_access_fault(USE_MMAP, MTE_SYNC_ERR,
MAP_SHARED),
+ "Check memory access from kernel in sync mode, shared mapping
and mmap memory\n");
+
+
[PATCH v2 4/6] kselftest/arm64: Verify all different mmap MTE options
This testcase checks the different unsupported/supported options for mmap
if used with PROT_MTE memory protection flag. These checks are,
* Either pstate.tco enable or prctl PR_MTE_TCF_NONE option should not cause
any tag mismatch faults.
* Different combinations of anonymous/file memory mmap, mprotect,
sync/async error mode and private/shared mappings should work.
* mprotect should not be able to clear the PROT_MTE page property.
Cc: Shuah Khan
Cc: Catalin Marinas
Cc: Will Deacon
Co-developed-by: Gabor Kertesz
Signed-off-by: Gabor Kertesz
Signed-off-by: Amit Daniel Kachhap
---
tools/testing/selftests/arm64/mte/.gitignore | 1 +
.../selftests/arm64/mte/check_mmap_options.c | 262 ++
2 files changed, 263 insertions(+)
create mode 100644 tools/testing/selftests/arm64/mte/check_mmap_options.c
diff --git a/tools/testing/selftests/arm64/mte/.gitignore
b/tools/testing/selftests/arm64/mte/.gitignore
index b5fcc0fb4d97..79a215d3bbd0 100644
--- a/tools/testing/selftests/arm64/mte/.gitignore
+++ b/tools/testing/selftests/arm64/mte/.gitignore
@@ -1,3 +1,4 @@
check_buffer_fill
check_tags_inclusion
check_child_memory
+check_mmap_options
diff --git a/tools/testing/selftests/arm64/mte/check_mmap_options.c
b/tools/testing/selftests/arm64/mte/check_mmap_options.c
new file mode 100644
index ..33b13b86199b
--- /dev/null
+++ b/tools/testing/selftests/arm64/mte/check_mmap_options.c
@@ -0,0 +1,262 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#define _GNU_SOURCE
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "kselftest.h"
+#include "mte_common_util.h"
+#include "mte_def.h"
+
+#define RUNS (MT_TAG_COUNT)
+#define UNDERFLOW MT_GRANULE_SIZE
+#define OVERFLOW MT_GRANULE_SIZE
+#define TAG_CHECK_ON 0
+#define TAG_CHECK_OFF 1
+
+static size_t page_size;
+static int sizes[] = {
+ 1, 537, 989, 1269, MT_GRANULE_SIZE - 1, MT_GRANULE_SIZE,
+ /* page size - 1*/ 0, /* page_size */ 0, /* page size + 1 */ 0
+};
+
+static int check_mte_memory(char *ptr, int size, int mode, int tag_check)
+{
+ mte_initialize_current_context(mode, (uintptr_t)ptr, size);
+ memset(ptr, '1', size);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid == true)
+ return KSFT_FAIL;
+
+ mte_initialize_current_context(mode, (uintptr_t)ptr, -UNDERFLOW);
+ memset(ptr - UNDERFLOW, '2', UNDERFLOW);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid == false && tag_check == TAG_CHECK_ON)
+ return KSFT_FAIL;
+ if (cur_mte_cxt.fault_valid == true && tag_check == TAG_CHECK_OFF)
+ return KSFT_FAIL;
+
+ mte_initialize_current_context(mode, (uintptr_t)ptr, size + OVERFLOW);
+ memset(ptr + size, '3', OVERFLOW);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid == false && tag_check == TAG_CHECK_ON)
+ return KSFT_FAIL;
+ if (cur_mte_cxt.fault_valid == true && tag_check == TAG_CHECK_OFF)
+ return KSFT_FAIL;
+
+ return KSFT_PASS;
+}
+
+static int check_anonymous_memory_mapping(int mem_type, int mode, int mapping,
int tag_check)
+{
+ char *ptr, *map_ptr;
+ int run, result, map_size;
+ int item = sizeof(sizes)/sizeof(int);
+
+ item = sizeof(sizes)/sizeof(int);
+ mte_switch_mode(mode, MTE_ALLOW_NON_ZERO_TAG);
+ for (run = 0; run < item; run++) {
+ map_size = sizes[run] + OVERFLOW + UNDERFLOW;
+ map_ptr = (char *)mte_allocate_memory(map_size, mem_type,
mapping, false);
+ if (check_allocated_memory(map_ptr, map_size, mem_type, false)
!= KSFT_PASS)
+ return KSFT_FAIL;
+
+ ptr = map_ptr + UNDERFLOW;
+ mte_initialize_current_context(mode, (uintptr_t)ptr,
sizes[run]);
+ /* Only mte enabled memory will allow tag insertion */
+ ptr = mte_insert_tags((void *)ptr, sizes[run]);
+ if (!ptr || cur_mte_cxt.fault_valid == true) {
+ ksft_print_msg("FAIL: Insert tags on anonymous mmap
memory\n");
+ munmap((void *)map_ptr, map_size);
+ return KSFT_FAIL;
+ }
+ result = check_mte_memory(ptr, sizes[run], mode, tag_check);
+ mte_clear_tags((void *)ptr, sizes[run]);
+ mte_free_memory((void *)map_ptr, map_size, mem_type, false);
+ if (result == KSFT_FAIL)
+ return KSFT_FAIL;
+ }
+ return KSFT_PASS;
+}
+
+static int check_file_memory_mapping(int mem_type, int mode, int mapping, int
tag_check)
+{
+ char *ptr, *map_ptr;
+ int run, fd, map_size;
+ int total = sizeof(sizes)/sizeof(int);
+ int result = KSFT_PASS;
+
+ mte_switch_mode(mode,
[PATCH v2 3/6] kselftest/arm64: Check forked child mte memory accessibility
This test covers the mte memory behaviour of the forked process with
different mapping properties and flags. It checks that all bytes of
forked child memory are accessible with the same tag as that of the
parent and memory accessed outside the tag range causes fault to
occur.
Cc: Shuah Khan
Cc: Catalin Marinas
Cc: Will Deacon
Co-developed-by: Gabor Kertesz
Signed-off-by: Gabor Kertesz
Signed-off-by: Amit Daniel Kachhap
---
tools/testing/selftests/arm64/mte/.gitignore | 1 +
.../selftests/arm64/mte/check_child_memory.c | 195 ++
2 files changed, 196 insertions(+)
create mode 100644 tools/testing/selftests/arm64/mte/check_child_memory.c
diff --git a/tools/testing/selftests/arm64/mte/.gitignore
b/tools/testing/selftests/arm64/mte/.gitignore
index c3fca255d3d6..b5fcc0fb4d97 100644
--- a/tools/testing/selftests/arm64/mte/.gitignore
+++ b/tools/testing/selftests/arm64/mte/.gitignore
@@ -1,2 +1,3 @@
check_buffer_fill
check_tags_inclusion
+check_child_memory
diff --git a/tools/testing/selftests/arm64/mte/check_child_memory.c
b/tools/testing/selftests/arm64/mte/check_child_memory.c
new file mode 100644
index ..97bebdecd29e
--- /dev/null
+++ b/tools/testing/selftests/arm64/mte/check_child_memory.c
@@ -0,0 +1,195 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#define _GNU_SOURCE
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "kselftest.h"
+#include "mte_common_util.h"
+#include "mte_def.h"
+
+#define BUFFER_SIZE(5 * MT_GRANULE_SIZE)
+#define RUNS (MT_TAG_COUNT)
+#define UNDERFLOW MT_GRANULE_SIZE
+#define OVERFLOW MT_GRANULE_SIZE
+
+static size_t page_size;
+static int sizes[] = {
+ 1, 537, 989, 1269, MT_GRANULE_SIZE - 1, MT_GRANULE_SIZE,
+ /* page size - 1*/ 0, /* page_size */ 0, /* page size + 1 */ 0
+};
+
+static int check_child_tag_inheritance(char *ptr, int size, int mode)
+{
+ int i, parent_tag, child_tag, fault, child_status;
+ pid_t child;
+
+ parent_tag = MT_FETCH_TAG((uintptr_t)ptr);
+ fault = 0;
+
+ child = fork();
+ if (child == -1) {
+ ksft_print_msg("FAIL: child process creation\n");
+ return KSFT_FAIL;
+ } else if (child == 0) {
+ mte_initialize_current_context(mode, (uintptr_t)ptr, size);
+ /* Do copy on write */
+ memset(ptr, '1', size);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid == true) {
+ fault = 1;
+ goto check_child_tag_inheritance_err;
+ }
+ for (i = 0 ; i < size ; i += MT_GRANULE_SIZE) {
+ child_tag =
MT_FETCH_TAG((uintptr_t)(mte_get_tag_address(ptr + i)));
+ if (parent_tag != child_tag) {
+ ksft_print_msg("FAIL: child mte tag
mismatch\n");
+ fault = 1;
+ goto check_child_tag_inheritance_err;
+ }
+ }
+ mte_initialize_current_context(mode, (uintptr_t)ptr,
-UNDERFLOW);
+ memset(ptr - UNDERFLOW, '2', UNDERFLOW);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid == false) {
+ fault = 1;
+ goto check_child_tag_inheritance_err;
+ }
+ mte_initialize_current_context(mode, (uintptr_t)ptr, size +
OVERFLOW);
+ memset(ptr + size, '3', OVERFLOW);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid == false) {
+ fault = 1;
+ goto check_child_tag_inheritance_err;
+ }
+check_child_tag_inheritance_err:
+ _exit(fault);
+ }
+ /* Wait for child process to terminate */
+ wait(_status);
+ if (WIFEXITED(child_status))
+ fault = WEXITSTATUS(child_status);
+ else
+ fault = 1;
+ return (fault) ? KSFT_FAIL : KSFT_PASS;
+}
+
+static int check_child_memory_mapping(int mem_type, int mode, int mapping)
+{
+ char *ptr;
+ int run, result;
+ int item = sizeof(sizes)/sizeof(int);
+
+ item = sizeof(sizes)/sizeof(int);
+ mte_switch_mode(mode, MTE_ALLOW_NON_ZERO_TAG);
+ for (run = 0; run < item; run++) {
+ ptr = (char *)mte_allocate_memory_tag_range(sizes[run],
mem_type, mapping,
+ UNDERFLOW,
OVERFLOW);
+ if (check_allocated_memory_range(ptr, sizes[run], mem_type,
+UNDERFLOW, OVERFLOW) !=
KSFT_PASS)
+ return KSFT_FAIL;
+ result = check_child_tag_inheritance(ptr, sizes[run], mode);
+ mte_free_memory_tag_range((void *)ptr,
[PATCH v2 0/6] kselftest: arm64/mte: Tests for user-space MTE
These patch series adds below kselftests to test the user-space support for the ARMv8.5 Memory Tagging Extension present in arm64 tree [1]. This patch series is based on Linux v5.9-rc3. 1) This test-case verifies that the memory allocated by kernel mmap interface can support tagged memory access. It first checks the presence of tags at address[56:59] and then proceeds with read and write. The pass criteria for this test is that tag fault exception should not happen. 2) This test-case crosses the valid memory to the invalid memory. In this memory area valid tags are not inserted so read and write should not pass. The pass criteria for this test is that tag fault exception should happen for all the illegal addresses. This test also verfies that PSTATE.TCO works properly. 3) This test-case verifies that the memory inherited by child process from parent process should have same tags copied. The pass criteria for this test is that tag fault exception should not happen. 4) This test checks different mmap flags with PROT_MTE memory protection. 5) This testcase checks that KSM should not merge pages containing different MTE tag values. However, if the tags are same then the pages may merge. This testcase uses the generic ksm sysfs interfaces to verify the MTE behaviour, so this testcase is not fullproof and may be impacted due to other load in the system. 6) Fifth test verifies that syscalls read/write etc works by considering that user pointer has valid/invalid allocation tags. Changes since v1 [2]: * Redefined MTE kernel header definitions to decouple kselftest compilations. * Removed gmi masking instructions in mte_insert_random_tag assembly function. This simplifies the tag inclusion mask test with only GCR mask register used. * Created a new mte_insert_random_tag function with gmi instruction. This is useful for the 6th test which reuses the original tag. * Now use /dev/shm/* to hold temporary files. * Updated the 6th test to handle the error properly in case of failure in accessing memory with invalid tag in kernel. * Code and comment clean-ups. Thanks, Amit Daniel [1]: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/mte [2]: https://patchwork.kernel.org/patch/11747791/ Amit Daniel Kachhap (6): kselftest/arm64: Add utilities and a test to validate mte memory kselftest/arm64: Verify mte tag inclusion via prctl kselftest/arm64: Check forked child mte memory accessibility kselftest/arm64: Verify all different mmap MTE options kselftest/arm64: Verify KSM page merge for MTE pages kselftest/arm64: Check mte tagged user address in kernel tools/testing/selftests/arm64/Makefile| 2 +- tools/testing/selftests/arm64/mte/.gitignore | 6 + tools/testing/selftests/arm64/mte/Makefile| 29 ++ .../selftests/arm64/mte/check_buffer_fill.c | 475 ++ .../selftests/arm64/mte/check_child_memory.c | 195 +++ .../selftests/arm64/mte/check_ksm_options.c | 159 ++ .../selftests/arm64/mte/check_mmap_options.c | 262 ++ .../arm64/mte/check_tags_inclusion.c | 185 +++ .../selftests/arm64/mte/check_user_mem.c | 111 .../selftests/arm64/mte/mte_common_util.c | 341 + .../selftests/arm64/mte/mte_common_util.h | 118 + tools/testing/selftests/arm64/mte/mte_def.h | 60 +++ .../testing/selftests/arm64/mte/mte_helper.S | 128 + 13 files changed, 2070 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/arm64/mte/.gitignore create mode 100644 tools/testing/selftests/arm64/mte/Makefile create mode 100644 tools/testing/selftests/arm64/mte/check_buffer_fill.c create mode 100644 tools/testing/selftests/arm64/mte/check_child_memory.c create mode 100644 tools/testing/selftests/arm64/mte/check_ksm_options.c create mode 100644 tools/testing/selftests/arm64/mte/check_mmap_options.c create mode 100644 tools/testing/selftests/arm64/mte/check_tags_inclusion.c create mode 100644 tools/testing/selftests/arm64/mte/check_user_mem.c create mode 100644 tools/testing/selftests/arm64/mte/mte_common_util.c create mode 100644 tools/testing/selftests/arm64/mte/mte_common_util.h create mode 100644 tools/testing/selftests/arm64/mte/mte_def.h create mode 100644 tools/testing/selftests/arm64/mte/mte_helper.S -- 2.17.1
[PATCH v2 2/6] kselftest/arm64: Verify mte tag inclusion via prctl
This testcase verifies that the tag generated with "irg" instruction
contains only included tags. This is done via prtcl call.
This test covers 4 scenarios,
* At least one included tag.
* More than one included tags.
* All included.
* None included.
Cc: Shuah Khan
Cc: Catalin Marinas
Cc: Will Deacon
Co-developed-by: Gabor Kertesz
Signed-off-by: Gabor Kertesz
Signed-off-by: Amit Daniel Kachhap
---
Changes in v2:
* Small fix to check all tags in check_multiple_included_tags function.
tools/testing/selftests/arm64/mte/.gitignore | 1 +
.../arm64/mte/check_tags_inclusion.c | 185 ++
2 files changed, 186 insertions(+)
create mode 100644 tools/testing/selftests/arm64/mte/check_tags_inclusion.c
diff --git a/tools/testing/selftests/arm64/mte/.gitignore
b/tools/testing/selftests/arm64/mte/.gitignore
index 3f8c1f6c82b9..c3fca255d3d6 100644
--- a/tools/testing/selftests/arm64/mte/.gitignore
+++ b/tools/testing/selftests/arm64/mte/.gitignore
@@ -1 +1,2 @@
check_buffer_fill
+check_tags_inclusion
diff --git a/tools/testing/selftests/arm64/mte/check_tags_inclusion.c
b/tools/testing/selftests/arm64/mte/check_tags_inclusion.c
new file mode 100644
index ..94d245a0ed56
--- /dev/null
+++ b/tools/testing/selftests/arm64/mte/check_tags_inclusion.c
@@ -0,0 +1,185 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2020 ARM Limited
+
+#define _GNU_SOURCE
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "kselftest.h"
+#include "mte_common_util.h"
+#include "mte_def.h"
+
+#define BUFFER_SIZE(5 * MT_GRANULE_SIZE)
+#define RUNS (MT_TAG_COUNT * 2)
+#define MTE_LAST_TAG_MASK (0x7FFF)
+
+static int verify_mte_pointer_validity(char *ptr, int mode)
+{
+ mte_initialize_current_context(mode, (uintptr_t)ptr, BUFFER_SIZE);
+ /* Check the validity of the tagged pointer */
+ memset((void *)ptr, '1', BUFFER_SIZE);
+ mte_wait_after_trig();
+ if (cur_mte_cxt.fault_valid)
+ return KSFT_FAIL;
+ /* Proceed further for nonzero tags */
+ if (!MT_FETCH_TAG((uintptr_t)ptr))
+ return KSFT_PASS;
+ mte_initialize_current_context(mode, (uintptr_t)ptr, BUFFER_SIZE + 1);
+ /* Check the validity outside the range */
+ ptr[BUFFER_SIZE] = '2';
+ mte_wait_after_trig();
+ if (!cur_mte_cxt.fault_valid)
+ return KSFT_FAIL;
+ else
+ return KSFT_PASS;
+}
+
+static int check_single_included_tags(int mem_type, int mode)
+{
+ char *ptr;
+ int tag, run, result = KSFT_PASS;
+
+ ptr = (char *)mte_allocate_memory(BUFFER_SIZE + MT_GRANULE_SIZE,
mem_type, 0, false);
+ if (check_allocated_memory(ptr, BUFFER_SIZE + MT_GRANULE_SIZE,
+ mem_type, false) != KSFT_PASS)
+ return KSFT_FAIL;
+
+ for (tag = 0; (tag < MT_TAG_COUNT) && (result == KSFT_PASS); tag++) {
+ mte_switch_mode(mode, MT_INCLUDE_VALID_TAG(tag));
+ /* Try to catch a excluded tag by a number of tries. */
+ for (run = 0; (run < RUNS) && (result == KSFT_PASS); run++) {
+ ptr = (char *)mte_insert_tags(ptr, BUFFER_SIZE);
+ /* Check tag value */
+ if (MT_FETCH_TAG((uintptr_t)ptr) == tag) {
+ ksft_print_msg("FAIL: wrong tag = 0x%x with
include mask=0x%x\n",
+ MT_FETCH_TAG((uintptr_t)ptr),
+ MT_INCLUDE_VALID_TAG(tag));
+ result = KSFT_FAIL;
+ break;
+ }
+ result = verify_mte_pointer_validity(ptr, mode);
+ }
+ }
+ mte_free_memory_tag_range((void *)ptr, BUFFER_SIZE, mem_type, 0,
MT_GRANULE_SIZE);
+ return result;
+}
+
+static int check_multiple_included_tags(int mem_type, int mode)
+{
+ char *ptr;
+ int tag, run, result = KSFT_PASS;
+ unsigned long excl_mask = 0;
+
+ ptr = (char *)mte_allocate_memory(BUFFER_SIZE + MT_GRANULE_SIZE,
mem_type, 0, false);
+ if (check_allocated_memory(ptr, BUFFER_SIZE + MT_GRANULE_SIZE,
+ mem_type, false) != KSFT_PASS)
+ return KSFT_FAIL;
+
+ for (tag = 0; (tag < MT_TAG_COUNT - 1) && (result == KSFT_PASS); tag++)
{
+ excl_mask |= 1 << tag;
+ mte_switch_mode(mode, MT_INCLUDE_VALID_TAGS(excl_mask));
+ /* Try to catch a excluded tag by a number of tries. */
+ for (run = 0; (run < RUNS) && (result == KSFT_PASS); run++) {
+ ptr = (char *)mte_insert_tags(ptr, BUFFER_SIZE);
+ /* Check tag value */
+ if (MT_FETCH_TAG((uintptr_t)ptr) < tag) {
+ ksft_print_msg("FAIL: wrong
Re: [PATCH 13/18] crypto: use semicolons rather than commas to separate statements
On Sun, Sep 27, 2020 at 09:12:23PM +0200, Julia Lawall wrote: > Replace commas with semicolons. What is done is essentially described by > the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- > drivers/crypto/amcc/crypto4xx_alg.c |2 +- > drivers/crypto/hifn_795x.c |4 ++-- > drivers/crypto/talitos.c|8 > 3 files changed, 7 insertions(+), 7 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 17/18] crypto: atmel-tdes - use semicolons rather than commas to separate statements
On Sun, Sep 27, 2020 at 09:12:27PM +0200, Julia Lawall wrote: > Replace commas with semicolons. What is done is essentially described by > the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- > drivers/crypto/atmel-tdes.c |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 06/18] hwrng: iproc-rng200 - use semicolons rather than commas to separate statements
On Sun, Sep 27, 2020 at 09:12:16PM +0200, Julia Lawall wrote: > Replace commas with semicolons. What is done is essentially described by > the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- > drivers/char/hw_random/iproc-rng200.c |8 > 1 file changed, 4 insertions(+), 4 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 1/4] of/fdt: Update zone_dma_bits when running in bcm2711
On Thu, Oct 01, 2020 at 07:31:19PM +0200, Nicolas Saenz Julienne wrote:
> On Thu, 2020-10-01 at 18:23 +0100, Catalin Marinas wrote:
> > On Thu, Oct 01, 2020 at 06:15:01PM +0100, Catalin Marinas wrote:
> > > On Thu, Oct 01, 2020 at 06:17:37PM +0200, Nicolas Saenz Julienne wrote:
> > > > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> > > > index 4602e467ca8b..cd0d115ef329 100644
> > > > --- a/drivers/of/fdt.c
> > > > +++ b/drivers/of/fdt.c
> > > > @@ -25,6 +25,7 @@
> > > > #include
> > > > #include
> > > > #include
> > > > +#include /* for zone_dma_bits */
> > > >
> > > > #include /* for COMMAND_LINE_SIZE */
> > > > #include
> > > > @@ -1198,6 +1199,14 @@ void __init early_init_dt_scan_nodes(void)
> > > > of_scan_flat_dt(early_init_dt_scan_memory, NULL);
> > > > }
> > > >
> > > > +void __init early_init_dt_update_zone_dma_bits(void)
> > > > +{
> > > > + unsigned long dt_root = of_get_flat_dt_root();
> > > > +
> > > > + if (of_flat_dt_is_compatible(dt_root, "brcm,bcm2711"))
> > > > + zone_dma_bits = 30;
> > > > +}
> > >
> > > I think we could keep this entirely in the arm64 setup_machine_fdt() and
> > > not pollute the core code with RPi4-specific code.
> >
> > Actually, even better, could we not move the check to
> > arm64_memblock_init() when we initialise zone_dma_bits?
>
> I did it this way as I vaguely remembered Rob saying he wanted to centralise
> all early boot fdt code in one place. But I'll be happy to move it there.
I can see Rob replied and I'm fine if that's his preference. However,
what I don't particularly like is that in the arm64 code, if
zone_dma_bits == 24, we set it to 32 assuming that it wasn't touched by
the early_init_dt_update_zone_dma_bits(). What if at some point we'll
get a platform that actually needs 24 here (I truly hope not, but just
the principle of relying on magic values)?
So rather than guessing, I'd prefer if the arch code can override
ZONE_DMA_BITS_DEFAULT. Then, in arm64, we'll just set it to 32 and no
need to explicitly touch the zone_dma_bits variable.
--
Catalin
Re: [PATCH 04/18] hwrng: stm32 - use semicolons rather than commas to separate statements
On Sun, Sep 27, 2020 at 09:12:14PM +0200, Julia Lawall wrote: > Replace commas with semicolons. What is done is essentially described by > the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- > drivers/char/hw_random/stm32-rng.c |8 > 1 file changed, 4 insertions(+), 4 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH 07/18] hwrng: mxc-rnga - use semicolons rather than commas to separate statements
On Sun, Sep 27, 2020 at 09:12:17PM +0200, Julia Lawall wrote: > Replace commas with semicolons. What is done is essentially described by > the following Coccinelle semantic patch (http://coccinelle.lip6.fr/): > > // > @@ expression e1,e2; @@ > e1 > -, > +; > e2 > ... when any > // > > Signed-off-by: Julia Lawall > > --- > drivers/char/hw_random/mxc-rnga.c |6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v2] crypto: sa2ul: Fix DMA mapping API usage
On Wed, Sep 23, 2020 at 01:11:44PM +0300, Peter Ujfalusi wrote: > Make sure that we call the dma_unmap_sg on the correct scatterlist on > completion with the correct sg_nents. > > Use sg_table to managed the DMA mapping and at the same time add the needed > dma_sync calls for the sg_table. > > Signed-off-by: Peter Ujfalusi > --- > Hi, > > Changes since v1: > - use sg_table to manage the mapped sgl as suggested by Christoph > > Regards, > Peter > > drivers/crypto/sa2ul.c | 215 ++--- > 1 file changed, 117 insertions(+), 98 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH -next] PCI: cadence: simplify the return expression of cdns_pcie_host_init_address_translation()
On Mon, Sep 21, 2020 at 09:10:53PM +0800, Qinglang Miao wrote: > Simplify the return expression. > > Signed-off-by: Qinglang Miao > --- > drivers/pci/controller/cadence/pcie-cadence-host.c | 8 ++-- > 1 file changed, 2 insertions(+), 6 deletions(-) Applied to pci/cadence, thanks. Lorenzo > diff --git a/drivers/pci/controller/cadence/pcie-cadence-host.c > b/drivers/pci/controller/cadence/pcie-cadence-host.c > index 4550e0d46..811c1cb2e 100644 > --- a/drivers/pci/controller/cadence/pcie-cadence-host.c > +++ b/drivers/pci/controller/cadence/pcie-cadence-host.c > @@ -337,7 +337,7 @@ static int cdns_pcie_host_init_address_translation(struct > cdns_pcie_rc *rc) > struct resource_entry *entry; > u64 cpu_addr = cfg_res->start; > u32 addr0, addr1, desc1; > - int r, err, busnr = 0; > + int r, busnr = 0; > > entry = resource_list_first_type(>windows, IORESOURCE_BUS); > if (entry) > @@ -383,11 +383,7 @@ static int > cdns_pcie_host_init_address_translation(struct cdns_pcie_rc *rc) > r++; > } > > - err = cdns_pcie_host_map_dma_ranges(rc); > - if (err) > - return err; > - > - return 0; > + return cdns_pcie_host_map_dma_ranges(rc); > } > > static int cdns_pcie_host_init(struct device *dev, > -- > 2.23.0 >
Re: [PATCH v2] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
On Fri, Oct 02, 2020 at 05:04:13PM +0530, Anant Thazhemadam wrote: > > On 02/10/20 7:45 am, David Miller wrote: > > From: Anant Thazhemadam > > Date: Thu, 1 Oct 2020 13:02:20 +0530 > > > >> When get_registers() fails (which happens when usb_control_msg() fails) > >> in set_ethernet_addr(), the uninitialized value of node_id gets copied > >> as the address. > >> > >> Checking for the return values appropriately, and handling the case > >> wherein set_ethernet_addr() fails like this, helps in avoiding the > >> mac address being incorrectly set in this manner. > >> > >> Reported-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com > >> Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com > >> Signed-off-by: Anant Thazhemadam > >> Acked-by: Petko Manolov > > First, please remove "Linux-kernel-mentees" from the Subject line. > > > > All patch submitters should have their work judged equally, whoever > > they are. So this Subject text gives no extra information, and it > > simply makes scanning Subject lines in one's mailer more difficult. > I will keep that in mind for all future submissions. Thank you. > > > Second, when a MAC address fails to probe a random MAC address should > > be selected. We have helpers for this. This way an interface still > > comes up and is usable, even in the event of a failed MAC address > > probe. > > Okay... I see. > But this patch is about ensuring that an uninitialized variable's > value (whatever that may be) is not set as the ethernet address > blindly (without any form of checking if get_registers() worked > as expected, or not). And I didn't think uninitialized values being > set as MAC address was considered a good outcome (after all, it > seemed to have triggered a bug), especially when it could have > been avoided by introducing a simple check that doesn't break > anything. If the read from the device for the MAC address fails, don't abort the whole probe process and make the device not work at all, call the networking core to assign a random MAC address. > However, if I was mistaken, and if that is something that we can live > with after all, then I don't really see the understand the purpose of > similar checks being made (in all the many places that the return > value of get_registers() (or a similar function gets checked) in the first > place at all. Different values and registers determine what should be done with an error. It's all relative. For this type of error, we should gracefully recover and keep on going. For others, maybe we just ignore the issue, or log it, or something else, it all depends. hope this helps, greg k-h
Re: [PATCH v1] hw_random: npcm: modify readl to readb
On Thu, Sep 24, 2020 at 12:23:05AM +0300, Tomer Maimon wrote: > Modify the read size to the correct HW random > registers size, 8bit. > The incorrect read size caused and faulty > HW random value. > > Signed-off-by: Tomer Maimon > --- > drivers/char/hw_random/npcm-rng.c | 14 +++--- > 1 file changed, 7 insertions(+), 7 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH v3 00/10] crypto: caam - xts(aes) updates
On Tue, Sep 22, 2020 at 07:03:18PM +0300, Andrei Botila wrote: > From: Andrei Botila > > This patch series fixes some problems in CAAM's implementation of xts(aes): > - CAAM until Era 9 can't process XTS with 16B IV > - CAAM can only process in hardware XTS key lengths of 16B and 32B > - These hardware limitations are resolved through a fallback > - CAAM used to return 0 for XTS block length equal to zero > > This patch series also adds a new feature in CAAM's xts(aes): > - CAAM is now able to process XTS with 16B IV in HW > > Changes since v2: > - modified xts_skcipher_ivsize() based on comments > - squashed the previous 7-9/12 commits > > Changes since v1: > - use only get_unaligned() for calculating XTS IV size > - fixed the double calling of crypto_skcipher_set_reqsize() in case of XTS > - added a patch which modifies the return value for XTS when block length > is equal to zero > > Andrei Botila (10): > crypto: caam/jr - add fallback for XTS with more than 8B IV > crypto: caam/qi - add fallback for XTS with more than 8B IV > crypto: caam/qi2 - add fallback for XTS with more than 8B IV > crypto: caam/jr - add support for more XTS key lengths > crypto: caam/qi - add support for more XTS key lengths > crypto: caam/qi2 - add support for more XTS key lengths > crypto: caam - add xts check for block length equal to zero > crypto: caam/jr - add support for XTS with 16B IV > crypto: caam/qi - add support for XTS with 16B IV > crypto: caam/qi2 - add support for XTS with 16B IV > > drivers/crypto/caam/Kconfig| 3 + > drivers/crypto/caam/caamalg.c | 94 +--- > drivers/crypto/caam/caamalg_desc.c | 27 --- > drivers/crypto/caam/caamalg_qi.c | 94 +--- > drivers/crypto/caam/caamalg_qi2.c | 111 ++--- > drivers/crypto/caam/caamalg_qi2.h | 2 + > 6 files changed, 293 insertions(+), 38 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH RESEND 0/4] crypto: hisilicon/zip - misc clean up
On Fri, Sep 25, 2020 at 10:06:13PM +0800, Yang Shen wrote: > This patchset fix some bug: > patch 1:clear the debug registers when remove driver > patch 2:intercept invalid input when using decompress > patch 3:replace the return value '-EBUSY' with '-EAGAIN' when > device is busy > patch 4:initialize the 'curr_qm_qp_num' when probe device > > Resend this patch series because it depends on > https://patchwork.kernel.org/cover/11760159/ > (crypto: hisilicon/zip - misc clean up). > Now the patch series has been applied. > > Hao Fang (1): > crypto: hisilicon/zip - fix the uncleared debug registers > > Sihang Chen (1): > crypto: hisilicon/zip - fix the uninitalized 'curr_qm_qp_num' > > Yang Shen (1): > crypto: hisilicon/zip - fix the return value when device is busy > > Zhou Wang (1): > crypto: hisilicon/zip - fix zero length input in GZIP decompress > > drivers/crypto/hisilicon/zip/zip_crypto.c | 26 +++--- > drivers/crypto/hisilicon/zip/zip_main.c | 19 +++ > 2 files changed, 38 insertions(+), 7 deletions(-) All applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH -next] crypto: mediatek - simplify the return expression of mtk_dfe_dse_reset()
On Mon, Sep 21, 2020 at 09:10:09PM +0800, Qinglang Miao wrote: > Simplify the return expression. > > Signed-off-by: Qinglang Miao > --- > drivers/crypto/mediatek/mtk-platform.c | 8 +--- > 1 file changed, 1 insertion(+), 7 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [RFC] Status of orinoco_usb
On 2020-10-02 13:37:25 [+0200], Greg Kroah-Hartman wrote: > > Is it possible to end up here in softirq context or is this a relic? > > I think it's a relic of where USB host controllers completed their urbs > in hard-irq mode. The BH/tasklet change is a pretty recent change. But the BH thingy for HCDs went in v3.12 for EHCI. XHCI was v5.5. My guess would be that people using orinoco USB are on EHCI :) > > Should it be removed? > > We can move it out to drivers/staging/ and then drop it to see if anyone > complains that they have the device and is willing to test any changes. Not sure moving is easy since it depends on other files in that folder. USB is one interface next to PCI for instance. Unless you meant to move the whole driver including all interfaces. I was suggesting to remove the USB bits. > thanks, > > greg k-h Sebastian
Re: [PATCH -next] crypto: marvell/octeontx - simplify the return expression of create_sysfs_eng_grps_info()
On Mon, Sep 21, 2020 at 09:10:07PM +0800, Qinglang Miao wrote: > Simplify the return expression. > > Signed-off-by: Qinglang Miao > --- > drivers/crypto/marvell/octeontx/otx_cptpf_ucode.c | 8 +--- > 1 file changed, 1 insertion(+), 7 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] crypto: ccp - fix error handling
On Mon, Sep 21, 2020 at 01:34:35PM +0200, Pavel Machek wrote: > Fix resource leak in error handling. > > Signed-off-by: Pavel Machek (CIP) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH -next] crypto: cpt - simplify the return expression of cav_register_algs
On Mon, Sep 21, 2020 at 04:24:28PM +0800, Liu Shixin wrote: > Simplify the return expression. > > Signed-off-by: Liu Shixin > --- > drivers/crypto/cavium/cpt/cptvf_algs.c | 8 +--- > 1 file changed, 1 insertion(+), 7 deletions(-) Patch applied. Thanks. -- Email: Herbert Xu Home Page: http://gondor.apana.org.au/~herbert/ PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
Re: [PATCH] media: rcar-vin: rcar-dma: Fix setting VNIS_REG for RAW8 formats
Hi Lad,
Thanks for catching and fixing this.
On 2020-10-02 11:26:52 +0100, Lad Prabhakar wrote:
> pixelformat in vin priv structure holds V4L2_PIX_FMT_* and not
> MEDIA_BUS_FMT_* so make sure we check against V4L2_PIX_FMT_* formats
> while setting the VNIS_REG.
>
> Fixes: 8c3e0f67df6c9 ("media: rcar-vin: Extend RAW8 support to all RGB
> layouts")
> Signed-off-by: Lad Prabhakar
> Reviewed-by: Biju Das
Reviewed-by: Niklas Söderlund
> ---
> Hi Hans,
>
> If it isn't too late for v5.10 could you please queue up this patch.
>
> Cheers,
> Prabhakar
> ---
> drivers/media/platform/rcar-vin/rcar-dma.c | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c
> index e9a47b705acc..692dea300b0d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -600,10 +600,10 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
>* format in 2 pixel unit hence configure VNIS_REG as stride / 2.
>*/
> switch (vin->format.pixelformat) {
> - case MEDIA_BUS_FMT_SBGGR8_1X8:
> - case MEDIA_BUS_FMT_SGBRG8_1X8:
> - case MEDIA_BUS_FMT_SGRBG8_1X8:
> - case MEDIA_BUS_FMT_SRGGB8_1X8:
> + case V4L2_PIX_FMT_SBGGR8:
> + case V4L2_PIX_FMT_SGBRG8:
> + case V4L2_PIX_FMT_SGRBG8:
> + case V4L2_PIX_FMT_SRGGB8:
> stride /= 2;
> break;
> default:
> --
> 2.17.1
>
--
Regards,
Niklas Söderlund
Re: [PATCH -next] PCI: iproc: use module_bcma_driver to simplify the code
On Fri, Sep 18, 2020 at 11:08:29AM +0800, Liu Shixin wrote:
> module_bcma_driver() makes the code simpler by eliminating
> boilerplate code.
>
> Signed-off-by: Liu Shixin
> ---
> drivers/pci/controller/pcie-iproc-bcma.c | 13 +
> 1 file changed, 1 insertion(+), 12 deletions(-)
Applied to pci/mobiveil, thanks.
Lorenzo
> diff --git a/drivers/pci/controller/pcie-iproc-bcma.c
> b/drivers/pci/controller/pcie-iproc-bcma.c
> index aa55b064f64d..56b8ee7bf330 100644
> --- a/drivers/pci/controller/pcie-iproc-bcma.c
> +++ b/drivers/pci/controller/pcie-iproc-bcma.c
> @@ -94,18 +94,7 @@ static struct bcma_driver iproc_pcie_bcma_driver = {
> .probe = iproc_pcie_bcma_probe,
> .remove = iproc_pcie_bcma_remove,
> };
> -
> -static int __init iproc_pcie_bcma_init(void)
> -{
> - return bcma_driver_register(_pcie_bcma_driver);
> -}
> -module_init(iproc_pcie_bcma_init);
> -
> -static void __exit iproc_pcie_bcma_exit(void)
> -{
> - bcma_driver_unregister(_pcie_bcma_driver);
> -}
> -module_exit(iproc_pcie_bcma_exit);
> +module_bcma_driver(iproc_pcie_bcma_driver);
>
> MODULE_AUTHOR("Hauke Mehrtens");
> MODULE_DESCRIPTION("Broadcom iProc PCIe BCMA driver");
> --
> 2.25.1
>
Re: [PATCH 2/2] ASoC: tas2764: Add the driver for the TAS2764
Mark
Thanks for the review
On 10/1/20 11:25 AM, Mark Brown wrote:
On Wed, Sep 30, 2020 at 11:38:09AM -0500, Dan Murphy wrote:
This all looks good - a few very minor things below but nothing
substantial:
+ default:
+ dev_err(tas2764->dev, "Not supported evevt\n");
+ return -EINVAL;
evevt -> event
OK
+static int tas2764_mute(struct snd_soc_dai *dai, int mute, int direction)
+{
+ struct snd_soc_component *component = dai->component;
+ int ret = snd_soc_component_update_bits(component, TAS2764_PWR_CTRL,
+ TAS2764_PWR_CTRL_MASK,
+ mute ? TAS2764_PWR_CTRL_MUTE :
0);
+
+ if (ret < 0)
+ return ret;
This looks weird with the ternery operator and extreme indentation -
could you please at least split the declaration of ret from the call to
make the line length a bit extreme?
I will fix it up
+ switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
+ case SND_SOC_DAIFMT_I2S:
+ case SND_SOC_DAIFMT_DSP_A:
+ tdm_rx_start_slot = 1;
+ break;
+ case SND_SOC_DAIFMT_DSP_B:
+ case SND_SOC_DAIFMT_LEFT_J:
+ tdm_rx_start_slot = 0;
+ break;
I'm not seeing any other handling that distinguishes between the I2S and
DSP modes anywhere - I'm guessing this is because the device is really
only implementing the DSP modes but because it's mono this is compatible
with the I2S modes? It'd be worth having a comment saying this since
while that would be OK not distinguishing between modes properly is a
common error in drivers so it'd help avoid cut'n'paste issues if someone
uses this code as a reference.
Ah it does do LEFT J and Right J so I will fix this
+static int tas2764_register_codec(struct tas2764_priv *tas2764)
+{
+ return devm_snd_soc_register_component(tas2764->dev,
+ _component_driver_tas2764,
+ tas2764_dai_driver,
+ ARRAY_SIZE(tas2764_dai_driver));
+}
This is a bit odd - can we not just inline the component registration
rather than having this function?
I will eliminate this completely and move to i2c_probe
Dan
Re: [PATCH -next] PCI: iproc: use module_bcma_driver to simplify the code
On Fri, Sep 18, 2020 at 11:08:29AM +0800, Liu Shixin wrote:
> module_bcma_driver() makes the code simpler by eliminating
> boilerplate code.
>
> Signed-off-by: Liu Shixin
> ---
> drivers/pci/controller/pcie-iproc-bcma.c | 13 +
> 1 file changed, 1 insertion(+), 12 deletions(-)
Applied to pci/iproc, thanks.
Lorenzo
> diff --git a/drivers/pci/controller/pcie-iproc-bcma.c
> b/drivers/pci/controller/pcie-iproc-bcma.c
> index aa55b064f64d..56b8ee7bf330 100644
> --- a/drivers/pci/controller/pcie-iproc-bcma.c
> +++ b/drivers/pci/controller/pcie-iproc-bcma.c
> @@ -94,18 +94,7 @@ static struct bcma_driver iproc_pcie_bcma_driver = {
> .probe = iproc_pcie_bcma_probe,
> .remove = iproc_pcie_bcma_remove,
> };
> -
> -static int __init iproc_pcie_bcma_init(void)
> -{
> - return bcma_driver_register(_pcie_bcma_driver);
> -}
> -module_init(iproc_pcie_bcma_init);
> -
> -static void __exit iproc_pcie_bcma_exit(void)
> -{
> - bcma_driver_unregister(_pcie_bcma_driver);
> -}
> -module_exit(iproc_pcie_bcma_exit);
> +module_bcma_driver(iproc_pcie_bcma_driver);
>
> MODULE_AUTHOR("Hauke Mehrtens");
> MODULE_DESCRIPTION("Broadcom iProc PCIe BCMA driver");
> --
> 2.25.1
>
Re: [GIT PULL]: Generic PHY updates for v5.10
On Fri, Oct 02, 2020 at 03:24:12PM +0530, Vinod Koul wrote: > Hello Greg, > > Please pull to receive updates for Generic phy susbsystem. Bunch of new > drivers and device support and a new core API this time. > > The following changes since commit ad7a7acaedcf45071c822b6c983f9c1e084041c9: > > phy: omap-usb2-phy: disable PHY charger detect (2020-08-31 14:30:59 +0530) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git > tags/phy-for-5.10 Pulled and pushed out, thanks. greg k-h
[PATCH v2 2/3] PM / EM: update the comments related to power scale
The Energy Model supports power values expressed in milli-Watts or in an
'abstract scale'. Update the related comments is the code to reflect that
state.
Signed-off-by: Lukasz Luba
---
include/linux/energy_model.h | 11 +--
kernel/power/energy_model.c | 2 +-
2 files changed, 6 insertions(+), 7 deletions(-)
diff --git a/include/linux/energy_model.h b/include/linux/energy_model.h
index b67a51c574b9..b19146b31760 100644
--- a/include/linux/energy_model.h
+++ b/include/linux/energy_model.h
@@ -13,9 +13,8 @@
/**
* em_perf_state - Performance state of a performance domain
* @frequency: The frequency in KHz, for consistency with CPUFreq
- * @power: The power consumed at this level, in milli-watts (by 1 CPU or
- by a registered device). It can be a total power: static and
- dynamic.
+ * @power: The power consumed at this level (by 1 CPU or by a registered
+ * device). It can be a total power: static and dynamic.
* @cost: The cost coefficient associated with this level, used during
* energy calculation. Equal to: power * max_frequency / frequency
*/
@@ -55,7 +54,7 @@ struct em_data_callback {
/**
* active_power() - Provide power at the next performance state of
* a device
-* @power : Active power at the performance state in mW
+* @power : Active power at the performance state
* (modified)
* @freq: Frequency at the performance state in kHz
* (modified)
@@ -66,8 +65,8 @@ struct em_data_callback {
* and frequency.
*
* In case of CPUs, the power is the one of a single CPU in the domain,
-* expressed in milli-watts. It is expected to fit in the
-* [0, EM_MAX_POWER] range.
+* expressed in milli-Watts or an abstract scale. It is expected to
+* fit in the [0, EM_MAX_POWER] range.
*
* Return 0 on success.
*/
diff --git a/kernel/power/energy_model.c b/kernel/power/energy_model.c
index c1ff7fa030ab..2bd2afbb83f5 100644
--- a/kernel/power/energy_model.c
+++ b/kernel/power/energy_model.c
@@ -130,7 +130,7 @@ static int em_create_perf_table(struct device *dev, struct
em_perf_domain *pd,
/*
* The power returned by active_state() is expected to be
-* positive, in milli-watts and to fit into 16 bits.
+* positive and to fit into 16 bits.
*/
if (!power || power > EM_MAX_POWER) {
dev_err(dev, "EM: invalid power: %lu\n",
--
2.17.1
Re: [PATCH v2 1/2] system_data_types.7: Add 'void *'
Hi Alex, On 10/2/20 10:48 AM, Alejandro Colomar wrote: > Hi Michael, > > On 2020-10-02 10:24, Alejandro Colomar wrote: >> On 2020-10-01 19:32, Paul Eggert wrote: >> > For 'void *' you should also mention that one cannot use arithmetic on >> > void * pointers, so they're special in that way too. >> >> Good suggestion! >> >> > Also, you should >> > warn that because one can convert from any pointer type to void * and >> > then to any other pointer type, it's a deliberate hole in C's >> > type-checking. >> >> Also good. I'll talk about generic function parameters for this. > I think the patch as is now is complete enough to be added. > > So I won't rewrite it for now. > Please review the patch as is, > and I'll add more info to this type in the future. Actually, I would rather prefer one patch series, rather than patches on patches please. It also makes review of the overall 'void *' text easier if it's all one patch. So, If you could squash the patches together and resubmit, that would be helful. Thanks, Michael -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/
[PATCH v2 3/3] dt-bindings: thermal: update sustainable-power with abstract scale
Update the documentation for the binding 'sustainable-power' and allow to provide values in an abstract scale. It is required when the cooling devices use an abstract scale for their power values. Signed-off-by: Lukasz Luba --- .../devicetree/bindings/thermal/thermal-zones.yaml | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml index 3ec9cc87ec50..4d8f2e37d1e6 100644 --- a/Documentation/devicetree/bindings/thermal/thermal-zones.yaml +++ b/Documentation/devicetree/bindings/thermal/thermal-zones.yaml @@ -99,10 +99,15 @@ patternProperties: sustainable-power: $ref: /schemas/types.yaml#/definitions/uint32 description: - An estimate of the sustainable power (in mW) that this thermal zone - can dissipate at the desired control temperature. For reference, the - sustainable power of a 4-inch phone is typically 2000mW, while on a - 10-inch tablet is around 4500mW. + An estimate of the sustainable power (in mW or in an abstract scale) + that this thermal zone can dissipate at the desired control + temperature. For reference, the sustainable power of a 4-inch phone + is typically 2000mW, while on a 10-inch tablet is around 4500mW. + + It is possible to express the sustainable power in an abstract + scale. This is the case when the related cooling devices use also + abstract scale to express their power usage. The scale must be + consistent. trips: type: object -- 2.17.1
[PATCH v2 1/3] docs: Clarify abstract scale usage for power values in Energy Model
The Energy Model (EM) can store power values in milli-Watts or in abstract scale. This might cause issues in the subsystems which use the EM for estimating the device power, such as: - mixing of different scales in a subsystem which uses multiple (cooling) devices (e.g. thermal Intelligent Power Allocation (IPA)) - assuming that energy [milli-Joules] can be derived from the EM power values which might not be possible since the power scale doesn't have to be in milli-Watts To avoid misconfiguration add the needed documentation to the EM and related subsystems: EAS and IPA. Signed-off-by: Lukasz Luba --- .../driver-api/thermal/power_allocator.rst | 13 - Documentation/power/energy-model.rst| 13 + Documentation/scheduler/sched-energy.rst| 5 + 3 files changed, 30 insertions(+), 1 deletion(-) diff --git a/Documentation/driver-api/thermal/power_allocator.rst b/Documentation/driver-api/thermal/power_allocator.rst index 67b6a3297238..b7992ae84fef 100644 --- a/Documentation/driver-api/thermal/power_allocator.rst +++ b/Documentation/driver-api/thermal/power_allocator.rst @@ -71,7 +71,10 @@ to the speed-grade of the silicon. `sustainable_power` is therefore simply an estimate, and may be tuned to affect the aggressiveness of the thermal ramp. For reference, the sustainable power of a 4" phone is typically 2000mW, while on a 10" tablet is around 4500mW (may vary -depending on screen size). +depending on screen size). It is possible to have the power value +expressed in an abstract scale. This is the case when the Energy Model +provides the power values in an abstract scale. The sustained power +should be aligned to the scale used by the related cooling devices. If you are using device tree, do add it as a property of the thermal-zone. For example:: @@ -269,3 +272,11 @@ won't be very good. Note that this is not particular to this governor, step-wise will also misbehave if you call its throttle() faster than the normal thermal framework tick (due to interrupts for example) as it will overreact. + +Energy Model requirements += + +Another important thing is the consistent scale of the power values +provided by the cooling devices. All of the cooling devices in a single +thermal zone should have power values reported either in milli-Watts +or scaled to the same 'abstract scale'. diff --git a/Documentation/power/energy-model.rst b/Documentation/power/energy-model.rst index a6fb986abe3c..ba7aa581b307 100644 --- a/Documentation/power/energy-model.rst +++ b/Documentation/power/energy-model.rst @@ -20,6 +20,19 @@ possible source of information on its own, the EM framework intervenes as an abstraction layer which standardizes the format of power cost tables in the kernel, hence enabling to avoid redundant work. +The power values might be expressed in milli-Watts or in an 'abstract scale'. +Multiple subsystems might use the EM and it is up to the system integrator to +check that the requirements for the power value scale types are met. An example +can be found in the Energy-Aware Scheduler documentation +Documentation/scheduler/sched-energy.rst. For some subsystems like thermal or +powercap power values expressed in an 'abstract scale' might cause issues. +These subsystems are more interested in estimation of power used in the past, +thus the real milli-Watts might be needed. An example of these requirements can +be found in the Intelligent Power Allocation in +Documentation/driver-api/thermal/power_allocator.rst. +Important thing to keep in mind is that when the power values are expressed in +an 'abstract scale' deriving real energy in milli-Joules would not be possible. + The figure below depicts an example of drivers (Arm-specific here, but the approach is applicable to any architecture) providing power costs to the EM framework, and interested clients reading the data from it:: diff --git a/Documentation/scheduler/sched-energy.rst b/Documentation/scheduler/sched-energy.rst index 001e09c95e1d..afe02d394402 100644 --- a/Documentation/scheduler/sched-energy.rst +++ b/Documentation/scheduler/sched-energy.rst @@ -350,6 +350,11 @@ independent EM framework in Documentation/power/energy-model.rst. Please also note that the scheduling domains need to be re-built after the EM has been registered in order to start EAS. +EAS uses the EM to make a forecasting decision on energy usage and thus it is +more focused on the difference when checking possible options for task +placement. For EAS it doesn't matter whether the EM power values are expressed +in milli-Watts or in an 'abstract scale'. + 6.3 - Energy Model complexity ^ -- 2.17.1
[PATCH v2 0/3] Clarify abstract scale usage for power values in Energy Model, EAS and IPA
Hi all, The Energy Model supports power values expressed in an abstract scale. This has an impact on Intelligent Power Allocation (IPA) and should be documented properly. There is also a need to update the DT binding for the 'sustainable-power' and allow it to have abstract scale as well. Changes: v2: - updated sustainable power section in IPA documentation - updated DT binding for the 'sustainable-power' The v1 of the patch set and related discussion can be found in [1]. Regards, Lukasz Luba [1] https://lore.kernel.org/linux-doc/20200929121610.16060-1-lukasz.l...@arm.com/ Lukasz Luba (3): docs: Clarify abstract scale usage for power values in Energy Model PM / EM: update the comments related to power scale dt-bindings: thermal: update sustainable-power with abstract scale .../devicetree/bindings/thermal/thermal-zones.yaml | 13 + .../driver-api/thermal/power_allocator.rst | 13 - Documentation/power/energy-model.rst| 13 + Documentation/scheduler/sched-energy.rst| 5 + include/linux/energy_model.h| 11 +-- kernel/power/energy_model.c | 2 +- 6 files changed, 45 insertions(+), 12 deletions(-) -- 2.17.1
[PATCH v2] net: hso: do not call unregister if not registered
From: Tuba Yavuz On an error path inside the hso_create_net_device function of the hso driver, hso_free_net_device gets called. This causes a use-after-free and a double-free if register_netdev has not been called yet as hso_free_net_device calls unregister_netdev regardless. I think the driver should distinguish these cases and call unregister_netdev only if register_netdev has been called. Signed-off-by: Tuba Yavuz Signed-off-by: Greg Kroah-Hartman --- v2: format cleaned up based on feedback from previous review Forward to Greg to submit due to email problems on Tuba's side diff --git a/drivers/net/usb/hso.c b/drivers/net/usb/hso.c index 2bb28db89432..e6b56bdf691d 100644 --- a/drivers/net/usb/hso.c +++ b/drivers/net/usb/hso.c @@ -2366,7 +2366,8 @@ static void hso_free_net_device(struct hso_device *hso_dev, bool bailout) remove_net_device(hso_net->parent); - if (hso_net->net) + if (hso_net->net && + hso_net->net->reg_state == NETREG_REGISTERED) unregister_netdev(hso_net->net); /* start freeing */
Re: [PATCH] mm: Remove src/dst mm parameter in copy_page_range()
On Wed, Sep 30, 2020 at 04:49:50PM -0400, Peter Xu wrote:
> Both of the mm pointers are not needed after commit 7a4830c380f3 ("mm/fork:
> Pass new vma pointer into copy_page_range()").
>
> Reported-by: Kirill A. Shutemov
> Signed-off-by: Peter Xu
> ---
> include/linux/mm.h | 3 +--
> kernel/fork.c | 2 +-
> mm/memory.c| 43 ++-
> 3 files changed, 24 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 16b799a0522c..8a0ec8dce5f6 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1645,8 +1645,7 @@ struct mmu_notifier_range;
>
> void free_pgd_range(struct mmu_gather *tlb, unsigned long addr,
> unsigned long end, unsigned long floor, unsigned long ceiling);
> -int copy_page_range(struct mm_struct *dst, struct mm_struct *src,
> - struct vm_area_struct *vma, struct vm_area_struct *new);
> +int copy_page_range(struct vm_area_struct *vma, struct vm_area_struct *new);
> int follow_pte_pmd(struct mm_struct *mm, unsigned long address,
> struct mmu_notifier_range *range,
> pte_t **ptepp, pmd_t **pmdpp, spinlock_t **ptlp);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index da8d360fb032..5f42d4afe0ae 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -589,7 +589,7 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>
> mm->map_count++;
> if (!(tmp->vm_flags & VM_WIPEONFORK))
> - retval = copy_page_range(mm, oldmm, mpnt, tmp);
> + retval = copy_page_range(mpnt, tmp);
>
> if (tmp->vm_ops && tmp->vm_ops->open)
> tmp->vm_ops->open(tmp);
> diff --git a/mm/memory.c b/mm/memory.c
> index fcfc4ca36eba..251bb5082f4e 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -957,11 +957,12 @@ page_copy_prealloc(struct mm_struct *src_mm, struct
> vm_area_struct *vma,
> return new_page;
> }
>
> -static int copy_pte_range(struct mm_struct *dst_mm, struct mm_struct *src_mm,
> -pmd_t *dst_pmd, pmd_t *src_pmd, struct vm_area_struct *vma,
> -struct vm_area_struct *new,
> +static int copy_pte_range(pmd_t *dst_pmd, pmd_t *src_pmd,
> +struct vm_area_struct *vma, struct vm_area_struct *new,
> unsigned long addr, unsigned long end)
I link this, my only minor quibble is the mixing of dst/src and new
language, and then reversing the order in each place. Would read
better to be consistent:
copy_pte_range(dst_vma, dst_pmd, src_vma, src_pmd, addr, end)
Regards,
Jason
Re: [PATCH v1] of: platform: Batch fwnode parsing in the init_machine() path
On 02/10/2020 02:19, Laurent Pinchart wrote:
Hi Saravana,
Thank you for the patch.
On Thu, Oct 01, 2020 at 03:59:51PM -0700, Saravana Kannan wrote:
When commit 93d2e4322aa7 ("of: platform: Batch fwnode parsing when
adding all top level devices") optimized the fwnode parsing when all top
level devices are added, it missed out optimizing this for platform
where the top level devices are added through the init_machine() path.
This commit does the optimization for all paths by simply moving the
fw_devlink_pause/resume() inside of_platform_default_populate().
Based on v5.9-rc5, before the patch:
[0.652887] cpuidle: using governor menu
[ 12.349476] No ATAGs?
After the patch:
[0.650460] cpuidle: using governor menu
[ 12.262101] No ATAGs?
:-(
This is kinda expected :( because omap2 arch doesn't call
of_platform_default_populate()
Call path:
board-generic.c
DT_MACHINE_START()
.init_machine= omap_generic_init,
omap_generic_init()
pdata_quirks_init(omap_dt_match_table);
of_platform_populate(NULL, omap_dt_match_table,
omap_auxdata_lookup, NULL);
Other affected platforms
arm: mach-ux500
some mips
some powerpc
there are also case when a lot of devices placed under bus node, in such case
of_platform_populate() calls from bus drivers will also suffer from this issue.
I think one option could be to add some parameter to _populate() or introduce
new api.
By the way, is there option to disable this feature at all?
Is there Kconfig option?
Is there any reasons why such complex and time consuming code added to the
kernel and not implemented on DTC level?
Also, I've came with another diff, pls check.
[0.00] Booting Linux on physical CPU 0x0
[0.00] Linux version 5.9.0-rc6-01791-g9acba6b38757-dirty
(grygorii@grygorii-XPS-13-9370) (arm-linux-gnueabihf-gcc (GNU Toolcha0
[0.00] CPU: ARMv7 Processor [412fc0f2] revision 2 (ARMv7), cr=10c5387d
[0.00] CPU: div instructions available: patching division code
[0.00] CPU: PIPT / VIPT nonaliasing data cache, PIPT instruction cache
[0.00] OF: fdt: Machine model: TI AM5718 IDK
...
[0.053443] cpuidle: using governor ladder
[0.053470] cpuidle: using governor menu
[0.089304] No ATAGs?
...
[3.092291] devtmpfs: mounted
[3.095804] Freeing unused kernel memory: 1024K
[3.100483] Run /sbin/init as init process
-- >< ---
diff --git a/drivers/of/platform.c b/drivers/of/platform.c
index 071f04da32c8..4521b26e7745 100644
--- a/drivers/of/platform.c
+++ b/drivers/of/platform.c
@@ -514,6 +514,12 @@ static const struct of_device_id reserved_mem_matches[] = {
{}
};
+static int __init of_platform_fw_devlink_pause(void)
+{
+ fw_devlink_pause();
+}
+core_initcall(of_platform_fw_devlink_pause);
+
static int __init of_platform_default_populate_init(void)
{
struct device_node *node;
@@ -538,9 +544,7 @@ static int __init of_platform_default_populate_init(void)
}
/* Populate everything else. */
- fw_devlink_pause();
of_platform_default_populate(NULL, NULL, NULL);
- fw_devlink_resume();
return 0;
}
@@ -548,6 +552,7 @@ arch_initcall_sync(of_platform_default_populate_init);
static int __init of_platform_sync_state_init(void)
{
+ fw_devlink_resume();
device_links_supplier_sync_state_resume();
return 0;
}
--
Best regards,
grygorii
Re: [PATCH 1/2] mmap locking API: Order lock of nascent mm outside lock of live mm
On Fri, Oct 02, 2020 at 02:17:49AM -0700, Michel Lespinasse wrote: > Also FYI I was going to play with these patches a bit to help answer > these questions on my own, but wasn't able to easily apply them as > they came lightly mangled (whitespace issues) when I saved them. Me too It seems OK, you've created sort of a SINGLE_DEPTH_NESTING but in reverse - instead of marking the child of the nest it marks the parent. It would be nice to add a note in the commit message where the nesting happens on this path. Thanks, Jason
Re: [PATCH v3] PCI: hv: Fix hibernation in case interrupts are not re-created
On Fri, Oct 02, 2020 at 01:51:58AM -0700, Dexuan Cui wrote:
> pci_restore_msi_state() directly writes the MSI/MSI-X related registers
> via MMIO. On a physical machine, this works perfectly; for a Linux VM
> running on a hypervisor, which typically enables IOMMU interrupt remapping,
> the hypervisor usually should trap and emulate the MMIO accesses in order
> to re-create the necessary interrupt remapping table entries in the IOMMU,
> otherwise the interrupts can not work in the VM after hibernation.
>
> Hyper-V is different from other hypervisors in that it does not trap and
> emulate the MMIO accesses, and instead it uses a para-virtualized method,
> which requires the VM to call hv_compose_msi_msg() to notify the hypervisor
> of the info that would be passed to the hypervisor in the case of the
> trap-and-emulate method. This is not an issue to a lot of PCI device
> drivers, which destroy and re-create the interrupts across hibernation, so
> hv_compose_msi_msg() is called automatically. However, some PCI device
> drivers (e.g. the in-tree GPU driver nouveau and the out-of-tree Nvidia
> proprietary GPU driver) do not destroy and re-create MSI/MSI-X interrupts
> across hibernation, so hv_pci_resume() has to call hv_compose_msi_msg(),
> otherwise the PCI device drivers can no longer receive interrupts after
> the VM resumes from hibernation.
>
> Hyper-V is also different in that chip->irq_unmask() may fail in a
> Linux VM running on Hyper-V (on a physical machine, chip->irq_unmask()
> can not fail because unmasking an MSI/MSI-X register just means an MMIO
> write): during hibernation, when a CPU is offlined, the kernel tries
> to move the interrupt to the remaining CPUs that haven't been offlined
> yet. In this case, hv_irq_unmask() -> hv_do_hypercall() always fails
> because the vmbus channel has been closed: here the early "return" in
> hv_irq_unmask() means the pci_msi_unmask_irq() is not called, i.e. the
> desc->masked remains "true", so later after hibernation, the MSI interrupt
> always remains masked, which is incorrect. Refer to cpu_disable_common()
> -> fixup_irqs() -> irq_migrate_all_off_this_cpu() -> migrate_one_irq():
>
> static bool migrate_one_irq(struct irq_desc *desc)
> {
> ...
> if (maskchip && chip->irq_mask)
> chip->irq_mask(d);
> ...
> err = irq_do_set_affinity(d, affinity, false);
> ...
> if (maskchip && chip->irq_unmask)
> chip->irq_unmask(d);
>
> Fix the issue by calling pci_msi_unmask_irq() unconditionally in
> hv_irq_unmask(). Also suppress the error message for hibernation because
> the hypercall failure during hibernation does not matter (at this time
> all the devices have been frozen). Note: the correct affinity info is
> still updated into the irqdata data structure in migrate_one_irq() ->
> irq_do_set_affinity() -> hv_set_affinity(), so later when the VM
> resumes, hv_pci_restore_msi_state() is able to correctly restore
> the interrupt with the correct affinity.
>
> Fixes: ac82fc832708 ("PCI: hv: Add hibernation support")
> Reviewed-by: Jake Oshins
> Signed-off-by: Dexuan Cui
> ---
>
> Changes in v2:
> Fixed a typo in the comment in hv_irq_unmask. Thanks to Michael!
> Added Jake's Reviewed-by.
>
> Changes in v3:
> Improved the commit log.
> Improved the comments.
> Improved the change in hv_irq_unmask(): removed the early "return"
> and call pci_msi_unmask_irq() unconditionally.
>
> drivers/pci/controller/pci-hyperv.c | 50 +++--
> 1 file changed, 47 insertions(+), 3 deletions(-)
Applied to pci/hv, thanks !
Lorenzo
> diff --git a/drivers/pci/controller/pci-hyperv.c
> b/drivers/pci/controller/pci-hyperv.c
> index fc4c3a15e570..a9df492fbffa 100644
> --- a/drivers/pci/controller/pci-hyperv.c
> +++ b/drivers/pci/controller/pci-hyperv.c
> @@ -1276,11 +1276,25 @@ static void hv_irq_unmask(struct irq_data *data)
> exit_unlock:
> spin_unlock_irqrestore(>retarget_msi_interrupt_lock, flags);
>
> - if (res) {
> + /*
> + * During hibernation, when a CPU is offlined, the kernel tries
> + * to move the interrupt to the remaining CPUs that haven't
> + * been offlined yet. In this case, the below hv_do_hypercall()
> + * always fails since the vmbus channel has been closed:
> + * refer to cpu_disable_common() -> fixup_irqs() ->
> + * irq_migrate_all_off_this_cpu() -> migrate_one_irq().
> + *
> + * Suppress the error message for hibernation because the failure
> + * during hibernation does not matter (at this time all the devices
> + * have been frozen). Note: the correct affinity info is still updated
> + * into the irqdata data structure in migrate_one_irq() ->
> + * irq_do_set_affinity() -> hv_set_affinity(), so later when the VM
> + * resumes, hv_pci_restore_msi_state() is able to correctly restore
> + * the interrupt with the correct affinity.
> + */
> + if (res &&
Re: [RFC] Status of orinoco_usb
On Fri, Oct 02, 2020 at 12:35:17PM +0200, Sebastian Andrzej Siewior wrote:
> I was trying to get rid of the in in_softirq() in ezusb_req_ctx_wait()
> within the orinoco usb driver,
> drivers/net/wireless/intersil/orinoco/orinoco_usb.c. A small snippet:
>
> | static void ezusb_req_ctx_wait(struct ezusb_priv *upriv,
> |struct request_context *ctx)
> …
> | if (in_softirq()) {
> | /* If we get called from a timer, timeout timers
> don't
> | * get the chance to run themselves. So we make sure
> | * that we don't sleep for ever */
> | int msecs = DEF_TIMEOUT * (1000 / HZ);
> |
> | while (!try_wait_for_completion(>done) &&
> msecs--)
> | udelay(1000);
> | } else {
> | wait_for_completion(>done);
> …
> | }
>
> This is broken. The EHCI and XHCI HCD will complete the URB in
> BH/tasklet. Should we ever get here in_softirq() then we will spin
> here/wait here until the timeout passes because the tasklet won't be
> able to run. OHCI/UHCI HCDs still complete in hard-IRQ so it would work
> here.
>
> Is it possible to end up here in softirq context or is this a relic?
I think it's a relic of where USB host controllers completed their urbs
in hard-irq mode. The BH/tasklet change is a pretty recent change.
> Well I have no hardware but I see this:
>
> orinoco_set_monitor_channel() [I assume that this is fully preemtible]
> -> orinoco_lock() [this should point to ezusb_lock_irqsave() which
> does spin_lock_bh(lock), so from here on
>in_softirq() returns true]
> -> hw->ops->cmd_wait() [-> ezusb_docmd_wait()]
> -> ezusb_alloc_ctx() [ sets ctx->in_rid to EZUSB_RID_ACK/0x0710 ]
> -> ezusb_access_ltv()
> -> if (ctx->in_rid)
>-> ezusb_req_ctx_wait(upriv, ctx);
>-> ctx->state should be EZUSB_CTX_REQ_COMPLETE so we end up in
> the while loop above. So we udelay() 3 * 1000 * 1ms = 3sec.
>-> Then ezusb_access_ltv() should return with an error due to
> timeout.
>
> This isn't limited to exotic features like monitor mode. orinoco_open()
> does orinoco_lock() followed by orinoco_hw_program_rids() which in the
> end invokes ezusb_write_ltv(,, EZUSB_RID_ACK) which is non-zero and also
> would block (ezusb_xmit() would use 0 as the last argument so it won't
> block).
>
> I don't see how this driver can work on EHCI/XHCI HCD as of today.
> The driver is an orphan since commit
>3a59babbee409 ("orinoco: update status in MAINTAINERS")
>
> which is ten years ago. If I replace in_softirq() with a `may_sleep'
> argument then it is still broken.
> Should it be removed?
We can move it out to drivers/staging/ and then drop it to see if anyone
complains that they have the device and is willing to test any changes.
thanks,
greg k-h
Re: [PATCH v2] net: usb: rtl8150: prevent set_ethernet_addr from setting uninit address
On 02/10/20 7:45 am, David Miller wrote: > From: Anant Thazhemadam > Date: Thu, 1 Oct 2020 13:02:20 +0530 > >> When get_registers() fails (which happens when usb_control_msg() fails) >> in set_ethernet_addr(), the uninitialized value of node_id gets copied >> as the address. >> >> Checking for the return values appropriately, and handling the case >> wherein set_ethernet_addr() fails like this, helps in avoiding the >> mac address being incorrectly set in this manner. >> >> Reported-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com >> Tested-by: syzbot+abbc768b560c84d92...@syzkaller.appspotmail.com >> Signed-off-by: Anant Thazhemadam >> Acked-by: Petko Manolov > First, please remove "Linux-kernel-mentees" from the Subject line. > > All patch submitters should have their work judged equally, whoever > they are. So this Subject text gives no extra information, and it > simply makes scanning Subject lines in one's mailer more difficult. I will keep that in mind for all future submissions. Thank you. > Second, when a MAC address fails to probe a random MAC address should > be selected. We have helpers for this. This way an interface still > comes up and is usable, even in the event of a failed MAC address > probe. Okay... I see. But this patch is about ensuring that an uninitialized variable's value (whatever that may be) is not set as the ethernet address blindly (without any form of checking if get_registers() worked as expected, or not). And I didn't think uninitialized values being set as MAC address was considered a good outcome (after all, it seemed to have triggered a bug), especially when it could have been avoided by introducing a simple check that doesn't break anything. However, if I was mistaken, and if that is something that we can live with after all, then I don't really see the understand the purpose of similar checks being made (in all the many places that the return value of get_registers() (or a similar function gets checked) in the first place at all. In all honesty, this confused me a little more than it provided clarity, and I hope someone could help me shift that balance back to clarity again. Thank you for your time. Thanks, Anant
Re: linux tooling mailing list
Hi Guys, >> linux-toolcha...@vger.kernel.org > Created. I have been able to subscribe to this list, so it looks like thunderbirds are go... Cheers Nick
Re: [PATCH v2 1/2] system_data_types.7: Add 'void *'
On Fri, 2 Oct 2020 at 12:49, Jonathan Wakely wrote: > > On Fri, 2 Oct 2020 at 09:28, Alejandro Colomar via Gcc > wrote: > > However, it might be good that someone starts a page called > > 'type_qualifiers(7)' or something like that. > > Who is this for? Who is trying to learn C from man pages? Should > somebody stop them? Yes, I think so. To add context, Alex has been doing a lot of work to build up the new system_data_types(7) page [1], which I think is especially useful for the POSIX system data types that are used with various APIs. With the addition of the integer types and 'void *' things are straying somewhat from POSIX into C. I think there is value in saying something about those types, but I'm somewhat neutral about their inclusion in the page. But Alex has done the work, and I'm willing to include those types in the page. I do think that something like type_qualifiers(7) strays over the line of what should be covered in Linux man-pages, which are primarily about the kernel + libc APIs. [2] Thanks, Michael [1] https://git.kernel.org/pub/scm/docs/man-pages/man-pages.git/tree/man7/system_data_types.7 [2] Mind you, man-pages trayed over the line already very many years ago with operators(7), because who ever remembers all of the C operator precedences. -- Michael Kerrisk Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/ Linux/UNIX System Programming Training: http://man7.org/training/
Re: [PATCH V3 1/8] sysfs: Add sysfs_emit and sysfs_emit_at to format sysfs output
On Wed, Sep 30, 2020 at 06:58:53AM -0700, Joe Perches wrote: > On Wed, 2020-09-30 at 13:57 +0200, Greg Kroah-Hartman wrote: > > Kees, and Rafael, I don't know if you saw this proposal from Joe for > > sysfs files, questions below: > > https://lore.kernel.org/linux-pm/5d606519698ce4c8f1203a2b35797d8254c6050a.1600285923.git@perches.com/T/ > > > So I guess I'm asking for another developer to at least agree that this > > feels like the right way forward here. I don't want to start down this > > path, only to roll them all back as it feels like pointless churn. > > https://lore.kernel.org/lkml/c256eba42a564c01a8e470320475d...@acums.aculab.com/T/#mb40d265bc1dabb8bb64b0dfa29dd8eda44be056e > > > > All now queued up, thanks! greg k-h
Re: [v5] mm: khugepaged: recalculate min_free_kbytes after memory hotplug as expected by khugepaged
On Wed 30-09-20 15:03:11, Mike Kravetz wrote:
> On 9/30/20 1:47 PM, Vijay Balakrishna wrote:
> > On 9/30/2020 11:20 AM, Mike Kravetz wrote:
> >> On 9/29/20 9:49 AM, Vijay Balakrishna wrote:
> >>
> >> Sorry for jumping in so late. Should we use this as an opportunity to
> >> also fix up the messages logged when (re)calculating mfk? They are wrong
> >> and could be quite confusing.
> >
> >
> > Sure. Please share your thoughts regarding appropriate message. Here is
> > what I'm thinking
> >
> > pr_warn("min_free_kbytes is not updated to %d because current value %d is
> > preferred\n", new_min_free_kbytes, min_free_kbytes);
> >
> > If above message is reasonable I can post a new revision (v6).
>
> Just considering the below example,
>
> >> For example consider the following sequence
> >> of operations and corresponding log messages produced.
> >>
> >> Freshly booted VM with 2 nodes and 8GB memory:
> >> # cat /proc/sys/vm/min_free_kbytes
> >> 90112
> >> # echo 9 > /proc/sys/vm/min_free_kbytes
> >> # cat /proc/sys/vm/min_free_kbytes
> >> 9
> >> # echo 0 > /sys/devices/system/node/node1/memory56/online
> >> [ 135.099947] Offlined Pages 32768
> >> [ 135.102362] min_free_kbytes is not updated to 11241 because user
> >> defined value 9 is preferred
>
> I am not sure if there is any value in printing the above line. Especially
> in this context as it becomes obsolete with the printing of the next line.
The original intention was to make it explicit that auto-tuning is
influenced by the user provided configuration.
> >> [ 135.109070] khugepaged: raising min_free_kbytes from 9 to 90112 to
> >> help t
> >> ransparent hugepage allocations
>
> IMO, the above line is the only one that should be output as a result of the
> recalculation.
Well, but khugepaged could be disabled and then the above might not get
printed. Sure the code could get reorganized and all that but is this
really worth that?
> I guess that brings up the question of 'should we continue to track the user
> defined value if we overwrite it?". If we quit tracking it may help with the
> next message.
Auto tuning and user provided override is quite tricky to get sensible.
Especially in the case here. Admin has provided an override but has the
potential memory hotplug been considered? Or to make it even more
complicated, consider that the hotplug happens without admin involvement
- e.g. memory gets hotremoved due to HW problems. Is the admin provided
value still meaningful? To be honest I do not have a good answer and I
am not sure we should care all that much until we see practical
problems.
--
Michal Hocko
SUSE Labs
Re: [net-next PATCH v1 6/7] net: dpaa2-mac: Add ACPI support for DPAA2 MAC driver
On 30/09/2020 17:04, Calvin Johnson wrote:
Modify dpaa2_mac_connect() to support ACPI along with DT.
Modify dpaa2_mac_get_node() to get the dpmac fwnode from either
DT or ACPI.
Replace of_get_phy_mode with fwnode_get_phy_mode to get
phy-mode for a dpmac_node.
Use helper function phylink_fwnode_phy_connect() to find phy_dev and
connect to mac->phylink.
Signed-off-by: Calvin Johnson
---
.../net/ethernet/freescale/dpaa2/dpaa2-mac.c | 79 ---
1 file changed, 50 insertions(+), 29 deletions(-)
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
index 90cd243070d7..18502ee83e46 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-mac.c
@@ -3,6 +3,7 @@
#include "dpaa2-eth.h"
#include "dpaa2-mac.h"
+#include
#define phylink_to_dpaa2_mac(config) \
container_of((config), struct dpaa2_mac, phylink_config)
@@ -35,38 +36,56 @@ static int phy_mode(enum dpmac_eth_if eth_if,
phy_interface_t *if_mode)
}
/* Caller must call of_node_put on the returned value */
-static struct device_node *dpaa2_mac_get_node(u16 dpmac_id)
+static struct fwnode_handle *dpaa2_mac_get_node(struct device *dev,
+ u16 dpmac_id)
{
- struct device_node *dpmacs, *dpmac = NULL;
- u32 id;
+ struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+ struct fwnode_handle *dpmacs, *dpmac = NULL;
+ unsigned long long adr;
+ acpi_status status;
int err;
+ u32 id;
- dpmacs = of_find_node_by_name(NULL, "dpmacs");
- if (!dpmacs)
- return NULL;
+ if (is_of_node(dev->parent->fwnode)) {
+ dpmacs = device_get_named_child_node(dev->parent, "dpmacs");
+ if (!dpmacs)
+ return NULL;
+
+ while ((dpmac = fwnode_get_next_child_node(dpmacs, dpmac))) {
+ err = fwnode_property_read_u32(dpmac, "reg", );
+ if (err)
+ continue;
+ if (id == dpmac_id)
+ return dpmac;
+ }
There is a change of behaviour here that isn't described in the patch
description, so I'm having trouble following the intent. The original
code searches the tree for a node named "dpmacs", but the new code
constrains the search to just the parent device.
Also, because the new code path is only used in the DT case, I don't see
why the behaviour change is required. If it is a bug fix, it should be
broken out into a separate patch. If it is something else, can you
describe what the reasoning is?
I also see that the change to the code has dropped the of_node_put() on
the exit path.
- while ((dpmac = of_get_next_child(dpmacs, dpmac)) != NULL) {
- err = of_property_read_u32(dpmac, "reg", );
- if (err)
- continue;
- if (id == dpmac_id)
- break;
+ } else if (is_acpi_node(dev->parent->fwnode)) {
+ device_for_each_child_node(dev->parent, dpmac) {
+ status =
acpi_evaluate_integer(ACPI_HANDLE_FWNODE(dpmac),
+ "_ADR", NULL, );
+ if (ACPI_FAILURE(status)) {
+ pr_debug("_ADR returned %d on %s\n",
+status, (char *)buffer.pointer);
+ continue;
+ } else {
+ id = (u32)adr;
+ if (id == dpmac_id)
+ return dpmac;
+ }
+ }
}
-
- of_node_put(dpmacs);
-
- return dpmac;
+ return NULL;
}
-static int dpaa2_mac_get_if_mode(struct device_node *node,
+static int dpaa2_mac_get_if_mode(struct fwnode_handle *dpmac_node,
struct dpmac_attr attr)
{
phy_interface_t if_mode;
int err;
- err = of_get_phy_mode(node, _mode);
- if (!err)
- return if_mode;
+ err = fwnode_get_phy_mode(dpmac_node);
+ if (err > 0)
+ return err;
Is this correct? The function prototype from patch 2 is:
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
It returns struct fwnode_handle *. Does this compile?
err = phy_mode(attr.eth_if, _mode);
if (!err)
@@ -303,7 +322,7 @@ int dpaa2_mac_connect(struct dpaa2_mac *mac)
{
struct fsl_mc_device *dpmac_dev = mac->mc_dev;
struct net_device *net_dev = mac->net_dev;
- struct device_node *dpmac_node;
+ struct fwnode_handle *dpmac_node = NULL;
struct phylink *phylink;
struct dpmac_attr attr;
int err;
@@ -323,7 +342,7 @@ int
Re: [PATCH v3 24/24] memory: mtk-smi: Add mt8192 support
On Wed, Sep 30, 2020 at 03:06:47PM +0800, Yong Wu wrote: > Add mt8192 smi support. > > Signed-off-by: Yong Wu > --- > drivers/memory/mtk-smi.c | 19 +++ > 1 file changed, 19 insertions(+) Does it depend on any of the previous patches (so can it be applied independently)? Best regards, Krzysztof
Re: [PATCH v3] pinctrl: mediatek: Free eint data on failure
On Thu, Oct 1, 2020 at 7:25 AM Enric Balletbo i Serra
wrote:
>
> The pinctrl driver can work without the EINT resource, but, if it is
> expected to have this resource but the mtk_build_eint() function fails
> after allocating their data (because can't get the resource or can't map
> the irq), the data is not freed and you end with a NULL pointer
> dereference. Fix this by freeing the data if mtk_build_eint() fails, so
> pinctrl still works and doesn't hang.
>
> This is noticeable after commit f97dbf48ca43 ("irqchip/mtk-sysirq: Convert
> to a platform driver") on MT8183 because, due this commit, the pinctrl driver
> fails to map the irq and spots the following bug:
>
> [1.947597] Unable to handle kernel NULL pointer dereference at virtual
> address 0004
> [1.956404] Mem abort info:
> [1.959203] ESR = 0x9604
> [1.962259] EC = 0x25: DABT (current EL), IL = 32 bits
> [1.967565] SET = 0, FnV = 0
> [1.970613] EA = 0, S1PTW = 0
> [1.973747] Data abort info:
> [1.976619] ISV = 0, ISS = 0x0004
> [1.980447] CM = 0, WnR = 0
> [1.983410] [0004] user address but active_mm is swapper
> [1.989759] Internal error: Oops: 9604 [#1] PREEMPT SMP
> [1.995322] Modules linked in:
> [1.998371] CPU: 7 PID: 1 Comm: swapper/0 Not tainted 5.9.0-rc1+ #44
> [2.004715] Hardware name: MediaTek krane sku176 board (DT)
> [2.010280] pstate: 6005 (nZCv daif -PAN -UAO BTYPE=--)
> [2.015850] pc : mtk_eint_set_debounce+0x48/0x1b8
> [2.020546] lr : mtk_eint_set_debounce+0x34/0x1b8
> [2.025239] sp : 80001008baa0
> [2.028544] x29: 80001008baa0 x28: ff7ff790
> [2.033847] x27: f9ec34b0 x26: f9ec3480
> [2.039150] x25: fa576410 x24: fa502800
> [2.044453] x23: 1388 x22: fa635f80
> [2.049755] x21: 0008 x20:
> [2.055058] x19: 0071 x18: 0001
> [2.060360] x17: x16:
> [2.065662] x15: facc8470 x14:
> [2.070965] x13: 0001 x12: 00c0
> [2.076267] x11: 0040 x10: 0070
> [2.081569] x9 : aec0063d24d8 x8 : fa800270
> [2.086872] x7 : x6 : 0011
> [2.092174] x5 : fa800248 x4 : fa800270
> [2.097476] x3 : 8000100c5000 x2 :
> [2.102778] x1 : x0 :
> [2.108081] Call trace:
> [2.110520] mtk_eint_set_debounce+0x48/0x1b8
> [2.114870] mtk_gpio_set_config+0x5c/0x78
> [2.118958] gpiod_set_config+0x5c/0x78
> [2.122786] gpiod_set_debounce+0x18/0x28
> [2.126789] gpio_keys_probe+0x50c/0x910
> [2.130705] platform_drv_probe+0x54/0xa8
> [2.134705] really_probe+0xe4/0x3b0
> [2.138271] driver_probe_device+0x58/0xb8
> [2.142358] device_driver_attach+0x74/0x80
> [2.146532] __driver_attach+0x58/0xe0
> [2.150274] bus_for_each_dev+0x70/0xc0
> [2.154100] driver_attach+0x24/0x30
> [2.157666] bus_add_driver+0x14c/0x1f0
> [2.161493] driver_register+0x64/0x120
> [2.165319] __platform_driver_register+0x48/0x58
> [2.170017] gpio_keys_init+0x1c/0x28
> [2.173672] do_one_initcall+0x54/0x1b4
> [2.177499] kernel_init_freeable+0x1d0/0x238
> [2.181848] kernel_init+0x14/0x118
> [2.185328] ret_from_fork+0x10/0x34
> [2.188899] Code: a9438ac1 12001266 f94006c3 121e766a (b9400421)
> [2.194991] ---[ end trace 168cf7b3324b6570 ]---
> [2.199611] Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x000b
> [2.207260] SMP: stopping secondary CPUs
> [2.211294] Kernel Offset: 0x2ebff480 from 0x80001000
> [2.217377] PHYS_OFFSET: 0xb505
> [2.221551] CPU features: 0x0240002,2188200c
> [2.225811] Memory Limit: none
> [2.228860] ---[ end Kernel panic - not syncing: Attempted to kill init!
> exitcode=0x000b ]---
>
> Fixes: 89132dd8ffd2 ("pinctrl: mediatek: extend eint build to
> pinctrl-mtk-common-v2.c")
> Signed-off-by: Enric Balletbo i Serra
Acked-by: Sean Wang
> ---
>
> Changes in v3:
> - Really release the resource when needed. For some reason the change
> was not reflected in patch v2.
>
> Changes in v2:
> - Free the resource when needed (Sean Wang)
>
> .../pinctrl/mediatek/pinctrl-mtk-common-v2.c | 29 ++-
> 1 file changed, 22 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> index 2f3dfb56c3fa..31724b80f133 100644
> --- a/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> +++ b/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c
> @@ -355,6 +355,7 @@ int mtk_build_eint(struct mtk_pinctrl *hw, struct
> platform_device *pdev)
> {
> struct device_node *np = pdev->dev.of_node;
>
Re: [PATCH] reset: Add reset controller API
Hi Amjad,
Thank you for the patch, comments below:
On Thu, 2020-10-01 at 15:55 +0200, Amjad Ouled-Ameur wrote:
> An update on the patch title, since we don't add an API but extend it,
> The title should rather be: Add a new call to the reset framework
I think it should even say what functionality is added, for example
"reset: make shared pulsed reset controls re-triggerable"
> Le jeu. 1 oct. 2020 à 15:28, Amjad Ouled-Ameur
> a écrit :
> > The current reset framework API does not allow to release what is done by
> > reset_control_reset(), IOW decrement triggered_count. Add the new
> > reset_control_resettable() call to do so.
> >
> > When reset_control_reset() has been called once, the counter
> > triggered_count, in the reset framework, is incremented i.e the resource
> > under the reset is in-use and the reset should not be done again.
> > reset_control_resettable() would be the way to state that the resource is
> > no longer used and, that from the caller's perspective, the reset can be
> > fired again if necessary.
> >
> > This patch will fix a usb suspend warning seen on the libretech-cc
> > related to the shared reset line. This warning was addressed by the
> > previously reverted commit 7a410953d1fb ("usb: dwc3: meson-g12a: fix shared
> > reset control use")
> >
> > Signed-off-by: Amjad Ouled-Ameur
> > Reported-by: Jerome Brunet
> > ---
> > drivers/reset/core.c | 57 +++
> > include/linux/reset.h | 1 +
> > 2 files changed, 58 insertions(+)
> >
> > diff --git a/drivers/reset/core.c b/drivers/reset/core.c
> > index 01c0c7aa835c..53653d4b55c4 100644
> > --- a/drivers/reset/core.c
> > +++ b/drivers/reset/core.c
> > @@ -207,6 +207,19 @@ static int reset_control_array_reset(struct
> > reset_control_array *resets)
> > return 0;
> > }
> >
> > +static int reset_control_array_resettable(struct reset_control_array
> > *resets)
> > +{
> > + int ret, i;
> > +
> > + for (i = 0; i < resets->num_rstcs; i++) {
> > + ret = reset_control_resettable(resets->rstc[i]);
> > + if (ret)
> > + return ret;
> > + }
This is tricky, as we can't really roll back decrementing
triggered_count in case just one of those fails.
I think reset_control_array_resettable has to be open coded to first
check for errors and only then loop through all controls and decrement
their triggered_count.
> > +
> > + return 0;
> > +}
> > +
> > static int reset_control_array_assert(struct reset_control_array *resets)
> > {
> > int ret, i;
> > @@ -324,6 +337,50 @@ int reset_control_reset(struct reset_control *rstc)
> > }
> > EXPORT_SYMBOL_GPL(reset_control_reset);
> >
> > +/**
> > + * reset_control_resettable - decrements triggered_count of the controlled
> > device
> > + * @rstc: reset controller
It is more important to document the purpose of the function than the
mechanism by which it is achieved. triggered_count is an implementation
detail.
Maybe "allow shared reset line to be triggered again" or similar.
> > + *
> > + * On a shared reset line the actual reset pulse is only triggered once
> > for the
> > + * lifetime of the reset_control instance, except if this function is used.
> > + * In fact, It decrements triggered_count that makes sure of not allowing
> > + * a reset if triggered_count is not null.
> > + *
> > + * This is a no-op in case triggered_count is already null i.e shared
> > reset line
> > + * is ready to be triggered.
This is not a good idea IMHO. It would be better to document that calls
to this function must be balanced with calls to reset_control_reset, and
then throw a big warning below in case deassert_count ever dips below 0.
Otherwise nothing stops drivers from silently decrementing other
driver's trigger count by accidentally calling this multiple times.
> > + *
> > + * Consumers must not use reset_control_(de)assert on shared reset lines
> > when
> > + * reset_control_reset has been used.
> > + *
> > + * If rstc is NULL it is an optional clear and the function will just
> > + * return 0.
> > + */
> > +int reset_control_resettable(struct reset_control *rstc)
> > +{
> > + if (!rstc)
> > + return 0;
> > +
> > + if (WARN_ON(IS_ERR(rstc)))
> > + return -EINVAL;
> > +
> > + if (reset_control_is_array(rstc))
> > + return reset_control_array_resettable(rstc_to_array(rstc));
> > +
> > + if (rstc->shared) {
> > + if (WARN_ON(atomic_read(>deassert_count) != 0))
> > + return -EINVAL;
> > +
> > + if (atomic_read(>triggered_count) > 0)
> > + atomic_dec(>triggered_count);
I think this should be
WARN_ON(atomic_dec_return(>triggered_count) < 0);
regards
Philipp
Re: [PATCH v3 3/9] media: vimc: Add usage count to subdevices
Am 19.08.20 um 20:04 schrieb Kaaira Gupta:
From: Niklas Söderlund
Prepare for multiple video streams from the same sensor by adding a use
counter to vimc_ent_device. The counter is increased for every s_stream(1)
and decremented for every s_stream(0) call.
The subdevice stream is not started or stopped unless the usage count go
from 0 to 1 (started) or from 1 to 0 (stopped). This allows for multiple
s_stream() calls to try to either start or stop the device while only
the first/last call will actually effect the state of the device.
Initialise and increment use_count for capture as well, as use_count
will be used in subsequent patches for starting process_frame as well.
[Kaaira: moved use_count to vimc entity device instead of declaring it
for each subdevice, used use_count for capture as well and rebased
the patch on current HEAD of master to help with the current series]
Signed-off-by: Niklas Söderlund
Signed-off-by: Kaaira Gupta
Hi,
maybe incrementing/decrementing the usage count can be
done from the streamer code instead of
the s_stream of each entity?
Thanks,
Dafna
---
drivers/media/test-drivers/vimc/vimc-capture.c | 3 +++
drivers/media/test-drivers/vimc/vimc-common.h | 2 ++
drivers/media/test-drivers/vimc/vimc-debayer.c | 7 +++
drivers/media/test-drivers/vimc/vimc-scaler.c | 7 +++
drivers/media/test-drivers/vimc/vimc-sensor.c | 6 ++
5 files changed, 25 insertions(+)
diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c
b/drivers/media/test-drivers/vimc/vimc-capture.c
index a8cbb8e4d5ba..93418cb5a139 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -243,6 +243,7 @@ static int vimc_cap_start_streaming(struct vb2_queue *vq,
unsigned int count)
struct media_entity *entity = >vdev.entity;
int ret;
+ atomic_inc(>ved.use_count);
vcap->sequence = 0;
/* Start the media pipeline */
@@ -270,6 +271,7 @@ static void vimc_cap_stop_streaming(struct vb2_queue *vq)
{
struct vimc_cap_device *vcap = vb2_get_drv_priv(vq);
+ atomic_dec(>ved.use_count);
vimc_streamer_s_stream(>stream, >ved, 0);
/* Stop the media pipeline */
@@ -424,6 +426,7 @@ static struct vimc_ent_device *vimc_cap_add(struct
vimc_device *vimc,
vcap->vdev.entity.name = vcfg_name;
vcap->vdev.entity.function = MEDIA_ENT_F_IO_V4L;
vcap->pad.flags = MEDIA_PAD_FL_SINK;
+ atomic_set(>ved.use_count, 0);
ret = media_entity_pads_init(>vdev.entity,
1, >pad);
if (ret)
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h
b/drivers/media/test-drivers/vimc/vimc-common.h
index 287d66edff49..c214f5ec7818 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -85,6 +85,7 @@ struct vimc_pix_map {
*
* @dev: a pointer of the device struct of the driver
* @ent: the pointer to struct media_entity for the node
+ * @use_count: a count to show the number of streams entity is part of
* @get_frame:callback that sends a frame processed by the
entity
* @process_frame:callback that processes a frame
* @vdev_get_format: callback that returns the current format a pad, used
@@ -102,6 +103,7 @@ struct vimc_pix_map {
struct vimc_ent_device {
struct device *dev;
struct media_entity *ent;
+ atomic_t use_count;
void * (*get_frame)(struct vimc_ent_device *ved);
int (*process_frame)(struct vimc_ent_device *ved);
void (*vdev_get_format)(struct vimc_ent_device *ved,
diff --git a/drivers/media/test-drivers/vimc/vimc-debayer.c
b/drivers/media/test-drivers/vimc/vimc-debayer.c
index f61e6e8899ac..60c4c0ec2030 100644
--- a/drivers/media/test-drivers/vimc/vimc-debayer.c
+++ b/drivers/media/test-drivers/vimc/vimc-debayer.c
@@ -343,6 +343,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int
enable)
const struct vimc_pix_map *vpix;
unsigned int frame_size;
+ if (atomic_inc_return(>ved.use_count) != 1)
+ return 0;
+
if (vdeb->src_frame)
return 0;
@@ -368,6 +371,9 @@ static int vimc_deb_s_stream(struct v4l2_subdev *sd, int enable)
return -ENOMEM;
} else {
+ if (atomic_dec_return(>ved.use_count) != 0)
+ return 0;
+
if (!vdeb->src_frame)
return 0;
@@ -608,6 +614,7 @@ static struct vimc_ent_device *vimc_deb_add(struct vimc_device *vimc,
vdeb->ved.get_frame = vimc_deb_get_frame;
vdeb->ved.dev = vimc->mdev.dev;
vdeb->mean_win_size = vimc_deb_ctrl_mean_win_size.def;
+ atomic_set(>ved.use_count, 0);
/* Initialize the frame format */
vdeb->sink_fmt = sink_fmt_default;
diff --git
Re: [PATCH v3 02/18] iommu/vt-d: Add DEV-MSI support
On Thu, Oct 01 2020 at 16:26, Megha Dey wrote:
> On 9/30/2020 11:32 AM, Thomas Gleixner wrote:
>> diff --git a/drivers/iommu/intel/irq_remapping.c
>> b/drivers/iommu/intel/irq_remapping.c
>> index 8f4ce72570ce..0c1ea8ceec31 100644
>> --- a/drivers/iommu/intel/irq_remapping.c
>> +++ b/drivers/iommu/intel/irq_remapping.c
>> @@ -1271,6 +1271,16 @@ static struct irq_chip intel_ir_chip = {
>> .irq_set_vcpu_affinity = intel_ir_set_vcpu_affinity,
>> };
>>
>> +static void irte_prepare_msg(struct msi_msg *msg, int index, int subhandle)
>> +{
>> +msg->address_hi = MSI_ADDR_BASE_HI;
>> +msg->data = sub_handle;
>> +msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_IR_EXT_INT |
>> + MSI_ADDR_IR_SHV |
>> + MSI_ADDR_IR_INDEX1(index) |
>> + MSI_ADDR_IR_INDEX2(index);
>> +}
>> +
>> static void intel_irq_remapping_prepare_irte(struct intel_ir_data *data,
>> struct irq_cfg *irq_cfg,
>> struct irq_alloc_info *info,
>> @@ -1312,19 +1322,18 @@ static void intel_irq_remapping_prepare_irte(struct
>> intel_ir_data *data,
>> break;
>>
>> case X86_IRQ_ALLOC_TYPE_HPET:
>> +set_hpet_sid(irte, info->hpet_id);
>> +irte_prepare_msg(msg, index, sub_handle);
>> +break;
>> +
>> case X86_IRQ_ALLOC_TYPE_MSI:
>> case X86_IRQ_ALLOC_TYPE_MSIX:
>> -if (info->type == X86_IRQ_ALLOC_TYPE_HPET)
>> -set_hpet_sid(irte, info->hpet_id);
>> -else
>> -set_msi_sid(irte, info->msi_dev);
>> -
>> -msg->address_hi = MSI_ADDR_BASE_HI;
>> -msg->data = sub_handle;
>> -msg->address_lo = MSI_ADDR_BASE_LO | MSI_ADDR_IR_EXT_INT |
>> - MSI_ADDR_IR_SHV |
>> - MSI_ADDR_IR_INDEX1(index) |
>> - MSI_ADDR_IR_INDEX2(index);
>> +set_msi_sid(irte, info->msi_dev);
>> +irte_prepare_msg(msg, index, sub_handle);
>> +break;
>> +
>> +case X86_IRQ_ALLOC_TYPE_DEV_MSI:
>> +irte_prepare_msg(msg, index, sub_handle);
>> break;
>>
>> default:
>>
>> Hmm?
>
> ok so I have no clue what happened here. This was the patch that was
> sent out:
> and this does not have the above change. Not sure what happened here.
Of course it was not there. I added this in my reply obviously for
illustration. It's not '> ' quoted, right?
Thanks,
tglx
Re: [PATCH v3 06/24] dt-bindings: mediatek: Add binding for mt8192 IOMMU
On Wed, Sep 30, 2020 at 03:06:29PM +0800, Yong Wu wrote: > This patch adds decriptions for mt8192 IOMMU and SMI. > > mt8192 also is MTK IOMMU gen2 which uses ARM Short-Descriptor translation > table format. The M4U-SMI HW diagram is as below: > > EMI >| > M4U >| > >SMI Common > >| > +---+--+--+--+---+ > | | | | .. | | > | | | | | | > larb0 larb1 larb2 larb4 .. larb19 larb20 > disp0 disp1 mdpvdec IPE IPE > > All the connections are HW fixed, SW can NOT adjust it. > > mt8192 M4U support 0~16GB iova range. we preassign different engines > into different iova ranges: > > domain-id module iova-range larbs >0 disp0 ~ 4G larb0/1 >1 vcodec 4G ~ 8G larb4/5/7 >2 cam/mdp 8G ~ 12G larb2/9/11/13/14/16/17/18/19/20 >3 CCU00x4000_ ~ 0x43ff_ larb13: port 9/10 >4 CCU10x4400_ ~ 0x47ff_ larb14: port 4/5 > > The iova range for CCU0/1(camera control unit) is HW requirement. > > Signed-off-by: Yong Wu > Reviewed-by: Rob Herring > --- > .../bindings/iommu/mediatek,iommu.yaml| 9 +- > .../mediatek,smi-common.yaml | 5 +- > .../memory-controllers/mediatek,smi-larb.yaml | 3 +- > include/dt-bindings/memory/mt8192-larb-port.h | 239 ++ > 4 files changed, 251 insertions(+), 5 deletions(-) > create mode 100644 include/dt-bindings/memory/mt8192-larb-port.h I see it depends on previous patches but does it have to be within one commit? Is it not bisectable? The memory changes/bindings could go via memory tree if this is split. Best regards, Krzysztof
[PATCH v2] mm: Optional full ASLR for mmap() and mremap()
Writing a new value of 3 to /proc/sys/kernel/randomize_va_space
enables full randomization of memory mappings created with mmap(NULL,
...). With 2, the base of the VMA used for such mappings is random,
but the mappings are created in predictable places within the VMA and
in sequential order. With 3, new VMAs are created to fully randomize
the mappings. Also mremap(..., MREMAP_MAYMOVE) will move the mappings
even if not necessary.
On 32 bit systems this may cause problems due to increased VM
fragmentation if the address space gets crowded.
In this example, with value of 2, ld.so.cache, libc, an anonymous mmap
and locale-archive are located close to each other:
$ strace /bin/sync
...
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=189096, ...}) = 0
mmap(NULL, 189096, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7d9c1e7f2000
...
openat(AT_FDCWD, "/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
read(3, "\177ELF\2\1\1\3\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\0n\2\0\0\0\0\0"...,
832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=1839792, ...}) = 0
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0x7d9c1e7f
mmap(NULL, 1852680, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0x7d9c1e62b000
...
openat(AT_FDCWD, "/usr/lib/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=5642592, ...}) = 0
mmap(NULL, 5642592, PROT_READ, MAP_PRIVATE, 3, 0) = 0x7d9c1e0c9000
With 3, they are located in unrelated addresses:
$ echo 3 > /proc/sys/kernel/randomize_va_space
$ /bin/sync
...
openat(AT_FDCWD, "/etc/ld.so.cache", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=189096, ...}) = 0
mmap(NULL, 189096, PROT_READ, MAP_PRIVATE, 3, 0) = 0xeda4fbea000
...
openat(AT_FDCWD, "/lib/x86_64-linux-gnu/libc.so.6", O_RDONLY|O_CLOEXEC) = 3
read(3, "\177ELF\2\1\1\3\0\0\0\0\0\0\0\0\3\0>\0\1\0\0\0\0n\2\0\0\0\0\0"...,
832) = 832
fstat(3, {st_mode=S_IFREG|0755, st_size=1839792, ...}) = 0
mmap(NULL, 8192, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) =
0xb8fb9c1d000
mmap(NULL, 1852680, PROT_READ, MAP_PRIVATE|MAP_DENYWRITE, 3, 0) = 0xaabd8598000
...
openat(AT_FDCWD, "/usr/lib/locale/locale-archive", O_RDONLY|O_CLOEXEC) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=5642592, ...}) = 0
mmap(NULL, 5642592, PROT_READ, MAP_PRIVATE, 3, 0) = 0xbe351ab8000
Signed-off-by: Topi Miettinen
---
v2: also randomize mremap(..., MREMAP_MAYMOVE)
---
Documentation/admin-guide/hw-vuln/spectre.rst | 6 +++---
Documentation/admin-guide/sysctl/kernel.rst | 11 +++
init/Kconfig | 2 +-
mm/mmap.c | 7 ++-
mm/mremap.c | 15 +++
5 files changed, 36 insertions(+), 5 deletions(-)
diff --git a/Documentation/admin-guide/hw-vuln/spectre.rst
b/Documentation/admin-guide/hw-vuln/spectre.rst
index e05e581af5cf..9ea250522077 100644
--- a/Documentation/admin-guide/hw-vuln/spectre.rst
+++ b/Documentation/admin-guide/hw-vuln/spectre.rst
@@ -254,7 +254,7 @@ Spectre variant 2
left by the previous process will also be cleared.
User programs should use address space randomization to make attacks
- more difficult (Set /proc/sys/kernel/randomize_va_space = 1 or 2).
+ more difficult (Set /proc/sys/kernel/randomize_va_space = 1, 2 or 3).
3. A virtualized guest attacking the host
^
@@ -499,8 +499,8 @@ Spectre variant 2
more overhead and run slower.
User programs should use address space randomization
- (/proc/sys/kernel/randomize_va_space = 1 or 2) to make attacks more
- difficult.
+ (/proc/sys/kernel/randomize_va_space = 1, 2 or 3) to make attacks
+ more difficult.
3. VM mitigation
diff --git a/Documentation/admin-guide/sysctl/kernel.rst
b/Documentation/admin-guide/sysctl/kernel.rst
index d4b32cc32bb7..acd0612155d9 100644
--- a/Documentation/admin-guide/sysctl/kernel.rst
+++ b/Documentation/admin-guide/sysctl/kernel.rst
@@ -1060,6 +1060,17 @@ that support this feature.
Systems with ancient and/or broken binaries should be configured
with ``CONFIG_COMPAT_BRK`` enabled, which excludes the heap from process
address space randomization.
+
+3 Additionally enable full randomization of memory mappings created
+with mmap(NULL, ...). With 2, the base of the VMA used for such
+mappings is random, but the mappings are created in predictable
+places within the VMA and in sequential order. With 3, new VMAs
+are created to fully randomize the mappings. Also mremap(...,
+MREMAP_MAYMOVE) will move the mappings even if not necessary.
+
+On 32 bit systems this may cause problems due to increased VM
+fragmentation if the address space gets crowded.
+
== ===
diff --git a/init/Kconfig b/init/Kconfig
index d6a0b31b13dc..c5ea2e694f6a
Re: [PATCH v3 2/9] media: vimc: Add get_frame callback
Hi,
Am 20.08.20 um 17:36 schrieb Kieran Bingham:
Hi Kaaira,
On 19/08/2020 19:04, Kaaira Gupta wrote:
In the process of making vimc compatible for multiple streams, we need
to create a frame passing process such that two different entities can
get the frame from a common entity. This isn't possible currently without
calling process_frame twice for the common entity, as process_frames
returns the frame which gets passed on.
So, to take care of this, add a get_frame callback to vimc device and
use it to get the frames for an entity from previous entity instead of
returning and passing the frames as an argument in process_frame.
Signed-off-by: Kaaira Gupta
---
.../media/test-drivers/vimc/vimc-capture.c| 18 +++---
drivers/media/test-drivers/vimc/vimc-common.h | 7 ---
.../media/test-drivers/vimc/vimc-debayer.c| 19 ---
drivers/media/test-drivers/vimc/vimc-scaler.c | 18 +++---
drivers/media/test-drivers/vimc/vimc-sensor.c | 11 +--
.../media/test-drivers/vimc/vimc-streamer.c | 10 ++
6 files changed, 65 insertions(+), 18 deletions(-)
diff --git a/drivers/media/test-drivers/vimc/vimc-capture.c
b/drivers/media/test-drivers/vimc/vimc-capture.c
index c63496b17b9a..a8cbb8e4d5ba 100644
--- a/drivers/media/test-drivers/vimc/vimc-capture.c
+++ b/drivers/media/test-drivers/vimc/vimc-capture.c
@@ -355,12 +355,13 @@ static void vimc_cap_unregister(struct vimc_ent_device
*ved)
video_unregister_device(>vdev);
}
-static void *vimc_cap_process_frame(struct vimc_ent_device *ved,
- const void *frame)
+static int vimc_cap_process_frame(struct vimc_ent_device *ved)
{
struct vimc_cap_device *vcap = container_of(ved, struct vimc_cap_device,
ved);
struct vimc_cap_buffer *vimc_buf;
+ struct v4l2_subdev *sd;
+ const void *frame;
void *vbuf;
spin_lock(>qlock);
@@ -370,7 +371,7 @@ static void *vimc_cap_process_frame(struct vimc_ent_device
*ved,
typeof(*vimc_buf), list);
if (!vimc_buf) {
spin_unlock(>qlock);
- return ERR_PTR(-EAGAIN);
+ return -EAGAIN;
}
/* Remove this entry from the list */
@@ -385,12 +386,22 @@ static void *vimc_cap_process_frame(struct
vimc_ent_device *ved,
vbuf = vb2_plane_vaddr(_buf->vb2.vb2_buf, 0);
+ sd = media_entity_to_v4l2_subdev(vimc_get_source_entity(ved->ent));
+ ved = v4l2_get_subdevdata(sd);
+ frame = ved->get_frame(ved);
Hrm, this code block is used in several places throughout this patch,
and it aliases the function parameter ved to a new device which isn't
nice. Not a problem as long as it's not used for the original VED after
of course.
But I wonder if we should instead add a helper into vimc-common.c:
struct vimc_ent_device *vimc_get_source_ved(struct vimc_ent_device *ved)
{
struct media_entity *ent;
struct v4l2_subdev *sd;
ent = vimc_get_source_entity(ved->ent);
if (!ent)
return NULL;
sd = media_entity_to_v4l2_subdev(ent);
return v4l2_get_subdevdata(sd);
}
It might not be necessary though, just an idea. If you like it, it can
be a patch on it's own after the vimc_get_source_entity() moving patch.
But it does show that vimc_get_source_entity() can return NULL which
might have to be checked... though perhaps we 'know' it will always be
valid ...
Also, following the links for each entity, for each frame sounds like
quite a lot of work. I wonder if the active source entity should be
cached in each VED ...
I think that we can add a pad number as an argument for the function and
only ask for source entity for that pad.
That could be done on top anyway...
Overall, this looks like it will work, so with comments addressed how
you wish,
Reviewed-by: Kieran Bingham
+
memcpy(vbuf, frame, vcap->format.sizeimage);
/* Set it as ready */
vb2_set_plane_payload(_buf->vb2.vb2_buf, 0,
vcap->format.sizeimage);
vb2_buffer_done(_buf->vb2.vb2_buf, VB2_BUF_STATE_DONE);
+
+ return 0;
+}
+
+static void *vimc_cap_get_frame(struct vimc_ent_device *ved)
+{
return NULL;
}
@@ -455,6 +466,7 @@ static struct vimc_ent_device *vimc_cap_add(struct vimc_device *vimc,
vcap->ved.ent = >vdev.entity;
vcap->ved.process_frame = vimc_cap_process_frame;
vcap->ved.vdev_get_format = vimc_cap_get_format;
+ vcap->ved.get_frame = vimc_cap_get_frame;
vcap->ved.dev = vimc->mdev.dev;
/* Initialize the video_device struct */
diff --git a/drivers/media/test-drivers/vimc/vimc-common.h
b/drivers/media/test-drivers/vimc/vimc-common.h
index 4c580d854007..287d66edff49 100644
--- a/drivers/media/test-drivers/vimc/vimc-common.h
+++ b/drivers/media/test-drivers/vimc/vimc-common.h
@@ -85,7
Re: [net-next PATCH v1 1/7] Documentation: ACPI: DSD: Document MDIO PHY
On 30/09/2020 17:37, Rafael J. Wysocki wrote:
On Wed, Sep 30, 2020 at 6:05 PM Calvin Johnson
wrote:
Introduce ACPI mechanism to get PHYs registered on a MDIO bus and
provide them to be connected to MAC.
Describe properties "phy-handle" and "phy-mode".
Signed-off-by: Calvin Johnson
---
Documentation/firmware-guide/acpi/dsd/phy.rst | 78 +++
1 file changed, 78 insertions(+)
create mode 100644 Documentation/firmware-guide/acpi/dsd/phy.rst
diff --git a/Documentation/firmware-guide/acpi/dsd/phy.rst
b/Documentation/firmware-guide/acpi/dsd/phy.rst
new file mode 100644
index ..f10feb24ec1c
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/phy.rst
@@ -0,0 +1,78 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=
+MDIO bus and PHYs in ACPI
+=
+
+The PHYs on an mdiobus are probed and registered using
+fwnode_mdiobus_register_phy().
+Later, for connecting these PHYs to MAC, the PHYs registered on the
+mdiobus have to be referenced.
+
+phy-handle
+---
+For each MAC node, a property "phy-handle" is used to reference the
+PHY that is registered on an MDIO bus.
It is not clear what "a property" means in this context.
This should refer to the documents introducing the _DSD-based generic
device properties rules, including the GUID used below.
You need to say whether or not the property is mandatory and if it
isn't mandatory, you need to say what the lack of it means.
+
+phy-mode
+
+Property "phy-mode" defines the type of PHY interface.
This needs to be more detailed too, IMO. At the very least, please
list all of the possible values of it and document their meaning.
If the goal is to align with DT, it would be appropriate to point to
where those properties are defined for DT rather than to have a separate
description here. I suggest something along the lines of:
The "phy-mode" _DSD property is used to describe the connection to
the PHY. The valid values for "phy-mode" are defined in
Documentation/devicetree/bindings/ethernet-controller.yaml
+
+An example of this is shown below::
+
+DSDT entry for MACs where PHY nodes are referenced
+--
+ Scope(\_SB.MCE0.PR17) // 1G
+ {
+ Name (_DSD, Package () {
+ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+Package () {
+Package (2) {"phy-mode", "rgmii-id"},
+Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY1}}
What is "phy-handle"?
You haven't introduced it above.
Can you elaborate? "phy-handle" has a section to itself in this
document. Agree that it needs to be defined more, but it does read to me
as having been defined.
+ }
+ })
+ }
+
+ Scope(\_SB.MCE0.PR18) // 1G
+ {
+ Name (_DSD, Package () {
+ ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+ Package () {
+ Package (2) {"phy-mode", "rgmii-id"},
+ Package (2) {"phy-handle", Package (){\_SB.MDI0.PHY2}}
+ }
+ })
+ }
+
+DSDT entry for MDIO node
+
+a) Silicon Component
What is this device, exactly?
+
+ Scope(_SB)
+ {
+ Device(MDI0) {
+ Name(_HID, "NXP0006")
+ Name(_CCA, 1)
+ Name(_UID, 0)
+ Name(_CRS, ResourceTemplate() {
+ Memory32Fixed(ReadWrite, MDI0_BASE, MDI_LEN)
+ Interrupt(ResourceConsumer, Level, ActiveHigh, Shared)
+ {
+MDI0_IT
+ }
+ }) // end of _CRS for MDI0
+ } // end of MDI0
+ }
+
+b) Platform Component
+-
+ Scope(\_SB.MDI0)
+ {
+ Device(PHY1) {
+ Name (_ADR, 0x1)
+ } // end of PHY1
+
+ Device(PHY2) {
+ Name (_ADR, 0x2)
+ } // end of PHY2
+ }
--
What is the connection between the last two pieces of ASL and the _DSD
definitions above?
Re: [PATCH v3 02/24] dt-bindings: memory: mediatek: Convert SMI to DT schema
On Wed, Sep 30, 2020 at 03:06:25PM +0800, Yong Wu wrote:
> Convert MediaTek SMI to DT schema.
>
> Signed-off-by: Yong Wu
> ---
> .../mediatek,smi-common.txt | 49 -
> .../mediatek,smi-common.yaml | 100 ++
> .../memory-controllers/mediatek,smi-larb.txt | 49 -
> .../memory-controllers/mediatek,smi-larb.yaml | 91
> 4 files changed, 191 insertions(+), 98 deletions(-)
> delete mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
> create mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> delete mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
> create mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
>
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
> deleted file mode 100644
> index b64573680b42..
> ---
> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -SMI (Smart Multimedia Interface) Common
> -
> -The hardware block diagram please check bindings/iommu/mediatek,iommu.txt
> -
> -Mediatek SMI have two generations of HW architecture, here is the list
> -which generation the SoCs use:
> -generation 1: mt2701 and mt7623.
> -generation 2: mt2712, mt6779, mt8173 and mt8183.
> -
> -There's slight differences between the two SMI, for generation 2, the
> -register which control the iommu port is at each larb's register base. But
> -for generation 1, the register is at smi ao base(smi always on register
> -base). Besides that, the smi async clock should be prepared and enabled for
> -SMI generation 1 to transform the smi clock into emi clock domain, but that
> is
> -not needed for SMI generation 2.
> -
> -Required properties:
> -- compatible : must be one of :
> - "mediatek,mt2701-smi-common"
> - "mediatek,mt2712-smi-common"
> - "mediatek,mt6779-smi-common"
> - "mediatek,mt7623-smi-common", "mediatek,mt2701-smi-common"
> - "mediatek,mt8173-smi-common"
> - "mediatek,mt8183-smi-common"
> -- reg : the register and size of the SMI block.
> -- power-domains : a phandle to the power domain of this local arbiter.
> -- clocks : Must contain an entry for each entry in clock-names.
> -- clock-names : must contain 3 entries for generation 1 smi HW and 2 entries
> - for generation 2 smi HW as follows:
> - - "apb" : Advanced Peripheral Bus clock, It's the clock for setting
> - the register.
> - - "smi" : It's the clock for transfer data and command.
> - They may be the same if both source clocks are the same.
> - - "async" : asynchronous clock, it help transform the smi clock into the
> emi
> - clock domain, this clock is only needed by generation 1 smi HW.
> - and these 2 option clocks for generation 2 smi HW:
> - - "gals0": the path0 clock of GALS(Global Async Local Sync).
> - - "gals1": the path1 clock of GALS(Global Async Local Sync).
> - Here is the list which has this GALS: mt6779 and mt8183.
> -
> -Example:
> - smi_common: smi@14022000 {
> - compatible = "mediatek,mt8173-smi-common";
> - reg = <0 0x14022000 0 0x1000>;
> - power-domains = < MT8173_POWER_DOMAIN_MM>;
> - clocks = < CLK_MM_SMI_COMMON>,
> - < CLK_MM_SMI_COMMON>;
> - clock-names = "apb", "smi";
> - };
> diff --git
> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> new file mode 100644
> index ..76ecc7205438
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id:
> http://devicetree.org/schemas/memory-controllers/mediatek,smi-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SMI (Smart Multimedia Interface) Common
> +
> +maintainers:
> + - Yong Wu
> +
> +description: |+
> + The hardware block diagram please check bindings/iommu/mediatek,iommu.yaml
> +
> + MediaTek SMI have two generations of HW architecture, here is the list
> + which generation the SoCs use:
> + generation 1: mt2701 and mt7623.
> + generation 2: mt2712, mt6779, mt8173 and mt8183.
> +
> + There's slight differences between the two SMI, for generation 2, the
> + register which control the iommu port is at each larb's register base. But
> + for generation 1, the register is at smi ao base(smi always on register
> + base). Besides that, the smi async clock should be prepared and enabled for
> + SMI generation 1 to transform the smi clock into emi clock
Re: [PATCH v3 seccomp 2/5] seccomp/cache: Add "emulator" to check if filter is constant allow
On Thu, Oct 1, 2020 at 4:05 PM Kees Cook wrote: > Right, but we depend on that test always doing the correct thing (and > continuing to do so into the future). I'm looking at this from the > perspective of future changes, maintenance, etc. I want the actions to > match the design principles as closely as possible so that future > evolutions of the code have lower risk to bugs causing security > failures. Right now, the code is simple. I want to design this so that > when it is complex, it will still fail toward safety in the face of > bugs. > > I'd prefer this way because for the loop, the tests, and the results only > make the bitmap more restrictive. The worst thing a bug in here can do is > leave the bitmap unchanged (which is certainly bad), but it can't _undo_ > an earlier restriction. > > The proposed loop's leading test_bit() becomes only an optimization, > rather than being required for policy enforcement. > > In other words, I prefer: > > inherit all prior prior bitmap restrictions > for all syscalls > if this filter not restricted > continue > set bitmap restricted > > within this loop (where the bulk of future logic may get added), > the worse-case future bug-induced failure mode for the syscall > bitmap is "skip *this* filter". > > > Instead of: > > set bitmap all restricted > for all syscalls > if previous bitmap not restricted and >filter not restricted > set bitmap unrestricted > > within this loop the worst-case future bug-induced failure mode > for the syscall bitmap is "skip *all* filters". > > > > > Or, to reword again, this: > > retain restrictions from previous caching decisions > for all syscalls > [evaluate this filter, maybe continue] > set restricted > > instead of: > > set new cache to all restricted > for all syscalls > [evaluate prior cache and this filter, maybe continue] > set unrestricted > > I expect the future code changes for caching to be in the "evaluate" > step, so I'd like the code designed to make things MORE restrictive not > less from the start, and remove any prior cache state tests from the > loop. > > At the end of the day I believe changing the design like this now lays > the groundwork to the caching mechanism being more robust against having > future bugs introduce security flaws. > I see. Makes sense. Thanks. Will do that in the v4 YiFei Zhu
Re: [PATCH v3 01/24] dt-bindings: iommu: mediatek: Convert IOMMU to DT schema
On Wed, Sep 30, 2020 at 03:06:24PM +0800, Yong Wu wrote: > Convert MediaTek IOMMU to DT schema. > > Signed-off-by: Yong Wu > --- > .../bindings/iommu/mediatek,iommu.txt | 103 > .../bindings/iommu/mediatek,iommu.yaml| 154 ++ > 2 files changed, 154 insertions(+), 103 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > create mode 100644 > Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml > > diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > deleted file mode 100644 > index c1ccd8582eb2.. > --- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > +++ /dev/null > @@ -1,103 +0,0 @@ > -* Mediatek IOMMU Architecture Implementation > - > - Some Mediatek SOCs contain a Multimedia Memory Management Unit (M4U), and > -this M4U have two generations of HW architecture. Generation one uses flat > -pagetable, and only supports 4K size page mapping. Generation two uses the > -ARM Short-Descriptor translation table format for address translation. > - > - About the M4U Hardware Block Diagram, please check below: > - > - EMI (External Memory Interface) > - | > - m4u (Multimedia Memory Management Unit) > - | > - ++ > - || > - gals0-rx gals1-rx(Global Async Local Sync rx) > - || > - || > - gals0-tx gals1-tx(Global Async Local Sync tx) > - || Some SoCs may have GALS. > - ++ > - | > - SMI Common(Smart Multimedia Interface Common) > - | > - ++--- > - || > - | gals-rxThere may be GALS in some larbs. > - || > - || > - | gals-tx > - || > - SMI larb0SMI larb1 ... SoCs have several SMI local > arbiter(larb). > - (display) (vdec) > - || > - || > - +-+-+ +++ > - | | | ||| > - | | |... ||| ... There are different ports in each larb. > - | | | ||| > -OVL0 RDMA0 WDMA0 MC PP VLD > - > - As above, The Multimedia HW will go through SMI and M4U while it > -access EMI. SMI is a bridge between m4u and the Multimedia HW. It contain > -smi local arbiter and smi common. It will control whether the Multimedia > -HW should go though the m4u for translation or bypass it and talk > -directly with EMI. And also SMI help control the power domain and clocks for > -each local arbiter. > - Normally we specify a local arbiter(larb) for each multimedia HW > -like display, video decode, and camera. And there are different ports > -in each larb. Take a example, There are many ports like MC, PP, VLD in the > -video decode local arbiter, all these ports are according to the video HW. > - In some SoCs, there may be a GALS(Global Async Local Sync) module between > -smi-common and m4u, and additional GALS module between smi-larb and > -smi-common. GALS can been seen as a "asynchronous fifo" which could help > -synchronize for the modules in different clock frequency. > - > -Required properties: > -- compatible : must be one of the following string: > - "mediatek,mt2701-m4u" for mt2701 which uses generation one m4u HW. > - "mediatek,mt2712-m4u" for mt2712 which uses generation two m4u HW. > - "mediatek,mt6779-m4u" for mt6779 which uses generation two m4u HW. > - "mediatek,mt7623-m4u", "mediatek,mt2701-m4u" for mt7623 which uses > - generation one m4u HW. > - "mediatek,mt8173-m4u" for mt8173 which uses generation two m4u HW. > - "mediatek,mt8183-m4u" for mt8183 which uses generation two m4u HW. > -- reg : m4u register base and size. > -- interrupts : the interrupt of m4u. > -- clocks : must contain one entry for each clock-names. > -- clock-names : Only 1 optional clock: > - - "bclk": the block clock of m4u. > - Here is the list which require this "bclk": > - - mt2701, mt2712, mt7623 and mt8173. > - Note that m4u use the EMI clock which always has been enabled before kernel > - if there is no this "bclk". > -- mediatek,larbs : List of phandle to the local arbiters in the current Socs. > - Refer to bindings/memory-controllers/mediatek,smi-larb.txt. It must sort > - according to the local arbiter index, like larb0, larb1, larb2... > -- iommu-cells : must be 1. This is the mtk_m4u_id according to the HW. > - Specifies the mtk_m4u_id as defined in > - dt-binding/memory/mt2701-larb-port.h for mt2701, mt7623 > - dt-binding/memory/mt2712-larb-port.h for mt2712, > - dt-binding/memory/mt6779-larb-port.h for mt6779, > -
Re: [PATCH v2 0/3] drm: commit_work scheduling
On Fri, Oct 02, 2020 at 01:52:56PM +0300, Ville Syrjälä wrote: > On Thu, Oct 01, 2020 at 05:25:55PM +0200, Daniel Vetter wrote: > > On Thu, Oct 1, 2020 at 5:15 PM Rob Clark wrote: > > > > > > On Thu, Oct 1, 2020 at 12:25 AM Daniel Vetter wrote: > > > > > > > > On Wed, Sep 30, 2020 at 11:16 PM Rob Clark wrote: > > > > > > > > > > From: Rob Clark > > > > > > > > > > The android userspace treats the display pipeline as a realtime > > > > > problem. > > > > > And arguably, if your goal is to not miss frame deadlines (ie. > > > > > vblank), > > > > > it is. (See https://lwn.net/Articles/809545/ for the best > > > > > explaination > > > > > that I found.) > > > > > > > > > > But this presents a problem with using workqueues for non-blocking > > > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can > > > > > preempt the worker. Which is not really the outcome you want.. once > > > > > the required fences are scheduled, you want to push the atomic commit > > > > > down to hw ASAP. > > > > > > > > > > But the decision of whether commit_work should be RT or not really > > > > > depends on what userspace is doing. For a pure CFS userspace display > > > > > pipeline, commit_work() should remain SCHED_NORMAL. > > > > > > > > > > To handle this, convert non-blocking commit_work() to use per-CRTC > > > > > kthread workers, instead of system_unbound_wq. Per-CRTC workers are > > > > > used to avoid serializing commits when userspace is using a per-CRTC > > > > > update loop. And the last patch exposes the task id to userspace as > > > > > a CRTC property, so that userspace can adjust the priority and sched > > > > > policy to fit it's needs. > > > > > > > > > > > > > > > v2: Drop client cap and in-kernel setting of priority/policy in > > > > > favor of exposing the kworker tid to userspace so that user- > > > > > space can set priority/policy. > > > > > > > > Yeah I think this looks more reasonable. Still a bit irky interface, > > > > so I'd like to get some kworker/rt ack on this. Other opens: > > > > - needs userspace, the usual drill > > > > > > fwiw, right now the userspace is "modetest + chrt".. *probably* the > > > userspace will become a standalone helper or daemon, mostly because > > > the chrome gpu-process sandbox does not allow setting SCHED_FIFO. I'm > > > still entertaining the possibility of switching between rt and cfs > > > depending on what is in the foreground (ie. only do rt for android > > > apps). > > > > > > > - we need this also for vblank workers, otherwise this wont work for > > > > drivers needing those because of another priority inversion. > > > > > > I have a thought on that, see below.. > > > > Hm, not seeing anything about vblank worker below? > > > > > > - we probably want some indication of whether this actually does > > > > something useful, not all drivers use atomic commit helpers. Not sure > > > > how to do that. > > > > > > I'm leaning towards converting the other drivers over to use the > > > per-crtc kwork, and then dropping the 'commit_work` from atomic state. > > > I can add a patch to that, but figured I could postpone that churn > > > until there is some by-in on this whole idea. > > > > i915 has its own commit code, it's not even using the current commit > > helpers (nor the commit_work). Not sure how much other fun there is. > > I don't think we want per-crtc threads for this in i915. Seems > to me easier to guarantee atomicity across multiple crtcs if > we just commit them from the same thread. Oh, and we may have to commit things in a very specific order to guarantee the hw doesn't fall over, so yeah definitely per-crtc thread is a no go. I don't even understand the serialization argument. If the commits are truly independent then why isn't the unbound wq enough to avoid the serialization? It should just spin up a new thread for each commit no? -- Ville Syrjälä Intel
Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()
On 30/09/2020 17:04, Calvin Johnson wrote:
Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.
Signed-off-by: Calvin Johnson
---
drivers/net/phy/phy_device.c | 32 +++-
include/linux/phy.h | 5 +
2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index c4aec56d0a95..162abde6223d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,6 +9,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+#include
#include
#include
#include
@@ -845,6 +846,27 @@ static int get_phy_c22_id(struct mii_bus *bus, int addr,
u32 *phy_id)
return 0;
}
+/* Extract the phy ID from the compatible string of the form
+ * ethernet-phy-id..
+ */
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+ unsigned int upper, lower;
+ const char *cp;
+ int ret;
+
+ ret = fwnode_property_read_string(fwnode, "compatible", );
+ if (ret)
+ return ret;
+
+ if (sscanf(cp, "ethernet-phy-id%4x.%4x", , ) == 2) {
+ *phy_id = ((upper & 0x) << 16) | (lower & 0x);
+ return 0;
+ }
+ return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_get_phy_id);
This block, and the changes in patch 4 duplicate functions from
drivers/of/of_mdio.c, but it doesn't refactor anything in
drivers/of/of_mdio.c to use the new path. Is your intent to bring all of
the parsing in these functions of "compatible" into the ACPI code path?
If so, then the existing code path needs to be refactored to work with
fwnode_handle instead of device_node.
If not, then the DT path in these functions should call out to of_mdio,
while the ACPI path only does what is necessary.
+
/**
* get_phy_device - reads the specified PHY device and returns its @phy_device
*struct
@@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
*/
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
{
- return fwnode_find_reference(fwnode, "phy-handle", 0);
+ struct fwnode_handle *phy_node;
+
+ phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
+ if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
+ return phy_node;
+ phy_node = fwnode_find_reference(fwnode, "phy", 0);
+ if (IS_ERR(phy_node))
+ phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
+ return phy_node;
}
EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 7b1bf3d46fd3..b6814e04092f 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1378,6 +1378,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus,
int addr, u32 phy_id,
bool is_c45,
struct phy_c45_device_ids *c45_ids);
#if IS_ENABLED(CONFIG_PHYLIB)
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
struct phy_device *device_phy_find_device(struct device *dev);
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
@@ -1385,6 +1386,10 @@ struct phy_device *get_phy_device(struct mii_bus *bus,
int addr, bool is_c45);
int phy_device_register(struct phy_device *phy);
void phy_device_free(struct phy_device *phydev);
#else
+static inline int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+ return 0;
+}
static inline
struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
{
Re: [PATCH 7/7] TC-ETF support PTP clocks
On 02/10/2020 02:33, Thomas Gleixner wrote:
> On Thu, Oct 01 2020 at 22:51, Erez Geva wrote:
>
>>- Add support for using a POSIX dynamic clock with
>> Traffic control Earliest TxTime First (ETF) Qdisc.
>
>
>
>> --- a/include/uapi/linux/net_tstamp.h
>> +++ b/include/uapi/linux/net_tstamp.h
>> @@ -167,6 +167,11 @@ enum txtime_flags {
>> SOF_TXTIME_FLAGS_MASK = (SOF_TXTIME_FLAGS_LAST - 1) |
>> SOF_TXTIME_FLAGS_LAST
>> };
>> +/*
>> + * Clock ID to use with POSIX clocks
>> + * The ID must be u8 to fit in (struct sock)->sk_clockid
>> + */
>> +#define SOF_TXTIME_POSIX_CLOCK_ID (0x77)
>
> Random number with a random name.
>
>> struct sock_txtime {
>> __kernel_clockid_t clockid;/* reference clockid */
>> diff --git a/net/sched/sch_etf.c b/net/sched/sch_etf.c
>> index c0de4c6f9299..8e3e0a61fa58 100644
>> --- a/net/sched/sch_etf.c
>> +++ b/net/sched/sch_etf.c
>> @@ -15,6 +15,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -40,19 +41,40 @@ struct etf_sched_data {
>> struct rb_root_cached head;
>> struct qdisc_watchdog watchdog;
>> ktime_t (*get_time)(void);
>> +#ifdef CONFIG_POSIX_TIMERS
>> +struct posix_clock *pclock; /* pointer to a posix clock */
>
> Tail comments suck because they disturb the reading flow and this
> comment has absolute zero value.
>
> Comments are required to explain things which are not obvious...
>
>> +#endif /* CONFIG_POSIX_TIMERS */
>
> Also this #ifdeffery is bonkers. How is TSN supposed to work without
> POSIX_TIMERS in the first place?
>
>> static const struct nla_policy etf_policy[TCA_ETF_MAX + 1] = {
>> [TCA_ETF_PARMS] = { .len = sizeof(struct tc_etf_qopt) },
>> };
>>
>> +static inline ktime_t get_now(struct Qdisc *sch, struct etf_sched_data *q)
>> +{
>> +#ifdef CONFIG_POSIX_TIMERS
>> +if (IS_ERR_OR_NULL(q->get_time)) {
>> +struct timespec64 ts;
>> +int err = posix_clock_gettime(q->pclock, );
>> +
>> +if (err) {
>> +pr_warn("Clock is disabled (%d) for queue %d\n",
>> +err, q->queue);
>> +return 0;
>
> That's really useful error handling.
>
>> +}
>> +return timespec64_to_ktime(ts);
>> +}
>> +#endif /* CONFIG_POSIX_TIMERS */
>> +return q->get_time();
>> +}
>> +
>> static inline int validate_input_params(struct tc_etf_qopt *qopt,
>> struct netlink_ext_ack *extack)
>> {
>> /* Check if params comply to the following rules:
>> * * Clockid and delta must be valid.
>> *
>> - * * Dynamic clockids are not supported.
>> + * * Dynamic CPU clockids are not supported.
>> *
>> * * Delta must be a positive or zero integer.
>> *
>> @@ -60,11 +82,22 @@ static inline int validate_input_params(struct
>> tc_etf_qopt *qopt,
>> * expect that system clocks have been synchronized to PHC.
>> */
>> if (qopt->clockid < 0) {
>> +#ifdef CONFIG_POSIX_TIMERS
>> +/**
>> + * Use of PTP clock through a posix clock.
>> + * The TC application must open the posix clock device file
>> + * and use the dynamic clockid from the file description.
>
> What? How is the code which calls into this guaranteed to have a valid
> file descriptor open for a particular dynamic posix clock?
>
>> + */
>> +if (!is_clockid_fd_clock(qopt->clockid)) {
>> +NL_SET_ERR_MSG(extack,
>> + "Dynamic CPU clockids are not
>> supported");
>> +return -EOPNOTSUPP;
>> +}
>> +#else /* CONFIG_POSIX_TIMERS */
>> NL_SET_ERR_MSG(extack, "Dynamic clockids are not supported");
>> return -ENOTSUPP;
>> -}
>> -
>> -if (qopt->clockid != CLOCK_TAI) {
>> +#endif /* CONFIG_POSIX_TIMERS */
>> +} else if (qopt->clockid != CLOCK_TAI) {
>> NL_SET_ERR_MSG(extack, "Invalid clockid. CLOCK_TAI must be
>> used");
>> return -EINVAL;
>> }
>> @@ -103,7 +136,7 @@ static bool is_packet_valid(struct Qdisc *sch, struct
>> etf_sched_data *q,
>> return false;
>>
>> skip:
>> -now = q->get_time();
>> +now = get_now(sch, q);
>
> Yuck.
>
> is_packet_valid() is invoked via:
>
> __dev_queue_xmit()
>__dev_xmit_skb()
> etf_enqueue_timesortedlist()
> is_packet_valid()
>
> __dev_queue_xmit() does
>
> rcu_read_lock_bh();
>
> and your get_now() does
>
> posix_clock_gettime()
> down_read(>rwsem);
>
> > FAIL
>
> down_read() might sleep and cannot be called from a BH disabled
> region. This clearly has never been tested with any mandatory debug
> option enabled. Why am I not surprised?
>
> Aside of accessing PCH clock being slow at hell this cannot ever work
> and there is no
Re: [PATCH v3 02/24] dt-bindings: memory: mediatek: Convert SMI to DT schema
On Wed, Sep 30, 2020 at 03:06:25PM +0800, Yong Wu wrote:
> Convert MediaTek SMI to DT schema.
>
> Signed-off-by: Yong Wu
> ---
> .../mediatek,smi-common.txt | 49 -
> .../mediatek,smi-common.yaml | 100 ++
> .../memory-controllers/mediatek,smi-larb.txt | 49 -
> .../memory-controllers/mediatek,smi-larb.yaml | 91
> 4 files changed, 191 insertions(+), 98 deletions(-)
> delete mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
> create mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> delete mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.txt
> create mode 100644
> Documentation/devicetree/bindings/memory-controllers/mediatek,smi-larb.yaml
>
> diff --git
> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
>
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
> deleted file mode 100644
> index b64573680b42..
> ---
> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.txt
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -SMI (Smart Multimedia Interface) Common
> -
> -The hardware block diagram please check bindings/iommu/mediatek,iommu.txt
> -
> -Mediatek SMI have two generations of HW architecture, here is the list
> -which generation the SoCs use:
> -generation 1: mt2701 and mt7623.
> -generation 2: mt2712, mt6779, mt8173 and mt8183.
> -
> -There's slight differences between the two SMI, for generation 2, the
> -register which control the iommu port is at each larb's register base. But
> -for generation 1, the register is at smi ao base(smi always on register
> -base). Besides that, the smi async clock should be prepared and enabled for
> -SMI generation 1 to transform the smi clock into emi clock domain, but that
> is
> -not needed for SMI generation 2.
> -
> -Required properties:
> -- compatible : must be one of :
> - "mediatek,mt2701-smi-common"
> - "mediatek,mt2712-smi-common"
> - "mediatek,mt6779-smi-common"
> - "mediatek,mt7623-smi-common", "mediatek,mt2701-smi-common"
> - "mediatek,mt8173-smi-common"
> - "mediatek,mt8183-smi-common"
> -- reg : the register and size of the SMI block.
> -- power-domains : a phandle to the power domain of this local arbiter.
> -- clocks : Must contain an entry for each entry in clock-names.
> -- clock-names : must contain 3 entries for generation 1 smi HW and 2 entries
> - for generation 2 smi HW as follows:
> - - "apb" : Advanced Peripheral Bus clock, It's the clock for setting
> - the register.
> - - "smi" : It's the clock for transfer data and command.
> - They may be the same if both source clocks are the same.
> - - "async" : asynchronous clock, it help transform the smi clock into the
> emi
> - clock domain, this clock is only needed by generation 1 smi HW.
> - and these 2 option clocks for generation 2 smi HW:
> - - "gals0": the path0 clock of GALS(Global Async Local Sync).
> - - "gals1": the path1 clock of GALS(Global Async Local Sync).
> - Here is the list which has this GALS: mt6779 and mt8183.
> -
> -Example:
> - smi_common: smi@14022000 {
> - compatible = "mediatek,mt8173-smi-common";
> - reg = <0 0x14022000 0 0x1000>;
> - power-domains = < MT8173_POWER_DOMAIN_MM>;
> - clocks = < CLK_MM_SMI_COMMON>,
> - < CLK_MM_SMI_COMMON>;
> - clock-names = "apb", "smi";
> - };
> diff --git
> a/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
>
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> new file mode 100644
> index ..76ecc7205438
> --- /dev/null
> +++
> b/Documentation/devicetree/bindings/memory-controllers/mediatek,smi-common.yaml
> @@ -0,0 +1,100 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
You relicense existing GPLv2 work. Please CC all contributors and
collect their acks/SoB.
> +%YAML 1.2
> +---
> +$id:
> http://devicetree.org/schemas/memory-controllers/mediatek,smi-common.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: SMI (Smart Multimedia Interface) Common
> +
> +maintainers:
> + - Yong Wu
> +
> +description: |+
> + The hardware block diagram please check bindings/iommu/mediatek,iommu.yaml
> +
> + MediaTek SMI have two generations of HW architecture, here is the list
> + which generation the SoCs use:
> + generation 1: mt2701 and mt7623.
> + generation 2: mt2712, mt6779, mt8173 and mt8183.
> +
> + There's slight differences between the two SMI, for generation 2, the
> + register which control the iommu port is at each larb's register base. But
> + for generation 1, the register is at smi ao base(smi always on register
> + base). Besides that, the smi async clock should be
Re: [PATCH] perf/x86/intel: Fix n_metric for the canceled group
On Wed, Sep 30, 2020 at 07:29:35AM -0700, kan.li...@linux.intel.com wrote:
> From: Kan Liang
>
> When a group that has TopDown members is failed to be scheduled, any
> later TopDown groups will not return valid values.
>
> Here is an example.
>
> A background perf that occupies all the GP counters and the fixed
> counter 1.
> $perf stat -e "{cycles,cycles,cycles,cycles,cycles,cycles,cycles,
> cycles,cycles}:D" -a
>
> A user monitors a TopDown group. It works well, because the fixed
> counter 3 and the PERF_METRICS are available.
> $perf stat -x, --topdown -- ./workload
>retiring,bad speculation,frontend bound,backend bound,
>18.0,16.1,40.4,25.5,
>
> Then the user tries to monitor a group that has TopDown members.
> Because of the cycles event, the group is failed to be scheduled.
> $perf stat -x, -e '{slots,topdown-retiring,topdown-be-bound,
> topdown-fe-bound,topdown-bad-spec,cycles}'
> -- ./workload
> ,,slots,0,0.00,,
> ,,topdown-retiring,0,0.00,,
> ,,topdown-be-bound,0,0.00,,
> ,,topdown-fe-bound,0,0.00,,
> ,,topdown-bad-spec,0,0.00,,
> ,,cycles,0,0.00,,
>
> The user tries to monitor a TopDown group again. It doesn't work anymore.
> $perf stat -x, --topdown -- ./workload
>
> ,
>
> In a txn, cancel_txn() is to truncate the event_list for a canceled
> group and update the number of events added in this transaction.
> However, the number of TopDown events added in this transaction is not
> updated. The kernel will probably fail to add new Topdown events.
>
> Check if the canceled group has Topdown events. If so, subtract the
> TopDown events from n_metric accordingly.
>
> Fixes: 7b2c05a15d29 ("perf/x86/intel: Generic support for hardware TopDown
> metrics")
> Reported-by: Andi Kleen
> Reviewed-by: Andi Kleen
> Signed-off-by: Kan Liang
> ---
> arch/x86/events/core.c | 10 ++
> 1 file changed, 10 insertions(+)
>
> diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
> index 0f3d01562ded..4cb3ccbe2d62 100644
> --- a/arch/x86/events/core.c
> +++ b/arch/x86/events/core.c
> @@ -2017,6 +2017,7 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
> {
> unsigned int txn_flags;
> struct cpu_hw_events *cpuc = this_cpu_ptr(_hw_events);
> + int i;
>
> WARN_ON_ONCE(!cpuc->txn_flags); /* no txn in flight */
>
> @@ -2031,6 +2032,15 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
>*/
> __this_cpu_sub(cpu_hw_events.n_added,
> __this_cpu_read(cpu_hw_events.n_txn));
> __this_cpu_sub(cpu_hw_events.n_events,
> __this_cpu_read(cpu_hw_events.n_txn));
> +
> + /* Subtract Topdown events in the canceled group from n_metric */
> + if (x86_pmu.intel_cap.perf_metrics && cpuc->n_metric) {
> + for (i = 0; i < cpuc->n_txn; i++) {
> + if (is_metric_event(cpuc->event_list[i +
> cpuc->n_events]))
> + __this_cpu_dec(cpu_hw_events.n_metric);
> + }
> + WARN_ON_ONCE(__this_cpu_read(cpu_hw_events.n_metric) < 0);
> + }
> perf_pmu_enable(pmu);
> }
Urgh, I'd much rather we add n_txn_metric. But also, while looking at
this, don't we have the same problem with n_pair ?
Something like this perhaps...
---
diff --git a/arch/x86/events/core.c b/arch/x86/events/core.c
index 757e49755e7c..9b7792c0b6fb 100644
--- a/arch/x86/events/core.c
+++ b/arch/x86/events/core.c
@@ -1066,6 +1066,7 @@ static int add_nr_metric_event(struct cpu_hw_events *cpuc,
if (cpuc->n_metric == INTEL_TD_METRIC_NUM)
return -EINVAL;
cpuc->n_metric++;
+ cpuc->n_txn_metric++;
}
return 0;
@@ -1089,8 +1090,10 @@ static int collect_event(struct cpu_hw_events *cpuc,
struct perf_event *event,
return -EINVAL;
cpuc->event_list[n] = event;
- if (is_counter_pair(>hw))
+ if (is_counter_pair(>hw)) {
cpuc->n_pair++;
+ cpuc->n_txn_pair++;
+ }
return 0;
}
@@ -2062,6 +2065,8 @@ static void x86_pmu_start_txn(struct pmu *pmu, unsigned
int txn_flags)
perf_pmu_disable(pmu);
__this_cpu_write(cpu_hw_events.n_txn, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_metric, 0);
+ __this_cpu_write(cpu_hw_events.n_txn_pair, 0);
}
/*
@@ -2087,6 +2092,8 @@ static void x86_pmu_cancel_txn(struct pmu *pmu)
*/
__this_cpu_sub(cpu_hw_events.n_added,
__this_cpu_read(cpu_hw_events.n_txn));
__this_cpu_sub(cpu_hw_events.n_events,
__this_cpu_read(cpu_hw_events.n_txn));
+ __this_cpu_sub(cpu_hw_events.n_metric,
__this_cpu_read(cpu_hw_events.n_txn_metric));
+ __this_cpu_sub(cpu_hw_events.n_pair,
__this_cpu_read(cpu_hw_events.n_txn_pair));
perf_pmu_enable(pmu);
}
diff --git a/arch/x86/events/perf_event.h b/arch/x86/events/perf_event.h
index 345442410a4d..6348105b6d30 100644
---
Re: [PATCH v2 0/3] drm: commit_work scheduling
On 09/30/20 14:17, Rob Clark wrote: > From: Rob Clark > > The android userspace treats the display pipeline as a realtime problem. > And arguably, if your goal is to not miss frame deadlines (ie. vblank), > it is. (See https://lwn.net/Articles/809545/ for the best explaination > that I found.) > > But this presents a problem with using workqueues for non-blocking > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can > preempt the worker. Which is not really the outcome you want.. once > the required fences are scheduled, you want to push the atomic commit > down to hw ASAP. For me thees 2 properties 1. Run ASAP 2. Finish the work un-interrupted Scream the workers need to be SCHED_FIFO by default. CFS can't give you these guarantees. IMO using sched_set_fifo() for these workers is the right thing. > > But the decision of whether commit_work should be RT or not really > depends on what userspace is doing. For a pure CFS userspace display > pipeline, commit_work() should remain SCHED_NORMAL. I'm not sure I agree with this. I think it's better to characterize tasks based on their properties/requirements rather than what the rest of the userspace is using. I do appreciate that maybe some of these tasks have varying requirements during their life time. e.g: they have RT property during specific critical section but otherwise are CFS tasks. I think the UI thread in Android behaves like that. It's worth IMO trying that approach I pointed out earlier to see if making RT try to pick an idle CPU rather than preempt CFS helps. Not sure if it'd be accepted but IMHO it's a better direction to consider and discuss. Or maybe you can wrap userspace pipeline critical section lock such that any task holding it will automatically be promoted to SCHED_FIFO and then demoted to CFS once it releases it. Haven't worked with display pipelines before, so hopefully this makes sense :-) Thanks -- Qais Yousef > > To handle this, convert non-blocking commit_work() to use per-CRTC > kthread workers, instead of system_unbound_wq. Per-CRTC workers are > used to avoid serializing commits when userspace is using a per-CRTC > update loop. And the last patch exposes the task id to userspace as > a CRTC property, so that userspace can adjust the priority and sched > policy to fit it's needs. > > > v2: Drop client cap and in-kernel setting of priority/policy in > favor of exposing the kworker tid to userspace so that user- > space can set priority/policy. > > Rob Clark (3): > drm/crtc: Introduce per-crtc kworker > drm/atomic: Use kthread worker for nonblocking commits > drm: Expose CRTC's kworker task id > > drivers/gpu/drm/drm_atomic_helper.c | 13 > drivers/gpu/drm/drm_crtc.c | 14 + > drivers/gpu/drm/drm_mode_config.c | 14 + > drivers/gpu/drm/drm_mode_object.c | 4 > include/drm/drm_atomic.h| 31 + > include/drm/drm_crtc.h | 8 > include/drm/drm_mode_config.h | 9 + > include/drm/drm_property.h | 9 + > 8 files changed, 98 insertions(+), 4 deletions(-) > > -- > 2.26.2 >
Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
On Fri, Oct 02, 2020 at 10:22:46AM +, Zulkifli, Muhammad Husaini wrote: > Hi Sudeep, > > >-Original Message- > >From: Michal Simek > >Sent: Friday, October 2, 2020 4:23 PM > >To: Sudeep Holla ; Zulkifli, Muhammad Husaini > > > >Cc: Hunter, Adrian ; michal.si...@xilinx.com; > >ulf.hans...@linaro.org; linux-...@vger.kernel.org; linux-arm- > >ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Raja Subramanian, > >Lakshmi Bai ; a...@arndb.de; Wan > >Mohamad, Wan Ahmad Zainie > >Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted > >Firmware Service call > > > >Hi Sudeep, > > > >On 01. 10. 20 17:35, Sudeep Holla wrote: > >> On Thu, Oct 01, 2020 at 10:21:48PM +0800, > >muhammad.husaini.zulki...@intel.com wrote: > >>> From: Muhammad Husaini Zulkifli > >>> > >>> Add generic firmware driver for Keem Bay SOC to support Arm Trusted > >>> Firmware Services call. > >>> > >>> Signed-off-by: Muhammad Husaini Zulkifli > >>> > >>> --- > >>> drivers/firmware/Kconfig | 1 + > >>> drivers/firmware/Makefile | 1 + > >>> drivers/firmware/intel/Kconfig | 14 +++ > >>> drivers/firmware/intel/Makefile| 4 + > >>> drivers/firmware/intel/keembay_smc.c | 119 + > >>> include/linux/firmware/intel/keembay_smc.h | 27 + > >>> 6 files changed, 166 insertions(+) > >>> create mode 100644 drivers/firmware/intel/Kconfig create mode > >>> 100644 drivers/firmware/intel/Makefile create mode 100644 > >>> drivers/firmware/intel/keembay_smc.c > >>> create mode 100644 include/linux/firmware/intel/keembay_smc.h > >>> > >>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig > >>> index fbd785dd0513..41de77d2720e 100644 > >>> --- a/drivers/firmware/Kconfig > >>> +++ b/drivers/firmware/Kconfig > >>> @@ -305,5 +305,6 @@ source "drivers/firmware/psci/Kconfig" > >>> source "drivers/firmware/smccc/Kconfig" > >>> source "drivers/firmware/tegra/Kconfig" > >>> source "drivers/firmware/xilinx/Kconfig" > >>> +source "drivers/firmware/intel/Kconfig" > >>> > >>> endmenu > >>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile > >>> index 99510be9f5ed..00f295ab9860 100644 > >>> --- a/drivers/firmware/Makefile > >>> +++ b/drivers/firmware/Makefile > >>> @@ -33,3 +33,4 @@ obj-y += psci/ > >>> obj-y+= smccc/ > >>> obj-y+= tegra/ > >>> obj-y+= xilinx/ > >>> +obj-y+= intel/ > >>> diff --git a/drivers/firmware/intel/Kconfig > >>> b/drivers/firmware/intel/Kconfig new file mode 100644 index > >>> ..b2b7a4e5410b > >>> --- /dev/null > >>> +++ b/drivers/firmware/intel/Kconfig > >>> @@ -0,0 +1,14 @@ > >>> +# SPDX-License-Identifier: GPL-2.0-only menu "Intel Firmware > >>> +Drivers" > >>> + > >>> +config KEEMBAY_FIRMWARE > >>> + bool "Enable Keem Bay firmware interface support" > >>> + depends on HAVE_ARM_SMCCC > >> > >> What is the version of SMCCC implemented ? > Our current Arm Trusted Firmware framework supports v1.1. > Does it mean I should use HAVE_ARM_SMCCC_DISCOVERY? Yes, HAVE_ARM_SMCCC_DISCOVERY is right dependency and allows you to get smccc version via arm_smccc_get_version which may be useful in future. > Not really clear about this. As for HAVE_ARM_SMCCC will include > support for the SMC and HVC. > Yes, but for sake of correctness I prefer HAVE_ARM_SMCCC_DISCOVERY. -- Regards, Sudeep
Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
Hi Michal, On Fri, Oct 02, 2020 at 10:23:02AM +0200, Michal Simek wrote: > Hi Sudeep, > > On 01. 10. 20 17:35, Sudeep Holla wrote: [...] > > > > What are the other uses of this KEEMBAY_SIP_* ? > > For now I tend to move this to the driver making use of it using > > arm_smccc_1_1_invoke directly if possible. I don't see the need for this > > to be separate driver. But do let us know the features implemented in the > > firmware. If it is not v1.1+, reasons for not upgrading as you need v1.1 > > for some CPU errata implementation. > > This driver has been created based on my request to move it out the mmc > driver. It looks quite hacky to have arm_smccc_res and call > arm_smccc_smc() also with some IDs where it is visible that the part of > ID is just based on any spec. OK, driver is fine but no dt-bindings as it is discoverable. It can also be just a wrapper library instead as it needs no explicit initialisation like drivers to setup. > Also in v1 he is just calling SMC. But maybe there is going a need to > call HVC instead which is something what device driver shouldn't decide > that's why IMHO doing step via firmware driver is much better approach. Agreed and one must use arm_smccc_get_conduit or something similar. No additional bindings for each and ever platform and driver that uses SMCCC please. > Of course if there is a better/cleaner way how this should be done I am > happy to get more information about it. > Let me know what you think about my thoughts stated above. -- Regards, Sudeep
Re: [PATCH v3 01/24] dt-bindings: iommu: mediatek: Convert IOMMU to DT schema
On Wed, Sep 30, 2020 at 03:06:24PM +0800, Yong Wu wrote: > Convert MediaTek IOMMU to DT schema. > > Signed-off-by: Yong Wu > --- > .../bindings/iommu/mediatek,iommu.txt | 103 > .../bindings/iommu/mediatek,iommu.yaml| 154 ++ > 2 files changed, 154 insertions(+), 103 deletions(-) > delete mode 100644 Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > create mode 100644 > Documentation/devicetree/bindings/iommu/mediatek,iommu.yaml > > diff --git a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > b/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > deleted file mode 100644 > index c1ccd8582eb2.. > --- a/Documentation/devicetree/bindings/iommu/mediatek,iommu.txt > +++ /dev/null > @@ -1,103 +0,0 @@ > -* Mediatek IOMMU Architecture Implementation > - > - Some Mediatek SOCs contain a Multimedia Memory Management Unit (M4U), and > -this M4U have two generations of HW architecture. Generation one uses flat > -pagetable, and only supports 4K size page mapping. Generation two uses the > -ARM Short-Descriptor translation table format for address translation. > - > - About the M4U Hardware Block Diagram, please check below: > - > - EMI (External Memory Interface) > - | > - m4u (Multimedia Memory Management Unit) > - | > - ++ > - || > - gals0-rx gals1-rx(Global Async Local Sync rx) > - || > - || > - gals0-tx gals1-tx(Global Async Local Sync tx) > - || Some SoCs may have GALS. > - ++ > - | > - SMI Common(Smart Multimedia Interface Common) > - | > - ++--- > - || > - | gals-rxThere may be GALS in some larbs. > - || > - || > - | gals-tx > - || > - SMI larb0SMI larb1 ... SoCs have several SMI local > arbiter(larb). > - (display) (vdec) > - || > - || > - +-+-+ +++ > - | | | ||| > - | | |... ||| ... There are different ports in each larb. > - | | | ||| > -OVL0 RDMA0 WDMA0 MC PP VLD > - > - As above, The Multimedia HW will go through SMI and M4U while it > -access EMI. SMI is a bridge between m4u and the Multimedia HW. It contain > -smi local arbiter and smi common. It will control whether the Multimedia > -HW should go though the m4u for translation or bypass it and talk > -directly with EMI. And also SMI help control the power domain and clocks for > -each local arbiter. > - Normally we specify a local arbiter(larb) for each multimedia HW > -like display, video decode, and camera. And there are different ports > -in each larb. Take a example, There are many ports like MC, PP, VLD in the > -video decode local arbiter, all these ports are according to the video HW. > - In some SoCs, there may be a GALS(Global Async Local Sync) module between > -smi-common and m4u, and additional GALS module between smi-larb and > -smi-common. GALS can been seen as a "asynchronous fifo" which could help > -synchronize for the modules in different clock frequency. > - > -Required properties: > -- compatible : must be one of the following string: > - "mediatek,mt2701-m4u" for mt2701 which uses generation one m4u HW. > - "mediatek,mt2712-m4u" for mt2712 which uses generation two m4u HW. > - "mediatek,mt6779-m4u" for mt6779 which uses generation two m4u HW. > - "mediatek,mt7623-m4u", "mediatek,mt2701-m4u" for mt7623 which uses > - generation one m4u HW. > - "mediatek,mt8173-m4u" for mt8173 which uses generation two m4u HW. > - "mediatek,mt8183-m4u" for mt8183 which uses generation two m4u HW. > -- reg : m4u register base and size. > -- interrupts : the interrupt of m4u. > -- clocks : must contain one entry for each clock-names. > -- clock-names : Only 1 optional clock: > - - "bclk": the block clock of m4u. > - Here is the list which require this "bclk": > - - mt2701, mt2712, mt7623 and mt8173. > - Note that m4u use the EMI clock which always has been enabled before kernel > - if there is no this "bclk". > -- mediatek,larbs : List of phandle to the local arbiters in the current Socs. > - Refer to bindings/memory-controllers/mediatek,smi-larb.txt. It must sort > - according to the local arbiter index, like larb0, larb1, larb2... > -- iommu-cells : must be 1. This is the mtk_m4u_id according to the HW. > - Specifies the mtk_m4u_id as defined in > - dt-binding/memory/mt2701-larb-port.h for mt2701, mt7623 > - dt-binding/memory/mt2712-larb-port.h for mt2712, > - dt-binding/memory/mt6779-larb-port.h for mt6779, > -
Re: [PATCH v2] drivers/perf: Add support for ARMv8.3-SPE
On Wed, Sep 30, 2020 at 05:31:35PM +0800, Wei Li wrote:
> Armv8.3 extends the SPE by adding:
> - Alignment field in the Events packet, and filtering on this event
> using PMSEVFR_EL1.
> - Support for the Scalable Vector Extension (SVE).
>
> The main additions for SVE are:
> - Recording the vector length for SVE operations in the Operation Type
> packet. It is not possible to filter on vector length.
> - Incomplete predicate and empty predicate fields in the Events packet,
> and filtering on these events using PMSEVFR_EL1.
>
> Update the check of pmsevfr for empty/partial predicated SVE and
> alignment event in SPE driver. For adaption by the version of SPE,
> expose 'pmsver' as cap attribute to userspace.
>
> Signed-off-by: Wei Li
> ---
> v1 -> v2:
> - Rename 'pmuver' to 'pmsver', change it's type to 'u16' from 'int'.
>(Suggested by Will and Leo.)
> - Expose 'pmsver' as cap attribute through sysfs, instead of printing.
>(Suggested by Will.)
> ---
> arch/arm64/include/asm/sysreg.h | 4 +++-
> drivers/perf/arm_spe_pmu.c | 18 --
> 2 files changed, 19 insertions(+), 3 deletions(-)
>
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 554a7e8ecb07..f4f9c1fc6398 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -281,7 +281,6 @@
> #define SYS_PMSFCR_EL1_ST_SHIFT 18
>
> #define SYS_PMSEVFR_EL1 sys_reg(3, 0, 9, 9, 5)
> -#define SYS_PMSEVFR_EL1_RES0 0x00ff0f55UL
>
> #define SYS_PMSLATFR_EL1 sys_reg(3, 0, 9, 9, 6)
> #define SYS_PMSLATFR_EL1_MINLAT_SHIFT0
> @@ -787,6 +786,9 @@
> #define ID_AA64DFR0_PMUVER_8_5 0x6
> #define ID_AA64DFR0_PMUVER_IMP_DEF 0xf
>
> +#define ID_AA64DFR0_PMSVER_8_2 0x1
> +#define ID_AA64DFR0_PMSVER_8_3 0x2
> +
> #define ID_DFR0_PERFMON_SHIFT24
>
> #define ID_DFR0_PERFMON_8_1 0x4
> diff --git a/drivers/perf/arm_spe_pmu.c b/drivers/perf/arm_spe_pmu.c
> index cc00915ad6d1..52e7869f5621 100644
> --- a/drivers/perf/arm_spe_pmu.c
> +++ b/drivers/perf/arm_spe_pmu.c
> @@ -54,7 +54,7 @@ struct arm_spe_pmu {
> struct hlist_node hotplug_node;
>
> int irq; /* PPI */
> -
> + u16 pmsver;
> u16 min_period;
> u16 counter_sz;
>
> @@ -80,6 +80,15 @@ struct arm_spe_pmu {
> /* Keep track of our dynamic hotplug state */
> static enum cpuhp_state arm_spe_pmu_online;
>
> +static u64 sys_pmsevfr_el1_mask[] = {
> + [ID_AA64DFR0_PMSVER_8_2] = GENMASK_ULL(63, 48) | GENMASK_ULL(31, 24) |
> + GENMASK_ULL(15, 12) | BIT_ULL(7) | BIT_ULL(5) | BIT_ULL(3) |
> + BIT_ULL(1),
> + [ID_AA64DFR0_PMSVER_8_3] = GENMASK_ULL(63, 48) | GENMASK_ULL(31, 24) |
> + GENMASK_ULL(18, 17) | GENMASK_ULL(15, 11) | BIT_ULL(7) |
> + BIT_ULL(5) | BIT_ULL(3) | BIT_ULL(1),
> +};
Ok, so I finally figured out what I don't like about this: it's the fact
that the RES0 mask only ever reduces, but we have no way of ensuring that
by construction with this approach. In other words, it's a bit brittle to
keep all of these things defined entirely separately from one another.
How about a small change so that we define things like:
#define SYS_PMSEVFR_EL1_RES0_8_2SYS_PMSEVFR_EL1_RES0 &
~(...)
#define SYS_PMSEVFR_EL1_RES0_8_3SYS_PMSEVFR_EL1_RES0_8_2 &
~(...)
where the '...' parts identify the bits that are no longer RES0 for that
version of the architecture?
What do you think?
Will
Re: [Linux-kernel-mentees][PATCH v2] bluetooth: hci_h5: close serdev device and free hu in h5_close
On 02/10/20 3:52 pm, Hans de Goede wrote:
> Hi,
>
> On 10/1/20 9:43 PM, Anant Thazhemadam wrote:
>> When h5_close() gets called, the memory allocated for the hu gets
>> freed only if hu->serdev doesn't exist. This leads to a memory leak.
>> So when h5_close() is requested, close the serdev device instance and
>> free the memory allocated to the hu entirely instead.
>>
>> Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated
>> devices")
>> Reported-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
>> Tested-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
>> Signed-off-by: Anant Thazhemadam
>
> So 2 comments to this:
>
> 1. You claim this change fixes a memory-leak, but in the serdev case
> the memory is allocated in h5_serdev_probe() like this:
>
> h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);
>
> So its lifetime is tied to the lifetime of the driver being bound
> to the serdev and it is automatically freed when the driver gets
> unbound. If you had looked at where the h5 struct gets allocated
> in h5_close()-s counterpart, h5_open() then you could have seen
> this there:
>
> if (hu->serdev) {
> h5 = serdev_device_get_drvdata(hu->serdev);
> } else {
> h5 = kzalloc(sizeof(*h5), GFP_KERNEL);
> if (!h5)
> return -ENOMEM;
> }
>
> So it is very clear here, that the kzalloc only happens in the
> if (!hu->serdev) which clearly makes the kfree() have the same
> condition the right thing todo. In the hu->serdev the data which
> was allocated by h5_serdev_probe() is used instead and no alloc
> happens inside h5_open()
>
> In general when looking at resource teardown you should also look
> at where they are allocated in the mirror function
> and the teardown should mirror the alloc code.
>
> So the main change of your commit is wrong:
>
> NACK.
>
>
> 2. You are making multiple changes in a single commit here, I count at
> least 3. When ever you are making multiple changes like this, you should
> really
> split the commit up in 1 commit per change and explain in each commit
> message why that change is being made (why it is necessary). Writing
> commit messages like this also leads to you double-checking your own
> work by carefully considering why you are making the changes.
>
> So about the 3 different changes:
>
> 2a) Make the kfree(h5) call unconditionally, which as mentioned above
> is wrong.
>
Hmm, I understand, thank you.
> 2b) Introduce a call to kfree_skb(h5->rx_skb); which is not mentioned in
> the commit message at all. This looks like it would actually be a sensible
> change, at least in the "if (!hu->serdev)" case. When using the serdev
> interface then just freeing it will leave a dangling pointer, so it
> would be better (for both the hu->serdev and the !hu->serdev cases)
> to call h5_reset_rx() on close instead which does:
>
> if (h5->rx_skb) {
> kfree_skb(h5->rx_skb);
> h5->rx_skb = NULL;
> }
>
This was the first thing I did! (and I tested and it works too)
https://syzkaller.appspot.com/text?tag=Patch=13acf91790
However, I was worried that doing so might mean freeing an skb
that might be required somewhere by the serdev connection (which could
possibly lead to a null ptr dereference), so I thought closing the connection
altogether might be a better alternative.
But now, since I've been told better, I'll submit a v3, that makes the more
sensible change and doesn't close the serdev_device, with a much more
informative commit message!
I also found a similar implementation in mrvl_close() where the serdev_device
was being closed and felt it might be the right way to go. But I now know that
I shouldn't have overlooked the fact that the open functions for both differ.
I'm sorry for this.
Like you mentioned, I should've given more priority to the mirror function,
and looked to see if even they were along the same lines, and I will be sure to
keep that in mind.
> 2c) Introduce a call to serdev_device_close(), without really explaining
> why in the commit message. Again if you would have looked at the mirror
> h5_close() function then you see no serdev_device_open() there.
> Actually serdev_device_open() is not called anywhere in the hci_h5.c code.
>
> Digging a little deeper (using grep) shows that hci_uart_register_device()
> calls serdev_device_open() and hci_uart_register_device() gets called from:
> h5_serdev_probe(), likewise hci_uart_unregister_device() calls
> serdev_device_close() for us and that gets called from h5_serdev_remove(),
> so there is no need to call serdev_device_close() from h5_close() and doing
> so will actually break things, because then the serdev will be left closed
> on a subsequent h5_open() call.
>
> Regards,
>
> Hans
>
Thank you so much for this review, and for your time! :)
Thanks,
Anant
Re: [PATCH v6 2/7] dt-bindings: leds: Convert pwm to yaml
On Fri, 2 Oct 2020 at 12:46, Alexander Dahl wrote: > > Hei hei, > > Am Freitag, 2. Oktober 2020, 11:31:09 CEST schrieb Krzysztof Kozlowski: > > On Thu, 1 Oct 2020 at 01:52, Alexander Dahl wrote: > > > The example was adapted in the following ways: > > > > > > - make use of the now supported 'function' and 'color' properties > > > - remove pwm nodes, those are documented elsewhere > > > - tweake node names to be matched by new dtschema rules > > > > tweak? or align? > > Depends on if schema actually checks it (child nodes) or if it's just DT > policy (parent node). I'll reword in v7. > > > > License was discussed with the original author. > > > > Since you relicense their work, you need an ack or signed off from > > every author. You cannot just say "I discussed" and post it. That way > > I could pretend (lie) I talked to Linus and try to relicense Linux to > > BSD... > > I know. Peter promised to give his Ack publicly on the list back when I > worked on v2 or v3, so he is in Cc since then, but apparently he did not yet > post it. ;-) > > > You need acks/SoB from Peter and Russel. > > Well, I should add Russel in v7, too, then. Yes, please. For the patch itself: Reviewed-by: Krzysztof Kozlowski Best regards, Krzysztof
Re: [PATCH v2 0/3] drm: commit_work scheduling
On Thu, Oct 01, 2020 at 05:25:55PM +0200, Daniel Vetter wrote: > On Thu, Oct 1, 2020 at 5:15 PM Rob Clark wrote: > > > > On Thu, Oct 1, 2020 at 12:25 AM Daniel Vetter wrote: > > > > > > On Wed, Sep 30, 2020 at 11:16 PM Rob Clark wrote: > > > > > > > > From: Rob Clark > > > > > > > > The android userspace treats the display pipeline as a realtime problem. > > > > And arguably, if your goal is to not miss frame deadlines (ie. vblank), > > > > it is. (See https://lwn.net/Articles/809545/ for the best explaination > > > > that I found.) > > > > > > > > But this presents a problem with using workqueues for non-blocking > > > > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can > > > > preempt the worker. Which is not really the outcome you want.. once > > > > the required fences are scheduled, you want to push the atomic commit > > > > down to hw ASAP. > > > > > > > > But the decision of whether commit_work should be RT or not really > > > > depends on what userspace is doing. For a pure CFS userspace display > > > > pipeline, commit_work() should remain SCHED_NORMAL. > > > > > > > > To handle this, convert non-blocking commit_work() to use per-CRTC > > > > kthread workers, instead of system_unbound_wq. Per-CRTC workers are > > > > used to avoid serializing commits when userspace is using a per-CRTC > > > > update loop. And the last patch exposes the task id to userspace as > > > > a CRTC property, so that userspace can adjust the priority and sched > > > > policy to fit it's needs. > > > > > > > > > > > > v2: Drop client cap and in-kernel setting of priority/policy in > > > > favor of exposing the kworker tid to userspace so that user- > > > > space can set priority/policy. > > > > > > Yeah I think this looks more reasonable. Still a bit irky interface, > > > so I'd like to get some kworker/rt ack on this. Other opens: > > > - needs userspace, the usual drill > > > > fwiw, right now the userspace is "modetest + chrt".. *probably* the > > userspace will become a standalone helper or daemon, mostly because > > the chrome gpu-process sandbox does not allow setting SCHED_FIFO. I'm > > still entertaining the possibility of switching between rt and cfs > > depending on what is in the foreground (ie. only do rt for android > > apps). > > > > > - we need this also for vblank workers, otherwise this wont work for > > > drivers needing those because of another priority inversion. > > > > I have a thought on that, see below.. > > Hm, not seeing anything about vblank worker below? > > > > - we probably want some indication of whether this actually does > > > something useful, not all drivers use atomic commit helpers. Not sure > > > how to do that. > > > > I'm leaning towards converting the other drivers over to use the > > per-crtc kwork, and then dropping the 'commit_work` from atomic state. > > I can add a patch to that, but figured I could postpone that churn > > until there is some by-in on this whole idea. > > i915 has its own commit code, it's not even using the current commit > helpers (nor the commit_work). Not sure how much other fun there is. I don't think we want per-crtc threads for this in i915. Seems to me easier to guarantee atomicity across multiple crtcs if we just commit them from the same thread. -- Ville Syrjälä Intel
Re: [PATCH v2 1/2] system_data_types.7: Add 'void *'
On Fri, 2 Oct 2020 at 09:28, Alejandro Colomar via Gcc wrote: > However, it might be good that someone starts a page called > 'type_qualifiers(7)' or something like that. Who is this for? Who is trying to learn C from man pages? Should somebody stop them?
Re: [net-next PATCH v1 3/7] net: phy: Introduce fwnode_get_phy_id()
On 01/10/2020 05:00, Calvin Johnson wrote:
On Wed, Sep 30, 2020 at 08:19:02PM +0200, Andrew Lunn wrote:
On Wed, Sep 30, 2020 at 07:07:25PM +0100, Russell King - ARM Linux admin wrote:
On Wed, Sep 30, 2020 at 06:34:40PM +0200, Andrew Lunn wrote:
@@ -2866,7 +2888,15 @@ EXPORT_SYMBOL_GPL(device_phy_find_device);
*/
struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
{
- return fwnode_find_reference(fwnode, "phy-handle", 0);
+ struct fwnode_handle *phy_node;
+
+ phy_node = fwnode_find_reference(fwnode, "phy-handle", 0);
+ if (is_acpi_node(fwnode) || !IS_ERR(phy_node))
+ return phy_node;
+ phy_node = fwnode_find_reference(fwnode, "phy", 0);
+ if (IS_ERR(phy_node))
+ phy_node = fwnode_find_reference(fwnode, "phy-device", 0);
+ return phy_node;
Why do you have three different ways to reference a PHY?
Compatibility with the DT version - note that "phy" and "phy-device"
are only used for non-ACPI fwnodes. This should allow us to convert
drivers where necessary without fear of causing DT regressions.
Ah.
A comment would be good here.
Sure. Will add comment.
Actually, I agree with Andrew. I don't see why new properties are being
defined for following a reference from a property to a PHY node. This
function shouldn't need to change at all.
g.
Re: [PATCH v6 2/7] dt-bindings: leds: Convert pwm to yaml
Hei hei, Am Freitag, 2. Oktober 2020, 11:31:09 CEST schrieb Krzysztof Kozlowski: > On Thu, 1 Oct 2020 at 01:52, Alexander Dahl wrote: > > The example was adapted in the following ways: > > > > - make use of the now supported 'function' and 'color' properties > > - remove pwm nodes, those are documented elsewhere > > - tweake node names to be matched by new dtschema rules > > tweak? or align? Depends on if schema actually checks it (child nodes) or if it's just DT policy (parent node). I'll reword in v7. > > License was discussed with the original author. > > Since you relicense their work, you need an ack or signed off from > every author. You cannot just say "I discussed" and post it. That way > I could pretend (lie) I talked to Linus and try to relicense Linux to > BSD... I know. Peter promised to give his Ack publicly on the list back when I worked on v2 or v3, so he is in Cc since then, but apparently he did not yet post it. ;-) > You need acks/SoB from Peter and Russel. Well, I should add Russel in v7, too, then. Thanks Alex
Re: [PATCH v2 08/12] ARM: dts: imx6dl-pico: fix board compatibles
On Fri, Oct 02, 2020 at 10:41:19AM +0200, Marco Felsch wrote: > Hi, > > sorry for jumping in. > > On 20-10-02 10:20, Krzysztof Kozlowski wrote: > > On Fri, Oct 02, 2020 at 09:41:28AM +0200, Ahmad Fatoum wrote: > > > Hello, > > > > > > On 10/1/20 12:37 PM, Krzysztof Kozlowski wrote: > > > >> The existing binding doesn't cover these boards then and needs to be > > > >> extended, no? How about following patch? > > > > > > > > What do you mean it doesn't cover? It was added exactly to handle them: > > > > + - technexion,imx6q-pico-dwarf # TechNexion i.MX6Q > > > > Pico-Dwarf > > > > + - technexion,imx6q-pico-hobbit # TechNexion i.MX6Q > > > > Pico-Hobbit > > > > + - technexion,imx6q-pico-nymph # TechNexion i.MX6Q > > > > Pico-Nymph > > > > + - technexion,imx6q-pico-pi # TechNexion i.MX6Q > > > > Pico-Pi > > > > > > > > > > Still they are unused. So I'd think these boards should be handled like > > > boards > > > that predated bindings: a binding is written that doesn't break existing > > > users. > > > > OK, let's assume the binding is not correct and DTSes are good. > > > > > > > > >> [I guess we need to keep the two-compatible list they were originally > > > >> in for compatibility even if it's unused among upstream device trees?] > > > > > > > > You want to change both the binding (thus breaking the ABI) and update > > > > the DTS to reflect new ABI. Then why having a binding at all? > > > > > > If we leave the old two-compatible enumeration intact, there is no ABI > > > broken. > > > > Just to clarify, because I don't get here the "no ABI broken" part: > > ABI is the binding, not the DTS. We can change intree DTS as we like, > > replace compatibles, add nodes, remove nodes. There is no stability > > requirement for DTS contents. > > > > If we leave two-compatible binding intact, it is a broken binding since > > beginning. Removing non-working, fake ABI is not breaking it because it > > could never work. > > The problem here is that it wasn't covered by the review and now we have > the mess. I see the DTB and the Bootloader as Firmware. Now imagine if > the bootloader for these boards had some dt-fixup logic which won't > apply anymore or if the bootloader board init won't get called anymore > since the bootloader folks used the compatible found in the DTS. This > can cause a regression if the old Bootloader tries to boot the new > Kernel+DTS. Good points. It's nice to have a binding documented but it is more likely that bootloader guys were depending on actual contents of DTS. > > > > > I would assume that either binding is correct or DTS. You propose that > > > > both are wrong and both need changes... in such case this is clearly > > > > broken. > > > > > > IMO the DTS is the correct one. If you want to honor the author's > > > intention > > > that each base board has a different compatible, it should be an extra > > > compatible and not replace the existing one that may be already in use. > > Question is what was the author's intention? @Fabio do you have any > comments here? > > > OK, we can go with DTS approach. I fixed few of such cases as well, > > assuming that DTS was intended and binding was incorrect. In such case > > all boards will be documented under one compatible technexion,imx6q-pico > > and DTS will not be changed. > > Or keep the exisiting bindings and adding the new one. Therefore the > yaml needs to handle two cases for each imx6[qdl]: > compatible = "technexion,imx6dl-pico-dwarf", "technexion,imx6dl-pico", > "fsl,imx6dl"; > and > compatible = "technexion,imx6dl-pico", "fsl,imx6dl"; This is the combination I wanted to avoid because it kind of proofs that both (binding and DTS) were incorrect or insufficient. If both are incorrect, then there might be no point to keep it stable. Few other i.MX boards use one compatible for multiple DTS. Usually it is a module's compatible and boards just do not add their own. Best regards, Krzysztof
Re: OpRegion conflicts for Skylake LPSS
On Fri, Oct 02, 2020 at 01:10:23AM +0300, Laurent Pinchart wrote: > Hi Mika, > > Reviving an old thread. Very old :-) > On Mon, May 02, 2016 at 01:35:01PM +0300, Mika Westerberg wrote: > > On Sun, May 01, 2016 at 12:47:58AM +0200, Ben Gamari wrote: > > > Mika Westerberg writes: > > > > On Fri, Apr 29, 2016 at 09:30:27AM +0200, Ben Gamari wrote: > > > >> Ben Gamari writes: > > > >> > > > >> > [ Unknown signature status ] > > > >> > Mika Westerberg writes: > > > >> > > > > >> >> On Tue, Apr 26, 2016 at 02:44:13AM +0200, Ben Gamari wrote: > > > >> >>> > > > >> > snip > > > >> > > > > >> >>> It looks very much like these are describing the same device. > > > >> >>> Perhaps > > > >> >>> the lpss driver should be binding to this ACPI node? Or perhaps > > > >> >>> this is > > > >> >>> a firmware issue? Any guidance would be greatly appreciated. > > > >> >> > > > >> >> Can you send me full acpidump of that machine? > > > >> > > > > >> > It can be found at > > > >> > https://github.com/bgamari/dell-e7470-dsdt/blob/master/acpi.log. > > > >> > > > > >> Did this provide any insight? Let me know if more information would be > > > >> helpful. > > > > > > > > Sorry about the delay. > > > > > > No worries. > > > > > > > The GEXP device is most probably a GPIO expander that is connected to > > > > one of the I2C buses. And it indeed looks to use directly the I2C host > > > > controller registers so kernel rightfully complains about that. > > > > > > > > Are you able to run Windows on that machine? If yes, it would be nice to > > > > know if the INT3446 I2C device is shown in the device manager. > > > > > > I had the original SSD that came with the machine with the original > > > Windows 7 installation intact. I popped it in and found no such device. > > > I then updated to Windows 10 (albeit still booting with the legacy BIOS, > > > not EFI) and found that once again there is no such device shown in > > > device manager. > > > > That's what I would expect. ACPI spec says that if there is an OpRegion > > touching the same registers than PCI device the OS should not load any > > driver for that device. I guess this is exactly what Windows does. > > > > Linux does it also but it in addition it issues a scary warning which > > might get users thinking there is something wrong with their system. > > I'm trying to get camera sensors detected on a Microsoft Surface Go 2 > machine (ACPI dumps available at > https://github.com/linux-surface/acpidumps/tree/master/surface_go_2). > The CPU is an Intel Pentium Gold 4425Y, based on Kaby Lake-Y. The DSDT > has been carefully designed, with great care to make it as useless as > possible, so I'm experiencing a few issues. I think Sakari has a laptop with PCA953x driver in ASL (AML). I remember it had some issues. > One of the camera sensors is connected to I2C4, backed by an LPSS I2C > controller. > > 00:19.2 Signal processing controller: Intel Corporation Sunrise Point-LP > Serial IO I2C Controller #4 (rev 21) > Subsystem: QUANTA Computer Inc Sunrise Point-LP Serial IO I2C > Controller > Flags: fast devsel, IRQ 34 > Memory at b1648000 (64-bit, non-prefetchable) [size=4K] > Capabilities: [80] Power Management version 3 > Capabilities: [90] Vendor Specific Information: Len=14 > Kernel modules: intel_lpss_pci > > Unfortunately the driver fails to probe due to the same issue reported > by Ben: > > [2.060237] intel-lpss :00:19.2: enabling device ( -> 0002) > [2.060483] ACPI Warning: SystemMemory range > 0xB1648000-0xB16481FF conflicts with OpRegion > 0xB1648000-0xB1648207 (\_SB.PCI0.GEXP.BAR0) > (20200528/utaddress-213) > [2.060489] ACPI: If an ACPI driver is available for this device, you > should use it instead of the native driver > [2.060726] intel-lpss: probe of :00:19.2 failed with error -16 > > I've checked the GEXP device in the DSDT, and it includes an LPSS I2C > host controller driver in AML, using an OpRegion that covers the I2C > controller registers. > > Adding acpi_enforce_resources=lax to the kernel command line allows the > I2C controller to be probed, but that's hardly a good solution, as two > drivers (one in the DSDT, one in the kernel) that poke the same hardware > is calling for trouble. > > I've noticed that Windows maps the devices to different addresses than > Linux. On Windows, the I2C controllers are at > > I2C0 (8086:9d60): 0xfe40f000 - 0xfe40 (IRQ 16) > I2C1 (8086:9d61): 0xfe40e000 - 0xfe40efff (IRQ 17) > I2C2 (8086:96d2): 0xfe40d000 - 0xfe40dfff (IRQ 18) > I2C3 (8086:96d3): 0xfe40c000 - 0xfe40cfff (IRQ 19) > I2C4 (8086:96d4): 0xfe409000 - 0xfe409fff (IRQ 34) > > while on Linux they're at > > I2C0 (8086:9d60): 0xb1642000 - 0xb1642fff (IRQ 16) > I2C1 (8086:9d61): 0xb1643000 - 0xb1643fff (IRQ 17) > I2C2 (8086:96d2): 0xb1644000 - 0xb1644fff (IRQ 18) > I2C3 (8086:96d3): 0xb1645000 - 0xb1645fff (IRQ 19) > I2C4 (8086:96d4):
[RFC] Status of orinoco_usb
I was trying to get rid of the in in_softirq() in ezusb_req_ctx_wait()
within the orinoco usb driver,
drivers/net/wireless/intersil/orinoco/orinoco_usb.c. A small snippet:
| static void ezusb_req_ctx_wait(struct ezusb_priv *upriv,
|struct request_context *ctx)
…
| if (in_softirq()) {
| /* If we get called from a timer, timeout timers don't
| * get the chance to run themselves. So we make sure
| * that we don't sleep for ever */
| int msecs = DEF_TIMEOUT * (1000 / HZ);
|
| while (!try_wait_for_completion(>done) &&
msecs--)
| udelay(1000);
| } else {
| wait_for_completion(>done);
…
| }
This is broken. The EHCI and XHCI HCD will complete the URB in
BH/tasklet. Should we ever get here in_softirq() then we will spin
here/wait here until the timeout passes because the tasklet won't be
able to run. OHCI/UHCI HCDs still complete in hard-IRQ so it would work
here.
Is it possible to end up here in softirq context or is this a relic?
Well I have no hardware but I see this:
orinoco_set_monitor_channel() [I assume that this is fully preemtible]
-> orinoco_lock() [this should point to ezusb_lock_irqsave() which
does spin_lock_bh(lock), so from here on
in_softirq() returns true]
-> hw->ops->cmd_wait() [-> ezusb_docmd_wait()]
-> ezusb_alloc_ctx() [ sets ctx->in_rid to EZUSB_RID_ACK/0x0710 ]
-> ezusb_access_ltv()
-> if (ctx->in_rid)
-> ezusb_req_ctx_wait(upriv, ctx);
-> ctx->state should be EZUSB_CTX_REQ_COMPLETE so we end up in
the while loop above. So we udelay() 3 * 1000 * 1ms = 3sec.
-> Then ezusb_access_ltv() should return with an error due to
timeout.
This isn't limited to exotic features like monitor mode. orinoco_open()
does orinoco_lock() followed by orinoco_hw_program_rids() which in the
end invokes ezusb_write_ltv(,, EZUSB_RID_ACK) which is non-zero and also
would block (ezusb_xmit() would use 0 as the last argument so it won't
block).
I don't see how this driver can work on EHCI/XHCI HCD as of today.
The driver is an orphan since commit
3a59babbee409 ("orinoco: update status in MAINTAINERS")
which is ten years ago. If I replace in_softirq() with a `may_sleep'
argument then it is still broken.
Should it be removed?
Sebastian
[PATCH] media: rcar-vin: rcar-dma: Fix setting VNIS_REG for RAW8 formats
pixelformat in vin priv structure holds V4L2_PIX_FMT_* and not
MEDIA_BUS_FMT_* so make sure we check against V4L2_PIX_FMT_* formats
while setting the VNIS_REG.
Fixes: 8c3e0f67df6c9 ("media: rcar-vin: Extend RAW8 support to all RGB layouts")
Signed-off-by: Lad Prabhakar
Reviewed-by: Biju Das
---
Hi Hans,
If it isn't too late for v5.10 could you please queue up this patch.
Cheers,
Prabhakar
---
drivers/media/platform/rcar-vin/rcar-dma.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
b/drivers/media/platform/rcar-vin/rcar-dma.c
index e9a47b705acc..692dea300b0d 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -600,10 +600,10 @@ void rvin_crop_scale_comp(struct rvin_dev *vin)
* format in 2 pixel unit hence configure VNIS_REG as stride / 2.
*/
switch (vin->format.pixelformat) {
- case MEDIA_BUS_FMT_SBGGR8_1X8:
- case MEDIA_BUS_FMT_SGBRG8_1X8:
- case MEDIA_BUS_FMT_SGRBG8_1X8:
- case MEDIA_BUS_FMT_SRGGB8_1X8:
+ case V4L2_PIX_FMT_SBGGR8:
+ case V4L2_PIX_FMT_SGBRG8:
+ case V4L2_PIX_FMT_SGRBG8:
+ case V4L2_PIX_FMT_SRGGB8:
stride /= 2;
break;
default:
--
2.17.1
Re: [RFC-PATCH 2/4] mm: Add __rcu_alloc_page_lockless() func.
On Fri, Oct 02, 2020 at 11:58:58AM +0200, Peter Zijlstra wrote: > > It's enabled by default by enough distros that adding too many checks > > is potentially painful. Granted it would be missed by most benchmarking > > which tend to control allocations from userspace but a lot of performance > > problems I see are the "death by a thousand cuts" variety. > > Oh quite agreed, aka death by accounting. But if people are enabling > DEBUG options in production kernels, there's something wrong, no? > You'd think but historically I believe DEBUG_VM was enabled for some distributions because it made certain classes of problems easier to debug early. There is also a recent trend for enabling various DEBUG options for "hardening" even when they protect very specific corner cases or are for intended for kernel development. I've pushed back where I have an opinion that matters but it's generally corrosive. > Should we now go add CONFIG_REALLY_DEBUG_STAY_AWAY_ALREADY options? It's heading in that direction :( -- Mel Gorman SUSE Labs
[tip: x86/core] x86/dumpstack: Fix misleading instruction pointer error message
The following commit has been merged into the x86/core branch of tip:
Commit-ID: 238c91115cd05c71447ea071624a4c9fe661f970
Gitweb:
https://git.kernel.org/tip/238c91115cd05c71447ea071624a4c9fe661f970
Author:Mark Mossberg
AuthorDate:Fri, 02 Oct 2020 04:29:16
Committer: Borislav Petkov
CommitterDate: Fri, 02 Oct 2020 11:33:55 +02:00
x86/dumpstack: Fix misleading instruction pointer error message
Printing "Bad RIP value" if copy_code() fails can be misleading for
userspace pointers, since copy_code() can fail if the instruction
pointer is valid but the code is paged out. This is because copy_code()
calls copy_from_user_nmi() for userspace pointers, which disables page
fault handling.
This is reproducible in OOM situations, where it's plausible that the
code may be reclaimed in the time between entry into the kernel and when
this message is printed. This leaves a misleading log in dmesg that
suggests instruction pointer corruption has occurred, which may alarm
users.
Change the message to state the error condition more precisely.
[ bp: Massage a bit. ]
Signed-off-by: Mark Mossberg
Signed-off-by: Borislav Petkov
Link: https://lkml.kernel.org/r/20201002042915.403558-1-mark.mossb...@gmail.com
---
arch/x86/kernel/dumpstack.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/x86/kernel/dumpstack.c b/arch/x86/kernel/dumpstack.c
index 48ce445..ea8d51e 100644
--- a/arch/x86/kernel/dumpstack.c
+++ b/arch/x86/kernel/dumpstack.c
@@ -115,7 +115,8 @@ void show_opcodes(struct pt_regs *regs, const char *loglvl)
unsigned long prologue = regs->ip - PROLOGUE_SIZE;
if (copy_code(regs, opcodes, prologue, sizeof(opcodes))) {
- printk("%sCode: Bad RIP value.\n", loglvl);
+ printk("%sCode: Unable to access opcode bytes at RIP 0x%lx.\n",
+ loglvl, prologue);
} else {
printk("%sCode: %" __stringify(PROLOGUE_SIZE) "ph <%02x> %"
__stringify(EPILOGUE_SIZE) "ph\n", loglvl, opcodes,
RE: How to use an ACPI declared GPIO in a userspace ...
Hi Andy,
with my custom SSDT table:
DefinitionBlock ("gpio_button.aml", "SSDT", 5, "ASEMsp", "GPIO_BTN", 1)
{
External (_SB_.GPO1, DeviceObj)
Scope (\_SB.GPO1)
{
Device (BTNS)
{
Name (_HID, "ASEM0005") // _HID: Hardware ID
PRP0001
Name (_UID, Zero) // _UID: Unique ID
Name (_DDN, "DDN - SW Readable Button") // _DDN: DOS
Device Name
Name (_STR, Unicode ("STR - SW Readable Button")) //
_STR: Description String
Name (_CRS, ResourceTemplate () // _CRS: Current
Resource Settings
{
GpioIo (
Shared, // Not shared
PullNone,// No need for
pulls
0, // Debounce
timeout
0, // Drive
strength
IoRestrictionInputOnly, // Only used
as input
"\\_SB.GPO1",// GPIO
controller
0, ResourceConsumer, , ) // Must be 0
{
25,// GPIO
number 25
}
})
}
}
}
I'm able to see the GPIO in:
/sys/bus/platform/devices/ASEM0005:00/firmware_node:
-r--r--r--1 root root 4096 Oct 2 12:10 description
-r--r--r--1 root root 4096 Oct 2 12:10 hid
-r--r--r--1 root root 4096 Oct 2 12:10 modalias
-r--r--r--1 root root 4096 Oct 2 12:10 path
lrwxrwxrwx1 root root 0 Oct 2 12:10 physical_node ->
../../../../platform/INT3452:01/ASEM0005:00
drwxr-xr-x2 root root 0 Oct 2 12:10 power
lrwxrwxrwx1 root root 0 Oct 2 12:10 subsystem ->
../../../../../bus/acpi
-rw-r--r--1 root root 4096 Oct 2 12:10 uevent
-r--r--r--1 root root 4096 Oct 2 12:10 uid
and so I can see some useful info:
# cat description
STR - SW Readable Button
# cat hid
ASEM0005
# cat modalias
acpi:ASEM0005:
bm-x86-64:/sys/bus/platform/devices/ASEM0005:00/firmware_node# cat path
\_SB_.GPO1.BTNS
So, from userspace, I can discover the GPIO controller /dev/gpiochip1,
but I don't know how to discover the GPIO number (25 in this case).
Do you have any suggestion about how to discover this GPIO number?
Thanks!
> --
> With Best Regards,
> Andy Shevchenko
Best regards,
Flavio
Re: [PATCH v3 00/21] spi: dw: Add full Baikal-T1 SPI Controllers support
On Fri, Oct 02, 2020 at 01:28:08AM +0300, Serge Semin wrote: > Originally I intended to merge a dedicated Baikal-T1 System Boot SPI > Controller driver into the kernel and leave the DW APB SSI driver > untouched. But after a long discussion (see the link at the bottom of the > letter) Mark and Andy persuaded me to integrate what we developed there > into the DW APB SSI core driver to be useful for another controllers, > which may have got the same peculiarities/problems as ours: > - No IRQ. > - No DMA. > - No GPIO CS, so a native CS is utilized. > - small Tx/Rx FIFO depth. > - Automatic CS assertion/de-assertion. > - Slow system bus. > All of them have been fixed in the framework of this patchset in some > extent at least for the SPI memory operations. As I expected it wasn't > that easy and the integration took that many patches as you can see from > the subject. Though some of them are mere cleanups or weakly related with > the subject fixes, but we just couldn't leave the code as is at some > places since we were working with the DW APB SSI driver anyway. Here is > what we did to fix the original DW APB SSI driver, to make it less messy. Maybe it's time to put your name into MAINTAINERS for this driver? -- With Best Regards, Andy Shevchenko
Re: [Linux-kernel-mentees][PATCH v2] bluetooth: hci_h5: close serdev device and free hu in h5_close
Hi,
On 10/1/20 9:43 PM, Anant Thazhemadam wrote:
When h5_close() gets called, the memory allocated for the hu gets
freed only if hu->serdev doesn't exist. This leads to a memory leak.
So when h5_close() is requested, close the serdev device instance and
free the memory allocated to the hu entirely instead.
Fixes: ce945552fde4 ("Bluetooth: hci_h5: Add support for serdev enumerated
devices")
Reported-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
Tested-by: syzbot+6ce141c55b2f7aafd...@syzkaller.appspotmail.com
Signed-off-by: Anant Thazhemadam
So 2 comments to this:
1. You claim this change fixes a memory-leak, but in the serdev case
the memory is allocated in h5_serdev_probe() like this:
h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);
So its lifetime is tied to the lifetime of the driver being bound
to the serdev and it is automatically freed when the driver gets
unbound. If you had looked at where the h5 struct gets allocated
in h5_close()-s counterpart, h5_open() then you could have seen
this there:
if (hu->serdev) {
h5 = serdev_device_get_drvdata(hu->serdev);
} else {
h5 = kzalloc(sizeof(*h5), GFP_KERNEL);
if (!h5)
return -ENOMEM;
}
So it is very clear here, that the kzalloc only happens in the
if (!hu->serdev) which clearly makes the kfree() have the same
condition the right thing todo. In the hu->serdev the data which
was allocated by h5_serdev_probe() is used instead and no alloc
happens inside h5_open()
In general when looking at resource teardown you should also look
at where they are allocated in the mirror function
and the teardown should mirror the alloc code.
So the main change of your commit is wrong:
NACK.
2. You are making multiple changes in a single commit here, I count at
least 3. When ever you are making multiple changes like this, you should really
split the commit up in 1 commit per change and explain in each commit
message why that change is being made (why it is necessary). Writing
commit messages like this also leads to you double-checking your own
work by carefully considering why you are making the changes.
So about the 3 different changes:
2a) Make the kfree(h5) call unconditionally, which as mentioned above
is wrong.
2b) Introduce a call to kfree_skb(h5->rx_skb); which is not mentioned in
the commit message at all. This looks like it would actually be a sensible
change, at least in the "if (!hu->serdev)" case. When using the serdev
interface then just freeing it will leave a dangling pointer, so it
would be better (for both the hu->serdev and the !hu->serdev cases)
to call h5_reset_rx() on close instead which does:
if (h5->rx_skb) {
kfree_skb(h5->rx_skb);
h5->rx_skb = NULL;
}
2c) Introduce a call to serdev_device_close(), without really explaining
why in the commit message. Again if you would have looked at the mirror
h5_close() function then you see no serdev_device_open() there.
Actually serdev_device_open() is not called anywhere in the hci_h5.c code.
Digging a little deeper (using grep) shows that hci_uart_register_device()
calls serdev_device_open() and hci_uart_register_device() gets called from:
h5_serdev_probe(), likewise hci_uart_unregister_device() calls
serdev_device_close() for us and that gets called from h5_serdev_remove(),
so there is no need to call serdev_device_close() from h5_close() and doing
so will actually break things, because then the serdev will be left closed
on a subsequent h5_open() call.
Regards,
Hans
---
Changes in v2:
* Fixed the Fixes tag
drivers/bluetooth/hci_h5.c | 8 ++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index e41854e0d79a..3d1585add572 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -248,8 +248,12 @@ static int h5_close(struct hci_uart *hu)
if (h5->vnd && h5->vnd->close)
h5->vnd->close(h5);
- if (!hu->serdev)
- kfree(h5);
+ if (hu->serdev)
+ serdev_device_close(hu->serdev);
+
+ kfree_skb(h5->rx_skb);
+ kfree(h5);
+ h5 = NULL;
return 0;
}
Re: [PATCH v3 03/21] spi: dw: Detach SPI device specific CR0 config method
On Fri, Oct 02, 2020 at 01:28:11AM +0300, Serge Semin wrote: > Indeed there is no point in detecting the SPI peripheral device parameters > and initializing the CR0 register fields each time an SPI transfer is > executed. Instead let's define a dedicated CR0 chip-data member, which > will be initialized in accordance with the SPI device settings at the > moment of setting it up. > > By doing so we'll finally make the SPI device chip_data serving as it's > supposed to - to preserve the SPI device specific DW SPI configuration. > See spi-fsl-dspi.c, spi-pl022.c, spi-pxa2xx.c drivers for example of the > way the chip data is utilized. > +static void dw_spi_update_cr0(struct dw_spi *dws, struct spi_device *spi, > + struct spi_transfer *transfer) Yep, why not to place this in previous patch exactly here? > + /* > + * Update CR0 data each time the setup callback is invoked since > + * the device parameters could have been changed, for instance, by > + * the MMC SPI driver or something else. > + */ > + chip->cr0 = dw_spi_get_cr0(dws, spi); I would rather name it prepare or alike. 'get' assumes getting value or something like that. -- With Best Regards, Andy Shevchenko
RE: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted Firmware Service call
Hi Sudeep,
>-Original Message-
>From: Michal Simek
>Sent: Friday, October 2, 2020 4:23 PM
>To: Sudeep Holla ; Zulkifli, Muhammad Husaini
>
>Cc: Hunter, Adrian ; michal.si...@xilinx.com;
>ulf.hans...@linaro.org; linux-...@vger.kernel.org; linux-arm-
>ker...@lists.infradead.org; linux-kernel@vger.kernel.org; Raja Subramanian,
>Lakshmi Bai ; a...@arndb.de; Wan
>Mohamad, Wan Ahmad Zainie
>Subject: Re: [PATCH v2 2/3] firmware: Keem Bay: Add support for Arm Trusted
>Firmware Service call
>
>Hi Sudeep,
>
>On 01. 10. 20 17:35, Sudeep Holla wrote:
>> On Thu, Oct 01, 2020 at 10:21:48PM +0800,
>muhammad.husaini.zulki...@intel.com wrote:
>>> From: Muhammad Husaini Zulkifli
>>>
>>> Add generic firmware driver for Keem Bay SOC to support Arm Trusted
>>> Firmware Services call.
>>>
>>> Signed-off-by: Muhammad Husaini Zulkifli
>>>
>>> ---
>>> drivers/firmware/Kconfig | 1 +
>>> drivers/firmware/Makefile | 1 +
>>> drivers/firmware/intel/Kconfig | 14 +++
>>> drivers/firmware/intel/Makefile| 4 +
>>> drivers/firmware/intel/keembay_smc.c | 119 +
>>> include/linux/firmware/intel/keembay_smc.h | 27 +
>>> 6 files changed, 166 insertions(+)
>>> create mode 100644 drivers/firmware/intel/Kconfig create mode
>>> 100644 drivers/firmware/intel/Makefile create mode 100644
>>> drivers/firmware/intel/keembay_smc.c
>>> create mode 100644 include/linux/firmware/intel/keembay_smc.h
>>>
>>> diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>>> index fbd785dd0513..41de77d2720e 100644
>>> --- a/drivers/firmware/Kconfig
>>> +++ b/drivers/firmware/Kconfig
>>> @@ -305,5 +305,6 @@ source "drivers/firmware/psci/Kconfig"
>>> source "drivers/firmware/smccc/Kconfig"
>>> source "drivers/firmware/tegra/Kconfig"
>>> source "drivers/firmware/xilinx/Kconfig"
>>> +source "drivers/firmware/intel/Kconfig"
>>>
>>> endmenu
>>> diff --git a/drivers/firmware/Makefile b/drivers/firmware/Makefile
>>> index 99510be9f5ed..00f295ab9860 100644
>>> --- a/drivers/firmware/Makefile
>>> +++ b/drivers/firmware/Makefile
>>> @@ -33,3 +33,4 @@ obj-y += psci/
>>> obj-y += smccc/
>>> obj-y += tegra/
>>> obj-y += xilinx/
>>> +obj-y += intel/
>>> diff --git a/drivers/firmware/intel/Kconfig
>>> b/drivers/firmware/intel/Kconfig new file mode 100644 index
>>> ..b2b7a4e5410b
>>> --- /dev/null
>>> +++ b/drivers/firmware/intel/Kconfig
>>> @@ -0,0 +1,14 @@
>>> +# SPDX-License-Identifier: GPL-2.0-only menu "Intel Firmware
>>> +Drivers"
>>> +
>>> +config KEEMBAY_FIRMWARE
>>> + bool "Enable Keem Bay firmware interface support"
>>> + depends on HAVE_ARM_SMCCC
>>
>> What is the version of SMCCC implemented ?
Our current Arm Trusted Firmware framework supports v1.1.
Does it mean I should use HAVE_ARM_SMCCC_DISCOVERY?
Not really clear about this. As for HAVE_ARM_SMCCC will include
support for the SMC and HVC.
>> If SMCCC v1.1+, use HAVE_ARM_SMCCC_DISCOVERY
>>
>>> + default n
>>> + help
>>> + Firmware interface driver is used by device drivers
>>> + to communicate with the arm-trusted-firmware
>>> + for platform management services.
>>> + If in doubt, say "N".
>>> +
>>> +endmenu
>>> diff --git a/drivers/firmware/intel/Makefile
>>> b/drivers/firmware/intel/Makefile new file mode 100644 index
>>> ..e6d2e1ea69a7
>>> --- /dev/null
>>> +++ b/drivers/firmware/intel/Makefile
>>> @@ -0,0 +1,4 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>> +# Makefile for Intel firmwares
>>> +
>>> +obj-$(CONFIG_KEEMBAY_FIRMWARE) = keembay_smc.o
>>> diff --git a/drivers/firmware/intel/keembay_smc.c
>>> b/drivers/firmware/intel/keembay_smc.c
>>> new file mode 100644
>>> index ..24013cd1f5da
>>> --- /dev/null
>>> +++ b/drivers/firmware/intel/keembay_smc.c
>>> @@ -0,0 +1,119 @@
>>> +// SPDX-License-Identifier: GPL-2.0
>>> +/*
>>> + * Copyright (C) 2020-2021, Intel Corporation */
>>> +
>>> +#include
>>> +#include
>>> +#include
>>> +#include
>>> +
>>> +#include
>>> +
>>> +static noinline int do_fw_call_fail(u64 arg0, u64 arg1) {
>>> + return -ENODEV;
>>> +}
>>> +
>>> +/**
>>> + * Simple wrapper functions to be able to use a function pointer
>>> + * Invoke do_fw_call_smc or others in future, depending on the
>>> +configuration */ static int (*do_fw_call)(u64, u64) =
>>> +do_fw_call_fail;
>>> +
>>> +/**
>>> + * do_fw_call_smc() - Call system-level platform management layer (SMC)
>>> + * @arg0: Argument 0 to SMC call
>>> + * @arg1: Argument 1 to SMC call
>>> + *
>>> + * Invoke platform management function via SMC call.
>>> + *
>>> + * Return: Returns status, either success or error */ static
>>> +noinline int do_fw_call_smc(u64 arg0, u64 arg1) {
>>> + struct arm_smccc_res res;
>>> +
>>> + arm_smccc_smc(arg0, arg1, 0, 0, 0, 0, 0, 0, );
>>> +
>>> + return
Re: [PATCH 1/3] i2c: imx: Fix reset of I2SR_IAL flag
Hello Christian, On Fri, Oct 02, 2020 at 10:01:30AM +0200, Christian Eggers wrote: > On Friday, 25 September 2020, 10:11:01 CEST, Uwe Kleine-König wrote: > > On Thu, Sep 17, 2020 at 04:13:50PM +0200, Christian Eggers wrote: > > IMHO the intention here (and also what happens on i.MX) is that exactly > > the AL interrupt pending bit should be cleared and the IF irq is > > supposed to be untouched. > > Yes. > > > Given there are only two irq flags in the I2SR register (which is called > > IBSR on Vybrid) ... > > Vybrid: > === > +---+-+--+-+--+-+-+--+--+ > | BIT | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | > +---+-+--+-+--+-+-+--+--+ > | READ | TCF | IAAS | IBB | IBAL | 0 | SRW | IBIF | RXAK | > +---+-+--+-+--+-+-+--+--+ > | WRITE | - | - | - | W1C | - | - | W1C | - | > +---+-+--+-+-^^^--+-+-+-^^^--+--+ > > i.MX: > === > +---+-+--+-+--+-+-+--+--+ > | BIT | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | > +---+-+--+-+--+-+-+--+--+ > | READ | ICF | IAAS | IBB | IAL | 0 | SRW | IIF | RXAK | > +---+-+--+-+--+-+-+--+--+ > | WRITE | - | - | - | W0C | - | - | W0C | - | > +---+-+--+-+-^^^--+-+-+-^^^--+--+ > > > > ... the status quo (i.e. without your patch) is: > > > > On i.MX IAL is cleared > > Yes > > > On Vybrid IIF (which is called IBIF there) is cleared. > If IBIF is set, then it's cleared (probably by accident). > But in the "if (temp & I2SR_IAL)" condition, I focus on > the IBAL flag, not IBIF. > > > With your patch we get: > > > > On i.MX IAL is cleared > > Yes > > > On Vybrid both IIF (aka IBIF) and IAL (aka IBAL) are cleared. > Agree. IBAL is cleared by intention, IBIF by accident (if set). > Do you see any problem if IBIF is also cleared? Yes. If there is a real problem I'm not sure, but it's enough of an issue that there are possible side effects on Vybrid. I refuse to think about real problems given that it is so easy to make it right. > > To get it right for both SoC types you have to do (e.g.): > > > > temp = ~i2c_imx->hwdata->i2sr_clr_opcode ^ I2SR_IAL; > Sorry, even if this is correct, it looks hard to understand for me. Maybe a comment would be appropriate, something like: /* * i2sr_clr_opcode is the value to clear all interrupts. Here we * want to clear no irq but I2SR_IAL, so we write * ~i2sr_clr_opcode with just the I2SR_IAL bit toggled. */ Maybe put this comment together with the code in a new function to have it only once. > > (and in i2c_imx_isr() the same using I2SR_IIF instead of I2SR_IAL > > because there currently IAL might be cleared by mistake on Vybrid). > > > > I considered creating a patch, but as I don't have a Vybrid on my desk > > and on i.MX there is no change, I let you do this. > I also don't own a Vybrid system. I consider my patch correct in terms of > clearing the IBAL flag (which was wrong before). Additional work may be > useful for NOT clearing the other status flag. I also would like to keep > this task for somebody who owns a Vybrid system. But the other patches > in this series fixes some more important problems, so maybe you could > give your acknowledge anyhow. No, please don't replace one bug found by another (now) known bug. Still more given that the newly introduced bug is much harder to trigger and debug. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
Re: [PATCH v3 02/21] spi: dw: Add DWC SSI capability
On Fri, Oct 02, 2020 at 01:28:10AM +0300, Serge Semin wrote: > Currently DWC SSI core is supported by means of setting up the > core-specific update_cr0() callback. It isn't suitable for multiple > reasons. First of all having exported several methods doing the same thing > but for different chips makes the code harder to maintain. Secondly the > spi-dw-core driver exports the methods, then the spi-dw-mmio driver sets > the private data callback with one of them so to be called by the core > driver again. That makes the code logic too complicated. Thirdly using > callbacks for just updating the CR0 register is problematic, since in case > if the register needed to be updated from different parts of the code, > we'd have to create another callback (for instance the SPI device-specific > parameters don't need to be calculated each time the SPI transfer is > submitted, so it's better to pre-calculate the CR0 data at the SPI-device > setup stage). > > So keeping all the above in mind let's discard the update_cr0() callbacks, > define a generic and static dw_spi_update_cr0() method and create the > DW_SPI_CAP_DWC_SSI capability, which when enabled would activate the > alternative CR0 register layout. > > While at it add the comments to the code path of the normal DW APB SSI > controller setup to make the dw_spi_update_cr0() method looking coherent. What the point to increase indentation level and produce additional churn? Can't you simply leave functions, unexport them, and call in one conditional of whatever new function is called? I have an impression that split of the series is done in a way that first patches in the series are not optimized to what is done in the last patches in the series. -- With Best Regards, Andy Shevchenko
Re: linux-next: manual merge of the akpm tree with the drm-intel tree
Quoting Daniel Vetter (2020-10-01 18:13:26)
> On Thu, Oct 1, 2020 at 5:08 PM Jani Nikula
> wrote:
> >
> > On Thu, 01 Oct 2020, Daniel Vetter wrote:
> > > On Thu, Oct 1, 2020 at 3:53 PM Christoph Hellwig wrote:
> > >>
> > >> On Thu, Oct 01, 2020 at 08:39:17PM +1000, Stephen Rothwell wrote:
> > >> > Hi all,
> > >> >
> > >> > Today's linux-next merge of the akpm tree got a conflict in:
> > >> >
> > >> > drivers/gpu/drm/i915/gem/i915_gem_pages.c
> > >> >
> > >> > between commit:
> > >> >
> > >> > 4caf017ee937 ("drm/i915/gem: Avoid implicit vmap for highmem on
> > >> > x86-32")
> > >> > ba2ebf605d5f ("drm/i915/gem: Prevent using pgprot_writecombine() if
> > >> > PAT is not supported")
> > >
> > > Uh these patches shouldn't be in linux-next because they're for 5.11,
> > > not the 5.10 merge window that will open soon. Joonas?
> >
> > I don't know anything else, but both are tagged Cc: stable.
>
> Uh right I got confused, they're on -fixes now. Well -next-fixes,
> which seems like the wrong one for a cc: stable, I guess this should
> go into 5.9 even. Apologies for my confusion.
Yep, they happen to be Fixes: (Cc: stable even) so I asked Rodrigo to
pull them to drm-intel-next-fixes.
If they weren't Fixes: then indeed they would only have been queued for
5.11.
With regards to 5.9, due to the hiccup of doing the split PR, all the
-fixes for GT area were in limbo until -rc7. We didn't feel comfortable
including all the new commits this late in the cycle, so we agreed stable
porting those will be more appropriate.
Regards, Joonas
> -Daniel
>
> >
> > BR,
> > Jani.
> >
> > >
> > >> > from the drm-intel tree and patch:
> > >> >
> > >> > "drm/i915: use vmap in i915_gem_object_map"
> > >> >
> > >> > from the akpm tree.
> > >> >
> > >> > I fixed it up (I just dropped the changes in the former commits) and
> > >>
> > >> Sigh. The solution is a bit more complicated, but I just redid my
> > >> patches to not depend on the above ones. I can revert back to the old
> > >> version, though. Andrew, let me know what works for you.
> > >
> > > Imo ignore, rebasing onto linux-next without those intel patches was
> > > the right thing for the 5.10 merge window.
> > > -Daniel
> >
> > --
> > Jani Nikula, Intel Open Source Graphics Center
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
[PATCH][next] media: zoran: fix spelling mistake and make error message more meaningful
From: Colin Ian King
There is a spelling mistake in a pci_err error message. Fix this and
make the error message a little more meaningful.
Signed-off-by: Colin Ian King
---
drivers/staging/media/zoran/zoran_driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/media/zoran/zoran_driver.c
b/drivers/staging/media/zoran/zoran_driver.c
index 808196ea5b81..d9f8b21edf6a 100644
--- a/drivers/staging/media/zoran/zoran_driver.c
+++ b/drivers/staging/media/zoran/zoran_driver.c
@@ -666,7 +666,7 @@ static int zoran_g_selection(struct file *file, void *__fh,
struct v4l2_selectio
if (sel->type != V4L2_BUF_TYPE_VIDEO_OUTPUT &&
sel->type != V4L2_BUF_TYPE_VIDEO_CAPTURE) {
- pci_err(zr->pci_dev, "%s invalid combinaison\n", __func__);
+ pci_err(zr->pci_dev, "%s invalid selection type combination\n",
__func__);
return -EINVAL;
}
--
2.27.0
Re: [PATCH v2 RESEND 2/9] spi: spi-s3s64xx: Add S3C64XX_SPI_QUIRK_CS_AUTO for Exynos3250
It was <2020-10-01 czw 21:04>, when Krzysztof Kozlowski wrote:
> On Thu, Oct 01, 2020 at 05:21:41PM +0200, Łukasz Stelmach wrote:
>> Fix issues with DMA transfers bigger than 512 bytes on Exynos3250. Without
>> the patches such transfers fail.
>>
>> The vendor kernel for ARTIK5 handles CS in a simmilar way.
>>
>> Signed-off-by: Łukasz Stelmach
>> Reviewed-by: Krzysztof Kozlowski
>> ---
>> drivers/spi/spi-s3c64xx.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/spi/spi-s3c64xx.c b/drivers/spi/spi-s3c64xx.c
>> index 26c7cb79cd78..22bf8c75580a 100644
>> --- a/drivers/spi/spi-s3c64xx.c
>> +++ b/drivers/spi/spi-s3c64xx.c
>> @@ -1379,6 +1379,7 @@ static struct s3c64xx_spi_port_config
>> exynos4_spi_port_config = {
>> .tx_st_done = 25,
>> .high_speed = true,
>> .clk_from_cmu = true,
>> +.quirks = S3C64XX_SPI_QUIRK_CS_AUTO,
>
> I spotted now: you have here double space after '='.
>
Fixed, thanks. v3 is coming.
--
Łukasz Stelmach
Samsung R Institute Poland
Samsung Electronics
signature.asc
Description: PGP signature
Re: [PATCH v3] RISC-V: Remove any memblock representing unusable memory area
On Fri, Oct 2, 2020 at 12:36 AM Atish Patra wrote:
>
> RISC-V limits the physical memory size by -PAGE_OFFSET. Any memory beyond
> that size from DRAM start is unusable. Just remove any memblock pointing
> to those memory region without worrying about computing the maximum size.
>
> Signed-off-by: Atish Patra
> Reviewed-by: Mike Rapoport
>
> ---
> Changes from v2->v3
> Updated comment as per Mike's suggestion.
> ---
> arch/riscv/mm/init.c | 15 +--
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index ca03762a3733..564e0be677b7 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -146,8 +146,6 @@ static phys_addr_t dtb_early_pa __initdata;
> void __init setup_bootmem(void)
> {
> struct memblock_region *reg;
> - phys_addr_t mem_size = 0;
> - phys_addr_t total_mem = 0;
> phys_addr_t mem_start, end = 0;
> phys_addr_t vmlinux_end = __pa_symbol(&_end);
> phys_addr_t vmlinux_start = __pa_symbol(&_start);
> @@ -155,21 +153,18 @@ void __init setup_bootmem(void)
> /* Find the memory region containing the kernel */
> for_each_memblock(memory, reg) {
> end = reg->base + reg->size;
> - if (!total_mem)
> + if (!mem_start)
> mem_start = reg->base;
> if (reg->base <= vmlinux_start && vmlinux_end <= end)
> BUG_ON(reg->size == 0);
> - total_mem = total_mem + reg->size;
> }
>
> /*
> -* Remove memblock from the end of usable area to the
> -* end of region
> +* The maximal physical memory size is -PAGE_OFFSET.
> +* Make sure that any memory beyond mem_start + (-PAGE_OFFSET) is
> removed
> +* as it is unusable by kernel.
> */
> - mem_size = min(total_mem, (phys_addr_t)-PAGE_OFFSET);
> - if (mem_start + mem_size < end)
> - memblock_remove(mem_start + mem_size,
> - end - mem_start - mem_size);
> + memblock_enforce_memory_limit(mem_start - PAGE_OFFSET);
>
> /* Reserve from the start of the kernel to the end of the kernel */
> memblock_reserve(vmlinux_start, vmlinux_end - vmlinux_start);
> --
> 2.25.1
>
Looks good to me.
Reviewed-by: Anup Patel
Regards,
Anup
Re: [PATCH] RISC-V: Make sure memblock reserves the memory containing DT
On Fri, Oct 2, 2020 at 12:35 AM Atish Patra wrote:
>
> Currently, the memory containing DT is not reserved. Thus, that region
> of memory can be reallocated or reused for other purposes. This may result
> in corrupted DT for nommu virt board in Qemu. We may not face any issue
> in kendryte as DT is embedded in the kernel image for that.
>
> Fixes: 6bd33e1ece52 ("riscv: add nommu support")
> Signed-off-by: Atish Patra
>
> ---
> @Palmer This patch is applicable for v5.9 & before. This fix is already
> part of the "RISC-V: Move DT mapping outof fixmap" patch from UEFI support
> series. That's why, this patch doesn't need to go into for-next.
> ---
> arch/riscv/mm/init.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c
> index 564e0be677b7..cabf1697e748 100644
> --- a/arch/riscv/mm/init.c
> +++ b/arch/riscv/mm/init.c
> @@ -510,6 +510,7 @@ asmlinkage void __init setup_vm(uintptr_t dtb_pa)
> #else
> dtb_early_va = (void *)dtb_pa;
> #endif
> + dtb_early_pa = dtb_pa;
> }
>
> static inline void setup_vm_final(void)
> --
> 2.25.1
>
Looks good to me.
Reviewed-by: Anup Patel
Regards,
Anup
Re: [PATCH v6 3/7] dt-bindings: mfd: Fix schema warnings for pwm-leds
On Fri, 2 Oct 2020 at 12:07, Alexander Dahl wrote: > > Hello Krzysztof, > > Am Freitag, 2. Oktober 2020, 11:21:10 CEST schrieb Krzysztof Kozlowski: > > On Thu, 1 Oct 2020 at 01:51, Alexander Dahl wrote: > > > The node names for devices using the pwm-leds driver follow a certain > > > naming scheme (now). > > > > What warning? Please post them here and in every DTS patch. > > ack > > > Your schema does not enforce pwmleds node naming (the main node, not > > children), or at least I could not see it. You change multiple files > > in your patchset so are you sure that all these are justified by > > warnings pointed out by schema? > > The rename was suggested by Rob [1], and I think you're right, those names are > not (yet) enforced by schema? So at least the commit message is misleading > for now. I'll have to see if I rather reword or update the schema again. If schema does not enforce it, then maybe just mention that you align the children names with schema and pwmleds node to device tree specification guidelines (node name should be generic, reflecting function of the device). However in such case changing only pwmleds node name in DTS (without changing children) would not be justified, IMHO. Best regards, Krzysztof
Re: [PATCH v6 3/7] dt-bindings: mfd: Fix schema warnings for pwm-leds
Hello Krzysztof, Am Freitag, 2. Oktober 2020, 11:21:10 CEST schrieb Krzysztof Kozlowski: > On Thu, 1 Oct 2020 at 01:51, Alexander Dahl wrote: > > The node names for devices using the pwm-leds driver follow a certain > > naming scheme (now). > > What warning? Please post them here and in every DTS patch. ack > Your schema does not enforce pwmleds node naming (the main node, not > children), or at least I could not see it. You change multiple files > in your patchset so are you sure that all these are justified by > warnings pointed out by schema? The rename was suggested by Rob [1], and I think you're right, those names are not (yet) enforced by schema? So at least the commit message is misleading for now. I'll have to see if I rather reword or update the schema again. Greets Alex [1] https://lore.kernel.org/linux-leds/20200922155747.GA2734659@bogus/
Re: [PATCH bpf-next v2 2/4] selftests: bpf: Add helper to compare socket cookies
On Thu, 1 Oct 2020 at 18:11, Alexei Starovoitov wrote: > > > > I think this might be the same problem I fixed for libbpf with [0]. > > Turns out, GCC explicitly calls out (somewhere in their docs) that > > uninitialized variable warnings work only when compiled in optimized > > mode, because some internal data structures used to detect this are > > only maintained in optimized mode build. > > > > Laurenz, can you try compiling your example with -O2? > > All of my experiments I did with -O2. If anybody wants to play with this more: https://godbolt.org/z/77P6P9 Seems like red hat GCC has some special sauce that fixes this behaviour? -- Lorenz Bauer | Systems Engineer 6th Floor, County Hall/The Riverside Building, SE1 7PB, UK www.cloudflare.com
Re: [PATCH v6 5/7] ARM: dts: Fix schema warnings for pwm-leds
On Fri, 2 Oct 2020 at 11:28, Alexander Dahl wrote: > > Hello Krzysztof, > > Am Freitag, 2. Oktober 2020, 11:12:50 CEST schrieb Krzysztof Kozlowski: > > On Thu, 1 Oct 2020 at 01:53, Alexander Dahl wrote: > > > The node names for devices using the pwm-leds driver follow a certain > > > naming scheme (now). > > > > > > Signed-off-by: Alexander Dahl > > > --- > > > > > > Notes: > > > v6: > > > * added this patch to series > > > > > > arch/arm/boot/dts/at91-kizbox.dts | 10 +++ > > > arch/arm/boot/dts/at91-kizbox2-common.dtsi| 8 +++--- > > > arch/arm/boot/dts/at91-kizbox3-hs.dts | 16 ++-- > > > arch/arm/boot/dts/at91-kizbox3_common.dtsi| 10 +++ > > > arch/arm/boot/dts/at91-kizboxmini-common.dtsi | 8 +++--- > > > arch/arm/boot/dts/at91sam9m10g45ek.dts| 10 +++ > > > arch/arm/boot/dts/at91sam9rlek.dts| 10 +++ > > > .../boot/dts/berlin2cd-google-chromecast.dts | 6 ++--- > > > arch/arm/boot/dts/exynos5422-odroidhc1.dts| 4 +-- > > > arch/arm/boot/dts/exynos5422-odroidxu4.dts| 4 +-- > > > > Somehow you did not CC the maintainers... please use > > scripts/get_maintainers.pl to obtain list of addresses. > > Well, that will be a huge list of Cc then. What is the policy? Everybody > gets the whole series or different list of receivers per patch? With git send email and get_maintainers.pl you can simplify it: 1. Send cover letter to everyone (could be skipped and instead send to mailing lists and then link included under --- in each patch). 2. Send automatically each patch only to maintainers, with adjusted in-reply-to to the cover letter already sent. Something like: git send-email --cc-cmd "scripts/get_maintainer.pl --no-git --no-roles --no-rolestats". Probably 1+2 could be even one command if you put manually lists as Cc in the cover letter. Best regards, Krzysztof
Re: [PATCH] usb: cdns3: platform_get_irq_byname_optional instead platform_get_irq_byname
Pawel, On 02/10/2020 12:08, Pawel Laszczak wrote: Roger, On 30/09/2020 09:57, Pawel Laszczak wrote: To avoid duplicate error information patch replaces platform_get_irq_byname into platform_get_irq_byname_optional. What is duplicate error information? The function platform_get_irq_byname print: " dev_err(>dev, "IRQ %s not found\n", name);" if error occurred. In core.c we have the another error message below invoking this function. e.g if (cdns->dev_irq < 0) dev_err(dev, "couldn't get peripheral irq\n"); So, it's looks like one dev_err is redundant. If we want all 3 IRQs to be valid irrespective of dr_mode then we should use platform_get_irq_byname() and error out in probe if (ret < 0 && ret != -EPROBE_DEFER). We can get rid of the irq check and duplicate error message in other places. cheers, -roger A change was suggested during reviewing CDNSP driver by Chunfeng Yun. Signed-off-by: Pawel Laszczak --- drivers/usb/cdns3/core.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index a0f73d4711ae..a3f6dc44cf3a 100644 --- a/drivers/usb/cdns3/core.c +++ b/drivers/usb/cdns3/core.c @@ -465,7 +465,7 @@ static int cdns3_probe(struct platform_device *pdev) cdns->xhci_res[1] = *res; - cdns->dev_irq = platform_get_irq_byname(pdev, "peripheral"); + cdns->dev_irq = platform_get_irq_byname_optional(pdev, "peripheral"); As per DT binding document, these are mandatory properties I think that name platform_get_irq_byname_optional is little confusing. Function descriptions show that both are almost identical: /** * platform_get_irq_byname_optional - get an optional IRQ for a device by name * @dev: platform device * @name: IRQ name * * Get an optional IRQ by name like platform_get_irq_byname(). Except that it * does not print an error message if an IRQ can not be obtained. * * Return: non-zero IRQ number on success, negative error number on failure. */ - interrupts: Interrupts used by cdns3 controller: "host" - interrupt used by XHCI driver. "peripheral" - interrupt used by device driver "otg" - interrupt used by DRD/OTG part of driver for dr_mode == "otg" -> all 3 are mandatory. for dr_mode == "host" -> "otg" and "peripheral" IRQs are not required. for dr_mode == "periphearal" -> "otg" and "host" IRQs are not required. if (cdns->dev_irq == -EPROBE_DEFER) return cdns->dev_irq; @@ -477,7 +477,7 @@ static int cdns3_probe(struct platform_device *pdev) return PTR_ERR(regs); cdns->dev_regs = regs; - cdns->otg_irq = platform_get_irq_byname(pdev, "otg"); + cdns->otg_irq = platform_get_irq_byname_optional(pdev, "otg"); if (cdns->otg_irq == -EPROBE_DEFER) return cdns->otg_irq; Regards, Pawel -- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
FYI: PoC: Running 100000 processes in 5.3.18 (SLES15 SP2)
Hi! Just in case someone is interested: As a Proof-of-Concept I started 100 thousand processes on a big machine (72 cores). It worked! However starting those too more than 30 minutes, and top needs more than 30 minutes to refresh ist display. Still, interactive input via SSH works nice, but any file-system access seems quite slow (my test processes just use CPU; the do no t do any I/O). Kernel messages while the processes were created: kernel: [65648.247688] perf: interrupt took too long (2516 > 2500), lowering kernel.perf_event_max_sample_rate to 79250 kernel: [65997.263218] perf: interrupt took too long (3146 > 3145), lowering kernel.perf_event_max_sample_rate to 63500 kernel: [66790.221057] perf: interrupt took too long (3938 > 3932), lowering kernel.perf_event_max_sample_rate to 50750 kernel: [69884.371426] perf: interrupt took too long (4925 > 4922), lowering kernel.perf_event_max_sample_rate to 40500 Last top output (more than 30 late): top - 11:16:19 up 19:19, 3 users, load average: 64164.56, 62997.24, 55597.09 Tasks: 101432 total, 60249 running, 41183 sleeping, 0 stopped, 0 zombie %Cpu(s): 98.0 us, 2.0 sy, 0.0 ni, 0.0 id, 0.0 wa, 0.0 hi, 0.0 si, 0.0 st MiB Mem : 772127.6+total, 755924.2+free, 14253.01+used, 1950.363 buff/cache MiB Swap: 773120.0+total, 772958.1+free, 161.816 used. 754248.8+avail Mem ... That's a load, isn't it? ;-) # cat /proc/uptime 72084.21 9356423.41 # cat /proc/loadavg 64188.31 64188.81 63636.08 64228/102328 134935 Regards, Ulrich
Re: lockdep null-ptr-deref
On Wed, Sep 30, 2020 at 11:49:37AM +0200, Peter Zijlstra wrote:
> On Wed, Sep 30, 2020 at 11:16:11AM +0200, Peter Zijlstra wrote:
> > On Wed, Sep 30, 2020 at 07:08:23AM +0800, Boqun Feng wrote:
> > > I think there are two problems here:
> > >
> > > 1) the "(null)" means we don't have the "usage_str" for a usage bit,
> > > which I think is the LOCK_USED_READ bit introduced by Peter at
> > > 23870f122768 ('locking/lockdep: Fix "USED" <- "IN-NMI" inversions').
> > >
> > > 2) the next null-ptr-deref, and I think this is also caused by
> > > LOCK_USED_READ bit, because in the loop inside
> > > print_lock_class_header(), we iterate from 0 to LOCK_USAGE_STATES (which
> > > is 4*2 + 3), however the class->usage_traces[] only has
> > > XXX_LOCK_USAGE_STATES (which is 4*2 + 1) elements, so if we have
> > > LOCK_USED_READ bit set in ->usage_mask, we will try to access an element
> > > out of the ->usage_traces[] array.
> > >
> > > Probably the following helps? And another possible fix is to enlarge the
> > > ->usage_trace[] array and record the call trace of LOCK_READ_USED.
> >
> > Urgh.. yeah, I wanted to avoid saving that trace; it's pretty useless :/
> > The existing USED trace is already mostly pointless, the consistent
> > thing would be to remove both but that might be too radical.
> >
> > But you're right in that I made a right mess of it. Not sure what's
> > best here.
> >
> > Let me have a play.
>
> How's something like this? It's bigger than I'd like, but I feel the
> result is more consistent/readable.
>
Qian, could you confirm this works for you?
> ---
> diff --git a/include/linux/lockdep_types.h b/include/linux/lockdep_types.h
> index bb35b449f533..a55b1d314ae8 100644
> --- a/include/linux/lockdep_types.h
> +++ b/include/linux/lockdep_types.h
> @@ -35,8 +35,12 @@ enum lockdep_wait_type {
> /*
> * We'd rather not expose kernel/lockdep_states.h this wide, but we do need
> * the total number of states... :-(
> + *
> + * XXX_LOCK_USAGE_STATES is the number of lines in lockdep_states.h, for each
> + * of those we generates 4 states, Additionally we (for now) report on USED.
> */
> -#define XXX_LOCK_USAGE_STATES(1+2*4)
> +#define XXX_LOCK_USAGE_STATES2
> +#define LOCK_TRACE_STATES(XXX_LOCK_USAGE_STATES*4 + 1)
>
> /*
> * NR_LOCKDEP_CACHING_CLASSES ... Number of classes
> @@ -106,7 +110,7 @@ struct lock_class {
>* IRQ/softirq usage tracking bits:
>*/
> unsigned long usage_mask;
> - const struct lock_trace *usage_traces[XXX_LOCK_USAGE_STATES];
> + const struct lock_trace *usage_traces[LOCK_TRACE_STATES];
>
> /*
>* Generation counter, when doing certain classes of graph walking,
> diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> index 454355c033d2..4f98ac8b4575 100644
> --- a/kernel/locking/lockdep.c
> +++ b/kernel/locking/lockdep.c
> @@ -600,6 +600,8 @@ static const char *usage_str[] =
> #include "lockdep_states.h"
> #undef LOCKDEP_STATE
> [LOCK_USED] = "INITIAL USE",
> + [LOCK_USED_READ] = "INITIAL READ USE",
> + /* abused as string storage for verify_lock_unused() */
> [LOCK_USAGE_STATES] = "IN-NMI",
> };
> #endif
> @@ -2231,7 +2233,7 @@ static void print_lock_class_header(struct lock_class
> *class, int depth)
> #endif
> printk(KERN_CONT " {\n");
>
> - for (bit = 0; bit < LOCK_USAGE_STATES; bit++) {
> + for (bit = 0; bit < LOCK_TRACE_STATES; bit++) {
> if (class->usage_mask & (1 << bit)) {
> int len = depth;
>
> @@ -4354,27 +4356,24 @@ static int mark_lock(struct task_struct *curr, struct
> held_lock *this,
> old_mask = hlock_class(this)->usage_mask;
> hlock_class(this)->usage_mask |= new_mask;
>
> - /*
> - * Save one usage_traces[] entry and map both LOCK_USED and
> - * LOCK_USED_READ onto the same entry.
> - */
> - if (new_bit == LOCK_USED || new_bit == LOCK_USED_READ) {
> - if (old_mask & (LOCKF_USED | LOCKF_USED_READ))
> - goto unlock;
> - new_bit = LOCK_USED;
> + if (new_bit < LOCK_TRACE_STATES) {
> + if (!(hlock_class(this)->usage_traces[new_bit] = save_trace()))
> + return 0;
> }
>
> - if (!(hlock_class(this)->usage_traces[new_bit] = save_trace()))
> - return 0;
> -
> switch (new_bit) {
> + case 0 ... LOCK_USED-1:
> + ret = mark_lock_irq(curr, this, new_bit);
> + if (!ret)
> + return 0;
> + break;
> +
> case LOCK_USED:
> debug_atomic_dec(nr_unused_locks);
> break;
> +
> default:
> - ret = mark_lock_irq(curr, this, new_bit);
> - if (!ret)
> - return 0;
> + break;
> }
>
> unlock:
> diff --git a/kernel/locking/lockdep_internals.h
>
Re: [PATCH v4 4/4] mm: convert page kmemcg type to a page memcg flag
On Thu, Oct 01, 2020 at 01:27:13PM -0400, Johannes Weiner wrote: > The activation code is the only path where page migration is not > excluded. Because unlike with page state statistics, we don't really > mind a race when counting an activation event. Thanks for the explanation. I see why the accessor trio is justified. > I do think there is a bug, though: mem_cgroup_move_account() should > use WRITE_ONCE() on page->mem_cgroup. If this were a bug, wouldn't be the proper approach rcu_assign_pointer()/rcu_dereference() pair? Michal signature.asc Description: Digital signature
RE: How to use an ACPI declared GPIO in a userspace ...
Hi Andy,
sorry for the delay!
> > > My SSDT table is:
> >
> > See the difference? I can't help here.
This is the DSDT table related to the GPIO controller of my board:
Device (GPO1)
{
Name (_ADR, Zero) // _ADR: Address
Name (_HID, "INT3452") // _HID: Hardware ID
Name (_CID, "INT3452") // _CID: Compatible ID
Name (_DDN, "General Purpose Input/Output (GPIO) Controller -
Northwest") // _DDN: DOS Device Name
Name (_UID, 0x02) // _UID: Unique ID
Name (RBUF, ResourceTemplate ()
{
Memory32Fixed (ReadWrite,
0x, // Address Base
0x4000, // Address Length
_Y08)
Interrupt (ResourceConsumer, Level, ActiveLow, Shared, ,, )
{
0x000E,
}
})
Method (_CRS, 0, NotSerialized) // _CRS: Current Resource Settings
{
CreateDWordField (RBUF, \_SB.GPO1._Y08._BAS, B0BA) // _BAS:
Base Address
CreateDWordField (RBUF, \_SB.GPO1._Y08._LEN, B0LN) // _LEN:
Length
B0BA = GP1A /* \GP1A */
B0LN = GP1L /* \GP1L */
Return (RBUF) /* \_SB_.GPO1.RBUF */
}
Method (_STA, 0, NotSerialized) // _STA: Status
{
If ((OSYS < 0x07DC))
{
Return (Zero)
}
Return (0x0F)
}
}
> --
> With Best Regards,
> Andy Shevchenko
Best regards,
Flavio
Re: [RFC-PATCH 2/4] mm: Add __rcu_alloc_page_lockless() func.
On Fri, Oct 02, 2020 at 10:45:02AM +0100, Mel Gorman wrote: > On Fri, Oct 02, 2020 at 11:07:29AM +0200, Peter Zijlstra wrote: > > On Fri, Oct 02, 2020 at 09:50:14AM +0100, Mel Gorman wrote: > > > On Fri, Oct 02, 2020 at 09:11:23AM +0200, Michal Hocko wrote: > > > > > > > +#define ___GFP_NO_LOCKS0x80u > > > > > > > > Even if a new gfp flag gains a sufficient traction and support I am > > > > _strongly_ opposed against consuming another flag for that. Bit space is > > > > limited. > > > > > > That is definitely true. I'm not happy with the GFP flag at all, the > > > comment is at best a damage limiting move. It still would be better for > > > a memory pool to be reserved and sized for critical allocations. > > > > This is one of the reasons I did a separate allocation function. No GFP > > flag to leak into general usage. > > > > Even a specific function with a hint that "this is for RCU only" will > not prevent abuse. Not exporting it for modules helps, but yes. > > > > Besides that we certainly do not want to allow craziness like > > > > __GFP_NO_LOCK | __GFP_RECLAIM (and similar), do we? > > > > > > That would deserve to be taken to a dumpster and set on fire. The flag > > > combination could be checked in the allocator but the allocator path fast > > > paths are bad enough already. > > > > Isn't that what we have CONFIG_DEBUG_VM for? > > It's enabled by default by enough distros that adding too many checks > is potentially painful. Granted it would be missed by most benchmarking > which tend to control allocations from userspace but a lot of performance > problems I see are the "death by a thousand cuts" variety. Oh quite agreed, aka death by accounting. But if people are enabling DEBUG options in production kernels, there's something wrong, no? Should we now go add CONFIG_REALLY_DEBUG_STAY_AWAY_ALREADY options?
