Patchwork [commit] r5286 - ...

login
register
about
Submitter Stefan Reinauer
Date 2010-03-25 21:58:41
Message ID <4BABDC91.5020502@coresystems.de>
Download mbox | patch
Permalink /patch/1158/
State Not Applicable
Headers show

Comments

Stefan Reinauer - 2010-03-25 21:58:41
On 3/25/10 10:47 PM, Segher Boessenkool wrote:
>>>>> -extern unsigned char AmlCode[];
>>>>> +extern const acpi_header_t AmlCode;
>>>>>           
>>>> And we're positive, this always does the right thing with gcc?
>>>>         
>>> I am told that AmlCode is defined as array of (unsigned) char in
>>> some other file.  Declaring it as some other type here is not
>>> valid C, and *will* break with GCC, with some options (-combine
>>> or LTO at least) -- it will not compile.
>>>       
>> The biggest worry for me is incorrect execution.  If it doesn't compile
>> when
>> it breaks, then that's a good thing.
>>     
> It will probably _work_ with current GCC and no whole-program stuff,
> but how do you *know* it does?  Better to just fix it.
>
>   
I changed one as an example now...
I guess we could optimize to not copy the header twice, but the header
is really small, so i didn't care to make the code uglier.
Myles Watson - 2010-03-25 22:07:29
> I changed one as an example now...
> I guess we could optimize to not copy the header twice, but the header
> is really small, so i didn't care to make the code uglier.
I agree.

> @@ -273,8 +273,10 @@
>      acpi_create_facs(facs);
> 
>      dsdt = (acpi_header_t *) current;
> -    current += AmlCode.length;
> -    memcpy((void *) dsdt, &AmlCode, AmlCode.length);
> +    memcpy((void *) dsdt, AmlCode, sizeof(acpi_header_t));
> +    int len = dsdt->length;
> +    current += len;
> +    memcpy((void *) dsdt, AmlCode, len);

Why not:
> +    current += dstd->length;
> +    memcpy((void *) dsdt, AmlCode, dsdt->length);

I don't think the extra variable adds anything.

I'll ack that.

Thanks,
Myles
Stefan Reinauer - 2010-03-25 22:14:16
On 3/25/10 11:07 PM, Myles Watson wrote:
>   
>> I changed one as an example now...
>> I guess we could optimize to not copy the header twice, but the header
>> is really small, so i didn't care to make the code uglier.
>>     
> I agree.
>
>   
>> @@ -273,8 +273,10 @@
>>      acpi_create_facs(facs);
>>
>>      dsdt = (acpi_header_t *) current;
>> -    current += AmlCode.length;
>> -    memcpy((void *) dsdt, &AmlCode, AmlCode.length);
>> +    memcpy((void *) dsdt, AmlCode, sizeof(acpi_header_t));
>> +    int len = dsdt->length;
>> +    current += len;
>> +    memcpy((void *) dsdt, AmlCode, len);
>>     
> Why not:
>   
>> +    current += dstd->length;
>> +    memcpy((void *) dsdt, AmlCode, dsdt->length);
>>     
> I don't think the extra variable adds anything.
>   
Ah, I thought in terms of "it's overwriting memory while it's using it".
Was I too careful? I think so.

Patch

Index: src/mainboard/intel/d945gclf/acpi_tables.c
===================================================================
--- src/mainboard/intel/d945gclf/acpi_tables.c    (revision 5297)
+++ src/mainboard/intel/d945gclf/acpi_tables.c    (working copy)
@@ -31,7 +31,7 @@ 
 
 #define OLD_ACPI 0
 
-extern const acpi_header_t AmlCode;
+extern unsigned char AmlCode[];
 #if CONFIG_HAVE_ACPI_SLIC
 unsigned long acpi_create_slic(unsigned long current);
 #endif
@@ -273,8 +273,10 @@ 
     acpi_create_facs(facs);
 
     dsdt = (acpi_header_t *) current;
-    current += AmlCode.length;
-    memcpy((void *) dsdt, &AmlCode, AmlCode.length);
+    memcpy((void *) dsdt, AmlCode, sizeof(acpi_header_t));
+    int len = dsdt->length;
+    current += len;
+    memcpy((void *) dsdt, AmlCode, len);
 
 #if OLD_ACPI
     for (i=0; i < dsdt->length; i++) {