[PATCH 0/5] Misc Google coreboot driver fixes/cleanups

[PATCH 0/5] Misc Google coreboot driver fixes/cleanups

[Stephen Boyd]

Here's some minor fixes and cleanups for the Google coreboot drivers
that I've had lying around in my tree for a little bit. They
tighten up the code a bit and get rid of some boiler plate.

Stephen Boyd (5):
  firmware: google: Add a module_coreboot_driver() macro and use it
  firmware: google: memconsole: Use devm_memremap()
  firmware: google: memconsole: Drop __iomem on memremap memory
  firmware: google: memconsole: Drop global func pointer
  firmware: google: coreboot: Drop unnecessary headers

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>
Cc: Guenter Roeck <groeck@chromium.org>

 drivers/firmware/google/coreboot_table.h      | 11 ++++++++-
 .../firmware/google/framebuffer-coreboot.c    | 14 +----------
 drivers/firmware/google/memconsole-coreboot.c | 24 ++++---------------
 drivers/firmware/google/memconsole.c          |  9 +++----
 drivers/firmware/google/vpd.c                 | 14 +----------
 drivers/firmware/google/vpd_decode.c          |  2 --
 6 files changed, 22 insertions(+), 52 deletions(-)


base-commit: e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd
-- 
Sent by a computer through tubes

[PATCH 1/5] firmware: google: Add a module_coreboot_driver() macro and use it

[Stephen Boyd]

Remove some boiler plate code we have in three drivers with a single
line each time. This also gets us a free assignment of the driver .owner
field, making these drivers work better as modules.

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>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table.h       | 10 ++++++++++
 drivers/firmware/google/framebuffer-coreboot.c | 14 +-------------
 drivers/firmware/google/memconsole-coreboot.c  | 14 +-------------
 drivers/firmware/google/vpd.c                  | 14 +-------------
 4 files changed, 13 insertions(+), 39 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 71a9de6b15fa..054fa9374c59 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -20,6 +20,7 @@
 #ifndef __COREBOOT_TABLE_H
 #define __COREBOOT_TABLE_H
 
+#include <linux/device.h>
 #include <linux/io.h>
 
 /* Coreboot table header structure */
@@ -91,4 +92,13 @@ 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);
 
+/* module_coreboot_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.  Each module may only use this macro once, and
+ * calling it replaces module_init() and module_exit()
+ */
+#define module_coreboot_driver(__coreboot_driver) \
+	module_driver(__coreboot_driver, coreboot_driver_register, \
+			coreboot_driver_unregister)
+
 #endif /* __COREBOOT_TABLE_H */
diff --git a/drivers/firmware/google/framebuffer-coreboot.c b/drivers/firmware/google/framebuffer-coreboot.c
index b8b49c067157..69a43116211c 100644
--- a/drivers/firmware/google/framebuffer-coreboot.c
+++ b/drivers/firmware/google/framebuffer-coreboot.c
@@ -97,19 +97,7 @@ static struct coreboot_driver framebuffer_driver = {
 	},
 	.tag = CB_TAG_FRAMEBUFFER,
 };
-
-static int __init coreboot_framebuffer_init(void)
-{
-	return coreboot_driver_register(&framebuffer_driver);
-}
-
-static void coreboot_framebuffer_exit(void)
-{
-	coreboot_driver_unregister(&framebuffer_driver);
-}
-
-module_init(coreboot_framebuffer_init);
-module_exit(coreboot_framebuffer_exit);
+module_coreboot_driver(framebuffer_driver);
 
 MODULE_AUTHOR("Samuel Holland <samuel@sholland.org>");
 MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index b29e10757bfb..86331807f1d5 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -116,19 +116,7 @@ static struct coreboot_driver memconsole_driver = {
 	},
 	.tag = CB_TAG_CBMEM_CONSOLE,
 };
