[PATCH 1/12] drivers/isdn/hisax/avm_pci.c: replace pci_find_device with pci_get_device

[PATCH 1/12] drivers/isdn/hisax/avm_pci.c: replace pci_find_device with pci_get_device

[S.Çağlar Onur]

Following patch replaces pci_find_device with pci_get_device to avoid 
following compiliation warning;

drivers/isdn/hisax/avm_pci.c: In function `setup_avm_pcipnp':
drivers/isdn/hisax/avm_pci.c:792: warning: `pci_find_device' is deprecated 
(declared at include/linux/pci.h:478)

Signed-off-by: S.Çağlar Onur <caglar@pardus.org.tr>

---
 drivers/isdn/hisax/avm_pci.c |   17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

Index: linux-2.6/drivers/isdn/hisax/avm_pci.c
===================================================================
--- linux-2.6.orig/drivers/isdn/hisax/avm_pci.c
+++ linux-2.6/drivers/isdn/hisax/avm_pci.c
@@ -789,19 +789,19 @@ setup_avm_pcipnp(struct IsdnCard *card)
 	}
 #endif
 #ifdef CONFIG_PCI
-	if ((dev_avm = pci_find_device(PCI_VENDOR_ID_AVM,
+	if ((dev_avm = pci_get_device(PCI_VENDOR_ID_AVM,
 		PCI_DEVICE_ID_AVM_A1,  dev_avm))) {
 		if (pci_enable_device(dev_avm))
-			return(0);
+			goto dev_avm_cleanup;
 		cs->irq = dev_avm->irq;
 		if (!cs->irq) {
 			printk(KERN_ERR "FritzPCI: No IRQ for PCI card found\n");
-			return(0);
+			goto dev_avm_cleanup;
 		}
 		cs->hw.avm.cfg_reg = pci_resource_start(dev_avm, 1);
 		if (!cs->hw.avm.cfg_reg) {
 			printk(KERN_ERR "FritzPCI: No IO-Adr for PCI card found\n");
-			return(0);
+			goto dev_avm_cleanup;
 		}
 		cs->subtyp = AVM_FRITZ_PCI;
 	} else {
@@ -822,7 +822,7 @@ ready:
 		       CardType[card->typ],
 		       cs->hw.avm.cfg_reg,
 		       cs->hw.avm.cfg_reg + 31);
-		return (0);
+		goto dev_avm_cleanup;
 	}
 	switch (cs->subtyp) {
 	  case AVM_FRITZ_PCI:
@@ -842,7 +842,7 @@ ready:
 		break;
 	  default:
 	  	printk(KERN_WARNING "AVM unknown subtype %d\n", cs->subtyp);
-	  	return(0);
+		goto dev_avm_cleanup;
 	}
 	printk(KERN_INFO "HiSax: %s config irq:%d base:0x%X\n",
 		(cs->subtyp == AVM_FRITZ_PCI) ? "AVM Fritz!PCI" : "AVM Fritz!PnP",
@@ -858,5 +858,10 @@ ready:
 	cs->irq_func = &avm_pcipnp_interrupt;
 	cs->writeisac(cs, ISAC_MASK, 0xFF);
 	ISACVersion(cs, (cs->subtyp == AVM_FRITZ_PCI) ? "AVM PCI:" : "AVM PnP:");
+	pci_dev_put(dev_avm);
 	return (1);
+
+dev_avm_cleanup:
+	pci_dev_put(dev_avm);
+	return (0);
 }

-- 
S.Çağlar Onur <caglar@pardus.org.tr>
http://cekirdek.pardus.org.tr/~caglar/

Linux is like living in a teepee. No Windows, no Gates and an Apache in house!

Re: [PATCH 1/12] drivers/isdn/hisax/avm_pci.c: replace pci_find_device with pci_get_device

[Jeff Garzik]

S.Çağlar Onur wrote:
> @@ -858,5 +858,10 @@ ready:
>  	cs->irq_func = &avm_pcipnp_interrupt;
>  	cs->writeisac(cs, ISAC_MASK, 0xFF);
>  	ISACVersion(cs, (cs->subtyp == AVM_FRITZ_PCI) ? "AVM PCI:" : "AVM PnP:");
> +	pci_dev_put(dev_avm);
>  	return (1);
> +
> +dev_avm_cleanup:
> +	pci_dev_put(dev_avm);
> +	return (0);
>  }


NAK -- every single one of these patches is wrong.  All you did was make 
the warning go away, while INTRODUCING new lifetime problems.

The ISDN PCI driver obviously continues execution after the setup 
function ends, yet you pci_dev_put() the device at the end of setup.  As 
a result, no reference is held even though we continue using the device.

	Jeff

Re: [PATCH 1/12] drivers/isdn/hisax/avm_pci.c: replace pci_find_device with pci_get_device

[Jeff Garzik]

surya.prabhakar@wipro.com wrote:
> 
> 
> S.Çaglar Onur wrote:
>  > @@ -858,5 +858,10 @@ ready:
>  >       cs->irq_func = &avm_pcipnp_interrupt;
>  >       cs->writeisac(cs, ISAC_MASK, 0xFF);
>  >       ISACVersion(cs, (cs->subtyp == AVM_FRITZ_PCI) ? "AVM PCI:" : 
> "AVM PnP:");
>  > +     pci_dev_put(dev_avm);
>  >       return (1);
>  > +
>  > +dev_avm_cleanup:
>  > +     pci_dev_put(dev_avm);
>  > +     return (0);
>  >  }
> 
> 
>  >NAK -- every single one of these patches is wrong.  All you did was make
>  >the warning go away, while INTRODUCING new lifetime problems.
> 
>  >The ISDN PCI driver obviously continues execution after the setup
>  >function ends, yet you pci_dev_put() the device at the end of setup.  As
>  >a result, no reference is held even though we continue using the device.
> 
> This series of patches were initially released by me and were also 
> accepted by kkeil.
> It was ack'ed by keil and is now in -mm tree list of andrew morton.
> http://marc.info/?l=linux-kernel&m=118436681515269&w=2 
> <http://marc.info/?l=linux-kernel&m=118436681515269&w=2>
> 
> There are many other drivers with similar implementation. Please have a 
> look at
> linux2.6/drivers/char/sonypi.c , ide/pci/cs5530.c etc..
> 
> Moreover my earlier patch for sound/oss/trident.c was ack'ed by Alan cox 
> and is now in the
> current kernel with similar implementation.

