Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Robert Hancock]

Jeff Garzik wrote:
> The unmodified tulip driver checks both MWI and cacheline-size because 
> one of the clones (PNIC or PNIC2) will let you set the MWI bit, but 
> hardwires cacheline size to zero.
> 
> If the arches do not behave consistently, we need to keep the check in 
> the tulip driver, to avoid incorrectly programming the csr0 MWI bit.
> 
>     Jeff

I should think that pci_set_mwi should fail if either the cache line 
size showed 0 (after either setting the correct size or assuming it 
should have been set already) or the MWI bit ended up clear after we 
tried to turn it on.

That pcibios_prep_mwi code for sparc64 looks wrong, if you plug in a 
device that doesn't implement MWI at all it will probably not let 
anything get written to PCI_CACHE_LINE_SIZE other than 0, but it is 
blindly succeeding in all cases. Even if the arch assumes the firmware 
already set the size properly it should still make sure it is non-zero 
before allowing MWI..

-- 
Robert Hancock      Saskatoon, SK, Canada
To email, remove "nospam" from hancockr@nospamshaw.ca
Home Page: http://www.roberthancock.com/

[PATCH 1/2] [PCI] Check that MWI bit really did get set

[Matthew Wilcox]

Since some devices may not implement the MWI bit, we should check that
the write did set it and return an error if it didn't.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index a544997..3d041f4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -900,13 +900,17 @@ #endif
 		return rc;
 
 	pci_read_config_word(dev, PCI_COMMAND, &cmd);
-	if (! (cmd & PCI_COMMAND_INVALIDATE)) {
-		pr_debug("PCI: Enabling Mem-Wr-Inval for device %s\n", pci_name(dev));
-		cmd |= PCI_COMMAND_INVALIDATE;
-		pci_write_config_word(dev, PCI_COMMAND, cmd);
-	}
-	
-	return 0;
+	if (cmd & PCI_COMMAND_INVALIDATE)
+		return 0;
+
+	pr_debug("PCI: Enabling Mem-Wr-Inval for device %s\n", pci_name(dev));
+	cmd |= PCI_COMMAND_INVALIDATE;
+	pci_write_config_word(dev, PCI_COMMAND, cmd);
+
+	/* read result from hardware (in case bit refused to enable) */
+	pci_read_config_word(dev, PCI_COMMAND, &cmd);
+
+	return (cmd & PCI_COMMAND_INVALIDATE) ? 0 : -EINVAL;
 }
 
 /**

[PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Matthew Wilcox]

We used to check whether pci_set_mwi() had succeeded by testing the
hardware MWI bit.  Now we need only check the return value (and failing
to do so is a warning).  Also, pci_set_mwi() will fail if the cache line
size is 0, so we don't need to check that ourselves any more.

Signed-off-by: Matthew Wilcox <matthew@wil.cx>

diff --git a/drivers/net/tulip/tulip_core.c b/drivers/net/tulip/tulip_core.c
index d11d28c..64d999b 100644
--- a/drivers/net/tulip/tulip_core.c
+++ b/drivers/net/tulip/tulip_core.c
@@ -1135,7 +1135,6 @@ static void __devinit tulip_mwi_config (
 {
 	struct tulip_private *tp = netdev_priv(dev);
 	u8 cache;
-	u16 pci_command;
 	u32 csr0;
 
 	if (tulip_debug > 3)
@@ -1153,21 +1152,15 @@ static void __devinit tulip_mwi_config (
 	/* set or disable MWI in the standard PCI command bit.
 	 * Check for the case where  mwi is desired but not available
 	 */
-	if (csr0 & MWI)	pci_set_mwi(pdev);
-	else		pci_clear_mwi(pdev);
-
-	/* read result from hardware (in case bit refused to enable) */
-	pci_read_config_word(pdev, PCI_COMMAND, &pci_command);
-	if ((csr0 & MWI) && (!(pci_command & PCI_COMMAND_INVALIDATE)))
-		csr0 &= ~MWI;
-
-	/* if cache line size hardwired to zero, no MWI */
-	pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cache);
-	if ((csr0 & MWI) && (cache == 0)) {
-		csr0 &= ~MWI;
+	if (csr0 & MWI)	{
+		if (pci_set_mwi(pdev))
+			csr0 &= ~MWI;
+	} else {
 		pci_clear_mwi(pdev);
 	}
 
+	pci_read_config_byte(pdev, PCI_CACHE_LINE_SIZE, &cache);
+
 	/* assign per-cacheline-size cache alignment and
 	 * burst length values
 	 */

Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Jeff Garzik]

