linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code
@ 2018-08-09 17:17 Stephen Boyd
  2018-08-09 17:17 ` [PATCH v3 1/7] firmware: coreboot: Let OF core populate platform device Stephen Boyd
                   ` (7 more replies)
  0 siblings, 8 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

This series reworks the coreboot firmware driver a bit to fix some bugs
and then simplify the code by changing the design to get rid of the
different platform drivers, remap memory with memremap(), and use devm
to simplify error paths. There's some risk in changing to memremap() but
I think that should work out alright. It will either return memory
directly, or fallback to ioremap_cache() as has been done before.

Changes from v2:
 * A bunch more patches
 * Fix iounmap missing on error path
 * Fix bus getting unregistered on driver unbind and never registered
   again
 * Request exclusive access to table region
 * Pull out table signature check to driver probe

Changes from v1:
 * Split out fixlet for DT based driver from platform driver change

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>

Stephen Boyd (7):
  firmware: coreboot: Let OF core populate platform device
  firmware: coreboot: Unmap ioregion on failure
  firmware: coreboot: Make bus registration symmetric
  firmware: coreboot: Collapse platform drivers into bus core
  firmware: coreboot: Remap RAM with memremap() instead of ioremap()
  firmware: coreboot: Only populate devices in coreboot_table_init()
  firmware: coreboot: Request table region for exclusive access

 drivers/firmware/google/Kconfig               |  26 ++--
 drivers/firmware/google/Makefile              |   2 -
 drivers/firmware/google/coreboot_table-acpi.c |  88 ------------
 drivers/firmware/google/coreboot_table-of.c   |  82 -----------
 drivers/firmware/google/coreboot_table.c      | 135 +++++++++++++-----
 drivers/firmware/google/coreboot_table.h      |   6 -
 6 files changed, 107 insertions(+), 232 deletions(-)
 delete mode 100644 drivers/firmware/google/coreboot_table-acpi.c
 delete mode 100644 drivers/firmware/google/coreboot_table-of.c

-- 
Sent by a computer through tubes


^ permalink raw reply	[flat|nested] 26+ messages in thread

* [PATCH v3 1/7] firmware: coreboot: Let OF core populate platform device
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
@ 2018-08-09 17:17 ` Stephen Boyd
  2018-08-09 17:31   ` Brian Norris
  2018-08-09 17:17 ` [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure Stephen Boyd
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

Now that the /firmware/coreboot node in DT is populated by the core DT
platform code with commit 3aa0582fdb82 ("of: platform: populate
/firmware/ node from of_platform_default_populate_init()") we should and
can remove the platform device creation here. Otherwise, the
of_platform_device_create() call will fail, the coreboot of driver won't
be registered, and this driver will never bind. At the same time, we
should move this driver to use MODULE_DEVICE_TABLE so that module
auto-load works properly when the coreboot device is auto-populated and
we should drop the of_node handling that was presumably placed here to
hold a reference to the DT node created during module init that no
longer happens.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Reviewed-by: Sudeep Holla <Sudeep.Holla@arm.com>
Fixes: 3aa0582fdb82 ("of: platform: populate /firmware/ node from of_platform_default_populate_init()")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table-of.c | 28 +++------------------
 1 file changed, 3 insertions(+), 25 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
index f15bf404c579..9b90c0fa4a0b 100644
--- a/drivers/firmware/google/coreboot_table-of.c
+++ b/drivers/firmware/google/coreboot_table-of.c
@@ -19,7 +19,6 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of_address.h>
-#include <linux/of_platform.h>
 #include <linux/platform_device.h>
 
 #include "coreboot_table.h"
@@ -30,7 +29,6 @@ static int coreboot_table_of_probe(struct platform_device *pdev)
 	void __iomem *ptr;
 
 	ptr = of_iomap(fw_dn, 0);
-	of_node_put(fw_dn);
 	if (!ptr)
 		return -ENOMEM;
 
@@ -44,8 +42,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
 
 static const struct of_device_id coreboot_of_match[] = {
 	{ .compatible = "coreboot" },
-	{},
+	{}
 };
+MODULE_DEVICE_TABLE(of, coreboot_of_match);
 
 static struct platform_driver coreboot_table_of_driver = {
 	.probe = coreboot_table_of_probe,
@@ -55,28 +54,7 @@ static struct platform_driver coreboot_table_of_driver = {
 		.of_match_table = coreboot_of_match,
 	},
 };
-
-static int __init platform_coreboot_table_of_init(void)
-{
-	struct platform_device *pdev;
-	struct device_node *of_node;
-
-	/* Limit device creation to the presence of /firmware/coreboot node */
-	of_node = of_find_node_by_path("/firmware/coreboot");
-	if (!of_node)
-		return -ENODEV;
-
-	if (!of_match_node(coreboot_of_match, of_node))
-		return -ENODEV;
-
-	pdev = of_platform_device_create(of_node, "coreboot_table_of", NULL);
-	if (!pdev)
-		return -ENODEV;
-
-	return platform_driver_register(&coreboot_table_of_driver);
-}
-
-module_init(platform_coreboot_table_of_init);
+module_platform_driver(coreboot_table_of_driver);
 
 MODULE_AUTHOR("Google, Inc.");
 MODULE_LICENSE("GPL");
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
  2018-08-09 17:17 ` [PATCH v3 1/7] firmware: coreboot: Let OF core populate platform device Stephen Boyd
@ 2018-08-09 17:17 ` Stephen Boyd
  2018-08-09 17:49   ` Brian Norris
  2018-08-09 17:17 ` [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric Stephen Boyd
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

Both callers of coreboot_table_init() ioremap the pointer that comes in
but they don't unmap the memory on failure. Both of them also fail probe
immediately with the return value of coreboot_table_init(), leaking a
mapping when it fails. Plug the leak so the mapping isn't left unused.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Fixes: 570d30c2823f ("firmware: coreboot: Expose the coreboot table as a bus")
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 19db5709ae28..0d3e140444ae 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -138,6 +138,9 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
 		ptr_entry += entry.size;
 	}
 
+	if (ret)
+		iounmap(ptr);
+
 	return ret;
 }
 EXPORT_SYMBOL(coreboot_table_init);
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
  2018-08-09 17:17 ` [PATCH v3 1/7] firmware: coreboot: Let OF core populate platform device Stephen Boyd
  2018-08-09 17:17 ` [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure Stephen Boyd
@ 2018-08-09 17:17 ` Stephen Boyd
  2018-08-09 18:10   ` Julius Werner
  2018-08-09 17:17 ` [PATCH v3 4/7] firmware: coreboot: Collapse platform drivers into bus core Stephen Boyd
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

The bus is registered in module_init() but is unregistered when the
platform driver remove() function calls coreboot_table_exit(). That
isn't symmetric and it causes the bus to appear on systems that compile
this code in, even when there isn't any coreboot firmware on the device.
Let's move the registration to the coreboot_table_init() function so
that it matches the exit path.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 0d3e140444ae..0e461d776ce6 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -70,12 +70,6 @@ static struct bus_type coreboot_bus_type = {
 	.remove		= coreboot_bus_remove,
 };
 
-static int __init coreboot_bus_init(void)
-{
-	return bus_register(&coreboot_bus_type);
-}
-module_init(coreboot_bus_init);
-
 static void coreboot_device_release(struct device *dev)
 {
 	struct coreboot_device *device = CB_DEV(dev);
@@ -113,6 +107,10 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
 		return -ENODEV;
 	}
 
+	ret = bus_register(&coreboot_bus_type);
+	if (ret)
+		return ret;
+
 	ptr_entry = (void *)ptr_header + header.header_bytes;
 	for (i = 0; i < header.table_entries; i++) {
 		memcpy_fromio(&entry, ptr_entry, sizeof(entry));
@@ -138,8 +136,10 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
 		ptr_entry += entry.size;
 	}
 
