| Message ID | 20170731150945.8925-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Kalle Valo |
| Headers | show |
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
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
"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.
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.
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
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
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
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