ibft: Fix finding ibft with ACPI tables

PATCH In review Yinghai Lu 4 participants 10 messages Dec 8, 2011 – Dec 12, 2011 iSCSI boot firmware table (iBFT) driver

lore

Share & tools

lore

Yinghai Lu

author

2011-12-08 08:23 UTC #0 lore raw

PATCH 1/1 ibft: Fix finding ibft with ACPI tables

Found one system with UEFI/iBFT is not detected.

the root cause: for x86, We move calling of find_ibft_region() much earlier. in setup_arch() before ACPI is enabled.

Try to all find_ibft_region() second times in ibft_init()

at that time ACPI iBFT already get permanent mapped with ioremap. So isa_virt_to_bus will get wrong phys from right virt address. We could just skip that printing.
For legacy one, print the found address early.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/firmware/iscsi_ibft.c      |   18 +++++++++++++++---
 drivers/firmware/iscsi_ibft_find.c |    1 +
 2 files changed, 16 insertions(+), 3 deletions(-)

drivers/firmware/iscsi_ibft.c +15 -3

Index: 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();
+	}
+
 	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.
+		 */
+		pr_info("iBFT detected.\n");
 
 		rc = ibft_check_device();
 		if (rc)

@@ -770,7 +782,7 @@ static int __init ibft_init(void)
 		if (rc)
 			goto out_free;
 	} else
-		printk(KERN_INFO "No iBFT detected.\n");
+		pr_info("No iBFT detected.\n");
 
 	return 0;

drivers/firmware/iscsi_ibft_find.c +1 -0

Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================
@@ -94,6 +94,7 @@ static int __init find_ibft_in_mem(void)
 				 * the table cannot be valid. */
 				if (pos + len <= (IBFT_END-1)) {
 					ibft_addr = (struct acpi_table_ibft *)virt;
+					pr_info("iBFT found at 0x%lx.\n", pos);
 					goto done;
 				}
 			}

Konrad Rzeszutek Wilk

2011-12-08 14:30 UTC #1 lore raw

Quoted 1 line · Yinghai Lu

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."

Quoted 3 lines

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' ?

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?

Quoted 2 lines

Try to all find_ibft_region() second times in ibft_init()

^^^ - all? ^^^^ - time

How many iBFT tables are there? You can drop the 'all'.

Quoted 3 lines

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."

Quoted 1 line

We could just skip that printing.

That sounds like another patch - a cleanup patch actually.

Quoted 14 lines

For legacy one, print the found address early.
Signed-off-by Yinghai Lu <[email protected]>
---
 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
===================================================================

drivers/firmware/iscsi_ibft.c static int __init ibft_init(void) · line 761

Quoted 9 lines

drivers/firmware/iscsi_ibft.c +6 -0

@@ -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.

drivers/firmware/iscsi_ibft.c static int __init ibft_init(void) · line 768

Quoted 9 lines

drivers/firmware/iscsi_ibft.c +6 -2

+	}
+
 	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."

Quoted 19 lines

drivers/firmware/iscsi_ibft.c +3 -1

+		 */
+		pr_info("iBFT detected.\n");
 
 		rc = ibft_check_device();
 		if (rc)

@@ -770,7 +782,7 @@ static int __init ibft_init(void)
 		if (rc)
 			goto out_free;
 	} else
-		printk(KERN_INFO "No iBFT detected.\n");
+		pr_info("No iBFT detected.\n");
 
 	return 0;

Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================

drivers/firmware/iscsi_ibft_find.c static int __init find_ibft_in_mem(void) · line 97

Quoted 4 lines

drivers/firmware/iscsi_ibft_find.c +1 -0

