Re: [PATCH] efi: add conditional include of asm/early_ioremap.h

Re: [PATCH] efi: add conditional include of asm/early_ioremap.h

[Matt Fleming]

(Cc'ing ia64 maintainers)

On Sun, 10 Jan, at 09:04:09PM, Ard Biesheuvel wrote:
> The code in efi.c uses early_memremap(), but relies on a transitive
> include rather than including asm/early_ioremap.h directly.
> Unfortunately, this header does not exist on ia64, so it cannot be
> included directly.
> 
> Commit f7d924894265 ("arm64/efi: refactor EFI init and runtime code
> for reuse by 32-bit ARM") attempted to work around this by including
> asm/efi.h, which transitively includes asm/early_ioremap.h on most
> architectures. However, since asm/efi.h does not exist on ia64 either,
> this is not much of an improvement.
> 
> So instead, just include asm/early_ioremap.h directly, unless CONFIG_IA64
> is defined.
> 
> Fixes: f7d924894265 ("arm64/efi: refactor EFI init and runtime code for reuse by 32-bit ARM")
> Cc: Will Deacon <will.deacon@arm.com>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  drivers/firmware/efi/efi.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index cffa89b3317b..53f71177824f 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -25,7 +25,9 @@
>  #include <linux/io.h>
>  #include <linux/platform_device.h>
>  
> -#include <asm/efi.h>
> +#ifndef CONFIG_IA64
> +#include <asm/early_ioremap.h>
> +#endif
>  
>  struct efi __read_mostly efi = {
>  	.mps			= EFI_INVALID_TABLE_ADDR,

I certainly can't think of a better way to fix this right now. Tony,
Fenghua do you guys have any suggestions?

Reviewed-by: Matt Fleming <matt@codeblueprint.co.uk>

Re: [PATCH] efi: add conditional include of asm/early_ioremap.h

[Guenter Roeck]

On 01/11/2016 06:40 AM, Matt Fleming wrote:
> (Cc'ing ia64 maintainers)
>
> On Sun, 10 Jan, at 09:04:09PM, Ard Biesheuvel wrote:
>> The code in efi.c uses early_memremap(), but relies on a transitive
>> include rather than including asm/early_ioremap.h directly.
>> Unfortunately, this header does not exist on ia64, so it cannot be
>> included directly.
>>
>> Commit f7d924894265 ("arm64/efi: refactor EFI init and runtime code
>> for reuse by 32-bit ARM") attempted to work around this by including
>> asm/efi.h, which transitively includes asm/early_ioremap.h on most
>> architectures. However, since asm/efi.h does not exist on ia64 either,
>> this is not much of an improvement.
>>
>> So instead, just include asm/early_ioremap.h directly, unless CONFIG_IA64
>> is defined.
>>
>> Fixes: f7d924894265 ("arm64/efi: refactor EFI init and runtime code for reuse by 32-bit ARM")
>> Cc: Will Deacon <will.deacon@arm.com>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>   drivers/firmware/efi/efi.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>> index cffa89b3317b..53f71177824f 100644
>> --- a/drivers/firmware/efi/efi.c
>> +++ b/drivers/firmware/efi/efi.c
>> @@ -25,7 +25,9 @@
>>   #include <linux/io.h>
>>   #include <linux/platform_device.h>
>>
>> -#include <asm/efi.h>
>> +#ifndef CONFIG_IA64
>> +#include <asm/early_ioremap.h>
>> +#endif
>>
>>   struct efi __read_mostly efi = {
>>   	.mps			= EFI_INVALID_TABLE_ADDR,
>
> I certainly can't think of a better way to fix this right now. Tony,
> Fenghua do you guys have any suggestions?
>

How about including asm/early_ioremap.h from arch/arm/include/asm/io.h,
as it is done by arm64 and x86 ?

Guenter

Re: [PATCH] efi: add conditional include of asm/early_ioremap.h

[Ard Biesheuvel]

(+ Dan)

On 11 January 2016 at 17:15, Guenter Roeck <linux@roeck-us.net> wrote:
> On 01/11/2016 06:40 AM, Matt Fleming wrote:
>>
>> (Cc'ing ia64 maintainers)
>>
>> On Sun, 10 Jan, at 09:04:09PM, Ard Biesheuvel wrote:
>>>
>>> The code in efi.c uses early_memremap(), but relies on a transitive
>>> include rather than including asm/early_ioremap.h directly.
>>> Unfortunately, this header does not exist on ia64, so it cannot be
>>> included directly.
>>>
>>> Commit f7d924894265 ("arm64/efi: refactor EFI init and runtime code
>>> for reuse by 32-bit ARM") attempted to work around this by including
>>> asm/efi.h, which transitively includes asm/early_ioremap.h on most
>>> architectures. However, since asm/efi.h does not exist on ia64 either,
>>> this is not much of an improvement.
>>>
>>> So instead, just include asm/early_ioremap.h directly, unless CONFIG_IA64
>>> is defined.
>>>
>>> Fixes: f7d924894265 ("arm64/efi: refactor EFI init and runtime code for
>>> reuse by 32-bit ARM")
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>   drivers/firmware/efi/efi.c | 4 +++-
>>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
>>> index cffa89b3317b..53f71177824f 100644
>>> --- a/drivers/firmware/efi/efi.c
>>> +++ b/drivers/firmware/efi/efi.c
>>> @@ -25,7 +25,9 @@
>>>   #include <linux/io.h>
>>>   #include <linux/platform_device.h>
>>>
>>> -#include <asm/efi.h>
>>> +#ifndef CONFIG_IA64
>>> +#include <asm/early_ioremap.h>
>>> +#endif
>>>
>>>   struct efi __read_mostly efi = {
>>>         .mps                    = EFI_INVALID_TABLE_ADDR,
>>
>>
>> I certainly can't think of a better way to fix this right now. Tony,
>> Fenghua do you guys have any suggestions?
>>
>
> How about including asm/early_ioremap.h from arch/arm/include/asm/io.h,
> as it is done by arm64 and x86 ?
>

That would work as well, but it does not address how early_memremap()
is so poorly defined. I think the correct way would be to add the
include unconditionally, and move ia64's definition to a new ia64
version of early_ioremap.h

Re: [PATCH] efi: add conditional include of asm/early_ioremap.h

[Guenter Roeck]

On Mon, Jan 11, 2016 at 06:35:44PM +0100, Ard Biesheuvel wrote:
[ ... ]

> >>
> >>
> >> I certainly can't think of a better way to fix this right now. Tony,
> >> Fenghua do you guys have any suggestions?
> >>
> >
> > How about including asm/early_ioremap.h from arch/arm/include/asm/io.h,
> > as it is done by arm64 and x86 ?
> >
> 
> That would work as well, but it does not address how early_memremap()
> is so poorly defined. I think the correct way would be to add the
> include unconditionally, and move ia64's definition to a new ia64
> version of early_ioremap.h

Depends on the definition of "poorly". Is the API exported through
asm/early_ioremap.h, or through asm/io.h ?

Guenter

RE: [PATCH] efi: add conditional include of asm/early_ioremap.h

[Luck, Tony]

On Sun, 10 Jan, at 09:04:09PM, Ard Biesheuvel wrote:
> The code in efi.c uses early_memremap(), but relies on a transitive
> include rather than including asm/early_ioremap.h directly.
> Unfortunately, this header does not exist on ia64, so it cannot be
> included directly.

I wouldn't be adverse to you making an asm/early_ioremap.h file
for ia64

-Tony

Re: [PATCH] efi: add conditional include of asm/early_ioremap.h

[Ard Biesheuvel]

On 11 January 2016 at 20:54, Luck, Tony <tony.luck@intel.com> wrote:
> On Sun, 10 Jan, at 09:04:09PM, Ard Biesheuvel wrote:
>> The code in efi.c uses early_memremap(), but relies on a transitive
>> include rather than including asm/early_ioremap.h directly.
>> Unfortunately, this header does not exist on ia64, so it cannot be
>> included directly.
>
> I wouldn't be adverse to you making an asm/early_ioremap.h file
> for ia64
>

How about an asm/efi.h? A lot of the EFI source files include both
linux/efi.h and asm/efi.h, so it would make sense to have one on ia64
as well

RE: [PATCH] efi: add conditional include of asm/early_ioremap.h

[Luck, Tony]

Pj4gSSB3b3VsZG4ndCBiZSBhZHZlcnNlIHRvIHlvdSBtYWtpbmcgYW4gYXNtL2Vhcmx5X2lvcmVt
YXAuaCBmaWxlDQo+PiBmb3IgaWE2NA0KPj4NCj4NCj4gSG93IGFib3V0IGFuIGFzbS9lZmkuaD8g
QSBsb3Qgb2YgdGhlIEVGSSBzb3VyY2UgZmlsZXMgaW5jbHVkZSBib3RoDQo+IGxpbnV4L2VmaS5o
IGFuZCBhc20vZWZpLmgsIHNvIGl0IHdvdWxkIG1ha2Ugc2Vuc2UgdG8gaGF2ZSBvbmUgb24gaWE2
NA0KPiBhcyB3ZWxsDQoNClBlcmhhcHMuICBJZiBJIGp1c3QgZW5kIHVwIHdpdGggYW4gZW1wdHkg
PGFzbS9zb21ldGhpbmcuaD4gZmlsZSBqdXN0DQpzbyBpdCBjYW4gYmUgaW5jbHVkZWQgd2l0aG91
dCBtYWtpbmcgYW4gZXJyb3IgdGhhdCdzIGxlc3MgZGVzaXJhYmxlIHRoYW4NCmhhdmluZyBzb21l
dGhpbmcgcmVsZXZhbnQgaW4gdGhlIGZpbGUuDQoNCi1Ub255DQo