[PATCH] IDE: fix PCI must_checks

[PATCH] IDE: fix PCI must_checks

[Randy Dunlap]

From: Randy Dunlap <randy.dunlap@oracle.com>

Check PCI interface function results for errors in drivers/ide/pci.

drivers/ide/pci/cs5530.c:244: warning: ignoring return value of 'pci_set_mwi', declared with attribute warn_unused_result

drivers/ide/pci/sc1200.c:397: warning: ignoring return value of 'pci_enable_device', declared with attribute warn_unused_result

drivers/ide/setup-pci.c:861: warning: ignoring return value of '__pci_register_driver', declared with attribute warn_unused_result

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/ide/pci/cs5530.c |    4 +++-
 drivers/ide/pci/sc1200.c |    8 +++++++-
 drivers/ide/setup-pci.c  |    6 +++++-
 3 files changed, 15 insertions(+), 3 deletions(-)

--- linux-2.6.21-rc5-mm4.orig/drivers/ide/pci/cs5530.c
+++ linux-2.6.21-rc5-mm4/drivers/ide/pci/cs5530.c
@@ -236,7 +236,9 @@ static unsigned int __devinit init_chips
 	 */
 
 	pci_set_master(cs5530_0);
-	pci_set_mwi(cs5530_0);
+	if (pci_set_mwi(cs5530_0))
+		printk(KERN_WARNING "%s: error enabling PCI MWI transactions\n",
+			name);
 
 	/*
 	 * Set PCI CacheLineSize to 16-bytes:
--- linux-2.6.21-rc5-mm4.orig/drivers/ide/pci/sc1200.c
+++ linux-2.6.21-rc5-mm4/drivers/ide/pci/sc1200.c
@@ -382,10 +382,16 @@ static int sc1200_suspend (struct pci_de
 static int sc1200_resume (struct pci_dev *dev)
 {
 	ide_hwif_t	*hwif = NULL;
+	int		err;
 
 	pci_set_power_state(dev, PCI_D0);	// bring chip back from sleep state
 	dev->current_state = PM_EVENT_ON;
-	pci_enable_device(dev);
+	err = pci_enable_device(dev);
+	if (err) {
+		printk(KERN_ERR "SC1200: cannot enable device for resume\n");
+		return err;
+	}
+
 	//
 	// loop over all interfaces that are part of this pci device:
 	//
--- linux-2.6.21-rc5-mm4.orig/drivers/ide/setup-pci.c
+++ linux-2.6.21-rc5-mm4/drivers/ide/setup-pci.c
@@ -874,9 +874,13 @@ void __init ide_scan_pcibus (int scan_di
 
 	list_for_each_safe(l, n, &ide_pci_drivers)
 	{
+		int err;
 		list_del(l);
 		d = list_entry(l, struct pci_driver, node);
-		__pci_register_driver(d, d->driver.owner, d->driver.mod_name);
+		err = __pci_register_driver(d, d->driver.owner, d->driver.mod_name);
+		if (err)
+			printk(KERN_ERR "%s: failed to register PCI driver\n",
+				d->driver.mod_name);
 	}
 }
 #endif

Re: [PATCH] IDE: fix PCI must_checks

[Alan Cox]

On Wed, 4 Apr 2007 21:37:04 -0700
Randy Dunlap <randy.dunlap@oracle.com> wrote:

> From: Randy Dunlap <randy.dunlap@oracle.com>
> 
> Check PCI interface function results for errors in drivers/ide/pci.
> 
> drivers/ide/pci/cs5530.c:244: warning: ignoring return value of 'pci_set_mwi', declared with attribute warn_unused_result

NAK (again) 

The bug is in pci_set_mwi, it should not force return value checking.
Please fix that instead.

Re: [PATCH] IDE: fix PCI must_checks

[Andrew Morton]

On Thu, 5 Apr 2007 12:13:02 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Wed, 4 Apr 2007 21:37:04 -0700
> Randy Dunlap <randy.dunlap@oracle.com> wrote:
> 
> > From: Randy Dunlap <randy.dunlap@oracle.com>
> > 
> > Check PCI interface function results for errors in drivers/ide/pci.
> > 
> > drivers/ide/pci/cs5530.c:244: warning: ignoring return value of 'pci_set_mwi', declared with attribute warn_unused_result
> 
> NAK (again) 

Disagree.

> The bug is in pci_set_mwi, it should not force return value checking.
> Please fix that instead.

You don't describe your reasoning, but I will assume that it is as follows:

pci_set_mwi() is an advisory thing, and on certain platforms it might fail
to set the cacheline size to the desired number.  This is not a fatal error
and the driver can successfully run at a lesser performance level.

If that description is accurate then I'd contend that pci_set_mwi() is
misdesigned.  It should not be returning a negative error code for
something which is not an error.

The changes we should make are

a) rename pci_set_pwi() to pci_try_set_mwi()

b) make pci_try_set_mwi() return a positive, non-zero code when the
   attempt to set the cacheline size failed.  And make it return a
   negative error code when fatal errors occur.

This way

a) drivers which _do_ care about whether or not the cacheline size was
   set successfully can test for it or generate warnings and

b) we can tell when pci_try_set_mwi() encountered a fatal error for
   other reasons.


Now this may all sound excessively anal, but it is simply the correct design.
And one of the reasons for adding the __must_check annotation is to weed out
design errors.  Simply turning the thing off is a step backwards.

And we *need* to be excessively anal in the PCI setup code.  We have metric
shitloads of bugs due to problems in that area, and the more formality and
error handling and error reporting we can get in there the better off we
will be.

Re: [PATCH] IDE: fix PCI must_checks

[Alan Cox]

> pci_set_mwi() is an advisory thing, and on certain platforms it might fail
> to set the cacheline size to the desired number.  This is not a fatal error
> and the driver can successfully run at a lesser performance level.

Correct.

> If that description is accurate then I'd contend that pci_set_mwi() is
> misdesigned.  It should not be returning a negative error code for
> something which is not an error.

It is an error to *some* drivers but not all. Kind of like setting some
of the other features may be essential for some chips and not others.

> And we *need* to be excessively anal in the PCI setup code.  We have metric
> shitloads of bugs due to problems in that area, and the more formality and
> error handling and error reporting we can get in there the better off we
> will be.

No argument there

If we want to deal with some of the mess we should also remove all direct
writing of PCI latency timers and replace them with a function to stop
drivers setting unsafe values and ignoring chip errata the core knows
about but they dont

Re: [PATCH] IDE: fix PCI must_checks

[Andrew Morton]

On Thu, 5 Apr 2007 21:16:09 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > pci_set_mwi() is an advisory thing, and on certain platforms it might fail
> > to set the cacheline size to the desired number.  This is not a fatal error
> > and the driver can successfully run at a lesser performance level.
> 
> Correct.
> 
> > If that description is accurate then I'd contend that pci_set_mwi() is
> > misdesigned.  It should not be returning a negative error code for
> > something which is not an error.
> 
> It is an error to *some* drivers but not all. Kind of like setting some
> of the other features may be essential for some chips and not others.
> 
> > And we *need* to be excessively anal in the PCI setup code.  We have metric
> > shitloads of bugs due to problems in that area, and the more formality and
> > error handling and error reporting we can get in there the better off we
> > will be.
> 
> No argument there
> 
> If we want to deal with some of the mess we should also remove all direct
> writing of PCI latency timers and replace them with a function to stop
> drivers setting unsafe values and ignoring chip errata the core knows
> about but they dont

hm. Well, what to do?

How about we prevail upon Randy to:

- rename pci_set_mwi() to pci_try_set_mwi()

- make it return 0 on success, 1 if the "try" failed

- make it return -EFOO on error (presently unimplemented)

- review callers

- remove __must_check

?



I have a feeling that this is "still wrong": even things like
pci_read_config_foo() can get errors, and there are various PCI-bus error
handling proposals floating about which might require that callers be more
careful in what they accept.

Re: [PATCH] IDE: fix PCI must_checks

[Randy Dunlap]

Andrew Morton wrote:
> On Thu, 5 Apr 2007 21:16:09 +0100
> Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> 
>>> pci_set_mwi() is an advisory thing, and on certain platforms it might fail
>>> to set the cacheline size to the desired number.  This is not a fatal error
>>> and the driver can successfully run at a lesser performance level.
>> Correct.
>>
>>> If that description is accurate then I'd contend that pci_set_mwi() is
>>> misdesigned.  It should not be returning a negative error code for
>>> something which is not an error.
>> It is an error to *some* drivers but not all. Kind of like setting some
>> of the other features may be essential for some chips and not others.
>>
>>> And we *need* to be excessively anal in the PCI setup code.  We have metric
>>> shitloads of bugs due to problems in that area, and the more formality and
>>> error handling and error reporting we can get in there the better off we
>>> will be.
>> No argument there
>>
>> If we want to deal with some of the mess we should also remove all direct
>> writing of PCI latency timers and replace them with a function to stop
>> drivers setting unsafe values and ignoring chip errata the core knows
>> about but they dont
> 
> hm. Well, what to do?
> 
> How about we prevail upon Randy to:
> 
> - rename pci_set_mwi() to pci_try_set_mwi()
> 
> - make it return 0 on success, 1 if the "try" failed
> 
> - make it return -EFOO on error (presently unimplemented)
> 
> - review callers
> 
> - remove __must_check
> 
> ?

That's fine with me.  Any comments on that, Alan?

I'm not sure that I like the proposed return values of
pci_try_set_mwi(), but I'm easy.

> I have a feeling that this is "still wrong": even things like
> pci_read_config_foo() can get errors, and there are various PCI-bus error
> handling proposals floating about which might require that callers be more
> careful in what they accept.


-- 
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [PATCH] IDE: fix PCI must_checks

[Alan Cox]

> That's fine with me.  Any comments on that, Alan?
> 
> I'm not sure that I like the proposed return values of
> pci_try_set_mwi(), but I'm easy.

Ditto on the return codes - we can just return -EOPNOTSUPP for
unsupported stuff and be saner.

[RFC/PATCH -mm] add pci_try_set_mwi

[Randy Dunlap]

On Thu, 5 Apr 2007 14:20:01 -0700 Andrew Morton wrote:

> hm. Well, what to do?
> 
> How about we prevail upon Randy to:
> 
> - rename pci_set_mwi() to pci_try_set_mwi()
> 
> - make it return 0 on success, 1 if the "try" failed
> 
> - make it return -EFOO on error (presently unimplemented)
> 
> - review callers
> 
> - remove __must_check
> 
> ?



From: Randy Dunlap <randy.dunlap@oracle.com>

As suggested by Andrew, add pci_try_set_mwi(), which does not require
return-value checking.

- add pci_try_set_mwi() without __must_check
- make it return 0 on success, errno if the "try" failed or error
- review callers

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 Documentation/pci.txt                          |    5 +++-
 drivers/ata/pata_cs5530.c                      |    2 -
 drivers/ide/pci/cs5530.c                       |    2 -
 drivers/net/cassini.c                          |    4 +--
 drivers/net/starfire.c                         |    2 -
 drivers/net/tulip/tulip_core.c                 |    2 -
 drivers/net/wireless/mac80211/p54/prism54pci.c |    2 -
 drivers/net/wireless/prism54/islpci_hotplug.c  |    3 --
 drivers/pci/pci.c                              |   28 +++++++++++++++++++++----
 drivers/scsi/lpfc/lpfc_init.c                  |    5 ----
 drivers/usb/gadget/net2280.c                   |    2 -
 include/linux/pci.h                            |    1 
 12 files changed, 39 insertions(+), 19 deletions(-)

--- linux-2.6.21-rc6-mm1.orig/include/linux/pci.h
+++ linux-2.6.21-rc6-mm1/include/linux/pci.h
@@ -547,6 +547,7 @@ void pci_set_master(struct pci_dev *dev)
 int pci_set_pcie_reset_state(struct pci_dev *dev, enum pcie_reset_state state);
 #define HAVE_PCI_SET_MWI
 int __must_check pci_set_mwi(struct pci_dev *dev);
+int pci_try_set_mwi(struct pci_dev *dev);
 void pci_clear_mwi(struct pci_dev *dev);
 void pci_intx(struct pci_dev *dev, int enable);
 void pci_msi_off(struct pci_dev *dev);
--- linux-2.6.21-rc6-mm1.orig/drivers/pci/pci.c
+++ linux-2.6.21-rc6-mm1/drivers/pci/pci.c
@@ -1162,6 +1162,11 @@ int pci_set_mwi(struct pci_dev *dev)
 	return 0;
 }
 
+int pci_try_set_mwi(struct pci_dev *dev)
+{
+	return 0;
+}
+
 void pci_clear_mwi(struct pci_dev *dev)
 {
 }
@@ -1218,9 +1223,7 @@ pci_set_cacheline_size(struct pci_dev *d
  * pci_set_mwi - enables memory-write-invalidate PCI transaction
  * @dev: the PCI device for which MWI is enabled
  *
- * Enables the Memory-Write-Invalidate transaction in %PCI_COMMAND,
- * and then calls @pcibios_set_mwi to do the needed arch specific
- * operations or a generic mwi-prep function.
+ * Enables the Memory-Write-Invalidate transaction in %PCI_COMMAND.
  *
  * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
  */
