Re: [PATCH v2 2/2] riscv/atomic: Strengthen implementations with fences

[Andrea Parri]

On Fri, Mar 09, 2018 at 04:21:37PM -0800, Daniel Lustig wrote:
> On 3/9/2018 2:57 PM, Palmer Dabbelt wrote:
> > On Fri, 09 Mar 2018 13:30:08 PST (-0800), parri.and...@gmail.com wrote:
> >> On Fri, Mar 09, 2018 at 10:54:27AM -0800, Palmer Dabbelt wrote:
> >>> On Fri, 09 Mar 2018 10:36:44 PST (-0800), parri.and...@gmail.com wrote:
> >>
> >> [...]
> >>
> >>> >This proposal relies on the generic definition,
> >>> >
> >>> >   include/linux/atomic.h ,
> >>> >
> >>> >and on the
> >>> >
> >>> >   __atomic_op_acquire()
> >>> >   __atomic_op_release()
> >>> >
> >>> >above to build the acquire/release atomics (except for the xchg,cmpxchg,
> >>> >where the ACQUIRE_BARRIER is inserted conditionally/on success).
> >>>
> >>> I thought we wanted to use the AQ and RL bits for AMOs, just not for LR/SC
> >>> sequences.  IIRC the AMOs are safe with the current memory model, but I 
> >>> might
> >>> just have some version mismatches in my head.
> >>
> >> AMO.aqrl are "safe" w.r.t. the LKMM (as they provide "full-ordering"); 
> >> OTOH,
> >> AMO.aq and AMO.rl present weaknesses that LKMM (and some kernel developers)
> >> do not "expect".  I was probing this issue in:
> >>
> >>   https://marc.info/?l=linux-kernel=151930201102853=2
> >>
> >> (c.f., e.g., test "RISCV-unlock-lock-read-ordering" from that post).
> >>
> >> Quoting from the commit message of my patch 1/2:
> >>
> >>   "Referring to the "unlock-lock-read-ordering" test reported below,
> >>    Daniel wrote:
> >>
> >>  I think an RCpc interpretation of .aq and .rl would in fact
> >>  allow the two normal loads in P1 to be reordered [...]
> >>
> >>  [...]
> >>
> >>  Likewise even if the unlock()/lock() is between two stores.
> >>  A control dependency might originate from the load part of
> >>  the amoswap.w.aq, but there still would have to be something
> >>  to ensure that this load part in fact performs after the store
> >>  part of the amoswap.w.rl performs globally, and that's not
> >>  automatic under RCpc.
> >>
> >>    Simulation of the RISC-V memory consistency model confirmed this
> >>    expectation."
> >>
> >> I have just (re)checked these observations against the latest 
> >> specification,
> >> and my results _confirmed_ these verdicts.
> > 
> > Thanks, I must have just gotten confused about a draft spec or something.  
> > I'm
> > pulling these on top or your other memory model related patch.  I've renamed
> > the branch "next-mm" to be a bit more descriptiove.
> 
> (Sorry for being out of the loop this week, I was out to deal with
> a family matter.)
> 
> I assume you're using the herd model?  Luc's doing a great job with
> that, but even so, nothing is officially confirmed until we ratify
> the model.  In other words, the herd model may end up changing too.
> If something is broken on our end, there's still time to fix it.

No need to say :) if you look back at the LKMM as from 2 years ago or as
presented last year in LWN, you won't recognize it as such ;-)  Spec. do
change/evolve, and so do implementations: if ratifications of the RISC-V
memory model (or of the LKMM) will enable optimizations/modifications to
these implementations (while preserving correctness), I'll be glad to do
or to help with them.

To answer your question: I used both the herd-based model from INRIA and
the operational model from the group in Cambridge (these are referred to
in the currently available RISC-V spec.).


> 
> Regarding AMOs, let me copy from something I wrote in a previous
> offline conversation:
> 
> > it seems to us that pairing a store-release of "amoswap.rl" with
> > a "ld; fence r,rw" doesn't actually give us the RC semantics we've
> > been discussing for LKMM.  For example:
> > 
> > (a) sd t0,0(s0)
> > (b) amoswap.d.rl x0,t1,0(s1)
> > ...
> > (c) ld a0,0(s1)
> > (d) fence r,rw
> > (e) sd t2,0(s2)
> > 
> > There, we won't get (a) ordered before (e) regardless of whether
> > (b) is RCpc or RCsc.  Do you agree?
> 
> At the moment, only the load part of (b) is in the predecessor
> set of (d), but the store part of (b) is not.  Likewise, the
> .rl annotation applies only to the store part of (b), not the
> load part.

Indeed.  (If you want, this is one reason why, with these patches, ".rl"
and "fence r,rw" are never "mixed" as in your snipped above unless there
is also a "fence rw,rw" in between.)


> 
> This gets back to a question Linus asked last week about
> whether the AMO is a single unit or whether it can be
> considered to split into a load and a store part (which still
> perform atomically).  For RISC-V, for right now at least, the
> answer is the latter.  Is it still the latter for Linux too?

Yes: (successful) atomic RMWs all generate (at least) one load event and
one store event in LKMM.  (This same approach is taken by other hardware
models as well...)


> 
> https://lkml.org/lkml/2018/2/26/606
> 
> > So I think we'll need to make sure we pair .rl with .aq, or that
> > we pair fence-based mappings with fence-based 

Re: [PATCH v2 2/2] riscv/atomic: Strengthen implementations with fences

[Andrea Parri]

On Fri, Mar 09, 2018 at 04:21:37PM -0800, Daniel Lustig wrote:
> On 3/9/2018 2:57 PM, Palmer Dabbelt wrote:
> > On Fri, 09 Mar 2018 13:30:08 PST (-0800), parri.and...@gmail.com wrote:
> >> On Fri, Mar 09, 2018 at 10:54:27AM -0800, Palmer Dabbelt wrote:
> >>> On Fri, 09 Mar 2018 10:36:44 PST (-0800), parri.and...@gmail.com wrote:
> >>
> >> [...]
> >>
> >>> >This proposal relies on the generic definition,
> >>> >
> >>> >   include/linux/atomic.h ,
> >>> >
> >>> >and on the
> >>> >
> >>> >   __atomic_op_acquire()
> >>> >   __atomic_op_release()
> >>> >
> >>> >above to build the acquire/release atomics (except for the xchg,cmpxchg,
> >>> >where the ACQUIRE_BARRIER is inserted conditionally/on success).
> >>>
> >>> I thought we wanted to use the AQ and RL bits for AMOs, just not for LR/SC
> >>> sequences.  IIRC the AMOs are safe with the current memory model, but I 
> >>> might
> >>> just have some version mismatches in my head.
> >>
> >> AMO.aqrl are "safe" w.r.t. the LKMM (as they provide "full-ordering"); 
> >> OTOH,
> >> AMO.aq and AMO.rl present weaknesses that LKMM (and some kernel developers)
> >> do not "expect".  I was probing this issue in:
> >>
> >>   https://marc.info/?l=linux-kernel=151930201102853=2
> >>
> >> (c.f., e.g., test "RISCV-unlock-lock-read-ordering" from that post).
> >>
> >> Quoting from the commit message of my patch 1/2:
> >>
> >>   "Referring to the "unlock-lock-read-ordering" test reported below,
> >>    Daniel wrote:
> >>
> >>  I think an RCpc interpretation of .aq and .rl would in fact
> >>  allow the two normal loads in P1 to be reordered [...]
> >>
> >>  [...]
> >>
> >>  Likewise even if the unlock()/lock() is between two stores.
> >>  A control dependency might originate from the load part of
> >>  the amoswap.w.aq, but there still would have to be something
> >>  to ensure that this load part in fact performs after the store
> >>  part of the amoswap.w.rl performs globally, and that's not
> >>  automatic under RCpc.
> >>
> >>    Simulation of the RISC-V memory consistency model confirmed this
> >>    expectation."
> >>
> >> I have just (re)checked these observations against the latest 
> >> specification,
> >> and my results _confirmed_ these verdicts.
> > 
> > Thanks, I must have just gotten confused about a draft spec or something.  
> > I'm
> > pulling these on top or your other memory model related patch.  I've renamed
> > the branch "next-mm" to be a bit more descriptiove.
> 
> (Sorry for being out of the loop this week, I was out to deal with
> a family matter.)
> 
> I assume you're using the herd model?  Luc's doing a great job with
> that, but even so, nothing is officially confirmed until we ratify
> the model.  In other words, the herd model may end up changing too.
> If something is broken on our end, there's still time to fix it.

No need to say :) if you look back at the LKMM as from 2 years ago or as
presented last year in LWN, you won't recognize it as such ;-)  Spec. do
change/evolve, and so do implementations: if ratifications of the RISC-V
memory model (or of the LKMM) will enable optimizations/modifications to
these implementations (while preserving correctness), I'll be glad to do
or to help with them.

To answer your question: I used both the herd-based model from INRIA and
the operational model from the group in Cambridge (these are referred to
in the currently available RISC-V spec.).


> 
> Regarding AMOs, let me copy from something I wrote in a previous
> offline conversation:
> 
> > it seems to us that pairing a store-release of "amoswap.rl" with
> > a "ld; fence r,rw" doesn't actually give us the RC semantics we've
> > been discussing for LKMM.  For example:
> > 
> > (a) sd t0,0(s0)
> > (b) amoswap.d.rl x0,t1,0(s1)
> > ...
> > (c) ld a0,0(s1)
> > (d) fence r,rw
> > (e) sd t2,0(s2)
> > 
> > There, we won't get (a) ordered before (e) regardless of whether
> > (b) is RCpc or RCsc.  Do you agree?
> 
> At the moment, only the load part of (b) is in the predecessor
> set of (d), but the store part of (b) is not.  Likewise, the
> .rl annotation applies only to the store part of (b), not the
> load part.

Indeed.  (If you want, this is one reason why, with these patches, ".rl"
and "fence r,rw" are never "mixed" as in your snipped above unless there
is also a "fence rw,rw" in between.)


> 
> This gets back to a question Linus asked last week about
> whether the AMO is a single unit or whether it can be
> considered to split into a load and a store part (which still
> perform atomically).  For RISC-V, for right now at least, the
> answer is the latter.  Is it still the latter for Linux too?

Yes: (successful) atomic RMWs all generate (at least) one load event and
one store event in LKMM.  (This same approach is taken by other hardware
models as well...)


> 
> https://lkml.org/lkml/2018/2/26/606
> 
> > So I think we'll need to make sure we pair .rl with .aq, or that
> > we pair fence-based mappings with fence-based 

[PATCH v3 10/20] firmware: enable run time change of forcing fallback loader

[Luis R. Rodriguez]

Currently one requires to test four kernel configurations to test the
firmware API completely:

0)
  CONFIG_FW_LOADER=y

1)
  o CONFIG_FW_LOADER=y
  o CONFIG_FW_LOADER_USER_HELPER=y

2)
  o CONFIG_FW_LOADER=y
  o CONFIG_FW_LOADER_USER_HELPER=y
  o CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y

3) When CONFIG_FW_LOADER=m the built-in stuff is disabled, we have
   no current tests for this.

We can reduce the requirements to three kernel configurations by making
fw_config.force_sysfs_fallback a proc knob we flip on off. For kernels that
disable CONFIG_IKCONFIG_PROC this can also enable one to inspect if
CONFIG_FW_LOADER_USER_HELPER_FALLBACK was enabled at build time by checking
the proc value at boot time.

Acked-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/fallback.c   |  1 +
 drivers/base/firmware_loader/fallback.h   |  4 +++-
 drivers/base/firmware_loader/fallback_table.c | 17 +
 kernel/sysctl.c   | 11 +++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 9b65837256d6..45cc40933a47 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "fallback.h"
 #include "firmware.h"
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index 550498c7fa4c..ca7e69a8417b 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -12,12 +12,14 @@
  *
  * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
  * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
+ * functionality on a kernel where that config entry has been disabled.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  * giving up, in seconds.
  */
 struct firmware_fallback_config {
-   const bool force_sysfs_fallback;
+   unsigned int force_sysfs_fallback;
int old_timeout;
int loading_timeout;
 };
diff --git a/drivers/base/firmware_loader/fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
index 981419044c7e..92365e053e30 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -19,6 +19,9 @@
 /* Module or buit-in */
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
+static unsigned int zero;
+static unsigned int one = 1;
+
 struct firmware_fallback_config fw_fallback_config = {
.force_sysfs_fallback = 
IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
.loading_timeout = 60,
@@ -26,4 +29,18 @@ struct firmware_fallback_config fw_fallback_config = {
 };
 EXPORT_SYMBOL_GPL(fw_fallback_config);
 
+struct ctl_table firmware_config_table[] = {
+   {
+   .procname   = "force_sysfs_fallback",
+   .data   = _fallback_config.force_sysfs_fallback,
+   .maxlen = sizeof(unsigned int),
+   .mode   = 0644,
+   .proc_handler   = proc_douintvec_minmax,
+   .extra1 = ,
+   .extra2 = ,
+   },
+   { }
+};
+EXPORT_SYMBOL_GPL(firmware_config_table);
+
 #endif
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6c68e771e11d..7a8aa6866764 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -253,6 +253,10 @@ extern struct ctl_table random_table[];
 extern struct ctl_table epoll_table[];
 #endif
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+extern struct ctl_table firmware_config_table[];
+#endif
+
 #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 int sysctl_legacy_va_layout;
 #endif
@@ -748,6 +752,13 @@ static struct ctl_table kern_table[] = {
.mode   = 0555,
.child  = usermodehelper_table,
},
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+   {
+   .procname   = "firmware_config",
+   .mode   = 0555,
+   .child  = firmware_config_table,
+   },
+#endif
{
.procname   = "overflowuid",
.data   = ,
-- 
2.16.2

[PATCH v3 10/20] firmware: enable run time change of forcing fallback loader

[Luis R. Rodriguez]

Currently one requires to test four kernel configurations to test the
firmware API completely:

0)
  CONFIG_FW_LOADER=y

1)
  o CONFIG_FW_LOADER=y
  o CONFIG_FW_LOADER_USER_HELPER=y

2)
  o CONFIG_FW_LOADER=y
  o CONFIG_FW_LOADER_USER_HELPER=y
  o CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y

3) When CONFIG_FW_LOADER=m the built-in stuff is disabled, we have
   no current tests for this.

We can reduce the requirements to three kernel configurations by making
fw_config.force_sysfs_fallback a proc knob we flip on off. For kernels that
disable CONFIG_IKCONFIG_PROC this can also enable one to inspect if
CONFIG_FW_LOADER_USER_HELPER_FALLBACK was enabled at build time by checking
the proc value at boot time.

Acked-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/fallback.c   |  1 +
 drivers/base/firmware_loader/fallback.h   |  4 +++-
 drivers/base/firmware_loader/fallback_table.c | 17 +
 kernel/sysctl.c   | 11 +++
 4 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 9b65837256d6..45cc40933a47 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -7,6 +7,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include "fallback.h"
 #include "firmware.h"
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index 550498c7fa4c..ca7e69a8417b 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -12,12 +12,14 @@
  *
  * @force_sysfs_fallback: force the sysfs fallback mechanism to be used
  * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
+ * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
+ * functionality on a kernel where that config entry has been disabled.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  * giving up, in seconds.
  */
 struct firmware_fallback_config {
-   const bool force_sysfs_fallback;
+   unsigned int force_sysfs_fallback;
int old_timeout;
int loading_timeout;
 };
diff --git a/drivers/base/firmware_loader/fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
index 981419044c7e..92365e053e30 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -19,6 +19,9 @@
 /* Module or buit-in */
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 
+static unsigned int zero;
+static unsigned int one = 1;
+
 struct firmware_fallback_config fw_fallback_config = {
.force_sysfs_fallback = 
IS_ENABLED(CONFIG_FW_LOADER_USER_HELPER_FALLBACK),
.loading_timeout = 60,
@@ -26,4 +29,18 @@ struct firmware_fallback_config fw_fallback_config = {
 };
 EXPORT_SYMBOL_GPL(fw_fallback_config);
 
+struct ctl_table firmware_config_table[] = {
+   {
+   .procname   = "force_sysfs_fallback",
+   .data   = _fallback_config.force_sysfs_fallback,
+   .maxlen = sizeof(unsigned int),
+   .mode   = 0644,
+   .proc_handler   = proc_douintvec_minmax,
+   .extra1 = ,
+   .extra2 = ,
+   },
+   { }
+};
+EXPORT_SYMBOL_GPL(firmware_config_table);
+
 #endif
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index 6c68e771e11d..7a8aa6866764 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -253,6 +253,10 @@ extern struct ctl_table random_table[];
 extern struct ctl_table epoll_table[];
 #endif
 
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+extern struct ctl_table firmware_config_table[];
+#endif
+
 #ifdef HAVE_ARCH_PICK_MMAP_LAYOUT
 int sysctl_legacy_va_layout;
 #endif
@@ -748,6 +752,13 @@ static struct ctl_table kern_table[] = {
.mode   = 0555,
.child  = usermodehelper_table,
},
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+   {
+   .procname   = "firmware_config",
+   .mode   = 0555,
+   .child  = firmware_config_table,
+   },
+#endif
{
.procname   = "overflowuid",
.data   = ,
-- 
2.16.2

[PATCH v3 11/20] firmware: enable to force disable the fallback mechanism at run time

[Luis R. Rodriguez]

You currently need four different kernel builds to test the firmware
API fully. By adding a proc knob to force disable the fallback mechanism
completely we are able to reduce the amount of kernels you need built
to test the firmware API down to two.

Acked-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/fallback.c   | 5 +
 drivers/base/firmware_loader/fallback.h   | 4 
 drivers/base/firmware_loader/fallback_table.c | 9 +
 3 files changed, 18 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 45cc40933a47..d6838e7ec00c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -643,6 +643,11 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
+   if (fw_fallback_config.ignore_sysfs_fallback) {
+   pr_info_once("Ignoring firmware sysfs fallback due to debugfs 
knob\n");
+   return false;
+   }
+
if ((opt_flags & FW_OPT_NOFALLBACK))
return false;
 
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index ca7e69a8417b..dfebc644ed35 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -14,12 +14,16 @@
  * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
  * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
  * functionality on a kernel where that config entry has been disabled.
+ * @ignore_sysfs_fallback: force to disable the sysfs fallback mechanism.
+ * This emulates the behaviour as if we had set the kernel
+ * config CONFIG_FW_LOADER_USER_HELPER=n.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  * giving up, in seconds.
  */
 struct firmware_fallback_config {
unsigned int force_sysfs_fallback;
+   unsigned int ignore_sysfs_fallback;
int old_timeout;
int loading_timeout;
 };
diff --git a/drivers/base/firmware_loader/fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
index 92365e053e30..7428659d8df9 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -39,6 +39,15 @@ struct ctl_table firmware_config_table[] = {
.extra1 = ,
.extra2 = ,
},
+   {
+   .procname   = "ignore_sysfs_fallback",
+   .data   = _fallback_config.ignore_sysfs_fallback,
+   .maxlen = sizeof(unsigned int),
+   .mode   = 0644,
+   .proc_handler   = proc_douintvec_minmax,
+   .extra1 = ,
+   .extra2 = ,
+   },
{ }
 };
 EXPORT_SYMBOL_GPL(firmware_config_table);
-- 
2.16.2

[PATCH v3 11/20] firmware: enable to force disable the fallback mechanism at run time

[Luis R. Rodriguez]

You currently need four different kernel builds to test the firmware
API fully. By adding a proc knob to force disable the fallback mechanism
completely we are able to reduce the amount of kernels you need built
to test the firmware API down to two.

Acked-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/fallback.c   | 5 +
 drivers/base/firmware_loader/fallback.h   | 4 
 drivers/base/firmware_loader/fallback_table.c | 9 +
 3 files changed, 18 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 45cc40933a47..d6838e7ec00c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -643,6 +643,11 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
 
 static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 {
+   if (fw_fallback_config.ignore_sysfs_fallback) {
+   pr_info_once("Ignoring firmware sysfs fallback due to debugfs 
knob\n");
+   return false;
+   }
+
if ((opt_flags & FW_OPT_NOFALLBACK))
return false;
 
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index ca7e69a8417b..dfebc644ed35 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -14,12 +14,16 @@
  * as if one had enabled CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y.
  * Useful to help debug a CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y
  * functionality on a kernel where that config entry has been disabled.
+ * @ignore_sysfs_fallback: force to disable the sysfs fallback mechanism.
+ * This emulates the behaviour as if we had set the kernel
+ * config CONFIG_FW_LOADER_USER_HELPER=n.
  * @old_timeout: for internal use
  * @loading_timeout: the timeout to wait for the fallback mechanism before
  * giving up, in seconds.
  */
 struct firmware_fallback_config {
unsigned int force_sysfs_fallback;
+   unsigned int ignore_sysfs_fallback;
int old_timeout;
int loading_timeout;
 };
diff --git a/drivers/base/firmware_loader/fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
index 92365e053e30..7428659d8df9 100644
--- a/drivers/base/firmware_loader/fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -39,6 +39,15 @@ struct ctl_table firmware_config_table[] = {
.extra1 = ,
.extra2 = ,
},
+   {
+   .procname   = "ignore_sysfs_fallback",
+   .data   = _fallback_config.ignore_sysfs_fallback,
+   .maxlen = sizeof(unsigned int),
+   .mode   = 0644,
+   .proc_handler   = proc_douintvec_minmax,
+   .extra1 = ,
+   .extra2 = ,
+   },
{ }
 };
 EXPORT_SYMBOL_GPL(firmware_config_table);
-- 
2.16.2

[PATCH v3 14/20] rename: _request_firmware_load() fw_load_sysfs_fallback()

[Luis R. Rodriguez]

This reflects much clearer what is being done.
While at it, kdoc'ify it.

Signed-off-by: Luis R. Rodriguez 
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst |  2 +-
 drivers/base/firmware_loader/fallback.c   | 13 ++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index 4055ac76b288..f353783ae0be 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -112,7 +112,7 @@ Since a device is created for the sysfs interface to help 
load firmware as a
 fallback mechanism userspace can be informed of the addition of the device by
 relying on kobject uevents. The addition of the device into the device
 hierarchy means the fallback mechanism for firmware loading has been initiated.
-For details of implementation refer to _request_firmware_load(), in particular
+For details of implementation refer to fw_load_sysfs_fallback(), in particular
 on the use of dev_set_uevent_suppress() and kobject_uevent().
 
 The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,
diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index d6838e7ec00c..0a8ec7fec585 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -535,8 +535,15 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
return fw_sysfs;
 }
 
-/* load a firmware via user helper */
-static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
+/**
+ * fw_load_sysfs_fallback - load a firmware via the syfs fallback mechanism
+ * @fw_sysfs: firmware syfs information for the firmware to load
+ * @opt_flags: flags of options, FW_OPT_*
+ * @timeout: timeout to wait for the load
+ *
+ * In charge of constructing a sysfs fallback interface for firmware loading.
+ **/
+static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
  unsigned int opt_flags, long timeout)
 {
int retval = 0;
@@ -621,7 +628,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
}
 
fw_sysfs->fw_priv = firmware->priv;
-   ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
+   ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
 
if (!ret)
ret = assign_fw(firmware, device, opt_flags);
-- 
2.16.2

[PATCH v3 14/20] rename: _request_firmware_load() fw_load_sysfs_fallback()

[Luis R. Rodriguez]

This reflects much clearer what is being done.
While at it, kdoc'ify it.

Signed-off-by: Luis R. Rodriguez 
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst |  2 +-
 drivers/base/firmware_loader/fallback.c   | 13 ++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index 4055ac76b288..f353783ae0be 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -112,7 +112,7 @@ Since a device is created for the sysfs interface to help 
load firmware as a
 fallback mechanism userspace can be informed of the addition of the device by
 relying on kobject uevents. The addition of the device into the device
 hierarchy means the fallback mechanism for firmware loading has been initiated.
-For details of implementation refer to _request_firmware_load(), in particular
+For details of implementation refer to fw_load_sysfs_fallback(), in particular
 on the use of dev_set_uevent_suppress() and kobject_uevent().
 
 The kernel's kobject uevent mechanism is implemented in lib/kobject_uevent.c,
diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index d6838e7ec00c..0a8ec7fec585 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -535,8 +535,15 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
return fw_sysfs;
 }
 
-/* load a firmware via user helper */
-static int _request_firmware_load(struct fw_sysfs *fw_sysfs,
+/**
+ * fw_load_sysfs_fallback - load a firmware via the syfs fallback mechanism
+ * @fw_sysfs: firmware syfs information for the firmware to load
+ * @opt_flags: flags of options, FW_OPT_*
+ * @timeout: timeout to wait for the load
+ *
+ * In charge of constructing a sysfs fallback interface for firmware loading.
+ **/
+static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
  unsigned int opt_flags, long timeout)
 {
int retval = 0;
@@ -621,7 +628,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
}
 
fw_sysfs->fw_priv = firmware->priv;
-   ret = _request_firmware_load(fw_sysfs, opt_flags, timeout);
+   ret = fw_load_sysfs_fallback(fw_sysfs, opt_flags, timeout);
 
if (!ret)
ret = assign_fw(firmware, device, opt_flags);
-- 
2.16.2

[PATCH v3 17/20] test_firmware: modify custom fallback tests to use unique files

[Luis R. Rodriguez]

Users of the custom firmware fallback interface is are not supposed to
use the firmware cache interface, this can happen if for instance the
one of the APIs which use the firmware cache is used first with one
firmware file and then the request_firmware_nowait(uevent=false) API
is used with the same file.

We'll soon become strict about this on the firmware interface to reject
such calls later, so correct the test scripts to avoid such uses as well.
We address this on the tests scripts by simply using unique names when
testing the custom fallback interface.

Signed-off-by: Luis R. Rodriguez 
---
 tools/testing/selftests/firmware/fw_fallback.sh   | 20 ++--
 tools/testing/selftests/firmware/fw_filesystem.sh | 11 +--
 tools/testing/selftests/firmware/fw_lib.sh| 23 +++
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh 
b/tools/testing/selftests/firmware/fw_fallback.sh
index 9337a0328627..8e2e34a2ca69 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -238,8 +238,10 @@ run_sysfs_main_tests()
 
 run_sysfs_custom_load_tests()
 {
-   if load_fw_custom "$NAME" "$FW" ; then
-   if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+   if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null 
; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -247,8 +249,10 @@ run_sysfs_custom_load_tests()
fi
fi
 
-   if load_fw_custom "$NAME" "$FW" ; then
-   if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+   if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null 
; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -256,8 +260,12 @@ run_sysfs_custom_load_tests()
fi
fi
 
-   if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
-   if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_REAL="$RANDOM_FILE_PATH"
+   FAKE_RANDOM_FILE_PATH=$(setup_random_file_fake)
+   FAKE_RANDOM_FILE="$(basename $FAKE_RANDOM_FILE_PATH)"
+
+   if load_fw_custom_cancel "$FAKE_RANDOM_FILE" "$RANDOM_FILE_REAL" ; then
+   if diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; 
then
echo "$0: firmware was expected to be cancelled" >&2
exit 1
else
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 7f47877fa7fa..6452d2129cd9 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -230,10 +230,13 @@ test_wait_and_cancel_custom_load()
 test_request_firmware_nowait_custom_nofile()
 {
echo -n "Batched request_firmware_nowait(uevent=false) nofile try #$1: "
+   config_reset
config_unset_uevent
-   config_set_name nope-test-firmware.bin
+   RANDOM_FILE_PATH=$(setup_random_file_fake)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   config_set_name $RANDOM_FILE
config_trigger_async &
-   test_wait_and_cancel_custom_load nope-test-firmware.bin
+   test_wait_and_cancel_custom_load $RANDOM_FILE
wait
release_all_firmware
echo "OK"
@@ -271,7 +274,11 @@ test_request_firmware_nowait_uevent()
 test_request_firmware_nowait_custom()
 {
echo -n "Batched request_firmware_nowait(uevent=false) try #$1: "
+   config_reset
config_unset_uevent
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   config_set_name $RANDOM_FILE
config_trigger_async
release_all_firmware
echo "OK"
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index 98dceb847ba0..9ea31b57d71a 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -104,6 +104,29 @@ setup_tmp_file()
fi
 }
 
+__setup_random_file()
+{
+   RANDOM_FILE_PATH="$(mktemp -p $FWPATH)"
+   # mktemp says dry-run -n is unsafe, so...
+   if [[ "$1" = "fake" ]]; then
+   rm -rf $RANDOM_FILE_PATH
+   sync
+   else
+   echo "ABCD0123" >"$RANDOM_FILE_PATH"
+   fi
+   echo $RANDOM_FILE_PATH
+}
+
+setup_random_file()
+{
+   echo $(__setup_random_file)
+}
+

[PATCH v3 15/20] firmware: fix checking for return values for fw_add_devm_name()

[Luis R. Rodriguez]

Currently fw_add_devm_name() returns 1 if the firmware cache
was already set. This makes it complicated for us to check for
correctness. It is actually non-fatal if the firmware cache
is already setup, so just return 0, and simplify the checkers.

fw_add_devm_name() adds device's name onto the devres for the
device so that prior to suspend we cache the firmware onto memory,
so that on resume the firmware is reliably available. We never
were checking for success for this call though, meaning in some
really rare cases we my have never setup the firmware cache for
a device, which could in turn make resume fail.

This is all theoretical, no known issues have been reported.
This small issue has been present way since the addition of the
devres firmware cache names on v3.7.

Fixes: f531f05ae9437 ("firmware loader: store firmware name into devres list")
Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/main.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index c8966c84bd44..f5046887e362 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -403,7 +403,7 @@ static int fw_add_devm_name(struct device *dev, const char 
*name)
 
fwn = fw_find_devm_name(dev, name);
if (fwn)
-   return 1;
+   return 0;
 
fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
   GFP_KERNEL);
@@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
  unsigned int opt_flags)
 {
struct fw_priv *fw_priv = fw->priv;
+   int ret;
 
mutex_lock(_lock);
if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
@@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
 */
/* don't cache firmware handled without uevent */
if (device && (opt_flags & FW_OPT_UEVENT) &&
-   !(opt_flags & FW_OPT_NOCACHE))
-   fw_add_devm_name(device, fw_priv->fw_name);
+   !(opt_flags & FW_OPT_NOCACHE)) {
+   ret = fw_add_devm_name(device, fw_priv->fw_name);
+   if (ret) {
+   mutex_unlock(_lock);
+   return ret;
+   }
+   }
 
