[PATCH] Fix MSI-X with NIU cards

[PATCH] Fix MSI-X with NIU cards

[Matthew Wilcox]

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 of the mask
register to after MSI-X is enabled.

Reported-by: David S. Miller <davem@davemloft.net>
Tested-by: David S. Miller <davem@davemloft.net>
Reviewed-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 6f2e629..3627732 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -455,8 +455,6 @@ 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);
 
 		list_add_tail(&entry->list, &dev->msi_list);
@@ -493,6 +491,12 @@ static int msix_capability_init(struct pci_dev *dev,
 	msix_set_enable(dev, 1);
 	dev->msix_enabled = 1;
 
+	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);
+	}
+
 	return 0;
 }
 

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] Fix MSI-X with NIU cards

[Michael Ellerman]

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

On Fri, 2009-05-08 at 07:13 -0600, Matthew Wilcox 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 of the mask
> register to after MSI-X is enabled.
> 
> Reported-by: David S. Miller <davem@davemloft.net>
> Tested-by: David S. Miller <davem@davemloft.net>
> Reviewed-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> 
> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
> index 6f2e629..3627732 100644
> --- a/drivers/pci/msi.c
> +++ b/drivers/pci/msi.c
> @@ -455,8 +455,6 @@ 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);

158 static void msix_mask_irq(struct msi_desc *desc, u32 flag)                  
159 {
160         u32 mask_bits = desc->masked;
...
165         writel(mask_bits, desc->mask_base + offset);


So I guess this device is just silently ignoring that write?

And aren't we violating the spec by writing 0x1 into the device there
(assuming desc->masked is 0x0 from the kzalloc), ie. we're supposed to
read and write back the reserved bits unchanged. (§ 6.8.2.9?)


> @@ -493,6 +491,12 @@ static int msix_capability_init(struct pci_dev *dev,
>  	msix_set_enable(dev, 1);
>  	dev->msix_enabled = 1;

Are we safe if we take an interrupt 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);
> +	}
> +
>  	return 0;
>  }
>  

cheers


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

Re: [PATCH] Fix MSI-X with NIU cards

[David Miller]

From: Michael Ellerman <michael@ellerman.id.au>
Date: Mon, 11 May 2009 11:21:51 +1000

> So I guess this device is just silently ignoring that write?

No, it signals a fault on the PCI bus, which is how we noticed
this problem in the first place.

Re: [PATCH] Fix MSI-X with NIU cards

[Michael Ellerman]

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

On Sun, 2009-05-10 at 22:36 -0700, David Miller wrote:
> From: Michael Ellerman <michael@ellerman.id.au>
> Date: Mon, 11 May 2009 11:21:51 +1000
> 
> > So I guess this device is just silently ignoring that write?
> 
> No, it signals a fault on the PCI bus, which is how we noticed
> this problem in the first place.

But the patch doesn't change that? It just removes the readl AFAICS:

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 6f2e629..3627732 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -455,8 +455,6 @@ 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);
 

Or has msix_mask_irq() changed since 413f81eba?

cheers

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

Re: [PATCH] Fix MSI-X with NIU cards

[David Miller]

From: Michael Ellerman <michael@ellerman.id.au>
Date: Tue, 12 May 2009 00:30:11 +1000

> On Sun, 2009-05-10 at 22:36 -0700, David Miller wrote:
>> From: Michael Ellerman <michael@ellerman.id.au>
>> Date: Mon, 11 May 2009 11:21:51 +1000
>> 
>> > So I guess this device is just silently ignoring that write?
>> 
>> No, it signals a fault on the PCI bus, which is how we noticed
>> this problem in the first place.
> 
> But the patch doesn't change that? It just removes the readl AFAICS:

I'm on slow satellite internet here out in the ocean, so I wish
the author of this patch would help with your feedback :-/

> Or has msix_mask_irq() changed since 413f81eba?

You could ask GIT, and to answer your question, indeed it has.

Please don't stall this patch very much, as the bug it is fixing konks
out one of my primary test machines from even booting up.

Re: [PATCH] Fix MSI-X with NIU cards

