Re: RFC (v2): Intel QST sensor driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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

_______________________________________________
lm-sensors mailing list
lm-sensors@xxxxxxxxxxxxxx
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors


[Index of Archives]     [Linux Kernel]     [Linux Hardware Monitoring]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux