Re: [PATCH] coredump: rename umh_pipe_setup() to coredump_pipe_setup()

[Luis R. Rodriguez]

On Fri, May 11, 2018 at 03:48:51AM +0100, Al Viro wrote:
> On Thu, May 10, 2018 at 11:32:47PM +0000, Luis R. Rodriguez wrote:
> 
> > I think net-next makes sense if Al Viro is OK with that. This way it could 
> > go
> > in regardless of the state of your series, but it also lines up with your 
> > work.
> 
> Fine by me...

OK thanks.

Dave, I'll bounce a copy of the original patch to you, if anything else is 
needed
please let me know.

  Luis

Re: [PATCH] coredump: rename umh_pipe_setup() to coredump_pipe_setup()

[Luis R. Rodriguez]

On Thu, May 10, 2018 at 04:19:09PM -0700, Alexei Starovoitov wrote:
> On Mon, May 07, 2018 at 04:30:02PM -0700, Luis R. Rodriguez wrote:
> > This makes it clearer this code is part of the coredump code, and
> > is not an exported generic helper from kernel/umh.c.
> > 
> > Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> > ---
> >  fs/coredump.c | 9 +
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/coredump.c b/fs/coredump.c
> > index 1e2c87acac9b..566504781683 100644
> > --- a/fs/coredump.c
> > +++ b/fs/coredump.c
> > @@ -508,7 +508,7 @@ static void wait_for_dump_helpers(struct file *file)
> >  }
> >  
> >  /*
> > - * umh_pipe_setup
> > + * coredump_pipe_setup
> >   * helper function to customize the process used
> >   * to collect the core in userspace.  Specifically
> >   * it sets up a pipe and installs it as fd 0 (stdin)
> > @@ -518,7 +518,7 @@ static void wait_for_dump_helpers(struct file *file)
> >   * is a special value that we use to trap recursive
> >   * core dumps
> >   */
> > -static int umh_pipe_setup(struct subprocess_info *info, struct cred *new)
> > +static int coredump_pipe_setup(struct subprocess_info *info, struct cred 
> > *new)
> 
> I think this renaming makes sense.
> How do we want to proceed?
> I can take it as part of my series and get the whole thing through net-next
> or folks want to apply this separately?

I think net-next makes sense if Al Viro is OK with that. This way it could go
in regardless of the state of your series, but it also lines up with your work.

  Luis

Re: [PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

On Tue, May 08, 2018 at 03:42:33PM -0700, Kees Cook wrote:
> On Tue, May 8, 2018 at 11:12 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > + This used to be the default firmware loading facility, and udev 
> > used
> > + to listen for uvents to load firmware for the kernel. The firmware
> > + loading facility functionality in udev has been removed, as such 
> > it
> > + can no longer be relied upon as a fallback mechanism. Linux no 
> > longer
> > + relies on or uses a fallback mechanism in userspace. If you need 
> > to
> > + rely on one refer to the permissively licensed firmwared:
> 
> Typo: firmware

Thanks fixed all typos except this one, this one is meant to be firmwared as
that is the name of the project, the url is below.

> 
> > +
> > + https://github.com/teg/firmwared

  Luis

Re: [PATCH v6 12/13] Documentation: remove stale firmware API reference

[Luis R. Rodriguez]

On Wed, May 09, 2018 at 12:12:09PM -0300, Mauro Carvalho Chehab wrote:
> Em Tue,  8 May 2018 11:12:46 -0700
> "Luis R. Rodriguez" <mcg...@kernel.org> escreveu:
> 
> > It refers to a pending patch, but this was merged eons ago.
> 
> Didn't know that such patch was already merged. Great!
> 
> Regards,
> Mauro
> 
> > 
> > Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
> > ---
> >  Documentation/dell_rbu.txt | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt
> > index 0fdb6aa2704c..077fdc29a0d0 100644
> > --- a/Documentation/dell_rbu.txt
> > +++ b/Documentation/dell_rbu.txt
> > @@ -121,9 +121,6 @@ read back the image downloaded.
> >  
> >  .. note::
> >  
> > -   This driver requires a patch for firmware_class.c which has the modified
> > -   request_firmware_nowait function.
> > -
> 
> > Also after updating the BIOS image a user mode application needs to 
> > execute
> > code which sends the BIOS update request to the BIOS. So on the next 
> > reboot
> > the BIOS knows about the new image downloaded and it updates itself.
> 
> You should likely remove the "Also" here.

Good catch. Will adjust.

> 
> With that,
> 
> Reviewed-by: Mauro Carvalho Chehab <mchehab+sams...@kernel.org>

Great, thanks.

  Luis

[PATCH v6 00/13] firmware_loader changes for v4.18

[Luis R. Rodriguez]

Greg,

Here is what I have queued up for the firmware_loader for v4.18. It
includes a slew of cleanup work, and the new firmware_request_nowarn()
which is quiet but enables the sysfs fallback mechanism. I've gone ahead
and also queued up a few minor fixes for the firmware loader documentation
which have come up recently. These changes are available on my git tree
both based on linux-next [0] and Linus' latest tree [1]. Folks working
on new developments for the firmware loader can use my linux-next
branch 20180508-firmware_loader_for-v4.18-try2 for now.

0-day sends its blessings.

The patches from Mimi's series still require a bit more discussion and
review. The discussion over the EFI firmware fallback mechanism is still
ongoing.

As for the rename that you wanted, perhaps we can do this late in the
merge window considering we're at rc4 now. I can prep something up for
that later.

Question, and specially rants are warmly welcomed.

[0] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180508-firmware_loader_for-v4.18-try2
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180508-firmware_loader_for-v4.18-try2

Andres Rodriguez (6):
  firmware: wrap FW_OPT_* into an enum
  firmware: use () to terminate kernel-doc function names
  firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()
  firmware: add firmware_request_nowarn() - load firmware without
warnings
  ath10k: use firmware_request_nowarn() to load firmware
  ath10k: re-enable the firmware fallback mechanism for testmode

Luis R. Rodriguez (7):
  firmware_loader: document firmware_sysfs_fallback()
  firmware_loader: enhance Kconfig documentation over FW_LOADER
  firmware_loader: move kconfig FW_LOADER entries to its own file
  firmware_loader: make firmware_fallback_sysfs() print more useful
  Documentation: fix few typos and clarifications for the firmware
loader
  Documentation: remove stale firmware API reference
  Documentation: clarify firmware_class provenance and why we can't
rename the module

 Documentation/dell_rbu.txt|   3 -
 .../firmware/fallback-mechanisms.rst  |  14 +-
 .../driver-api/firmware/firmware_cache.rst|   4 +-
 .../driver-api/firmware/request_firmware.rst  |   5 +
 drivers/base/Kconfig  |  90 ++
 drivers/base/firmware_loader/Kconfig  | 154 ++
 drivers/base/firmware_loader/fallback.c   |  53 --
 drivers/base/firmware_loader/fallback.h   |  18 +-
 drivers/base/firmware_loader/firmware.h   |  37 -
 drivers/base/firmware_loader/main.c   |  57 +--
 drivers/net/wireless/ath/ath10k/core.c|   2 +-
 drivers/net/wireless/ath/ath10k/testmode.c|   2 +-
 include/linux/firmware.h  |  10 ++
 13 files changed, 319 insertions(+), 130 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

-- 
2.17.0

[PATCH v6 01/13] firmware: wrap FW_OPT_* into an enum

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 12 
 drivers/base/firmware_loader/fallback.h |  6 ++--
 drivers/base/firmware_loader/firmware.h | 37 +++--
 drivers/base/firmware_loader/main.c |  6 ++--
 4 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..b57a7b3b4122 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -512,7 +512,7 @@ static const struct attribute_group *fw_dev_attr_groups[] = 
{
 
 static struct fw_sysfs *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
-  struct device *device, unsigned int opt_flags)
+  struct device *device, enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -545,7 +545,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
  * 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)
+ enum fw_opt opt_flags, long timeout)
 {
int retval = 0;
struct device *f_dev = _sysfs->dev;
@@ -599,7 +599,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
 
 static int fw_load_from_user_helper(struct firmware *firmware,
const char *name, struct device *device,
-   unsigned int opt_flags)
+   enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -640,7 +640,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
return ret;
 }
 
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -649,7 +649,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
 }
 
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl 
knob\n");
@@ -664,7 +664,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
 #include 
 #include 
 
+#include "firmware.h"
+
 /**
  * struct firmware_fallback_config - firmware fallback configuration settings
  *
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
@@ -43,7 +45,7 @@ void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
-   unsigned int opt_flags,
+   enum fw_opt opt_flags,
int ret)
 {
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 64acbb1a392c..4f433b447367 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -2,6 +2,7 @@
 #ifndef __FIRMWARE_LOADER_H
 #define __FIRMWARE_LOADER_H
 
+#include 
 #include 
 #include 
 #include 
@@ -10,13 +11,33 @@
 
 #include 
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT  (1U << 0)
-#define FW_OPT_NOWAIT  (1U << 1)
-#define FW_OPT_USERHELPER  (1U << 2)
-#define

[PATCH v6 02/13] firmware: use () to terminate kernel-doc function names

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The kernel-doc spec dictates a function name ends in ().

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Randy Dunlap <rdun...@infradead.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: adjust since the wide API rename is not yet merged]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/main.c | 22 +++---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index b57a7b3b4122..529f7013616f 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -125,7 +125,7 @@ static ssize_t timeout_show(struct class *class, struct 
class_attribute *attr,
 }
 
 /**
- * firmware_timeout_store - set number of seconds to wait for firmware
+ * firmware_timeout_store() - set number of seconds to wait for firmware
  * @class: device class pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for timeout value
@@ -239,7 +239,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
 }
 
 /**
- * firmware_loading_store - set value in the 'loading' control file
+ * firmware_loading_store() - set value in the 'loading' control file
  * @dev: device pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for loading control value
@@ -431,7 +431,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int 
min_size)
 }
 
 /**
- * firmware_data_write - write method for firmware
+ * firmware_data_write() - write method for firmware
  * @filp: open sysfs file
  * @kobj: kobject for the device
  * @bin_attr: bin_attr structure
@@ -537,7 +537,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
 }
 
 /**
- * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
+ * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
  * @fw_sysfs: firmware sysfs information for the firmware to load
  * @opt_flags: flags of options, FW_OPT_*
  * @timeout: timeout to wait for the load
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 9919f0e6a7cc..4d11efadb3a4 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -597,7 +597,7 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 
 /**
- * request_firmware: - send firmware request and wait for it
+ * request_firmware() - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -632,7 +632,7 @@ request_firmware(const struct firmware **firmware_p, const 
char *name,
 EXPORT_SYMBOL(request_firmware);
 
 /**
- * request_firmware_direct: - load firmware directly without usermode helper
+ * request_firmware_direct() - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -657,7 +657,7 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
 /**
- * firmware_request_cache: - cache firmware for suspend so resume can use it
+ * firmware_request_cache() - cache firmware for suspend so resume can use it
  * @name: name of firmware file
  * @device: device for which firmware should be cached for
  *
@@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const 
char *name)
 EXPORT_SYMBOL_GPL(firmware_request_cache);
 
 /**
- * request_firmware_into_buf - load firmware into a previously allocated buffer
+ * request_firmware_into_buf() - load firmware into a previously allocated 
buffer
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded and DMA region allocated
@@ -713,7 +713,7 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
- * release_firmware: - release the resource associated with a firmware image
+ * release_firmware() - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
 void release_firmware(const struct firmware *fw)
@@ -755,7 +755,7 @@ static void request_firmware_work_func(struct work_struct 
*work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_nowait() - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  * is non-zero else the firmware copy must be done manually.
@@ -824,7 +824,7 @@ EXPORT_SYMBOL(request_firmware_nowait);
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
 /**

[PATCH v6 04/13] firmware_loader: document firmware_sysfs_fallback()

[Luis R. Rodriguez]

This also sets the expecations for future fallback interfaces, even
if they are not exported.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 3db9e0f225ac..9169e7b9800c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,6 +662,26 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
+/**
+ * firmware_fallback_sysfs() - use the fallback mechanism to find firmware
+ * @fw: pointer to firmware image
+ * @name: name of firmware file to look for
+ * @device: device for which firmware is being loaded
+ * @opt_flags: options to control firmware loading behaviour
+ * @ret: return value from direct lookup which triggered the fallback mechanism
+ *
+ * This function is called if direct lookup for the firmware failed, it enables
+ * a fallback mechanism through userspace by exposing a sysfs loading
+ * interface. Userspace is in charge of loading the firmware through the syfs
+ * loading interface. This syfs fallback mechanism may be disabled completely
+ * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
+ * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
+ * flag, if so it would also disable the fallback mechanism. A system may want
+ * to enfoce the sysfs fallback mechanism at all times, it can do this by
+ * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
+ * Enabling force_sysfs_fallback is functionally equivalent to build a kernel
+ * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
+ **/
 int firmware_fallback_sysfs(struct firmware *fw, const char *name,
struct device *device,
enum fw_opt opt_flags,
-- 
2.17.0

[PATCH v6 05/13] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

If you try to read FW_LOADER today it speaks of old riddles and
unless you have been following development closely you will loose
track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD
is a bit fuzzy and how it fits into this big picture.

Give the FW_LOADER kconfig documentation some love with more up to
date developments and recommendations. While at it, wrap the FW_LOADER
code into its own menu to compartamentalize and make it clearer which
components really are part of the FW_LOADER. This should also make
it easier to later move these kconfig entries into the firmware_loader/
directory later.

This also now recommends using firmwared [0] for folks left needing a uevent
handler in userspace for the sysfs firmware fallback mechanis given udev's
uevent firmware mechanism was ripped out a while ago.

[0] https://github.com/teg/firmwared

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 165 ++-
 1 file changed, 131 insertions(+), 34 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 29b0eb452b3a..a4fe86caecca 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -70,39 +70,64 @@ config STANDALONE
  If unsure, say Y.
 
 config PREVENT_FIRMWARE_BUILD
-   bool "Prevent firmware from being built"
+   bool "Disable drivers features which enable custom firmware building"
default y
help
- Say yes to avoid building firmware. Firmware is usually shipped
- with the driver and only when updating the firmware should a
- rebuild be made.
- If unsure, say Y here.
+ Say yes to disable driver features which enable building a custom
+ driver firmwar at kernel build time. These drivers do not use the
+ kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they
+ use their own custom loading mechanism. The required firmware is
+ usually shipped with the driver, building the driver firmware
+ should only be needed if you have an updated firmware source.
+
+ Firmware should not be being built as part of kernel, these days
+ you should always prevent this and say Y here. There are only two
+ old drivers which enable building of its firmware at kernel build
+ time:
+
+   o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
+   o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
+
+menu "Firmware loader"
 
 config FW_LOADER
-   tristate "Userspace firmware loading support" if EXPERT
+   tristate "Firmware loading facility" if EXPERT
default y
---help---
- This option is provided for the case where none of the in-tree modules
- require userspace firmware loading support, but a module built
- out-of-tree does.
+ This enables the firmware loading facility in the kernel. The kernel
+ will first look for built-in firmware, if it has any. Next, it will
+ look for the requested firmware in a series of filesystem paths:
+
+   o firmware_class path module parameter or kernel boot param
+   o /lib/firmware/updates/UTS_RELEASE
+   o /lib/firmware/updates
+   o /lib/firmware/UTS_RELEASE
+   o /lib/firmware
+
+ Enabling this feature only increases your kernel image by about
+ 828 bytes, enable this option unless you are certain you don't
+ need firmware.
+
+ You typically want this built-in (=y) but you can also enable this
+ as a module, in which case the firmware_class module will be built.
+ You also want to be sure to enable this built-in if you are going to
+ enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
+
+if FW_LOADER
 
 config EXTRA_FIRMWARE
-   string "External firmware blobs to build into the kernel binary"
-   depends on FW_LOADER
+   string "Build these firmware blobs into the kernel binary"
help
- Various drivers in the kernel source tree may require firmware,
- which is generally available in your distribution's linux-firmware
- package.
+ Device drivers which require firmware can typically deal with
+ having the kernel load firmware from the various supported
+ /lib/firmware/ paths. This option enables you to build into the
+ kernel firmware files. Built-in firmware searches are preceeded
+ over firmware lookups using your filesystem over the supported
+ /lib/firmware paths documented on CONFIG_FW_LOADER.
 
- The linux-firmware package should install firmware into
- /lib/firmware/ on your system, so they can be loaded by userspace
- helpers on request.
-
- This option allows firmware to be built into the kernel for the case
- where t

[PATCH v6 03/13] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This is done since this call is now exposed through kernel-doc,
and since this also paves the way for different future types of
fallback mechanims.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/fallback.h | 16 
 drivers/base/firmware_loader/firmware.h |  2 +-
 drivers/base/firmware_loader/main.c |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 529f7013616f..3db9e0f225ac 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,10 +662,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret)
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index a3b73a09db6c..21063503e4ea 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -31,10 +31,10 @@ struct firmware_fallback_config {
 };
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret);
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
 void fw_fallback_set_cache_timeout(void);
@@ -43,10 +43,10 @@ void fw_fallback_set_default_timeout(void);
 int register_sysfs_loader(void);
 void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