/*
 * After caching firmware image is started, let it piggyback
-- 
2.16.2

[PATCH v3 17/20] test_firmware: modify custom fallback tests to use unique files

[Luis R. Rodriguez]

Users of the custom firmware fallback interface is are not supposed to
use the firmware cache interface, this can happen if for instance the
one of the APIs which use the firmware cache is used first with one
firmware file and then the request_firmware_nowait(uevent=false) API
is used with the same file.

We'll soon become strict about this on the firmware interface to reject
such calls later, so correct the test scripts to avoid such uses as well.
We address this on the tests scripts by simply using unique names when
testing the custom fallback interface.

Signed-off-by: Luis R. Rodriguez 
---
 tools/testing/selftests/firmware/fw_fallback.sh   | 20 ++--
 tools/testing/selftests/firmware/fw_filesystem.sh | 11 +--
 tools/testing/selftests/firmware/fw_lib.sh| 23 +++
 3 files changed, 46 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh 
b/tools/testing/selftests/firmware/fw_fallback.sh
index 9337a0328627..8e2e34a2ca69 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -238,8 +238,10 @@ run_sysfs_main_tests()
 
 run_sysfs_custom_load_tests()
 {
-   if load_fw_custom "$NAME" "$FW" ; then
-   if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+   if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null 
; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -247,8 +249,10 @@ run_sysfs_custom_load_tests()
fi
fi
 
-   if load_fw_custom "$NAME" "$FW" ; then
-   if ! diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   if load_fw_custom "$RANDOM_FILE" "$RANDOM_FILE_PATH" ; then
+   if ! diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null 
; then
echo "$0: firmware was not loaded" >&2
exit 1
else
@@ -256,8 +260,12 @@ run_sysfs_custom_load_tests()
fi
fi
 
-   if load_fw_custom_cancel "nope-$NAME" "$FW" ; then
-   if diff -q "$FW" /dev/test_firmware >/dev/null ; then
+   RANDOM_FILE_REAL="$RANDOM_FILE_PATH"
+   FAKE_RANDOM_FILE_PATH=$(setup_random_file_fake)
+   FAKE_RANDOM_FILE="$(basename $FAKE_RANDOM_FILE_PATH)"
+
+   if load_fw_custom_cancel "$FAKE_RANDOM_FILE" "$RANDOM_FILE_REAL" ; then
+   if diff -q "$RANDOM_FILE_PATH" /dev/test_firmware >/dev/null ; 
then
echo "$0: firmware was expected to be cancelled" >&2
exit 1
else
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 7f47877fa7fa..6452d2129cd9 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -230,10 +230,13 @@ test_wait_and_cancel_custom_load()
 test_request_firmware_nowait_custom_nofile()
 {
echo -n "Batched request_firmware_nowait(uevent=false) nofile try #$1: "
+   config_reset
config_unset_uevent
-   config_set_name nope-test-firmware.bin
+   RANDOM_FILE_PATH=$(setup_random_file_fake)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   config_set_name $RANDOM_FILE
config_trigger_async &
-   test_wait_and_cancel_custom_load nope-test-firmware.bin
+   test_wait_and_cancel_custom_load $RANDOM_FILE
wait
release_all_firmware
echo "OK"
@@ -271,7 +274,11 @@ test_request_firmware_nowait_uevent()
 test_request_firmware_nowait_custom()
 {
echo -n "Batched request_firmware_nowait(uevent=false) try #$1: "
+   config_reset
config_unset_uevent
+   RANDOM_FILE_PATH=$(setup_random_file)
+   RANDOM_FILE="$(basename $RANDOM_FILE_PATH)"
+   config_set_name $RANDOM_FILE
config_trigger_async
release_all_firmware
echo "OK"
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index 98dceb847ba0..9ea31b57d71a 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -104,6 +104,29 @@ setup_tmp_file()
fi
 }
 
+__setup_random_file()
+{
+   RANDOM_FILE_PATH="$(mktemp -p $FWPATH)"
+   # mktemp says dry-run -n is unsafe, so...
+   if [[ "$1" = "fake" ]]; then
+   rm -rf $RANDOM_FILE_PATH
+   sync
+   else
+   echo "ABCD0123" >"$RANDOM_FILE_PATH"
+   fi
+   echo $RANDOM_FILE_PATH
+}
+
+setup_random_file()
+{
+   echo $(__setup_random_file)
+}
+

[PATCH v3 15/20] firmware: fix checking for return values for fw_add_devm_name()

[Luis R. Rodriguez]

Currently fw_add_devm_name() returns 1 if the firmware cache
was already set. This makes it complicated for us to check for
correctness. It is actually non-fatal if the firmware cache
is already setup, so just return 0, and simplify the checkers.

fw_add_devm_name() adds device's name onto the devres for the
device so that prior to suspend we cache the firmware onto memory,
so that on resume the firmware is reliably available. We never
were checking for success for this call though, meaning in some
really rare cases we my have never setup the firmware cache for
a device, which could in turn make resume fail.

This is all theoretical, no known issues have been reported.
This small issue has been present way since the addition of the
devres firmware cache names on v3.7.

Fixes: f531f05ae9437 ("firmware loader: store firmware name into devres list")
Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/main.c | 12 +---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index c8966c84bd44..f5046887e362 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -403,7 +403,7 @@ static int fw_add_devm_name(struct device *dev, const char 
*name)
 
fwn = fw_find_devm_name(dev, name);
if (fwn)
-   return 1;
+   return 0;
 
fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
   GFP_KERNEL);
@@ -431,6 +431,7 @@ int assign_fw(struct firmware *fw, struct device *device,
  unsigned int opt_flags)
 {
struct fw_priv *fw_priv = fw->priv;
+   int ret;
 
mutex_lock(_lock);
if (!fw_priv->size || fw_state_is_aborted(fw_priv)) {
@@ -447,8 +448,13 @@ int assign_fw(struct firmware *fw, struct device *device,
 */
/* don't cache firmware handled without uevent */
if (device && (opt_flags & FW_OPT_UEVENT) &&
-   !(opt_flags & FW_OPT_NOCACHE))
-   fw_add_devm_name(device, fw_priv->fw_name);
+   !(opt_flags & FW_OPT_NOCACHE)) {
+   ret = fw_add_devm_name(device, fw_priv->fw_name);
+   if (ret) {
+   mutex_unlock(_lock);
+   return ret;
+   }
+   }
 
/*
 * After caching firmware image is started, let it piggyback
-- 
2.16.2

[PATCH v3 16/20] firmware: add helper to check to see if fw cache is setup

[Luis R. Rodriguez]

Add a helper to check if the firmware cache is already setup for a device.
This will be used later.

Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/main.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index f5046887e362..b569d8a09392 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -396,13 +396,23 @@ static struct fw_name_devm *fw_find_devm_name(struct 
device *dev,
return fwn;
 }
 
-/* add firmware name into devres list */
-static int fw_add_devm_name(struct device *dev, const char *name)
+static bool fw_cache_is_setup(struct device *dev, const char *name)
 {
struct fw_name_devm *fwn;
 
fwn = fw_find_devm_name(dev, name);
if (fwn)
+   return true;
+
+   return false;
+}
+
+/* add firmware name into devres list */
+static int fw_add_devm_name(struct device *dev, const char *name)
+{
+   struct fw_name_devm *fwn;
+
+   if (fw_cache_is_setup(dev, name))
return 0;
 
fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
-- 
2.16.2

[PATCH v3 16/20] firmware: add helper to check to see if fw cache is setup

[Luis R. Rodriguez]

Add a helper to check if the firmware cache is already setup for a device.
This will be used later.

Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/main.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index f5046887e362..b569d8a09392 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -396,13 +396,23 @@ static struct fw_name_devm *fw_find_devm_name(struct 
device *dev,
return fwn;
 }
 
-/* add firmware name into devres list */
-static int fw_add_devm_name(struct device *dev, const char *name)
+static bool fw_cache_is_setup(struct device *dev, const char *name)
 {
struct fw_name_devm *fwn;
 
fwn = fw_find_devm_name(dev, name);
if (fwn)
+   return true;
+
+   return false;
+}
+
+/* add firmware name into devres list */
+static int fw_add_devm_name(struct device *dev, const char *name)
+{
+   struct fw_name_devm *fwn;
+
+   if (fw_cache_is_setup(dev, name))
return 0;
 
fwn = devres_alloc(fw_name_devm_release, sizeof(struct fw_name_devm),
-- 
2.16.2

[PATCH v3 12/20] test_firmware: expand on library with shared helpers

[Luis R. Rodriguez]

This expands our library with as many things we could find which
both scripts we use share.

Signed-off-by: Luis R. Rodriguez 
---
 tools/testing/selftests/firmware/fw_fallback.sh   | 31 +++---
 tools/testing/selftests/firmware/fw_filesystem.sh | 41 +++---
 tools/testing/selftests/firmware/fw_lib.sh| 52 +++
 3 files changed, 63 insertions(+), 61 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh 
b/tools/testing/selftests/firmware/fw_fallback.sh
index 323c921a2469..9337a0328627 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -6,30 +6,17 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
+TEST_REQS_FW_SYSFS_FALLBACK="yes"
+TEST_REQS_FW_SET_CUSTOM_PATH="no"
 TEST_DIR=$(dirname $0)
 source $TEST_DIR/fw_lib.sh
 
 check_mods
+check_setup
+verify_reqs
+setup_tmp_file
 
-HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y)
-HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
-
-if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-   OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
-else
-   echo "usermode helper disabled so ignoring test"
-   exit 0
-fi
-
-FWPATH=$(mktemp -d)
-FW="$FWPATH/test-firmware.bin"
-
-test_finish()
-{
-   echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-   rm -f "$FW"
-   rmdir "$FWPATH"
-}
+trap "test_finish" EXIT
 
 load_fw()
 {
@@ -168,12 +155,6 @@ load_fw_fallback_with_child()
return $RET
 }
 
-trap "test_finish" EXIT
-
-# This is an unlikely real-world firmware content. :)
-echo "ABCD0123" >"$FW"
-NAME=$(basename "$FW")
-
 test_syfs_timeout()
 {
DEVPATH="$DIR"/"nope-$NAME"/loading
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 748f2f5737e9..7f47877fa7fa 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -6,38 +6,15 @@
 # know so we can be sure we're not accidentally testing the user helper.
 set -e
 
+TEST_REQS_FW_SYSFS_FALLBACK="no"
+TEST_REQS_FW_SET_CUSTOM_PATH="yes"
 TEST_DIR=$(dirname $0)
 source $TEST_DIR/fw_lib.sh
 
 check_mods
-
-# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
-# These days most distros enable CONFIG_FW_LOADER_USER_HELPER but disable
-# CONFIG_FW_LOADER_USER_HELPER_FALLBACK. We use /sys/class/firmware/ as an
-# indicator for CONFIG_FW_LOADER_USER_HELPER.
-HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; 
else echo no; fi)
-
-if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-   OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
-fi
-
-OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path)
-
-FWPATH=$(mktemp -d)
-FW="$FWPATH/test-firmware.bin"
-
-test_finish()
-{
-   if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-   echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-   fi
-   if [ "$OLD_FWPATH" = "" ]; then
-   OLD_FWPATH=" "
-   fi
-   echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path
-   rm -f "$FW"
-   rmdir "$FWPATH"
-}
+check_setup
+verify_reqs
+setup_tmp_file
 
 trap "test_finish" EXIT
 
@@ -46,14 +23,6 @@ if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
echo 1 >/sys/class/firmware/timeout
 fi
 
-# Set the kernel search path.
-echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path
-
-# This is an unlikely real-world firmware content. :)
-echo "ABCD0123" >"$FW"
-
-NAME=$(basename "$FW")
-
 if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then
echo "$0: empty filename should not succeed" >&2
exit 1
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index 467567c758b9..e3cc4483fdba 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -43,6 +43,58 @@ check_mods()
fi
 }
 
+check_setup()
+{
+   HAS_FW_LOADER_USER_HELPER="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER=y)"
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)"
+
+   if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
+  OLD_TIMEOUT="$(cat /sys/class/firmware/timeout)"
+   fi
+
+   OLD_FWPATH="$(cat /sys/module/firmware_class/parameters/path)"
+}
+
+verify_reqs()
+{
+   if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then
+   if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
+   echo "usermode helper disabled so ignoring test"
+   exit 0
+   fi
+   fi
+}
+
+setup_tmp_file()
+{
+   FWPATH=$(mktemp -d)
+   FW="$FWPATH/test-firmware.bin"
+   echo "ABCD0123" >"$FW"
+   NAME=$(basename "$FW")
+   if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then

[PATCH v3 12/20] test_firmware: expand on library with shared helpers

[Luis R. Rodriguez]

This expands our library with as many things we could find which
both scripts we use share.

Signed-off-by: Luis R. Rodriguez 
---
 tools/testing/selftests/firmware/fw_fallback.sh   | 31 +++---
 tools/testing/selftests/firmware/fw_filesystem.sh | 41 +++---
 tools/testing/selftests/firmware/fw_lib.sh| 52 +++
 3 files changed, 63 insertions(+), 61 deletions(-)

diff --git a/tools/testing/selftests/firmware/fw_fallback.sh 
b/tools/testing/selftests/firmware/fw_fallback.sh
index 323c921a2469..9337a0328627 100755
--- a/tools/testing/selftests/firmware/fw_fallback.sh
+++ b/tools/testing/selftests/firmware/fw_fallback.sh
@@ -6,30 +6,17 @@
 # won't find so that we can do the load ourself manually.
 set -e
 
+TEST_REQS_FW_SYSFS_FALLBACK="yes"
+TEST_REQS_FW_SET_CUSTOM_PATH="no"
 TEST_DIR=$(dirname $0)
 source $TEST_DIR/fw_lib.sh
 
 check_mods
+check_setup
+verify_reqs
+setup_tmp_file
 
-HAS_FW_LOADER_USER_HELPER=$(kconfig_has CONFIG_FW_LOADER_USER_HELPER=y)
-HAS_FW_LOADER_USER_HELPER_FALLBACK=$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)
-
-if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-   OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
-else
-   echo "usermode helper disabled so ignoring test"
-   exit 0
-fi
-
-FWPATH=$(mktemp -d)
-FW="$FWPATH/test-firmware.bin"
-
-test_finish()
-{
-   echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-   rm -f "$FW"
-   rmdir "$FWPATH"
-}
+trap "test_finish" EXIT
 
 load_fw()
 {
@@ -168,12 +155,6 @@ load_fw_fallback_with_child()
return $RET
 }
 
-trap "test_finish" EXIT
-
-# This is an unlikely real-world firmware content. :)
-echo "ABCD0123" >"$FW"
-NAME=$(basename "$FW")
-
 test_syfs_timeout()
 {
DEVPATH="$DIR"/"nope-$NAME"/loading
diff --git a/tools/testing/selftests/firmware/fw_filesystem.sh 
b/tools/testing/selftests/firmware/fw_filesystem.sh
index 748f2f5737e9..7f47877fa7fa 100755
--- a/tools/testing/selftests/firmware/fw_filesystem.sh
+++ b/tools/testing/selftests/firmware/fw_filesystem.sh
@@ -6,38 +6,15 @@
 # know so we can be sure we're not accidentally testing the user helper.
 set -e
 
+TEST_REQS_FW_SYSFS_FALLBACK="no"
+TEST_REQS_FW_SET_CUSTOM_PATH="yes"
 TEST_DIR=$(dirname $0)
 source $TEST_DIR/fw_lib.sh
 
 check_mods
-
-# CONFIG_FW_LOADER_USER_HELPER has a sysfs class under /sys/class/firmware/
-# These days most distros enable CONFIG_FW_LOADER_USER_HELPER but disable
-# CONFIG_FW_LOADER_USER_HELPER_FALLBACK. We use /sys/class/firmware/ as an
-# indicator for CONFIG_FW_LOADER_USER_HELPER.
-HAS_FW_LOADER_USER_HELPER=$(if [ -d /sys/class/firmware/ ]; then echo yes; 
else echo no; fi)
-
-if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-   OLD_TIMEOUT=$(cat /sys/class/firmware/timeout)
-fi
-
-OLD_FWPATH=$(cat /sys/module/firmware_class/parameters/path)
-
-FWPATH=$(mktemp -d)
-FW="$FWPATH/test-firmware.bin"
-
-test_finish()
-{
-   if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
-   echo "$OLD_TIMEOUT" >/sys/class/firmware/timeout
-   fi
-   if [ "$OLD_FWPATH" = "" ]; then
-   OLD_FWPATH=" "
-   fi
-   echo -n "$OLD_FWPATH" >/sys/module/firmware_class/parameters/path
-   rm -f "$FW"
-   rmdir "$FWPATH"
-}
+check_setup
+verify_reqs
+setup_tmp_file
 
 trap "test_finish" EXIT
 
@@ -46,14 +23,6 @@ if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
echo 1 >/sys/class/firmware/timeout
 fi
 
-# Set the kernel search path.
-echo -n "$FWPATH" >/sys/module/firmware_class/parameters/path
-
-# This is an unlikely real-world firmware content. :)
-echo "ABCD0123" >"$FW"
-
-NAME=$(basename "$FW")
-
 if printf '\000' >"$DIR"/trigger_request 2> /dev/null; then
echo "$0: empty filename should not succeed" >&2
exit 1
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index 467567c758b9..e3cc4483fdba 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -43,6 +43,58 @@ check_mods()
fi
 }
 
