Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core
Hi Stephen,
I love your patch! Perhaps something to improve:
[auto build test WARNING on linus/master]
[also build test WARNING on v4.18-rc8 next-20180809]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system]
url:
https://github.com/0day-ci/linux/commits/Stephen-Boyd/firmware-coreboot-Fix-probe-and-simplify-code/20180810-015624
reproduce:
# apt-get install sparse
make ARCH=x86_64 allmodconfig
make C=1 CF=-D__CHECK_ENDIAN__
sparse warnings: (new ones prefixed by >>)
drivers/firmware/google/coreboot_table.c:113:22: sparse: cast removes
address space of expression
drivers/firmware/google/coreboot_table.c:115:39: sparse: incorrect type in
argument 2 (different address spaces) @@expected void const volatile
[noderef] *addr @@got noderef] *addr @@
drivers/firmware/google/coreboot_table.c:115:39:expected void const
volatile [noderef] *addr
drivers/firmware/google/coreboot_table.c:115:39:got void *[assigned]
ptr_entry
drivers/firmware/google/coreboot_table.c:127:47: sparse: incorrect type in
argument 2 (different address spaces) @@expected void const volatile
[noderef] *addr @@got noderef] *addr @@
drivers/firmware/google/coreboot_table.c:127:47:expected void const
volatile [noderef] *addr
drivers/firmware/google/coreboot_table.c:127:47:got void *[assigned]
ptr_entry
>> drivers/firmware/google/coreboot_table.c:163:29: sparse: dereference of
>> noderef expression
drivers/firmware/google/coreboot_table.c:163:52: sparse: dereference of
noderef expression
vim +163 drivers/firmware/google/coreboot_table.c
96
97 static int coreboot_table_init(struct device *dev, void __iomem *ptr)
98 {
99 int i, ret;
100 void *ptr_entry;
101 struct coreboot_device *device;
102 struct coreboot_table_entry entry;
103 struct coreboot_table_header header;
104
105 ptr_header = ptr;
106 memcpy_fromio(&header, ptr_header, sizeof(header));
107
108 if (strncmp(header.signature, "LBIO",
sizeof(header.signature))) {
109 pr_warn("coreboot_table: coreboot table missing or
corrupt!\n");
110 return -ENODEV;
111 }
112
> 113 ptr_entry = (void *)ptr_header + header.header_bytes;
114 for (i = 0; i < header.table_entries; i++) {
115 memcpy_fromio(&entry, ptr_entry, sizeof(entry));
116
117 device = kzalloc(sizeof(struct device) + entry.size,
GFP_KERNEL);
118 if (!device) {
119 ret = -ENOMEM;
120 break;
121 }
122
123 dev_set_name(&device->dev, "coreboot%d", i);
124 device->dev.parent = dev;
125 device->dev.bus = &coreboot_bus_type;
126 device->dev.release = coreboot_device_release;
127 memcpy_fromio(&device->entry, ptr_entry, entry.size);
128
129 ret = device_register(&device->dev);
130 if (ret) {
131 put_device(&device->dev);
132 break;
133 }
134
135 ptr_entry += entry.size;
136 }
137
138 return ret;
139 }
140
141 static int coreboot_table_probe(struct platform_device *pdev)
142 {
143 phys_addr_t phyaddr;
144 resource_size_t len;
145 struct coreboot_table_header __iomem *header = NULL;
146 struct resource *res;
147 void __iomem *ptr = NULL;
148
149 res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
150 if (!res)
151 return -EINVAL;
152
153 len = resource_size(res);
154 if (!res->start || !len)
155 return -EINVAL;
156
157 phyaddr = res->start;
158 header = ioremap_cache(phyaddr, sizeof(*header));
159 if (header == NULL)
160 return -ENOMEM;
161
162 ptr = ioremap_cache(phyaddr,
> 163 header->header_bytes + header->table_bytes);
164 iounmap(header);
165 if (!ptr)
166 return -ENOMEM;
167
168 return coreboot_table_init(&pdev->dev, ptr);
169 }
170
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core
Hi Stephen, I love your patch! Yet something to improve: [auto build test ERROR on linus/master] [also build test ERROR on v4.18-rc8 next-20180808] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Stephen-Boyd/firmware-coreboot-Fix-probe-and-simplify-code/20180810-015624 config: mips-allmodconfig (attached as .config) compiler: mips-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=mips All error/warnings (new ones prefixed by >>): In file included from arch/mips/include/asm/page.h:194:0, from include/linux/mmzone.h:21, from include/linux/gfp.h:6, from include/linux/slab.h:15, from include/linux/resource_ext.h:19, from include/linux/acpi.h:26, from drivers/firmware//google/coreboot_table.c:19: drivers/firmware//google/coreboot_table.c: In function 'coreboot_table_probe': >> arch/mips/include/asm/io.h:277:35: error: '_page_cachable_default' >> undeclared (first use in this function) __ioremap_mode((offset), (size), _page_cachable_default) ^ >> arch/mips/include/asm/io.h:278:23: note: in expansion of macro >> 'ioremap_cachable' #define ioremap_cache ioremap_cachable ^~~~ >> drivers/firmware//google/coreboot_table.c:158:11: note: in expansion of >> macro 'ioremap_cache' header = ioremap_cache(phyaddr, sizeof(*header)); ^ arch/mips/include/asm/io.h:277:35: note: each undeclared identifier is reported only once for each function it appears in __ioremap_mode((offset), (size), _page_cachable_default) ^ >> arch/mips/include/asm/io.h:278:23: note: in expansion of macro >> 'ioremap_cachable' #define ioremap_cache ioremap_cachable ^~~~ >> drivers/firmware//google/coreboot_table.c:158:11: note: in expansion of >> macro 'ioremap_cache' header = ioremap_cache(phyaddr, sizeof(*header)); ^ -- In file included from arch/mips/include/asm/page.h:194:0, from include/linux/mmzone.h:21, from include/linux/gfp.h:6, from include/linux/slab.h:15, from include/linux/resource_ext.h:19, from include/linux/acpi.h:26, from drivers/firmware/google/coreboot_table.c:19: drivers/firmware/google/coreboot_table.c: In function 'coreboot_table_probe': >> arch/mips/include/asm/io.h:277:35: error: '_page_cachable_default' >> undeclared (first use in this function) __ioremap_mode((offset), (size), _page_cachable_default) ^ >> arch/mips/include/asm/io.h:278:23: note: in expansion of macro >> 'ioremap_cachable' #define ioremap_cache ioremap_cachable ^~~~ drivers/firmware/google/coreboot_table.c:158:11: note: in expansion of macro 'ioremap_cache' header = ioremap_cache(phyaddr, sizeof(*header)); ^ arch/mips/include/asm/io.h:277:35: note: each undeclared identifier is reported only once for each function it appears in __ioremap_mode((offset), (size), _page_cachable_default) ^ >> arch/mips/include/asm/io.h:278:23: note: in expansion of macro >> 'ioremap_cachable' #define ioremap_cache ioremap_cachable ^~~~ drivers/firmware/google/coreboot_table.c:158:11: note: in expansion of macro 'ioremap_cache' header = ioremap_cache(phyaddr, sizeof(*header)); ^ vim +/_page_cachable_default +277 arch/mips/include/asm/io.h ^1da177e include/asm-mips/io.h Linus Torvalds2005-04-16 260 ^1da177e include/asm-mips/io.h Linus Torvalds2005-04-16 261 /* 778e2ac5 include/asm-mips/io.h Ralf Baechle 2006-02-28 262 * ioremap_cachable -map bus memory into CPU space 778e2ac5 include/asm-mips/io.h Ralf Baechle 2006-02-28 263 * @offset: bus address of the memory 778e2ac5 include/asm-mips/io.h Ralf Baechle 2006-02-28 264 * @size:size of the resource to map 778e2ac5 include/asm-mips/io.h Ralf Baechle 2006-02-28 265 * 778e2ac5 include/asm-mips/io.h Ralf Baechle 2006-02-28 266 * ioremap_nocache performs a platform specific sequence of operations to 778e2ac5 include/asm-mips/io.h Ralf Baechle 2006-02-28 267 * make bus memory CPU accessible via the readb/readw/readl/writeb/ 778e2ac5 include/as
Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core
Quoting Julius Werner (2018-08-08 12:07:30)
> > +config GOOGLE_COREBOOT_TABLE_ACPI
> > + tristate
> > + default GOOGLE_COREBOOT_TABLE
>
> I don't think this helps in upgrading (as your commit message says)
> unless you also keep the 'select GOOGLE_COREBOOT_TABLE' here, right?
Oh yes should be select, not default.
>
> > -int coreboot_table_init(struct device *dev, void __iomem *ptr)
> > +static int coreboot_table_init(struct device *dev, void __iomem *ptr)
>
> nit: There's little reason to keep coreboot_table_init() a separate
> function now. Could maybe compact the code a little more if you merge
> it into probe()? (Also could then do the signature sanity check before
> trusting the length values to map the whole thing, which is probably a
> good idea.)
Sure. I can make another patch for squashing that all together.
>
> > if (ptr_header) {
> > bus_unregister(&coreboot_bus_type);
> > iounmap(ptr_header);
>
> Could ptr_header be handled by devm now, somehow?
Yes. It hasn't been devmified yet because that would be more things in
one big patch. This is quickly blowing up!
> Also, don't you have
> two bus_unregister() now (here and in coreboot_exit())? Or is that
> intentional?
That's nice. I didn't notice that module_init() was registering the bus
and then platform drivers could remove the bus later with the driver
unbind. I'll move them both into the driver bind/unbind path, in another
patch.
>
> > +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),
>
> Who takes precedence if they both exist? Will we have two
> coreboot_table busses? (That would probably not be so good...)
I'm not aware of a system that has both ACPI and devicetree, so this
isn't a problem.
Re: [PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core
> +config GOOGLE_COREBOOT_TABLE_ACPI
> + tristate
> + default GOOGLE_COREBOOT_TABLE
I don't think this helps in upgrading (as your commit message says)
unless you also keep the 'select GOOGLE_COREBOOT_TABLE' here, right?
> -int coreboot_table_init(struct device *dev, void __iomem *ptr)
> +static int coreboot_table_init(struct device *dev, void __iomem *ptr)
nit: There's little reason to keep coreboot_table_init() a separate
function now. Could maybe compact the code a little more if you merge
it into probe()? (Also could then do the signature sanity check before
trusting the length values to map the whole thing, which is probably a
good idea.)
> if (ptr_header) {
> bus_unregister(&coreboot_bus_type);
> iounmap(ptr_header);
Could ptr_header be handled by devm now, somehow? Also, don't you have
two bus_unregister() now (here and in coreboot_exit())? Or is that
intentional?
> +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),
Who takes precedence if they both exist? Will we have two
coreboot_table busses? (That would probably not be so good...)
[PATCH v2 2/2] firmware: coreboot: Collapse platform drivers into bus core
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 default to 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
Cc: Julius Werner
Cc: Brian Norris
Cc: Samuel Holland
Cc: Sudeep Holla
Signed-off-by: Stephen Boyd
---
drivers/firmware/google/Kconfig | 28 +++---
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 | 96 ---
drivers/firmware/google/coreboot_table.h | 6 --
6 files changed, 96 insertions(+), 184 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 a456a48b..42bea4dfdc4b 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
+ default GOOGLE_COREBOOT_TABLE
+
config GOOGLE_COREBOOT_TABLE_OF
- tristate "Coreboot Table Access - Device Tree"
- depends on OF
- 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.
+ tristate
+ default GOOGLE_COREBOOT_TABLE
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..
--- 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
-#include
-#include
-#include
-#include
-#include
-#include
-#include
-
-#include "coreboot_table.h"
-
-static int coreboot_table_acpi_probe(struct platform_device *pdev)
-{
- phys_addr_t phyaddr;
- resource_si
