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

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

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> 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

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

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 3134 bytes --]

On Wed, 2007-03-28 at 00:29 -0600, Eric W. Biederman wrote:
> Michael Ellerman <michael@ellerman.id.au> 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.

I thought about doing it in the MSI enable methods, but I think it
really belongs in the (nonexistant) routine that allocs and sets up a
pci_dev.

I think it's pretty dicy to be passing around a pci_dev with an
uninitialised msi_list. Even if currently no code outside the MSI enable
methods looks at it, I think we're asking for bugs in the future.

So I'll do a patch which adds alloc_pci_dev(), update the callers, and
then put the msi_list initialisation in there.

> > --- 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"

Oops, I'll fix that.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

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

[Eric W. Biederman]

Michael Ellerman <michael@ellerman.id.au> writes:

>
> I thought about doing it in the MSI enable methods, but I think it
> really belongs in the (nonexistant) routine that allocs and sets up a
> pci_dev.

I agree that would be a good place for it as well.

> I think it's pretty dicy to be passing around a pci_dev with an
> uninitialised msi_list. Even if currently no code outside the MSI enable
> methods looks at it, I think we're asking for bugs in the future.

Reasonable.

> So I'll do a patch which adds alloc_pci_dev(), update the callers, and
> then put the msi_list initialisation in there.

Sounds good.  That will allow us to initialize all of the fields in struct
pci_dev to a default value in one place.

Eric