firmware.agent and trailing newlines

[Duncan Sands <baldrick@free.fr>]

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