Re: msi_free_irqs #2
On Thu, May 24, 2007 at 02:53:08PM -0600, Eric W. Biederman wrote:
>
>
> Could you please try the patch below. Unless I have misread something
> this should fix your problem
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 000c9ae..2e1d4af 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev,
> entry->dev = dev;
> entry->mask_base = base;
>
> - list_add(>list, >msi_list);
> + list_add_tail(>list, >msi_list);
> }
>
> ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
>
> > Michael or Eric, would you please review this patch and see if it's OK?
> > Adding
> > an else
> > between the the if (list_is) and the writel resolved the Oops. I'm not
> > sure
> > how this
> > is supposed to work but using entry->mask_base after iounmap'ing seems
> > wrong.
>
> I think I would rather just swap those two lines of code.
> We are manually setting the mask bit to make certain the irq doesn't
> fire after we free it, and we clearly want to set the mask bit for
> all the irq entries.
>
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index d9cbd58..0e67723 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -560,10 +560,11 @@ static int msi_free_irqs(struct pci_dev* dev)
> > if (entry->msi_attrib.type == PCI_CAP_ID_MSIX) {
> > if (list_is_last(>list, >msi_list))
> > iounmap(entry->mask_base);
> > -
> > - writel(1, entry->mask_base + entry->msi_attrib.entry_nr
> > - * PCI_MSIX_ENTRY_SIZE
> > - + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> > + else
> > + writel(1, entry->mask_base
> > + + entry->msi_attrib.entry_nr
> > + * PCI_MSIX_ENTRY_SIZE
> > + + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> > }
> > list_del(>list);
> > kfree(entry);
Here's a patch that just reverses the 2 lines of code as Eric suggests. Please
consider this for inclusion.
Signed-off-by: Mike Miller <[EMAIL PROTECTED]>
Signed-off-by: Chase Maupin <[EMAIL PROTECTED]>
Thanks,
mikem
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index e01380b..6632150 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -558,12 +558,12 @@ static int msi_free_irqs(struct pci_dev* dev)
list_for_each_entry_safe(entry, tmp, >msi_list, list) {
if (entry->msi_attrib.type == PCI_CAP_ID_MSIX) {
- if (list_is_last(>list, >msi_list))
- iounmap(entry->mask_base);
-
writel(1, entry->mask_base + entry->msi_attrib.entry_nr
* PCI_MSIX_ENTRY_SIZE
+ PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+
+ if (list_is_last(>list, >msi_list))
+ iounmap(entry->mask_base);
}
list_del(>list);
kfree(entry);
-
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: msi_free_irqs #2
On Thu, May 24, 2007 at 02:53:08PM -0600, Eric W. Biederman wrote:
>
>
> Could you please try the patch below. Unless I have misread something
> this should fix your problem
>
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 000c9ae..2e1d4af 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev,
> entry->dev = dev;
> entry->mask_base = base;
>
> - list_add(>list, >msi_list);
> + list_add_tail(>list, >msi_list);
> }
Yes, this is what we found. But we made 2 changes to list_add. Just sent the
patch.
>
> ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
>
> > Michael or Eric, would you please review this patch and see if it's OK?
> > Adding
> > an else
> > between the the if (list_is) and the writel resolved the Oops. I'm not
> > sure
> > how this
> > is supposed to work but using entry->mask_base after iounmap'ing seems
> > wrong.
>
> I think I would rather just swap those two lines of code.
> We are manually setting the mask bit to make certain the irq doesn't
> fire after we free it, and we clearly want to set the mask bit for
> all the irq entries.
OK, new patch on the way.
mikem
>
> > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> > index d9cbd58..0e67723 100644
> > --- a/drivers/pci/msi.c
> > +++ b/drivers/pci/msi.c
> > @@ -560,10 +560,11 @@ static int msi_free_irqs(struct pci_dev* dev)
> > if (entry->msi_attrib.type == PCI_CAP_ID_MSIX) {
> > if (list_is_last(>list, >msi_list))
> > iounmap(entry->mask_base);
> > -
> > - writel(1, entry->mask_base + entry->msi_attrib.entry_nr
> > - * PCI_MSIX_ENTRY_SIZE
> > - + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> > + else
> > + writel(1, entry->mask_base
> > + + entry->msi_attrib.entry_nr
> > + * PCI_MSIX_ENTRY_SIZE
> > + + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> > }
> > list_del(>list);
> > kfree(entry);
> > -
> >
> > I hope this clears up a little of the fog.
>
> Yes it does thanks.
>
> 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: msi_free_irqs #2
Could you please try the patch below. Unless I have misread something
this should fix your problem
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 000c9ae..2e1d4af 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev,
entry->dev = dev;
entry->mask_base = base;
- list_add(>list, >msi_list);
+ list_add_tail(>list, >msi_list);
}
ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
> Michael or Eric, would you please review this patch and see if it's OK? Adding
> an else
> between the the if (list_is) and the writel resolved the Oops. I'm not
> sure
> how this
> is supposed to work but using entry->mask_base after iounmap'ing seems wrong.
I think I would rather just swap those two lines of code.
We are manually setting the mask bit to make certain the irq doesn't
fire after we free it, and we clearly want to set the mask bit for
all the irq entries.
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index d9cbd58..0e67723 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -560,10 +560,11 @@ static int msi_free_irqs(struct pci_dev* dev)
> if (entry->msi_attrib.type == PCI_CAP_ID_MSIX) {
> if (list_is_last(>list, >msi_list))
> iounmap(entry->mask_base);
> -
> - writel(1, entry->mask_base + entry->msi_attrib.entry_nr
> - * PCI_MSIX_ENTRY_SIZE
> - + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> + else
> + writel(1, entry->mask_base
> + + entry->msi_attrib.entry_nr
> + * PCI_MSIX_ENTRY_SIZE
> + + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
> }
> list_del(>list);
> kfree(entry);
> -
>
> I hope this clears up a little of the fog.
Yes it does thanks.
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: msi_free_irqs #2
On Thu, May 24, 2007 at 01:42:58PM -0600, Eric W. Biederman wrote: > Andrew Morton <[EMAIL PROTECTED]> writes: > > > On Thu, 24 May 2007 11:07:56 -0500 > > "Mike Miller (OS Dev)" <[EMAIL PROTECTED]> wrote: > > > >> So I guess I found the answer to my own question. msi_free_irqs was > >> apparently > > added > >> in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So > >> somebody > > broke a > >> couple of things. > >> The most noticable is cciss hangs after turning on interrupts. The reason > >> for > > that is > >> the kernel now looks at my array of MSI-X vectors in reverse order. We > >> have 4 > > ways of > >> generating an interrupt on Smart Array hw. They are: > >> > >> # define DOORBELL_INT 0 > >> # define PERF_MODE_INT1 > >> # define SIMPLE_MODE_INT 2 > >> # define MEMQ_MODE_INT3 > >> > >> For INTx these four lines are OR'ed together and run to one interrupt > > pin. MSI-X > >> breaks this hardware OR'ing so we must register either all 4 or at the > >> least > > the > >> correct interrupt. When I first submitted the MSI/X support I was > >> registering > > all 4. > >> Someone changed that to only register a single MSI-X vector. That worked > >> fine > > until > >> 2.6.22-something. > >> Now it appears that the kernel is looking at the array in reverse order. > >> IOW, > > I must > >> register PERF_MODE_INT in order for cciss to work. That's messed up. > >> Anybody > > want to > >> `fess up to making these changes? :) > >> I'll keep working this, but I'm going to undo someones change when I figure > > out where > >> it's broke. > >> > > > > I'd guess that you're referring to Michael's changes. If you can identify > > the offending code in a less vague fashion, more confident answers can > > be given ;) > > > > I canot find any sign of anyone altering the IRQ handling in cciss.c after > > your initial MSI-support merge. But that's perhaps isn't what you meant. > > it's all rather foggy. Please either quote file-n-line, or grab a copy > > of git-blame. > > Or perhaps git-bisect to find the offending patch. > > I don't recall seeing anything that looked to bad but there was a fair > amount of change needed to get the last bit of portability into the msi > code, so it is possible something slipped through. > > Possibly someone changed the default enable or disable state? > > > > Which reminds me. Now that we have a reasonable list, we really need > to reduce pci_enable_msix: > > - int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int > nvec); > + int pci_enable_msix(struct pci_dev* dev, int nvec); > > And just have drivers that use more the one irq walk the list off of pci_dev > of all of the msi irqs. I did a little review a while ago and only > 0-(nvec -1) are allocated and the are always in order in entries so it > shouldn't be to bad to generate a patch for that case, and not having > to worry about out of order or holes in the irq allocator would be > good. > > Eric Found what seems the problem with our vectors being listed backward. In drivers/pci/msi.c we should be using list_add_tail rather than list_add to preserve the ordering across various kernels. Please consider this for inclusion. diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 0e67723..d74975d 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -333,7 +333,7 @@ static int msi_capability_init(struct pci_dev *dev) msi_mask_bits_reg(pos, is_64bit_address(control)), maskbits); } - list_add(>list, >msi_list); + list_add_tail(>list, >msi_list); /* Configure MSI capability structure */ ret = arch_setup_msi_irqs(dev, 1, PCI_CAP_ID_MSI); @@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev, entry->dev = dev; entry->mask_base = base; - list_add(>list, >msi_list); + list_add_tail(>list, >msi_list); } ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX); This patch undoes my dirty little hack. diff --git a/drivers/block/cciss.h b/drivers/block/cciss.h index 26b5866..b70988d 100644 --- a/drivers/block/cciss.h +++ b/drivers/block/cciss.h @@ -70,8 +70,8 @@ struct ctlr_info int highest_lun; int usage_count; /* number of opens all all minor devices */ # define DOORBELL_INT 0 -# define PERF_MODE_INT2 -# define SIMPLE_MODE_INT 1 +# define PERF_MODE_INT1 +# define SIMPLE_MODE_INT 2 # define MEMQ_MODE_INT3 unsigned int intr[4]; unsigned int msix_vector; - 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
Re: msi_free_irqs #2
On Thu, May 24, 2007 at 10:27:02AM -0700, Andrew Morton wrote:
> On Thu, 24 May 2007 11:07:56 -0500
> "Mike Miller (OS Dev)" <[EMAIL PROTECTED]> wrote:
>
> > So I guess I found the answer to my own question. msi_free_irqs was
> > apparently added
> > in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So
> > somebody broke a
> > couple of things.
> > The most noticable is cciss hangs after turning on interrupts. The reason
> > for that is
> > the kernel now looks at my array of MSI-X vectors in reverse order. We have
> > 4 ways of
> > generating an interrupt on Smart Array hw. They are:
> >
> > # define DOORBELL_INT 0
> > # define PERF_MODE_INT1
> > # define SIMPLE_MODE_INT 2
> > # define MEMQ_MODE_INT3
> >
I apologize for the vagueness of the message. This dirty hack makes cciss work
in the
.22-rc kernel. I have not yet figured out what broke.
-
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 5acc6c4..7383483 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -3494,7 +3494,7 @@ static void cciss_shutdown(struct pci_dev *pdev)
} else {
printk(KERN_WARNING "Error flushing cache on controller %d\n",
i);
}
- free_irq(hba[i]->intr[2], hba[i]);
+ free_irq(hba[i]->intr[SIMPLE_MODE_INT], hba[i]);
}
static void __devexit cciss_remove_one(struct pci_dev *pdev)
diff --git a/drivers/block/cciss.h b/drivers/block/cciss.h
index b70988d..26b5866 100644
--- a/drivers/block/cciss.h
+++ b/drivers/block/cciss.h
@@ -70,8 +70,8 @@ struct ctlr_info
int highest_lun;
int usage_count; /* number of opens all all minor devices */
# define DOORBELL_INT 0
-# define PERF_MODE_INT1
-# define SIMPLE_MODE_INT 2
+# define PERF_MODE_INT2
+# define SIMPLE_MODE_INT 1
# define MEMQ_MODE_INT3
unsigned int intr[4];
unsigned int msix_vector;
-
> > For INTx these four lines are OR'ed together and run to one interrupt pin.
> > MSI-X
> > breaks this hardware OR'ing so we must register either all 4 or at the
> > least the
> > correct interrupt. When I first submitted the MSI/X support I was
> > registering all 4.
> > Someone changed that to only register a single MSI-X vector. That worked
> > fine until
> > 2.6.22-something.
> > Now it appears that the kernel is looking at the array in reverse order.
> > IOW, I must
> > register PERF_MODE_INT in order for cciss to work. That's messed up.
> > Anybody want to
Have not yet found the change that caused this but my nasty little hack gets
around it
for my testing. After I return from the long weekend I'll try to hunt this down.
> > `fess up to making these changes? :)
> > I'll keep working this, but I'm going to undo someones change when I figure
> > out where
> > it's broke.
> >
>
> I'd guess that you're referring to Michael's changes. If you can identify
> the offending code in a less vague fashion, more confident answers can
> be given ;)
>
> I canot find any sign of anyone altering the IRQ handling in cciss.c after
> your initial MSI-support merge. But that's perhaps isn't what you meant.
> it's all rather foggy. Please either quote file-n-line, or grab a copy
> of git-blame.
Now for my original mail where the driver Oops'ed on rmmod. This patch prevents
the
Oops but I'm not 100% sure it's right. Here's the Oops:
Completed flushing cache on controller 2
BUG: unable to handle kernel paging request at virtual address f8b2200c
printing eip:
c01e9cc7
*pdpt = 3001
*pde = 37e48067
*pte =
Oops: 0002 [#1]
SMP
Modules linked in: cciss ipv6 parport_pc lp parport autofs4 i2c_dev i2c_core
sunrpc loop dm_multipath button battery asus_acpi ac tg3 floppy sg dm_snapshot
dm_zero dm_mirror ext3 jbd dm_mod ata_piix libata mptsas scsi_transport_sas
mptspi scsi_transport_spi mptscsih mptbase sd_mod scsi_mod
CPU:1
EIP:0060:[]Not tainted VLI
EFLAGS: 00010286 (2.6.22-rc2-gd2579053 #1)
EIP is at msi_free_irqs+0x81/0xbe
eax: f8b22000 ebx: f71f3180 ecx: f7fff280 edx: c1886eb8
esi: f7c4e800 edi: f7c4ec48 ebp: 0002 esp: f5a0dec8
ds: 007b es: 007b fs: 00d8 gs: 0033 ss: 0068
Process rmmod (pid: 5286, ti=f5a0d000 task=c47d2550 task.ti=f5a0d000)
Stack: 0002 f8b72294 0400 f8b69ca7 f8b6bc6c 0002
f5a997f4 f8b69d61 f7c5a4b0 f7c4e848 f7c4e848
f7c4e800 f7c4e800 f8b72294 f7c4e848 f8b72294 c01e3cdf f7c4e848 c024c469
Call Trace:
[] cciss_shutdown+0xae/0xc3 [cciss]
[] cciss_remove_one+0xa5/0x178 [cciss]
[] pci_device_remove+0x16/0x35
[] __device_release_driver+0x71/0x8e
[] driver_detach+0xa0/0xde
[] bus_remove_driver+0x27/0x41
[] pci_unregister_driver+0xb/0x13
[]
Re: msi_free_irqs #2
Andrew Morton <[EMAIL PROTECTED]> writes: > On Thu, 24 May 2007 11:07:56 -0500 > "Mike Miller (OS Dev)" <[EMAIL PROTECTED]> wrote: > >> So I guess I found the answer to my own question. msi_free_irqs was >> apparently > added >> in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So >> somebody > broke a >> couple of things. >> The most noticable is cciss hangs after turning on interrupts. The reason for > that is >> the kernel now looks at my array of MSI-X vectors in reverse order. We have 4 > ways of >> generating an interrupt on Smart Array hw. They are: >> >> # define DOORBELL_INT 0 >> # define PERF_MODE_INT1 >> # define SIMPLE_MODE_INT 2 >> # define MEMQ_MODE_INT3 >> >> For INTx these four lines are OR'ed together and run to one interrupt > pin. MSI-X >> breaks this hardware OR'ing so we must register either all 4 or at the least > the >> correct interrupt. When I first submitted the MSI/X support I was registering > all 4. >> Someone changed that to only register a single MSI-X vector. That worked fine > until >> 2.6.22-something. >> Now it appears that the kernel is looking at the array in reverse order. IOW, > I must >> register PERF_MODE_INT in order for cciss to work. That's messed up. Anybody > want to >> `fess up to making these changes? :) >> I'll keep working this, but I'm going to undo someones change when I figure > out where >> it's broke. >> > > I'd guess that you're referring to Michael's changes. If you can identify > the offending code in a less vague fashion, more confident answers can > be given ;) > > I canot find any sign of anyone altering the IRQ handling in cciss.c after > your initial MSI-support merge. But that's perhaps isn't what you meant. > it's all rather foggy. Please either quote file-n-line, or grab a copy > of git-blame. Or perhaps git-bisect to find the offending patch. I don't recall seeing anything that looked to bad but there was a fair amount of change needed to get the last bit of portability into the msi code, so it is possible something slipped through. Possibly someone changed the default enable or disable state? Which reminds me. Now that we have a reasonable list, we really need to reduce pci_enable_msix: - int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec); + int pci_enable_msix(struct pci_dev* dev, int nvec); And just have drivers that use more the one irq walk the list off of pci_dev of all of the msi irqs. I did a little review a while ago and only 0-(nvec -1) are allocated and the are always in order in entries so it shouldn't be to bad to generate a patch for that case, and not having to worry about out of order or holes in the irq allocator would be good. 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: msi_free_irqs #2
On Thu, 24 May 2007 11:07:56 -0500 "Mike Miller (OS Dev)" <[EMAIL PROTECTED]> wrote: > So I guess I found the answer to my own question. msi_free_irqs was > apparently added > in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So > somebody broke a > couple of things. > The most noticable is cciss hangs after turning on interrupts. The reason for > that is > the kernel now looks at my array of MSI-X vectors in reverse order. We have 4 > ways of > generating an interrupt on Smart Array hw. They are: > > # define DOORBELL_INT 0 > # define PERF_MODE_INT1 > # define SIMPLE_MODE_INT 2 > # define MEMQ_MODE_INT3 > > For INTx these four lines are OR'ed together and run to one interrupt pin. > MSI-X > breaks this hardware OR'ing so we must register either all 4 or at the least > the > correct interrupt. When I first submitted the MSI/X support I was registering > all 4. > Someone changed that to only register a single MSI-X vector. That worked fine > until > 2.6.22-something. > Now it appears that the kernel is looking at the array in reverse order. IOW, > I must > register PERF_MODE_INT in order for cciss to work. That's messed up. Anybody > want to > `fess up to making these changes? :) > I'll keep working this, but I'm going to undo someones change when I figure > out where > it's broke. > I'd guess that you're referring to Michael's changes. If you can identify the offending code in a less vague fashion, more confident answers can be given ;) I canot find any sign of anyone altering the IRQ handling in cciss.c after your initial MSI-support merge. But that's perhaps isn't what you meant. it's all rather foggy. Please either quote file-n-line, or grab a copy of git-blame. - 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/
msi_free_irqs #2
So I guess I found the answer to my own question. msi_free_irqs was apparently added in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So somebody broke a couple of things. The most noticable is cciss hangs after turning on interrupts. The reason for that is the kernel now looks at my array of MSI-X vectors in reverse order. We have 4 ways of generating an interrupt on Smart Array hw. They are: # define DOORBELL_INT 0 # define PERF_MODE_INT1 # define SIMPLE_MODE_INT 2 # define MEMQ_MODE_INT3 For INTx these four lines are OR'ed together and run to one interrupt pin. MSI-X breaks this hardware OR'ing so we must register either all 4 or at the least the correct interrupt. When I first submitted the MSI/X support I was registering all 4. Someone changed that to only register a single MSI-X vector. That worked fine until 2.6.22-something. Now it appears that the kernel is looking at the array in reverse order. IOW, I must register PERF_MODE_INT in order for cciss to work. That's messed up. Anybody want to `fess up to making these changes? :) I'll keep working this, but I'm going to undo someones change when I figure out where it's broke. mikem - 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/
msi_free_irqs #2
So I guess I found the answer to my own question. msi_free_irqs was apparently added in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So somebody broke a couple of things. The most noticable is cciss hangs after turning on interrupts. The reason for that is the kernel now looks at my array of MSI-X vectors in reverse order. We have 4 ways of generating an interrupt on Smart Array hw. They are: # define DOORBELL_INT 0 # define PERF_MODE_INT1 # define SIMPLE_MODE_INT 2 # define MEMQ_MODE_INT3 For INTx these four lines are OR'ed together and run to one interrupt pin. MSI-X breaks this hardware OR'ing so we must register either all 4 or at the least the correct interrupt. When I first submitted the MSI/X support I was registering all 4. Someone changed that to only register a single MSI-X vector. That worked fine until 2.6.22-something. Now it appears that the kernel is looking at the array in reverse order. IOW, I must register PERF_MODE_INT in order for cciss to work. That's messed up. Anybody want to `fess up to making these changes? :) I'll keep working this, but I'm going to undo someones change when I figure out where it's broke. mikem - 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: msi_free_irqs #2
On Thu, 24 May 2007 11:07:56 -0500 Mike Miller (OS Dev) [EMAIL PROTECTED] wrote: So I guess I found the answer to my own question. msi_free_irqs was apparently added in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So somebody broke a couple of things. The most noticable is cciss hangs after turning on interrupts. The reason for that is the kernel now looks at my array of MSI-X vectors in reverse order. We have 4 ways of generating an interrupt on Smart Array hw. They are: # define DOORBELL_INT 0 # define PERF_MODE_INT1 # define SIMPLE_MODE_INT 2 # define MEMQ_MODE_INT3 For INTx these four lines are OR'ed together and run to one interrupt pin. MSI-X breaks this hardware OR'ing so we must register either all 4 or at the least the correct interrupt. When I first submitted the MSI/X support I was registering all 4. Someone changed that to only register a single MSI-X vector. That worked fine until 2.6.22-something. Now it appears that the kernel is looking at the array in reverse order. IOW, I must register PERF_MODE_INT in order for cciss to work. That's messed up. Anybody want to `fess up to making these changes? :) I'll keep working this, but I'm going to undo someones change when I figure out where it's broke. I'd guess that you're referring to Michael's changes. If you can identify the offending code in a less vague fashion, more confident answers can be given ;) I canot find any sign of anyone altering the IRQ handling in cciss.c after your initial MSI-support merge. But that's perhaps isn't what you meant. it's all rather foggy. Please either quote file-n-line, or grab a copy of git-blame. - 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: msi_free_irqs #2
Andrew Morton [EMAIL PROTECTED] writes: On Thu, 24 May 2007 11:07:56 -0500 Mike Miller (OS Dev) [EMAIL PROTECTED] wrote: So I guess I found the answer to my own question. msi_free_irqs was apparently added in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So somebody broke a couple of things. The most noticable is cciss hangs after turning on interrupts. The reason for that is the kernel now looks at my array of MSI-X vectors in reverse order. We have 4 ways of generating an interrupt on Smart Array hw. They are: # define DOORBELL_INT 0 # define PERF_MODE_INT1 # define SIMPLE_MODE_INT 2 # define MEMQ_MODE_INT3 For INTx these four lines are OR'ed together and run to one interrupt pin. MSI-X breaks this hardware OR'ing so we must register either all 4 or at the least the correct interrupt. When I first submitted the MSI/X support I was registering all 4. Someone changed that to only register a single MSI-X vector. That worked fine until 2.6.22-something. Now it appears that the kernel is looking at the array in reverse order. IOW, I must register PERF_MODE_INT in order for cciss to work. That's messed up. Anybody want to `fess up to making these changes? :) I'll keep working this, but I'm going to undo someones change when I figure out where it's broke. I'd guess that you're referring to Michael's changes. If you can identify the offending code in a less vague fashion, more confident answers can be given ;) I canot find any sign of anyone altering the IRQ handling in cciss.c after your initial MSI-support merge. But that's perhaps isn't what you meant. it's all rather foggy. Please either quote file-n-line, or grab a copy of git-blame. Or perhaps git-bisect to find the offending patch. I don't recall seeing anything that looked to bad but there was a fair amount of change needed to get the last bit of portability into the msi code, so it is possible something slipped through. Possibly someone changed the default enable or disable state? Which reminds me. Now that we have a reasonable list, we really need to reduce pci_enable_msix: - int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec); + int pci_enable_msix(struct pci_dev* dev, int nvec); And just have drivers that use more the one irq walk the list off of pci_dev of all of the msi irqs. I did a little review a while ago and only 0-(nvec -1) are allocated and the are always in order in entries so it shouldn't be to bad to generate a patch for that case, and not having to worry about out of order or holes in the irq allocator would be good. 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: msi_free_irqs #2
On Thu, May 24, 2007 at 10:27:02AM -0700, Andrew Morton wrote:
On Thu, 24 May 2007 11:07:56 -0500
Mike Miller (OS Dev) [EMAIL PROTECTED] wrote:
So I guess I found the answer to my own question. msi_free_irqs was
apparently added
in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So
somebody broke a
couple of things.
The most noticable is cciss hangs after turning on interrupts. The reason
for that is
the kernel now looks at my array of MSI-X vectors in reverse order. We have
4 ways of
generating an interrupt on Smart Array hw. They are:
# define DOORBELL_INT 0
# define PERF_MODE_INT1
# define SIMPLE_MODE_INT 2
# define MEMQ_MODE_INT3
I apologize for the vagueness of the message. This dirty hack makes cciss work
in the
.22-rc kernel. I have not yet figured out what broke.
-
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 5acc6c4..7383483 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -3494,7 +3494,7 @@ static void cciss_shutdown(struct pci_dev *pdev)
} else {
printk(KERN_WARNING Error flushing cache on controller %d\n,
i);
}
- free_irq(hba[i]-intr[2], hba[i]);
+ free_irq(hba[i]-intr[SIMPLE_MODE_INT], hba[i]);
}
static void __devexit cciss_remove_one(struct pci_dev *pdev)
diff --git a/drivers/block/cciss.h b/drivers/block/cciss.h
index b70988d..26b5866 100644
--- a/drivers/block/cciss.h
+++ b/drivers/block/cciss.h
@@ -70,8 +70,8 @@ struct ctlr_info
int highest_lun;
int usage_count; /* number of opens all all minor devices */
# define DOORBELL_INT 0
-# define PERF_MODE_INT1
-# define SIMPLE_MODE_INT 2
+# define PERF_MODE_INT2
+# define SIMPLE_MODE_INT 1
# define MEMQ_MODE_INT3
unsigned int intr[4];
unsigned int msix_vector;
-
For INTx these four lines are OR'ed together and run to one interrupt pin.
MSI-X
breaks this hardware OR'ing so we must register either all 4 or at the
least the
correct interrupt. When I first submitted the MSI/X support I was
registering all 4.
Someone changed that to only register a single MSI-X vector. That worked
fine until
2.6.22-something.
Now it appears that the kernel is looking at the array in reverse order.
IOW, I must
register PERF_MODE_INT in order for cciss to work. That's messed up.
Anybody want to
Have not yet found the change that caused this but my nasty little hack gets
around it
for my testing. After I return from the long weekend I'll try to hunt this down.
`fess up to making these changes? :)
I'll keep working this, but I'm going to undo someones change when I figure
out where
it's broke.
I'd guess that you're referring to Michael's changes. If you can identify
the offending code in a less vague fashion, more confident answers can
be given ;)
I canot find any sign of anyone altering the IRQ handling in cciss.c after
your initial MSI-support merge. But that's perhaps isn't what you meant.
it's all rather foggy. Please either quote file-n-line, or grab a copy
of git-blame.
Now for my original mail where the driver Oops'ed on rmmod. This patch prevents
the
Oops but I'm not 100% sure it's right. Here's the Oops:
Completed flushing cache on controller 2
BUG: unable to handle kernel paging request at virtual address f8b2200c
printing eip:
c01e9cc7
*pdpt = 3001
*pde = 37e48067
*pte =
Oops: 0002 [#1]
SMP
Modules linked in: cciss ipv6 parport_pc lp parport autofs4 i2c_dev i2c_core
sunrpc loop dm_multipath button battery asus_acpi ac tg3 floppy sg dm_snapshot
dm_zero dm_mirror ext3 jbd dm_mod ata_piix libata mptsas scsi_transport_sas
mptspi scsi_transport_spi mptscsih mptbase sd_mod scsi_mod
CPU:1
EIP:0060:[c01e9cc7]Not tainted VLI
EFLAGS: 00010286 (2.6.22-rc2-gd2579053 #1)
EIP is at msi_free_irqs+0x81/0xbe
eax: f8b22000 ebx: f71f3180 ecx: f7fff280 edx: c1886eb8
esi: f7c4e800 edi: f7c4ec48 ebp: 0002 esp: f5a0dec8
ds: 007b es: 007b fs: 00d8 gs: 0033 ss: 0068
Process rmmod (pid: 5286, ti=f5a0d000 task=c47d2550 task.ti=f5a0d000)
Stack: 0002 f8b72294 0400 f8b69ca7 f8b6bc6c 0002
f5a997f4 f8b69d61 f7c5a4b0 f7c4e848 f7c4e848
f7c4e800 f7c4e800 f8b72294 f7c4e848 f8b72294 c01e3cdf f7c4e848 c024c469
Call Trace:
[f8b69ca7] cciss_shutdown+0xae/0xc3 [cciss]
[f8b69d61] cciss_remove_one+0xa5/0x178 [cciss]
[c01e3cdf] pci_device_remove+0x16/0x35
[c024c469] __device_release_driver+0x71/0x8e
[c024c56e] driver_detach+0xa0/0xde
[c024bc5c] bus_remove_driver+0x27/0x41
[c01e3ef3] pci_unregister_driver+0xb/0x13
[f8b6a343]
Re: msi_free_irqs #2
Could you please try the patch below. Unless I have misread something
this should fix your problem
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 000c9ae..2e1d4af 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev,
entry-dev = dev;
entry-mask_base = base;
- list_add(entry-list, dev-msi_list);
+ list_add_tail(entry-list, dev-msi_list);
}
ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
Michael or Eric, would you please review this patch and see if it's OK? Adding
an else
between the the if (list_is) and the writel resolved the Oops. I'm not
sure
how this
is supposed to work but using entry-mask_base after iounmap'ing seems wrong.
I think I would rather just swap those two lines of code.
We are manually setting the mask bit to make certain the irq doesn't
fire after we free it, and we clearly want to set the mask bit for
all the irq entries.
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d9cbd58..0e67723 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -560,10 +560,11 @@ static int msi_free_irqs(struct pci_dev* dev)
if (entry-msi_attrib.type == PCI_CAP_ID_MSIX) {
if (list_is_last(entry-list, dev-msi_list))
iounmap(entry-mask_base);
-
- writel(1, entry-mask_base + entry-msi_attrib.entry_nr
- * PCI_MSIX_ENTRY_SIZE
- + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+ else
+ writel(1, entry-mask_base
+ + entry-msi_attrib.entry_nr
+ * PCI_MSIX_ENTRY_SIZE
+ + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
}
list_del(entry-list);
kfree(entry);
-
I hope this clears up a little of the fog.
Yes it does thanks.
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: msi_free_irqs #2
On Thu, May 24, 2007 at 01:42:58PM -0600, Eric W. Biederman wrote: Andrew Morton [EMAIL PROTECTED] writes: On Thu, 24 May 2007 11:07:56 -0500 Mike Miller (OS Dev) [EMAIL PROTECTED] wrote: So I guess I found the answer to my own question. msi_free_irqs was apparently added in 2.6.22-something. I don't find it in 2.6.21.2 or anywhere else. So somebody broke a couple of things. The most noticable is cciss hangs after turning on interrupts. The reason for that is the kernel now looks at my array of MSI-X vectors in reverse order. We have 4 ways of generating an interrupt on Smart Array hw. They are: # define DOORBELL_INT 0 # define PERF_MODE_INT1 # define SIMPLE_MODE_INT 2 # define MEMQ_MODE_INT3 For INTx these four lines are OR'ed together and run to one interrupt pin. MSI-X breaks this hardware OR'ing so we must register either all 4 or at the least the correct interrupt. When I first submitted the MSI/X support I was registering all 4. Someone changed that to only register a single MSI-X vector. That worked fine until 2.6.22-something. Now it appears that the kernel is looking at the array in reverse order. IOW, I must register PERF_MODE_INT in order for cciss to work. That's messed up. Anybody want to `fess up to making these changes? :) I'll keep working this, but I'm going to undo someones change when I figure out where it's broke. I'd guess that you're referring to Michael's changes. If you can identify the offending code in a less vague fashion, more confident answers can be given ;) I canot find any sign of anyone altering the IRQ handling in cciss.c after your initial MSI-support merge. But that's perhaps isn't what you meant. it's all rather foggy. Please either quote file-n-line, or grab a copy of git-blame. Or perhaps git-bisect to find the offending patch. I don't recall seeing anything that looked to bad but there was a fair amount of change needed to get the last bit of portability into the msi code, so it is possible something slipped through. Possibly someone changed the default enable or disable state? Which reminds me. Now that we have a reasonable list, we really need to reduce pci_enable_msix: - int pci_enable_msix(struct pci_dev* dev, struct msix_entry *entries, int nvec); + int pci_enable_msix(struct pci_dev* dev, int nvec); And just have drivers that use more the one irq walk the list off of pci_dev of all of the msi irqs. I did a little review a while ago and only 0-(nvec -1) are allocated and the are always in order in entries so it shouldn't be to bad to generate a patch for that case, and not having to worry about out of order or holes in the irq allocator would be good. Eric Found what seems the problem with our vectors being listed backward. In drivers/pci/msi.c we should be using list_add_tail rather than list_add to preserve the ordering across various kernels. Please consider this for inclusion. diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 0e67723..d74975d 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -333,7 +333,7 @@ static int msi_capability_init(struct pci_dev *dev) msi_mask_bits_reg(pos, is_64bit_address(control)), maskbits); } - list_add(entry-list, dev-msi_list); + list_add_tail(entry-list, dev-msi_list); /* Configure MSI capability structure */ ret = arch_setup_msi_irqs(dev, 1, PCI_CAP_ID_MSI); @@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev, entry-dev = dev; entry-mask_base = base; - list_add(entry-list, dev-msi_list); + list_add_tail(entry-list, dev-msi_list); } ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX); This patch undoes my dirty little hack. diff --git a/drivers/block/cciss.h b/drivers/block/cciss.h index 26b5866..b70988d 100644 --- a/drivers/block/cciss.h +++ b/drivers/block/cciss.h @@ -70,8 +70,8 @@ struct ctlr_info int highest_lun; int usage_count; /* number of opens all all minor devices */ # define DOORBELL_INT 0 -# define PERF_MODE_INT2 -# define SIMPLE_MODE_INT 1 +# define PERF_MODE_INT1 +# define SIMPLE_MODE_INT 2 # define MEMQ_MODE_INT3 unsigned int intr[4]; unsigned int msix_vector; - 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: msi_free_irqs #2
On Thu, May 24, 2007 at 02:53:08PM -0600, Eric W. Biederman wrote:
Could you please try the patch below. Unless I have misread something
this should fix your problem
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 000c9ae..2e1d4af 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev,
entry-dev = dev;
entry-mask_base = base;
- list_add(entry-list, dev-msi_list);
+ list_add_tail(entry-list, dev-msi_list);
}
Yes, this is what we found. But we made 2 changes to list_add. Just sent the
patch.
ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
Michael or Eric, would you please review this patch and see if it's OK?
Adding
an else
between the the if (list_is) and the writel resolved the Oops. I'm not
sure
how this
is supposed to work but using entry-mask_base after iounmap'ing seems
wrong.
I think I would rather just swap those two lines of code.
We are manually setting the mask bit to make certain the irq doesn't
fire after we free it, and we clearly want to set the mask bit for
all the irq entries.
OK, new patch on the way.
mikem
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d9cbd58..0e67723 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -560,10 +560,11 @@ static int msi_free_irqs(struct pci_dev* dev)
if (entry-msi_attrib.type == PCI_CAP_ID_MSIX) {
if (list_is_last(entry-list, dev-msi_list))
iounmap(entry-mask_base);
-
- writel(1, entry-mask_base + entry-msi_attrib.entry_nr
- * PCI_MSIX_ENTRY_SIZE
- + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+ else
+ writel(1, entry-mask_base
+ + entry-msi_attrib.entry_nr
+ * PCI_MSIX_ENTRY_SIZE
+ + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
}
list_del(entry-list);
kfree(entry);
-
I hope this clears up a little of the fog.
Yes it does thanks.
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: msi_free_irqs #2
On Thu, May 24, 2007 at 02:53:08PM -0600, Eric W. Biederman wrote:
Could you please try the patch below. Unless I have misread something
this should fix your problem
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 000c9ae..2e1d4af 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -404,7 +404,7 @@ static int msix_capability_init(struct pci_dev *dev,
entry-dev = dev;
entry-mask_base = base;
- list_add(entry-list, dev-msi_list);
+ list_add_tail(entry-list, dev-msi_list);
}
ret = arch_setup_msi_irqs(dev, nvec, PCI_CAP_ID_MSIX);
Michael or Eric, would you please review this patch and see if it's OK?
Adding
an else
between the the if (list_is) and the writel resolved the Oops. I'm not
sure
how this
is supposed to work but using entry-mask_base after iounmap'ing seems
wrong.
I think I would rather just swap those two lines of code.
We are manually setting the mask bit to make certain the irq doesn't
fire after we free it, and we clearly want to set the mask bit for
all the irq entries.
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index d9cbd58..0e67723 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -560,10 +560,11 @@ static int msi_free_irqs(struct pci_dev* dev)
if (entry-msi_attrib.type == PCI_CAP_ID_MSIX) {
if (list_is_last(entry-list, dev-msi_list))
iounmap(entry-mask_base);
-
- writel(1, entry-mask_base + entry-msi_attrib.entry_nr
- * PCI_MSIX_ENTRY_SIZE
- + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+ else
+ writel(1, entry-mask_base
+ + entry-msi_attrib.entry_nr
+ * PCI_MSIX_ENTRY_SIZE
+ + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
}
list_del(entry-list);
kfree(entry);
Here's a patch that just reverses the 2 lines of code as Eric suggests. Please
consider this for inclusion.
Signed-off-by: Mike Miller [EMAIL PROTECTED]
Signed-off-by: Chase Maupin [EMAIL PROTECTED]
Thanks,
mikem
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index e01380b..6632150 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -558,12 +558,12 @@ static int msi_free_irqs(struct pci_dev* dev)
list_for_each_entry_safe(entry, tmp, dev-msi_list, list) {
if (entry-msi_attrib.type == PCI_CAP_ID_MSIX) {
- if (list_is_last(entry-list, dev-msi_list))
- iounmap(entry-mask_base);
-
writel(1, entry-mask_base + entry-msi_attrib.entry_nr
* PCI_MSIX_ENTRY_SIZE
+ PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET);
+
+ if (list_is_last(entry-list, dev-msi_list))
+ iounmap(entry-mask_base);
}
list_del(entry-list);
kfree(entry);
-
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/
