Re: [PATCH 1/2] dell-wmi: Check if Dell WMI descriptor structure is valid

[Andy Lutomirski]

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.

> +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.

> +/**
> + * Descriptor buffer is 128 byte long and contains:

This isn't kerneldoc format, so I think this should just be "/*".

> +       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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/