Re: [PATCH] Add irqdevice interface + userint implementation

[Christoph Hellwig <hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>]

> diff --git a/drivers/kvm/kvm_irqdevice.h b/drivers/kvm/kvm_irqdevice.h
> new file mode 100644
> index 0000000..9c91d15
> --- /dev/null
> +++ b/drivers/kvm/kvm_irqdevice.h
> @@ -0,0 +1,206 @@
> +/*
> + * kvm_irqdevice.h

No need to mention the file name in the top of file comment.  Quite contrary,
this comment can easily get out of sync and thus is not encouraged.

> + *
> + * Defines an interface for an abstract interrupt controller.  The model 
> + * consists of a unit with an arbitrary number of input lines (IRQ0-N), an
> + * output line (INTR), and methods for completing an interrupt-acknowledge
> + * cycle (INTA).  A particular implementation of this model will define
> + * various policies, such as irq-to-vector translation, INTA/auto-EOI policy,
> + * etc.  
> + * 
> + * In addition, the INTR callback mechanism allows the unit to be "wired" to
> + * an interruptible source in a very flexible manner. For instance, an 
> + * irqdevice could have its INTR wired to a VCPU (ala LAPIC), or another 
> + * interrupt controller (ala cascaded i8259s)
> + *
> + * Copyright (C) 2007 Novell
> + *
> + * Authors:
> + *   Gregory Haskins <ghaskins-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope 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.

It would be nice if you could follow the top of file comment style used
in the rest of the kvm code instead of using a very different one.

> +/*
> + * kvm_irq_init()
> + *
> + * Description:
> + *    Initialized the kvm_irqdevice for use.  Should be called before calling
> + *    any derived implementation init functions
> + * 
> + * Parameters:
> + *    struct kvm_irqdevice *dev: This device
> + *
> + * Returns: (void)
> + */

Please use kernel-doc style comments that can be used to auto-generate
documentation.

> +static inline int kvm_irq_pending(struct kvm_irqdevice *dev, int flags)
> +{
> +	return dev->pending(dev, flags);
> +}

This wrapper is not needed at all, just call the method directly.

> +static inline int kvm_irq_read_vector(struct kvm_irqdevice *dev, int flags)
> +{
> +	return dev->read_vector(dev, flags);
> +}

ditto

> +static inline int kvm_irq_set_pin(struct kvm_irqdevice *dev, int pin, 
> +				  int level, int flags)
> +{
> +	return dev->set_pin(dev, pin, level, flags);
> +}

ditto

> +static inline int kvm_irq_summary(struct kvm_irqdevice *dev, void *data)
> +{
> +	return dev->summary(dev, data);
> +}

ditto.

> +static inline void kvm_irq_register_sink(struct kvm_irqdevice *dev, 
> +					 const struct kvm_irqsink *sink)
> +{
> +	dev->sink = *sink;
> +}

Completely useless wrapper, just do the assignment in the caller.
Also the convention of copying the operation vectors is rather unnatural
for linux, just set the pointer (and make sure the operations regustired
are file-scope not local-scope as in your current code)

> +static inline void kvm_irq_raise_intr(struct kvm_irqdevice *dev)
> +{
> +	struct kvm_irqsink *sink = &dev->sink;
> +	if (sink->raise_intr)
> +		sink->raise_intr(sink, dev);
> +}

Similarly this is rather pointless and could be opencoded in the only caller.

> +/*----------------------------------------------------------------------
> + * optimized bitarray object - works like bitarrays in bitops, but uses 
> + * a summary field to accelerate lookups.  Assumes external locking 
> + *---------------------------------------------------------------------*/
> +
> +struct bitarray {
> +	unsigned long summary; /* 1 per word in pending */
> +	unsigned long pending[NR_IRQ_WORDS];
> +};
> +
> +static inline int bitarray_pending(struct bitarray *this)
> +{
> +	return this->summary ? 1 : 0;	
> +}
> +
> +static inline int bitarray_findhighest(struct bitarray *this)
> +{
> +	if (!this->summary)
> +		return -1;
> +	else {
> +	    int word_index = __ffs(this->summary);
> +	    int bit_index  = __ffs(this->pending[word_index]);
> +
> +	    return word_index * BITS_PER_LONG + bit_index;	
> +	}
> +}
> +
> +static inline void bitarray_set(struct bitarray *this, int nr)
> +{
> +	__set_bit(nr, &this->pending);
> +	__set_bit(nr / BITS_PER_LONG, &this->summary); 
> +} 
> +
> +static inline void bitarray_clear(struct bitarray *this, int nr)
> +{
> +	int word = nr / BITS_PER_LONG;
> +
> +	__clear_bit(nr, &this->pending);
> +	if (!this->pending[word])
> +		__clear_bit(word, &this->summary);
> +}
> +
> +static inline int bitarray_test(struct bitarray *this, int nr)
> +{
> +	return test_bit(nr, &this->pending);
> +}

This should go into a separate header, probably even in include/linux/

> +/*----------------------------------------------------------------------
> + * userint interface - provides the actual kvm_irqdevice implementation
> + *---------------------------------------------------------------------*/
> +
> +typedef struct {
> +	spinlock_t      lock;
> +	struct bitarray irq_pending;
> +	int             nmi_pending;
> +}kvm_userint;

Please just use a struct type instead of the typedef.

> +int kvm_userint_init(struct kvm_irqdevice *dev)
> +{
> +	kvm_userint *s;
> +
> +	s = (kvm_userint *)kzalloc(sizeof(kvm_userint), GFP_KERNEL);

No need to case the kzalloc return value.  But you need to check
the return value for NULL and handle the error.


-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV