USB virt 2.6 split driver---xenidc platform
PATCH 1/17 USB virt 2.6 split driver---xenidc platform
This patch implements some platform facilities used by the xenidc transport which is in turn used by the USB driver:
o - A callback object for asynchronous completion of requests submitted to services.
o - A work item object for scheduling tasks which builds on the native linux work queues.
o - Some simple trace macros for the xenidc code.
o - An error type which is supposed to be platform independent.
linux-2.6-xen-sparse/arch/xen/Kconfig +8 -0
@@ -38,6 +38,14 @@ (e.g., hard drives, network cards). This allows you to configure such devices and also includes some low-level support that is otherwise not compiled into the kernel. + +config XEN_IDC_TRACE + bool "Inter-domain communication code tracing" + default n + help + This option causes the IDC code to output a continual trace of its + activity. + Say N here unless you are trying to debug this code. config XEN_BLKDEV_BACKEND bool "Block-device backend driver"
linux-2.6-xen-sparse/drivers/xen/Makefile +1 -0
@@ -7,6 +7,7 @@ obj-y += balloon/ obj-y += privcmd/ obj-y += xenbus/ +obj-y += xenidc/ obj-$(CONFIG_XEN_BLKDEV_BACKEND) += blkback/ obj-$(CONFIG_XEN_NETDEV_BACKEND) += netback/
linux-2.6-xen-sparse/drivers/xen/xenidc/Makefile new file +5 -0
@@ -0,0 +1,5 @@ +obj-y += xenidc.o + +xenidc-objs = +xenidc-objs += xenidc_callback.o +xenidc-objs += xenidc_work.o
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +57 -0
57 lines not on this page · raw message
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h new file +56 -0
56 lines not on this page · raw message
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_work.c new file +103 -0
103 lines not on this page · raw message
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +151 -0
151 lines not on this page · raw message
linux-2.6-xen-sparse/include/asm-xen/xenidc_error.h new file +121 -0
121 lines not on this page · raw message
linux-2.6-xen-sparse/include/asm-xen/xenidc_work.h new file +182 -0
182 lines not on this page · raw message
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
I'm going to give every comment once, not everywhere it happens. Please apply as appropriate to all recurring occurences. Also, this is my personal opinion of what Linux code should like like, please feel free to disagree, provided the alternative is just as "Linuxy".
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c
Quoted 4 lines · harry
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +4
-0+/* This program is free software; you can redistribute it and/or modify it */ +/* under the terms of the GNU General Public License as published by the */ +/* Free Software Foundation; either version 2 of the License, or (at your */ +/* option) any later version.
*/
Do we really need the GPL in every source file?
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c line 24
Quoted 2 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +2
-0+#include "xenidc_callback.h" +#include <linux/module.h>
local includes after global includes (and asm after linux)
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c line 37
Quoted 3 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +3
-0+while ((!list_empty (&serialiser->list)) + && (!serialiser->running) + ) {
This should be
while ((!list_empty(&serialiser->list)) && !serialiser->running) {(no paren around a simple expression, fit it all on one line if possible in <80 chars)
Quoted 1 line
+EXPORT_SYMBOL(xenidc_callback_serialiser_function);
EXPORT_SYMBOL_GPL?
Quoted 1 line
+#if (
defined( CONFIG_XEN_IDC_TRACE )||defined( XEN_IDC_TRACE ))
No spaces after and before parens
#if (defined(CONFIG_XEN_IDC_TRACE) || defined(XEN_IDC_TRACE))
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h line 42
Quoted 13 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h new file +13
-0+#define trace0 ( format ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__ ) + +#define trace1 ( format, a0 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0 ) + +#define trace2 ( format, a0, a1 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1 ) + +#define trace3 ( format, a0, a1, a2 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1, a2 ) + +#define trace () trace0( "" )
gcc has variable argument support in macros, please use it.
Quoted 1 line
+#define traceonly( S ) S
What is this for? lose the parens.
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_work.c line 44
Quoted 9 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_work.c new file +9
-0+void xenidc_work_wake_up (void ) +{ +unsigned long flags; + +spin_lock_irqsave (&xenidc_work_list_lock, flags); + +while (!list_empty (&xenidc_work_condition)) { +list_del_init (xenidc_work_condition.next); + }
No need for braces here.
Quoted 1 line
+typedef struct
xenidc_callback_structxenidc_callback;
no typedef for structs.
Quoted 1 line
+#define
XENIDC_CALLBACK_LINKwork.XENIDC_WORK_LINK
Please don't, don't hide structure members and don't name them in all UPPERCASE.
Review
Quoted 2 lines
(quoted diff) +2
-0+static inline void xenidc_callback_init + (xenidc_callback * callback, xenidc_callback_function *
function) {
This hsould be
static inline void xenid_callback_init(struct xenid_callback* callback, xenidc_callback_function* function)
{
{
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h line 123
Quoted 8 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +8
-0+#define XENIDC_CALLBACK_SERIALISER_INIT ( name ) \ +{ \ + SPIN_LOCK_UNLOCKED, \ + LIST_HEAD_INIT( name.list ), \ + XENIDC_WORK_INIT \ + ( name.work, xenidc_callback_serialiser_function, &name ), \ + 0 \ +}
Does this really need a macro? I know a bunch of Linux code does it, but it's always preferable if you can do it as an inline function. Also, what does the SPIN_LOCK_UNLOCKED do there?
Review
Quoted 2 lines
(quoted diff) +2
-0+ list_add_tail + (xenidc_callback_to_link(callback),
&serialiser->list);
This should be
list_add_tail(xenidc_callback_to_link(callback),
&serialiser->list);Quoted 1 line
+typedef s32
xenidc_error;
Why? also, why signed, and why just 32 bits even on 64? does it go over the wire?
Quoted 1 line
+#define
XENIDC_ERROR_SUCCESS( (xenidc_error)0 )
No parens. Also, can you use errno values rather than inventing your own?
Quoted 1 line
+#define
XENIDC_WORK_LINKlink
Why is the renaming necessary?
Quoted 1 line
+int
xenidc_work_schedule(xenidc_work * work);
The '*' should be aligned to the left.
Quoted 48 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_work.h new file +48
-0+/* from a work item and works even when the condition will only be satisfied */ +/* by another work item. */ + +#define xenidc_work_until ( condition ) \ +do \ +{ \ + unsigned long flags; \ + \ + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ + \ + for( ; ; ) \ + { \ + while \ + ( \ + ( !list_empty( &xenidc_work_list ) ) \ + && \ + ( !( condition ) ) \ + ) \ + { \ + xenidc_work * work = list_entry \ + ( \ + xenidc_work_list.next, \ + xenidc_work, \ + link \ + ); \ + \ + list_del_init( &work->link ); \ + \ + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ + \ + xenidc_work_perform_synchronously( work ); \ + \ + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ + } \ + \ + if( condition ) \ + { \ + break; \ + } \ + \ + { \ + struct list_head link; \ + \ + INIT_LIST_HEAD( &link ); \ + \ + list_add_tail( &link, &xenidc_work_condition ); \ + \ + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \
linux-2.6-xen-sparse/include/asm-xen/xenidc_work.h line 175
Quoted 10 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_work.h new file +10
-0+ \ +wait_event ( xenidc_work_waitqueue,list_empty ( &link ) ); \ + \ +spin_lock_irqsave ( &xenidc_work_list_lock, flags ); \ + } \ + } \ + \ +spin_unlock_irqrestore ( &xenidc_work_list_lock, flags ); \ +} \ +while (0 )
Argh! I hate these macros with a passion. We could really use a lambda here, but how about just passing an int (*func)(void* p) instead of the condition and making this into a function? I'm aware of the fact that this is how Linux does it (albeit the macros are not quite this long) but debugging it is awful.
More to come.
Cheers,
Muli
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/
Quoted 11 lines · Muli Ben-Yehuda
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c
Quoted 6 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +4
-0+/* This program is free software; you can redistribute it and/or modify it */ +/* under the terms of the GNU General Public License as published by the */ +/* Free Software Foundation; either version 2 of the License, or (at your */ +/* option) any later version. */
Do we really need the GPL in every source file?
Hi,
i'm not in the position to criticise your code here, but from my single point of view, it does make
sense to include the GPL or whatever license into every single source file. since every single file _could_ be used in other projects, this makes it very clear to every developer
how to handle the 'borrowed' code.
at some projects i was always happy to find some GPL'ed or LGPL'ed code with somewhat trivial but
timeconsumpting functions e.g. multibyte_aes256_crypt . if you're looking for some special function,
there's surely no library for inclusion available :)
anyway, a license note in your source files doesn't harm at all. (well, with some editor you might need
to push the page down button twice...)
greetings
Stephan
Quoted 6 lines · Stephan Seitz
Hi,
i'm not in the position to criticise your code here, but from my single point of view, it does make
sense to include the GPL or whatever license into every single source file.
I have nothing against a single line saying that the file is under the GPL, and see the toplevel license or README or whatever for the exact license. But anything else appears to be to be a waste of good bits :-)
Cheers,
Muli
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/
Quoted 5 lines · Muli Ben-Yehuda
I'm going to give every comment once, not everywhere it happens. Please apply as appropriate to all recurring occurences. Also, this is my personal opinion of what Linux code should like like, please feel free to disagree, provided the alternative is just as "Linuxy".
Thanks, this is really good feedback...
Quoted 10 lines · harry
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c
Quoted 5 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +4
-0+/* This program is free software; you can redistribute it and/or modify it */ +/* under the terms of the GNU General Public License as published by the */ +/* Free Software Foundation; either version 2 of the License, or (at your */ +/* option) any later version. */
Do we really need the GPL in every source file?
I asked my team lead to ask the lawyers what to put at the top of the files. 4 months later I don't have an official reply ;-)
Quoted 5 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c line 24
Quoted 3 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +2
-0+#include "xenidc_callback.h" +#include <linux/module.h> local includes after global includes (and asm after linux)
OK
Quoted 11 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c line 37
Quoted 4 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c new file +3
-0+while ((!list_empty (&serialiser->list)) + && (!serialiser->running) + ) {This should be
while ((!list_empty(&serialiser->list)) && !serialiser->running) {(no paren around a simple expression, fit it all on one line if possible in <80 chars)
Or:
while (!list_empty(&serialiser->list) && !serialiser->running) {
?
Quoted 4 lines
Quoted 2 lines
+EXPORT_SYMBOL(xenidc_callback_serialiser_function);
EXPORT_SYMBOL_GPL?
I don't care. Actually, If the xenidc stuff is going to be really useful we might want to get agreement from all the copyright holders to license it under a different license.
Quoted 5 lines
Quoted 3 lines
+#if (
defined( CONFIG_XEN_IDC_TRACE )||defined( XEN_IDC_TRACE ))No spaces after and before parens
OK. Lindent must have missed this.
Quoted 18 lines
#if (
defined(CONFIG_XEN_IDC_TRACE)||defined(XEN_IDC_TRACE))linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h line 42
Quoted 13 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h new file +13
-0+#define trace0 ( format ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__ ) + +#define trace1 ( format, a0 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0 ) + +#define trace2 ( format, a0, a1 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1 ) + +#define trace3 ( format, a0, a1, a2 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1, a2 ) + +#define trace () trace0( "" )gcc has variable argument support in macros, please use it.
OK
Quoted 4 lines
Quoted 2 lines
+#define traceonly( S ) S
What is this for? lose the parens.
For code which should only be compiled in when tracing is turned on. The parens are required, no?
Quoted 12 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_work.c line 44
Quoted 10 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_work.c new file +9
-0+void xenidc_work_wake_up (void ) +{ +unsigned long flags; + +spin_lock_irqsave (&xenidc_work_list_lock, flags); + +while (!list_empty (&xenidc_work_condition)) { +list_del_init (xenidc_work_condition.next); + }No need for braces here.
OK.
Quoted 4 lines
Quoted 2 lines
+typedef struct
xenidc_callback_structxenidc_callback;no typedef for structs.
OK.
Quoted 5 lines
Quoted 2 lines
+#define
XENIDC_CALLBACK_LINKwork.XENIDC_WORK_LINKPlease don't, don't hide structure members and don't name them in all UPPERCASE.
I didn't like this either. The problem is that the linux list code list_for_each_entry needs to access the link field of the structres on the list being iterated over. If the structures on the list have a public link field which is nested in a member structure then you don't necessarily want to make public which nested structure it is in. So, this is a definition which says that the public link of the callback structure is the one in the work member and this definition can be used in the list_for_each_entry macro. Perhaps there's a better way of doing this without coupling the code using list_for_each_entry to the implementation of the structure with the public link.
Quoted 11 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+static inline void xenidc_callback_init + (xenidc_callback * callback, xenidc_callback_function *function) {
This hsould be
static inline void
xenid_callback_init(struct xenid_callback* callback, xenidc_callback_function* function)
{
OK
Quoted 14 lines
{
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h line 123
Quoted 8 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +8
-0+#define XENIDC_CALLBACK_SERIALISER_INIT ( name ) \ +{ \ + SPIN_LOCK_UNLOCKED, \ + LIST_HEAD_INIT( name.list ), \ + XENIDC_WORK_INIT \ + ( name.work, xenidc_callback_serialiser_function, &name ), \ + 0 \ +}Does this really need a macro? I know a bunch of Linux code does it, but it's always preferable if you can do it as an inline function. Also, what does the
SPIN_LOCK_UNLOCKEDdo there?
This is for initialisation of statics.
i.e.
static xenidc_callback_serialiser fred =XENIDC_CALLBACK_SERIALISER_INIT( fred );
Can't do this with an inline function.
Quoted 9 lines
Review
Quoted 3 lines
(quoted diff) +2
-0+ list_add_tail + (xenidc_callback_to_link(callback),
&serialiser->list);This should be
list_add_tail(xenidc_callback_to_link(callback), &serialiser->list);
OK. Lindent messed up a lot of these.
Quoted 5 lines
Quoted 2 lines
+typedef s32
xenidc_error;Why? also, why signed, and why just 32 bits even on 64? does it go over the wire?
Yes, it goes over the wire. Of all the code in the patches, I think I'm least happy about the error codes.
Quoted 5 lines
Quoted 2 lines
+#define
XENIDC_ERROR_SUCCESS( (xenidc_error)0 )No parens. Also, can you use errno values rather than inventing your own?
Maybe. The interdomain error codes are communicated potentially between different operating systems. Making them look obviously different might be an advantage.
Quoted 4 lines
Quoted 2 lines
+#define
XENIDC_WORK_LINKlinkWhy is the renaming necessary?
This is the public link of this structure for use in list_for_each_entry.
Quoted 4 lines
Quoted 2 lines
+int
xenidc_work_schedule(xenidc_work * work);The '*' should be aligned to the left.
OK
Quoted 57 lines
Quoted 57 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_work.h new file +56
-0+/* from a work item and works even when the condition will only be satisfied */ +/* by another work item. */ + +#define xenidc_work_until ( condition ) \ +do \ +{ \ + unsigned long flags; \ + \ + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ + \ + for( ; ; ) \ + { \ + while \ + ( \ + ( !list_empty( &xenidc_work_list ) ) \ + && \ + ( !( condition ) ) \ + ) \ + { \ + xenidc_work * work = list_entry \ + ( \ + xenidc_work_list.next, \ + xenidc_work, \ + link \ + ); \ + \ + list_del_init( &work->link ); \ + \ + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ + \ + xenidc_work_perform_synchronously( work ); \ + \ + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ + } \ + \ + if( condition ) \ + { \ + break; \ + } \ + \ + { \ + struct list_head link; \ + \ + INIT_LIST_HEAD( &link ); \ + \ + list_add_tail( &link, &xenidc_work_condition ); \ + \ + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \ + \ + wait_event( xenidc_work_waitqueue, list_empty( &link ) ); \ + \ + spin_lock_irqsave( &xenidc_work_list_lock, flags ); \ + } \ + } \ + \ + spin_unlock_irqrestore( &xenidc_work_list_lock, flags ); \
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +2
-0+} \ +while( 0 )Argh! I hate these macros with a passion. We could really use a lambda here, but how about just passing an int (*func)(void* p) instead of the condition and making this into a function? I'm aware of the fact that this is how Linux does it (albeit the macros are not quite this long) but debugging it is awful.
Yeah, this code sucks. I would have preferred if the underlying Linux code worked differently. I could do the function pointer thing but it makes the client code harder to write.
Quoted 5 lines
More to come.
Cheers,
Muli
Signature · 1 line
Harry Butterworth <[email protected]>
Quoted 8 lines · Harry Butterworth
Quoted 5 lines · Muli Ben-Yehuda
I'm going to give every comment once, not everywhere it happens. Please apply as appropriate to all recurring occurences. Also, this is my personal opinion of what Linux code should like like, please feel free to disagree, provided the alternative is just as "Linuxy".
Thanks, this is really good feedback...
You're welcome.
Quoted 1 line
while (!list_empty(&serialiser->list) && !serialiser->running) {
Even better.
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
+EXPORT_SYMBOL(xenidc_callback_serialiser_function);
EXPORT_SYMBOL_GPL?I don't care. Actually, If the xenidc stuff is going to be really useful we might want to get agreement from all the copyright holders to license it under a different license.
Errr... I'm of the opinion that any linux kernel code had to be GPL'd, but IANAL nor do I play one on TV.
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+#define traceonly( S ) S
What is this for? lose the parens.
For code which should only be compiled in when tracing is turned on. The parens are required, no?
My mistake, I meant lose the spaces inside the parens.
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +4
-0+#define XENIDC_CALLBACK_SERIALISER_INIT ( name ) \ +{ \ + SPIN_LOCK_UNLOCKED, \ + LIST_HEAD_INIT( name.list ), \Quoted 8 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h line 123
Quoted 4 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +4
-0+XENIDC_WORK_INIT \ + ( name.work, xenidc_callback_serialiser_function, &name ), \ + 0 \ +}Does this really need a macro? I know a bunch of Linux code does it, but it's always preferable if you can do it as an inline function. Also, what does the
SPIN_LOCK_UNLOCKEDdo there?
Quoted 8 lines
This is for initialisation of statics.
i.e.
static
xenidc_callback_serialiserfred =XENIDC_CALLBACK_SERIALISER_INIT( fred );Can't do this with an inline function.
ok, in that case, it would've been clearer to use C99 structure initializers, e.g.
#define XENIDC_CALLBACK_SERIALISER_INIT(name) {
.foo = SPIN_LOCK_UNLOCKED,
...
}
etc.
Quoted 1 line
Quoted 1 line
More to come.
FWIW, I plan to wait for an updated version that fixes the superficial stuff and then get down to actually reviewing the xenidc code.
Cheers,
Muli
Signature · 2 lines
Muli Ben-Yehuda http://www.mulix.org | http://mulix.livejournal.com/
Quoted 18 lines · Muli Ben-Yehuda
Quoted 8 lines · Harry Butterworth
Quoted 5 lines · Muli Ben-Yehuda
I'm going to give every comment once, not everywhere it happens. Please apply as appropriate to all recurring occurences. Also, this is my personal opinion of what Linux code should like like, please feel free to disagree, provided the alternative is just as "Linuxy".
Thanks, this is really good feedback...
You're welcome.
Quoted 1 line
while (!list_empty(&serialiser->list) && !serialiser->running) {
Even better.
Quoted 2 lines
Quoted 2 lines
Quoted 1 line
+EXPORT_SYMBOL(xenidc_callback_serialiser_function);
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
EXPORT_SYMBOL_GPL?I don't care. Actually, If the xenidc stuff is going to be really useful we might want to get agreement from all the copyright holders to license it under a different license.
Errr... I'm of the opinion that any linux kernel code had to be GPL'd, but IANAL nor do I play one on TV.
I think that Linux kernel code has to be GPL compatible but if it's not a derived work of Linux then it doesn't have to be GPL.
I didn't derive the xenidc code from Linux and it's intended to be useful for use in other OSs too so there's a possibility it could be licensed differently. I'd have to get approval for my parts and the other copyright holders would have to agree.
IANAL either.
Quoted 9 lines
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
+#define traceonly( S ) S
What is this for? lose the parens.
For code which should only be compiled in when tracing is turned on. The parens are required, no?
My mistake, I meant lose the spaces inside the parens.
OK
Quoted 23 lines
Quoted 14 lines
Quoted 13 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h line 123
Quoted 9 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +8
-0+#define XENIDC_CALLBACK_SERIALISER_INIT ( name ) \ +{ \ + SPIN_LOCK_UNLOCKED, \ + LIST_HEAD_INIT( name.list ), \ + XENIDC_WORK_INIT \ + ( name.work, xenidc_callback_serialiser_function, &name ), \ + 0 \ +}Does this really need a macro? I know a bunch of Linux code does it, but it's always preferable if you can do it as an inline function. Also, what does the
SPIN_LOCK_UNLOCKEDdo there?Quoted 8 lines
This is for initialisation of statics.
i.e.
static
xenidc_callback_serialiserfred =XENIDC_CALLBACK_SERIALISER_INIT( fred );Can't do this with an inline function.
Quoted 7 lines
ok, in that case, it would've been clearer to use C99 structure initializers, e.g.
#define
XENIDC_CALLBACK_SERIALISER_INIT(name){
.foo =SPIN_LOCK_UNLOCKED,
...
}
OK
Quoted 10 lines
etc.
Quoted 1 line
Quoted 1 line
More to come.
FWIW, I plan to wait for an updated version that fixes the superficial stuff and then get down to actually reviewing the xenidc code.
Cheers,
Muli
Quoted 2 lines
I think that Linux kernel code has to be GPL compatible but if it's not a derived work of Linux then it doesn't have to be GPL.
I agree. Not all the code in Linux is GPL anyhow, there's some BSD-licensed code in there.
Quoted 4 lines
I didn't derive the xenidc code from Linux and it's intended to be useful for use in other OSs too so there's a possibility it could be licensed differently. I'd have to get approval for my parts and the other copyright holders would have to agree.
If you haven't used GPL code that other people have copyright on, then it's not a derived work in my IANAL opition. If you have, you just need their consent. We did this for the frontend drivers, amongst other stuff, to make it easier for BSD ports to pull in code.
Cheers,
Mark
Quoted 59 lines
IANAL either.
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
+#define traceonly( S ) S
What is this for? lose the parens.
For code which should only be compiled in when tracing is turned on. The parens are required, no?
My mistake, I meant lose the spaces inside the parens.
OK
Quoted 22 lines
Quoted 13 lines
Quoted 4 lines
Quoted 4 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +4
-0+#define XENIDC_CALLBACK_SERIALISER_INIT ( name ) \ +{ \ + SPIN_LOCK_UNLOCKED, \ + LIST_HEAD_INIT( name.list ), \Quoted 8 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h line 123
Quoted 4 lines
linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h new file +4
-0+XENIDC_WORK_INIT \ + ( name.work, xenidc_callback_serialiser_function, &name ), \ + 0 \ +}Does this really need a macro? I know a bunch of Linux code does it, but it's always preferable if you can do it as an inline function. Also, what does the
SPIN_LOCK_UNLOCKEDdo there?Quoted 8 lines
This is for initialisation of statics.
i.e.
static
xenidc_callback_serialiserfred =XENIDC_CALLBACK_SERIALISER_INIT( fred );Can't do this with an inline function.
Quoted 7 lines
ok, in that case, it would've been clearer to use C99 structure initializers, e.g.
#define
XENIDC_CALLBACK_SERIALISER_INIT(name){
.foo =SPIN_LOCK_UNLOCKED,
...
}OK
Quoted 9 lines
etc.
Quoted 1 line
Quoted 1 line
More to come.
FWIW, I plan to wait for an updated version that fixes the superficial stuff and then get down to actually reviewing the xenidc code.
Cheers,
Muli_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 18 lines · Muli Ben-Yehuda
I'm going to give every comment once, not everywhere it happens. Please apply as appropriate to all recurring occurences. Also, this is my personal opinion of what Linux code should like like, please feel free to disagree, provided the alternative is just as "Linuxy".
Quoted 9 lines · harry
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h new file +9
-0+#define trace0 ( format ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__ ) + +#define trace1 ( format, a0 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0 ) + +#define trace2 ( format, a0, a1 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1 ) +
Quoted 10 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h line 42
Quoted 6 lines
linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h new file +4
-0+#define trace3 ( format, a0, a1, a2 ) \ +printk( KERN_INFO "xenidc %s:" format "\n", __PRETTY_FUNCTION__, a0, a1, a2 ) + +#define trace () trace0( "" )gcc has variable argument support in macros, please use it.
Please use the C99 version instead of the GCC version. That would be:
#define trace(format, ...) printk(KERN_INFO "xenidc %s: " format "\n", __FUNCTION__, ## __VA_ARGS__)
The '##' is technically a GCC extension but the old style is deprecated anyway :-)
Regards,
Anthony Liguori