[Jesse Barnes]

On Mon, 11 May 2009 16:40:17 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:

> From: Michael Ellerman <michael@ellerman.id.au>
> Date: Tue, 12 May 2009 00:30:11 +1000
> 
> > On Sun, 2009-05-10 at 22:36 -0700, David Miller wrote:
> >> From: Michael Ellerman <michael@ellerman.id.au>
> >> Date: Mon, 11 May 2009 11:21:51 +1000
> >> 
> >> > So I guess this device is just silently ignoring that write?
> >> 
> >> No, it signals a fault on the PCI bus, which is how we noticed
> >> this problem in the first place.
> > 
> > But the patch doesn't change that? It just removes the readl AFAICS:
> 
> I'm on slow satellite internet here out in the ocean, so I wish
> the author of this patch would help with your feedback :-/
> 
> > Or has msix_mask_irq() changed since 413f81eba?
> 
> You could ask GIT, and to answer your question, indeed it has.
> 
> Please don't stall this patch very much, as the bug it is fixing konks
> out one of my primary test machines from even booting up.

Ok I'll stuff it into my for-linus queue now... Hopefully Matthew can
answer the question in the meantime.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] Fix MSI-X with NIU cards

[Michael Ellerman]

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

On Mon, 2009-05-11 at 16:40 -0700, David Miller wrote:
> From: Michael Ellerman <michael@ellerman.id.au>
> Date: Tue, 12 May 2009 00:30:11 +1000
> 
> > On Sun, 2009-05-10 at 22:36 -0700, David Miller wrote:
> >> From: Michael Ellerman <michael@ellerman.id.au>
> >> Date: Mon, 11 May 2009 11:21:51 +1000
> >> 
> >> > So I guess this device is just silently ignoring that write?
> >> 
> >> No, it signals a fault on the PCI bus, which is how we noticed
> >> this problem in the first place.
> > 
> > But the patch doesn't change that? It just removes the readl AFAICS:
> 
> I'm on slow satellite internet here out in the ocean, so I wish
> the author of this patch would help with your feedback :-/

Me too :) *cough*

> > Or has msix_mask_irq() changed since 413f81eba?
> 
> You could ask GIT, and to answer your question, indeed it has.

Sorry it was late. I don't think it has actually:

concordia powerpc $ git log 413f81eba..1d80cac0f drivers/pci/msi.c 
concordia powerpc $ 

And I don't see any changes in Jesse's git tree via gitweb.

I still see:

158 static void msix_mask_irq(struct msi_desc *desc, u32 flag)                  
159 {
160         u32 mask_bits = desc->masked;
161         unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
162                                         PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
163         mask_bits &= ~1;
164         mask_bits |= flag;
165         writel(mask_bits, desc->mask_base + offset);
166         desc->masked = mask_bits;
167 }

So I don't see how this patch works, all it's doing is moving the readl().

> Please don't stall this patch very much, as the bug it is fixing konks
> out one of my primary test machines from even booting up.

Sure, chuck it in. I just don't get how it's solving the problem, and it
looks like it's writing to the mask bit without reading the reserved
ones first, which seems to violate the spec AFAICS[1]. But we can fix it
later.

cheers

[1]: I probably don't have the latest version.

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

Re: [PATCH] Fix MSI-X with NIU cards

[Hidetoshi Seto]

Michael Ellerman wrote:
> On Fri, 2009-05-08 at 07:13 -0600, Matthew Wilcox 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 of the mask
>> register to after MSI-X is enabled.
>>
>> Reported-by: David S. Miller <davem@davemloft.net>
>> Tested-by: David S. Miller <davem@davemloft.net>
>> Reviewed-by: David S. Miller <davem@davemloft.net>
>> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
>>
>> diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
>> index 6f2e629..3627732 100644
>> --- a/drivers/pci/msi.c
>> +++ b/drivers/pci/msi.c
>> @@ -455,8 +455,6 @@ 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);
> 
> 158 static void msix_mask_irq(struct msi_desc *desc, u32 flag)                  
> 159 {
> 160         u32 mask_bits = desc->masked;
> ...
> 165         writel(mask_bits, desc->mask_base + offset);
> 
> 
> So I guess this device is just silently ignoring that write?

I guess that write can be ignored while reads are not.

It seems that this issue was introduced by Matthew's commit:
commit f2440d9acbe866b917b16cc0f927366341ce9215
<quote>
@@ -435,11 +432,12 @@ static int msix_capability_init(struct pci_dev *dev,
                entry->msi_attrib.is_msix = 1;
                entry->msi_attrib.is_64 = 1;
                entry->msi_attrib.entry_nr = j;
-               entry->msi_attrib.maskbit = 1;
-               entry->msi_attrib.masked = 1;
                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);

                list_add_tail(&entry->list, &dev->msi_list);
        }