-	if (ret)
+	if (ret) {
+		bus_unregister(&coreboot_bus_type);
 		iounmap(ptr);
+	}
 
 	return ret;
 }
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 4/7] firmware: coreboot: Collapse platform drivers into bus core
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
                   ` (2 preceding siblings ...)
  2018-08-09 17:17 ` [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric Stephen Boyd
@ 2018-08-09 17:17 ` Stephen Boyd
  2018-08-09 17:17 ` [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap() Stephen Boyd
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

The DT based and ACPI based platform drivers here do the same thing; map
some memory and hand it over to the coreboot bus to populate devices.
The only major difference is that the DT based driver doesn't map the
coreboot table header to figure out how large of a region to map for the
whole coreboot table and it uses of_iomap() instead of ioremap_cache().
A cached or non-cached mapping shouldn't matter here and mapping some
smaller region first before mapping the whole table is just more work
but should be OK. In the end, we can remove two files and combine the
code all in one place making it easier to reason about things.

We leave the old Kconfigs in place for a little while longer but make
them hidden and select the previously hidden config option. This way
users can upgrade without having to know to reselect this config in the
future. Later on we can remove the old hidden configs.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/Kconfig               | 26 +++---
 drivers/firmware/google/Makefile              |  2 -
 drivers/firmware/google/coreboot_table-acpi.c | 88 -------------------
 drivers/firmware/google/coreboot_table-of.c   | 60 -------------
 drivers/firmware/google/coreboot_table.c      | 66 +++++++++++++-
 drivers/firmware/google/coreboot_table.h      |  6 --
 6 files changed, 72 insertions(+), 176 deletions(-)
 delete mode 100644 drivers/firmware/google/coreboot_table-acpi.c
 delete mode 100644 drivers/firmware/google/coreboot_table-of.c

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index a456a000048b..16564cfa47ef 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -19,28 +19,22 @@ config GOOGLE_SMI
 	  variables.
 
 config GOOGLE_COREBOOT_TABLE
-	tristate
-	depends on GOOGLE_COREBOOT_TABLE_ACPI || GOOGLE_COREBOOT_TABLE_OF
-
-config GOOGLE_COREBOOT_TABLE_ACPI
-	tristate "Coreboot Table Access - ACPI"
-	depends on ACPI
-	select GOOGLE_COREBOOT_TABLE
+	tristate "Coreboot Table Access"
+	depends on ACPI || OF
 	help
 	  This option enables the coreboot_table module, which provides other
-	  firmware modules to access to the coreboot table. The coreboot table
-	  pointer is accessed through the ACPI "GOOGCB00" object.
+	  firmware modules access to the coreboot table. The coreboot table
+	  pointer is accessed through the ACPI "GOOGCB00" object or the
+	  device tree node /firmware/coreboot.
 	  If unsure say N.
 
+config GOOGLE_COREBOOT_TABLE_ACPI
+	tristate
+	select GOOGLE_COREBOOT_TABLE
+
 config GOOGLE_COREBOOT_TABLE_OF
-	tristate "Coreboot Table Access - Device Tree"
-	depends on OF
+	tristate
 	select GOOGLE_COREBOOT_TABLE
-	help
-	  This option enable the coreboot_table module, which provide other
-	  firmware modules to access coreboot table. The coreboot table pointer
-	  is accessed through the device tree node /firmware/coreboot.
-	  If unsure say N.
 
 config GOOGLE_MEMCONSOLE
 	tristate
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d0b3fba96194..d17caded5d88 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -2,8 +2,6 @@
 
 obj-$(CONFIG_GOOGLE_SMI)		+= gsmi.o
 obj-$(CONFIG_GOOGLE_COREBOOT_TABLE)        += coreboot_table.o
-obj-$(CONFIG_GOOGLE_COREBOOT_TABLE_ACPI)   += coreboot_table-acpi.o
-obj-$(CONFIG_GOOGLE_COREBOOT_TABLE_OF)     += coreboot_table-of.o
 obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT)  += framebuffer-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
diff --git a/drivers/firmware/google/coreboot_table-acpi.c b/drivers/firmware/google/coreboot_table-acpi.c
deleted file mode 100644
index 77197fe3d42f..000000000000
--- a/drivers/firmware/google/coreboot_table-acpi.c
+++ /dev/null
@@ -1,88 +0,0 @@
-/*
- * coreboot_table-acpi.c
- *
- * Using ACPI to locate Coreboot table and provide coreboot table access.
- *
- * Copyright 2017 Google Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License v2.0 as published by
- * the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <linux/acpi.h>
-#include <linux/device.h>
-#include <linux/err.h>
-#include <linux/init.h>
-#include <linux/io.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-
-#include "coreboot_table.h"
-
-static int coreboot_table_acpi_probe(struct platform_device *pdev)
-{
-	phys_addr_t phyaddr;
-	resource_size_t len;
-	struct coreboot_table_header __iomem *header = NULL;
-	struct resource *res;
-	void __iomem *ptr = NULL;
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res)
-		return -EINVAL;
-
-	len = resource_size(res);
-	if (!res->start || !len)
-		return -EINVAL;
-
-	phyaddr = res->start;
-	header = ioremap_cache(phyaddr, sizeof(*header));
-	if (header == NULL)
-		return -ENOMEM;
-
-	ptr = ioremap_cache(phyaddr,
-			    header->header_bytes + header->table_bytes);
-	iounmap(header);
-	if (!ptr)
-		return -ENOMEM;
-
-	return coreboot_table_init(&pdev->dev, ptr);
-}
-
-static int coreboot_table_acpi_remove(struct platform_device *pdev)
-{
-	return coreboot_table_exit();
-}
-
-static const struct acpi_device_id cros_coreboot_acpi_match[] = {
-	{ "GOOGCB00", 0 },
-	{ "BOOT0000", 0 },
-	{ }
-};
-MODULE_DEVICE_TABLE(acpi, cros_coreboot_acpi_match);
-
-static struct platform_driver coreboot_table_acpi_driver = {
-	.probe = coreboot_table_acpi_probe,
-	.remove = coreboot_table_acpi_remove,
-	.driver = {
-		.name = "coreboot_table_acpi",
-		.acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
-	},
-};
-
-static int __init coreboot_table_acpi_init(void)
-{
-	return platform_driver_register(&coreboot_table_acpi_driver);
-}
-
-module_init(coreboot_table_acpi_init);
-
-MODULE_AUTHOR("Google, Inc.");
-MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
deleted file mode 100644
index 9b90c0fa4a0b..000000000000
--- a/drivers/firmware/google/coreboot_table-of.c
+++ /dev/null
@@ -1,60 +0,0 @@
-/*
- * coreboot_table-of.c
- *
- * Coreboot table access through open firmware.
- *
- * Copyright 2017 Google Inc.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License v2.0 as published by
- * the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#include <linux/device.h>
-#include <linux/io.h>
-#include <linux/module.h>
-#include <linux/of_address.h>
-#include <linux/platform_device.h>
-
-#include "coreboot_table.h"
-
-static int coreboot_table_of_probe(struct platform_device *pdev)
-{
-	struct device_node *fw_dn = pdev->dev.of_node;
-	void __iomem *ptr;
-
-	ptr = of_iomap(fw_dn, 0);
-	if (!ptr)
-		return -ENOMEM;
-
-	return coreboot_table_init(&pdev->dev, ptr);
-}
-
-static int coreboot_table_of_remove(struct platform_device *pdev)
-{
-	return coreboot_table_exit();
-}
-
-static const struct of_device_id coreboot_of_match[] = {
-	{ .compatible = "coreboot" },
-	{}
-};
-MODULE_DEVICE_TABLE(of, coreboot_of_match);
-
-static struct platform_driver coreboot_table_of_driver = {
-	.probe = coreboot_table_of_probe,
-	.remove = coreboot_table_of_remove,
-	.driver = {
-		.name = "coreboot_table_of",
-		.of_match_table = coreboot_of_match,
-	},
-};
-module_platform_driver(coreboot_table_of_driver);
-
-MODULE_AUTHOR("Google, Inc.");
-MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 0e461d776ce6..feb31502f64b 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -16,12 +16,15 @@
  * GNU General Public License for more details.
  */
 
+#include <linux/acpi.h>
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
 #include <linux/slab.h>
 
 #include "coreboot_table.h"
@@ -91,7 +94,7 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
 }
 EXPORT_SYMBOL(coreboot_driver_unregister);
 
-int coreboot_table_init(struct device *dev, void __iomem *ptr)
+static int coreboot_table_init(struct device *dev, void __iomem *ptr)
 {
 	int i, ret;
 	void *ptr_entry;
@@ -143,9 +146,38 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
 
 	return ret;
 }
-EXPORT_SYMBOL(coreboot_table_init);
 
