Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1673846 > unrolled thread

[PATCH] firmware: remove request_firmware_into_buf()

Back to article view | Back to linux.kernel

Contents

  [PATCH] firmware: remove request_firmware_into_buf() Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-23 22:30 +0200
    Re: [PATCH] firmware: remove request_firmware_into_buf() Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-26 22:30 +0200
      Re: [PATCH] firmware: remove request_firmware_into_buf() Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2017-06-27 09:00 +0200
        Re: [PATCH] firmware: remove request_firmware_into_buf() Bjorn Andersson <bjorn.andersson@linaro.org> - 2017-06-27 19:40 +0200
      Re: [PATCH] firmware: remove request_firmware_into_buf() Alan Cox <alan@linux.intel.com> - 2017-06-27 15:00 +0200

#1673846 — [PATCH] firmware: remove request_firmware_into_buf()

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.

Reported-by: "Luis R. Rodriguez" <mcgrof@kernel.org>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 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

[toc] | [next] | [standalone]

#1675087

On Fri, Jun 23, 2017 at 9:03 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> 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" <mcgrof@kernel.org>
> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  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

[toc] | [prev] | [next] | [standalone]

#1675339

On Mon, Jun 26, 2017 at 01:22:41PM -0700, Bjorn Andersson wrote:
> On Fri, Jun 23, 2017 at 9:03 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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().

Why would we keep it if there is no in-tree user for it?  If you want it
sometime in the future, great, we can revert the deletion then, but
keeping it around for nothing isn't ok, you know that :)

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1676032

On Mon 26 Jun 23:52 PDT 2017, Greg Kroah-Hartman wrote:

> On Mon, Jun 26, 2017 at 01:22:41PM -0700, Bjorn Andersson wrote:
> > On Fri, Jun 23, 2017 at 9:03 AM, Greg Kroah-Hartman
> > <gregkh@linuxfoundation.org> 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().
> 
> Why would we keep it if there is no in-tree user for it?  If you want it
> sometime in the future, great, we can revert the deletion then, but
> keeping it around for nothing isn't ok, you know that :)
> 

Of course I know that :)

I did put a patch in the tubes for this yesterday [1], it's late for
v4.13, but I would be happy to see the API stay and we would have a user
in v4.14 (and tick this off Qualcomm's "required" list).

[1] https://lkml.org/lkml/2017/6/26/693

Regards,
Bjorn

[toc] | [prev] | [next] | [standalone]

#1675655

On Mon, 2017-06-26 at 13:22 -0700, Bjorn Andersson wrote:
> On Fri, Jun 23, 2017 at 9:03 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> 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.

It may also at some point be very useful for FPGA partial reprogramming
- the FPGA bitstreams are *not* small and most FPGAs you don't need all
of it in kernel memory at the same moment.

Alan

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web