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" <[email protected]>
Signed-off-by: Greg Kroah-Hartman <[email protected]>
---
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
On Fri, Jun 23, 2017 at 9:03 AM, Greg Kroah-Hartman
<[email protected]> 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" <[email protected]>
> Signed-off-by: Greg Kroah-Hartman <[email protected]>
> ---
> 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
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
> <[email protected]> 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
On Mon, 2017-06-26 at 13:22 -0700, Bjorn Andersson wrote:
> On Fri, Jun 23, 2017 at 9:03 AM, Greg Kroah-Hartman
> <[email protected]> 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
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
> > <[email protected]> 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
On Tue, Jun 27, 2017 at 10:37:11AM -0700, Bjorn Andersson wrote:
> On Mon 26 Jun 23:52 PDT 2017, Greg Kroah-Hartman wrote:
> > 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
Greg,
What have you decided to do?
Also what is the threshold for number of drivers to use a new feature for us to
add it? Note that there is a bundle of features queued up now and as per your
own preference it would seem you want a new API call for each new feature...
Can you clarify that is what you wish for?
Are you *certain* you want to take this approach?
Note that this patch alone was not sufficient to revert all all the stuff for
request_firmware_into_buf(), there was another patch which added the option to
make caching optional, but it was only used internally. Folks already have a
use case for that though *and* existing upstream drivers already have a use
case for that -- the iwlwifi driver is such a case, as they do their own
caching for its driver.
There are similar features in the pipeline which are minor variations to requests
such as optional requests -- do you *really* expect a new API call then to be
created for minor variations of each major call for say optional requests or
requests for without caching?
I had to think about all these things, so now I ask you to also consider this as well.
I ask how many drivers are needed as users for a feature as I think its important
to be fair for the other features in the pipeline which I did start reviewing
and *do* consider sensible to add support for. This was an example feature which
went in with 0 users at all for a while... and now it seems we only have *one*
user still...
Luis
On Thu, Jul 13, 2017 at 01:44:34AM +0200, Luis R. Rodriguez wrote:
> On Tue, Jun 27, 2017 at 10:37:11AM -0700, Bjorn Andersson wrote:
> > On Mon 26 Jun 23:52 PDT 2017, Greg Kroah-Hartman wrote:
> > > 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
>
> Greg,
>
> What have you decided to do?
Given that this didn't go in for 4.13-rc1, I figured it was kind of
obvious, I'm waiting to see if a user shows up for this release :)
And it's always trivial to revert a patch to add an api back if someone
does need/want it, so this shouldn't be a big deal to anyone.
> Also what is the threshold for number of drivers to use a new feature for us to
> add it? Note that there is a bundle of features queued up now and as per your
> own preference it would seem you want a new API call for each new feature...
Usually it's a good engineering practice to have 3 users of any
interface to prove that you got the interface correct, right? But I'm
not making any hard and fast rule here, let's see the patches and judge
it from there...
thanks,
greg k-h