@@ -1236,7 +1239,8 @@ pci_set_mwi(struct pci_dev *dev)
 
 	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));
+		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);
 	}
@@ -1245,6 +1249,21 @@ pci_set_mwi(struct pci_dev *dev)
 }
 
 /**
+ * pci_try_set_mwi - enables memory-write-invalidate PCI transaction
+ * @dev: the PCI device for which MWI is enabled
+ *
+ * Enables the Memory-Write-Invalidate transaction in %PCI_COMMAND.
+ * Callers are not required to check the return value.
+ *
+ * RETURNS: An appropriate -ERRNO error value on error, or zero for success.
+ */
+int pci_try_set_mwi(struct pci_dev *dev)
+{
+	int rc = pci_set_mwi(dev);
+	return rc;
+}
+
+/**
  * pci_clear_mwi - disables Memory-Write-Invalidate for device dev
  * @dev: the PCI device to disable
  *
@@ -1418,6 +1437,7 @@ EXPORT_SYMBOL(pci_release_selected_regio
 EXPORT_SYMBOL(pci_request_selected_regions);
 EXPORT_SYMBOL(pci_set_master);
 EXPORT_SYMBOL(pci_set_mwi);
+EXPORT_SYMBOL(pci_try_set_mwi);
 EXPORT_SYMBOL(pci_clear_mwi);
 EXPORT_SYMBOL_GPL(pci_intx);
 EXPORT_SYMBOL(pci_set_dma_mask);
--- linux-2.6.21-rc6-mm1.orig/drivers/ata/pata_cs5530.c
+++ linux-2.6.21-rc6-mm1/drivers/ata/pata_cs5530.c
@@ -270,7 +270,7 @@ static int cs5530_init_chip(void)
 	}
 
 	pci_set_master(cs5530_0);
-	pci_set_mwi(cs5530_0);
+	pci_try_set_mwi(cs5530_0);
 
 	/*
 	 * Set PCI CacheLineSize to 16-bytes:
--- linux-2.6.21-rc6-mm1.orig/drivers/ide/pci/cs5530.c
+++ linux-2.6.21-rc6-mm1/drivers/ide/pci/cs5530.c
@@ -236,7 +236,7 @@ static unsigned int __devinit init_chips
 	 */
 
 	pci_set_master(cs5530_0);