</quote>

I'm not sure why Matthew changed it to read/write...

> And aren't we violating the spec by writing 0x1 into the device there
> (assuming desc->masked is 0x0 from the kzalloc), ie. we're supposed to
> read and write back the reserved bits unchanged. (§ 6.8.2.9?)

Spec says:
 "This bit's state after reset is 1 (entry is masked).
  This bit is read/write."

>> @@ -493,6 +491,12 @@ static int msix_capability_init(struct pci_dev *dev,
>>  	msix_set_enable(dev, 1);
>>  	dev->msix_enabled = 1;
> 
> Are we safe if we take an interrupt here?

If reset was properly done on the device, we will be safe since entries are
masked.  Otherwise, ... I'm not sure.

>> +	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);
>> +	}
>> +
>>  	return 0;
>>  }

I think we can remove read/write here because it was same as before the
Matthew's change above.

If we really need read/write here, then I believe Matthew can provide
an incremental patch for that.


Thanks,
H.Seto

[PATCH] PCI MSI: Yet another fix for MSI-X with NIU cards

[Hidetoshi Seto]

Hi David,

Could you review & test following patch for your issue?

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 removing the read & write of
the mask register in msix_capability_init().  It will be safe since PCI
spac says the maskbit's state after reset is always 1 (= masked).

Reported-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
---
 drivers/pci/msi.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 6f2e629..b680a5b 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);
 	}
-- 
1.6.3

Re: [PATCH] Fix MSI-X with NIU cards

[Hidetoshi Seto]

Hidetoshi Seto wrote:
> It seems that this issue was introduced by Matthew's commit:
> commit f2440d9acbe866b917b16cc0f927366341ce9215
> <quote>
> @@ -435,11 +432,12 @@ static int msix_capability_init(struct pci_dev *dev,
>                 entry->msi_attrib.is_msix = 1;
>                 entry->msi_attrib.is_64 = 1;
>                 entry->msi_attrib.entry_nr = j;
> -               entry->msi_attrib.maskbit = 1;
> -               entry->msi_attrib.masked = 1;
>                 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);
> 
>                 list_add_tail(&entry->list, &dev->msi_list);
>         }
> </quote>
> 
> I'm not sure why Matthew changed it to read/write...

Sorry, I got it.

The problem is not the bit[0] for mask, but the reserved bits[31::01].
According to the Spec:
"31::01 Reserved 
  After reset, the state of these bits must be 0.
  However, for potential future use, software must preserve the value of
  these reserved bits when modifying the value of other Vector Control bits.
  If software modifies the value of these reserved bits, the result is
  undefined."

I'll post a revised fix soon.

Thanks,
H.Seto

Re: [PATCH] Fix MSI-X with NIU cards

[Michael Ellerman]

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

