linux-wireless.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
@ 2017-07-31 15:09 Rafał Miłecki
  2017-07-31 15:09 ` [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds Rafał Miłecki
                   ` (2 more replies)
  0 siblings, 3 replies; 10+ messages in thread
From: Rafał Miłecki @ 2017-07-31 15:09 UTC (permalink / raw)
  To: Luis R . Rodriguez, Greg Kroah-Hartman
  Cc: Bjorn Andersson, Daniel Wagner, David Woodhouse,
	Arend van Spriel, Rafael J . Wysocki, yi1.li, atull,
	Moritz Fischer, pmladek, Johannes Berg, emmanuel.grumbach,
	luciano.coelho, Kalle Valo, luto, Linus Torvalds, Kees Cook,
	AKASHI Takahiro, David Howells, pjones, Hans de Goede, alan,
	tytso, lkml, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
	Wright Feng, Pieter-Paul Giesberts, linux-wireless, netdev,
	Rafał Miłecki

From: Rafał Miłecki <rafal@milecki.pl>

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 <rafal@milecki.pl>
---
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, 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(&fw_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;
+
+	if (fw_opts->optional)
+		opt_flags |= FW_OPT_NO_WARN;
+
+	return __request_firmware_nowait(module, opt_flags, name, dev,
+					 GFP_KERNEL, context, cont);
+}
+EXPORT_SYMBOL(__request_firmware_async);
+
 #ifdef CONFIG_PM_SLEEP
 static ASYNC_DOMAIN_EXCLUSIVE(fw_cache_domain);
 
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0ccb8ac..a32b6e67462d 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -82,6 +82,19 @@ static inline int request_firmware_into_buf(const struct firmware **firmware_p,
 {
 	return -EINVAL;
 }
-
 #endif
+
+struct firmware_opts {
+	bool optional;
+};
+
+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));
+
+#define request_firmware_async(name, fw_opts, dev, context, cont)	\
+	__request_firmware_async(THIS_MODULE, name, fw_opts, dev,	\
+				 context, cont)
+
 #endif
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds
  2017-07-31 15:09 [PATCH V5 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki
@ 2017-07-31 15:09 ` Rafał Miłecki
  2017-08-01 21:20   ` Arend van Spriel
  2017-07-31 23:01 ` [PATCH V5 1/2] firmware: add more flexible request_firmware_async function kbuild test robot
  2017-08-02 21:30 ` Luis R. Rodriguez
  2 siblings, 1 reply; 10+ messages in thread
From: Rafał Miłecki @ 2017-07-31 15:09 UTC (permalink / raw)
  To: Luis R . Rodriguez, Greg Kroah-Hartman
  Cc: Bjorn Andersson, Daniel Wagner, David Woodhouse,
	Arend van Spriel, Rafael J . Wysocki, yi1.li, atull,
	Moritz Fischer, pmladek, Johannes Berg, emmanuel.grumbach,
	luciano.coelho, Kalle Valo, luto, Linus Torvalds, Kees Cook,
	AKASHI Takahiro, David Howells, pjones, Hans de Goede, alan,
	tytso, lkml, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
	Wright Feng, Pieter-Paul Giesberts, linux-wireless, netdev,
	Rafał Miłecki

From: Rafał Miłecki <rafal@milecki.pl>

Failing to load NVRAM *file* isn't critical if we manage to get platform
NVRAM in the fallback path. It means warnings like:
[   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
are unnecessary & disturbing for people with *platform* NVRAM as they
are not expected to have NVRAM file. This is a very common case for
Broadcom home routers.

Instead of printing warning immediately within the firmware subsystem
let's try our fallback code first. If that fails as well, then it's a
right moment to print an error.

This should reduce amount of false reports from users seeing this
warning while having wireless working perfectly fine with the platform
NVRAM.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
    messages to the firmware.c.
V3: Set FW_OPT_UEVENT to don't change behavior
V4: Switch to the new request_firmware_async syntax
V5: Rebase, update commit message, resend after drvdata discussion
---
 .../wireless/broadcom/brcm80211/brcmfmac/firmware.c    | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
index d231042f19d6..524442b3870f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
@@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 		raw_nvram = false;
 	} else {
 		data = bcm47xx_nvram_get_contents(&data_len);
-		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
-			goto fail;
+		if (!data) {
+			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");
+			if (!(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL)) {
+				brcmf_err("Loading NVRAM from %s and using platform one both failed\n",
+					  fwctx->nvram_name);
+				goto fail;
+			}
+		}
 		raw_nvram = true;
 	}
 
@@ -491,6 +497,9 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
 static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 {
 	struct brcmf_fw *fwctx = ctx;
+	struct firmware_opts fw_opts = {
+		.optional = true,
+	};
 	int ret = 0;
 
 	brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(fwctx->dev));
@@ -503,9 +512,8 @@ static void brcmf_fw_request_code_done(const struct firmware *fw, void *ctx)
 		goto done;
 
 	fwctx->code = fw;
-	ret = request_firmware_nowait(THIS_MODULE, true, fwctx->nvram_name,
-				      fwctx->dev, GFP_KERNEL, fwctx,
-				      brcmf_fw_request_nvram_done);
+	ret = request_firmware_async(fwctx->nvram_name, &fw_opts, fwctx->dev,
+				     fwctx, brcmf_fw_request_nvram_done);
 
 	/* pass NULL to nvram callback for bcm47xx fallback */
 	if (ret)
-- 
2.11.0

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
  2017-07-31 15:09 [PATCH V5 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki
  2017-07-31 15:09 ` [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds Rafał Miłecki
@ 2017-07-31 23:01 ` kbuild test robot
  2017-08-02 21:30 ` Luis R. Rodriguez
  2 siblings, 0 replies; 10+ messages in thread
From: kbuild test robot @ 2017-07-31 23:01 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: kbuild-all, Luis R . Rodriguez, Greg Kroah-Hartman,
	Bjorn Andersson, Daniel Wagner, David Woodhouse,
	Arend van Spriel, Rafael J . Wysocki, yi1.li, atull,
	Moritz Fischer, pmladek, Johannes Berg, emmanuel.grumbach,
	luciano.coelho, Kalle Valo, luto, Linus Torvalds, Kees Cook,
	AKASHI Takahiro, David Howells, pjones, Hans de Goede, alan,
	tytso, lkml, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
	Wright Feng, Pieter-Paul Giesberts, linux-wireless, netdev,
	Rafał Miłecki

[-- Attachment #1: Type: text/plain, Size: 11769 bytes --]

Hi Rafał,

[auto build test WARNING on driver-core/driver-core-testing]
[also build test WARNING on v4.13-rc3 next-20170731]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Rafa-Mi-ecki/firmware-add-more-flexible-request_firmware_async-function/20170801-033319
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
   include/linux/init.h:1: warning: no structured comments found
   include/linux/mod_devicetable.h:687: warning: Excess struct/union/enum/typedef member 'ver_major' description in 'fsl_mc_device_id'
   include/linux/mod_devicetable.h:687: warning: Excess struct/union/enum/typedef member 'ver_minor' description in 'fsl_mc_device_id'
   kernel/sched/core.c:2080: warning: No description found for parameter 'rf'
   kernel/sched/core.c:2080: warning: Excess function parameter 'cookie' description in 'try_to_wake_up_local'
   include/linux/wait.h:555: warning: No description found for parameter 'wq'
   include/linux/wait.h:555: warning: Excess function parameter 'wq_head' description in 'wait_event_interruptible_hrtimeout'
   include/linux/wait.h:759: warning: No description found for parameter 'wq_head'
   include/linux/wait.h:759: warning: Excess function parameter 'wq' description in 'wait_event_killable'
   include/linux/kthread.h:26: warning: Excess function parameter '...' description in 'kthread_create'
   kernel/sys.c:1: warning: no structured comments found
   include/linux/device.h:968: warning: No description found for parameter 'dma_ops'
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
>> drivers/base/firmware_class.c:1: warning: no structured comments found
   include/linux/iio/iio.h:603: warning: No description found for parameter 'trig_readonly'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'indio_dev'
   include/linux/iio/trigger.h:151: warning: No description found for parameter 'trig'
   include/linux/device.h:969: warning: No description found for parameter 'dma_ops'
   drivers/ata/libata-eh.c:1449: warning: No description found for parameter 'link'
   drivers/ata/libata-eh.c:1449: warning: Excess function parameter 'ap' description in 'ata_eh_done'
   drivers/ata/libata-eh.c:1590: warning: No description found for parameter 'qc'
   drivers/ata/libata-eh.c:1590: warning: Excess function parameter 'dev' description in 'ata_eh_request_sense'
   drivers/mtd/nand/nand_base.c:2751: warning: Excess function parameter 'cached' description in 'nand_write_page'
   drivers/mtd/nand/nand_base.c:2751: warning: Excess function parameter 'cached' description in 'nand_write_page'
   arch/s390/include/asm/cmb.h:1: warning: no structured comments found
   drivers/scsi/scsi_lib.c:1116: warning: No description found for parameter 'rq'
   drivers/scsi/constants.c:1: warning: no structured comments found
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'claimed'
   include/linux/usb/gadget.h:230: warning: No description found for parameter 'enabled'
   include/linux/usb/gadget.h:412: warning: No description found for parameter 'quirk_altset_not_supp'
   include/linux/usb/gadget.h:412: warning: No description found for parameter 'quirk_stall_not_supp'
   include/linux/usb/gadget.h:412: warning: No description found for parameter 'quirk_zlp_not_supp'
   fs/inode.c:1666: warning: No description found for parameter 'rcu'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_transaction'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_next_transaction'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_list'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_vfs_inode'
   include/linux/jbd2.h:443: warning: No description found for parameter 'i_flags'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_rsv_handle'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_reserved'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_type'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_line_no'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_start_jiffies'
   include/linux/jbd2.h:497: warning: No description found for parameter 'h_requested_credits'
   include/linux/jbd2.h:497: warning: No description found for parameter 'saved_alloc_context'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_chkpt_bhs'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_devname'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_average_commit_time'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_min_batch_time'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_max_batch_time'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_commit_callback'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_failed_commit'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_chksum_driver'
   include/linux/jbd2.h:1050: warning: No description found for parameter 'j_csum_seed'
   fs/jbd2/transaction.c:511: warning: No description found for parameter 'type'
   fs/jbd2/transaction.c:511: warning: No description found for parameter 'line_no'
   fs/jbd2/transaction.c:641: warning: No description found for parameter 'gfp_mask'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'set_busid'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'debugfs_init'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_open_object'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_close_object'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'prime_handle_to_fd'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'prime_fd_to_handle'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_export'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_import'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_pin'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_unpin'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_res_obj'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_get_sg_table'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_import_sg_table'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_vmap'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_vunmap'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_prime_mmap'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'gem_vm_ops'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'major'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'minor'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'patchlevel'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'name'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'desc'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'date'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'driver_features'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'ioctls'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'num_ioctls'
   include/drm/drm_drv.h:537: warning: No description found for parameter 'fops'
   include/drm/drm_color_mgmt.h:1: warning: no structured comments found
   drivers/gpu/drm/drm_syncobj.c:341: warning: Excess function parameter 'dev' description in 'drm_syncobj_open'
   drivers/gpu/drm/drm_syncobj.c:366: warning: Excess function parameter 'dev' description in 'drm_syncobj_release'
   include/drm/drm_syncobj.h:1: warning: no structured comments found
   drivers/gpu/drm/drm_syncobj.c:342: warning: Excess function parameter 'dev' description in 'drm_syncobj_open'
   drivers/gpu/drm/drm_syncobj.c:367: warning: Excess function parameter 'dev' description in 'drm_syncobj_release'
   drivers/gpu/host1x/bus.c:50: warning: Excess function parameter 'driver' description in 'host1x_subdev_add'
   Documentation/doc-guide/sphinx.rst:121: ERROR: Unknown target name: "sphinx c domain".
   kernel/sched/fair.c:7584: WARNING: Inline emphasis start-string without end-string.
   kernel/time/timer.c:1200: ERROR: Unexpected indentation.
   kernel/time/timer.c:1202: ERROR: Unexpected indentation.
   kernel/time/timer.c:1203: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:108: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:111: ERROR: Unexpected indentation.
   include/linux/wait.h:113: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:991: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:323: WARNING: Inline literal start-string without end-string.
   kernel/rcu/tree.c:3187: ERROR: Unexpected indentation.
   kernel/rcu/tree.c:3214: ERROR: Unexpected indentation.
   kernel/rcu/tree.c:3215: WARNING: Bullet list ends without a blank line; unexpected unindent.
   include/linux/iio/iio.h:219: ERROR: Unexpected indentation.
   include/linux/iio/iio.h:220: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/iio/iio.h:226: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/iio/industrialio-core.c:633: ERROR: Unknown target name: "iio_val".
   drivers/iio/industrialio-core.c:640: ERROR: Unknown target name: "iio_val".
   drivers/ata/libata-core.c:5906: ERROR: Unknown target name: "hw".
   drivers/message/fusion/mptbase.c:5051: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1897: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/pci/pci.c:3470: ERROR: Unexpected indentation.
   include/linux/regulator/driver.h:271: ERROR: Unknown target name: "regulator_regmap_x_voltage".

vim +1 drivers/base/firmware_class.c

^1da177e Linus Torvalds    2005-04-16 @1  /*
^1da177e Linus Torvalds    2005-04-16  2   * firmware_class.c - Multi purpose firmware loading support
^1da177e Linus Torvalds    2005-04-16  3   *
87d37a4f Markus Rechberger 2007-06-04  4   * Copyright (c) 2003 Manuel Estrada Sainz
^1da177e Linus Torvalds    2005-04-16  5   *
^1da177e Linus Torvalds    2005-04-16  6   * Please see Documentation/firmware_class/ for more information.
^1da177e Linus Torvalds    2005-04-16  7   *
^1da177e Linus Torvalds    2005-04-16  8   */
^1da177e Linus Torvalds    2005-04-16  9  

:::::: The code at line 1 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6750 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds
  2017-07-31 15:09 ` [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds Rafał Miłecki
@ 2017-08-01 21:20   ` Arend van Spriel
  0 siblings, 0 replies; 10+ messages in thread
From: Arend van Spriel @ 2017-08-01 21:20 UTC (permalink / raw)
  To: Rafał Miłecki, Luis R . Rodriguez, Greg Kroah-Hartman
  Cc: Bjorn Andersson, Daniel Wagner, David Woodhouse,
	Rafael J . Wysocki, yi1.li, atull, Moritz Fischer, pmladek,
	Johannes Berg, emmanuel.grumbach, luciano.coelho, Kalle Valo,
	luto, Linus Torvalds, Kees Cook, AKASHI Takahiro, David Howells,
	pjones, Hans de Goede, alan, tytso, lkml, Franky Lin,
	Hante Meuleman, Chi-Hsien Lin, Wright Feng,
	Pieter-Paul Giesberts, linux-wireless, netdev,
	Rafał Miłecki

On 31-07-17 17:09, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Failing to load NVRAM *file* isn't critical if we manage to get platform
> NVRAM in the fallback path. It means warnings like:
> [   10.801506] brcmfmac 0000:01:00.0: Direct firmware load for brcm/brcmfmac43602-pcie.txt failed with error -2
> are unnecessary & disturbing for people with *platform* NVRAM as they
> are not expected to have NVRAM file. This is a very common case for
> Broadcom home routers.
> 
> Instead of printing warning immediately within the firmware subsystem
> let's try our fallback code first. If that fails as well, then it's a
> right moment to print an error.
> 
> This should reduce amount of false reports from users seeing this
> warning while having wireless working perfectly fine with the platform
> NVRAM.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> V2: Update commit message as it wasn't clear enough (thanks Andy) & add extra
>     messages to the firmware.c.
> V3: Set FW_OPT_UEVENT to don't change behavior
> V4: Switch to the new request_firmware_async syntax
> V5: Rebase, update commit message, resend after drvdata discussion
> ---
>  .../wireless/broadcom/brcm80211/brcmfmac/firmware.c    | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> index d231042f19d6..524442b3870f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c
> @@ -462,8 +462,14 @@ static void brcmf_fw_request_nvram_done(const struct firmware *fw, void *ctx)
>  		raw_nvram = false;
>  	} else {
>  		data = bcm47xx_nvram_get_contents(&data_len);
> -		if (!data && !(fwctx->flags & BRCMF_FW_REQ_NV_OPTIONAL))
> -			goto fail;
> +		if (!data) {
> +			brcmf_dbg(TRACE, "Failed to get platform NVRAM\n");

Better make this INFO level instead of TRACE. The intent of TRACE level
is for entry/exit points in functions.

Regards,
Arend

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
  2017-07-31 15:09 [PATCH V5 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki
  2017-07-31 15:09 ` [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds Rafał Miłecki
  2017-07-31 23:01 ` [PATCH V5 1/2] firmware: add more flexible request_firmware_async function kbuild test robot
@ 2017-08-02 21:30 ` Luis R. Rodriguez
  2017-08-03  5:23   ` Kalle Valo
  2 siblings, 1 reply; 10+ messages in thread
From: Luis R. Rodriguez @ 2017-08-02 21:30 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Luis R . Rodriguez, Greg Kroah-Hartman, Bjorn Andersson,
	Daniel Wagner, David Woodhouse, Arend van Spriel,
	Rafael J . Wysocki, yi1.li, atull, Moritz Fischer, pmladek,
	Johannes Berg, emmanuel.grumbach, luciano.coelho, Kalle Valo,
	luto, Linus Torvalds, Kees Cook, AKASHI Takahiro, David Howells,
	pjones, Hans de Goede, alan, tytso, lkml, Franky Lin,
	Hante Meuleman, Chi-Hsien Lin, Wright Feng,
	Pieter-Paul Giesberts, linux-wireless, netdev,
	Rafał Miłecki

On Mon, Jul 31, 2017 at 05:09:44PM +0200, Rafał Miłecki wrote:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> 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 <rafal@milecki.pl>
> ---
> 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, 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(&fw_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 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.

  Luis

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
  2017-08-02 21:30 ` Luis R. Rodriguez
@ 2017-08-03  5:23   ` Kalle Valo
  2017-08-03  5:55     ` Coelho, Luciano
  2017-08-10 17:05     ` Luis R. Rodriguez
  0 siblings, 2 replies; 10+ messages in thread
From: Kalle Valo @ 2017-08-03  5:23 UTC (permalink / raw)
  To: Luis R. Rodriguez
  Cc: Rafał Miłecki, Greg Kroah-Hartman, Bjorn Andersson,
	Daniel Wagner, David Woodhouse, Arend van Spriel,
	Rafael J . Wysocki, yi1.li, atull, Moritz Fischer, pmladek,
	Johannes Berg, emmanuel.grumbach, luciano.coelho, luto,
	Linus Torvalds, Kees Cook, AKASHI Takahiro, David Howells,
	pjones, Hans de Goede, alan, tytso, lkml, Franky Lin,
	Hante Meuleman, Chi-Hsien Lin, Wright Feng,
	Pieter-Paul Giesberts, linux-wireless, netdev,
	Rafał Miłecki

"Luis R. Rodriguez" <mcgrof@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.

-- 
Kalle Valo

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
  2017-08-03  5:23   ` Kalle Valo
@ 2017-08-03  5:55     ` Coelho, Luciano
  2017-08-03 16:31       ` Luis R. Rodriguez
  2017-08-10 17:07       ` Luis R. Rodriguez
  2017-08-10 17:05     ` Luis R. Rodriguez
  1 sibling, 2 replies; 10+ messages in thread
From: Coelho, Luciano @ 2017-08-03  5:55 UTC (permalink / raw)
  To: kvalo, mcgrof
  Cc: pieter-paul.giesberts, bjorn.andersson, arend.vanspriel,
	hante.meuleman, gregkh, keescook, linux-wireless, alan,
	moritz.fischer, pjones, wagi, pmladek, atull, yi1.li,
	wright.feng, torvalds, netdev, luto, dwmw2, takahiro.akashi, rjw,
	hdegoede, rafal, Berg, Johannes, zajec5, tytso, dhowells,
	Grumbach, Emmanuel, chi-hsien.lin, linux-kernel, franky.lin

T24gVGh1LCAyMDE3LTA4LTAzIGF0IDA4OjIzICswMzAwLCBLYWxsZSBWYWxvIHdyb3RlOg0KPiAi
THVpcyBSLiBSb2RyaWd1ZXoiIDxtY2dyb2ZAa2VybmVsLm9yZz4gd3JpdGVzOg0KPiANCj4gPiA+
ICtpbnQgcmVxdWVzdF9maXJtd2FyZV9ub3dhaXQoc3RydWN0IG1vZHVsZSAqbW9kdWxlLCBib29s
IHVldmVudCwNCj4gPiA+ICsJCQkgICAgY29uc3QgY2hhciAqbmFtZSwgc3RydWN0IGRldmljZSAq
ZGV2aWNlLCBnZnBfdCBnZnAsDQo+ID4gPiArCQkJICAgIHZvaWQgKmNvbnRleHQsDQo+ID4gPiAr
CQkJICAgIHZvaWQgKCpjb250KShjb25zdCBzdHJ1Y3QgZmlybXdhcmUgKmZ3LCB2b2lkICpjb250
ZXh0KSkNCj4gPiA+ICt7DQo+ID4gPiArCXVuc2lnbmVkIGludCBvcHRfZmxhZ3MgPSBGV19PUFRf
RkFMTEJBQ0sgfA0KPiA+ID4gKwkJKHVldmVudCA/IEZXX09QVF9VRVZFTlQgOiBGV19PUFRfVVNF
UkhFTFBFUik7DQo+ID4gPiArDQo+ID4gPiArCXJldHVybiBfX3JlcXVlc3RfZmlybXdhcmVfbm93
YWl0KG1vZHVsZSwgb3B0X2ZsYWdzLCBuYW1lLCBkZXZpY2UsIGdmcCwNCj4gPiA+ICsJCQkJCSBj
b250ZXh0LCBjb250KTsNCj4gPiA+ICt9DQo+ID4gPiAgRVhQT1JUX1NZTUJPTChyZXF1ZXN0X2Zp
cm13YXJlX25vd2FpdCk7DQo+ID4gPiAgDQo+ID4gPiAraW50IF9fcmVxdWVzdF9maXJtd2FyZV9h
c3luYyhzdHJ1Y3QgbW9kdWxlICptb2R1bGUsIGNvbnN0IGNoYXIgKm5hbWUsDQo+ID4gPiArCQkJ
ICAgICBzdHJ1Y3QgZmlybXdhcmVfb3B0cyAqZndfb3B0cywgc3RydWN0IGRldmljZSAqZGV2LA0K
PiA+ID4gKwkJCSAgICAgdm9pZCAqY29udGV4dCwNCj4gPiA+ICsJCQkgICAgIHZvaWQgKCpjb250
KShjb25zdCBzdHJ1Y3QgZmlybXdhcmUgKmZ3LCB2b2lkICpjb250ZXh0KSkNCj4gPiA+ICt7DQo+
ID4gPiArCXVuc2lnbmVkIGludCBvcHRfZmxhZ3MgPSBGV19PUFRfVUVWRU5UOw0KPiA+IA0KPiA+
IFRoaXMgZXhwb3NlcyBhIGxvbmcgaXNzdWUuIFRoaW5rIC0tIHdoeSBkbyB3ZSB3YW50IHRoaXMg
ZW5hYmxlZCBieSBkZWZhdWx0PyBJdHMNCj4gPiBhY3R1YWxseSBiZWNhdXNlIGV2ZW4gdGhvdWdo
IHRoZSBmYWxsYmFjayBzdHVmZiBpcyBvcHRpb25hbCBhbmQgY2FuIGJlLCB0aGUgdWV2ZW50DQo+
ID4gaW50ZXJuYWwgZmxhZyAqYWxzbyogcHJvdmlkZXMgY2FjaGluZyBzdXBwb3J0IGFzIGEgc2lk
ZSBjb25zZXF1ZW5jZSBvbmx5LiBXZQ0KPiA+IGRvbid0IHdhbnQgdG8gYWRkIGEgbmV3IEFQSSB3
aXRob3V0IGZpcnN0IGNsZWFuaW5nIHVwIHRoYXQgbWVzcy4NCj4gPiANCj4gPiBUaGlzIGlzIGEg
c2xpcGVyeSBzbG9wZSBhbmQgYmVzdCB0byBjbGVhbiB0aGF0IHVwIGJlZm9yZSBhZGRpbmcgYW55
IG5ldyBBUEkuDQo+ID4gDQo+ID4gVGhhdCBhbmQgYWxzbyBHcmVnIHJlY2VudGx5IHN0YXRlZCBo
ZSB3b3VsZCBsaWtlIHRvIHNlZSBhdCBsZWFzdCAzIHVzZXJzIG9mDQo+ID4gYSBmZWF0dXJlIGJl
Zm9yZSBhZGRpbmcgaXQuIEFsdGhvdWdoIEkgdGhpbmsgdGhhdCdzIHByZXR0eSBhcmJpdHJhcnks
IGFuZA0KPiA+IGNvbnNpZGVyaW5nIHRoYXQgcmVxdWVzdF9maXJtd2FyZV9pbnRvX2J1ZigpIG9u
bHkgaGFzICpvbmUqIHVzZXIgLS0gaXRzIHdoYXQNCj4gPiBoZSB3aXNoZXMuDQo+IA0KPiBhdGgx
MGsgYXQgbGVhc3QgbmVlZHMgYSB3YXkgdG8gc2lsZW5jZSB0aGUgd2FybmluZyBmb3IgbWlzc2lu
ZyBmaXJtd2FyZQ0KPiBhbmQgSSB0aGluayBpd2x3aWZpIGFsc28uDQoNClllcywgaXdsd2lmaSBu
ZWVkcyB0byBzaWxlbmNlIHRoZSB3YXJuaW5nLiAgSXQgdGhlIGZlYXR1cmUgKG9ubHkgb25lLA0K
cmVhbGx5KSB0aGF0IEkndmUgYmVlbiB3YWl0aW5nIGZvci4NCg0KLS0NCkx1Y2Eu

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
  2017-08-03  5:55     ` Coelho, Luciano
@ 2017-08-03 16:31       ` Luis R. Rodriguez
  2017-08-10 17:07       ` Luis R. Rodriguez
  1 sibling, 0 replies; 10+ messages in thread
From: Luis R. Rodriguez @ 2017-08-03 16:31 UTC (permalink / raw)
  To: Coelho, Luciano
  Cc: kvalo, mcgrof, pieter-paul.giesberts, bjorn.andersson,
	arend.vanspriel, hante.meuleman, gregkh, keescook,
	linux-wireless, alan, moritz.fischer, pjones, wagi, pmladek,
	atull, yi1.li, wright.feng, torvalds, netdev, luto, dwmw2,
	takahiro.akashi, rjw, hdegoede, rafal, Berg, Johannes, zajec5,
	tytso, dhowells, Grumbach, Emmanuel, chi-hsien.lin, linux-kernel,
	franky.lin

On Thu, Aug 03, 2017 at 05:55:18AM +0000, Coelho, Luciano wrote:
> On Thu, 2017-08-03 at 08:23 +0300, Kalle Valo wrote:
> > "Luis R. Rodriguez" <mcgrof@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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
  2017-08-03  5:23   ` Kalle Valo
  2017-08-03  5:55     ` Coelho, Luciano
@ 2017-08-10 17:05     ` Luis R. Rodriguez
  1 sibling, 0 replies; 10+ messages in thread
From: Luis R. Rodriguez @ 2017-08-10 17:05 UTC (permalink / raw)
  To: Kalle Valo
  Cc: Luis R. Rodriguez, Rafał Miłecki, Greg Kroah-Hartman,
	Bjorn Andersson, Daniel Wagner, David Woodhouse,
	Arend van Spriel, Rafael J . Wysocki, yi1.li, atull,
	Moritz Fischer, pmladek, Johannes Berg, emmanuel.grumbach,
	luciano.coelho, luto, Linus Torvalds, Kees Cook, AKASHI Takahiro,
	David Howells, pjones, Hans de Goede, alan, tytso, lkml,
	Franky Lin, Hante Meuleman, Chi-Hsien Lin, Wright Feng,
	Pieter-Paul Giesberts, linux-wireless, netdev,
	Rafał Miłecki

On Thu, Aug 03, 2017 at 08:23:00AM +0300, Kalle Valo wrote:
> "Luis R. Rodriguez" <mcgrof@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-<bus>-<id>.bin */                                            
        scnprintf(filename, sizeof(filename), "pre-cal-%s-%s.bin", 

2)

        /* cal-<bus>-<id>.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,             
                                                       &ar->normal_mode_fw.fw_file);
                if (!ret)                                                       
                        goto success;                                           
        }    

This ends up using the ar->hw_params.fw.dir/fw_name but fw_name is constructed
above using a API revision scheme. The fw_name can be of two types as per
ath10k_core_get_fw_name():

For SDIO:

                scnprintf(fw_name, fw_name_len, "%s-%s-%d.bin",                 
                          ATH10K_FW_FILE_BASE, ath10k_bus_str(ar->hif.bus),     
                          fw_api); 

For PCI/AHB:

                scnprintf(fw_name, fw_name_len, "%s-%d.bin",                    
                          ATH10K_FW_FILE_BASE, fw_api);   

For this the problem is at least one firmware is required so no complaint is
issued using request_firmware_direct() at all so you have to do that yourself
on ath10k_core_fetch_firmware_files():

                                                                                
        /* we end up here if we couldn't fetch any firmware */                  
    
        ath10k_err(ar, "Failed to find firmware-N.bin (N between %d and %d) from %s: %d",
                   ATH10K_FW_API_MIN, ATH10K_FW_API_MAX, ar->hw_params.fw.dir,  
                   ret);  

This is *fine* as per the API but it just means you are troubled to keep this
error warning. Correct me if I'm wrong but this does not seem to me like a big
issue and your issue is not in any way related to the patch proposed.

*But* the whole API revision scheme is something I figured could be generalized
and have code which lets us specify a name template, and let the driver
specific a series of API revision numbers and then the firmware API will do the
hunting for us. Then at least one API versioned file is hunted for. Since you
got from ATH10K_FW_API_MAX (6) down to ATH10K_FW_API_MIN (2) and I had used u8
for it then we have a match in terms of compatibility.

Turns out Intel uses a similar scheme, this is what it would look like for
iwlwifi to change to it, your driver similarly could cope:

https://lkml.kernel.org/r/20170605213937.26215-6-mcgrof@kernel.org

If this seems appealing I could do the conversion for you later once
I add this upstream. I'd need one more driver which has a similar API
revision scheme.

 Luis

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH V5 1/2] firmware: add more flexible request_firmware_async function
  2017-08-03  5:55     ` Coelho, Luciano
  2017-08-03 16:31       ` Luis R. Rodriguez
@ 2017-08-10 17:07       ` Luis R. Rodriguez
  1 sibling, 0 replies; 10+ messages in thread
From: Luis R. Rodriguez @ 2017-08-10 17:07 UTC (permalink / raw)
  To: Coelho, Luciano
  Cc: kvalo, mcgrof, pieter-paul.giesberts, bjorn.andersson,
	arend.vanspriel, hante.meuleman, gregkh, keescook,
	linux-wireless, alan, moritz.fischer, pjones, wagi, pmladek,
	atull, yi1.li, wright.feng, torvalds, netdev, luto, dwmw2,
	takahiro.akashi, rjw, hdegoede, rafal, Berg, Johannes, zajec5,
	tytso, dhowells, Grumbach, Emmanuel, chi-hsien.lin, linux-kernel,
	franky.lin

On Thu, Aug 03, 2017 at 05:55:18AM +0000, Coelho, Luciano wrote:
> On Thu, 2017-08-03 at 08:23 +0300, Kalle Valo wrote:
> > "Luis R. Rodriguez" <mcgrof@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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2017-08-10 17:07 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-07-31 15:09 [PATCH V5 1/2] firmware: add more flexible request_firmware_async function Rafał Miłecki
2017-07-31 15:09 ` [PATCH V5 2/2] brcmfmac: don't warn user about NVRAM if fallback to the platform one succeeds Rafał Miłecki
2017-08-01 21:20   ` Arend van Spriel
2017-07-31 23:01 ` [PATCH V5 1/2] firmware: add more flexible request_firmware_async function kbuild test robot
2017-08-02 21:30 ` Luis R. Rodriguez
2017-08-03  5:23   ` Kalle Valo
2017-08-03  5:55     ` Coelho, Luciano
2017-08-03 16:31       ` Luis R. Rodriguez
2017-08-10 17:07       ` Luis R. Rodriguez
2017-08-10 17:05     ` Luis R. Rodriguez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).