subject:"\[LinuxBIOS\] r2890 \- in trunk\/LinuxBIOSv2\/src\: arch\/i386\/boot boot cpu\/x86\/mtrr cpu\/x86\/tsc devices"

Re: [LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

2007-10-24 Thread Uwe Hermann

On Wed, Oct 24, 2007 at 05:51:17PM +0200, Stefan Reinauer wrote:
> Uwe Hermann wrote:
> >> Modified: trunk/LinuxBIOSv2/src/boot/elfboot.c
> >> ===
> >> --- trunk/LinuxBIOSv2/src/boot/elfboot.c   2007-10-23 19:23:52 UTC (rev 
> >> 2889)
> >> +++ trunk/LinuxBIOSv2/src/boot/elfboot.c   2007-10-23 22:17:45 UTC (rev 
> >> 2890)
> >> @@ -144,7 +144,7 @@
> >>  {
> >>struct verify_callback *cb_chain;
> >>unsigned char *note, *end;
> >> -  char *program, *version;
> >> +  unsigned char *program, *version;
> >> 
> >
> > This doesn't look like the correct fix. Strings should always be 'char *'
> > or 'const char *' IMO, so the code where program/version is used should
> > be fixed instead.
> >   
> 
> Oh, but the code expects an unsigned char, so it should get one.

Depends, in this case the code may also be "wrong" in that it uses
'unsigned char' where it should rather use 'uint8_t' (didn't check though).


> This is processing ELF header data, 8bit, so, according to the rule you
> set up below, this fix should be correct.

uint8_t would be correct here, if it's 8bit data.


> The fact that in this case the code is parsing a string out of that data
> is secondary I think.

Yes.


> >>if (resource->flags & IORESOURCE_STORED) {
> >> -  unsigned char buf[10];
> >> +  char buf[10];
> >> 
> >
> > If buf contains a string, yes. If it should contain 8bit data, it should
> > be uint8_t or u8.
> >   
> Ok, then I assume you are saying everything is ok.

I think so (but I didn't really check the code).


> 
> >>  #if OPT_HT_LINK == 1
> >> @@ -123,15 +126,17 @@
> >>  
> >>  static int ht_setup_link(struct ht_link *prev, device_t dev, unsigned pos)
> >>  {
> >> +#if OPT_HT_LINK == 1
> >>static const uint8_t link_width_to_pow2[]= { 3, 4, 0, 5, 1, 2, 0, 0 };
> >>static const uint8_t pow2_to_link_width[] = { 0x7, 4, 5, 0, 1, 3 };
> >> -  struct ht_link cur[1];
> >>unsigned present_width_cap,upstream_width_cap;
> >>unsigned present_freq_cap, upstream_freq_cap;
> >>unsigned ln_present_width_in,  ln_upstream_width_in; 
> >>unsigned ln_present_width_out, ln_upstream_width_out;
> >>unsigned freq, old_freq;
> >>unsigned present_width, upstream_width, old_width;
> >> +#endif
> >> +  struct ht_link cur[1];
> >> 
> >
> > This change is not trivial, as it might very well have effects on the
> > run-time behaviour of LinuxBIOS / the hardware.
> >
> > Can we say for sure that there's no reason to (re-)initialize here?
> > Is this tested on hardware?
> >   
> Oh yes, this is highly trivial. Look at the code, all these are unused
> variables. The produced code does not change.
> The OPT_HT_LINK define is tested already below, where the real magic
> happens. But when this was introduced, all the variables were left unused.

Ah, yes, I see. There are further '#if OPT_HT_LINK == 1' down below
which actually do stuff with the variables (but only in the "== 1" case).


Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org


signature.asc
Description: Digital signature
-- 
linuxbios mailing list
linuxbios@linuxbios.org
http://www.linuxbios.org/mailman/listinfo/linuxbios

Re: [LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

2007-10-24 Thread Stefan Reinauer

Uwe Hermann wrote:
>> Modified: trunk/LinuxBIOSv2/src/boot/elfboot.c
>> ===
>> --- trunk/LinuxBIOSv2/src/boot/elfboot.c 2007-10-23 19:23:52 UTC (rev 
>> 2889)
>> +++ trunk/LinuxBIOSv2/src/boot/elfboot.c 2007-10-23 22:17:45 UTC (rev 
>> 2890)
>> @@ -144,7 +144,7 @@
>>  {
>>  struct verify_callback *cb_chain;
>>  unsigned char *note, *end;
>> -char *program, *version;
>> +unsigned char *program, *version;
>> 
>
> This doesn't look like the correct fix. Strings should always be 'char *'
> or 'const char *' IMO, so the code where program/version is used should
> be fixed instead.
>   

Oh, but the code expects an unsigned char, so it should get one. It's
not that I cast anything around wildly.
This is processing ELF header data, 8bit, so, according to the rule you
set up below, this fix should be correct.
The fact that in this case the code is parsing a string out of that data
is secondary I think. Better fixes or ideas are no doubt highly welcome.

>> Modified: trunk/LinuxBIOSv2/src/devices/device_util.c
>> ===
>> --- trunk/LinuxBIOSv2/src/devices/device_util.c  2007-10-23 19:23:52 UTC 
>> (rev 2889)
>> +++ trunk/LinuxBIOSv2/src/devices/device_util.c  2007-10-23 22:17:45 UTC 
>> (rev 2890)
>> @@ -454,7 +454,7 @@
>>  void report_resource_stored(device_t dev, struct resource *resource, const 
>> char *comment)
>>  {
>>  if (resource->flags & IORESOURCE_STORED) {
>> -unsigned char buf[10];
>> +char buf[10];
>> 
>
> If buf contains a string, yes. If it should contain 8bit data, it should
> be uint8_t or u8.
>   
Ok, then I assume you are saying everything is ok.

>>  #if OPT_HT_LINK == 1
>> @@ -123,15 +126,17 @@
>>  
>>  static int ht_setup_link(struct ht_link *prev, device_t dev, unsigned pos)
>>  {
>> +#if OPT_HT_LINK == 1
>>  static const uint8_t link_width_to_pow2[]= { 3, 4, 0, 5, 1, 2, 0, 0 };
>>  static const uint8_t pow2_to_link_width[] = { 0x7, 4, 5, 0, 1, 3 };
>> -struct ht_link cur[1];
>>  unsigned present_width_cap,upstream_width_cap;
>>  unsigned present_freq_cap, upstream_freq_cap;
>>  unsigned ln_present_width_in,  ln_upstream_width_in; 
>>  unsigned ln_present_width_out, ln_upstream_width_out;
>>  unsigned freq, old_freq;
>>  unsigned present_width, upstream_width, old_width;
>> +#endif
>> +struct ht_link cur[1];
>> 
>
> This change is not trivial, as it might very well have effects on the
> run-time behaviour of LinuxBIOS / the hardware.
>
> Can we say for sure that there's no reason to (re-)initialize here?
> Is this tested on hardware?
>   
Oh yes, this is highly trivial. Look at the code, all these are unused
variables. The produced code does not change.
The OPT_HT_LINK define is tested already below, where the real magic
happens. But when this was introduced, all the variables were left unused.

Stefan

-- 
coresystems GmbH • Brahmsstr. 16 • D-79104 Freiburg i. Br.
  Tel.: +49 761 7668825 • Fax: +49 761 7664613
Email: [EMAIL PROTECTED]  • http://www.coresystems.de/
Registergericht: Amtsgericht Freiburg • HRB 7656
Geschäftsführer: Stefan Reinauer • Ust-IdNr.: DE245674866


-- 
linuxbios mailing list
linuxbios@linuxbios.org
http://www.linuxbios.org/mailman/listinfo/linuxbios

Re: [LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

2007-10-24 Thread Uwe Hermann

On Wed, Oct 24, 2007 at 12:17:45AM +0200, [EMAIL PROTECTED] wrote:
> Modified: trunk/LinuxBIOSv2/src/arch/i386/boot/linuxbios_table.c
> ===
> --- trunk/LinuxBIOSv2/src/arch/i386/boot/linuxbios_table.c2007-10-23 
> 19:23:52 UTC (rev 2889)
> +++ trunk/LinuxBIOSv2/src/arch/i386/boot/linuxbios_table.c2007-10-23 
> 22:17:45 UTC (rev 2890)
> @@ -120,7 +120,7 @@
>  {
>   static const struct {
>   uint32_t tag;
> - const uint8_t *string;
> + const char *string;

Yep.


> Modified: trunk/LinuxBIOSv2/src/boot/elfboot.c
> ===
> --- trunk/LinuxBIOSv2/src/boot/elfboot.c  2007-10-23 19:23:52 UTC (rev 
> 2889)
> +++ trunk/LinuxBIOSv2/src/boot/elfboot.c  2007-10-23 22:17:45 UTC (rev 
> 2890)
> @@ -144,7 +144,7 @@
>  {
>   struct verify_callback *cb_chain;
>   unsigned char *note, *end;
> - char *program, *version;
> + unsigned char *program, *version;

This doesn't look like the correct fix. Strings should always be 'char *'
or 'const char *' IMO, so the code where program/version is used should
be fixed instead.


> Modified: trunk/LinuxBIOSv2/src/devices/device_util.c
> ===
> --- trunk/LinuxBIOSv2/src/devices/device_util.c   2007-10-23 19:23:52 UTC 
> (rev 2889)
> +++ trunk/LinuxBIOSv2/src/devices/device_util.c   2007-10-23 22:17:45 UTC 
> (rev 2890)
> @@ -454,7 +454,7 @@
>  void report_resource_stored(device_t dev, struct resource *resource, const 
> char *comment)
>  {
>   if (resource->flags & IORESOURCE_STORED) {
> - unsigned char buf[10];
> + char buf[10];

If buf contains a string, yes. If it should contain 8bit data, it should
be uint8_t or u8.


>   unsigned long long base, end;
>   base = resource->base;
>   end = resource_end(resource);
> 
> Modified: trunk/LinuxBIOSv2/src/devices/hypertransport.c
> ===
> --- trunk/LinuxBIOSv2/src/devices/hypertransport.c2007-10-23 19:23:52 UTC 
> (rev 2889)
> +++ trunk/LinuxBIOSv2/src/devices/hypertransport.c2007-10-23 22:17:45 UTC 
> (rev 2890)
> @@ -37,6 +37,9 @@
>  #include 
>  #include 
>  
> +/* The hypertransport link is already optimized in pre-ram code
> + * so don't do it again
> + */
>  #define OPT_HT_LINK 0
>  
>  #if OPT_HT_LINK == 1
> @@ -123,15 +126,17 @@
>  
>  static int ht_setup_link(struct ht_link *prev, device_t dev, unsigned pos)
>  {
> +#if OPT_HT_LINK == 1
>   static const uint8_t link_width_to_pow2[]= { 3, 4, 0, 5, 1, 2, 0, 0 };
>   static const uint8_t pow2_to_link_width[] = { 0x7, 4, 5, 0, 1, 3 };
> - struct ht_link cur[1];
>   unsigned present_width_cap,upstream_width_cap;
>   unsigned present_freq_cap, upstream_freq_cap;
>   unsigned ln_present_width_in,  ln_upstream_width_in; 
>   unsigned ln_present_width_out, ln_upstream_width_out;
>   unsigned freq, old_freq;
>   unsigned present_width, upstream_width, old_width;
> +#endif
> + struct ht_link cur[1];

This change is not trivial, as it might very well have effects on the
run-time behaviour of LinuxBIOS / the hardware.

Can we say for sure that there's no reason to (re-)initialize here?
Is this tested on hardware?



Uwe.
-- 
http://www.hermann-uwe.de  | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org


signature.asc
Description: Digital signature
-- 
linuxbios mailing list
linuxbios@linuxbios.org
http://www.linuxbios.org/mailman/listinfo/linuxbios

[LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

2007-10-23 Thread svn

Author: stepan
Date: 2007-10-24 00:17:45 +0200 (Wed, 24 Oct 2007)
New Revision: 2890

Modified:
   trunk/LinuxBIOSv2/src/arch/i386/boot/linuxbios_table.c
   trunk/LinuxBIOSv2/src/arch/i386/boot/tables.c
   trunk/LinuxBIOSv2/src/boot/elfboot.c
   trunk/LinuxBIOSv2/src/cpu/x86/mtrr/mtrr.c
   trunk/LinuxBIOSv2/src/cpu/x86/tsc/delay_tsc.c
   trunk/LinuxBIOSv2/src/devices/device_util.c
   trunk/LinuxBIOSv2/src/devices/hypertransport.c
Log:
fix a whole bunch of warnings. (trivial)

Signed-off-by: Stefan Reinauer <[EMAIL PROTECTED]>
Acked-by: Stefan Reinauer <[EMAIL PROTECTED]>



Modified: trunk/LinuxBIOSv2/src/arch/i386/boot/linuxbios_table.c
===
--- trunk/LinuxBIOSv2/src/arch/i386/boot/linuxbios_table.c  2007-10-23 
19:23:52 UTC (rev 2889)
+++ trunk/LinuxBIOSv2/src/arch/i386/boot/linuxbios_table.c  2007-10-23 
22:17:45 UTC (rev 2890)
@@ -120,7 +120,7 @@
 {
static const struct {
uint32_t tag;
-   const uint8_t *string;
+   const char *string;
} strings[] = {
{ LB_TAG_VERSION,linuxbios_version,},
{ LB_TAG_EXTRA_VERSION,  linuxbios_extra_version,  },

Modified: trunk/LinuxBIOSv2/src/arch/i386/boot/tables.c
===
--- trunk/LinuxBIOSv2/src/arch/i386/boot/tables.c   2007-10-23 19:23:52 UTC 
(rev 2889)
+++ trunk/LinuxBIOSv2/src/arch/i386/boot/tables.c   2007-10-23 22:17:45 UTC 
(rev 2890)
@@ -8,6 +8,7 @@
 #include 
 #include 
 #include 
+#include 
 #include "linuxbios_table.h"
 
 // Global Descriptor Table, defined in c_start.S

Modified: trunk/LinuxBIOSv2/src/boot/elfboot.c
===
--- trunk/LinuxBIOSv2/src/boot/elfboot.c2007-10-23 19:23:52 UTC (rev 
2889)
+++ trunk/LinuxBIOSv2/src/boot/elfboot.c2007-10-23 22:17:45 UTC (rev 
2890)
@@ -144,7 +144,7 @@
 {
struct verify_callback *cb_chain;
unsigned char *note, *end;
-   char *program, *version;
+   unsigned char *program, *version;
 
cb_chain = 0;
note = header + offset;

Modified: trunk/LinuxBIOSv2/src/cpu/x86/mtrr/mtrr.c
===
--- trunk/LinuxBIOSv2/src/cpu/x86/mtrr/mtrr.c   2007-10-23 19:23:52 UTC (rev 
2889)
+++ trunk/LinuxBIOSv2/src/cpu/x86/mtrr/mtrr.c   2007-10-23 22:17:45 UTC (rev 
2890)
@@ -349,7 +349,6 @@
  * mtrrs.  If this doesn't work out we can get smart again 
  * and clear out the mtrrs.
  */
-struct var_mtrr_state var_state;
 
 printk_debug("\n");
 /* Initialized the fixed_mtrrs to uncached */

Modified: trunk/LinuxBIOSv2/src/cpu/x86/tsc/delay_tsc.c
===
--- trunk/LinuxBIOSv2/src/cpu/x86/tsc/delay_tsc.c   2007-10-23 19:23:52 UTC 
(rev 2889)
+++ trunk/LinuxBIOSv2/src/cpu/x86/tsc/delay_tsc.c   2007-10-23 22:17:45 UTC 
(rev 2890)
@@ -102,8 +102,7 @@
 static unsigned long long calibrate_tsc(void)
 {
unsigned long long start, end, delta;
-   unsigned long allones = (unsigned long) -1, result;
-   unsigned long count;
+   unsigned long result, count;

start = rdtscll();
// no udivdi3, dammit.

Modified: trunk/LinuxBIOSv2/src/devices/device_util.c
===
--- trunk/LinuxBIOSv2/src/devices/device_util.c 2007-10-23 19:23:52 UTC (rev 
2889)
+++ trunk/LinuxBIOSv2/src/devices/device_util.c 2007-10-23 22:17:45 UTC (rev 
2890)
@@ -454,7 +454,7 @@
 void report_resource_stored(device_t dev, struct resource *resource, const 
char *comment)
 {
if (resource->flags & IORESOURCE_STORED) {
-   unsigned char buf[10];
+   char buf[10];
unsigned long long base, end;
base = resource->base;
end = resource_end(resource);

Modified: trunk/LinuxBIOSv2/src/devices/hypertransport.c
===
--- trunk/LinuxBIOSv2/src/devices/hypertransport.c  2007-10-23 19:23:52 UTC 
(rev 2889)
+++ trunk/LinuxBIOSv2/src/devices/hypertransport.c  2007-10-23 22:17:45 UTC 
(rev 2890)
@@ -37,6 +37,9 @@
 #include 
 #include 
 
+/* The hypertransport link is already optimized in pre-ram code
+ * so don't do it again
+ */
 #define OPT_HT_LINK 0
 
 #if OPT_HT_LINK == 1
@@ -123,15 +126,17 @@
 
 static int ht_setup_link(struct ht_link *prev, device_t dev, unsigned pos)
 {
+#if OPT_HT_LINK == 1
static const uint8_t link_width_to_pow2[]= { 3, 4, 0, 5, 1, 2, 0, 0 };
static const uint8_t pow2_to_link_width[] = { 0x7, 4, 5, 0, 1, 3 };
-   struct ht_link cur[1];
unsigned present_width_cap,upstream_width_cap;
unsigned present_freq_cap, upstream_freq_cap;
unsigned ln_present_width_in

Re: [LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

Re: [LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

Re: [LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

[LinuxBIOS] r2890 - in trunk/LinuxBIOSv2/src: arch/i386/boot boot cpu/x86/mtrr cpu/x86/tsc devices

4 matches

Site Navigation

Mail list logo

Footer information