Patchwork [03/33] sfc: Do not use pci_disable_device() to disable bus mastering

login
register
mail settings
Submitter Ben Hutchings
Date Dec. 12, 2008, 12:49 p.m.
Message ID <20081212124913.GC10372@solarflare.com>
Download mbox | patch
Permalink /patch/13705/
State Rejected
Delegated to: David Miller
Headers show

Comments

Ben Hutchings - Dec. 12, 2008, 12:49 p.m.
pci_disable_device() disables many features, like MSI-X, which we
never reenable in efx_reset().  Further, calls to pci_enable_device()
and pci_disable_device() must be matched since the nesting count was
introduced.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/falcon.c |   15 +++++++++++++--
 1 files changed, 13 insertions(+), 2 deletions(-)
David Miller - Dec. 13, 2008, 5:31 a.m.
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Fri, 12 Dec 2008 12:49:14 +0000

> pci_disable_device() disables many features, like MSI-X, which we
> never reenable in efx_reset().  Further, calls to pci_enable_device()
> and pci_disable_device() must be matched since the nesting count was
> introduced.
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>

Since the PCI layer provides interfaces which do manage
this setting, there is no way you should be doing this
behind it's back.

You know why?

Because someone is going to change some of the internals
of this stuff in the PIC layer and it will break your
driver.

Please instead submit an interface to the PCI layer
maintainers that does what you want, then use it.

Patch not applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ben Hutchings - Dec. 13, 2008, 6:27 a.m.
David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Fri, 12 Dec 2008 12:49:14 +0000
> 
> > pci_disable_device() disables many features, like MSI-X, which we
> > never reenable in efx_reset().  Further, calls to pci_enable_device()
> > and pci_disable_device() must be matched since the nesting count was
> > introduced.
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> 
> Since the PCI layer provides interfaces which do manage
> this setting, there is no way you should be doing this
> behind it's back.
> 
> You know why?
> 
> Because someone is going to change some of the internals
> of this stuff in the PIC layer and it will break your
> driver.
> 
> Please instead submit an interface to the PCI layer
> maintainers that does what you want, then use it.

I did ask on #linux-pci why this didn't exist; apparently it wasn't
considered to be worth providing.  So obviously I can submit a patch to
the PCI layer but I'm not sure it would be accepted.

Ben.

> Patch not applied.

Patch

diff --git a/drivers/net/sfc/falcon.c b/drivers/net/sfc/falcon.c
index 271cbf8..ac0b671 100644
--- a/drivers/net/sfc/falcon.c
+++ b/drivers/net/sfc/falcon.c
@@ -1344,6 +1344,17 @@  static inline void falcon_irq_ack_a1(struct efx_nic *efx)
 	falcon_readl(efx, &reg, WORK_AROUND_BROKEN_PCI_READS_REG_KER_A1);
 }
 
+static void efx_pci_clear_master(struct pci_dev *dev)
+{
+	u16 command;
+
+	if (dev->is_busmaster &&
+	    !pci_read_config_word(dev, PCI_COMMAND, &command) &&
+	    !pci_write_config_word(dev, PCI_COMMAND,
+				   command & ~PCI_COMMAND_MASTER))
+		dev->is_busmaster = false;
+}
+
 /* Process a fatal interrupt
  * Disable bus mastering ASAP and schedule a reset
  */
@@ -1375,9 +1386,9 @@  static irqreturn_t falcon_fatal_interrupt(struct efx_nic *efx)
 	}
 
 	/* Disable both devices */
-	pci_disable_device(efx->pci_dev);
+	efx_pci_clear_master(efx->pci_dev);
 	if (FALCON_IS_DUAL_FUNC(efx))
-		pci_disable_device(nic_data->pci_dev2);
+		efx_pci_clear_master(nic_data->pci_dev2);
 	falcon_disable_interrupts(efx);
 
 	if (++n_int_errors < FALCON_MAX_INT_ERRORS) {