-
-static void coreboot_memconsole_exit(void)
-{
-	coreboot_driver_unregister(&memconsole_driver);
-}
-
-static int __init coreboot_memconsole_init(void)
-{
-	return coreboot_driver_register(&memconsole_driver);
-}
-
-module_exit(coreboot_memconsole_exit);
-module_init(coreboot_memconsole_init);
+module_coreboot_driver(memconsole_driver);
 
 MODULE_AUTHOR("Google, Inc.");
 MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
index c0c0b4e4e281..4e978f4914ee 100644
--- a/drivers/firmware/google/vpd.c
+++ b/drivers/firmware/google/vpd.c
@@ -324,19 +324,7 @@ static struct coreboot_driver vpd_driver = {
 	},
 	.tag = CB_TAG_VPD,
 };
-
-static int __init coreboot_vpd_init(void)
-{
-	return coreboot_driver_register(&vpd_driver);
-}
-
-static void __exit coreboot_vpd_exit(void)
-{
-	coreboot_driver_unregister(&vpd_driver);
-}
-
-module_init(coreboot_vpd_init);
-module_exit(coreboot_vpd_exit);
+module_coreboot_driver(vpd_driver);
 
 MODULE_AUTHOR("Google, Inc.");
 MODULE_LICENSE("GPL");
-- 
Sent by a computer through tubes

[PATCH 2/5] firmware: google: memconsole: Use devm_memremap()

[Stephen Boyd]

Use the devm version of memremap so that we can delete the unmapping
code in driver remove, but more importantly so that we can unmap this
memory region if memconsole_sysfs_init() errors out for some reason.

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>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index 86331807f1d5..0b29b27b86f5 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -85,7 +85,7 @@ static int memconsole_probe(struct coreboot_device *dev)
 
 	/* Read size only once to prevent overrun attack through /dev/mem. */
 	cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