+check_setup()
+{
+   HAS_FW_LOADER_USER_HELPER="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER=y)"
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)"
+
+   if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
+  OLD_TIMEOUT="$(cat /sys/class/firmware/timeout)"
+   fi
+
+   OLD_FWPATH="$(cat /sys/module/firmware_class/parameters/path)"
+}
+
+verify_reqs()
+{
+   if [ "$TEST_REQS_FW_SYSFS_FALLBACK" = "yes" ]; then
+   if [ ! "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
+   echo "usermode helper disabled so ignoring test"
+   exit 0
+   fi
+   fi
+}
+
+setup_tmp_file()
+{
+   FWPATH=$(mktemp -d)
+   FW="$FWPATH/test-firmware.bin"
+   echo "ABCD0123" >"$FW"
+   NAME=$(basename "$FW")
+   if [ "$TEST_REQS_FW_SET_CUSTOM_PATH" = "yes" ]; then
+   

[PATCH v3 18/20] firmware: ensure the firmware cache is not used on incompatible calls

[Luis R. Rodriguez]

request_firmware_into_buf() explicitly disables the firmware cache,
meanwhile the firmware cache cannot be used when request_firmware_nowait()
is used without the uevent. Enforce a sanity check for this to avoid future
issues undocumented behaviours should misuses of the firmware cache
happen later.

One of the reasons we want to enforce this is the firmware cache is
used for helping with suspend/resume, and if incompatible calls use it
they can stall suspend.

Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/main.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index b569d8a09392..2913bb0e5e7b 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -431,6 +431,11 @@ static int fw_add_devm_name(struct device *dev, const char 
*name)
return 0;
 }
 #else
+static bool fw_cache_is_setup(struct device *dev, const char *name)
+{
+   return false;
+}
+
 static int fw_add_devm_name(struct device *dev, const char *name)
 {
return 0;
@@ -672,6 +677,9 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 {
int ret;
 
+   if (fw_cache_is_setup(device, name))
+   return -EOPNOTSUPP;
+
__module_get(THIS_MODULE);
ret = _request_firmware(firmware_p, name, device, buf, size,
FW_OPT_UEVENT | FW_OPT_NOCACHE);
@@ -769,6 +777,12 @@ request_firmware_nowait(
fw_work->opt_flags = FW_OPT_NOWAIT |
(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
 
+   if (!uevent && fw_cache_is_setup(device, name)) {
+   kfree_const(fw_work->name);
+   kfree(fw_work);
+   return -EOPNOTSUPP;
+   }
+
if (!try_module_get(module)) {
kfree_const(fw_work->name);
kfree(fw_work);
-- 
2.16.2

[PATCH v3 18/20] firmware: ensure the firmware cache is not used on incompatible calls

[Luis R. Rodriguez]

request_firmware_into_buf() explicitly disables the firmware cache,
meanwhile the firmware cache cannot be used when request_firmware_nowait()
is used without the uevent. Enforce a sanity check for this to avoid future
issues undocumented behaviours should misuses of the firmware cache
happen later.

One of the reasons we want to enforce this is the firmware cache is
used for helping with suspend/resume, and if incompatible calls use it
they can stall suspend.

Signed-off-by: Luis R. Rodriguez 
---
 drivers/base/firmware_loader/main.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index b569d8a09392..2913bb0e5e7b 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -431,6 +431,11 @@ static int fw_add_devm_name(struct device *dev, const char 
*name)
return 0;
 }
 #else
+static bool fw_cache_is_setup(struct device *dev, const char *name)
+{
+   return false;
+}
+
 static int fw_add_devm_name(struct device *dev, const char *name)
 {
return 0;
@@ -672,6 +677,9 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 {
int ret;
 
+   if (fw_cache_is_setup(device, name))
+   return -EOPNOTSUPP;
+
__module_get(THIS_MODULE);
ret = _request_firmware(firmware_p, name, device, buf, size,
FW_OPT_UEVENT | FW_OPT_NOCACHE);
@@ -769,6 +777,12 @@ request_firmware_nowait(
fw_work->opt_flags = FW_OPT_NOWAIT |
(uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
 
+   if (!uevent && fw_cache_is_setup(device, name)) {
+   kfree_const(fw_work->name);
+   kfree(fw_work);
+   return -EOPNOTSUPP;
+   }
+
if (!try_module_get(module)) {
kfree_const(fw_work->name);
kfree(fw_work);
-- 
2.16.2

[PATCH v3 13/20] test_firmware: test three firmware kernel configs using a proc knob

[Luis R. Rodriguez]

Since we now have knobs to twiddle what used to be set on kernel
configurations we can build one base kernel configuration and modify
behaviour to mimic such kernel configurations to test them.

Provided you build a kernel with:

CONFIG_TEST_FIRMWARE=y
CONFIG_FW_LOADER=y
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y

We should now be able test all possible kernel configurations
when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
the built-in functionality of the built-in firmware.

If you're on an old kernel and either don't have /proc/config.gz
(CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
we cannot run these dynamic tests, so just run both scripts just
as we used to before making blunt assumptions about your setup
and requirements exactly as we did before.

Acked-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 tools/testing/selftests/firmware/Makefile|  2 +-
 tools/testing/selftests/firmware/fw_lib.sh   | 51 +
 tools/testing/selftests/firmware/fw_run_tests.sh | 70 
 3 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

diff --git a/tools/testing/selftests/firmware/Makefile 
b/tools/testing/selftests/firmware/Makefile
index 1894d625af2d..826f38d5dd19 100644
--- a/tools/testing/selftests/firmware/Makefile
+++ b/tools/testing/selftests/firmware/Makefile
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_fallback.sh
+TEST_PROGS := fw_run_tests.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index e3cc4483fdba..98dceb847ba0 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -47,6 +47,34 @@ check_setup()
 {
HAS_FW_LOADER_USER_HELPER="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER=y)"
HAS_FW_LOADER_USER_HELPER_FALLBACK="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)"
+   PROC_FW_IGNORE_SYSFS_FALLBACK="0"
+   PROC_FW_FORCE_SYSFS_FALLBACK="0"
+
+   if [ -z $PROC_SYS_DIR ]; then
+   PROC_SYS_DIR="/proc/sys/kernel"
+   fi
+
+   FW_PROC="${PROC_SYS_DIR}/firmware_config"
+   FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback"
+   FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback"
+
+   if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+   PROC_FW_FORCE_SYSFS_FALLBACK="$(cat $FW_FORCE_SYSFS_FALLBACK)"
+   fi
+
+   if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+   PROC_FW_IGNORE_SYSFS_FALLBACK="$(cat $FW_IGNORE_SYSFS_FALLBACK)"
+   fi
+
+   if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then
+   HAS_FW_LOADER_USER_HELPER="yes"
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="yes"
+   fi
+
+   if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="no"
+   HAS_FW_LOADER_USER_HELPER="no"
+   fi
 
if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
   OLD_TIMEOUT="$(cat /sys/class/firmware/timeout)"
@@ -76,6 +104,28 @@ setup_tmp_file()
fi
 }
 
+proc_set_force_sysfs_fallback()
+{
+   if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+   echo -n $1 > $FW_FORCE_SYSFS_FALLBACK
+   check_setup
+   fi
+}
+
+proc_set_ignore_sysfs_fallback()
+{
+   if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+   echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK
+   check_setup
+   fi
+}
+
+proc_restore_defaults()
+{
+   proc_set_force_sysfs_fallback 0
+   proc_set_ignore_sysfs_fallback 0
+}
+
 test_finish()
 {
if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
@@ -93,6 +143,7 @@ test_finish()
if [ -d $FWPATH ]; then
rm -rf "$FWPATH"
fi
+   proc_restore_defaults
 }
 
 kconfig_has()
diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh 
b/tools/testing/selftests/firmware/fw_run_tests.sh
new file mode 100755
index ..06d638e9dc62
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_run_tests.sh
@@ -0,0 +1,70 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This runs all known tests across all known possible configurations we could
+# emulate in one run.
+
+set -e
+
+TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
+
+export HAS_FW_LOADER_USER_HELPER=""
+export HAS_FW_LOADER_USER_HELPER_FALLBACK=""
+
+run_tests()
+{
+   proc_set_force_sysfs_fallback $1
+   proc_set_ignore_sysfs_fallback $2
+   $TEST_DIR/fw_filesystem.sh
+
+   proc_set_force_sysfs_fallback $1
+   proc_set_ignore_sysfs_fallback $2
+   $TEST_DIR/fw_fallback.sh
+}
+
+run_test_config_0001()
+{
+   echo "-"
+   echo "Running 

[PATCH v3 13/20] test_firmware: test three firmware kernel configs using a proc knob

[Luis R. Rodriguez]

Since we now have knobs to twiddle what used to be set on kernel
configurations we can build one base kernel configuration and modify
behaviour to mimic such kernel configurations to test them.

Provided you build a kernel with:

CONFIG_TEST_FIRMWARE=y
CONFIG_FW_LOADER=y
CONFIG_FW_LOADER_USER_HELPER=y
CONFIG_IKCONFIG=y
CONFIG_IKCONFIG_PROC=y

We should now be able test all possible kernel configurations
when FW_LOADER=y. Note that when FW_LOADER=m we just don't provide
the built-in functionality of the built-in firmware.

If you're on an old kernel and either don't have /proc/config.gz
(CONFIG_IKCONFIG_PROC) or haven't enabled CONFIG_FW_LOADER_USER_HELPER
we cannot run these dynamic tests, so just run both scripts just
as we used to before making blunt assumptions about your setup
and requirements exactly as we did before.

Acked-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 tools/testing/selftests/firmware/Makefile|  2 +-
 tools/testing/selftests/firmware/fw_lib.sh   | 51 +
 tools/testing/selftests/firmware/fw_run_tests.sh | 70 
 3 files changed, 122 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

diff --git a/tools/testing/selftests/firmware/Makefile 
b/tools/testing/selftests/firmware/Makefile
index 1894d625af2d..826f38d5dd19 100644
--- a/tools/testing/selftests/firmware/Makefile
+++ b/tools/testing/selftests/firmware/Makefile
@@ -3,7 +3,7 @@
 # No binaries, but make sure arg-less "make" doesn't trigger "run_tests"
 all:
 
-TEST_PROGS := fw_filesystem.sh fw_fallback.sh
+TEST_PROGS := fw_run_tests.sh
 
 include ../lib.mk
 
diff --git a/tools/testing/selftests/firmware/fw_lib.sh 
b/tools/testing/selftests/firmware/fw_lib.sh
index e3cc4483fdba..98dceb847ba0 100755
--- a/tools/testing/selftests/firmware/fw_lib.sh
+++ b/tools/testing/selftests/firmware/fw_lib.sh
@@ -47,6 +47,34 @@ check_setup()
 {
HAS_FW_LOADER_USER_HELPER="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER=y)"
HAS_FW_LOADER_USER_HELPER_FALLBACK="$(kconfig_has 
CONFIG_FW_LOADER_USER_HELPER_FALLBACK=y)"
+   PROC_FW_IGNORE_SYSFS_FALLBACK="0"
+   PROC_FW_FORCE_SYSFS_FALLBACK="0"
+
+   if [ -z $PROC_SYS_DIR ]; then
+   PROC_SYS_DIR="/proc/sys/kernel"
+   fi
+
+   FW_PROC="${PROC_SYS_DIR}/firmware_config"
+   FW_FORCE_SYSFS_FALLBACK="$FW_PROC/force_sysfs_fallback"
+   FW_IGNORE_SYSFS_FALLBACK="$FW_PROC/ignore_sysfs_fallback"
+
+   if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+   PROC_FW_FORCE_SYSFS_FALLBACK="$(cat $FW_FORCE_SYSFS_FALLBACK)"
+   fi
+
+   if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+   PROC_FW_IGNORE_SYSFS_FALLBACK="$(cat $FW_IGNORE_SYSFS_FALLBACK)"
+   fi
+
+   if [ "$PROC_FW_FORCE_SYSFS_FALLBACK" = "1" ]; then
+   HAS_FW_LOADER_USER_HELPER="yes"
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="yes"
+   fi
+
+   if [ "$PROC_FW_IGNORE_SYSFS_FALLBACK" = "1" ]; then
+   HAS_FW_LOADER_USER_HELPER_FALLBACK="no"
+   HAS_FW_LOADER_USER_HELPER="no"
+   fi
 
if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
   OLD_TIMEOUT="$(cat /sys/class/firmware/timeout)"
@@ -76,6 +104,28 @@ setup_tmp_file()
fi
 }
 
+proc_set_force_sysfs_fallback()
+{
+   if [ -f $FW_FORCE_SYSFS_FALLBACK ]; then
+   echo -n $1 > $FW_FORCE_SYSFS_FALLBACK
+   check_setup
+   fi
+}
+
+proc_set_ignore_sysfs_fallback()
+{
+   if [ -f $FW_IGNORE_SYSFS_FALLBACK ]; then
+   echo -n $1 > $FW_IGNORE_SYSFS_FALLBACK
+   check_setup
+   fi
+}
+
+proc_restore_defaults()
+{
+   proc_set_force_sysfs_fallback 0
+   proc_set_ignore_sysfs_fallback 0
+}
+
 test_finish()
 {
if [ "$HAS_FW_LOADER_USER_HELPER" = "yes" ]; then
@@ -93,6 +143,7 @@ test_finish()
if [ -d $FWPATH ]; then
rm -rf "$FWPATH"
fi
+   proc_restore_defaults
 }
 
 kconfig_has()
diff --git a/tools/testing/selftests/firmware/fw_run_tests.sh 
b/tools/testing/selftests/firmware/fw_run_tests.sh
new file mode 100755
index ..06d638e9dc62
--- /dev/null
+++ b/tools/testing/selftests/firmware/fw_run_tests.sh
@@ -0,0 +1,70 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This runs all known tests across all known possible configurations we could
+# emulate in one run.
+
+set -e
+
+TEST_DIR=$(dirname $0)
+source $TEST_DIR/fw_lib.sh
+
+export HAS_FW_LOADER_USER_HELPER=""
+export HAS_FW_LOADER_USER_HELPER_FALLBACK=""
+
+run_tests()
+{
+   proc_set_force_sysfs_fallback $1
+   proc_set_ignore_sysfs_fallback $2
+   $TEST_DIR/fw_filesystem.sh
+
+   proc_set_force_sysfs_fallback $1
+   proc_set_ignore_sysfs_fallback $2
+   $TEST_DIR/fw_fallback.sh
+}
+
+run_test_config_0001()
+{
+   echo "-"
+   echo "Running kernel configuration test 1 -- rare"
+   

[PATCH v3 19/20] firmware: add request_firmware_cache() to help with cache on reboot

[Luis R. Rodriguez]

Some devices have an optimization in place to enable the firmware to
be retaineed during a system reboot, so after reboot the device can skip
requesting and loading the firmware. This can save up to 1s in load
time. The mt7601u 802.11 device happens to be such a device.

When these devices retain the firmware on a reboot and then suspend
they can miss looking for the firmware on resume. To help with this we
need a way to cache the firmware when such an optimization has taken
place.

Signed-off-by: Luis R. Rodriguez 
---
 .../driver-api/firmware/request_firmware.rst   | 14 +
 drivers/base/firmware_loader/main.c| 24 ++
 include/linux/firmware.h   |  3 +++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index cc0aea880824..b554f4338859 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,6 +44,20 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
:functions: request_firmware_nowait
 
+Special optimizations on reboot
+===
+
+Some devices have an optimization in place to enable the firmware to be
+retained during system reboot. When such optimizations are used the driver
+author must ensure the firmware is still available on resume from suspend,
+this can be done with request_firmware_cache() insted of requesting for the
+firmare to be loaded.
+
+request_firmware_cache()
+---
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_load
+
 request firmware API expected driver use
 
 
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 2913bb0e5e7b..04bf853f60e9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -656,6 +656,30 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * request_firmware_cache: - cache firmware for suspend so resume can use it
+ * @name: name of firmware file
+ * @device: device for which firmware should be cached for
+ *
+ * There are some devices with an optimization that enables the device to not
+ * require loading firmware on system reboot. This optimization may still
+ * require the firmware present on resume from suspend. This routine can be
+ * used to ensure the firmware is present on resume from suspend in these
+ * situations. This helper is not compatible with drivers which use
+ * request_firmware_into_buf() or request_firmware_nowait() with no uevent set.
+ **/
+int request_firmware_cache(struct device *device, const char *name)
+{
+   int ret;
+
+   mutex_lock(_lock);
+   ret = fw_add_devm_name(device, name);
+   mutex_unlock(_lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_cache);
+
 /**
  * request_firmware_into_buf - load firmware into a previously allocated buffer
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d4508080348d..166867910ebc 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -85,4 +85,7 @@ static inline int request_firmware_into_buf(const struct 
firmware **firmware_p,
 }
 
 #endif
+
+int request_firmware_cache(struct device *device, const char *name);
+
 #endif
-- 
2.16.2

[PATCH v3 19/20] firmware: add request_firmware_cache() to help with cache on reboot

[Luis R. Rodriguez]

Some devices have an optimization in place to enable the firmware to
be retaineed during a system reboot, so after reboot the device can skip
requesting and loading the firmware. This can save up to 1s in load
time. The mt7601u 802.11 device happens to be such a device.

When these devices retain the firmware on a reboot and then suspend
they can miss looking for the firmware on resume. To help with this we
need a way to cache the firmware when such an optimization has taken
place.

Signed-off-by: Luis R. Rodriguez 
---
 .../driver-api/firmware/request_firmware.rst   | 14 +
 drivers/base/firmware_loader/main.c| 24 ++
 include/linux/firmware.h   |  3 +++
 3 files changed, 41 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index cc0aea880824..b554f4338859 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -44,6 +44,20 @@ request_firmware_nowait
 .. kernel-doc:: drivers/base/firmware_class.c
:functions: request_firmware_nowait
 
+Special optimizations on reboot
+===
+
+Some devices have an optimization in place to enable the firmware to be
+retained during system reboot. When such optimizations are used the driver
+author must ensure the firmware is still available on resume from suspend,
+this can be done with request_firmware_cache() insted of requesting for the
+firmare to be loaded.
+
+request_firmware_cache()
+---
+.. kernel-doc:: drivers/base/firmware_class.c
+   :functions: request_firmware_load
+
 request firmware API expected driver use
 
 
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 2913bb0e5e7b..04bf853f60e9 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -656,6 +656,30 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 }
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
+/**
+ * request_firmware_cache: - cache firmware for suspend so resume can use it
+ * @name: name of firmware file
+ * @device: device for which firmware should be cached for
+ *
+ * There are some devices with an optimization that enables the device to not
+ * require loading firmware on system reboot. This optimization may still
+ * require the firmware present on resume from suspend. This routine can be
+ * used to ensure the firmware is present on resume from suspend in these
+ * situations. This helper is not compatible with drivers which use
+ * request_firmware_into_buf() or request_firmware_nowait() with no uevent set.
+ **/
+int request_firmware_cache(struct device *device, const char *name)
+{
+   int ret;
+
+   mutex_lock(_lock);
+   ret = fw_add_devm_name(device, name);
+   mutex_unlock(_lock);
+
+   return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_cache);
+
 /**
  * request_firmware_into_buf - load firmware into a previously allocated buffer
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index d4508080348d..166867910ebc 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -85,4 +85,7 @@ static inline int request_firmware_into_buf(const struct 
firmware **firmware_p,
 }
 
 #endif
+
+int request_firmware_cache(struct device *device, const char *name);
+
 #endif
-- 
2.16.2

[PATCH v3 20/20] mt7601u: use request_firmware_cache() to address cache on reboot

[Luis R. Rodriguez]

request_firmware_cache() will ensure the firmware is available on resume
from suspend if on reboot the device retains the firmware.

This optimization is in place given otherwise on reboot we have to
reload the firmware, the opmization saves us about max 1s, minimum 10ms.

Cantabile has reported back this fixes his woes with both suspend and
hibernation.

Reported-by: Cantabile 
Tested-by: Cantabile 
Signed-off-by: Luis R. Rodriguez 
---
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c 
b/drivers/net/wireless/mediatek/mt7601u/mcu.c
index 65a8004418ea..b90456a4b4d7 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
@@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
 MT_USB_DMA_CFG_TX_BULK_EN));
 
if (firmware_running(dev))
-   return 0;
+   return request_firmware_cache(dev->dev, MT7601U_FIRMWARE);
 
ret = request_firmware(, MT7601U_FIRMWARE, dev->dev);
if (ret)
-- 
2.16.2

[PATCH v3 20/20] mt7601u: use request_firmware_cache() to address cache on reboot

[Luis R. Rodriguez]

request_firmware_cache() will ensure the firmware is available on resume
from suspend if on reboot the device retains the firmware.

This optimization is in place given otherwise on reboot we have to
reload the firmware, the opmization saves us about max 1s, minimum 10ms.

Cantabile has reported back this fixes his woes with both suspend and
hibernation.

Reported-by: Cantabile 
Tested-by: Cantabile 
Signed-off-by: Luis R. Rodriguez 
---
 drivers/net/wireless/mediatek/mt7601u/mcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c 
b/drivers/net/wireless/mediatek/mt7601u/mcu.c
index 65a8004418ea..b90456a4b4d7 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
@@ -421,7 +421,7 @@ static int mt7601u_load_firmware(struct mt7601u_dev *dev)
 MT_USB_DMA_CFG_TX_BULK_EN));
 
if (firmware_running(dev))
-   return 0;
+   return request_firmware_cache(dev->dev, MT7601U_FIRMWARE);
 
ret = request_firmware(, MT7601U_FIRMWARE, dev->dev);
if (ret)
-- 
2.16.2

[PATCH v3 09/20] firmware: move firmware loader into its own directory

[Luis R. Rodriguez]

This will make it much easier to manage as we manage to
keep trimming componnents down into their own files to more
easily manage and maintain this codebase.

Suggested-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 MAINTAINERS   | 2 +-
 drivers/base/Makefile | 7 ++-
 drivers/base/firmware_loader/Makefile | 7 +++
 drivers/base/{firmware_fallback.c => firmware_loader/fallback.c}  | 4 ++--
 drivers/base/{firmware_fallback.h => firmware_loader/fallback.h}  | 0
 .../fallback_table.c} | 4 ++--
 drivers/base/{firmware_loader.h => firmware_loader/firmware.h}| 0
 drivers/base/{firmware_loader.c => firmware_loader/main.c}| 8 
 8 files changed, 18 insertions(+), 14 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Makefile
 rename drivers/base/{firmware_fallback.c => firmware_loader/fallback.c} (99%)
 rename drivers/base/{firmware_fallback.h => firmware_loader/fallback.h} (100%)
 rename drivers/base/{firmware_fallback_table.c => 
firmware_loader/fallback_table.c} (90%)
 rename drivers/base/{firmware_loader.h => firmware_loader/firmware.h} (100%)
 rename drivers/base/{firmware_loader.c => firmware_loader/main.c} (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index e03a130902cd..6ddd6f4aaffa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5589,7 +5589,7 @@ M:Luis R. Rodriguez 
 L: linux-kernel@vger.kernel.org
 S: Maintained
 F: Documentation/firmware_class/
-F: drivers/base/firmware*.c
+F: drivers/base/firmware_loader/
 F: include/linux/firmware.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index b946a408256d..b9539abec675 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -5,17 +5,14 @@ obj-y := component.o core.o bus.o dd.o 
syscore.o \
   driver.o class.o platform.o \
   cpu.o firmware.o init.o map.o devres.o \
   attribute_container.o transport_class.o \
-  topology.o container.o property.o cacheinfo.o \
-  firmware_fallback_table.o
+  topology.o container.o property.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS) += devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y  += power/
 obj-$(CONFIG_HAS_DMA)  += dma-mapping.o
 obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
 obj-$(CONFIG_ISA_BUS_API)  += isa.o
-obj-$(CONFIG_FW_LOADER)+= firmware_class.o
-firmware_class-objs := firmware_loader.o
-firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += firmware_fallback.o
+obj-y  += firmware_loader/
 obj-$(CONFIG_NUMA) += node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)
diff --git a/drivers/base/firmware_loader/Makefile 
b/drivers/base/firmware_loader/Makefile
new file mode 100644
index ..a97eeb0be1d8
--- /dev/null
+++ b/drivers/base/firmware_loader/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for the Linux firmware loader
+
+obj-y  := fallback_table.o
+obj-$(CONFIG_FW_LOADER)+= firmware_class.o
+firmware_class-objs := main.o
+firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
diff --git a/drivers/base/firmware_fallback.c 
b/drivers/base/firmware_loader/fallback.c
similarity index 99%
rename from drivers/base/firmware_fallback.c
rename to drivers/base/firmware_loader/fallback.c
index 47690207e0ee..9b65837256d6 100644
--- a/drivers/base/firmware_fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -8,8 +8,8 @@
 #include 
 #include 
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback mechanism
diff --git a/drivers/base/firmware_fallback.h 
b/drivers/base/firmware_loader/fallback.h
similarity index 100%
rename from drivers/base/firmware_fallback.h
rename to drivers/base/firmware_loader/fallback.h
diff --git a/drivers/base/firmware_fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
similarity index 90%
rename from drivers/base/firmware_fallback_table.c
rename to drivers/base/firmware_loader/fallback_table.c
index 53cc4e492520..981419044c7e 100644
--- a/drivers/base/firmware_fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -9,8 +9,8 @@
 #include 
 #include 
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback configuration table
diff --git a/drivers/base/firmware_loader.h 
b/drivers/base/firmware_loader/firmware.h
similarity index 100%
rename from drivers/base/firmware_loader.h
rename to 

[PATCH v3 09/20] firmware: move firmware loader into its own directory

[Luis R. Rodriguez]

This will make it much easier to manage as we manage to
keep trimming componnents down into their own files to more
easily manage and maintain this codebase.

Suggested-by: Kees Cook 
Signed-off-by: Luis R. Rodriguez 
---
 MAINTAINERS   | 2 +-
 drivers/base/Makefile | 7 ++-
 drivers/base/firmware_loader/Makefile | 7 +++
 drivers/base/{firmware_fallback.c => firmware_loader/fallback.c}  | 4 ++--
 drivers/base/{firmware_fallback.h => firmware_loader/fallback.h}  | 0
 .../fallback_table.c} | 4 ++--
 drivers/base/{firmware_loader.h => firmware_loader/firmware.h}| 0
 drivers/base/{firmware_loader.c => firmware_loader/main.c}| 8 
 8 files changed, 18 insertions(+), 14 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Makefile
 rename drivers/base/{firmware_fallback.c => firmware_loader/fallback.c} (99%)
 rename drivers/base/{firmware_fallback.h => firmware_loader/fallback.h} (100%)
 rename drivers/base/{firmware_fallback_table.c => 
firmware_loader/fallback_table.c} (90%)
 rename drivers/base/{firmware_loader.h => firmware_loader/firmware.h} (100%)
 rename drivers/base/{firmware_loader.c => firmware_loader/main.c} (99%)

diff --git a/MAINTAINERS b/MAINTAINERS
index e03a130902cd..6ddd6f4aaffa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -5589,7 +5589,7 @@ M:Luis R. Rodriguez 
 L: linux-kernel@vger.kernel.org
 S: Maintained
 F: Documentation/firmware_class/
-F: drivers/base/firmware*.c
+F: drivers/base/firmware_loader/
 F: include/linux/firmware.h
 
 FLASH ADAPTER DRIVER (IBM Flash Adapter 900GB Full Height PCI Flash Card)
diff --git a/drivers/base/Makefile b/drivers/base/Makefile
index b946a408256d..b9539abec675 100644
--- a/drivers/base/Makefile
+++ b/drivers/base/Makefile
@@ -5,17 +5,14 @@ obj-y := component.o core.o bus.o dd.o 
syscore.o \
   driver.o class.o platform.o \
   cpu.o firmware.o init.o map.o devres.o \
   attribute_container.o transport_class.o \
-  topology.o container.o property.o cacheinfo.o \
-  firmware_fallback_table.o
+  topology.o container.o property.o cacheinfo.o
 obj-$(CONFIG_DEVTMPFS) += devtmpfs.o
 obj-$(CONFIG_DMA_CMA) += dma-contiguous.o
 obj-y  += power/
 obj-$(CONFIG_HAS_DMA)  += dma-mapping.o
 obj-$(CONFIG_HAVE_GENERIC_DMA_COHERENT) += dma-coherent.o
 obj-$(CONFIG_ISA_BUS_API)  += isa.o
-obj-$(CONFIG_FW_LOADER)+= firmware_class.o
-firmware_class-objs := firmware_loader.o
-firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += firmware_fallback.o
+obj-y  += firmware_loader/
 obj-$(CONFIG_NUMA) += node.o
 obj-$(CONFIG_MEMORY_HOTPLUG_SPARSE) += memory.o
 ifeq ($(CONFIG_SYSFS),y)
diff --git a/drivers/base/firmware_loader/Makefile 
b/drivers/base/firmware_loader/Makefile
new file mode 100644
index ..a97eeb0be1d8
--- /dev/null
+++ b/drivers/base/firmware_loader/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+# Makefile for the Linux firmware loader
+
+obj-y  := fallback_table.o
+obj-$(CONFIG_FW_LOADER)+= firmware_class.o
+firmware_class-objs := main.o
+firmware_class-$(CONFIG_FW_LOADER_USER_HELPER) += fallback.o
diff --git a/drivers/base/firmware_fallback.c 
b/drivers/base/firmware_loader/fallback.c
similarity index 99%
rename from drivers/base/firmware_fallback.c
rename to drivers/base/firmware_loader/fallback.c
index 47690207e0ee..9b65837256d6 100644
--- a/drivers/base/firmware_fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -8,8 +8,8 @@
 #include 
 #include 
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback mechanism
diff --git a/drivers/base/firmware_fallback.h 
b/drivers/base/firmware_loader/fallback.h
similarity index 100%
rename from drivers/base/firmware_fallback.h
rename to drivers/base/firmware_loader/fallback.h
diff --git a/drivers/base/firmware_fallback_table.c 
b/drivers/base/firmware_loader/fallback_table.c
similarity index 90%
rename from drivers/base/firmware_fallback_table.c
rename to drivers/base/firmware_loader/fallback_table.c
index 53cc4e492520..981419044c7e 100644
--- a/drivers/base/firmware_fallback_table.c
+++ b/drivers/base/firmware_loader/fallback_table.c
@@ -9,8 +9,8 @@
 #include 
 #include 
 
-#include "firmware_fallback.h"
-#include "firmware_loader.h"
+#include "fallback.h"
+#include "firmware.h"
 
 /*
  * firmware fallback configuration table
diff --git a/drivers/base/firmware_loader.h 
b/drivers/base/firmware_loader/firmware.h
similarity index 100%
rename from drivers/base/firmware_loader.h
rename to drivers/base/firmware_loader/firmware.h
diff --git 

[PATCH v3 00/20] firmware: development for v4.17

[Luis R. Rodriguez]

Greg,

Here's a respin of what I have queued up for v4.17 for the firmware API. It
combines the cleanup I've been working on and the addition of the new API call
request_firmware_cache() for fixing a corner case suspend issue on some type of
cards with an optimization in place where the firmware is *not* needed on
reboot.

The cleanup work allows us to test the firmware API with one kernel
configuration. I've addressed Kees' feedback on this respin and
combined the code into drivers/base/firmware_class/.

I've made one new test_firmware change in consideration for one firmware
change, the patch "firmware: ensure the firmware cache is not used on
incompatible calls" requires us to modify our tests scripts to use
the APIs sanely as well.

I've put up these changes on my git tree, refer to the branch
"20180307-firmware-dev-for-v4.17" based on linux-next [0] and
the same name based on Linus' tree [1].

Questions, feedback, and specially rants are always welcomed.

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180307-firmware-dev-for-v4.17
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180307-firmware-dev-for-v4.17

Luis R. Rodriguez (20):
  test_firmware: add simple firmware firmware test library
  test_firmware: enable custom fallback testing on limited kernel
configs
  test_firmware: replace syfs fallback check with kconfig_has helper
  firmware: enable to split firmware_class into separate target files
  firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further
  firmware: use helpers for setting up a temporary cache timeout
  firmware: move loading timeout under struct firmware_fallback_config
  firmware: split firmware fallback functionality into its own file
  firmware: move firmware loader into its own directory
  firmware: enable run time change of forcing fallback loader
  firmware: enable to force disable the fallback mechanism at run time
  test_firmware: expand on library with shared helpers
  test_firmware: test three firmware kernel configs using a proc knob
  rename: _request_firmware_load() fw_load_sysfs_fallback()
  firmware: fix checking for return values for fw_add_devm_name()
  firmware: add helper to check to see if fw cache is setup
  test_firmware: modify custom fallback tests to use unique files
  firmware: ensure the firmware cache is not used on incompatible calls
  firmware: add request_firmware_cache() to help with cache on reboot
  mt7601u: use request_firmware_cache() to address cache on reboot

 .../driver-api/firmware/fallback-mechanisms.rst|   2 +-
 .../driver-api/firmware/request_firmware.rst   |  14 +
 MAINTAINERS|   2 +-
 drivers/base/Makefile  |   2 +-
 drivers/base/firmware_loader/Makefile  |   7 +
 drivers/base/firmware_loader/fallback.c| 674 +
 drivers/base/firmware_loader/fallback.h|  67 ++
 drivers/base/firmware_loader/fallback_table.c  |  55 ++
 drivers/base/firmware_loader/firmware.h| 115 +++
 .../{firmware_class.c => firmware_loader/main.c}   | 833 ++---
 drivers/net/wireless/mediatek/mt7601u/mcu.c|   2 +-
 include/linux/firmware.h   |   3 +
 kernel/sysctl.c|  11 +
 tools/testing/selftests/firmware/Makefile  |   2 +-
 tools/testing/selftests/firmware/config|   4 +
 tools/testing/selftests/firmware/fw_fallback.sh|  65 +-
 tools/testing/selftests/firmware/fw_filesystem.sh  |  72 +-
 tools/testing/selftests/firmware/fw_lib.sh | 194 +
 tools/testing/selftests/firmware/fw_run_tests.sh   |  70 ++
 19 files changed, 1332 insertions(+), 862 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Makefile
 create mode 100644 drivers/base/firmware_loader/fallback.c
 create mode 100644 drivers/base/firmware_loader/fallback.h
 create mode 100644 drivers/base/firmware_loader/fallback_table.c
 create mode 100644 drivers/base/firmware_loader/firmware.h
 rename drivers/base/{firmware_class.c => firmware_loader/main.c} (60%)
 create mode 100755 tools/testing/selftests/firmware/fw_lib.sh
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

-- 
2.16.2

[PATCH v3 00/20] firmware: development for v4.17

[Luis R. Rodriguez]

Greg,

Here's a respin of what I have queued up for v4.17 for the firmware API. It
combines the cleanup I've been working on and the addition of the new API call
request_firmware_cache() for fixing a corner case suspend issue on some type of
cards with an optimization in place where the firmware is *not* needed on
reboot.

The cleanup work allows us to test the firmware API with one kernel
configuration. I've addressed Kees' feedback on this respin and
combined the code into drivers/base/firmware_class/.

I've made one new test_firmware change in consideration for one firmware
change, the patch "firmware: ensure the firmware cache is not used on
incompatible calls" requires us to modify our tests scripts to use
the APIs sanely as well.

I've put up these changes on my git tree, refer to the branch
"20180307-firmware-dev-for-v4.17" based on linux-next [0] and
the same name based on Linus' tree [1].

Questions, feedback, and specially rants are always welcomed.

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180307-firmware-dev-for-v4.17
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180307-firmware-dev-for-v4.17

Luis R. Rodriguez (20):
  test_firmware: add simple firmware firmware test library
  test_firmware: enable custom fallback testing on limited kernel
configs
  test_firmware: replace syfs fallback check with kconfig_has helper
  firmware: enable to split firmware_class into separate target files
  firmware: simplify CONFIG_FW_LOADER_USER_HELPER_FALLBACK further
  firmware: use helpers for setting up a temporary cache timeout
  firmware: move loading timeout under struct firmware_fallback_config
  firmware: split firmware fallback functionality into its own file
  firmware: move firmware loader into its own directory
  firmware: enable run time change of forcing fallback loader
  firmware: enable to force disable the fallback mechanism at run time
  test_firmware: expand on library with shared helpers
  test_firmware: test three firmware kernel configs using a proc knob
  rename: _request_firmware_load() fw_load_sysfs_fallback()
  firmware: fix checking for return values for fw_add_devm_name()
  firmware: add helper to check to see if fw cache is setup
  test_firmware: modify custom fallback tests to use unique files
  firmware: ensure the firmware cache is not used on incompatible calls
  firmware: add request_firmware_cache() to help with cache on reboot
  mt7601u: use request_firmware_cache() to address cache on reboot

 .../driver-api/firmware/fallback-mechanisms.rst|   2 +-
 .../driver-api/firmware/request_firmware.rst   |  14 +
 MAINTAINERS|   2 +-
 drivers/base/Makefile  |   2 +-
 drivers/base/firmware_loader/Makefile  |   7 +
 drivers/base/firmware_loader/fallback.c| 674 +
 drivers/base/firmware_loader/fallback.h|  67 ++
 drivers/base/firmware_loader/fallback_table.c  |  55 ++
 drivers/base/firmware_loader/firmware.h| 115 +++
 .../{firmware_class.c => firmware_loader/main.c}   | 833 ++---
 drivers/net/wireless/mediatek/mt7601u/mcu.c|   2 +-
 include/linux/firmware.h   |   3 +
 kernel/sysctl.c|  11 +
 tools/testing/selftests/firmware/Makefile  |   2 +-
 tools/testing/selftests/firmware/config|   4 +
 tools/testing/selftests/firmware/fw_fallback.sh|  65 +-
 tools/testing/selftests/firmware/fw_filesystem.sh  |  72 +-
 tools/testing/selftests/firmware/fw_lib.sh | 194 +
 tools/testing/selftests/firmware/fw_run_tests.sh   |  70 ++
 19 files changed, 1332 insertions(+), 862 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Makefile
 create mode 100644 drivers/base/firmware_loader/fallback.c
 create mode 100644 drivers/base/firmware_loader/fallback.h
 create mode 100644 drivers/base/firmware_loader/fallback_table.c
 create mode 100644 drivers/base/firmware_loader/firmware.h
 rename drivers/base/{firmware_class.c => firmware_loader/main.c} (60%)
 create mode 100755 tools/testing/selftests/firmware/fw_lib.sh
 create mode 100755 tools/testing/selftests/firmware/fw_run_tests.sh

-- 
2.16.2

Re: [PATCH 0/3] module: process aliasing when debugging

[Luis R. Rodriguez]

*Poke*

  Luis

On Wed, Nov 29, 2017 at 6:36 PM, Luis R. Rodriguez  wrote:
> Debugging ineractions with userspace can often be a bit of pain, specially
> when trying to figure out who is at fault for an issue. Having the kernel
> process aliases when debugging can help us much faster find who is the
> culprit to an issue.
>
> I've been carrying this around privately in my tree since 2016 but it seems
> others can benefit from similar debugging functionality so pushing this out
> for integration and wider review.
>
> Further testing, thoughts, reviews, flames are all equally appreciated.
>
> Luis R. Rodriguez (3):
>   module: use goto errors on check_modinfo() and layout_and_allocate()
>   module: add helper get_modinfo_idx()
>   module: add debugging alias parsing support
>
>  include/linux/module.h |   4 ++
>  init/Kconfig   |   7 +++
>  kernel/module.c| 129 
> +++--
>  3 files changed, 135 insertions(+), 5 deletions(-)
>
> --
> 2.15.0
>

Re: [PATCH 0/3] module: process aliasing when debugging

[Luis R. Rodriguez]

*Poke*

  Luis

On Wed, Nov 29, 2017 at 6:36 PM, Luis R. Rodriguez  wrote:
> Debugging ineractions with userspace can often be a bit of pain, specially
> when trying to figure out who is at fault for an issue. Having the kernel
> process aliases when debugging can help us much faster find who is the
> culprit to an issue.
>
> I've been carrying this around privately in my tree since 2016 but it seems
> others can benefit from similar debugging functionality so pushing this out
> for integration and wider review.
>
> Further testing, thoughts, reviews, flames are all equally appreciated.
>
> Luis R. Rodriguez (3):
>   module: use goto errors on check_modinfo() and layout_and_allocate()
>   module: add helper get_modinfo_idx()
>   module: add debugging alias parsing support
>
>  include/linux/module.h |   4 ++
>  init/Kconfig   |   7 +++
>  kernel/module.c| 129 
> +++--
>  3 files changed, 135 insertions(+), 5 deletions(-)
>
> --
> 2.15.0
>

Re: [PATCH net-next] modules: allow modprobe load regular elf binaries

[Luis R. Rodriguez]

On Fri, Mar 09, 2018 at 06:34:18PM -0800, Alexei Starovoitov wrote:
> On 3/9/18 11:38 AM, Linus Torvalds wrote:
> > On Fri, Mar 9, 2018 at 11:12 AM, Linus Torvalds
> >  wrote:
> > > 
> > > How are you going to handle five processes doing the same setup
> > > concurrently?

Let's keep in mind we don't have a formal way to specify this as well
for modules as well, other than kconfig. Ie it'd be up to the author
of the modules to pick this up and make things mutually exclusive.

> > Side note: it's not just serialization. It's also "is it actually up
> > and running".
> > 
> > The rule for "request_module()" (for a real module) has been that it
> > returns when the module is actually alive and active and have done
> > their initcalls.

Unfortunately this is not accurate though, the loose grammar around this fact
is one of the reasons why long term I think either new API should be added, or
the existing request_module() API modified.

request_module() is not deterministic today, try_then_request_module() *is* but
its IMHO its a horrible grammatical solution, and I'm not a fan of the idea of
umh modules perpetuating this nonsense *long term*. Details below.

> > The UMH_WAIT_EXEC behavior (ignore the serialization - you could do
> > that in the caller) behavior doesn't actually have any semantics AT
> > ALL. It only means that you get the error returns from execve()
> > itself, so you know that the executable file actually existed and
> > parsed right enough to get started.
> > 
> > But you don't actually have any reason to believe that it has *done*
> > anything, and started processing any requests. There's no reason
> > what-so-ever to believe that it has registered itself for any
> > asynchronous requests or anything like that.
> 
> I agree that sync approach nicely fits this use case, but waiting
> for umh brings the whole set of suspend/resume issues that
> Luis explained earlier and if I understood his points that stuff
> is still not quite working right

It works enough that suspend works well enough on Linux today, but the primary
reason is that the big kernel/umh.c API user today is the firmware API for the
old fallback firmware interface and it has in place the sync
usermodehelper_read_trylock() and async async usermodehelper_read_lock_wait().
Note that in the fallback case there is just the uevent call.

> and he's planning a set of fixes.

The move of the umh locks out of the non-fallback case was a long term goal I
had which I was planning on doing slowly, but recently got jump started via
v4.14 via commit f007cad159e9 ("Revert "firmware: add sanity check on
shutdown/suspend"). As such that goal is now complete thanks to Linus correctly
pushing it forward.

> I really don't want the unknown timeline of fixes for 'waiting umh'
> to become a blocker for the bpfilter project ...

The reason for me to bring up the suspend stuff was that there no other
*heavy* UMH API users in the kernel today, and just to highlight that
care must be taken if we want to consider in the future further
possibly heavy UMH callers which we *can* expect to be called around
suspend and resume.

*Iff* that will be the case for these new umh userspace modules, we can
evaluate a future plan. But not that this is as vague as suggesting the
same for any further kernel future UMH API user! If the only umh module
we expect for a while will be bpfilter and it we don't expect heavy use
during suspend/resume this is a non-issue and likely won't be for a while,
and all that *should be done* is become aware of the today's limitations
as we *document* any new API, even if its the simple and easy
request_umh_module*() calls.

Today's use of the UMH API to always use helper_lock() and prevent suspend
while a call is being issued should suffice, so long as these umh modules don't
become heavy users during suspend/resume.

Note that there even *further* further advanced suspend/resume considerations
with filesystems but we have a reasonable hand on what to do there [0] and
it frankly isn't *that* much work as I have most of it done already.

The only other suspend optimization I could think of left to evaluate may be
whether or not to we should generalize something like the firmware cache for
other UMH callers but that's really a long term topic.

So I would not say there is pending work left to do, it should suffice
to document the semantics and limitations if the umh modules are to be
added. That's it.

Linus has proven me right that the *concerns* I've had for these corner
cases are just that, and I do believe documenting the limitations should
suffice for new APIs.

[0] https://lkml.kernel.org/r/20180126090923.gd12...@wotan.suse.de

> Also I like Luis suggestion to use some other name than request_module()
> Something like:
> request_umh_module_sync("foo");
> request_umh_module_nowait("foo");
> 
> in both cases the kernel would create a pipe, call umh either
> with UMH_WAIT_PROC or UMH_WAIT_EXEC and make 

Re: [PATCH net-next] modules: allow modprobe load regular elf binaries

[Luis R. Rodriguez]

On Fri, Mar 09, 2018 at 06:34:18PM -0800, Alexei Starovoitov wrote:
> On 3/9/18 11:38 AM, Linus Torvalds wrote:
> > On Fri, Mar 9, 2018 at 11:12 AM, Linus Torvalds
> >  wrote:
> > > 
> > > How are you going to handle five processes doing the same setup
> > > concurrently?

Let's keep in mind we don't have a formal way to specify this as well
for modules as well, other than kconfig. Ie it'd be up to the author
of the modules to pick this up and make things mutually exclusive.

> > Side note: it's not just serialization. It's also "is it actually up
> > and running".
> > 
> > The rule for "request_module()" (for a real module) has been that it
> > returns when the module is actually alive and active and have done
> > their initcalls.

Unfortunately this is not accurate though, the loose grammar around this fact
is one of the reasons why long term I think either new API should be added, or
the existing request_module() API modified.

request_module() is not deterministic today, try_then_request_module() *is* but
its IMHO its a horrible grammatical solution, and I'm not a fan of the idea of
umh modules perpetuating this nonsense *long term*. Details below.

> > The UMH_WAIT_EXEC behavior (ignore the serialization - you could do
> > that in the caller) behavior doesn't actually have any semantics AT
> > ALL. It only means that you get the error returns from execve()
> > itself, so you know that the executable file actually existed and
> > parsed right enough to get started.
> > 
> > But you don't actually have any reason to believe that it has *done*
> > anything, and started processing any requests. There's no reason
> > what-so-ever to believe that it has registered itself for any
> > asynchronous requests or anything like that.
> 
> I agree that sync approach nicely fits this use case, but waiting
> for umh brings the whole set of suspend/resume issues that
> Luis explained earlier and if I understood his points that stuff
> is still not quite working right

It works enough that suspend works well enough on Linux today, but the primary
reason is that the big kernel/umh.c API user today is the firmware API for the
old fallback firmware interface and it has in place the sync
usermodehelper_read_trylock() and async async usermodehelper_read_lock_wait().
Note that in the fallback case there is just the uevent call.

> and he's planning a set of fixes.

The move of the umh locks out of the non-fallback case was a long term goal I
had which I was planning on doing slowly, but recently got jump started via
v4.14 via commit f007cad159e9 ("Revert "firmware: add sanity check on
shutdown/suspend"). As such that goal is now complete thanks to Linus correctly
pushing it forward.

> I really don't want the unknown timeline of fixes for 'waiting umh'
> to become a blocker for the bpfilter project ...

The reason for me to bring up the suspend stuff was that there no other
*heavy* UMH API users in the kernel today, and just to highlight that
care must be taken if we want to consider in the future further
possibly heavy UMH callers which we *can* expect to be called around
suspend and resume.

*Iff* that will be the case for these new umh userspace modules, we can
evaluate a future plan. But not that this is as vague as suggesting the
same for any further kernel future UMH API user! If the only umh module
we expect for a while will be bpfilter and it we don't expect heavy use
during suspend/resume this is a non-issue and likely won't be for a while,
and all that *should be done* is become aware of the today's limitations
as we *document* any new API, even if its the simple and easy
request_umh_module*() calls.

Today's use of the UMH API to always use helper_lock() and prevent suspend
while a call is being issued should suffice, so long as these umh modules don't
become heavy users during suspend/resume.

Note that there even *further* further advanced suspend/resume considerations
with filesystems but we have a reasonable hand on what to do there [0] and
it frankly isn't *that* much work as I have most of it done already.

The only other suspend optimization I could think of left to evaluate may be
whether or not to we should generalize something like the firmware cache for
other UMH callers but that's really a long term topic.

So I would not say there is pending work left to do, it should suffice
to document the semantics and limitations if the umh modules are to be
added. That's it.

Linus has proven me right that the *concerns* I've had for these corner
cases are just that, and I do believe documenting the limitations should
suffice for new APIs.

[0] https://lkml.kernel.org/r/20180126090923.gd12...@wotan.suse.de

> Also I like Luis suggestion to use some other name than request_module()
> Something like:
> request_umh_module_sync("foo");
> request_umh_module_nowait("foo");
> 
> in both cases the kernel would create a pipe, call umh either
> with UMH_WAIT_PROC or UMH_WAIT_EXEC and make sure that umh
> responds to first 

Re: [PATCH] EDAC, sb_edac: Remove VLA usage

[Borislav Petkov]

On Fri, Mar 09, 2018 at 09:02:18PM -0600, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wvla, remove VLA and replace it
> with a fixed-length array instead.
> 
> Fixed as part of the directive to remove all VLAs from
> the kernel: https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
> 
> Notice that due to this change, the field max_interleave is no longer
> used after it has been initialized. Maybe it should be removed?

Yes.

> @@ -110,6 +110,7 @@ static const u32 knl_interleave_list[] = {
>   0xdc, 0xe4, 0xec, 0xf4, 0xfc, /* 15-19 */
>   0x104, 0x10c, 0x114, 0x11c,   /* 20-23 */
>  };
> +#define MAX_INTERLEAVE   ARRAY_SIZE(knl_interleave_list)

define that as the max of all interleave lists array sizes so that
people can update it properly when new interleave lists get added.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] EDAC, sb_edac: Remove VLA usage

[Borislav Petkov]

On Fri, Mar 09, 2018 at 09:02:18PM -0600, Gustavo A. R. Silva wrote:
> In preparation to enabling -Wvla, remove VLA and replace it
> with a fixed-length array instead.
> 
> Fixed as part of the directive to remove all VLAs from
> the kernel: https://lkml.org/lkml/2018/3/7/621
> 
> Signed-off-by: Gustavo A. R. Silva 
> ---
> 
> Notice that due to this change, the field max_interleave is no longer
> used after it has been initialized. Maybe it should be removed?

Yes.

> @@ -110,6 +110,7 @@ static const u32 knl_interleave_list[] = {
>   0xdc, 0xe4, 0xec, 0xf4, 0xfc, /* 15-19 */
>   0x104, 0x10c, 0x114, 0x11c,   /* 20-23 */
>  };
> +#define MAX_INTERLEAVE   ARRAY_SIZE(knl_interleave_list)

define that as the max of all interleave lists array sizes so that
people can update it properly when new interleave lists get added.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings

[Fabio Estevam]

Hi Tim,

On Mon, Mar 5, 2018 at 7:02 PM, Tim Harvey  wrote:

> +
> +   hwmon@1 { /* A1: Input Voltage */
> +   type = <1>;
> +   reg = <0x02>;

Unit address (@1) does not match the 'reg' value.

If someone build this example with W=1 a warning will be shown.

Same problem happens in other places of this example.

Re: [PATCH v2 1/4] dt-bindings: mfd: Add Gateworks System Controller bindings

[Fabio Estevam]

Hi Tim,

On Mon, Mar 5, 2018 at 7:02 PM, Tim Harvey  wrote:

> +
> +   hwmon@1 { /* A1: Input Voltage */
> +   type = <1>;
> +   reg = <0x02>;

Unit address (@1) does not match the 'reg' value.

If someone build this example with W=1 a warning will be shown.

Same problem happens in other places of this example.

Re: [RFC PATCH -tip 0/9] kprobes: Cleanup jprobe implementation

[Masami Hiramatsu]

On Fri, 9 Mar 2018 18:54:02 -0500
Steven Rostedt  wrote:

> On Fri,  9 Mar 2018 21:35:17 +0900
> Masami Hiramatsu  wrote:
> 
> > Hello,
> > 
> > Since we decided to remove jprobe from kernel last year,
> > its APIs are disabled and we worked on moving in-kernel
> > jprobe users to kprobes or trace-events. And now no jprobe
> > users are here anymore.
> > 
> > I think it is good time to get rid of jprobe implementation
> > from the kernel. However, I need other arch developers help
> > to complete it, since jprobe is implemented multi arch wide.
> > I can remove those code, but can not test all of those.
> > 
> > Here is the series of patches to show how to do that.
> > I tried to remove it from x86 tree. Basically we need to
> > do 3 things;
> > 
> >  - Remove jprobe functions (register/unregister,
> >setjump/longjump) from generic/arch-dependent code.
> >[1/9][2/9][3/9]
> >  - Remove break_handler related code.
> >[4/9][5/9][6/9]
> >  - Do not disable preemption on exception handler
> >[7/9][8/9][9/9]
> > 
> > The [3/9] and [6/9] are destractive changes except for x86
> > (means causes build errors) since those arch still have some
> > references of those functions. So we need to write patches
> > similar to [2/9] and [5/9] for each arch before applying those.
> > In this series I sorted it as this order just for review,
> > [3/9] and [6/9] should be applied after all archs have
> > been fixed.
> > 
> > Also, [7/9] is a kind of destractive, which changes required
> > behavior for the pre_handlers which changes regs->ip.
> > So we also need a patch similar to [7/9] for each arch too.
> > Fortunately, current in-tree such user is very limited, both
> > works only on x86. So it is not hurry, but we need to change
> > arch dependent code.
> >
> 
> Hi Masami,
> 
> thanks for doing all this. I do want to review this and your other
> patch set.

Thanks!

> I've just been traveling a lot. I came home from California
> yesterday and will be leaving Sunday to Portland for ELC. Will you be
> there?

Oh, no, sorry I'll not be there. Anyway, I will wait for your review. :)

> 
> -- Steve


-- 
Masami Hiramatsu 

Re: [RFC PATCH -tip 0/9] kprobes: Cleanup jprobe implementation

[Masami Hiramatsu]

On Fri, 9 Mar 2018 18:54:02 -0500
Steven Rostedt  wrote:

> On Fri,  9 Mar 2018 21:35:17 +0900
> Masami Hiramatsu  wrote:
> 
> > Hello,
> > 
> > Since we decided to remove jprobe from kernel last year,
> > its APIs are disabled and we worked on moving in-kernel
> > jprobe users to kprobes or trace-events. And now no jprobe
> > users are here anymore.
> > 
> > I think it is good time to get rid of jprobe implementation
> > from the kernel. However, I need other arch developers help
> > to complete it, since jprobe is implemented multi arch wide.
> > I can remove those code, but can not test all of those.
> > 
> > Here is the series of patches to show how to do that.
> > I tried to remove it from x86 tree. Basically we need to
> > do 3 things;
> > 
> >  - Remove jprobe functions (register/unregister,
> >setjump/longjump) from generic/arch-dependent code.
> >[1/9][2/9][3/9]
> >  - Remove break_handler related code.
> >[4/9][5/9][6/9]
> >  - Do not disable preemption on exception handler
> >[7/9][8/9][9/9]
> > 
> > The [3/9] and [6/9] are destractive changes except for x86
> > (means causes build errors) since those arch still have some
> > references of those functions. So we need to write patches
> > similar to [2/9] and [5/9] for each arch before applying those.
> > In this series I sorted it as this order just for review,
> > [3/9] and [6/9] should be applied after all archs have
> > been fixed.
> > 
> > Also, [7/9] is a kind of destractive, which changes required
> > behavior for the pre_handlers which changes regs->ip.
> > So we also need a patch similar to [7/9] for each arch too.
> > Fortunately, current in-tree such user is very limited, both
> > works only on x86. So it is not hurry, but we need to change
> > arch dependent code.
> >
> 
> Hi Masami,
> 
> thanks for doing all this. I do want to review this and your other
> patch set.

Thanks!

> I've just been traveling a lot. I came home from California
> yesterday and will be leaving Sunday to Portland for ELC. Will you be
> there?

Oh, no, sorry I'll not be there. Anyway, I will wait for your review. :)

> 
> -- Steve


-- 
Masami Hiramatsu 

Re: [PATCH] device_handler: remove VLAs

[Stephen Kitt]

Hi Bart,

On Fri, 9 Mar 2018 22:48:10 +, Bart Van Assche 
wrote:
> On Fri, 2018-03-09 at 23:32 +0100, Stephen Kitt wrote:
> > In preparation to enabling -Wvla, remove VLAs and replace them with
> > fixed-length arrays instead.
> > 
> > scsi_dh_{alua,emc,rdac} use variable-length array declarations to
> > store command blocks, with the appropriate size as determined by
> > COMMAND_SIZE. This patch replaces these with fixed-sized arrays using
> > MAX_COMMAND_SIZE, so that the array size can be determined at compile
> > time.  
> 
> If COMMAND_SIZE() is evaluated at compile time, do we still need this patch?

The two patches I sent were supposed to be alternative solutions; see
https://marc.info/?l=linux-scsi=152063671005295=2 for the introduction (I
seem to have messed up the headers, so the mails didn’t end up threaded
properly).

The MAX_COMMAND_SIZE approach is nice and simple, but I thought it worth
eliminating scsi_command_size_tbl while I was at it...

Regards,

Stephen


pgpMBvDcL8Ium.pgp
Description: OpenPGP digital signature

Re: [PATCH] device_handler: remove VLAs

[Stephen Kitt]

Hi Bart,

On Fri, 9 Mar 2018 22:48:10 +, Bart Van Assche 
wrote:
> On Fri, 2018-03-09 at 23:32 +0100, Stephen Kitt wrote:
> > In preparation to enabling -Wvla, remove VLAs and replace them with
> > fixed-length arrays instead.
> > 
> > scsi_dh_{alua,emc,rdac} use variable-length array declarations to
> > store command blocks, with the appropriate size as determined by
> > COMMAND_SIZE. This patch replaces these with fixed-sized arrays using
> > MAX_COMMAND_SIZE, so that the array size can be determined at compile
> > time.  
> 
> If COMMAND_SIZE() is evaluated at compile time, do we still need this patch?

The two patches I sent were supposed to be alternative solutions; see
https://marc.info/?l=linux-scsi=152063671005295=2 for the introduction (I
seem to have messed up the headers, so the mails didn’t end up threaded
properly).

The MAX_COMMAND_SIZE approach is nice and simple, but I thought it worth
eliminating scsi_command_size_tbl while I was at it...

Regards,

Stephen


pgpMBvDcL8Ium.pgp
Description: OpenPGP digital signature

[GIT PULL] Kbuild fixes for v4.16-rc5

[Masahiro Yamada]

Hi Linus,

Please pull more Kbuild fixes for v4.16.



The following changes since commit 661e50bc853209e41a5c14a290ca4decc43cbfd1:

  Linux 4.16-rc4 (2018-03-04 14:54:11 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
tags/kbuild-fixes-v4.16-2

for you to fetch changes up to 55fe6da9efba102866e2fb5b40b04b6a4b26c19e:

  kbuild: Handle builtin dtb file names containing hyphens (2018-03-09
01:14:38 +0900)


Kbuild fixes for v4.16 (2nd)

- make fixdep parse kconfig.h to fix missing rebuild

- replace hyphens with underscores in builtin DTB label names

- fix typos


James Hogan (1):
  kbuild: Handle builtin dtb file names containing hyphens

Matteo Croce (1):
  scripts/bloat-o-meter: fix typos in help

Rasmus Villemoes (3):
  fixdep: remove stale references to uml-config.h
  fixdep: remove some false CONFIG_ matches
  fixdep: do not ignore kconfig.h

 scripts/Makefile.lib   |  8 
 scripts/basic/fixdep.c | 15 +--
 scripts/bloat-o-meter  |  2 +-
 3 files changed, 10 insertions(+), 15 deletions(-)


-- 
Best Regards
Masahiro Yamada

[GIT PULL] Kbuild fixes for v4.16-rc5

[Masahiro Yamada]

Hi Linus,

Please pull more Kbuild fixes for v4.16.



The following changes since commit 661e50bc853209e41a5c14a290ca4decc43cbfd1:

  Linux 4.16-rc4 (2018-03-04 14:54:11 -0800)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/masahiroy/linux-kbuild.git
tags/kbuild-fixes-v4.16-2

for you to fetch changes up to 55fe6da9efba102866e2fb5b40b04b6a4b26c19e:

  kbuild: Handle builtin dtb file names containing hyphens (2018-03-09
01:14:38 +0900)


Kbuild fixes for v4.16 (2nd)

- make fixdep parse kconfig.h to fix missing rebuild

- replace hyphens with underscores in builtin DTB label names

- fix typos


James Hogan (1):
  kbuild: Handle builtin dtb file names containing hyphens

Matteo Croce (1):
  scripts/bloat-o-meter: fix typos in help

Rasmus Villemoes (3):
  fixdep: remove stale references to uml-config.h
  fixdep: remove some false CONFIG_ matches
  fixdep: do not ignore kconfig.h

 scripts/Makefile.lib   |  8 
 scripts/basic/fixdep.c | 15 +--
 scripts/bloat-o-meter  |  2 +-
 3 files changed, 10 insertions(+), 15 deletions(-)


-- 
Best Regards
Masahiro Yamada

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Stephen Kitt]

Hi Bart,

On Fri, 9 Mar 2018 22:47:12 +, Bart Van Assche 
wrote:
> On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:
> > +/*
> > + * SCSI command sizes are as follows, in bytes, for fixed size commands,
> > per
> > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
> > + * determine its group.
> > + * The size table is encoded into a 32-bit value by subtracting each
> > value
> > + * from 16, resulting in a value of 1715488362
> > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 +
> > 10).
> > + * Command group 3 is reserved and should never be used.
> > + */
> > +#define COMMAND_SIZE(opcode) \
> > +   (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)  
> 
> To me this seems hard to read and hard to verify. Could this have been
> written as a combination of ternary expressions, e.g. using a gcc statement
> expression to ensure that opcode is evaluated once?

That’s what I’d tried initially, e.g.

#define COMMAND_SIZE(opcode) ({ \
int index = ((opcode) >> 5) & 7; \
index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : 10); \
})

But gcc still reckons that results in a VLA, defeating the initial purpose of
the exercise.

Does it help if I make the magic value construction clearer?

#define SCSI_COMMAND_SIZE_TBL ( \
   (16 -  6)\
+ ((16 - 10) <<  4) \
+ ((16 - 10) <<  8) \
+ ((16 - 12) << 12) \
+ ((16 - 16) << 16) \
+ ((16 - 12) << 20) \
+ ((16 - 10) << 24) \
+ ((16 - 10) << 28))

#define COMMAND_SIZE(opcode)\
  (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & 7)

Regards,

Stephen


pgp03pWqk7H6k.pgp
Description: OpenPGP digital signature

Re: [PATCH] scsi: resolve COMMAND_SIZE at compile time

[Stephen Kitt]

Hi Bart,

On Fri, 9 Mar 2018 22:47:12 +, Bart Van Assche 
wrote:
> On Fri, 2018-03-09 at 23:33 +0100, Stephen Kitt wrote:
> > +/*
> > + * SCSI command sizes are as follows, in bytes, for fixed size commands,
> > per
> > + * group: 6, 10, 10, 12, 16, 12, 10, 10. The top three bits of an opcode
> > + * determine its group.
> > + * The size table is encoded into a 32-bit value by subtracting each
> > value
> > + * from 16, resulting in a value of 1715488362
> > + * (6 << 28 + 6 << 24 + 4 << 20 + 0 << 16 + 4 << 12 + 6 << 8 + 6 << 4 +
> > 10).
> > + * Command group 3 is reserved and should never be used.
> > + */
> > +#define COMMAND_SIZE(opcode) \
> > +   (16 - (15 & (1715488362 >> (4 * (((opcode) >> 5) & 7)  
> 
> To me this seems hard to read and hard to verify. Could this have been
> written as a combination of ternary expressions, e.g. using a gcc statement
> expression to ensure that opcode is evaluated once?

That’s what I’d tried initially, e.g.

#define COMMAND_SIZE(opcode) ({ \
int index = ((opcode) >> 5) & 7; \
index == 0 ? 6 : (index == 4 ? 16 : index == 3 || index == 5 ? 12 : 10); \
})

But gcc still reckons that results in a VLA, defeating the initial purpose of
the exercise.

Does it help if I make the magic value construction clearer?

#define SCSI_COMMAND_SIZE_TBL ( \
   (16 -  6)\
+ ((16 - 10) <<  4) \
+ ((16 - 10) <<  8) \
+ ((16 - 12) << 12) \
+ ((16 - 16) << 16) \
+ ((16 - 12) << 20) \
+ ((16 - 10) << 24) \
+ ((16 - 10) << 28))

#define COMMAND_SIZE(opcode)\
  (16 - (15 & (SCSI_COMMAND_SIZE_TBL >> (4 * (((opcode) >> 5) & 7)

Regards,

Stephen


pgp03pWqk7H6k.pgp
Description: OpenPGP digital signature

Re: [PATCH 0/8] Move most GPIO documentation to driver-api/gpio/ and ReST

[Jonathan Neuschäfer]

On Sat, Mar 10, 2018 at 12:50:46PM +0100, Linus Walleij wrote:
> On Fri, Mar 9, 2018 at 12:40 AM, Jonathan Neuschäfer
>  wrote:
> 
> > The aim of this patchset is to move the GPIO subsystem's documentation
> > under Documentation/driver-api/gpio/ such that it is picked up by Sphinx
> > and compiled into HTML.
> 
> Awesome!
> 
> > I moved everything except for sysfs.txt, because
> > this file describes the legacy sysfs ABI, and doesn't seem to serve much
> > (non-historical) purpose anymore.
> >
> > There are still some rough edges:
> >
> > * I think the API documentation (kernel-doc) should be moved out of
> >   index.rst into more appropriate files.
> > * The headings are arguably wrong, because driver.rst, consumer.rst,
> >   etc. use the "document title" style, even though they are part of the
> >   GPIO chapter. But the resulting TOC tree is consistent, and I did not
> >   want to change almost all headings.
> > * Some of the files could use more :c:func:`...` references and general
> >   ReST polish.
> >
> > But I don't want to make this patchset too large.
> 
> It's fine, we take it one step at a time.
> 
> On Fri, Mar 09, 2018 at 10:41:20AM -0700, Jonathan Corbet wrote:
> 
> > Anyway, I'm happy to take these through the docs tree or see them go via
> > GPIO; Linus, what's your preference?
> 
> I might have some doc patches that could collide so I can take them.
> 
> I take it there will be a v2?

Not necessarily, but if I need to rebase on a different branch, or on
future linux-next, I'll do that. (Or if some other issue is found.)

I thought that patch 8/8 currently requires commit 93db446a424c ("mtd:
nand: move raw NAND related code to the raw/ subdir"), which is in next
through Boris Brezillon's nand tree, but this isn't true. The patchset
(currently) applies to both v4.16-rc4 and next, AFAICS.


Thanks,
Jonathan Neuschäfer


signature.asc
Description: PGP signature

Re: [PATCH 0/8] Move most GPIO documentation to driver-api/gpio/ and ReST

[Jonathan Neuschäfer]

On Sat, Mar 10, 2018 at 12:50:46PM +0100, Linus Walleij wrote:
> On Fri, Mar 9, 2018 at 12:40 AM, Jonathan Neuschäfer
>  wrote:
> 
> > The aim of this patchset is to move the GPIO subsystem's documentation
> > under Documentation/driver-api/gpio/ such that it is picked up by Sphinx
> > and compiled into HTML.
> 
> Awesome!
> 
> > I moved everything except for sysfs.txt, because
> > this file describes the legacy sysfs ABI, and doesn't seem to serve much
> > (non-historical) purpose anymore.
> >
> > There are still some rough edges:
> >
> > * I think the API documentation (kernel-doc) should be moved out of
> >   index.rst into more appropriate files.
> > * The headings are arguably wrong, because driver.rst, consumer.rst,
> >   etc. use the "document title" style, even though they are part of the
> >   GPIO chapter. But the resulting TOC tree is consistent, and I did not
> >   want to change almost all headings.
> > * Some of the files could use more :c:func:`...` references and general
> >   ReST polish.
> >
> > But I don't want to make this patchset too large.
> 
> It's fine, we take it one step at a time.
> 
> On Fri, Mar 09, 2018 at 10:41:20AM -0700, Jonathan Corbet wrote:
> 
> > Anyway, I'm happy to take these through the docs tree or see them go via
> > GPIO; Linus, what's your preference?
> 
> I might have some doc patches that could collide so I can take them.
> 
> I take it there will be a v2?

Not necessarily, but if I need to rebase on a different branch, or on
future linux-next, I'll do that. (Or if some other issue is found.)

I thought that patch 8/8 currently requires commit 93db446a424c ("mtd:
nand: move raw NAND related code to the raw/ subdir"), which is in next
through Boris Brezillon's nand tree, but this isn't true. The patchset
(currently) applies to both v4.16-rc4 and next, AFAICS.


Thanks,
Jonathan Neuschäfer


signature.asc
Description: PGP signature

arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   cdb06e9d8f520c969676e7d6778cffe5894f079f
commit: 101110f6271ce956a049250c907bc960030577f8 Kbuild: always define 
endianess in kconfig.h
date:   2 weeks ago
config: h8300-h8300h-sim_defconfig (attached as .config)
compiler: h8300-linux-gcc (GCC) 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 101110f6271ce956a049250c907bc960030577f8
# save the attached .config to linux build tree
make.cross ARCH=h8300 

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bitops/le.h:6:0,
from arch/h8300/include/asm/bitops.h:177,
from include/linux/bitops.h:38,
from include/linux/log2.h:16,
from include/asm-generic/getorder.h:8,
from include/asm-generic/page.h:99,
from arch/h8300/include/asm/page.h:5,
from arch/h8300/include/asm/string.h:8,
from include/linux/string.h:20,
from include/linux/uuid.h:20,
from include/linux/mod_devicetable.h:13,
from scripts/mod/devicetable-offsets.c:3:
>> arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined
#define __BIG_ENDIAN __ORDER_BIG_ENDIAN__

   In file included from :0:0:
   include/linux/kconfig.h:8:0: note: this is the location of the previous 
definition
#define __BIG_ENDIAN 4321

--
   In file included from include/asm-generic/bitops/le.h:6:0,
from arch/h8300/include/asm/bitops.h:177,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from include/asm-generic/bug.h:18,
from arch/h8300/include/asm/bug.h:8,
from include/linux/bug.h:5,
from include/linux/page-flags.h:10,
from kernel/bounds.c:10:
>> arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined
#define __BIG_ENDIAN __ORDER_BIG_ENDIAN__

   In file included from :0:0:
   include/linux/kconfig.h:8:0: note: this is the location of the previous 
definition
#define __BIG_ENDIAN 4321

   In file included from include/asm-generic/bitops/le.h:6:0,
from arch/h8300/include/asm/bitops.h:177,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from include/asm-generic/bug.h:18,
from arch/h8300/include/asm/bug.h:8,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/h8300/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from arch/h8300/kernel/asm-offsets.c:13:
>> arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined
#define __BIG_ENDIAN __ORDER_BIG_ENDIAN__

   In file included from :0:0:
   include/linux/kconfig.h:8:0: note: this is the location of the previous 
definition
#define __BIG_ENDIAN 4321


vim +/__BIG_ENDIAN +5 arch/h8300/include/asm/byteorder.h

d2a5f499 Yoshinori Sato 2015-05-11  4  
d2a5f499 Yoshinori Sato 2015-05-11 @5  #define __BIG_ENDIAN __ORDER_BIG_ENDIAN__
d2a5f499 Yoshinori Sato 2015-05-11  6  #include 
d2a5f499 Yoshinori Sato 2015-05-11  7  

:: The code at line 5 was first introduced by commit
:: d2a5f4999f6c211adf30d9788349e13988d6f2a7 h8300: Assembly headers

:: TO: Yoshinori Sato 
:: CC: Yoshinori Sato 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 
master
head:   cdb06e9d8f520c969676e7d6778cffe5894f079f
commit: 101110f6271ce956a049250c907bc960030577f8 Kbuild: always define 
endianess in kconfig.h
date:   2 weeks ago
config: h8300-h8300h-sim_defconfig (attached as .config)
compiler: h8300-linux-gcc (GCC) 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 101110f6271ce956a049250c907bc960030577f8
# save the attached .config to linux build tree
make.cross ARCH=h8300 

All warnings (new ones prefixed by >>):

   In file included from include/asm-generic/bitops/le.h:6:0,
from arch/h8300/include/asm/bitops.h:177,
from include/linux/bitops.h:38,
from include/linux/log2.h:16,
from include/asm-generic/getorder.h:8,
from include/asm-generic/page.h:99,
from arch/h8300/include/asm/page.h:5,
from arch/h8300/include/asm/string.h:8,
from include/linux/string.h:20,
from include/linux/uuid.h:20,
from include/linux/mod_devicetable.h:13,
from scripts/mod/devicetable-offsets.c:3:
>> arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined
#define __BIG_ENDIAN __ORDER_BIG_ENDIAN__

   In file included from :0:0:
   include/linux/kconfig.h:8:0: note: this is the location of the previous 
definition
#define __BIG_ENDIAN 4321

--
   In file included from include/asm-generic/bitops/le.h:6:0,
from arch/h8300/include/asm/bitops.h:177,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from include/asm-generic/bug.h:18,
from arch/h8300/include/asm/bug.h:8,
from include/linux/bug.h:5,
from include/linux/page-flags.h:10,
from kernel/bounds.c:10:
>> arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined
#define __BIG_ENDIAN __ORDER_BIG_ENDIAN__

   In file included from :0:0:
   include/linux/kconfig.h:8:0: note: this is the location of the previous 
definition
#define __BIG_ENDIAN 4321

   In file included from include/asm-generic/bitops/le.h:6:0,
from arch/h8300/include/asm/bitops.h:177,
from include/linux/bitops.h:38,
from include/linux/kernel.h:11,
from include/asm-generic/bug.h:18,
from arch/h8300/include/asm/bug.h:8,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/h8300/include/generated/asm/current.h:1,
from include/linux/sched.h:12,
from arch/h8300/kernel/asm-offsets.c:13:
>> arch/h8300/include/asm/byteorder.h:5:0: warning: "__BIG_ENDIAN" redefined
#define __BIG_ENDIAN __ORDER_BIG_ENDIAN__

   In file included from :0:0:
   include/linux/kconfig.h:8:0: note: this is the location of the previous 
definition
#define __BIG_ENDIAN 4321


vim +/__BIG_ENDIAN +5 arch/h8300/include/asm/byteorder.h

d2a5f499 Yoshinori Sato 2015-05-11  4  
d2a5f499 Yoshinori Sato 2015-05-11 @5  #define __BIG_ENDIAN __ORDER_BIG_ENDIAN__
d2a5f499 Yoshinori Sato 2015-05-11  6  #include 
d2a5f499 Yoshinori Sato 2015-05-11  7  

:: The code at line 5 was first introduced by commit
:: d2a5f4999f6c211adf30d9788349e13988d6f2a7 h8300: Assembly headers

:: TO: Yoshinori Sato 
:: CC: Yoshinori Sato 

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it

[Maciej S. Szmigiero]

On 10.03.2018 14:12, Borislav Petkov wrote:
> On Sat, Mar 10, 2018 at 01:26:00PM +0100, Maciej S. Szmigiero wrote:
>> Without them, it is easy to crash the driver when just playing with
>> microcode files
> 
> You're not supposed to play with the microcode files. If you do and
> something breaks, you get to keep the pieces.
> 
> If the official microcode files have a problem, then I'm all ears.
> Anything else contrived which doesn't actually happen unless someone
> manipulates them is not an issue the microcode loader should protect
> against.
> 

So, shall we remove data consistency checks of various root-only
syscalls then? :)

Maciej

Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it

[Maciej S. Szmigiero]

On 10.03.2018 14:12, Borislav Petkov wrote:
> On Sat, Mar 10, 2018 at 01:26:00PM +0100, Maciej S. Szmigiero wrote:
>> Without them, it is easy to crash the driver when just playing with
>> microcode files
> 
> You're not supposed to play with the microcode files. If you do and
> something breaks, you get to keep the pieces.
> 
> If the official microcode files have a problem, then I'm all ears.
> Anything else contrived which doesn't actually happen unless someone
> manipulates them is not an issue the microcode loader should protect
> against.
> 

So, shall we remove data consistency checks of various root-only
syscalls then? :)

Maciej

[PATCH] ARM: dts: uniphier: use proper SPDX-License-Identifier style

[Masahiro Yamada]

According to Documentation/process/license-rules.rst, move the SPDX
License Identifier to the very top of the file.  I used C++ comment
style not only for the SPDX line but for the entire block because
this seems Linus' preference [1].  I also dropped the parentheses to
follow the examples in that document.

[1] https://lkml.org/lkml/2017/11/25/133

Signed-off-by: Masahiro Yamada 
---

 arch/arm/boot/dts/uniphier-ld4-ref.dts   | 14 ++
 arch/arm/boot/dts/uniphier-ld4.dtsi  | 14 ++
 arch/arm/boot/dts/uniphier-ld6b-ref.dts  | 14 ++
 arch/arm/boot/dts/uniphier-ld6b.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-pinctrl.dtsi  | 14 ++
 arch/arm/boot/dts/uniphier-pro4-ace.dts  | 14 ++
 arch/arm/boot/dts/uniphier-pro4-ref.dts  | 14 ++
 arch/arm/boot/dts/uniphier-pro4-sanji.dts| 14 ++
 arch/arm/boot/dts/uniphier-pro4.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-pro5.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-pxs2-gentil.dts   | 14 ++
 arch/arm/boot/dts/uniphier-pxs2-vodka.dts| 14 ++
 arch/arm/boot/dts/uniphier-pxs2.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-ref-daughter.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-sld8-ref.dts  | 14 ++
 arch/arm/boot/dts/uniphier-sld8.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-support-card.dtsi | 14 ++
 17 files changed, 102 insertions(+), 136 deletions(-)

diff --git a/arch/arm/boot/dts/uniphier-ld4-ref.dts 
b/arch/arm/boot/dts/uniphier-ld4-ref.dts
index a3afd0c..21407e1 100644
--- a/arch/arm/boot/dts/uniphier-ld4-ref.dts
+++ b/arch/arm/boot/dts/uniphier-ld4-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD4 Reference Board
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD4 Reference Board
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /dts-v1/;
 #include "uniphier-ld4.dtsi"
diff --git a/arch/arm/boot/dts/uniphier-ld4.dtsi 
b/arch/arm/boot/dts/uniphier-ld4.dtsi
index 0459e84..37950ad 100644
--- a/arch/arm/boot/dts/uniphier-ld4.dtsi
+++ b/arch/arm/boot/dts/uniphier-ld4.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD4 SoC
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD4 SoC
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 #include 
 
diff --git a/arch/arm/boot/dts/uniphier-ld6b-ref.dts 
b/arch/arm/boot/dts/uniphier-ld6b-ref.dts
index 9327f6b..a0a44a4 100644
--- a/arch/arm/boot/dts/uniphier-ld6b-ref.dts
+++ b/arch/arm/boot/dts/uniphier-ld6b-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD6b Reference Board
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD6b Reference Board
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /dts-v1/;
 #include "uniphier-ld6b.dtsi"
diff --git a/arch/arm/boot/dts/uniphier-ld6b.dtsi 
b/arch/arm/boot/dts/uniphier-ld6b.dtsi
index 9a7b25c..4d07a94 100644
--- a/arch/arm/boot/dts/uniphier-ld6b.dtsi
+++ b/arch/arm/boot/dts/uniphier-ld6b.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD6b SoC
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD6b SoC
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /*
  * LD6b consists of two silicon dies: D-chip and A-chip.
diff --git a/arch/arm/boot/dts/uniphier-pinctrl.dtsi 
b/arch/arm/boot/dts/uniphier-pinctrl.dtsi
index de481c3..0468d9a 100644
--- a/arch/arm/boot/dts/uniphier-pinctrl.dtsi
+++ b/arch/arm/boot/dts/uniphier-pinctrl.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier SoCs default pinctrl settings
- *
- * Copyright (C) 2015-2017 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier SoCs default pinctrl settings
+//
+// Copyright (C) 

[PATCH] ARM: dts: uniphier: use proper SPDX-License-Identifier style

[Masahiro Yamada]

According to Documentation/process/license-rules.rst, move the SPDX
License Identifier to the very top of the file.  I used C++ comment
style not only for the SPDX line but for the entire block because
this seems Linus' preference [1].  I also dropped the parentheses to
follow the examples in that document.

[1] https://lkml.org/lkml/2017/11/25/133

Signed-off-by: Masahiro Yamada 
---

 arch/arm/boot/dts/uniphier-ld4-ref.dts   | 14 ++
 arch/arm/boot/dts/uniphier-ld4.dtsi  | 14 ++
 arch/arm/boot/dts/uniphier-ld6b-ref.dts  | 14 ++
 arch/arm/boot/dts/uniphier-ld6b.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-pinctrl.dtsi  | 14 ++
 arch/arm/boot/dts/uniphier-pro4-ace.dts  | 14 ++
 arch/arm/boot/dts/uniphier-pro4-ref.dts  | 14 ++
 arch/arm/boot/dts/uniphier-pro4-sanji.dts| 14 ++
 arch/arm/boot/dts/uniphier-pro4.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-pro5.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-pxs2-gentil.dts   | 14 ++
 arch/arm/boot/dts/uniphier-pxs2-vodka.dts| 14 ++
 arch/arm/boot/dts/uniphier-pxs2.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-ref-daughter.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-sld8-ref.dts  | 14 ++
 arch/arm/boot/dts/uniphier-sld8.dtsi | 14 ++
 arch/arm/boot/dts/uniphier-support-card.dtsi | 14 ++
 17 files changed, 102 insertions(+), 136 deletions(-)

diff --git a/arch/arm/boot/dts/uniphier-ld4-ref.dts 
b/arch/arm/boot/dts/uniphier-ld4-ref.dts
index a3afd0c..21407e1 100644
--- a/arch/arm/boot/dts/uniphier-ld4-ref.dts
+++ b/arch/arm/boot/dts/uniphier-ld4-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD4 Reference Board
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD4 Reference Board
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /dts-v1/;
 #include "uniphier-ld4.dtsi"
diff --git a/arch/arm/boot/dts/uniphier-ld4.dtsi 
b/arch/arm/boot/dts/uniphier-ld4.dtsi
index 0459e84..37950ad 100644
--- a/arch/arm/boot/dts/uniphier-ld4.dtsi
+++ b/arch/arm/boot/dts/uniphier-ld4.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD4 SoC
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD4 SoC
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 #include 
 
diff --git a/arch/arm/boot/dts/uniphier-ld6b-ref.dts 
b/arch/arm/boot/dts/uniphier-ld6b-ref.dts
index 9327f6b..a0a44a4 100644
--- a/arch/arm/boot/dts/uniphier-ld6b-ref.dts
+++ b/arch/arm/boot/dts/uniphier-ld6b-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD6b Reference Board
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD6b Reference Board
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /dts-v1/;
 #include "uniphier-ld6b.dtsi"
diff --git a/arch/arm/boot/dts/uniphier-ld6b.dtsi 
b/arch/arm/boot/dts/uniphier-ld6b.dtsi
index 9a7b25c..4d07a94 100644
--- a/arch/arm/boot/dts/uniphier-ld6b.dtsi
+++ b/arch/arm/boot/dts/uniphier-ld6b.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD6b SoC
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD6b SoC
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /*
  * LD6b consists of two silicon dies: D-chip and A-chip.
diff --git a/arch/arm/boot/dts/uniphier-pinctrl.dtsi 
b/arch/arm/boot/dts/uniphier-pinctrl.dtsi
index de481c3..0468d9a 100644
--- a/arch/arm/boot/dts/uniphier-pinctrl.dtsi
+++ b/arch/arm/boot/dts/uniphier-pinctrl.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier SoCs default pinctrl settings
- *
- * Copyright (C) 2015-2017 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier SoCs default pinctrl settings
+//
+// Copyright (C) 2015-2017 Socionext Inc.
+//   Author: Masahiro Yamada 
 
  {
pinctrl_aout: aout {
diff --git a/arch/arm/boot/dts/uniphier-pro4-ace.dts 
b/arch/arm/boot/dts/uniphier-pro4-ace.dts
index 92b5cbb..db1b089 100644
--- a/arch/arm/boot/dts/uniphier-pro4-ace.dts
+++ b/arch/arm/boot/dts/uniphier-pro4-ace.dts
@@ -1,11 

[PATCH] arm64: dts: uniphier: use proper SPDX-License-Identifier style

[Masahiro Yamada]

According to Documentation/process/license-rules.rst, move the SPDX
License Identifier to the very top of the file.  I used C++ comment
style not only for the SPDX line but for the entire block because
this seems Linus' preference [1].  I also dropped the parentheses to
follow the examples in that document.

[1] https://lkml.org/lkml/2017/11/25/133

Signed-off-by: Masahiro Yamada 
---

 arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts | 16 +++-
 arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts| 14 ++
 arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi   | 14 ++
 arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts | 16 +++-
 arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts| 14 ++
 arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi   | 14 ++
 arch/arm64/boot/dts/socionext/uniphier-pxs3-ref.dts| 14 ++
 arch/arm64/boot/dts/socionext/uniphier-pxs3.dtsi   | 14 ++
 8 files changed, 50 insertions(+), 66 deletions(-)

diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts
index 0feec41..9b4dc41 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts
@@ -1,12 +1,10 @@
-/*
- * Device Tree Source for UniPhier LD11 Global Board
- *
- * Copyright (C) 2016-2017 Socionext Inc.
- *   Author: Masahiro Yamada 
- *   Kunihiko Hayashi 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD11 Global Board
+//
+// Copyright (C) 2016-2017 Socionext Inc.
+//   Author: Masahiro Yamada 
+//   Kunihiko Hayashi 
 
 /dts-v1/;
 #include 
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts
index dacdea5..b8f6273 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD11 Reference Board
- *
- * Copyright (C) 2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD11 Reference Board
+//
+// Copyright (C) 2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /dts-v1/;
 #include "uniphier-ld11.dtsi"
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi 
b/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi
index f9014c8..5f2da73 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD11 SoC
- *
- * Copyright (C) 2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD11 SoC
+//
+// Copyright (C) 2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 #include 
 #include 
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts
index 6bee22e..fe6608e 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts
@@ -1,12 +1,10 @@
-/*
- * Device Tree Source for UniPhier LD20 Global Board
- *
- * Copyright (C) 2015-2017 Socionext Inc.
- *   Author: Masahiro Yamada 
- *   Kunihiko Hayashi 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD20 Global Board
+//
+// Copyright (C) 2015-2017 Socionext Inc.
+//   Author: Masahiro Yamada 
+//   Kunihiko Hayashi 
 
 /dts-v1/;
 #include 
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts
index 092574d..2c1a92f 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD20 Reference Board
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD20 Reference Board
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro 

[PATCH] arm64: dts: uniphier: use proper SPDX-License-Identifier style

[Masahiro Yamada]

According to Documentation/process/license-rules.rst, move the SPDX
License Identifier to the very top of the file.  I used C++ comment
style not only for the SPDX line but for the entire block because
this seems Linus' preference [1].  I also dropped the parentheses to
follow the examples in that document.

[1] https://lkml.org/lkml/2017/11/25/133

Signed-off-by: Masahiro Yamada 
---

 arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts | 16 +++-
 arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts| 14 ++
 arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi   | 14 ++
 arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts | 16 +++-
 arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts| 14 ++
 arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi   | 14 ++
 arch/arm64/boot/dts/socionext/uniphier-pxs3-ref.dts| 14 ++
 arch/arm64/boot/dts/socionext/uniphier-pxs3.dtsi   | 14 ++
 8 files changed, 50 insertions(+), 66 deletions(-)

diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts
index 0feec41..9b4dc41 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld11-global.dts
@@ -1,12 +1,10 @@
-/*
- * Device Tree Source for UniPhier LD11 Global Board
- *
- * Copyright (C) 2016-2017 Socionext Inc.
- *   Author: Masahiro Yamada 
- *   Kunihiko Hayashi 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD11 Global Board
+//
+// Copyright (C) 2016-2017 Socionext Inc.
+//   Author: Masahiro Yamada 
+//   Kunihiko Hayashi 
 
 /dts-v1/;
 #include 
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts
index dacdea5..b8f6273 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld11-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD11 Reference Board
- *
- * Copyright (C) 2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD11 Reference Board
+//
+// Copyright (C) 2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /dts-v1/;
 #include "uniphier-ld11.dtsi"
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi 
b/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi
index f9014c8..5f2da73 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld11.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD11 SoC
- *
- * Copyright (C) 2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD11 SoC
+//
+// Copyright (C) 2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 #include 
 #include 
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts
index 6bee22e..fe6608e 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld20-global.dts
@@ -1,12 +1,10 @@
-/*
- * Device Tree Source for UniPhier LD20 Global Board
- *
- * Copyright (C) 2015-2017 Socionext Inc.
- *   Author: Masahiro Yamada 
- *   Kunihiko Hayashi 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD20 Global Board
+//
+// Copyright (C) 2015-2017 Socionext Inc.
+//   Author: Masahiro Yamada 
+//   Kunihiko Hayashi 
 
 /dts-v1/;
 #include 
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts 
b/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts
index 092574d..2c1a92f 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld20-ref.dts
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD20 Reference Board
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: Masahiro Yamada 
- *
- * SPDX-License-Identifier: (GPL-2.0+ OR MIT)
- */
+// SPDX-License-Identifier: GPL-2.0+ OR MIT
+//
+// Device Tree Source for UniPhier LD20 Reference Board
+//
+// Copyright (C) 2015-2016 Socionext Inc.
+//   Author: Masahiro Yamada 
 
 /dts-v1/;
 #include "uniphier-ld20.dtsi"
diff --git a/arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi 
b/arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi
index 3d5ca4b..458b5b8 100644
--- a/arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi
+++ b/arch/arm64/boot/dts/socionext/uniphier-ld20.dtsi
@@ -1,11 +1,9 @@
-/*
- * Device Tree Source for UniPhier LD20 SoC
- *
- * Copyright (C) 2015-2016 Socionext Inc.
- *   Author: 

Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy

[Peter Zijlstra]

On Fri, Mar 09, 2018 at 06:06:29PM -0500, Waiman Long wrote:
> So you are talking about sched_relax_domain_level and

That one I wouldn't be sad to see the back of.

> sched_load_balance.

This one, that's critical. And this is the perfect time to try and fix
the whole isolcpus issue.

The primary issue is that to make equivalent functionality available
through cpuset, we need to basically start all tasks outside the root
group.

The equivalent of isolcpus=xxx is a cgroup setup like:

root
/  \
  systemother

Where other has the @xxx cpus and system the remainder and
root.sched_load_balance = 0.

Back before cgroups (and the new workqueue stuff), we could've started
everything in the !root group, no worry. But now that doesn't work,
because a bunch of controllers can't deal with that and everything
cgroup expects the cgroupfs to be empty on boot.

It's one of my biggest regrets that I didn't 'fix' this before cgroups
came along.

> I have not removed any bits. I just haven't exposed
> them yet. It does seem like these 2 control knobs are useful from the
> scheduling perspective. Do we also need cpu_exclusive or just the two
> sched control knobs are enough?

I always forget if we need exclusive for load_balance to work; I'll
peruse the document/code.

Re: [PATCH v4] cpuset: Enable cpuset controller in default hierarchy

[Peter Zijlstra]

On Fri, Mar 09, 2018 at 06:06:29PM -0500, Waiman Long wrote:
> So you are talking about sched_relax_domain_level and

That one I wouldn't be sad to see the back of.

> sched_load_balance.

This one, that's critical. And this is the perfect time to try and fix
the whole isolcpus issue.

The primary issue is that to make equivalent functionality available
through cpuset, we need to basically start all tasks outside the root
group.

The equivalent of isolcpus=xxx is a cgroup setup like:

root
/  \
  systemother

Where other has the @xxx cpus and system the remainder and
root.sched_load_balance = 0.

Back before cgroups (and the new workqueue stuff), we could've started
everything in the !root group, no worry. But now that doesn't work,
because a bunch of controllers can't deal with that and everything
cgroup expects the cgroupfs to be empty on boot.

It's one of my biggest regrets that I didn't 'fix' this before cgroups
came along.

> I have not removed any bits. I just haven't exposed
> them yet. It does seem like these 2 control knobs are useful from the
> scheduling perspective. Do we also need cpu_exclusive or just the two
> sched control knobs are enough?

I always forget if we need exclusive for load_balance to work; I'll
peruse the document/code.

Re: 4.16-rc3 fails to resume on MacBookPro10,1 -

[Pavel Machek]

Hi!

> > Ok, I've just tested linux-next, and it works ok for me on thinkpad
> > x60. (But that's probably rather different configuration from the
> > macbook).
> >
> > Unfortunately, I could not deduce anything useful from the
> > backtraces. Andrew, could you try v4.15 with KPTI disabled ?
> >
> > Thanks,
> > 
> > Pavel
> 
> This is 4.15 kernel with KPTI disabled:
> % grep PAGE_TABLE .config
> # CONFIG_PAGE_TABLE_ISOLATION is not set
> 
> As I expected it appears no more infomative to me.
> 
> What I really need is some clue as to what is supposed to be happening
> at this point.

You may want to add some printks to see where it hangs.

But maybe before that, you may want to test CPU hotplug, and make sure
s2ram works, etc...

Perhaps basic-pm-debugging.txt is good place to start?

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: 4.16-rc3 fails to resume on MacBookPro10,1 -

[Pavel Machek]

Hi!

> > Ok, I've just tested linux-next, and it works ok for me on thinkpad
> > x60. (But that's probably rather different configuration from the
> > macbook).
> >
> > Unfortunately, I could not deduce anything useful from the
> > backtraces. Andrew, could you try v4.15 with KPTI disabled ?
> >
> > Thanks,
> > 
> > Pavel
> 
> This is 4.15 kernel with KPTI disabled:
> % grep PAGE_TABLE .config
> # CONFIG_PAGE_TABLE_ISOLATION is not set
> 
> As I expected it appears no more infomative to me.
> 
> What I really need is some clue as to what is supposed to be happening
> at this point.

You may want to add some printks to see where it hangs.

But maybe before that, you may want to test CPU hotplug, and make sure
s2ram works, etc...

Perhaps basic-pm-debugging.txt is good place to start?

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: Bug: Microblaze stopped booting after 0fa1c579349fdd90173381712ad78aa99c09d38b

[Michal Simek]

On 9.3.2018 20:05, Rob Herring wrote:
> On Fri, Mar 9, 2018 at 6:51 AM, Alvaro G. M.  wrote:
>> Hi,
>>
>> I've found via git bisect that 0fa1c579349fdd90173381712ad78aa99c09d38b
>> makes microblaze unbootable.
>>
>> I'm sorry I can't provide any console output, as nothing appears at all,
>> even when setting earlyprintk (or at least I wasn't able to get anything
>> back!).
> 
> Ah, looks like microblaze doesn't set CONFIG_NO_BOOTMEM and so
> memblock_virt_alloc() doesn't work for CONFIG_HAVE_MEMBLOCK &&
> !CONFIG_NO_BOOTMEM. AFAICT, microblaze doesn't really need bootmem and
> it can be removed, but I'm still investigating. Can you try out this
> branch[1].
> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> microblaze-fixes
> 

Let me take a look at it on Monday with the rest of patches sent by you.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs




signature.asc
Description: OpenPGP digital signature

Re: Bug: Microblaze stopped booting after 0fa1c579349fdd90173381712ad78aa99c09d38b

[Michal Simek]

On 9.3.2018 20:05, Rob Herring wrote:
> On Fri, Mar 9, 2018 at 6:51 AM, Alvaro G. M.  wrote:
>> Hi,
>>
>> I've found via git bisect that 0fa1c579349fdd90173381712ad78aa99c09d38b
>> makes microblaze unbootable.
>>
>> I'm sorry I can't provide any console output, as nothing appears at all,
>> even when setting earlyprintk (or at least I wasn't able to get anything
>> back!).
> 
> Ah, looks like microblaze doesn't set CONFIG_NO_BOOTMEM and so
> memblock_virt_alloc() doesn't work for CONFIG_HAVE_MEMBLOCK &&
> !CONFIG_NO_BOOTMEM. AFAICT, microblaze doesn't really need bootmem and
> it can be removed, but I'm still investigating. Can you try out this
> branch[1].
> 
> Rob
> 
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git
> microblaze-fixes
> 

Let me take a look at it on Monday with the rest of patches sent by you.

Thanks,
Michal


-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP SoCs




signature.asc
Description: OpenPGP digital signature

Re: KASAN: use-after-free Read in sctp_association_free (2)

[Neil Horman]

On Sat, Mar 10, 2018 at 03:58:04PM +0800, Xin Long wrote:
> On Sat, Mar 10, 2018 at 6:08 AM, Neil Horman  wrote:
> > On Fri, Mar 09, 2018 at 12:59:06PM -0800, syzbot wrote:
> >> Hello,
> >>
> >> syzbot hit the following crash on net-next commit
> >> fd372a7a9e5e9d8011a0222d10edd3523abcd3b1 (Thu Mar 8 19:43:48 2018 +)
> >> Merge tag 'mlx5-updates-2018-02-28-2' of
> >> git://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux
> >>
> >> So far this crash happened 2 times on net-next.
> >> C reproducer is attached.
> >> syzkaller reproducer is attached.
> >> Raw console output is attached.
> >> compiler: gcc (GCC) 7.1.1 20170620
> >> .config is attached.
> >>
> >> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >> Reported-by: syzbot+a4e4112c3aff00c8c...@syzkaller.appspotmail.com
> >> It will help syzbot understand when the bug is fixed. See footer for
> >> details.
> >> If you forward the report, please keep this part and the footer.
> >>
> >> IPVS: ftp: loaded support on port[0] = 21
> >> IPVS: ftp: loaded support on port[0] = 21
> >> IPVS: ftp: loaded support on port[0] = 21
> >> IPVS: ftp: loaded support on port[0] = 21
> >> ==
> >> BUG: KASAN: use-after-free in sctp_association_free+0x7b7/0x930
> >> net/sctp/associola.c:332
> >> Read of size 8 at addr 8801d8006ae0 by task syzkaller914861/4202
> >>
> >> CPU: 1 PID: 4202 Comm: syzkaller914861 Not tainted 4.16.0-rc4+ #258
> >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> >> Google 01/01/2011
> >> Call Trace:
> >>  __dump_stack lib/dump_stack.c:17 [inline]
> >>  dump_stack+0x194/0x24d lib/dump_stack.c:53
> >>  print_address_description+0x73/0x250 mm/kasan/report.c:256
> >>  kasan_report_error mm/kasan/report.c:354 [inline]
> >>  kasan_report+0x23c/0x360 mm/kasan/report.c:412
> >>  __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
> >>  sctp_association_free+0x7b7/0x930 net/sctp/associola.c:332
> >>  sctp_sendmsg+0xc67/0x1a80 net/sctp/socket.c:2075
> >>  inet_sendmsg+0x11f/0x5e0 net/ipv4/af_inet.c:763
> >>  sock_sendmsg_nosec net/socket.c:629 [inline]
> >>  sock_sendmsg+0xca/0x110 net/socket.c:639
> >>  SYSC_sendto+0x361/0x5c0 net/socket.c:1748
> >>  SyS_sendto+0x40/0x50 net/socket.c:1716
> >>  do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
> >>  entry_SYSCALL_64_after_hwframe+0x42/0xb7
> >> RIP: 0033:0x446d09
> >> RSP: 002b:7f5dbac21da8 EFLAGS: 0216 ORIG_RAX: 002c
> >> RAX: ffda RBX: 006e29fc RCX: 00446d09
> >> RDX: 0001 RSI: 2340 RDI: 0003
> >> RBP: 006e29f8 R08: 204d9000 R09: 001c
> >> R10:  R11: 0216 R12: 
> >> R13: 7fff7b26fb1f R14: 7f5dbac229c0 R15: 006e2b60
> >>
> > I think we have a corner case with a0ff660058b88d12625a783ce9e5c1371c87951f
> > here.  If a peeloff event happens during a wait for sendbuf space, EPIPE 
> > will be
> > returned, and the code path appears to call sctp_association_put twice, 
> > leading
> > to the use after free situation.  I'll write a patch this weekend
> Hi, Neil, you're right.
> 
> I didn't expect peeloff can be done on a NEW asoc, as peeloff needs
> assoc_id, which can only be set when connecting has started.
> 
> But I realized that:
> f84af33 sctp: factor out sctp_sendmsg_to_asoc from sctp_sendmsg
> 
> moved sctp_primitive_ASSOCIATE(connecting) before sctp_wait_for_sndbuf
> (snd buffer waiting). It means peeloff can be done on a NEW asoc.
> So you may want to move it back.
> 
I agree with the root cause, but I'm not sure I agree with just moving the
wait_for_sndbuf call back above the call to associate.  I'm not sure I like
relying on placing a call in a spcific order solely to avoid an error condition
that might legitimately occur.  I think would rather check the return code at
the call site for the complete set of conditions for which we should not free
the association.  Something like this:

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 7d3476a4860d..a68846d2b0ef 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2071,8 +2071,9 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr 
*msg, size_t msg_len)
 
/* Send msg to the asoc */
err = sctp_sendmsg_to_asoc(asoc, msg, msg_len, transport, sinfo);
-   if (err < 0 && err != -ESRCH && new)
-   sctp_association_free(asoc);
+   if ((err != -ESRCH) && (err != -EPIPE))
+   if (err < 0 && new)
+   sctp_association_free(asoc);
 
 out_unlock:
release_sock(sk);

Which I think also avoids the noted conflict.

Thoughts?

Neil

> One good thing is the fix shouldn't touch the conflict on
> https://lkml.org/lkml/2018/3/7/1175
> We can fix it right now, I think. But pls double check it before
> submitting the patch. We just can't grow up that 

Re: KASAN: use-after-free Read in sctp_association_free (2)

[Neil Horman]

On Sat, Mar 10, 2018 at 03:58:04PM +0800, Xin Long wrote:
> On Sat, Mar 10, 2018 at 6:08 AM, Neil Horman  wrote:
> > On Fri, Mar 09, 2018 at 12:59:06PM -0800, syzbot wrote:
> >> Hello,
> >>
> >> syzbot hit the following crash on net-next commit
> >> fd372a7a9e5e9d8011a0222d10edd3523abcd3b1 (Thu Mar 8 19:43:48 2018 +)
> >> Merge tag 'mlx5-updates-2018-02-28-2' of
> >> git://git.kernel.org/pub/scm/linux/kernel/git/mellanox/linux
> >>
> >> So far this crash happened 2 times on net-next.
> >> C reproducer is attached.
> >> syzkaller reproducer is attached.
> >> Raw console output is attached.
> >> compiler: gcc (GCC) 7.1.1 20170620
> >> .config is attached.
> >>
> >> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> >> Reported-by: syzbot+a4e4112c3aff00c8c...@syzkaller.appspotmail.com
> >> It will help syzbot understand when the bug is fixed. See footer for
> >> details.
> >> If you forward the report, please keep this part and the footer.
> >>
> >> IPVS: ftp: loaded support on port[0] = 21
> >> IPVS: ftp: loaded support on port[0] = 21
> >> IPVS: ftp: loaded support on port[0] = 21
> >> IPVS: ftp: loaded support on port[0] = 21
> >> ==
> >> BUG: KASAN: use-after-free in sctp_association_free+0x7b7/0x930
> >> net/sctp/associola.c:332
> >> Read of size 8 at addr 8801d8006ae0 by task syzkaller914861/4202
> >>
> >> CPU: 1 PID: 4202 Comm: syzkaller914861 Not tainted 4.16.0-rc4+ #258
> >> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
> >> Google 01/01/2011
> >> Call Trace:
> >>  __dump_stack lib/dump_stack.c:17 [inline]
> >>  dump_stack+0x194/0x24d lib/dump_stack.c:53
> >>  print_address_description+0x73/0x250 mm/kasan/report.c:256
> >>  kasan_report_error mm/kasan/report.c:354 [inline]
> >>  kasan_report+0x23c/0x360 mm/kasan/report.c:412
> >>  __asan_report_load8_noabort+0x14/0x20 mm/kasan/report.c:433
> >>  sctp_association_free+0x7b7/0x930 net/sctp/associola.c:332
> >>  sctp_sendmsg+0xc67/0x1a80 net/sctp/socket.c:2075
> >>  inet_sendmsg+0x11f/0x5e0 net/ipv4/af_inet.c:763
> >>  sock_sendmsg_nosec net/socket.c:629 [inline]
> >>  sock_sendmsg+0xca/0x110 net/socket.c:639
> >>  SYSC_sendto+0x361/0x5c0 net/socket.c:1748
> >>  SyS_sendto+0x40/0x50 net/socket.c:1716
> >>  do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
> >>  entry_SYSCALL_64_after_hwframe+0x42/0xb7
> >> RIP: 0033:0x446d09
> >> RSP: 002b:7f5dbac21da8 EFLAGS: 0216 ORIG_RAX: 002c
> >> RAX: ffda RBX: 006e29fc RCX: 00446d09
> >> RDX: 0001 RSI: 2340 RDI: 0003
> >> RBP: 006e29f8 R08: 204d9000 R09: 001c
> >> R10:  R11: 0216 R12: 
> >> R13: 7fff7b26fb1f R14: 7f5dbac229c0 R15: 006e2b60
> >>
> > I think we have a corner case with a0ff660058b88d12625a783ce9e5c1371c87951f
> > here.  If a peeloff event happens during a wait for sendbuf space, EPIPE 
> > will be
> > returned, and the code path appears to call sctp_association_put twice, 
> > leading
> > to the use after free situation.  I'll write a patch this weekend
> Hi, Neil, you're right.
> 
> I didn't expect peeloff can be done on a NEW asoc, as peeloff needs
> assoc_id, which can only be set when connecting has started.
> 
> But I realized that:
> f84af33 sctp: factor out sctp_sendmsg_to_asoc from sctp_sendmsg
> 
> moved sctp_primitive_ASSOCIATE(connecting) before sctp_wait_for_sndbuf
> (snd buffer waiting). It means peeloff can be done on a NEW asoc.
> So you may want to move it back.
> 
I agree with the root cause, but I'm not sure I agree with just moving the
wait_for_sndbuf call back above the call to associate.  I'm not sure I like
relying on placing a call in a spcific order solely to avoid an error condition
that might legitimately occur.  I think would rather check the return code at
the call site for the complete set of conditions for which we should not free
the association.  Something like this:

diff --git a/net/sctp/socket.c b/net/sctp/socket.c
index 7d3476a4860d..a68846d2b0ef 100644
--- a/net/sctp/socket.c
+++ b/net/sctp/socket.c
@@ -2071,8 +2071,9 @@ static int sctp_sendmsg(struct sock *sk, struct msghdr 
*msg, size_t msg_len)
 
/* Send msg to the asoc */
err = sctp_sendmsg_to_asoc(asoc, msg, msg_len, transport, sinfo);
-   if (err < 0 && err != -ESRCH && new)
-   sctp_association_free(asoc);
+   if ((err != -ESRCH) && (err != -EPIPE))
+   if (err < 0 && new)
+   sctp_association_free(asoc);
 
 out_unlock:
release_sock(sk);

Which I think also avoids the noted conflict.

Thoughts?

Neil

> One good thing is the fix shouldn't touch the conflict on
> https://lkml.org/lkml/2018/3/7/1175
> We can fix it right now, I think. But pls double check it before
> submitting the patch. We just can't grow up that fixup for linus
> tree's 

Re: [PATCH] net/9p: avoid -ERESTARTSYS leak to userspace

[Greg Kurz]

Hi Andrew,

Thank you very much for taking care of this.

Please find my answers to your remarks below.

On Fri, 9 Mar 2018 14:12:52 -0800
Andrew Morton  wrote:

> On Fri, 09 Mar 2018 21:41:38 +0100 Greg Kurz  wrote:
> 
> > If it was interrupted by a signal, the 9p client may need to send some
> > more requests to the server for cleanup before returning to userspace.
> > 
> > To avoid such a last minute request to be interrupted right away, the
> > client memorizes if a signal is pending, clears TIF_SIGPENDING, handles
> > the request and calls recalc_sigpending() before returning.
> > 
> > Unfortunately, if the transmission of this cleanup request fails for any
> > reason, the transport returns an error and the client propagates it right
> > away, without calling recalc_sigpending().
> > 
> > This ends up with -ERESTARTSYS from the initially interrupted request
> > crawling up to syscall exit, with TIF_SIGPENDING cleared by the cleanup
> > request. The specific signal handling code, which is responsible for
> > converting -ERESTARTSYS to -EINTR is not called, and userspace receives
> > the confusing errno value:
> > 
> > open: Unknown error 512 (512)
> > 
> > This is really hard to hit in real life. I discovered the issue while
> > working on hot-unplug of a virtio-9p-pci device with an instrumented
> > QEMU allowing to control request completion.
> > 
> > Both p9_client_zc_rpc() and p9_client_rpc() functions have this buggy
> > error path actually. Their code flow is a bit obscure and the best
> > thing to do would probably be a full rewrite: to really ensure this
> > situation of clearing TIF_SIGPENDING and returning -ERESTARTSYS can
> > never happen.
> > 
> > But given the general lack of interest for the 9p code, I won't risk
> > breaking more things. So this patch simply fixes the buggy paths in
> > both functions with a trivial label+goto.
> > 
> > Thanks to Laurent Dufour for his help and suggestions on how to find
> > the root cause and how to fix it.  
> 
> That's a fairly straightforward-looking bug.  However the code still
> looks a bit racy:
> 
> 
> : if (signal_pending(current)) {
> : sigpending = 1;
> : clear_thread_flag(TIF_SIGPENDING);
> : } else
> : sigpending = 0;
> : 
> 
> what happens if signal_pending(current) becomes true right here?
>

Depending on the transport c->trans_mod->request() could possibly
return -ERESTARTSYS, and we would then recalc_sigpending() and
propagate -ERESTARTSYS to the caller.

> : err = c->trans_mod->request(c, req);
> 
> 
> I'm surprised that the 9p client is mucking with signals at all. 
> Signals are a userspace IPC scheme and kernel code should instead be
> using the more powerful messaging mechanisms which we've developed. 
> Ones which don't disrupt userspace state.
> 
> Why is this happening?  Is there some userspace/kernel interoperation
> involved?
> 

Before commit 9523feac272ccad2ad8186ba4fcc89103754de52, we used to rely
on wait_event_interruptible() instead of wait_event_killable(). IIUC,
the purpose of all this sigpending tweaking was just to allow subsequent
cleanup related requests to be issued, without them being interrupted right
away because of the initial signal.

BTW the issue tentatively fixed by commit 
9523feac272ccad2ad8186ba4fcc89103754de52
was more deeply investigated since then. It was caused by a bug in QEMU that 
got 
fixed, and will be available in the next release (2.12). And to cope with 
existing
buggy QEMUs, we can assume that a -EINTR response from the server necessarily 
means
the corresponding request has been successfully canceled.

Cheers,

--
Greg

Re: [PATCH] net/9p: avoid -ERESTARTSYS leak to userspace

[Greg Kurz]

Hi Andrew,

Thank you very much for taking care of this.

Please find my answers to your remarks below.

On Fri, 9 Mar 2018 14:12:52 -0800
Andrew Morton  wrote:

> On Fri, 09 Mar 2018 21:41:38 +0100 Greg Kurz  wrote:
> 
> > If it was interrupted by a signal, the 9p client may need to send some
> > more requests to the server for cleanup before returning to userspace.
> > 
> > To avoid such a last minute request to be interrupted right away, the
> > client memorizes if a signal is pending, clears TIF_SIGPENDING, handles
> > the request and calls recalc_sigpending() before returning.
> > 
> > Unfortunately, if the transmission of this cleanup request fails for any
> > reason, the transport returns an error and the client propagates it right
> > away, without calling recalc_sigpending().
> > 
> > This ends up with -ERESTARTSYS from the initially interrupted request
> > crawling up to syscall exit, with TIF_SIGPENDING cleared by the cleanup
> > request. The specific signal handling code, which is responsible for
> > converting -ERESTARTSYS to -EINTR is not called, and userspace receives
> > the confusing errno value:
> > 
> > open: Unknown error 512 (512)
> > 
> > This is really hard to hit in real life. I discovered the issue while
> > working on hot-unplug of a virtio-9p-pci device with an instrumented
> > QEMU allowing to control request completion.
> > 
> > Both p9_client_zc_rpc() and p9_client_rpc() functions have this buggy
> > error path actually. Their code flow is a bit obscure and the best
> > thing to do would probably be a full rewrite: to really ensure this
> > situation of clearing TIF_SIGPENDING and returning -ERESTARTSYS can
> > never happen.
> > 
> > But given the general lack of interest for the 9p code, I won't risk
> > breaking more things. So this patch simply fixes the buggy paths in
> > both functions with a trivial label+goto.
> > 
> > Thanks to Laurent Dufour for his help and suggestions on how to find
> > the root cause and how to fix it.  
> 
> That's a fairly straightforward-looking bug.  However the code still
> looks a bit racy:
> 
> 
> : if (signal_pending(current)) {
> : sigpending = 1;
> : clear_thread_flag(TIF_SIGPENDING);
> : } else
> : sigpending = 0;
> : 
> 
> what happens if signal_pending(current) becomes true right here?
>

Depending on the transport c->trans_mod->request() could possibly
return -ERESTARTSYS, and we would then recalc_sigpending() and
propagate -ERESTARTSYS to the caller.

> : err = c->trans_mod->request(c, req);
> 
> 
> I'm surprised that the 9p client is mucking with signals at all. 
> Signals are a userspace IPC scheme and kernel code should instead be
> using the more powerful messaging mechanisms which we've developed. 
> Ones which don't disrupt userspace state.
> 
> Why is this happening?  Is there some userspace/kernel interoperation
> involved?
> 

Before commit 9523feac272ccad2ad8186ba4fcc89103754de52, we used to rely
on wait_event_interruptible() instead of wait_event_killable(). IIUC,
the purpose of all this sigpending tweaking was just to allow subsequent
cleanup related requests to be issued, without them being interrupted right
away because of the initial signal.

BTW the issue tentatively fixed by commit 
9523feac272ccad2ad8186ba4fcc89103754de52
was more deeply investigated since then. It was caused by a bug in QEMU that 
got 
fixed, and will be available in the next release (2.12). And to cope with 
existing
buggy QEMUs, we can assume that a -EINTR response from the server necessarily 
means
the corresponding request has been successfully canceled.

Cheers,

--
Greg

Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it

[Borislav Petkov]

On Sat, Mar 10, 2018 at 01:26:00PM +0100, Maciej S. Szmigiero wrote:
> Without them, it is easy to crash the driver when just playing with
> microcode files

You're not supposed to play with the microcode files. If you do and
something breaks, you get to keep the pieces.

If the official microcode files have a problem, then I'm all ears.
Anything else contrived which doesn't actually happen unless someone
manipulates them is not an issue the microcode loader should protect
against.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it

[Borislav Petkov]

On Sat, Mar 10, 2018 at 01:26:00PM +0100, Maciej S. Szmigiero wrote:
> Without them, it is easy to crash the driver when just playing with
> microcode files

You're not supposed to play with the microcode files. If you do and
something breaks, you get to keep the pieces.

If the official microcode files have a problem, then I'm all ears.
Anything else contrived which doesn't actually happen unless someone
manipulates them is not an issue the microcode loader should protect
against.

-- 
Regards/Gruss,
Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v3 3/5] tpm: migrate tpm2_probe() to use struct tpm_buf

[Jarkko Sakkinen]

On Thu, 2018-03-08 at 13:47 -0800, J Freyensee wrote:
> Looks better :-).
> 
> 
> Acked-by: Jay Freyensee 

Thank you.

/Jarkko

Re: [PATCH v3 3/5] tpm: migrate tpm2_probe() to use struct tpm_buf

[Jarkko Sakkinen]

On Thu, 2018-03-08 at 13:47 -0800, J Freyensee wrote:
> Looks better :-).
> 
> 
> Acked-by: Jay Freyensee 

Thank you.

/Jarkko

Re: [PATCH][V2] firmware: dmi_scan: add DMI_OEM_STRING support to dmi_matches

[Jean Delvare]

On Fri, 9 Mar 2018 10:56:07 -0800, Alex Hung wrote:
> On Fri, Mar 9, 2018 at 5:33 AM, Jean Delvare  wrote:
> > However it doesn't make sense to commit this change unless there will
> > be at least one user of it. What is the status of the piece of code
> > which was supposed to use this new feature?  
> 
> The original use of DMI on _OSI is no needed anymore - the OEM _OSI
> string will always enabled; however, this patch is still needed
> because DMI_OEM_STRING are more suitable for many DMI quirks,
> especially for Dell systems, and many, if not all, DMI quirks for Dell
> systems with DMI_PRODUCT_NAME can be (and should be) replaced by
> DMI_OEM_STRING because 1) OEM string contains system id, 2) multiple
> product names can be used for the same system id and 3) the number DMI
> quirks can be reduced.
> 
> For example, the DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 9020M") in
> commit 1f59ab2783aed04f131 can be replaced by
> DMI_MATCH_EXACT(DMI_OEM_STRING, "1[0669]")
> 
> I will start sending DMI quirks with DMI_OEM_STRING myself and perhaps
> sending a clean up patch to replace DMI_PRODUCT_NAME by DMI_OEM_STRING
> for the Dell systems I have access to. With this patch in place first,
> I am able to convince others to use DMI_OEM_STRING because there will
> fewer risks to spend time in vain.

Fair enough. I've fixed the blank line issue and queued your patch for
kernel v4.17, thanks for your contribution.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH][V2] firmware: dmi_scan: add DMI_OEM_STRING support to dmi_matches

[Jean Delvare]

On Fri, 9 Mar 2018 10:56:07 -0800, Alex Hung wrote:
> On Fri, Mar 9, 2018 at 5:33 AM, Jean Delvare  wrote:
> > However it doesn't make sense to commit this change unless there will
> > be at least one user of it. What is the status of the piece of code
> > which was supposed to use this new feature?  
> 
> The original use of DMI on _OSI is no needed anymore - the OEM _OSI
> string will always enabled; however, this patch is still needed
> because DMI_OEM_STRING are more suitable for many DMI quirks,
> especially for Dell systems, and many, if not all, DMI quirks for Dell
> systems with DMI_PRODUCT_NAME can be (and should be) replaced by
> DMI_OEM_STRING because 1) OEM string contains system id, 2) multiple
> product names can be used for the same system id and 3) the number DMI
> quirks can be reduced.
> 
> For example, the DMI_MATCH(DMI_PRODUCT_NAME, "OptiPlex 9020M") in
> commit 1f59ab2783aed04f131 can be replaced by
> DMI_MATCH_EXACT(DMI_OEM_STRING, "1[0669]")
> 
> I will start sending DMI quirks with DMI_OEM_STRING myself and perhaps
> sending a clean up patch to replace DMI_PRODUCT_NAME by DMI_OEM_STRING
> for the Dell systems I have access to. With this patch in place first,
> I am able to convince others to use DMI_OEM_STRING because there will
> fewer risks to spend time in vain.

Fair enough. I've fixed the blank line issue and queued your patch for
kernel v4.17, thanks for your contribution.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 11:35 -0500, Mimi Zohar wrote:
> On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> > On Tue, 06 Mar 2018 13:36:36 -0500
> > Mimi Zohar  wrote:
> > 
> > > I've heard that some maintainers are moving away from cover letters,
> > > since they are not include in the git repo and are lost.
> > 
> > If I get a patch series with a cover letter that should be preserved, I
> > apply the series in a branch then do a no-ff merge; the cover letter can
> > then go into the merge commit.  There's no reason why cover letters need to
> > be lost.
> 
> Thanks, Jon.  That sounds like a really, good idea.
> 
> Some maintainers are saying to put the Changelog after the "---" so
> that it isn't included in the patch description.
> 
> One of the reasons for including the Changelog in the patch
> description, is to credit people with bug fixes, important rework of
> the patch, etc.

This is a really good point. By adding the cover letter to the
GIT history helps to better track people who to thank or blame :-)

/Jarkko

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 11:35 -0500, Mimi Zohar wrote:
> On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> > On Tue, 06 Mar 2018 13:36:36 -0500
> > Mimi Zohar  wrote:
> > 
> > > I've heard that some maintainers are moving away from cover letters,
> > > since they are not include in the git repo and are lost.
> > 
> > If I get a patch series with a cover letter that should be preserved, I
> > apply the series in a branch then do a no-ff merge; the cover letter can
> > then go into the merge commit.  There's no reason why cover letters need to
> > be lost.
> 
> Thanks, Jon.  That sounds like a really, good idea.
> 
> Some maintainers are saying to put the Changelog after the "---" so
> that it isn't included in the patch description.
> 
> One of the reasons for including the Changelog in the patch
> description, is to credit people with bug fixes, important rework of
> the patch, etc.

This is a really good point. By adding the cover letter to the
GIT history helps to better track people who to thank or blame :-)