-	pci_set_mwi(cs5530_0);
+	pci_try_set_mwi(cs5530_0);
 
 	/*
 	 * Set PCI CacheLineSize to 16-bytes:
--- linux-2.6.21-rc6-mm1.orig/drivers/usb/gadget/net2280.c
+++ linux-2.6.21-rc6-mm1/drivers/usb/gadget/net2280.c
@@ -2964,7 +2964,7 @@ static int net2280_probe (struct pci_dev
 			, &dev->pci->pcimstctl);
 	/* erratum 0115 shouldn't appear: Linux inits PCI_LATENCY_TIMER */
 	pci_set_master (pdev);
-	pci_set_mwi (pdev);
+	pci_try_set_mwi (pdev);
 
 	/* ... also flushes any posted pci writes */
 	dev->chiprev = get_idx_reg (dev->regs, REG_CHIPREV) & 0xffff;
--- linux-2.6.21-rc6-mm1.orig/drivers/scsi/lpfc/lpfc_init.c
+++ linux-2.6.21-rc6-mm1/drivers/scsi/lpfc/lpfc_init.c
@@ -1494,10 +1494,7 @@ lpfc_pci_probe_one(struct pci_dev *pdev,
 
 
 	pci_set_master(pdev);
-	retval = pci_set_mwi(pdev);
-	if (retval)
-		dev_printk(KERN_WARNING, &pdev->dev,
-			   "Warning: pci_set_mwi returned %d\n", retval);
+	pci_try_set_mwi(pdev);
 
 	if (pci_set_dma_mask(phba->pcidev, DMA_64BIT_MASK) != 0)
 		if (pci_set_dma_mask(phba->pcidev, DMA_32BIT_MASK) != 0)
--- linux-2.6.21-rc6-mm1.orig/drivers/net/wireless/prism54/islpci_hotplug.c
+++ linux-2.6.21-rc6-mm1/drivers/net/wireless/prism54/islpci_hotplug.c
@@ -167,8 +167,7 @@ prism54_probe(struct pci_dev *pdev, cons
 	pci_set_master(pdev);
 
 	/* enable MWI */
-	if (!pci_set_mwi(pdev))
-		printk(KERN_INFO "%s: pci_set_mwi(pdev) succeeded\n", DRV_NAME);
+	pci_try_set_mwi(pdev);
 
 	/* setup the network device interface and its structure */
 	if (!(ndev = islpci_setup(pdev))) {
--- linux-2.6.21-rc6-mm1.orig/drivers/net/cassini.c
+++ linux-2.6.21-rc6-mm1/drivers/net/cassini.c
@@ -4919,10 +4919,10 @@ static int __devinit cas_init_one(struct
 	pci_cmd &= ~PCI_COMMAND_SERR;
 	pci_cmd |= PCI_COMMAND_PARITY;
 	pci_write_config_word(pdev, PCI_COMMAND, pci_cmd);
-	pci_set_mwi(pdev);
+	pci_try_set_mwi(pdev);
 	/*
 	 * On some architectures, the default cache line size set
-	 * by pci_set_mwi reduces perforamnce.  We have to increase
+	 * by pci_set_mwi reduces performance.  We have to increase
 	 * it for this case.  To start, we'll print some configuration
 	 * data.
 	 */
--- linux-2.6.21-rc6-mm1.orig/drivers/net/starfire.c
+++ linux-2.6.21-rc6-mm1/drivers/net/starfire.c
@@ -740,7 +740,7 @@ static int __devinit starfire_init_one(s
 	pci_set_master(pdev);
 
 	/* enable MWI -- it vastly improves Rx performance on sparc64 */
-	pci_set_mwi(pdev);
+	pci_try_set_mwi(pdev);
 
 #ifdef ZEROCOPY
 	/* Starfire can do TCP/UDP checksumming */
--- linux-2.6.21-rc6-mm1.orig/drivers/net/tulip/tulip_core.c
+++ linux-2.6.21-rc6-mm1/drivers/net/tulip/tulip_core.c
@@ -1155,7 +1155,7 @@ 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);
+	if (csr0 & MWI)	pci_try_set_mwi(pdev);
 	else		pci_clear_mwi(pdev);
 
 	/* read result from hardware (in case bit refused to enable) */
--- linux-2.6.21-rc6-mm1.orig/drivers/net/wireless/mac80211/p54/prism54pci.c
+++ linux-2.6.21-rc6-mm1/drivers/net/wireless/mac80211/p54/prism54pci.c
@@ -539,7 +539,7 @@ static int __devinit p54p_probe(struct p
 	}
 
 	pci_set_master(pdev);
-	pci_set_mwi(pdev);
+	pci_try_set_mwi(pdev);
 
 	pci_write_config_byte(pdev, 0x40, 0);
 	pci_write_config_byte(pdev, 0x41, 0);
--- linux-2.6.21-rc6-mm1.orig/Documentation/pci.txt
+++ linux-2.6.21-rc6-mm1/Documentation/pci.txt
@@ -299,7 +299,10 @@ If the PCI device can use the PCI Memory
 call pci_set_mwi().  This enables the PCI_COMMAND bit for Mem-Wr-Inval
 and also ensures that the cache line size register is set correctly.
 Check the return value of pci_set_mwi() as not all architectures
-or chip-sets may support Memory-Write-Invalidate.
+or chip-sets may support Memory-Write-Invalidate.  Alternatively,
+if Mem-Wr-Inval would be nice to have but is not required, call
+pci_try_set_mwi() to have the system do its best effort at enabling
+Mem-Wr-Inval.
 
 
 3.2 Request MMIO/IOP resources