On Wed, 2009-05-13 at 13:40 +0900, Hidetoshi Seto wrote:
> Hidetoshi Seto wrote:
> > It seems that this issue was introduced by Matthew's commit:
> > commit f2440d9acbe866b917b16cc0f927366341ce9215
> > <quote>
> > @@ -435,11 +432,12 @@ static int msix_capability_init(struct pci_dev *dev,
> >                 entry->msi_attrib.is_msix = 1;
> >                 entry->msi_attrib.is_64 = 1;
> >                 entry->msi_attrib.entry_nr = j;
> > -               entry->msi_attrib.maskbit = 1;
> > -               entry->msi_attrib.masked = 1;
> >                 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);
> > 
> >                 list_add_tail(&entry->list, &dev->msi_list);
> >         }
> > </quote>
> > 
> > I'm not sure why Matthew changed it to read/write...
> 
> Sorry, I got it.
> 
> The problem is not the bit[0] for mask, but the reserved bits[31::01].
> According to the Spec:
> "31::01 Reserved 
>   After reset, the state of these bits must be 0.
>   However, for potential future use, software must preserve the value of
>   these reserved bits when modifying the value of other Vector Control bits.
>   If software modifies the value of these reserved bits, the result is
>   undefined."

Yes that's what I was referring to about it being out of spec.

So to work around the NIU bug and be in spec I think we need to enable
MSI, then read the mask bits, then write them back with the mask bit
set.

cheers


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

[PATCH] PCI MSI: Yet another fix for MSI-X with NIU cards, v2

[Hidetoshi Seto]

Hi David,

Please ignore previous version, and could you review & test following
v2 instead?

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 |   18 +++++++++++++++---
 1 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 6f2e629..7b3cf4a 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;
 }
 
-- 
1.6.3

Re: [PATCH] PCI MSI: Yet another fix for MSI-X with NIU cards, v2

[Hidetoshi Seto]

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(-)

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;
 }
 
-- 
1.6.3

Re: [PATCH] PCI MSI: Yet another fix for MSI-X with NIU cards

[David Miller]

From: Hidetoshi Seto <seto.hidetoshi@jp.fujitsu.com>
Date: Wed, 13 May 2009 13:10:17 +0900

> Could you review & test following patch for your issue?

I cannot test until Saturday as I am on a cruise.

Re: [PATCH] PCI MSI: Yet another fix for MSI-X with NIU cards, v2

[Michael Ellerman]

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

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
 


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

Re: [PATCH] Fix MSI-X with NIU cards

[Matthew Wilcox]

On Tue, May 12, 2009 at 12:21:26PM +1000, Michael Ellerman wrote:
> 158 static void msix_mask_irq(struct msi_desc *desc, u32 flag)                  
> 159 {
> 160         u32 mask_bits = desc->masked;
> 161         unsigned offset = desc->msi_attrib.entry_nr * PCI_MSIX_ENTRY_SIZE +
> 162                                         PCI_MSIX_ENTRY_VECTOR_CTRL_OFFSET;
> 163         mask_bits &= ~1;
> 164         mask_bits |= flag;
> 165         writel(mask_bits, desc->mask_base + offset);
> 166         desc->masked = mask_bits;
> 167 }
> 
> So I don't see how this patch works, all it's doing is moving the readl().

I think you're right.  My patch is clearly wrong.  I don't understand how
it worked for Dave.  Maybe his system is ignoring the errors on write
but can't ignore errors on read?  Or maybe his card silently ignores
writes and generates errors on read.

Here's an updated version:

----

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 of the mask
register to after MSI-X is enabled.

Reported-by: David S. Miller <davem@davemloft.net>
Tested-by: David S. Miller <davem@davemloft.net>
Reviewed-by: David S. Miller <davem@davemloft.net>
Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
Cc: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>


diff --git a/drivers/pci/msi.c b/drivers/pci/msi.c
index 6f2e629..cd66579 100644
--- a/drivers/pci/msi.c
+++ b/drivers/pci/msi.c
@@ -455,9 +455,6 @@ 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);
 
 		list_add_tail(&entry->list, &dev->msi_list);
 	}
@@ -493,6 +490,13 @@ static int msix_capability_init(struct pci_dev *dev,
 	msix_set_enable(dev, 1);
 	dev->msix_enabled = 1;
 
+	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);
+		msix_mask_irq(entry, 1);
+	}
+
 	return 0;
 }
 

-- 
Matthew Wilcox				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] PCI MSI: Yet another fix for MSI-X with NIU cards, v2

[Matthew Wilcox]

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				Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH] PCI MSI: Yet another fix for MSI-X with NIU cards, v2

[Hidetoshi Seto]

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