/Jarkko

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> On Tue, 06 Mar 2018 13:36:36 -0500
> Mimi Zohar  wrote:
> 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.
> 
> If I get a patch series with a cover letter that should be preserved, I
> apply the series in a branch then do a no-ff merge; the cover letter can
> then go into the merge commit.  There's no reason why cover letters need to
> be lost.

That is an awesome idea. I'll start using this approach. Thank you.

/Jarkko

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Wed, 2018-03-07 at 08:54 -0700, Jonathan Corbet wrote:
> On Tue, 06 Mar 2018 13:36:36 -0500
> Mimi Zohar  wrote:
> 
> > I've heard that some maintainers are moving away from cover letters,
> > since they are not include in the git repo and are lost.
> 
> If I get a patch series with a cover letter that should be preserved, I
> apply the series in a branch then do a no-ff merge; the cover letter can
> then go into the merge commit.  There's no reason why cover letters need to
> be lost.

That is an awesome idea. I'll start using this approach. Thank you.

/Jarkko

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Tue, 2018-03-06 at 13:36 -0500, Mimi Zohar wrote:
> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.  I've seen
> Andrew Morton cut and paste the cover letter in the first patch
> description of the patch set.

When I contribute code, the cover letter helps me to do the Right
Thing.. Taking the time to write a proper cover letter helps to do a
"mental exercise" that