-   struct device *device,
-   enum fw_opt opt_flags,
-   int ret)
+static inline int firmware_fallback_sysfs(struct firmware *fw, const char 
*name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
 {
/* Keep carrying over the same error */
return ret;
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 4f433b447367..4c1395f8e7ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -20,7 +20,7 @@
  * @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
  * @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
  * filesystem lookup fails at finding the firmware.  For details refer to
- * fw_sysfs_fallback().
+ * firmware_fallback_sysfs().
  * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
  * @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
  * cache the firmware upon suspend, so that upon resume races against the
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 4d11efadb3a4..abdc4e4d55f1 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -581,7 +581,7 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
dev_warn(device,
 "Direct firmware load for %s failed with error 
%d\n",
 name, ret);
-   ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+   ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
ret = assign_fw(fw, device, opt_flags);
 
-- 
2.17.0

[PATCH v6 06/13] firmware_loader: move kconfig FW_LOADER entries to its own file

[Luis R. Rodriguez]

This will make it easier to track and easier to understand
what components and features are part of the FW_LOADER. There
are some components related to firmware which have *nothing* to
do with the FW_LOADER, souch as PREVENT_FIRMWARE_BUILD.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 155 +--
 drivers/base/firmware_loader/Kconfig | 154 ++
 2 files changed, 155 insertions(+), 154 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index a4fe86caecca..06d6e27784fa 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -88,160 +88,7 @@ config PREVENT_FIRMWARE_BUILD
o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
 
-menu "Firmware loader"
-
-config FW_LOADER
-   tristate "Firmware loading facility" if EXPERT
-   default y
-   ---help---
- This enables the firmware loading facility in the kernel. The kernel
- will first look for built-in firmware, if it has any. Next, it will
- look for the requested firmware in a series of filesystem paths:
-
-   o firmware_class path module parameter or kernel boot param
-   o /lib/firmware/updates/UTS_RELEASE
-   o /lib/firmware/updates
-   o /lib/firmware/UTS_RELEASE
-   o /lib/firmware
-
- Enabling this feature only increases your kernel image by about
- 828 bytes, enable this option unless you are certain you don't
- need firmware.
-
- You typically want this built-in (=y) but you can also enable this
- as a module, in which case the firmware_class module will be built.
- You also want to be sure to enable this built-in if you are going to
- enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
-
-if FW_LOADER
-
-config EXTRA_FIRMWARE
-   string "Build these firmware blobs into the kernel binary"
-   help
- Device drivers which require firmware can typically deal with
- having the kernel load firmware from the various supported
- /lib/firmware/ paths. This option enables you to build into the
- kernel firmware files. Built-in firmware searches are preceeded
- over firmware lookups using your filesystem over the supported
- /lib/firmware paths documented on CONFIG_FW_LOADER.
-
- This may be useful for testing or if the firmware is required early on
- in boot and cannot rely on the firmware being placed in an initrd or
- initramfs.
-
- This option is a string and takes the (space-separated) names of the
- firmware files -- the same names that appear in MODULE_FIRMWARE()
- and request_firmware() in the source. These files should exist under
- the directory specified by the EXTRA_FIRMWARE_DIR option, which is
- /lib/firmware by default.
-
- For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
- the usb8388.bin file into /lib/firmware, and build the kernel. Then
- any request_firmware("usb8388.bin") will be satisfied internally
- inside the kernel without ever looking at your filesystem at runtime.
-
- WARNING: If you include additional firmware files into your binary
- kernel image that are not available under the terms of the GPL,
- then it may be a violation of the GPL to distribute the resulting
- image since it combines both GPL and non-GPL work. You should
- consult a lawyer of your own before distributing such an image.
-
-config EXTRA_FIRMWARE_DIR
-   string "Firmware blobs root directory"
-   depends on EXTRA_FIRMWARE != ""
-   default "/lib/firmware"
-   help
- This option controls the directory in which the kernel build system
- looks for the firmware files listed in the EXTRA_FIRMWARE option.
-
-config FW_LOADER_USER_HELPER
-   bool "Enable the firmware sysfs fallback mechanism"
-   help
- This option enables a sysfs loading facility to enable firmware
- loading to the kernel through userspace as a fallback mechanism
- if and only if the kernel's direct filesystem lookup for the
- firmware failed using the different /lib/firmware/ paths, or the
- path specified in the firmware_class path module parameter, or the
- firmware_class path kernel boot parameter if the firmware_class is
- built-in. For details on how to work with the sysfs fallback mechanism
- refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
-
- The direct filesystem lookup for firwmare is always used first now.
-
- If the kernel's direct filesystem loo

[PATCH v6 09/13] ath10k: use firmware_request_nowarn() to load firmware

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This reduces the unnecessary spew when trying to load optional firmware:
"Direct firmware load for ... failed with error -2"

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/ath/ath10k/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index 4cf54a7ef09a..ad4f6e3c0737 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -694,7 +694,7 @@ static const struct firmware *ath10k_fetch_fw_file(struct 
ath10k *ar,
dir = ".";
 
snprintf(filename, sizeof(filename), "%s/%s", dir, file);
-   ret = request_firmware(, filename, ar->dev);
+   ret = firmware_request_nowarn(, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_BOOT, "boot fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v6 07/13] firmware_loader: make firmware_fallback_sysfs() print more useful

[Luis R. Rodriguez]

If we resort to using the sysfs fallback mechanism we don't print
the filename. This can be deceiving given we could have a series of
callers intertwined and it'd be unclear exactly for what firmware
this was meant for.

Additionally, although we don't currently use FW_OPT_NO_WARN when
dealing with the fallback mechanism, we will soon, so just respect
its use consistently.

And even if you *don't* want to print always on failure, you may
want to print when debugging so enable dynamic debug print when
FW_OPT_NO_WARN is used.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 9169e7b9800c..b676a99c469c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -690,6 +690,11 @@ int firmware_fallback_sysfs(struct firmware *fw, const 
char *name,
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
 
-   dev_warn(device, "Falling back to user helper\n");
+   if (!(opt_flags & FW_OPT_NO_WARN))
+   dev_warn(device, "Falling back to syfs fallback for: %s\n",
+name);
+   else
+   dev_dbg(device, "Falling back to sysfs fallback for: %s\n",
+   name);
return fw_load_from_user_helper(fw, name, device, opt_flags);
 }
-- 
2.17.0

[PATCH v6 10/13] ath10k: re-enable the firmware fallback mechanism for testmode

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The ath10k testmode uses request_firmware_direct() in order to avoid
producing firmware load warnings. Disabling the fallback mechanism was a
side effect of disabling warnings.

We now have a new API that allows us to avoid warnings while keeping the
fallback mechanism enabled. So use that instead.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Kalle Valo <kv...@codeaurora.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/net/wireless/ath/ath10k/testmode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/testmode.c 
b/drivers/net/wireless/ath/ath10k/testmode.c
index 568810b41657..c24ee616833c 100644
--- a/drivers/net/wireless/ath/ath10k/testmode.c
+++ b/drivers/net/wireless/ath/ath10k/testmode.c
@@ -157,7 +157,7 @@ static int ath10k_tm_fetch_utf_firmware_api_1(struct ath10k 
*ar,
 ar->hw_params.fw.dir, ATH10K_FW_UTF_FILE);
 
/* load utf firmware image */
-   ret = request_firmware_direct(_file->firmware, filename, ar->dev);
+   ret = firmware_request_nowarn(_file->firmware, filename, ar->dev);
ath10k_dbg(ar, ATH10K_DBG_TESTMODE, "testmode fw request '%s': %d\n",
   filename, ret);
 
-- 
2.17.0

[PATCH v6 11/13] Documentation: fix few typos and clarifications for the firmware loader

[Luis R. Rodriguez]

Fix a few typos, and clarify a few sentences.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 Documentation/driver-api/firmware/fallback-mechanisms.rst | 5 +++--
 Documentation/driver-api/firmware/firmware_cache.rst  | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index f353783ae0be..a39323ef7d29 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -83,7 +83,7 @@ and a file upload firmware into:
   * /sys/$DEVPATH/data
 
 To upload firmware you will echo 1 onto the loading file to indicate
-you are loading firmware. You then cat the firmware into the data file,
+you are loading firmware. You then write the firmware into the data file,
 and you notify the kernel the firmware is ready by echo'ing 0 onto
 the loading file.
 
@@ -136,7 +136,8 @@ by kobject uevents. This is specially exacerbated due to 
the fact that most
 distributions today disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
 
 Refer to do_firmware_uevent() for details of the kobject event variables
-setup. Variables passwdd with a kobject add event:
+setup. The variables currently passed to userspace with a "kobject add"
+event are:
 
 * FIRMWARE=firmware name
 * TIMEOUT=timeout value
diff --git a/Documentation/driver-api/firmware/firmware_cache.rst 
b/Documentation/driver-api/firmware/firmware_cache.rst
index 2210e5bfb332..c2e69d9c6bf1 100644
--- a/Documentation/driver-api/firmware/firmware_cache.rst
+++ b/Documentation/driver-api/firmware/firmware_cache.rst
@@ -29,8 +29,8 @@ Some implementation details about the firmware cache setup:
 * If an asynchronous call is used the firmware cache is only set up for a
   device if if the second argument (uevent) to request_firmware_nowait() is
   true. When uevent is true it requests that a kobject uevent be sent to
-  userspace for the firmware request. For details refer to the Fackback
-  mechanism documented below.
+  userspace for the firmware request through the sysfs fallback mechanism
+  if the firmware file is not found.
 
 * If the firmware cache is determined to be needed as per the above two
   criteria the firmware cache is setup by adding a devres entry for the
-- 
2.17.0

[PATCH v6 08/13] firmware: add firmware_request_nowarn() - load firmware without warnings

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

Currently the firmware loader only exposes one silent path for querying
optional firmware, and that is firmware_request_direct(). This function
also disables the sysfs fallback mechanism, which might not always be the
desired behaviour [0].

This patch introduces a variations of request_firmware() that enable the
caller to disable the undesired warning messages but enables the sysfs
fallback mechanism. This is equivalent to adding FW_OPT_NO_WARN to the
old behaviour.

[0]: https://git.kernel.org/linus/c0cc00f250e1

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: used the old API calls as the full rename is not done yet, and
 add the caller for when FW_LOADER is disabled, enhance documentation ]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/request_firmware.rst  |  5 
 drivers/base/firmware_loader/main.c   | 27 +++
 include/linux/firmware.h  | 10 +++
 3 files changed, 42 insertions(+)

diff --git a/Documentation/driver-api/firmware/request_firmware.rst 
b/Documentation/driver-api/firmware/request_firmware.rst
index d5ec95a7195b..f62bdcbfed5b 100644
--- a/Documentation/driver-api/firmware/request_firmware.rst
+++ b/Documentation/driver-api/firmware/request_firmware.rst
@@ -20,6 +20,11 @@ request_firmware
 .. kernel-doc:: drivers/base/firmware_loader/main.c
:functions: request_firmware
 
+firmware_request_nowarn
+---
+.. kernel-doc:: drivers/base/firmware_loader/main.c
+   :functions: firmware_request_nowarn
+
 request_firmware_direct
 ---
 .. kernel-doc:: drivers/base/firmware_loader/main.c
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index abdc4e4d55f1..2be79ee773c0 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -631,6 +631,33 @@ request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 EXPORT_SYMBOL(request_firmware);
 
+/**
+ * firmware_request_nowarn() - request for an optional fw module
+ * @firmware: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function is similar in behaviour to request_firmware(), except
+ * it doesn't produce warning messages when the file is not found.
+ * The sysfs fallback mechanism is enabled if direct filesystem lookup fails,
+ * however, however failures to find the firmware file with it are still
+ * supressed. It is therefore up to the driver to check for the return value
+ * of this call and to decide when to inform the users of errors.
+ **/
+int firmware_request_nowarn(const struct firmware **firmware, const char *name,
+   struct device *device)
+{
+   int ret;
+
+   /* Need to pin this module until return */
+   __module_get(THIS_MODULE);
+   ret = _request_firmware(firmware, name, device, NULL, 0,
+   FW_OPT_UEVENT | FW_OPT_NO_WARN);
+   module_put(THIS_MODULE);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(firmware_request_nowarn);
+
 /**
  * request_firmware_direct() - load firmware directly without usermode helper
  * @firmware_p: pointer to firmware image
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 41050417cafb..2dd566c91d44 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -42,6 +42,8 @@ struct builtin_fw {
 #if defined(CONFIG_FW_LOADER) || (defined(CONFIG_FW_LOADER_MODULE) && 
defined(MODULE))
 int request_firmware(const struct firmware **fw, const char *name,
 struct device *device);
+int firmware_request_nowarn(const struct firmware **fw, const char *name,
+   struct device *device);
 int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
@@ -59,6 +61,14 @@ static inline int request_firmware(const struct firmware 
**fw,
 {
return -EINVAL;
 }
+
+static inline int firmware_request_nowarn(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+   return -EINVAL;
+}
+
 static inline int request_firmware_nowait(
struct module *module, bool uevent,
const char *name, struct device *device, gfp_t gfp, void *context,
-- 
2.17.0

[PATCH v6 12/13] Documentation: remove stale firmware API reference

[Luis R. Rodriguez]

It refers to a pending patch, but this was merged eons ago.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 Documentation/dell_rbu.txt | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/Documentation/dell_rbu.txt b/Documentation/dell_rbu.txt
index 0fdb6aa2704c..077fdc29a0d0 100644
--- a/Documentation/dell_rbu.txt
+++ b/Documentation/dell_rbu.txt
@@ -121,9 +121,6 @@ read back the image downloaded.
 
 .. note::
 
-   This driver requires a patch for firmware_class.c which has the modified
-   request_firmware_nowait function.
-
Also after updating the BIOS image a user mode application needs to execute
code which sends the BIOS update request to the BIOS. So on the next reboot
the BIOS knows about the new image downloaded and it updates itself.
-- 
2.17.0

[PATCH v6 13/13] Documentation: clarify firmware_class provenance and why we can't rename the module

[Luis R. Rodriguez]

Clarify the provenance of the firmware loader firmware_class module name
and why we cannot rename the module in the future.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 .../driver-api/firmware/fallback-mechanisms.rst  | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst 
b/Documentation/driver-api/firmware/fallback-mechanisms.rst
index a39323ef7d29..a8047be4a96e 100644
--- a/Documentation/driver-api/firmware/fallback-mechanisms.rst
+++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst
@@ -72,9 +72,12 @@ the firmware requested, and establishes it in the device 
hierarchy by
 associating the device used to make the request as the device's parent.
 The sysfs directory's file attributes are defined and controlled through
 the new device's class (firmware_class) and group (fw_dev_attr_groups).
-This is actually where the original firmware_class.c file name comes from,
-as originally the only firmware loading mechanism available was the
-mechanism we now use as a fallback mechanism.
+This is actually where the original firmware_class module name came from,
+given that originally the only firmware loading mechanism available was the
+mechanism we now use as a fallback mechanism, which which registers a
+struct class firmware_class. Because the attributes exposed are part of the
+module name, the module name firmware_class cannot be renamed in the future, to
+ensure backward compatibilty with old userspace.
 
 To load firmware using the sysfs interface we expose a loading indicator,
 and a file upload firmware into:
-- 
2.17.0

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 10:43:49AM -0700, Luis R. Rodriguez wrote:
> Greg,
> 
> I've reviewed the pending patches for the firmware_laoder and as for
> v4.18, the following 3 patches from Andres have been iterated enough
> that they're ready after I made some final minor changes, mostly just
> style fixes and re-arrangements in terms of order. The new API he was
> suggesting to add requires just a bit more review.

We're done with review of the new API for the sync no warn call,
so I'll just re-issue another patch series with those patches
in a new series.

Coming right up.

  Luis

Re: [PATCH v2 net-next 2/4] net: add skeleton of bpfilter kernel module

[Luis R. Rodriguez]

On Wed, May 02, 2018 at 09:36:02PM -0700, Alexei Starovoitov wrote:
> bpfilter.ko consists of bpfilter_kern.c (normal kernel module code)
> and user mode helper code that is embedded into bpfilter.ko
> 
> The steps to build bpfilter.ko are the following:
> - main.c is compiled by HOSTCC into the bpfilter_umh elf executable file
> - with quite a bit of objcopy and Makefile magic the bpfilter_umh elf file
>   is converted into bpfilter_umh.o object file
>   with _binary_net_bpfilter_bpfilter_umh_start and _end symbols
>   Example:
>   $ nm ./bld_x64/net/bpfilter/bpfilter_umh.o
>   4cf8 T _binary_net_bpfilter_bpfilter_umh_end
>   4cf8 A _binary_net_bpfilter_bpfilter_umh_size
>    T _binary_net_bpfilter_bpfilter_umh_start
> - bpfilter_umh.o and bpfilter_kern.o are linked together into bpfilter.ko
> 
> bpfilter_kern.c is a normal kernel module code that calls
> the fork_usermode_blob() helper to execute part of its own data
> as a user mode process.
> 
> Notice that _binary_net_bpfilter_bpfilter_umh_start - end
> is placed into .init.rodata section, so it's freed as soon as __init
> function of bpfilter.ko is finished.
> As part of __init the bpfilter.ko does first request/reply action
> via two unix pipe provided by fork_usermode_blob() helper to
> make sure that umh is healthy. If not it will kill it via pid.

It does this very fast, right away. On a really slow system how are you sure
that this won't race and the execution of the check happens early on prior to
letting the actual setup trigger? After all, we're calling the userpsace
process in async mode. We could preempt it now.

> Later bpfilter_process_sockopt() will be called from bpfilter hooks
> in get/setsockopt() to pass iptable commands into umh via bpfilter.ko
> 
> If admin does 'rmmod bpfilter' the __exit code bpfilter.ko will
> kill umh as well.
> 
> Signed-off-by: Alexei Starovoitov 
> ---
>  include/linux/bpfilter.h  | 15 +++
>  include/uapi/linux/bpfilter.h | 21 ++
>  net/Kconfig   |  2 +
>  net/Makefile  |  1 +
>  net/bpfilter/Kconfig  | 17 
>  net/bpfilter/Makefile | 24 +++
>  net/bpfilter/bpfilter_kern.c  | 93 
> +++
>  net/bpfilter/main.c   | 63 +
>  net/bpfilter/msgfmt.h | 17 
>  net/ipv4/Makefile |  2 +
>  net/ipv4/bpfilter/Makefile|  2 +
>  net/ipv4/bpfilter/sockopt.c   | 42 +++
>  net/ipv4/ip_sockglue.c| 17 
>  13 files changed, 316 insertions(+)
>  create mode 100644 include/linux/bpfilter.h
>  create mode 100644 include/uapi/linux/bpfilter.h
>  create mode 100644 net/bpfilter/Kconfig
>  create mode 100644 net/bpfilter/Makefile
>  create mode 100644 net/bpfilter/bpfilter_kern.c
>  create mode 100644 net/bpfilter/main.c
>  create mode 100644 net/bpfilter/msgfmt.h
>  create mode 100644 net/ipv4/bpfilter/Makefile
>  create mode 100644 net/ipv4/bpfilter/sockopt.c
> 
> diff --git a/include/linux/bpfilter.h b/include/linux/bpfilter.h
> new file mode 100644
> index ..687b1760bb9f
> --- /dev/null
> +++ b/include/linux/bpfilter.h
> @@ -0,0 +1,15 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _LINUX_BPFILTER_H
> +#define _LINUX_BPFILTER_H
> +
> +#include 
> +
> +struct sock;
> +int bpfilter_ip_set_sockopt(struct sock *sk, int optname, char *optval,
> + unsigned int optlen);
> +int bpfilter_ip_get_sockopt(struct sock *sk, int optname, char *optval,
> + int *optlen);
> +extern int (*bpfilter_process_sockopt)(struct sock *sk, int optname,
> +char __user *optval,
> +unsigned int optlen, bool is_set);
> +#endif
> diff --git a/include/uapi/linux/bpfilter.h b/include/uapi/linux/bpfilter.h
> new file mode 100644
> index ..2ec3cc99ea4c
> --- /dev/null
> +++ b/include/uapi/linux/bpfilter.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _UAPI_LINUX_BPFILTER_H
> +#define _UAPI_LINUX_BPFILTER_H
> +
> +#include 
> +
> +enum {
> + BPFILTER_IPT_SO_SET_REPLACE = 64,
> + BPFILTER_IPT_SO_SET_ADD_COUNTERS = 65,
> + BPFILTER_IPT_SET_MAX,
> +};
> +
> +enum {
> + BPFILTER_IPT_SO_GET_INFO = 64,
> + BPFILTER_IPT_SO_GET_ENTRIES = 65,
> + BPFILTER_IPT_SO_GET_REVISION_MATCH = 66,
> + BPFILTER_IPT_SO_GET_REVISION_TARGET = 67,
> + BPFILTER_IPT_GET_MAX,
> +};
> +
> +#endif /* _UAPI_LINUX_BPFILTER_H */
> diff --git a/net/Kconfig b/net/Kconfig
> index b62089fb1332..ed6368b306fa 100644
> --- a/net/Kconfig
> +++ b/net/Kconfig
> @@ -201,6 +201,8 @@ source "net/bridge/netfilter/Kconfig"
>  
>  endif
>  
> +source "net/bpfilter/Kconfig"
> +
>  source "net/dccp/Kconfig"
>  source "net/sctp/Kconfig"
>  source "net/rds/Kconfig"
> diff --git a/net/Makefile b/net/Makefile
> index a6147c61b174..7f982b7682bd 

Re: [PATCH v2 net-next 1/4] umh: introduce fork_usermode_blob() helper

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 06:37:11PM -0700, Alexei Starovoitov wrote:
> On Fri, May 04, 2018 at 07:56:43PM +0000, Luis R. Rodriguez wrote:
> > What a mighty short list of reviewers. Adding some more. My review below.
> > I'd appreciate a Cc on future versions of these patches.
> 
> sure.
> 
> > On Wed, May 02, 2018 at 09:36:01PM -0700, Alexei Starovoitov wrote:
> > > Introduce helper:
> > > int fork_usermode_blob(void *data, size_t len, struct umh_info *info);
> > > struct umh_info {
> > >struct file *pipe_to_umh;
> > >struct file *pipe_from_umh;
> > >pid_t pid;
> > > };
> > > 
> > > that GPLed kernel modules (signed or unsigned) can use it to execute part
> > > of its own data as swappable user mode process.
> > > 
> > > The kernel will do:
> > > - mount "tmpfs"
> > 
> > Actually its a *shared* vfsmount tmpfs for all umh blobs.
> 
> yep

OK just note CONFIG_TMPFS can be disabled, and likewise for CONFIG_SHMEM,
in which case tmpfs and shmem are replaced by a simple ramfs code, more
appropriate for systems without swap.

> > > +static struct vfsmount *umh_fs;
> > > +
> > > +static int init_tmpfs(void)
> > 
> > Please use umh_init_tmpfs(). 
> 
> ok
> 
> > Also see init/main.c do_basic_setup() which calls
> > usermodehelper_enable() prior to do_initcalls(). Now, fortunately TMPFS is 
> > only
> > bool, saving us from some races and we do call tmpfs's init first 
> > shmem_init():
> > 
> > static void __init do_basic_setup(void)
> > {
> > cpuset_init_smp();
> > shmem_init();
> > driver_init();
> > init_irq_proc();
> > do_ctors();
> > usermodehelper_enable();
> > do_initcalls();
> > }
> > 
> > But it also means we're enabling your new call call fork_usermode_blob() on
> > early init code even if we're not setup. Since this umh tmpfs vfsmount is
> > shared I'd say just call this init right before usermodehelper_enable()
> > on do_basic_setup().
> 
> Not following.
> Why init_tmpfs() should be called by __init function?

Nope, not at all, I was suggesting:

diff --git a/init/main.c b/init/main.c
index 0697284a28ee..67a48fbd96ca 100644
--- a/init/main.c
+++ b/init/main.c
@@ -973,6 +973,7 @@ static void __init do_basic_setup(void)
driver_init();
init_irq_proc();
do_ctors();
+   umh_init_tmpfs();
usermodehelper_enable();
do_initcalls();
 }

Mainly to avoid the locking situation Jann Horn noted, and also provide
proper kernel ordering expectations.

> Are you saying make 'static struct vfsmount *shm_mnt;'
> global and use it here? so no init_tmpfs() necessary?
> I think that can work, but feels that having two
> tmpfs mounts (one for shmem and one for umh) is cleaner.

No, but now that you mention it... if a shared vfsmount is not used the
/sys/kernel/mm/transparent_hugepage/shmem_enabled knob for using huge pages
would not be followed for umh modules. For the i915 driver this was *why*
they ended up adding shmem_file_setup_with_mnt(), they wanted huge pages to
support huge-gtt-pages. What is the rationale behind umh.c using it for
umh modules?

Users of shmem_kernel_file_setup() spawned later out of the desire to
*avoid* LSMs since it didn't make sense in their case as their inodes
are never exposed to userspace. Such is the case for ipc/shm.c and
security/keys/big_key.c. Refer to commit c7277090927a5 ("security: shmem:
implement kernel private shmem inodes") and then commit e1832f2923ec9
("ipc: use private shmem or hugetlbfs inodes for shm segments").

In this new umh usermode modules case we are:

  a) actually mapping data already extracted by the kernel somehow from
 a file somehow, presumably from /lib/modules/ path somewhere, but
 again this is not visible to umc.c, as it just gets called with:

fork_usermode_blob(void *data, size_t len, struct umh_info *info)

  b) Creating the respective tmpfs file with shmem_file_setup_with_mnt()
 with our on our own shared struct vfsmount *umh_fs.

  c) Populating the file created and stuffing it with our data passed

  d) Calling do_execve_file() on it.

Its not clear to me why you used shmem_file_setup_with_mnt() in this case. What
are the gains? It would make sense to use shmem_kernel_file_setup() to avoid an
LSM check on step b) *but* only if we already had a proper LSM check on step
a).

I checked how you use fork_usermode_blob() in a) and in this case the kernel
module bpfilter would be loaded first, and for that we already have an LSM
check / hook for that module. From my review then, the magic done on your
second patch to stuff the userspace application into the 

Re: [PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Luis R. Rodriguez]

On Fri, May 04, 2018 at 09:17:08PM +0200, Krzysztof Halasa wrote:
> "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> 
> >   * CONFIG_WANXL --> CONFIG_WANXL_BUILD_FIRMWARE
> >   * CONFIG_SCSI_AIC79XX --> CONFIG_AIC79XX_BUILD_FIRMWARE
> >
> > To this day both of these drivers are building driver *firmwares* when
> > the option CONFIG_PREVENT_FIRMWARE_BUILD is disabled, and they don't
> > even make use of the firmware API at all.
> 
> Don't know for sure about Adaptec, but wanXL firmware fits every
> definition of "stable". BTW it's a 1997 or so early PCI card, built
> around the PLX PCI9060 bus mastering PCI bridge and Motorola 68360
> (the original QUICC) processor. Maximum bit rate of 2 Mb/s on each sync
> serial port.

So we can nuke CONFIG_WANXL_BUILD_FIRMWARE now?

> It's more about delivering the .S source for the firmware, I guess.
> Nobody is expected to build it. The fw is about 2.5 KB and is directly
> linked with the driver.

:P Future work I guess would be to just use the firmware API and stuff
it into linux-firmware?

  Luis

Re: [PATCH v2 net-next 1/4] umh: introduce fork_usermode_blob() helper

[Luis R. Rodriguez]

What a mighty short list of reviewers. Adding some more. My review below.
I'd appreciate a Cc on future versions of these patches.

On Wed, May 02, 2018 at 09:36:01PM -0700, Alexei Starovoitov wrote:
> Introduce helper:
> int fork_usermode_blob(void *data, size_t len, struct umh_info *info);
> struct umh_info {
>struct file *pipe_to_umh;
>struct file *pipe_from_umh;
>pid_t pid;
> };
> 
> that GPLed kernel modules (signed or unsigned) can use it to execute part
> of its own data as swappable user mode process.
> 
> The kernel will do:
> - mount "tmpfs"

Actually its a *shared* vfsmount tmpfs for all umh blobs.

> - allocate a unique file in tmpfs
> - populate that file with [data, data + len] bytes
> - user-mode-helper code will do_execve that file and, before the process
>   starts, the kernel will create two unix pipes for bidirectional
>   communication between kernel module and umh
> - close tmpfs file, effectively deleting it
> - the fork_usermode_blob will return zero on success and populate
>   'struct umh_info' with two unix pipes and the pid of the user process

But since its using UMH_WAIT_EXEC, all we can guarantee currently is the
inception point was intended, well though out, and will run, but the return
value in no way reflects the success or not of the execution. More below.

> As the first step in the development of the bpfilter project
> the fork_usermode_blob() helper is introduced to allow user mode code
> to be invoked from a kernel module. The idea is that user mode code plus
> normal kernel module code are built as part of the kernel build
> and installed as traditional kernel module into distro specified location,
> such that from a distribution point of view, there is
> no difference between regular kernel modules and kernel modules + umh code.
> Such modules can be signed, modprobed, rmmod, etc. The use of this new helper
> by a kernel module doesn't make it any special from kernel and user space
> tooling point of view.
> 
> Such approach enables kernel to delegate functionality traditionally done
> by the kernel modules into the user space processes (either root or !root) and
> reduces security attack surface of the new code. The buggy umh code would 
> crash
> the user process, but not the kernel. Another advantage is that umh code
> of the kernel module can be debugged and tested out of user space
> (e.g. opening the possibility to run clang sanitizers, fuzzers or
> user space test suites on the umh code).
> In case of the bpfilter project such architecture allows complex control plane
> to be done in the user space while bpf based data plane stays in the kernel.
> 
> Since umh can crash, can be oom-ed by the kernel, killed by the admin,
> the kernel module that uses them (like bpfilter) needs to manage life
> time of umh on its own via two unix pipes and the pid of umh.
> 
> The exit code of such kernel module should kill the umh it started,
> so that rmmod of the kernel module will cleanup the corresponding umh.
> Just like if the kernel module does kmalloc() it should kfree() it in the 
> exit code.
> 
> Signed-off-by: Alexei Starovoitov 
> ---
>  fs/exec.c   |  38 ---
>  include/linux/binfmts.h |   1 +
>  include/linux/umh.h |  12 
>  kernel/umh.c| 176 
> +++-
>  4 files changed, 215 insertions(+), 12 deletions(-)
> 
> diff --git a/fs/exec.c b/fs/exec.c
> index 183059c427b9..30a36c2a39bf 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1706,14 +1706,13 @@ static int exec_binprm(struct linux_binprm *bprm)
>  /*
>   * sys_execve() executes a new program.
>   */
> -static int do_execveat_common(int fd, struct filename *filename,
> -   struct user_arg_ptr argv,
> -   struct user_arg_ptr envp,
> -   int flags)
> +static int __do_execve_file(int fd, struct filename *filename,
> + struct user_arg_ptr argv,
> + struct user_arg_ptr envp,
> + int flags, struct file *file)
>  {
>   char *pathbuf = NULL;
>   struct linux_binprm *bprm;
> - struct file *file;
>   struct files_struct *displaced;
>   int retval;

Keeping in mind a fuzzer...

Note, right below this, and not shown here in the hunk, is:

if (IS_ERR(filename))   
return PTR_ERR(filename)
>  
> @@ -1752,7 +1751,8 @@ static int do_execveat_common(int fd, struct filename 
> *filename,
>   check_unsafe_exec(bprm);
>   current->in_execve = 1;
>  
> - file = do_open_execat(fd, filename, flags);
> + if (!file)
> + file = do_open_execat(fd, filename, flags);


Here we now seem to allow !file and open the file with the passed fd as in
the old days. This is an expected change.

>   retval = PTR_ERR(file);
>   if (IS_ERR(file))
>   goto 

[PATCH v5 1/6] firmware: wrap FW_OPT_* into an enum

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This should let us associate enum kdoc to these values.
While at it, kdocify the fw_opt.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: coding style fixes, merge kdoc with enum move]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 12 
 drivers/base/firmware_loader/fallback.h |  6 ++--
 drivers/base/firmware_loader/firmware.h | 37 +++--
 drivers/base/firmware_loader/main.c |  6 ++--
 4 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 358354148dec..b57a7b3b4122 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -512,7 +512,7 @@ static const struct attribute_group *fw_dev_attr_groups[] = 
{
 
 static struct fw_sysfs *
 fw_create_instance(struct firmware *firmware, const char *fw_name,
-  struct device *device, unsigned int opt_flags)
+  struct device *device, enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
struct device *f_dev;
@@ -545,7 +545,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
  * 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)
+ enum fw_opt opt_flags, long timeout)
 {
int retval = 0;
struct device *f_dev = _sysfs->dev;
@@ -599,7 +599,7 @@ static int fw_load_sysfs_fallback(struct fw_sysfs *fw_sysfs,
 
 static int fw_load_from_user_helper(struct firmware *firmware,
const char *name, struct device *device,
-   unsigned int opt_flags)
+   enum fw_opt opt_flags)
 {
struct fw_sysfs *fw_sysfs;
long timeout;
@@ -640,7 +640,7 @@ static int fw_load_from_user_helper(struct firmware 
*firmware,
return ret;
 }
 
-static bool fw_force_sysfs_fallback(unsigned int opt_flags)
+static bool fw_force_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.force_sysfs_fallback)
return true;
@@ -649,7 +649,7 @@ static bool fw_force_sysfs_fallback(unsigned int opt_flags)
return true;
 }
 
