linux-kernel - Re: [PATCH 7/8] firmware: vpd: remove platform driver

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]

Message-ID: <20170525023833.GB30589@dtor-ws>
Date:   Wed, 24 May 2017 19:38:33 -0700
From:   Dmitry Torokhov <dmitry.torokhov@...il.com>
To:     Julius Werner <jwerner@...gle.com>
Cc:     Guenter Roeck <groeck@...gle.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Wei-Ning Huang <wnhuang@...gle.com>,
        Julius Werner <jwerner@...omium.org>,
        Guenter Roeck <groeck@...omium.org>,
        linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 7/8] firmware: vpd: remove platform driver

On Wed, May 24, 2017 at 05:04:41PM -0700, Julius Werner wrote:
> I'm not a kernel expert so maybe I don't understand this right, but...
> I think this might have been done this way to ensure that the driver
> can get initialized correctly regardless of probe ordering.
> coreboot_table_find() may fail with -EPROBE_DEFER if the
> coreboot_table driver and its dependent (coreboot_table-acpi or
> coreboot_table-of) haven't been probed yet. In that case we want the
> VPD driver to wait and try again later, after they've been probed. I
> believe with the way this used to be written the driver core did that
> automatically for us, but with your change I'm not sure if it still
> does.

Hmm, I missed that. OK, I think we have a maze of platform drivers
there, I'll see what can be done.

> 
> On Wed, May 24, 2017 at 10:22 AM, Guenter Roeck <groeck@...gle.com> wrote:
> > On Tue, May 23, 2017 at 5:07 PM, Dmitry Torokhov
> > <dmitry.torokhov@...il.com> wrote:
> >> There is no reason why VPD should register platform device and driver,
> >> given that we do not use their respective kobjects to attach attributes,
> >> nor do we need suspend/resume hooks, or any other features of device
> >> core.
> >>
> >> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@...il.com>
> >
> > One reason might be to use devm_ functions for memory allocations to
> > simplify error handling and cleanup. Without that, I agree.
> >
> > Reviewed-by: Guenter Roeck <groeck@...omium.org>
> >
> >> ---
> >>  drivers/firmware/google/vpd.c | 44 ++++++++++++++++---------------------------
> >>  1 file changed, 16 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/drivers/firmware/google/vpd.c b/drivers/firmware/google/vpd.c
> >> index 78945729388e..f4766bf6785a 100644
> >> --- a/drivers/firmware/google/vpd.c
> >> +++ b/drivers/firmware/google/vpd.c
> >> @@ -22,8 +22,6 @@
> >>  #include <linux/kobject.h>
> >>  #include <linux/list.h>
> >>  #include <linux/module.h>
> >> -#include <linux/of_address.h>
> >> -#include <linux/platform_device.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/sysfs.h>
> >>
> >> @@ -279,47 +277,37 @@ static int vpd_sections_init(phys_addr_t physaddr)
> >>                 ret = vpd_section_init("rw", &rw_vpd,
> >>                                        physaddr + sizeof(struct vpd_cbmem) +
> >>                                        header.ro_size, header.rw_size);
> >> -               if (ret)
> >> +               if (ret) {
> >> +                       vpd_section_destroy(&ro_vpd);
> >>                         return ret;
> >> +               }
> >>         }
> >>
> >>         return 0;
> >>  }
> >>
> >> -static int vpd_probe(struct platform_device *pdev)
> >> -{
> >> -       int ret;
> >> -       struct lb_cbmem_ref entry;
> >> -
> >> -       ret = coreboot_table_find(CB_TAG_VPD, &entry, sizeof(entry));
> >> -       if (ret)
> >> -               return ret;
> >> -
> >> -       return vpd_sections_init(entry.cbmem_addr);
> >> -}
> >> -
> >> -static struct platform_driver vpd_driver = {
> >> -       .probe = vpd_probe,
> >> -       .driver = {
> >> -               .name = "vpd",
> >> -       },
> >> -};
> >> -
> >>  static int __init vpd_platform_init(void)
> >>  {
> >> -       struct platform_device *pdev;
> >> -
> >> -       pdev = platform_device_register_simple("vpd", -1, NULL, 0);
> >> -       if (IS_ERR(pdev))
> >> -               return PTR_ERR(pdev);
> >> +       struct lb_cbmem_ref entry;
> >> +       int err;
> >>
> >>         vpd_kobj = kobject_create_and_add("vpd", firmware_kobj);
> >>         if (!vpd_kobj)
> >>                 return -ENOMEM;
> >>
> >> -       platform_driver_register(&vpd_driver);
> >> +       err = coreboot_table_find(CB_TAG_VPD, &entry, sizeof(entry));
> >> +       if (err)
> >> +               goto err_kobject_put;
> >> +
> >> +       err = vpd_sections_init(entry.cbmem_addr);
> >> +       if (err)
> >> +               goto err_kobject_put;
> >>
> >>         return 0;
> >> +
> >> +err_kobject_put:
> >> +       kobject_put(vpd_kobj);
> >> +       return err;
> >>  }
> >>
> >>  static void __exit vpd_platform_exit(void)
> >> --
> >> 2.13.0.219.gdb65acc882-goog
> >>



-- 
Dmitry