1. The changes make sense in the first place.
2. Only the necessary is done, not more or less.

Even for a small patch set the time used to write the cover letter
will pay off because it helps the maitainer to make a fair and
educated decision.

/Jarkko

Re: [PATCH 1/3 RESEND] tpm: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Tue, 2018-03-06 at 13:36 -0500, Mimi Zohar wrote:
> I've heard that some maintainers are moving away from cover letters,
> since they are not include in the git repo and are lost.  I've seen
> Andrew Morton cut and paste the cover letter in the first patch
> description of the patch set.

When I contribute code, the cover letter helps me to do the Right
Thing.. Taking the time to write a proper cover letter helps to do a
"mental exercise" that

1. The changes make sense in the first place.
2. Only the necessary is done, not more or less.

Even for a small patch set the time used to write the cover letter
will pay off because it helps the maitainer to make a fair and
educated decision.

/Jarkko

Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it

[Maciej S. Szmigiero]

On 10.03.2018 10:18, Borislav Petkov wrote:
> On Sat, Mar 10, 2018 at 01:34:45AM +0100, Maciej S. Szmigiero wrote:
>> Currently, it is very easy to make the AMD microcode update driver crash
>> or spin on a malformed microcode file since it does very little
>> consistency checking on data loaded from such file.
> 
> Sorry but if one has enough permissions to install malformed microcode,
> crashing the loader is the least of your problems. IOW, I don't see the
> justification for the unnecessary complication with all those checks.

