[PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Neil Horman]

The direct firmware loading interface is a bit quiet about failures.  Failures
that occur during loading are masked if firmware exists in multiple locations,
and may be masked entirely in the event that we fall back to the user mode
helper code.  It would be nice to see some of the more unexpected errors get
logged, so in the event that you expect the direct firmware loader to work (like
if CONFIG_FW_LOADER_USER_HELPER is enabled), and something goes wrong, you can
figure out what happened.

Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
CC: Ming Lei <ming.lei@canonical.com>
CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/base/firmware_class.c | 38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 10a4467..eb8fb94 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -282,31 +282,35 @@ static noinline_for_stack long fw_file_size(struct file *file)
 	return st.size;
 }
 
-static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
+static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
 {
 	long size;
 	char *buf;
+	int rc;
 
 	size = fw_file_size(file);
 	if (size <= 0)
-		return false;
+		return -EINVAL;
 	buf = vmalloc(size);
 	if (!buf)
-		return false;
-	if (kernel_read(file, 0, buf, size) != size) {
+		return -ENOMEM;
+	rc = kernel_read(file, 0, buf, size);
+	if (rc != size) {
+		if (rc > 0)
+			rc = -EIO;
 		vfree(buf);
-		return false;
+		return rc;
 	}
 	fw_buf->data = buf;
 	fw_buf->size = size;
-	return true;
+	return 0;
 }
 
-static bool fw_get_filesystem_firmware(struct device *device,
+static int fw_get_filesystem_firmware(struct device *device,
 				       struct firmware_buf *buf)
 {
 	int i;
-	bool success = false;
+	int rc = -ENOENT;
 	char *path = __getname();
 
 	for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
@@ -321,14 +325,17 @@ static bool fw_get_filesystem_firmware(struct device *device,
 		file = filp_open(path, O_RDONLY, 0);
 		if (IS_ERR(file))
 			continue;
-		success = fw_read_file_contents(file, buf);
+		rc = fw_read_file_contents(file, buf);
 		fput(file);
-		if (success)
+		if (rc)
+			dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
+				path, rc);
+		else
 			break;
 	}
 	__putname(path);
 
-	if (success) {
+	if (!rc) {
 		dev_dbg(device, "firmware: direct-loading firmware %s\n",
 			buf->fw_id);
 		mutex_lock(&fw_lock);
@@ -337,7 +344,7 @@ static bool fw_get_filesystem_firmware(struct device *device,
 		mutex_unlock(&fw_lock);
 	}
 
-	return success;
+	return rc;
 }
 
 /* firmware holds the ownership of pages */
@@ -1086,9 +1093,14 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
 		}
 	}
 
-	if (!fw_get_filesystem_firmware(device, fw->priv))
+	ret = fw_get_filesystem_firmware(device, fw->priv);
+	if (ret) {
+		dev_warn(device, "Direct firmware load failed with error %d\n",
+			 ret);
+		dev_warn(device, "Falling back to user helper\n");
 		ret = fw_load_from_user_helper(fw, name, device,
 					       uevent, nowait, timeout);
+	}
 
 	/* don't cache firmware handled without uevent */
 	if (!ret)
-- 
1.8.1.4

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Ming Lei]

On Sat, Sep 7, 2013 at 3:36 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> The direct firmware loading interface is a bit quiet about failures.  Failures

Because there are several pre-defined search paths, and generally the
requested firmware only exists in one of these paths.

> that occur during loading are masked if firmware exists in multiple locations,
> and may be masked entirely in the event that we fall back to the user mode

You still can figure out the request falls back to user mode loading since we
have the "firmware: direct-loading firmware %s" log.

> helper code.  It would be nice to see some of the more unexpected errors get

What are the unexpected errors?

> logged, so in the event that you expect the direct firmware loader to work (like
> if CONFIG_FW_LOADER_USER_HELPER is enabled), and something goes wrong, you can
> figure out what happened.

Looks we didn't meet this case, do you have real examples?

