On 02/05/2014 04:33 PM, Mario Kicherer wrote:
Hi Daniel,
thank you very much for your comments! I hope I fixed most of the points but I also have a few questions/comments:
On 05.02.2014 11:54, Daniel Mack wrote:
+#ifdef CONFIG_USB_DEBUG +#define DEBUG 1 +#else +#define DEBUG 0 +#endif
Please use the define directly instead of introducing a new one.
I replaced it with:
#ifndef CONFIG_USB_DEBUG #define CONFIG_USB_DEBUG 0 #endif
No, don't fiddle with CONFIG_* symbols. What I mean is: use the CONFIG_ macro in your code instead if 'DEBUG'. And also, maybe use CONFIG_SND_DEBUG instead.
As it can be undefined. I hope that's okay.
Ah, so I'd recommend you move code that depends on this define into a separate function, and stub it out with a no-op if it's not defined. Something like this:
#ifdef CONFIG_SND_DEBUG static void foo_dump() { ... } #else static inline void foo_dump() {} #endif
... { ... foo_dump(); ... }
It makes to code more readable if there are less #ifdef.
Your patch has a huge number of style issues, which scripts/checkpatch.pl will point you to. I got:
Interesting, I copied many issues from other modules. :)
You're welcome to send patches to fix them :) A general rule is: "Do as we say, not as we do".
If fixed most of them except these:
WARNING: quoted string split across lines #310: FILE: sound/usb/bcd2000/bcd2000.c:254:
"bcd2000_midi_send(%p): usb_submit_urb() failed""ret=%d, len=%d\n", substream, ret, len);WARNING: quoted string split across lines #392: FILE: sound/usb/bcd2000/bcd2000.c:336:
"bcd2000_init_device: usb_submit_urb()failed,"
"ret=%d: ", ret);I don't know how to avoid them, as all strings in one line would be longer than 80 characters.
Yep, I'd say that's okay. I'd prefer non-split string over the 80-char rule. However, consider using __func__ rather than the manual function name copy; that might solve your problem as well.
/* determine the number of bytes we want to copy this time */tocopy = min( 3 - bcd2k->midi_cmd_offset, length - (buf_offset - 1));if (tocopy > 0) {tocopy is unsigned char, so this check is always true, and the copy routine can overflow in case bcd2k->midi_cmd_offset < 3.
I don't understand your point here. It could be zero.
Yes, I'm sorry. I was looking at the '< 0' condition that can occur if 'bcd2k->midi_cmd_offset < 3'.
But I changed it to a condition that checks if both offsets are within the length of the two involved buffers.
Great. Please also double-check whether a maliciously behaving device could cause any harm to your buffer logic. It's easy to hack a simple microcontroller to match the driver and then through random stuff at your routines. Your code must be able to cope with everything it is fed to.
Please, no dead code. Remove those lines entirely if you don't need them.
Ah okay, I planned to reactive them with the audio part. But I removed them for now.
As far as I understood, I should repost the new patch as [PATCH v2].
Correct.
I will check this evening if the device still works and send it to the list afterwards.
Thanks, Daniel