linux-2.5.59/sound/sound_firmware.c attempts to use the
user level system call interface from the kernel, which I understand
works on i386 and perhaps all architectures, but requires a variable
named "errno." (Actually, it mixed things like close() and
sys_close(), but that's beside the point.)
I could just declare a "static int errno;" in the file, but I
thought it might be helpful to use filp_open, vfs_read and filp_close
instead, which shaves a few cpu cycles and avoids the need to allocate
a file descriptor number. I think that this also seems to be the
convention for other kernel facilities that want to open their own
files briefly. I've attached a patch below. The patch shrinks
sound_firmware.c by six lines. I have only verified that the patch
compiles and that the soundcore module now loads.
Comments are welcome. If this patch looks good, then I'd
appreciate it if someone more connected to sound development would
integrate it and forward it to Linus.
--
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
Hi,
"Adam J. Richter" <[email protected]> writes:
> linux-2.5.59/sound/sound_firmware.c attempts to use the
> user level system call interface from the kernel, which I understand
> works on i386 and perhaps all architectures, but requires a variable
> named "errno."
Which is provided in-kernel (not for modules) by 'lib/errno.c'.
> (Actually, it mixed things like close() and sys_close(), but that's
> beside the point.)
Those are provided by <linux/unistd.h>, with __KERNEL_SYSCALLS__
defined.
> I could just declare a "static int errno;" in the file,
That was originally there, but removed in 2.5.57 IIRC.
<linux/unistd.h> has 'extern int errno;' -- so 'static int errno;'
would be a bug.
- Hari
--
Raja R Harinath ------------------------------ [email protected]
On Fri, Jan 17, 2003 at 08:49:36PM -0600, Raja R Harinath wrote:
> Hi,
>
> "Adam J. Richter" <[email protected]> writes:
>
> > linux-2.5.59/sound/sound_firmware.c attempts to use the
> > user level system call interface from the kernel, which I understand
> > works on i386 and perhaps all architectures, but requires a variable
> > named "errno."
>
> Which is provided in-kernel (not for modules) by 'lib/errno.c'.
Not safe. We should either remove errno from kernel syscall wrappers
completely when building __KERNEL__ (just return -1 and nothing more
specific), or even disallow use of unistd.h wrappers from kernel
completely (which is best solution IMHO). Or we must make errno
per-thread variable, which is unnecessary complication...
> > (Actually, it mixed things like close() and sys_close(), but that's
> > beside the point.)
>
> Those are provided by <linux/unistd.h>, with __KERNEL_SYSCALLS__
> defined.
>
> > I could just declare a "static int errno;" in the file,
>
> That was originally there, but removed in 2.5.57 IIRC.
> <linux/unistd.h> has 'extern int errno;' -- so 'static int errno;'
> would be a bug.
I sent patch below few days ago, but I received only confirmations that
fixes compilation - nobody with hardware which uses this firmware
loader confirmed that it works.
Today morning I decided to send it to Linus anyway, as at worst
less broken is better than more broken, but unfortunately Linus was
already at airport :-(
BTW, static int errno is by far best solution if you do not agree with
patch below: due to toolchain behavior soundcore will use its own
errno for syscall wrappers it uses, and it is nearest to the behavior
we wanted...
Best regards,
Petr Vandrovec
[email protected]
diff -ur linux-2.5.58-c952.dist/sound/sound_firmware.c linux-2.5.58-c952/sound/sound_firmware.c
--- linux-2.5.58-c952.dist/sound/sound_firmware.c 2003-01-16 22:33:32.000000000 +0100
+++ linux-2.5.58-c952/sound/sound_firmware.c 2003-01-17 14:54:40.000000000 +0100
@@ -1,47 +1,46 @@
#include <linux/vmalloc.h>
-#define __KERNEL_SYSCALLS__
#include <linux/module.h>
#include <linux/fs.h>
#include <linux/mm.h>
#include <linux/slab.h>
-#include <linux/unistd.h>
#include <asm/uaccess.h>
static int do_mod_firmware_load(const char *fn, char **fp)
{
- int fd;
+ struct file* filp;
long l;
char *dp;
+ loff_t pos;
- fd = open(fn, 0, 0);
- if (fd == -1)
+ filp = filp_open(fn, 0, 0);
+ if (IS_ERR(filp))
{
printk(KERN_INFO "Unable to load '%s'.\n", fn);
return 0;
}
- l = lseek(fd, 0L, 2);
+ l = filp->f_dentry->d_inode->i_size;
if (l <= 0 || l > 131072)
{
printk(KERN_INFO "Invalid firmware '%s'\n", fn);
- sys_close(fd);
+ filp_close(filp, NULL);
return 0;
}
- lseek(fd, 0L, 0);
dp = vmalloc(l);
if (dp == NULL)
{
printk(KERN_INFO "Out of memory loading '%s'.\n", fn);
- sys_close(fd);
+ filp_close(filp, NULL);
return 0;
}
- if (read(fd, dp, l) != l)
+ pos = 0;
+ if (vfs_read(filp, dp, l, &pos) != l)
{
printk(KERN_INFO "Failed to read '%s'.\n", fn);
vfree(dp);
- sys_close(fd);
+ filp_close(filp, NULL);
return 0;
}
- close(fd);
+ filp_close(filp, NULL);
*fp = dp;
return (int) l;
}
Hi,
Petr Vandrovec <[email protected]> writes:
> On Fri, Jan 17, 2003 at 08:49:36PM -0600, Raja R Harinath wrote:
>> Hi,
>>
>> "Adam J. Richter" <[email protected]> writes:
>>
>> > linux-2.5.59/sound/sound_firmware.c attempts to use the
>> > user level system call interface from the kernel, which I understand
>> > works on i386 and perhaps all architectures, but requires a variable
>> > named "errno."
>>
>> Which is provided in-kernel (not for modules) by 'lib/errno.c'.
>
> Not safe. We should either remove errno from kernel syscall wrappers
> completely when building __KERNEL__ (just return -1 and nothing more
> specific), or even disallow use of unistd.h wrappers from kernel
> completely (which is best solution IMHO).
__KERNEL_SYSCALLS__ appears to be defined in several files. Don't
know if they actually use any of them.
> BTW, static int errno is by far best solution if you do not agree with
> patch below: due to toolchain behavior soundcore will use its own
> errno for syscall wrappers it uses, and it is nearest to the behavior
> we wanted...
[snip]
> static int do_mod_firmware_load(const char *fn, char **fp)
> {
> - int fd;
> + struct file* filp;
> long l;
> char *dp;
> + loff_t pos;
>
> - fd = open(fn, 0, 0);
> - if (fd == -1)
> + filp = filp_open(fn, 0, 0);
> + if (IS_ERR(filp))
> {
> printk(KERN_INFO "Unable to load '%s'.\n", fn);
> return 0;
> }
[snip]
I noticed that do_mod_firmware_load is wrapped by a
set_fs(get_gs())/set_fs(fs) pair in mod_firmware_load, presumably
because it performs an 'int 0x80' kernel syscall in there. The
cleanup to use the VFS directly should probably kill the wrapper too.
- Hari
--
Raja R Harinath ------------------------------ [email protected]
On Sat, Jan 18, 2003 at 09:40:36AM -0600, Raja R Harinath wrote:
> > - if (fd == -1)
> > + filp = filp_open(fn, 0, 0);
> > + if (IS_ERR(filp))
> > {
> > printk(KERN_INFO "Unable to load '%s'.\n", fn);
> > return 0;
> > }
> [snip]
>
> I noticed that do_mod_firmware_load is wrapped by a
> set_fs(get_gs())/set_fs(fs) pair in mod_firmware_load, presumably
> because it performs an 'int 0x80' kernel syscall in there. The
> cleanup to use the VFS directly should probably kill the wrapper too.
vfs_read expects userspace pointer, so we must do set_fs() at least
around vfs_read, and so I left it around whole function, to minimize
changes in code.
In original message I asked whether I should convert firmware loader to
the "if (error) goto quit;" style to move error handling to one place,
but only answer I got was 'Ask Rob' ;-)
And as I do not have sound hardware which needs firmware, I do not
want to make more changes than absolutely necessary to the code I cannot
verify. Of course if you'll find someone with hardware...
Best regards,
Petr Vandrovec
[email protected]
Petr Vandrovec <[email protected]> wrote:
>
> And as I do not have sound hardware which needs firmware, I do not
> want to make more changes than absolutely necessary to the code I cannot
> verify. Of course if you'll find someone with hardware...
Your patch was clearly an improvement on what was there, so I shall be
sending it in to Linus. If it breaks then we will hear about it soon enough.
>In original message I asked whether I should convert firmware loader to
>the "if (error) goto quit;" style to move error handling to one place,
>but only answer I got was 'Ask Rob' ;-)
To my knowledge, a goto in this case is not necessary for
avoiding code duplication. If there are a small number of failable
steps that may need to be unwound, you could adopt the style of my patch
(which shortened the code slightly):
if (step1() == ok) {
if (step2() == ok) {
if (strep3() == ok)
return OK;
undo_step2();
}
undo_step1();
}
return failure;
If the nesting gets any deeper than this, then a more
understandable solution for readability than using goto would be to
define a separate inline routine.
In general, I recommend using goto only when it is
topologically necessary to avoid code duplication or due to some
compiler quirk where you want to sqeeze a few more cycles out of code
in a critical path. That way, the use of goto basically flags these
unusual cases for other programmers.
Examples of cases where a goto really is needed to avoid code
duplication include a switch() or a group of if() branches need to
flow back together after starting out by doing different things, and
where you want to break from an outer loop and for some reason don't
want to write the outer loop as an inline routine (for example, you
also have a "return" statement for the existing subroutine in this
loop).
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
"Adam J. Richter" <[email protected]> said:
> To my knowledge, a goto in this case is not necessary for
> avoiding code duplication. If there are a small number of failable
> steps that may need to be unwound, you could adopt the style of my patch
> (which shortened the code slightly):
>
> if (step1() == ok) {
> if (step2() == ok) {
> if (strep3() == ok)
> return OK;
> undo_step2();
> }
> undo_step1();
> }
> return failure;
The "undo_stepX()"'s pollute the CPU's cache, and (even much worse) the
gentle reader's.
> If the nesting gets any deeper than this, then a more
> understandable solution for readability than using goto would be to
> define a separate inline routine.
Can't be done (cleanly) in many cases due to function semantics in C,
polutes CPU cache as above, screws up or gives bad code due to compiler
bugs. Plus has the gentle reader who wants to check error handling chasing
all over the place.
> In general, I recommend using goto only when it is
> topologically necessary to avoid code duplication or due to some
> compiler quirk where you want to sqeeze a few more cycles out of code
> in a critical path. That way, the use of goto basically flags these
> unusual cases for other programmers.
IMVHO, any general criterion that is not strictly based on code
understandability, possibly mitigated by a justified need of maximal speed,
is flawed. This might come close, but won't cut it for me.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
On Mon, 20 Jan 2003, Horst von Brand wrote:
>"Adam J. Richter" <[email protected]> said:
>> To my knowledge, a goto in this case is not necessary for
>> avoiding code duplication. If there are a small number of failable
>> steps that may need to be unwound, you could adopt the style of my patch
>> (which shortened the code slightly):
>>
>> if (step1() == ok) {
>> if (step2() == ok) {
>> if (strep3() == ok)
>> return OK;
>> undo_step2();
>> }
>> undo_step1();
>> }
>> return failure;
>The "undo_stepX()"'s pollute the CPU's cache,
I believe my example should generate exactly the same machine
object code as what Petr was describing, that is, something like this,
which is longer and has more labels to remember or potentially a
mistake in jumping to the wrong label:
if (step1() != ok)
goto abort1;
if (step2() != ok)
goto abort2;
if (step3() == ok)
return OK;
undo_step2();
abort2:
undo_step1();
abort1:
return failure;
In both cases, the compiler is normally going to put all of
error handling code after all of the success code, so the only extra
instructions read into the cache will be from the tail end of the
>and (even much worse) the gentle reader's.
Don't know what you mean here, given that the number of
steps is small.
>> If the nesting gets any deeper than this, then a more
>> understandable solution for readability than using goto would be to
>> define a separate inline routine.
>Can't be done (cleanly) in many cases due to function semantics in C,
I said that sometimes using goto is topologically necessary.
Your use of the term "(cleanly)" is apparently something
subjective about which we disagree. To convert it to an objective
critereon would require cognitive research (for example, having dozens
of programmers try to debug code written one way or the other, under
carefully controlled conditions).
>polutes CPU cache as above,
(Addressed above.)
>screws up or gives bad code due to compiler
>bugs.
Can you elaborate on this? "if()" obviously is a widely used
facility, so I assume you're talking about something more specific,
but I don't know what. I'd be interested if you could point me to the
specific bug or bugs you are refererring to.
>Plus has the gentle reader who wants to check error handling chasing
>all over the place.
But that is even more the case with gotos, which involves
remembering labels and noramlly has no indentation to show the
structure of the branches.
>> In general, I recommend using goto only when it is
>> topologically necessary to avoid code duplication or due to some
>> compiler quirk where you want to sqeeze a few more cycles out of code
>> in a critical path. That way, the use of goto basically flags these
>> unusual cases for other programmers.
>IMVHO, any general criterion that is not strictly based on code
>understandability, possibly mitigated by a justified need of maximal speed,
>is flawed. This might come close, but won't cut it for me.
Firstly, I think my recommendation happens to be "based on
code understandability, possibly mititgated by a justified need for
maximal speed".
Secondly, at the risk of going off on a tangent, I also happen
to think your thesis is not well defined and also wrong in some cases.
Your thesis uses the terms "strictly" and "flawed" without
defining them well. If you were to replace those terms with a more
objective descriptions (e.g., "will likely cause more bugs to be
missed" if that is only what you mean) then it would be clearer
whether I (or any other reader) agrees or disagrees with your claim,
and exactly where.
To the extent that I think we might agree on possible meanings
for terms like "flawed", I think your thesis is technically false
because it fails to consider that there may be other desired benefits.
I can think of numerous general criteria that are not "strictly based
on the need for understandability, possibly mititigated by speed."
For instance, deleting functionality can make code more understandable
and faster, and yet I would not say that every criteria of the form
"functionality X is should be provided for reason Y" is flawed.
Otherwise, perhaps you would want to run the following very
understandable and fast kernel:
/* This is the entire kernel. It is very understandable
and fast! */
void linux_version_3_0(void) {
for(;;)
;
}
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
Sorry, I failed to complete a sentence in my previous reply:
> In both cases, the compiler is normally going to put all of
>error handling code after all of the success code, so the only extra
>instructions read into the cache will be from the tail end of the
...cache line containing the "return success" code.
Adam J. Richter __ ______________ 575 Oroville Road
[email protected] \ / Milpitas, California 95035
+1 408 309-6081 | g g d r a s i l United States of America
"Free Software For The Rest Of Us."
"Adam J. Richter" <[email protected]> said:
> On Mon, 20 Jan 2003, Horst von Brand wrote:
> >"Adam J. Richter" <[email protected]> said:
> >> To my knowledge, a goto in this case is not necessary for
> >> avoiding code duplication. If there are a small number of failable
> >> steps that may need to be unwound, you could adopt the style of my patch
> >> (which shortened the code slightly):
> >>
> >> if (step1() == ok) {
> >> if (step2() == ok) {
> >> if (strep3() == ok)
> >> return OK;
> >> undo_step2();
> >> }
> >> undo_step1();
> >> }
> >> return failure;
> >The "undo_stepX()"'s pollute the CPU's cache,
>
> I believe my example should generate exactly the same machine
> object code as what Petr was describing,
You very much overestimate current C compilers. The code in real examples
is much more complicated than a single undo_foo line, with its own control
structures.
> that is, something like this,
> which is longer and has more labels to remember or potentially a
> mistake in jumping to the wrong label:
>
> if (step1() != ok)
> goto abort1;
>
> if (step2() != ok)
> goto abort2;
>
> if (step3() == ok)
> return OK;
>
> undo_step2();
> abort2:
> undo_step1();
> abort1:
> return failure;
>
> In both cases, the compiler is normally going to put all of
> error handling code after all of the success code,
In a simple case like this, maybe; in complex cases it won't.
> so the only extra
> instructions read into the cache will be from the tail end of the
> >and (even much worse) the gentle reader's.
>
> Don't know what you mean here, given that the number of
> steps is small.
The number of steps in cleanups generally isn't all that small. Besides, a
stack of:
label2:
undo_2;
label1:
undo_1;
label0:
finish_up;
return;
is easier to check that all undoX' get done in the right order.
[...]
> >screws up or gives bad code due to compiler
> >bugs.
>
> Can you elaborate on this? "if()" obviously is a widely used
> facility, so I assume you're talking about something more specific,
> but I don't know what. I'd be interested if you could point me to the
> specific bug or bugs you are refererring to.
There have been cases where gcc generated rather funny code for inline
functions, or just didn't optimize over inline function boundaries.
> >Plus has the gentle reader who wants to check error handling chasing
> >all over the place.
>
> But that is even more the case with gotos, which involves
> remembering labels and noramlly has no indentation to show the
> structure of the branches.
The structure (in the kernel's use of this) is very simple, linear as shown
above. Sure, you can make a real mess, but I'm saying you should be careful
not to.
> >> In general, I recommend using goto only when it is
> >> topologically necessary to avoid code duplication or due to some
> >> compiler quirk where you want to sqeeze a few more cycles out of code
> >> in a critical path. That way, the use of goto basically flags these
> >> unusual cases for other programmers.
>
> >IMVHO, any general criterion that is not strictly based on code
> >understandability, possibly mitigated by a justified need of maximal speed,
> >is flawed. This might come close, but won't cut it for me.
>
> Firstly, I think my recommendation happens to be "based on
> code understandability, possibly mititgated by a justified need for
> maximal speed".
Topology sure has a relation to the cognitive processes involved in code
understanding, but can't be all AFAICS. "Topologically necessary" doesn't
consider size of code involved (perhaps duplicating one simple line is
clearer code in the end), and doesn't distinguish between functions of code
streches, i.e., main code vs auxiliary code (like error handling). Perhaps
a goto is topologically unnecessary, placing the error handling into the
main code stream, or even worse, placing main and auxiliary code in similar
positions, where it is harder to distinguish between them. And code is put
to different uses, sometimes I'll concentrate on the main flow (error
handling is irrelevant), others I'm checking errors are handled in the
right order (main flow is unimportant then), a separation is useful. What
distinguishes stepX() and undoX() in your example is just their names, if
they where a dozen lines of code each with their own control structures,
you'd have no clue which is main code and which is cleanup. In Linus'
style, the error handling code is all together, at the end, clearly
separated from the main flow of control.
> Secondly, at the risk of going off on a tangent, I also happen
> to think your thesis is not well defined and also wrong in some cases.
> Your thesis uses the terms "strictly" and "flawed" without
> defining them well. If you were to replace those terms with a more
> objective descriptions (e.g., "will likely cause more bugs to be
> missed" if that is only what you mean) then it would be clearer
> whether I (or any other reader) agrees or disagrees with your claim,
> and exactly where.
There has not been (and probably can't be) a quantitative comparison of
debuggability of code written with differing styles and restrictions of
goto use, so this is moot.
> To the extent that I think we might agree on possible meanings
> for terms like "flawed", I think your thesis is technically false
> because it fails to consider that there may be other desired benefits.
> I can think of numerous general criteria that are not "strictly based
> on the need for understandability, possibly mititigated by speed."
> For instance, deleting functionality can make code more understandable
> and faster, and yet I would not say that every criteria of the form
> "functionality X is should be provided for reason Y" is flawed.
If you are bound to "reason Y" has to be provided, the question is not IF
but HOW. And the "cut out crap" is an extremely fruitful design strategy.
> Otherwise, perhaps you would want to run the following very
> understandable and fast kernel:
>
> /* This is the entire kernel. It is very understandable
> and fast! */
> void linux_version_3_0(void) {
> for(;;)
> ;
Right. Full POSIX compatible, I presume.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513
On Mon, 20 Jan 2003, Horst von Brand wrote:
[ the war between effeciency and maintainability continues ]
> "Adam J. Richter" <[email protected]> said:
> > To my knowledge, a goto in this case is not necessary for
> > avoiding code duplication. If there are a small number of failable
> > steps that may need to be unwound, you could adopt the style of my patch
> > (which shortened the code slightly):
> >
> > if (step1() == ok) {
> > if (step2() == ok) {
> > if (strep3() == ok)
> > return OK;
> > undo_step2();
> > }
> > undo_step1();
> > }
> > return failure;
>
> The "undo_stepX()"'s pollute the CPU's cache, and (even much worse) the
> gentle reader's.
Given the probably effect of the steps on the cache, I think the
readability argument is a better one, particularly if you have more than a
few steps. There is an effect, but it's relatively small. But use of goto
need not be unreadable.
if (step1() != ok) goto errex0;
if (step2() != ok) goto errex1;
if (step3() == ok) {
if (step4() == ok) return OK;
undo_step3();
}
undo_step2();
errex1:
undo_step1();
errex0:
/* in case there is something other than just return, jump here */
return FAIL;
Less indenting, and the undo's falling through look visually like a switch
without the overhead.
I have not looked at the code this generates, it's a comment on human
readability rather than an actual implementation, and I'm sure someone
will argue that the first failure should just be a return if there's
nothing else which needs to be done. On the other hand the return inline
would be more bytes, so someone else can argue against.
--
bill davidsen <[email protected]>
CTO, TMR Associates, Inc
Doing interesting things with little computers since 1979.
Bill Davidsen <[email protected]> said:
[...]
> I have not looked at the code this generates, it's a comment on human
> readability rather than an actual implementation, and I'm sure someone
> will argue that the first failure should just be a return if there's
> nothing else which needs to be done. On the other hand the return inline
> would be more bytes, so someone else can argue against.
Older gccs used to generate horrible code for several "return foo;" in the
function, dunno how it is today.
Besides, today there might be nothing else to do, but that can change. In
that case you'd better have all exits together.
--
Dr. Horst H. von Brand User #22616 counter.li.org
Departamento de Informatica Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria +56 32 654239
Casilla 110-V, Valparaiso, Chile Fax: +56 32 797513