>
> Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> CC: Ming Lei <ming.lei@canonical.com>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> ---
>  drivers/base/firmware_class.c | 38 +++++++++++++++++++++++++-------------
>  1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 10a4467..eb8fb94 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -282,31 +282,35 @@ static noinline_for_stack long fw_file_size(struct file *file)
>         return st.size;
>  }
>
> -static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
>  {
>         long size;
>         char *buf;
> +       int rc;
>
>         size = fw_file_size(file);
>         if (size <= 0)
> -               return false;
> +               return -EINVAL;
>         buf = vmalloc(size);
>         if (!buf)
> -               return false;
> -       if (kernel_read(file, 0, buf, size) != size) {
> +               return -ENOMEM;
> +       rc = kernel_read(file, 0, buf, size);
> +       if (rc != size) {
> +               if (rc > 0)
> +                       rc = -EIO;
>                 vfree(buf);
> -               return false;
> +               return rc;
>         }
>         fw_buf->data = buf;
>         fw_buf->size = size;
> -       return true;
> +       return 0;
>  }
>
> -static bool fw_get_filesystem_firmware(struct device *device,
> +static int fw_get_filesystem_firmware(struct device *device,
>                                        struct firmware_buf *buf)
>  {
>         int i;
> -       bool success = false;
> +       int rc = -ENOENT;
>         char *path = __getname();
>
>         for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> @@ -321,14 +325,17 @@ static bool fw_get_filesystem_firmware(struct device *device,
>                 file = filp_open(path, O_RDONLY, 0);
>                 if (IS_ERR(file))
>                         continue;
> -               success = fw_read_file_contents(file, buf);
> +               rc = fw_read_file_contents(file, buf);
>                 fput(file);
> -               if (success)
> +               if (rc)
> +                       dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> +                               path, rc);

The above may introduce noises.

> +               else
>                         break;
>         }
>         __putname(path);
>
> -       if (success) {
> +       if (!rc) {
>                 dev_dbg(device, "firmware: direct-loading firmware %s\n",
>                         buf->fw_id);
>                 mutex_lock(&fw_lock);
> @@ -337,7 +344,7 @@ static bool fw_get_filesystem_firmware(struct device *device,
>                 mutex_unlock(&fw_lock);
>         }
>
> -       return success;
> +       return rc;
>  }
>
>  /* firmware holds the ownership of pages */
> @@ -1086,9 +1093,14 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>                 }
>         }
>
> -       if (!fw_get_filesystem_firmware(device, fw->priv))
> +       ret = fw_get_filesystem_firmware(device, fw->priv);
> +       if (ret) {
> +               dev_warn(device, "Direct firmware load failed with error %d\n",
> +                        ret);
> +               dev_warn(device, "Falling back to user helper\n");

You should merge the two warnings to into one?

>                 ret = fw_load_from_user_helper(fw, name, device,
>                                                uevent, nowait, timeout);
> +       }
>
>         /* don't cache firmware handled without uevent */
>         if (!ret)


Thanks,
--
Ming Lei

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Neil Horman]

On Wed, Sep 11, 2013 at 07:54:28PM +0800, Ming Lei wrote:
> On Sat, Sep 7, 2013 at 3:36 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > The direct firmware loading interface is a bit quiet about failures.  Failures
> 
> Because there are several pre-defined search paths, and generally the
> requested firmware only exists in one of these paths.
> 
This is true, but you'll note this patch doesn't make any noise in the event
that a firmware isn't found until all the search paths are exhausted.  I didn't
consider this "unexpected".

> > that occur during loading are masked if firmware exists in multiple locations,
> > and may be masked entirely in the event that we fall back to the user mode
> 
> You still can figure out the request falls back to user mode loading since we
> have the "firmware: direct-loading firmware %s" log.
> 
Yes, but you're looking at it backwards, that only prints out if the direct load
works.  If it doesn't, you get silence, which is bad.

> > helper code.  It would be nice to see some of the more unexpected errors get
> 
> What are the unexpected errors?
> 
If you get a short read in the direct load path for example, or if someone
mounts an nfs share over the firmware search path and you get an ESTALE.
Alternatively, if the vmalloc fails during the direct load path, these would be
"unexpected" errors