-	cbmem_console = memremap(dev->cbmem_ref.cbmem_addr,
+	cbmem_console = devm_memremap(&dev->dev, dev->cbmem_ref.cbmem_addr,
 				 cbmem_console_size + sizeof(*cbmem_console),
 				 MEMREMAP_WB);
 	memunmap(tmp_cbmc);
@@ -102,9 +102,6 @@ static int memconsole_remove(struct coreboot_device *dev)
 {
 	memconsole_exit();
 
-	if (cbmem_console)
-		memunmap(cbmem_console);
-
 	return 0;
 }
 
-- 
Sent by a computer through tubes

[PATCH 3/5] firmware: google: memconsole: Drop __iomem on memremap memory

[Stephen Boyd]

memremap() doesn't return __iomem marked memory, so drop the marking
here. This makes static analysis tools like sparse happy again.

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>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/memconsole-coreboot.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index 0b29b27b86f5..ab0fe93b88ad 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -34,7 +34,7 @@ struct cbmem_cons {
 #define CURSOR_MASK ((1 << 28) - 1)
 #define OVERFLOW (1 << 31)
 
-static struct cbmem_cons __iomem *cbmem_console;
+static struct cbmem_cons *cbmem_console;
 static u32 cbmem_console_size;
 
 /*
@@ -75,7 +75,7 @@ static ssize_t memconsole_coreboot_read(char *buf, loff_t pos, size_t count)
 
 static int memconsole_probe(struct coreboot_device *dev)
 {
-	struct cbmem_cons __iomem *tmp_cbmc;
+	struct cbmem_cons *tmp_cbmc;
 
 	tmp_cbmc = memremap(dev->cbmem_ref.cbmem_addr,
 			    sizeof(*tmp_cbmc), MEMREMAP_WB);
-- 
Sent by a computer through tubes

[PATCH 4/5] firmware: google: memconsole: Drop global func pointer

[Stephen Boyd]

We can store this function pointer directly in the bin_attribute
structure's private field. Do this to save one global pointer.

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>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/memconsole.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c
index 166f07c68c02..968135025e4f 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -22,14 +22,16 @@
 
 #include "memconsole.h"
 
-static ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
-
 static ssize_t memconsole_read(struct file *filp, struct kobject *kobp,
 			       struct bin_attribute *bin_attr, char *buf,
 			       loff_t pos, size_t count)
 {
+	ssize_t (*memconsole_read_func)(char *, loff_t, size_t);
+
+	memconsole_read_func = bin_attr->private;
 	if (WARN_ON_ONCE(!memconsole_read_func))
 		return -EIO;
+
 	return memconsole_read_func(buf, pos, count);
 }
 
@@ -40,7 +42,7 @@ static struct bin_attribute memconsole_bin_attr = {
 
 void memconsole_setup(ssize_t (*read_func)(char *, loff_t, size_t))
 {
-	memconsole_read_func = read_func;
+	memconsole_bin_attr.private = read_func;
 }
 EXPORT_SYMBOL(memconsole_setup);
 
-- 
Sent by a computer through tubes

[PATCH 5/5] firmware: google: coreboot: Drop unnecessary headers

[Stephen Boyd]

These headers aren't used by the files they're included in, so drop
them. The memconsole file uses memremap() though, so include io.h there
so that the include is explicit.

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>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---
 drivers/firmware/google/coreboot_table.h      | 1 -
 drivers/firmware/google/memconsole-coreboot.c | 1 +
 drivers/firmware/google/memconsole.c          | 1 -
 drivers/firmware/google/vpd_decode.c          | 2 --
 4 files changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index 054fa9374c59..d33c139447f6 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -21,7 +21,6 @@
 #define __COREBOOT_TABLE_H
 
 #include <linux/device.h>
-#include <linux/io.h>
 
 /* Coreboot table header structure */
 struct coreboot_table_header {
diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index ab0fe93b88ad..f8a66354e5a4 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -16,6 +16,7 @@
  */
 
 #include <linux/device.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 
diff --git a/drivers/firmware/google/memconsole.c b/drivers/firmware/google/memconsole.c
index 968135025e4f..c8156db0e3a0 100644
--- a/drivers/firmware/google/memconsole.c
+++ b/drivers/firmware/google/memconsole.c
@@ -15,7 +15,6 @@
  * GNU General Public License for more details.
  */
 
-#include <linux/init.h>
 #include <linux/sysfs.h>
 #include <linux/kobject.h>
 #include <linux/module.h>
diff --git a/drivers/firmware/google/vpd_decode.c b/drivers/firmware/google/vpd_decode.c
index 943acaa8aa76..f8c9143472df 100644
--- a/drivers/firmware/google/vpd_decode.c
+++ b/drivers/firmware/google/vpd_decode.c
@@ -15,8 +15,6 @@
  * GNU General Public License for more details.
  */
 
-#include <linux/export.h>
-
 #include "vpd_decode.h"
 
 static int vpd_decode_len(const s32 max_len, const u8 *in,
-- 
Sent by a computer through tubes

Re: [PATCH 0/5] Misc Google coreboot driver fixes/cleanups

[Julius Werner]

> Here's some minor fixes and cleanups for the Google coreboot drivers
> that I've had lying around in my tree for a little bit. They
> tighten up the code a bit and get rid of some boiler plate.
>
> Stephen Boyd (5):
>   firmware: google: Add a module_coreboot_driver() macro and use it
>   firmware: google: memconsole: Use devm_memremap()
>   firmware: google: memconsole: Drop __iomem on memremap memory
>   firmware: google: memconsole: Drop global func pointer
>   firmware: google: coreboot: Drop unnecessary headers

Thanks, these all look good to me.

Reviewed-by: Julius Werner <jwerner@chromium.org>

[PATCH v2 2/5] firmware: google: memconsole: Use devm_memremap()

[Stephen Boyd]

Use the devm version of memremap so that we can delete the unmapping
code in driver remove, but more importantly so that we can unmap this
memory region if memconsole_sysfs_init() errors out for some reason.

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>
Cc: Guenter Roeck <groeck@chromium.org>
Signed-off-by: Stephen Boyd <swboyd@chromium.org>
---

Quoting Stephen Boyd (2019-05-10 11:01:48)
> diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
> index 86331807f1d5..0b29b27b86f5 100644
> --- a/drivers/firmware/google/memconsole-coreboot.c
> +++ b/drivers/firmware/google/memconsole-coreboot.c
> @@ -85,7 +85,7 @@ static int memconsole_probe(struct coreboot_device *dev)
>  
>         /* Read size only once to prevent overrun attack through /dev/mem. */
>         cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
> -       cbmem_console = memremap(dev->cbmem_ref.cbmem_addr,
> +       cbmem_console = devm_memremap(&dev->dev, dev->cbmem_ref.cbmem_addr,

Whoops, this returns an error pointer now. Here's an updated patch.

 drivers/firmware/google/memconsole-coreboot.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/google/memconsole-coreboot.c b/drivers/firmware/google/memconsole-coreboot.c
index 86331807f1d5..cc3797f1ba85 100644
--- a/drivers/firmware/google/memconsole-coreboot.c
+++ b/drivers/firmware/google/memconsole-coreboot.c
@@ -85,13 +85,13 @@ static int memconsole_probe(struct coreboot_device *dev)
 
 	/* Read size only once to prevent overrun attack through /dev/mem. */
 	cbmem_console_size = tmp_cbmc->size_dont_access_after_boot;
-	cbmem_console = memremap(dev->cbmem_ref.cbmem_addr,
+	cbmem_console = devm_memremap(&dev->dev, dev->cbmem_ref.cbmem_addr,
 				 cbmem_console_size + sizeof(*cbmem_console),
 				 MEMREMAP_WB);
 	memunmap(tmp_cbmc);
 
-	if (!cbmem_console)
-		return -ENOMEM;
+	if (IS_ERR(cbmem_console))
+		return PTR_ERR(cbmem_console);
 
 	memconsole_setup(memconsole_coreboot_read);
 
@@ -102,9 +102,6 @@ static int memconsole_remove(struct coreboot_device *dev)
 {
 	memconsole_exit();
 
-	if (cbmem_console)
-		memunmap(cbmem_console);
-
 	return 0;
 }
 

base-commit: e93c9c99a629c61837d5a7fc2120cd2b6c70dbdd
prerequisite-patch-id: b7c2a1e21fb108364f0e8cfaf1970cbc7903c750
-- 
Sent by a computer through tubes

Re: [PATCH 0/5] Misc Google coreboot driver fixes/cleanups

[Samuel Holland]

On 5/10/19 1:01 PM, Stephen Boyd wrote:
> Here's some minor fixes and cleanups for the Google coreboot drivers
> that I've had lying around in my tree for a little bit. They
> tighten up the code a bit and get rid of some boiler plate.
> 
> Stephen Boyd (5):
>   firmware: google: Add a module_coreboot_driver() macro and use it
>   firmware: google: memconsole: Use devm_memremap()
>   firmware: google: memconsole: Drop __iomem on memremap memory
>   firmware: google: memconsole: Drop global func pointer
>   firmware: google: coreboot: Drop unnecessary headers

With v2 of patch 2:

Reviewed-by: Samuel Holland <samuel@sholland.org>

Re: [PATCH 1/5] firmware: google: Add a module_coreboot_driver() macro and use it

[Brian Norris]

On Fri, May 10, 2019 at 11:01:47AM -0700, Stephen Boyd wrote:
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h

> @@ -91,4 +92,13 @@ 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);
>  
> +/* module_coreboot_driver() - Helper macro for drivers that don't do

Have you been writing too much net/ code recently? :) Or just copying
from platform_device.h I guess. Oh well.

Series looks fine to me.

Brian

> + * anything special in module init/exit.  This eliminates a lot of
> + * boilerplate.  Each module may only use this macro once, and
> + * calling it replaces module_init() and module_exit()
> + */
> +#define module_coreboot_driver(__coreboot_driver) \
> +	module_driver(__coreboot_driver, coreboot_driver_register, \
> +			coreboot_driver_unregister)
> +
>  #endif /* __COREBOOT_TABLE_H */