Matthew Wilcox wrote:
> Also, pci_set_mwi() will fail if the cache line
> size is 0, so we don't need to check that ourselves any more.

NAK, not true on all arches.  sparc64 at least presumes that the 
firmware DTRT with cacheline size, which hurts us now given this tulip patch

	Jeff

Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Matthew Wilcox]

On Fri, Oct 06, 2006 at 03:15:15PM -0400, Jeff Garzik wrote:
> Matthew Wilcox wrote:
> >Also, pci_set_mwi() will fail if the cache line
> >size is 0, so we don't need to check that ourselves any more.
> 
> NAK, not true on all arches.  sparc64 at least presumes that the 
> firmware DTRT with cacheline size, which hurts us now given this tulip patch

How does it hurt us?

int pcibios_prep_mwi(struct pci_dev *dev)
{
        /* We set correct PCI_CACHE_LINE_SIZE register values for every
         * device probed on this platform.  So there is nothing to check
         * and this always succeeds.
         */
        return 0;
}

If Dave's wrong about that, it hurts him, not us ;-)

It's still not necessary for the Tulip driver to check.

Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Jeff Garzik]

Matthew Wilcox wrote:
> On Fri, Oct 06, 2006 at 03:15:15PM -0400, Jeff Garzik wrote:
>> Matthew Wilcox wrote:
>>> Also, pci_set_mwi() will fail if the cache line
>>> size is 0, so we don't need to check that ourselves any more.
>> NAK, not true on all arches.  sparc64 at least presumes that the 
>> firmware DTRT with cacheline size, which hurts us now given this tulip patch
> 
> How does it hurt us?
> 
> int pcibios_prep_mwi(struct pci_dev *dev)
> {
>         /* We set correct PCI_CACHE_LINE_SIZE register values for every
>          * device probed on this platform.  So there is nothing to check
>          * and this always succeeds.
>          */
>         return 0;
> }
> 
> If Dave's wrong about that, it hurts him, not us ;-)
> 
> It's still not necessary for the Tulip driver to check.

The unmodified tulip driver checks both MWI and cacheline-size because 
one of the clones (PNIC or PNIC2) will let you set the MWI bit, but 
hardwires cacheline size to zero.

If the arches do not behave consistently, we need to keep the check in 
the tulip driver, to avoid incorrectly programming the csr0 MWI bit.

	Jeff

Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Grant Grundler]

On Fri, Oct 06, 2006 at 03:59:57PM -0400, Jeff Garzik wrote:
> The unmodified tulip driver checks both MWI and cacheline-size because 
> one of the clones (PNIC or PNIC2) will let you set the MWI bit, but 
> hardwires cacheline size to zero.

Maybe the generic pci_set_mwi() can verify cacheline size is non-zero?
I don't think each driver should need to enforce this.

> If the arches do not behave consistently, we need to keep the check in 
> the tulip driver, to avoid incorrectly programming the csr0 MWI bit.

Why not fix the arches to be consistent?
There's alot more drivers than arches...and we have control
of the arch specific PCI support.

thanks,
grant

Re: [PATCH 2/2] [TULIP] Check the return value from pci_set_mwi()

[Jeff Garzik]

Grant Grundler wrote:
> On Fri, Oct 06, 2006 at 03:59:57PM -0400, Jeff Garzik wrote:
>> The unmodified tulip driver checks both MWI and cacheline-size because 
>> one of the clones (PNIC or PNIC2) will let you set the MWI bit, but 
>> hardwires cacheline size to zero.
> 
> Maybe the generic pci_set_mwi() can verify cacheline size is non-zero?
> I don't think each driver should need to enforce this.

Agreed.


>> If the arches do not behave consistently, we need to keep the check in 
>> the tulip driver, to avoid incorrectly programming the csr0 MWI bit.
> 
> Why not fix the arches to be consistent?
> There's alot more drivers than arches...and we have control
> of the arch specific PCI support.

Agreed.

	Jeff