Re: [PATCH 14/21] MSI: Use a list instead of the custom link structure
Michael Ellerman <[EMAIL PROTECTED]> 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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/21] MSI: Use a list instead of the custom link structure
On Wed, 2007-03-28 at 00:29 -0600, Eric W. Biederman wrote:
> Michael Ellerman <[EMAIL PROTECTED]> 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
> > +
> > 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
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 14/21] MSI: Use a list instead of the custom link structure
On Wed, 2007-03-28 at 00:29 -0600, Eric W. Biederman wrote:
Michael Ellerman [EMAIL PROTECTED] 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
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 14/21] MSI: Use a list instead of the custom link structure
Michael Ellerman [EMAIL PROTECTED] 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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/21] MSI: Use a list instead of the custom link structure
Michael Ellerman <[EMAIL PROTECTED]> 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
> +
> 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 14/21] MSI: Use a list instead of the custom link structure
Michael Ellerman [EMAIL PROTECTED] 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 [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