> > logged, so in the event that you expect the direct firmware loader to work (like
> > if CONFIG_FW_LOADER_USER_HELPER is enabled), and something goes wrong, you can
> > figure out what happened.
> 
> Looks we didn't meet this case, do you have real examples?
> 
Yeah, we had a vmalloc failure in the direct load path, and unknowingly had
forgot to configure CONFIG_FW_LOADER_USER_HELPER, so the module load failed with
an ENOENT, even though the firmware was clearly present on the filesystem.  This
patch helped us track that down.

Neil

> >
> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
> > CC: Ming Lei <ming.lei@canonical.com>
> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > ---
> >  drivers/base/firmware_class.c | 38 +++++++++++++++++++++++++-------------
> >  1 file changed, 25 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > index 10a4467..eb8fb94 100644
> > --- a/drivers/base/firmware_class.c
> > +++ b/drivers/base/firmware_class.c
> > @@ -282,31 +282,35 @@ static noinline_for_stack long fw_file_size(struct file *file)
> >         return st.size;
> >  }
> >
> > -static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> > +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
> >  {
> >         long size;
> >         char *buf;
> > +       int rc;
> >
> >         size = fw_file_size(file);
> >         if (size <= 0)
> > -               return false;
> > +               return -EINVAL;
> >         buf = vmalloc(size);
> >         if (!buf)
> > -               return false;
> > -       if (kernel_read(file, 0, buf, size) != size) {
> > +               return -ENOMEM;
> > +       rc = kernel_read(file, 0, buf, size);
> > +       if (rc != size) {
> > +               if (rc > 0)
> > +                       rc = -EIO;
> >                 vfree(buf);
> > -               return false;
> > +               return rc;
> >         }
> >         fw_buf->data = buf;
> >         fw_buf->size = size;
> > -       return true;
> > +       return 0;
> >  }
> >
> > -static bool fw_get_filesystem_firmware(struct device *device,
> > +static int fw_get_filesystem_firmware(struct device *device,
> >                                        struct firmware_buf *buf)
> >  {
> >         int i;
> > -       bool success = false;
> > +       int rc = -ENOENT;
> >         char *path = __getname();
> >
> >         for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
> > @@ -321,14 +325,17 @@ static bool fw_get_filesystem_firmware(struct device *device,
> >                 file = filp_open(path, O_RDONLY, 0);
> >                 if (IS_ERR(file))
> >                         continue;
> > -               success = fw_read_file_contents(file, buf);
> > +               rc = fw_read_file_contents(file, buf);
> >                 fput(file);
> > -               if (success)
> > +               if (rc)
> > +                       dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
> > +                               path, rc);
> 
> The above may introduce noises.
> 
> > +               else
> >                         break;
> >         }
> >         __putname(path);
> >
> > -       if (success) {
> > +       if (!rc) {
> >                 dev_dbg(device, "firmware: direct-loading firmware %s\n",
> >                         buf->fw_id);
> >                 mutex_lock(&fw_lock);
> > @@ -337,7 +344,7 @@ static bool fw_get_filesystem_firmware(struct device *device,
> >                 mutex_unlock(&fw_lock);
> >         }
> >
> > -       return success;
> > +       return rc;
> >  }
> >
> >  /* firmware holds the ownership of pages */
> > @@ -1086,9 +1093,14 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
> >                 }
> >         }
> >
> > -       if (!fw_get_filesystem_firmware(device, fw->priv))
> > +       ret = fw_get_filesystem_firmware(device, fw->priv);
> > +       if (ret) {
> > +               dev_warn(device, "Direct firmware load failed with error %d\n",
> > +                        ret);
> > +               dev_warn(device, "Falling back to user helper\n");
> 
> You should merge the two warnings to into one?
> 
> >                 ret = fw_load_from_user_helper(fw, name, device,
> >                                                uevent, nowait, timeout);
> > +       }
> >
> >         /* don't cache firmware handled without uevent */
> >         if (!ret)
> 
> 
> Thanks,
> --
> Ming Lei
> 

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Ming Lei]

On Wed, Sep 11, 2013 at 10:19 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> On Wed, Sep 11, 2013 at 07:54:28PM +0800, Ming Lei wrote:
>> On Sat, Sep 7, 2013 at 3:36 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
>> > The direct firmware loading interface is a bit quiet about failures.  Failures
>>
>> Because there are several pre-defined search paths, and generally the
>> requested firmware only exists in one of these paths.
>>
> This is true, but you'll note this patch doesn't make any noise in the event
> that a firmware isn't found until all the search paths are exhausted.  I didn't
> consider this "unexpected".

