Re: [LinuxBIOS] r2602 - trunk/LinuxBIOSv2/util/getpir

[Uwe Hermann]

On Wed, Apr 11, 2007 at 08:44:43PM +0200, [e-mail] wrote:
> Jeremy Jackson wrote:
> I'm guessing nobody has tried compiling it with 64bit userspace?
>
> Patch makes it compile cleanly and stops a "SEGV instead of working"
> issue.
>
> I also added a few checks for errors on system calls.
>
> Signed-off-by: Jeremy Jackson
>
> Reworked and
> Acked-by: Stefan Reinauer

I think this should either be

Signed-off-by: Stefan Reinauer 
Acked-by: Stefan Reinauer

(sign-off because you modified the code, acked-by because you acked it
and it needs an ack before it can be committed)

or:

You repost the modified patch for review (if your modifications are
nontrivial), with the original sign-off and your new sign-off and
waiting for an ack:

Signed-off-by: Jeremy Jackson 
Signed-off-by: Stefan Reinauer

(See also my other post, answering Peter's question from a few days ago)
> all: getpir checkpir
> ./checkpir

Don't do this please. If I type 'make' I don't expect the build process
to _run_ the resulting binary for me (won't work anyways as it needs
root access and you usually don't compile as root).
> +irq_tables.c: getpir
> ./getpir

See above.
> -test:   checkpir
> ./checkpir ;\
> exit 0;

An extra target such as this ('test' or 'run' or so) would be fine, though.

Uwe.

[LinuxBIOS] r2602 - trunk/LinuxBIOSv2/util/getpir

[Stefan Reinauer]

* Uwe Hermann  [070411 22:32]:
> > all: getpir checkpir
> > ./checkpir
>
> Don't do this please. If I type 'make' I don't expect the build process
> to _run_ the resulting binary for me (won't work anyways as it needs
> root access and you usually don't compile as root).

In which case the result will be exactly the same as before, except it
tells you to become root to continue.

The intended result of this utility is irq_tables.c, not getpir. It's
just a side effect.

I agree, removing this might make sense though, since looking at the
code reveals that getpir does the same thing as checkpir, itself.
> > +irq_tables.c: getpir
> > ./getpir
>
> See above.

This is a valid rule, it autocreates a file required for the build
process. We do that all over the place in LinuxBIOS (like, we build lar,
and then call it later on)
> > -test:   checkpir
> > ./checkpir ;\
> > exit 0;
>
> An extra target such as this ('test' or 'run' or so) would be fine, though.

Stefan

[LinuxBIOS] [PATCH] make vs. make checkpir

[Uwe Hermann]

On Wed, Apr 11, 2007 at 10:58:51PM +0200, Stefan Reinauer wrote:
> Uwe Hermann  [070411 22:32]:
> > > all: getpir checkpir
> > > ./checkpir
> >
> > Don't do this please. If I type 'make' I don't expect the build process
> > to _run_ the resulting binary for me (won't work anyways as it needs
> > root access and you usually don't compile as root).
>
> In which case the result will be exactly the same as before, except it
> tells you to become root to continue.

Sure, but it's way more intuitive. When you type 'make' you expect it to
build some binary or library, but not _execute_ it.
> I agree, removing this might make sense though, since looking at the
> code reveals that getpir does the same thing as checkpir, itself.

Yeah, I think so too. So we remove checkpir completely?
> > > +irq_tables.c: getpir
> > > ./getpir
> >
> > See above.
>
> This is a valid rule, it autocreates a file required for the build
> process. We do that all over the place in LinuxBIOS (like, we build lar,
> and then call it later on)

Ah, ok, makes sense. Sort of. I would have expected something like

./checkpir --validate irq_tables.c

but that would be overkill in this case, I guess. It's easier to
integrate the irq_tables.c directly in the code.

Please see attached patch. It makes 'make' just build getpir, and
'make checkpir' create and validate the irq_tables.c file.

Uwe.

[LinuxBIOS] [PATCH] make vs. make checkpir

[Peter Stuge]

On Wed, Apr 11, 2007 at 11:33:42PM +0200, Uwe Hermann wrote:
> Please see attached patch. It makes 'make' just build getpir, and
> 'make checkpir' create and validate the irq_tables.c file.

This is just wrong.

Makefile targets are filenames.

The default target may not neccessarily have to be irq_tables.c
though.

//Peter

[LinuxBIOS] [PATCH] make vs. make checkpir

[Corey Osgood]

Can someone run some tests on getpir and checkpir? Last time I ran it,
about a week ago on the jetway j7f2we1g, getpir came up with a different
checksum than checkpir did. I haven't even looked to see where the error
might have come from. I'll test it again with this patch when I get some
time.

Also, am I correct in the understanding that if Linux boots with
acpi=off and acpi disabled in the BIOS, that getpir should produce a
usable irq_tables.c?

-Corey

[LinuxBIOS] [PATCH] make vs. make checkpir

[Stefan Reinauer]

* Corey Osgood  [070412 05:19]:
> Can someone run some tests on getpir and checkpir? Last time I ran it,
> about a week ago on the jetway j7f2we1g, getpir came up with a different
> checksum than checkpir did.

This means the pirq table is broken.
> Also, am I correct in the understanding that if Linux boots with
> acpi=off and acpi disabled in the BIOS, that getpir should produce a
> usable irq_tables.c?

Not necessarily. There's also the mptable for interrupts and the bios
might just set up interrupts in the hardware without producing a correct
table at all.

Usually mptable handles your interrupts and pirq table handles your
peer busses, iirc. It's really broken stuff. In the BIOS and in Linux.