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@xxxxxxxxxx> 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@xxxxxxxxx> > > > > +config WMI_BMOF > > + tristate "WMI embedded Binary MOF driver" > > + depends on ACPI_WMI > > > + default y > > Since it can be module it would be better to have more sane default > (distros usually prefers modules over built-in). > Thus, I would go, for example, with > > default ACPI_WMI Good point, done. > > > + ---help--- > > + Say Y here if you want to be able to read a firmware-embedded > > + WMI Binary MOF data. Using this requires userspace tools and may be > > + rather tedious. > > + > > + To compile this driver as a module, choose M here: the module will > > + be called wmi-bmof. > > > +#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. Hrm. There seems to be plenty of similar suggestions on the mailing lists, but nothing documented in coding-style.rst. If this is a thing we are going to ask of our contributors, it should be documented. I'm happy to reorder, would you consider sending the coding-style patch? > > > +#define WMI_BMOF_GUID "05901221-D566-11D1-B2F0-00A0C9062910" > > > +MODULE_ALIAS("wmi:" WMI_BMOF_GUID); > > I would gather all MODULE_* together, but it's also matter of taste. > Sure, done. > > +static ssize_t > > +read_bmof(struct file *filp, struct kobject *kobj, > > + struct bin_attribute *attr, > > + char *buf, loff_t off, size_t count) > > +{ > > + struct bmof_priv *priv = > > + container_of(attr, struct bmof_priv, bmof_bin_attr); > > + > > + if (off >= priv->bmofdata->buffer.length) > > + return 0; > > Shouldn't we return an error code here? -ERANGE or alike? > I took some time and compared this with: read(2) lseek(2) fseek(3) memory_read_from_buffer() If offset is <0, we should return EINVAL If offset is >end_of_buffer.... it's not so cut and dry. It is simpler to just return 0, and as far as how it affects usage... returning 0 seems perfectly acceptable for typical read loop usage. As loff_t is a long long, it could conceivably be < 0, so I've added a check for that and return -EINVAL in that case. > > +static int wmi_bmof_probe(struct wmi_device *wdev) > > +{ > > > + int ret; > > + > > + struct bmof_priv *priv = > > + devm_kzalloc(&wdev->dev, sizeof(struct bmof_priv), GFP_KERNEL); > > I'm not a fan of memory allocation in definition block, so, I would rewrite this > > struct bmof_priv *priv; > int ret; > > priv = devm_kzalloc(&wdev->dev, sizeof(struct bmof_priv), GFP_KERNEL); > Agreed, changed. Thanks for the review Andy. -- Darren Hart VMware Open Source Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
