[Kernel-janitors] Re: [PATCH -mm 7/9] pci_find_device to

[Kernel-janitors] Re: [PATCH -mm 7/9] pci_find_device to

[Greg KH]

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

On Mon, Sep 20, 2004 at 11:53:06PM -0700, Scott Feldman wrote:
> Use pci_get_device instead of pci_find_device as pci device returned
> from pci_find_device can disappear at any momemt in time.
> 
> Signed-off-by: Scott Feldman <sfeldma@pobox.com>
> 
> diff -Naurp linux-2.6.9-rc2-mm1/drivers/net/tg3.c linux-2.6.9-rc2-mm1-dsf/drivers/net/tg3.c
> --- linux-2.6.9-rc2-mm1/drivers/net/tg3.c	2004-09-20 22:36:25.000865840 -0700
> +++ linux-2.6.9-rc2-mm1-dsf/drivers/net/tg3.c	2004-09-20 22:39:02.362943168 -0700
> @@ -7325,6 +7325,7 @@ static int __devinit tg3_get_invariants(
>  	u32 pci_state_reg, grc_misc_cfg;
>  	u32 val;
>  	u16 pci_cmd;
> +	struct pci_dev *pdev;
>  	int err;
>  
>  #ifdef CONFIG_SPARC64
> @@ -7338,17 +7339,19 @@ static int __devinit tg3_get_invariants(
>  	 * every mailbox register write to force the writes to be
>  	 * posted to the chip in order.
>  	 */
> -	if (pci_find_device(PCI_VENDOR_ID_INTEL,
> -			    PCI_DEVICE_ID_INTEL_82801AA_8, NULL) ||
> -	    pci_find_device(PCI_VENDOR_ID_INTEL,
> -			    PCI_DEVICE_ID_INTEL_82801AB_8, NULL) ||
> -	    pci_find_device(PCI_VENDOR_ID_INTEL,
> -			    PCI_DEVICE_ID_INTEL_82801BA_11, NULL) ||
> -	    pci_find_device(PCI_VENDOR_ID_INTEL,
> -			    PCI_DEVICE_ID_INTEL_82801BA_6, NULL) ||
> -	    pci_find_device(PCI_VENDOR_ID_AMD,
> -			    PCI_DEVICE_ID_AMD_FE_GATE_700C, NULL))
> +	if ((pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> +			    PCI_DEVICE_ID_INTEL_82801AA_8, NULL)) ||
> +	    (pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> +			    PCI_DEVICE_ID_INTEL_82801AB_8, NULL)) ||
> +	    (pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> +			    PCI_DEVICE_ID_INTEL_82801BA_11, NULL)) ||
> +	    (pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> +			    PCI_DEVICE_ID_INTEL_82801BA_6, NULL)) ||
> +	    (pdev = pci_get_device(PCI_VENDOR_ID_AMD,
> +			    PCI_DEVICE_ID_AMD_FE_GATE_700C, NULL))) {
>  		tp->tg3_flags |= TG3_FLAG_MBOX_WRITE_REORDER;
> +		pci_dev_put(pdev);
> +	}

If you have more than one device that matches these different tests, you
will have grabbed a reference and lost it, without freeing it.

What we need is a simple "Is this pci device present right now" type
function, to solve the mess that logic like this needs.

thanks,

greg k-h

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors

Re: [Kernel-janitors] Re: [PATCH -mm 7/9] pci_find_device to

[Matthew Wilcox]

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

On Tue, Sep 21, 2004 at 03:52:43PM -0700, Greg KH wrote:
> > @@ -7338,17 +7339,19 @@ static int __devinit tg3_get_invariants(
> >  	 * every mailbox register write to force the writes to be
> >  	 * posted to the chip in order.
> >  	 */
> > -	if (pci_find_device(PCI_VENDOR_ID_INTEL,
> > -			    PCI_DEVICE_ID_INTEL_82801AA_8, NULL) ||
> > -	    pci_find_device(PCI_VENDOR_ID_INTEL,
> > -			    PCI_DEVICE_ID_INTEL_82801AB_8, NULL) ||
> > -	    pci_find_device(PCI_VENDOR_ID_INTEL,
> > -			    PCI_DEVICE_ID_INTEL_82801BA_11, NULL) ||
> > -	    pci_find_device(PCI_VENDOR_ID_INTEL,
> > -			    PCI_DEVICE_ID_INTEL_82801BA_6, NULL) ||
> > -	    pci_find_device(PCI_VENDOR_ID_AMD,
> > -			    PCI_DEVICE_ID_AMD_FE_GATE_700C, NULL))
> > +	if ((pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> > +			    PCI_DEVICE_ID_INTEL_82801AA_8, NULL)) ||
> > +	    (pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> > +			    PCI_DEVICE_ID_INTEL_82801AB_8, NULL)) ||
> > +	    (pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> > +			    PCI_DEVICE_ID_INTEL_82801BA_11, NULL)) ||
> > +	    (pdev = pci_get_device(PCI_VENDOR_ID_INTEL,
> > +			    PCI_DEVICE_ID_INTEL_82801BA_6, NULL)) ||
> > +	    (pdev = pci_get_device(PCI_VENDOR_ID_AMD,
> > +			    PCI_DEVICE_ID_AMD_FE_GATE_700C, NULL))) {
> >  		tp->tg3_flags |= TG3_FLAG_MBOX_WRITE_REORDER;
> > +		pci_dev_put(pdev);
> > +	}
> 
> If you have more than one device that matches these different tests, you
> will have grabbed a reference and lost it, without freeing it.

The || short-circuits.  As soon as one of these returns a non-null pdev,
the others won't be called.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
Kernel-janitors mailing list
Kernel-janitors@lists.osdl.org
http://lists.osdl.org/mailman/listinfo/kernel-janitors