Re: [PATCH] perf test record+probe_libc_inet_pton: expect [unknown] for ping as well
On 06/29/2018 11:17 PM, Arnaldo Carvalho de Melo wrote:
Em Thu, Jun 28, 2018 at 03:16:00PM +0800, Li Zhijian escreveu:
On system which has not installed debuginfo of iputils(ping) will fail like:
~/lkp/linux/tools/perf$ sudo ./perf test ping -v
I think that we should try to check if the required debuginfo package is
installed and if so, expect the symbol resolution that takes place in
that case. Other wise expect [unknown], this way we don't leave this
test accepting almost anything in any case.
Looks ping binary in different distros include different symbol.
without this patch, ubuntu 16.04 is good and debian 9 is bad when they both
have not installed debuginfo for ping.
and then after i installed debuginfo for ping(iputils-ping-dbgsym), test become
passed as well on debian 9
Debian 9
bofore installing debuginfo, it includes [unknown]
root@vm-lkp-nex04-8G-10
/usr/src/linux-perf-x86_64-randconfig-w0-06061439-fb43d6cb91ef57d9e58d5f69b423784ff4a4c374/tools/perf#
./perf test ping -v
62: probe libc's inet_pton & backtrace it with ping :
--- start ---
test child forked, pid 6324
ping 6341 [000] 4470.361330: probe_libc:inet_pton: (7f9f3900eaa0)
105aa0 inet_pton (/lib/x86_64-linux-gnu/libc-2.24.so)
d52f8 getaddrinfo (/lib/x86_64-linux-gnu/libc-2.24.so)
3ebe [unknown] (/bin/ping)
test child finished with 0
--
after install debuginfo
root@vm-lkp-nex04-8G-10
/usr/src/linux-perf-x86_64-randconfig-w0-06061439-fb43d6cb91ef57d9e58d5f69b423784ff4a4c374/tools/perf#
./perf test ping -v
62: probe libc's inet_pton & backtrace it with ping :
--- start ---
test child forked, pid 6387
ping 6404 [000] 4488.855086: probe_libc:inet_pton: (7fb5bcc7caa0)
105aa0 inet_pton (/lib/x86_64-linux-gnu/libc-2.24.so)
d52f8 getaddrinfo (/lib/x86_64-linux-gnu/libc-2.24.so)
3ebe main (/bin/ping)
test child finished with 0
Thanks
- Arnaldo
63: probe libc's inet_pton & backtrace it with ping :
--- start ---
test child forked, pid 4207
ping 4224 [007] 3034121.295510: probe_libc:inet_pton: (7fedfccb2200)
7fedfccb2200 __GI___inet_pton+0x0 (/lib/x86_64-linux-gnu/libc-2.23.so)
7fedfcc7ad5e getaddrinfo+0xee (/lib/x86_64-linux-gnu/libc-2.23.so)
55e3239a9f4d [unknown] (/bin/ping)
FAIL: expected backtrace entry 3
".*\+0x[[:xdigit:]]+[[:space:]]\(.*/bin/ping.*\)$" got "55e3239a9f4d [unknown]
(/bin/ping)"
test child finished with -1
end
probe libc's inet_pton & backtrace it with ping: FAILED!
To be compatible with this system, expects [unknown] as well
Fixes: 7903a7086723 ("perf script: Show symbol offsets by default")
CC: Sandipan Das
CC: Thomas Richter
Reported-by: kernel test robot
Signed-off-by: Li Zhijian
---
tools/perf/tests/shell/record+probe_libc_inet_pton.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
index 2630570..ebb6549 100755
--- a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
+++ b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
@@ -22,12 +22,12 @@ trace_libc_inet_pton_backtrace() {
eventattr='call-graph=dwarf,max-stack=4'
expected[2]="gaih_inet.*\+0x[[:xdigit:]]+[[:space:]]\($libc|inlined\)$"
expected[3]="(__GI_)?getaddrinfo\+0x[[:xdigit:]]+[[:space:]]\($libc|inlined\)$"
- expected[4]="main\+0x[[:xdigit:]]+[[:space:]]\(.*/bin/ping.*\)$"
+
expected[4]=".*(\+0x[[:xdigit:]]|\[unknown\])+[[:space:]]\(.*/bin/ping.*\)$"
;;
*)
eventattr='max-stack=3'
expected[2]="getaddrinfo\+0x[[:xdigit:]]+[[:space:]]\($libc\)$"
- expected[3]=".*\+0x[[:xdigit:]]+[[:space:]]\(.*/bin/ping.*\)$"
+
expected[3]=".*(\+0x[[:xdigit:]]|\[unknown\])+[[:space:]]\(.*/bin/ping.*\)$"
;;
esac
--
2.7.4
.
--
Best regards.
Li Zhijian (8528)
Re: [PATCH] perf test record+probe_libc_inet_pton: expect [unknown] for ping as well
On 06/29/2018 11:17 PM, Arnaldo Carvalho de Melo wrote:
Em Thu, Jun 28, 2018 at 03:16:00PM +0800, Li Zhijian escreveu:
On system which has not installed debuginfo of iputils(ping) will fail like:
~/lkp/linux/tools/perf$ sudo ./perf test ping -v
I think that we should try to check if the required debuginfo package is
installed and if so, expect the symbol resolution that takes place in
that case. Other wise expect [unknown], this way we don't leave this
test accepting almost anything in any case.
Looks ping binary in different distros include different symbol.
without this patch, ubuntu 16.04 is good and debian 9 is bad when they both
have not installed debuginfo for ping.
and then after i installed debuginfo for ping(iputils-ping-dbgsym), test become
passed as well on debian 9
Debian 9
bofore installing debuginfo, it includes [unknown]
root@vm-lkp-nex04-8G-10
/usr/src/linux-perf-x86_64-randconfig-w0-06061439-fb43d6cb91ef57d9e58d5f69b423784ff4a4c374/tools/perf#
./perf test ping -v
62: probe libc's inet_pton & backtrace it with ping :
--- start ---
test child forked, pid 6324
ping 6341 [000] 4470.361330: probe_libc:inet_pton: (7f9f3900eaa0)
105aa0 inet_pton (/lib/x86_64-linux-gnu/libc-2.24.so)
d52f8 getaddrinfo (/lib/x86_64-linux-gnu/libc-2.24.so)
3ebe [unknown] (/bin/ping)
test child finished with 0
--
after install debuginfo
root@vm-lkp-nex04-8G-10
/usr/src/linux-perf-x86_64-randconfig-w0-06061439-fb43d6cb91ef57d9e58d5f69b423784ff4a4c374/tools/perf#
./perf test ping -v
62: probe libc's inet_pton & backtrace it with ping :
--- start ---
test child forked, pid 6387
ping 6404 [000] 4488.855086: probe_libc:inet_pton: (7fb5bcc7caa0)
105aa0 inet_pton (/lib/x86_64-linux-gnu/libc-2.24.so)
d52f8 getaddrinfo (/lib/x86_64-linux-gnu/libc-2.24.so)
3ebe main (/bin/ping)
test child finished with 0
Thanks
- Arnaldo
63: probe libc's inet_pton & backtrace it with ping :
--- start ---
test child forked, pid 4207
ping 4224 [007] 3034121.295510: probe_libc:inet_pton: (7fedfccb2200)
7fedfccb2200 __GI___inet_pton+0x0 (/lib/x86_64-linux-gnu/libc-2.23.so)
7fedfcc7ad5e getaddrinfo+0xee (/lib/x86_64-linux-gnu/libc-2.23.so)
55e3239a9f4d [unknown] (/bin/ping)
FAIL: expected backtrace entry 3
".*\+0x[[:xdigit:]]+[[:space:]]\(.*/bin/ping.*\)$" got "55e3239a9f4d [unknown]
(/bin/ping)"
test child finished with -1
end
probe libc's inet_pton & backtrace it with ping: FAILED!
To be compatible with this system, expects [unknown] as well
Fixes: 7903a7086723 ("perf script: Show symbol offsets by default")
CC: Sandipan Das
CC: Thomas Richter
Reported-by: kernel test robot
Signed-off-by: Li Zhijian
---
tools/perf/tests/shell/record+probe_libc_inet_pton.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
index 2630570..ebb6549 100755
--- a/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
+++ b/tools/perf/tests/shell/record+probe_libc_inet_pton.sh
@@ -22,12 +22,12 @@ trace_libc_inet_pton_backtrace() {
eventattr='call-graph=dwarf,max-stack=4'
expected[2]="gaih_inet.*\+0x[[:xdigit:]]+[[:space:]]\($libc|inlined\)$"
expected[3]="(__GI_)?getaddrinfo\+0x[[:xdigit:]]+[[:space:]]\($libc|inlined\)$"
- expected[4]="main\+0x[[:xdigit:]]+[[:space:]]\(.*/bin/ping.*\)$"
+
expected[4]=".*(\+0x[[:xdigit:]]|\[unknown\])+[[:space:]]\(.*/bin/ping.*\)$"
;;
*)
eventattr='max-stack=3'
expected[2]="getaddrinfo\+0x[[:xdigit:]]+[[:space:]]\($libc\)$"
- expected[3]=".*\+0x[[:xdigit:]]+[[:space:]]\(.*/bin/ping.*\)$"
+
expected[3]=".*(\+0x[[:xdigit:]]|\[unknown\])+[[:space:]]\(.*/bin/ping.*\)$"
;;
esac
--
2.7.4
.
--
Best regards.
Li Zhijian (8528)
[PATCH 2/2] x86/intel_rdt: cpu information accessible for pseudo-locked regions
When a resource group enters pseudo-locksetup mode it reflects that the
platform supports cache pseudo-locking and the resource group is unused,
ready to be used for a pseudo-locked region. Until it is set up as a
pseudo-locked region the resource group is "locked down" such that no
new tasks or cpus can be assigned to it. This is accomplished in a user
visible way by making the cpus, cpus_list, and tasks resctrl files
inaccassible (user cannot read from or write to these files).
When the resource group changes to pseudo-locked mode it represents a
cache pseudo-locked region. While not appropriate to make any changes to
the cpus assigned to this region it is useful to make it easy for the
user to see which cpus are associated with the pseudo-locked region.
Modify the permissions of the cpus/cpus_list file when the resource
group changes to pseudo-locked mode to support reading (not writing).
The information presented to the user when reading the file are the cpus
associated with the pseudo-locked region.
Signed-off-by: Reinette Chatre
---
Documentation/x86/intel_rdt_ui.txt | 3 +++
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 3 +++
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c| 8 ++--
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/Documentation/x86/intel_rdt_ui.txt
b/Documentation/x86/intel_rdt_ui.txt
index acac30b67c62..f662d3c530e5 100644
--- a/Documentation/x86/intel_rdt_ui.txt
+++ b/Documentation/x86/intel_rdt_ui.txt
@@ -178,6 +178,9 @@ All groups contain the following files:
CPUs to/from this group. As with the tasks file a hierarchy is
maintained where MON groups may only include CPUs owned by the
parent CTRL_MON group.
+ When the resouce group is in pseudo-locked mode this file will
+ only be readable, reflecting the CPUs associated with the
+ pseudo-locked region.
"cpus_list":
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 5dfe4008c58f..751c78f9992f 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -1303,6 +1303,9 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
rdtgrp->mode = RDT_MODE_PSEUDO_LOCKED;
closid_free(rdtgrp->closid);
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus", 0444);
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus_list", 0444);
+
ret = 0;
goto out;
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index c20b51afd62d..d6d7ea7349d0 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -265,8 +265,12 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (rdtgrp) {
- seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
- cpumask_pr_args(>cpu_mask));
+ if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)
+ seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
+ cpumask_pr_args(>plr->d->cpu_mask));
+ else
+ seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
+ cpumask_pr_args(>cpu_mask));
} else {
ret = -ENOENT;
}
--
2.17.0
[PATCH 0/2] x86/intel_rdt: Display CPUs associated with pseudo-locked region
Dear Maintainers, The cpus and cpus_list resctrl files are locked down when the resource group enters pseudo-locksetup mode and remains locked down when the resource group transitions to pseudo-locked mode. This was done to ensure a resource group in pseudo-locksetup mode is and remain unused until it transitions to pseudo-locked mode. With this series read (but not write) access is restored to the cpus and cpus_list files after a resource group enters pseudo-locked mode. The information displayed is also adjusted appropriately. It is now possible for users to view which cpus are associated with a pseudo-locked region through the existing cpus and cpus_list resctrl files. Only the tasks file remains locked down. For example: # mount -t resctrl resctrl /sys/fs/resctrl # cd /sys/fs/resctrl # echo 'L2:1=0xfc' > schemata # mkdir p1 # ls -l p1/cpus* p1/tasks -rw-r--r-- 1 root root 0 Jun 30 06:30 p1/cpus -rw-r--r-- 1 root root 0 Jun 30 06:30 p1/cpus_list -rw-r--r-- 1 root root 0 Jun 30 06:30 p1/tasks # echo pseudo-locksetup > p1/mode # ls -l p1/cpus* p1/tasks -- 1 root root 0 Jun 30 06:30 p1/cpus -- 1 root root 0 Jun 30 06:30 p1/cpus_list -- 1 root root 0 Jun 30 06:30 p1/tasks # echo 'L2:1=0x3' > p1/schemata # ls -l p1/cpus* p1/tasks -r--r--r-- 1 root root 0 Jun 30 06:30 p1/cpus -r--r--r-- 1 root root 0 Jun 30 06:30 p1/cpus_list -- 1 root root 0 Jun 30 06:30 p1/tasks # grep . p1/* p1/cpus:c p1/cpus_list:2-3 p1/mode:pseudo-locked p1/schemata:L2:1=3 p1/size:L2:1=262144 grep: p1/tasks: Permission denied Reinette Chatre (2): x86/intel_rdt: Support restoration of subset of permissions x86/intel_rdt: cpu information accessible for pseudo-locked regions Documentation/x86/intel_rdt_ui.txt | 3 +++ arch/x86/kernel/cpu/intel_rdt.h | 3 ++- arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 17 ++--- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c| 14 ++ 4 files changed, 25 insertions(+), 12 deletions(-) -- 2.17.0
[PATCH 2/2] x86/intel_rdt: cpu information accessible for pseudo-locked regions
When a resource group enters pseudo-locksetup mode it reflects that the
platform supports cache pseudo-locking and the resource group is unused,
ready to be used for a pseudo-locked region. Until it is set up as a
pseudo-locked region the resource group is "locked down" such that no
new tasks or cpus can be assigned to it. This is accomplished in a user
visible way by making the cpus, cpus_list, and tasks resctrl files
inaccassible (user cannot read from or write to these files).
When the resource group changes to pseudo-locked mode it represents a
cache pseudo-locked region. While not appropriate to make any changes to
the cpus assigned to this region it is useful to make it easy for the
user to see which cpus are associated with the pseudo-locked region.
Modify the permissions of the cpus/cpus_list file when the resource
group changes to pseudo-locked mode to support reading (not writing).
The information presented to the user when reading the file are the cpus
associated with the pseudo-locked region.
Signed-off-by: Reinette Chatre
---
Documentation/x86/intel_rdt_ui.txt | 3 +++
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 3 +++
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c| 8 ++--
3 files changed, 12 insertions(+), 2 deletions(-)
diff --git a/Documentation/x86/intel_rdt_ui.txt
b/Documentation/x86/intel_rdt_ui.txt
index acac30b67c62..f662d3c530e5 100644
--- a/Documentation/x86/intel_rdt_ui.txt
+++ b/Documentation/x86/intel_rdt_ui.txt
@@ -178,6 +178,9 @@ All groups contain the following files:
CPUs to/from this group. As with the tasks file a hierarchy is
maintained where MON groups may only include CPUs owned by the
parent CTRL_MON group.
+ When the resouce group is in pseudo-locked mode this file will
+ only be readable, reflecting the CPUs associated with the
+ pseudo-locked region.
"cpus_list":
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 5dfe4008c58f..751c78f9992f 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -1303,6 +1303,9 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup *rdtgrp)
rdtgrp->mode = RDT_MODE_PSEUDO_LOCKED;
closid_free(rdtgrp->closid);
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus", 0444);
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus_list", 0444);
+
ret = 0;
goto out;
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index c20b51afd62d..d6d7ea7349d0 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -265,8 +265,12 @@ static int rdtgroup_cpus_show(struct kernfs_open_file *of,
rdtgrp = rdtgroup_kn_lock_live(of->kn);
if (rdtgrp) {
- seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
- cpumask_pr_args(>cpu_mask));
+ if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)
+ seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
+ cpumask_pr_args(>plr->d->cpu_mask));
+ else
+ seq_printf(s, is_cpu_list(of) ? "%*pbl\n" : "%*pb\n",
+ cpumask_pr_args(>cpu_mask));
} else {
ret = -ENOENT;
}
--
2.17.0
[PATCH 0/2] x86/intel_rdt: Display CPUs associated with pseudo-locked region
Dear Maintainers, The cpus and cpus_list resctrl files are locked down when the resource group enters pseudo-locksetup mode and remains locked down when the resource group transitions to pseudo-locked mode. This was done to ensure a resource group in pseudo-locksetup mode is and remain unused until it transitions to pseudo-locked mode. With this series read (but not write) access is restored to the cpus and cpus_list files after a resource group enters pseudo-locked mode. The information displayed is also adjusted appropriately. It is now possible for users to view which cpus are associated with a pseudo-locked region through the existing cpus and cpus_list resctrl files. Only the tasks file remains locked down. For example: # mount -t resctrl resctrl /sys/fs/resctrl # cd /sys/fs/resctrl # echo 'L2:1=0xfc' > schemata # mkdir p1 # ls -l p1/cpus* p1/tasks -rw-r--r-- 1 root root 0 Jun 30 06:30 p1/cpus -rw-r--r-- 1 root root 0 Jun 30 06:30 p1/cpus_list -rw-r--r-- 1 root root 0 Jun 30 06:30 p1/tasks # echo pseudo-locksetup > p1/mode # ls -l p1/cpus* p1/tasks -- 1 root root 0 Jun 30 06:30 p1/cpus -- 1 root root 0 Jun 30 06:30 p1/cpus_list -- 1 root root 0 Jun 30 06:30 p1/tasks # echo 'L2:1=0x3' > p1/schemata # ls -l p1/cpus* p1/tasks -r--r--r-- 1 root root 0 Jun 30 06:30 p1/cpus -r--r--r-- 1 root root 0 Jun 30 06:30 p1/cpus_list -- 1 root root 0 Jun 30 06:30 p1/tasks # grep . p1/* p1/cpus:c p1/cpus_list:2-3 p1/mode:pseudo-locked p1/schemata:L2:1=3 p1/size:L2:1=262144 grep: p1/tasks: Permission denied Reinette Chatre (2): x86/intel_rdt: Support restoration of subset of permissions x86/intel_rdt: cpu information accessible for pseudo-locked regions Documentation/x86/intel_rdt_ui.txt | 3 +++ arch/x86/kernel/cpu/intel_rdt.h | 3 ++- arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 17 ++--- arch/x86/kernel/cpu/intel_rdt_rdtgroup.c| 14 ++ 4 files changed, 25 insertions(+), 12 deletions(-) -- 2.17.0
[PATCH 1/2] x86/intel_rdt: Support restoration of subset of permissions
As the mode of a resource group changes, the operations it can support
may also change. One way in which the supported operations are managed
is to modify the permissions of the files within the resource group's
resctrl directory.
At the moment only two possible permissions are supported: the default
permissions or no permissions in support for when the operation is
"locked down". It is possible where an operation on a resource group may
have more possibilities. For example, if by default changes can be made to
the resource group by writing to a resctrl file while the current
settings can be obtained by reading from the file, then it may be
possible that in another mode it is only possible to read the current
settings, and not change them.
Make it possible to modify some of the permissions of a resctrl file in
support of a more flexible way to manage the operations on a resource
group. In this preparation work the original behavior is maintained
where all permissions are restored.
Signed-off-by: Reinette Chatre
---
arch/x86/kernel/cpu/intel_rdt.h | 3 ++-
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 14 +++---
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c| 6 --
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt.h b/arch/x86/kernel/cpu/intel_rdt.h
index 2d9cbb9d7a58..4e588f36228f 100644
--- a/arch/x86/kernel/cpu/intel_rdt.h
+++ b/arch/x86/kernel/cpu/intel_rdt.h
@@ -512,7 +512,8 @@ void rdt_ctrl_update(void *arg);
struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn);
void rdtgroup_kn_unlock(struct kernfs_node *kn);
int rdtgroup_kn_mode_restrict(struct rdtgroup *r, const char *name);
-int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name);
+int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name,
+umode_t mask);
struct rdt_domain *rdt_find_domain(struct rdt_resource *r, int id,
struct list_head **pos);
ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 8fd79c281ee6..5dfe4008c58f 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -590,11 +590,11 @@ static int rdtgroup_locksetup_user_restrict(struct
rdtgroup *rdtgrp)
goto out;
err_cpus_list:
- rdtgroup_kn_mode_restore(rdtgrp, "cpus_list");
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus_list", 0777);
err_cpus:
- rdtgroup_kn_mode_restore(rdtgrp, "cpus");
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus", 0777);
err_tasks:
- rdtgroup_kn_mode_restore(rdtgrp, "tasks");
+ rdtgroup_kn_mode_restore(rdtgrp, "tasks", 0777);
out:
return ret;
}
@@ -615,20 +615,20 @@ static int rdtgroup_locksetup_user_restore(struct
rdtgroup *rdtgrp)
{
int ret;
- ret = rdtgroup_kn_mode_restore(rdtgrp, "tasks");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "tasks", 0777);
if (ret)
return ret;
- ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus", 0777);
if (ret)
goto err_tasks;
- ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus_list");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus_list", 0777);
if (ret)
goto err_cpus;
if (rdt_mon_capable) {
- ret = rdtgroup_kn_mode_restore(rdtgrp, "mon_groups");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "mon_groups", 0777);
if (ret)
goto err_cpus_list;
}
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 7b4a09d81a30..c20b51afd62d 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -1405,13 +1405,15 @@ int rdtgroup_kn_mode_restrict(struct rdtgroup *r, const
char *name)
* rdtgroup_kn_mode_restore - Restore user access to named resctrl file
* @r: The resource group with which the file is associated.
* @name: Name of the file
+ * @mask: Mask of permissions that should be restored
*
* Restore the permissions of the named file. If @name is a directory the
* permissions of its parent will be used.
*
* Return: 0 on success, <0 on failure.
*/
-int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name)
+int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name,
+umode_t mask)
{
struct iattr iattr = {.ia_valid = ATTR_MODE,};
struct kernfs_node *kn, *parent;
@@ -1423,7 +1425,7 @@ int rdtgroup_kn_mode_restore(struct rdtgroup *r, const
char *name)
for (rft = rfts; rft < rfts + len; rft++) {
if (!strcmp(rft->name, name))
- iattr.ia_mode = rft->mode;
+ iattr.ia_mode = rft->mode & mask;
[PATCH 1/2] x86/intel_rdt: Support restoration of subset of permissions
As the mode of a resource group changes, the operations it can support
may also change. One way in which the supported operations are managed
is to modify the permissions of the files within the resource group's
resctrl directory.
At the moment only two possible permissions are supported: the default
permissions or no permissions in support for when the operation is
"locked down". It is possible where an operation on a resource group may
have more possibilities. For example, if by default changes can be made to
the resource group by writing to a resctrl file while the current
settings can be obtained by reading from the file, then it may be
possible that in another mode it is only possible to read the current
settings, and not change them.
Make it possible to modify some of the permissions of a resctrl file in
support of a more flexible way to manage the operations on a resource
group. In this preparation work the original behavior is maintained
where all permissions are restored.
Signed-off-by: Reinette Chatre
---
arch/x86/kernel/cpu/intel_rdt.h | 3 ++-
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 14 +++---
arch/x86/kernel/cpu/intel_rdt_rdtgroup.c| 6 --
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt.h b/arch/x86/kernel/cpu/intel_rdt.h
index 2d9cbb9d7a58..4e588f36228f 100644
--- a/arch/x86/kernel/cpu/intel_rdt.h
+++ b/arch/x86/kernel/cpu/intel_rdt.h
@@ -512,7 +512,8 @@ void rdt_ctrl_update(void *arg);
struct rdtgroup *rdtgroup_kn_lock_live(struct kernfs_node *kn);
void rdtgroup_kn_unlock(struct kernfs_node *kn);
int rdtgroup_kn_mode_restrict(struct rdtgroup *r, const char *name);
-int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name);
+int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name,
+umode_t mask);
struct rdt_domain *rdt_find_domain(struct rdt_resource *r, int id,
struct list_head **pos);
ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 8fd79c281ee6..5dfe4008c58f 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -590,11 +590,11 @@ static int rdtgroup_locksetup_user_restrict(struct
rdtgroup *rdtgrp)
goto out;
err_cpus_list:
- rdtgroup_kn_mode_restore(rdtgrp, "cpus_list");
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus_list", 0777);
err_cpus:
- rdtgroup_kn_mode_restore(rdtgrp, "cpus");
+ rdtgroup_kn_mode_restore(rdtgrp, "cpus", 0777);
err_tasks:
- rdtgroup_kn_mode_restore(rdtgrp, "tasks");
+ rdtgroup_kn_mode_restore(rdtgrp, "tasks", 0777);
out:
return ret;
}
@@ -615,20 +615,20 @@ static int rdtgroup_locksetup_user_restore(struct
rdtgroup *rdtgrp)
{
int ret;
- ret = rdtgroup_kn_mode_restore(rdtgrp, "tasks");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "tasks", 0777);
if (ret)
return ret;
- ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus", 0777);
if (ret)
goto err_tasks;
- ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus_list");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "cpus_list", 0777);
if (ret)
goto err_cpus;
if (rdt_mon_capable) {
- ret = rdtgroup_kn_mode_restore(rdtgrp, "mon_groups");
+ ret = rdtgroup_kn_mode_restore(rdtgrp, "mon_groups", 0777);
if (ret)
goto err_cpus_list;
}
diff --git a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
index 7b4a09d81a30..c20b51afd62d 100644
--- a/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
+++ b/arch/x86/kernel/cpu/intel_rdt_rdtgroup.c
@@ -1405,13 +1405,15 @@ int rdtgroup_kn_mode_restrict(struct rdtgroup *r, const
char *name)
* rdtgroup_kn_mode_restore - Restore user access to named resctrl file
* @r: The resource group with which the file is associated.
* @name: Name of the file
+ * @mask: Mask of permissions that should be restored
*
* Restore the permissions of the named file. If @name is a directory the
* permissions of its parent will be used.
*
* Return: 0 on success, <0 on failure.
*/
-int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name)
+int rdtgroup_kn_mode_restore(struct rdtgroup *r, const char *name,
+umode_t mask)
{
struct iattr iattr = {.ia_valid = ATTR_MODE,};
struct kernfs_node *kn, *parent;
@@ -1423,7 +1425,7 @@ int rdtgroup_kn_mode_restore(struct rdtgroup *r, const
char *name)
for (rft = rfts; rft < rfts + len; rft++) {
if (!strcmp(rft->name, name))
- iattr.ia_mode = rft->mode;
+ iattr.ia_mode = rft->mode & mask;
[PATCH 1/2] x86/intel_rdt: Move pseudo_lock_region_clear
The pseudo_lock_region_clear() function is moved to earlier in the
file in preparation for its use in functions that currently appear
before it. No functional change.
Signed-off-by: Reinette Chatre
---
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 46 ++---
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 6e83f61552a5..1860ec10302d 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -246,6 +246,29 @@ static int pseudo_lock_cstates_constrain(struct
pseudo_lock_region *plr)
return ret;
}
+/**
+ * pseudo_lock_region_clear - Reset pseudo-lock region data
+ * @plr: pseudo-lock region
+ *
+ * All content of the pseudo-locked region is reset - any memory allocated
+ * freed.
+ *
+ * Return: void
+ */
+static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
+{
+ plr->size = 0;
+ plr->line_size = 0;
+ kfree(plr->kmem);
+ plr->kmem = NULL;
+ plr->r = NULL;
+ if (plr->d)
+ plr->d->plr = NULL;
+ plr->d = NULL;
+ plr->cbm = 0;
+ plr->debugfs_dir = NULL;
+}
+
/**
* pseudo_lock_region_init - Initialize pseudo-lock region information
* @plr: pseudo-lock region
@@ -318,29 +341,6 @@ static int pseudo_lock_init(struct rdtgroup *rdtgrp)
return 0;
}
-/**
- * pseudo_lock_region_clear - Reset pseudo-lock region data
- * @plr: pseudo-lock region
- *
- * All content of the pseudo-locked region is reset - any memory allocated
- * freed.
- *
- * Return: void
- */
-static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
-{
- plr->size = 0;
- plr->line_size = 0;
- kfree(plr->kmem);
- plr->kmem = NULL;
- plr->r = NULL;
- if (plr->d)
- plr->d->plr = NULL;
- plr->d = NULL;
- plr->cbm = 0;
- plr->debugfs_dir = NULL;
-}
-
/**
* pseudo_lock_region_alloc - Allocate kernel memory that will be pseudo-locked
* @plr: pseudo-lock region
--
2.17.0
[PATCH 1/2] x86/intel_rdt: Move pseudo_lock_region_clear
The pseudo_lock_region_clear() function is moved to earlier in the
file in preparation for its use in functions that currently appear
before it. No functional change.
Signed-off-by: Reinette Chatre
---
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 46 ++---
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 6e83f61552a5..1860ec10302d 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -246,6 +246,29 @@ static int pseudo_lock_cstates_constrain(struct
pseudo_lock_region *plr)
return ret;
}
+/**
+ * pseudo_lock_region_clear - Reset pseudo-lock region data
+ * @plr: pseudo-lock region
+ *
+ * All content of the pseudo-locked region is reset - any memory allocated
+ * freed.
+ *
+ * Return: void
+ */
+static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
+{
+ plr->size = 0;
+ plr->line_size = 0;
+ kfree(plr->kmem);
+ plr->kmem = NULL;
+ plr->r = NULL;
+ if (plr->d)
+ plr->d->plr = NULL;
+ plr->d = NULL;
+ plr->cbm = 0;
+ plr->debugfs_dir = NULL;
+}
+
/**
* pseudo_lock_region_init - Initialize pseudo-lock region information
* @plr: pseudo-lock region
@@ -318,29 +341,6 @@ static int pseudo_lock_init(struct rdtgroup *rdtgrp)
return 0;
}
-/**
- * pseudo_lock_region_clear - Reset pseudo-lock region data
- * @plr: pseudo-lock region
- *
- * All content of the pseudo-locked region is reset - any memory allocated
- * freed.
- *
- * Return: void
- */
-static void pseudo_lock_region_clear(struct pseudo_lock_region *plr)
-{
- plr->size = 0;
- plr->line_size = 0;
- kfree(plr->kmem);
- plr->kmem = NULL;
- plr->r = NULL;
- if (plr->d)
- plr->d->plr = NULL;
- plr->d = NULL;
- plr->cbm = 0;
- plr->debugfs_dir = NULL;
-}
-
/**
* pseudo_lock_region_alloc - Allocate kernel memory that will be pseudo-locked
* @plr: pseudo-lock region
--
2.17.0
[PATCH 0/2] x86/intel_rdt: Fix cache pseudo-locking error path cleanup
Dear Maintainers, A bug exists in the error handling code during pseudo-lock region creation. When an error occurs early during pseudo-lock region creation the pseudo_lock_region struct is not cleaned up properly but remains associated with the resource group (since it remains in pseudo-locksetup mode). This partially initialized struct causes problems when other areas need to obtain resource group data - when partially initialized the resource group is treated as a pseudo-locked region. Following is an example of the error being encountered. First a pseudo-locked region of larger than 4MB is attempted. This fails early because of lack for support. Since this is not cleaned up properly, a subsequent attempt fails because it is (incorrectly) believed that a pseudo-locked region already exists, also the bit_usage file reports incorrect data. # mount -t resctrl resctrl /sys/fs/resctrl # cd /sys/fs/resctrl/ # mkdir p1 # echo 'L3:1=0x0' > schemata # echo pseudo-locksetup > p1/mode # echo 'L3:1=0xf' > p1/schemata -bash: echo: write error: Argument list too long # cat info/last_cmd_status requested region exceeds maximum size # echo 'L3:1=0x1' > p1/schemata -bash: echo: write error: Invalid argument # cat info/last_cmd_status pseudo-locked region in hierarchy # cat info/L3/bit_usage 0=XXSS;1=XXSS After the fixes in this series have been applied: # mount -t resctrl resctrl /sys/fs/resctrl/ # cd /sys/fs/resctrl/ # mkdir p1 # echo pseudo-locksetup > p1/mode # echo 'L3:1=0x0' > schemata # echo 'L3:1=0xf' > p1/schemata -bash: echo: write error: Argument list too long # cat info/last_cmd_status requested region exceeds maximum size # cat info/L3/bit_usage 0=XXSS;1=XXSS # echo 'L3:1=0x1' > p1/schemata # cat info/L3/bit_usage 0=XXSS;1=XXSS000P Reinette Chatre (2): x86/intel_rdt: Move pseudo_lock_region_clear x86/intel_rdt: Fix cleanup of plr structure on error arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 68 - 1 file changed, 40 insertions(+), 28 deletions(-) -- 2.17.0
[PATCH 0/2] x86/intel_rdt: Fix cache pseudo-locking error path cleanup
Dear Maintainers, A bug exists in the error handling code during pseudo-lock region creation. When an error occurs early during pseudo-lock region creation the pseudo_lock_region struct is not cleaned up properly but remains associated with the resource group (since it remains in pseudo-locksetup mode). This partially initialized struct causes problems when other areas need to obtain resource group data - when partially initialized the resource group is treated as a pseudo-locked region. Following is an example of the error being encountered. First a pseudo-locked region of larger than 4MB is attempted. This fails early because of lack for support. Since this is not cleaned up properly, a subsequent attempt fails because it is (incorrectly) believed that a pseudo-locked region already exists, also the bit_usage file reports incorrect data. # mount -t resctrl resctrl /sys/fs/resctrl # cd /sys/fs/resctrl/ # mkdir p1 # echo 'L3:1=0x0' > schemata # echo pseudo-locksetup > p1/mode # echo 'L3:1=0xf' > p1/schemata -bash: echo: write error: Argument list too long # cat info/last_cmd_status requested region exceeds maximum size # echo 'L3:1=0x1' > p1/schemata -bash: echo: write error: Invalid argument # cat info/last_cmd_status pseudo-locked region in hierarchy # cat info/L3/bit_usage 0=XXSS;1=XXSS After the fixes in this series have been applied: # mount -t resctrl resctrl /sys/fs/resctrl/ # cd /sys/fs/resctrl/ # mkdir p1 # echo pseudo-locksetup > p1/mode # echo 'L3:1=0x0' > schemata # echo 'L3:1=0xf' > p1/schemata -bash: echo: write error: Argument list too long # cat info/last_cmd_status requested region exceeds maximum size # cat info/L3/bit_usage 0=XXSS;1=XXSS # echo 'L3:1=0x1' > p1/schemata # cat info/L3/bit_usage 0=XXSS;1=XXSS000P Reinette Chatre (2): x86/intel_rdt: Move pseudo_lock_region_clear x86/intel_rdt: Fix cleanup of plr structure on error arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 68 - 1 file changed, 40 insertions(+), 28 deletions(-) -- 2.17.0
[PATCH 2/2] x86/intel_rdt: Fix cleanup of plr structure on error
When a resource group enters pseudo-locksetup mode a pseudo_lock_region
is associated with it. When the user writes to the resource group's
schemata file the CBM of the requested pseudo-locked region is entered
into the pseudo_lock_region struct. If any part of pseudo-lock region
creation fails the resource group will remain in pseudo-locksetup mode
with the pseudo_lock_region associated with it.
In case of failure during pseudo-lock region creation care needs to be
taken to ensure that the pseudo_lock_region struct associated with the
resource group is cleared from any pseudo-locking data - especially the
CBM. This is because the existence of a pseudo_lock_region struct with a
CBM is significant in other areas of the code, for example, the display
of bit_usage and initialization of a new resource group.
Fix the error path of pseudo-lock region creation to ensure that the
pseudo_lock_region struct is cleared at each error exit.
Signed-off-by: Reinette Chatre
---
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 22 -
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 1860ec10302d..8fd79c281ee6 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -290,6 +290,7 @@ static void pseudo_lock_region_clear(struct
pseudo_lock_region *plr)
static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
{
struct cpu_cacheinfo *ci;
+ int ret;
int i;
/* Pick the first cpu we find that is associated with the cache. */
@@ -298,7 +299,8 @@ static int pseudo_lock_region_init(struct
pseudo_lock_region *plr)
if (!cpu_online(plr->cpu)) {
rdt_last_cmd_printf("cpu %u associated with cache not online\n",
plr->cpu);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_region;
}
ci = get_cpu_cacheinfo(plr->cpu);
@@ -312,8 +314,11 @@ static int pseudo_lock_region_init(struct
pseudo_lock_region *plr)
}
}
+ ret = -1;
rdt_last_cmd_puts("unable to determine cache line size\n");
- return -1;
+out_region:
+ pseudo_lock_region_clear(plr);
+ return ret;
}
/**
@@ -365,16 +370,23 @@ static int pseudo_lock_region_alloc(struct
pseudo_lock_region *plr)
*/
if (plr->size > KMALLOC_MAX_SIZE) {
rdt_last_cmd_puts("requested region exceeds maximum size\n");
- return -E2BIG;
+ ret = -E2BIG;
+ goto out_region;
}
plr->kmem = kzalloc(plr->size, GFP_KERNEL);
if (!plr->kmem) {
rdt_last_cmd_puts("unable to allocate memory\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_region;
}
- return 0;
+ ret = 0;
+ goto out;
+out_region:
+ pseudo_lock_region_clear(plr);
+out:
+ return ret;
}
/**
--
2.17.0
[PATCH 2/2] x86/intel_rdt: Fix cleanup of plr structure on error
When a resource group enters pseudo-locksetup mode a pseudo_lock_region
is associated with it. When the user writes to the resource group's
schemata file the CBM of the requested pseudo-locked region is entered
into the pseudo_lock_region struct. If any part of pseudo-lock region
creation fails the resource group will remain in pseudo-locksetup mode
with the pseudo_lock_region associated with it.
In case of failure during pseudo-lock region creation care needs to be
taken to ensure that the pseudo_lock_region struct associated with the
resource group is cleared from any pseudo-locking data - especially the
CBM. This is because the existence of a pseudo_lock_region struct with a
CBM is significant in other areas of the code, for example, the display
of bit_usage and initialization of a new resource group.
Fix the error path of pseudo-lock region creation to ensure that the
pseudo_lock_region struct is cleared at each error exit.
Signed-off-by: Reinette Chatre
---
arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c | 22 -
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
index 1860ec10302d..8fd79c281ee6 100644
--- a/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
+++ b/arch/x86/kernel/cpu/intel_rdt_pseudo_lock.c
@@ -290,6 +290,7 @@ static void pseudo_lock_region_clear(struct
pseudo_lock_region *plr)
static int pseudo_lock_region_init(struct pseudo_lock_region *plr)
{
struct cpu_cacheinfo *ci;
+ int ret;
int i;
/* Pick the first cpu we find that is associated with the cache. */
@@ -298,7 +299,8 @@ static int pseudo_lock_region_init(struct
pseudo_lock_region *plr)
if (!cpu_online(plr->cpu)) {
rdt_last_cmd_printf("cpu %u associated with cache not online\n",
plr->cpu);
- return -ENODEV;
+ ret = -ENODEV;
+ goto out_region;
}
ci = get_cpu_cacheinfo(plr->cpu);
@@ -312,8 +314,11 @@ static int pseudo_lock_region_init(struct
pseudo_lock_region *plr)
}
}
+ ret = -1;
rdt_last_cmd_puts("unable to determine cache line size\n");
- return -1;
+out_region:
+ pseudo_lock_region_clear(plr);
+ return ret;
}
/**
@@ -365,16 +370,23 @@ static int pseudo_lock_region_alloc(struct
pseudo_lock_region *plr)
*/
if (plr->size > KMALLOC_MAX_SIZE) {
rdt_last_cmd_puts("requested region exceeds maximum size\n");
- return -E2BIG;
+ ret = -E2BIG;
+ goto out_region;
}
plr->kmem = kzalloc(plr->size, GFP_KERNEL);
if (!plr->kmem) {
rdt_last_cmd_puts("unable to allocate memory\n");
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out_region;
}
- return 0;
+ ret = 0;
+ goto out;
+out_region:
+ pseudo_lock_region_clear(plr);
+out:
+ return ret;
}
/**
--
2.17.0
Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On June 30, 2018 3:33:29 PM GMT+02:00, Manivannan Sadhasivam
wrote:
>Add Actions Semiconductor Owl family S900 I2C driver.
>
>Signed-off-by: Manivannan Sadhasivam
>---
> drivers/i2c/busses/Kconfig | 7 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-owl.c | 477 +++
> 3 files changed, 485 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-owl.c
>
>diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>index 4f8df2ec87b1..8c8025f87ce4 100644
>--- a/drivers/i2c/busses/Kconfig
>+++ b/drivers/i2c/busses/Kconfig
>@@ -762,6 +762,13 @@ config I2C_OMAP
> Like OMAP1510/1610/1710/5912 and OMAP242x.
> For details see http://www.ti.com/omap.
>
>+config I2C_OWL
>+ tristate "Actions Semiconductor Owl I2C Controller"
>+ depends on ARCH_ACTIONS || COMPILE_TEST
>+ help
>+Say Y here if you want to use the I2C bus controller on
>+the Actions Semiconductor Owl SoC's.
>+
> config I2C_PASEMI
> tristate "PA Semi SMBus interface"
> depends on PPC_PASEMI && PCI
>diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>index 5a869144a0c5..b71618f77880 100644
>--- a/drivers/i2c/busses/Makefile
>+++ b/drivers/i2c/busses/Makefile
>@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_MXS)+= i2c-mxs.o
> obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o
> obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o
> obj-$(CONFIG_I2C_OMAP)+= i2c-omap.o
>+obj-$(CONFIG_I2C_OWL) += i2c-owl.o
> obj-$(CONFIG_I2C_PASEMI) += i2c-pasemi.o
> obj-$(CONFIG_I2C_PCA_PLATFORM)+= i2c-pca-platform.o
> obj-$(CONFIG_I2C_PMCMSP) += i2c-pmcmsp.o
>diff --git a/drivers/i2c/busses/i2c-owl.c
>b/drivers/i2c/busses/i2c-owl.c
>new file mode 100644
>index ..144a249bf994
>--- /dev/null
>+++ b/drivers/i2c/busses/i2c-owl.c
>@@ -0,0 +1,477 @@
>+// SPDX-License-Identifier: GPL-2.0+
>+/*
>+ * Actions Semiconductor Owl SoC's I2C driver
>+ *
>+ * Copyright (c) 2014 Actions Semi Inc.
>+ * Author: David Liu
>+ *
>+ * Copyright (c) 2018 Linaro Ltd.
>+ * Author: Manivannan Sadhasivam
>+ */
>+
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+
>+/* I2C registers */
>+#define OWL_I2C_REG_CTL 0x
>+#define OWL_I2C_REG_CLKDIV0x0004
>+#define OWL_I2C_REG_STAT 0x0008
>+#define OWL_I2C_REG_ADDR 0x000C
>+#define OWL_I2C_REG_TXDAT 0x0010
>+#define OWL_I2C_REG_RXDAT 0x0014
>+#define OWL_I2C_REG_CMD 0x0018
>+#define OWL_I2C_REG_FIFOCTL 0x001C
>+#define OWL_I2C_REG_FIFOSTAT 0x0020
>+#define OWL_I2C_REG_DATCNT0x0024
>+#define OWL_I2C_REG_RCNT 0x0028
>+
>+/* I2Cx_CTL Bit Mask */
>+#define OWL_I2C_CTL_RBBIT(1)
>+#define OWL_I2C_CTL_GBCC(x) (((x) & 0x3) << 2)
>+#define OWL_I2C_CTL_GBCC_NONE OWL_I2C_CTL_GBCC(0)
>+#define OWL_I2C_CTL_GBCC_START OWL_I2C_CTL_GBCC(1)
>+#define OWL_I2C_CTL_GBCC_STOP OWL_I2C_CTL_GBCC(2)
>+#define OWL_I2C_CTL_GBCC_RSTART OWL_I2C_CTL_GBCC(3)
>+#define OWL_I2C_CTL_IRQE BIT(5)
>+#define OWL_I2C_CTL_ENBIT(7)
>+#define OWL_I2C_CTL_AEBIT(8)
>+#define OWL_I2C_CTL_SHSM BIT(10)
>+
>+#define OWL_I2C_DIV_FACTOR(x) ((x) & 0xff)
>+
>+/* I2Cx_STAT Bit Mask */
>+#define OWL_I2C_STAT_RACK BIT(0)
>+#define OWL_I2C_STAT_BEB BIT(1)
>+#define OWL_I2C_STAT_IRQP BIT(2)
>+#define OWL_I2C_STAT_LAB BIT(3)
>+#define OWL_I2C_STAT_STPD BIT(4)
>+#define OWL_I2C_STAT_STAD BIT(5)
>+#define OWL_I2C_STAT_BBB BIT(6)
>+#define OWL_I2C_STAT_TCB BIT(7)
>+#define OWL_I2C_STAT_LBST BIT(8)
>+#define OWL_I2C_STAT_SAMB BIT(9)
>+#define OWL_I2C_STAT_SRGC BIT(10)
>+
>+/* I2Cx_CMD Bit Mask */
>+#define OWL_I2C_CMD_SBE BIT(0)
>+#define OWL_I2C_CMD_RBE BIT(4)
>+#define OWL_I2C_CMD_DEBIT(8)
>+#define OWL_I2C_CMD_NSBIT(9)
>+#define OWL_I2C_CMD_SEBIT(10)
>+#define OWL_I2C_CMD_MSS BIT(11)
>+#define OWL_I2C_CMD_WRS BIT(12)
>+#define OWL_I2C_CMD_SECL BIT(15)
>+
>+#define OWL_I2C_CMD_AS(x) (((x) & 0x7) << 1)
>+#define OWL_I2C_CMD_SAS(x)(((x) & 0x7) << 5)
>+
>+/* I2Cx_FIFOCTL Bit Mask */
>+#define OWL_I2C_FIFOCTL_NIB BIT(0)
>+#define OWL_I2C_FIFOCTL_RFR BIT(1)
>+#define OWL_I2C_FIFOCTL_TFR BIT(2)
>+
>+/* I2Cc_FIFOSTAT Bit Mask */
>+#define OWL_I2C_FIFOSTAT_RNB BIT(1)
>+#define OWL_I2C_FIFOSTAT_RFE BIT(2)
>+#define OWL_I2C_FIFOSTAT_TFF BIT(5)
>+#define OWL_I2C_FIFOSTAT_TFD GENMASK(23, 16)
>+#define OWL_I2C_FIFOSTAT_RFD GENMASK(15, 8)
>+
>+/* I2C bus timeout */
>+#define OWL_I2C_TIMEOUT msecs_to_jiffies(4 * 1000)
>+
>+#define OWL_I2C_MAX_RETRIES 50
>+
>+#define OWL_I2C_DEF_SPEED_HZ 10
>+#define OWL_I2C_MAX_SPEED_HZ 40
>+
>+struct owl_i2c_dev {
>+ struct i2c_adapter adap;
>+ struct i2c_msg *msg;
>+ struct completion
Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On June 30, 2018 3:33:29 PM GMT+02:00, Manivannan Sadhasivam
wrote:
>Add Actions Semiconductor Owl family S900 I2C driver.
>
>Signed-off-by: Manivannan Sadhasivam
>---
> drivers/i2c/busses/Kconfig | 7 +
> drivers/i2c/busses/Makefile | 1 +
> drivers/i2c/busses/i2c-owl.c | 477 +++
> 3 files changed, 485 insertions(+)
> create mode 100644 drivers/i2c/busses/i2c-owl.c
>
>diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>index 4f8df2ec87b1..8c8025f87ce4 100644
>--- a/drivers/i2c/busses/Kconfig
>+++ b/drivers/i2c/busses/Kconfig
>@@ -762,6 +762,13 @@ config I2C_OMAP
> Like OMAP1510/1610/1710/5912 and OMAP242x.
> For details see http://www.ti.com/omap.
>
>+config I2C_OWL
>+ tristate "Actions Semiconductor Owl I2C Controller"
>+ depends on ARCH_ACTIONS || COMPILE_TEST
>+ help
>+Say Y here if you want to use the I2C bus controller on
>+the Actions Semiconductor Owl SoC's.
>+
> config I2C_PASEMI
> tristate "PA Semi SMBus interface"
> depends on PPC_PASEMI && PCI
>diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
>index 5a869144a0c5..b71618f77880 100644
>--- a/drivers/i2c/busses/Makefile
>+++ b/drivers/i2c/busses/Makefile
>@@ -76,6 +76,7 @@ obj-$(CONFIG_I2C_MXS)+= i2c-mxs.o
> obj-$(CONFIG_I2C_NOMADIK) += i2c-nomadik.o
> obj-$(CONFIG_I2C_OCORES) += i2c-ocores.o
> obj-$(CONFIG_I2C_OMAP)+= i2c-omap.o
>+obj-$(CONFIG_I2C_OWL) += i2c-owl.o
> obj-$(CONFIG_I2C_PASEMI) += i2c-pasemi.o
> obj-$(CONFIG_I2C_PCA_PLATFORM)+= i2c-pca-platform.o
> obj-$(CONFIG_I2C_PMCMSP) += i2c-pmcmsp.o
>diff --git a/drivers/i2c/busses/i2c-owl.c
>b/drivers/i2c/busses/i2c-owl.c
>new file mode 100644
>index ..144a249bf994
>--- /dev/null
>+++ b/drivers/i2c/busses/i2c-owl.c
>@@ -0,0 +1,477 @@
>+// SPDX-License-Identifier: GPL-2.0+
>+/*
>+ * Actions Semiconductor Owl SoC's I2C driver
>+ *
>+ * Copyright (c) 2014 Actions Semi Inc.
>+ * Author: David Liu
>+ *
>+ * Copyright (c) 2018 Linaro Ltd.
>+ * Author: Manivannan Sadhasivam
>+ */
>+
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+#include
>+
>+/* I2C registers */
>+#define OWL_I2C_REG_CTL 0x
>+#define OWL_I2C_REG_CLKDIV0x0004
>+#define OWL_I2C_REG_STAT 0x0008
>+#define OWL_I2C_REG_ADDR 0x000C
>+#define OWL_I2C_REG_TXDAT 0x0010
>+#define OWL_I2C_REG_RXDAT 0x0014
>+#define OWL_I2C_REG_CMD 0x0018
>+#define OWL_I2C_REG_FIFOCTL 0x001C
>+#define OWL_I2C_REG_FIFOSTAT 0x0020
>+#define OWL_I2C_REG_DATCNT0x0024
>+#define OWL_I2C_REG_RCNT 0x0028
>+
>+/* I2Cx_CTL Bit Mask */
>+#define OWL_I2C_CTL_RBBIT(1)
>+#define OWL_I2C_CTL_GBCC(x) (((x) & 0x3) << 2)
>+#define OWL_I2C_CTL_GBCC_NONE OWL_I2C_CTL_GBCC(0)
>+#define OWL_I2C_CTL_GBCC_START OWL_I2C_CTL_GBCC(1)
>+#define OWL_I2C_CTL_GBCC_STOP OWL_I2C_CTL_GBCC(2)
>+#define OWL_I2C_CTL_GBCC_RSTART OWL_I2C_CTL_GBCC(3)
>+#define OWL_I2C_CTL_IRQE BIT(5)
>+#define OWL_I2C_CTL_ENBIT(7)
>+#define OWL_I2C_CTL_AEBIT(8)
>+#define OWL_I2C_CTL_SHSM BIT(10)
>+
>+#define OWL_I2C_DIV_FACTOR(x) ((x) & 0xff)
>+
>+/* I2Cx_STAT Bit Mask */
>+#define OWL_I2C_STAT_RACK BIT(0)
>+#define OWL_I2C_STAT_BEB BIT(1)
>+#define OWL_I2C_STAT_IRQP BIT(2)
>+#define OWL_I2C_STAT_LAB BIT(3)
>+#define OWL_I2C_STAT_STPD BIT(4)
>+#define OWL_I2C_STAT_STAD BIT(5)
>+#define OWL_I2C_STAT_BBB BIT(6)
>+#define OWL_I2C_STAT_TCB BIT(7)
>+#define OWL_I2C_STAT_LBST BIT(8)
>+#define OWL_I2C_STAT_SAMB BIT(9)
>+#define OWL_I2C_STAT_SRGC BIT(10)
>+
>+/* I2Cx_CMD Bit Mask */
>+#define OWL_I2C_CMD_SBE BIT(0)
>+#define OWL_I2C_CMD_RBE BIT(4)
>+#define OWL_I2C_CMD_DEBIT(8)
>+#define OWL_I2C_CMD_NSBIT(9)
>+#define OWL_I2C_CMD_SEBIT(10)
>+#define OWL_I2C_CMD_MSS BIT(11)
>+#define OWL_I2C_CMD_WRS BIT(12)
>+#define OWL_I2C_CMD_SECL BIT(15)
>+
>+#define OWL_I2C_CMD_AS(x) (((x) & 0x7) << 1)
>+#define OWL_I2C_CMD_SAS(x)(((x) & 0x7) << 5)
>+
>+/* I2Cx_FIFOCTL Bit Mask */
>+#define OWL_I2C_FIFOCTL_NIB BIT(0)
>+#define OWL_I2C_FIFOCTL_RFR BIT(1)
>+#define OWL_I2C_FIFOCTL_TFR BIT(2)
>+
>+/* I2Cc_FIFOSTAT Bit Mask */
>+#define OWL_I2C_FIFOSTAT_RNB BIT(1)
>+#define OWL_I2C_FIFOSTAT_RFE BIT(2)
>+#define OWL_I2C_FIFOSTAT_TFF BIT(5)
>+#define OWL_I2C_FIFOSTAT_TFD GENMASK(23, 16)
>+#define OWL_I2C_FIFOSTAT_RFD GENMASK(15, 8)
>+
>+/* I2C bus timeout */
>+#define OWL_I2C_TIMEOUT msecs_to_jiffies(4 * 1000)
>+
>+#define OWL_I2C_MAX_RETRIES 50
>+
>+#define OWL_I2C_DEF_SPEED_HZ 10
>+#define OWL_I2C_MAX_SPEED_HZ 40
>+
>+struct owl_i2c_dev {
>+ struct i2c_adapter adap;
>+ struct i2c_msg *msg;
>+ struct completion
Re: [PATCH v2] PCI/AER: Fix aerdrv loading with "pcie_ports=native" parameter
On 06/30/2018 04:31 PM, Bjorn Helgaas wrote: [+cc Borislav, linux-acpi, since this involves APEI/HEST] Borislav is not the relevant maintainer here, since we're not contingent on APEI handling. I think Keith has a lot more experience with this part of the kernel. On Tue, Jun 19, 2018 at 02:58:20PM -0500, Alexandru Gagniuc wrote: According to the documentation, "pcie_ports=native", linux should use native AER and DPC services. While that is true for the _OSC method parsing, this is not the only place that is checked. Should the HEST table list PCIe ports as firmware-first, linux will not use native services. Nothing in ACPI-land looks at pcie_ports_native. How should ACPI things work in the "pcie_ports=native" case? I guess we still have to expect to receive error records from the firmware, because it may certainly send us non-PCI errors (machine checks, etc) and maybe even some PCI errors (even if the Linux AER driver claims AER interrupts, we don't know what notification mechanisms the firmware may be using). I think ACPI land shouldn't care about this. We care about it from the PCIe stand point at the interface with ACPI. FW might see a delta in the sense that we request control of some features via _OSC, which we otherwise would not do without pcie_ports=native. I guess best-case, we'll get ACPI error records for all non-PCI things, and the Linux AER driver will see all the AER errors. It might affect FW's ability to catch errors, but that's dependent on the root port implementation. Worst-case, I don't really know what to expect. Duplicate reporting of AER errors via firmware and Linux AER driver? Some kind of confusion about who acknowledges and clears them? Once user enters pcie_ports=native, all bets are off: you broke the contract you have with the FW -- whether or not you have this patch. Out of curiosity, what is your use case for "pcie_ports=native"? Presumably there's something that works better when using it, and things work even *better* with this patch? Corectness. It bothers me that actual behavior does not match the documentation: native Use native PCIe services associated with PCIe ports unconditionally. I know people do use it, because I often see it mentioned in forums and bug reports, but I really don't expect it to work very well because we're ignoring the usage model the firmware is designed around. My unproven suspicion is that most uses are in the black magic category of "there's a bug here, and we don't know how to fix it, but pcie_ports=native makes it work better". There exist cases that firmware didn't consider. I would not call them "firmware bugs", but there are cases where the user understands the platform better than firmware. Example: on certain PCIe switches, a hardware PCIe error may bring the switch downstream ports into a state where they stop notifying hotplug events. Depending on the platform, firmware may or may not fix this condition, but "pcie_ports=native" enables DPC. DPC contains the error without the switch downstream port entering the weird error state in the first place. All bets are off at this point. Obviously I would much rather find and fix those bugs so people wouldn't have to stumble over the problem in the first place. Using native services when firmware asks us not to is a crapshoot every time. I don't condone the use of this feature, but if we do get a pcie_ports=native request, we should at least honor it. This happens because aer_acpi_firmware_first() doesn't take 'pcie_ports' into account. This is wrong. DPC uses the same logic when it decides whether to load or not, so fixing this also fixes DPC not loading. Signed-off-by: Alexandru Gagniuc --- drivers/pci/pcie/aer.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) Changes since v1: - Re-tested with latest and greatest (v4.18-rc1) -- works great diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index a2e88386af28..98ced0f7c850 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -291,7 +291,7 @@ static void aer_set_firmware_first(struct pci_dev *pci_dev) rc = apei_hest_parse(aer_hest_parse, ); - if (rc) + if (rc || pcie_ports_native) pci_dev->__aer_firmware_first = 0; else pci_dev->__aer_firmware_first = info.firmware_first; @@ -327,6 +327,9 @@ bool aer_acpi_firmware_first(void) apei_hest_parse(aer_hest_parse, ); aer_firmware_first = info.firmware_first; parsed = true; + if (pcie_ports_native) + aer_firmware_first = 0; + } return aer_firmware_first; } -- 2.14.3
Re: [PATCH v2] PCI/AER: Fix aerdrv loading with "pcie_ports=native" parameter
On 06/30/2018 04:31 PM, Bjorn Helgaas wrote: [+cc Borislav, linux-acpi, since this involves APEI/HEST] Borislav is not the relevant maintainer here, since we're not contingent on APEI handling. I think Keith has a lot more experience with this part of the kernel. On Tue, Jun 19, 2018 at 02:58:20PM -0500, Alexandru Gagniuc wrote: According to the documentation, "pcie_ports=native", linux should use native AER and DPC services. While that is true for the _OSC method parsing, this is not the only place that is checked. Should the HEST table list PCIe ports as firmware-first, linux will not use native services. Nothing in ACPI-land looks at pcie_ports_native. How should ACPI things work in the "pcie_ports=native" case? I guess we still have to expect to receive error records from the firmware, because it may certainly send us non-PCI errors (machine checks, etc) and maybe even some PCI errors (even if the Linux AER driver claims AER interrupts, we don't know what notification mechanisms the firmware may be using). I think ACPI land shouldn't care about this. We care about it from the PCIe stand point at the interface with ACPI. FW might see a delta in the sense that we request control of some features via _OSC, which we otherwise would not do without pcie_ports=native. I guess best-case, we'll get ACPI error records for all non-PCI things, and the Linux AER driver will see all the AER errors. It might affect FW's ability to catch errors, but that's dependent on the root port implementation. Worst-case, I don't really know what to expect. Duplicate reporting of AER errors via firmware and Linux AER driver? Some kind of confusion about who acknowledges and clears them? Once user enters pcie_ports=native, all bets are off: you broke the contract you have with the FW -- whether or not you have this patch. Out of curiosity, what is your use case for "pcie_ports=native"? Presumably there's something that works better when using it, and things work even *better* with this patch? Corectness. It bothers me that actual behavior does not match the documentation: native Use native PCIe services associated with PCIe ports unconditionally. I know people do use it, because I often see it mentioned in forums and bug reports, but I really don't expect it to work very well because we're ignoring the usage model the firmware is designed around. My unproven suspicion is that most uses are in the black magic category of "there's a bug here, and we don't know how to fix it, but pcie_ports=native makes it work better". There exist cases that firmware didn't consider. I would not call them "firmware bugs", but there are cases where the user understands the platform better than firmware. Example: on certain PCIe switches, a hardware PCIe error may bring the switch downstream ports into a state where they stop notifying hotplug events. Depending on the platform, firmware may or may not fix this condition, but "pcie_ports=native" enables DPC. DPC contains the error without the switch downstream port entering the weird error state in the first place. All bets are off at this point. Obviously I would much rather find and fix those bugs so people wouldn't have to stumble over the problem in the first place. Using native services when firmware asks us not to is a crapshoot every time. I don't condone the use of this feature, but if we do get a pcie_ports=native request, we should at least honor it. This happens because aer_acpi_firmware_first() doesn't take 'pcie_ports' into account. This is wrong. DPC uses the same logic when it decides whether to load or not, so fixing this also fixes DPC not loading. Signed-off-by: Alexandru Gagniuc --- drivers/pci/pcie/aer.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) Changes since v1: - Re-tested with latest and greatest (v4.18-rc1) -- works great diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c index a2e88386af28..98ced0f7c850 100644 --- a/drivers/pci/pcie/aer.c +++ b/drivers/pci/pcie/aer.c @@ -291,7 +291,7 @@ static void aer_set_firmware_first(struct pci_dev *pci_dev) rc = apei_hest_parse(aer_hest_parse, ); - if (rc) + if (rc || pcie_ports_native) pci_dev->__aer_firmware_first = 0; else pci_dev->__aer_firmware_first = info.firmware_first; @@ -327,6 +327,9 @@ bool aer_acpi_firmware_first(void) apei_hest_parse(aer_hest_parse, ); aer_firmware_first = info.firmware_first; parsed = true; + if (pcie_ports_native) + aer_firmware_first = 0; + } return aer_firmware_first; } -- 2.14.3
Re: [PATCH v2] iio: humidity: hts221: Fix sensor reads after resume
On Thu, 2018-06-28 at 11:05 +0200, Lorenzo Bianconi wrote:
> > AV_CONF register (RH & TEMP. oversampling ratio's) and CTRL1 register
> > (ODR & BDU settings) values are lost after suspend.
> >
> > While the change in AV_CONF updates the sensor resolution modes
> > (overriding the user configuration before the device went to suspend);
> > loss of the contents of the CTRL1 register leads to failure in reading
> > sensor output.
> >
> > This patch restores the AV_CONF & CTRL1 registers after resume.
> >
>
> Hi Shrirang,
>
> I still suspect we have some hw issue (IoT Gateway) here (cc ST folks
> to get more info) and anyway I guess the proposed patch will fix the issue
> completely since OD and HL configuration is not restored properly.
> Could you please double check HL/OD regs are properly preserved after resume
> phase?
Yes, certainly. Will include the i2c register dumps before and after the resume
to support
the patch.
Regards,
Shrirang
>
> Regards,
> Lorenzo
>
> > Changes from v1:
> > - Don't drop enabling the sensor during resume
> > - Restore AV_CONF register along with CTRL1
> > (As demonstrated with i2c register dumps of hts221 captured on Dell
> > IoT Gateways 300x before suspend & after resume [1])
> >
> > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR
> > reconfiguration)
> > Signed-off-by: Shrirang Bagul
> >
> > [v1] https://marc.info/?l=linux-iio=152506543000442=2
> > [1] https://marc.info/?l=linux-iio=152534455701742=2
> >
> > This patch is based on:
> > git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git -b testing
> > ---
> > drivers/iio/humidity/hts221_core.c | 31 +-
> > 1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/humidity/hts221_core.c
> > b/drivers/iio/humidity/hts221_core.c
> > index 166946d4978d..fc5599497f55 100644
> > --- a/drivers/iio/humidity/hts221_core.c
> > +++ b/drivers/iio/humidity/hts221_core.c
> > @@ -656,14 +656,43 @@ static int __maybe_unused hts221_resume(struct device
> > *dev)
> > {
> > struct iio_dev *iio_dev = dev_get_drvdata(dev);
> > struct hts221_hw *hw = iio_priv(iio_dev);
> > + const struct hts221_avg *avg;
> > + u8 data, idx;
> > int err = 0;
> >
> > + /* Restore contents of AV_CONF (RH & TEMP. oversampling ratio's) */
> > + avg = _avg_list[HTS221_SENSOR_H];
> > + idx = hw->sensors[HTS221_SENSOR_H].cur_avg_idx;
> > + data = avg->avg_avl[idx];
> > + err = hts221_update_avg(hw, HTS221_SENSOR_H, data);
> > + if (err < 0)
> > + goto fail_err;
> > +
> > + avg = _avg_list[HTS221_SENSOR_T];
> > + idx = hw->sensors[HTS221_SENSOR_T].cur_avg_idx;
> > + data = avg->avg_avl[idx];
> > + err = hts221_update_avg(hw, HTS221_SENSOR_T, data);
> > + if (err < 0)
> > + goto fail_err;
>
> just return err, no need for goto statement
Okay.
>
> > +
> > + /* Restore contents of CTRL1 (BDU & ODR) */
> > + err = regmap_update_bits(hw->regmap, HTS221_REG_CNTRL1_ADDR,
> > +HTS221_BDU_MASK,
> > +FIELD_PREP(HTS221_BDU_MASK, 1));
> > + if (err < 0)
> > + goto fail_err;
> > +
> > + err = hts221_update_odr(hw, hw->odr);
> > + if (err < 0)
> > + goto fail_err;
> > +
>
> just return err, no need for goto statement
Okay.
>
> > if (hw->enabled)
> > err = regmap_update_bits(hw->regmap, HTS221_REG_CNTRL1_ADDR,
> > HTS221_ENABLE_MASK,
> > FIELD_PREP(HTS221_ENABLE_MASK,
> > true));
> > - return err;
> > +fail_err:
> > + return err < 0 ? err : 0;
> > }
> >
> > const struct dev_pm_ops hts221_pm_ops = {
> > --
> > 2.17.1
> >
signature.asc
Description: This is a digitally signed message part
Re: [PATCH v2] iio: humidity: hts221: Fix sensor reads after resume
On Thu, 2018-06-28 at 11:05 +0200, Lorenzo Bianconi wrote:
> > AV_CONF register (RH & TEMP. oversampling ratio's) and CTRL1 register
> > (ODR & BDU settings) values are lost after suspend.
> >
> > While the change in AV_CONF updates the sensor resolution modes
> > (overriding the user configuration before the device went to suspend);
> > loss of the contents of the CTRL1 register leads to failure in reading
> > sensor output.
> >
> > This patch restores the AV_CONF & CTRL1 registers after resume.
> >
>
> Hi Shrirang,
>
> I still suspect we have some hw issue (IoT Gateway) here (cc ST folks
> to get more info) and anyway I guess the proposed patch will fix the issue
> completely since OD and HL configuration is not restored properly.
> Could you please double check HL/OD regs are properly preserved after resume
> phase?
Yes, certainly. Will include the i2c register dumps before and after the resume
to support
the patch.
Regards,
Shrirang
>
> Regards,
> Lorenzo
>
> > Changes from v1:
> > - Don't drop enabling the sensor during resume
> > - Restore AV_CONF register along with CTRL1
> > (As demonstrated with i2c register dumps of hts221 captured on Dell
> > IoT Gateways 300x before suspend & after resume [1])
> >
> > Fixes: ffebe74b7c95 (iio: humidity: hts221: avoid useless ODR
> > reconfiguration)
> > Signed-off-by: Shrirang Bagul
> >
> > [v1] https://marc.info/?l=linux-iio=152506543000442=2
> > [1] https://marc.info/?l=linux-iio=152534455701742=2
> >
> > This patch is based on:
> > git://git.kernel.org/pub/scm/linux/kernel/git/jic23/iio.git -b testing
> > ---
> > drivers/iio/humidity/hts221_core.c | 31 +-
> > 1 file changed, 30 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/iio/humidity/hts221_core.c
> > b/drivers/iio/humidity/hts221_core.c
> > index 166946d4978d..fc5599497f55 100644
> > --- a/drivers/iio/humidity/hts221_core.c
> > +++ b/drivers/iio/humidity/hts221_core.c
> > @@ -656,14 +656,43 @@ static int __maybe_unused hts221_resume(struct device
> > *dev)
> > {
> > struct iio_dev *iio_dev = dev_get_drvdata(dev);
> > struct hts221_hw *hw = iio_priv(iio_dev);
> > + const struct hts221_avg *avg;
> > + u8 data, idx;
> > int err = 0;
> >
> > + /* Restore contents of AV_CONF (RH & TEMP. oversampling ratio's) */
> > + avg = _avg_list[HTS221_SENSOR_H];
> > + idx = hw->sensors[HTS221_SENSOR_H].cur_avg_idx;
> > + data = avg->avg_avl[idx];
> > + err = hts221_update_avg(hw, HTS221_SENSOR_H, data);
> > + if (err < 0)
> > + goto fail_err;
> > +
> > + avg = _avg_list[HTS221_SENSOR_T];
> > + idx = hw->sensors[HTS221_SENSOR_T].cur_avg_idx;
> > + data = avg->avg_avl[idx];
> > + err = hts221_update_avg(hw, HTS221_SENSOR_T, data);
> > + if (err < 0)
> > + goto fail_err;
>
> just return err, no need for goto statement
Okay.
>
> > +
> > + /* Restore contents of CTRL1 (BDU & ODR) */
> > + err = regmap_update_bits(hw->regmap, HTS221_REG_CNTRL1_ADDR,
> > +HTS221_BDU_MASK,
> > +FIELD_PREP(HTS221_BDU_MASK, 1));
> > + if (err < 0)
> > + goto fail_err;
> > +
> > + err = hts221_update_odr(hw, hw->odr);
> > + if (err < 0)
> > + goto fail_err;
> > +
>
> just return err, no need for goto statement
Okay.
>
> > if (hw->enabled)
> > err = regmap_update_bits(hw->regmap, HTS221_REG_CNTRL1_ADDR,
> > HTS221_ENABLE_MASK,
> > FIELD_PREP(HTS221_ENABLE_MASK,
> > true));
> > - return err;
> > +fail_err:
> > + return err < 0 ? err : 0;
> > }
> >
> > const struct dev_pm_ops hts221_pm_ops = {
> > --
> > 2.17.1
> >
signature.asc
Description: This is a digitally signed message part
/tmp/ccqW4TZ5.s:35: Error: .err encountered
Hi Dmitry, FYI, the error/warning still remains. tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 883c9ab9eb595f8542d01e55d29a346c8d96862e commit: 758517202bd2e427664857c9f2aa59da36848aca arm: port KCOV to arm date: 2 weeks ago config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 758517202bd2e427664857c9f2aa59da36848aca # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm All errors (new ones prefixed by >>): /tmp/ccqW4TZ5.s: Assembler messages: >> /tmp/ccqW4TZ5.s:35: Error: .err encountered /tmp/ccqW4TZ5.s:36: Error: .err encountered /tmp/ccqW4TZ5.s:37: Error: .err encountered --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
/tmp/ccqW4TZ5.s:35: Error: .err encountered
Hi Dmitry, FYI, the error/warning still remains. tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 883c9ab9eb595f8542d01e55d29a346c8d96862e commit: 758517202bd2e427664857c9f2aa59da36848aca arm: port KCOV to arm date: 2 weeks ago config: arm-allmodconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 758517202bd2e427664857c9f2aa59da36848aca # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=arm All errors (new ones prefixed by >>): /tmp/ccqW4TZ5.s: Assembler messages: >> /tmp/ccqW4TZ5.s:35: Error: .err encountered /tmp/ccqW4TZ5.s:36: Error: .err encountered /tmp/ccqW4TZ5.s:37: Error: .err encountered --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, Jun 30, 2018 at 8:42 PM Benjamin Herrenschmidt wrote: > > But what if something *else* still holds a reference to the kobject ? > It could be anything really... t But that's fine. Then the object will continue to exist, and the sysfs file will continue to exist, and you won't get a new glue directory. In fact, what you describe is a problem with *your* patch, exactly because you introduce another counter that is *not* the reference count, and now you have the problem with "old directory kobject is still live, but I removed the sysfs part, and now I'm creating a new object with the same name". Hmm? > It is and there's a WARN_ON about it inside kobject_get(). I don't > think anybody argues against that, you are absolutely right. No. That the zero kobject_get() will not result in a warning. It just does a kref_get(), no warnings anywhere. Yes, there is a kobject_get() warning, but that's about an _uninitialized_ kobject, not a already released one! You will get no warning that I can see from the "oh, you just got a stale one". Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, Jun 30, 2018 at 8:42 PM Benjamin Herrenschmidt wrote: > > But what if something *else* still holds a reference to the kobject ? > It could be anything really... t But that's fine. Then the object will continue to exist, and the sysfs file will continue to exist, and you won't get a new glue directory. In fact, what you describe is a problem with *your* patch, exactly because you introduce another counter that is *not* the reference count, and now you have the problem with "old directory kobject is still live, but I removed the sysfs part, and now I'm creating a new object with the same name". Hmm? > It is and there's a WARN_ON about it inside kobject_get(). I don't > think anybody argues against that, you are absolutely right. No. That the zero kobject_get() will not result in a warning. It just does a kref_get(), no warnings anywhere. Yes, there is a kobject_get() warning, but that's about an _uninitialized_ kobject, not a already released one! You will get no warning that I can see from the "oh, you just got a stale one". Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
(This is a resend with lkml added for background & archival purposes) On Sat, 2018-06-30 at 12:29 -0700, Linus Torvalds wrote: > On Thu, Jun 28, 2018 at 7:19 PM Benjamin Herrenschmidt > wrote: > > > > For devices with a class, we create a "glue" directory between > > the parent device and the new device with the class name. > > > > This directory is never "explicitely" removed when empty however, > > this is left to the implicit sysfs removal done by kobjects when > > they are released on the last kobject_put(). > > > > This is problematic because as long as it's not been removed from > > sysfs, it is still present in the class kset and in sysfs directory > > structure. > > Ok, so I don't hate the patch per se, but looking around, I think this > is still wrong in the end. > > Why? > > Because normally, the last kobject_put() really does do a synchronous > kobject_del(). So normally, this is all completely pointless, and the > normal kobject lifetime rules should be sufficient. Not entirely sadly There's a small race between the kref going down to 0 and the last kobject_put(). Something might still "catch" the 0-reference object during that window. In this specific case our bacon is mostly saved by the gdp_mutex which is taken by cleanup_glue_dir() around what *should* be the lats kobject_put but what if it isn't ? If anything else happens to hold a reference to the directory object (open files in sysfs maybe ?), then the last put will come from elsewhere and will happen without that mutex being held, thus re- opening the tiny race. Is this possible ? > So I *think* your problem happens because you have > CONFIG_DEBUG_KOBJECT_RELEASE enabled, and that intentionally delays > the cleanup. I think it just opens an existing race more widely. The race always exist becaues another CPU can observe the object between the reference going to 0 and the last kobject_del done by kobject_release. That's one main reason why I dislike this "auto-clean" mechanism. One other way to solve it, which I just thought about, could be to, inside kobject_put() itself, check that the reference is *1* and do kobject_del() before the last kref_put. That does mean that somebody can snatch it in that window after it's been removed from sysfs though, is that ok ? It won't crash I suppose... > This is actually not really what DEBUG_KOBJECT_RELEASE is really > documented to do. It is documented as a "let's debug problems where > drivers think deletion is immediate", but the sysfs interaction with > the same-name issue really smells different. > > So what the patch does is basically to just fight > DEBUG_KOBJECT_RELEASE delaying rules, and that kind of stinks. Not entirely, it fight an existing race that DEBUG_KOBJECT_RELEASE just opens more widely. > To me, it really feels like either we should see the > DEBUG_KOBJECT_RELEASE rules are "real" (in which case fighting them is > wrong), or we should admit that DEBUG_KOBJECT_RELEASE causes problems > (in which case we should probably try to fix the debug aid). > > Ben, can you confirm that your problem just goes away if you don't > select DEBUG_KOBJECT_RELEASE? The easily reproducable crash goes away, because the device I've been observing these is a tiny slow single core ARM embedded thing. But I'm not sure the therical race is solved. > Greg - comments? The pattern of "remove last device, add a new device > of the same class" really seems to be a valid pattern, and > CONFIG_DEBUG_KOBJECT_RELEASE seems to actively break it. > > Could we perhaps add a synthetic test for exactly this pattern (add a > silly device with a bogus class, remove it, and add another device > with the same class)? > > Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
(This is a resend with lkml added for background & archival purposes) On Sat, 2018-06-30 at 12:29 -0700, Linus Torvalds wrote: > On Thu, Jun 28, 2018 at 7:19 PM Benjamin Herrenschmidt > wrote: > > > > For devices with a class, we create a "glue" directory between > > the parent device and the new device with the class name. > > > > This directory is never "explicitely" removed when empty however, > > this is left to the implicit sysfs removal done by kobjects when > > they are released on the last kobject_put(). > > > > This is problematic because as long as it's not been removed from > > sysfs, it is still present in the class kset and in sysfs directory > > structure. > > Ok, so I don't hate the patch per se, but looking around, I think this > is still wrong in the end. > > Why? > > Because normally, the last kobject_put() really does do a synchronous > kobject_del(). So normally, this is all completely pointless, and the > normal kobject lifetime rules should be sufficient. Not entirely sadly There's a small race between the kref going down to 0 and the last kobject_put(). Something might still "catch" the 0-reference object during that window. In this specific case our bacon is mostly saved by the gdp_mutex which is taken by cleanup_glue_dir() around what *should* be the lats kobject_put but what if it isn't ? If anything else happens to hold a reference to the directory object (open files in sysfs maybe ?), then the last put will come from elsewhere and will happen without that mutex being held, thus re- opening the tiny race. Is this possible ? > So I *think* your problem happens because you have > CONFIG_DEBUG_KOBJECT_RELEASE enabled, and that intentionally delays > the cleanup. I think it just opens an existing race more widely. The race always exist becaues another CPU can observe the object between the reference going to 0 and the last kobject_del done by kobject_release. That's one main reason why I dislike this "auto-clean" mechanism. One other way to solve it, which I just thought about, could be to, inside kobject_put() itself, check that the reference is *1* and do kobject_del() before the last kref_put. That does mean that somebody can snatch it in that window after it's been removed from sysfs though, is that ok ? It won't crash I suppose... > This is actually not really what DEBUG_KOBJECT_RELEASE is really > documented to do. It is documented as a "let's debug problems where > drivers think deletion is immediate", but the sysfs interaction with > the same-name issue really smells different. > > So what the patch does is basically to just fight > DEBUG_KOBJECT_RELEASE delaying rules, and that kind of stinks. Not entirely, it fight an existing race that DEBUG_KOBJECT_RELEASE just opens more widely. > To me, it really feels like either we should see the > DEBUG_KOBJECT_RELEASE rules are "real" (in which case fighting them is > wrong), or we should admit that DEBUG_KOBJECT_RELEASE causes problems > (in which case we should probably try to fix the debug aid). > > Ben, can you confirm that your problem just goes away if you don't > select DEBUG_KOBJECT_RELEASE? The easily reproducable crash goes away, because the device I've been observing these is a tiny slow single core ARM embedded thing. But I'm not sure the therical race is solved. > Greg - comments? The pattern of "remove last device, add a new device > of the same class" really seems to be a valid pattern, and > CONFIG_DEBUG_KOBJECT_RELEASE seems to actively break it. > > Could we perhaps add a synthetic test for exactly this pattern (add a > silly device with a bogus class, remove it, and add another device > with the same class)? > > Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, 2018-06-30 at 19:18 -0700, Linus Torvalds wrote: > On Sat, Jun 30, 2018 at 7:07 PM Linus Torvalds > wrote: > > > > Those locks won't protect kobj races in _general_ (ie there is no > > locking between two totally unrelated buses), but they *should* > > serialize the case of a device being added within one class. No? > > Side note: there had *better* be some locking whenever there is a way > to find an object, because otherwise you have a fundamental lifetime > problem: one thread finding the object at the same time another thread > frees it for the last time. Even the "unless_zero()" won't fix it, > because the final free will release the underlying object itself, so > the "zero" state is ephemeral. Right. We agree. The problem here relates to that built-in cleanup in kobject_release() which violates that rule: it removes the object from sysfs (and from any kset) after the reference has been dropped. Users such as the gluedir code try to workaround this by using outer mutexes or locks, but it's fundamentally racy if there's any chance that another 3rd party holds the last reference, since in that case, the cleanup happens outside of any added locking. That's why I tend to think that anything that relies on that "late removal from sysfs" is broken by design :) Anything that uses kobject should ensure they are taken out of sysfs and their respective ksets before the last reference drops. Now it's probably all fine for most cases, I assume sysfs itself checks the reference to be non-0 in lookup path (I haven't checked), at least to prevent use-after-free. This is still open to the duplicate name issue. The bigger issue of use-after-free only happens in the rare cases where there's a separate list being maintained and cleaned up late. The gluedir is such an example because it "manually" thrawls the kset list. So you are right, kobject_get should probably always be _unless_zero, but my point is we even with that, we probably still need to ensure the gluedir is taken out of the kset before we drop the last reference, so this can be done with the appropriate upper layer subsystem locking, and thus without racing with a new device being bound. > That locking might be just RCU during lookup, and rcu-delaying the > release, of course. I think that's all the sysfs code needs, for > example, since that's what lookup uses. > > And for any other embedded kobj cases, where you can reach the object > using some random subsystem pointers, there had better be other > locking in place for that pointer lookup vs the last removal. > > kobject itself doesn't provide that locking, it only provides the > reference counting. But that's partly why it really has to disallow > any kobject_get() of a zero object, because it means that the > tear-down has been started, but the tear-down itself may not have had > time to get the lock yet (ie kobject_release() may be just about to > call the t->release() function). > > But maybe I'm missing something subtle. > >Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, 2018-06-30 at 19:18 -0700, Linus Torvalds wrote: > On Sat, Jun 30, 2018 at 7:07 PM Linus Torvalds > wrote: > > > > Those locks won't protect kobj races in _general_ (ie there is no > > locking between two totally unrelated buses), but they *should* > > serialize the case of a device being added within one class. No? > > Side note: there had *better* be some locking whenever there is a way > to find an object, because otherwise you have a fundamental lifetime > problem: one thread finding the object at the same time another thread > frees it for the last time. Even the "unless_zero()" won't fix it, > because the final free will release the underlying object itself, so > the "zero" state is ephemeral. Right. We agree. The problem here relates to that built-in cleanup in kobject_release() which violates that rule: it removes the object from sysfs (and from any kset) after the reference has been dropped. Users such as the gluedir code try to workaround this by using outer mutexes or locks, but it's fundamentally racy if there's any chance that another 3rd party holds the last reference, since in that case, the cleanup happens outside of any added locking. That's why I tend to think that anything that relies on that "late removal from sysfs" is broken by design :) Anything that uses kobject should ensure they are taken out of sysfs and their respective ksets before the last reference drops. Now it's probably all fine for most cases, I assume sysfs itself checks the reference to be non-0 in lookup path (I haven't checked), at least to prevent use-after-free. This is still open to the duplicate name issue. The bigger issue of use-after-free only happens in the rare cases where there's a separate list being maintained and cleaned up late. The gluedir is such an example because it "manually" thrawls the kset list. So you are right, kobject_get should probably always be _unless_zero, but my point is we even with that, we probably still need to ensure the gluedir is taken out of the kset before we drop the last reference, so this can be done with the appropriate upper layer subsystem locking, and thus without racing with a new device being bound. > That locking might be just RCU during lookup, and rcu-delaying the > release, of course. I think that's all the sysfs code needs, for > example, since that's what lookup uses. > > And for any other embedded kobj cases, where you can reach the object > using some random subsystem pointers, there had better be other > locking in place for that pointer lookup vs the last removal. > > kobject itself doesn't provide that locking, it only provides the > reference counting. But that's partly why it really has to disallow > any kobject_get() of a zero object, because it means that the > tear-down has been started, but the tear-down itself may not have had > time to get the lock yet (ie kobject_release() may be just about to > call the t->release() function). > > But maybe I'm missing something subtle. > >Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, 2018-06-30 at 19:07 -0700, Linus Torvalds wrote: > [ Ben - feel free to post the missing emails to lkml too ] > > On Sat, Jun 30, 2018 at 6:56 PM Benjamin Herrenschmidt > wrote: > > > > On Sat, 2018-06-30 at 12:29 -0700, Linus Torvalds wrote: > > > > > > Because normally, the last kobject_put() really does do a synchronous > > > kobject_del(). So normally, this is all completely pointless, and the > > > normal kobject lifetime rules should be sufficient. > > > > Not entirely sadly > > > > There's a small race between the kref going down to 0 and the last > > kobject_put(). Something might still "catch" the 0-reference object > > during that window. > > That's only true if the underlying subsystem doesn't have any > serialization itself. > > Which I don't think is normal. > > IOW, if you have (say) a PCI hotplug model, the PCI layer will have > the pci_hp_mutex, for example, which protects the PCI hotplug slot > lists and the kobj things. > > Those locks won't protect kobj races in _general_ (ie there is no > locking between two totally unrelated buses), but they *should* > serialize the case of a device being added within one class. No? > > If not, maybe we need to add some class-based serialization. Well, yes and no ... yes I agree there should be upper level serialization such that you can't hit the "add" path until the "del" path has completed. And this will work as long as the kobject_put done at the end of "del" is indeed the very last one. But what if something *else* still holds a reference to the kobject ? It could be anything really... the driver itself, that is now unbound, might have a client that's holding a stale reference because some chardev is still open, for example. I don't know if sysfs can also hold one because a users still has an open directory there, but in the general case I don't thing one can assume that the kobject_put() done by the end of the "owner" subsystem delete path (in our example device_del) is the very last kobject_put. And since some other random thing might be doing that last put, it may also do it without any of the upper level locking/serialization. Which is the reason I so dislike the whole "cleanup sysfs in the last kobject_put" we do in kobject_release(). This is also why we clearly differenciate in the device model device_del from the final device_put etc... We generally separeate "visibility to the outside world" from the actual object lifetime, the latter being what the kref controls. .. except when we dont, such as this gluedir case. And I think that's wrong in principle. This is the reason why I prefer the approach of explicitely doing the kobject_del() instead of kobject_put() in the tail of device_del (what this 2/2 patch does), because it restores that separation between the object being "active/visible" or not from the actual lifetime of the structure. > > I think it just opens an existing race more widely. The race always > > exist becaues another CPU can observe the object between the reference > > going to 0 and the last kobject_del done by kobject_release. > > No it really damn well can't, exactly because we should not allow it - > and allowing it would be fundamentally racy. > > We should never allow a kobject_get() with a zero reference count. > It's always a *fundamental* bug - see my other email on your 1/2 > patch. It is and there's a WARN_ON about it inside kobject_get(). I don't think anybody argues against that, you are absolutely right. That said, the fact that it happens is I think the result of a deeper problem of conflating the visibility of the object and its refcount, and the fact that we do the sysfs "cleanup" (visibility) after we've dropped the last reference. Yes, the race is generally not going to happen if the put() done by the owning subystem is indeed the last one, because as you said, the subsystem should have higher level locking to precent racing between removal and addition. However, it *can* happen because by definition kobjects can have references held elsewhere for any reason, and those can be dropped outside of any subsystem locking. > So there is no race, because the reference count itself protects the > object. Either another CPU gets it in time (before it went down to > zero), or it went down to zero and it is dead (because at that point, > deletion will have been scheduled. > > Note that this is entirely independent of the auto-clean actions, > since even with absolutely zero cleanup, you still have the > fundamental issue of "reference count goes to zero causes the object > to be free'd". Yes. Cheers, Ben.
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, 2018-06-30 at 19:07 -0700, Linus Torvalds wrote: > [ Ben - feel free to post the missing emails to lkml too ] > > On Sat, Jun 30, 2018 at 6:56 PM Benjamin Herrenschmidt > wrote: > > > > On Sat, 2018-06-30 at 12:29 -0700, Linus Torvalds wrote: > > > > > > Because normally, the last kobject_put() really does do a synchronous > > > kobject_del(). So normally, this is all completely pointless, and the > > > normal kobject lifetime rules should be sufficient. > > > > Not entirely sadly > > > > There's a small race between the kref going down to 0 and the last > > kobject_put(). Something might still "catch" the 0-reference object > > during that window. > > That's only true if the underlying subsystem doesn't have any > serialization itself. > > Which I don't think is normal. > > IOW, if you have (say) a PCI hotplug model, the PCI layer will have > the pci_hp_mutex, for example, which protects the PCI hotplug slot > lists and the kobj things. > > Those locks won't protect kobj races in _general_ (ie there is no > locking between two totally unrelated buses), but they *should* > serialize the case of a device being added within one class. No? > > If not, maybe we need to add some class-based serialization. Well, yes and no ... yes I agree there should be upper level serialization such that you can't hit the "add" path until the "del" path has completed. And this will work as long as the kobject_put done at the end of "del" is indeed the very last one. But what if something *else* still holds a reference to the kobject ? It could be anything really... the driver itself, that is now unbound, might have a client that's holding a stale reference because some chardev is still open, for example. I don't know if sysfs can also hold one because a users still has an open directory there, but in the general case I don't thing one can assume that the kobject_put() done by the end of the "owner" subsystem delete path (in our example device_del) is the very last kobject_put. And since some other random thing might be doing that last put, it may also do it without any of the upper level locking/serialization. Which is the reason I so dislike the whole "cleanup sysfs in the last kobject_put" we do in kobject_release(). This is also why we clearly differenciate in the device model device_del from the final device_put etc... We generally separeate "visibility to the outside world" from the actual object lifetime, the latter being what the kref controls. .. except when we dont, such as this gluedir case. And I think that's wrong in principle. This is the reason why I prefer the approach of explicitely doing the kobject_del() instead of kobject_put() in the tail of device_del (what this 2/2 patch does), because it restores that separation between the object being "active/visible" or not from the actual lifetime of the structure. > > I think it just opens an existing race more widely. The race always > > exist becaues another CPU can observe the object between the reference > > going to 0 and the last kobject_del done by kobject_release. > > No it really damn well can't, exactly because we should not allow it - > and allowing it would be fundamentally racy. > > We should never allow a kobject_get() with a zero reference count. > It's always a *fundamental* bug - see my other email on your 1/2 > patch. It is and there's a WARN_ON about it inside kobject_get(). I don't think anybody argues against that, you are absolutely right. That said, the fact that it happens is I think the result of a deeper problem of conflating the visibility of the object and its refcount, and the fact that we do the sysfs "cleanup" (visibility) after we've dropped the last reference. Yes, the race is generally not going to happen if the put() done by the owning subystem is indeed the last one, because as you said, the subsystem should have higher level locking to precent racing between removal and addition. However, it *can* happen because by definition kobjects can have references held elsewhere for any reason, and those can be dropped outside of any subsystem locking. > So there is no race, because the reference count itself protects the > object. Either another CPU gets it in time (before it went down to > zero), or it went down to zero and it is dead (because at that point, > deletion will have been scheduled. > > Note that this is entirely independent of the auto-clean actions, > since even with absolutely zero cleanup, you still have the > fundamental issue of "reference count goes to zero causes the object > to be free'd". Yes. Cheers, Ben.
I will wait for positive answer
Good day, My name is Mr.Yuehan Pan, working as head of risk in Bank of China. I'm looking for an investment manager / partner who will represents me in a mutual business. Please contact me in my private email for more details. e-mail (yuehanpa...@gmail.com) Waiting for your news. Thank you, Yuehan Pan
I will wait for positive answer
Good day, My name is Mr.Yuehan Pan, working as head of risk in Bank of China. I'm looking for an investment manager / partner who will represents me in a mutual business. Please contact me in my private email for more details. e-mail (yuehanpa...@gmail.com) Waiting for your news. Thank you, Yuehan Pan
Re: [PATCH v5] ARM: dts: imx51-zii-rdu1: fix touchscreen pinctrl
On Thu, Jun 21, 2018 at 07:10:00PM +0100, Nick Dyer wrote:
> The pinctrl settings were incorrect for the touchscreen interrupt line,
> causing
> an interrupt storm. This change has been tested with both the atmel_mxt_ts and
> RMI4 drivers on the RDU1 units.
>
> The value 0x4 comes from the value of register IOMUXC_SW_PAD_CTL_PAD_CSI1_D8
> from the old vendor kernel.
>
> Signed-off-by: Nick Dyer
> Fixes: ceef0396f367 ("ARM: dts: imx: add ZII RDU1 board")
> Cc: # 4.15+
> Reviewed-by: Fabio Estevam
> Tested-by: Chris Healy
Applied, thanks.
> ---
> Changes in v5:
> - Add tested by Chris Healy
> Changes in v4:
> - Add reviewed by Fabio
You do not have to send new version just for collecting review and test
tags. I will do that when applying.
Shawn
> Changes in v3:
> - Update commit message to add source of 0x4 value, fixes tag and CC stable
> Changes in v2:
> - Use hex, only alter IRQ line config
>
> arch/arm/boot/dts/imx51-zii-rdu1.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts
> b/arch/arm/boot/dts/imx51-zii-rdu1.dts
> index df9eca94d812..8a878687197b 100644
> --- a/arch/arm/boot/dts/imx51-zii-rdu1.dts
> +++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts
> @@ -770,7 +770,7 @@
>
> pinctrl_ts: tsgrp {
> fsl,pins = <
> - MX51_PAD_CSI1_D8__GPIO3_12 0x85
> + MX51_PAD_CSI1_D8__GPIO3_12 0x04
> MX51_PAD_CSI1_D9__GPIO3_13 0x85
> >;
> };
> --
> 2.17.1
>
Re: [PATCH v5] ARM: dts: imx51-zii-rdu1: fix touchscreen pinctrl
On Thu, Jun 21, 2018 at 07:10:00PM +0100, Nick Dyer wrote:
> The pinctrl settings were incorrect for the touchscreen interrupt line,
> causing
> an interrupt storm. This change has been tested with both the atmel_mxt_ts and
> RMI4 drivers on the RDU1 units.
>
> The value 0x4 comes from the value of register IOMUXC_SW_PAD_CTL_PAD_CSI1_D8
> from the old vendor kernel.
>
> Signed-off-by: Nick Dyer
> Fixes: ceef0396f367 ("ARM: dts: imx: add ZII RDU1 board")
> Cc: # 4.15+
> Reviewed-by: Fabio Estevam
> Tested-by: Chris Healy
Applied, thanks.
> ---
> Changes in v5:
> - Add tested by Chris Healy
> Changes in v4:
> - Add reviewed by Fabio
You do not have to send new version just for collecting review and test
tags. I will do that when applying.
Shawn
> Changes in v3:
> - Update commit message to add source of 0x4 value, fixes tag and CC stable
> Changes in v2:
> - Use hex, only alter IRQ line config
>
> arch/arm/boot/dts/imx51-zii-rdu1.dts | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/imx51-zii-rdu1.dts
> b/arch/arm/boot/dts/imx51-zii-rdu1.dts
> index df9eca94d812..8a878687197b 100644
> --- a/arch/arm/boot/dts/imx51-zii-rdu1.dts
> +++ b/arch/arm/boot/dts/imx51-zii-rdu1.dts
> @@ -770,7 +770,7 @@
>
> pinctrl_ts: tsgrp {
> fsl,pins = <
> - MX51_PAD_CSI1_D8__GPIO3_12 0x85
> + MX51_PAD_CSI1_D8__GPIO3_12 0x04
> MX51_PAD_CSI1_D9__GPIO3_13 0x85
> >;
> };
> --
> 2.17.1
>
[PATCH] iio: add channel type for frequency
This adds a new type for frequency to the IIO channel type enumeration.
Units are in Hz.
Signed-off-by: David Lechner
---
drivers/iio/industrialio-core.c | 1 +
include/uapi/linux/iio/types.h | 1 +
tools/iio/iio_event_monitor.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 19bdf3d2962a..f3c2c9e4b997 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -85,6 +85,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_COUNT] = "count",
[IIO_INDEX] = "index",
[IIO_GRAVITY] = "gravity",
+ [IIO_FREQUENCY] = "frequency",
};
static const char * const iio_modifier_names[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 4213cdf88e3c..9fee86e2046f 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -44,6 +44,7 @@ enum iio_chan_type {
IIO_COUNT,
IIO_INDEX,
IIO_GRAVITY,
+ IIO_FREQUENCY,
};
enum iio_modifier {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index b61245e1181d..c3ef20057d52 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -58,6 +58,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_PH] = "ph",
[IIO_UVINDEX] = "uvindex",
[IIO_GRAVITY] = "gravity",
+ [IIO_FREQUENCY] = "frequency",
};
static const char * const iio_ev_type_text[] = {
--
2.17.1
[PATCH] iio: add channel type for frequency
This adds a new type for frequency to the IIO channel type enumeration.
Units are in Hz.
Signed-off-by: David Lechner
---
drivers/iio/industrialio-core.c | 1 +
include/uapi/linux/iio/types.h | 1 +
tools/iio/iio_event_monitor.c | 1 +
3 files changed, 3 insertions(+)
diff --git a/drivers/iio/industrialio-core.c b/drivers/iio/industrialio-core.c
index 19bdf3d2962a..f3c2c9e4b997 100644
--- a/drivers/iio/industrialio-core.c
+++ b/drivers/iio/industrialio-core.c
@@ -85,6 +85,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_COUNT] = "count",
[IIO_INDEX] = "index",
[IIO_GRAVITY] = "gravity",
+ [IIO_FREQUENCY] = "frequency",
};
static const char * const iio_modifier_names[] = {
diff --git a/include/uapi/linux/iio/types.h b/include/uapi/linux/iio/types.h
index 4213cdf88e3c..9fee86e2046f 100644
--- a/include/uapi/linux/iio/types.h
+++ b/include/uapi/linux/iio/types.h
@@ -44,6 +44,7 @@ enum iio_chan_type {
IIO_COUNT,
IIO_INDEX,
IIO_GRAVITY,
+ IIO_FREQUENCY,
};
enum iio_modifier {
diff --git a/tools/iio/iio_event_monitor.c b/tools/iio/iio_event_monitor.c
index b61245e1181d..c3ef20057d52 100644
--- a/tools/iio/iio_event_monitor.c
+++ b/tools/iio/iio_event_monitor.c
@@ -58,6 +58,7 @@ static const char * const iio_chan_type_name_spec[] = {
[IIO_PH] = "ph",
[IIO_UVINDEX] = "uvindex",
[IIO_GRAVITY] = "gravity",
+ [IIO_FREQUENCY] = "frequency",
};
static const char * const iio_ev_type_text[] = {
--
2.17.1
Re: [PATCH 2/2] ARM: dts: imx6ul: Add DTS for ConnectCore 6UL SBC Express
On Tue, Jun 19, 2018 at 09:32:14AM +0200, Alex Gonzalez wrote:
> The ConnectCore 6UL Single Board Computer (SBC) Express contains the
> ConnectCore 6UL System-On-Module.
>
> Its hardware specifications are:
>
> * 256MB DDR3 memory
> * 256MB NAND flash
> * Single Ethernet
> * USB Host and USB-OTG
> * MicroSD external storage
> * Groove connectors and Raspberry Pi Hat compatible expansion header
>
> Signed-off-by: Alex Gonzalez
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts | 205
>
> 2 files changed, 206 insertions(+)
> create mode 100644 arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 37a3de760d40..6135d3dc381c 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -533,6 +533,7 @@ dtb-$(CONFIG_SOC_IMX6SX) += \
> imx6sx-udoo-neo-full.dtb
> dtb-$(CONFIG_SOC_IMX6UL) += \
> imx6ul-14x14-evk.dtb \
> + imx6ul-ccimx6ulsbcexpress.dtb \
> imx6ul-geam.dtb \
> imx6ul-isiot-emmc.dtb \
> imx6ul-isiot-nand.dtb \
> diff --git a/arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
> b/arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
> new file mode 100644
> index ..1e3c4200b924
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Digi International's ConnectCore6UL SBC Express board device tree source
> + *
> + * Copyright 2018 Digi International, Inc.
> + *
> + */
> +
> +/dts-v1/;
> +#include
> +#include
> +#include "imx6ul.dtsi"
> +#include "imx6ul-ccimx6ulsom.dtsi"
> +
> +/ {
> + model = "Digi International ConnectCore 6UL SBC Express.";
> + compatible = "digi,ccimx6ulsbcexpress", "digi,ccimx6ulsom",
> + "fsl,imx6ul";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_adc1>;
> + adc-ch-list = <4>;
Undocumented DT property.
> + status = "okay";
> +};
> +
> + {
> + fsl,spi-num-chipselects = <1>;
This property is obsoleted.
> + cs-gpios = < 20 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_ecspi3_master>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_enet1>;
> + phy-mode = "rmii";
> + phy-handle = <>;
> + status = "okay";
> +
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy0: ethernet-phy@0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + smsc,disable-energy-detect;
> + reg = <0>;
> + };
> + };
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_flexcan1>;
> + xceiver-supply = <_3v3>;
> + status = "okay";
> +};
Nodes are well sorted alphabetically in label name, but this one is not.
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_i2c2>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pwm1>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_uart4>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_uart5>;
> + status = "okay";
> +};
> +
> + {
> + dr_mode = "host";
> + disable-over-current;
> + status = "okay";
> +};
> +
> + {
> + dr_mode = "host";
> + disable-over-current;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_usdhc2>;
> + broken-cd; /* no carrier detect line (use polling) */
> + no-1-8-v;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_hog>;
> +
> + imx6ul-ccimx6ul {
Drop this container node.
Shawn
> + pinctrl_adc1: adc1grp {
> + fsl,pins = <
> + /* GPIO1_4/ADC1_IN4 (pin 7 of the expansion
> header) */
> + MX6UL_PAD_GPIO1_IO04__GPIO1_IO040xb0
> + >;
> + };
> +
> + pinctrl_ecspi3_master: ecspi3grp1 {
> + fsl,pins = <
> + MX6UL_PAD_UART2_RX_DATA__ECSPI3_SCLK0x10b0
> + MX6UL_PAD_UART2_CTS_B__ECSPI3_MOSI 0x10b0
> + MX6UL_PAD_UART2_RTS_B__ECSPI3_MISO 0x10b0
> + MX6UL_PAD_UART2_TX_DATA__GPIO1_IO20 0x10b0
> /* Chip Select */
> + >;
> + };
> +
> + pinctrl_ecspi3_slave: ecspi3grp2 {
> + fsl,pins = <
> + MX6UL_PAD_UART2_RX_DATA__ECSPI3_SCLK0x10b0
> + MX6UL_PAD_UART2_CTS_B__ECSPI3_MOSI 0x10b0
> +
Re: [PATCH 2/2] ARM: dts: imx6ul: Add DTS for ConnectCore 6UL SBC Express
On Tue, Jun 19, 2018 at 09:32:14AM +0200, Alex Gonzalez wrote:
> The ConnectCore 6UL Single Board Computer (SBC) Express contains the
> ConnectCore 6UL System-On-Module.
>
> Its hardware specifications are:
>
> * 256MB DDR3 memory
> * 256MB NAND flash
> * Single Ethernet
> * USB Host and USB-OTG
> * MicroSD external storage
> * Groove connectors and Raspberry Pi Hat compatible expansion header
>
> Signed-off-by: Alex Gonzalez
> ---
> arch/arm/boot/dts/Makefile | 1 +
> arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts | 205
>
> 2 files changed, 206 insertions(+)
> create mode 100644 arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
>
> diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
> index 37a3de760d40..6135d3dc381c 100644
> --- a/arch/arm/boot/dts/Makefile
> +++ b/arch/arm/boot/dts/Makefile
> @@ -533,6 +533,7 @@ dtb-$(CONFIG_SOC_IMX6SX) += \
> imx6sx-udoo-neo-full.dtb
> dtb-$(CONFIG_SOC_IMX6UL) += \
> imx6ul-14x14-evk.dtb \
> + imx6ul-ccimx6ulsbcexpress.dtb \
> imx6ul-geam.dtb \
> imx6ul-isiot-emmc.dtb \
> imx6ul-isiot-nand.dtb \
> diff --git a/arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
> b/arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
> new file mode 100644
> index ..1e3c4200b924
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6ul-ccimx6ulsbcexpress.dts
> @@ -0,0 +1,205 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Digi International's ConnectCore6UL SBC Express board device tree source
> + *
> + * Copyright 2018 Digi International, Inc.
> + *
> + */
> +
> +/dts-v1/;
> +#include
> +#include
> +#include "imx6ul.dtsi"
> +#include "imx6ul-ccimx6ulsom.dtsi"
> +
> +/ {
> + model = "Digi International ConnectCore 6UL SBC Express.";
> + compatible = "digi,ccimx6ulsbcexpress", "digi,ccimx6ulsom",
> + "fsl,imx6ul";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_adc1>;
> + adc-ch-list = <4>;
Undocumented DT property.
> + status = "okay";
> +};
> +
> + {
> + fsl,spi-num-chipselects = <1>;
This property is obsoleted.
> + cs-gpios = < 20 GPIO_ACTIVE_LOW>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_ecspi3_master>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_enet1>;
> + phy-mode = "rmii";
> + phy-handle = <>;
> + status = "okay";
> +
> + mdio {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + ethphy0: ethernet-phy@0 {
> + compatible = "ethernet-phy-ieee802.3-c22";
> + smsc,disable-energy-detect;
> + reg = <0>;
> + };
> + };
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_flexcan1>;
> + xceiver-supply = <_3v3>;
> + status = "okay";
> +};
Nodes are well sorted alphabetically in label name, but this one is not.
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_i2c2>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_pwm1>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_uart4>;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_uart5>;
> + status = "okay";
> +};
> +
> + {
> + dr_mode = "host";
> + disable-over-current;
> + status = "okay";
> +};
> +
> + {
> + dr_mode = "host";
> + disable-over-current;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_usdhc2>;
> + broken-cd; /* no carrier detect line (use polling) */
> + no-1-8-v;
> + status = "okay";
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_hog>;
> +
> + imx6ul-ccimx6ul {
Drop this container node.
Shawn
> + pinctrl_adc1: adc1grp {
> + fsl,pins = <
> + /* GPIO1_4/ADC1_IN4 (pin 7 of the expansion
> header) */
> + MX6UL_PAD_GPIO1_IO04__GPIO1_IO040xb0
> + >;
> + };
> +
> + pinctrl_ecspi3_master: ecspi3grp1 {
> + fsl,pins = <
> + MX6UL_PAD_UART2_RX_DATA__ECSPI3_SCLK0x10b0
> + MX6UL_PAD_UART2_CTS_B__ECSPI3_MOSI 0x10b0
> + MX6UL_PAD_UART2_RTS_B__ECSPI3_MISO 0x10b0
> + MX6UL_PAD_UART2_TX_DATA__GPIO1_IO20 0x10b0
> /* Chip Select */
> + >;
> + };
> +
> + pinctrl_ecspi3_slave: ecspi3grp2 {
> + fsl,pins = <
> + MX6UL_PAD_UART2_RX_DATA__ECSPI3_SCLK0x10b0
> + MX6UL_PAD_UART2_CTS_B__ECSPI3_MOSI 0x10b0
> +
Re: [PATCH 1/2] ARM: dts: imx6ul: Add DTS for ConnectCore 6UL System-On-Module (SOM)
On Tue, Jun 19, 2018 at 09:32:13AM +0200, Alex Gonzalez wrote:
> The ConnectCore 6UL System-On-Module has the following hardware
> specification:
>
> * Based on a NXP i.MX6UL SoC
> * Industrial temperature ranges (-40ºC to +85ºC)
> * Up to 1GB DDR3 memory
> * Up to 2GB NAND flash
> * Dual Ethernet
> * On module 802.11 WiFi and Bluetooth 4.2 (QCA6564)
> * On module NXP Kinetis KL03
> * On module Microchip ATECC508A crypto element
>
> Signed-off-by: Alex Gonzalez
> ---
> arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi | 199
> ++
> 1 file changed, 199 insertions(+)
> create mode 100644 arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
>
> diff --git a/arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
> b/arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
> new file mode 100644
> index ..23484a48f6e0
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Digi International's ConnectCore 6UL System-On-Module device tree source
> + *
> + * Copyright 2018 Digi International, Inc.
> + *
> + */
> +
> +/ {
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + linux,cma {
> + compatible = "shared-dma-pool";
> + reusable;
> + size = <0x400>;
> + linux,cma-default;
> + };
> + };
> +};
> +
> + {
> + vref-supply = <_adc_3v3>;
> + max-channel = <9>;
Undocumented DT property?
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_gpmi_nand>;
> + status = "okay";
> +};
> +
> + {
> + clock-frequency = <10>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_i2c1>;
> + status = "okay";
> +
> + pmic: pfuze3000@8 {
Node name should be generic, while label can be specific.
pfuze3000: pmic@8 {
> + compatible = "fsl,pfuze3000";
> + reg = <0x08>;
> +
> + regulators {
> + int_3v3: sw1a {
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <330>;
> + regulator-ramp-delay = <6250>;
> + regulator-boot-on;
> + regulator-always-on;
Please have a newline between property list and child node.
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vdd_arm_soc_in: sw1b {
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <1475000>;
> + regulator-ramp-delay = <6250>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <925000>;
> + };
> + };
> +
> + ext_3v3: sw2 {
> + regulator-min-microvolt = <250>;
> + regulator-max-microvolt = <330>;
> + regulator-ramp-delay = <6250>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vcc_ddr3: sw3 {
> + regulator-min-microvolt = <90>;
> + regulator-max-microvolt = <165>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <130>;
> + };
> + };
> +
> + swbst_reg: swbst {
> + regulator-min-microvolt = <500>;
> + regulator-max-microvolt = <515>;
> + };
> +
> + vdd_snvs_3v3: vsnvs {
> + regulator-min-microvolt = <100>;
> + regulator-max-microvolt = <300>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + vrefddr: vrefddr {
> + regulator-boot-on;
> +
Re: [PATCH 1/2] ARM: dts: imx6ul: Add DTS for ConnectCore 6UL System-On-Module (SOM)
On Tue, Jun 19, 2018 at 09:32:13AM +0200, Alex Gonzalez wrote:
> The ConnectCore 6UL System-On-Module has the following hardware
> specification:
>
> * Based on a NXP i.MX6UL SoC
> * Industrial temperature ranges (-40ºC to +85ºC)
> * Up to 1GB DDR3 memory
> * Up to 2GB NAND flash
> * Dual Ethernet
> * On module 802.11 WiFi and Bluetooth 4.2 (QCA6564)
> * On module NXP Kinetis KL03
> * On module Microchip ATECC508A crypto element
>
> Signed-off-by: Alex Gonzalez
> ---
> arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi | 199
> ++
> 1 file changed, 199 insertions(+)
> create mode 100644 arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
>
> diff --git a/arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
> b/arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
> new file mode 100644
> index ..23484a48f6e0
> --- /dev/null
> +++ b/arch/arm/boot/dts/imx6ul-ccimx6ulsom.dtsi
> @@ -0,0 +1,199 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Digi International's ConnectCore 6UL System-On-Module device tree source
> + *
> + * Copyright 2018 Digi International, Inc.
> + *
> + */
> +
> +/ {
> + reserved-memory {
> + #address-cells = <1>;
> + #size-cells = <1>;
> + ranges;
> +
> + linux,cma {
> + compatible = "shared-dma-pool";
> + reusable;
> + size = <0x400>;
> + linux,cma-default;
> + };
> + };
> +};
> +
> + {
> + vref-supply = <_adc_3v3>;
> + max-channel = <9>;
Undocumented DT property?
> +};
> +
> + {
> + pinctrl-names = "default";
> + pinctrl-0 = <_gpmi_nand>;
> + status = "okay";
> +};
> +
> + {
> + clock-frequency = <10>;
> + pinctrl-names = "default";
> + pinctrl-0 = <_i2c1>;
> + status = "okay";
> +
> + pmic: pfuze3000@8 {
Node name should be generic, while label can be specific.
pfuze3000: pmic@8 {
> + compatible = "fsl,pfuze3000";
> + reg = <0x08>;
> +
> + regulators {
> + int_3v3: sw1a {
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <330>;
> + regulator-ramp-delay = <6250>;
> + regulator-boot-on;
> + regulator-always-on;
Please have a newline between property list and child node.
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vdd_arm_soc_in: sw1b {
> + regulator-min-microvolt = <70>;
> + regulator-max-microvolt = <1475000>;
> + regulator-ramp-delay = <6250>;
> + regulator-boot-on;
> + regulator-always-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <925000>;
> + };
> + };
> +
> + ext_3v3: sw2 {
> + regulator-min-microvolt = <250>;
> + regulator-max-microvolt = <330>;
> + regulator-ramp-delay = <6250>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-off-in-suspend;
> + };
> + };
> +
> + vcc_ddr3: sw3 {
> + regulator-min-microvolt = <90>;
> + regulator-max-microvolt = <165>;
> + regulator-always-on;
> + regulator-boot-on;
> + regulator-state-mem {
> + regulator-on-in-suspend;
> + regulator-suspend-microvolt = <130>;
> + };
> + };
> +
> + swbst_reg: swbst {
> + regulator-min-microvolt = <500>;
> + regulator-max-microvolt = <515>;
> + };
> +
> + vdd_snvs_3v3: vsnvs {
> + regulator-min-microvolt = <100>;
> + regulator-max-microvolt = <300>;
> + regulator-boot-on;
> + regulator-always-on;
> + };
> +
> + vrefddr: vrefddr {
> + regulator-boot-on;
> +
tools/include/asm-generic/bitsperlong.h:14:2: error: #error Inconsistent word size. Check asm/bitsperlong.h
Hi Alexei, FYI, the error/warning still remains. tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 883c9ab9eb595f8542d01e55d29a346c8d96862e commit: 819dd92b9c0bc7bce9097d8c1f14240f471bb386 bpfilter: switch to CC from HOSTCC date: 4 weeks ago config: alpha-allyesconfig (attached as .config) compiler: alpha-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 819dd92b9c0bc7bce9097d8c1f14240f471bb386 # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=alpha All errors (new ones prefixed by >>): In file included from tools/include/uapi/asm/bitsperlong.h:17:0, from /usr/alpha-linux-gnu/include/asm-generic/int-l64.h:11, from /usr/alpha-linux-gnu/include/asm/types.h:12, from tools/include/linux/types.h:10, from ./include/uapi/linux/bpf.h:11, from net/bpfilter/main.c:9: >> tools/include/asm-generic/bitsperlong.h:14:2: error: #error Inconsistent >> word size. Check asm/bitsperlong.h #error Inconsistent word size. Check asm/bitsperlong.h ^ vim +14 tools/include/asm-generic/bitsperlong.h bb970707 Arnaldo Carvalho de Melo 2016-07-12 12 2a00f026 Arnaldo Carvalho de Melo 2016-07-13 13 #if BITS_PER_LONG != __BITS_PER_LONG bb970707 Arnaldo Carvalho de Melo 2016-07-12 @14 #error Inconsistent word size. Check asm/bitsperlong.h bb970707 Arnaldo Carvalho de Melo 2016-07-12 15 #endif bb970707 Arnaldo Carvalho de Melo 2016-07-12 16 :: The code at line 14 was first introduced by commit :: bb9707077b4ee5f77bc9939b057ff8a0d410296f tools: Copy the bitsperlong.h files from the kernel :: TO: Arnaldo Carvalho de Melo :: CC: Arnaldo Carvalho de Melo --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
tools/include/asm-generic/bitsperlong.h:14:2: error: #error Inconsistent word size. Check asm/bitsperlong.h
Hi Alexei, FYI, the error/warning still remains. tree: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master head: 883c9ab9eb595f8542d01e55d29a346c8d96862e commit: 819dd92b9c0bc7bce9097d8c1f14240f471bb386 bpfilter: switch to CC from HOSTCC date: 4 weeks ago config: alpha-allyesconfig (attached as .config) compiler: alpha-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 819dd92b9c0bc7bce9097d8c1f14240f471bb386 # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=alpha All errors (new ones prefixed by >>): In file included from tools/include/uapi/asm/bitsperlong.h:17:0, from /usr/alpha-linux-gnu/include/asm-generic/int-l64.h:11, from /usr/alpha-linux-gnu/include/asm/types.h:12, from tools/include/linux/types.h:10, from ./include/uapi/linux/bpf.h:11, from net/bpfilter/main.c:9: >> tools/include/asm-generic/bitsperlong.h:14:2: error: #error Inconsistent >> word size. Check asm/bitsperlong.h #error Inconsistent word size. Check asm/bitsperlong.h ^ vim +14 tools/include/asm-generic/bitsperlong.h bb970707 Arnaldo Carvalho de Melo 2016-07-12 12 2a00f026 Arnaldo Carvalho de Melo 2016-07-13 13 #if BITS_PER_LONG != __BITS_PER_LONG bb970707 Arnaldo Carvalho de Melo 2016-07-12 @14 #error Inconsistent word size. Check asm/bitsperlong.h bb970707 Arnaldo Carvalho de Melo 2016-07-12 15 #endif bb970707 Arnaldo Carvalho de Melo 2016-07-12 16 :: The code at line 14 was first introduced by commit :: bb9707077b4ee5f77bc9939b057ff8a0d410296f tools: Copy the bitsperlong.h files from the kernel :: TO: Arnaldo Carvalho de Melo :: CC: Arnaldo Carvalho de Melo --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
Re: [PATCH V2 2/2] ARM: dts: imx6ul: add GPIO clocks
On Fri, Jun 29, 2018 at 11:39:38AM -0700, Stephen Boyd wrote: > Quoting Shawn Guo (2018-06-27 17:52:18) > > On Mon, Jun 25, 2018 at 03:14:39AM +, Anson Huang wrote: > > > Gentle Ping... > > > > I cannot apply this dts patch until the clock patch is landed on > > mainline, because it has a dependency on new clock ID IMX6UL_CLK_GPIO1 > > created by clock patch. > > > > Does it matter if that clk ID changes across branches? Or is everything > good if it just exists as some number? We use macro IMX6UL_CLK_GPIO1 instead of hard-coded number in DTS. So it doesn't matter if IMX6UL_CLK_GPIO1 changes. > The patch adding the define > conflicts with another patch to fix the numbering scheme to be > incremental instead of changing IMX6UL and IMX6ULL which is annoying. > I'll probably make a topic branch just for the fix and merge it into > clk-fixes and this branch so you can pick it up from there if you like. It will be good if you can have a topic branch. Otherwise, we are also fine to wait till the clock patches lands on mainline. Shawn
Re: [PATCH V2 2/2] ARM: dts: imx6ul: add GPIO clocks
On Fri, Jun 29, 2018 at 11:39:38AM -0700, Stephen Boyd wrote: > Quoting Shawn Guo (2018-06-27 17:52:18) > > On Mon, Jun 25, 2018 at 03:14:39AM +, Anson Huang wrote: > > > Gentle Ping... > > > > I cannot apply this dts patch until the clock patch is landed on > > mainline, because it has a dependency on new clock ID IMX6UL_CLK_GPIO1 > > created by clock patch. > > > > Does it matter if that clk ID changes across branches? Or is everything > good if it just exists as some number? We use macro IMX6UL_CLK_GPIO1 instead of hard-coded number in DTS. So it doesn't matter if IMX6UL_CLK_GPIO1 changes. > The patch adding the define > conflicts with another patch to fix the numbering scheme to be > incremental instead of changing IMX6UL and IMX6ULL which is annoying. > I'll probably make a topic branch just for the fix and merge it into > clk-fixes and this branch so you can pick it up from there if you like. It will be good if you can have a topic branch. Otherwise, we are also fine to wait till the clock patches lands on mainline. Shawn
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, Jun 30, 2018 at 7:07 PM Linus Torvalds wrote: > > Those locks won't protect kobj races in _general_ (ie there is no > locking between two totally unrelated buses), but they *should* > serialize the case of a device being added within one class. No? Side note: there had *better* be some locking whenever there is a way to find an object, because otherwise you have a fundamental lifetime problem: one thread finding the object at the same time another thread frees it for the last time. Even the "unless_zero()" won't fix it, because the final free will release the underlying object itself, so the "zero" state is ephemeral. That locking might be just RCU during lookup, and rcu-delaying the release, of course. I think that's all the sysfs code needs, for example, since that's what lookup uses. And for any other embedded kobj cases, where you can reach the object using some random subsystem pointers, there had better be other locking in place for that pointer lookup vs the last removal. kobject itself doesn't provide that locking, it only provides the reference counting. But that's partly why it really has to disallow any kobject_get() of a zero object, because it means that the tear-down has been started, but the tear-down itself may not have had time to get the lock yet (ie kobject_release() may be just about to call the t->release() function). But maybe I'm missing something subtle. Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
On Sat, Jun 30, 2018 at 7:07 PM Linus Torvalds wrote: > > Those locks won't protect kobj races in _general_ (ie there is no > locking between two totally unrelated buses), but they *should* > serialize the case of a device being added within one class. No? Side note: there had *better* be some locking whenever there is a way to find an object, because otherwise you have a fundamental lifetime problem: one thread finding the object at the same time another thread frees it for the last time. Even the "unless_zero()" won't fix it, because the final free will release the underlying object itself, so the "zero" state is ephemeral. That locking might be just RCU during lookup, and rcu-delaying the release, of course. I think that's all the sysfs code needs, for example, since that's what lookup uses. And for any other embedded kobj cases, where you can reach the object using some random subsystem pointers, there had better be other locking in place for that pointer lookup vs the last removal. kobject itself doesn't provide that locking, it only provides the reference counting. But that's partly why it really has to disallow any kobject_get() of a zero object, because it means that the tear-down has been started, but the tear-down itself may not have had time to get the lock yet (ie kobject_release() may be just about to call the t->release() function). But maybe I'm missing something subtle. Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
[ Ben - feel free to post the missing emails to lkml too ] On Sat, Jun 30, 2018 at 6:56 PM Benjamin Herrenschmidt wrote: > > On Sat, 2018-06-30 at 12:29 -0700, Linus Torvalds wrote: > > > > Because normally, the last kobject_put() really does do a synchronous > > kobject_del(). So normally, this is all completely pointless, and the > > normal kobject lifetime rules should be sufficient. > > Not entirely sadly > > There's a small race between the kref going down to 0 and the last > kobject_put(). Something might still "catch" the 0-reference object > during that window. That's only true if the underlying subsystem doesn't have any serialization itself. Which I don't think is normal. IOW, if you have (say) a PCI hotplug model, the PCI layer will have the pci_hp_mutex, for example, which protects the PCI hotplug slot lists and the kobj things. Those locks won't protect kobj races in _general_ (ie there is no locking between two totally unrelated buses), but they *should* serialize the case of a device being added within one class. No? If not, maybe we need to add some class-based serialization. > I think it just opens an existing race more widely. The race always > exist becaues another CPU can observe the object between the reference > going to 0 and the last kobject_del done by kobject_release. No it really damn well can't, exactly because we should not allow it - and allowing it would be fundamentally racy. We should never allow a kobject_get() with a zero reference count. It's always a *fundamental* bug - see my other email on your 1/2 patch. So there is no race, because the reference count itself protects the object. Either another CPU gets it in time (before it went down to zero), or it went down to zero and it is dead (because at that point, deletion will have been scheduled. Note that this is entirely independent of the auto-clean actions, since even with absolutely zero cleanup, you still have the fundamental issue of "reference count goes to zero causes the object to be free'd". Linus
Re: [PATCH 2/2] drivers: core: Remove glue dirs from sysfs earlier
[ Ben - feel free to post the missing emails to lkml too ] On Sat, Jun 30, 2018 at 6:56 PM Benjamin Herrenschmidt wrote: > > On Sat, 2018-06-30 at 12:29 -0700, Linus Torvalds wrote: > > > > Because normally, the last kobject_put() really does do a synchronous > > kobject_del(). So normally, this is all completely pointless, and the > > normal kobject lifetime rules should be sufficient. > > Not entirely sadly > > There's a small race between the kref going down to 0 and the last > kobject_put(). Something might still "catch" the 0-reference object > during that window. That's only true if the underlying subsystem doesn't have any serialization itself. Which I don't think is normal. IOW, if you have (say) a PCI hotplug model, the PCI layer will have the pci_hp_mutex, for example, which protects the PCI hotplug slot lists and the kobj things. Those locks won't protect kobj races in _general_ (ie there is no locking between two totally unrelated buses), but they *should* serialize the case of a device being added within one class. No? If not, maybe we need to add some class-based serialization. > I think it just opens an existing race more widely. The race always > exist becaues another CPU can observe the object between the reference > going to 0 and the last kobject_del done by kobject_release. No it really damn well can't, exactly because we should not allow it - and allowing it would be fundamentally racy. We should never allow a kobject_get() with a zero reference count. It's always a *fundamental* bug - see my other email on your 1/2 patch. So there is no race, because the reference count itself protects the object. Either another CPU gets it in time (before it went down to zero), or it went down to zero and it is dead (because at that point, deletion will have been scheduled. Note that this is entirely independent of the auto-clean actions, since even with absolutely zero cleanup, you still have the fundamental issue of "reference count goes to zero causes the object to be free'd". Linus
[PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
The firmware found in the touch screen of the Surface Pro 3 is slightly
buggy and occasionally doesn't send lift off reports for contacts; add
MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
reports.
Signed-off-by: Joey Pabalinas
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 45968f7970f87775fa..a793076139d7d0db9b 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -242,11 +242,12 @@ static struct mt_class mt_classes[] = {
.quirks = MT_QUIRK_ALWAYS_VALID |
MT_QUIRK_IGNORE_DUPLICATES |
MT_QUIRK_HOVERING |
MT_QUIRK_CONTACT_CNT_ACCURATE |
MT_QUIRK_STICKY_FINGERS |
- MT_QUIRK_WIN8_PTP_BUTTONS },
+ MT_QUIRK_WIN8_PTP_BUTTONS |
+ MT_QUIRK_NOT_SEEN_MEANS_UP },
{ .name = MT_CLS_EXPORT_ALL_INPUTS,
.quirks = MT_QUIRK_ALWAYS_VALID |
MT_QUIRK_CONTACT_CNT_ACCURATE,
.export_all_inputs = true },
{ .name = MT_CLS_WIN_8_DUAL,
--
2.18.0
[PATCH 1/4] HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks
The firmware found in the touch screen of the Surface Pro 3 is slightly
buggy and occasionally doesn't send lift off reports for contacts; add
MT_QUIRK_NOT_SEEN_MEANS_UP to .quirks to compensate for the missed
reports.
Signed-off-by: Joey Pabalinas
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 45968f7970f87775fa..a793076139d7d0db9b 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -242,11 +242,12 @@ static struct mt_class mt_classes[] = {
.quirks = MT_QUIRK_ALWAYS_VALID |
MT_QUIRK_IGNORE_DUPLICATES |
MT_QUIRK_HOVERING |
MT_QUIRK_CONTACT_CNT_ACCURATE |
MT_QUIRK_STICKY_FINGERS |
- MT_QUIRK_WIN8_PTP_BUTTONS },
+ MT_QUIRK_WIN8_PTP_BUTTONS |
+ MT_QUIRK_NOT_SEEN_MEANS_UP },
{ .name = MT_CLS_EXPORT_ALL_INPUTS,
.quirks = MT_QUIRK_ALWAYS_VALID |
MT_QUIRK_CONTACT_CNT_ACCURATE,
.export_all_inputs = true },
{ .name = MT_CLS_WIN_8_DUAL,
--
2.18.0
[PATCH 4/4] HID: multitouch: remove unneeded else conditional cases
Elide lone `else` cases and replace `else if` clauses
with plain `if` conditionals when they occur immediately
after return statements.
Signed-off-by: Joey Pabalinas
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 08b50e5908cecdda66..30b1a2c67f39a41325 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -205,12 +205,12 @@ static void mt_post_parse(struct mt_device *td);
static int cypress_compute_slot(struct mt_device *td)
{
if (td->curdata.contactid != 0 || td->num_received == 0)
return td->curdata.contactid;
- else
- return -1;
+
+ return -1;
}
static struct mt_class mt_classes[] = {
{ .name = MT_CLS_DEFAULT,
.quirks = MT_QUIRK_ALWAYS_VALID |
@@ -803,12 +803,12 @@ static int mt_compute_timestamp(struct mt_device *td,
struct hid_field *field,
delta *= 100;
if (jdelta > MAX_TIMESTAMP_INTERVAL)
/* No data received for a while, resync the timestamp. */
return 0;
- else
- return td->timestamp + delta;
+
+ return td->timestamp + delta;
}
static int mt_touch_event(struct hid_device *hid, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
@@ -1110,11 +1110,12 @@ static int mt_input_mapping(struct hid_device *hdev,
struct hid_input *hi,
* HID_DG_CONTACTCOUNT from the pen report as it is outside the physical
* collection, but within the report ID.
*/
if (field->physical == HID_DG_STYLUS)
return 0;
- else if ((field->physical == 0) &&
+
+ if ((field->physical == 0) &&
(field->report->id != td->mt_report_id) &&
(td->mt_report_id != -1))
return 0;
if (field->application == HID_DG_TOUCHSCREEN ||
--
2.18.0
[PATCH 0/4] reduce Surface Pro 3 multitouch jitter
The Surface Pro 3 firmware doesn't reliably send contact lift off reports nor handle invalid report values gracefully. To reduce touchscreen input jitter: - add MT_QUIRK_NOT_SEEN_MEANS_UP to the MT_CLS_WIN_8 - drop invalid report values Joey Pabalinas (4): HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol HID: multitouch: drop reports containing invalid values HID: multitouch: remove unneeded else conditional cases drivers/hid/hid-multitouch.c | 27 --- 1 file changed, 20 insertions(+), 7 deletions(-) -- 2.18.0
[PATCH 4/4] HID: multitouch: remove unneeded else conditional cases
Elide lone `else` cases and replace `else if` clauses
with plain `if` conditionals when they occur immediately
after return statements.
Signed-off-by: Joey Pabalinas
1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 08b50e5908cecdda66..30b1a2c67f39a41325 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -205,12 +205,12 @@ static void mt_post_parse(struct mt_device *td);
static int cypress_compute_slot(struct mt_device *td)
{
if (td->curdata.contactid != 0 || td->num_received == 0)
return td->curdata.contactid;
- else
- return -1;
+
+ return -1;
}
static struct mt_class mt_classes[] = {
{ .name = MT_CLS_DEFAULT,
.quirks = MT_QUIRK_ALWAYS_VALID |
@@ -803,12 +803,12 @@ static int mt_compute_timestamp(struct mt_device *td,
struct hid_field *field,
delta *= 100;
if (jdelta > MAX_TIMESTAMP_INTERVAL)
/* No data received for a while, resync the timestamp. */
return 0;
- else
- return td->timestamp + delta;
+
+ return td->timestamp + delta;
}
static int mt_touch_event(struct hid_device *hid, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
@@ -1110,11 +1110,12 @@ static int mt_input_mapping(struct hid_device *hdev,
struct hid_input *hi,
* HID_DG_CONTACTCOUNT from the pen report as it is outside the physical
* collection, but within the report ID.
*/
if (field->physical == HID_DG_STYLUS)
return 0;
- else if ((field->physical == 0) &&
+
+ if ((field->physical == 0) &&
(field->report->id != td->mt_report_id) &&
(td->mt_report_id != -1))
return 0;
if (field->application == HID_DG_TOUCHSCREEN ||
--
2.18.0
[PATCH 0/4] reduce Surface Pro 3 multitouch jitter
The Surface Pro 3 firmware doesn't reliably send contact lift off reports nor handle invalid report values gracefully. To reduce touchscreen input jitter: - add MT_QUIRK_NOT_SEEN_MEANS_UP to the MT_CLS_WIN_8 - drop invalid report values Joey Pabalinas (4): HID: multitouch: add MT_QUIRK_NOT_SEEN_MEANS_UP to MT_CLS_WIN_8 quirks HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol HID: multitouch: drop reports containing invalid values HID: multitouch: remove unneeded else conditional cases drivers/hid/hid-multitouch.c | 27 --- 1 file changed, 20 insertions(+), 7 deletions(-) -- 2.18.0
[PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
The HID_GROUP_MULTITOUCH_WIN_8 group never needs to check for the serial
protocol, so avoid setting `td->serial_maybe = true;` in order to avoid
an unnecessary mt_post_parse_default_settings() call
Signed-off-by: Joey Pabalinas
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index a793076139d7d0db9b..c0654db0b736543ca0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1460,11 +1460,13 @@ static int mt_probe(struct hid_device *hdev, const
struct hid_device_id *id)
if (!td->fields) {
dev_err(>dev, "cannot allocate multitouch fields data\n");
return -ENOMEM;
}
- if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
+ if (id->vendor == HID_ANY_ID
+ && id->product == HID_ANY_ID
+ && id->group != HID_GROUP_MULTITOUCH_WIN_8)
td->serial_maybe = true;
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
--
2.18.0
[PATCH 3/4] HID: multitouch: drop reports containing invalid values
Avoid processing reports containing invalid values to reduce
multitouch input stutter.
Signed-off-by: Joey Pabalinas
1 file changed, 9 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c0654db0b736543ca0..08b50e5908cecdda66 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -694,10 +694,19 @@ static void mt_complete_slot(struct mt_device *td, struct
input_dev *input)
{
if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
td->num_received >= td->num_expected)
return;
+ /* drop invalid values after counting them */
+ if (td->curdata.x == 0x &&
+ td->curdata.y == 0x &&
+ td->curdata.w == 0x &&
+ td->curdata.h == 0x) {
+ td->num_received++;
+ return;
+ }
+
if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
int active;
int slotnum = mt_compute_slot(td, input);
struct mt_slot *s = >curdata;
struct input_mt *mt = input->mt;
--
2.18.0
[PATCH 2/4] HID: multitouch: don't check HID_GROUP_MULTITOUCH_WIN_8 for serial protocol
The HID_GROUP_MULTITOUCH_WIN_8 group never needs to check for the serial
protocol, so avoid setting `td->serial_maybe = true;` in order to avoid
an unnecessary mt_post_parse_default_settings() call
Signed-off-by: Joey Pabalinas
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index a793076139d7d0db9b..c0654db0b736543ca0 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -1460,11 +1460,13 @@ static int mt_probe(struct hid_device *hdev, const
struct hid_device_id *id)
if (!td->fields) {
dev_err(>dev, "cannot allocate multitouch fields data\n");
return -ENOMEM;
}
- if (id->vendor == HID_ANY_ID && id->product == HID_ANY_ID)
+ if (id->vendor == HID_ANY_ID
+ && id->product == HID_ANY_ID
+ && id->group != HID_GROUP_MULTITOUCH_WIN_8)
td->serial_maybe = true;
/* This allows the driver to correctly support devices
* that emit events over several HID messages.
*/
--
2.18.0
[PATCH 3/4] HID: multitouch: drop reports containing invalid values
Avoid processing reports containing invalid values to reduce
multitouch input stutter.
Signed-off-by: Joey Pabalinas
1 file changed, 9 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index c0654db0b736543ca0..08b50e5908cecdda66 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -694,10 +694,19 @@ static void mt_complete_slot(struct mt_device *td, struct
input_dev *input)
{
if ((td->mtclass.quirks & MT_QUIRK_CONTACT_CNT_ACCURATE) &&
td->num_received >= td->num_expected)
return;
+ /* drop invalid values after counting them */
+ if (td->curdata.x == 0x &&
+ td->curdata.y == 0x &&
+ td->curdata.w == 0x &&
+ td->curdata.h == 0x) {
+ td->num_received++;
+ return;
+ }
+
if (td->curvalid || (td->mtclass.quirks & MT_QUIRK_ALWAYS_VALID)) {
int active;
int slotnum = mt_compute_slot(td, input);
struct mt_slot *s = >curdata;
struct input_mt *mt = input->mt;
--
2.18.0
Re: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops
@Rosen: Ok, then I'll make a patch, and will ask you to test it. Wait for a moment. Thanks for your reply. (It's a burden for me, too. ;-) From: Rosen Penev Subject: Re: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops Date: Sat, 30 Jun 2018 11:57:32 -0700 >> Hi, Rosen. Thank you for your patch. (@Jan Steffens and @Pablo Cholaky: >> Thanks for your reports.) But I'm afraid some more work is necessary. Let me >> propose some points. > Too much work for me. I'll just use the kernel pareameter. Teika (Teika kazura)
Re: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops
@Rosen: Ok, then I'll make a patch, and will ask you to test it. Wait for a moment. Thanks for your reply. (It's a burden for me, too. ;-) From: Rosen Penev Subject: Re: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops Date: Sat, 30 Jun 2018 11:57:32 -0700 >> Hi, Rosen. Thank you for your patch. (@Jan Steffens and @Pablo Cholaky: >> Thanks for your reports.) But I'm afraid some more work is necessary. Let me >> propose some points. > Too much work for me. I'll just use the kernel pareameter. Teika (Teika kazura)
[PATCH v2] thermal: i.MX: Allow thermal probe to fail gracefully in case of bad calibration.
Without this fix, the thermal probe on i.MX6 might trigger a division by zero exception later in the probe if the calibration does fail. Note: This linux behavior (Division by zero in kernel) has been triggered on a Qemu i.MX6 emulation where parameters in nvmem were not set. With this fix the division by zero is not triggeed anymore as the thermal probe does fail early. Signed-off-by: Jean-Christophe Dubois Reviewed-by: Fabio Estevam --- v2 changes: - improved subject line - added thermal maintainers in the destination list. drivers/thermal/imx_thermal.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 334d98be03b9..b1f82d64253e 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -604,7 +604,10 @@ static int imx_init_from_nvmem_cells(struct platform_device *pdev) ret = nvmem_cell_read_u32(>dev, "calib", ); if (ret) return ret; - imx_init_calib(pdev, val); + + ret = imx_init_calib(pdev, val); + if (ret) + return ret; ret = nvmem_cell_read_u32(>dev, "temp_grade", ); if (ret) -- 2.17.1
[PATCH v2] thermal: i.MX: Allow thermal probe to fail gracefully in case of bad calibration.
Without this fix, the thermal probe on i.MX6 might trigger a division by zero exception later in the probe if the calibration does fail. Note: This linux behavior (Division by zero in kernel) has been triggered on a Qemu i.MX6 emulation where parameters in nvmem were not set. With this fix the division by zero is not triggeed anymore as the thermal probe does fail early. Signed-off-by: Jean-Christophe Dubois Reviewed-by: Fabio Estevam --- v2 changes: - improved subject line - added thermal maintainers in the destination list. drivers/thermal/imx_thermal.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 334d98be03b9..b1f82d64253e 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -604,7 +604,10 @@ static int imx_init_from_nvmem_cells(struct platform_device *pdev) ret = nvmem_cell_read_u32(>dev, "calib", ); if (ret) return ret; - imx_init_calib(pdev, val); + + ret = imx_init_calib(pdev, val); + if (ret) + return ret; ret = nvmem_cell_read_u32(>dev, "temp_grade", ); if (ret) -- 2.17.1
Re: [PATCH v2] PCI/AER: Fix aerdrv loading with "pcie_ports=native" parameter
[+cc Borislav, linux-acpi, since this involves APEI/HEST] On Tue, Jun 19, 2018 at 02:58:20PM -0500, Alexandru Gagniuc wrote: > According to the documentation, "pcie_ports=native", linux should use > native AER and DPC services. While that is true for the _OSC method > parsing, this is not the only place that is checked. Should the HEST > table list PCIe ports as firmware-first, linux will not use native > services. Nothing in ACPI-land looks at pcie_ports_native. How should ACPI things work in the "pcie_ports=native" case? I guess we still have to expect to receive error records from the firmware, because it may certainly send us non-PCI errors (machine checks, etc) and maybe even some PCI errors (even if the Linux AER driver claims AER interrupts, we don't know what notification mechanisms the firmware may be using). I guess best-case, we'll get ACPI error records for all non-PCI things, and the Linux AER driver will see all the AER errors. Worst-case, I don't really know what to expect. Duplicate reporting of AER errors via firmware and Linux AER driver? Some kind of confusion about who acknowledges and clears them? Out of curiosity, what is your use case for "pcie_ports=native"? Presumably there's something that works better when using it, and things work even *better* with this patch? I know people do use it, because I often see it mentioned in forums and bug reports, but I really don't expect it to work very well because we're ignoring the usage model the firmware is designed around. My unproven suspicion is that most uses are in the black magic category of "there's a bug here, and we don't know how to fix it, but pcie_ports=native makes it work better". Obviously I would much rather find and fix those bugs so people wouldn't have to stumble over the problem in the first place. > This happens because aer_acpi_firmware_first() doesn't take > 'pcie_ports' into account. This is wrong. DPC uses the same logic when > it decides whether to load or not, so fixing this also fixes DPC not > loading. > > Signed-off-by: Alexandru Gagniuc > --- > drivers/pci/pcie/aer.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > Changes since v1: > - Re-tested with latest and greatest (v4.18-rc1) -- works great > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index a2e88386af28..98ced0f7c850 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -291,7 +291,7 @@ static void aer_set_firmware_first(struct pci_dev > *pci_dev) > > rc = apei_hest_parse(aer_hest_parse, ); > > - if (rc) > + if (rc || pcie_ports_native) > pci_dev->__aer_firmware_first = 0; > else > pci_dev->__aer_firmware_first = info.firmware_first; > @@ -327,6 +327,9 @@ bool aer_acpi_firmware_first(void) > apei_hest_parse(aer_hest_parse, ); > aer_firmware_first = info.firmware_first; > parsed = true; > + if (pcie_ports_native) > + aer_firmware_first = 0; > + > } > return aer_firmware_first; > } > -- > 2.14.3 >
Re: [PATCH v2] PCI/AER: Fix aerdrv loading with "pcie_ports=native" parameter
[+cc Borislav, linux-acpi, since this involves APEI/HEST] On Tue, Jun 19, 2018 at 02:58:20PM -0500, Alexandru Gagniuc wrote: > According to the documentation, "pcie_ports=native", linux should use > native AER and DPC services. While that is true for the _OSC method > parsing, this is not the only place that is checked. Should the HEST > table list PCIe ports as firmware-first, linux will not use native > services. Nothing in ACPI-land looks at pcie_ports_native. How should ACPI things work in the "pcie_ports=native" case? I guess we still have to expect to receive error records from the firmware, because it may certainly send us non-PCI errors (machine checks, etc) and maybe even some PCI errors (even if the Linux AER driver claims AER interrupts, we don't know what notification mechanisms the firmware may be using). I guess best-case, we'll get ACPI error records for all non-PCI things, and the Linux AER driver will see all the AER errors. Worst-case, I don't really know what to expect. Duplicate reporting of AER errors via firmware and Linux AER driver? Some kind of confusion about who acknowledges and clears them? Out of curiosity, what is your use case for "pcie_ports=native"? Presumably there's something that works better when using it, and things work even *better* with this patch? I know people do use it, because I often see it mentioned in forums and bug reports, but I really don't expect it to work very well because we're ignoring the usage model the firmware is designed around. My unproven suspicion is that most uses are in the black magic category of "there's a bug here, and we don't know how to fix it, but pcie_ports=native makes it work better". Obviously I would much rather find and fix those bugs so people wouldn't have to stumble over the problem in the first place. > This happens because aer_acpi_firmware_first() doesn't take > 'pcie_ports' into account. This is wrong. DPC uses the same logic when > it decides whether to load or not, so fixing this also fixes DPC not > loading. > > Signed-off-by: Alexandru Gagniuc > --- > drivers/pci/pcie/aer.c | 5 - > 1 file changed, 4 insertions(+), 1 deletion(-) > > Changes since v1: > - Re-tested with latest and greatest (v4.18-rc1) -- works great > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > index a2e88386af28..98ced0f7c850 100644 > --- a/drivers/pci/pcie/aer.c > +++ b/drivers/pci/pcie/aer.c > @@ -291,7 +291,7 @@ static void aer_set_firmware_first(struct pci_dev > *pci_dev) > > rc = apei_hest_parse(aer_hest_parse, ); > > - if (rc) > + if (rc || pcie_ports_native) > pci_dev->__aer_firmware_first = 0; > else > pci_dev->__aer_firmware_first = info.firmware_first; > @@ -327,6 +327,9 @@ bool aer_acpi_firmware_first(void) > apei_hest_parse(aer_hest_parse, ); > aer_firmware_first = info.firmware_first; > parsed = true; > + if (pcie_ports_native) > + aer_firmware_first = 0; > + > } > return aer_firmware_first; > } > -- > 2.14.3 >
Re: [PATCH v4 1/2] iio: dac: Add AD5758 support
On Fri, Jun 29, 2018 at 11:38 AM, Stefan Popa wrote:
> The AD5758 is a single channel DAC with 16-bit precision which uses the
> SPI interface that operates at clock rates up to 50MHz.
>
> The output can be configured as voltage or current and is available on a
> single terminal.
>
> Datasheet:
> http://www.analog.com/media/en/technical-documentation/data-sheets/ad5758.pdf
Thanks for an update.
Few comments below.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Perhaps keep them ordered?
> +
> +#include
> +#include
> +
> +#include
ASM? Hmm...
> +static int cmpfunc(const void *a, const void *b)
> +{
> + return (*(int *)a - *(int *)b);
Surrounding parens are not needed.
> +}
> +
> +static int ad5758_find_closest_match(const int *array,
> +unsigned int size, int val)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if (val <= array[i])
> + return i;
> + }
> +
> + return size - 1;
> +}
Isn't it what bsearch() covers as well?
> + do {
> + ret = ad5758_spi_reg_read(st, reg);
> + if (ret < 0)
> + return ret;
> +
> + if (!(ret & mask))
> + return 0;
> +
> + udelay(100);
If it's not called from atomic context, perhaps switch to usleep_range() ?
> + } while (--timeout);
> +static int ad5758_soft_reset(struct ad5758_state *st)
> +{
> + int ret;
> +
> + ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_1);
> + if (ret < 0)
> + return ret;
> +
> + ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_2);
> +
> + /* Perform a software reset and wait 100us */
> + udelay(100);
Ditto.
> +
> + return ret;
> +}
> +static int ad5758_find_out_range(struct ad5758_state *st,
> +const struct ad5758_range *range,
> +unsigned int size,
> +int min, int max)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if ((min == range[i].min) && (max == range[i].max)) {
> + st->out_range.reg = range[i].reg;
> + st->out_range.min = range[i].min;
> + st->out_range.max = range[i].max;
> +
> + return 0;
> + }
> + }
One more candidate to use bsearch().
> +
> + return -EINVAL;
> +}
> + index = (int *) bsearch(, ad5758_dc_dc_ilim,
> + ARRAY_SIZE(ad5758_dc_dc_ilim),
> + sizeof(int), cmpfunc);
I'm not sure you need that casting.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v4 1/2] iio: dac: Add AD5758 support
On Fri, Jun 29, 2018 at 11:38 AM, Stefan Popa wrote:
> The AD5758 is a single channel DAC with 16-bit precision which uses the
> SPI interface that operates at clock rates up to 50MHz.
>
> The output can be configured as voltage or current and is available on a
> single terminal.
>
> Datasheet:
> http://www.analog.com/media/en/technical-documentation/data-sheets/ad5758.pdf
Thanks for an update.
Few comments below.
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
Perhaps keep them ordered?
> +
> +#include
> +#include
> +
> +#include
ASM? Hmm...
> +static int cmpfunc(const void *a, const void *b)
> +{
> + return (*(int *)a - *(int *)b);
Surrounding parens are not needed.
> +}
> +
> +static int ad5758_find_closest_match(const int *array,
> +unsigned int size, int val)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if (val <= array[i])
> + return i;
> + }
> +
> + return size - 1;
> +}
Isn't it what bsearch() covers as well?
> + do {
> + ret = ad5758_spi_reg_read(st, reg);
> + if (ret < 0)
> + return ret;
> +
> + if (!(ret & mask))
> + return 0;
> +
> + udelay(100);
If it's not called from atomic context, perhaps switch to usleep_range() ?
> + } while (--timeout);
> +static int ad5758_soft_reset(struct ad5758_state *st)
> +{
> + int ret;
> +
> + ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_1);
> + if (ret < 0)
> + return ret;
> +
> + ret = ad5758_spi_reg_write(st, AD5758_KEY, AD5758_KEY_CODE_RESET_2);
> +
> + /* Perform a software reset and wait 100us */
> + udelay(100);
Ditto.
> +
> + return ret;
> +}
> +static int ad5758_find_out_range(struct ad5758_state *st,
> +const struct ad5758_range *range,
> +unsigned int size,
> +int min, int max)
> +{
> + int i;
> +
> + for (i = 0; i < size; i++) {
> + if ((min == range[i].min) && (max == range[i].max)) {
> + st->out_range.reg = range[i].reg;
> + st->out_range.min = range[i].min;
> + st->out_range.max = range[i].max;
> +
> + return 0;
> + }
> + }
One more candidate to use bsearch().
> +
> + return -EINVAL;
> +}
> + index = (int *) bsearch(, ad5758_dc_dc_ilim,
> + ARRAY_SIZE(ad5758_dc_dc_ilim),
> + sizeof(int), cmpfunc);
I'm not sure you need that casting.
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v2] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM
On Fri, Jun 29, 2018 at 9:19 PM, Bjorn Helgaas wrote: > On Fri, Jun 29, 2018 at 10:34:40AM +0200, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki >> >> It is reported that commit c62ec4610c40 (PM / core: Fix direct_complete >> handling for devices with no callbacks) introduced a system suspend >> regression on Samsung 305V4A by allowing a PCI bridge (not a PCIe >> port) to stay in D3 over suspend-to-RAM, which is a side effect of >> setting power.direct_complete for the children of that bridge that >> have no PM callbacks. >> >> On the majority of systems PCI bridges are not allowed to be >> runtime-suspended (the power/control sysfs attribute is set to "on" >> for them by default), but user space can change that setting and if >> it does so and a given bridge has no children with PM callbacks, the >> direct_complete optimization will be applied to it and it will stay >> in suspend over system suspend. Apparently, that confuses the >> platform firmware on the affected machine and that may very well >> happen elsewhere, so avoid the direct_complete optimization for >> PCI bridges with no drivers (if there is a driver, it should take >> care of the PM handling) on suspend-to-RAM altogether (that should >> not matter for suspend-to-idle as platform firmware is not involved >> in it). >> >> Fixes: c62ec4610c40 (PM / core: Fix direct_complete handling for devices >> with no callbacks) >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199941 >> Reported-by: nb.n0...@gmail.com >> Tested-by: nb.n0...@gmail.com >> Reviewed-by: Mika Westerberg >> Acked-by: Bjorn Helgaas >> Signed-off-by: Rafael J. Wysocki >> --- >> >> -> v2: Make the patch work for PCI bridges without ACPI companion objects. >> >> Since that doesn't really change the idea etc, but only makes the check >> to be made earlier, I've added the Reviewed-by and Acked-by tags given to >> the previous version. If that's not right, please let me know. > > Keeping my ack is fine, and I assume you'll apply this. I will, thanks! > It would be nice to see that ACPI spec reference (the spec version and > section number, not necessarily all the text) in the changelog or > maybe even a comment in the code. OK
Re: [PATCH v2] PCI / ACPI / PM: Resume bridges w/o drivers on suspend-to-RAM
On Fri, Jun 29, 2018 at 9:19 PM, Bjorn Helgaas wrote: > On Fri, Jun 29, 2018 at 10:34:40AM +0200, Rafael J. Wysocki wrote: >> From: Rafael J. Wysocki >> >> It is reported that commit c62ec4610c40 (PM / core: Fix direct_complete >> handling for devices with no callbacks) introduced a system suspend >> regression on Samsung 305V4A by allowing a PCI bridge (not a PCIe >> port) to stay in D3 over suspend-to-RAM, which is a side effect of >> setting power.direct_complete for the children of that bridge that >> have no PM callbacks. >> >> On the majority of systems PCI bridges are not allowed to be >> runtime-suspended (the power/control sysfs attribute is set to "on" >> for them by default), but user space can change that setting and if >> it does so and a given bridge has no children with PM callbacks, the >> direct_complete optimization will be applied to it and it will stay >> in suspend over system suspend. Apparently, that confuses the >> platform firmware on the affected machine and that may very well >> happen elsewhere, so avoid the direct_complete optimization for >> PCI bridges with no drivers (if there is a driver, it should take >> care of the PM handling) on suspend-to-RAM altogether (that should >> not matter for suspend-to-idle as platform firmware is not involved >> in it). >> >> Fixes: c62ec4610c40 (PM / core: Fix direct_complete handling for devices >> with no callbacks) >> Link: https://bugzilla.kernel.org/show_bug.cgi?id=199941 >> Reported-by: nb.n0...@gmail.com >> Tested-by: nb.n0...@gmail.com >> Reviewed-by: Mika Westerberg >> Acked-by: Bjorn Helgaas >> Signed-off-by: Rafael J. Wysocki >> --- >> >> -> v2: Make the patch work for PCI bridges without ACPI companion objects. >> >> Since that doesn't really change the idea etc, but only makes the check >> to be made earlier, I've added the Reviewed-by and Acked-by tags given to >> the previous version. If that's not right, please let me know. > > Keeping my ack is fine, and I assume you'll apply this. I will, thanks! > It would be nice to see that ACPI spec reference (the spec version and > section number, not necessarily all the text) in the changelog or > maybe even a comment in the code. OK
Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On Sat, Jun 30, 2018 at 4:33 PM, Manivannan Sadhasivam
wrote:
> Add Actions Semiconductor Owl family S900 I2C driver.
Thanks for an update. Few left comments and it would LGTM.
> +static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
> +{
> + mdelay(1);
But now, since it's not used in atomic context, we may switch to
usleep_range() / msleep() instead.
> + owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> + OWL_I2C_CTL_EN, true);
> +
> + /* Wait 50ms for FIFO reset complete */
> + do {
> + mdelay(1);
Especially in this case it's very important.
> + } while (timeout++ < OWL_I2C_MAX_RETRIES);
> +}
> + val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
> + (i2c_dev->bus_freq * 16);
This is effectively DIV_ROUND_UP(->clk_rate, ->bus_freq * 16).
> + /*
> +* By default, 0 will be returned if interrupt occurred but no
> +* read or write happened. Else if msg_ptr equals to message length,
> +* message count will be returned.
> +*/
> + if (i2c_dev->msg_ptr == msg->len)
> + ret = num;
I dunno if
ret = ->msg_ptr == len ? num : 0;
would be slightly more explicit (yes, I aware about ret == 0).
Up to you to choose.
> + /* We support only frequencies of 100k and 400k for now */
> + if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
> + i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {
I think it should be != in the second case as well.
> + dev_err(dev, "invalid clock-frequency %d\n",
> i2c_dev->bus_freq);
> + return -EINVAL;
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 5/6] i2c: Add Actions Semiconductor Owl family S900 I2C driver
On Sat, Jun 30, 2018 at 4:33 PM, Manivannan Sadhasivam
wrote:
> Add Actions Semiconductor Owl family S900 I2C driver.
Thanks for an update. Few left comments and it would LGTM.
> +static int owl_i2c_reset(struct owl_i2c_dev *i2c_dev)
> +{
> + mdelay(1);
But now, since it's not used in atomic context, we may switch to
usleep_range() / msleep() instead.
> + owl_i2c_update_reg(i2c_dev->base + OWL_I2C_REG_CTL,
> + OWL_I2C_CTL_EN, true);
> +
> + /* Wait 50ms for FIFO reset complete */
> + do {
> + mdelay(1);
Especially in this case it's very important.
> + } while (timeout++ < OWL_I2C_MAX_RETRIES);
> +}
> + val = (i2c_dev->clk_rate + i2c_dev->bus_freq * 16 - 1) /
> + (i2c_dev->bus_freq * 16);
This is effectively DIV_ROUND_UP(->clk_rate, ->bus_freq * 16).
> + /*
> +* By default, 0 will be returned if interrupt occurred but no
> +* read or write happened. Else if msg_ptr equals to message length,
> +* message count will be returned.
> +*/
> + if (i2c_dev->msg_ptr == msg->len)
> + ret = num;
I dunno if
ret = ->msg_ptr == len ? num : 0;
would be slightly more explicit (yes, I aware about ret == 0).
Up to you to choose.
> + /* We support only frequencies of 100k and 400k for now */
> + if (i2c_dev->bus_freq != OWL_I2C_DEF_SPEED_HZ &&
> + i2c_dev->bus_freq > OWL_I2C_MAX_SPEED_HZ) {
I think it should be != in the second case as well.
> + dev_err(dev, "invalid clock-frequency %d\n",
> i2c_dev->bus_freq);
> + return -EINVAL;
> + }
--
With Best Regards,
Andy Shevchenko
Re: [PATCH v3 2/6] arm64: dts: actions: Add Actions Semiconductor S900 I2C controller nodes
On June 30, 2018 3:33:26 PM GMT+02:00, Manivannan Sadhasivam
wrote:
>Add I2C controller nodes for Actions Semiconductor S900 SoC.
>
>Signed-off-by: Manivannan Sadhasivam
>---
> arch/arm64/boot/dts/actions/s900.dtsi | 60 +++
> 1 file changed, 60 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/actions/s900.dtsi
>b/arch/arm64/boot/dts/actions/s900.dtsi
>index 7ae8b931f000..6f7b89edbe4d 100644
>--- a/arch/arm64/boot/dts/actions/s900.dtsi
>+++ b/arch/arm64/boot/dts/actions/s900.dtsi
>@@ -174,6 +174,66 @@
> #clock-cells = <1>;
> };
>
>+ i2c0: i2c@e017 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe017 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ pinctrl-names = "default";
>+ pinctrl-0 = <_default>;
>+ };
>+
>+ i2c1: i2c@e0172000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0172000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ pinctrl-names = "default";
>+ pinctrl-0 = <_default>;
>+ };
>+
>+ i2c2: i2c@e0174000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0174000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ pinctrl-names = "default";
>+ pinctrl-0 = <_default>;
>+ };
>+
>+ i2c3: i2c@e0176000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0176000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ };
>+
>+ i2c4: i2c@e0178000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0178000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ };
>+
>+ i2c5: i2c@e017a000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe017a000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ };
>+
> pinctrl: pinctrl@e01b {
> compatible = "actions,s900-pinctrl";
> reg = <0x0 0xe01b 0x0 0x1000>;
This patch *still* depends on patch 3/6. You need to reorder the series so that
it is properly bisectable.
Cheers,
Peter
Re: [PATCH v3 2/6] arm64: dts: actions: Add Actions Semiconductor S900 I2C controller nodes
On June 30, 2018 3:33:26 PM GMT+02:00, Manivannan Sadhasivam
wrote:
>Add I2C controller nodes for Actions Semiconductor S900 SoC.
>
>Signed-off-by: Manivannan Sadhasivam
>---
> arch/arm64/boot/dts/actions/s900.dtsi | 60 +++
> 1 file changed, 60 insertions(+)
>
>diff --git a/arch/arm64/boot/dts/actions/s900.dtsi
>b/arch/arm64/boot/dts/actions/s900.dtsi
>index 7ae8b931f000..6f7b89edbe4d 100644
>--- a/arch/arm64/boot/dts/actions/s900.dtsi
>+++ b/arch/arm64/boot/dts/actions/s900.dtsi
>@@ -174,6 +174,66 @@
> #clock-cells = <1>;
> };
>
>+ i2c0: i2c@e017 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe017 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ pinctrl-names = "default";
>+ pinctrl-0 = <_default>;
>+ };
>+
>+ i2c1: i2c@e0172000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0172000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ pinctrl-names = "default";
>+ pinctrl-0 = <_default>;
>+ };
>+
>+ i2c2: i2c@e0174000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0174000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ pinctrl-names = "default";
>+ pinctrl-0 = <_default>;
>+ };
>+
>+ i2c3: i2c@e0176000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0176000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ };
>+
>+ i2c4: i2c@e0178000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe0178000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ };
>+
>+ i2c5: i2c@e017a000 {
>+ compatible = "actions,s900-i2c";
>+ reg = <0 0xe017a000 0 0x1000>;
>+ interrupts = ;
>+ #address-cells = <1>;
>+ #size-cells = <0>;
>+ status = "disabled";
>+ };
>+
> pinctrl: pinctrl@e01b {
> compatible = "actions,s900-pinctrl";
> reg = <0x0 0xe01b 0x0 0x1000>;
This patch *still* depends on patch 3/6. You need to reorder the series so that
it is properly bisectable.
Cheers,
Peter
Re: [PATCH v3] add param that allows bootline control of hardened usercopy
On 06/30/2018 04:11 PM, Kees Cook wrote:
> On Wed, Jun 27, 2018 at 5:07 AM, Chris von Recklinghausen
> wrote:
>> Enabling HARDENED_USER_COPY causes measurable regressions in
> nit: HARDENED_USERCOPY
>
>> networking performance, up to 8% under UDP flood.
>>
>> I'm running an a small packet UDP flood using pktgen vs. a host b2b
>> connected. On the receiver side the UDP packets are processed by a
>> simple user space process that just reads and drops them:
>>
>> https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c
>>
>> Not very useful from a functional PoV, but it helps to pin-point
>> bottlenecks in the networking stack.
>>
>> When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
>> regression in the receive tput, compared to the same kernel without
>> this option enabled.
>>
>> With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
>> cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
>>
>> The call-chain is:
>>
>> __GI___libc_recvfrom
>> entry_SYSCALL_64_after_hwframe
>> do_syscall_64
>> __x64_sys_recvfrom
>> __sys_recvfrom
>> inet_recvmsg
>> udp_recvmsg
>> __check_object_size
>>
>> udp_recvmsg() actually calls copy_to_iter() (inlined) and the latters
>> calls check_copy_size() (again, inlined).
> Thanks for including these details!
>
>> A generic distro may want to enable HARDENED_USER_COPY in their default
> same nit :)
Sorry, I'll fix those.
>
>> kernel config, but at the same time, such distro may want to be able to
>> avoid the performance penalties in with the default configuration and
>> disable the stricter check on a per-boot basis.
>>
>> This change adds a boot parameter that conditionally disables
>> HARDENED_USERCOPY at boot time.
>>
>> v2->v3:
>> add benchmark details to commit comments
>> Don't add new item to Documentation/admin-guide/kernel-parameters.rst
>> rename boot param to "hardened_usercopy="
>> update description in Documentation/admin-guide/kernel-parameters.txt
>> static_branch_likely -> static_branch_unlikely
>> add __ro_after_init versions of DEFINE_STATIC_KEY_FALSE,
>> DEFINE_STATIC_KEY_TRUE
>> disable_huc_atboot -> enable_checks (strtobool "on" == true)
>>
>> v1->v2:
>> remove CONFIG_HUC_DEFAULT_OFF
>> default is now enabled, boot param disables
>> move check to __check_object_size so as to not break optimization of
>> __builtin_constant_p()
>> include linux/atomic.h before linux/jump_label.h
>>
>> Signed-off-by: Chris von Recklinghausen
>> ---
>> .../admin-guide/kernel-parameters.txt | 11
>> include/linux/jump_label.h| 6 +
>> include/linux/thread_info.h | 5
>> mm/usercopy.c | 26 +++
>> 4 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index efc7aa7a0670..560d4dc66f02 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -816,6 +816,17 @@
>> disable=[IPV6]
>> See Documentation/networking/ipv6.txt.
>>
>> + hardened_usercopy=
>> +[KNL] Under CONFIG_HARDENED_USERCOPY, whether
>> +hardening is enabled for this boot. Hardened
>> +usercopy checking is used to protect the kernel
>> +from reading or writing beyond known memory
>> +allocation boundaries as a proactive defense
>> +against bounds-checking flaws in the kernel's
>> +copy_to_user()/copy_from_user() interface.
>> +on Perform hardened usercopy checks (default).
>> +off Disable hardened usercopy checks.
>> +
>> disable_radix [PPC]
>> Disable RADIX MMU mode on POWER9
>>
>> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
>> index b46b541c67c4..1a0b6f17a5d6 100644
>> --- a/include/linux/jump_label.h
>> +++ b/include/linux/jump_label.h
>> @@ -299,12 +299,18 @@ struct static_key_false {
>> #define DEFINE_STATIC_KEY_TRUE(name) \
>> struct static_key_true name = STATIC_KEY_TRUE_INIT
>>
>> +#define DEFINE_STATIC_KEY_TRUE_RO(name)\
>> + struct static_key_true name __ro_after_init = STATIC_KEY_TRUE_INIT
>> +
>> #define DECLARE_STATIC_KEY_TRUE(name) \
>> extern struct static_key_true name
>>
>> #define DEFINE_STATIC_KEY_FALSE(name) \
>> struct static_key_false name = STATIC_KEY_FALSE_INIT
>>
>> +#define DEFINE_STATIC_KEY_FALSE_RO(name) \
>> + struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT
>> +
>> #define DECLARE_STATIC_KEY_FALSE(name) \
>> extern struct
Re: [PATCH v3] add param that allows bootline control of hardened usercopy
On 06/30/2018 04:11 PM, Kees Cook wrote:
> On Wed, Jun 27, 2018 at 5:07 AM, Chris von Recklinghausen
> wrote:
>> Enabling HARDENED_USER_COPY causes measurable regressions in
> nit: HARDENED_USERCOPY
>
>> networking performance, up to 8% under UDP flood.
>>
>> I'm running an a small packet UDP flood using pktgen vs. a host b2b
>> connected. On the receiver side the UDP packets are processed by a
>> simple user space process that just reads and drops them:
>>
>> https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c
>>
>> Not very useful from a functional PoV, but it helps to pin-point
>> bottlenecks in the networking stack.
>>
>> When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
>> regression in the receive tput, compared to the same kernel without
>> this option enabled.
>>
>> With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
>> cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
>>
>> The call-chain is:
>>
>> __GI___libc_recvfrom
>> entry_SYSCALL_64_after_hwframe
>> do_syscall_64
>> __x64_sys_recvfrom
>> __sys_recvfrom
>> inet_recvmsg
>> udp_recvmsg
>> __check_object_size
>>
>> udp_recvmsg() actually calls copy_to_iter() (inlined) and the latters
>> calls check_copy_size() (again, inlined).
> Thanks for including these details!
>
>> A generic distro may want to enable HARDENED_USER_COPY in their default
> same nit :)
Sorry, I'll fix those.
>
>> kernel config, but at the same time, such distro may want to be able to
>> avoid the performance penalties in with the default configuration and
>> disable the stricter check on a per-boot basis.
>>
>> This change adds a boot parameter that conditionally disables
>> HARDENED_USERCOPY at boot time.
>>
>> v2->v3:
>> add benchmark details to commit comments
>> Don't add new item to Documentation/admin-guide/kernel-parameters.rst
>> rename boot param to "hardened_usercopy="
>> update description in Documentation/admin-guide/kernel-parameters.txt
>> static_branch_likely -> static_branch_unlikely
>> add __ro_after_init versions of DEFINE_STATIC_KEY_FALSE,
>> DEFINE_STATIC_KEY_TRUE
>> disable_huc_atboot -> enable_checks (strtobool "on" == true)
>>
>> v1->v2:
>> remove CONFIG_HUC_DEFAULT_OFF
>> default is now enabled, boot param disables
>> move check to __check_object_size so as to not break optimization of
>> __builtin_constant_p()
>> include linux/atomic.h before linux/jump_label.h
>>
>> Signed-off-by: Chris von Recklinghausen
>> ---
>> .../admin-guide/kernel-parameters.txt | 11
>> include/linux/jump_label.h| 6 +
>> include/linux/thread_info.h | 5
>> mm/usercopy.c | 26 +++
>> 4 files changed, 48 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt
>> b/Documentation/admin-guide/kernel-parameters.txt
>> index efc7aa7a0670..560d4dc66f02 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -816,6 +816,17 @@
>> disable=[IPV6]
>> See Documentation/networking/ipv6.txt.
>>
>> + hardened_usercopy=
>> +[KNL] Under CONFIG_HARDENED_USERCOPY, whether
>> +hardening is enabled for this boot. Hardened
>> +usercopy checking is used to protect the kernel
>> +from reading or writing beyond known memory
>> +allocation boundaries as a proactive defense
>> +against bounds-checking flaws in the kernel's
>> +copy_to_user()/copy_from_user() interface.
>> +on Perform hardened usercopy checks (default).
>> +off Disable hardened usercopy checks.
>> +
>> disable_radix [PPC]
>> Disable RADIX MMU mode on POWER9
>>
>> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
>> index b46b541c67c4..1a0b6f17a5d6 100644
>> --- a/include/linux/jump_label.h
>> +++ b/include/linux/jump_label.h
>> @@ -299,12 +299,18 @@ struct static_key_false {
>> #define DEFINE_STATIC_KEY_TRUE(name) \
>> struct static_key_true name = STATIC_KEY_TRUE_INIT
>>
>> +#define DEFINE_STATIC_KEY_TRUE_RO(name)\
>> + struct static_key_true name __ro_after_init = STATIC_KEY_TRUE_INIT
>> +
>> #define DECLARE_STATIC_KEY_TRUE(name) \
>> extern struct static_key_true name
>>
>> #define DEFINE_STATIC_KEY_FALSE(name) \
>> struct static_key_false name = STATIC_KEY_FALSE_INIT
>>
>> +#define DEFINE_STATIC_KEY_FALSE_RO(name) \
>> + struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT
>> +
>> #define DECLARE_STATIC_KEY_FALSE(name) \
>> extern struct
Re: /tmp/cctnQ1CM.s:35: Error: .err encountered
On Sat, Jun 30, 2018 at 11:12 AM, Andrew Morton
wrote:
> On Sat, 30 Jun 2018 11:07:20 -0700 Andrew Morton
> wrote:
>
>> On Sat, 30 Jun 2018 12:27:09 +0200 Dmitry Vyukov wrote:
>>
>> > > tree:
>> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>> > > head: 1904148a361a07fb2d7cba1261d1d2c2f33c8d2e
>> > > commit: 758517202bd2e427664857c9f2aa59da36848aca arm: port KCOV to arm
>> > > date: 2 weeks ago
>> > > config: arm-allmodconfig (attached as .config)
>> > > compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
>> > > reproduce:
>> > > wget
>> > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
>> > > -O ~/bin/make.cross
>> > > chmod +x ~/bin/make.cross
>> > > git checkout 758517202bd2e427664857c9f2aa59da36848aca
>> > > # save the attached .config to linux build tree
>> > > GCC_VERSION=7.2.0 make.cross ARCH=arm
>> > >
>> > > All errors (new ones prefixed by >>):
>> > >
>> > >/tmp/cctnQ1CM.s: Assembler messages:
>> > >>> /tmp/cctnQ1CM.s:35: Error: .err encountered
>> > >/tmp/cctnQ1CM.s:36: Error: .err encountered
>> > >/tmp/cctnQ1CM.s:37: Error: .err encountered
>> >
>> > Hi kbuild test robot,
>> >
>> > The fix was mailed more than a month ago, but still not merged into
>> > the tree. That's linux...
>>
>> That was a rather unhelpful email.
>>
>> I've just scanned all your lkml emails since the start of May and
>> cannot find anything which looks like a fix for this issue.
>>
>> Please resend. About three weks ago :(
>
> OK, with a bi of amazing sleuthing I found this from Arnd, which is what
> I presume you're referring to?
>
>
>
> From: Arnd Bergmann
> Subject: ARM: disable KCOV for trusted foundations code
>
> The ARM trusted foundations code is currently broken in linux-next when
> CONFIG_KCOV_INSTRUMENT_ALL is set:
>
> /tmp/ccHdQsCI.s: Assembler messages:
> /tmp/ccHdQsCI.s:37: Error: .err encountered
> /tmp/ccHdQsCI.s:38: Error: .err encountered
> /tmp/ccHdQsCI.s:39: Error: .err encountered
> scripts/Makefile.build:311: recipe for target
> 'arch/arm/firmware/trusted_foundations.o' failed
>
> I could not find a function attribute that lets me disable
> -fsanitize-coverage=trace-pc for just one function, so this turns it off
> for the entire file instead.
>
> Link: http://lkml.kernel.org/r/20180529103636.1535457-1-a...@arndb.de
> Fixes: 758517202bd2e4 ("arm: port KCOV to arm")
> Signed-off-by: Arnd Bergmann
> Cc: Dmitry Vyukov
> Cc: Mark Rutland
> Signed-off-by: Andrew Morton
Solves it on my builder at least. Would be good to get this in.
Acked-by: Olof Johansson
-Olof
Re: /tmp/cctnQ1CM.s:35: Error: .err encountered
On Sat, Jun 30, 2018 at 11:12 AM, Andrew Morton
wrote:
> On Sat, 30 Jun 2018 11:07:20 -0700 Andrew Morton
> wrote:
>
>> On Sat, 30 Jun 2018 12:27:09 +0200 Dmitry Vyukov wrote:
>>
>> > > tree:
>> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
>> > > head: 1904148a361a07fb2d7cba1261d1d2c2f33c8d2e
>> > > commit: 758517202bd2e427664857c9f2aa59da36848aca arm: port KCOV to arm
>> > > date: 2 weeks ago
>> > > config: arm-allmodconfig (attached as .config)
>> > > compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
>> > > reproduce:
>> > > wget
>> > > https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross
>> > > -O ~/bin/make.cross
>> > > chmod +x ~/bin/make.cross
>> > > git checkout 758517202bd2e427664857c9f2aa59da36848aca
>> > > # save the attached .config to linux build tree
>> > > GCC_VERSION=7.2.0 make.cross ARCH=arm
>> > >
>> > > All errors (new ones prefixed by >>):
>> > >
>> > >/tmp/cctnQ1CM.s: Assembler messages:
>> > >>> /tmp/cctnQ1CM.s:35: Error: .err encountered
>> > >/tmp/cctnQ1CM.s:36: Error: .err encountered
>> > >/tmp/cctnQ1CM.s:37: Error: .err encountered
>> >
>> > Hi kbuild test robot,
>> >
>> > The fix was mailed more than a month ago, but still not merged into
>> > the tree. That's linux...
>>
>> That was a rather unhelpful email.
>>
>> I've just scanned all your lkml emails since the start of May and
>> cannot find anything which looks like a fix for this issue.
>>
>> Please resend. About three weks ago :(
>
> OK, with a bi of amazing sleuthing I found this from Arnd, which is what
> I presume you're referring to?
>
>
>
> From: Arnd Bergmann
> Subject: ARM: disable KCOV for trusted foundations code
>
> The ARM trusted foundations code is currently broken in linux-next when
> CONFIG_KCOV_INSTRUMENT_ALL is set:
>
> /tmp/ccHdQsCI.s: Assembler messages:
> /tmp/ccHdQsCI.s:37: Error: .err encountered
> /tmp/ccHdQsCI.s:38: Error: .err encountered
> /tmp/ccHdQsCI.s:39: Error: .err encountered
> scripts/Makefile.build:311: recipe for target
> 'arch/arm/firmware/trusted_foundations.o' failed
>
> I could not find a function attribute that lets me disable
> -fsanitize-coverage=trace-pc for just one function, so this turns it off
> for the entire file instead.
>
> Link: http://lkml.kernel.org/r/20180529103636.1535457-1-a...@arndb.de
> Fixes: 758517202bd2e4 ("arm: port KCOV to arm")
> Signed-off-by: Arnd Bergmann
> Cc: Dmitry Vyukov
> Cc: Mark Rutland
> Signed-off-by: Andrew Morton
Solves it on my builder at least. Would be good to get this in.
Acked-by: Olof Johansson
-Olof
[GIT PULL] parisc architecture code cleanups and fixes
Hi Linus, please pull some patches for the parisc architecture from: git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git parisc-4.18-1 Nothing exiting in this patchset, just small cleanups of header files, kernel now defaults to 4 CPUs when building a SMP kernel, mark 16- and 64-kB page sizes broken and addition of the new io_pgetevents syscall. Thanks, Helge Andy Shevchenko (1): parisc: Convert printk(KERN_LEVEL) to pr_lvl() Helge Deller (6): parisc: Drop struct sigaction from not exported header file parisc: Mark 16kB and 64kB page sizes BROKEN parisc: Default to 4 SMP CPUs parisc: Wire up io_pgetevents syscall parisc: Reduce debug output in unwind code parisc: Build kernel without -ffunction-sections arch/parisc/Kconfig | 6 +++--- arch/parisc/Makefile | 4 arch/parisc/include/asm/signal.h | 8 arch/parisc/include/uapi/asm/unistd.h | 3 ++- arch/parisc/kernel/drivers.c | 25 + arch/parisc/kernel/syscall_table.S| 1 + arch/parisc/kernel/unwind.c | 4 ++-- 7 files changed, 17 insertions(+), 34 deletions(-)
[GIT PULL] parisc architecture code cleanups and fixes
Hi Linus, please pull some patches for the parisc architecture from: git://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git parisc-4.18-1 Nothing exiting in this patchset, just small cleanups of header files, kernel now defaults to 4 CPUs when building a SMP kernel, mark 16- and 64-kB page sizes broken and addition of the new io_pgetevents syscall. Thanks, Helge Andy Shevchenko (1): parisc: Convert printk(KERN_LEVEL) to pr_lvl() Helge Deller (6): parisc: Drop struct sigaction from not exported header file parisc: Mark 16kB and 64kB page sizes BROKEN parisc: Default to 4 SMP CPUs parisc: Wire up io_pgetevents syscall parisc: Reduce debug output in unwind code parisc: Build kernel without -ffunction-sections arch/parisc/Kconfig | 6 +++--- arch/parisc/Makefile | 4 arch/parisc/include/asm/signal.h | 8 arch/parisc/include/uapi/asm/unistd.h | 3 ++- arch/parisc/kernel/drivers.c | 25 + arch/parisc/kernel/syscall_table.S| 1 + arch/parisc/kernel/unwind.c | 4 ++-- 7 files changed, 17 insertions(+), 34 deletions(-)
Re: [PATCH v3] add param that allows bootline control of hardened usercopy
On Wed, Jun 27, 2018 at 5:07 AM, Chris von Recklinghausen
wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in
nit: HARDENED_USERCOPY
> networking performance, up to 8% under UDP flood.
>
> I'm running an a small packet UDP flood using pktgen vs. a host b2b
> connected. On the receiver side the UDP packets are processed by a
> simple user space process that just reads and drops them:
>
> https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c
>
> Not very useful from a functional PoV, but it helps to pin-point
> bottlenecks in the networking stack.
>
> When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
> regression in the receive tput, compared to the same kernel without
> this option enabled.
>
> With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
> cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
>
> The call-chain is:
>
> __GI___libc_recvfrom
> entry_SYSCALL_64_after_hwframe
> do_syscall_64
> __x64_sys_recvfrom
> __sys_recvfrom
> inet_recvmsg
> udp_recvmsg
> __check_object_size
>
> udp_recvmsg() actually calls copy_to_iter() (inlined) and the latters
> calls check_copy_size() (again, inlined).
Thanks for including these details!
>
> A generic distro may want to enable HARDENED_USER_COPY in their default
same nit :)
> kernel config, but at the same time, such distro may want to be able to
> avoid the performance penalties in with the default configuration and
> disable the stricter check on a per-boot basis.
>
> This change adds a boot parameter that conditionally disables
> HARDENED_USERCOPY at boot time.
>
> v2->v3:
> add benchmark details to commit comments
> Don't add new item to Documentation/admin-guide/kernel-parameters.rst
> rename boot param to "hardened_usercopy="
> update description in Documentation/admin-guide/kernel-parameters.txt
> static_branch_likely -> static_branch_unlikely
> add __ro_after_init versions of DEFINE_STATIC_KEY_FALSE,
> DEFINE_STATIC_KEY_TRUE
> disable_huc_atboot -> enable_checks (strtobool "on" == true)
>
> v1->v2:
> remove CONFIG_HUC_DEFAULT_OFF
> default is now enabled, boot param disables
> move check to __check_object_size so as to not break optimization of
> __builtin_constant_p()
> include linux/atomic.h before linux/jump_label.h
>
> Signed-off-by: Chris von Recklinghausen
> ---
> .../admin-guide/kernel-parameters.txt | 11
> include/linux/jump_label.h| 6 +
> include/linux/thread_info.h | 5
> mm/usercopy.c | 26 +++
> 4 files changed, 48 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index efc7aa7a0670..560d4dc66f02 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -816,6 +816,17 @@
> disable=[IPV6]
> See Documentation/networking/ipv6.txt.
>
> + hardened_usercopy=
> +[KNL] Under CONFIG_HARDENED_USERCOPY, whether
> +hardening is enabled for this boot. Hardened
> +usercopy checking is used to protect the kernel
> +from reading or writing beyond known memory
> +allocation boundaries as a proactive defense
> +against bounds-checking flaws in the kernel's
> +copy_to_user()/copy_from_user() interface.
> +on Perform hardened usercopy checks (default).
> +off Disable hardened usercopy checks.
> +
> disable_radix [PPC]
> Disable RADIX MMU mode on POWER9
>
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index b46b541c67c4..1a0b6f17a5d6 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -299,12 +299,18 @@ struct static_key_false {
> #define DEFINE_STATIC_KEY_TRUE(name) \
> struct static_key_true name = STATIC_KEY_TRUE_INIT
>
> +#define DEFINE_STATIC_KEY_TRUE_RO(name)\
> + struct static_key_true name __ro_after_init = STATIC_KEY_TRUE_INIT
> +
> #define DECLARE_STATIC_KEY_TRUE(name) \
> extern struct static_key_true name
>
> #define DEFINE_STATIC_KEY_FALSE(name) \
> struct static_key_false name = STATIC_KEY_FALSE_INIT
>
> +#define DEFINE_STATIC_KEY_FALSE_RO(name) \
> + struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT
> +
> #define DECLARE_STATIC_KEY_FALSE(name) \
> extern struct static_key_false name
>
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 8d8821b3689a..ab24fe2d3f87 100644
> --- a/include/linux/thread_info.h
> +++
Re: [PATCH v3] add param that allows bootline control of hardened usercopy
On Wed, Jun 27, 2018 at 5:07 AM, Chris von Recklinghausen
wrote:
> Enabling HARDENED_USER_COPY causes measurable regressions in
nit: HARDENED_USERCOPY
> networking performance, up to 8% under UDP flood.
>
> I'm running an a small packet UDP flood using pktgen vs. a host b2b
> connected. On the receiver side the UDP packets are processed by a
> simple user space process that just reads and drops them:
>
> https://github.com/netoptimizer/network-testing/blob/master/src/udp_sink.c
>
> Not very useful from a functional PoV, but it helps to pin-point
> bottlenecks in the networking stack.
>
> When running a kernel with CONFIG_HARDENED_USERCOPY=y, I see a 5-8%
> regression in the receive tput, compared to the same kernel without
> this option enabled.
>
> With CONFIG_HARDENED_USERCOPY=y, perf shows ~6% of CPU time spent
> cumulatively in __check_object_size (~4%) and __virt_addr_valid (~2%).
>
> The call-chain is:
>
> __GI___libc_recvfrom
> entry_SYSCALL_64_after_hwframe
> do_syscall_64
> __x64_sys_recvfrom
> __sys_recvfrom
> inet_recvmsg
> udp_recvmsg
> __check_object_size
>
> udp_recvmsg() actually calls copy_to_iter() (inlined) and the latters
> calls check_copy_size() (again, inlined).
Thanks for including these details!
>
> A generic distro may want to enable HARDENED_USER_COPY in their default
same nit :)
> kernel config, but at the same time, such distro may want to be able to
> avoid the performance penalties in with the default configuration and
> disable the stricter check on a per-boot basis.
>
> This change adds a boot parameter that conditionally disables
> HARDENED_USERCOPY at boot time.
>
> v2->v3:
> add benchmark details to commit comments
> Don't add new item to Documentation/admin-guide/kernel-parameters.rst
> rename boot param to "hardened_usercopy="
> update description in Documentation/admin-guide/kernel-parameters.txt
> static_branch_likely -> static_branch_unlikely
> add __ro_after_init versions of DEFINE_STATIC_KEY_FALSE,
> DEFINE_STATIC_KEY_TRUE
> disable_huc_atboot -> enable_checks (strtobool "on" == true)
>
> v1->v2:
> remove CONFIG_HUC_DEFAULT_OFF
> default is now enabled, boot param disables
> move check to __check_object_size so as to not break optimization of
> __builtin_constant_p()
> include linux/atomic.h before linux/jump_label.h
>
> Signed-off-by: Chris von Recklinghausen
> ---
> .../admin-guide/kernel-parameters.txt | 11
> include/linux/jump_label.h| 6 +
> include/linux/thread_info.h | 5
> mm/usercopy.c | 26 +++
> 4 files changed, 48 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt
> b/Documentation/admin-guide/kernel-parameters.txt
> index efc7aa7a0670..560d4dc66f02 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -816,6 +816,17 @@
> disable=[IPV6]
> See Documentation/networking/ipv6.txt.
>
> + hardened_usercopy=
> +[KNL] Under CONFIG_HARDENED_USERCOPY, whether
> +hardening is enabled for this boot. Hardened
> +usercopy checking is used to protect the kernel
> +from reading or writing beyond known memory
> +allocation boundaries as a proactive defense
> +against bounds-checking flaws in the kernel's
> +copy_to_user()/copy_from_user() interface.
> +on Perform hardened usercopy checks (default).
> +off Disable hardened usercopy checks.
> +
> disable_radix [PPC]
> Disable RADIX MMU mode on POWER9
>
> diff --git a/include/linux/jump_label.h b/include/linux/jump_label.h
> index b46b541c67c4..1a0b6f17a5d6 100644
> --- a/include/linux/jump_label.h
> +++ b/include/linux/jump_label.h
> @@ -299,12 +299,18 @@ struct static_key_false {
> #define DEFINE_STATIC_KEY_TRUE(name) \
> struct static_key_true name = STATIC_KEY_TRUE_INIT
>
> +#define DEFINE_STATIC_KEY_TRUE_RO(name)\
> + struct static_key_true name __ro_after_init = STATIC_KEY_TRUE_INIT
> +
> #define DECLARE_STATIC_KEY_TRUE(name) \
> extern struct static_key_true name
>
> #define DEFINE_STATIC_KEY_FALSE(name) \
> struct static_key_false name = STATIC_KEY_FALSE_INIT
>
> +#define DEFINE_STATIC_KEY_FALSE_RO(name) \
> + struct static_key_false name __ro_after_init = STATIC_KEY_FALSE_INIT
> +
> #define DECLARE_STATIC_KEY_FALSE(name) \
> extern struct static_key_false name
>
> diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
> index 8d8821b3689a..ab24fe2d3f87 100644
> --- a/include/linux/thread_info.h
> +++
Re: [PATCH] PCI/AER: Adopt lspci naming convention for AER prints
On Tue, Jun 26, 2018 at 11:44:15AM -0400, Tyler Baicar wrote:
> lspci uses abbreviated naming for AER error strings. Adopt the
> same naming convention for the AER printing so they match.
>
> Signed-off-by: Tyler Baicar
Applied with Oza's reviewed-by to pci/aer for v4.19, thanks!
> ---
> drivers/pci/pcie/aer.c | 46 +++---
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index a2e8838..08a5219 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -459,22 +459,22 @@ int pci_aer_init(struct pci_dev *dev)
> };
>
> static const char *aer_correctable_error_string[] = {
> - "Receiver Error", /* Bit Position 0 */
> + "RxErr",/* Bit Position 0 */
> NULL,
> NULL,
> NULL,
> NULL,
> NULL,
> - "Bad TLP", /* Bit Position 6 */
> - "Bad DLLP", /* Bit Position 7 */
> - "RELAY_NUM Rollover", /* Bit Position 8 */
> + "BadTLP", /* Bit Position 6 */
> + "BadDLLP", /* Bit Position 7 */
> + "Rollover", /* Bit Position 8 */
> NULL,
> NULL,
> NULL,
> - "Replay Timer Timeout", /* Bit Position 12 */
> - "Advisory Non-Fatal", /* Bit Position 13 */
> - "Corrected Internal Error", /* Bit Position 14 */
> - "Header Log Overflow", /* Bit Position 15 */
> + "Timeout", /* Bit Position 12 */
> + "NonFatalErr", /* Bit Position 13 */
> + "CorrIntErr", /* Bit Position 14 */
> + "HeaderOF", /* Bit Position 15 */
> };
>
> static const char *aer_uncorrectable_error_string[] = {
> @@ -482,28 +482,28 @@ int pci_aer_init(struct pci_dev *dev)
> NULL,
> NULL,
> NULL,
> - "Data Link Protocol", /* Bit Position 4 */
> - "Surprise Down Error", /* Bit Position 5 */
> + "DLP", /* Bit Position 4 */
> + "SDES", /* Bit Position 5 */
> NULL,
> NULL,
> NULL,
> NULL,
> NULL,
> NULL,
> - "Poisoned TLP", /* Bit Position 12 */
> - "Flow Control Protocol",/* Bit Position 13 */
> - "Completion Timeout", /* Bit Position 14 */
> - "Completer Abort", /* Bit Position 15 */
> - "Unexpected Completion",/* Bit Position 16 */
> - "Receiver Overflow",/* Bit Position 17 */
> - "Malformed TLP",/* Bit Position 18 */
> + "TLP", /* Bit Position 12 */
> + "FCP", /* Bit Position 13 */
> + "CmpltTO", /* Bit Position 14 */
> + "CmpltAbrt",/* Bit Position 15 */
> + "UnxCmplt", /* Bit Position 16 */
> + "RxOF", /* Bit Position 17 */
> + "MalfTLP", /* Bit Position 18 */
> "ECRC", /* Bit Position 19 */
> - "Unsupported Request", /* Bit Position 20 */
> - "ACS Violation",/* Bit Position 21 */
> - "Uncorrectable Internal Error", /* Bit Position 22 */
> - "MC Blocked TLP", /* Bit Position 23 */
> - "AtomicOp Egress Blocked", /* Bit Position 24 */
> - "TLP Prefix Blocked Error", /* Bit Position 25 */
> + "UnsupReq", /* Bit Position 20 */
> + "ACSViol", /* Bit Position 21 */
> + "UncorrIntErr", /* Bit Position 22 */
> + "BlockedTLP", /* Bit Position 23 */
> + "AtomicOpBlocked", /* Bit Position 24 */
> + "TLPBlockedErr",/* Bit Position 25 */
> };
>
> static const char *aer_agent_string[] = {
> --
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
>
Re: [PATCH] PCI/AER: Adopt lspci naming convention for AER prints
On Tue, Jun 26, 2018 at 11:44:15AM -0400, Tyler Baicar wrote:
> lspci uses abbreviated naming for AER error strings. Adopt the
> same naming convention for the AER printing so they match.
>
> Signed-off-by: Tyler Baicar
Applied with Oza's reviewed-by to pci/aer for v4.19, thanks!
> ---
> drivers/pci/pcie/aer.c | 46 +++---
> 1 file changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c
> index a2e8838..08a5219 100644
> --- a/drivers/pci/pcie/aer.c
> +++ b/drivers/pci/pcie/aer.c
> @@ -459,22 +459,22 @@ int pci_aer_init(struct pci_dev *dev)
> };
>
> static const char *aer_correctable_error_string[] = {
> - "Receiver Error", /* Bit Position 0 */
> + "RxErr",/* Bit Position 0 */
> NULL,
> NULL,
> NULL,
> NULL,
> NULL,
> - "Bad TLP", /* Bit Position 6 */
> - "Bad DLLP", /* Bit Position 7 */
> - "RELAY_NUM Rollover", /* Bit Position 8 */
> + "BadTLP", /* Bit Position 6 */
> + "BadDLLP", /* Bit Position 7 */
> + "Rollover", /* Bit Position 8 */
> NULL,
> NULL,
> NULL,
> - "Replay Timer Timeout", /* Bit Position 12 */
> - "Advisory Non-Fatal", /* Bit Position 13 */
> - "Corrected Internal Error", /* Bit Position 14 */
> - "Header Log Overflow", /* Bit Position 15 */
> + "Timeout", /* Bit Position 12 */
> + "NonFatalErr", /* Bit Position 13 */
> + "CorrIntErr", /* Bit Position 14 */
> + "HeaderOF", /* Bit Position 15 */
> };
>
> static const char *aer_uncorrectable_error_string[] = {
> @@ -482,28 +482,28 @@ int pci_aer_init(struct pci_dev *dev)
> NULL,
> NULL,
> NULL,
> - "Data Link Protocol", /* Bit Position 4 */
> - "Surprise Down Error", /* Bit Position 5 */
> + "DLP", /* Bit Position 4 */
> + "SDES", /* Bit Position 5 */
> NULL,
> NULL,
> NULL,
> NULL,
> NULL,
> NULL,
> - "Poisoned TLP", /* Bit Position 12 */
> - "Flow Control Protocol",/* Bit Position 13 */
> - "Completion Timeout", /* Bit Position 14 */
> - "Completer Abort", /* Bit Position 15 */
> - "Unexpected Completion",/* Bit Position 16 */
> - "Receiver Overflow",/* Bit Position 17 */
> - "Malformed TLP",/* Bit Position 18 */
> + "TLP", /* Bit Position 12 */
> + "FCP", /* Bit Position 13 */
> + "CmpltTO", /* Bit Position 14 */
> + "CmpltAbrt",/* Bit Position 15 */
> + "UnxCmplt", /* Bit Position 16 */
> + "RxOF", /* Bit Position 17 */
> + "MalfTLP", /* Bit Position 18 */
> "ECRC", /* Bit Position 19 */
> - "Unsupported Request", /* Bit Position 20 */
> - "ACS Violation",/* Bit Position 21 */
> - "Uncorrectable Internal Error", /* Bit Position 22 */
> - "MC Blocked TLP", /* Bit Position 23 */
> - "AtomicOp Egress Blocked", /* Bit Position 24 */
> - "TLP Prefix Blocked Error", /* Bit Position 25 */
> + "UnsupReq", /* Bit Position 20 */
> + "ACSViol", /* Bit Position 21 */
> + "UncorrIntErr", /* Bit Position 22 */
> + "BlockedTLP", /* Bit Position 23 */
> + "AtomicOpBlocked", /* Bit Position 24 */
> + "TLPBlockedErr",/* Bit Position 25 */
> };
>
> static const char *aer_agent_string[] = {
> --
> Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
> Technologies, Inc.
> Qualcomm Technologies, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.
>
Re: [PATCH 1/2] drivers: core: Don't try to use a dead glue_dir
On Thu, Jun 28, 2018 at 7:21 PM Benjamin Herrenschmidt wrote: > > Under some circumstances (such as when using kobject debugging) > a gluedir whose kref is 0 might remain in the class kset for > a long time. The reason is that we don't actively remove glue > dirs when they become empty, but instead rely on the implicit > removal done by kobject_release(), which can happen some amount > of time after the last kobject_put(). > > Using such a dead object is a bad idea and will lead to warnings > and crashes. So with the other patch in mind, here's my comments on this one. Pick one of two scenarios: (a) it's obviously correct. We obviously can *not* take an object with a zero refcount, because it is already been scheduled for kobject_cleanup(), and incrementing the refcount is simply fundamentally wrong, because incrementing the refcount won't unschedule the deletion of the object. (b) the patch is wrong, and our "kobject_get()" should cancel the kobject_cleanup() instead. There are problems with both of the above cases. The "patch is obviously correct" case leads to another issue: why would kobject_get() _ever_ succeed on an object wioth a zero refcount? IOW, why do we have kobject_get() vs kobject_get_unless_zero() in the first place? It is *never* ok to get an kobject with a zero refcount because of the above "it's already scheduled for deletion" issue. The (b) case sounds nice, and would actually fix the problem that patch 2/2 was tryihng to address, and would make CONFIG_DEBUG_KOBJECT_RELEASE work. HOWEVER. It's completely untenable in reality - it's a nightmare from a locking standpoint, because kref_put() literally depends not on locking, but on the exclusive "went to zero". So I think (b) is practically not acceptable. Which means that (a) is the right reaction, and "kobject_get()" on an object with a zero refcount is _always_ wrong. But that says that "yes, the patch is obviously correct", but it also says "the patch should be pointless, because kobject_get() should just _always_ have the semantics of "kobject_get_unless_zero()", and the latter shouldn't even exist. Greg? When would it possibly be valid to do "kobject_get()" on a zero refcount object? I don't see it. But this is all very much your code. Linus
Re: [PATCH 1/2] drivers: core: Don't try to use a dead glue_dir
On Thu, Jun 28, 2018 at 7:21 PM Benjamin Herrenschmidt wrote: > > Under some circumstances (such as when using kobject debugging) > a gluedir whose kref is 0 might remain in the class kset for > a long time. The reason is that we don't actively remove glue > dirs when they become empty, but instead rely on the implicit > removal done by kobject_release(), which can happen some amount > of time after the last kobject_put(). > > Using such a dead object is a bad idea and will lead to warnings > and crashes. So with the other patch in mind, here's my comments on this one. Pick one of two scenarios: (a) it's obviously correct. We obviously can *not* take an object with a zero refcount, because it is already been scheduled for kobject_cleanup(), and incrementing the refcount is simply fundamentally wrong, because incrementing the refcount won't unschedule the deletion of the object. (b) the patch is wrong, and our "kobject_get()" should cancel the kobject_cleanup() instead. There are problems with both of the above cases. The "patch is obviously correct" case leads to another issue: why would kobject_get() _ever_ succeed on an object wioth a zero refcount? IOW, why do we have kobject_get() vs kobject_get_unless_zero() in the first place? It is *never* ok to get an kobject with a zero refcount because of the above "it's already scheduled for deletion" issue. The (b) case sounds nice, and would actually fix the problem that patch 2/2 was tryihng to address, and would make CONFIG_DEBUG_KOBJECT_RELEASE work. HOWEVER. It's completely untenable in reality - it's a nightmare from a locking standpoint, because kref_put() literally depends not on locking, but on the exclusive "went to zero". So I think (b) is practically not acceptable. Which means that (a) is the right reaction, and "kobject_get()" on an object with a zero refcount is _always_ wrong. But that says that "yes, the patch is obviously correct", but it also says "the patch should be pointless, because kobject_get() should just _always_ have the semantics of "kobject_get_unless_zero()", and the latter shouldn't even exist. Greg? When would it possibly be valid to do "kobject_get()" on a zero refcount object? I don't see it. But this is all very much your code. Linus
Re: [PATCH] staging: mt7621-pinctrl: Replaces "unsigned" with "unsigned int", fixes mixed indentation, and puts spaces after commas.
On Sat, Jun 30, 2018 at 01:17:29PM -0400, Peter Vernia wrote: > Signed-off-by: Peter Vernia The subject is over 72 characters or whatever the limit is and there isn't a commit message. regards, dan carpenter
Re: [PATCH] staging: mt7621-pinctrl: Replaces "unsigned" with "unsigned int", fixes mixed indentation, and puts spaces after commas.
On Sat, Jun 30, 2018 at 01:17:29PM -0400, Peter Vernia wrote: > Signed-off-by: Peter Vernia The subject is over 72 characters or whatever the limit is and there isn't a commit message. regards, dan carpenter
[GIT PULL] ARM: SoC fixes
Hi Linus, The following changes since commit 86676c4685f774d818f7a6c401c925bbf2a92903: arm64: dts: uniphier: fix widget name of headphone for LD11/LD20 boards (2018-06-27 07:14:47 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git tags/armsoc-fixes for you to fetch changes up to 4ca2f0b945fad835959fa596df47817797987cd2: Merge tag 'davinci-fixes-for-v4.18' of git://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci into fixes (2018-06-29 14:08:27 -0700) ARM: SoC fixes for 4.18-rc A smaller batch for the end of the week (let's see if I can keep the weekly cadence going for once). All medium-grade fixes here, nothing worrisome: - Fixes for some fairly old bugs around SD card write-protect detection and GPIO interrupt assignments on Davinci. - Wifi module suspend fix for Hikey. - Minor DT tweaks to fix inaccuracies for Amlogic platforms, on of which solves booting with third-party u-boot. Adam Ford (1): ARM: davinci: board-da850-evm: fix WP pin polarity for MMC/SD Jerome Brunet (2): ARM64: dts: meson: disable sd-uhs modes on the libretech-cc ARM64: dts: meson-axg: fix ethernet stability issue Keerthy (1): ARM: dts: da850: Fix interrups property for gpio Kevin Hilman (2): ARM64: dts: meson: fix register ranges for SD/eMMC ARM64: dts: meson-gx: fix ATF reserved memory region Martin Blumenstingl (1): ARM64: dts: meson-gxl: fix Mali GPU compatible string Neil Armstrong (1): ARM64: dts: meson-gxl-s905x-p212: Add phy-supply for usb0 Olof Johansson (3): Merge tag 'amlogic-fixes' of https://git.kernel.org/.../khilman/linux-amlogic into fixes Merge tag 'hisi-fixes-for-4.18' of git://github.com/hisilicon/linux-hisi into fixes Merge tag 'davinci-fixes-for-v4.18' of git://git.kernel.org/.../nsekhar/linux-davinci into fixes oscardagrach (2): arm64: dts: hikey: Define wl1835 power capabilities arm64: dts: hikey960: Define wl1837 power capabilities arch/arm/boot/dts/da850.dtsi | 6 +- arch/arm/mach-davinci/board-da850-evm.c | 2 +- arch/arm64/boot/dts/amlogic/meson-axg-s400.dts| 15 ++- arch/arm64/boot/dts/amlogic/meson-axg.dtsi| 4 ++-- arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 12 +--- arch/arm64/boot/dts/amlogic/meson-gxl-mali.dtsi | 2 +- .../boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts | 3 --- arch/arm64/boot/dts/amlogic/meson-gxl-s905x-p212.dtsi | 7 +++ arch/arm64/boot/dts/amlogic/meson-gxl.dtsi| 8 arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 2 ++ arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts| 2 ++ 11 files changed, 39 insertions(+), 24 deletions(-)
[GIT PULL] ARM: SoC fixes
Hi Linus, The following changes since commit 86676c4685f774d818f7a6c401c925bbf2a92903: arm64: dts: uniphier: fix widget name of headphone for LD11/LD20 boards (2018-06-27 07:14:47 -0700) are available in the git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc.git tags/armsoc-fixes for you to fetch changes up to 4ca2f0b945fad835959fa596df47817797987cd2: Merge tag 'davinci-fixes-for-v4.18' of git://git.kernel.org/pub/scm/linux/kernel/git/nsekhar/linux-davinci into fixes (2018-06-29 14:08:27 -0700) ARM: SoC fixes for 4.18-rc A smaller batch for the end of the week (let's see if I can keep the weekly cadence going for once). All medium-grade fixes here, nothing worrisome: - Fixes for some fairly old bugs around SD card write-protect detection and GPIO interrupt assignments on Davinci. - Wifi module suspend fix for Hikey. - Minor DT tweaks to fix inaccuracies for Amlogic platforms, on of which solves booting with third-party u-boot. Adam Ford (1): ARM: davinci: board-da850-evm: fix WP pin polarity for MMC/SD Jerome Brunet (2): ARM64: dts: meson: disable sd-uhs modes on the libretech-cc ARM64: dts: meson-axg: fix ethernet stability issue Keerthy (1): ARM: dts: da850: Fix interrups property for gpio Kevin Hilman (2): ARM64: dts: meson: fix register ranges for SD/eMMC ARM64: dts: meson-gx: fix ATF reserved memory region Martin Blumenstingl (1): ARM64: dts: meson-gxl: fix Mali GPU compatible string Neil Armstrong (1): ARM64: dts: meson-gxl-s905x-p212: Add phy-supply for usb0 Olof Johansson (3): Merge tag 'amlogic-fixes' of https://git.kernel.org/.../khilman/linux-amlogic into fixes Merge tag 'hisi-fixes-for-4.18' of git://github.com/hisilicon/linux-hisi into fixes Merge tag 'davinci-fixes-for-v4.18' of git://git.kernel.org/.../nsekhar/linux-davinci into fixes oscardagrach (2): arm64: dts: hikey: Define wl1835 power capabilities arm64: dts: hikey960: Define wl1837 power capabilities arch/arm/boot/dts/da850.dtsi | 6 +- arch/arm/mach-davinci/board-da850-evm.c | 2 +- arch/arm64/boot/dts/amlogic/meson-axg-s400.dts| 15 ++- arch/arm64/boot/dts/amlogic/meson-axg.dtsi| 4 ++-- arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 12 +--- arch/arm64/boot/dts/amlogic/meson-gxl-mali.dtsi | 2 +- .../boot/dts/amlogic/meson-gxl-s905x-libretech-cc.dts | 3 --- arch/arm64/boot/dts/amlogic/meson-gxl-s905x-p212.dtsi | 7 +++ arch/arm64/boot/dts/amlogic/meson-gxl.dtsi| 8 arch/arm64/boot/dts/hisilicon/hi3660-hikey960.dts | 2 ++ arch/arm64/boot/dts/hisilicon/hi6220-hikey.dts| 2 ++ 11 files changed, 39 insertions(+), 24 deletions(-)
Re: [PATCH V3] PCI: Enable PASID when End-to-End TLP is supported by all bridges
On Sat, Jun 30, 2018 at 11:24:24AM -0400, Sinan Kaya wrote:
> A PCIe endpoint carries the process address space identifier (PASID) in
> the TLP prefix as part of the memory read/write transaction. The address
> information in the TLP is relevant only for a given PASID context.
>
> An IOMMU takes PASID value and the address information from the
> TLP to look up the physical address in the system.
>
> PASID is an End-End TLP Prefix (PCIe r4.0, sec 6.20). Sec 2.2.10.2 says
>
> It is an error to receive a TLP with an End-End TLP Prefix by a
> Receiver that does not support End-End TLP Prefixes. A TLP in
> violation of this rule is handled as a Malformed TLP. This is a
> reported error associated with the Receiving Port (see Section 6.2).
>
> Prevent error condition by proactively requiring End-to-End TLP
> prefix to be supported on the entire data path between the endpoint and
> the root port before enabling PASID.
>
> Signed-off-by: Sinan Kaya
Applied to pci/virtualization for v4.19, thanks!
> ---
> drivers/pci/ats.c | 3 +++
> drivers/pci/probe.c | 24
> include/linux/pci.h | 1 +
> include/uapi/linux/pci_regs.h | 1 +
> 4 files changed, 29 insertions(+)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 4923a2a..5b78f3b 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -273,6 +273,9 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
> if (WARN_ON(pdev->pasid_enabled))
> return -EBUSY;
>
> + if (!pdev->eetlp_prefix_path)
> + return -EINVAL;
> +
> pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
> if (!pos)
> return -EINVAL;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e3..39fd3ac 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2042,6 +2042,29 @@ static void pci_configure_ltr(struct pci_dev *dev)
> #endif
> }
>
> +static void pci_configure_eetlp_prefix(struct pci_dev *dev)
> +{
> +#ifdef CONFIG_PCI_PASID
> + struct pci_dev *bridge;
> + u32 cap;
> +
> + if (!pci_is_pcie(dev))
> + return;
> +
> + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, );
> + if (!(cap & PCI_EXP_DEVCAP2_E2ETLP))
> + return;
> +
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> + dev->eetlp_prefix_path = 1;
> + else {
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->eetlp_prefix_path)
> + dev->eetlp_prefix_path = 1;
> + }
> +#endif
> +}
> +
> static void pci_configure_device(struct pci_dev *dev)
> {
> struct hotplug_params hpp;
> @@ -2051,6 +2074,7 @@ static void pci_configure_device(struct pci_dev *dev)
> pci_configure_extended_tags(dev, NULL);
> pci_configure_relaxed_ordering(dev);
> pci_configure_ltr(dev);
> + pci_configure_eetlp_prefix(dev);
>
> memset(, 0, sizeof(hpp));
> ret = pci_get_hp_params(dev, );
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 340029b..6ba8184 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -350,6 +350,7 @@ struct pci_dev {
> unsigned intltr_path:1; /* Latency Tolerance Reporting
> supported from root to here */
> #endif
> + unsigned inteetlp_prefix_path:1;/* End-to-End TLP Prefix */
>
> pci_channel_state_t error_state;/* Current connectivity state */
> struct device dev;/* Generic device interface */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 4da87e2..a617ab2 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -636,6 +636,7 @@
> #define PCI_EXP_DEVCAP2_OBFF_MASK 0x000c /* OBFF support mechanism */
> #define PCI_EXP_DEVCAP2_OBFF_MSG0x0004 /* New message signaling */
> #define PCI_EXP_DEVCAP2_OBFF_WAKE 0x0008 /* Re-use WAKE# for OBFF */
> +#define PCI_EXP_DEVCAP2_E2ETLP 0x0020 /* End-to-End TLP
> Prefix */
> #define PCI_EXP_DEVCTL2 40 /* Device Control 2 */
> #define PCI_EXP_DEVCTL2_COMP_TIMEOUT0x000f /* Completion Timeout
> Value */
> #define PCI_EXP_DEVCTL2_COMP_TMOUT_DIS 0x0010 /* Completion Timeout
> Disable */
> --
> 2.7.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH V3] PCI: Enable PASID when End-to-End TLP is supported by all bridges
On Sat, Jun 30, 2018 at 11:24:24AM -0400, Sinan Kaya wrote:
> A PCIe endpoint carries the process address space identifier (PASID) in
> the TLP prefix as part of the memory read/write transaction. The address
> information in the TLP is relevant only for a given PASID context.
>
> An IOMMU takes PASID value and the address information from the
> TLP to look up the physical address in the system.
>
> PASID is an End-End TLP Prefix (PCIe r4.0, sec 6.20). Sec 2.2.10.2 says
>
> It is an error to receive a TLP with an End-End TLP Prefix by a
> Receiver that does not support End-End TLP Prefixes. A TLP in
> violation of this rule is handled as a Malformed TLP. This is a
> reported error associated with the Receiving Port (see Section 6.2).
>
> Prevent error condition by proactively requiring End-to-End TLP
> prefix to be supported on the entire data path between the endpoint and
> the root port before enabling PASID.
>
> Signed-off-by: Sinan Kaya
Applied to pci/virtualization for v4.19, thanks!
> ---
> drivers/pci/ats.c | 3 +++
> drivers/pci/probe.c | 24
> include/linux/pci.h | 1 +
> include/uapi/linux/pci_regs.h | 1 +
> 4 files changed, 29 insertions(+)
>
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index 4923a2a..5b78f3b 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -273,6 +273,9 @@ int pci_enable_pasid(struct pci_dev *pdev, int features)
> if (WARN_ON(pdev->pasid_enabled))
> return -EBUSY;
>
> + if (!pdev->eetlp_prefix_path)
> + return -EINVAL;
> +
> pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_PASID);
> if (!pos)
> return -EINVAL;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index ac876e3..39fd3ac 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2042,6 +2042,29 @@ static void pci_configure_ltr(struct pci_dev *dev)
> #endif
> }
>
> +static void pci_configure_eetlp_prefix(struct pci_dev *dev)
> +{
> +#ifdef CONFIG_PCI_PASID
> + struct pci_dev *bridge;
> + u32 cap;
> +
> + if (!pci_is_pcie(dev))
> + return;
> +
> + pcie_capability_read_dword(dev, PCI_EXP_DEVCAP2, );
> + if (!(cap & PCI_EXP_DEVCAP2_E2ETLP))
> + return;
> +
> + if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT)
> + dev->eetlp_prefix_path = 1;
> + else {
> + bridge = pci_upstream_bridge(dev);
> + if (bridge && bridge->eetlp_prefix_path)
> + dev->eetlp_prefix_path = 1;
> + }
> +#endif
> +}
> +
> static void pci_configure_device(struct pci_dev *dev)
> {
> struct hotplug_params hpp;
> @@ -2051,6 +2074,7 @@ static void pci_configure_device(struct pci_dev *dev)
> pci_configure_extended_tags(dev, NULL);
> pci_configure_relaxed_ordering(dev);
> pci_configure_ltr(dev);
> + pci_configure_eetlp_prefix(dev);
>
> memset(, 0, sizeof(hpp));
> ret = pci_get_hp_params(dev, );
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 340029b..6ba8184 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -350,6 +350,7 @@ struct pci_dev {
> unsigned intltr_path:1; /* Latency Tolerance Reporting
> supported from root to here */
> #endif
> + unsigned inteetlp_prefix_path:1;/* End-to-End TLP Prefix */
>
> pci_channel_state_t error_state;/* Current connectivity state */
> struct device dev;/* Generic device interface */
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 4da87e2..a617ab2 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -636,6 +636,7 @@
> #define PCI_EXP_DEVCAP2_OBFF_MASK 0x000c /* OBFF support mechanism */
> #define PCI_EXP_DEVCAP2_OBFF_MSG0x0004 /* New message signaling */
> #define PCI_EXP_DEVCAP2_OBFF_WAKE 0x0008 /* Re-use WAKE# for OBFF */
> +#define PCI_EXP_DEVCAP2_E2ETLP 0x0020 /* End-to-End TLP
> Prefix */
> #define PCI_EXP_DEVCTL2 40 /* Device Control 2 */
> #define PCI_EXP_DEVCTL2_COMP_TIMEOUT0x000f /* Completion Timeout
> Value */
> #define PCI_EXP_DEVCTL2_COMP_TMOUT_DIS 0x0010 /* Completion Timeout
> Disable */
> --
> 2.7.4
>
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH V2] PCI: Enable PASID when End-to-End TLP is supported by all bridges
On Sat, Jun 30, 2018 at 10:45:21AM -0400, Sinan Kaya wrote: > On 6/29/2018 8:49 PM, Bjorn Helgaas wrote: > > On Tue, Jun 19, 2018 at 10:14:46PM -0400, Sinan Kaya wrote: > >> A PCIe endpoint carries the process address space identifier (PASID) in > >> the TLP prefix as part of the memory read/write transaction. The address > >> information in the TLP is relevant only for a given PASID context. > >> > >> An IOMMU takes PASID value and the address information from the > >> TLP to look up the physical address in the system. > >> > >> If a bridge drops the TLP prefix, the translation agent can resolve the > >> address to an incorrect location and cause data corruption. Prevent > >> this condition by requiring End-to-End TLP prefix to be supported on the > >> entire data path between the endpoint and the root port. > > > > PASID is an End-End TLP Prefix (PCIe r4.0, sec 6.20). Sec 2.2.10.2 says > > > > It is an error to receive a TLP with an End-End TLP Prefix by a > > Receiver that does not support End-End TLP Prefixes. A TLP in > > violation of this rule is handled as a Malformed TLP. This is a > > reported error associated with the Receiving Port (see Section 6.2). > > > > So I agree that we shouldn't enable PASID in an endpoint unless all > > the switch ports leading to it support End-End prefixes. But I don't > > see how a bridge can drop a prefix and cause data corruption -- if it > > doesn't support End-End prefixes, shouldn't the bridge raise a > > Malformed TLP error instead of forwarding the TLP? > > It should under normal circumstances. > > I remember reading that most PCIe switches don't support TLP prefixes. > I don't know if it is because of buggy behavior or if it is just plain > unsupported while dropping the request as Malformed TLP. > > I was trying to be proactive and not enable PASID if the entire path > is incapable. Absolutely, that makes perfect sense. Much better to fail to enable PASID rather than enabling it and seeing Malformed TLP errors or data corruption later. I was trying to figure out if you can actually force data corruption this way. If you can, I'd say that sounds like a buggy switch that we might want to be aware of. Bjorn
Re: [PATCH V2] PCI: Enable PASID when End-to-End TLP is supported by all bridges
On Sat, Jun 30, 2018 at 10:45:21AM -0400, Sinan Kaya wrote: > On 6/29/2018 8:49 PM, Bjorn Helgaas wrote: > > On Tue, Jun 19, 2018 at 10:14:46PM -0400, Sinan Kaya wrote: > >> A PCIe endpoint carries the process address space identifier (PASID) in > >> the TLP prefix as part of the memory read/write transaction. The address > >> information in the TLP is relevant only for a given PASID context. > >> > >> An IOMMU takes PASID value and the address information from the > >> TLP to look up the physical address in the system. > >> > >> If a bridge drops the TLP prefix, the translation agent can resolve the > >> address to an incorrect location and cause data corruption. Prevent > >> this condition by requiring End-to-End TLP prefix to be supported on the > >> entire data path between the endpoint and the root port. > > > > PASID is an End-End TLP Prefix (PCIe r4.0, sec 6.20). Sec 2.2.10.2 says > > > > It is an error to receive a TLP with an End-End TLP Prefix by a > > Receiver that does not support End-End TLP Prefixes. A TLP in > > violation of this rule is handled as a Malformed TLP. This is a > > reported error associated with the Receiving Port (see Section 6.2). > > > > So I agree that we shouldn't enable PASID in an endpoint unless all > > the switch ports leading to it support End-End prefixes. But I don't > > see how a bridge can drop a prefix and cause data corruption -- if it > > doesn't support End-End prefixes, shouldn't the bridge raise a > > Malformed TLP error instead of forwarding the TLP? > > It should under normal circumstances. > > I remember reading that most PCIe switches don't support TLP prefixes. > I don't know if it is because of buggy behavior or if it is just plain > unsupported while dropping the request as Malformed TLP. > > I was trying to be proactive and not enable PASID if the entire path > is incapable. Absolutely, that makes perfect sense. Much better to fail to enable PASID rather than enabling it and seeing Malformed TLP errors or data corruption later. I was trying to figure out if you can actually force data corruption this way. If you can, I'd say that sounds like a buggy switch that we might want to be aware of. Bjorn
Re: [PATCH] vmw_pvrdma: Release netdev when vmxnet3 module is removed
Hi Neil,
I love your patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Neil-Horman/vmw_pvrdma-Release-netdev-when-vmxnet3-module-is-removed/20180628-232414
smatch warnings:
drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c:987 pvrdma_pci_probe() warn:
variable dereferenced before check 'dev->netdev' (see line 985)
#
https://github.com/0day-ci/linux/commit/d5bb424e18e2ecab4ac590a66b0cca9dfb96d3da
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout d5bb424e18e2ecab4ac590a66b0cca9dfb96d3da
vim +987 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
29c8d9eb Adit Ranadive 2016-10-02 784
29c8d9eb Adit Ranadive 2016-10-02 785 static int pvrdma_pci_probe(struct
pci_dev *pdev,
29c8d9eb Adit Ranadive 2016-10-02 786 const
struct pci_device_id *id)
29c8d9eb Adit Ranadive 2016-10-02 787 {
29c8d9eb Adit Ranadive 2016-10-02 788 struct pci_dev *pdev_net;
29c8d9eb Adit Ranadive 2016-10-02 789 struct pvrdma_dev *dev;
29c8d9eb Adit Ranadive 2016-10-02 790 int ret;
29c8d9eb Adit Ranadive 2016-10-02 791 unsigned long start;
29c8d9eb Adit Ranadive 2016-10-02 792 unsigned long len;
29c8d9eb Adit Ranadive 2016-10-02 793 dma_addr_t slot_dma = 0;
29c8d9eb Adit Ranadive 2016-10-02 794
29c8d9eb Adit Ranadive 2016-10-02 795 dev_dbg(>dev,
"initializing driver %s\n", pci_name(pdev));
29c8d9eb Adit Ranadive 2016-10-02 796
29c8d9eb Adit Ranadive 2016-10-02 797 /* Allocate zero-out device */
29c8d9eb Adit Ranadive 2016-10-02 798 dev = (struct pvrdma_dev
*)ib_alloc_device(sizeof(*dev));
29c8d9eb Adit Ranadive 2016-10-02 799 if (!dev) {
29c8d9eb Adit Ranadive 2016-10-02 800 dev_err(>dev,
"failed to allocate IB device\n");
29c8d9eb Adit Ranadive 2016-10-02 801 return -ENOMEM;
29c8d9eb Adit Ranadive 2016-10-02 802 }
29c8d9eb Adit Ranadive 2016-10-02 803
29c8d9eb Adit Ranadive 2016-10-02 804
mutex_lock(_device_list_lock);
29c8d9eb Adit Ranadive 2016-10-02 805 list_add(>device_link,
_device_list);
29c8d9eb Adit Ranadive 2016-10-02 806
mutex_unlock(_device_list_lock);
29c8d9eb Adit Ranadive 2016-10-02 807
29c8d9eb Adit Ranadive 2016-10-02 808 ret = pvrdma_init_device(dev);
29c8d9eb Adit Ranadive 2016-10-02 809 if (ret)
29c8d9eb Adit Ranadive 2016-10-02 810 goto err_free_device;
29c8d9eb Adit Ranadive 2016-10-02 811
29c8d9eb Adit Ranadive 2016-10-02 812 dev->pdev = pdev;
29c8d9eb Adit Ranadive 2016-10-02 813 pci_set_drvdata(pdev, dev);
29c8d9eb Adit Ranadive 2016-10-02 814
29c8d9eb Adit Ranadive 2016-10-02 815 ret = pci_enable_device(pdev);
29c8d9eb Adit Ranadive 2016-10-02 816 if (ret) {
29c8d9eb Adit Ranadive 2016-10-02 817 dev_err(>dev,
"cannot enable PCI device\n");
29c8d9eb Adit Ranadive 2016-10-02 818 goto err_free_device;
29c8d9eb Adit Ranadive 2016-10-02 819 }
29c8d9eb Adit Ranadive 2016-10-02 820
29c8d9eb Adit Ranadive 2016-10-02 821 dev_dbg(>dev, "PCI
resource flags BAR0 %#lx\n",
29c8d9eb Adit Ranadive 2016-10-02 822
pci_resource_flags(pdev, 0));
29c8d9eb Adit Ranadive 2016-10-02 823 dev_dbg(>dev, "PCI
resource len %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 824 (unsigned long
long)pci_resource_len(pdev, 0));
29c8d9eb Adit Ranadive 2016-10-02 825 dev_dbg(>dev, "PCI
resource start %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 826 (unsigned long
long)pci_resource_start(pdev, 0));
29c8d9eb Adit Ranadive 2016-10-02 827 dev_dbg(>dev, "PCI
resource flags BAR1 %#lx\n",
29c8d9eb Adit Ranadive 2016-10-02 828
pci_resource_flags(pdev, 1));
29c8d9eb Adit Ranadive 2016-10-02 829 dev_dbg(>dev, "PCI
resource len %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 830 (unsigned long
long)pci_resource_len(pdev, 1));
29c8d9eb Adit Ranadive 2016-10-02 831 dev_dbg(>dev, "PCI
resource start %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 832 (unsigned long
long)pci_resource_start(pdev, 1));
29c8d9eb Adit Ranadive 2016-10-02 833
29c8d9eb Adit Ranadive 2016-10-02 834 if (!(pci_resource_flags(pdev,
0) & IORESOURCE_MEM) ||
29c8d9eb Adit Ranadive 2016-10-02 835 !(pci_resource_flags(pdev,
1) & IORESOURCE_MEM)) {
29c8d9eb Adit Ranadive 2016-10-02 836 dev_err(>dev,
"PCI BAR region not MMIO\n");
29c8d9eb Adit Ranadive 2016-10-02 837 ret = -ENOMEM;
29c8d9eb Adit Ranadive 2016-10-02 838 goto err_free_device;
29c8d9eb Adit Ranadive 2016-10-02 839 }
29c8d9eb Adit Ranadive 2016-10-02 840
29c8d9eb Adit
Re: [PATCH] vmw_pvrdma: Release netdev when vmxnet3 module is removed
Hi Neil,
I love your patch! Perhaps something to improve:
url:
https://github.com/0day-ci/linux/commits/Neil-Horman/vmw_pvrdma-Release-netdev-when-vmxnet3-module-is-removed/20180628-232414
smatch warnings:
drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c:987 pvrdma_pci_probe() warn:
variable dereferenced before check 'dev->netdev' (see line 985)
#
https://github.com/0day-ci/linux/commit/d5bb424e18e2ecab4ac590a66b0cca9dfb96d3da
git remote add linux-review https://github.com/0day-ci/linux
git remote update linux-review
git checkout d5bb424e18e2ecab4ac590a66b0cca9dfb96d3da
vim +987 drivers/infiniband/hw/vmw_pvrdma/pvrdma_main.c
29c8d9eb Adit Ranadive 2016-10-02 784
29c8d9eb Adit Ranadive 2016-10-02 785 static int pvrdma_pci_probe(struct
pci_dev *pdev,
29c8d9eb Adit Ranadive 2016-10-02 786 const
struct pci_device_id *id)
29c8d9eb Adit Ranadive 2016-10-02 787 {
29c8d9eb Adit Ranadive 2016-10-02 788 struct pci_dev *pdev_net;
29c8d9eb Adit Ranadive 2016-10-02 789 struct pvrdma_dev *dev;
29c8d9eb Adit Ranadive 2016-10-02 790 int ret;
29c8d9eb Adit Ranadive 2016-10-02 791 unsigned long start;
29c8d9eb Adit Ranadive 2016-10-02 792 unsigned long len;
29c8d9eb Adit Ranadive 2016-10-02 793 dma_addr_t slot_dma = 0;
29c8d9eb Adit Ranadive 2016-10-02 794
29c8d9eb Adit Ranadive 2016-10-02 795 dev_dbg(>dev,
"initializing driver %s\n", pci_name(pdev));
29c8d9eb Adit Ranadive 2016-10-02 796
29c8d9eb Adit Ranadive 2016-10-02 797 /* Allocate zero-out device */
29c8d9eb Adit Ranadive 2016-10-02 798 dev = (struct pvrdma_dev
*)ib_alloc_device(sizeof(*dev));
29c8d9eb Adit Ranadive 2016-10-02 799 if (!dev) {
29c8d9eb Adit Ranadive 2016-10-02 800 dev_err(>dev,
"failed to allocate IB device\n");
29c8d9eb Adit Ranadive 2016-10-02 801 return -ENOMEM;
29c8d9eb Adit Ranadive 2016-10-02 802 }
29c8d9eb Adit Ranadive 2016-10-02 803
29c8d9eb Adit Ranadive 2016-10-02 804
mutex_lock(_device_list_lock);
29c8d9eb Adit Ranadive 2016-10-02 805 list_add(>device_link,
_device_list);
29c8d9eb Adit Ranadive 2016-10-02 806
mutex_unlock(_device_list_lock);
29c8d9eb Adit Ranadive 2016-10-02 807
29c8d9eb Adit Ranadive 2016-10-02 808 ret = pvrdma_init_device(dev);
29c8d9eb Adit Ranadive 2016-10-02 809 if (ret)
29c8d9eb Adit Ranadive 2016-10-02 810 goto err_free_device;
29c8d9eb Adit Ranadive 2016-10-02 811
29c8d9eb Adit Ranadive 2016-10-02 812 dev->pdev = pdev;
29c8d9eb Adit Ranadive 2016-10-02 813 pci_set_drvdata(pdev, dev);
29c8d9eb Adit Ranadive 2016-10-02 814
29c8d9eb Adit Ranadive 2016-10-02 815 ret = pci_enable_device(pdev);
29c8d9eb Adit Ranadive 2016-10-02 816 if (ret) {
29c8d9eb Adit Ranadive 2016-10-02 817 dev_err(>dev,
"cannot enable PCI device\n");
29c8d9eb Adit Ranadive 2016-10-02 818 goto err_free_device;
29c8d9eb Adit Ranadive 2016-10-02 819 }
29c8d9eb Adit Ranadive 2016-10-02 820
29c8d9eb Adit Ranadive 2016-10-02 821 dev_dbg(>dev, "PCI
resource flags BAR0 %#lx\n",
29c8d9eb Adit Ranadive 2016-10-02 822
pci_resource_flags(pdev, 0));
29c8d9eb Adit Ranadive 2016-10-02 823 dev_dbg(>dev, "PCI
resource len %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 824 (unsigned long
long)pci_resource_len(pdev, 0));
29c8d9eb Adit Ranadive 2016-10-02 825 dev_dbg(>dev, "PCI
resource start %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 826 (unsigned long
long)pci_resource_start(pdev, 0));
29c8d9eb Adit Ranadive 2016-10-02 827 dev_dbg(>dev, "PCI
resource flags BAR1 %#lx\n",
29c8d9eb Adit Ranadive 2016-10-02 828
pci_resource_flags(pdev, 1));
29c8d9eb Adit Ranadive 2016-10-02 829 dev_dbg(>dev, "PCI
resource len %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 830 (unsigned long
long)pci_resource_len(pdev, 1));
29c8d9eb Adit Ranadive 2016-10-02 831 dev_dbg(>dev, "PCI
resource start %#llx\n",
29c8d9eb Adit Ranadive 2016-10-02 832 (unsigned long
long)pci_resource_start(pdev, 1));
29c8d9eb Adit Ranadive 2016-10-02 833
29c8d9eb Adit Ranadive 2016-10-02 834 if (!(pci_resource_flags(pdev,
0) & IORESOURCE_MEM) ||
29c8d9eb Adit Ranadive 2016-10-02 835 !(pci_resource_flags(pdev,
1) & IORESOURCE_MEM)) {
29c8d9eb Adit Ranadive 2016-10-02 836 dev_err(>dev,
"PCI BAR region not MMIO\n");
29c8d9eb Adit Ranadive 2016-10-02 837 ret = -ENOMEM;
29c8d9eb Adit Ranadive 2016-10-02 838 goto err_free_device;
29c8d9eb Adit Ranadive 2016-10-02 839 }
29c8d9eb Adit Ranadive 2016-10-02 840
29c8d9eb Adit
Re: [GIT PULL] x86 fixes
On Sat, Jun 30, 2018 at 1:49 AM Ingo Molnar wrote:
>
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -477,7 +477,7 @@ ENTRY(entry_SYSENTER_32)
> * whereas POPF does not.)
> */
> addl$PT_EFLAGS-PT_DS, %esp /* point esp at pt_regs->flags */
> - btr $X86_EFLAGS_IF_BIT, (%esp)
> + btrl$X86_EFLAGS_IF_BIT, (%esp)
> popfl
Ho humm. Just looking at this patch, my reaction was "why isn't this
an 'andl $~X86_EFLAGS_IF' instead"?
Yeah, I guess the 'andl' is two bytes longer (due to the 32-bit
constant - because IF is bit 9, you can't use a byte constant, and you
don't want to get a partial word write just before the popfl).
But btr is really pretty heavy operation for older CPU's (it's gotten
better, but 32-bit code presumably cares more about the older CPUs).
It really doesn't matter, I guess. The btr goes back to commit
c2c9b52fab0d ("x86/entry/32: Restore FLAGS on SYSEXIT").
Andy?
Linus
Re: [GIT PULL] x86 fixes
On Sat, Jun 30, 2018 at 1:49 AM Ingo Molnar wrote:
>
> --- a/arch/x86/entry/entry_32.S
> +++ b/arch/x86/entry/entry_32.S
> @@ -477,7 +477,7 @@ ENTRY(entry_SYSENTER_32)
> * whereas POPF does not.)
> */
> addl$PT_EFLAGS-PT_DS, %esp /* point esp at pt_regs->flags */
> - btr $X86_EFLAGS_IF_BIT, (%esp)
> + btrl$X86_EFLAGS_IF_BIT, (%esp)
> popfl
Ho humm. Just looking at this patch, my reaction was "why isn't this
an 'andl $~X86_EFLAGS_IF' instead"?
Yeah, I guess the 'andl' is two bytes longer (due to the 32-bit
constant - because IF is bit 9, you can't use a byte constant, and you
don't want to get a partial word write just before the popfl).
But btr is really pretty heavy operation for older CPU's (it's gotten
better, but 32-bit code presumably cares more about the older CPUs).
It really doesn't matter, I guess. The btr goes back to commit
c2c9b52fab0d ("x86/entry/32: Restore FLAGS on SYSEXIT").
Andy?
Linus
Re: [PATCH v2 0/2] PCI Quirk Patchset for Microsemi Switchtec NTB
On Fri, Jun 29, 2018 at 08:50:02PM -0500, Bjorn Helgaas wrote:
> On Wed, May 23, 2018 at 01:18:04PM -0700, dme...@gigaio.com wrote:
> > From: Doug Meyer
> >
> > This is a resend of the patch series to enable Microsemi Switchtec
> > NTB configurations to run with the IOMMU in the hosts turned on.
> > Because of the nature PCI Quirk implementation, it was preferable
> > to migrate the Microsemi PCI vendor and device definitions to the
> > Linux canonical location. Logan Gunthorpe requested that this
> > migration be done as a separate patch in a set, and so this patch
> > series was created as shown here.
> >
> > The first patch encapsulates the movement of constants from
> > switchtec.h to pci_ids.h, with commensurate changes to the source
> > files. This patch is not dependent on any other work.
> >
> > The second patch is the PCI quirk implementation itself, and is
> > completely dependent upon the first patch in this series.
> >
> > Testing of the quirk was done on with a 2-host x86-64 system
> > with all combinations of IOMMU off/on. The ntb_perf module was
> > used as test stimulus.
> >
> > Blessings,
> > Doug Meyer
> >
> > Changes since v1:
> > - Call pci_device_disable() at return points to clean up properly.
> > - Changed all dev_* print macros to pci_* macros.
> > - Removed superfluous variable initializations.
> >
> > Doug Meyer (2):
> > NTB: Migrate PCI Constants to Cannonical PCI Header
> > NTB: PCI Quirk to Enable Switchtec NT Functionality with IOMMU On
> >
> > drivers/ntb/hw/mscc/ntb_hw_switchtec.c | 3 +-
> > drivers/pci/quirks.c | 197
> > +
> > drivers/pci/switch/switchtec.c | 15 ++-
> > include/linux/pci_ids.h| 32 ++
> > include/linux/switchtec.h | 4 -
> > 5 files changed, 238 insertions(+), 13 deletions(-)
>
> I applied these with Logan's reviewed-by to my pci/switchtec branch for
> v4.19 with the following updates:
>
> - You removed the SPDX tag from drivers/pci/switch/switchtec.c. I assume
> that was a mistake, so I restored it.
>
> - I moved the PCI_VENDOR_ID_MICROSEMI definition to keep the file sorted.
>
> - I dropped the Device ID definitions per the policy at the top of the
> file (which I mentioned on your v1 posting).
>
> - I converted all the Device ID definition uses to raw hex constants. I
> noticed that the following were defined by your patch, but not used:
>
> PCI_DEVICE_ID_MICROSEMI_PSX24XG3
> PCI_DEVICE_ID_MICROSEMI_PSX32XG3
>
> I can't tell whether the quirk is supposed to apply to them or not.
>
> Please review and holler if I broke something.
>
> This touches drivers/ntb/hw/..., which isn't my area. Let me know if you'd
> rather take these through a different tree.
I also:
- Fixed the sparse warnings reported by the kbuild test robot.
- Removed some (void * __iomem) casts in the ioread32() parameters and
the (u64) cast on the return value. I noticed that
switchtec_ntb_init_sndev() does an ioread64() of ep_map instead of two
ioread32() calls. I suspect both places could and should do it the
same way.
- Used %02x.%d instead of %02d.%d when printing the DMA alias, so it
matches the other places that print PCI device addresses.
- Replaced DECLARE_PCI_FIXUP_CLASS_FINAL with DECLARE_PCI_FIXUP_FINAL,
since it should be sufficient to filter on Vendor and Device ID,
without worrying about the class.
Here's the diff from what you originally posted to what's currently on my
pci/switchtec branch:
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f178cecdc001..d54a182a09cf 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4787,14 +4787,13 @@ static void quirk_switchtec_ntb_dma_alias(struct
pci_dev *pdev)
pci_info(pdev, "Setting Switchtec proxy ID aliases\n");
mmio_ntb = mmio + SWITCHTEC_GAS_NTB_OFFSET;
- mmio_ctrl = (void * __iomem) mmio_ntb + SWITCHTEC_NTB_REG_CTRL_OFFSET;
+ mmio_ctrl = (void __iomem *) mmio_ntb + SWITCHTEC_NTB_REG_CTRL_OFFSET;
mmio_sys_info = mmio + SWITCHTEC_GAS_SYS_INFO_OFFSET;
partition = ioread8(_ntb->partition_id);
- partition_map = (u64) ioread32((void * __iomem) _ntb->ep_map);
- partition_map |=
- ((u64) ioread32((void * __iomem) _ntb->ep_map + 4)) << 32;
+ partition_map = ioread32(_ntb->ep_map);
+ partition_map |= ((u64) ioread32(_ntb->ep_map + 4)) << 32;
partition_map &= ~(1ULL << partition);
for (pp = 0; pp < (sizeof(partition_map) * 8); pp++) {
@@ -4829,7 +4828,7 @@ static void quirk_switchtec_ntb_dma_alias(struct pci_dev
*pdev)
rid_entry = ioread32(_peer_ctrl->req_id_table[te]);
devfn = (rid_entry >> 1) & 0xFF;
pci_dbg(pdev,
- "Aliasing Partition %d Proxy ID %02d.%d\n",
+ "Aliasing
Re: [PATCH v2 0/2] PCI Quirk Patchset for Microsemi Switchtec NTB
On Fri, Jun 29, 2018 at 08:50:02PM -0500, Bjorn Helgaas wrote:
> On Wed, May 23, 2018 at 01:18:04PM -0700, dme...@gigaio.com wrote:
> > From: Doug Meyer
> >
> > This is a resend of the patch series to enable Microsemi Switchtec
> > NTB configurations to run with the IOMMU in the hosts turned on.
> > Because of the nature PCI Quirk implementation, it was preferable
> > to migrate the Microsemi PCI vendor and device definitions to the
> > Linux canonical location. Logan Gunthorpe requested that this
> > migration be done as a separate patch in a set, and so this patch
> > series was created as shown here.
> >
> > The first patch encapsulates the movement of constants from
> > switchtec.h to pci_ids.h, with commensurate changes to the source
> > files. This patch is not dependent on any other work.
> >
> > The second patch is the PCI quirk implementation itself, and is
> > completely dependent upon the first patch in this series.
> >
> > Testing of the quirk was done on with a 2-host x86-64 system
> > with all combinations of IOMMU off/on. The ntb_perf module was
> > used as test stimulus.
> >
> > Blessings,
> > Doug Meyer
> >
> > Changes since v1:
> > - Call pci_device_disable() at return points to clean up properly.
> > - Changed all dev_* print macros to pci_* macros.
> > - Removed superfluous variable initializations.
> >
> > Doug Meyer (2):
> > NTB: Migrate PCI Constants to Cannonical PCI Header
> > NTB: PCI Quirk to Enable Switchtec NT Functionality with IOMMU On
> >
> > drivers/ntb/hw/mscc/ntb_hw_switchtec.c | 3 +-
> > drivers/pci/quirks.c | 197
> > +
> > drivers/pci/switch/switchtec.c | 15 ++-
> > include/linux/pci_ids.h| 32 ++
> > include/linux/switchtec.h | 4 -
> > 5 files changed, 238 insertions(+), 13 deletions(-)
>
> I applied these with Logan's reviewed-by to my pci/switchtec branch for
> v4.19 with the following updates:
>
> - You removed the SPDX tag from drivers/pci/switch/switchtec.c. I assume
> that was a mistake, so I restored it.
>
> - I moved the PCI_VENDOR_ID_MICROSEMI definition to keep the file sorted.
>
> - I dropped the Device ID definitions per the policy at the top of the
> file (which I mentioned on your v1 posting).
>
> - I converted all the Device ID definition uses to raw hex constants. I
> noticed that the following were defined by your patch, but not used:
>
> PCI_DEVICE_ID_MICROSEMI_PSX24XG3
> PCI_DEVICE_ID_MICROSEMI_PSX32XG3
>
> I can't tell whether the quirk is supposed to apply to them or not.
>
> Please review and holler if I broke something.
>
> This touches drivers/ntb/hw/..., which isn't my area. Let me know if you'd
> rather take these through a different tree.
I also:
- Fixed the sparse warnings reported by the kbuild test robot.
- Removed some (void * __iomem) casts in the ioread32() parameters and
the (u64) cast on the return value. I noticed that
switchtec_ntb_init_sndev() does an ioread64() of ep_map instead of two
ioread32() calls. I suspect both places could and should do it the
same way.
- Used %02x.%d instead of %02d.%d when printing the DMA alias, so it
matches the other places that print PCI device addresses.
- Replaced DECLARE_PCI_FIXUP_CLASS_FINAL with DECLARE_PCI_FIXUP_FINAL,
since it should be sufficient to filter on Vendor and Device ID,
without worrying about the class.
Here's the diff from what you originally posted to what's currently on my
pci/switchtec branch:
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index f178cecdc001..d54a182a09cf 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -4787,14 +4787,13 @@ static void quirk_switchtec_ntb_dma_alias(struct
pci_dev *pdev)
pci_info(pdev, "Setting Switchtec proxy ID aliases\n");
mmio_ntb = mmio + SWITCHTEC_GAS_NTB_OFFSET;
- mmio_ctrl = (void * __iomem) mmio_ntb + SWITCHTEC_NTB_REG_CTRL_OFFSET;
+ mmio_ctrl = (void __iomem *) mmio_ntb + SWITCHTEC_NTB_REG_CTRL_OFFSET;
mmio_sys_info = mmio + SWITCHTEC_GAS_SYS_INFO_OFFSET;
partition = ioread8(_ntb->partition_id);
- partition_map = (u64) ioread32((void * __iomem) _ntb->ep_map);
- partition_map |=
- ((u64) ioread32((void * __iomem) _ntb->ep_map + 4)) << 32;
+ partition_map = ioread32(_ntb->ep_map);
+ partition_map |= ((u64) ioread32(_ntb->ep_map + 4)) << 32;
partition_map &= ~(1ULL << partition);
for (pp = 0; pp < (sizeof(partition_map) * 8); pp++) {
@@ -4829,7 +4828,7 @@ static void quirk_switchtec_ntb_dma_alias(struct pci_dev
*pdev)
rid_entry = ioread32(_peer_ctrl->req_id_table[te]);
devfn = (rid_entry >> 1) & 0xFF;
pci_dbg(pdev,
- "Aliasing Partition %d Proxy ID %02d.%d\n",
+ "Aliasing
Re: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops
On Sat, Jun 23, 2018 at 11:05 PM Teika Kazura wrote:
>
> Hi, Rosen. Thank you for your patch. (@Jan Steffens and @Pablo Cholaky:
> Thanks for your reports.) But I'm afraid some more work is necessary. Let me
> propose some points.
Too much work for me. I'll just use the kernel pareameter.
>
> I'm not a kernel developer, but have been working on this issue to help them.
> (See https://www.spinics.net/lists/linux-input/msg55950.html )
>
> 0.
> Could you resend it, cc-ing to the relevant developers. (Use the one of my
> reply here. You can use /scripts/get_maintainer.pl next
> time.) Otherwise, it is unlikely to be noticed by them.
>
> 1.
> You added your device to forcepad_pnp_ids[], but I guess it should be
> smbus_pnp_ids[].
>
> When making the patch again, please use the kernel 4.17 or newer, because
> smbus_pnp_ids was changed recently.
>
> If it works by using smbus_pnp_ids, please test it for a week or so,
> especially making sure that suspend/resume works without any problem.
>
> 2.
> Though it's my personal impression, the change description looked somewhat
> personal. At the same time, it's important, guaranteeing the stability of the
> patch.- I know you originally reported it last August [1]. Furthermore, there
> have been two other users [2] who reported SMBus worked for the same device,
> namely SYN1219. (They two are cc-ed to in this reply.)
>
> You can use "$ git format-patch --cover-letter"; it will generate a
> "cover-letter", the introductory part of the patch, explaining the
> acceptability of the patch to kernel developers. (For an example, see
> https://www.spinics.net/lists/linux-input/msg57041.html)
>
> So the full description can be given in the cover letter, and the real patch
> description can be something like "This enables SMBus for xxx", being almost
> identical to the email subject.
>
> 3. This is not essential, but the style of the email subject that the device
> maintainer prefers might be:
>
> [PATCH v2] Input: synaptics - Enable RMI4 for Clevo P870DM laptops
>
> after looking at this tree [3]. ("v2" meaning version 2, due to the above
> change.) You can feed "--subject-prefix='PATCH v2'" to "$ git format-patch".
>
> 4. If you want to give the credit for my reply, you can add
>
> Suggested-by: Teika Kazura
>
> after Signed-off-by. It's completely ok without it. :-)
>
> [1] https://marc.info/?l=linux-input=150284057602358=2
> [2] https://marc.info/?l=linux-input=150049625613055=2
> https://marc.info/?l=linux-input=15009456026=2
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/
>
> Best regards,
> Teika (Teika kazura)
>
>
> From: Rosen Penev
> Subject: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops
> Date: Sat, 16 Jun 2018 18:42:16 -0700
>
> > I have been testing this for half a year with no issues to report.
> >
> > Signed-off-by: Rosen Penev
> > ---
> > drivers/input/mouse/synaptics.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/input/mouse/synaptics.c
> > b/drivers/input/mouse/synaptics.c
> > index 55d33500d55e..23f5bb2cf9da 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -183,6 +183,7 @@ static const char * const smbus_pnp_ids[] = {
> > };
> >
> > static const char * const forcepad_pnp_ids[] = {
> > + "SYN1219",
> > "SYN300D",
> > "SYN3014",
> > NULL
> > --
> > 2.17.1
> >
Re: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops
On Sat, Jun 23, 2018 at 11:05 PM Teika Kazura wrote:
>
> Hi, Rosen. Thank you for your patch. (@Jan Steffens and @Pablo Cholaky:
> Thanks for your reports.) But I'm afraid some more work is necessary. Let me
> propose some points.
Too much work for me. I'll just use the kernel pareameter.
>
> I'm not a kernel developer, but have been working on this issue to help them.
> (See https://www.spinics.net/lists/linux-input/msg55950.html )
>
> 0.
> Could you resend it, cc-ing to the relevant developers. (Use the one of my
> reply here. You can use /scripts/get_maintainer.pl next
> time.) Otherwise, it is unlikely to be noticed by them.
>
> 1.
> You added your device to forcepad_pnp_ids[], but I guess it should be
> smbus_pnp_ids[].
>
> When making the patch again, please use the kernel 4.17 or newer, because
> smbus_pnp_ids was changed recently.
>
> If it works by using smbus_pnp_ids, please test it for a week or so,
> especially making sure that suspend/resume works without any problem.
>
> 2.
> Though it's my personal impression, the change description looked somewhat
> personal. At the same time, it's important, guaranteeing the stability of the
> patch.- I know you originally reported it last August [1]. Furthermore, there
> have been two other users [2] who reported SMBus worked for the same device,
> namely SYN1219. (They two are cc-ed to in this reply.)
>
> You can use "$ git format-patch --cover-letter"; it will generate a
> "cover-letter", the introductory part of the patch, explaining the
> acceptability of the patch to kernel developers. (For an example, see
> https://www.spinics.net/lists/linux-input/msg57041.html)
>
> So the full description can be given in the cover letter, and the real patch
> description can be something like "This enables SMBus for xxx", being almost
> identical to the email subject.
>
> 3. This is not essential, but the style of the email subject that the device
> maintainer prefers might be:
>
> [PATCH v2] Input: synaptics - Enable RMI4 for Clevo P870DM laptops
>
> after looking at this tree [3]. ("v2" meaning version 2, due to the above
> change.) You can feed "--subject-prefix='PATCH v2'" to "$ git format-patch".
>
> 4. If you want to give the credit for my reply, you can add
>
> Suggested-by: Teika Kazura
>
> after Signed-off-by. It's completely ok without it. :-)
>
> [1] https://marc.info/?l=linux-input=150284057602358=2
> [2] https://marc.info/?l=linux-input=150049625613055=2
> https://marc.info/?l=linux-input=15009456026=2
> [3] https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git/log/
>
> Best regards,
> Teika (Teika kazura)
>
>
> From: Rosen Penev
> Subject: [PATCH] synaptics: Enable RMI4 for Clevo P870DM laptops
> Date: Sat, 16 Jun 2018 18:42:16 -0700
>
> > I have been testing this for half a year with no issues to report.
> >
> > Signed-off-by: Rosen Penev
> > ---
> > drivers/input/mouse/synaptics.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/input/mouse/synaptics.c
> > b/drivers/input/mouse/synaptics.c
> > index 55d33500d55e..23f5bb2cf9da 100644
> > --- a/drivers/input/mouse/synaptics.c
> > +++ b/drivers/input/mouse/synaptics.c
> > @@ -183,6 +183,7 @@ static const char * const smbus_pnp_ids[] = {
> > };
> >
> > static const char * const forcepad_pnp_ids[] = {
> > + "SYN1219",
> > "SYN300D",
> > "SYN3014",
> > NULL
> > --
> > 2.17.1
> >
