Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752060AbdFZUXF (ORCPT ); Mon, 26 Jun 2017 16:23:05 -0400 Received: from mail-pg0-f44.google.com ([74.125.83.44]:36651 "EHLO mail-pg0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751934AbdFZUW6 (ORCPT ); Mon, 26 Jun 2017 16:22:58 -0400 MIME-Version: 1.0 In-Reply-To: <20170623160321.GA19720@kroah.com> References: <20170623160321.GA19720@kroah.com> From: Bjorn Andersson Date: Mon, 26 Jun 2017 13:22:41 -0700 Message-ID: Subject: Re: [PATCH] firmware: remove request_firmware_into_buf() To: Greg Kroah-Hartman Cc: "Luis R. Rodriguez" , Daniel Wagner , David Woodhouse , rafal@milecki.pl, Arend van Spriel , "Rafael J. Wysocki" , yi1.li@linux.intel.com, atull@opensource.altera.com, Moritz Fischer , pmladek@suse.com, Johannes Berg , emmanuel.grumbach@intel.com, luciano.coelho@intel.com, Kalle Valo , luto@kernel.org, Linus Torvalds , Kees Cook , "AKASHI, Takahiro" , David Howells , pjones@redhat.com, Hans de Goede , alan@linux.intel.com, tytso@mit.edu, lkml Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7543 Lines: 173 On Fri, Jun 23, 2017 at 9:03 AM, Greg Kroah-Hartman wrote: > As Luis pointed out, there are no in-kernel users of > request_firmware_into_buf(), so remove it, and the now unused internal > flag, which simplifies the logic around buffer handling a bit. > This API was implemented to reduce the memory pressure during firmware load in the Qualcomm remoteprocs, but it wasn't available when I upstreamed that code and I apparently forgot to send out the patch moving us over to use this API... Especially when loading the Qualcomm modem we have a couple of files that we request_firmware() that are 10-15MB in size, so this functionality is definitely wanted. As we are calling release_firmware() immediately following the request_firmware() I did attempt to just call kernel_read_file_from_path() directly, but as I don't have access to the fw_path[] this becomes inconsistent, so I would like to keep request_firmware_into_buf(). Regards, Bjorn > Reported-by: "Luis R. Rodriguez" > Signed-off-by: Greg Kroah-Hartman > --- > Documentation/driver-api/firmware/fallback-mechanisms.rst | 3 - > Documentation/driver-api/firmware/firmware_cache.rst | 2 > Documentation/driver-api/firmware/request_firmware.rst | 5 - > drivers/base/firmware_class.c | 37 -------------- > include/linux/firmware.h | 8 --- > 5 files changed, 4 insertions(+), 51 deletions(-) > > > diff --git a/Documentation/driver-api/firmware/fallback-mechanisms.rst b/Documentation/driver-api/firmware/fallback-mechanisms.rst > index d19354794e67..0c3562148161 100644 > --- a/Documentation/driver-api/firmware/fallback-mechanisms.rst > +++ b/Documentation/driver-api/firmware/fallback-mechanisms.rst > @@ -38,8 +38,7 @@ fallback mechanism: > * Race against access with the root filesystem upon bootup. > > * Races upon resume from suspend. This is resolved by the firmware cache, but > - the firmware cache is only supported if you use uevents, and its not > - supported for request_firmware_into_buf(). > + the firmware cache is only supported if you use uevents. > > * Firmware is not accessible through typical means: > * It cannot be installed into the root filesystem > diff --git a/Documentation/driver-api/firmware/firmware_cache.rst b/Documentation/driver-api/firmware/firmware_cache.rst > index 2210e5bfb332..e53f8189fac1 100644 > --- a/Documentation/driver-api/firmware/firmware_cache.rst > +++ b/Documentation/driver-api/firmware/firmware_cache.rst > @@ -24,7 +24,7 @@ root filesystem mounts. > Some implementation details about the firmware cache setup: > > * The firmware cache is setup by adding a devres entry for each device that > - uses all synchronous call except :c:func:`request_firmware_into_buf`. > + uses a synchronous call. > > * 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 > diff --git a/Documentation/driver-api/firmware/request_firmware.rst b/Documentation/driver-api/firmware/request_firmware.rst > index cc0aea880824..a5dacf96ab0f 100644 > --- a/Documentation/driver-api/firmware/request_firmware.rst > +++ b/Documentation/driver-api/firmware/request_firmware.rst > @@ -25,11 +25,6 @@ request_firmware_direct > .. kernel-doc:: drivers/base/firmware_class.c > :functions: request_firmware_direct > > -request_firmware_into_buf > -------------------------- > -.. kernel-doc:: drivers/base/firmware_class.c > - :functions: request_firmware_into_buf > - > Asynchronous firmware requests > ============================== > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c > index ac350c518e0c..a913400c873e 100644 > --- a/drivers/base/firmware_class.c > +++ b/drivers/base/firmware_class.c > @@ -196,7 +196,6 @@ static int __fw_state_check(struct fw_state *fw_st, enum fw_status status) > #define FW_OPT_FALLBACK 0 > #endif > #define FW_OPT_NO_WARN (1U << 3) > -#define FW_OPT_NOCACHE (1U << 4) > > struct firmware_cache { > /* firmware_buf instance will be added into the below list */ > @@ -1143,16 +1142,14 @@ static int assign_firmware_buf(struct firmware *fw, struct device *device, > * should be fixed in devres or driver core. > */ > /* don't cache firmware handled without uevent */ > - if (device && (opt_flags & FW_OPT_UEVENT) && > - !(opt_flags & FW_OPT_NOCACHE)) > + if (device && (opt_flags & FW_OPT_UEVENT)) > fw_add_devm_name(device, buf->fw_id); > > /* > * After caching firmware image is started, let it piggyback > * on request firmware. > */ > - if (!(opt_flags & FW_OPT_NOCACHE) && > - buf->fwc->state == FW_LOADER_START_CACHE) { > + if (buf->fwc->state == FW_LOADER_START_CACHE) { > if (fw_cache_piggyback_on_request(buf->fw_id)) > kref_get(&buf->ref); > } > @@ -1292,36 +1289,6 @@ int request_firmware_direct(const struct firmware **firmware_p, > EXPORT_SYMBOL_GPL(request_firmware_direct); > > /** > - * 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 > - * @buf: address of buffer to load firmware into > - * @size: size of buffer > - * > - * This function works pretty much like request_firmware(), but it doesn't > - * allocate a buffer to hold the firmware data. Instead, the firmware > - * is loaded directly into the buffer pointed to by @buf and the @firmware_p > - * data member is pointed at @buf. > - * > - * This function doesn't cache firmware either. > - */ > -int > -request_firmware_into_buf(const struct firmware **firmware_p, const char *name, > - struct device *device, void *buf, size_t size) > -{ > - int ret; > - > - __module_get(THIS_MODULE); > - ret = _request_firmware(firmware_p, name, device, buf, size, > - FW_OPT_UEVENT | FW_OPT_FALLBACK | > - FW_OPT_NOCACHE); > - module_put(THIS_MODULE); > - return ret; > -} > -EXPORT_SYMBOL(request_firmware_into_buf); > - > -/** > * release_firmware: - release the resource associated with a firmware image > * @fw: firmware resource to release > **/ > diff --git a/include/linux/firmware.h b/include/linux/firmware.h > index b1f9f0ccb8ac..5c41c5e75b5c 100644 > --- a/include/linux/firmware.h > +++ b/include/linux/firmware.h > @@ -47,8 +47,6 @@ int request_firmware_nowait( > void (*cont)(const struct firmware *fw, void *context)); > int request_firmware_direct(const struct firmware **fw, const char *name, > struct device *device); > -int request_firmware_into_buf(const struct firmware **firmware_p, > - const char *name, struct device *device, void *buf, size_t size); > > void release_firmware(const struct firmware *fw); > #else > @@ -77,11 +75,5 @@ static inline int request_firmware_direct(const struct firmware **fw, > return -EINVAL; > } > > -static inline int request_firmware_into_buf(const struct firmware **firmware_p, > - const char *name, struct device *device, void *buf, size_t size) > -{ > - return -EINVAL; > -} > - > #endif > #endif