firmware.agent and trailing newlines

firmware.agent and trailing newlines

[Duncan Sands]

After upgrading to a newer kernel (2.6.12 + ubuntu patches) I'm
having all kinds of problems with the kernel firmware loader.
The first problem is that request_firmware is not loading
firmware that is present.  This is due to firmware.agent
using echo rather than echo -n when writing to loading, i.e.:

load() {
    echo 1 > $SYSFS/$DEVPATH/loading
    cat "$1" > $SYSFS/$DEVPATH/data
    echo 0 > $SYSFS/$DEVPATH/loading
    exit
}

and

    echo -1 > $SYSFS/$DEVPATH/loading

rather than

load() {
    echo -n 1 > $SYSFS/$DEVPATH/loading
    cat "$1" > $SYSFS/$DEVPATH/data
    echo -n 0 > $SYSFS/$DEVPATH/loading
    exit
}

and

    echo -n -1 > $SYSFS/$DEVPATH/loading

This seems to cause firmware_loading_store to be
called twice for each echo: once with the number,
and once with a string containing just the newline
(or an empty string, I didn't check this point).
This results in messages like this:

firmware_loading_store: unexpected value (0)

and the firmware load being aborted.

(A) Since distributions are shipping kernels
with this behaviour, I think the following patch
should be applied to firmware.agent:

--- firmware.agent.orig	2005-09-01 16:47:24.000000000 +0200
+++ firmware.agent	2005-09-02 13:10:16.000000000 +0200
@@ -19,9 +19,9 @@
 . ./hotplug.functions
 
 load() {
-    echo 1 > $SYSFS/$DEVPATH/loading
+    echo -n 1 > $SYSFS/$DEVPATH/loading
     cat "$1" > $SYSFS/$DEVPATH/data
-    echo 0 > $SYSFS/$DEVPATH/loading
+    echo -n 0 > $SYSFS/$DEVPATH/loading
     exit
 }
 
@@ -59,7 +59,7 @@
     done
 
     # the firmware was not found
-    echo -1 > $SYSFS/$DEVPATH/loading
+    echo -n -1 > $SYSFS/$DEVPATH/loading
 
     ;;
 
(B) Is whitespace stripped off values written to
sysfs attributes?  If so, why?  What if you want
trailing/leading spaces?  What about trailing
newlines - are they stripped?  Should they be?
What is the policy/rationale on sysfs attributes
and whitespace?

(C) firmware_loading_store doesn't do much checking
on the values it gets: since it uses simple_strtol,
it will happily accept eg "1A" and interpret it as
"1"; it interprets empty strings as "0".  Wouldn't
it be better to add some validity checks?

(D) firmware_loading_store aborts firmware loading
on the slightest error.  If someone writes a strange
value to loading, maybe it should just return an
error, rather than returning an error and aborting the
load.

Thoughts?

All the best,

Duncan.


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: firmware.agent and trailing newlines

[Greg KH]

On Fri, Sep 02, 2005 at 01:41:54PM +0200, Duncan Sands wrote:
> (B) Is whitespace stripped off values written to
> sysfs attributes?  If so, why?  What if you want
> trailing/leading spaces?  What about trailing
> newlines - are they stripped?  Should they be?
> What is the policy/rationale on sysfs attributes
> and whitespace?

There is a patch in my tree, and in -mm that does this (incorrectly too,
I have an update from it from the author.)  However I'm going to just
drop it entirely, as we should not be stripping stuff like this from the
kernel.  I am guessing that Ubuntu put this patch in their kernel?

thanks,

greg k-h


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel

Re: firmware.agent and trailing newlines

[Duncan Sands]

Hi Greg,

On Sunday 04 September 2005 18:07, Greg KH wrote:
> On Fri, Sep 02, 2005 at 01:41:54PM +0200, Duncan Sands wrote:
> > (B) Is whitespace stripped off values written to
> > sysfs attributes?  If so, why?  What if you want
> > trailing/leading spaces?  What about trailing
> > newlines - are they stripped?  Should they be?
> > What is the policy/rationale on sysfs attributes
> > and whitespace?
> 
> There is a patch in my tree, and in -mm that does this (incorrectly too,
> I have an update from it from the author.)  However I'm going to just
> drop it entirely, as we should not be stripping stuff like this from the
> kernel.  I am guessing that Ubuntu put this patch in their kernel?

it turned out that the problem was due to having /sbin/hotplug rather
than /sbin/udevsend in /proc/sys/kernel/hotplug, leading to two copies
of firmware.agent being run in parallel.  Changing echo to echo -n must
have changed the timing slightly, causing things to work for a while,
though not for long :-/  I should have debugged the problem more thoroughly
rather than shooting from the hip - sorry about that.

Best wishes,

Duncan.


-------------------------------------------------------
SF.Net email is Sponsored by the Better Software Conference & EXPO
September 19-22, 2005 * San Francisco, CA * Development Lifecycle Practices
Agile & Plan-Driven Development * Managing Projects & Teams * Testing & QA
Security * Process Improvement & Measurement * http://www.sqe.com/bsce5sf
_______________________________________________
Linux-hotplug-devel mailing list  http://linux-hotplug.sourceforge.net
Linux-hotplug-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-hotplug-devel