While I agree this is not a security problem, I cannot agree that these
checks are unnecessary driver complication.

First, these checks are really just very basic checks like "check
whether the loaded file is long enough to actually contain some
structure before accessing it" or "don't iterate an array in file
without checking if it actually has a terminating element" or "check
whether microcode patch length isn't something like 2GB before allocating
memory for it".

Without them, it is easy to crash the driver when just playing with
microcode files (and it turns out that AMD-released microcode files in
linux-firmware actually don't contain the newest microcode versions,
even for older CPUs).

Second, since these checks happen only on a microcode file load
(something that 99.9% of systems probably will do just once at boot
time) it is hardly a performance-critical path.

Third, we still do check consistency of data provided to various
root-only syscalls (and these might be much more performance-critical
than this code).
 
> Thx.
> 

Thanks,
Maciej

Re: [PATCH] x86/microcode/AMD: check microcode file sanity before loading it

[Maciej S. Szmigiero]

On 10.03.2018 10:18, Borislav Petkov wrote:
> On Sat, Mar 10, 2018 at 01:34:45AM +0100, Maciej S. Szmigiero wrote:
>> Currently, it is very easy to make the AMD microcode update driver crash
>> or spin on a malformed microcode file since it does very little
>> consistency checking on data loaded from such file.
> 
> Sorry but if one has enough permissions to install malformed microcode,
> crashing the loader is the least of your problems. IOW, I don't see the
> justification for the unnecessary complication with all those checks.

While I agree this is not a security problem, I cannot agree that these
checks are unnecessary driver complication.

First, these checks are really just very basic checks like "check
whether the loaded file is long enough to actually contain some
structure before accessing it" or "don't iterate an array in file
without checking if it actually has a terminating element" or "check
whether microcode patch length isn't something like 2GB before allocating
memory for it".

Without them, it is easy to crash the driver when just playing with
microcode files (and it turns out that AMD-released microcode files in
linux-firmware actually don't contain the newest microcode versions,
even for older CPUs).

Second, since these checks happen only on a microcode file load
(something that 99.9% of systems probably will do just once at boot
time) it is hardly a performance-critical path.

Third, we still do check consistency of data provided to various
root-only syscalls (and these might be much more performance-critical
than this code).
 
> Thx.
> 

Thanks,
Maciej

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On Fri, 09 Mar 2018 21:36:12 +,
Christoffer Dall wrote:
> 
> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:
> > I'd be more confident if we did forbid P+A for such interrupts
> > altogether, as they really feel like another kind of HW interrupt.
> 
> How about a slightly bigger hammer:  Can we avoid doing P+A for level
> interrupts completely?  I don't think that really makes much sense, and
> I think we simply everything if we just come back out and resample the
> line.  For an edge, something like a network card, there's a potential
> performance win to appending a new pending state, but I doubt that this
> is the case for level interrupts.

I started implementing the same thing yesterday. Somehow, it feels
slightly better to have the same flow for all level interrupts,
including the timer, and we only use the MI on EOI as a way to trigger
the next state of injection. Still testing, but looking good so far.

I'm still puzzled that we have this level-but-not-quite behaviour for
VFIO interrupts. At some point, it is going to bite us badly.

M.

-- 
Jazz is not dead, it just smell funny.

Re: [RFC PATCH] KVM: arm/arm64: vgic: change condition for level interrupt resampling

[Marc Zyngier]

On Fri, 09 Mar 2018 21:36:12 +,
Christoffer Dall wrote:
> 
> On Thu, Mar 08, 2018 at 05:28:44PM +, Marc Zyngier wrote:
> > I'd be more confident if we did forbid P+A for such interrupts
> > altogether, as they really feel like another kind of HW interrupt.
> 
> How about a slightly bigger hammer:  Can we avoid doing P+A for level
> interrupts completely?  I don't think that really makes much sense, and
> I think we simply everything if we just come back out and resample the
> line.  For an edge, something like a network card, there's a potential
> performance win to appending a new pending state, but I doubt that this
> is the case for level interrupts.

I started implementing the same thing yesterday. Somehow, it feels
slightly better to have the same flow for all level interrupts,
including the timer, and we only use the MI on EOI as a way to trigger
the next state of injection. Still testing, but looking good so far.

I'm still puzzled that we have this level-but-not-quite behaviour for
VFIO interrupts. At some point, it is going to bite us badly.

M.

-- 
Jazz is not dead, it just smell funny.

Re: Nokia N900: v4.16-rc4: oops in iio when grepping sysfs

[Pavel Machek]

Hi!

