| Message ID | 4A0A5556.9050209@jp.fujitsu.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Wed, 2009-05-13 at 14:06 +0900, Hidetoshi Seto wrote: > The NIU device refuses to allow accesses to MSI-X registers before MSI-X > is enabled. This patch fixes the problem by moving the read & write the > mask register (for preserved bits) to after MSI-X is enabled. > > Reported-by: David S. Miller <davem@davemloft.net> > Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> > --- > drivers/pci/msi.c | 21 ++++++++++++++++++--- > 1 files changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c > index 6f2e629..44085e0 100644 > --- a/drivers/pci/msi.c > +++ b/drivers/pci/msi.c > @@ -455,9 +455,7 @@ static int msix_capability_init(struct pci_dev *dev, > entry->msi_attrib.default_irq = dev->irq; > entry->msi_attrib.pos = pos; > entry->mask_base = base; > - entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE + > - PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); > - msix_mask_irq(entry, 1); > + entry->masked = 1; > > list_add_tail(&entry->list, &dev->msi_list); > } > @@ -493,6 +491,23 @@ static int msix_capability_init(struct pci_dev *dev, > msix_set_enable(dev, 1); > dev->msix_enabled = 1; > > + /* > + * The states of Reserved bits[31:01] of Vector Control for MSI-X > + * Table Entries must be 0. However, for potential future use, > + * software must preserve the value of these reserved bits. > + * Refer PCI spec 3.0, 6.8.2.9. > + * > + * Note that there are some device that refuses access to MSI-X > + * Table Entries before MSI-X is enabled. Therefore we do it here. > + */ > + list_for_each_entry(entry, &dev->msi_list, list) { > + int vector = entry->msi_attrib.entry_nr; > + entry->masked = readl(base + vector * PCI_MSIX_ENTRY_SIZE + > + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); > + /* Make sure it is masked */ > + msix_mask_irq(entry, 1); > + } > + > return 0; > } That looks better to me, hopefully it fixes DaveM's device too :) Acked-by: Michael Ellerman <michael@ellerman.id.au> cheers
On Wed, May 13, 2009 at 02:06:30PM +0900, Hidetoshi Seto wrote: > entry->mask_base = base; > - entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE + > - PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); > - msix_mask_irq(entry, 1); > + entry->masked = 1; > Why do you add the setting of entry->masked here? > > + /* > + * The states of Reserved bits[31:01] of Vector Control for MSI-X > + * Table Entries must be 0. However, for potential future use, > + * software must preserve the value of these reserved bits. > + * Refer PCI spec 3.0, 6.8.2.9. > + * > + * Note that there are some device that refuses access to MSI-X > + * Table Entries before MSI-X is enabled. Therefore we do it here. > + */ I think you need to refer to PCIe 2.1 (or an ECN incorporated into it). Some of these bits are now used. > + list_for_each_entry(entry, &dev->msi_list, list) { > + int vector = entry->msi_attrib.entry_nr; > + entry->masked = readl(base + vector * PCI_MSIX_ENTRY_SIZE + > + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); > + /* Make sure it is masked */ > + msix_mask_irq(entry, 1); > + } > + > return 0; This looks to be the same as the replacement patch I sent earlier.
Matthew Wilcox wrote: > On Wed, May 13, 2009 at 02:06:30PM +0900, Hidetoshi Seto wrote: >> entry->mask_base = base; >> - entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE + >> - PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); >> - msix_mask_irq(entry, 1); >> + entry->masked = 1; >> > > Why do you add the setting of entry->masked here? That was a temporal value for a window between later MSI-X and read/write. But I agree that using global mask bit like your "Better fix" is better idea. >> + /* >> + * The states of Reserved bits[31:01] of Vector Control for MSI-X >> + * Table Entries must be 0. However, for potential future use, >> + * software must preserve the value of these reserved bits. >> + * Refer PCI spec 3.0, 6.8.2.9. >> + * >> + * Note that there are some device that refuses access to MSI-X >> + * Table Entries before MSI-X is enabled. Therefore we do it here. >> + */ > > I think you need to refer to PCIe 2.1 (or an ECN incorporated into it). > Some of these bits are now used. Wow, thank you for telling me that PCIe 2.1 is available now! Anyway I recommend you to add a comment like this, to tell why we do read/write, and why it is placed here. It will be help for future developers not to move this before enable of MSI-X. >> + list_for_each_entry(entry, &dev->msi_list, list) { >> + int vector = entry->msi_attrib.entry_nr; >> + entry->masked = readl(base + vector * PCI_MSIX_ENTRY_SIZE + >> + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); >> + /* Make sure it is masked */ >> + msix_mask_irq(entry, 1); >> + } >> + >> return 0; > > This looks to be the same as the replacement patch I sent earlier. Yes. I posted it because there were no response from you in last few days. Thanks, H.Seto -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c index 6f2e629..44085e0 100644 --- a/drivers/pci/msi.c +++ b/drivers/pci/msi.c @@ -455,9 +455,7 @@ static int msix_capability_init(struct pci_dev *dev, entry->msi_attrib.default_irq = dev->irq; entry->msi_attrib.pos = pos; entry->mask_base = base; - entry->masked = readl(base + j * PCI_MSIX_ENTRY_SIZE + - PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); - msix_mask_irq(entry, 1); + entry->masked = 1; list_add_tail(&entry->list, &dev->msi_list); } @@ -493,6 +491,23 @@ static int msix_capability_init(struct pci_dev *dev, msix_set_enable(dev, 1); dev->msix_enabled = 1; + /* + * The states of Reserved bits[31:01] of Vector Control for MSI-X + * Table Entries must be 0. However, for potential future use, + * software must preserve the value of these reserved bits. + * Refer PCI spec 3.0, 6.8.2.9. + * + * Note that there are some device that refuses access to MSI-X + * Table Entries before MSI-X is enabled. Therefore we do it here. + */ + list_for_each_entry(entry, &dev->msi_list, list) { + int vector = entry->msi_attrib.entry_nr; + entry->masked = readl(base + vector * PCI_MSIX_ENTRY_SIZE + + PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET); + /* Make sure it is masked */ + msix_mask_irq(entry, 1); + } + return 0; }
Hi David and all, Sorry, if you have problem with wrong diffstat in patch header (it was because I hand-fixed the comment in patch), please use following instead. Contents are not changed, still v2. Thanks, H.Seto The NIU device refuses to allow accesses to MSI-X registers before MSI-X is enabled. This patch fixes the problem by moving the read & write the mask register (for preserved bits) to after MSI-X is enabled. Reported-by: David S. Miller <davem@davemloft.net> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com> --- drivers/pci/msi.c | 21 ++++++++++++++++++--- 1 files changed, 18 insertions(+), 3 deletions(-)