Couple of problems I noticed when browsing through the code.Thanks for your helpful comments.
- 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.
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.
- 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.
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.
- Don't use strict_str functions. Use kstr functions instead (checkpatch should
tell you that, actually).
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.
- 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'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 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).
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.I've added documents that explain the QST protocol and also the designFor my part I like the architecture of your driver. Wonder how difficult
of the driver.
it would be to implement the functionality supported by the in-kernel driver
(eg watchdog) with your infrastructure.
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.
Overall it would be great if you and Tomas could get together and come up
with a unified implementation.