On Sat 2018-03-10 12:19:29, Lars-Peter Clausen wrote:
> On 03/10/2018 12:01 AM, Pavel Machek wrote:
> [...]
> >> What file are you opening to cause this?
> > 
> > Strace says:
> > 
> > openat(7, "in_intensity_both_thresh_rising_en",
> >>> O_RDONLY|O_LARGEFILE|O_NOFOLLOW) = 3
> > fstat64(3, {st_mode=S_IFREG|0644, st_size=4096, ...}) = 0
> > ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbe83b714) = -1 ENOTTY
> >>> (Inappropriate ioctl for device)
> > read(3,
> > Message from syslogd@localhost at Mar  9 23:54:39 ...
> >  kernel:[ 3097.357696] Internal error: Oops: 8007 [#2] ARM
> > 
> > So that would be:
> > 
> > ./devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1/events/in_intensity_both_thresh_rising_en
> > 
> > And indeed, manually cat-ing that file reproduces the problem.
> > 
> > pavel@n900:/sys/devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1$
> > cat name
> > tsl2563
> > 
> > I can not find tsl2563 in MAINTAINERS, file is
> > ./drivers/iio/light/tsl2563.c . I added few people pointed by git log.
> 
> The driver registers event attributes, but does not provide a handle to
> access those attributes.
> 
> Now the question is how to best handle this case.
> 
> 1) Return an error when the device is registered and abort registration
> 2) Skip registering the event attributes
> 3) Skip registering the event attributes, but print a warning
> 4) Register the attributes, but return an error when they are accessed
> 
> I'd prefer 2 since it offers a nice method of disabling all events for a
> device (e.g. if not interrupt is provided).

2 works for me.

Tested-by: Pavel Machek 
Reported-by: Pavel Machek 

Now grep -ri asdfasdf /sys finishes.

Thanks,
Pavel

> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -477,7 +477,8 @@ int iio_device_register_eventset(struct iio_dev
>   struct attribute **attr;
> 
>   if (!(indio_dev->info->event_attrs ||
> -   iio_check_for_dynamic_events(indio_dev)))
> +   iio_check_for_dynamic_events(indio_dev)) ||
> + !indio_dev->info->read_event_config)
>   return 0;
> 
>   indio_dev->event_interface =

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: Nokia N900: v4.16-rc4: oops in iio when grepping sysfs

[Pavel Machek]

Hi!

On Sat 2018-03-10 12:19:29, Lars-Peter Clausen wrote:
> On 03/10/2018 12:01 AM, Pavel Machek wrote:
> [...]
> >> What file are you opening to cause this?
> > 
> > Strace says:
> > 
> > openat(7, "in_intensity_both_thresh_rising_en",
> >>> O_RDONLY|O_LARGEFILE|O_NOFOLLOW) = 3
> > fstat64(3, {st_mode=S_IFREG|0644, st_size=4096, ...}) = 0
> > ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbe83b714) = -1 ENOTTY
> >>> (Inappropriate ioctl for device)
> > read(3,
> > Message from syslogd@localhost at Mar  9 23:54:39 ...
> >  kernel:[ 3097.357696] Internal error: Oops: 8007 [#2] ARM
> > 
> > So that would be:
> > 
> > ./devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1/events/in_intensity_both_thresh_rising_en
> > 
> > And indeed, manually cat-ing that file reproduces the problem.
> > 
> > pavel@n900:/sys/devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1$
> > cat name
> > tsl2563
> > 
> > I can not find tsl2563 in MAINTAINERS, file is
> > ./drivers/iio/light/tsl2563.c . I added few people pointed by git log.
> 
> The driver registers event attributes, but does not provide a handle to
> access those attributes.
> 
> Now the question is how to best handle this case.
> 
> 1) Return an error when the device is registered and abort registration
> 2) Skip registering the event attributes
> 3) Skip registering the event attributes, but print a warning
> 4) Register the attributes, but return an error when they are accessed
> 
> I'd prefer 2 since it offers a nice method of disabling all events for a
> device (e.g. if not interrupt is provided).

2 works for me.

Tested-by: Pavel Machek 
Reported-by: Pavel Machek 

Now grep -ri asdfasdf /sys finishes.

Thanks,
Pavel

> --- a/drivers/iio/industrialio-event.c
> +++ b/drivers/iio/industrialio-event.c
> @@ -477,7 +477,8 @@ int iio_device_register_eventset(struct iio_dev
>   struct attribute **attr;
> 
>   if (!(indio_dev->info->event_attrs ||
> -   iio_check_for_dynamic_events(indio_dev)))
> +   iio_check_for_dynamic_events(indio_dev)) ||
> + !indio_dev->info->read_event_config)
>   return 0;
> 
>   indio_dev->event_interface =

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH] mmc: card: Don't show eMMC RPMB and BOOT areas in /proc/partitions

[Linus Walleij]

On Thu, Mar 8, 2018 at 9:36 PM, Alex Lemberg  wrote:
> On 3/2/18 4:53 AM, Linus Walleij wrote:

>> What we need to do is make the "special partitions" part of the
>> main block device and stop spawning these special block
>> devices for each boot partions or general partitions. In addition,
>> each of these boot partitions or general partitions will get their
>> own block queue and state container which is not cheap in
>> runtime memory footprint terms.
>>
>> So what I want to do (unless someone beats me to it) it to come
>> up with some way making boot and general partitions part
>> of the main block device. Possibly the core kernel partitioning
>> code needs to be augmented. The tentative idea is to just
>> put the sectors representing these partitions after the main
>> block device and access them from there, with an offset.
>
> I don't think that hiding the Boot and RPMB will resolve the problem
> described above.

Me neither. I'm just trying to discuss the problem lurking behind
these partitions.

> Boot partition (same as RPMB) in eMMC device is a separate
> "physical" partition.
> It has its own logical address range and different from general
> partition characteristics.

Yep.

> From the protocol - the access to this partition it requires switch
> partition command.

Yeah I saw that as well... it's a bit funny.

> From the device side - it can be managed in totally different manner
> (SLC vs. MLC blocks, etc.)
> I think it completely makes sense to allow access to Boot partition from the
> user space. For example - to allow R/W the boot image.

But this patch doesn't hide the partition from userspace does it?

They will still appear in /dev/mmcblk0boot1 etc.

Just not reported as "real" partitions in /proc/partitions.

Or do I misunderstand it?

> AFAIK, in case of SCSI/UFS devices - Boot LUN's are represented as
> separate block
> device partitions (/dev/sdb, dev/sdc...).
> Shouldn't we have the same for eMMC?

I think we should, but we have the problem with the boot partitions
and general partitions that they do not work anything like SCSCI
LUNs.

On a SCSI device dd from the whole device will copy all data on
the device, the partition table and contents of all partitions.

For say /dev/mmcblk0 this is not true of the device contains
boot or general partitions, those other partitions will not be
copied.

Yours,
Linus Walleij

Re: [PATCH] mmc: card: Don't show eMMC RPMB and BOOT areas in /proc/partitions

[Linus Walleij]

On Thu, Mar 8, 2018 at 9:36 PM, Alex Lemberg  wrote:
> On 3/2/18 4:53 AM, Linus Walleij wrote:

>> What we need to do is make the "special partitions" part of the
>> main block device and stop spawning these special block
>> devices for each boot partions or general partitions. In addition,
>> each of these boot partitions or general partitions will get their
>> own block queue and state container which is not cheap in
>> runtime memory footprint terms.
>>
>> So what I want to do (unless someone beats me to it) it to come
>> up with some way making boot and general partitions part
>> of the main block device. Possibly the core kernel partitioning
>> code needs to be augmented. The tentative idea is to just
>> put the sectors representing these partitions after the main
>> block device and access them from there, with an offset.
>
> I don't think that hiding the Boot and RPMB will resolve the problem
> described above.

Me neither. I'm just trying to discuss the problem lurking behind
these partitions.

> Boot partition (same as RPMB) in eMMC device is a separate
> "physical" partition.
> It has its own logical address range and different from general
> partition characteristics.

Yep.

> From the protocol - the access to this partition it requires switch
> partition command.

Yeah I saw that as well... it's a bit funny.

> From the device side - it can be managed in totally different manner
> (SLC vs. MLC blocks, etc.)
> I think it completely makes sense to allow access to Boot partition from the
> user space. For example - to allow R/W the boot image.

But this patch doesn't hide the partition from userspace does it?

They will still appear in /dev/mmcblk0boot1 etc.

Just not reported as "real" partitions in /proc/partitions.

Or do I misunderstand it?

> AFAIK, in case of SCSI/UFS devices - Boot LUN's are represented as
> separate block
> device partitions (/dev/sdb, dev/sdc...).
> Shouldn't we have the same for eMMC?

I think we should, but we have the problem with the boot partitions
and general partitions that they do not work anything like SCSCI
LUNs.

On a SCSI device dd from the whole device will copy all data on
the device, the partition table and contents of all partitions.

For say /dev/mmcblk0 this is not true of the device contains
boot or general partitions, those other partitions will not be
copied.

Yours,
Linus Walleij

Re: [PATCH 0/8] Move most GPIO documentation to driver-api/gpio/ and ReST

[Linus Walleij]

On Fri, Mar 9, 2018 at 12:40 AM, Jonathan Neuschäfer
 wrote:

> The aim of this patchset is to move the GPIO subsystem's documentation
> under Documentation/driver-api/gpio/ such that it is picked up by Sphinx
> and compiled into HTML.

Awesome!

> I moved everything except for sysfs.txt, because
> this file describes the legacy sysfs ABI, and doesn't seem to serve much
> (non-historical) purpose anymore.
>
> There are still some rough edges:
>
> * I think the API documentation (kernel-doc) should be moved out of
>   index.rst into more appropriate files.
> * The headings are arguably wrong, because driver.rst, consumer.rst,
>   etc. use the "document title" style, even though they are part of the
>   GPIO chapter. But the resulting TOC tree is consistent, and I did not
>   want to change almost all headings.
> * Some of the files could use more :c:func:`...` references and general
>   ReST polish.
>
> But I don't want to make this patchset too large.

It's fine, we take it one step at a time.

On Fri, Mar 09, 2018 at 10:41:20AM -0700, Jonathan Corbet wrote:

> Anyway, I'm happy to take these through the docs tree or see them go via
> GPIO; Linus, what's your preference?

I might have some doc patches that could collide so I can take them.

I take it there will be a v2?

Yours,
Linus Walleij

Re: [PATCH 0/8] Move most GPIO documentation to driver-api/gpio/ and ReST

[Linus Walleij]

On Fri, Mar 9, 2018 at 12:40 AM, Jonathan Neuschäfer
 wrote:

> The aim of this patchset is to move the GPIO subsystem's documentation
> under Documentation/driver-api/gpio/ such that it is picked up by Sphinx
> and compiled into HTML.

Awesome!

> I moved everything except for sysfs.txt, because
> this file describes the legacy sysfs ABI, and doesn't seem to serve much
> (non-historical) purpose anymore.
>
> There are still some rough edges:
>
> * I think the API documentation (kernel-doc) should be moved out of
>   index.rst into more appropriate files.
> * The headings are arguably wrong, because driver.rst, consumer.rst,
>   etc. use the "document title" style, even though they are part of the
>   GPIO chapter. But the resulting TOC tree is consistent, and I did not
>   want to change almost all headings.
> * Some of the files could use more :c:func:`...` references and general
>   ReST polish.
>
> But I don't want to make this patchset too large.

It's fine, we take it one step at a time.

On Fri, Mar 09, 2018 at 10:41:20AM -0700, Jonathan Corbet wrote:

> Anyway, I'm happy to take these through the docs tree or see them go via
> GPIO; Linus, what's your preference?

I might have some doc patches that could collide so I can take them.

I take it there will be a v2?

Yours,
Linus Walleij

Re: [PATCH] KVM:arm/arm64: dcache need be coherent unconditionally

[Marc Zyngier]

On Sat, 10 Mar 2018 03:23:18 +,
peng hao wrote:

