Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 10:08:19PM +0100, Pali Rohár wrote:
> On Friday 10 November 2017 21:26:01 Luis R. Rodriguez wrote:
> > On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> > > This function works pretty much like request_firmware(), but it prefer
> > > usermode helper. If usermode helper fails then it fallback to direct
> > > access. Useful for dynamic or model specific firmware data.
> > > 
> > > Signed-off-by: Pali Rohár 
> > > ---
> > >  drivers/base/firmware_class.c |   45 
> > > +++--
> > >  include/linux/firmware.h  |9 +
> > >  2 files changed, 52 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 4b57cf5..c3a9fe5 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, 
> > > enum fw_status status)
> > >  #endif
> > >  #define FW_OPT_NO_WARN   (1U << 3)
> > >  #define FW_OPT_NOCACHE   (1U << 4)
> > > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> > > +#define FW_OPT_PREFER_USER   (1U << 5)
> > > +#else
> > > +#define FW_OPT_PREFER_USER   0
> > > +#endif
> > 
> > I've been cleaning these up these flags [0], which I'll shortly respin based
> > on feedback, so this sort of stuff should be avoided at all costs.
> > 
> > Regardless of this even if you *leave* the flag in place and a driver 
> > required
> > this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> > calling fw_load_from_user_helper would just already return -ENOENT, as such 
> > it
> > would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> > needed.
> > 
> > [0] https://lkml.kernel.org/r/20170914225422.31034-1-mcg...@kernel.org
> > 
> > >  struct firmware_cache {
> > >   /* firmware_buf instance will be added into the below list */
> > > @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware 
> > > *fw)
> > >   if (ret <= 0) /* error or already assigned */
> > >   goto out;
> > >  
> > > - ret = fw_get_filesystem_firmware(device, fw->priv);
> > > + if (opt_flags & FW_OPT_PREFER_USER) {
> > > + ret = fw_load_from_user_helper(fw, name, device, opt_flags, 
> > > timeout);
> > > + if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> > > + dev_warn(device,
> > > +  "User helper firmware load for %s failed with 
> > > error %d\n",
> > > +  name, ret);
> > > + dev_warn(device, "Falling back to direct firmware 
> > > load\n");
> > 
> > As I had noted before, the usermode helper was really not well designed,
> > as such extending further use of it is something we should shy away unless 
> > we
> > determine its completely necessary.
> > 
> > So what's wrong with this driver failing at direct access, which should be 
> > fast,
> > and relying on a uevent to then work using the current fallback mechanisms?
> > 
> > The commit log in no way documents any of the justifications for further
> > extending use of the usermode helper.
> 
> Hi! See patch 6/6. It is needed to avoid direct access and wl1251 on
> Nokia N900 needs to use userspace helper which prepares firmware data.

My point is your commit log in no way describes the shortcomings of the
current affairs for device drivers which only can access the data it
needs using the firmware fallback mechanism.

In order for a change to go in, specially if its extending use of the
fallback mechanism through sysfs now as primary citizen, the justification
should be well documented on the commit log.

For instance you may want to highlight that what I documented here:

https://www.kernel.org/doc/html/latest/driver-api/firmware/fallback-mechanisms.html

"CONFIG_FW_LOADER_USER_HELPER: enables building the firmware fallback
mechanism. Most distributions enable this option today. If enabled but
CONFIG_FW_LOADER_USER_HELPER_FALLBACK is disabled, only the custom fallback
mechanism is available and for the request_firmware_nowait() call."

Since most distros disable CONFIG_FW_LOADER_USER_HELPER_FALLBACK, in practice
this means the fallback mechanism is never actually triggered, and the
only way to do that in practice this for most distros is to use the
custom fallback mechanism, and the only difference there is that the
custom fallback mechanism has an infinite timeout since we have no clear
way to know what it is or when it will complete, other than when it
actually does its work.

That begs the question, why cant you just use request_firmware_nowait()
with the custom fallback mechanism?

Your commit log should explain the shortcomings of the current API.

Also note that "usermode helper" refers to kernel/umh.c, and the only
code being used from that UMH API is the usermodehelper_read_lock_wait()
on async, or usermodehelper_read_trylock() on sync. Nothing else. The
rest of the 

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Pali Rohár]

On Friday 10 November 2017 21:26:01 Luis R. Rodriguez wrote:
> On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> > This function works pretty much like request_firmware(), but it prefer
> > usermode helper. If usermode helper fails then it fallback to direct
> > access. Useful for dynamic or model specific firmware data.
> > 
> > Signed-off-by: Pali Rohár 
> > ---
> >  drivers/base/firmware_class.c |   45 
> > +++--
> >  include/linux/firmware.h  |9 +
> >  2 files changed, 52 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 4b57cf5..c3a9fe5 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, 
> > enum fw_status status)
> >  #endif
> >  #define FW_OPT_NO_WARN (1U << 3)
> >  #define FW_OPT_NOCACHE (1U << 4)
> > +#ifdef CONFIG_FW_LOADER_USER_HELPER
> > +#define FW_OPT_PREFER_USER (1U << 5)
> > +#else
> > +#define FW_OPT_PREFER_USER 0
> > +#endif
> 
> I've been cleaning these up these flags [0], which I'll shortly respin based
> on feedback, so this sort of stuff should be avoided at all costs.
> 
> Regardless of this even if you *leave* the flag in place and a driver required
> this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> calling fw_load_from_user_helper would just already return -ENOENT, as such it
> would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> needed.
> 
> [0] https://lkml.kernel.org/r/20170914225422.31034-1-mcg...@kernel.org
> 
> >  struct firmware_cache {
> > /* firmware_buf instance will be added into the below list */
> > @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
> > if (ret <= 0) /* error or already assigned */
> > goto out;
> >  
> > -   ret = fw_get_filesystem_firmware(device, fw->priv);
> > +   if (opt_flags & FW_OPT_PREFER_USER) {
> > +   ret = fw_load_from_user_helper(fw, name, device, opt_flags, 
> > timeout);
> > +   if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> > +   dev_warn(device,
> > +"User helper firmware load for %s failed with 
> > error %d\n",
> > +name, ret);
> > +   dev_warn(device, "Falling back to direct firmware 
> > load\n");
> 
> As I had noted before, the usermode helper was really not well designed,
> as such extending further use of it is something we should shy away unless we
> determine its completely necessary.
> 
> So what's wrong with this driver failing at direct access, which should be 
> fast,
> and relying on a uevent to then work using the current fallback mechanisms?
> 
> The commit log in no way documents any of the justifications for further
> extending use of the usermode helper.

Hi! See patch 6/6. It is needed to avoid direct access and wl1251 on
Nokia N900 needs to use userspace helper which prepares firmware data.

Direct access is just fallback when userspace helper is not available.
Without userspace helper on devices where wl1251 do not have own eeprom
memory, wl1251 cannot work.

I know that usermode helper is not well designed, but it is the best
option what we can do for wl1251.

-- 
Pali Rohár
pali.ro...@gmail.com

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 12:26 PM, Luis R. Rodriguez  wrote:
> even if you *leave* the flag in place and a driver required
> this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
> calling fw_load_from_user_helper would just already return -ENOENT, as such it
> would in turn fallback to direct fs loading so the #ifef'ery seems to be not
> needed.

> The commit log in no way documents any of the justifications for further
> extending use of the usermode helper.

Also, any new functionality introduced should have a respective test
case in tools/testing/selftests/firmware/ and lib/test_firmware.c, as
well as extending existing documentation on
Documentation/driver-api/firmware/.

  Luis

Re: [PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Luis R. Rodriguez]

On Fri, Nov 10, 2017 at 12:38:27AM +0100, Pali Rohár wrote:
> This function works pretty much like request_firmware(), but it prefer
> usermode helper. If usermode helper fails then it fallback to direct
> access. Useful for dynamic or model specific firmware data.
> 
> Signed-off-by: Pali Rohár 
> ---
>  drivers/base/firmware_class.c |   45 
> +++--
>  include/linux/firmware.h  |9 +
>  2 files changed, 52 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 4b57cf5..c3a9fe5 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum 
> fw_status status)
>  #endif
>  #define FW_OPT_NO_WARN   (1U << 3)
>  #define FW_OPT_NOCACHE   (1U << 4)
> +#ifdef CONFIG_FW_LOADER_USER_HELPER
> +#define FW_OPT_PREFER_USER   (1U << 5)
> +#else
> +#define FW_OPT_PREFER_USER   0
> +#endif

I've been cleaning these up these flags [0], which I'll shortly respin based
on feedback, so this sort of stuff should be avoided at all costs.

Regardless of this even if you *leave* the flag in place and a driver required
this, but the kernel was compiled without CONFIG_FW_LOADER_USER_HELPER then
calling fw_load_from_user_helper would just already return -ENOENT, as such it
would in turn fallback to direct fs loading so the #ifef'ery seems to be not
needed.

[0] https://lkml.kernel.org/r/20170914225422.31034-1-mcg...@kernel.org

>  struct firmware_cache {
>   /* firmware_buf instance will be added into the below list */
> @@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
>   if (ret <= 0) /* error or already assigned */
>   goto out;
>  
> - ret = fw_get_filesystem_firmware(device, fw->priv);
> + if (opt_flags & FW_OPT_PREFER_USER) {
> + ret = fw_load_from_user_helper(fw, name, device, opt_flags, 
> timeout);
> + if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
> + dev_warn(device,
> +  "User helper firmware load for %s failed with 
> error %d\n",
> +  name, ret);
> + dev_warn(device, "Falling back to direct firmware 
> load\n");

As I had noted before, the usermode helper was really not well designed,
as such extending further use of it is something we should shy away unless we
determine its completely necessary.

So what's wrong with this driver failing at direct access, which should be fast,
and relying on a uevent to then work using the current fallback mechanisms?

The commit log in no way documents any of the justifications for further
extending use of the usermode helper.

  Luis

[PATCH v2 5/6] firmware: Add request_firmware_prefer_user() function

[Pali Rohár]

This function works pretty much like request_firmware(), but it prefer
usermode helper. If usermode helper fails then it fallback to direct
access. Useful for dynamic or model specific firmware data.

Signed-off-by: Pali Rohár 
---
 drivers/base/firmware_class.c |   45 +++--
 include/linux/firmware.h  |9 +
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 4b57cf5..c3a9fe5 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -195,6 +195,11 @@ static int __fw_state_check(struct fw_state *fw_st, enum 
fw_status status)
 #endif
 #define FW_OPT_NO_WARN (1U << 3)
 #define FW_OPT_NOCACHE (1U << 4)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_PREFER_USER (1U << 5)
+#else
+#define FW_OPT_PREFER_USER 0
+#endif
 
 struct firmware_cache {
/* firmware_buf instance will be added into the below list */
@@ -1214,13 +1219,26 @@ static void fw_abort_batch_reqs(struct firmware *fw)
if (ret <= 0) /* error or already assigned */
goto out;
 
-   ret = fw_get_filesystem_firmware(device, fw->priv);
+   if (opt_flags & FW_OPT_PREFER_USER) {
+   ret = fw_load_from_user_helper(fw, name, device, opt_flags, 
timeout);
+   if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
+   dev_warn(device,
+"User helper firmware load for %s failed with 
error %d\n",
+name, ret);
+   dev_warn(device, "Falling back to direct firmware 
load\n");
+   }
+   } else {
+   ret = -EINVAL;
+   }
+
+   if (ret)
+   ret = fw_get_filesystem_firmware(device, fw->priv);
if (ret) {
if (!(opt_flags & FW_OPT_NO_WARN))
dev_warn(device,
 "Direct firmware load for %s failed with error 
%d\n",
 name, ret);
-   if (opt_flags & FW_OPT_USERHELPER) {
+   if ((opt_flags & FW_OPT_USERHELPER) && !(opt_flags & 
FW_OPT_PREFER_USER)) {
dev_warn(device, "Falling back to user helper\n");
ret = fw_load_from_user_helper(fw, name, device,
   opt_flags);
@@ -1329,6 +1347,29 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
+ * request_firmware_prefer_user: - prefer usermode helper for loading firmware
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but it prefer
+ * usermode helper. If usermode helper fails then it fallback to direct access.
+ * Useful for dynamic or model specific firmware data.
+ **/
+int request_firmware_prefer_user(const struct firmware **firmware_p,
+   const char *name, struct device *device)
+{
+   int ret;
+
+   __module_get(THIS_MODULE);
+   ret = _request_firmware(firmware_p, name, device, NULL, 0,
+   FW_OPT_UEVENT | FW_OPT_PREFER_USER);
+   module_put(THIS_MODULE);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
+
+/**
  * 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 d450808..8584528 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -48,6 +48,8 @@ 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_prefer_user(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);
 
@@ -78,6 +80,13 @@ static inline int request_firmware_direct(const struct 
firmware **fw,
return -EINVAL;
 }
 
+static inline int request_firmware_prefer_user(const struct firmware **fw,
+  const char *name,
+  struct device *device)
+{
+   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)
 {
-- 
1.7.9.5