Yes, it will cause noise, suppose the firmware is in the last search path, and
we may always get the warning during the first three searches, and it
is certainly
annoying, isn't it?

>
>> > that occur during loading are masked if firmware exists in multiple locations,
>> > and may be masked entirely in the event that we fall back to the user mode
>>
>> You still can figure out the request falls back to user mode loading since we
>> have the "firmware: direct-loading firmware %s" log.
>>
> Yes, but you're looking at it backwards, that only prints out if the direct load
> works.  If it doesn't, you get silence, which is bad.

OK, you can change to only log the failure.

>
>> > helper code.  It would be nice to see some of the more unexpected errors get
>>
>> What are the unexpected errors?
>>
> If you get a short read in the direct load path for example, or if someone
> mounts an nfs share over the firmware search path and you get an ESTALE.

That is easy to find, and no one should mount one fs on firmware path.

> Alternatively, if the vmalloc fails during the direct load path, these would be
> "unexpected" errors

This one might make sense since size of some firmwares may be several mega
bytes, and vmalloc space is a bit limited on 32bit arch, so how about just log
this failure in fw_read_file_contents()?

>
>> > logged, so in the event that you expect the direct firmware loader to work (like
>> > if CONFIG_FW_LOADER_USER_HELPER is enabled), and something goes wrong, you can
>> > figure out what happened.
>>
>> Looks we didn't meet this case, do you have real examples?
>>
> Yeah, we had a vmalloc failure in the direct load path, and unknowingly had
> forgot to configure CONFIG_FW_LOADER_USER_HELPER, so the module load failed with
> an ENOENT, even though the firmware was clearly present on the filesystem.  This
> patch helped us track that down.

Fair enough, looks it is helpful to add some log, :-)

