Re: [PATCH 14/21] MSI: Use a list instead of the custom link structure

[Eric W. Biederman]

Michael Ellerman <michael@xxxxxxxxxxxxxx> writes:

> The msi descriptors are linked together with what looks a lot like
> a linked list, but isn't a struct list_head list. Make it one.
>
> The only complication is that previously we walked a list of irqs, and
> got the descriptor for each with get_irq_msi(). Now we have a list of
> descriptors and need to get the irq out of it, so it needs to be in the
> actual struct msi_desc. We use 0 to indicate no irq is setup.
>
> At some point after a pci_dev is created we need to initialise its
> msi_list. pci_device_add() looks like the right place to do that, although
> I'm not convinced it's 100% safe. In drivers/char/agp/alpha-agp.c we create
> a pci_dev and I don't see that it ever gets passed to pci_device_add(), but
> we probably don't care.

Well that one appears to be a dummy place holder and probably should at
least have a kzalloc to initialize all of the fields to know values.

Regardless the normal pci device allocation does use kzalloc so we will
have well defined if not beautiful behavior if we try and use it.

Until we have a case where we need to use the msi_list outside of 
where we enable and disable msi we should be perfectly fine initializing
the list somewhere inside of pci_enable_msi, and pci_enable_msix.
With dev->msi_enabled and dev->msix_enabled serving as flags to the
rest of the world that it is safe to look at the list.

It certainly sounds safer to me then becoming to closely coupled with
code that doesn't really care about how msi works.  Heck even though
we repeat the call twice I bet it will even be less code.

> --- msi-new.orig/include/linux/msi.h
> +++ msi-new/include/linux/msi.h
> @@ -1,6 +1,8 @@
>  #ifndef LINUX_MSI_H
>  #define LINUX_MSI_H
>  
> +#include <linux/list.h>
> +
>  struct msi_msg {
>  	u32	address_lo;	/* low 32 bits of msi message address */
>  	u32	address_hi;	/* high 32 bits of msi message address */
> @@ -24,10 +26,8 @@ struct msi_desc {
>  		unsigned default_irq;	/* default pre-assigned irq	  */
>  	}msi_attrib;
>  
> -	struct {
> -		__u16	head;
> -		__u16	tail;
> -	}link;
> +	int irq;
This should be "unsigned int irq"
> +	struct list_head list;
>  
>  	void __iomem *mask_base;
>  	struct pci_dev *dev;

Eric
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/