RFC (v2): Intel QST sensor driver

RFC (v2): Intel QST sensor driver

[Simon J. Rowe]

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.

I've added documents that explain the QST protocol and also the design
of the driver.


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.

Re: RFC (v2): Intel QST sensor driver

[Guenter Roeck]

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

Re: RFC (v2): Intel QST sensor driver

[Simon J. Rowe]

On 19/03/13 00:27, Guenter Roeck wrote:
> 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.
Thanks for your helpful comments.

In some of the low-level code I decided to use return 0 to indicate 
nothing was transmitted. Probably these situations should be regarded as 
an error and -EAGAIN used. I'll check them and fix this.
>
> - 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.
I'd noticed -EIO was used quite a bit in some existing modules (e.g. 
abitguru3.ko) and thought this was a general convention. I'll switch to 
using the original return codes.
>
> - Don't use strict_str functions. Use kstr functions instead (checkpatch should
>    tell you that, actually).
Ah, I'd run checkpatch on my dev box (which runs 2.6.39), the newer 
source trees do indeed flag this up, I'll fix it.
>
> - 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().
The client objects don't contain a struct device. Multiple clients have 
a pointer to the underlying supporting device but from what I understand 
of devm_kzalloc() that would defer freeing memory until the device is 
shut down (which only happens on module unload). That could leave an 
increasing amount of memory tied up.
>
> - 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'd avoided using kzalloc() when I knew I'd need to initialize members, 
but none of the code is on a hot path and it avoids oversights when new 
members get added.
>> 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.
The MEI watchdog? that would be quite straightforward to create a module 
for. I had planned to write one but didn't have access to any hardware 
with this function.
>
> Overall it would be great if you and Tomas could get together and come up
> with a unified implementation.
>
>
I'd be happy to help getting a driver that fits everybody's needs. The 
difficult is there are slight differences in approach. From what I can 
see from the QST SDK the kernel driver was written to provide a minimal 
implementation with the majority of the logic in a cross-platform 
userspace library. My driver was aimed at providing a base to make it 
easy to write other kernel modules like the QST one.

There's no reason why an adaptation layer that provides the same 
ioctl()/dev interface as the current Intel driver couldn't be created.

Simon

Re: RFC (v2): Intel QST sensor driver

[Guenter Roeck]

On Tue, Mar 19, 2013 at 09:46:43PM +0000, Simon J. Rowe wrote:
> On 19/03/13 00:27, Guenter Roeck wrote:
> >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.
> Thanks for your helpful comments.
> 
> In some of the low-level code I decided to use return 0 to indicate
> nothing was transmitted. Probably these situations should be
> regarded as an error and -EAGAIN used. I'll check them and fix this.

Code such as

	if (ret <= 0) {
		pr_err(...);
		goto out;
	}

does look like an error, though. If it isn't, there should be no error message.

And if it is a function such as qst_get_mon_config(), which ends up printing
the content of the non-retrieved message, it most definitely looks wrong.

The same really applies to each case I have noticed.

> >
> >- 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.
> I'd noticed -EIO was used quite a bit in some existing modules (e.g.
> abitguru3.ko) and thought this was a general convention. I'll switch
> to using the original return codes.

Other drivers doing the same bad thing doesn't mean you should do it as well :).

Just think about the best error to use. For example, you use -EINVAL if
wait_event_interruptible_timeout times out. That should really, at least most
likely, be -ETIMEDOUT. Actually those calls are problematic anyway, as they 
are interruptable and might leave the mei device in an undefined state.
Can that happen ?

> >
> >- Don't use strict_str functions. Use kstr functions instead (checkpatch should
> >   tell you that, actually).
> Ah, I'd run checkpatch on my dev box (which runs 2.6.39), the newer
> source trees do indeed flag this up, I'll fix it.
> >
> >- 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().
> The client objects don't contain a struct device. Multiple clients
> have a pointer to the underlying supporting device but from what I
> understand of devm_kzalloc() that would defer freeing memory until
> the device is shut down (which only happens on module unload). That
> could leave an increasing amount of memory tied up.

Ok, makes sense.

> >
> >- 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'd avoided using kzalloc() when I knew I'd need to initialize
> members, but none of the code is on a hot path and it avoids
> oversights when new members get added.

Hmm .. usually one initializes the structure to zero to avoid unpredictable
behavior. The argument for zeroing out a data structure is pretty much the
same as yours for not zeroing it out.

With your approach you hope to get either a compiler error or some
unpredictable behavior / crash if you forget to initialize an element.
I don't think that is such a good idea.

> >>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.
> The MEI watchdog? that would be quite straightforward to create a
> module for. I had planned to write one but didn't have access to any
> hardware with this function.
> >
> >Overall it would be great if you and Tomas could get together and come up
> >with a unified implementation.
> >
> >
> I'd be happy to help getting a driver that fits everybody's needs.
> The difficult is there are slight differences in approach. From what
> I can see from the QST SDK the kernel driver was written to provide
> a minimal implementation with the majority of the logic in a
> cross-platform userspace library. My driver was aimed at providing a
> base to make it easy to write other kernel modules like the QST one.
> 
> There's no reason why an adaptation layer that provides the same
> ioctl()/dev interface as the current Intel driver couldn't be
> created.
> 
The kernel community in general prefers an incremental approach, where an
existing driver is not replaced but continually improved. I don't know what the
best approach is here, but you'll need to find a way to introduce your code
in a non-disruptive way.

Couple of additional comments.

return; at the end of a void function is really not necessary.

There are several functions which can get an error from underlying code, display
an error to the log, yet return void. Maybe there is a good reason for that, but
it would be better to pass errors to calling code whenever possible.

In some cases, such as mei_attach_client(), it definitely looks wrong that
errors are just ignored - especially since the calling code does have a return
value.

It is also a bit odd that functions like handle_ver_rsp() report an error to
the log, yet return no indication to the caller about it. Anything resulting
in a pr_err log message should really be reported to the caller, or it is not
an error.

Thanks,
Guenter