Re: [PATCH v4 12/14] platform/x86: wmi: create character devices when requested by drivers

[Greg KH <greg@kroah.com>]

On Thu, Oct 05, 2017 at 02:35:43PM +0000, Mario.Limonciello@dell.com wrote:
> > 
> > > +		strcpy(buf, "wmi/");
> > > +		strcpy(buf + 4, wdriver->driver.name);
> > > +		wblock->misc_dev.minor = MISC_DYNAMIC_MINOR;
> > > +		wblock->misc_dev.name = buf;
> > > +		wblock->misc_dev.fops = &wmi_fops;
> > > +		ret = misc_register(&wblock->misc_dev);
> > > +		if (ret) {
> > > +			dev_warn(dev, "failed to register char dev: %d", ret);
> > > +			kfree(buf);
> > 
> > Again, no unwinding needed?  Error message value returned?
> 
> It comes down to if the character device should be considered optional.  I'll
> make it fail if it can't create it.

Given that you are relying on it in this patch, it would be good to fail
if something is going wrong.

> > > +	if (wdriver->file_operations) {
> > > +		kfree(wblock->misc_dev.name);
> > > +		misc_deregister(&wblock->misc_dev);
> > 
> > Unregister before freeing the device name, right?
> 
> Well if you unregister and then free the name you'll have lost the pointer.
> So isn't that the right order?

Yes, but save the pointer to the name and then free it after the larger
structure is gone.  misc_deregister() does not free anything, so you
still have the memory around here, no need to even use a temp variable.

> > > --- /dev/null
> > > +++ b/include/uapi/linux/wmi.h
> > > @@ -0,0 +1,10 @@
> > > +#ifndef _UAPI_LINUX_WMI_H
> > > +#define _UAPI_LINUX_WMI_H
> > > +
> > > +#define WMI_IOC 'W'
> > > +#define WMI_IO(instance)	_IO(WMI_IOC, instance)
> > > +#define WMI_IOR(instance)	_IOR(WMI_IOC, instance, void*)
> > > +#define WMI_IOW(instance)	_IOW(WMI_IOC, instance, void*)
> > > +#define WMI_IOWR(instance)	_IOWR(WMI_IOC, instance, void*)
> > 
> > Ugh, void *, this is going to be "fun"...
> > 
> > My comments on just how fun is left for the actual driver that attempted
> > to implement these...
> > 
> 
> So until in kernel MOF parsing is available you can't predict the format of
> what an individual ACPI method will expect for its input.  Even when the in
> kernel MOF parsing is made available the data types may be complex structures.


I have no idea what MOF is, what "parsing" is involved, or how in the
world ACPI got involved here...

good luck!

greg k-h