Simple question:  where is a reference -held-, when the PCI device is in 
use?

If you cannot show this, your patch is provably wrong.  And I did not 
see a reference kept in any of your patches.

If you can show me where a reference is stored, I rescind my NAK. 
Otherwise, the patches are demonstrably, provably incorrect.

ACKs cannot get around the -facts-.

	Jeff

Re: [PATCH 1/12] drivers/isdn/hisax/avm_pci.c: replace pci_find_device with pci_get_device

[Jeff Garzik]

To be more clear, your solution is incorrect unless the pci_dev_put() 
occurs after the last reference to hw.{elsa,diva,hfc,njet,...}.dev, 
which is where the HiSax ISDN drivers store their reference to struct 
pci_dev during the runtime life of the PCI device.

Am I missing where your patch does this?

By way of further interest, a few hours _before_ (yes, really) I saw 
your patches, I resumed converting the ISDN HiSax PCI drivers to use the 
PCI driver API.  You can find this work in 
git://git.kernel.org/.../jgarzik/misc-2.6.git#isdn-pci.

If you fix your patches' lifetime problems, I will ACK them myself, 
since my effort is a spare time effort.  But just wanted you to be aware 
that I am deep into the code you are fixing, and can at least speak 
somewhat knowledgeably on the specific lines of code you are changing.

	Jeff

Re: [PATCH 1/12] drivers/isdn/hisax/avm_pci.c: replace pci_find_device with pci_get_device

[Surya Prabhakar N]

On Sun, 2007-07-15 at 03:40 -0400, Jeff Garzik wrote:
> To be more clear, your solution is incorrect unless the pci_dev_put() 
> occurs after the last reference to hw.{elsa,diva,hfc,njet,...}.dev, 
> which is where the HiSax ISDN drivers store their reference to struct 
> pci_dev during the runtime life of the PCI device.
> 
> Am I missing where your patch does this?
It is really missing :-(
> 
> By way of further interest, a few hours _before_ (yes, really) I saw 
> your patches, I resumed converting the ISDN HiSax PCI drivers to use the 
> PCI driver API.  You can find this work in 
> git://git.kernel.org/.../jgarzik/misc-2.6.git#isdn-pci.
> 
> If you fix your patches' lifetime problems, I will ACK them myself, 
> since my effort is a spare time effort.  But just wanted you to be aware 
> that I am deep into the code you are fixing, and can at least speak 
> somewhat knowledgeably on the specific lines of code you are changing.
I can see the bug. Thanks for updating me. I really missed the point
that these whole set of drivers are a part of hisax.ko...

most of the initialization is done in hisax/config.c
Now are you suggesting that we should have a pci_dev_put in config.c
where you have the module_exit.

I am not sure where I can call the dev_put.
consider hisax/avm_pci.c  

in which if we have a code like the below
dev_avm = pci_get_device(PCI_VENDOR_ID_AVM,
		PCI_DEVICE_ID_AVM_A1,  dev_avm))) {

dev_avm is getting initialized in this file 
static struct pci_dev *dev_avm __devinitdata = NULL;

so can I do an
extern static struct pci_dev *dev_avm; 
in the config.c 

and call pci_dev_put(dev_avm) in config.c file's module_exit or
whereever there is an error return?


> 
> 	Jeff
> 
> 
> 
-surya.