* [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
@ 2005-11-21 13:18 harry
2005-11-21 20:10 ` Muli Ben-Yehuda
0 siblings, 1 reply; 9+ messages in thread
From: harry @ 2005-11-21 13:18 UTC (permalink / raw)
To: xen-devel
[-- Attachment #1: Type: text/plain, Size: 464 bytes --]
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.
Signed-off-by: Harry Butterworth <butterwo@uk.ibm.com>
[-- Attachment #2: p1-xenidc-platform.patch --]
[-- Type: text/x-patch, Size: 33219 bytes --]
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/arch/xen/Kconfig
--- a/linux-2.6-xen-sparse/arch/xen/Kconfig Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/arch/xen/Kconfig Sun Nov 20 14:53:27 2005
@@ -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"
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/drivers/xen/Makefile
--- a/linux-2.6-xen-sparse/drivers/xen/Makefile Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/Makefile Sun Nov 20 14:53:27 2005
@@ -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/
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/drivers/xen/xenidc/Makefile
--- /dev/null Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenidc/Makefile Sun Nov 20 14:53:27 2005
@@ -0,0 +1,5 @@
+obj-y += xenidc.o
+
+xenidc-objs =
+xenidc-objs += xenidc_callback.o
+xenidc-objs += xenidc_work.o
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c
--- /dev/null Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_callback.c Sun Nov 20 14:53:27 2005
@@ -0,0 +1,57 @@
+/*****************************************************************************/
+/* A callback object for use in scheduling completion of asynchronous */
+/* requests. */
+/* */
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* 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. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+
+#include "xenidc_callback.h"
+#include <linux/module.h>
+
+void xenidc_callback_serialiser_function(void *context)
+{
+ xenidc_callback_serialiser *serialiser =
+ (xenidc_callback_serialiser *) context;
+
+ unsigned long flags;
+
+ spin_lock_irqsave(&serialiser->lock, flags);
+
+ while ((!list_empty(&serialiser->list))
+ && (!serialiser->running)
+ ) {
+ xenidc_callback *callback =
+ xenidc_callback_link_to(serialiser->list.next);
+
+ list_del_init(xenidc_callback_to_link(callback));
+
+ serialiser->running = 1;
+
+ spin_unlock_irqrestore(&serialiser->lock, flags);
+
+ xenidc_callback_complete_synchronously(callback);
+
+ spin_lock_irqsave(&serialiser->lock, flags);
+
+ serialiser->running = 0;
+ }
+
+ spin_unlock_irqrestore(&serialiser->lock, flags);
+}
+
+EXPORT_SYMBOL(xenidc_callback_serialiser_function);
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h
--- /dev/null Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_trace.h Sun Nov 20 14:53:27 2005
@@ -0,0 +1,56 @@
+/*****************************************************************************/
+/* Simple trace macros. */
+/* */
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* 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. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+
+#ifndef XENIDC_TRACE_H
+#define XENIDC_TRACE_H
+
+#include <linux/config.h>
+#include <linux/kernel.h>
+
+#if ( defined( CONFIG_XEN_IDC_TRACE ) || defined( XEN_IDC_TRACE ) )
+
+#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( "" )
+
+#define traceonly( S ) S
+
+#else
+
+#define trace0( format )
+#define trace1( format,a0 )
+#define trace2( format,a0, a1 )
+#define trace3( format,a0, a1, a2 )
+#define trace()
+#define traceonly( S )
+#endif
+
+#endif
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_work.c
--- /dev/null Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/drivers/xen/xenidc/xenidc_work.c Sun Nov 20 14:53:27 2005
@@ -0,0 +1,103 @@
+/*****************************************************************************/
+/* Enhanced work queue service */
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* 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. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+
+#include <linux/module.h>
+#include "xenidc_work.h"
+
+DEFINE_SPINLOCK(xenidc_work_list_lock);
+
+LIST_HEAD(xenidc_work_list);
+
+static void xenidc_work_function(void *ignored);
+
+DECLARE_WORK(xenidc_work_work, xenidc_work_function, NULL);
+
+DECLARE_WAIT_QUEUE_HEAD(xenidc_work_waitqueue);
+
+LIST_HEAD(xenidc_work_condition);
+
+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);
+ }
+
+ spin_unlock_irqrestore(&xenidc_work_list_lock, flags);
+
+ wake_up(&xenidc_work_waitqueue);
+}
+
+int xenidc_work_schedule(xenidc_work * work)
+{
+ int scheduled = 0;
+
+ unsigned long flags;
+
+ spin_lock_irqsave(&xenidc_work_list_lock, flags);
+
+ if (list_empty(&work->link)) {
+ list_add_tail(&work->link, &xenidc_work_list);
+
+ scheduled = 1;
+ }
+
+ spin_unlock_irqrestore(&xenidc_work_list_lock, flags);
+
+ if (scheduled) {
+ xenidc_work_wake_up();
+
+ schedule_work(&xenidc_work_work);
+ }
+
+ return scheduled;
+}
+
+static void xenidc_work_function(void *ignored)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&xenidc_work_list_lock, flags);
+
+ while (!list_empty(&xenidc_work_list)) {
+ 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);
+ }
+
+ spin_unlock_irqrestore(&xenidc_work_list_lock, flags);
+}
+
+EXPORT_SYMBOL(xenidc_work_schedule);
+EXPORT_SYMBOL(xenidc_work_list);
+EXPORT_SYMBOL(xenidc_work_condition);
+EXPORT_SYMBOL(xenidc_work_waitqueue);
+EXPORT_SYMBOL(xenidc_work_wake_up);
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h
--- /dev/null Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/include/asm-xen/xenidc_callback.h Sun Nov 20 14:53:27 2005
@@ -0,0 +1,151 @@
+/*****************************************************************************/
+/* A callback object for use in scheduling completion of asynchronous */
+/* requests. */
+/* */
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* 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. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+
+#ifndef XENIDC_CALLBACK_H
+#define XENIDC_CALLBACK_H
+
+#include <linux/spinlock.h>
+#include "xenidc_error.h"
+#include "xenidc_work.h"
+
+/* Service parameter blocks contain callbacks for asynchronous completions. */
+
+typedef struct xenidc_callback_struct xenidc_callback;
+
+struct xenidc_callback_struct {
+ xenidc_work work;
+ xenidc_error error;
+};
+
+#define XENIDC_CALLBACK_LINK work.XENIDC_WORK_LINK
+
+/* Client of service initialises callback with its callback function. */
+
+typedef void (xenidc_callback_function) (xenidc_callback * callback);
+
+static inline void xenidc_callback_init
+ (xenidc_callback * callback, xenidc_callback_function * function) {
+ xenidc_work_init(&callback->work, (void (*)(void *))function, callback);
+
+ callback->error = XENIDC_ERROR_SUCCESS;
+}
+
+/* Client may use link whilst it owns parameter block. Service may use link */
+/* whilst it owns parameter block. Link is reserved whilst callback is */
+/* scheduled for completion. */
+
+static inline struct list_head *xenidc_callback_to_link
+ (xenidc_callback * callback) {
+ return xenidc_work_to_link(&callback->work);
+}
+
+/* Cast back from contained link of callback to callback. */
+
+static inline xenidc_callback *xenidc_callback_link_to(struct list_head *link) {
+ return container_of(xenidc_work_link_to(link), xenidc_callback, work);
+}
+
+/* Service which completes requests concurrently may call */
+/* xenidc_callback_complete or xenidc_callback_success to complete the */
+/* callback. */
+
+static inline void xenidc_callback_complete
+ (xenidc_callback * callback, xenidc_error error) {
+ callback->error = error;
+
+ xenidc_work_schedule(&callback->work);
+}
+
+static inline void xenidc_callback_success(xenidc_callback * callback) {
+ xenidc_callback_complete(callback, 0);
+}
+
+/* These functions used by serialiser below. */
+
+static inline void xenidc_callback_set_error
+ (xenidc_callback * callback, xenidc_error error) {
+ callback->error = error;
+}
+
+static inline void xenidc_callback_complete_synchronously
+ (xenidc_callback * callback) {
+ xenidc_work_perform_synchronously(&callback->work);
+}
+
+/* When callback completes, client may call xenidc_callback_query_error to */
+/* get the error code. */
+
+static inline xenidc_error xenidc_callback_query_error
+ (xenidc_callback * callback) {
+ return callback->error;
+}
+
+/* Services which must serialise completions to preserve submission order */
+/* can use one of these. */
+
+typedef struct xenidc_callback_serialiser_struct xenidc_callback_serialiser;
+
+struct xenidc_callback_serialiser_struct {
+ spinlock_t lock;
+ struct list_head list;
+ xenidc_work work;
+ int running;
+};
+
+void xenidc_callback_serialiser_function(void *context);
+
+#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 \
+}
+
+#define XENIDC_CALLBACK_SERIALISER( name ) \
+xenidc_callback_serialiser name = \
+ XENIDC_CALLBACK_SERIALISER_INIT( name )
+
+/* The service completes the callback to the serialiser which serialises the */
+/* completions to the client, performing them in submission order. */
+
+static inline void xenidc_callback_serialiser_complete_callback
+ (xenidc_callback_serialiser * serialiser,
+ xenidc_callback * callback, xenidc_error error) {
+ xenidc_callback_set_error(callback, error);
+
+ {
+ unsigned long flags;
+
+ spin_lock_irqsave(&serialiser->lock, flags);
+
+ list_add_tail
+ (xenidc_callback_to_link(callback), &serialiser->list);
+
+ spin_unlock_irqrestore(&serialiser->lock, flags);
+ }
+
+ xenidc_work_schedule(&serialiser->work);
+}
+
+#endif
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/include/asm-xen/xenidc_error.h
--- /dev/null Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/include/asm-xen/xenidc_error.h Sun Nov 20 14:53:27 2005
@@ -0,0 +1,121 @@
+/*****************************************************************************/
+/* The xenidc_error type is supposed to be safe for use on the wire between */
+/* different operating systems which may use different values for the posix */
+/* error types or may not even use the posix error types at all. */
+/* */
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* 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. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+/* */
+/* Ideally, we'd simply define all the error values we needed here and map */
+/* the local error values to those and back again. In practice, we are */
+/* building on top of another operating system which has its own set of */
+/* error values and driver interfaces which don't define precisely which */
+/* ones may be returned under what conditions. */
+/* */
+/* Some error return values are significant for the operation of the */
+/* inter-domain protocols and some are not. In general, the correct mapping */
+/* of protocol-significant error values between operating systems may be a */
+/* function of the protocol. */
+/* */
+/* The strategy adopted here is to allow individual protocols to extract the */
+/* error return values which are significant to them and map them into a */
+/* range of error values reserved for protocol use. Any values not */
+/* specifically significant to a protocol are handled by the fallback */
+/* mapping code in this file. */
+/* */
+/* This makes it possible to construct a mapping as a function of protocol */
+/* and allows us to avoid discarding potentially useful error code */
+/* information in the general case. */
+/* */
+/* For the fallback case of any error codes we are not specifically */
+/* interested in for a protocol-specific purpose we use the linux definition */
+/* of the posix error values. Other operating systems will need their own */
+/* equivalent of this file to map their error values to and from these. */
+
+#ifndef __ASM_XEN_XENIDC_ERROR_H__
+#define __ASM_XEN_XENIDC_ERROR_H__
+
+#include <asm/types.h>
+#include <linux/errno.h>
+
+typedef s32 xenidc_error;
+
+/* The error values we single out with some OS independent, protocol */
+/* specific meaning are all positive. The negative ones are all potentially */
+/* OS specific and might not have an exact representation in the local OS. */
+/* Assuming we have singled out the right ones in the protocol specific code */
+/* this shouldn't matter. */
+
+/* The first few error numbers are reserved for success and transport errors */
+/* which are common to all IDC protocols. */
+
+#define XENIDC_ERROR_SUCCESS ( (xenidc_error)0 )
+#define XENIDC_ERROR_FAILURE ( (xenidc_error)1 )
+#define XENIDC_ERROR_DISCONNECT ( (xenidc_error)2 )
+/* Unexpected transaction/transaction in wrong sequence, underlength etc: */
+#define XENIDC_ERROR_INVALID_PROTOCOL ( (xenidc_error)3 )
+/* Parameter value wrong: */
+#define XENIDC_ERROR_INVALID_PARAMETER ( (xenidc_error)4 )
+/* Something about a request exceeded a hard-coded limit: */
+#define XENIDC_ERROR_TOO_BIG ( (xenidc_error)5 )
+
+/* Protocols can define their own set of protocol specific errors starting */
+/* at this one. The driver code on either side must translate between the */
+/* local OS specific error codes and the protocol specific error codes. */
+
+#define XENIDC_ERROR_PROTOCOL_SPECIFIC_FIRST ( (xenidc_error)1024 )
+
+/* These functions map between the wire error type and the local error type. */
+
+static inline xenidc_error xenidc_error_map_local_to(int error)
+{
+ switch (error) {
+ case 0:
+ return XENIDC_ERROR_SUCCESS;
+ case -ENOTCONN:
+ return XENIDC_ERROR_DISCONNECT;
+ case -EPROTO:
+ return XENIDC_ERROR_INVALID_PROTOCOL;
+ case -EINVAL:
+ return XENIDC_ERROR_INVALID_PARAMETER;
+ case -E2BIG:
+ return XENIDC_ERROR_TOO_BIG;
+ default:
+ return (xenidc_error)error;
+ }
+}
+
+static inline int xenidc_error_map_to_local(xenidc_error error)
+{
+ switch (error) {
+ case XENIDC_ERROR_SUCCESS:
+ return 0;
+ case XENIDC_ERROR_DISCONNECT:
+ return -ENOTCONN;
+ case XENIDC_ERROR_INVALID_PROTOCOL:
+ return -EPROTO;
+ case XENIDC_ERROR_INVALID_PARAMETER:
+ return -EINVAL;
+ case XENIDC_ERROR_TOO_BIG:
+ return -E2BIG;
+ default:
+ return (int)error;
+ }
+}
+
+#endif
diff -r 6a666940fa04 -r 7adcceaaf851 linux-2.6-xen-sparse/include/asm-xen/xenidc_work.h
--- /dev/null Sun Nov 20 09:19:38 2005
+++ b/linux-2.6-xen-sparse/include/asm-xen/xenidc_work.h Sun Nov 20 14:53:27 2005
@@ -0,0 +1,182 @@
+/*****************************************************************************/
+/* Enhanced work queue service which allows work items to wait for */
+/* conditions met by other work items. */
+/* */
+/* Copyright (c) 2005 Harry Butterworth IBM Corporation */
+/* */
+/* 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. */
+/* */
+/* This program is distributed in the hope that it will be useful, but */
+/* WITHOUT ANY WARRANTY; without even the implied warranty of */
+/* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU General */
+/* Public License for more details. */
+/* */
+/* You should have received a copy of the GNU General Public License along */
+/* with this program; if not, write to the Free Software Foundation, Inc., */
+/* 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA */
+/* */
+/*****************************************************************************/
+
+/* This work queue service is a lot like the linux work queue service except */
+/* that it provides the interface xenidc_work_until which allows the client */
+/* to wait for a condition to be met even when the client is executing on a */
+/* work thread and the condition will only be satisfied by execution of */
+/* another work item. */
+/* */
+/* The other significant difference is that xenidc_work work items are run */
+/* by multiple threads (threads waiting in xenidc_work_until() run */
+/* work_items until their condition is met) so it's possible for a work item */
+/* to be executed multiple times concurrently and clients must use locking */
+/* to protect against this where necessary. */
+
+#ifndef XENIDC_WORK_H
+#define XENIDC_WORK_H
+
+#include <linux/list.h>
+#include <linux/sched.h>
+#include <linux/spinlock.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+/* Clients allocate xenidc_work work items to use the service. */
+
+typedef struct xenidc_work_struct xenidc_work;
+
+struct xenidc_work_struct {
+ struct list_head link;
+ void (*function) (void *context);
+ void *context;
+};
+
+/* The xenidc_work object contains a link for use by the service whist the */
+/* work item is scheduled. The link may be used by the client whilst the */
+/* work item is not scheduled. The link is initialised by the init function */
+/* and must be left initialised if used by the client. */
+
+#define XENIDC_WORK_LINK link
+
+/* Macro for initialisation of static xenidc_work objects. */
+
+#define XENIDC_WORK_INIT( name, fn, ctx ) \
+{ LIST_HEAD_INIT( name.link ), fn, ctx }
+
+/* Macro to declare a static xenidc_work object. */
+
+#define XENIDC_WORK( name, fn, ctx ) \
+xenidc_work name = XENIDC_WORK_INIT( name, fn, ctx )
+
+static inline void xenidc_work_init
+ (xenidc_work * work, void (*function) (void *context), void *context) {
+ INIT_LIST_HEAD(&work->link);
+
+ work->function = function;
+ work->context = context;
+}
+
+/* Cast xenidc_work object to contained link. */
+
+static inline struct list_head *xenidc_work_to_link(xenidc_work * work)
+{
+ return &work->link;
+}
+
+/* Cast contained link of xenidc_work object back to xenidc_work object. */
+
+static inline xenidc_work *xenidc_work_link_to(struct list_head *link)
+{
+ return container_of(link, xenidc_work, link);
+}
+
+/* Schedule a xenidc_work object for later execution. It is legal to call */
+/* this even if the object is already scheduled. Returns 1 if scheduled this */
+/* time or 0 if already scheduled. */
+
+int xenidc_work_schedule(xenidc_work * work);
+
+/* Perform the task represented by the xenidc_work object synchronously on */
+/* the callers thread. */
+
+static inline void xenidc_work_perform_synchronously(xenidc_work * work)
+{
+ work->function(work->context);
+}
+
+/* Don't use any of these, they're just exposed for the macro below. */
+
+extern spinlock_t xenidc_work_list_lock;
+
+extern struct list_head xenidc_work_list;
+
+extern wait_queue_head_t xenidc_work_waitqueue;
+
+extern struct list_head xenidc_work_condition;
+
+/* Wait for a condition to be met. This works whether or not you call it */
+/* 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 ); \
+} \
+while( 0 )
+
+/* When you satisfy a condition, you should call this to kick any threads */
+/* waiting on the condition. */
+
+void xenidc_work_wake_up(void);
+
+#endif
[-- Attachment #3: Type: text/plain, Size: 138 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-21 13:18 [PATCH][1/17] USB virt 2.6 split driver---xenidc platform harry
@ 2005-11-21 20:10 ` Muli Ben-Yehuda
2005-11-21 21:03 ` Stephan Seitz
` (2 more replies)
0 siblings, 3 replies; 9+ messages in thread
From: Muli Ben-Yehuda @ 2005-11-21 20:10 UTC (permalink / raw)
To: harry; +Cc: 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".
On Mon, Nov 21, 2005 at 01:18:39PM +0000, harry wrote:
> +/* 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?
> +#include "xenidc_callback.h"
> +#include <linux/module.h>
local includes after global includes (and asm after linux)
> + 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)
> +EXPORT_SYMBOL(xenidc_callback_serialiser_function);
EXPORT_SYMBOL_GPL?
> +#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))
> +#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.
> +#define traceonly( S ) S
What is this for? lose the parens.
> +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.
> +typedef struct xenidc_callback_struct xenidc_callback;
no typedef for structs.
> +#define XENIDC_CALLBACK_LINK work.XENIDC_WORK_LINK
Please don't, don't hide structure members and don't name them in all
UPPERCASE.
> +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)
{
{
> +#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?
> + list_add_tail
> + (xenidc_callback_to_link(callback),
&serialiser->list);
This should be
list_add_tail(xenidc_callback_to_link(callback),
&serialiser->list);
> +typedef s32 xenidc_error;
Why? also, why signed, and why just 32 bits even on 64? does it go
over the wire?
> +#define XENIDC_ERROR_SUCCESS ( (xenidc_error)0 )
No parens. Also, can you use errno values rather than inventing your
own?
> +#define XENIDC_WORK_LINK link
Why is the renaming necessary?
> +int xenidc_work_schedule(xenidc_work * work);
The '*' should be aligned to the left.
> +/* 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 ); \
> +} \
> +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
--
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-21 20:10 ` Muli Ben-Yehuda
@ 2005-11-21 21:03 ` Stephan Seitz
2005-11-21 21:07 ` Muli Ben-Yehuda
2005-11-21 21:21 ` Harry Butterworth
2005-11-22 0:10 ` Anthony Liguori
2 siblings, 1 reply; 9+ messages in thread
From: Stephan Seitz @ 2005-11-21 21:03 UTC (permalink / raw)
To: Muli Ben-Yehuda; +Cc: harry, xen-devel
Muli Ben-Yehuda schrieb:
>>+/* 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-21 21:03 ` Stephan Seitz
@ 2005-11-21 21:07 ` Muli Ben-Yehuda
0 siblings, 0 replies; 9+ messages in thread
From: Muli Ben-Yehuda @ 2005-11-21 21:07 UTC (permalink / raw)
To: Stephan Seitz; +Cc: harry, xen-devel
On Mon, Nov 21, 2005 at 10:03:53PM +0100, Stephan Seitz wrote:
> 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
--
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-21 20:10 ` Muli Ben-Yehuda
2005-11-21 21:03 ` Stephan Seitz
@ 2005-11-21 21:21 ` Harry Butterworth
2005-11-22 10:51 ` Muli Ben-Yehuda
2005-11-22 0:10 ` Anthony Liguori
2 siblings, 1 reply; 9+ messages in thread
From: Harry Butterworth @ 2005-11-21 21:21 UTC (permalink / raw)
To: Muli Ben-Yehuda; +Cc: xen-devel
On Mon, 2005-11-21 at 22:10 +0200, Muli Ben-Yehuda wrote:
> 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...
>
> On Mon, Nov 21, 2005 at 01:18:39PM +0000, harry wrote:
>
> > +/* 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 ;-)
>
> > +#include "xenidc_callback.h"
> > +#include <linux/module.h>
>
> local includes after global includes (and asm after linux)
OK
>
> > + 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) {
?
>
> > +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.
>
>
> > +#if ( defined( CONFIG_XEN_IDC_TRACE ) || defined( XEN_IDC_TRACE ) )
>
> No spaces after and before parens
OK. Lindent must have missed this.
>
> #if (defined(CONFIG_XEN_IDC_TRACE) || defined(XEN_IDC_TRACE))
>
> > +#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
>
> > +#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?
>
> > +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.
>
> > +typedef struct xenidc_callback_struct xenidc_callback;
>
> no typedef for structs.
OK.
>
> > +#define XENIDC_CALLBACK_LINK work.XENIDC_WORK_LINK
>
> Please 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.
>
> > +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
> {
>
> > +#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?
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.
>
> > + 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.
>
> > +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.
>
> > +#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.
>
> > +#define XENIDC_WORK_LINK link
>
> Why is the renaming necessary?
This is the public link of this structure for use in
list_for_each_entry.
>
> > +int xenidc_work_schedule(xenidc_work * work);
>
> The '*' should be aligned to the left.
OK
>
> > +/* 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 ); \
> > +} \
> > +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.
>
> More to come.
>
> Cheers,
> Muli
--
Harry Butterworth <harry@hebutterworth.freeserve.co.uk>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-21 20:10 ` Muli Ben-Yehuda
2005-11-21 21:03 ` Stephan Seitz
2005-11-21 21:21 ` Harry Butterworth
@ 2005-11-22 0:10 ` Anthony Liguori
2 siblings, 0 replies; 9+ messages in thread
From: Anthony Liguori @ 2005-11-22 0:10 UTC (permalink / raw)
To: Muli Ben-Yehuda; +Cc: harry, xen-devel
Muli Ben-Yehuda wrote:
>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".
>
>On Mon, Nov 21, 2005 at 01:18:39PM +0000, harry wrote:
>
>
>>+#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.
>
>
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-21 21:21 ` Harry Butterworth
@ 2005-11-22 10:51 ` Muli Ben-Yehuda
2005-11-22 11:05 ` harry
0 siblings, 1 reply; 9+ messages in thread
From: Muli Ben-Yehuda @ 2005-11-22 10:51 UTC (permalink / raw)
To: Harry Butterworth; +Cc: xen-devel
On Mon, Nov 21, 2005 at 09:21:20PM +0000, Harry Butterworth wrote:
> On Mon, 2005-11-21 at 22:10 +0200, Muli Ben-Yehuda wrote:
> > 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.
> while (!list_empty(&serialiser->list) && !serialiser->running) {
Even better.
> > > +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.
> > > +#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.
> > > +#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?
>
> 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.
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.
> > 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
--
Muli Ben-Yehuda
http://www.mulix.org | http://mulix.livejournal.com/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-22 10:51 ` Muli Ben-Yehuda
@ 2005-11-22 11:05 ` harry
2005-11-22 15:05 ` Mark Williamson
0 siblings, 1 reply; 9+ messages in thread
From: harry @ 2005-11-22 11:05 UTC (permalink / raw)
To: Muli Ben-Yehuda; +Cc: xen-devel
On Tue, 2005-11-22 at 12:51 +0200, Muli Ben-Yehuda wrote:
> On Mon, Nov 21, 2005 at 09:21:20PM +0000, Harry Butterworth wrote:
> > On Mon, 2005-11-21 at 22:10 +0200, Muli Ben-Yehuda wrote:
> > > 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.
>
> > while (!list_empty(&serialiser->list) && !serialiser->running) {
>
> Even better.
>
> > > > +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.
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.
>
> > > > +#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
>
> > > > +#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?
> >
> > 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.
>
> 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
>
> etc.
>
> > > 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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH][1/17] USB virt 2.6 split driver---xenidc platform
2005-11-22 11:05 ` harry
@ 2005-11-22 15:05 ` Mark Williamson
0 siblings, 0 replies; 9+ messages in thread
From: Mark Williamson @ 2005-11-22 15:05 UTC (permalink / raw)
To: xen-devel; +Cc: harry
> 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.
> 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
> IANAL either.
>
> > > > > +#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
>
> > > > > +#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?
> > >
> > > 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.
> >
> > 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
>
> > etc.
> >
> > > > 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
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2005-11-22 15:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2005-11-21 13:18 [PATCH][1/17] USB virt 2.6 split driver---xenidc platform harry
2005-11-21 20:10 ` Muli Ben-Yehuda
2005-11-21 21:03 ` Stephan Seitz
2005-11-21 21:07 ` Muli Ben-Yehuda
2005-11-21 21:21 ` Harry Butterworth
2005-11-22 10:51 ` Muli Ben-Yehuda
2005-11-22 11:05 ` harry
2005-11-22 15:05 ` Mark Williamson
2005-11-22 0:10 ` Anthony Liguori
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.