lkml.org 
[lkml]   [2011]   [Dec]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] ibft: Fix finding ibft with ACPI tables
On 12/08/2011 06:29 AM, Konrad Rzeszutek Wilk wrote:

> On Thu, Dec 08, 2011 at 12:22:19AM -0800, Yinghai Lu wrote:
>> Found one system with UEFI/iBFT is not detected.
>
> Excellent.
>
> I have some comment in regards to the patch - it needs to be
> split in two: one part being _just_ the bug-fix, and the other
> being the cleanup/fixing printk.
>
> Please fix the subject - it should say: "Fix finding IBFT ACPI tables
> on UEFI."

>

>>
>> the root cause: for x86, We move calling of find_ibft_region() much earlier.
>> in setup_arch() before ACPI is enabled.
>
> We move calling? When did the find_ibft_region() get moved?
>
> I think you mean "find_ibft_region() gets called in setup_arch(), which
> is done before ACPI is enabled on UEFI. Hence it does not find the IBFT
> table' ?


it is called before
acpi_boot_table_init();
that is too early.

>

> What about the 'memblock_reserve' that find_ibft_region calls? Do
> we need to make a special call on UEFI to reserve that region? Or is
> that not neccessary since it is an ACPI table and has already
> been reserved?


yes, acpi table is reserved already.

>>
>> Try to all find_ibft_region() second times in ibft_init()
> ^^^ - all? ^^^^ - time
>
> How many iBFT tables are there? You can drop the 'all'.


will fix the typo.

>
>>
>> at that time ACPI iBFT already get permanent mapped with ioremap.
>> So isa_virt_to_bus will get wrong phys from right virt address.
>
> .. "will get wrong physical address from the virtual address."
>
>
>> We could just skip that printing.
>
> That sounds like another patch - a cleanup patch actually.


I prefer to having them together. otherwise on uefi/acpi case.

isa_virt_to_bus() will find one strange phys addr from virt with ioremap()

that could cause confuse.

>
>> For legacy one, print the found address early.
>>
>> Signed-off-by: Yinghai Lu <yinghai.lu@kernel.org>
>>
>> ---
>> drivers/firmware/iscsi_ibft.c | 18 +++++++++++++++---
>> drivers/firmware/iscsi_ibft_find.c | 1 +
>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> Index: linux-2.6/drivers/firmware/iscsi_ibft.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/firmware/iscsi_ibft.c
>> +++ linux-2.6/drivers/firmware/iscsi_ibft.c
>> @@ -753,9 +753,21 @@ static int __init ibft_init(void)
>> {
>> int rc = 0;
>>
>> + /* find that from acpi tables */
>> + if (!ibft_addr) {
>> + unsigned long size = 0;
>> +
>> + find_ibft_region(&size);
>> + barrier();
>
> barrier? Please provide a comment detailing why you need it.


will remove that.

>
>> + }
>> +
>> if (ibft_addr) {
>> - printk(KERN_INFO "iBFT detected at 0x%llx.\n",
>> - (u64)isa_virt_to_bus(ibft_addr));
>> + /*
>> + * Second try is from acpi permanent map with ioremap
>> + * can not simply convert back to phys addr.
>> + * and We don't need to print that table phys addr.
>
> That comment makes sense in the git description but not in this
> code path (b/c when you look at the code you won't think of printing
> the "iBFT detected at XXX" comment.
>
> You should move part of this comment to the "if (!ibft_addr)" and just
> say:
> "Retry as on UEFI systems the setup_arch is called before ACPI tables
> are parsed is setup
> so we never get the data."


ok.


will send out updated version.

Thanks

Yinghai


\
 
 \ /
  Last update: 2011-12-08 17:55    [W:0.077 / U:0.956 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site