LKML Archive on lore.kernel.org
 help / color / Atom feed
From: Julius Werner <jwerner@chromium.org>
To: swboyd@chromium.org, Aaron Durbin <adurbin@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	LKML <linux-kernel@vger.kernel.org>,
	Wei-Ning Huang <wnhuang@chromium.org>,
	Julius Werner <jwerner@chromium.org>,
	Brian Norris <briannorris@chromium.org>,
	samuel@sholland.org
Subject: Re: [PATCH v3 5/7] firmware: coreboot: Remap RAM with memremap() instead of ioremap()
Date: Thu, 9 Aug 2018 11:24:29 -0700
Message-ID: <CAODwPW8x=ghbVpo4q48jh-xGScP+2O_Vt7LTtSqhLvAAnCVXuA@mail.gmail.com> (raw)
In-Reply-To: <20180809171722.144325-6-swboyd@chromium.org>

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
>

  reply index

Thread overview: 26+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
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 [this message]
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

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAODwPW8x=ghbVpo4q48jh-xGScP+2O_Vt7LTtSqhLvAAnCVXuA@mail.gmail.com' \
    --to=jwerner@chromium.org \
    --cc=adurbin@chromium.org \
    --cc=briannorris@chromium.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=samuel@sholland.org \
    --cc=swboyd@chromium.org \
    --cc=wnhuang@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org linux-kernel@archiver.kernel.org
	public-inbox-index lkml


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/ public-inbox