@@ -94,6 +94,7 @@ static int __init find_ibft_in_mem(void)
 				 * the table cannot be valid. */
 				if (pos + len <= (IBFT_END-1)) {
 					ibft_addr = (struct acpi_table_ibft *)virt;
+					pr_info("iBFT found at 0x%lx.\n", pos);

On legacy hardware we will get then:

iBFT found at 0xXXX
...
iBFT detected.

Which I am not sure is really needed. We could just get rid of this iBFT found at 0xXX - but that is a seperate patch - a cleanup patch.

Peter, do you know if Anaconda scans the dmesg for the 'iBFT'? I presume not (since the SysFS exists).

Please resend the fix by itself and also a cleanup patch. Thank you!

Peter Jones

maintainer

2011-12-08 15:02 UTC #2 lore raw

Quoted 2 lines · Konrad Rzeszutek Wilk

Peter, do you know if Anaconda scans the dmesg for the 'iBFT'? I presume not (since the SysFS exists).

It does not.

Signature · 1 line

        Peter

Yinghai Lu

author

2011-12-08 16:52 UTC #3 lore raw

In reply to Konrad Rzeszutek Wilk

Quoted 15 lines · Konrad Rzeszutek Wilk

Quoted 1 line · Yinghai Lu
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."
Quoted 1 line

Quoted 8 lines

Quoted 2 lines
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.

Quoted 6 lines

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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 11 lines

Quoted 4 lines
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."
Quoted 1 line
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.

Quoted 18 lines

Quoted 18 lines
For legacy one, print the found address early.
Signed-off-by Yinghai Lu <[email protected]>
---
 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
===================================================================
drivers/firmware/iscsi_ibft.c +0 -0
@@ -753,9 +753,21 @@ static int __init ibft_init(void)
 {
 	int rc = 0;
 

Quoted 8 lines

drivers/firmware/iscsi_ibft.c static int __init ibft_init(void) · line 761
Quoted 6 lines
drivers/firmware/iscsi_ibft.c +6 -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.

Quoted 15 lines

drivers/firmware/iscsi_ibft.c static int __init ibft_init(void) · line 768
Quoted 10 lines
drivers/firmware/iscsi_ibft.c +6 -2
+	}
+
 	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.

Quoted 5 lines

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

Yinghai Lu

author

2011-12-08 16:54 UTC #4 lore raw

PATCH 1/1 ibft: Fix finding IBFT ACPI table on UEFI

In reply to Konrad Rzeszutek Wilk

Found one system with UEFI/iBFT, Kernel does not detect the iBFT during iscsi_ibft module loading.

the root cause: for x86, We have calling of find_ibft_region() much early. in setup_arch() before ACPI is enabled.

Try to call find_ibft_region() second time in ibft_init().

At that time ACPI iBFT already get permanent mapped with ioremap. So isa_virt_to_bus will get wrong phys from right virt address. We could just skip that phys address printing.

For legacy one, print the found address early.

-v2: update comments and description according to Konrad.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/firmware/iscsi_ibft.c      |   14 ++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |    1 +
 2 files changed, 13 insertions(+), 2 deletions(-)

drivers/firmware/iscsi_ibft.c +12 -2

Index: linux-2.6/drivers/firmware/iscsi_ibft.c
===================================================================
@@ -753,9 +753,19 @@ static int __init ibft_init(void)
 {
 	int rc = 0;
 
+	/*
+	   Retry as on UEFI systems the setup_arch()/find_ibft_region()
+	   is called before ACPI tables are parsed so we never
+	   get the data.
+	*/
+	if (!ibft_addr) {
+		unsigned long size = 0;
+
+		find_ibft_region(&size);
+	}
+
 	if (ibft_addr) {
-		printk(KERN_INFO "iBFT detected at 0x%llx.\n",
-		       (u64)isa_virt_to_bus(ibft_addr));
+		pr_info("iBFT detected.\n");
 
 		rc = ibft_check_device();
 		if (rc)

drivers/firmware/iscsi_ibft_find.c +1 -0

Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================
@@ -94,6 +94,7 @@ static int __init find_ibft_in_mem(void)
 				 * the table cannot be valid. */
 				if (pos + len <= (IBFT_END-1)) {
 					ibft_addr = (struct acpi_table_ibft *)virt;
+					pr_info("iBFT found at 0x%lx.\n", pos);
 					goto done;
 				}
 			}

Konrad Rzeszutek Wilk

2011-12-08 19:08 UTC #5 lore raw

Re: [PATCH -v2] ibft: Fix finding IBFT ACPI table on UEFI

Quoted 2 lines · Yinghai Lu

Found one system with UEFI/iBFT, Kernel does not detect the iBFT during

                                   ^^^^^ - lowercase 'kernel' please.

Quoted 1 line

iscsi_ibft module loading.

I get this when compiling it:

Setup is 16588 bytes (padded to 16896 bytes).
System is 6277 kB
CRC 7fe65506
Kernel: arch/x86/boot/bzImage is ready (#2)
ERROR: "find_ibft_region" [drivers/firmware/iscsi_ibft.ko] undefined!

This is on i386 x86 build when doing 'make allmodconfig'.

Quoted 2 lines

the root cause: for x86, We have calling of find_ibft_region() much early.

^^ - You only need to uppercase it if you
start a sentence - which is not what you are doing. So please make it lowercase.

Quoted 1 line

in setup_arch() before ACPI is enabled.

This is what I changed the git description to:

Root cause: on x86 (UEFI), we are calling find_ibft_region() much earlier - specifically in setup_arch() before ACPI is enabled.

Quoted 61 lines

Try to call find_ibft_region() second time in ibft_init().
At that time ACPI iBFT already get permanent mapped with ioremap. So isa_virt_to_bus will get wrong phys from right virt address. We could just skip that phys address printing.
For legacy one, print the found address early.
-v2: update comments and description according to Konrad.
Signed-off-by Yinghai Lu <[email protected]>
---
 drivers/firmware/iscsi_ibft.c      |   14 ++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |    1 +
 2 files changed, 13 insertions(+), 2 deletions(-)
Index: linux-2.6/drivers/firmware/iscsi_ibft.c
===================================================================
drivers/firmware/iscsi_ibft.c +12 -2
@@ -753,9 +753,19 @@ static int __init ibft_init(void)
 {
 	int rc = 0;
 
+	/*
+	   Retry as on UEFI systems the setup_arch()/find_ibft_region()
+	   is called before ACPI tables are parsed so we never
+	   get the data.
+	*/
+	if (!ibft_addr) {
+		unsigned long size = 0;
+
+		find_ibft_region(&size);
+	}
+
 	if (ibft_addr) {
-		printk(KERN_INFO "iBFT detected at 0x%llx.\n",
-		       (u64)isa_virt_to_bus(ibft_addr));
+		pr_info("iBFT detected.\n");
 
 		rc = ibft_check_device();
 		if (rc)
Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================
drivers/firmware/iscsi_ibft_find.c +1 -1
@@ -94,6 +94,7 @@ static int __init find_ibft_in_mem(void)
 				 * the table cannot be valid. */
 				if (pos + len <= (IBFT_END-1)) {
 					ibft_addr = (struct acpi_table_ibft *)virt;
+					pr_info("iBFT found at 0x%lx.\n", pos);
 					goto done;
 				}
 			}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Yinghai Lu

author

2011-12-08 21:18 UTC #6 lore raw

Re: [PATCH -v2] ibft: Fix finding IBFT ACPI table on UEFI

Quoted 8 lines · Konrad Rzeszutek Wilk

Quoted 2 lines · Yinghai Lu
Found one system with UEFI/iBFT, Kernel does not detect the iBFT during
                                   ^^^^^ - lowercase 'kernel' please.
Quoted 1 line
iscsi_ibft module loading.
I get this when compiling it:

Quoted 7 lines

Setup is 16588 bytes (padded to 16896 bytes).
System is 6277 kB
CRC 7fe65506
Kernel: arch/x86/boot/bzImage is ready (#2)
ERROR: "find_ibft_region" [drivers/firmware/iscsi_ibft.ko] undefined!
This is on i386 x86 build when doing 'make allmodconfig'.

then will need more cleaner change.

Quoted 6 lines

Quoted 3 lines
the root cause: for x86, We have calling of find_ibft_region() much early.
^^ - You only need to uppercase it if you
start a sentence - which is not what you are doing. So please make it lowercase.

Quoted 7 lines

Quoted 1 line
in setup_arch() before ACPI is enabled.
This is what I changed the git description to:
Root cause: on x86 (UEFI), we are calling find_ibft_region() much earlier - specifically in setup_arch() before ACPI is enabled.

will use that...

Thanks

Yinghai

Yinghai Lu

author

2011-12-08 21:19 UTC #7 lore raw

PATCH 1/1 ibft: Fix finding IBFT ACPI table on UEFI

In reply to Konrad Rzeszutek Wilk

Found one system with UEFI/iBFT, kernel does not detect the iBFT during iscsi_ibft module loading.

Root cause: on x86 (UEFI), we are calling of find_ibft_region() much earlier
- specifically in setup_arch() before ACPI is enabled.

Try to split acpi checking code out and call that later

At that time ACPI iBFT already get permanent mapped with ioremap. So isa_virt_to_bus() will get wrong phys from right virt address. We could just skip that phys address printing.

For legacy one, print the found address early.

-v2: update comments and description according to Konrad. -v3: fix problem about module use case that is found by Konrad.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/firmware/iscsi_ibft.c      |   43 +++++++++++++++++++++++++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |   26 +---------------------
 2 files changed, 43 insertions(+), 26 deletions(-)

drivers/firmware/iscsi_ibft.c +41 -2

Index: linux-2.6/drivers/firmware/iscsi_ibft.c
===================================================================
@@ -746,6 +746,38 @@ static void __exit ibft_exit(void)
 	ibft_cleanup();
 }
 
+#ifdef CONFIG_ACPI
+static const struct {
+	char *sign;
+} ibft_signs[] = {
+	/*
+	 * One spec says "IBFT", the other says "iBFT". We have to check
+	 * for both.
+	 */
+	{ ACPI_SIG_IBFT },
+	{ "iBFT" },
+};
+
+static int __init acpi_find_ibft(struct acpi_table_header *header)
+{
+	ibft_addr = (struct acpi_table_ibft *)header;
+
+	return 0;
+}
+
+static void __init acpi_find_ibft_region(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
+		acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
+}
+#else
+static void __init acpi_find_ibft_region(void)
+{
+}
+#endif
+
 /*
  * ibft_init() - creates sysfs tree entries for the iBFT data.
  */

@@ -753,9 +785,16 @@ static int __init ibft_init(void)
 {
 	int rc = 0;
 
+	/*
+	   As on UEFI systems the setup_arch()/find_ibft_region()
+	   is called before ACPI tables are parsed and it only does
+	   legacy finding.
+	*/
+	if (!ibft_addr)
+		acpi_find_ibft_region();
+
 	if (ibft_addr) {
-		printk(KERN_INFO "iBFT detected at 0x%llx.\n",
-		       (u64)isa_virt_to_bus(ibft_addr));
+		pr_info("iBFT detected.\n");
 
 		rc = ibft_check_device();
 		if (rc)

drivers/firmware/iscsi_ibft_find.c +2 ~~-24~~

Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================
@@ -45,13 +45,6 @@ EXPORT_SYMBOL_GPL(ibft_addr);
 static const struct {
 	char *sign;
 } ibft_signs[] = {
-#ifdef CONFIG_ACPI
-	/*
-	 * One spec says "IBFT", the other says "iBFT". We have to check
-	 * for both.
-	 */
-	{ ACPI_SIG_IBFT },
-#endif
 	{ "iBFT" },
 	{ "BIFT" },	/* Broadcom iSCSI Offload */
 };

@@ -62,14 +55,6 @@ static const struct {
 #define VGA_MEM 0xA0000 /* VGA buffer */
 #define VGA_SIZE 0x20000 /* 128kB */
 
-#ifdef CONFIG_ACPI
-static int __init acpi_find_ibft(struct acpi_table_header *header)
-{
-	ibft_addr = (struct acpi_table_ibft *)header;
-	return 0;
-}
-#endif /* CONFIG_ACPI */
-
 static int __init find_ibft_in_mem(void)
 {
 	unsigned long pos;

@@ -94,6 +79,7 @@ static int __init find_ibft_in_mem(void)
 				 * the table cannot be valid. */
 				if (pos + len <= (IBFT_END-1)) {
 					ibft_addr = (struct acpi_table_ibft *)virt;
+					pr_info("iBFT found at 0x%lx.\n", pos);
 					goto done;
 				}
 			}

@@ -108,20 +94,12 @@ done:
  */
 unsigned long __init find_ibft_region(unsigned long *sizep)
 {
-#ifdef CONFIG_ACPI
-	int i;
-#endif
 	ibft_addr = NULL;
 
-#ifdef CONFIG_ACPI
-	for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
-		acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
-#endif /* CONFIG_ACPI */
-
 	/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will
 	 * only use ACPI for this */
 
-	if (!ibft_addr && !efi_enabled)
+	if (!efi_enabled)
 		find_ibft_in_mem();
 
 	if (ibft_addr) {

Konrad Rzeszutek Wilk

2011-12-12 17:19 UTC #8 lore raw

Re: [PATCH -v3] ibft: Fix finding IBFT ACPI table on UEFI

Quoted 6 lines · Yinghai Lu

Found one system with UEFI/iBFT, kernel does not detect the iBFT during iscsi_ibft module loading.
Root cause: on x86 (UEFI), we are calling of find_ibft_region() much earlier
- specifically in setup_arch() before ACPI is enabled.

I get this when compiling with make allmodconfig:

BUILD arch/x86/boot/bzImage
Setup is 16636 bytes (padded to 16896 bytes).
System is 4680 kB
CRC 88f70464
Kernel: arch/x86/boot/bzImage is ready (#2)
ERROR: "acpi_table_parse" [drivers/firmware/iscsi_ibft.ko] undefined! make[3]: *** [__modpost] Error 1
make[2]: *** [modules] Error 2

on 64-bit Fedora Core 16.

Quoted 149 lines

Try to split acpi checking code out and call that later

At that time ACPI iBFT already get permanent mapped with ioremap. So isa_virt_to_bus() will get wrong phys from right virt address. We could just skip that phys address printing.

For legacy one, print the found address early.

(quoted diff) +0 -2

-v2: update comments and description according to Konrad.
-v3: fix problem about module use case that is found by Konrad.

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/firmware/iscsi_ibft.c      |   43 +++++++++++++++++++++++++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |   26 +---------------------
 2 files changed, 43 insertions(+), 26 deletions(-)

Index: linux-2.6/drivers/firmware/iscsi_ibft.c
===================================================================

drivers/firmware/iscsi_ibft.c +41 -2

@@ -746,6 +746,38 @@ static void __exit ibft_exit(void)
 	ibft_cleanup();
 }
 
+#ifdef CONFIG_ACPI
+static const struct {
+	char *sign;
+} ibft_signs[] = {
+	/*
+	 * One spec says "IBFT", the other says "iBFT". We have to check
+	 * for both.
+	 */
+	{ ACPI_SIG_IBFT },
+	{ "iBFT" },
+};
+
+static int __init acpi_find_ibft(struct acpi_table_header *header)
+{
+	ibft_addr = (struct acpi_table_ibft *)header;
+
+	return 0;
+}
+
+static void __init acpi_find_ibft_region(void)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
+		acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
+}
+#else
+static void __init acpi_find_ibft_region(void)
+{
+}
+#endif
+
 /*
  * ibft_init() - creates sysfs tree entries for the iBFT data.
  */

@@ -753,9 +785,16 @@ static int __init ibft_init(void)
 {
 	int rc = 0;
 
+	/*
+	   As on UEFI systems the setup_arch()/find_ibft_region()
+	   is called before ACPI tables are parsed and it only does
+	   legacy finding.
+	*/
+	if (!ibft_addr)
+		acpi_find_ibft_region();
+
 	if (ibft_addr) {
-		printk(KERN_INFO "iBFT detected at 0x%llx.\n",
-		       (u64)isa_virt_to_bus(ibft_addr));
+		pr_info("iBFT detected.\n");
 
 		rc = ibft_check_device();
 		if (rc)

Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================

drivers/firmware/iscsi_ibft_find.c +2 ~~-25~~

@@ -45,13 +45,6 @@ EXPORT_SYMBOL_GPL(ibft_addr);
 static const struct {
 	char *sign;
 } ibft_signs[] = {
-#ifdef CONFIG_ACPI
-	/*
-	 * One spec says "IBFT", the other says "iBFT". We have to check
-	 * for both.
-	 */
-	{ ACPI_SIG_IBFT },
-#endif
 	{ "iBFT" },
 	{ "BIFT" },	/* Broadcom iSCSI Offload */
 };

@@ -62,14 +55,6 @@ static const struct {
 #define VGA_MEM 0xA0000 /* VGA buffer */
 #define VGA_SIZE 0x20000 /* 128kB */
 
-#ifdef CONFIG_ACPI
-static int __init acpi_find_ibft(struct acpi_table_header *header)
-{
-	ibft_addr = (struct acpi_table_ibft *)header;
-	return 0;
-}
-#endif /* CONFIG_ACPI */
-
 static int __init find_ibft_in_mem(void)
 {
 	unsigned long pos;

@@ -94,6 +79,7 @@ static int __init find_ibft_in_mem(void)
 				 * the table cannot be valid. */
 				if (pos + len <= (IBFT_END-1)) {
 					ibft_addr = (struct acpi_table_ibft *)virt;
+					pr_info("iBFT found at 0x%lx.\n", pos);
 					goto done;
 				}
 			}

@@ -108,20 +94,12 @@ done:
  */
 unsigned long __init find_ibft_region(unsigned long *sizep)
 {
-#ifdef CONFIG_ACPI
-	int i;
-#endif
 	ibft_addr = NULL;
 
-#ifdef CONFIG_ACPI
-	for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
-		acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
-#endif /* CONFIG_ACPI */
-
 	/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will
 	 * only use ACPI for this */
 
-	if (!ibft_addr && !efi_enabled)
+	if (!efi_enabled)
 		find_ibft_in_mem();
 
 	if (ibft_addr) {
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Yinghai Lu

author

2011-12-12 20:40 UTC #9 lore raw

Re: [PATCH -v3] ibft: Fix finding IBFT ACPI table on UEFI

Quoted 21 lines · Konrad Rzeszutek Wilk

Quoted 6 lines · Yinghai Lu
Found one system with UEFI/iBFT, kernel does not detect the iBFT during iscsi_ibft module loading.
Root cause: on x86 (UEFI), we are calling of find_ibft_region() much earlier
- specifically in setup_arch() before ACPI is enabled.
I get this when compiling with make allmodconfig:
BUILD arch/x86/boot/bzImage
Setup is 16636 bytes (padded to 16896 bytes).
System is 4680 kB
CRC 88f70464
Kernel: arch/x86/boot/bzImage is ready (#2)
ERROR: "acpi_table_parse" [drivers/firmware/iscsi_ibft.ko] undefined! make[3]: *** [__modpost] Error 1
make[2]: *** [modules] Error 2

Quoted 1 line

on 64-bit Fedora Core 16.

oh, my bad!

Sorry for that.

please check this one....

[PATCH -v4] ibft: Fix finding IBFT ACPI table on UEFI

Found one system with UEFI/iBFT, kernel does not detect the iBFT during iscsi_ibft module loading.

Root cause: on x86 (UEFI), we are calling of find_ibft_region() much earlier
- specifically in setup_arch() before ACPI is enabled.

Try to split acpi checking code out and call that later

At that time ACPI iBFT already get permanent mapped with ioremap. So isa_virt_to_bus() will get wrong phys from right virt address. We could just skip that phys address printing.

For legacy one, print the found address early.

-v2: update comments and description according to Konrad.
-v3: fix problem about module use case that is found by Konrad.
-v4: use acpi_get_table() instead of acpi_table_parse() to handle module use case that is found by Konrad again..

Signed-off-by Yinghai Lu <[email protected]>

---
 drivers/firmware/iscsi_ibft.c      |   42 +++++++++++++++++++++++++++++++++++--
 drivers/firmware/iscsi_ibft_find.c |   26 +---------------------
 2 files changed, 42 insertions(+), 26 deletions(-)

drivers/firmware/iscsi_ibft.c +40 -2

Index: linux-2.6/drivers/firmware/iscsi_ibft.c
===================================================================
@@ -746,6 +746,37 @@ static void __exit ibft_exit(void)
 	ibft_cleanup();
 }
 
+#ifdef CONFIG_ACPI
+static const struct {
+	char *sign;
+} ibft_signs[] = {
+	/*
+	 * One spec says "IBFT", the other says "iBFT". We have to check
+	 * for both.
+	 */
+	{ ACPI_SIG_IBFT },
+	{ "iBFT" },
+};
+
+static void __init acpi_find_ibft_region(void)
+{
+	int i;
+	struct acpi_table_header *table = NULL;
+
+	if (acpi_disabled)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++) {
+		acpi_get_table(ibft_signs[i].sign, 0, &table);
+		ibft_addr = (struct acpi_table_ibft *)table;
+	}
+}
+#else
+static void __init acpi_find_ibft_region(void)
+{
+}
+#endif
+
 /*
  * ibft_init() - creates sysfs tree entries for the iBFT data.
  */

@@ -753,9 +784,16 @@ static int __init ibft_init(void)
 {
 	int rc = 0;
 
+	/*
+	   As on UEFI systems the setup_arch()/find_ibft_region()
+	   is called before ACPI tables are parsed and it only does
+	   legacy finding.
+	*/
+	if (!ibft_addr)
+		acpi_find_ibft_region();
+
 	if (ibft_addr) {
-		printk(KERN_INFO "iBFT detected at 0x%llx.\n",
-		       (u64)isa_virt_to_bus(ibft_addr));
+		pr_info("iBFT detected.\n");
 
 		rc = ibft_check_device();
 		if (rc)

drivers/firmware/iscsi_ibft_find.c +2 ~~-24~~

Index: linux-2.6/drivers/firmware/iscsi_ibft_find.c
===================================================================
@@ -45,13 +45,6 @@ EXPORT_SYMBOL_GPL(ibft_addr);
 static const struct {
 	char *sign;
 } ibft_signs[] = {
-#ifdef CONFIG_ACPI
-	/*
-	 * One spec says "IBFT", the other says "iBFT". We have to check
-	 * for both.
-	 */
-	{ ACPI_SIG_IBFT },
-#endif
 	{ "iBFT" },
 	{ "BIFT" },	/* Broadcom iSCSI Offload */
 };

@@ -62,14 +55,6 @@ static const struct {
 #define VGA_MEM 0xA0000 /* VGA buffer */
 #define VGA_SIZE 0x20000 /* 128kB */
 
-#ifdef CONFIG_ACPI
-static int __init acpi_find_ibft(struct acpi_table_header *header)
-{
-	ibft_addr = (struct acpi_table_ibft *)header;
-	return 0;
-}
-#endif /* CONFIG_ACPI */
-
 static int __init find_ibft_in_mem(void)
 {
 	unsigned long pos;

@@ -94,6 +79,7 @@ static int __init find_ibft_in_mem(void)
 				 * the table cannot be valid. */
 				if (pos + len <= (IBFT_END-1)) {
 					ibft_addr = (struct acpi_table_ibft *)virt;
+					pr_info("iBFT found at 0x%lx.\n", pos);
 					goto done;
 				}
 			}

@@ -108,20 +94,12 @@ done:
  */
 unsigned long __init find_ibft_region(unsigned long *sizep)
 {
-#ifdef CONFIG_ACPI
-	int i;
-#endif
 	ibft_addr = NULL;
 
-#ifdef CONFIG_ACPI
-	for (i = 0; i < ARRAY_SIZE(ibft_signs) && !ibft_addr; i++)
-		acpi_table_parse(ibft_signs[i].sign, acpi_find_ibft);
-#endif /* CONFIG_ACPI */
-
 	/* iBFT 1.03 section 1.4.3.1 mandates that UEFI machines will
 	 * only use ACPI for this */
 
-	if (!ibft_addr && !efi_enabled)
+	if (!efi_enabled)
 		find_ibft_in_mem();
 
 	if (ibft_addr) {