Re: RFC (v2): Intel QST sensor driver
On Tue, Mar 19, 2013 at 09:46:43PM +, 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.
Re: RFC (v2): Intel QST sensor driver
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: RFC (v2): Intel QST sensor driver
Hi Simon,
On Mon, Mar 18, 2013 at 09:20:53PM +, 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.0 +
> +++ lm_sensors-3.3.1/lib/sysfs.c2012-11-14 21:48:52.144860375 +
> @@ -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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RFC (v2): Intel QST sensor driver
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.0 +
+++ lm_sensors-3.3.1/lib/sysfs.c2012-11-14 21:48:52.144860375 +
@@ -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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
