[patch] pciehp: dont call pci_enable_dev

[patch] pciehp: dont call pci_enable_dev

[Kristen Accardi]

Don't call pci_enable_device from pciehp because the pcie port service driver
already does this.

Signed-off-by: Kristen Carlson Accardi <kristen.c.accardi@intel.com>
---
 drivers/pci/hotplug/pciehp_hpc.c |    3 ---
 1 files changed, 3 deletions(-)

--- 2.6-git-pcie.orig/drivers/pci/hotplug/pciehp_hpc.c
+++ 2.6-git-pcie/drivers/pci/hotplug/pciehp_hpc.c
@@ -1404,9 +1404,6 @@ int pcie_init(struct controller * ctrl, 
 	info("HPC vendor_id %x device_id %x ss_vid %x ss_did %x\n", pdev->vendor, pdev->device, 
 		pdev->subsystem_vendor, pdev->subsystem_device);
 
-	if (pci_enable_device(pdev))
-		goto abort_free_ctlr;
-	
 	mutex_init(&ctrl->crit_sect);
 	/* setup wait queue */
 	init_waitqueue_head(&ctrl->queue);

Re: [patch] pciehp: dont call pci_enable_dev

[Arjan van de Ven]

On Mon, 2006-04-24 at 15:50 -0700, Kristen Accardi wrote:
> Don't call pci_enable_device from pciehp because the pcie port service driver
> already does this.

hmmmm shouldn't pci_enable_device on a previously enabled device just
succeed? Sounds more than logical to me to make it that way at least...

Re: [patch] pciehp: dont call pci_enable_dev

[Kristen Accardi]

On Tue, 2006-04-25 at 08:16 +0200, Arjan van de Ven wrote:
> On Mon, 2006-04-24 at 15:50 -0700, Kristen Accardi wrote:
> > Don't call pci_enable_device from pciehp because the pcie port service driver
> > already does this.
> 
> hmmmm shouldn't pci_enable_device on a previously enabled device just
> succeed? Sounds more than logical to me to make it that way at least...

I can't think of any reason why not.  Something like this what you had
in mind perhaps?

---
 drivers/pci/pci.c |   14 +++++++++-----
 1 files changed, 9 insertions(+), 5 deletions(-)

--- 2.6-git-pcie.orig/drivers/pci/pci.c
+++ 2.6-git-pcie/drivers/pci/pci.c
@@ -504,11 +504,15 @@ pci_enable_device_bars(struct pci_dev *d
 int
 pci_enable_device(struct pci_dev *dev)
 {
-	int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
-	if (err)
-		return err;
-	pci_fixup_device(pci_fixup_enable, dev);
-	dev->is_enabled = 1;
+	int err;
+
+	if (!dev->is_enabled) {
+		err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
+		if (err)
+			return err;
+		pci_fixup_device(pci_fixup_enable, dev);
+		dev->is_enabled = 1;
+	}
 	return 0;
 }
 

Re: [Pcihpd-discuss] Re: [patch] pciehp: dont call pci_enable_dev

[Rolf Eike Beer]

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

Am Mittwoch, 26. April 2006 00:00 schrieb Kristen Accardi:
>On Tue, 2006-04-25 at 08:16 +0200, Arjan van de Ven wrote:
>> On Mon, 2006-04-24 at 15:50 -0700, Kristen Accardi wrote:
>> > Don't call pci_enable_device from pciehp because the pcie port service
>> > driver already does this.
>>
>> hmmmm shouldn't pci_enable_device on a previously enabled device just
>> succeed? Sounds more than logical to me to make it that way at least...
>
>I can't think of any reason why not.  Something like this what you had
>in mind perhaps?
>
>---
> drivers/pci/pci.c |   14 +++++++++-----
> 1 files changed, 9 insertions(+), 5 deletions(-)
>
>--- 2.6-git-pcie.orig/drivers/pci/pci.c
>+++ 2.6-git-pcie/drivers/pci/pci.c
>@@ -504,11 +504,15 @@ pci_enable_device_bars(struct pci_dev *d
> int
> pci_enable_device(struct pci_dev *dev)
> {
>-	int err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
>-	if (err)
>-		return err;
>-	pci_fixup_device(pci_fixup_enable, dev);
>-	dev->is_enabled = 1;
>+	int err;
>+
>+	if (!dev->is_enabled) {
>+		err = pci_enable_device_bars(dev, (1 << PCI_NUM_RESOURCES) - 1);
>+		if (err)
>+			return err;
>+		pci_fixup_device(pci_fixup_enable, dev);
>+		dev->is_enabled = 1;
>+	}
> 	return 0;
> }

What about

if (dev->is_enabled)
	return 0;

and leaving the rest as it is? This would save one level of identation. 
Opinions?

Eike

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [patch] pciehp: dont call pci_enable_dev

[Arjan van de Ven]

On Tue, 2006-04-25 at 15:00 -0700, Kristen Accardi wrote:
> On Tue, 2006-04-25 at 08:16 +0200, Arjan van de Ven wrote:
> > On Mon, 2006-04-24 at 15:50 -0700, Kristen Accardi wrote:
> > > Don't call pci_enable_device from pciehp because the pcie port service driver
> > > already does this.
> > 
> > hmmmm shouldn't pci_enable_device on a previously enabled device just
> > succeed? Sounds more than logical to me to make it that way at least...
> 
> I can't think of any reason why not.  Something like this what you had
> in mind perhaps?
> 
> ---

the question then becomes if enable/disable should become "counting", eg
enable twice disable once leaves enabled at count one....

Re: [patch] pciehp: dont call pci_enable_dev

[Kristen Accardi]

On Wed, 2006-04-26 at 17:52 +0200, Arjan van de Ven wrote:
> On Tue, 2006-04-25 at 15:00 -0700, Kristen Accardi wrote:
> > On Tue, 2006-04-25 at 08:16 +0200, Arjan van de Ven wrote:
> > > On Mon, 2006-04-24 at 15:50 -0700, Kristen Accardi wrote:
> > > > Don't call pci_enable_device from pciehp because the pcie port service driver
> > > > already does this.
> > > 
> > > hmmmm shouldn't pci_enable_device on a previously enabled device just
> > > succeed? Sounds more than logical to me to make it that way at least...
> > 
> > I can't think of any reason why not.  Something like this what you had
> > in mind perhaps?
> > 
> > ---
> 
> the question then becomes if enable/disable should become "counting", eg
> enable twice disable once leaves enabled at count one....

ugh, no.  1) I think we should avoid adding more counting unless it's
absolutely necessary. 2) if a device calls pci_disable_device it should
always actually disable the device, because it is generally called in
drivers either when the device is being shutdown, or suspended.