>
> Neil
>
>> >
>> > Signed-off-by: Neil Horman <nhorman@tuxdriver.com>
>> > CC: Ming Lei <ming.lei@canonical.com>
>> > CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
>> > ---
>> >  drivers/base/firmware_class.c | 38 +++++++++++++++++++++++++-------------
>> >  1 file changed, 25 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
>> > index 10a4467..eb8fb94 100644
>> > --- a/drivers/base/firmware_class.c
>> > +++ b/drivers/base/firmware_class.c
>> > @@ -282,31 +282,35 @@ static noinline_for_stack long fw_file_size(struct file *file)
>> >         return st.size;
>> >  }
>> >
>> > -static bool fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
>> > +static int fw_read_file_contents(struct file *file, struct firmware_buf *fw_buf)
>> >  {
>> >         long size;
>> >         char *buf;
>> > +       int rc;
>> >
>> >         size = fw_file_size(file);
>> >         if (size <= 0)
>> > -               return false;
>> > +               return -EINVAL;
>> >         buf = vmalloc(size);
>> >         if (!buf)
>> > -               return false;
>> > -       if (kernel_read(file, 0, buf, size) != size) {
>> > +               return -ENOMEM;
>> > +       rc = kernel_read(file, 0, buf, size);
>> > +       if (rc != size) {
>> > +               if (rc > 0)
>> > +                       rc = -EIO;
>> >                 vfree(buf);
>> > -               return false;
>> > +               return rc;
>> >         }
>> >         fw_buf->data = buf;
>> >         fw_buf->size = size;
>> > -       return true;
>> > +       return 0;
>> >  }
>> >
>> > -static bool fw_get_filesystem_firmware(struct device *device,
>> > +static int fw_get_filesystem_firmware(struct device *device,
>> >                                        struct firmware_buf *buf)
>> >  {
>> >         int i;
>> > -       bool success = false;
>> > +       int rc = -ENOENT;
>> >         char *path = __getname();
>> >
>> >         for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
>> > @@ -321,14 +325,17 @@ static bool fw_get_filesystem_firmware(struct device *device,
>> >                 file = filp_open(path, O_RDONLY, 0);
>> >                 if (IS_ERR(file))
>> >                         continue;
>> > -               success = fw_read_file_contents(file, buf);
>> > +               rc = fw_read_file_contents(file, buf);
>> >                 fput(file);
>> > -               if (success)
>> > +               if (rc)
>> > +                       dev_warn(device, "firmware, attempted to load %s, but failed with error %d\n",
>> > +                               path, rc);
>>
>> The above may introduce noises.
>>
>> > +               else
>> >                         break;
>> >         }
>> >         __putname(path);
>> >
>> > -       if (success) {
>> > +       if (!rc) {
>> >                 dev_dbg(device, "firmware: direct-loading firmware %s\n",
>> >                         buf->fw_id);
>> >                 mutex_lock(&fw_lock);
>> > @@ -337,7 +344,7 @@ static bool fw_get_filesystem_firmware(struct device *device,
>> >                 mutex_unlock(&fw_lock);
>> >         }
>> >
>> > -       return success;
>> > +       return rc;
>> >  }
>> >
>> >  /* firmware holds the ownership of pages */
>> > @@ -1086,9 +1093,14 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>> >                 }
>> >         }
>> >
>> > -       if (!fw_get_filesystem_firmware(device, fw->priv))
>> > +       ret = fw_get_filesystem_firmware(device, fw->priv);
>> > +       if (ret) {
>> > +               dev_warn(device, "Direct firmware load failed with error %d\n",
>> > +                        ret);
>> > +               dev_warn(device, "Falling back to user helper\n");
>>
>> You should merge the two warnings to into one?
>>
>> >                 ret = fw_load_from_user_helper(fw, name, device,
>> >                                                uevent, nowait, timeout);
>> > +       }
>> >
>> >         /* don't cache firmware handled without uevent */
>> >         if (!ret)
>>
>>
>> Thanks,
>> --
>> Ming Lei
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Neil Horman]

On Thu, Sep 12, 2013 at 10:25:58AM +0800, Ming Lei wrote:
> On Wed, Sep 11, 2013 at 10:19 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Wed, Sep 11, 2013 at 07:54:28PM +0800, Ming Lei wrote:
> >> On Sat, Sep 7, 2013 at 3:36 AM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >> > The direct firmware loading interface is a bit quiet about failures.  Failures
> >>
> >> Because there are several pre-defined search paths, and generally the
> >> requested firmware only exists in one of these paths.
> >>
> > This is true, but you'll note this patch doesn't make any noise in the event
> > that a firmware isn't found until all the search paths are exhausted.  I didn't
> > consider this "unexpected".
> 
> Yes, it will cause noise, suppose the firmware is in the last search path, and
> we may always get the warning during the first three searches, and it
> is certainly
> annoying, isn't it?
> 
Please re-read the patch, then point out to me which printk the above action
will trigger, because its not happening in my testing. If you'll take a look at
fw_get_filesystem_firmware, you'll see that if the filp_open on a firmware file
fails, we continue the for loop through the list of available search paths.  No
error is generated in the case you describe above.

The exceptions to that rule are:

1) If no file is found in _any_ of the search paths, in which case
fw_get_filesystem_firmware will return -ENOENT, causing _request_firmware to
print "Direct firmware load failed with error -ENOENT.  Falling back to user
helper".

2) If a file is successfully opened by fw_get_filesystem_firmware, but something
goes wrong during the read in fw_read_file_contents, we print "firmware
attempted to load <file>, but failed with error <X>", followed by the printk
from (1) indicating that we are falling back to the user mode helper path.

Both of these execptions should be rare, and are something the administrator
will want to know about, so as not to confuse the real error with the mystery
-ENOENT you would get if you fell back to the user mode helepr and it wansn't
configured on in the running kernel.

> >
> >> > that occur during loading are masked if firmware exists in multiple locations,
> >> > and may be masked entirely in the event that we fall back to the user mode
> >>
> >> You still can figure out the request falls back to user mode loading since we
> >> have the "firmware: direct-loading firmware %s" log.
> >>
> > Yes, but you're looking at it backwards, that only prints out if the direct load
> > works.  If it doesn't, you get silence, which is bad.
> 
> OK, you can change to only log the failure.
> 
That is exactly what this patch does.

