Re: RFC (v2): Intel QST sensor driver

From: Guenter Roeck
Date: Mon Mar 18 2013 - 20:27:29 EST


Hi Simon,

On Mon, Mar 18, 2013 at 09:20:53PM +0000, Simon J. Rowe wrote:
> Hello,
>
> I've made changes to my driver for the Intel Quiet System Technology
> (QST) function that I posted at the end of last year and would again
> appreciate feedback on it.
>
> As before the git repo can be found here:
>
> http://mose.dyndns.org/mei.git
>
>
> Changes from v1
> ---------------
>
> The module has been re-written to be data-driven rather than use
> macros. Only v1 of the protocol is implemented but adding support for
> v2 only requires three arrays and nine stub functions to be defined.
>
> The code has been compiled and tested on 3.9 rc2.
>
> The code has been fixed up after running checkpatch.pl.
>
Couple of problems I noticed when browsing through the code.

- Some functions return errors with return code 0.

if (ret <= 0)
goto out;
...
out:
return ret;

For values of 0, the calling code will likely miss the error.

- In some cases, returned errors are replaced with another error

if (ret < 0)
return -EIO;

You should return the original error.

- Try using something better than -EIO is possible. For example, you can use
-EINVAL for invalid parameters.

- Don't use strict_str functions. Use kstr functions instead (checkpatch should
tell you that, actually).

- Try using dev_ messages as much as possible (instead of pr_)

- Try allocating memory with devm_ functions. This way you can drop the matching
calls to kfree().

- I notice you use kmalloc() a lot. That is ok if you know that you'll
initialize all fields, but it is kind of risky. Better use kzalloc().
(if you start using devm_kzalloc, the issue becomes mostly irrelevant,
as there is no devm_kmalloc).

> I've added documents that explain the QST protocol and also the design
> of the driver.
>
For my part I like the architecture of your driver. Wonder how difficult
it would be to implement the functionality supported by the in-kernel driver
(eg watchdog) with your infrastructure.

Overall it would be great if you and Tomas could get together and come up
with a unified implementation.

Thanks,
Guenter

>
> Unchanged from v1
> -----------------
>
> The driver still uses my MEI implementation. I've taken a look at the
> Intel-written driver in 3.9 and it still has no obvious way to be used
> by another driver, in the same directory or otherwise. The lack of
> documentation may mean I've overlooked something obvious.
>
> The following patch is still required to prevent libsensors from
> ignoring the hwmon directory
>
> diff -ur lm_sensors-3.3.1.org/lib/sysfs.c lm_sensors-3.3.1/lib/sysfs.c
> --- lm_sensors-3.3.1.org/lib/sysfs.c 2011-03-04 20:37:43.000000000 +0000
> +++ lm_sensors-3.3.1/lib/sysfs.c 2012-11-14 21:48:52.144860375 +0000
> @@ -701,6 +701,12 @@
> /* As of kernel 2.6.32, the hid device names don't
> look good */
> entry.chip.bus.nr = bus;
> entry.chip.addr = id;
> + } else
> + if (subsys && !strcmp(subsys, "intel-mei") &&
> + sscanf(dev_name, "mei%d:%d", &bus, &fn) == 2) {
> + entry.chip.bus.type = SENSORS_BUS_TYPE_PCI;
> + entry.chip.bus.nr = bus;
> + entry.chip.addr = fn;
> } else {
> /* Ignore unknown device */
> err = 0;
>
> PWM is still unimplemented.
>
--
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/