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