linux-kernel - Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose embedded Binary WMI MOF metadata

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, 6 Jun 2017 09:54:43 -0700
From:   Darren Hart <dvhart@...radead.org>
To:     Andy Shevchenko <andy.shevchenko@...il.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        Pali Rohár <pali.rohar@...il.com>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Andy Lutomirski <luto@...capital.net>,
        Mario Limonciello <mario_limonciello@...l.com>,
        Rafael Wysocki <rjw@...ysocki.net>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>
Subject: Re: [PATCH v2] platform/x86: wmi-bmof: New driver to expose embedded
 Binary WMI MOF metadata

On Tue, Jun 06, 2017 at 12:30:38PM +0300, Andy Shevchenko wrote:
> On Tue, Jun 6, 2017 at 6:16 AM, Andy Lutomirski <luto@...nel.org> wrote:
> > Many laptops (and maybe servers?) have embedded WMI Binary MOF metadata.
> > We do not yet have open-source tools for processing the data, although
> > one is in the works thanks to Pali:
> >
> >         https://github.com/pali/bmfdec
> >
> > There is currently no interface to get the data in the first place. By
> > exposing it, we facilitate the development of new tools.
> 
> My comments below.
> Overall, FWIW,
> Reviewed-by: Andy Shevchenko <andy.shevchenko@...il.com>
> 
> 
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/types.h>
> > +#include <linux/input.h>
> > +#include <linux/input/sparse-keymap.h>
> > +#include <linux/acpi.h>
> > +#include <linux/string.h>
> > +#include <linux/dmi.h>
> > +#include <linux/wmi.h>
> > +#include <acpi/video.h>
> 
> Alphabetical order? Up to you.

OK, I failed to audit this... lots we don't need in here.

The minimum to build is:

#include <linux/wmi.h>

So assuming this was copy/pasted from another file.

Again, no guidance in coding-style.rst on includes. Seems to me we should
include what we specifically require, regardless of whether or not another
header also happens to include it. We need acpi for example, even though wmi
also includes it.

We should include modules, even though acpi includes it.

We use several other things we aren't including for, like

memcpy
dev_kzalloc
sysfs_create_bin_file

So I suggest:

#include <linux/acpi.h>
#include <linux/device.h>
#include <linux/fs.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/string.h>
#include <linux/sysfs.h>
#include <linux/types.h>
#include <linux/wmi.h>

Which removes:
#include <acpi/video.h>
#include <linux/dmi.h>
#include <linux/init.h>
#include <linux/input.h>
#include <linux/input/sparse-keymap.h>
#include <linux/slab.h>

And adds:
#include <linux/device.h>
#include <linux/fs.h>
#include <linux/sysfs.h>

-- 
Darren Hart
VMware Open Source Technology Center