> >
> >> > helper code.  It would be nice to see some of the more unexpected errors get
> >>
> >> What are the unexpected errors?
> >>
> > If you get a short read in the direct load path for example, or if someone
> > mounts an nfs share over the firmware search path and you get an ESTALE.
> 
> That is easy to find, and no one should mount one fs on firmware path.
> 
Thats really rather the problem isn't it? this patch is an assertion that its
not that easy to find the root cause of a firmware load problem currently, even
if you haven't done something dumb, like mount a network FS on your firmware
path.  And I agree you shouldn't do such things, but that doesnt' mean that
people won't or have good reason to try (I can certainly see mounting the
firmware path on an NFS mount for embedded system development with limited
storage).  Regardless, when people do do something silly, we should tell them
so, not mask the problem behind a mystery -ENOENT error.

> > Alternatively, if the vmalloc fails during the direct load path, these would be
> > "unexpected" errors
> 
> This one might make sense since size of some firmwares may be several mega
> bytes, and vmalloc space is a bit limited on 32bit arch, so how about just log
> this failure in fw_read_file_contents()?
> 
It does exactly that, and a 
little more.  Why just catch the vmalloc error?  What if kernel_read fails for
some reason?  I see no reason to catch the vmalloc error specifcally, when we
can catch any error generated by direct read path (that doesn't include simple
file not found errors for any single entry in the search path).

> >
> >> > logged, so in the event that you expect the direct firmware loader to work (like
> >> > if CONFIG_FW_LOADER_USER_HELPER is enabled), and something goes wrong, you can
> >> > figure out what happened.
> >>
> >> Looks we didn't meet this case, do you have real examples?
> >>
> > Yeah, we had a vmalloc failure in the direct load path, and unknowingly had
> > forgot to configure CONFIG_FW_LOADER_USER_HELPER, so the module load failed with
> > an ENOENT, even though the firmware was clearly present on the filesystem.  This
> > patch helped us track that down.
> 
> Fair enough, looks it is helpful to add some log, :-)
> 
Thanks :)
Neil

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Ming Lei]

On Thu, Sep 12, 2013 at 9:16 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
>>
> Please re-read the patch, then point out to me which printk the above action
> will trigger, because its not happening in my testing. If you'll take a look at
> fw_get_filesystem_firmware, you'll see that if the filp_open on a firmware file
> fails, we continue the for loop through the list of available search paths.  No
> error is generated in the case you describe above.

You are right, sorry for missing "if (IS_ERR(file))  continue;",  and looks the
patch is good.

Acked-by: Ming Lei <ming.lei@canonical.com>

Thanks,
--
Ming Lei

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Neil Horman]

On Thu, Sep 12, 2013 at 10:30:59PM +0800, Ming Lei wrote:
> On Thu, Sep 12, 2013 at 9:16 PM, Neil Horman <nhorman@tuxdriver.com> wrote:
> >>
> > Please re-read the patch, then point out to me which printk the above action
> > will trigger, because its not happening in my testing. If you'll take a look at
> > fw_get_filesystem_firmware, you'll see that if the filp_open on a firmware file
> > fails, we continue the for loop through the list of available search paths.  No
> > error is generated in the case you describe above.
> 
> You are right, sorry for missing "if (IS_ERR(file))  continue;",  and looks the
> patch is good.
> 
> Acked-by: Ming Lei <ming.lei@canonical.com>
> 
> Thanks,
Awesome, thanks!
Neil

> --
> Ming Lei
> 

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Henrique de Moraes Holschuh]

On Thu, 12 Sep 2013, Neil Horman wrote:
> Both of these execptions should be rare, and are something the administrator
> will want to know about, so as not to confuse the real error with the mystery
> -ENOENT you would get if you fell back to the user mode helepr and it wansn't
> configured on in the running kernel.

Except, of course, for Intel processor microcode updates, which are going to
cause ENOENT on a large number of systems.

This will generate a large number of questions by users on the distro MLs.

However, IMHO this is *not* a reason to refuse this patch series.  If
anything, at least for Debian I will use it as an opportunity to educate
people about the existence of microcode update packages in "non-free".

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Neil Horman]

