Re: RFC (v2): Intel QST sensor driver

[Guenter Roeck <linux@roeck-us.net>]

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