-static bool fw_run_sysfs_fallback(unsigned int opt_flags)
+static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
 {
if (fw_fallback_config.ignore_sysfs_fallback) {
pr_info_once("Ignoring firmware sysfs fallback due to sysctl 
knob\n");
@@ -664,7 +664,7 @@ static bool fw_run_sysfs_fallback(unsigned int opt_flags)
 
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index f8255670a663..a3b73a09db6c 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -5,6 +5,8 @@
 #include 
 #include 
 
+#include "firmware.h"
+
 /**
  * struct firmware_fallback_config - firmware fallback configuration settings
  *
@@ -31,7 +33,7 @@ struct firmware_fallback_config {
 #ifdef CONFIG_FW_LOADER_USER_HELPER
 int fw_sysfs_fallback(struct firmware *fw, const char *name,
  struct device *device,
- unsigned int opt_flags,
+ enum fw_opt opt_flags,
  int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
@@ -43,7 +45,7 @@ void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
 static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
struct device *device,
-   unsigned int opt_flags,
+   enum fw_opt opt_flags,
int ret)
 {
/* Keep carrying over the same error */
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 64acbb1a392c..4f433b447367 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -2,6 +2,7 @@
 #ifndef __FIRMWARE_LOADER_H
 #define __FIRMWARE_LOADER_H
 
+#include 
 #include 
 #include 
 #include 
@@ -10,13 +11,33 @@
 
 #include 
 
-/* firmware behavior options */
-#define FW_OPT_UEVENT  (1U << 0)
-#define FW_OPT_NOWAIT  (1U << 1)
-#define FW_OPT_USERHELPER  (1U << 2)
-#define

[PATCH v5 0/6] firmware_loader: cleanups for v4.18

[Luis R. Rodriguez]

Greg,

I've reviewed the pending patches for the firmware_laoder and as for
v4.18, the following 3 patches from Andres have been iterated enough
that they're ready after I made some final minor changes, mostly just
style fixes and re-arrangements in terms of order. The new API he was
suggesting to add requires just a bit more review.

The last 3 patches are my own and are new, so I'd like further review
from others on them, but considering the changes Hans de Goede is having
us consider, I think this will prove useful to his work in terms of
splitting up code and documenting things properly. One thing that was
clear -- is our Kconfig entries for FW_LOADER were *extremely* outdated,
as such I've gone ahead and updated them.

The kconfig wording update for FW_LOADER includes prior conclusions made
to help justify keeping the split of the firmware fallback sysfs
interface in terms of size which was discussed with Josh Triplett a
while ago. It also includes modern recommendations, which would otherwise
get lost, on what to do about corner case firmware situations on
provisioning situations which folks have brought to my attention before
and architectural solutions based on firmwared [0] for a few years now.
Finally this work also reveals that a couple of candidate drivers could
likely move to staging considering their age, *or* we could just remove
the respective firmware build options. SCSI folks? Networking folks? To
my surprise *nothing* has been done about PREVENT_FIRMWARE_BUILD for
them since pre-git days!  These sneaky litte buggers are:

  * CONFIG_WANXL --> CONFIG_WANXL_BUILD_FIRMWARE
  * CONFIG_SCSI_AIC79XX --> CONFIG_AIC79XX_BUILD_FIRMWARE

To this day both of these drivers are building driver *firmwares* when
the option CONFIG_PREVENT_FIRMWARE_BUILD is disabled, and they don't
even make use of the firmware API at all. I find it *highly unlikely*
pre-git day drivers are raging in new radical firmware updates these
days. I'll go put a knife into some of that unless I hear back from
SCSI or networking folks that WANXL_BUILD_FIRMWARE and
AIC79XX_BUILD_FIRMWARE are still hip and very much needed.

On my radar as well are Mimi's latest firmware_loader proposed changes,
but I think those need considerable review and attention from more security
folks, Android folks, and the linux-wireless community, our own
scattered random folks of firmware reviewer folks.

These patches are based on top of linux-next next-20180504, they are
also available in a respective git branch, both for linux-next [1] and
linux [2].

Question, and specially rants are greatly appreciated, and of course...
may the 4th be with you.

[0] https://github.com/teg/firmwared
[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux-next.git/log/?h=20180504-firmware_loader
[2] 
https://git.kernel.org/pub/scm/linux/kernel/git/mcgrof/linux.git/log/?h=20180504-firmware_loader

  Luis

Andres Rodriguez (3):
  firmware: wrap FW_OPT_* into an enum
  firmware: use () to terminate kernel-doc function names
  firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

Luis R. Rodriguez (3):
  firmware_loader: document firmware_sysfs_fallback()
  firmware_loader: enhance Kconfig documentation over FW_LOADER
  firmware_loader: move kconfig FW_LOADER entries to its own file

 drivers/base/Kconfig|  90 +++---
 drivers/base/firmware_loader/Kconfig| 149 
 drivers/base/firmware_loader/fallback.c |  46 +---
 drivers/base/firmware_loader/fallback.h |  18 +--
 drivers/base/firmware_loader/firmware.h |  37 --
 drivers/base/firmware_loader/main.c |  28 ++---
 6 files changed, 252 insertions(+), 116 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

-- 
2.17.0

[PATCH v5 3/6] firmware: rename fw_sysfs_fallback to firmware_fallback_sysfs()

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

This is done since this call is now exposed through kernel-doc,
and since this also paves the way for different future types of
fallback mechanims.

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
[mcgrof: small coding style changes]
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/fallback.h | 16 
 drivers/base/firmware_loader/firmware.h |  2 +-
 drivers/base/firmware_loader/main.c |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 529f7013616f..3db9e0f225ac 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,10 +662,10 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret)
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret)
 {
if (!fw_run_sysfs_fallback(opt_flags))
return ret;
diff --git a/drivers/base/firmware_loader/fallback.h 
b/drivers/base/firmware_loader/fallback.h
index a3b73a09db6c..21063503e4ea 100644
--- a/drivers/base/firmware_loader/fallback.h
+++ b/drivers/base/firmware_loader/fallback.h
@@ -31,10 +31,10 @@ struct firmware_fallback_config {
 };
 
 #ifdef CONFIG_FW_LOADER_USER_HELPER
-int fw_sysfs_fallback(struct firmware *fw, const char *name,
- struct device *device,
- enum fw_opt opt_flags,
- int ret);
+int firmware_fallback_sysfs(struct firmware *fw, const char *name,
+   struct device *device,
+   enum fw_opt opt_flags,
+   int ret);
 void kill_pending_fw_fallback_reqs(bool only_kill_custom);
 
 void fw_fallback_set_cache_timeout(void);
@@ -43,10 +43,10 @@ void fw_fallback_set_default_timeout(void);
 int register_sysfs_loader(void);
 void unregister_sysfs_loader(void);
 #else /* CONFIG_FW_LOADER_USER_HELPER */
-static inline int fw_sysfs_fallback(struct firmware *fw, const char *name,
-   struct device *device,
-   enum fw_opt opt_flags,
-   int ret)
+static inline int firmware_fallback_sysfs(struct firmware *fw, const char 
*name,
+ struct device *device,
+ enum fw_opt opt_flags,
+ int ret)
 {
/* Keep carrying over the same error */
return ret;
diff --git a/drivers/base/firmware_loader/firmware.h 
b/drivers/base/firmware_loader/firmware.h
index 4f433b447367..4c1395f8e7ed 100644
--- a/drivers/base/firmware_loader/firmware.h
+++ b/drivers/base/firmware_loader/firmware.h
@@ -20,7 +20,7 @@
  * @FW_OPT_NOWAIT: Used to describe the firmware request is asynchronous.
  * @FW_OPT_USERHELPER: Enable the fallback mechanism, in case the direct
  * filesystem lookup fails at finding the firmware.  For details refer to
- * fw_sysfs_fallback().
+ * firmware_fallback_sysfs().
  * @FW_OPT_NO_WARN: Quiet, avoid printing warning messages.
  * @FW_OPT_NOCACHE: Disables firmware caching. Firmware caching is used to
  * cache the firmware upon suspend, so that upon resume races against the
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 7f2bc7e8e3c0..d951af29017a 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -581,7 +581,7 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
dev_warn(device,
 "Direct firmware load for %s failed with error 
%d\n",
 name, ret);
-   ret = fw_sysfs_fallback(fw, name, device, opt_flags, ret);
+   ret = firmware_fallback_sysfs(fw, name, device, opt_flags, ret);
} else
ret = assign_fw(fw, device, opt_flags);
 
-- 
2.17.0

[PATCH v5 2/6] firmware: use () to terminate kernel-doc function names

[Luis R. Rodriguez]

From: Andres Rodriguez <andre...@gmail.com>

The kernel-doc spec dictates a function name ends in ().

Signed-off-by: Andres Rodriguez <andre...@gmail.com>
Acked-by: Randy Dunlap <rdun...@infradead.org>
Acked-by: Luis R. Rodriguez <mcg...@kernel.org>
Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c |  8 
 drivers/base/firmware_loader/main.c | 20 ++--
 2 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index b57a7b3b4122..529f7013616f 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -125,7 +125,7 @@ static ssize_t timeout_show(struct class *class, struct 
class_attribute *attr,
 }
 
 /**
- * firmware_timeout_store - set number of seconds to wait for firmware
+ * firmware_timeout_store() - set number of seconds to wait for firmware
  * @class: device class pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for timeout value
@@ -239,7 +239,7 @@ static int map_fw_priv_pages(struct fw_priv *fw_priv)
 }
 
 /**
- * firmware_loading_store - set value in the 'loading' control file
+ * firmware_loading_store() - set value in the 'loading' control file
  * @dev: device pointer
  * @attr: device attribute pointer
  * @buf: buffer to scan for loading control value
@@ -431,7 +431,7 @@ static int fw_realloc_pages(struct fw_sysfs *fw_sysfs, int 
min_size)
 }
 
 /**
- * firmware_data_write - write method for firmware
+ * firmware_data_write() - write method for firmware
  * @filp: open sysfs file
  * @kobj: kobject for the device
  * @bin_attr: bin_attr structure
@@ -537,7 +537,7 @@ fw_create_instance(struct firmware *firmware, const char 
*fw_name,
 }
 
 /**
- * fw_load_sysfs_fallback - load a firmware via the sysfs fallback mechanism
+ * fw_load_sysfs_fallback() - load a firmware via the sysfs fallback mechanism
  * @fw_sysfs: firmware sysfs information for the firmware to load
  * @opt_flags: flags of options, FW_OPT_*
  * @timeout: timeout to wait for the load
diff --git a/drivers/base/firmware_loader/main.c 
b/drivers/base/firmware_loader/main.c
index 9919f0e6a7cc..7f2bc7e8e3c0 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -597,7 +597,7 @@ _request_firmware(const struct firmware **firmware_p, const 
char *name,
 }
 
 /**
- * request_firmware: - send firmware request and wait for it
+ * request_firmware() - send firmware request and wait for it
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded
@@ -657,7 +657,7 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 EXPORT_SYMBOL_GPL(request_firmware_direct);
 
 /**
- * firmware_request_cache: - cache firmware for suspend so resume can use it
+ * firmware_request_cache() - cache firmware for suspend so resume can use it
  * @name: name of firmware file
  * @device: device for which firmware should be cached for
  *
@@ -681,7 +681,7 @@ int firmware_request_cache(struct device *device, const 
char *name)
 EXPORT_SYMBOL_GPL(firmware_request_cache);
 
 /**
- * request_firmware_into_buf - load firmware into a previously allocated buffer
+ * request_firmware_into_buf() - load firmware into a previously allocated 
buffer
  * @firmware_p: pointer to firmware image
  * @name: name of firmware file
  * @device: device for which firmware is being loaded and DMA region allocated
@@ -713,7 +713,7 @@ request_firmware_into_buf(const struct firmware 
**firmware_p, const char *name,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
- * release_firmware: - release the resource associated with a firmware image
+ * release_firmware() - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
 void release_firmware(const struct firmware *fw)
@@ -755,7 +755,7 @@ static void request_firmware_work_func(struct work_struct 
*work)
 }
 
 /**
- * request_firmware_nowait - asynchronous version of request_firmware
+ * request_firmware_nowait() - asynchronous version of request_firmware
  * @module: module requesting the firmware
  * @uevent: sends uevent to copy the firmware image if this flag
  * is non-zero else the firmware copy must be done manually.
@@ -824,7 +824,7 @@ EXPORT_SYMBOL(request_firmware_nowait);
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
 /**
- * cache_firmware - cache one firmware image in kernel memory space
+ * cache_firmware() - cache one firmware image in kernel memory space
  * @fw_name: the firmware image name
  *
  * Cache firmware in kernel memory so that drivers can use it when
@@ -866,7 +866,7 @@ static struct fw_priv *lookup_fw_priv(const char *fw_name)
 }
 
 /**
- * uncache_firmware - remove one cached firmware image
+ * uncache_firmware() - remove one cached firmware image
  * @fw_name: the firmware 

[PATCH v5 4/6] firmware_loader: document firmware_sysfs_fallback()

[Luis R. Rodriguez]

This also sets the expecations for future fallback interfaces, even
if they are not exported.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/firmware_loader/fallback.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/base/firmware_loader/fallback.c 
b/drivers/base/firmware_loader/fallback.c
index 3db9e0f225ac..9169e7b9800c 100644
--- a/drivers/base/firmware_loader/fallback.c
+++ b/drivers/base/firmware_loader/fallback.c
@@ -662,6 +662,26 @@ static bool fw_run_sysfs_fallback(enum fw_opt opt_flags)
return fw_force_sysfs_fallback(opt_flags);
 }
 
+/**
+ * firmware_fallback_sysfs() - use the fallback mechanism to find firmware
+ * @fw: pointer to firmware image
+ * @name: name of firmware file to look for
+ * @device: device for which firmware is being loaded
+ * @opt_flags: options to control firmware loading behaviour
+ * @ret: return value from direct lookup which triggered the fallback mechanism
+ *
+ * This function is called if direct lookup for the firmware failed, it enables
+ * a fallback mechanism through userspace by exposing a sysfs loading
+ * interface. Userspace is in charge of loading the firmware through the syfs
+ * loading interface. This syfs fallback mechanism may be disabled completely
+ * on a system by setting the proc sysctl value ignore_sysfs_fallback to true.
+ * If this false we check if the internal API caller set the @FW_OPT_NOFALLBACK
+ * flag, if so it would also disable the fallback mechanism. A system may want
+ * to enfoce the sysfs fallback mechanism at all times, it can do this by
+ * setting ignore_sysfs_fallback to false and force_sysfs_fallback to true.
+ * Enabling force_sysfs_fallback is functionally equivalent to build a kernel
+ * with CONFIG_FW_LOADER_USER_HELPER_FALLBACK.
+ **/
 int firmware_fallback_sysfs(struct firmware *fw, const char *name,
struct device *device,
enum fw_opt opt_flags,
-- 
2.17.0

[PATCH v5 6/6] firmware_loader: move kconfig FW_LOADER entries to its own file

[Luis R. Rodriguez]

This will make it easier to track and easier to understand
what components and features are part of the FW_LOADER. There
are some components related to firmware which have *nothing* to
do with the FW_LOADER, souch as PREVENT_FIRMWARE_BUILD.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 150 +--
 drivers/base/firmware_loader/Kconfig | 149 ++
 2 files changed, 150 insertions(+), 149 deletions(-)
 create mode 100644 drivers/base/firmware_loader/Kconfig

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index bf2d464b0e2c..06d6e27784fa 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -88,155 +88,7 @@ config PREVENT_FIRMWARE_BUILD
o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
 
-menu "Firmware loader"
-
-config FW_LOADER
-   tristate "Firmware loading facility" if EXPERT
-   default y
-   ---help---
- This enables the firmware loading facility in the kernel. The kernel
- will first look for built-in firmware, if it has any. Next, it will
- look for the requested firmware in a series of filesystem paths:
-
-   o firmware_class path module parameter or kernel boot param
-   o /lib/firmware/updates/UTS_RELEASE
-   o /lib/firmware/updates
-   o /lib/firmware/UTS_RELEASE
-   o /lib/firmware
-
- Enabling this feature only increases your kernel image by about
- 828 bytes, enable this option unless you are certain you don't
- need firmware.
-
- You typically want this built-in (=y) but you can also enable this
- as a module, in which case the firmware_class module will be built.
- You also want to be sure to enable this built-in if you are going to
- enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
-
-if FW_LOADER
-
-config EXTRA_FIRMWARE
-   string "Build these firmware blobs into the kernel binary"
-   help
- Device drivers which require firmware can typically deal with
- having the kernel load firmware from the various supported
- /lib/firmware/ paths. This option enables you to build into the
- kernel firmware files. Built-in firmware searches are preceeded
- over firmware lookups using your filesystem over the supported
- /lib/firmware paths documented on CONFIG_FW_LOADER.
-
- This may be useful for testing or if the firmware is required early on
- in boot and cannot rely on the firmware being placed in an initrd or
- initramfs.
-
- This option is a string and takes the (space-separated) names of the
- firmware files -- the same names that appear in MODULE_FIRMWARE()
- and request_firmware() in the source. These files should exist under
- the directory specified by the EXTRA_FIRMWARE_DIR option, which is
- /lib/firmware by default.
-
- For example, you might set CONFIG_EXTRA_FIRMWARE="usb8388.bin", copy
- the usb8388.bin file into /lib/firmware, and build the kernel. Then
- any request_firmware("usb8388.bin") will be satisfied internally
- inside the kernel without ever looking at your filesystem at runtime.
-
- WARNING: If you include additional firmware files into your binary
- kernel image that are not available under the terms of the GPL,
- then it may be a violation of the GPL to distribute the resulting
- image since it combines both GPL and non-GPL work. You should
- consult a lawyer of your own before distributing such an image.
-
-config EXTRA_FIRMWARE_DIR
-   string "Firmware blobs root directory"
-   depends on EXTRA_FIRMWARE != ""
-   default "/lib/firmware"
-   help
- This option controls the directory in which the kernel build system
- looks for the firmware files listed in the EXTRA_FIRMWARE option.
-
-config FW_LOADER_USER_HELPER
-   bool "Enable the firmware sysfs fallback mechanism"
-   help
- This option enables a sysfs loading facility to enable firmware
- loading to the kernel through userspace as a fallback mechanism
- if and only if the kernel's direct filesystem lookup for the
- firmware failed using the different /lib/firmware/ paths, or the
- path specified in the firmware_class path module parameter, or the
- firmware_class path kernel boot parameter if the firmware_class is
- built-in. For details on how to work with the sysfs fallback mechanism
- refer to Documentation/driver-api/firmware/fallback-mechanisms.rst.
-
- The direct filesystem lookup for firwmare is always used first now.
-
- If the kernel's direct filesystem loo

[PATCH v5 5/6] firmware_loader: enhance Kconfig documentation over FW_LOADER

[Luis R. Rodriguez]

If you try to read FW_LOADER today it speaks of old riddles and
unless you have been following development closely you will loose
track of what is what. Even the documentation for PREVENT_FIRMWARE_BUILD
is a bit fuzzy and how it fits into this big picture.

Give the FW_LOADER kconfig documentation some love with more up to
date developments and recommendations. While at it, wrap the FW_LOADER
code into its own menu to compartamentalize and make it clearer which
components really are part of the FW_LOADER. This should also make
it easier to later move these kconfig entries into the firmware_loader/
directory later.

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---
 drivers/base/Kconfig | 160 ++-
 1 file changed, 126 insertions(+), 34 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 29b0eb452b3a..bf2d464b0e2c 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -70,39 +70,64 @@ config STANDALONE
  If unsure, say Y.
 
 config PREVENT_FIRMWARE_BUILD
-   bool "Prevent firmware from being built"
+   bool "Disable drivers features which enable custom firmware building"
default y
help
- Say yes to avoid building firmware. Firmware is usually shipped
- with the driver and only when updating the firmware should a
- rebuild be made.
- If unsure, say Y here.
+ Say yes to disable driver features which enable building a custom
+ driver firmwar at kernel build time. These drivers do not use the
+ kernel firmware API to load firmware (CONFIG_FW_LOADER), instead they
+ use their own custom loading mechanism. The required firmware is
+ usually shipped with the driver, building the driver firmware
+ should only be needed if you have an updated firmware source.
+
+ Firmware should not be being built as part of kernel, these days
+ you should always prevent this and say Y here. There are only two
+ old drivers which enable building of its firmware at kernel build
+ time:
+
+   o CONFIG_WANXL through CONFIG_WANXL_BUILD_FIRMWARE
+   o CONFIG_SCSI_AIC79XX through CONFIG_AIC79XX_BUILD_FIRMWARE
+
+menu "Firmware loader"
 
 config FW_LOADER
-   tristate "Userspace firmware loading support" if EXPERT
+   tristate "Firmware loading facility" if EXPERT
default y
---help---
- This option is provided for the case where none of the in-tree modules
- require userspace firmware loading support, but a module built
- out-of-tree does.
+ This enables the firmware loading facility in the kernel. The kernel
+ will first look for built-in firmware, if it has any. Next, it will
+ look for the requested firmware in a series of filesystem paths:
+
+   o firmware_class path module parameter or kernel boot param
+   o /lib/firmware/updates/UTS_RELEASE
+   o /lib/firmware/updates
+   o /lib/firmware/UTS_RELEASE
+   o /lib/firmware
+
+ Enabling this feature only increases your kernel image by about
+ 828 bytes, enable this option unless you are certain you don't
+ need firmware.
+
+ You typically want this built-in (=y) but you can also enable this
+ as a module, in which case the firmware_class module will be built.
+ You also want to be sure to enable this built-in if you are going to
+ enable built-in firmware (CONFIG_EXTRA_FIRMWARE).
+
+if FW_LOADER
 
 config EXTRA_FIRMWARE
-   string "External firmware blobs to build into the kernel binary"
-   depends on FW_LOADER
+   string "Build these firmware blobs into the kernel binary"
help
- Various drivers in the kernel source tree may require firmware,
- which is generally available in your distribution's linux-firmware
- package.
+ Device drivers which require firmware can typically deal with
+ having the kernel load firmware from the various supported
+ /lib/firmware/ paths. This option enables you to build into the
+ kernel firmware files. Built-in firmware searches are preceeded
+ over firmware lookups using your filesystem over the supported
+ /lib/firmware paths documented on CONFIG_FW_LOADER.
 
- The linux-firmware package should install firmware into
- /lib/firmware/ on your system, so they can be loaded by userspace
- helpers on request.
-
- This option allows firmware to be built into the kernel for the case
- where the user either cannot or doesn't want to provide it from
- userspace at runtime (for example, when the firmware in question is
- required for accessing the boot device, and the user doesn't want to
- use an initrd).
+

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

[Luis R. Rodriguez]

On Thu, Mar 22, 2018 at 3:15 PM, Andy Lutomirski  wrote:
>  All we need to do is to make sure that, if this is
> distributed as a module, that it's init routine doesn't wait for a
> long time, right?

Yeap.

 Luis

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

[Luis R. Rodriguez]

On Sat, Mar 10, 2018 at 03:16:52PM +, Luis R. Rodriguez wrote:
> On Sat, Mar 10, 2018 at 02:08:43PM +0000, Luis R. Rodriguez wrote:
> > The alternative to this would be a simple equivalent of 
> > try_then_request_module()
> > for UMH modules: try_umhm_then_request_umh_module() or whatever. So just as 
> > I
> > argued earlier over UMH limitations, this is not the end of the world for 
> > umh
> > modules, and it doesn't mean you can't get *properly* add umh modules 
> > upstream,
> > it would *just mean* we'd be perpetuating today's (IMHO) horrible and loose
> > semantics.
> 
> I was about to suggest that perhaps a try_umhm_then_request_umh_module() or
> whatever should not be a macro -- but instead an actual routine, and we don't
> export say the simple form to avoid non-deterministic uses of it from the
> start... but the thing is *it'd have to be a macro* given that the *check* for
> the module *has to be loose*, just as try_then_request_module()...
> 
> *Ugh* gross.
> 
> Another reason for me to want an actual deterministic clean proper solution
> from the start.

I just thought of another consideration which should be made here for the long
term.

Some init systems have a timeout for kmod workers, that is the userspace
process which issues the modprobe call.

That was very well intentioned, however it ended up being nonsense, so at least
on SLE systemd we disable the timeout for kmod workers.  What others do... is
unclear to me.  Upstream wise the timeout was increased considerably, however,
*if* such timeout is in effect for users it has some implicit implications on
the number of devices a driver could support:

number_devices =  systemd_timeout   
  - 
  max known probe time for driver  

I've documented the logic to these conclusions [0].

It sounds like we *do* want a full sync wait mechanism, and as I noted I think
we should fix the determinism aspect of it. Since no aliases will be supported
for usermode modules this will be much easier to support, and I can volunteer
to help with that.

However given the above... if we're going to use request_module() API (or a
really fixed deterministic version of it later) for usermode kernel modules,
the limitation above still applies.

Are these usermode modules doing all the handy work on init? Or can it be
deferred once loaded? How much loading on init should a usermode module need?

If we can ensure that these usermode modules don't take *any time at all* on
their init *from the start*, it would be wonderful and we'd end up avoiding
some really odd corner case issues later.

[0] http://www.do-not-panic.com/2015/12/linux-asynchronous-probe.html

  Luis

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

[Luis R. Rodriguez]

On Sat, Mar 10, 2018 at 02:08:43PM +, Luis R. Rodriguez wrote:
> The alternative to this would be a simple equivalent of 
> try_then_request_module()
> for UMH modules: try_umhm_then_request_umh_module() or whatever. So just as I
> argued earlier over UMH limitations, this is not the end of the world for umh
> modules, and it doesn't mean you can't get *properly* add umh modules 
> upstream,
> it would *just mean* we'd be perpetuating today's (IMHO) horrible and loose
> semantics.

I was about to suggest that perhaps a try_umhm_then_request_umh_module() or
whatever should not be a macro -- but instead an actual routine, and we don't
export say the simple form to avoid non-deterministic uses of it from the
start... but the thing is *it'd have to be a macro* given that the *check* for
the module *has to be loose*, just as try_then_request_module()...

*Ugh* gross.

Another reason for me to want an actual deterministic clean proper solution
from the start.

  Luis

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

[Luis R. Rodriguez]

Also,

Alexei you never answered my questions out aliases with the umh modules.
Long term this important to consider.

  Luis

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 Thu, Mar 08, 2018 at 03:07:01PM -0800, Alexei Starovoitov wrote:
> On 3/7/18 5:23 PM, Luis R. Rodriguez wrote:
> > 
> > request_module() has its own world though too. How often in your proof of
> > concept is request_module() called? How many times do you envision it being
> > called?
> 
> once.

What about other users later in the kernel?

> > > +static int run_umh(struct file *file)
> > > +{
> > > + struct subprocess_info *sub_info = call_usermodehelper_setup_file(file);
> > > +
> > > + if (!sub_info)
> > > + return -ENOMEM;
> > > + return call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
> > > +}
> > 
> > run_umh() calls the program and waits. Note that while we are running a UMH 
> > we
> > can't suspend. What if they take forever, who is hosing them down with an
> > equivalent:
> > 
> > schedule();
> > try_to_freeze();
> > 
> > Say they are buggy and never return, will they stall suspend, have you 
> > tried?
> > 
> > call_usermodehelper_exec() uses helper_lock() which in turn is used for
> > umh's accounting for number of running umh's. One of the sad obscure uses
> > for this is to deal with suspend/resume. Refer to __usermodehelper* calls
> > on kernel/power/process.c
> > 
> > Note how you use UMH_WAIT_EXEC too, so this is all synchronous.
> 
> looks like you misread this code
>
> #define UMH_NO_WAIT 0   /* don't wait at all */
> #define UMH_WAIT_EXEC   1   /* wait for the exec, but not the process */
> #define UMH_WAIT_PROC   2   /* wait for the process to complete */
> #define UMH_KILLABLE4   /* wait for EXEC/PROC killable */

I certainly did get the incorrect impression this was a sync op, sorry
about that. In that case its odd to see a request_module() call, when
what is really meant is more of a request_module_nowait(), its also
UMH_NO_WAIT on the modprobe call itself as well.

In fact I think I'd much prefer at the very least to see a different wrapper
for this, even if its using the same bolts and nuts underneath for userspace
processes compiled on the kernel. The sanity checks in place for
request_module() may change later and this use case seems rather simple and
limited. Keeping tabs on this type of users seems desirable. At the *very
least*

diff --git a/include/linux/kmod.h b/include/linux/kmod.h
index 40c89ad4bea6..7530e00da03b 100644
--- a/include/linux/kmod.h
+++ b/include/linux/kmod.h
@@ -37,11 +37,13 @@ extern __printf(2, 3)
 int __request_module(bool wait, const char *name, ...);
 #define request_module(mod...) __request_module(true, mod)
 #define request_module_nowait(mod...) __request_module(false, mod)
+#define request_umh_proc(mod...) __request_module(false, mod)
 #define try_then_request_module(x, mod...) \
((x) ?: (__request_module(true, mod), (x)))
 #else
 static inline int request_module(const char *name, ...) { return -ENOSYS; }
 static inline int request_module_nowait(const char *name, ...) { return 
-ENOSYS; }
+static inline int request_umh_proc(const char *name, ...) { return -ENOSYS; }
 #define try_then_request_module(x, mod...) (x)
 #endif

Modulo, kernel/umh.c is already its own thing, so pick a name that helps
identify these things clearly.

> and the rest of your concerns with suspend/resume are not applicable any
> more.

Agreed, except it does still mean these userspace processes are limited to
execution only the kernel is up, and not going to suspend, but I think that's
a proper sanity check by the umh API.

> bpftiler.ko is started once and runs non-stop from there on.
> Unless it crashes, but it's a different discussion.

Sure.

  Luis

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

[Luis R. Rodriguez]

On Mon, Mar 05, 2018 at 05:34:57PM -0800, Alexei Starovoitov wrote:
> As the first step in development of bpfilter project [1]

So meta :) The URL refers an lwn article, which in turn refers to this effort's
first RFC.  As someone only getting *one* of these patches in emails, It would
be useful if the cover letter referenced instead an optional git tree and
branch so one could easily get the patches for more careful inspection. In the
meantime, can you make such tree available with a branch?

Also, since kernel/module.c is involved it would be wise to include Jessica,
which I've Cc'd. I'm going to guess Kees may like to review this too. Mimi
might want to help review as well.

Rafael may care about suspend/resume implications of these "umh modules" as
you put it.

> the request_module()
> code is extended to allow user mode helpers to be invoked. 

Upon inspection you never touch or use request_module() so this is
false and misleading. You don't use or extend request_module() at all, you rely
on extending finit_module() so that load_module() itself will now execute (via
modified do_execve_file()) the same file which was loaded as an module.

This is *very* different given request_module() has its own full magic and is
in and of itself a UMH of the kernel implemented in kernel/kmod.c.

Nevertheless, why?

> Idea is that
> user mode helpers are built as part of the kernel build and installed as
> traditional kernel modules with .ko file extension into distro specified
> location, such that from a distribution point of view, they are no different
> than regular kernel modules.

It sounds like finit_module() module loading is used as a convenience factor
simply to take advantage of being able to ship / maintain/ compile these umh
programs as part of the kernel. Is that right?

So the finit_module() interface was just a convenient mechanism. Is that right?

Ie, if folks had these binaries in place the regular UMH interface / API could 
be
used so that these could be looked for, but instead we want to carry these
in tandem with the kernel?

If so this still seems like an overly complex way to deal with this.

> Thus, allow request_module() logic to load such
> user mode helper (umh) modules via:
> 
>   request_module("foo") ->
> call_umh("modprobe foo") ->
>   sys_finit_module(FD of /lib/modules/.../foo.ko) ->
> call_umh(struct file)

OK so the use case envisioned here was for networking code to do
something like:

if (!loaded) {
err = request_module("bpfilter");
...
}

This is visible on your third patch (this is from your RFC, not this series):

https://www.mail-archive.com/netfilter-devel@vger.kernel.org/msg11129.html

So indeed all this patch does in the end is just putting tons of wrappers in
place so that kernel code can load certain trusted UMH programs we ship, and
maintain in the kernel.

request_module() has its own world though too. How often in your proof of
concept is request_module() called? How many times do you envision it being
called?

Please review lib/test_kmod.c and tools/testing/selftests/kmod/ for testing
your stuff too or consider extending appropriately.

Are aliases something which you expect we'll need to support for these
userspace... modules?

> Such approach enables kernel to delegate functionality traditionally done
> by kernel modules into user space processes (either root or !root) and
> reduces security attack surface of such new code, meaning in case of
> potential bugs only the umh would crash but not the kernel.

Now, this sounds great, however I think that the proof of concept chosen is
pretty complex to start off with. Even if its not designed to be a real
world life use case, a much simpler proof of concept to do something
more simple may be useful, if possible. One wouldn't need to to have it
replace a kernel functionality in real life. lib/ is full of CONFIG_TEST_*
examples, a simple new stupid kernel functionality which can in turn be replaced
with a respective userspace counterpart may be useful, and both kconfig
entries would be mutually exclusive.

> Another
> advantage coming with that would be that bpfilter.ko 

You mean foo.ko

> can be debugged and
> tested out of user space as well (e.g. opening the possibility to run
> all clang sanitizers, fuzzers or test suites for checking translation).

Great too.

> Also, such architecture makes the kernel/user boundary very precise:
> control plane is done by the user space while data plane stays in the kernel.

I don't see how this is defining any boundary, I see just a loader for a
userspace program, and re-using a kernel interface known, finit_module() which
makes it convenient for us to load pre-compiled kernel junk. I'm still
not convinced this is the right approach.

> It's easy to distinguish "umh module" from traditional kernel module:

Ah you said it, "umh module". I don't see what makes it a "umh module" so far,
all we are doing is executing a 

Re: [PATCH] selftest: fix kselftest-merge depend on 'RUNTIME_TESTING_MENU'

[Luis R. Rodriguez]

On Thu, Feb 22, 2018 at 07:53:07PM +0800, Zong Li wrote:
> Since the 'commit d3deafaa8b5c ("lib/: make RUNTIME_TESTS a menuconfig
> to ease disabling it all")', the make kselftest-merge cannot merge the
> config dependencies of kselftest to the existing .config file.
> 
> These config dependencies of kselftest need to enable the
> 'CONFIG_RUNTIME_TESTING_MENU=y' at the same time.
> 
> Signed-off-by: Zong Li 
> Cc: Greentime Hu 

Please add respective Fixes: tag with the sha1sum, and commit name.

  Luis

Re: [PATCH v2 0/6] wl1251: Fix MAC address for Nokia N900

[Luis R. Rodriguez]

On Tue, Jan 02, 2018 at 08:23:45PM +0100, Pali Rohár wrote:
> On Friday 10 November 2017 00:38:22 Pali Rohár wrote:
> > This patch series fix processing MAC address for wl1251 chip found in Nokia 
> > N900.
> > 
> > Changes since v1:
> > * Added Acked-by for Pavel Machek
> > * Fixed grammar
> > * Magic numbers for NVS offsets are replaced by defines
> > * Check for validity of mac address NVS data is moved into function
> > * Changed order of patches as Pavel requested
> > 
> > Pali Rohár (6):
> >   wl1251: Update wl->nvs_len after wl->nvs is valid
> >   wl1251: Generate random MAC address only if driver does not have
> > valid
> >   wl1251: Parse and use MAC address from supplied NVS data
> >   wl1251: Set generated MAC address back to NVS data
> >   firmware: Add request_firmware_prefer_user() function
> >   wl1251: Use request_firmware_prefer_user() for loading NVS
> > calibration data
> > 
> >  drivers/base/firmware_class.c  |   45 +-
> >  drivers/net/wireless/ti/wl1251/Kconfig |1 +
> >  drivers/net/wireless/ti/wl1251/main.c  |  104 
> > ++--
> >  include/linux/firmware.h   |9 +++
> >  4 files changed, 138 insertions(+), 21 deletions(-)
> 
> Hi! Are there any comments for first 4 patches? If not, could they be
> accepted and merged?

Since the first 4 patches do not touch the firmware API they seem fine to me so
long as the maintainer accepts them. Maybe resend and clarify you have dropped
the other ones and amend with the new tags.

  Luis

Re: [PATCH v5 next 2/5] modules:capabilities: add cap_kernel_module_request() permission check

[Luis R. Rodriguez]

On Mon, Nov 27, 2017 at 06:18:35PM +0100, Djalal Harouni wrote:
> +/* Determine whether a module auto-load operation is permitted. */
> +int may_autoload_module(char *kmod_name, int required_cap,
> + const char *kmod_prefix);
> +

While we are reviewing a general LSM for this, it has me wondering if an LSM or
userspace feed info may every want to use other possible context we could add 
for
free to make a determination.

For instance since all request_module() calls are in header files, we could 
for add for free THIS_MODULE as context to may_autoload_module() as well, so
struct module. The LSM could in theory then also help ensure only specific
modules are allowed to request a module load. Perhaps userspace could say
only built-in code could request certain modules.

Just a thought.

  Luis

Re: [PATCH v5 next 3/5] modules:capabilities: automatic module loading restriction

[Luis R. Rodriguez]

On Mon, Nov 27, 2017 at 06:18:36PM +0100, Djalal Harouni wrote:
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 5cbb239..c36aed8 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -261,7 +261,16 @@ struct notifier_block;
>  
>  #ifdef CONFIG_MODULES
>  
> -extern int modules_disabled; /* for sysctl */
> +enum {
> + MODULES_AUTOLOAD_ALLOWED= 0,
> + MODULES_AUTOLOAD_PRIVILEGED = 1,
> + MODULES_AUTOLOAD_DISABLED   = 2,
> +};
> +

Can you kdocify these and add a respective rst doc file?  Maybe stuff your
extensive docs which you are currently adding to
Documentation/sysctl/kernel.txt to this new file and in kernel.txt just refer
to it. This way this can be also nicely visibly documented on the web with the
new sphinx.

This way you can take advantage of the kdocs you are already adding and refer
to them.

> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index 2fb4e27..0b6f0c8 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -683,6 +688,15 @@ static struct ctl_table kern_table[] = {
>   .extra1 = ,
>   .extra2 = ,
>   },
> + {
> + .procname   = "modules_autoload_mode",
> + .data   = _autoload_mode,
> + .maxlen = sizeof(int),
> + .mode   = 0644,
> + .proc_handler   = modules_autoload_dointvec_minmax,

It would seem this is a unint ... so why not reflect that?

> @@ -2499,6 +2513,20 @@ static int proc_dointvec_minmax_sysadmin(struct 
> ctl_table *table, int write,
>  }
>  #endif
>  
> +#ifdef CONFIG_MODULES
> +static int modules_autoload_dointvec_minmax(struct ctl_table *table, int 
> write,
> + void __user *buffer, size_t *lenp, loff_t *ppos)
> +{
> + /*
> +  * Only CAP_SYS_MODULE in init user namespace are allowed to change this
> +  */
> + if (write && !capable(CAP_SYS_MODULE))
> + return -EPERM;
> +
> + return proc_dointvec_minmax(table, write, buffer, lenp, ppos);
> +}
> +#endif

We now have proc_douintvec_minmax().

  Luis

Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap()

[Luis R. Rodriguez]

On Tue, Nov 28, 2017 at 02:18:18PM -0800, Kees Cook wrote:
> On Tue, Nov 28, 2017 at 2:12 PM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > On Tue, Nov 28, 2017 at 01:39:58PM -0800, Kees Cook wrote:
> >> On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcg...@kernel.org> 
> >> wrote:
> >> > And *all* auto-loading uses aliases? What's the difference between 
> >> > auto-loading
> >> > and direct-loading?
> >>
> >> The difference is the process privileges. Unprivilged autoloading
> >> (e.g. int n_hdlc = N_HDLC; ioctl(fd,
> >> TIOCSETD, _hdlc)), triggers a privileged call to finit_module()
> >> under CAP_SYS_MODULE.
> >
> > Ah, so system call implicated request_module() calls.
> 
> Yup. Unprivileged user does something that ultimately hits a
> request_module() in the kernel. Then the kernel calls out with the
> usermode helper (which has CAP_SYS_MODULE) and calls finit_module().

Thanks, using this terminology is much better to understand than auto-loading,
given it does make it clear an unprivileged call was one that initiated the
request_module() call, there are many uses of request_module() which *are*
privileged.

> > OK and since CAP_SYS_MODULE is much more restrictive one could argue, 
> > what's the
> > point here?
> 
> The goal is to block an unprivileged user from being able to trigger a
> module load without blocking root from loading modules directly.

I see now. Do we have an audit of all system calls which implicate a
request_module() call? Networking is a good example for sure to start
off with but I was curious if we have a grasp of how wide spread this
could be.

I'll go review the patches again now with all this in mind.

  Luis

Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap()

[Luis R. Rodriguez]

On Tue, Nov 28, 2017 at 10:33:27PM +0100, Djalal Harouni wrote:
> On Tue, Nov 28, 2017 at 10:16 PM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > On Tue, Nov 28, 2017 at 12:11:34PM -0800, Kees Cook wrote:
> >> On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcg...@kernel.org> 
> >> wrote:
> >> > kmod is just a helper to poke userpsace to load a module, that's it.
> >> >
> >> > The old init_module() and newer finit_module() do the real handy work or
> >> > module loading, and both currently only use may_init_module():
> >> >
> >> > static int may_init_module(void)
> >> > {
> >> > if (!capable(CAP_SYS_MODULE) || modules_disabled)
> >> > return -EPERM;
> >> >
> >> > return 0;
> >> > }
> >> >
> >> > This begs the question:
> >> >
> >> >   o If userspace just tries to just use raw finit_module() do we want 
> >> > similar
> >> > checks?
> >> >
> >> > Otherwise, correct me if I'm wrong this all seems pointless.
> >>
> >> Hm? That's direct-loading, not auto-loading. This series is only about
> >> auto-loading.
> >
> > And *all* auto-loading uses aliases? What's the difference between 
> > auto-loading
> > and direct-loading?
> 
> Not all auto-loading uses aliases, auto-loading is when kernel code
> calls request_module() to loads the feature that was not present, 

It seems the actual interest here is system call implicated request_module()
calls? Because there are uses of request_module() which may be module hacks,
and not implicated via system calls.

> and direct-loading in this thread is the direct syscalls like
> finit_module().

OK.

> >> We already have a global sysctl for blocking direct-loading 
> >> (modules_disabled).
> >
> > My point was that even if you have a CAP_NET_ADMIN check on 
> > request_module(),
> > finit_module() will not check for it, so a crafty userspace could still try
> > to just finit_module() directly, and completely then bypass the 
> > CAP_NET_ADMIN
> > check.
> 
> The finit_module() uses CAP_SYS_MODULE which should allow all modules
> and in this context it should be more privileged than CAP_NET_ADMIN
> which is only for "netdev-%s" (to not load arbitrary modules with it).
> 
> finit_module() coming from request_module() always has the
> CAP_NET_ADMIN, hence the check is done before.

But since CAP_SYS_MODULE is more restrictive, what's the point in checking
for CAP_NET_ADMIN?

> > So unless I'm missing something, I see no point in adding extra checks for
> > request_module() but nothing for the respective load_module().
> 
> I see, request_module() is called from kernel context which runs in
> init namespace will full capabilities, the spawned userspace modprobe
> will get CAP_SYS_MODULE and all other caps, then after comes modprobe
> and load_module().

Right, so defining the gains of adding this extra check is not very clear
yet. It would seem a benefit exists, what is it?

> Btw as suggested by Linus I will update with request_module_cap() and > I can
> offer my help maintaining these bits too.

Can you start by extending lib/test_module.c and
tools/testing/selftests/kmod/kmod.sh with a proof of concept of the gains here,
as well as ensuring things work as expected ?

  Luis

Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap()

[Luis R. Rodriguez]

On Tue, Nov 28, 2017 at 01:39:58PM -0800, Kees Cook wrote:
> On Tue, Nov 28, 2017 at 1:16 PM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > And *all* auto-loading uses aliases? What's the difference between 
> > auto-loading
> > and direct-loading?
> 
> The difference is the process privileges. Unprivilged autoloading
> (e.g. int n_hdlc = N_HDLC; ioctl(fd,
> TIOCSETD, _hdlc)), triggers a privileged call to finit_module()
> under CAP_SYS_MODULE.

Ah, so system call implicated request_module() calls.

> >> We already have a global sysctl for blocking direct-loading 
> >> (modules_disabled).
> >
> > My point was that even if you have a CAP_NET_ADMIN check on 
> > request_module(),
> > finit_module() will not check for it, so a crafty userspace could still try
> > to just finit_module() directly, and completely then bypass the 
> > CAP_NET_ADMIN
> > check.
> 
> You need CAP_SYS_MODULE to run finit_module().

OK and since CAP_SYS_MODULE is much more restrictive one could argue, what's the
point here?

  Luis

Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap()

[Luis R. Rodriguez]

On Tue, Nov 28, 2017 at 12:11:34PM -0800, Kees Cook wrote:
> On Tue, Nov 28, 2017 at 11:14 AM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> > kmod is just a helper to poke userpsace to load a module, that's it.
> >
> > The old init_module() and newer finit_module() do the real handy work or
> > module loading, and both currently only use may_init_module():
> >
> > static int may_init_module(void)
> > {
> > if (!capable(CAP_SYS_MODULE) || modules_disabled)
> > return -EPERM;
> >
> > return 0;
> > }
> >
> > This begs the question:
> >
> >   o If userspace just tries to just use raw finit_module() do we want 
> > similar
> > checks?
> >
> > Otherwise, correct me if I'm wrong this all seems pointless.
> 
> Hm? That's direct-loading, not auto-loading. This series is only about
> auto-loading.

And *all* auto-loading uses aliases? What's the difference between auto-loading
and direct-loading?

> We already have a global sysctl for blocking direct-loading 
> (modules_disabled).

My point was that even if you have a CAP_NET_ADMIN check on request_module(),
finit_module() will not check for it, so a crafty userspace could still try
to just finit_module() directly, and completely then bypass the CAP_NET_ADMIN
check.

So unless I'm missing something, I see no point in adding extra checks for
request_module() but nothing for the respective load_module().

  Luis

Re: [PATCH v5 next 1/5] modules:capabilities: add request_module_cap()

[Luis R. Rodriguez]

On Mon, Nov 27, 2017 at 06:18:34PM +0100, Djalal Harouni wrote:
...

> After a discussion with Rusty Russell [1], the suggestion was to pass
> the capability from request_module() to security_kernel_module_request()
> for 'netdev-%s' modules that need CAP_NET_ADMIN, and after review from
> Kees Cook [2] and experimenting with the code, the patch now does the
> following:
> 
> * Adds the request_module_cap() function.
> * Updates the __request_module() to take the "required_cap" argument
> with the "prefix".

...

> Signed-off-by: Djalal Harouni 
> ---
> diff --git a/kernel/kmod.c b/kernel/kmod.c
> index bc6addd..679d401 100644
> --- a/kernel/kmod.c
> +++ b/kernel/kmod.c
> @@ -139,13 +147,22 @@ int __request_module(bool wait, const char *fmt, ...)
>   if (!modprobe_path[0])
>   return 0;
>  
> + /*
> +  * Lets attach the prefix to the module name
> +  */
> + if (prefix != NULL && *prefix != '\0') {
> + len += snprintf(module_name, MODULE_NAME_LEN, "%s-", prefix);
> + if (len >= MODULE_NAME_LEN)
> + return -ENAMETOOLONG;
> + }
> +
>   va_start(args, fmt);
> - ret = vsnprintf(module_name, MODULE_NAME_LEN, fmt, args);
> + ret = vsnprintf(module_name + len, MODULE_NAME_LEN - len, fmt, args);
>   va_end(args);
> - if (ret >= MODULE_NAME_LEN)
> + if (ret >= MODULE_NAME_LEN - len)
>   return -ENAMETOOLONG;
>  
> - ret = security_kernel_module_request(module_name);
> + ret = security_kernel_module_request(module_name, required_cap, prefix);
>   if (ret)
>   return ret;
>  

kmod is just a helper to poke userpsace to load a module, that's it.

The old init_module() and newer finit_module() do the real handy work or
module loading, and both currently only use may_init_module():

static int may_init_module(void)
{
if (!capable(CAP_SYS_MODULE) || modules_disabled)
return -EPERM;  

return 0;
}

This begs the question:

  o If userspace just tries to just use raw finit_module() do we want similar
checks?

Otherwise, correct me if I'm wrong this all seems pointless.

If we want something similar I think we might need to be processing aliases and
check for the aliases for their desired restrictions on finit_module(),
otherwise userspace can skip through the checks if the module name does not
match the alias prefix.

To be clear, aliases are completely ignored today on load_module(), so loading
'xfs' with finit_module() will just have the kernel know about 'xfs' not
'fs-xfs'.

So we currently do not process aliases in kernel.

I have debugging patches to enable us to process them, but they are just for
debugging and I've been meaning to send them in for review. I designed them
only for debugging given last time someone suggested for aliases processing to
be added, the only use case we found was a pre-optimizations we decided to avoid
pursuing. Debugging is a good reason to have alias processing in-kernel though.

The pre-optimization we decided to stay away from was to check if the requested
module via request_module() was already loaded *and* also check if the name 
passed
matches any of the existing module aliases for currently loaded modules. Today
request_module() does not even check if a requested module is already loaded,
its a stupid loader, it just goes to userspace, and lets userspace figure it
out. Userspace in turn could check for aliases, but it could lie, or not be up
to date to do that.

The pre-optmization is a theoretical gain only then, and if userspace had
proper alias checking it is arguable that it may perform just as equal.
To help valuate these sorts of things we now have:

tools/testing/selftests/kmod/kmod.sh

So further patches can use and test impact with it.

Anyway -- so aliasing is currently only a debugging consideration, but without
processing aliases, all this work seems pointless to me as the real loader is
in finit_module().
 
kmod is just a stupid loader and uses the kernel usermode helper to do work.

  Luis

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 10:08:19PM +0100, Pali Rohár wrote:
> On Friday 10 November 2017 21:26:01 Luis R. Rodriguez wrote:
> > On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> > > This function works pretty much like request_firmware(), but it prefer
> > > usermode helper. If usermode helper fails then it fallback to direct
> > > access. Useful for dynamic or model specific firmware data.
> > > 
> > > Signed-off-by: Pali Rohár <pali.ro...@gmail.com>
> > > ---
> > >  drivers/base/firmware_class.c |   45 
> > > +++--
> > >  include/linux/firmware.h  |9 +
> > >  2 files changed, 52 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 4b57cf5..c3a9fe5 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, 
> > > enum fw_status status)
> > >  #endif
> > >  #define FW_OPT_NO_WARN   (1U << 3)
> > >  #define FW_OPT_NOCACHE   (1U << 4)
> > > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> > > +#define FW_OPT_PREFER_USER   (1U << 5)
> > > +#else
> > > +#define FW_OPT_PREFER_USER   0
> > > +#endif
> > 
> > I've been cleaning these up these flags [0], which I'll shortly respin based
> > on feedback, so this sort of stuff should be avoided at all costs.
> > 
> > Regardless of this even if you *leave* the flag in place and a driver 
> > required
> > this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> > calling fw_load_from_user_helper would just already return -ENOENT, as such 
> > it
> > would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> > needed.
> > 
> > [0] https://lkml.kernel.org/r/20170914225422.31034-1-mcg...@kernel.org
> > 
> > >  struct firmware_cache {
> > >   /* firmware_buf instance will be added into the below list */
> > > @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware 
> > > *fw)
> > >   if (ret <= 0) /* error or already assigned */
> > >   goto out;
> > >  
> > > - ret = fw_get_filesystem_firmware(device, fw->priv);
> > > + if (opt_flags & FW_OPT_PREFER_USER) {
> > > + ret = fw_load_from_user_helper(fw, name, device, opt_flags, 
> > > timeout);
> > > + if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> > > + dev_warn(device,
> > > +  "User helper firmware load for %s failed with 
> > > error %d\n",
> > > +  name, ret);
> > > + dev_warn(device, "Falling back to direct firmware 
> > > load\n");
> > 
> > As I had noted before, the usermode helper was really not well designed,
> > as such extending further use of it is something we should shy away unless 
> > we
> > determine its completely necessary.
> > 
> > So what's wrong with this driver failing at direct access, which should be 
> > fast,
> > and relying on a uevent to then work using the current fallback mechanisms?
> > 
> > The commit log in no way documents any of the justifications for further
> > extending use of the usermode helper.
> 
> Hi! See patch 6/6. It is needed to avoid direct access and wl1251 on
> Nokia N900 needs to use userspace helper which prepares firmware data.

My point is your commit log in no way describes the shortcomings of the
current affairs for device drivers which only can access the data it
needs using the firmware fallback mechanism.

In order for a change to go in, specially if its extending use of the
fallback mechanism through sysfs now as primary citizen, the justification
should be well documented on the commit log.

For instance you may want to highlight that what I documented here:

https://www.kernel.org/doc/html/latest/driver-api/firmware/fallback-mechanisms.html

"CONFIG_FW_LOADER_USER_HELPER: enables building the firmware fallback
mechanism. Most distributions enable this option today. If enabled but
CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled, only the custom fallback
mechanism is available and for the request_firmware_nowait() call."

Since most distros disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK, in practice
this means the fallback mechanism is never actually triggered, and the
only way to do that in practice this for most distros is to u

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 12:26 PM, Luis R. Rodriguez <mcg...@kernel.org> wrote:
> even if you *leave* the flag in place and a driver required
> this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> calling fw_load_from_user_helper would just already return -ENOENT, as such it
> would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> needed.

> The commit log in no way documents any of the justifications for further
> extending use of the usermode helper.

Also, any new functionality introduced should have a respective test
case in tools/testing/selftests/firmware/ and lib/test_firmware.c, as
well as extending existing documentation on
Documentation/driver-api/firmware/.

  Luis

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> This function works pretty much like request_firmware(), but it prefer
> usermode helper. If usermode helper fails then it fallback to direct
> access. Useful for dynamic or model specific firmware data.
> 
> Signed-off-by: Pali Rohár 
> ---
>  drivers/base/firmware_class.c |   45 
> +++--
>  include/linux/firmware.h  |9 +
>  2 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 4b57cf5..c3a9fe5 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum 
> fw_status status)
>  #endif
>  #define FW_OPT_NO_WARN   (1U << 3)
>  #define FW_OPT_NOCACHE   (1U << 4)
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +#define FW_OPT_PREFER_USER   (1U << 5)
> +#else
> +#define FW_OPT_PREFER_USER   0
> +#endif

I've been cleaning these up these flags [0], which I'll shortly respin based
on feedback, so this sort of stuff should be avoided at all costs.

Regardless of this even if you *leave* the flag in place and a driver required
this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
calling fw_load_from_user_helper would just already return -ENOENT, as such it
would in turn fallback to direct fs loading so the #ifef'ery seems to be not
needed.

[0] https://lkml.kernel.org/r/20170914225422.31034-1-mcg...@kernel.org

>  struct firmware_cache {
>   /* firmware_buf instance will be added into the below list */
> @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
>   if (ret <= 0) /* error or already assigned */
>   goto out;
>  
> - ret = fw_get_filesystem_firmware(device, fw->priv);
> + if (opt_flags & FW_OPT_PREFER_USER) {
> + ret = fw_load_from_user_helper(fw, name, device, opt_flags, 
> timeout);
> + if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> + dev_warn(device,
> +  "User helper firmware load for %s failed with 
> error %d\n",
> +  name, ret);
> + dev_warn(device, "Falling back to direct firmware 
> load\n");

As I had noted before, the usermode helper was really not well designed,
as such extending further use of it is something we should shy away unless we
determine its completely necessary.

So what's wrong with this driver failing at direct access, which should be fast,
and relying on a uevent to then work using the current fallback mechanisms?

The commit log in no way documents any of the justifications for further
extending use of the usermode helper.

  Luis

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Fri, Jun 30, 2017 at 11:35:41PM +0200, Arend van Spriel wrote:
> On 23-06-17 23:53, Luis R. Rodriguez wrote:
> > On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote:
> >> On 16-5-2017 1:13, Luis R. Rodriguez wrote:
> >>> Since no upstream delta is needed for firmwared I'd like to first 
> >>> encourage
> >>> evaluating the above. While distributions don't carry it yet that may be 
> >>> seen as
> >>> an issue but since what we are looking for are corner cases, only folks 
> >>> needing
> >>> to deploy a specific solution would need it or a custom proprietary 
> >>> solution.
> >>
> >> Ok. I will go try and run firmwared in OpenWrt on a router platform.
> >> Have to steal one from a colleague :-p Will study firmwared.
> > 
> > Arend, curious how this effort goes. Its important to me as we know then 
> > that
> > if this works its a good approach to recommend moving forward which should 
> > also
> > prove less complex than that soup we had with the custom fallback stuff.
> 
> Hi Luis,
> 
> So I looked at firmwared and we basically need to extend it.

And actually as me and Johannes spoke long ago at Plumbers, its rather
expected folks would need this or just fork it off completely. firmwared
would just be a reference codebase on how to do these custom loaders.

*How regular* Linux distributions embrace this is still up in the air then,
because as Lennart has pointed out there is no plan to merge firmwared
to systemd.

*If* this is a requirement only for non-upstream drivers or patches on
which will not be merged upstream then this will work long term. If you
need regular distros to do something custom then their respective upstream
tools would need to be evaluated for how something similar would be done.

As far as I can tell perhaps the remote-proc thing would count as one
possible use-case for upstream drivers -- but even then the systems
that use it seem likely to use Android and then some custom userspace.

> For our
> router platform we need to obtain nvram calibration data from an MTD
> partition which contains the raw data, ie. no file-system on it. So
> firmwared would need some sort of configuration to map a particular
> firmware file to some action obtaining the data like kicking off some
> mtd-utils in my case.

I see. So platform specific.

Thanks for the update!
 
 Luis

Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function

[Luis R. Rodriguez]

On Thu, Aug 03, 2017 at 05:55:18AM +, Coelho, Luciano wrote:
> On Thu, 2017-08-03 at 08:23 +0300, Kalle Valo wrote:
> > "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> > 
> > > > +int request_firmware_nowait(struct module *module, bool uevent,
> > > > +   const char *name, struct device *device, 
> > > > gfp_t gfp,
> > > > +   void *context,
> > > > +   void (*cont)(const struct firmware *fw, 
> > > > void *context))
> > > > +{
> > > > +   unsigned int opt_flags = FW_OPT_FALLBACK |
> > > > +   (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> > > > +
> > > > +   return __request_firmware_nowait(module, opt_flags, name, 
> > > > device, gfp,
> > > > +context, cont);
> > > > +}
> > > >  EXPORT_SYMBOL(request_firmware_nowait);
> > > >  
> > > > +int __request_firmware_async(struct module *module, const char *name,
> > > > +struct firmware_opts *fw_opts, struct 
> > > > device *dev,
> > > > +void *context,
> > > > +void (*cont)(const struct firmware *fw, 
> > > > void *context))
> > > > +{
> > > > +   unsigned int opt_flags = FW_OPT_UEVENT;
> > > 
> > > This exposes a long issue. Think -- why do we want this enabled by 
> > > default? Its
> > > actually because even though the fallback stuff is optional and can be, 
> > > the uevent
> > > internal flag *also* provides caching support as a side consequence only. 
> > > We
> > > don't want to add a new API without first cleaning up that mess.
> > > 
> > > This is a slipery slope and best to clean that up before adding any new 
> > > API.
> > > 
> > > That and also Greg recently stated he would like to see at least 3 users 
> > > of
> > > a feature before adding it. Although I think that's pretty arbitrary, and
> > > considering that request_firmware_into_buf() only has *one* user -- its 
> > > what
> > > he wishes.
> > 
> > ath10k at least needs a way to silence the warning for missing firmware
> > and I think iwlwifi also.
> 
> Yes, iwlwifi needs to silence the warning.  It the feature (only one,
> really) that I've been waiting for.

Luca,

can you confirm? The API revision thing is one thing but as I noted to
Kalle that can be done using a different API scheme as I had proposed on
the driver data API.

Other than that are there specific firmware requests which are async which
are not API revision style (min...max) for which an async request is optional?

 Luis

Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function

[Luis R. Rodriguez]

On Thu, Aug 03, 2017 at 08:23:00AM +0300, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> 
> >> +int request_firmware_nowait(struct module *module, bool uevent,
> >> +  const char *name, struct device *device, gfp_t gfp,
> >> +  void *context,
> >> +  void (*cont)(const struct firmware *fw, void 
> >> *context))
> >> +{
> >> +  unsigned int opt_flags = FW_OPT_FALLBACK |
> >> +  (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> >> +
> >> +  return __request_firmware_nowait(module, opt_flags, name, device, gfp,
> >> +   context, cont);
> >> +}
> >>  EXPORT_SYMBOL(request_firmware_nowait);
> >>  
> >> +int __request_firmware_async(struct module *module, const char *name,
> >> +   struct firmware_opts *fw_opts, struct device *dev,
> >> +   void *context,
> >> +   void (*cont)(const struct firmware *fw, void 
> >> *context))
> >> +{
> >> +  unsigned int opt_flags = FW_OPT_UEVENT;
> >
> > This exposes a long issue. Think -- why do we want this enabled by default? 
> > Its
> > actually because even though the fallback stuff is optional and can be, the 
> > uevent
> > internal flag *also* provides caching support as a side consequence only. We
> > don't want to add a new API without first cleaning up that mess.
> >
> > This is a slipery slope and best to clean that up before adding any new API.
> >
> > That and also Greg recently stated he would like to see at least 3 users of
> > a feature before adding it. Although I think that's pretty arbitrary, and
> > considering that request_firmware_into_buf() only has *one* user -- its what
> > he wishes.
> 
> ath10k at least needs a way to silence the warning for missing firmware
> and I think iwlwifi also.

It would seem ath10k actually does not need an async version of the no-warn
thing proposed here.

Below an analysis of how ath10k uses the firmware API. Note that I think
that iwlwifi may be on the same boat but the case and issue is slightly
different: both drivers use an API revision scheme, intermediate files
which are not found are not fatal as revisions go from min..max and only
one file is needed. A warning may be needed only if *no* file is found
at all.

I had proposed a new API call to handle this in the driver API series but
had only converted iwlwifi. It would seem ath10k can also be converted to
it and also they could end up sharing the same revision loop scheme so
less code on both drivers.

Below my analysis of how ath10k uses the firmware API:

You noted in private that ath10k long ago addressed the warning issue by
switching to request_firmware_direct() entirely:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9f5bcfe93315d75da4cc46bd30b536966559359a

Below is a list of the file name schemes used for all the different firmwares
types used in ath10k:

1)
/* pre-cal--.bin */
scnprintf(filename, sizeof(filename), "pre-cal-%s-%s.bin", 

2)

/* cal--.bin */
scnprintf(filename, sizeof(filename), "cal-%s-%s.bin",  
  ath10k_bus_str(ar->hif.bus), dev_name(ar->dev)); 


This seems fine as-is if indeed they are optional.

3) ath10k_core_fetch_board_data_api_1():

ar->hw_params.fw.dir/ar->hw_params.fw.board

This seems fine if indeed its optional.

3) Then you have a board file on ath10k_core_fetch_board_data_api_n():

boardname/ATH10K_BOARD_API2_FILE

This seems fine if indeed its optional.

4) Finally you have the actual firmware files which use a revision scheme
in ath10k_core_fetch_firmware_api_n():

for (i = ATH10K_FW_API_MAX; i >= ATH10K_FW_API_MIN; i--) {  
ar->fw_api = i; 
ath10k_dbg(ar, ATH10K_DBG_BOOT, "trying fw api %d\n",   
   ar->fw_api); 

ath10k_core_get_fw_name(ar, fw_name, sizeof(fw_name), 
ar->fw_api);
ret = ath10k_core_fetch_firmware_api_n(ar, fw_name, 
   
>normal_mode_fw.fw_file);
if (!ret)   
goto success;   
}

This ends up using the ar

Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function

[Luis R. Rodriguez]

On Thu, Aug 03, 2017 at 05:55:18AM +, Coelho, Luciano wrote:
> On Thu, 2017-08-03 at 08:23 +0300, Kalle Valo wrote:
> > "Luis R. Rodriguez" <mcg...@kernel.org> writes:
> > 
> > > > +int request_firmware_nowait(struct module *module, bool uevent,
> > > > +   const char *name, struct device *device, 
> > > > gfp_t gfp,
> > > > +   void *context,
> > > > +   void (*cont)(const struct firmware *fw, 
> > > > void *context))
> > > > +{
> > > > +   unsigned int opt_flags = FW_OPT_FALLBACK |
> > > > +   (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> > > > +
> > > > +   return __request_firmware_nowait(module, opt_flags, name, 
> > > > device, gfp,
> > > > +context, cont);
> > > > +}
> > > >  EXPORT_SYMBOL(request_firmware_nowait);
> > > >  
> > > > +int __request_firmware_async(struct module *module, const char *name,
> > > > +struct firmware_opts *fw_opts, struct 
> > > > device *dev,
> > > > +void *context,
> > > > +void (*cont)(const struct firmware *fw, 
> > > > void *context))
> > > > +{
> > > > +   unsigned int opt_flags = FW_OPT_UEVENT;
> > > 
> > > This exposes a long issue. Think -- why do we want this enabled by 
> > > default? Its
> > > actually because even though the fallback stuff is optional and can be, 
> > > the uevent
> > > internal flag *also* provides caching support as a side consequence only. 
> > > We
> > > don't want to add a new API without first cleaning up that mess.
> > > 
> > > This is a slipery slope and best to clean that up before adding any new 
> > > API.
> > > 
> > > That and also Greg recently stated he would like to see at least 3 users 
> > > of
> > > a feature before adding it. Although I think that's pretty arbitrary, and
> > > considering that request_firmware_into_buf() only has *one* user -- its 
> > > what
> > > he wishes.
> > 
> > ath10k at least needs a way to silence the warning for missing firmware
> > and I think iwlwifi also.
> 
> Yes, iwlwifi needs to silence the warning.  It the feature (only one,
> really) that I've been waiting for.

Perfect, can you guys send patches on top of these changes? Even though I still
think the flag stuff needs to be worked out I can try to iron that out myself
and fold the rest of the delta, and then I can set a new set out which is much
more suitable.

  Luis

Re: sysctl, argument parsing, possible bug

[Luis R. Rodriguez]

On Tue, Aug 01, 2017 at 02:54:51PM -0700, Cong Wang wrote:
> On Tue, Aug 1, 2017 at 2:34 PM, Massimo Sala  
> wrote:
> > Do you confirm it is a sysctl parsing bug ?
> >
> > Bosybox handles these cases, so I think also standalone sysctl have to.
> >
> > Or at least someone must update sysctl docs / MAN about this.
> >
> 
> Maybe, sysctl seems (I never look into it) just to interpret dot as
> a separator, unless it reads from the /proc/sys/ directory, it can
> not know eth0.100 is actually a netdev name.

Using echo or sysctl -w works for me, try upgrading your procps,
I have procps-3.3.12. Busybox sysctl fails though, so it would
seems this is an issue with busybox, go fix that.

With procps-3.3.12.:

Setting:

ergon:~ # cat /proc/sys/net/ipv4/conf/eth0.100/forwarding 
1

ergon:~ # sysctl -w net.ipv4.conf.eth0/100.forwarding=0
net.ipv4.conf.eth0/100.forwarding = 0

ergon:~ # cat /proc/sys/net/ipv4/conf/eth0.100/forwarding 
0

Querying:

ergon:~ # sysctl net.ipv4.conf.eth0/100.forwarding
net.ipv4.conf.eth0/100.forwarding = 0

With busybox:

ergon:~ # busybox-static sysctl net.ipv4.conf.eth0/100.forwarding
sysctl: error: 'net.ipv4.conf.eth0.100.forwarding' is an unknown key

  Luis

Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function

[Luis R. Rodriguez]

On Mon, Jul 31, 2017 at 05:09:44PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki 
> 
> So far we got only one function for loading firmware asynchronously:
> request_firmware_nowait. It didn't allow much customization of firmware
> loading process - there is only one bool uevent argument. Moreover this
> bool also controls user helper in an unclear way.

> Some drivers need more flexible helper providing more options. This
> includes control over uevent or warning for the missing firmware. Of
> course this list may grow up in the future.
> 
> To handle this easily this patch adds a generic request_firmware_async
> function. It takes struct with options as an argument which will allow
> extending it in the future without massive changes.
> 
> This is a really cheap change (no new independent API) which works
> nicely with the existing code. The old request_firmware_nowait is kept
> as a simple helper calling new helper.
> 
> Signed-off-by: Rafał Miłecki 
> ---
> V3: Don't expose all FW_OPT_* flags.
> As Luis noted we want a struct so add struct firmware_opts for real
> flexibility.
> Thank you Luis for your review!
> V5: Rebase, update commit message, resend after drvdata discussion
> ---
>  drivers/base/firmware_class.c | 43 
> ++-
>  include/linux/firmware.h  | 15 ++-
>  2 files changed, 48 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index b9f907eedbf7..cde85b05e4cb 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -1375,7 +1375,7 @@ static void request_firmware_work_func(struct 
> work_struct *work)
>   _request_firmware(, fw_work->name, fw_work->device, NULL, 0,
> fw_work->opt_flags);
>   fw_work->cont(fw, fw_work->context);
> - put_device(fw_work->device); /* taken in request_firmware_nowait() */
> + put_device(fw_work->device); /* taken in __request_firmware_nowait() */
>  
>   module_put(fw_work->module);
>   kfree_const(fw_work->name);
> @@ -1383,10 +1383,9 @@ static void request_firmware_work_func(struct 
> work_struct *work)
>  }
>  
>  /**
> - * request_firmware_nowait - asynchronous version of request_firmware
> + * __request_firmware_nowait - asynchronous version of request_firmware
>   * @module: module requesting the firmware
> - * @uevent: sends uevent to copy the firmware image if this flag
> - *   is non-zero else the firmware copy must be done manually.
> + * @opt_flags: flags that control firmware loading process, see FW_OPT_*
>   * @name: name of firmware file
>   * @device: device for which firmware is being loaded
>   * @gfp: allocation flags
> @@ -1405,9 +1404,9 @@ static void request_firmware_work_func(struct 
> work_struct *work)
>   *
>   *   - can't sleep at all if @gfp is GFP_ATOMIC.
>   **/
> -int
> -request_firmware_nowait(
> - struct module *module, bool uevent,
> +static int
> +__request_firmware_nowait(
> + struct module *module, unsigned int opt_flags,
>   const char *name, struct device *device, gfp_t gfp, void *context,
>   void (*cont)(const struct firmware *fw, void *context))
>  {
> @@ -1426,8 +1425,7 @@ request_firmware_nowait(
>   fw_work->device = device;
>   fw_work->context = context;
>   fw_work->cont = cont;
> - fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
> - (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> + fw_work->opt_flags = FW_OPT_NOWAIT | opt_flags;
>  
>   if (!try_module_get(module)) {
>   kfree_const(fw_work->name);
> @@ -1440,8 +1438,35 @@ request_firmware_nowait(
>   schedule_work(_work->work);
>   return 0;
>  }
> +
> +int request_firmware_nowait(struct module *module, bool uevent,
> + const char *name, struct device *device, gfp_t gfp,
> + void *context,
> + void (*cont)(const struct firmware *fw, void 
> *context))
> +{
> + unsigned int opt_flags = FW_OPT_FALLBACK |
> + (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
> +
> + return __request_firmware_nowait(module, opt_flags, name, device, gfp,
> +  context, cont);
> +}
>  EXPORT_SYMBOL(request_firmware_nowait);
>  
> +int __request_firmware_async(struct module *module, const char *name,
> +  struct firmware_opts *fw_opts, struct device *dev,
> +  void *context,
> +  void (*cont)(const struct firmware *fw, void 
> *context))
> +{
> + unsigned int opt_flags = FW_OPT_UEVENT;

This exposes a long issue. Think -- why do we want this enabled by default? Its
actually because even though the fallback stuff is optional and can be, the 
uevent
internal flag *also* provides caching support as a side consequence only. We
don't want to add a new API without first 

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote:
> On 16-5-2017 1:13, Luis R. Rodriguez wrote:
> > Since no upstream delta is needed for firmwared I'd like to first encourage
> > evaluating the above. While distributions don't carry it yet that may be 
> > seen as
> > an issue but since what we are looking for are corner cases, only folks 
> > needing
> > to deploy a specific solution would need it or a custom proprietary 
> > solution.
> 
> Ok. I will go try and run firmwared in OpenWrt on a router platform.
> Have to steal one from a colleague :-p Will study firmwared.

Arend, curious how this effort goes. Its important to me as we know then that
if this works its a good approach to recommend moving forward which should also
prove less complex than that soup we had with the custom fallback stuff.

  Luis

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Tue, May 16, 2017 at 10:41:08AM +0200, Arend Van Spriel wrote:
> On 16-5-2017 1:13, Luis R. Rodriguez wrote:
> > On Fri, May 12, 2017 at 11:02:26PM +0200, Arend Van Spriel wrote:
> >> try again.. replacing email address from Michał
> >> On 12-5-2017 22:55, Arend Van Spriel wrote:
> >>> Let me explain the idea to refresh your memory (and mine). It started
> >>> when we were working on adding driver support for OpenWrt in brcmfmac.
> >>> The driver requests for firmware calibration data, but on routers it is
> >>> stored in flash. So after failing on the firmware request we now call a
> >>> platform specific API. That was my itch, but it was not bad enough to go
> >>> and scratch. Now for N900 case there is a similar scenario alhtough it
> >>> has additional requirement to go to user-space due to need to use a
> >>> proprietary library to obtain the NVS calibration data. My thought: Why
> >>> should firmware_class care?
> > 
> > Agreed.
> > 
> >>> So the idea is that firmware_class provides
> >>> a registry for modules that can produce a certain firmware "file". Those
> >>> modules can do whatever is needed. If they need to use umh so be it.
> >>> They would only register themselves with firmware_class on platforms
> >>> that need them. It would basically be replacing the fallback mechanism
> >>> and only be effective on certain platforms.
> > 
> > Sure, so it sounds like the work that Daniel Wagner and Tom Gundersen worked
> > [0] on which provides a firmwared with two modes: best-effort, and 
> > final-mode,
> > would address what you are looking for but without requiring any upstream
> > changes, *and* it also helps solve the rootfs race remote-proc folks had
> > concerns over.
> > 
> > The other added gain over this solution is if folks need their own 
> > proprietary
> > concoction they can just fork firmwared and have that do whatever it needs
> > for the specific device on the specific rootfs. That is, firmwared can be 
> > the
> > upstream solution if folks need it, but if folks need something custom they 
> > can
> > just mimic the implementation: best-effort, and and final-mode.
> > 
> > Yet another added gain over this solution we can do *not* support the
> > custom fallback mechanism as its not needed, the udev event should suffice
> > to let userspace do what it needs.
> > 
> > Lastly, if we did not want to deal with timeouts for the way the driver data
> > API implements it I think we might be able to do away with them for for 
> > async
> > requests if we assume there will be a daemon that spawns in final-mode 
> > eventually,
> > and since it *knows* when the rootfs is ready it should be able to do a 
> > final
> > lookup, if it returns -ENOENT; then indeed we know we can give up. Now, 
> > perhaps
> > how and if we want to deal with timeouts when using the driver data API for
> > the fallback mechanism is worth considering given it does not have a 
> > fallback
> > mechanism support yet. If we *add* them it would seem this would also put an
> > implicit race against userspace finishing initialization and running 
> > firmwared
> > in final-mode.
> 
> Just to be clear. When you are saying "rootfs" in this story, you mean
> any (mounted) file-system which may hold the firmware. At least that was
> one of the arguments. In kernel space we can not know how the system is
> setup in terms of mount points, let alone on which mounted file-system
> the firmware resides.

Right, wherever the hell that thing is on, which could be on a crypic fuse
drive waiting for some bits to be decrypted from Elon Musk on a spaceship on his
way to Mars, and only userspace knows how to decrypt this thing through some
evil proprietary thing, way way after a full bootup.

> > Johannes, do you recall the corner cases we spoke about regarding timeouts?
> > Does this match what we spoke about?
> > 
> >>> Let me know if this idea is still of interest and I will rebase what I
> >>> have for an RFC round.
> > 
> > Since no upstream delta is needed for firmwared I'd like to first encourage
> > evaluating the above. While distributions don't carry it yet that may be 
> > seen as
> > an issue but since what we are looking for are corner cases, only folks 
> > needing
> > to deploy a specific solution would need it or a custom proprietary 
> > solution.
> 
> Ok. I will go try and run firmwared in OpenWrt on a router platform.
> Have to steal one from 

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Fri, May 12, 2017 at 11:02:26PM +0200, Arend Van Spriel wrote:
> try again.. replacing email address from Michał
> On 12-5-2017 22:55, Arend Van Spriel wrote:
> > Let me explain the idea to refresh your memory (and mine). It started
> > when we were working on adding driver support for OpenWrt in brcmfmac.
> > The driver requests for firmware calibration data, but on routers it is
> > stored in flash. So after failing on the firmware request we now call a
> > platform specific API. That was my itch, but it was not bad enough to go
> > and scratch. Now for N900 case there is a similar scenario alhtough it
> > has additional requirement to go to user-space due to need to use a
> > proprietary library to obtain the NVS calibration data. My thought: Why
> > should firmware_class care?

Agreed.

> > So the idea is that firmware_class provides
> > a registry for modules that can produce a certain firmware "file". Those
> > modules can do whatever is needed. If they need to use umh so be it.
> > They would only register themselves with firmware_class on platforms
> > that need them. It would basically be replacing the fallback mechanism
> > and only be effective on certain platforms.

Sure, so it sounds like the work that Daniel Wagner and Tom Gundersen worked
[0] on which provides a firmwared with two modes: best-effort, and final-mode,
would address what you are looking for but without requiring any upstream
changes, *and* it also helps solve the rootfs race remote-proc folks had
concerns over.

The other added gain over this solution is if folks need their own proprietary
concoction they can just fork firmwared and have that do whatever it needs
for the specific device on the specific rootfs. That is, firmwared can be the
upstream solution if folks need it, but if folks need something custom they can
just mimic the implementation: best-effort, and and final-mode.

Yet another added gain over this solution we can do *not* support the
custom fallback mechanism as its not needed, the udev event should suffice
to let userspace do what it needs.

Lastly, if we did not want to deal with timeouts for the way the driver data
API implements it I think we might be able to do away with them for for async
requests if we assume there will be a daemon that spawns in final-mode 
eventually,
and since it *knows* when the rootfs is ready it should be able to do a final
lookup, if it returns -ENOENT; then indeed we know we can give up. Now, perhaps
how and if we want to deal with timeouts when using the driver data API for
the fallback mechanism is worth considering given it does not have a fallback
mechanism support yet. If we *add* them it would seem this would also put an
implicit race against userspace finishing initialization and running firmwared
in final-mode.

Johannes, do you recall the corner cases we spoke about regarding timeouts?
Does this match what we spoke about?

> > Let me know if this idea is still of interest and I will rebase what I
> > have for an RFC round.

Since no upstream delta is needed for firmwared I'd like to first encourage
evaluating the above. While distributions don't carry it yet that may be seen as
an issue but since what we are looking for are corner cases, only folks needing
to deploy a specific solution would need it or a custom proprietary solution.

[0] https://github.com/teg/firmwared.git

PS.

Note that firmware signing will require an additional file, the detached
signature. The driver data API does not currently support the fallback
mechanism so we would not have to worry about that yet but once we add
fallback support we'd need to consider this.

  Luis

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Wed, May 03, 2017 at 09:02:20PM +0200, Arend Van Spriel wrote:
> On 3-1-2017 18:59, Luis R. Rodriguez wrote:
> > On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
> >>
> >> Right question is "should we solve it without user-space help"?
> >>
> >> Answer is no, too. Way too complex. Yes, it would be nice if hardware
> >> was designed in such a way that getting calibration data from kernel
> >> is easy, and if you design hardware, please design it like that. But
> >> N900 is not designed like that and getting the calibration through
> >> userspace looks like only reasonable solution.
> > 
> > Arend seems to have a better alternative in mind possible for other
> > devices which *can* probably pull of doing this easily and nicely,
> > given the nasty history of the usermode helper crap we should not
> > in any way discourage such efforts.
> > 
> > Arend -- please look at the firmware cache, it not a hash but a hash
> > table for an O(1) lookups would be a welcomed change, then it could
> > be repurposed for what you describe, I think the only difference is
> > you'd perhaps want a custom driver hook to fetch the calibration data
> > so the driver does whatever it needs.
> 
> Hi Luis,
> 
> I let my idea catch dust on the shelf for a while. 

:) BTW did you get to check out Daniel Wagner and Tom Gundersen's firmware work
[0] ? This can provide a proper clear fallback mechanism which *also* helps
address the race between "critical mount points ready" problem we had discussed
long ago. IIRC it did this by having two modes of operation a best effort-mode
and a final-mode. The final-mode would only be used once all the real rootfs is
ready. Userspace decides when to kick / signal firmwared to switch to final-mode
as only userspace will know for sure when that is ready. The best-effort mode
would be used in initramfs. There is also no need for a "custom fallback", the
uevent fallback mechanism can be relied upon alone. Custom tools can just fork
off and do something similar to firmward then in terms of architecture. This is
a form of fallback mechanism I think I'd be happy to see enabled on the new
driver data API. But of course, I recall also liking what you had in mind as 
well
so would be happy to see more alternatives! The cleaner the better !

[0] https://github.com/teg/firmwared

> Actually had a couple
> of patches ready, but did get around testing them. So I wanted to rebase
> them on your linux-next tree. I bumped into the umh lock thing and was
> wondering why direct filesystem access was under that lock as well.

Indeed, it was an odd thing.

> In your tree I noticed a fix for that. 

Yup!

It took a lot of git archeology to reach a sound approach forward which makes
me feel comfortable without regressing the kernel, this should not regress
the kernel.

> The more reason to base my work on
> top of your firmware_class changes. Now my question is what is the best
> branch to choose, because you have a "few" in that repo to choose from ;-)

I have a series of topical changes, and I rebase onto the latest linux-next
regularly before posting patches, if 0-day finds issues, I dish out a next
try2 or tryX iteration until all issues are fixed. So in this case you'd
just go for the latest driver-data-$(next_date) and if there is a try
postfix use the latest tryX.

In this case 20170501-driver-data-try2, this is based on linux-next tag
next-20170501. If you have issues booting on that next tag though you
could instead try the v4.11-rc8-driver-data-try3 branch based on v4.11-rc8.
But naturally patches ultimately should be based on the latest linux-next
tag for actual submission.

PS. after my changes the fallback mechanism can easily be shoved into its
own file, not sure if that helps with how clean of a solution your work
will be.

  Luis

[RFC] igmp: address pmc kmemleak from on igmpv3_del_delrec()

[Luis R. Rodriguez]

When we igmpv3_add_delrec() we kzalloc the pmc, but when users
calligmpv3_del_delrec() we never free the pmc. This was caught
by the following kmemleak splat:

unreferenced object 0x99666ff43b40 (size 192):
  comm "systemd-resolve", pid 1258, jiffies 4309905600 (age 2138.352s)
  hex dump (first 32 bytes):
00 6a 64 72 66 99 ff ff e0 00 00 fc 00 00 00 00  .jdrf...
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00  
  backtrace:
[] kmemleak_alloc+0x4a/0xa0
[] kmem_cache_alloc_trace+0x107/0x240
[] igmp_group_dropped+0xfd/0x270
[] ip_mc_dec_group+0xaf/0x110
[] ip_mc_leave_group+0xb6/0x140
[] do_ip_setsockopt.isra.13+0x4c7/0xed0
[] ip_setsockopt+0x34/0xb0
[] udp_setsockopt+0x1b/0x30
[] sock_common_setsockopt+0x14/0x20
[] SyS_setsockopt+0x80/0xe0
[] do_syscall_64+0x5b/0xc0
[] return_from_SYSCALL_64+0x0/0x6a
[] 0x

Signed-off-by: Luis R. Rodriguez <mcg...@kernel.org>
---

I can reproduce this over time on a qemu box running next-20170125.
After running this for a while I no longer see the splat. This needs
confirmation form folks more familiar with the code, hence RFC. If
this is a real fix we need appropriate tags for the patch.

 net/ipv4/igmp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 5b15459955f8..44fd86de2823 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -1172,6 +1172,7 @@ static void igmpv3_del_delrec(struct in_device *in_dev, 
struct ip_mc_list *im)
psf->sf_crcount = im->crcount;
}
in_dev_put(pmc->interface);
+   kfree(pmc);
}
spin_unlock_bh(>lock);
 }
-- 
2.11.0

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Fri, Jan 27, 2017 at 02:11:46PM +0100, Pali Rohár wrote:
> So there are only two options:
> 
> 1) Disallow it and so these users will have non-working wifi.
> 
> 2) Allow those data to be used as fallback mechanism.

There is one "custom fallback" user in kernel which we recently
determined was a total mistake. A sysfs interface should have
been defined to enable custom LED settings. Can't a series of
sysfs interfaces be used to enable override ? So is that not a
third option worth consideration?

  Luis

Re: [PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Luis R. Rodriguez]

On Mon, Dec 26, 2016 at 05:35:59PM +0100, Pavel Machek wrote:
> 
> Right question is "should we solve it without user-space help"?
> 
> Answer is no, too. Way too complex. Yes, it would be nice if hardware
> was designed in such a way that getting calibration data from kernel
> is easy, and if you design hardware, please design it like that. But
> N900 is not designed like that and getting the calibration through
> userspace looks like only reasonable solution.

Arend seems to have a better alternative in mind possible for other
devices which *can* probably pull of doing this easily and nicely,
given the nasty history of the usermode helper crap we should not
in any way discourage such efforts.

Arend -- please look at the firmware cache, it not a hash but a hash
table for an O(1) lookups would be a welcomed change, then it could
be repurposed for what you describe, I think the only difference is
you'd perhaps want a custom driver hook to fetch the calibration data
so the driver does whatever it needs.

> Now... how exactly to do that is other question. (But this is looks
> very reasonable. Maybe I'd add request_firmware_with_flags(, ...int
> flags), but.. that's a tiny detail.). But userspace needs to be
> involved.

No, no, we keep adding yet-another-exported symbol for requesting firmware,
instead of just adding a set of parameters possible and easily extending
functionality. Please review the patches posted on my last set which adds
a flexible API with only 2 calls, sync and async, and lets us customize
our requests using a parameter:

https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux-next.git/log/?h=20161216-drvdata-v3-try3

This also documents the "usermode helper" properly and explains some of
the issues and limitations you will need to consider if you use it, its
one reason I'd highly encourage to consider an alternative as what Arend
is considering. *Iff* you insist on using the (now using the proper term,
as per the documentation update I am providing) "custom fallback mechanism"
I welcome such a change but I ask we *really* think this through well so
we avoid the stupid issues which have historically made the custom fallback
mechanism more of a nuisance for Linux distributions, users and developers.
To this end -- I ask you check out Daniel Wagner and Tom Gundersen's firmwared
work [0] which I referred you to in December. Although the drvdata API does
not yet use a custom fallback mechanism, after and its merged the goal here
would be to *only* support a clean custom fallback mechanism which aligns
itself *well* with firmwared or solutions like it. Your patch set then could
just become a patch set to add the custom fallback mechaism support to drvdata
API with the new options/prefernce you are looking for to be specified in
the new parameters, not a new exported symbol.

One of the cruxes we should consider addressing before the drvdata API gets a
custom fallback mechanism support added is that the usermode helper lock should
be replaced with a generic solution for the races it was intended to address:
use of the API on suspend/resume and implicitly later avoid a race on init. To
this end we should consider the same race for *other* real kernel "user mode
helpers", I've documented this on a wiki [1] which documents the *real*
kernel usermode helpers users, one of which was the kobject uevent which is
one of the fallback mechanisms.

I should also note that the idea of fallback mechanism using kobject uevents
should really suffice, in review with Johannes Berg at least, he seemed
convinced just letting either the upstream firmwared, a custom firmwared or
a custom userspace solution should be able to just monitor for uevents for
drvdata and do the right thing, this whole "custom fallback mechanism"
in retrospect seems not really needed as far as I can tell.

[0] https://github.com/teg/firmwared
[1] https://kernelnewbies.org/KernelProjects/usermode-helper-enhancements

  Luis

Re: wl1251 & mac address & calibration data

[Luis R. Rodriguez]

On Thu, Dec 15, 2016 at 2:12 PM, Arend Van Spriel
 wrote:
> On 15-12-2016 16:33, Pali Rohár wrote:
>> On Thu Dec 15 09:18:44 2016 Kalle Valo  wrote:
>>> (Adding Luis because he has been working on request_firmware() lately)
>>>
>>> Pali Rohár  writes:
>>>
>> So no, there is no argument against... request_firmware() in
>> fallback mode with userspace helper is by design blocking and
>> waiting for userspace. But waiting for some change in DTS in
>> kernel is just nonsense.
>
> I would just mark the wlan device with status = "disabled" and
> enable it in the overlay together with adding the NVS & MAC info.

 So if you think that this solution make sense, we can wait what net
 wireless maintainers say about it...

 For me it looks like that solution can be:

 extending request_firmware() to use only userspace helper
>>>
>>> I haven't followed the discussion very closely but this is my preference
>>> what drivers should do:
>>>
>>> 1) First the driver should do try to get the calibration data and mac
>>>address from the device tree.
>>>
>>
>> Ok, but there is no (dynamic, device specific) data in DTS for N900. So 1) 
>> is noop.
>
> Uhm. What do you mean? You can propose a patch to the DT bindings [1] to
> get it in there and create your N900 DTB or am I missing something here.
> Are there hardware restrictions that do not allow you to boot with your
> own DTB.
>
>>> 2) If they are not in DT the driver should retrieve the calibration data
>>>with request_firmware(). BUT with an option for user space to
>>>implement that with a helper script so that the data can be created
>>>dynamically, which I believe openwrt does with ath10k calibration
>>>data right now.
>>
>> Currently there is flag for request_firmware() that it should fallback to 
>> user helper if direct VFS access not find needed firmware.
>>
>> But this flag is not suitable as /lib/firmware already provides default (not 
>> device specific) calibration data.
>>
>> So I would suggest to add another flag/function which will primary use user 
>> helper.
> I recall Luis saying that user-mode helper (fallback) should be
> discouraged, because there is no assurance that there is a user-mode
> helper so you might just be pissing in the wind.

There's tons of issues with the current status quo of the so called
"firmware usermode helper". To start off with its a complete misnomer,
the kernel's usermode helper infrastructure is implemented in
lib/kmod.c and it provides an API to help call out to userspace some
helper for you. That's the real kernel usermode helper. In so far as
the firmware_class.c driver is concerned -- it only makes use of the
kernel user mode helper as an option if you have
CONFIG_UEVENT_HELPER_PATH set to help address kobject uevents. Most
distributions do not use this, and back in the day systemd udev, and
prior to that hotplug used to process firmware kobject uevents.
systemd udev is long gone. Gone. This kobject uevents really are a
"fallback mechanism" for firmware only -- if cached firmware, built-in
firmware, or direct filesystem lookup fails. These each are their own
beast. Finally kobject uevents are only one of the fallback
mechanisms, "custom fallback mechanisms" are the other option and its
what you and others seem to describe to want for these sorts of custom
things.

There are issues with the existing request_firmware() API in that
everyone and their mother keeps extending it with stupid small API
extensions to do yet-another-tweak, and then having to go and change
tons of drivers. Or a new API call added for just one custom knob.
Naturally this is all plain dumb, so yet-another-API call or new
argument is not going to help us. We don't have "flags" persi in this
API, that was one issue with the design of the API, but there are much
more. The entire change in mechanism of "firmware fallback mechanism"
depending on which kernel config options you have is another. Its a
big web of gum put together. All this needs to end.

I've recently proposed a new patch to first help with understanding
each of the individual items I've listed above with as much new
information as is possible. I myself need to re-read it to just keep
tabs on what the hell is going on at times. After this I have a new
API proposal which I've been working on for a long time now which adds
an extensible API with only 2 calls: sync, async, and lets us modify
attributes through a parameters struct. This is what we should use in
the future for further extensions.

For the new API a solution for "fallback mechanisms" should be clean
though and I am looking to stay as far as possible from the existing
mess. A solution to help both the old API and new API is possible for
the "fallback mechanism" though -- but for that I can only refer you
at this point to some of Daniel Wagner and Tom Gunderson's 

Re: [PATCH 0/2] wireless: Use complete() instead complete_all()

[Luis R. Rodriguez]

On Thu, Aug 18, 2016 at 03:12:04PM +0200, Daniel Wagner wrote:
> This series ignores all complete_all() usages in the firmware loading
> path. They will be hopefully address by Luis' sysdata patches [0].
> That leaves a couple of complete_all() calls.

I had not considered this as a gain, but glad to know the sysdata series
could help with RT as well, thanks for the clarification.

 Luis

Re: [Intel-gfx] [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA

[Luis R. Rodriguez]

On Wed, Apr 20, 2016 at 01:17:30PM +0200, Daniel Vetter wrote:
> On Wed, Apr 20, 2016 at 11:10:54AM +0200, Luis R. Rodriguez wrote:
> > Reason I ask is since I noticed a while ago a lot of drivers
> > were using info->fix.smem_start and info->fix.smem_len consistently
> > for their ioremap'd areas it might make sense instead to let the
> > internal framebuffer (register_framebuffer()) optionally manage the
> > ioremap_wc() for drivers, given that this is pretty generic stuff.
> 
> All that legacy fbdev stuff is just for legacy support, and I prefer to
> have that as dumb as possible. There's been some discussion even around
> lifting the "kick out firmware fb driver" out of fbdev, since we'd need it
> to have a simple drm driver for e.g. uefi.
> 
> But I definitely don't want a legacy horror show like fbdev to
> automagically take care of device mappings for drivers.

Makes sense, it also still begs the question if more modern APIs
could manage the ioremap for you. Evidence shows people get
sloppy and if things were done internally with helpers it may
be easier to later make adjustments.

  Luis

Re: [PATCH 02/19] io-mapping: Specify mapping size for io_mapping_map_wc()

[Luis R. Rodriguez]

On Wed, Apr 20, 2016 at 08:14:32PM +0100, Chris Wilson wrote:
> On Wed, Apr 20, 2016 at 08:58:44PM +0200, Luis R. Rodriguez wrote:
> > On Wed, Apr 20, 2016 at 07:42:13PM +0100, Chris Wilson wrote:
> > > The ioremap() hidden behind the io_mapping_map_wc() convenience helper
> > > can be used for remapping multiple pages. Extend the helper so that
> > > future callers can use it for larger ranges.
> > > 
> > > Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> > > Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> > > Cc: Daniel Vetter <daniel.vet...@intel.com>
> > > Cc: Jani Nikula <jani.nik...@linux.intel.com>
> > > Cc: David Airlie <airl...@linux.ie>
> > > Cc: Yishai Hadas <yish...@mellanox.com>
> > > Cc: Dan Williams <dan.j.willi...@intel.com>
> > > Cc: Ingo Molnar <mi...@kernel.org>
> > > Cc: "Peter Zijlstra (Intel)" <pet...@infradead.org>
> > > Cc: David Hildenbrand <d...@linux.vnet.ibm.com>
> > > Cc: Luis R. Rodriguez <mcg...@kernel.org>
> > > Cc: intel-...@lists.freedesktop.org
> > > Cc: dri-de...@lists.freedesktop.org
> > > Cc: netdev@vger.kernel.org
> > > Cc: linux-r...@vger.kernel.org
> > > Cc: linux-ker...@vger.kernel.org
> > 
> > We have 2 callers today, in the future, can you envision
> > this API getting more options? If so, in order to avoid the
> > pain of collateral evolutions I can suggest a descriptor
> > being passed with the required settings / options. This lets
> > you evolve the API without needing to go in and modify
> > old users. If you choose not to that's fine too, just
> > figured I'd chime in with that as I've seen the pain
> > with other APIs, and I'm putting an end to the needless
> > set of collateral evolutions this way.
> 
> Do you have a good example in mind? I've one more patch to try and take
> advantage of the io-mapping (that may or not be such a good idea in
> practice) but I may as well see if I can make io_mapping more useful
> when I do.

Sure, here's my current version of the revamp of the firmware API
to a more flexible API, which lets us compartamentalize the
usermode helper, and through the new API avoids the issues with further
future collateral evolutions. It is still being baked, I'm fine tuning
the SmPL to folks automatically do conversion if they want:

https://git.kernel.org/cgit/linux/kernel/git/mcgrof/linux.git/log/?h=20160417-sysdata-api-v1

It also has a test driver (which I'd also recommend if you can pull off).
It would be kind of hard to do something like a lib/io-mapping_test.c
given there is no real device to ioremap -- _but_ perhaps regular
RAM can be used for fake a device MMIO. I am not sure if its even
possible... but if so it would not only be useful for something
like your API but also for testing ioremap() and friends, and
any possible aliasing bombs we may want to vet for. It also hints
how we may in the future be able to automatically write test drivers
for APIs for us through inference, but that needs a lot of more love
to make it tangible.

  Luis

Re: [PATCH 02/19] io-mapping: Specify mapping size for io_mapping_map_wc()

[Luis R. Rodriguez]

On Wed, Apr 20, 2016 at 07:42:13PM +0100, Chris Wilson wrote:
> The ioremap() hidden behind the io_mapping_map_wc() convenience helper
> can be used for remapping multiple pages. Extend the helper so that
> future callers can use it for larger ranges.
> 
> Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
> Cc: Daniel Vetter <daniel.vet...@intel.com>
> Cc: Jani Nikula <jani.nik...@linux.intel.com>
> Cc: David Airlie <airl...@linux.ie>
> Cc: Yishai Hadas <yish...@mellanox.com>
> Cc: Dan Williams <dan.j.willi...@intel.com>
> Cc: Ingo Molnar <mi...@kernel.org>
> Cc: "Peter Zijlstra (Intel)" <pet...@infradead.org>
> Cc: David Hildenbrand <d...@linux.vnet.ibm.com>
> Cc: Luis R. Rodriguez <mcg...@kernel.org>
> Cc: intel-...@lists.freedesktop.org
> Cc: dri-de...@lists.freedesktop.org
> Cc: netdev@vger.kernel.org
> Cc: linux-r...@vger.kernel.org
> Cc: linux-ker...@vger.kernel.org

We have 2 callers today, in the future, can you envision
this API getting more options? If so, in order to avoid the
pain of collateral evolutions I can suggest a descriptor
being passed with the required settings / options. This lets
you evolve the API without needing to go in and modify
old users. If you choose not to that's fine too, just
figured I'd chime in with that as I've seen the pain
with other APIs, and I'm putting an end to the needless
set of collateral evolutions this way.

Other than that possible API optimization:

Reviewed-by: Luis R. Rodriguez <mcg...@kernel.org>

  Luis

Re: [PATCH 4/4] drm/i915: Move ioremap_wc tracking onto VMA

[Luis R. Rodriguez]

On Tue, Apr 19, 2016 at 01:33:58PM +0100, Chris Wilson wrote:
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6ce2c31b9a81..9ef47329e8ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3346,6 +3346,15 @@ static void i915_gem_object_finish_gtt(struct 
> drm_i915_gem_object *obj)
>   old_write_domain);
>  }
>  
> +static void __i915_vma_iounmap(struct i915_vma *vma)
> +{
> + if (vma->iomap == NULL)
> + return;
> +
> + io_mapping_unmap(vma->iomap);

The NULL check could just be done by io_mapping_unmap() then you
can avoid this in other drivers too.

> + vma->iomap = NULL;

You added accounting here, by simple int and inc / dec'ing it.
I cannot confirm if it is correctly avoiding races, can you
confirm?

Also you added accounting for the custom vma pinning thing and do
GEM_BUG_ON(vma->pin_count == 0); when you unpin one instance but *you do not*
do something like GEM_BUG_ON(vma->pin_count != 0); when you do the final full
iounmap. That seems rather sloppy.

iomapping stuff has its own custom data structure, why not just use that data
structure instead of the struct i915_vma and generalize this ? Drivers can
be buggy and best if we avoid custom driver accounting and just do it in a neat
generic fashion.

Then other drivers could use this too.

> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index 79ac202f3870..93f54a10042f 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -244,22 +245,23 @@ static int intelfb_create(struct drm_fb_helper *helper,
>   info->flags = FBINFO_DEFAULT | FBINFO_CAN_FORCE_OUTPUT;
>   info->fbops = _ops;
>  
> + vma = i915_gem_obj_to_ggtt(obj);
> +
>   /* setup aperture base/size for vesafb takeover */
>   info->apertures->ranges[0].base = dev->mode_config.fb_base;
>   info->apertures->ranges[0].size = ggtt->mappable_end;
>  
> - info->fix.smem_start = dev->mode_config.fb_base + 
> i915_gem_obj_ggtt_offset(obj);
> - info->fix.smem_len = size;
> + info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;
> + info->fix.smem_len = vma->node.size;
>  
> - info->screen_base =
> - ioremap_wc(ggtt->mappable_base + i915_gem_obj_ggtt_offset(obj),
> -size);
> - if (!info->screen_base) {
> + vaddr = i915_vma_pin_iomap(vma);
> + if (IS_ERR(vaddr)) {
>   DRM_ERROR("Failed to remap framebuffer into virtual memory\n");
> - ret = -ENOSPC;
> + ret = PTR_ERR(vaddr);
>   goto out_destroy_fbi;
>   }
> - info->screen_size = size;
> + info->screen_base = vaddr;
> + info->screen_size = vma->node.size;

some framebuffer drivers tend to use a generic start address of
iinfo->fix.smem_start and a length of info->fix.smem_len, this
driver sets the smem_start above, but its different than what
gets ioremap for a start address:

+   ptr = io_mapping_map_wc(i915_vm_to_ggtt(vma->vm)->mappable,
+   vma->node.start,
+   vma->node.size);

fix.smem_start is :


> + info->fix.smem_start = dev->mode_config.fb_base + vma->node.start;

The smem_len matches though. Can you clarify if its correct for
the io_mapping_map_wc() should not be using info->fix.smem_start
(which is dev->mode_config.fb_base + vma->node.start)?

Reason I ask is since I noticed a while ago a lot of drivers
were using info->fix.smem_start and info->fix.smem_len consistently
for their ioremap'd areas it might make sense instead to let the
internal framebuffer (register_framebuffer()) optionally manage the
ioremap_wc() for drivers, given that this is pretty generic stuff.

  Luis

Re: [PATCH v2 2/4] io-mapping: Specify mapping size for io_mapping_map_wc()

[Luis R. Rodriguez]

On Tue, Apr 19, 2016 at 01:02:25PM +0100, Chris Wilson wrote:
> On Mon, Apr 18, 2016 at 12:14:00PM +0100, Chris Wilson wrote:
> > The ioremap() hidden behind the io_mapping_map_wc() convenience helper
> > can be used for remapping multiple pages. Extend the helper so that
> > future callers can use it for larger ranges.
> 
> Adding Luis Rodriguez as he has been active in the area of ioremap_*().

Can someone bounce me a copy of the original e-mails? Can you also use
mcg...@kernel.org ?

  Luis

[PATCH v4 11/11] mtrr: bury MTRR - unexport mtrr_add() and mtrr_del()

[Luis R. Rodriguez]

From: Luis R. Rodriguez mcg...@suse.com

The crusade to replace mtrr_add() with architecture agnostic
arch_phys_wc_add() is complete, this will ensure write-combining
implementations (PAT on x86) is taken advantage instead of using
MTRR. With the crusade done now, hide direct MTRR access for
drivers.

Update x86 documentation on MTRR to reflect the completion of the
phasing out of direct access to MTRR, also add a note on platform
firmware code use of MTRRs based on the obituary discussion of
MTRRs on Linux [0].

[0] http://lkml.kernel.org/r/1438991330.3109.196.ca...@hp.com

Cc: Toshi Kani toshi.k...@hp.com
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@redhat.com
Cc: H. Peter Anvin h...@zytor.com
Cc: Borislav Petkov b...@suse.de
Cc: Dave Hansen dave.han...@linux.intel.com
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Ingo Molnar mi...@elte.hu
Cc: Juergen Gross jgr...@suse.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Andy Lutomirski l...@amacapital.net
Cc: Dave Airlie airl...@redhat.com
Cc: Antonino Daplas adap...@gmail.com
Cc: Jean-Christophe Plagniol-Villard plagn...@jcrosoft.com
Cc: Tomi Valkeinen tomi.valkei...@ti.com
Cc: Ville Syrjälä syrj...@sci.fi
Cc: Mel Gorman mgor...@suse.de
Cc: Vlastimil Babka vba...@suse.cz
Cc: Davidlohr Bueso dbu...@suse.de
Cc: Doug Ledford dledf...@redhat.com
Cc: Andy Walls awa...@md.metrocast.net
Cc: x...@kernel.org
Cc: netdev@vger.kernel.org
Cc: linux-me...@vger.kernel.org
Cc: linux-fb...@vger.kernel.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Luis R. Rodriguez mcg...@suse.com
---
 Documentation/x86/mtrr.txt  | 20 
 arch/x86/kernel/cpu/mtrr/main.c |  2 --
 2 files changed, 16 insertions(+), 6 deletions(-)

diff --git a/Documentation/x86/mtrr.txt b/Documentation/x86/mtrr.txt
index 860bc3adc223..8a0bdb6e7370 100644
--- a/Documentation/x86/mtrr.txt
+++ b/Documentation/x86/mtrr.txt
@@ -6,10 +6,22 @@ Luis R. Rodriguez mcg...@do-not-panic.com - April 9, 2015
 ===
 Phasing out MTRR use
 
-MTRR use is replaced on modern x86 hardware with PAT. Over time the only type
-of effective MTRR that is expected to be supported will be for write-combining.
-As MTRR use is phased out device drivers should use arch_phys_wc_add() to make
-MTRR effective on non-PAT systems while a no-op on PAT enabled systems.
+MTRR use is replaced on modern x86 hardware with PAT. Direct MTRR use by
+drivers on Linux is now completely phased out, device drivers should use
+arch_phys_wc_add() in combination with ioremap_wc() to make MTRR effective on
+non-PAT systems while a no-op but equally effective on PAT enabled systems.
+
+Even if Linux does not use MTRR directly some x86 platform firmware may still
+set up MTRRs early before booting the OS, they do this as some platform
+firmware may still have implemented access to MTRRs which would be controlled
+and handled by the platform firmware directly. An example of platform use of
+MTRR is through the use of SMI handlers, one case could be for fan control,
+the platform code would need uncachable access to some of its fan control
+registers. Such platform access does not need any Operating System MTRR code in
+place other than mtrr_type_lookup() to ensure any OS specific mapping requests
+are aligned with platform MTRR setup. If MTRRs are only set up by the platform
+firmware code though and the OS does not make any specific MTRR mapping
+requests mtrr_type_lookup() should always return MTRR_TYPE_INVALID.
 
 For details refer to Documentation/x86/pat.txt.
 
diff --git a/arch/x86/kernel/cpu/mtrr/main.c b/arch/x86/kernel/cpu/mtrr/main.c
index e7ed0d8ebacb..f891b4750f04 100644
--- a/arch/x86/kernel/cpu/mtrr/main.c
+++ b/arch/x86/kernel/cpu/mtrr/main.c
@@ -448,7 +448,6 @@ int mtrr_add(unsigned long base, unsigned long size, 
unsigned int type,
return mtrr_add_page(base  PAGE_SHIFT, size  PAGE_SHIFT, type,
 increment);
 }
-EXPORT_SYMBOL(mtrr_add);
 
 /**
  * mtrr_del_page - delete a memory type region
@@ -537,7 +536,6 @@ int mtrr_del(int reg, unsigned long base, unsigned long 
size)
return -EINVAL;
return mtrr_del_page(reg, base  PAGE_SHIFT, size  PAGE_SHIFT);
 }
-EXPORT_SYMBOL(mtrr_del);
 
 /**
  * arch_phys_wc_add - add a WC MTRR and handle errors if PAT is unavailable
-- 
2.4.3

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] README: clarify redistribution requirements covering patents

[Luis R. Rodriguez]

On Thu, May 28, 2015 at 5:48 PM, Luis R. Rodriguez mcg...@suse.com wrote:
 On Tue, May 19, 2015 at 1:22 PM, Luis R. Rodriguez
 mcg...@do-not-panic.com wrote:
 This v2 just changes licence to license as requested by Arend.

 Please let me know if there is anything else needed.

Just a friendly reminder.

 Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] README: clarify redistribution requirements covering patents

[Luis R. Rodriguez]

On Tue, May 19, 2015 at 1:22 PM, Luis R. Rodriguez
mcg...@do-not-panic.com wrote:
 This v2 just changes licence to license as requested by Arend.

Please let me know if there is anything else needed.

 Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Tue, Apr 21, 2015 at 3:02 PM, Luis R. Rodriguez mcg...@suse.com wrote:
 Andy, can we live without MTRR support on this driver for future kernels? This
 would only leave ipath as the only offending driver.

Sorry to be clear, can we live with removal of write-combining on this driver?

 Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Tue, Apr 21, 2015 at 06:51:26PM -0400, Andy Walls wrote:
 Sorry for the top post; mobile work email account.
 
 Luis,
 
 You do the changes to remove MTTR and point me to your dev repo and branch.
 Also point me to the new functions/primitives I'll need.

There is nothing new actually needed for ivtv, unless of course
the ivtv driver is bounded to use a large MTRR that includes
the non-framebuffer region, if so then the ioremap_uc() would
be needed, and you can just cherry pick that patch:

https://marc.info/?l=linux-kernelm=142964809110516w=1

I'll bounce that patch to you as well. Might help reading this
patch too:

https://marc.info/?l=linux-kernelm=142964809710517w=1

If your write-combining area is not restricted by size constraints
so that it also include the non-framebuffer areas then you can just
do a simple conversion of the driver to use ioremap_wc() on the
framebuffer followed by arch_phys_wc_add().

An example driver that required changes to split this with size
contraints is atyfb, here are the changes for it:

https://marc.info/?l=linux-kernelm=142964818810539w=1
https://marc.info/?l=linux-kernelm=142964813610531w=1
https://marc.info/?l=linux-kernelm=142964811010524w=1
https://marc.info/?l=linux-kernelm=142964814810532w=1

If you are not constrained by MTRR's limitation on size then
a simple trivial driver conversion is sufficient. For example:

https://marc.info/?l=linux-kernelm=142964744610286w=1

I should also note that we are strivoing to also not use overlapping ioremap()
calls as we want to avoid that mess. Overlapping iroemap() calls with different
types could in theory work but its best we just design clean drivers and avoid
this.

As per Andy Lutomirski, what we'd need done on ivtv likely is
for it to ioremap() for an initial bring up of the device, then
infer the framebuffer offset, and only when that is being used
then iounmap and then ioremap() again split areas on the driver,
one with ioremap.

 I'll do the changes to add write-combining back into ivtv and ivtvfb, test
 them with my hardware and push them to my linuxtv.org git repo.

Great! The above sounded like a complexity you did not wish to
take on, but if you're up for the change, that'd be awesome!

 I know there is at least one English speaking user in India using ivtv with
 old PVR hardware, and probably folks in less developed places also using it.

If the above is too much work for that few amount of users I'd hope
we can just have them use older kernels, for the sake of sane APIs and
clean driver architecture.

  Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] ethernet: myri10ge: use arch_phys_wc_add()

[Luis R. Rodriguez]

From: Luis R. Rodriguez mcg...@suse.com

This driver already uses ioremap_wc() on the same range
so when write-combining is available that will be used
instead.

Cc: Hyong-Youb Kim hy...@myri.com
Cc: Andy Lutomirski l...@amacapital.net
Cc: Suresh Siddha sbsid...@gmail.com
Cc: Ingo Molnar mi...@elte.hu
Cc: Thomas Gleixner t...@linutronix.de
Cc: Juergen Gross jgr...@suse.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: netdev@vger.kernel.org
Cc: Juergen Gross jgr...@suse.com
Cc: Daniel Vetter daniel.vet...@ffwll.ch
Cc: Andy Lutomirski l...@amacapital.net
Cc: Dave Airlie airl...@redhat.com
Cc: Antonino Daplas adap...@gmail.com
Cc: Jean-Christophe Plagniol-Villard plagn...@jcrosoft.com
Cc: Tomi Valkeinen tomi.valkei...@ti.com
Cc: linux-ker...@vger.kernel.org
Cc: netdev@vger.kernel.org
Signed-off-by: Luis R. Rodriguez mcg...@suse.com
---
 drivers/net/ethernet/myricom/myri10ge/myri10ge.c | 38 ++--
 1 file changed, 9 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c 
b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
index 1412f5a..2bae502 100644
--- a/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
+++ b/drivers/net/ethernet/myricom/myri10ge/myri10ge.c
@@ -69,11 +69,7 @@
 #include net/ip.h
 #include net/tcp.h
 #include asm/byteorder.h
-#include asm/io.h
 #include asm/processor.h
-#ifdef CONFIG_MTRR
-#include asm/mtrr.h
-#endif
 #include net/busy_poll.h
 
 #include myri10ge_mcp.h
@@ -242,8 +238,7 @@ struct myri10ge_priv {
unsigned int rdma_tags_available;
int intr_coal_delay;
__be32 __iomem *intr_coal_delay_ptr;
-   int mtrr;
-   int wc_enabled;
+   int wc_cookie;
int down_cnt;
wait_queue_head_t down_wq;
struct work_struct watchdog_work;
@@ -1905,7 +1900,7 @@ static const char 
myri10ge_gstrings_main_stats[][ETH_GSTRING_LEN] = {
tx_aborted_errors, tx_carrier_errors, tx_fifo_errors,
tx_heartbeat_errors, tx_window_errors,
/* device-specific stats */
-   tx_boundary, WC, irq, MSI, MSIX,
+   tx_boundary, irq, MSI, MSIX,
read_dma_bw_MBs, write_dma_bw_MBs, read_write_dma_bw_MBs,
serial_number, watchdog_resets,
 #ifdef CONFIG_MYRI10GE_DCA
@@ -1984,7 +1979,6 @@ myri10ge_get_ethtool_stats(struct net_device *netdev,
data[i] = ((u64 *)link_stats)[i];
 
data[i++] = (unsigned int)mgp-tx_boundary;
-   data[i++] = (unsigned int)mgp-wc_enabled;
data[i++] = (unsigned int)mgp-pdev-irq;
data[i++] = (unsigned int)mgp-msi_enabled;
data[i++] = (unsigned int)mgp-msix_enabled;
@@ -4040,14 +4034,7 @@ static int myri10ge_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
 
mgp-board_span = pci_resource_len(pdev, 0);
mgp-iomem_base = pci_resource_start(pdev, 0);
-   mgp-mtrr = -1;
-   mgp-wc_enabled = 0;
-#ifdef CONFIG_MTRR
-   mgp-mtrr = mtrr_add(mgp-iomem_base, mgp-board_span,
-MTRR_TYPE_WRCOMB, 1);
-   if (mgp-mtrr = 0)
-   mgp-wc_enabled = 1;
-#endif
+   mgp-wc_cookie = arch_phys_wc_add(mgp-iomem_base, mgp-board_span);
mgp-sram = ioremap_wc(mgp-iomem_base, mgp-board_span);
if (mgp-sram == NULL) {
dev_err(pdev-dev, ioremap failed for %ld bytes at 0x%lx\n,
@@ -4146,14 +4133,14 @@ static int myri10ge_probe(struct pci_dev *pdev, const 
struct pci_device_id *ent)
goto abort_with_state;
}
if (mgp-msix_enabled)
-   dev_info(dev, %d MSI-X IRQs, tx bndry %d, fw %s, WC %s\n,
+   dev_info(dev, %d MSI-X IRQs, tx bndry %d, fw %s, MTRR %s, WC 
Enabled\n,
 mgp-num_slices, mgp-tx_boundary, mgp-fw_name,
-(mgp-wc_enabled ? Enabled : Disabled));
+(mgp-wc_cookie  0 ? Enabled : Disabled));
else
-   dev_info(dev, %s IRQ %d, tx bndry %d, fw %s, WC %s\n,
+   dev_info(dev, %s IRQ %d, tx bndry %d, fw %s, MTRR %s, WC 
Enabled\n,
 mgp-msi_enabled ? MSI : xPIC,
 pdev-irq, mgp-tx_boundary, mgp-fw_name,
-(mgp-wc_enabled ? Enabled : Disabled));
+(mgp-wc_cookie  0 ? Enabled : Disabled));
 
board_number++;
return 0;
@@ -4175,10 +4162,7 @@ abort_with_ioremap:
iounmap(mgp-sram);
 
 abort_with_mtrr:
-#ifdef CONFIG_MTRR
-   if (mgp-mtrr = 0)
-   mtrr_del(mgp-mtrr, mgp-iomem_base, mgp-board_span);
-#endif
+   arch_phys_wc_del(mgp-wc_cookie);
dma_free_coherent(pdev-dev, sizeof(*mgp-cmd),
  mgp-cmd, mgp-cmd_bus);
 
@@ -4220,11 +4204,7 @@ static void myri10ge_remove(struct pci_dev *pdev)
pci_restore_state(pdev);
 
iounmap(mgp-sram);
-
-#ifdef CONFIG_MTRR
-   if (mgp-mtrr = 0)
-   mtrr_del(mgp-mtrr, mgp-iomem_base, mgp-board_span);
-#endif
+   arch_phys_wc_del(mgp-wc_cookie

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Wed, Apr 15, 2015 at 09:07:37PM -0400, Andy Walls wrote:
 On Thu, 2015-04-16 at 01:58 +0200, Luis R. Rodriguez wrote:
  Hey Andy, thanks for your review,  adding Hyong-Youb Kim for  review of the
  full range ioremap_wc() idea below.
  
  On Wed, Apr 15, 2015 at 06:38:51PM -0400, Andy Walls wrote:
   Hi All,
   
   On Mon, 2015-04-13 at 19:49 +0200, Luis R. Rodriguez wrote:
From the beginning it seems only framebuffer devices used MTRR/WC,
   [snip]
 The ivtv device is a good example of the worst type of
situations and these days. So perhap __arch_phys_wc_add() and a
ioremap_ucminus() might be something more than transient unless 
hardware folks
get a good memo or already know how to just Do The Right Thing (TM).
   
   Just to reiterate a subtle point, use of the ivtvfb is *optional*.  A
   user may or may not load it.  When the user does load the ivtvfb driver,
   the ivtv driver has already been initialized and may have functions of
   the card already in use by userspace.
  
  I suspected this and its why I note that a rewrite to address a clean
  split with separate ioremap seems rather difficult in this case.
  
   Hopefully no one is trying to use the OSD as framebuffer and the video
   decoder/output engine for video display at the same time. 
  
  Worst case concern I have also is the implications of having overlapping
  ioremap() calls (as proposed in my last reply) for different memory types
  and having the different virtual memory addresse used by different parts
  of the driver. Its not clear to me what the hardware implications of this
  are.
  
But the video
   decoder/output device nodes may already be open for performing ioctl()
   functions so unmapping the decoder IO space out from under them, when
   loading the ivtvfb driver module, might not be a good thing. 
  
  Using overlapping ioremap() calls with different memory types would address
  this concern provided hardware won't barf both on the device and CPU. 
  Hardware
  folks could provide feedback or an ivtvfb user could test the patch supplied
  on both non-PAT and PAT systems. Even so, who knows,  this might work on 
  some
  systems while not on others, only hardware folks would know.
 
 The CX2341[56] firmware+hardware has a track record for being really
 picky about sytem hardware.  It's primary symptoms are for the DMA
 engine or Mailbox protocol to get hung up.  So yeah, it could barf
 easily on some users.
 
  An alternative... is to just ioremap_wc() the entire region, including
  MMIO registers for these old devices.
 
 That's my thought; as long as implementing PCI write then read can force
 writes to be posted and that setting that many pages as WC doesn't cause
 some sort of PAT resource exhaustion. (I know very little about PAT).

So upon review that strategy won't work well unless we implemnt some
sort of of hack on the driver. That's also quite a bit of work.

Andy, can we live without MTRR support on this driver for future kernels? This
would only leave ipath as the only offending driver.

  Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

On Thu, Apr 16, 2015 at 01:18:37PM +0900, Hyong-Youb Kim wrote:
 On Thu, Apr 16, 2015 at 01:58:16AM +0200, Luis R. Rodriguez wrote:
  
  An alternative... is to just ioremap_wc() the entire region, including
  MMIO registers for these old devices. I see one ethernet driver that does
  this, myri10ge, and am curious how and why they ended up deciding this
  and if they have run into any issues. I wonder if this is a reasonable
  comrpomise for these 2 remaining corner cases.
  
 
 For myri10ge, it a performance thing.  Descriptor rings are in NIC
 memory BAR0, not in host memory.  Say, to send a packet, the driver
 writes the send descriptor to the ioremap'd NIC memory.  It is a
 multi-word descriptor.  So, to send it as one PCIE MWr transaction,
 the driver maps the whole BAR0 as WC and does copy descriptor; wmb.

Interesting, so you burst write multi-word descriptor writes using
write-combining here for the Ethernet device.

 Without WC, descriptors would end up as multiple 4B or 8B MWr packets
 to the NIC, which has a pretty big performance impact on this
 particular NIC.

How big are the descriptors?

 Most registers that do not want WC are actually in BAR2, which is not
 mapped as WC.  For registers that are in BAR0, we do write to the
 register; wmb.  If we want to wait till the NIC has seen the write,
 we do write; wmb; read.

Interesting, thanks, yeah using this as a work around to the problem sounds
plausible however it still would require likely making just as many changes to
the ivtv and ipath driver as to just do a proper split. I do wonder however if
this sort of work around can be generalized somehow though so that others could
use, if this sort of thing is going to become prevalent. If so then this would
serve two purposes: work around for the corner cases of MTRR use on Linux and
also these sorts of device constraints.

In order to determine if this is likely to be generally useful could you 
elaborate
a bit more about the detals of the performance issues of not bursting writes
for the descriptor on this device.

Even if that is done a conversion over to this work around seems it may require
device specific nitpicks. For instance I note in myri10ge_submit_req() for
small writes you just do a reverse write and do the first set last, then
finally the last 32 bits are rewritten, I guess to trigger something?

 This approach has worked for this device for many years.  I cannot say
 whether it works for other devices, though.

I think it should but the more interesting question would be exactly
*why* it was needed for this device, who determined that, and why?

  Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: ioremap_uc() followed by set_memory_wc() - burrying MTRR

[Luis R. Rodriguez]

Hey Andy, thanks for your review,  adding Hyong-Youb Kim for  review of the
full range ioremap_wc() idea below.

On Wed, Apr 15, 2015 at 06:38:51PM -0400, Andy Walls wrote:
 Hi All,
 
 On Mon, 2015-04-13 at 19:49 +0200, Luis R. Rodriguez wrote:
  From the beginning it seems only framebuffer devices used MTRR/WC,
 [snip]
   The ivtv device is a good example of the worst type of
  situations and these days. So perhap __arch_phys_wc_add() and a
  ioremap_ucminus() might be something more than transient unless hardware 
  folks
  get a good memo or already know how to just Do The Right Thing (TM).
 
 Just to reiterate a subtle point, use of the ivtvfb is *optional*.  A
 user may or may not load it.  When the user does load the ivtvfb driver,
 the ivtv driver has already been initialized and may have functions of
 the card already in use by userspace.

I suspected this and its why I note that a rewrite to address a clean
split with separate ioremap seems rather difficult in this case.

 Hopefully no one is trying to use the OSD as framebuffer and the video
 decoder/output engine for video display at the same time. 

Worst case concern I have also is the implications of having overlapping
ioremap() calls (as proposed in my last reply) for different memory types
and having the different virtual memory addresse used by different parts
of the driver. Its not clear to me what the hardware implications of this
are.

  But the video
 decoder/output device nodes may already be open for performing ioctl()
 functions so unmapping the decoder IO space out from under them, when
 loading the ivtvfb driver module, might not be a good thing. 

Using overlapping ioremap() calls with different memory types would address
this concern provided hardware won't barf both on the device and CPU. Hardware
folks could provide feedback or an ivtvfb user could test the patch supplied
on both non-PAT and PAT systems. Even so, who knows,  this might work on some
systems while not on others, only hardware folks would know.

An alternative... is to just ioremap_wc() the entire region, including
MMIO registers for these old devices. I see one ethernet driver that does
this, myri10ge, and am curious how and why they ended up deciding this
and if they have run into any issues. I wonder if this is a reasonable
comrpomise for these 2 remaining corner cases.

  Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Atheros driver

[Luis R. Rodriguez]

On Jan 6, 2008 4:21 PM, John W. Linville [EMAIL PROTECTED] wrote:

 On Sun, Jan 06, 2008 at 09:04:02PM +0100, Ralph Spitzner wrote:
  With Kernel 2.6.23.12  ath5k I get this:
  --
 
  Jan  6 20:42:44 meepmeep kernel: CPU:0
  Jan  6 20:42:44 meepmeep kernel: EIP:0060:[d0d98e61]Not tainted
  VLI
  Jan  6 20:42:44 meepmeep kernel: EFLAGS: 00010246   (2.6.23.12 #1)
  Jan  6 20:42:44 meepmeep kernel: EIP is at
  ieee80211_generic_frame_duration+0x21/0x70 [mac80211]
  Jan  6 20:42:44 meepmeep kernel: eax: c2c68180   ebx: c2c68180   ecx:
  000a   edx: 
  Jan  6 20:42:44 meepmeep kernel: esi:    edi: 000a   ebp:
  03e8   esp: c3a03dd8
  Jan  6 20:42:44 meepmeep kernel: ds: 007b   es: 007b   fs:   gs: 0033
  ss: 0068
  Jan  6 20:42:44 meepmeep kernel: Process ifconfig (pid: 5119, ti=c3a02000
  task=c363c570 task.ti=c3a02000)
  Jan  6 20:42:44 meepmeep kernel: Stack: d0e7539b 0002 876c 
  c4555000 d0e71c67 000a 1fb748ed
  Jan  6 20:42:44 meepmeep kernel:006b c2c68180  d0e76e44
  c2c68eec   0003
  Jan  6 20:42:44 meepmeep kernel:0002 0002 0014 
  d0e76e20 c2c68de0 0001 0003
  Jan  6 20:42:44 meepmeep kernel: Call Trace:
  Jan  6 20:42:44 meepmeep kernel:  [d0e7539b] ath5k_hw_rfgain+0x4b/0x80
  [ath5k]
  Jan  6 20:42:44 meepmeep kernel:  [d0e71c67] ath5k_hw_reset+0xaa7/0xeb0
  [ath5k]
  Jan  6 20:42:44 meepmeep kernel:  [d0e6a766] ath5k_init+0x46/0x110 [ath5k]
  Jan  6 20:42:44 meepmeep kernel:  [d0d86b7c] ieee80211_open+0x19c/0x4c0
  [mac80211]
  Jan  6 20:42:44 meepmeep kernel:  [c0142235] filemap_fault+0x1c5/0x3e0
  Jan  6 20:42:44 meepmeep kernel:  [c06a1353] dev_open+0x33/0x80
  Jan  6 20:42:44 meepmeep kernel:  [c069f1a9] dev_change_flags+0x149/0x1c0
  Jan  6 20:42:44 meepmeep kernel:  [c06e7261] devinet_ioctl+0x521/0x6c0
  Jan  6 20:42:44 meepmeep kernel:  [c069efbf] __dev_get_by_name+0x6f/0x90
  Jan  6 20:42:44 meepmeep kernel:  [c0693f4f] sock_ioctl+0xbf/0x230
  Jan  6 20:42:44 meepmeep kernel:  [c0113b2b] do_page_fault+0x18b/0x6d0
  Jan  6 20:42:44 meepmeep kernel:  [c0693e90] sock_ioctl+0x0/0x230
  Jan  6 20:42:44 meepmeep kernel:  [c01682cf] do_ioctl+0x1f/0x70
  Jan  6 20:42:44 meepmeep kernel:  [c016837c] vfs_ioctl+0x5c/0x260
  Jan  6 20:42:44 meepmeep kernel:  [c01685f2] sys_ioctl+0x72/0x90
  Jan  6 20:42:44 meepmeep kernel:  [c0104012] syscall_call+0x7/0xb
  Jan  6 20:42:44 meepmeep kernel:  [c071] __svc_create+0x1c0/0x1d0
  Jan  6 20:42:44 meepmeep kernel:  ===
  Jan  6 20:42:44 meepmeep kernel: Code: 5d c3 90 8d b4 26 00 00 00 00 83 ec
  14 89 5c 24 08 89 c3 89 74 24 0c 89 d6 89 7c 24 10
  89 cf 8b 4c 24 18 83 78 0c 02 74 31 31 d2 8b 86 d0 fd ff ff 89 14 24 89
  fa 83 e0 08 89 44 24 04 89 d8 e8
  Jan  6 20:42:44 meepmeep kernel: EIP: [d0d98e61]
  ieee80211_generic_frame_duration+0x21/0x70 [mac80211] SS:ESP 0068:c3a03dd8
  Jan  6 20:47:27 meepmeep kernel: ACPI: RTC can wake from S4
 
  --
 
 
  Just in case someone is interested in this..

 I'm sure someone is interested

This is fixed by the patch titled: [PATCH] ath5k: Fix frame duration
oops posted on January 4rth.

  Luis
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Fix infinite loop on dev_mc_unsync()

[Luis R. Rodriguez]

While reviewing net/core/dev_mcast.c I found what I think is an 
infinite loop on dev_mc_unsync(). This fixes it. We make use of
this guy on mac80211 in ieee80211_stop(). This is untested.

Signed-off-by: Luis R. Rodriguez [EMAIL PROTECTED]

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 15241cf..5373c03 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct 
net_device *from)
da = from-mc_list;
while (da != NULL) {
next = da-next;
-   if (!da-da_synced)
+   if (!da-da_synced) {
+   da = next;
continue;
+   }
__dev_addr_delete(to-mc_list, to-mc_count,
  da-da_addr, da-da_addrlen, 0);
da-da_synced = 0;
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Luis R. Rodriguez]

Sorry, forgot to CC David.

On Fri, Nov 09, 2007 at 10:11:35AM -0500, Luis R. Rodriguez wrote:

While reviewing net/core/dev_mcast.c I found what I think is an 
infinite loop on dev_mc_unsync(). This fixes it. We make use of
this guy on mac80211 in ieee80211_stop(). This is untested.

Signed-off-by: Luis R. Rodriguez [EMAIL PROTECTED]

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 15241cf..5373c03 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct 
net_device *from)
da = from-mc_list;
while (da != NULL) {
next = da-next;
-   if (!da-da_synced)
+   if (!da-da_synced) {
+   da = next;
continue;
+   }
__dev_addr_delete(to-mc_list, to-mc_count,
  da-da_addr, da-da_addrlen, 0);
da-da_synced = 0;
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Fix infinite loop on dev_mc_unsync()

[Luis R. Rodriguez]

On Fri, Nov 09, 2007 at 11:07:16AM -0800, Joe Perches wrote:
 On Fri, 2007-11-09 at 13:51 -0500, Luis R. Rodriguez wrote:
  While reviewing net/core/dev_mcast.c I found what I think is an 
  infinite loop on dev_mc_unsync(). This fixes it. We make use of
  this guy on mac80211 in ieee80211_stop(). This is untested.
  
  Signed-off-by: Luis R. Rodriguez [EMAIL PROTECTED]
  
  diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
  index 15241cf..5373c03 100644
  --- a/net/core/dev_mcast.c
  +++ b/net/core/dev_mcast.c
  @@ -168,8 +168,10 @@ void dev_mc_unsync(struct net_device *to, struct 
  net_device *from)
  da = from-mc_list;
  while (da != NULL) {
  next = da-next;
  -   if (!da-da_synced)
  +   if (!da-da_synced) {
  +   da = next;
  continue;
  +   }
  __dev_addr_delete(to-mc_list, to-mc_count,
da-da_addr, da-da_addrlen, 0);
  da-da_synced = 0;
  -
  To unsubscribe from this list: send the line unsubscribe netdev in
  the body of a message to [EMAIL PROTECTED]
  More majordomo info at  http://vger.kernel.org/majordomo-info.html
 
 Perhaps this is clearer as:
 
 void dev_mc_unsync(struct net_device *to, struct net_device *from)
 {
   struct dev_addr_list *da;
 
   netif_tx_lock_bh(from);
   netif_tx_lock_bh(to);
 
   da = from-mc_list;
   while (da) {
   if (da-da_synced) {
   __dev_addr_delete(to-mc_list, to-mc_count,
 da-da_addr, da-da_addrlen, 0);
   __dev_addr_delete(from-mc_list, from-mc_count,
 da-da_addr, da-da_addrlen, 0);
   da-da_synced = 0;
   }
   da = da-next;
   }
 
   __dev_set_rx_mode(to);
 
   netif_tx_unlock_bh(to);
   netif_tx_unlock_bh(from);
 }
 EXPORT_SYMBOL(dev_mc_unsync);

Sure, or better with a for loop and do away with next pointer then:

void dev_mc_unsync(struct net_device *to, struct net_device *from)
{
struct dev_addr_list *da;

netif_tx_lock_bh(from);
netif_tx_lock_bh(to);

for (da = from-mc_list; da; da = da-next) {
if (!da-da_synced)
continue;
__dev_addr_delete(to-mc_list, to-mc_count,
  da-da_addr, da-da_addrlen, 0);
da-da_synced = 0;
__dev_addr_delete(from-mc_list, from-mc_count,
  da-da_addr, da-da_addrlen, 0);
}
__dev_set_rx_mode(to);

netif_tx_unlock_bh(to);
netif_tx_unlock_bh(from);
}
EXPORT_SYMBOL(dev_mc_unsync);

Patch below.

Signed-off-by: Luis R. Rodriguez [EMAIL PROTECTED]

diff --git a/net/core/dev_mcast.c b/net/core/dev_mcast.c
index 15241cf..2aea8e1 100644
--- a/net/core/dev_mcast.c
+++ b/net/core/dev_mcast.c
@@ -160,14 +160,12 @@ EXPORT_SYMBOL(dev_mc_sync);
  */
 void dev_mc_unsync(struct net_device *to, struct net_device *from)
 {
-   struct dev_addr_list *da, *next;
+   struct dev_addr_list *da;
 
netif_tx_lock_bh(from);
netif_tx_lock_bh(to);
 
-   da = from-mc_list;
-   while (da != NULL) {
-   next = da-next;
+   for (da = from-mc_list; da; da = da-next) {
if (!da-da_synced)
continue;
__dev_addr_delete(to-mc_list, to-mc_count,
@@ -175,7 +173,6 @@ void dev_mc_unsync(struct net_device *to, struct net_device 
*from)
da-da_synced = 0;
__dev_addr_delete(from-mc_list, from-mc_count,
  da-da_addr, da-da_addrlen, 0);
-   da = next;
}
__dev_set_rx_mode(to);
 
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RFC: Reproducible oops with lockdep on count_matching_names()

[Luis R. Rodriguez]

On 11/2/07, Peter Zijlstra [EMAIL PROTECTED] wrote:
 On Thu, 2007-11-01 at 19:26 -0400, Michael Wu wrote:
  On Thursday 01 November 2007 15:17:16 Luis R. Rodriguez wrote:
   [EMAIL PROTECTED]:~/devel/wireless-2.6$ git-describe
   v2.6.24-rc1-146-g2280253
  
   So I hit segfault with lockdep on count_matching_names() on the
   strcmp() multiple times now. This is reproducible and with different
   wireless drivers.
  
  I've found the problem. It appears to be in lockdep. struct lock_class has a
  const char *name field which points to a statically allocated string that
  comes from the code which uses the lock. If that code/string is in a module
  and gets unloaded, the pointer in |name| is no longer valid. Next time this
  field is dereferenced (count_matching_names, in this case), we crash.
 
  The following patch fixes the issue but there's probably a better way.

 Thanks, and indeed. From my understanding lockdep_free_key_range()
 should destroy all classes of a module on module unload.

 So I'm not quite sure what has gone wrong here..

I've tried digging more and just am still not sure what caused this.
At first I thought perhaps all_lock_classes list had some element not
yet removed as lockdep_free_key_range() iterates over the hash tables
but this doesn't seem to be the case.

I was using SLAB and ran into other strange oops, as the one below,
but after switching to SLUB, after Michael Buesch's suggestion that
one went away... The lockdep segfault is still present, however.

Just not sure what's going on. Any ideas?

- oops with slab, not reproducible with slub:

[EMAIL PROTECTED]:~$ sudo rmmod tg3
[EMAIL PROTECTED]:~$ sudo rmmod sr_mod

*** dmesg -c

ACPI: PCI interrupt for device :02:00.0 disabled
BUG: unable to handle kernel paging request at virtual address f88a4a05
printing eip: f88a4a05 *pde = 0267 *pte = 
Oops:  [#1]
Modules linked in: sr_mod uinput thinkpad_acpi hwmon backlight nvram
ipv6 acpi_cpufreq cpufreq_userspace cpufreq_powersave cpufreq_ondemand
cpufreq_conservative dock arc4 ecb blkcipher cryptomgr crypto_algapi
rc80211_simple ath5k mac80211 cfg80211 pcmcia crc32 snd_hda_intel
snd_pcm_oss snd_mixer_oss snd_pcm snd_page_alloc snd_hwdep snd_seq_oss
ipw2200 snd_seq_midi_event ieee80211 ieee80211_crypt sg ehci_hcd
uhci_hcd yenta_socket rsrc_nonstatic snd_seq snd_timer snd_seq_device
firmware_class cdrom pcmcia_core usbcore evdev rng_core rtc snd
soundcore

Pid: 2908, comm: modprobe Not tainted (2.6.24-rc1 #18)
EIP: 0060:[f88a4a05] EFLAGS: 00010086 CPU: 0
EIP is at 0xf88a4a05
EAX: c20b75c8 EBX: c2f86f38 ECX: f88a4a05 EDX: c2f86f38
ESI: c20b75c8 EDI: c2f89c00 EBP: c3897bfc ESP: c3897be0
 DS: 007b ES: 007b FS:  GS: 0033 SS: 0068
Process modprobe (pid: 2908, ti=c3896000 task=c3935150 task.ti=c3896000)
Stack: c01b2afc c2f82d98 c3897bf4 c01ba8b6 c2f86f38 c20b75c8 c2f82c00 c3897c24
   c02186dd c2f86f38 c3897c24 c01b54c0 c20b75c8 0001 c20b75c8 c2f86f38
   c20b75c8 c3897c30 c01b54ed 0001 c3897c54 c01b556c 0001 c3897cd4
Call Trace:
 [c0104cec] show_trace_log_lvl+0x1a/0x2f
 [c0104d9e] show_stack_log_lvl+0x9d/0xa5
 [c0104e53] show_registers+0xad/0x17c
 [c0105017] die+0xf5/0x1c6
 [c0112715] do_page_fault+0x450/0x537
 [c02a835a] error_code+0x6a/0x70
 [c02186dd] scsi_request_fn+0x5f/0x2ec
 [c01b54ed] __generic_unplug_device+0x20/0x23
 [c01b556c] blk_execute_rq_nowait+0x7c/0x8f
 [c01b69e5] blk_execute_rq+0xb1/0xcf
 [c0217f53] scsi_execute+0xc4/0xd7
 [c0218014] scsi_execute_req+0xae/0xcb
 [f885f571] sr_probe+0x1d5/0x557 [sr_mod]
 [c020fd33] driver_probe_device+0xe8/0x168
 [c020fec9] __driver_attach+0x6a/0xa1
 [c020f271] bus_for_each_dev+0x36/0x5b
 [c020fb7f] driver_attach+0x19/0x1b
 [c020f556] bus_add_driver+0x73/0x1aa
 [c02100a5] driver_register+0x67/0x6c
 [c021b4f8] scsi_register_driver+0xf/0x11
 [f8863023] init_sr+0x23/0x3d [sr_mod]
 [c013a461] sys_init_module+0x1142/0x1262
 [c0103d7e] sysenter_past_esp+0x5f/0xa5
 ===
Code:  Bad EIP value.
EIP: [f88a4a05] 0xf88a4a05 SS:ESP 0068:c3897be0

  Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] pegasos_eth.c: Fix compile error over MV643XX_ defines

[Luis R. Rodriguez]

On 10/29/07, Dale Farnsworth [EMAIL PROTECTED] wrote:
 On Mon, Oct 29, 2007 at 05:27:29PM -0400, Luis R. Rodriguez wrote:
  This commit made an incorrect assumption:
  --
  Author: Lennert Buytenhek [EMAIL PROTECTED]
   Date:   Fri Oct 19 04:10:10 2007 +0200
 
  mv643xx_eth: Move ethernet register definitions into private header
 
  Move the mv643xx's ethernet-related register definitions from
  include/linux/mv643xx.h into drivers/net/mv643xx_eth.h, since
  they aren't of any use outside the ethernet driver.
 
  Signed-off-by: Lennert Buytenhek [EMAIL PROTECTED]
  Acked-by: Tzachi Perelstein [EMAIL PROTECTED]
  Signed-off-by: Dale Farnsworth [EMAIL PROTECTED]
  --
 
  arch/powerpc/platforms/chrp/pegasos_eth.c made use of a 3 defines there.
 
  [EMAIL PROTECTED]:~/devel/wireless-2.6$ git-describe
 
  v2.6.24-rc1-138-g0119130
 
  This patch fixes this by internalizing 3 defines onto pegasos which are
  simply no longer available elsewhere. Without this your compile will fail

 That compile failure was fixed in commit
 30e69bf4cce16d4c2dcfd629a60fcd8e1aba9fee by Al Viro.

 However, as I examine that commit, I see that it defines offsets from
 the eth block in the chip, rather than the full chip registeri block
 as the Pegasos 2 code expects.  So, I think it fixes the compile
 failure, but leaves the Pegasos 2 broken.

 Luis, do you have Pegasos 2 hardware?  Can you (or anyone) verify that
 the following patch is needed for the Pegasos 2?

Nope, sorry.

  Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] pegasos_eth.c: Fix compile error over MV643XX_ defines

[Luis R. Rodriguez]

This commit made an incorrect assumption:
--
Author: Lennert Buytenhek [EMAIL PROTECTED]
 Date:   Fri Oct 19 04:10:10 2007 +0200

mv643xx_eth: Move ethernet register definitions into private header

Move the mv643xx's ethernet-related register definitions from
include/linux/mv643xx.h into drivers/net/mv643xx_eth.h, since
they aren't of any use outside the ethernet driver.

Signed-off-by: Lennert Buytenhek [EMAIL PROTECTED]
Acked-by: Tzachi Perelstein [EMAIL PROTECTED]
Signed-off-by: Dale Farnsworth [EMAIL PROTECTED]
--

arch/powerpc/platforms/chrp/pegasos_eth.c made use of a 3 defines there.

[EMAIL PROTECTED]:~/devel/wireless-2.6$ git-describe 

v2.6.24-rc1-138-g0119130

This patch fixes this by internalizing 3 defines onto pegasos which are
simply no longer available elsewhere. Without this your compile will fail
whenever you enable 'Common Hardware Reference Platform (CHRP) based machines',
(CONFIG_PPC_CHRP) as the Makefile for chrp adds it always:

obj-y   += setup.o time.o pegasos_eth.o pci.o

If these defines are platform specific then they should later just be added 
back to include/linux/mv643xx.h.

Compile error:

  CC  arch/powerpc/platforms/chrp/pegasos_eth.o
arch/powerpc/platforms/chrp/pegasos_eth.c: In function 'Enable_SRAM':
arch/powerpc/platforms/chrp/pegasos_eth.c:150: error: 'MV643XX_ETH_BAR_4' 
undeclared (first use in this function)
arch/powerpc/platforms/chrp/pegasos_eth.c:150: error: (Each undeclared 
identifier is reported only once
arch/powerpc/platforms/chrp/pegasos_eth.c:150: error: for each function it 
appears in.)
arch/powerpc/platforms/chrp/pegasos_eth.c:152: error: 
'MV643XX_ETH_SIZE_REG_4' undeclared (first use in this function)
arch/powerpc/platforms/chrp/pegasos_eth.c:154: error: 
'MV643XX_ETH_BASE_ADDR_ENABLE_REG' undeclared (first use in this function)
make[2]: *** [arch/powerpc/platforms/chrp/pegasos_eth.o] Error 1
make[1]: *** [arch/powerpc/platforms/chrp] Error 2
make: *** [arch/powerpc/platforms] Error 2

Signed-off-by: Luis R. Rodriguez [EMAIL PROTECTED]
---

 arch/powerpc/platforms/chrp/pegasos_eth.c |   11 +++
 1 files changed, 7 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/platforms/chrp/pegasos_eth.c 
b/arch/powerpc/platforms/chrp/pegasos_eth.c
index 5bcc58d..1fc9e8c 100644
--- a/arch/powerpc/platforms/chrp/pegasos_eth.c
+++ b/arch/powerpc/platforms/chrp/pegasos_eth.c
@@ -24,6 +24,9 @@
 #define PEGASOS2_SRAM_BASE_ETH0(PEGASOS2_SRAM_BASE)
 #define PEGASOS2_SRAM_BASE_ETH1
(PEGASOS2_SRAM_BASE_ETH0 + (PEGASOS2_SRAM_SIZE / 2) )
 
+#define PEGASOS2_ETH_BAR_4 0x2220
+#define PEGASOS2_ETH_SIZE_REG_40x2224
+#define PEGASOS2_ETH_BASE_ADDR_ENABLE_REG  0x2290
 
 #define PEGASOS2_SRAM_RXRING_SIZE  (PEGASOS2_SRAM_SIZE/4)
 #define PEGASOS2_SRAM_TXRING_SIZE  (PEGASOS2_SRAM_SIZE/4)
@@ -147,13 +150,13 @@ static int Enable_SRAM(void)
 
ALong = 0x02;
ALong |= PEGASOS2_SRAM_BASE  0x;
-   MV_WRITE(MV643XX_ETH_BAR_4, ALong);
+   MV_WRITE(PEGASOS2_ETH_BAR_4, ALong);
 
-   MV_WRITE(MV643XX_ETH_SIZE_REG_4, (PEGASOS2_SRAM_SIZE-1)  0x);
+   MV_WRITE(PEGASOS2_ETH_SIZE_REG_4, (PEGASOS2_SRAM_SIZE-1)  0x);
 
-   MV_READ(MV643XX_ETH_BASE_ADDR_ENABLE_REG, ALong);
+   MV_READ(PEGASOS2_ETH_BASE_ADDR_ENABLE_REG, ALong);
ALong = ~(1  4);
-   MV_WRITE(MV643XX_ETH_BASE_ADDR_ENABLE_REG, ALong);
+   MV_WRITE(PEGASOS2_ETH_BASE_ADDR_ENABLE_REG, ALong);
 
 #ifdef BE_VERBOSE
printk(Pegasos II/Marvell MV64361: register unmapped\n);
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Fwd: That whole Linux stealing our code thing

[Luis R. Rodriguez]

I urge developers to not bait into this and just leave this alone.
Those involved know what they are doing and have a strong team of
attorneys watching their backs. Any *necessary* discussions are be
done privately.

  Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: That whole Linux stealing our code thing

[Luis R. Rodriguez]

On 9/1/07, Constantine A. Murenin [EMAIL PROTECTED] wrote:
 On 01/09/07, Jeff Garzik [EMAIL PROTECTED] wrote:
  Jason Dixon wrote:
   Once the grantor (Reyk) releases his code under that license, it must
   remain.  You are free to derive work and redistribute under your
   license, but the original copyright and license permission remains
   intact.  Many other entities (Microsoft, Apple, Sun, etc) have used BSD
   code and have no problem understanding this.  Why is this so difficult
   for the Linux brain share to absorb?
 
  Why is it so difficult to understand dual licensing?

 Maybe because Reyk's code was never dual-licensed?

We asked SFLC to work with us to make sure that everyone's copyrights
were respected in the right places, and that the licenses various developers
wanted for their copyrights were implemented correctly.  The patch I sent
implements SFLC's suggestions in that regard.

  Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/5] Net: ath5k, license is GPLv2

[Luis R. Rodriguez]

On 8/28/07, Christoph Hellwig [EMAIL PROTECTED] wrote:
 On Tue, Aug 28, 2007 at 12:00:50PM -0400, Jiri Slaby wrote:
  ath5k, license is GPLv2
 
  The files are available only under GPLv2 since now.

 Is this really a good idea?  Most of the reverse-engineering was
 done by the OpenBSD folks, and it would certainly be helpful to
 work together with them on new hardware revisions, etc..

Technically the best we can do is to leave the license as dual
licensed, but keep in that technically that means nothing and is just
for show, the GPL is what would apply as its derivative work and is
the most restrictive license. This applies to any other driver in the
kernel right now with a dual license tag.

  Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] Net: ath5k, split hw into hw, phy and initvals

[Luis R. Rodriguez]

On 8/28/07, Larry Finger [EMAIL PROTECTED] wrote:
 Johannes Berg wrote:
  On Tue, 2007-08-28 at 18:10 +0100, Christoph Hellwig wrote:
  On Tue, Aug 28, 2007 at 11:58:52AM -0400, Jiri Slaby wrote:
  -ath5k-objs = ath5k_base.o ath5k_hw.o ath5k_regdom.o
  +ath5k-objs = ath5k_base.o ath5k_hw.o ath5k_regdom.o \
  + ath5k_hw_phy.o ath5k_hw_inivals.o
  And while I'm at nitpicking :)
 
  ath5k_hw_phy.o should probably be ath5k_phy.o by conventions used by
  most drivers and ath5k_hw_inivals.o mights aswell be something like
  ath5k_init.o
 
  While we're at names... I personally much prefer a new directory ath5k
  and then ath5k/hw.c etc. but it seems I'm pretty alone in that. But here
  we already have at least four files now.

 I agree with you. If a wireless driver takes more than one .c and one .h 
 file, it belongs in its own
 directory, and the file names stripped of any driver prefix. Not only are the 
 file names simpler,
 but Makefile and Kconfig in wireless benefits.

NACK, I don't agree with this patch. The ath5k_regdom.* files will be
gone soon anyway and don't see the reason to split the files even
more. Now iwlwifi... -- that could is own directory IMHO :)

  Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marvell Libertas 8388 802.11 USB - added to Orbit

[Luis R. Rodriguez]

I've slapped the two Marvell Libertas 8388 802.11 USB cards onto
Winlab's Orbit testbed on sandbox 8. This allows anyone willing to
help hack on the driver with access to a node with the wireless card.

http://www.orbit-lab.org/wiki/Documentation/Developers

 Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Madwifi-devel] ar5k and Atheros AR5005G

[Luis R. Rodriguez]

On 12/5/06, Michael Renzmann [EMAIL PROTECTED] wrote:
openhal here.


I've set up the suggested branches, with slight changing to the proposed
names:

* http://svn.madwifi.org/branches/madwifi-old-openhal (based on r1142)
* http://svn.madwifi.org/branches/dadwifi-openhal (based on r1827)

Neither of them has received any modifications yet, they are basically
copies of the mentioned revisions. I'm currently lacking the time to
import Nick's work into the madwifi-old-openhal branch, for example - it
would be nice if someone else could work on that.


CC'ing Reyk Foeter.

Committed Nick's port of ar5k (openal) to both branches, both now have
the openhal. Please note (very important):

We are basing our openhal on OpenBSD's ar5k. Because of this and since
the GPL does not allow us to commit changes on the GPL version back to
the BSD version please sumbit your non-linux enhancements to the HAL
to Reyk Floeter [EMAIL PROTECTED] and CC madwifi-devel.

We have an openhal.org which we can start to use, should Reyk want, to
use as the master tree of the ar5k to make sure any enhancements on
the HAL go to both BSD and Linux. The licensing on the master tree
should be kept BSD as Reyk wants it to be. This way Linux or BSD
contributions can go into one tree and later revisions can be added to
dadwifi/openbsd with their own specific BSD'isms and Linux'isms. Keep
in mind the Linux port will remain dual licensed BSD/GPL. Let me know
what you think Reyk.

 Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Madwifi-devel] ar5k and Atheros AR5005G

[Luis R. Rodriguez]

On 12/5/06, Stephen Hemminger [EMAIL PROTECTED] wrote:

On Tue, 5 Dec 2006 10:15:33 -0500
Luis R. Rodriguez [EMAIL PROTECTED] wrote:

 On 12/5/06, Michael Renzmann [EMAIL PROTECTED] wrote:
  openhal here.
 
  I've set up the suggested branches, with slight changing to the proposed
  names:
 
  * http://svn.madwifi.org/branches/madwifi-old-openhal (based on r1142)
  * http://svn.madwifi.org/branches/dadwifi-openhal (based on r1827)
 
  Neither of them has received any modifications yet, they are basically
  copies of the mentioned revisions. I'm currently lacking the time to
  import Nick's work into the madwifi-old-openhal branch, for example - it
  would be nice if someone else could work on that.

 CC'ing Reyk Foeter.

 Committed Nick's port of ar5k (openal) to both branches, both now have
 the openhal. Please note (very important):

 We are basing our openhal on OpenBSD's ar5k. Because of this and since
 the GPL does not allow us to commit changes on the GPL version back to
 the BSD version please sumbit your non-linux enhancements to the HAL
 to Reyk Floeter [EMAIL PROTECTED] and CC madwifi-devel.

 We have an openhal.org which we can start to use, should Reyk want, to
 use as the master tree of the ar5k to make sure any enhancements on
 the HAL go to both BSD and Linux. The licensing on the master tree
 should be kept BSD as Reyk wants it to be. This way Linux or BSD
 contributions can go into one tree and later revisions can be added to
 dadwifi/openbsd with their own specific BSD'isms and Linux'isms. Keep
 in mind the Linux port will remain dual licensed BSD/GPL. Let me know
 what you think Reyk.

   Luis

All the more reason to merge the HAL in and make it disappear


Agreed in the long term -- but this should not happen immediately as
otherwise we'd only get a completely free driver by the time this
hardware is outdated and reached its end of life. For now I'd say lets
stick to dadwifi+openhal effort, analyze net80211 from the ground up
and port to devicescape any features we are currently lacking. I can
help work on this fulltime.

 Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [Madwifi-devel] ar5k and Atheros AR5005G

[Luis R. Rodriguez]

On 11/29/06, Stephen Hemminger [EMAIL PROTECTED] wrote:

On Wed, 29 Nov 2006 08:03:28 -0800
David Kimdon [EMAIL PROTECTED] wrote:

 On Wed, Nov 29, 2006 at 04:38:56PM +0100, Michael Buesch wrote:
  On Wednesday 29 November 2006 16:24, David Kimdon wrote:
   On Wed, Nov 29, 2006 at 04:12:33PM +0100, Michael Buesch wrote:
On Wednesday 29 November 2006 15:34, Nick Kossifidis wrote:
   Why do you say that?
  
   There is absolutely no reason why dadwifi can't be merged into the
   mainline once the hal issue is resolved.
 
  Last time we talked about that stuff, it was decided that
  we don't want a HAL... See archives.

 To be clear, that is all part of the hal issue that needs to be
 resolved.  Removing the hal abstraction is not difficult for an
 interested party once source for the hal is available.  The next step
 in such an effort would be to add an open hal to dadwifi, IMO.


Isn't it obvious. Planning from goal through intermediate steps gives:

0 - today (raw materials)
* softmac stack: d80211
* open hal: ar5k
* glue layer: dadwifi

1- put pieces together
* d80211 + dadwifi + ar5k


The problem actually is that ar5k port to Linux Nick wrote currently
only works with an older version of madwifi (svn 1142) which itself
needs some patching and that dadwifi is based on the latest and
greatest madwifi source base. So a d80211 + dadwifi + ar5k will only
work if Nick's ar5k port is extended to support the latest
madwifi/dadwifi. I haven't had time yet to determine the exact
requirements on Nick's ported openhal to work with dadwifi but that
should be our focus for now in parallel with completing dadwifi.

For those curious the attached patch fixes madwifi 1142 release for
use with kernels = 2.6.18-rc1. Add Nick's ar5k port to linux and
you'll have a Free driver ready. Since I see the mb's openhal git
repos is down here is a link for it as I checked it out on 2006-08-03:

http://www.kernel.org/pub/linux/kernel/people/mcgrof/openhal-2006-08-03.tar.gz

(give it sometime to sync)

To make things easier for development I'd like to suggest a few madwif
ibranches created:

* madwifi-1152-openhal: based on madwifi-1152, patched with my patch,
added openhal, old hal removed. Working free solution. Should exist
just as a reference and to allow users to checkout a working free
alternative in the mean time.

* madwifi-dadwifi-openal: based on the latest dadwifi with the
openhal, old hal removed. As dadwifi gets updated you can pull updates
to this branch. As the openhal advances you can make updates to the
openhal here.

Once we get dadwifi+openhal branch working I believe it should become
the trunk of madwifi.

Lastly, if anyone needs access to atheros hardware for development of
this driver or the openhal please let me know and you will get access
to a lot of nodes for development.

 Luis
diff -Naurp madwifi-openhal/ath/if_ath.c madwifi-openhal-fixed/ath/if_ath.c
--- madwifi-openhal/ath/if_ath.c	2005-06-24 06:41:22.0 -0400
+++ madwifi-openhal-fixed/ath/if_ath.c	2006-08-04 14:49:38.0 -0400
@@ -245,13 +245,20 @@ enum {
 #endif
 
 static	int countrycode = -1;
-MODULE_PARM(countrycode, i);
-MODULE_PARM_DESC(countrycode, Override default country code);
 static	int outdoor = -1;
-MODULE_PARM(outdoor, i);
-MODULE_PARM_DESC(outdoor, Enable/disable outdoor use);
 static	int xchanmode = -1;
+#if (LINUX_VERSION_CODE  KERNEL_VERSION(2,5,52))
+MODULE_PARM(countrycode, i);
+MODULE_PARM(outdoor, i);
 MODULE_PARM(xchanmode, i);
+#else
+#include linux/moduleparam.h
+module_param(countrycode, int, 0);
+module_param(outdoor, int, 0);
+module_param(xchanmode, int, 0);
+#endif
+MODULE_PARM_DESC(countrycode, Override default country code);
+MODULE_PARM_DESC(outdoor, Enable/disable outdoor use);
 MODULE_PARM_DESC(xchanmode, Enable/disable extended channel mode);
 
 int
diff -Naurp madwifi-openhal/openhal/ar5xxx.h madwifi-openhal-fixed/openhal/ar5xxx.h
--- madwifi-openhal/openhal/ar5xxx.h	2006-08-03 15:24:30.0 -0400
+++ madwifi-openhal-fixed/openhal/ar5xxx.h	2006-08-04 14:42:37.0 -0400
@@ -1425,7 +1425,7 @@ struct ar5k_srev_name {
 	{ 5111,	AR5K_VERSION_RAD,	AR5K_SREV_RAD_5111 },	\
 	{ 2111,	AR5K_VERSION_RAD,	AR5K_SREV_RAD_2111 },	\
 	{ 5112,	AR5K_VERSION_RAD,	AR5K_SREV_RAD_5112 },	\
-	{ 5112a,	AR5K_VERSION_RAD,	AR5K_SREV_RAD_5112A },	\ 
+	{ 5112a,	AR5K_VERSION_RAD,	AR5K_SREV_RAD_5112A },	\
 	{ 2112,	AR5K_VERSION_RAD,	AR5K_SREV_RAD_2112 },	\
 	{ 2112a,	AR5K_VERSION_RAD,	AR5K_SREV_RAD_2112A },	\
 	{ ,	AR5K_VERSION_RAD,	AR5K_SREV_UNKNOWN }	\

Re: [Madwifi-devel] ar5k and Atheros AR5005G

[Luis R. Rodriguez]

On 12/1/06, Luis R. Rodriguez [EMAIL PROTECTED] wrote:


* madwifi-1152-openhal: based on madwifi-1152, patched with my patch,
added openhal, old hal removed. Working free solution. Should exist
just as a reference and to allow users to checkout a working free
alternative in the mean time.


typo meant 1142
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH wireless-2.6-git] prism54: WPA/RSN support for fullmac cards

[Luis R. Rodriguez]

On 11/9/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:

Am Mittwoch, 8. November 2006 01:39 schrieben Sie:
 On Fri, Nov 03, 2006 at 01:41:46PM -0500, Luis R. Rodriguez wrote:
  On 11/3/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:
  yes, especially mgt_commit_list caused alot headaches, until I removed
  DOT11_OID_PSM from the cache list.
  Now, I can hammer it with ping -f for hours.
 
  nice, perhaps that's been the culprit all along... going to dig to see
  if I find a fullmac prism card. Will like to get this merged in.

 Any resolution on this?

no replies.
Seems like it works for just fine for everybody. ;)


I found a card, I just need time to test it. Dan didn't you say you
ran into issues with the patch on your card?

 Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH wireless-2.6-git] prism54: WPA/RSN support for fullmac cards

[Luis R. Rodriguez]

On 11/9/06, Luis R. Rodriguez [EMAIL PROTECTED] wrote:

On 11/9/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:
 Am Mittwoch, 8. November 2006 01:39 schrieben Sie:
  On Fri, Nov 03, 2006 at 01:41:46PM -0500, Luis R. Rodriguez wrote:
   On 11/3/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:
   yes, especially mgt_commit_list caused alot headaches, until I removed
   DOT11_OID_PSM from the cache list.
   Now, I can hammer it with ping -f for hours.
  
   nice, perhaps that's been the culprit all along... going to dig to see
   if I find a fullmac prism card. Will like to get this merged in.
 
  Any resolution on this?

 no replies.
 Seems like it works for just fine for everybody. ;)

I found a card, I just need time to test it. Dan didn't you say you
ran into issues with the patch on your card?

  Luis


CC'ing Dan to make sure he gets it ;)
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH wireless-2.6-git] prism54: WPA/RSN support for fullmac cards

[Luis R. Rodriguez]

On 11/3/06, Dan Williams [EMAIL PROTECTED] wrote:

On Tue, 2006-10-31 at 11:05 -0500, Dan Williams wrote:
 On Mon, 2006-10-30 at 15:17 -0500, Luis R. Rodriguez wrote:
  On 10/29/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:
   This patch completes WPA/RSN with TKIP support for all fullmac prism54 
cards.
   I removed all parts which are not relevant for wpa_supplicant (managed 
mode).
 
  This is great effort and thanks for it! My only concern with the patch
  is breaking compatiblity with 2.4 but then again I'm tired of having
  to keep this compat work up too and soon, after d80211 full force push
  it may not be possible. So I ACK it.

 I'd like to test it a bit first with my fullmac card before it gets
 committed/added...  But it's great to get some movement on this
 card/driver!

So my prism54 fullmac card appears to be broken, since it refuses to
associate with anything at all, even unencrypted access points.  It
doesn't throw any errors at all, and it appears to be operating
correctly other than not associating.


chunkeey, I figured your patch was well tested as you had it sitting
there for a while  I gotta find a fullmac working card (most of them
broke while debugging) to debug.


Is there any way to get more information out of the firmware about what
it's doing?


The driver supports prism54_get_oid and prism54_set_u32. The list of
OIDs available are on isl_oid.h on oid_num_t enum. Just an FYI you if
you need to access the *EX OIDs you just need to be in extended mode.
Also for WPA you need to be in extended mode. To say the least last
the driver was pretty unstable in extended mode last I used worked on
it. Dan do you get any funky mgt timeouts on your ring buffer?
Anything useful there?

 Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH wireless-2.6-git] prism54: WPA/RSN support for fullmac cards

[Luis R. Rodriguez]

On 11/3/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:

Am Freitag, 3. November 2006 17:06 schrieben Sie:
 On 11/3/06, Dan Williams [EMAIL PROTECTED] wrote:
  On Tue, 2006-10-31 at 11:05 -0500, Dan Williams wrote:
   On Mon, 2006-10-30 at 15:17 -0500, Luis R. Rodriguez wrote:
On 10/29/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:
 This patch completes WPA/RSN with TKIP support for all fullmac
 prism54 cards. I removed all parts which are not relevant for
 wpa_supplicant (managed mode).
   
This is great effort and thanks for it! My only concern with the
patch is breaking compatiblity with 2.4 but then again I'm tired of
having to keep this compat work up too and soon, after d80211 full
force push it may not be possible. So I ACK it.
  
   I'd like to test it a bit first with my fullmac card before it gets
   committed/added...  But it's great to get some movement on this
   card/driver!
 
  So my prism54 fullmac card appears to be broken, since it refuses to
  associate with anything at all, even unencrypted access points.  It
  doesn't throw any errors at all, and it appears to be operating
  correctly other than not associating.

 chunkeey, I figured your patch was well tested as you had it sitting
 there for a while  I gotta find a fullmac working card (most of them
 broke while debugging) to debug.

well, it was already tested by:
Maxi, Dan S., Gabriel, C and me.
The best configuration is: WPA2 with TKIP!

  Is there any way to get more information out of the firmware about what
  it's doing?

 The driver supports prism54_get_oid and prism54_set_u32. The list of
 OIDs available are on isl_oid.h on oid_num_t enum. Just an FYI you if
 you need to access the *EX OIDs you just need to be in extended mode.
 Also for WPA you need to be in extended mode. To say the least last
 the driver was pretty unstable in extended mode last I used worked on
 it. Dan do you get any funky mgt timeouts on your ring buffer?
 Anything useful there?

   Luis
yes, especially mgt_commit_list caused alot headaches, until I removed
DOT11_OID_PSM from the cache list.
Now, I can hammer it with ping -f for hours.


nice, perhaps that's been the culprit all along... going to dig to see
if I find a fullmac prism card. Will like to get this merged in.

 Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [patch] d80211: use pfifo_qdisc_ops rather than d80211-specific qdisc

[Luis R. Rodriguez]

On 11/2/06, Stephen Hemminger [EMAIL PROTECTED] wrote:


Please have Ethernet (and wireless) devices show up as eth%d. For the
master device, choose something else (mac%d ?).


If ultimately we're going to make wireless devices, as John puts it,
1st class citizens by making 802.11 a full protocol we should just
start sticking to a new convention. Before when we didn't have any
softmac drivers in the kernel ethX made sense, with softmac this isn't
so clear, but with 802.11 as a full protocol perhaps wlanX will
finally make perfect sense. For the master device yes, how about mac%d
as its there do 802.11 MAC management.
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH wireless-2.6-git] prism54: WPA/RSN support for fullmac cards

[Luis R. Rodriguez]

On 10/29/06, [EMAIL PROTECTED] [EMAIL PROTECTED] wrote:

This patch completes WPA/RSN with TKIP support for all fullmac prism54 cards.
I removed all parts which are not relevant for wpa_supplicant (managed mode).


This is great effort and thanks for it! My only concern with the patch
is breaking compatiblity with 2.4 but then again I'm tired of having
to keep this compat work up too and soon, after d80211 full force push
it may not be possible. So I ACK it.

 Luis
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html