On Thu, Sep 12, 2013 at 03:46:30PM -0300, Henrique de Moraes Holschuh wrote:
> On Thu, 12 Sep 2013, Neil Horman wrote:
> > Both of these execptions should be rare, and are something the administrator
> > will want to know about, so as not to confuse the real error with the mystery
> > -ENOENT you would get if you fell back to the user mode helepr and it wansn't
> > configured on in the running kernel.
> 
> Except, of course, for Intel processor microcode updates, which are going to
> cause ENOENT on a large number of systems.
> 
> This will generate a large number of questions by users on the distro MLs.
> 
> However, IMHO this is *not* a reason to refuse this patch series.  If
> anything, at least for Debian I will use it as an opportunity to educate
> people about the existence of microcode update packages in "non-free".
> 
I agree. If people are running with downlevel microcode, they shold know about
it.  You can't expect request_firmware to fail silently.  If people complain, I
think the right solution would be to add a test to the microcode_request_fw
function to check for the existence of the file before requesting it.

Neil

> -- 
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh
> 

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Henrique de Moraes Holschuh]

On Thu, 12 Sep 2013, Neil Horman wrote:
> On Thu, Sep 12, 2013 at 03:46:30PM -0300, Henrique de Moraes Holschuh wrote:
> > On Thu, 12 Sep 2013, Neil Horman wrote:
> > > Both of these execptions should be rare, and are something the administrator
> > > will want to know about, so as not to confuse the real error with the mystery
> > > -ENOENT you would get if you fell back to the user mode helepr and it wansn't
> > > configured on in the running kernel.
> > 
> > Except, of course, for Intel processor microcode updates, which are going to
> > cause ENOENT on a large number of systems.
> > 
> > This will generate a large number of questions by users on the distro MLs.
> > 
> > However, IMHO this is *not* a reason to refuse this patch series.  If
> > anything, at least for Debian I will use it as an opportunity to educate
> > people about the existence of microcode update packages in "non-free".
> > 
> I agree. If people are running with downlevel microcode, they shold know about
> it.  You can't expect request_firmware to fail silently.  If people complain, I
> think the right solution would be to add a test to the microcode_request_fw
> function to check for the existence of the file before requesting it.

Make it a firmware loader API for "optional" firmware (i.e. which doesn't
complain on ENOENT), leaving for the caller the detail of whether a
firmware-not-there failure should be logged or not.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH] firmware: Be a bit more verbose about direct firmware loading failure

[Neil Horman]

On Fri, Sep 13, 2013 at 10:56:39AM -0300, Henrique de Moraes Holschuh wrote:
> On Thu, 12 Sep 2013, Neil Horman wrote:
> > On Thu, Sep 12, 2013 at 03:46:30PM -0300, Henrique de Moraes Holschuh wrote:
> > > On Thu, 12 Sep 2013, Neil Horman wrote:
> > > > Both of these execptions should be rare, and are something the administrator
> > > > will want to know about, so as not to confuse the real error with the mystery
> > > > -ENOENT you would get if you fell back to the user mode helepr and it wansn't
> > > > configured on in the running kernel.
> > > 
> > > Except, of course, for Intel processor microcode updates, which are going to
> > > cause ENOENT on a large number of systems.
> > > 
> > > This will generate a large number of questions by users on the distro MLs.
> > > 
> > > However, IMHO this is *not* a reason to refuse this patch series.  If
> > > anything, at least for Debian I will use it as an opportunity to educate
> > > people about the existence of microcode update packages in "non-free".
> > > 
> > I agree. If people are running with downlevel microcode, they shold know about
> > it.  You can't expect request_firmware to fail silently.  If people complain, I
> > think the right solution would be to add a test to the microcode_request_fw
> > function to check for the existence of the file before requesting it.
> 
> Make it a firmware loader API for "optional" firmware (i.e. which doesn't
> complain on ENOENT), leaving for the caller the detail of whether a
> firmware-not-there failure should be logged or not.
> 
That makes sense, I'll subit that in a separate patch this afternoon
Thanks!
Neil

> -- 
>   "One disk to rule them all, One disk to find them. One disk to bring
>   them all and in the darkness grind them. In the Land of Redmond
>   where the shadows lie." -- The Silicon Valley Tarot
>   Henrique Holschuh
>