> >> For emulation devices just like vga, keeping coherent dcache between
> >> guest and host timely is needed.
> >> Now the display of vnc-viewer will not update continuously and the
> >> patch can fix up.
> >> 
> >> Signed-off-by: Peng Hao 
> >> ---
> >>  virt/kvm/arm/mmu.c | 6 ++
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ec62d1c..4a28395e 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -1416,8 +1416,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, 
> >> phys_addr_t fault_ipa,
> >>  kvm_set_pfn_dirty(pfn);
> >>  }
> >>  
> >> -if (fault_status != FSC_PERM)
> >> -clean_dcache_guest_page(pfn, PMD_SIZE);
> >> +clean_dcache_guest_page(pfn, PMD_SIZE);
> >>  
> >>  if (exec_fault) {
> >>  new_pmd = kvm_s2pmd_mkexec(new_pmd);
> >> @@ -1438,8 +1437,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, 
> >> phys_addr_t fault_ipa,
> >>  mark_page_dirty(kvm, gfn);
> >>  }
> >>  
> >> -if (fault_status != FSC_PERM)
> >> -clean_dcache_guest_page(pfn, PAGE_SIZE);
> >> +clean_dcache_guest_page(pfn, PAGE_SIZE);
> >>  
> >>  if (exec_fault) {
> >>  new_pte = kvm_s2pte_mkexec(new_pte);
> >> 
> 
> >I'm sorry, but I have to NAK this.
> 
> >You're papering over the fundamental issue that you're accessing a
> >cacheable alias of a non cacheable memory. The architecture is very
> >clear about why this doesn't work, and KVM implements the architecture.
> 
> 
> 
> I find that I just encounter the problem after the commit
> '15303ba5d1cd9b28d03a980456c0978c0ea3b208 " .

This should really be a9c0e12ebee5 ("KVM: arm/arm64: Only clean the
dcache on translation fault").

> The commit contains "icache invalidation optimizations, improving VM
> startup time",it changes from unconditionally calling
> coherent_cache_guest_page(including dcache handle) to conditionally
> calling clean_dcache_guest_page.
> 
> I trace the display of vnc abnormally and find it generate data
> abort in vga address region with FSC_PERM,and it will not call
> clean_dcache_guest_page . So I think should recover to uncontionally
> calling clean_dcache_guest_page.

[I really hate ranting on a Saturday morning as the sun is finally out
and I could be relaxing in the garden, but hey, I guess that's the
fate of a kernel hacker who made the silly mistake of reading email on
a Saturday morning instead of being out in the garden...]

Even if you enabled dirty tracking on the VGA memory (and thus making
it RO to generate a permission fault), you would end-up cleaning to
the PoC *before* any write has been committed to the page. How useful
is that? You're also pretty lucky that this does a clean+invalidate
(rather than a clean which would be good enough), meaning that QEMU
will in turn miss in the cache (PIPT caches) and read some *stale*
data from memory.

Repeat that often enough, and yes, you can get some sort of
display. Is that the right thing to do? Hell no. You might just as
well have a userspace process thrashing the cache at the PoC, running
concurrently with your VM, it would have a similar effect. It may work
on your particular platform, in some specific conditions, but it just
doesn't work in general.

What you are describing is a long standing issue. VGA emulation never
worked on ARM, because it is fundamentally incompatible with the ARM
memory architecture. Read the various discussions on the list over the
past 4 years or so, read the various presentations I and others have
given about this problem and how to address it[1].

The *real* fix is a one-liner in the Linux VGA driver: map the VGA
memory as cacheable, and stop lying to the guest. Or if you want to
lie to the guest, perform cache maintenance from userspace in QEMU
based on the dirty tracking bitmap that it uses for the VGA memory.

Anything else is out of the scope of the architecture, and I'm not
going to add more crap to satisfy requirements that cannot be
implemented on real HW, let alone in a virtualized environment.

Thanks,

M.

[1] http://events17.linuxfoundation.org/sites/events/files/slides/slides_10.pdf

-- 
Jazz is not dead, it just smell funny.

Re: [PATCH] KVM:arm/arm64: dcache need be coherent unconditionally

[Marc Zyngier]

On Sat, 10 Mar 2018 03:23:18 +,
peng hao wrote:

> >> For emulation devices just like vga, keeping coherent dcache between
> >> guest and host timely is needed.
> >> Now the display of vnc-viewer will not update continuously and the
> >> patch can fix up.
> >> 
> >> Signed-off-by: Peng Hao 
> >> ---
> >>  virt/kvm/arm/mmu.c | 6 ++
> >>  1 file changed, 2 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c
> >> index ec62d1c..4a28395e 100644
> >> --- a/virt/kvm/arm/mmu.c
> >> +++ b/virt/kvm/arm/mmu.c
> >> @@ -1416,8 +1416,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, 
> >> phys_addr_t fault_ipa,
> >>  kvm_set_pfn_dirty(pfn);
> >>  }
> >>  
> >> -if (fault_status != FSC_PERM)
> >> -clean_dcache_guest_page(pfn, PMD_SIZE);
> >> +clean_dcache_guest_page(pfn, PMD_SIZE);
> >>  
> >>  if (exec_fault) {
> >>  new_pmd = kvm_s2pmd_mkexec(new_pmd);
> >> @@ -1438,8 +1437,7 @@ static int user_mem_abort(struct kvm_vcpu *vcpu, 
> >> phys_addr_t fault_ipa,
> >>  mark_page_dirty(kvm, gfn);
> >>  }
> >>  
> >> -if (fault_status != FSC_PERM)
> >> -clean_dcache_guest_page(pfn, PAGE_SIZE);
> >> +clean_dcache_guest_page(pfn, PAGE_SIZE);
> >>  
> >>  if (exec_fault) {
> >>  new_pte = kvm_s2pte_mkexec(new_pte);
> >> 
> 
> >I'm sorry, but I have to NAK this.
> 
> >You're papering over the fundamental issue that you're accessing a
> >cacheable alias of a non cacheable memory. The architecture is very
> >clear about why this doesn't work, and KVM implements the architecture.
> 
> 
> 
> I find that I just encounter the problem after the commit
> '15303ba5d1cd9b28d03a980456c0978c0ea3b208 " .

This should really be a9c0e12ebee5 ("KVM: arm/arm64: Only clean the
dcache on translation fault").

> The commit contains "icache invalidation optimizations, improving VM
> startup time",it changes from unconditionally calling
> coherent_cache_guest_page(including dcache handle) to conditionally
> calling clean_dcache_guest_page.
> 
> I trace the display of vnc abnormally and find it generate data
> abort in vga address region with FSC_PERM,and it will not call
> clean_dcache_guest_page . So I think should recover to uncontionally
> calling clean_dcache_guest_page.

[I really hate ranting on a Saturday morning as the sun is finally out
and I could be relaxing in the garden, but hey, I guess that's the
fate of a kernel hacker who made the silly mistake of reading email on
a Saturday morning instead of being out in the garden...]

Even if you enabled dirty tracking on the VGA memory (and thus making
it RO to generate a permission fault), you would end-up cleaning to
the PoC *before* any write has been committed to the page. How useful
is that? You're also pretty lucky that this does a clean+invalidate
(rather than a clean which would be good enough), meaning that QEMU
will in turn miss in the cache (PIPT caches) and read some *stale*
data from memory.

Repeat that often enough, and yes, you can get some sort of
display. Is that the right thing to do? Hell no. You might just as
well have a userspace process thrashing the cache at the PoC, running
concurrently with your VM, it would have a similar effect. It may work
on your particular platform, in some specific conditions, but it just
doesn't work in general.

What you are describing is a long standing issue. VGA emulation never
worked on ARM, because it is fundamentally incompatible with the ARM
memory architecture. Read the various discussions on the list over the
past 4 years or so, read the various presentations I and others have
given about this problem and how to address it[1].

The *real* fix is a one-liner in the Linux VGA driver: map the VGA
memory as cacheable, and stop lying to the guest. Or if you want to
lie to the guest, perform cache maintenance from userspace in QEMU
based on the dirty tracking bitmap that it uses for the VGA memory.

Anything else is out of the scope of the architecture, and I'm not
going to add more crap to satisfy requirements that cannot be
implemented on real HW, let alone in a virtualized environment.

Thanks,

M.

[1] http://events17.linuxfoundation.org/sites/events/files/slides/slides_10.pdf

-- 
Jazz is not dead, it just smell funny.

Re: Nokia N900: refcount_t underflow, use after free

[Pavel Machek]

Hi!

> >>> Well, there certainly seems to be an obvious bug wherein
> >>> isp_detach_iommu() just releases the mapping directly without calling
> >>> arm_iommu_detach_device() to balance the equivalent attach. That can't
> >>> be helping.
> >>
> >> Indeed, I have been able to reproduce the same warning using a
> >> standalone test module, and the missing arm_iommu_detach_device() is
> >> causing the warning after probe (during failure path) or during
> >> remove.
> > 
> > Ok do you have an idea how to fix the isp error paths? Untested patch
> > would be fine... But it seems that you know what needs to be fixed and
> > I don't.
> > 
> 
> OK, see if the following fixes the issue for you, only build tested.

Word-wrapped, so I applied by hand. And yes, the oops at boot is
gone. Thanks!

(Camera still does not work in -next... kills system. Oh well. Lets
debug that some other day.)

> 8< -
> >From bac9a48fb646dc51f2030d676a0dbe3298c3b134 Mon Sep 17 00:00:00 2001
> From: Suman Anna 
> Date: Fri, 9 Mar 2018 16:39:59 -0600
> Subject: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
> 
> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> ARM DMA backend. The current code creates a dma_iommu_mapping and
> attaches this to the ISP device, but never detaches the mapping in
> either the probe failure paths or the driver remove path resulting
> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> 
> Reported-by: Pavel Machek 
> Signed-off-by: Suman Anna 

Tested-by: Pavel Machek 
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 09/11] media: vsp1: Provide support for extended command pools

[Kieran Bingham]

On 09/03/18 22:04, Kieran Bingham wrote:
> VSPD and VSP-DL devices can provide extended display lists supporting
> extended command display list objects.
> 
> These extended commands require their own dma memory areas for a header
> and body specific to the command type.
> 
> Implement a command pool to allocate all necessary memory in a single
> DMA allocation to reduce pressure on the TLB, and provide convenvient

s/convenvient/convenient/

> re-usable command objects for the entities to utilise.
> Signed-off-by: Kieran Bingham 
> ---

I feel like this adds quite a bit of 'duplication' against the body pool
implementation - and there is scope for re-factoring somehow to make a lot more
of this common.

I think this is still fine to go in as is for now (as an approach that is) - but
I'd like to work out how to make this better as a later task.

Then with a reusable implementation then we can easily move the excess display
list headers (which are currently being allocated 1 for *every dlb* rather than
1 for every dl) to their own pool and allocate as appropriate.

Essentially we have the following 'object's which want to have minimal DMA
allocations (to reduce TLB pressure) - and are all sharing the same size.

 - Display list headers (72 or 96 bytes)
 - Display list bodys   (variable size - multiple per header)
if (VSPD) {
 - Extended display list header (16 bytes * number of bodies)
 - Extended display list body   (autodisp 96 bytes, autofld 160 bytes)
}

The dma_pool API's don't seem to be suitable here because as far as I can tell
it is still calling dma_alloc_coherent for each page.., rather than creating a
large pre-allocated slab and carving from it.

There certainly doesn't seem to be a way to say the number of elements to
pre-allocate... If I'm missing something obvious here - I'd love to hear it as I
don't want to re-invent any wheels!

Surely this similar pattern occurs elsewhere in the kernel ?

--
Kieran


>  drivers/media/platform/vsp1/vsp1_dl.c | 189 +++-
>  drivers/media/platform/vsp1/vsp1_dl.h |   3 +-
>  2 files changed, 192 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 36440a2a2c8b..6d17b8bfa21c 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -121,6 +121,30 @@ struct vsp1_dl_body_pool {
>  };
>  
>  /**
> + * struct vsp1_cmd_pool - display list body pool
> + * @dma: DMA address of the entries
> + * @size: size of the full DMA memory pool in bytes
> + * @mem: CPU memory pointer for the pool
> + * @bodies: Array of DLB structures for the pool
> + * @free: List of free DLB entries
> + * @lock: Protects the pool and free list
> + * @vsp1: the VSP1 device
> + */
> +struct vsp1_dl_cmd_pool {
> + /* DMA allocation */
> + dma_addr_t dma;
> + size_t size;
> + void *mem;
> +
> + struct vsp1_dl_ext_cmd *cmds;
> + struct list_head free;
> +
> + spinlock_t lock;
> +
> + struct vsp1_device *vsp1;
> +};
> +
> +/**
>   * struct vsp1_dl_list - Display list
>   * @list: entry in the display list manager lists
>   * @dlm: the display list manager
> @@ -176,6 +200,7 @@ struct vsp1_dl_manager {
>   struct vsp1_dl_list *pending;
>  
>   struct vsp1_dl_body_pool *pool;
> + struct vsp1_dl_cmd_pool *autfld_cmds;
>  };
>  
>  /* 
> -
> @@ -339,6 +364,139 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 
> reg, u32 data)
>  }
>  
>  /* 
> -
> + * Display List Extended Command Management
> + */
> +
> +enum vsp1_extcmd_type {
> + VSP1_EXTCMD_AUTODISP,
> + VSP1_EXTCMD_AUTOFLD,
> +};
> +
> +struct vsp1_extended_command_info {
> + u16 opcode;
> + size_t body_size;
> +} vsp1_extended_commands[] = {
> + [VSP1_EXTCMD_AUTODISP] = { 0x02, 96 },
> + [VSP1_EXTCMD_AUTOFLD]  = { 0x03, 160 },
> +};
> +
> +/**
> + * vsp1_dl_cmd_pool_create - Create a pool of commands from a single 
> allocation
> + * @vsp1: The VSP1 device
> + * @type: The command pool type
> + * @num_commands: The quantity of commands to allocate
> + *
> + * Allocate a pool of commands each with enough memory to contain the private
> + * data of each command. The allocation sizes are dependent upon the command
> + * type.
> + *
> + * Return a pointer to a pool on success or NULL if memory can't be 
> allocated.
> + */
> +struct vsp1_dl_cmd_pool *
> +vsp1_dl_cmd_pool_create(struct vsp1_device *vsp1, enum vsp1_extcmd_type type,
> + unsigned int num_cmds)
> +{
> + struct vsp1_dl_cmd_pool *pool;
> + unsigned int i;
> + size_t cmd_size;
> +
> + pool = kzalloc(sizeof(*pool), GFP_KERNEL);
> + if (!pool)
> + return NULL;
> +
> + pool->cmds = kcalloc(num_cmds, sizeof(*pool->cmds), GFP_KERNEL);
> +   

Re: Nokia N900: refcount_t underflow, use after free

[Pavel Machek]

Hi!

> >>> Well, there certainly seems to be an obvious bug wherein
> >>> isp_detach_iommu() just releases the mapping directly without calling
> >>> arm_iommu_detach_device() to balance the equivalent attach. That can't
> >>> be helping.
> >>
> >> Indeed, I have been able to reproduce the same warning using a
> >> standalone test module, and the missing arm_iommu_detach_device() is
> >> causing the warning after probe (during failure path) or during
> >> remove.
> > 
> > Ok do you have an idea how to fix the isp error paths? Untested patch
> > would be fine... But it seems that you know what needs to be fixed and
> > I don't.
> > 
> 
> OK, see if the following fixes the issue for you, only build tested.

Word-wrapped, so I applied by hand. And yes, the oops at boot is
gone. Thanks!

(Camera still does not work in -next... kills system. Oh well. Lets
debug that some other day.)

> 8< -
> >From bac9a48fb646dc51f2030d676a0dbe3298c3b134 Mon Sep 17 00:00:00 2001
> From: Suman Anna 
> Date: Fri, 9 Mar 2018 16:39:59 -0600
> Subject: [PATCH] media: omap3isp: fix unbalanced dma_iommu_mapping
> 
> The OMAP3 ISP driver manages its MMU mappings through the IOMMU-aware
> ARM DMA backend. The current code creates a dma_iommu_mapping and
> attaches this to the ISP device, but never detaches the mapping in
> either the probe failure paths or the driver remove path resulting
> in an unbalanced mapping refcount and a memory leak. Fix this properly.
> 
> Reported-by: Pavel Machek 
> Signed-off-by: Suman Anna 

Tested-by: Pavel Machek 
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 09/11] media: vsp1: Provide support for extended command pools

[Kieran Bingham]

On 09/03/18 22:04, Kieran Bingham wrote:
> VSPD and VSP-DL devices can provide extended display lists supporting
> extended command display list objects.
> 
> These extended commands require their own dma memory areas for a header
> and body specific to the command type.
> 
> Implement a command pool to allocate all necessary memory in a single
> DMA allocation to reduce pressure on the TLB, and provide convenvient

s/convenvient/convenient/

> re-usable command objects for the entities to utilise.
> Signed-off-by: Kieran Bingham 
> ---

I feel like this adds quite a bit of 'duplication' against the body pool
implementation - and there is scope for re-factoring somehow to make a lot more
of this common.

I think this is still fine to go in as is for now (as an approach that is) - but
I'd like to work out how to make this better as a later task.

Then with a reusable implementation then we can easily move the excess display
list headers (which are currently being allocated 1 for *every dlb* rather than
1 for every dl) to their own pool and allocate as appropriate.

Essentially we have the following 'object's which want to have minimal DMA
allocations (to reduce TLB pressure) - and are all sharing the same size.

 - Display list headers (72 or 96 bytes)
 - Display list bodys   (variable size - multiple per header)
if (VSPD) {
 - Extended display list header (16 bytes * number of bodies)
 - Extended display list body   (autodisp 96 bytes, autofld 160 bytes)
}

The dma_pool API's don't seem to be suitable here because as far as I can tell
it is still calling dma_alloc_coherent for each page.., rather than creating a
large pre-allocated slab and carving from it.

There certainly doesn't seem to be a way to say the number of elements to
pre-allocate... If I'm missing something obvious here - I'd love to hear it as I
don't want to re-invent any wheels!

Surely this similar pattern occurs elsewhere in the kernel ?

--
Kieran


>  drivers/media/platform/vsp1/vsp1_dl.c | 189 +++-
>  drivers/media/platform/vsp1/vsp1_dl.h |   3 +-
>  2 files changed, 192 insertions(+)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c 
> b/drivers/media/platform/vsp1/vsp1_dl.c
> index 36440a2a2c8b..6d17b8bfa21c 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -121,6 +121,30 @@ struct vsp1_dl_body_pool {
>  };
>  
>  /**
> + * struct vsp1_cmd_pool - display list body pool
> + * @dma: DMA address of the entries
> + * @size: size of the full DMA memory pool in bytes
> + * @mem: CPU memory pointer for the pool
> + * @bodies: Array of DLB structures for the pool
> + * @free: List of free DLB entries
> + * @lock: Protects the pool and free list
> + * @vsp1: the VSP1 device
> + */
> +struct vsp1_dl_cmd_pool {
> + /* DMA allocation */
> + dma_addr_t dma;
> + size_t size;
> + void *mem;
> +
> + struct vsp1_dl_ext_cmd *cmds;
> + struct list_head free;
> +
> + spinlock_t lock;
> +
> + struct vsp1_device *vsp1;
> +};
> +
> +/**
>   * struct vsp1_dl_list - Display list
>   * @list: entry in the display list manager lists
>   * @dlm: the display list manager
> @@ -176,6 +200,7 @@ struct vsp1_dl_manager {
>   struct vsp1_dl_list *pending;
>  
>   struct vsp1_dl_body_pool *pool;
> + struct vsp1_dl_cmd_pool *autfld_cmds;
>  };
>  
>  /* 
> -
> @@ -339,6 +364,139 @@ void vsp1_dl_body_write(struct vsp1_dl_body *dlb, u32 
> reg, u32 data)
>  }
>  
>  /* 
> -
> + * Display List Extended Command Management
> + */
> +
> +enum vsp1_extcmd_type {
> + VSP1_EXTCMD_AUTODISP,
> + VSP1_EXTCMD_AUTOFLD,
> +};
> +
> +struct vsp1_extended_command_info {
> + u16 opcode;
> + size_t body_size;
> +} vsp1_extended_commands[] = {
> + [VSP1_EXTCMD_AUTODISP] = { 0x02, 96 },
> + [VSP1_EXTCMD_AUTOFLD]  = { 0x03, 160 },
> +};
> +
> +/**
> + * vsp1_dl_cmd_pool_create - Create a pool of commands from a single 
> allocation
> + * @vsp1: The VSP1 device
> + * @type: The command pool type
> + * @num_commands: The quantity of commands to allocate
> + *
> + * Allocate a pool of commands each with enough memory to contain the private
> + * data of each command. The allocation sizes are dependent upon the command
> + * type.
> + *
> + * Return a pointer to a pool on success or NULL if memory can't be 
> allocated.
> + */
> +struct vsp1_dl_cmd_pool *
> +vsp1_dl_cmd_pool_create(struct vsp1_device *vsp1, enum vsp1_extcmd_type type,
> + unsigned int num_cmds)
> +{
> + struct vsp1_dl_cmd_pool *pool;
> + unsigned int i;
> + size_t cmd_size;
> +
> + pool = kzalloc(sizeof(*pool), GFP_KERNEL);
> + if (!pool)
> + return NULL;
> +
> + pool->cmds = kcalloc(num_cmds, sizeof(*pool->cmds), GFP_KERNEL);
> + if (!pool->cmds) {
> + 

Re: [PATCH 3/5 V2] tpm2: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Tue, 2018-03-06 at 15:19 +, Winkler, Tomas wrote:
> > On Tue, 2018-03-06 at 11:25 +0200, Tomas Winkler wrote:
> > > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve
> > > generation of crypto keys which can be a computationally intensive task.
> > > The timeout is set to 3min.
> > > Rather than increasing default timeout a new constant is added, to not
> > > stall for too long on regular commands failures.
> > > 
> > > Signed-off-by: Tomas Winkler 
> > 
> > Why are you radically chaging the default timeout? The commit message
> > does not tell anything about that change.
> > 
> 
> Let me, recheck but it should be same value just converted to msecs.
>  
> > Why couldn't we just have two timeouts: one default and one long that
> > would be at least as long as the longest timeout defined in the spec?
> 
> I've tried to explain it in the commit message but apparently has failed. 
> 
> We have a default or undefined which should be the same as it was unless I did
> some silly mistake in conversion to msecs (will check), ass all others are in
> msecs.
> It was 2 min = 2 * 60 * HZ (in jiffies) which would be msecs_to_jiffies(2 * 60
> * 1000 = 12) 
> TPM2_DURATION_DEFAULT   = 12

Aah, of course :-) The problem was that I had somehow a blid spot with
seeing the msec_to_jiffies() conversion.

TPM_NUM_DURATIONS would a better name than TPM_DURATION_MAX because
TPM_DURATION_MAX is easy to confuse with TPM_DURATION_* constants.

/Jarkko

Re: [PATCH 3/5 V2] tpm2: add longer timeouts for creation commands.

[Jarkko Sakkinen]

On Tue, 2018-03-06 at 15:19 +, Winkler, Tomas wrote:
> > On Tue, 2018-03-06 at 11:25 +0200, Tomas Winkler wrote:
> > > TPM2_CC_Create(0x153) and TPM2_CC_CreatePrimary (0x131) involve
> > > generation of crypto keys which can be a computationally intensive task.
> > > The timeout is set to 3min.
> > > Rather than increasing default timeout a new constant is added, to not
> > > stall for too long on regular commands failures.
> > > 
> > > Signed-off-by: Tomas Winkler 
> > 
> > Why are you radically chaging the default timeout? The commit message
> > does not tell anything about that change.
> > 
> 
> Let me, recheck but it should be same value just converted to msecs.
>  
> > Why couldn't we just have two timeouts: one default and one long that
> > would be at least as long as the longest timeout defined in the spec?
> 
> I've tried to explain it in the commit message but apparently has failed. 
> 
> We have a default or undefined which should be the same as it was unless I did
> some silly mistake in conversion to msecs (will check), ass all others are in
> msecs.
> It was 2 min = 2 * 60 * HZ (in jiffies) which would be msecs_to_jiffies(2 * 60
> * 1000 = 12) 
> TPM2_DURATION_DEFAULT   = 12

Aah, of course :-) The problem was that I had somehow a blid spot with
seeing the msec_to_jiffies() conversion.

TPM_NUM_DURATIONS would a better name than TPM_DURATION_MAX because
TPM_DURATION_MAX is easy to confuse with TPM_DURATION_* constants.

/Jarkko

Re: Nokia N900: v4.16-rc4: oops in iio when grepping sysfs

[Akinobu Mita]

2018-03-10 8:01 GMT+09:00 Pavel Machek :
> Hi!
>
>> > Hmm. Looks like there's a lot of fun to be had with sysfs.
>> >
>> >
>> > pavel@n900:~$ uname -a
>> > Linux n900 4.16.0-rc4-59690-g7f84626-dirty #543 Thu Mar 8 19:53:30 CET
>> > 2018 armv7l GNU/Linux
>> >
>> > [  306.402496] bq2415x: command Timer reset
>> > [  312.761322] adp1653 2-0030: Read Addr:03 Val:00 ok
>> > [  313.264129] Unable to handle kernel NULL pointer dereference at
>> > virtual address 0
>> > 000
>> > [  313.272308] pgd = 01336620
>> > [  313.275146] [] *pgd=800af831, *pte=, *ppte=
>> > [  313.281463] Internal error: Oops: 8007 [#1] ARM
>> > [  313.286376] Modules linked in:
>> > [  313.289459] CPU: 0 PID: 3584 Comm: grep Tainted: GW
>> > 4.16.0-rc4-59690-g
>> > 7f84626-dirty #543
>> > [  313.298919] Hardware name: Nokia RX-51 board
>> > [  313.303222] PC is at   (null)
>> > [  313.306213] LR is at iio_ev_state_show+0x38/0x54
>> > [  313.310852] pc : [<>]lr : []psr: a013
>> > [  313.317169] sp : c7b47e70  ip : c087bb24  fp : 0001
>> > [  313.322418] r10: cb19e000  r9 : c0857220  r8 : 1000
>> > [  313.327667] r7 : 0fff  r6 : cb711c80  r5 : cb19e000  r4 :
>> > 
>> > [  313.334228] r3 : 0001  r2 :   r1 : c087b4dc  r0 :
>> > ce584800
>> > [  313.340789] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
>> > Segment none
>> > [  313.347991] Control: 10c5387d  Table: 87bec019  DAC: 0051
>> > [  313.353759] Process grep (pid: 3584, stack limit = 0xc4e45eab)
>> > [  313.359619] Stack: (0xc7b47e70 to 0xc7b48000)
>> > [  313.364013] 7e60: c05d6988
>> > cb711b00 ce585c00 c0450d68
>> > [  313.471038] [] (iio_ev_state_show) from []
>> > (dev_attr_show+0x1c/0x4c)
>> > [  313.479187] [] (dev_attr_show) from []
>> > (sysfs_kf_seq_show+0x90/0x108)
>> > [  313.487426] [] (sysfs_kf_seq_show) from []
>> > (kernfs_seq_show+0x24/0x28)
>> > [  313.495758] [] (kernfs_seq_show) from []
>> > (seq_read+0x1dc/0x500)
>> > [  313.503479] [] (seq_read) from []
>> > (__vfs_read+0x2c/0x120)
>> > [  313.510681] [] (__vfs_read) from []
>> > (vfs_read+0x88/0x114)
>> > [  313.517852] [] (vfs_read) from []
>> > (SyS_read+0x40/0x8c)
>> > [  313.524780] [] (SyS_read) from []
>> > (ret_fast_syscall+0x0/0x54)
>> > [  313.532318] Exception stack(0xc7b47fa8 to 0xc7b47ff0)
>> > [  313.537414] 7fa0:   00035330 00042000 0003
>> > 00042000 8000 8000
>> >
>>
>> What file are you opening to cause this?
>
> Strace says:
>
> openat(7, "in_intensity_both_thresh_rising_en",
>> > O_RDONLY|O_LARGEFILE|O_NOFOLLOW) = 3
> fstat64(3, {st_mode=S_IFREG|0644, st_size=4096, ...}) = 0
> ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbe83b714) = -1 ENOTTY
>> > (Inappropriate ioctl for device)
> read(3,
> Message from syslogd@localhost at Mar  9 23:54:39 ...
>  kernel:[ 3097.357696] Internal error: Oops: 8007 [#2] ARM
>
> So that would be:
>
> ./devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1/events/in_intensity_both_thresh_rising_en
>
> And indeed, manually cat-ing that file reproduces the problem.

This problem happens when no irq is defined for this device.

In this case, tsl2563_info_no_irq whose read_event_config field is NULL
is selected as iio_info.  On the other hand, iio_chan_spec for this
driver always registers event_spec.

So sysfs files related to the channel events are always created even if
no irq is defined.

I think we can fix this issue by defining another iio_chan_spec with
no event_spec for no irq case.

Jonathan, do you have any other idea how to fix this issue?

Re: Nokia N900: v4.16-rc4: oops in iio when grepping sysfs

[Akinobu Mita]

2018-03-10 8:01 GMT+09:00 Pavel Machek :
> Hi!
>
>> > Hmm. Looks like there's a lot of fun to be had with sysfs.
>> >
>> >
>> > pavel@n900:~$ uname -a
>> > Linux n900 4.16.0-rc4-59690-g7f84626-dirty #543 Thu Mar 8 19:53:30 CET
>> > 2018 armv7l GNU/Linux
>> >
>> > [  306.402496] bq2415x: command Timer reset
>> > [  312.761322] adp1653 2-0030: Read Addr:03 Val:00 ok
>> > [  313.264129] Unable to handle kernel NULL pointer dereference at
>> > virtual address 0
>> > 000
>> > [  313.272308] pgd = 01336620
>> > [  313.275146] [] *pgd=800af831, *pte=, *ppte=
>> > [  313.281463] Internal error: Oops: 8007 [#1] ARM
>> > [  313.286376] Modules linked in:
>> > [  313.289459] CPU: 0 PID: 3584 Comm: grep Tainted: GW
>> > 4.16.0-rc4-59690-g
>> > 7f84626-dirty #543
>> > [  313.298919] Hardware name: Nokia RX-51 board
>> > [  313.303222] PC is at   (null)
>> > [  313.306213] LR is at iio_ev_state_show+0x38/0x54
>> > [  313.310852] pc : [<>]lr : []psr: a013
>> > [  313.317169] sp : c7b47e70  ip : c087bb24  fp : 0001
>> > [  313.322418] r10: cb19e000  r9 : c0857220  r8 : 1000
>> > [  313.327667] r7 : 0fff  r6 : cb711c80  r5 : cb19e000  r4 :
>> > 
>> > [  313.334228] r3 : 0001  r2 :   r1 : c087b4dc  r0 :
>> > ce584800
>> > [  313.340789] Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM
>> > Segment none
>> > [  313.347991] Control: 10c5387d  Table: 87bec019  DAC: 0051
>> > [  313.353759] Process grep (pid: 3584, stack limit = 0xc4e45eab)
>> > [  313.359619] Stack: (0xc7b47e70 to 0xc7b48000)
>> > [  313.364013] 7e60: c05d6988
>> > cb711b00 ce585c00 c0450d68
>> > [  313.471038] [] (iio_ev_state_show) from []
>> > (dev_attr_show+0x1c/0x4c)
>> > [  313.479187] [] (dev_attr_show) from []
>> > (sysfs_kf_seq_show+0x90/0x108)
>> > [  313.487426] [] (sysfs_kf_seq_show) from []
>> > (kernfs_seq_show+0x24/0x28)
>> > [  313.495758] [] (kernfs_seq_show) from []
>> > (seq_read+0x1dc/0x500)
>> > [  313.503479] [] (seq_read) from []
>> > (__vfs_read+0x2c/0x120)
>> > [  313.510681] [] (__vfs_read) from []
>> > (vfs_read+0x88/0x114)
>> > [  313.517852] [] (vfs_read) from []
>> > (SyS_read+0x40/0x8c)
>> > [  313.524780] [] (SyS_read) from []
>> > (ret_fast_syscall+0x0/0x54)
>> > [  313.532318] Exception stack(0xc7b47fa8 to 0xc7b47ff0)
>> > [  313.537414] 7fa0:   00035330 00042000 0003
>> > 00042000 8000 8000
>> >
>>
>> What file are you opening to cause this?
>
> Strace says:
>
> openat(7, "in_intensity_both_thresh_rising_en",
>> > O_RDONLY|O_LARGEFILE|O_NOFOLLOW) = 3
> fstat64(3, {st_mode=S_IFREG|0644, st_size=4096, ...}) = 0
> ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbe83b714) = -1 ENOTTY
>> > (Inappropriate ioctl for device)
> read(3,
> Message from syslogd@localhost at Mar  9 23:54:39 ...
>  kernel:[ 3097.357696] Internal error: Oops: 8007 [#2] ARM
>
> So that would be:
>
> ./devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1/events/in_intensity_both_thresh_rising_en
>
> And indeed, manually cat-ing that file reproduces the problem.

This problem happens when no irq is defined for this device.

In this case, tsl2563_info_no_irq whose read_event_config field is NULL
is selected as iio_info.  On the other hand, iio_chan_spec for this
driver always registers event_spec.

So sysfs files related to the channel events are always created even if
no irq is defined.

I think we can fix this issue by defining another iio_chan_spec with
no event_spec for no irq case.

Jonathan, do you have any other idea how to fix this issue?

Re: Nokia N900: v4.16-rc4: oops in iio when grepping sysfs

[Lars-Peter Clausen]

On 03/10/2018 12:01 AM, Pavel Machek wrote:
[...]
>> What file are you opening to cause this?
> 
> Strace says:
> 
> openat(7, "in_intensity_both_thresh_rising_en",
>>> O_RDONLY|O_LARGEFILE|O_NOFOLLOW) = 3
> fstat64(3, {st_mode=S_IFREG|0644, st_size=4096, ...}) = 0
> ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbe83b714) = -1 ENOTTY
>>> (Inappropriate ioctl for device)
> read(3,
> Message from syslogd@localhost at Mar  9 23:54:39 ...
>  kernel:[ 3097.357696] Internal error: Oops: 8007 [#2] ARM
> 
> So that would be:
> 
> ./devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1/events/in_intensity_both_thresh_rising_en
> 
> And indeed, manually cat-ing that file reproduces the problem.
> 
> pavel@n900:/sys/devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1$
> cat name
> tsl2563
> 
> I can not find tsl2563 in MAINTAINERS, file is
> ./drivers/iio/light/tsl2563.c . I added few people pointed by git log.

The driver registers event attributes, but does not provide a handle to
access those attributes.

Now the question is how to best handle this case.

1) Return an error when the device is registered and abort registration
2) Skip registering the event attributes
3) Skip registering the event attributes, but print a warning
4) Register the attributes, but return an error when they are accessed

I'd prefer 2 since it offers a nice method of disabling all events for a
device (e.g. if not interrupt is provided).

On the other hand it could cause some confusion during development,
potentially causing the developer to wonder why he doesn't get any event
attributes even though he setup his channel description to have event
attributes.

Patch for 2.

--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -477,7 +477,8 @@ int iio_device_register_eventset(struct iio_dev
struct attribute **attr;

if (!(indio_dev->info->event_attrs ||
- iio_check_for_dynamic_events(indio_dev)))
+ iio_check_for_dynamic_events(indio_dev)) ||
+   !indio_dev->info->read_event_config)
return 0;

indio_dev->event_interface =

Re: Nokia N900: v4.16-rc4: oops in iio when grepping sysfs

[Lars-Peter Clausen]

On 03/10/2018 12:01 AM, Pavel Machek wrote:
[...]
>> What file are you opening to cause this?
> 
> Strace says:
> 
> openat(7, "in_intensity_both_thresh_rising_en",
>>> O_RDONLY|O_LARGEFILE|O_NOFOLLOW) = 3
> fstat64(3, {st_mode=S_IFREG|0644, st_size=4096, ...}) = 0
> ioctl(3, SNDCTL_TMR_TIMEBASE or TCGETS, 0xbe83b714) = -1 ENOTTY
>>> (Inappropriate ioctl for device)
> read(3,
> Message from syslogd@localhost at Mar  9 23:54:39 ...
>  kernel:[ 3097.357696] Internal error: Oops: 8007 [#2] ARM
> 
> So that would be:
> 
> ./devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1/events/in_intensity_both_thresh_rising_en
> 
> And indeed, manually cat-ing that file reproduces the problem.
> 
> pavel@n900:/sys/devices/platform/6800.ocp/48072000.i2c/i2c-2/2-0029/iio:device1$
> cat name
> tsl2563
> 
> I can not find tsl2563 in MAINTAINERS, file is
> ./drivers/iio/light/tsl2563.c . I added few people pointed by git log.

The driver registers event attributes, but does not provide a handle to
access those attributes.

Now the question is how to best handle this case.

1) Return an error when the device is registered and abort registration
2) Skip registering the event attributes
3) Skip registering the event attributes, but print a warning
4) Register the attributes, but return an error when they are accessed

I'd prefer 2 since it offers a nice method of disabling all events for a
device (e.g. if not interrupt is provided).

On the other hand it could cause some confusion during development,
potentially causing the developer to wonder why he doesn't get any event
attributes even though he setup his channel description to have event
attributes.

Patch for 2.

--- a/drivers/iio/industrialio-event.c
+++ b/drivers/iio/industrialio-event.c
@@ -477,7 +477,8 @@ int iio_device_register_eventset(struct iio_dev
struct attribute **attr;

if (!(indio_dev->info->event_attrs ||
- iio_check_for_dynamic_events(indio_dev)))
+ iio_check_for_dynamic_events(indio_dev)) ||
+   !indio_dev->info->read_event_config)
return 0;

indio_dev->event_interface =

Re: [PATCH 0/8] Move most GPIO documentation to driver-api/gpio/ and ReST

[Jonathan Neuschäfer]

On Fri, Mar 09, 2018 at 10:41:20AM -0700, Jonathan Corbet wrote:
> On Fri,  9 Mar 2018 00:40:16 +0100
> Jonathan Neuschäfer  wrote:
> 
> > The aim of this patchset is to move the GPIO subsystem's documentation
> > under Documentation/driver-api/gpio/ such that it is picked up by Sphinx
> > and compiled into HTML. I moved everything except for sysfs.txt, because
> > this file describes the legacy sysfs ABI, and doesn't seem to serve much
> > (non-historical) purpose anymore.
> > 
> > There are still some rough edges:
> > 
> > * I think the API documentation (kernel-doc) should be moved out of
> >   index.rst into more appropriate files.
> > * The headings are arguably wrong, because driver.rst, consumer.rst,
> >   etc. use the "document title" style, even though they are part of the
> >   GPIO chapter. But the resulting TOC tree is consistent, and I did not
> >   want to change almost all headings.
> > * Some of the files could use more :c:func:`...` references and general
> >   ReST polish.
> > 
> > But I don't want to make this patchset too large.
> 
> [For future reference, if you're going to CC me on most of a patch series,
> I'd really rather get the whole thing so I don't have to go looking for
> it.]

Noted.

> From a quick look, it seems like a reasonable RST conversion to me.  I do
> wonder if sysfs.txt should just be removed, since it documents something
> we don't want people to use at this point?  Historical purposes are well
> served by the repository history.
> 
> I'd say changing the headings is worthwhile if it produces a better
> result.

I just tried this on one file (driver.rst) and it made no difference in
the output (neither in the TOC in index.html nor in driver.html).

Thanks,
Jonathan Neuschäfer


> OTOH I'd be wary of adding too much "polish"; we really want to
> retain the readability of the plain-text files.
> 
> Anyway, I'm happy to take these through the docs tree or see them go via
> GPIO; Linus, what's your preference?
> 
> Thanks,
> 
> jon




diff --git a/Documentation/driver-api/gpio/driver.rst 
b/Documentation/driver-api/gpio/driver.rst
index 505ee906d7d9..8eb08005be55 100644
--- a/Documentation/driver-api/gpio/driver.rst
+++ b/Documentation/driver-api/gpio/driver.rst
@@ -1,4 +1,3 @@
-
 GPIO Descriptor Driver Interface
 
 
@@ -13,7 +12,7 @@ the structures used to define a GPIO driver:
 
 
 Internal Representation of GPIOs
-
+
 
 Inside a GPIO driver, individual GPIOs are identified by their hardware number,
 which is a unique number between 0 and n, n being the number of GPIOs managed 
by
@@ -36,7 +35,7 @@ identify GPIOs in a bank of I2C GPIO expanders.
 
 
 Controller Drivers: gpio_chip
-=
+-
 
 In the gpiolib framework each GPIO controller is packaged as a "struct
 gpio_chip" (see linux/gpio/driver.h for its complete definition) with members
@@ -74,7 +73,7 @@ not be required.
 
 
 GPIO electrical configuration
--
+~
 
 GPIOs can be configured for several electrical modes of operation by using the
 .set_config() callback. Currently this API supports setting debouncing and
@@ -95,7 +94,7 @@ numbers on the pin controller so they can properly 
cross-reference each other.
 
 
 GPIOs with debounce support

+~~~
 
 Debouncing is a configuration set to a pin indicating that it is connected to
 a mechanical switch or button, or similar that may bounce. Bouncing means the
@@ -112,7 +111,7 @@ is not configurable.
 
 
 GPIOs with open drain/source support
-
+
 
 Open drain (CMOS) or open collector (TTL) means the line is not actively driven
 high: instead you provide the drain/collector as output, so when the transistor
@@ -209,7 +208,7 @@ of actively driving the line low, it is set to input.
 
 
 GPIO drivers providing IRQs

+~~~
 It is custom that GPIO drivers (GPIO chips) are also providing interrupts,
 most often cascaded off a parent interrupt controller, and in some special
 cases the GPIO logic is melded with a SoC's primary interrupt controller.
@@ -359,7 +358,7 @@ below exists.
 
 
 Locking IRQ usage
--
+~
 Input GPIOs can be used as IRQ signals. When this happens, a driver is 
requested
 to mark the GPIO as being used as an IRQ::
 
@@ -378,7 +377,7 @@ When using the gpiolib irqchip helpers, these callback are 
automatically
 assigned.
 
 Real-Time compliance for GPIO IRQ chips

+~~~
 
 Any provider of irqchips needs to be carefully tailored to support Real Time
 preemption. It is desirable that all irqchips in the GPIO subsystem 

Re: [PATCH 0/8] Move most GPIO documentation to driver-api/gpio/ and ReST

[Jonathan Neuschäfer]

On Fri, Mar 09, 2018 at 10:41:20AM -0700, Jonathan Corbet wrote:
> On Fri,  9 Mar 2018 00:40:16 +0100
> Jonathan Neuschäfer  wrote:
> 
> > The aim of this patchset is to move the GPIO subsystem's documentation
> > under Documentation/driver-api/gpio/ such that it is picked up by Sphinx
> > and compiled into HTML. I moved everything except for sysfs.txt, because
> > this file describes the legacy sysfs ABI, and doesn't seem to serve much
> > (non-historical) purpose anymore.
> > 
> > There are still some rough edges:
> > 
> > * I think the API documentation (kernel-doc) should be moved out of
> >   index.rst into more appropriate files.
> > * The headings are arguably wrong, because driver.rst, consumer.rst,
> >   etc. use the "document title" style, even though they are part of the
> >   GPIO chapter. But the resulting TOC tree is consistent, and I did not
> >   want to change almost all headings.
> > * Some of the files could use more :c:func:`...` references and general
> >   ReST polish.
> > 
> > But I don't want to make this patchset too large.
> 
> [For future reference, if you're going to CC me on most of a patch series,
> I'd really rather get the whole thing so I don't have to go looking for
> it.]

Noted.

> From a quick look, it seems like a reasonable RST conversion to me.  I do
> wonder if sysfs.txt should just be removed, since it documents something
> we don't want people to use at this point?  Historical purposes are well
> served by the repository history.
> 
> I'd say changing the headings is worthwhile if it produces a better
> result.

I just tried this on one file (driver.rst) and it made no difference in
the output (neither in the TOC in index.html nor in driver.html).

Thanks,
Jonathan Neuschäfer


> OTOH I'd be wary of adding too much "polish"; we really want to
> retain the readability of the plain-text files.
> 
> Anyway, I'm happy to take these through the docs tree or see them go via
> GPIO; Linus, what's your preference?
> 
> Thanks,
> 
> jon




diff --git a/Documentation/driver-api/gpio/driver.rst 
b/Documentation/driver-api/gpio/driver.rst
index 505ee906d7d9..8eb08005be55 100644
--- a/Documentation/driver-api/gpio/driver.rst
+++ b/Documentation/driver-api/gpio/driver.rst
@@ -1,4 +1,3 @@
-
 GPIO Descriptor Driver Interface
 
 
@@ -13,7 +12,7 @@ the structures used to define a GPIO driver:
 
 
 Internal Representation of GPIOs
-
+
 
 Inside a GPIO driver, individual GPIOs are identified by their hardware number,
 which is a unique number between 0 and n, n being the number of GPIOs managed 
by
@@ -36,7 +35,7 @@ identify GPIOs in a bank of I2C GPIO expanders.
 
 
 Controller Drivers: gpio_chip
-=
+-
 
 In the gpiolib framework each GPIO controller is packaged as a "struct
 gpio_chip" (see linux/gpio/driver.h for its complete definition) with members
@@ -74,7 +73,7 @@ not be required.
 
 
 GPIO electrical configuration
--
+~
 
 GPIOs can be configured for several electrical modes of operation by using the
 .set_config() callback. Currently this API supports setting debouncing and
@@ -95,7 +94,7 @@ numbers on the pin controller so they can properly 
cross-reference each other.
 
 
 GPIOs with debounce support

+~~~
 
 Debouncing is a configuration set to a pin indicating that it is connected to
 a mechanical switch or button, or similar that may bounce. Bouncing means the
@@ -112,7 +111,7 @@ is not configurable.
 
 
 GPIOs with open drain/source support
-
+
 
 Open drain (CMOS) or open collector (TTL) means the line is not actively driven
 high: instead you provide the drain/collector as output, so when the transistor
@@ -209,7 +208,7 @@ of actively driving the line low, it is set to input.
 
 
 GPIO drivers providing IRQs

+~~~
 It is custom that GPIO drivers (GPIO chips) are also providing interrupts,
 most often cascaded off a parent interrupt controller, and in some special
 cases the GPIO logic is melded with a SoC's primary interrupt controller.
@@ -359,7 +358,7 @@ below exists.
 
 
 Locking IRQ usage
--
+~
 Input GPIOs can be used as IRQ signals. When this happens, a driver is 
requested
 to mark the GPIO as being used as an IRQ::
 
@@ -378,7 +377,7 @@ When using the gpiolib irqchip helpers, these callback are 
automatically
 assigned.
 
 Real-Time compliance for GPIO IRQ chips

+~~~
 
 Any provider of irqchips needs to be carefully tailored to support Real Time
 preemption. It is desirable that all irqchips in the GPIO subsystem keep this
@@ -404,7