linux-kernel - Re: [PATCH] firmware: coreboot: Let OF core populate platform device

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 07 Aug 2018 23:00:50 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Brian Norris <briannorris@...omium.org>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-kernel@...r.kernel.org,
        Wei-Ning Huang <wnhuang@...omium.org>,
        Julius Werner <jwerner@...omium.org>,
        Samuel Holland <samuel@...lland.org>,
        Thierry Escande <thierry.escande@...labora.com>
Subject: Re: [PATCH] firmware: coreboot: Let OF core populate platform device

Quoting Brian Norris (2018-08-07 14:41:04)
> On Mon, Aug 06, 2018 at 10:10:47AM -0700, Stephen Boyd wrote:
> > diff --git a/drivers/firmware/google/coreboot_table-of.c b/drivers/firmware/google/coreboot_table-of.c
> > index f15bf404c579..270f112bdc54 100644
> > --- a/drivers/firmware/google/coreboot_table-of.c
> > +++ b/drivers/firmware/google/coreboot_table-of.c
> > @@ -18,21 +18,20 @@
> >  #include <linux/device.h>
> >  #include <linux/io.h>
> >  #include <linux/module.h>
> > -#include <linux/of_address.h>
> > -#include <linux/of_platform.h>
> > +#include <linux/mod_devicetable.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;
> > +     struct resource *res;
> >  
> > -     ptr = of_iomap(fw_dn, 0);
> > -     of_node_put(fw_dn);
> > -     if (!ptr)
> > -             return -ENOMEM;
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +     ptr = devm_ioremap_resource(&pdev->dev, res);
> 
> You're making this a devm_* ioremap, but coreboot_table_exit() will
> handle unmapping this for you already. So you're introducing a potential
> double-unmap.

Thanks! I'll split this patch into essential and non-essential pieces
then and layer shortening patches on top.

> 
> > +     if (IS_ERR(ptr))
> > +             return PTR_ERR(ptr);
> >  
> >       return coreboot_table_init(&pdev->dev, ptr);
> >  }
> > @@ -44,8 +43,9 @@ static int coreboot_table_of_remove(struct platform_device *pdev)
> >  
> >  static const struct of_device_id coreboot_of_match[] = {
> >       { .compatible = "coreboot" },
> > -     {},
> > +     {}
> 
> ...and since I had a *real* comment, I'll mention this bit I noticed
> previously...this diff seems a bit superfluous. I understand we don't
> really want to extend this list beyond the NULL terminator, but
> normally, I'd expect we leave it alone.
> 

I was adding MODULE_DEVICE_TABLE nearby. I'll leave the removal here
unless you really hate the style cleanup.

> >  };
> > +MODULE_DEVICE_TABLE(of, coreboot_of_match);
> >