-int coreboot_table_exit(void)
+static int coreboot_table_probe(struct platform_device *pdev)
+{
+	phys_addr_t phyaddr;
+	resource_size_t len;
+	struct coreboot_table_header __iomem *header = NULL;
+	struct resource *res;
+	void __iomem *ptr = NULL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	len = resource_size(res);
+	if (!res->start || !len)
+		return -EINVAL;
+
+	phyaddr = res->start;
+	header = ioremap_cache(phyaddr, sizeof(*header));
+	if (header == NULL)
+		return -ENOMEM;
+
+	ptr = ioremap_cache(phyaddr,
+			    header->header_bytes + header->table_bytes);
+	iounmap(header);
+	if (!ptr)
+		return -ENOMEM;
+
+	return coreboot_table_init(&pdev->dev, ptr);
+}
+
+static int coreboot_table_remove(struct platform_device *pdev)
 {
 	if (ptr_header) {
 		bus_unregister(&coreboot_bus_type);
@@ -155,7 +187,33 @@ int coreboot_table_exit(void)
 
 	return 0;
 }
-EXPORT_SYMBOL(coreboot_table_exit);
 
+#ifdef CONFIG_ACPI
+static const struct acpi_device_id cros_coreboot_acpi_match[] = {
+	{ "GOOGCB00", 0 },
+	{ "BOOT0000", 0 },
+	{ }
+};
+MODULE_DEVICE_TABLE(acpi, cros_coreboot_acpi_match);
+#endif
+
+#ifdef CONFIG_OF
+static const struct of_device_id coreboot_of_match[] = {
+	{ .compatible = "coreboot" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, coreboot_of_match);
+#endif
+
+static struct platform_driver coreboot_table_driver = {
+	.probe = coreboot_table_probe,
+	.remove = coreboot_table_remove,
+	.driver = {
+		.name = "coreboot_table",
+		.acpi_match_table = ACPI_PTR(cros_coreboot_acpi_match),
+		.of_match_table = of_match_ptr(coreboot_of_match),
+	},
+};
+module_platform_driver(coreboot_table_driver);
 MODULE_AUTHOR("Google, Inc.");
 MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 8ad95a94481b..71a9de6b15fa 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -91,10 +91,4 @@ int coreboot_driver_register(struct coreboot_driver *driver);
 /* Unregister a driver that uses the data from a coreboot table. */
 void coreboot_driver_unregister(struct coreboot_driver *driver);
 
-/* Initialize coreboot table module given a pointer to iomem */
-int coreboot_table_init(struct device *dev, void __iomem *ptr);
-
-/* Cleanup coreboot table module */
-int coreboot_table_exit(void);
-
 #endif /* __COREBOOT_TABLE_H */
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
                   ` (3 preceding siblings ...)
  2018-08-09 17:17 ` [PATCH v3 4/7] firmware: coreboot: Collapse platform drivers into bus core Stephen Boyd
@ 2018-08-09 17:17 ` Stephen Boyd
  2018-08-09 18:24   ` Julius Werner
  2018-08-09 17:17 ` [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init() Stephen Boyd
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

This is all system memory, so we shouldn't be mapping this all with
ioremap() as these aren't I/O regions. Instead, they're memory regions
so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
RAM directly if that's possible, otherwise it falls back to
ioremap_cache() like is being done here already. This also nicely
silences the sparse warnings in this code and reduces the need to copy
anything around anymore.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table.c | 42 +++++++++++-------------
 1 file changed, 20 insertions(+), 22 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index feb31502f64b..f343dbe86448 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -32,7 +32,7 @@
 #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
 #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
 
-static struct coreboot_table_header __iomem *ptr_header;
+static struct coreboot_table_header *ptr_header;
 
 static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
 {
@@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
 }
 EXPORT_SYMBOL(coreboot_driver_unregister);
 
-static int coreboot_table_init(struct device *dev, void __iomem *ptr)
+static int coreboot_table_init(struct device *dev, void *ptr)
 {
 	int i, ret;
 	void *ptr_entry;
 	struct coreboot_device *device;
-	struct coreboot_table_entry entry;
-	struct coreboot_table_header header;
+	struct coreboot_table_entry *entry;
+	struct coreboot_table_header *header;
 
 	ptr_header = ptr;
-	memcpy_fromio(&header, ptr_header, sizeof(header));
+	header = ptr;
 
-	if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
+	if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
 		pr_warn("coreboot_table: coreboot table missing or corrupt!\n");
 		return -ENODEV;
 	}
@@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
 	if (ret)
 		return ret;
 
-	ptr_entry = (void *)ptr_header + header.header_bytes;
-	for (i = 0; i < header.table_entries; i++) {
-		memcpy_fromio(&entry, ptr_entry, sizeof(entry));
+	ptr_entry = ptr_header + header->header_bytes;
+	for (i = 0; i < header->table_entries; i++) {
+		entry = ptr_entry;
 
-		device = kzalloc(sizeof(struct device) + entry.size, GFP_KERNEL);
+		device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL);
 		if (!device) {
 			ret = -ENOMEM;
 			break;
@@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
 		device->dev.parent = dev;
 		device->dev.bus = &coreboot_bus_type;
 		device->dev.release = coreboot_device_release;
-		memcpy_fromio(&device->entry, ptr_entry, entry.size);
+		memcpy(&device->entry, ptr_entry, entry->size);
 
 		ret = device_register(&device->dev);
 		if (ret) {
@@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
 			break;
 		}
 
-		ptr_entry += entry.size;
+		ptr_entry += entry->size;
 	}
 
 	if (ret) {
 		bus_unregister(&coreboot_bus_type);
-		iounmap(ptr);
+		memunmap(ptr);
 	}
 
 	return ret;
@@ -149,11 +149,10 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
 
 static int coreboot_table_probe(struct platform_device *pdev)
 {
-	phys_addr_t phyaddr;
 	resource_size_t len;
-	struct coreboot_table_header __iomem *header = NULL;
+	struct coreboot_table_header *header;
 	struct resource *res;
-	void __iomem *ptr = NULL;
+	void *ptr;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
@@ -163,14 +162,13 @@ static int coreboot_table_probe(struct platform_device *pdev)
 	if (!res->start || !len)
 		return -EINVAL;
 
-	phyaddr = res->start;
-	header = ioremap_cache(phyaddr, sizeof(*header));
+	header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
 	if (header == NULL)
 		return -ENOMEM;
 
-	ptr = ioremap_cache(phyaddr,
-			    header->header_bytes + header->table_bytes);
-	iounmap(header);
+	ptr = memremap(res->start, header->header_bytes + header->table_bytes,
+		       MEMREMAP_WB);
+	memunmap(header);
 	if (!ptr)
 		return -ENOMEM;
 
@@ -181,7 +179,7 @@ static int coreboot_table_remove(struct platform_device *pdev)
 {
 	if (ptr_header) {
 		bus_unregister(&coreboot_bus_type);
-		iounmap(ptr_header);
+		memunmap(ptr_header);
 		ptr_header = NULL;
 	}
 
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init()
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
                   ` (4 preceding siblings ...)
  2018-08-09 17:17 ` [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap() Stephen Boyd
@ 2018-08-09 17:17 ` Stephen Boyd
  2018-08-09 21:02   ` Julius Werner
  2018-08-09 17:17 ` [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access Stephen Boyd
  2018-08-09 18:03 ` [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Brian Norris
  7 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

This function checks the header for sanity, registers a bus, and
populates devices for each coreboot table entry. Let's just populate
devices here and pull the other bits up into the caller so that this
function can be repurposed for pure device creation and registration. We
can devm()ify the memory mapping at the same time to keep error paths
simpler.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Suggested-by: Julius Werner <jwerner@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table.c | 66 +++++++++++-------------
 1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index f343dbe86448..814913606d22 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -32,8 +32,6 @@
 #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
 #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
 
-static struct coreboot_table_header *ptr_header;
-
 static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
 {
 	struct coreboot_device *device = CB_DEV(dev);
@@ -94,35 +92,21 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
 }
 EXPORT_SYMBOL(coreboot_driver_unregister);
 
-static int coreboot_table_init(struct device *dev, void *ptr)
+static int coreboot_table_populate(struct device *dev, void *ptr)
 {
 	int i, ret;
 	void *ptr_entry;
 	struct coreboot_device *device;
 	struct coreboot_table_entry *entry;
-	struct coreboot_table_header *header;
-
-	ptr_header = ptr;
-	header = ptr;
-
-	if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
-		pr_warn("coreboot_table: coreboot table missing or corrupt!\n");
-		return -ENODEV;
-	}
-
-	ret = bus_register(&coreboot_bus_type);
-	if (ret)
-		return ret;
+	struct coreboot_table_header *header = ptr;
 
-	ptr_entry = ptr_header + header->header_bytes;
+	ptr_entry = ptr + header->header_bytes;
 	for (i = 0; i < header->table_entries; i++) {
 		entry = ptr_entry;
 
 		device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL);
-		if (!device) {
-			ret = -ENOMEM;
-			break;
-		}
+		if (!device)
+			return -ENOMEM;
 
 		dev_set_name(&device->dev, "coreboot%d", i);
 		device->dev.parent = dev;
@@ -133,18 +117,13 @@ static int coreboot_table_init(struct device *dev, void *ptr)
 		ret = device_register(&device->dev);
 		if (ret) {
 			put_device(&device->dev);
-			break;
+			return ret;
 		}
 
 		ptr_entry += entry->size;
 	}
 
-	if (ret) {
-		bus_unregister(&coreboot_bus_type);
-		memunmap(ptr);
-	}
-
-	return ret;
+	return 0;
 }
 
 static int coreboot_table_probe(struct platform_device *pdev)
@@ -152,7 +131,9 @@ static int coreboot_table_probe(struct platform_device *pdev)
 	resource_size_t len;
 	struct coreboot_table_header *header;
 	struct resource *res;
+	struct device *dev = &pdev->dev;
 	void *ptr;
+	int ret;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res)
@@ -162,26 +143,37 @@ static int coreboot_table_probe(struct platform_device *pdev)
 	if (!res->start || !len)
 		return -EINVAL;
 
+	/* Map and check just the header first to make sure things are sane */
 	header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
-	if (header == NULL)
+	if (!header)
 		return -ENOMEM;
 
-	ptr = memremap(res->start, header->header_bytes + header->table_bytes,
-		       MEMREMAP_WB);
+	if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
+		dev_warn(dev, "coreboot table missing or corrupt!\n");
+		return -ENODEV;
+	}
+
+	ptr = devm_memremap(dev, res->start,
+			    header->header_bytes + header->table_bytes,
+			    MEMREMAP_WB);
 	memunmap(header);
 	if (!ptr)
 		return -ENOMEM;
 
-	return coreboot_table_init(&pdev->dev, ptr);
+	ret = bus_register(&coreboot_bus_type);
+	if (ret)
+		return ret;
+
+	ret = coreboot_table_populate(dev, ptr);
+	if (ret)
+		bus_unregister(&coreboot_bus_type);
+
+	return ret;
 }
 
 static int coreboot_table_remove(struct platform_device *pdev)
 {
-	if (ptr_header) {
-		bus_unregister(&coreboot_bus_type);
-		memunmap(ptr_header);
-		ptr_header = NULL;
-	}
+	bus_unregister(&coreboot_bus_type);
 
 	return 0;
 }
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
                   ` (5 preceding siblings ...)
  2018-08-09 17:17 ` [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init() Stephen Boyd
@ 2018-08-09 17:17 ` Stephen Boyd
  2018-08-09 21:07   ` Julius Werner
  2018-08-09 18:03 ` [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Brian Norris
  7 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 17:17 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: linux-kernel, Wei-Ning Huang, Julius Werner, Brian Norris,
	Samuel Holland

Call request_mem_region() on the entire coreboot table to make sure
other devices don't attempt to map the coreboot table in their drivers.
If drivers need that support, it would be better to provide bus APIs
they can use to do that through the mapping created in this file.

Cc: Wei-Ning Huang <wnhuang@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Cc: Brian Norris <briannorris@chromium.org>
Cc: Samuel Holland <samuel@sholland.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index 814913606d22..94c41d814ea5 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -132,6 +132,7 @@ static int coreboot_table_probe(struct platform_device *pdev)
 	struct coreboot_table_header *header;
 	struct resource *res;
 	struct device *dev = &pdev->dev;
+	const char *name;
 	void *ptr;
 	int ret;
 
@@ -153,10 +154,17 @@ static int coreboot_table_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	ptr = devm_memremap(dev, res->start,
-			    header->header_bytes + header->table_bytes,
-			    MEMREMAP_WB);
+	len = header->header_bytes + header->table_bytes;
+	res->end = res->start + len - 1;
+	name = res->name ?: dev_name(dev);
 	memunmap(header);
+
+	if (!devm_request_mem_region(dev, res->start, len, name)) {
+		dev_err(dev, "can't request region for resource %pR\n", res);
+		return -EBUSY;
+	}
+
+	ptr = devm_memremap(dev, res->start, len, MEMREMAP_WB);
 	if (!ptr)
 		return -ENOMEM;
 
-- 
Sent by a computer through tubes


^ permalink raw reply related	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 1/7] firmware: coreboot: Let OF core populate platform device
  2018-08-09 17:17 ` [PATCH v3 1/7] firmware: coreboot: Let OF core populate platform device Stephen Boyd
@ 2018-08-09 17:31   ` Brian Norris
  0 siblings, 0 replies; 26+ messages in thread
From: Brian Norris @ 2018-08-09 17:31 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland

Hi,

On Thu, Aug 09, 2018 at 10:17:16AM -0700, Stephen Boyd wrote:
> Now that the /firmware/coreboot node in DT is populated by the core DT
> platform code with commit 3aa0582fdb82 ("of: platform: populate
> /firmware/ node from of_platform_default_populate_init()") we should and
> can remove the platform device creation here. Otherwise, the
> of_platform_device_create() call will fail, the coreboot of driver won't
> be registered, and this driver will never bind. At the same time, we
> should move this driver to use MODULE_DEVICE_TABLE so that module
> auto-load works properly when the coreboot device is auto-populated and
> we should drop the of_node handling that was presumably placed here to
> hold a reference to the DT node created during module init that no
> longer happens.

Yep, the of_node_put() was to complement of_find_compatible_node() which
was originally done in probe(), and then of_find_node_by_path() in
__init when it was upstreamed, and now it's no longer needed.

> 
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Samuel Holland <samuel@sholland.org>
> Reviewed-by: Sudeep Holla <Sudeep.Holla@arm.com>
> Fixes: 3aa0582fdb82 ("of: platform: populate /firmware/ node from of_platform_default_populate_init()")
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table-of.c | 28 +++------------------
>  1 file changed, 3 insertions(+), 25 deletions(-)

Change looks good:

Reviewed-by: Brian Norris <briannorris@chromium.org>

Thanks!

Now to go figure out how a simple bugfix managed to blow up into a
7-patch series... If this goes on any longer, patch 1 should probably go
by itself, to fix the regression.

Brian

> diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
> index f15bf404c579..9b90c0fa4a0b 100644
> --- a/drivers/firmware/google/coreboot_table-of.c
> +++ b/drivers/firmware/google/coreboot_table-of.c
> @@ -19,7 +19,6 @@
>  #include <linux/io.h>
>  #include <linux/module.h>
>  #include <linux/of_address.h>
> -#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  
>  #include "coreboot_table.h"
> @@ -30,7 +29,6 @@ static int coreboot_table_of_probe(struct platform_device *pdev)
>  	void __iomem *ptr;
>  
>  	ptr = of_iomap(fw_dn, 0);
> -	of_node_put(fw_dn);
>  	if (!ptr)
>  		return -ENOMEM;
>  
> @@ -44,8 +42,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
>  
>  static const struct of_device_id coreboot_of_match[] = {
>  	{ .compatible = "coreboot" },
> -	{},
> +	{}
>  };
> +MODULE_DEVICE_TABLE(of, coreboot_of_match);
>  
>  static struct platform_driver coreboot_table_of_driver = {
>  	.probe = coreboot_table_of_probe,
> @@ -55,28 +54,7 @@ static struct platform_driver coreboot_table_of_driver = {
>  		.of_match_table = coreboot_of_match,
>  	},
>  };
> -
> -static int __init platform_coreboot_table_of_init(void)
> -{
> -	struct platform_device *pdev;
> -	struct device_node *of_node;
> -
> -	/* Limit device creation to the presence of /firmware/coreboot node */
> -	of_node = of_find_node_by_path("/firmware/coreboot");
> -	if (!of_node)
> -		return -ENODEV;
> -
> -	if (!of_match_node(coreboot_of_match, of_node))
> -		return -ENODEV;
> -
> -	pdev = of_platform_device_create(of_node, "coreboot_table_of", NULL);
> -	if (!pdev)
> -		return -ENODEV;
> -
> -	return platform_driver_register(&coreboot_table_of_driver);
> -}
> -
> -module_init(platform_coreboot_table_of_init);
> +module_platform_driver(coreboot_table_of_driver);
>  
>  MODULE_AUTHOR("Google, Inc.");
>  MODULE_LICENSE("GPL");
> -- 
> Sent by a computer through tubes
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure
  2018-08-09 17:17 ` [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure Stephen Boyd
@ 2018-08-09 17:49   ` Brian Norris
  2018-08-09 19:40     ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Brian Norris @ 2018-08-09 17:49 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland

On Thu, Aug 09, 2018 at 10:17:17AM -0700, Stephen Boyd wrote:
> Both callers of coreboot_table_init() ioremap the pointer that comes in
> but they don't unmap the memory on failure. Both of them also fail probe
> immediately with the return value of coreboot_table_init(), leaking a
> mapping when it fails. Plug the leak so the mapping isn't left unused.
> 
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Samuel Holland <samuel@sholland.org>
> Fixes: 570d30c2823f ("firmware: coreboot: Expose the coreboot table as a bus")

I suppose this is fair, since that commit introduced error paths and
didn't clean them up. But one warning below:

> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index 19db5709ae28..0d3e140444ae 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -138,6 +138,9 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
>  		ptr_entry += entry.size;
>  	}
>  
> +	if (ret)
> +		iounmap(ptr);

This works because no sub-driver is using this mapping any more (i.e.,
because we killed coreboot_table_find()). Otherwise, we'd need to
explicitly kill all the sub-devices first. IOW, if this gets backported
to older kernels, it would need to go along with this and its other
dependencies:

b616cf53aa7a firmware: coreboot: Remove unused coreboot_table_find

But I guess that's a question for -stable. Or, we remove the 'Fixes'
tag? Or add another tag, to list other dependencies? Or just ignore it.

But for this change as applied to mainline:

Reviewed-by: Brian Norris <briannorris@chromium.org>

> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(coreboot_table_init);
> -- 
> Sent by a computer through tubes
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code
  2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
                   ` (6 preceding siblings ...)
  2018-08-09 17:17 ` [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access Stephen Boyd
@ 2018-08-09 18:03 ` Brian Norris
  7 siblings, 0 replies; 26+ messages in thread
From: Brian Norris @ 2018-08-09 18:03 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland

Hi Stepehen,

On Thu, Aug 09, 2018 at 10:17:15AM -0700, Stephen Boyd wrote:
> This series reworks the coreboot firmware driver a bit to fix some bugs
> and then simplify the code by changing the design to get rid of the
> different platform drivers, remap memory with memremap(), and use devm
> to simplify error paths. There's some risk in changing to memremap() but
> I think that should work out alright. It will either return memory
> directly, or fallback to ioremap_cache() as has been done before.
> 
> Changes from v2:
>  * A bunch more patches
>  * Fix iounmap missing on error path
>  * Fix bus getting unregistered on driver unbind and never registered
>    again
>  * Request exclusive access to table region
>  * Pull out table signature check to driver probe
> 
> Changes from v1:
>  * Split out fixlet for DT based driver from platform driver change
> 
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Samuel Holland <samuel@sholland.org>
> 
> Stephen Boyd (7):
>   firmware: coreboot: Let OF core populate platform device
>   firmware: coreboot: Unmap ioregion on failure
>   firmware: coreboot: Make bus registration symmetric
>   firmware: coreboot: Collapse platform drivers into bus core
>   firmware: coreboot: Remap RAM with memremap() instead of ioremap()
>   firmware: coreboot: Only populate devices in coreboot_table_init()
>   firmware: coreboot: Request table region for exclusive access

I gave patches 1 and 2 closer review (and individual replies); the
remaining patches look good to me too, so for the whole series:

Reviewed-by: Brian Norris <briannorris@chromium.org>

>  drivers/firmware/google/Kconfig               |  26 ++--
>  drivers/firmware/google/Makefile              |   2 -
>  drivers/firmware/google/coreboot_table-acpi.c |  88 ------------
>  drivers/firmware/google/coreboot_table-of.c   |  82 -----------
>  drivers/firmware/google/coreboot_table.c      | 135 +++++++++++++-----
>  drivers/firmware/google/coreboot_table.h      |   6 -
>  6 files changed, 107 insertions(+), 232 deletions(-)
>  delete mode 100644 drivers/firmware/google/coreboot_table-acpi.c
>  delete mode 100644 drivers/firmware/google/coreboot_table-of.c
> 
> -- 
> Sent by a computer through tubes
> 

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric
  2018-08-09 17:17 ` [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric Stephen Boyd
@ 2018-08-09 18:10   ` Julius Werner
  2018-08-09 23:30     ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Julius Werner @ 2018-08-09 18:10 UTC (permalink / raw)
  To: swboyd
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

> @@ -138,8 +136,10 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
>                 ptr_entry += entry.size;
>         }
>
> -       if (ret)
> +       if (ret) {
> +               bus_unregister(&coreboot_bus_type);
>                 iounmap(ptr);
> +       }

nit: maybe cleaner to just do if (ret) coreboot_table_exit(); here?
You're essentially writing the same code again.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
  2018-08-09 17:17 ` [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap() Stephen Boyd
@ 2018-08-09 18:24   ` Julius Werner
  2018-08-09 22:07     ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Julius Werner @ 2018-08-09 18:24 UTC (permalink / raw)
  To: swboyd, Aaron Durbin
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

One thing to note is that we still want this space to be mappable by
userspace applications via /dev/mem, so we need to make sure that
there's no weird memory type mismatch that causes problems with that.
Adding Aaron to see if he has any concerns here, since I think he's
seen something like that in the past (not sure if it was related to
what this kernel driver does).

Can you please test this on an x86 Chromebook and run the 'cbmem'
userspace utility, make sure it doesn't fail after this?

Also, stupid question after taking a step back and looking at this
again: why do we keep a mapping alive for the lifetime of the driver
at all? It used to be necessary when this driver was
find-entry-on-demand, but nowadays it just goes through all entries
once at probe time and immediately memcpy_fromio()s out all the
relevant information into (struct coreboot_device)s. After that we're
done accessing the "real" coreboot table, forever. Why not just unmap
it again at the end of coreboot_table_init()?
On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> This is all system memory, so we shouldn't be mapping this all with
> ioremap() as these aren't I/O regions. Instead, they're memory regions
> so we should use memremap(). Pick MEMREMAP_WB so we can map memory from
> RAM directly if that's possible, otherwise it falls back to
> ioremap_cache() like is being done here already. This also nicely
> silences the sparse warnings in this code and reduces the need to copy
> anything around anymore.
>
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Samuel Holland <samuel@sholland.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table.c | 42 +++++++++++-------------
>  1 file changed, 20 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index feb31502f64b..f343dbe86448 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,7 +32,7 @@
>  #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
>  #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header __iomem *ptr_header;
> +static struct coreboot_table_header *ptr_header;
>
>  static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
>  {
> @@ -94,18 +94,18 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void *ptr)
>  {
>         int i, ret;
>         void *ptr_entry;
>         struct coreboot_device *device;
> -       struct coreboot_table_entry entry;
> -       struct coreboot_table_header header;
> +       struct coreboot_table_entry *entry;
> +       struct coreboot_table_header *header;
>
>         ptr_header = ptr;
> -       memcpy_fromio(&header, ptr_header, sizeof(header));
> +       header = ptr;
>
> -       if (strncmp(header.signature, "LBIO", sizeof(header.signature))) {
> +       if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
>                 pr_warn("coreboot_table: coreboot table missing or corrupt!\n");
>                 return -ENODEV;
>         }
> @@ -114,11 +114,11 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
>         if (ret)
>                 return ret;
>
> -       ptr_entry = (void *)ptr_header + header.header_bytes;
> -       for (i = 0; i < header.table_entries; i++) {
> -               memcpy_fromio(&entry, ptr_entry, sizeof(entry));
> +       ptr_entry = ptr_header + header->header_bytes;
> +       for (i = 0; i < header->table_entries; i++) {
> +               entry = ptr_entry;
>
> -               device = kzalloc(sizeof(struct device) + entry.size, GFP_KERNEL);
> +               device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL);
>                 if (!device) {
>                         ret = -ENOMEM;
>                         break;
> @@ -128,7 +128,7 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
>                 device->dev.parent = dev;
>                 device->dev.bus = &coreboot_bus_type;
>                 device->dev.release = coreboot_device_release;
> -               memcpy_fromio(&device->entry, ptr_entry, entry.size);
> +               memcpy(&device->entry, ptr_entry, entry->size);
>
>                 ret = device_register(&device->dev);
>                 if (ret) {
> @@ -136,12 +136,12 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
>                         break;
>                 }
>
> -               ptr_entry += entry.size;
> +               ptr_entry += entry->size;
>         }
>
>         if (ret) {
>                 bus_unregister(&coreboot_bus_type);
> -               iounmap(ptr);
> +               memunmap(ptr);
>         }
>
>         return ret;
> @@ -149,11 +149,10 @@ static int coreboot_table_init(struct device *dev, void __iomem *ptr)
>
>  static int coreboot_table_probe(struct platform_device *pdev)
>  {
> -       phys_addr_t phyaddr;
>         resource_size_t len;
> -       struct coreboot_table_header __iomem *header = NULL;
> +       struct coreboot_table_header *header;
>         struct resource *res;
> -       void __iomem *ptr = NULL;
> +       void *ptr;
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         if (!res)
> @@ -163,14 +162,13 @@ static int coreboot_table_probe(struct platform_device *pdev)
>         if (!res->start || !len)
>                 return -EINVAL;
>
> -       phyaddr = res->start;
> -       header = ioremap_cache(phyaddr, sizeof(*header));
> +       header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
>         if (header == NULL)
>                 return -ENOMEM;
>
> -       ptr = ioremap_cache(phyaddr,
> -                           header->header_bytes + header->table_bytes);
> -       iounmap(header);
> +       ptr = memremap(res->start, header->header_bytes + header->table_bytes,
> +                      MEMREMAP_WB);
> +       memunmap(header);
>         if (!ptr)
>                 return -ENOMEM;
>
> @@ -181,7 +179,7 @@ static int coreboot_table_remove(struct platform_device *pdev)
>  {
>         if (ptr_header) {
>                 bus_unregister(&coreboot_bus_type);
> -               iounmap(ptr_header);
> +               memunmap(ptr_header);
>                 ptr_header = NULL;
>         }
>
> --
> Sent by a computer through tubes
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure
  2018-08-09 17:49   ` Brian Norris
@ 2018-08-09 19:40     ` Stephen Boyd
  2018-08-09 19:52       ` Brian Norris
  0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 19:40 UTC (permalink / raw)
  To: Brian Norris
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland

Quoting Brian Norris (2018-08-09 10:49:38)
> On Thu, Aug 09, 2018 at 10:17:17AM -0700, Stephen Boyd wrote:
> > Both callers of coreboot_table_init() ioremap the pointer that comes in
> > but they don't unmap the memory on failure. Both of them also fail probe
> > immediately with the return value of coreboot_table_init(), leaking a
> > mapping when it fails. Plug the leak so the mapping isn't left unused.
> > 
> > Cc: Wei-Ning Huang <wnhuang@chromium.org>
> > Cc: Julius Werner <jwerner@chromium.org>
> > Cc: Brian Norris <briannorris@chromium.org>
> > Cc: Samuel Holland <samuel@sholland.org>
> > Fixes: 570d30c2823f ("firmware: coreboot: Expose the coreboot table as a bus")
> 
> I suppose this is fair, since that commit introduced error paths and
> didn't clean them up. But one warning below:
> 
> > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > ---
> >  drivers/firmware/google/coreboot_table.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> > index 19db5709ae28..0d3e140444ae 100644
> > --- a/drivers/firmware/google/coreboot_table.c
> > +++ b/drivers/firmware/google/coreboot_table.c
> > @@ -138,6 +138,9 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
> >               ptr_entry += entry.size;
> >       }
> >  
> > +     if (ret)
> > +             iounmap(ptr);
> 
> This works because no sub-driver is using this mapping any more (i.e.,
> because we killed coreboot_table_find()). Otherwise, we'd need to
> explicitly kill all the sub-devices first. IOW, if this gets backported
> to older kernels, it would need to go along with this and its other
> dependencies:

The memory is copied out of the table. So do the devices actually use
the memory that we remap here? I don't see how it's a problem if we
unmap the table after we populate devices.

> 
> b616cf53aa7a firmware: coreboot: Remove unused coreboot_table_find
> 
> But I guess that's a question for -stable. Or, we remove the 'Fixes'
> tag? Or add another tag, to list other dependencies? Or just ignore it.
> 
> But for this change as applied to mainline:
> 
> Reviewed-by: Brian Norris <briannorris@chromium.org>
> 

Thanks!


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure
  2018-08-09 19:40     ` Stephen Boyd
@ 2018-08-09 19:52       ` Brian Norris
  2018-08-09 23:25         ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Brian Norris @ 2018-08-09 19:52 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland

Hi,

On Thu, Aug 09, 2018 at 12:40:31PM -0700, Stephen Boyd wrote:
> Quoting Brian Norris (2018-08-09 10:49:38)
> > On Thu, Aug 09, 2018 at 10:17:17AM -0700, Stephen Boyd wrote:
> > > Both callers of coreboot_table_init() ioremap the pointer that comes in
> > > but they don't unmap the memory on failure. Both of them also fail probe
> > > immediately with the return value of coreboot_table_init(), leaking a
> > > mapping when it fails. Plug the leak so the mapping isn't left unused.
> > > 
> > > Cc: Wei-Ning Huang <wnhuang@chromium.org>
> > > Cc: Julius Werner <jwerner@chromium.org>
> > > Cc: Brian Norris <briannorris@chromium.org>
> > > Cc: Samuel Holland <samuel@sholland.org>
> > > Fixes: 570d30c2823f ("firmware: coreboot: Expose the coreboot table as a bus")
> > 
> > I suppose this is fair, since that commit introduced error paths and
> > didn't clean them up. But one warning below:
> > 
> > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > ---
> > >  drivers/firmware/google/coreboot_table.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> > > index 19db5709ae28..0d3e140444ae 100644
> > > --- a/drivers/firmware/google/coreboot_table.c
> > > +++ b/drivers/firmware/google/coreboot_table.c
> > > @@ -138,6 +138,9 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
> > >               ptr_entry += entry.size;
> > >       }
> > >  
> > > +     if (ret)
> > > +             iounmap(ptr);
> > 
> > This works because no sub-driver is using this mapping any more (i.e.,
> > because we killed coreboot_table_find()). Otherwise, we'd need to
> > explicitly kill all the sub-devices first. IOW, if this gets backported
> > to older kernels, it would need to go along with this and its other
> > dependencies:
> 
> The memory is copied out of the table. So do the devices actually use
> the memory that we remap here? I don't see how it's a problem if we
> unmap the table after we populate devices.

No, the memory is (or was) copied each time. See:

int coreboot_table_find(int tag, void *data, size_t data_size)
{
...
	memcpy_fromio(&header, ptr_header, sizeof(header));
...

(where ptr_header is an alias for 'ptr')

So before commit b616cf53aa7a and friends, this patch is a bad idea.

Just to reiterate/clarify: none of this is a criticism of this patch as
applied to mainline. It's just a criticism of what might happen with the
'Fixes' tag if we aren't careful.

Brian


> > b616cf53aa7a firmware: coreboot: Remove unused coreboot_table_find

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init()
  2018-08-09 17:17 ` [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init() Stephen Boyd
@ 2018-08-09 21:02   ` Julius Werner
  2018-08-09 23:43     ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Julius Werner @ 2018-08-09 21:02 UTC (permalink / raw)
  To: swboyd
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> This function checks the header for sanity, registers a bus, and
> populates devices for each coreboot table entry. Let's just populate
> devices here and pull the other bits up into the caller so that this
> function can be repurposed for pure device creation and registration. We
> can devm()ify the memory mapping at the same time to keep error paths
> simpler.
>
> Cc: Wei-Ning Huang <wnhuang@chromium.org>
> Cc: Julius Werner <jwerner@chromium.org>
> Cc: Brian Norris <briannorris@chromium.org>
> Cc: Samuel Holland <samuel@sholland.org>
> Suggested-by: Julius Werner <jwerner@chromium.org>
> Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> ---
>  drivers/firmware/google/coreboot_table.c | 66 +++++++++++-------------
>  1 file changed, 29 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> index f343dbe86448..814913606d22 100644
> --- a/drivers/firmware/google/coreboot_table.c
> +++ b/drivers/firmware/google/coreboot_table.c
> @@ -32,8 +32,6 @@
>  #define CB_DEV(d) container_of(d, struct coreboot_device, dev)
>  #define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
>
> -static struct coreboot_table_header *ptr_header;
> -
>  static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
>  {
>         struct coreboot_device *device = CB_DEV(dev);
> @@ -94,35 +92,21 @@ void coreboot_driver_unregister(struct coreboot_driver *driver)
>  }
>  EXPORT_SYMBOL(coreboot_driver_unregister);
>
> -static int coreboot_table_init(struct device *dev, void *ptr)
> +static int coreboot_table_populate(struct device *dev, void *ptr)
>  {
>         int i, ret;
>         void *ptr_entry;
>         struct coreboot_device *device;
>         struct coreboot_table_entry *entry;
> -       struct coreboot_table_header *header;
> -
> -       ptr_header = ptr;
> -       header = ptr;
> -
> -       if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> -               pr_warn("coreboot_table: coreboot table missing or corrupt!\n");
> -               return -ENODEV;
> -       }
> -
> -       ret = bus_register(&coreboot_bus_type);
> -       if (ret)
> -               return ret;
> +       struct coreboot_table_header *header = ptr;
>
> -       ptr_entry = ptr_header + header->header_bytes;
> +       ptr_entry = ptr + header->header_bytes;
>         for (i = 0; i < header->table_entries; i++) {
>                 entry = ptr_entry;
>
>                 device = kzalloc(sizeof(struct device) + entry->size, GFP_KERNEL);
> -               if (!device) {
> -                       ret = -ENOMEM;
> -                       break;
> -               }
> +               if (!device)
> +                       return -ENOMEM;
>
>                 dev_set_name(&device->dev, "coreboot%d", i);
>                 device->dev.parent = dev;
> @@ -133,18 +117,13 @@ static int coreboot_table_init(struct device *dev, void *ptr)
>                 ret = device_register(&device->dev);
>                 if (ret) {
>                         put_device(&device->dev);
> -                       break;
> +                       return ret;
>                 }
>
>                 ptr_entry += entry->size;
>         }
>
> -       if (ret) {
> -               bus_unregister(&coreboot_bus_type);
> -               memunmap(ptr);
> -       }
> -
> -       return ret;
> +       return 0;
>  }
>
>  static int coreboot_table_probe(struct platform_device *pdev)
> @@ -152,7 +131,9 @@ static int coreboot_table_probe(struct platform_device *pdev)
>         resource_size_t len;
>         struct coreboot_table_header *header;
>         struct resource *res;
> +       struct device *dev = &pdev->dev;
>         void *ptr;
> +       int ret;
>
>         res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>         if (!res)
> @@ -162,26 +143,37 @@ static int coreboot_table_probe(struct platform_device *pdev)
>         if (!res->start || !len)
>                 return -EINVAL;
>
> +       /* Map and check just the header first to make sure things are sane */
>         header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
> -       if (header == NULL)
> +       if (!header)
>                 return -ENOMEM;
>
> -       ptr = memremap(res->start, header->header_bytes + header->table_bytes,
> -                      MEMREMAP_WB);
> +       if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> +               dev_warn(dev, "coreboot table missing or corrupt!\n");
> +               return -ENODEV;

Leaking the mapping here.

> +       }
> +
> +       ptr = devm_memremap(dev, res->start,
> +                           header->header_bytes + header->table_bytes,
> +                           MEMREMAP_WB);
>         memunmap(header);
>         if (!ptr)
>                 return -ENOMEM;
>
> -       return coreboot_table_init(&pdev->dev, ptr);
> +       ret = bus_register(&coreboot_bus_type);
> +       if (ret)
> +               return ret;
> +
> +       ret = coreboot_table_populate(dev, ptr);
> +       if (ret)
> +               bus_unregister(&coreboot_bus_type);
> +
> +       return ret;
>  }
>
>  static int coreboot_table_remove(struct platform_device *pdev)
>  {
> -       if (ptr_header) {
> -               bus_unregister(&coreboot_bus_type);
> -               memunmap(ptr_header);
> -               ptr_header = NULL;
> -       }
> +       bus_unregister(&coreboot_bus_type);
>
>         return 0;
>  }
> --
> Sent by a computer through tubes
>

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access
  2018-08-09 17:17 ` [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access Stephen Boyd
@ 2018-08-09 21:07   ` Julius Werner
  2018-08-09 23:03     ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Julius Werner @ 2018-08-09 21:07 UTC (permalink / raw)
  To: swboyd
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
>
> Call request_mem_region() on the entire coreboot table to make sure
> other devices don't attempt to map the coreboot table in their drivers.
> If drivers need that support, it would be better to provide bus APIs
> they can use to do that through the mapping created in this file.
>

Does this prevent userspace from mapping this region via /dev/mem? If
so, let's please not do it to not break compatibility with existing
tools. (I guess an alternative would be to rewrite 'cbmem' to use
/sys/bus/coreboot/devices if available to get its coreboot table
information. But we'd still need to maintain the old path for
backwards compatibility anyway, so that would really just make it more
complicated.)

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
  2018-08-09 18:24   ` Julius Werner
@ 2018-08-09 22:07     ` Stephen Boyd
  0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 22:07 UTC (permalink / raw)
  To: Aaron Durbin, Julius Werner
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

Quoting Julius Werner (2018-08-09 11:24:29)
> One thing to note is that we still want this space to be mappable by
> userspace applications via /dev/mem, so we need to make sure that
> there's no weird memory type mismatch that causes problems with that.
> Adding Aaron to see if he has any concerns here, since I think he's
> seen something like that in the past (not sure if it was related to
> what this kernel driver does).
> 
> Can you please test this on an x86 Chromebook and run the 'cbmem'
> userspace utility, make sure it doesn't fail after this?

Sure!

> 
> Also, stupid question after taking a step back and looking at this
> again: why do we keep a mapping alive for the lifetime of the driver
> at all? It used to be necessary when this driver was
> find-entry-on-demand, but nowadays it just goes through all entries
> once at probe time and immediately memcpy_fromio()s out all the
> relevant information into (struct coreboot_device)s. After that we're
> done accessing the "real" coreboot table, forever. Why not just unmap
> it again at the end of coreboot_table_init()?

Yes, we just copy everything over so it makes it simpler to just unmap
at the end of coreboot_table_init(). Then userspace is free to make a
questionable mapping into system RAM to find the table itself.

I'll make this change.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access
  2018-08-09 21:07   ` Julius Werner
@ 2018-08-09 23:03     ` Stephen Boyd
  2018-08-09 23:37       ` Julius Werner
  0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 23:03 UTC (permalink / raw)
  To: Julius Werner
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

Quoting Julius Werner (2018-08-09 14:07:31)
> On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > Call request_mem_region() on the entire coreboot table to make sure
> > other devices don't attempt to map the coreboot table in their drivers.
> > If drivers need that support, it would be better to provide bus APIs
> > they can use to do that through the mapping created in this file.
> >
> 
> Does this prevent userspace from mapping this region via /dev/mem? If
> so, let's please not do it to not break compatibility with existing
> tools.

No it doesn't break. I can still read the memory here with /dev/mem (and
cbmem). But that seems to be because of a couple reasons. First, I have
CONFIG_STRICT_DEVMEM=y but CONFIG_IO_STRICT_DEVMEM=n. This allows me to
map memory that isn't system ram. If CONFIG_IO_STRICT_DEVMEM=y then
mapping this I/O region through /dev/mem here would be denied .
Furthermore, I see that my system RAM excludes this coreboot table so it
doesn't fall into the bucket that CONFIG_STRICT_DEVMEM would find.

> (I guess an alternative would be to rewrite 'cbmem' to use
> /sys/bus/coreboot/devices if available to get its coreboot table
> information. But we'd still need to maintain the old path for
> backwards compatibility anyway, so that would really just make it more
> complicated.)

This sounds like a good idea. Userspace reaching into /dev/mem is not
good from a kernel hardening perspective. That's why those strict devmem
configs exist. Can cbmem be updated to query information from device
drivers instead, so that we can enable CONFIG_IO_STRICT_DEVMEM as well?


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure
  2018-08-09 19:52       ` Brian Norris
@ 2018-08-09 23:25         ` Stephen Boyd
  0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 23:25 UTC (permalink / raw)
  To: Brian Norris
  Cc: Greg Kroah-Hartman, linux-kernel, Wei-Ning Huang, Julius Werner,
	Samuel Holland

Quoting Brian Norris (2018-08-09 12:52:13)
> Hi,
> 
> On Thu, Aug 09, 2018 at 12:40:31PM -0700, Stephen Boyd wrote:
> > Quoting Brian Norris (2018-08-09 10:49:38)
> > > On Thu, Aug 09, 2018 at 10:17:17AM -0700, Stephen Boyd wrote:
> > > > Both callers of coreboot_table_init() ioremap the pointer that comes in
> > > > but they don't unmap the memory on failure. Both of them also fail probe
> > > > immediately with the return value of coreboot_table_init(), leaking a
> > > > mapping when it fails. Plug the leak so the mapping isn't left unused.
> > > > 
> > > > Cc: Wei-Ning Huang <wnhuang@chromium.org>
> > > > Cc: Julius Werner <jwerner@chromium.org>
> > > > Cc: Brian Norris <briannorris@chromium.org>
> > > > Cc: Samuel Holland <samuel@sholland.org>
> > > > Fixes: 570d30c2823f ("firmware: coreboot: Expose the coreboot table as a bus")
> > > 
> > > I suppose this is fair, since that commit introduced error paths and
> > > didn't clean them up. But one warning below:
> > > 
> > > > Signed-off-by: Stephen Boyd <swboyd@chromium.org>
> > > > ---
> > > >  drivers/firmware/google/coreboot_table.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
> > > > index 19db5709ae28..0d3e140444ae 100644
> > > > --- a/drivers/firmware/google/coreboot_table.c
> > > > +++ b/drivers/firmware/google/coreboot_table.c
> > > > @@ -138,6 +138,9 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
> > > >               ptr_entry += entry.size;
> > > >       }
> > > >  
> > > > +     if (ret)
> > > > +             iounmap(ptr);
> > > 
> > > This works because no sub-driver is using this mapping any more (i.e.,
> > > because we killed coreboot_table_find()). Otherwise, we'd need to
> > > explicitly kill all the sub-devices first. IOW, if this gets backported
> > > to older kernels, it would need to go along with this and its other
> > > dependencies:
> > 
> > The memory is copied out of the table. So do the devices actually use
> > the memory that we remap here? I don't see how it's a problem if we
> > unmap the table after we populate devices.
> 
> No, the memory is (or was) copied each time. See:
> 
> int coreboot_table_find(int tag, void *data, size_t data_size)
> {
> ...
>         memcpy_fromio(&header, ptr_header, sizeof(header));
> ...
> 
> (where ptr_header is an alias for 'ptr')
> 
> So before commit b616cf53aa7a and friends, this patch is a bad idea.
> 
> Just to reiterate/clarify: none of this is a criticism of this patch as
> applied to mainline. It's just a criticism of what might happen with the
> 'Fixes' tag if we aren't careful.
> 

Ok. I misread your email. Either way, both of these commits we're
talking about here are only in v4.18-rc series, so backporting for
stable will be fine either way.

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric
  2018-08-09 18:10   ` Julius Werner
@ 2018-08-09 23:30     ` Stephen Boyd
  0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 23:30 UTC (permalink / raw)
  To: Julius Werner
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

Quoting Julius Werner (2018-08-09 11:10:42)
> > @@ -138,8 +136,10 @@ int coreboot_table_init(struct device *dev, void __iomem *ptr)
> >                 ptr_entry += entry.size;
> >         }
> >
> > -       if (ret)
> > +       if (ret) {
> > +               bus_unregister(&coreboot_bus_type);
> >                 iounmap(ptr);
> > +       }
> 
> nit: maybe cleaner to just do if (ret) coreboot_table_exit(); here?
> You're essentially writing the same code again.

Ok.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access
  2018-08-09 23:03     ` Stephen Boyd
@ 2018-08-09 23:37       ` Julius Werner
  2018-08-09 23:44         ` Julius Werner
  0 siblings, 1 reply; 26+ messages in thread
From: Julius Werner @ 2018-08-09 23:37 UTC (permalink / raw)
  To: swboyd
  Cc: Julius Werner, Greg Kroah-Hartman, LKML, Wei-Ning Huang,
	Brian Norris, samuel

> Furthermore, I see that my system RAM excludes this coreboot table so it
> doesn't fall into the bucket that CONFIG_STRICT_DEVMEM would find.

Yes, that is intentional. We don't want the kernel to try to use that
memory for anything else (since we want those tables to survive), so
we mark them as reserved in the e820 map.

> > (I guess an alternative would be to rewrite 'cbmem' to use
> > /sys/bus/coreboot/devices if available to get its coreboot table
> > information. But we'd still need to maintain the old path for
> > backwards compatibility anyway, so that would really just make it more
> > complicated.)
>
> This sounds like a good idea. Userspace reaching into /dev/mem is not
> good from a kernel hardening perspective. That's why those strict devmem
> configs exist. Can cbmem be updated to query information from device
> drivers instead, so that we can enable CONFIG_IO_STRICT_DEVMEM as well?

Well... problem is that cbmem doesn't just access the coreboot tables,
it accesses more stuff. There is actually a larger memory region
called CBMEM (that's what the utility is named after) which contains
all sorts of random memory allocations that coreboot wanted to survive
for the lifetime of the system. The coreboot table is one section in
there, and it sort of serves as a directory for some of the others
(although there's also just a general CBMEM directory... there's some
redundancy there). But cbmem can also print some of the other CBMEM
sections which it finds by querying the coreboot table, such as the
firmware log or the boot timestamps.

So the question is how we can get to that content if /dev/mem isn't
available anymore. One option would be to just write separate kernel
drivers to completely replace the cbmem utility (we already have one
for the log, for example), but I think Linux generally doesn't want to
have too much logic and parsing and stuff in kernel drivers. Another
option is to add a driver that just exposes a sysfs file through which
you could read (we don't need to write) the CBMEM area... but then
we'd essentially want that to take absolute addresses because that's
what the coreboot table pointers contain, so we would've just built
/dev/mem by another name (for a restricted range).

The nicest thing, really, would be if there was a way for a kernel
driver to mark specific regions as "allowed" by /dev/mem. I don't
suppose we'd be willing to introduce a mechanism like that?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init()
  2018-08-09 21:02   ` Julius Werner
@ 2018-08-09 23:43     ` Stephen Boyd
  0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-08-09 23:43 UTC (permalink / raw)
  To: Julius Werner
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Julius Werner,
	Brian Norris, samuel

Quoting Julius Werner (2018-08-09 14:02:53)
> On Thu, Aug 9, 2018 at 10:17 AM Stephen Boyd <swboyd@chromium.org> wrote:
> > @@ -162,26 +143,37 @@ static int coreboot_table_probe(struct platform_device *pdev)
> >         if (!res->start || !len)
> >                 return -EINVAL;
> >
> > +       /* Map and check just the header first to make sure things are sane */
> >         header = memremap(res->start, sizeof(*header), MEMREMAP_WB);
> > -       if (header == NULL)
> > +       if (!header)
> >                 return -ENOMEM;
> >
> > -       ptr = memremap(res->start, header->header_bytes + header->table_bytes,
> > -                      MEMREMAP_WB);
> > +       if (strncmp(header->signature, "LBIO", sizeof(header->signature))) {
> > +               dev_warn(dev, "coreboot table missing or corrupt!\n");
> > +               return -ENODEV;
> 
> Leaking the mapping here.

Thanks. Fixed.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access
  2018-08-09 23:37       ` Julius Werner
@ 2018-08-09 23:44         ` Julius Werner
  2018-08-10  2:54           ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Julius Werner @ 2018-08-09 23:44 UTC (permalink / raw)
  To: Julius Werner
  Cc: swboyd, Greg Kroah-Hartman, LKML, Wei-Ning Huang, Brian Norris, samuel

Actually, looking at what IO_STRICT_DEVMEM really does, would it
really prevent userspace accesses to these areas? Because it seems
that it only prevents accesses to areas marked as IORESOURCE_BUSY, and
while I can't fully follow how the kernel assigns that, comments
suggest that this is only set when "Driver has marked this resource
busy".

So after you make the change to the other patch where we immediately
unmap the coreboot table again at the end of the probe() function,
shouldn't it become available to userspace again even with
IO_STRICT_DEVMEM set?
On Thu, Aug 9, 2018 at 4:37 PM Julius Werner <jwerner@chromium.org> wrote:
>
> > Furthermore, I see that my system RAM excludes this coreboot table so it
> > doesn't fall into the bucket that CONFIG_STRICT_DEVMEM would find.
>
> Yes, that is intentional. We don't want the kernel to try to use that
> memory for anything else (since we want those tables to survive), so
> we mark them as reserved in the e820 map.
>
> > > (I guess an alternative would be to rewrite 'cbmem' to use
> > > /sys/bus/coreboot/devices if available to get its coreboot table
> > > information. But we'd still need to maintain the old path for
> > > backwards compatibility anyway, so that would really just make it more
> > > complicated.)
> >
> > This sounds like a good idea. Userspace reaching into /dev/mem is not
> > good from a kernel hardening perspective. That's why those strict devmem
> > configs exist. Can cbmem be updated to query information from device
> > drivers instead, so that we can enable CONFIG_IO_STRICT_DEVMEM as well?
>
> Well... problem is that cbmem doesn't just access the coreboot tables,
> it accesses more stuff. There is actually a larger memory region
> called CBMEM (that's what the utility is named after) which contains
> all sorts of random memory allocations that coreboot wanted to survive
> for the lifetime of the system. The coreboot table is one section in
> there, and it sort of serves as a directory for some of the others
> (although there's also just a general CBMEM directory... there's some
> redundancy there). But cbmem can also print some of the other CBMEM
> sections which it finds by querying the coreboot table, such as the
> firmware log or the boot timestamps.
>
> So the question is how we can get to that content if /dev/mem isn't
> available anymore. One option would be to just write separate kernel
> drivers to completely replace the cbmem utility (we already have one
> for the log, for example), but I think Linux generally doesn't want to
> have too much logic and parsing and stuff in kernel drivers. Another
> option is to add a driver that just exposes a sysfs file through which
> you could read (we don't need to write) the CBMEM area... but then
> we'd essentially want that to take absolute addresses because that's
> what the coreboot table pointers contain, so we would've just built
> /dev/mem by another name (for a restricted range).
>
> The nicest thing, really, would be if there was a way for a kernel
> driver to mark specific regions as "allowed" by /dev/mem. I don't
> suppose we'd be willing to introduce a mechanism like that?

^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access
  2018-08-09 23:44         ` Julius Werner
@ 2018-08-10  2:54           ` Stephen Boyd
  2018-08-10 23:24             ` Stephen Boyd
  0 siblings, 1 reply; 26+ messages in thread
From: Stephen Boyd @ 2018-08-10  2:54 UTC (permalink / raw)
  To: Julius Werner
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Brian Norris, samuel

What's with the top posting? ;-)

Quoting Julius Werner (2018-08-09 16:44:43)
> Actually, looking at what IO_STRICT_DEVMEM really does, would it
> really prevent userspace accesses to these areas? Because it seems
> that it only prevents accesses to areas marked as IORESOURCE_BUSY, and
> while I can't fully follow how the kernel assigns that, comments
> suggest that this is only set when "Driver has marked this resource
> busy".

Requesting the memory region as is done in this patch marks it as busy.

> 
> So after you make the change to the other patch where we immediately
> unmap the coreboot table again at the end of the probe() function,
> shouldn't it become available to userspace again even with
> IO_STRICT_DEVMEM set?

Yes, if we unmap the region immediately after devices are populated then
this whole point is moot with the assumption that this code isn't
running at the same time as the cbmem utility. I've done this already
and it is working on arm. I still need to build/boot/test on an x86
platform which I should be able to do tomorrow.


^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access
  2018-08-10  2:54           ` Stephen Boyd
@ 2018-08-10 23:24             ` Stephen Boyd
  0 siblings, 0 replies; 26+ messages in thread
From: Stephen Boyd @ 2018-08-10 23:24 UTC (permalink / raw)
  To: Julius Werner
  Cc: Greg Kroah-Hartman, LKML, Wei-Ning Huang, Brian Norris, samuel

Quoting Stephen Boyd (2018-08-09 19:54:27)
> What's with the top posting? ;-)
> 
> Quoting Julius Werner (2018-08-09 16:44:43)
> > Actually, looking at what IO_STRICT_DEVMEM really does, would it
> > really prevent userspace accesses to these areas? Because it seems
> > that it only prevents accesses to areas marked as IORESOURCE_BUSY, and
> > while I can't fully follow how the kernel assigns that, comments
> > suggest that this is only set when "Driver has marked this resource
> > busy".
> 
> Requesting the memory region as is done in this patch marks it as busy.
> 
> > 
> > So after you make the change to the other patch where we immediately
> > unmap the coreboot table again at the end of the probe() function,
> > shouldn't it become available to userspace again even with
> > IO_STRICT_DEVMEM set?
> 
> Yes, if we unmap the region immediately after devices are populated then
> this whole point is moot with the assumption that this code isn't
> running at the same time as the cbmem utility. I've done this already
> and it is working on arm. I still need to build/boot/test on an x86
> platform which I should be able to do tomorrow.
> 

I tried my latest version of the patches (unplubished so far) and those
work on x86 with cbmem. So things look good and we don't need to keep
the mapping around.


^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2018-08-10 23:25 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-08-09 17:17 [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Stephen Boyd
2018-08-09 17:17 ` [PATCH v3 1/7] firmware: coreboot: Let OF core populate platform device Stephen Boyd
2018-08-09 17:31   ` Brian Norris
2018-08-09 17:17 ` [PATCH v3 2/7] firmware: coreboot: Unmap ioregion on failure Stephen Boyd
2018-08-09 17:49   ` Brian Norris
2018-08-09 19:40     ` Stephen Boyd
2018-08-09 19:52       ` Brian Norris
2018-08-09 23:25         ` Stephen Boyd
2018-08-09 17:17 ` [PATCH v3 3/7] firmware: coreboot: Make bus registration symmetric Stephen Boyd
2018-08-09 18:10   ` Julius Werner
2018-08-09 23:30     ` Stephen Boyd
2018-08-09 17:17 ` [PATCH v3 4/7] firmware: coreboot: Collapse platform drivers into bus core Stephen Boyd
2018-08-09 17:17 ` [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap() Stephen Boyd
2018-08-09 18:24   ` Julius Werner
2018-08-09 22:07     ` Stephen Boyd
2018-08-09 17:17 ` [PATCH v3 6/7] firmware: coreboot: Only populate devices in coreboot_table_init() Stephen Boyd
2018-08-09 21:02   ` Julius Werner
2018-08-09 23:43     ` Stephen Boyd
2018-08-09 17:17 ` [PATCH v3 7/7] firmware: coreboot: Request table region for exclusive access Stephen Boyd
2018-08-09 21:07   ` Julius Werner
2018-08-09 23:03     ` Stephen Boyd
2018-08-09 23:37       ` Julius Werner
2018-08-09 23:44         ` Julius Werner
2018-08-10  2:54           ` Stephen Boyd
2018-08-10 23:24             ` Stephen Boyd
2018-08-09 18:03 ` [PATCH v3 0/7] firmware: coreboot: Fix probe and simplify code Brian Norris

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).