On Friday 25 December 2015 02:23:04 Andy Lutomirski wrote: > On Thu, Dec 24, 2015 at 1:18 PM, Pali Rohár <pali.rohar@xxxxxxxxx> > wrote: > > According to Dell WMI document mentioned in ML dicussion archived > > at http://www.spinics.net/lists/platform-driver-x86/msg07220.html > > OS should check Dell WMI descriptor structure. Structure also > > provide Dell WMI interface version which is used later. > > I will rebase my big series on top of this. It'll give me a good > excuse to test that I got the probe ordering right. (The code is > explicitly intended to support use cases like this, and now I'll have > a real-world test for it.) I'll also test this in a bit. Ok! > > +MODULE_ALIAS("wmi:"DELL_DESCRIPTOR_GUID); > > I don't think this is necessary. The driver will only work if both > wmi devices and, hence, modaliases are present, so there's no need to > cause just one or the other to trigger dell-wmi autoloading. Maybe now when you are working on big WMI patch series is time to change modalias support in WMI to support AND-conjunction (&&) on WMI aliases, not just OR (one alias match). Something like: load dell-wmi.ko driver if system provides both WMI GUIDs. > > +/** > > > + * Descriptor buffer is 128 byte long and contains: > This isn't kerneldoc format, so I think this should just be "/*". > Ok, I will fix this in next version. > > + if (obj->buffer.length != 128) { > > + pr_err("Dell descriptor buffer has invalid length > > (%d)\n", + obj->buffer.length); > > + kfree(obj); > > + return -EINVAL; > > + } > > I would advocate for being more permissive: a buffer that is actually > too short for the fields we need would result in -EINVAL, but a > buffer that isn't 128 bytes would just be a warning and not cause > module load to fail. > > --Andy Sounds good, I will change this part. -- Pali Rohár pali.rohar@xxxxxxxxx
Attachment:
signature.asc
Description: This is a digitally signed message part.
