Re: [PATCH]2.6.7 MSI-X Update

[Roland Dreier <roland@topspin.com>]

A couple of other comments on the patch:

  > +Argument entries is a pointer of unsigned integer type. The number of
  > +elements is indicated in argument nvec. The content of each element 
  > +will be mapped to the following struct defined in /driver/pci/msi.h.
  > +
  > +struct msix_entry {
  > +	__u32 	vector	: 16; /* kernel uses to write alloc vector */
  > +	__u32	entry	: 16; /* driver uses to specify entry */
  > +};
  > +
  > +A device driver is responsible for initializing the field entry of 
  > +each element with unique entry supported by MSI-X table.

I think this structure should be defined in a header in include/linux,
probably <linux/pci.h>.  We could create a new <linux/msi.h> include
but I don't think it's worth it at this point.  Also I don't see any
reason to use bitfields or userspace types like __u32 (since no
userspace code is going to use this include file).  I would just
declare the type as

struct msix_entry {
       u16 vector;
       u16 entry;
};

  > +int pci_enable_msix(struct pci_dev *dev, u32 *entries, int nvec)

Since this function takes an array of struct msix_entry in its entries
parameter, I think entries should be declared as struct msix_entry *
rather than just u32 *.  That is, I would write the prototype as

int pci_enable_msix(struct pci_dev *dev, struct msix_entry *entries,
		    int nvec);

  > +		j = (entries + i)->entry;
  > +		(entries + i)->vector = vector;

Finally, this is a nitpick, but this just looks odd to me.  Why not
write this as

                j = entries[i].entry;
                entries[i].vector = vector;

By the way, I have MSI working with my device with this patch
applied.  I am getting ready to test MSI-X, which is why I have
comments about the MSI-X API now.

 - Roland