[PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[Greg KH]

Hi,

In going through the tree and auditing the usage of pci_module_init(), I
noticed that the amd64-agp driver was assuming that the return value of
this function could be greater than 0 (which is what could happen in 2.2
and 2.4 kernels.)  As this is no longer true, I think the following
patch is correct.

I can add this to my bk-pci tree if you wish, otherwise feel free to
send it upwards.

thanks,

greg k-h

-----

AGP: Fix up pci_module_init() usage in amd64-agp.c

pci_module_init() can not return a positive number in the 2.6 kernel, so
this code path was never getting run, so might as well just delete it.


Signed-off-by: Greg Kroah-Hartman <greg@kroah.com>

--- a/drivers/char/agp/amd64-agp.c	2004-08-24 07:55:13 -07:00
+++ b/drivers/char/agp/amd64-agp.c	2004-09-30 11:37:35 -07:00
@@ -624,39 +624,9 @@
 /* Not static due to IOMMU code calling it early. */
 int __init agp_amd64_init(void)
 {
-	int err = 0;
 	if (agp_off)
 		return -EINVAL;
-	if (pci_module_init(&agp_amd64_pci_driver) > 0) {
-		struct pci_dev *dev;
-		if (!agp_try_unsupported && !agp_try_unsupported_boot) {
-			printk(KERN_INFO PFX "No supported AGP bridge found.\n");
-#ifdef MODULE
-			printk(KERN_INFO PFX "You can try agp_try_unsupported=1\n");
-#else
-			printk(KERN_INFO PFX "You can boot with agp=try_unsupported\n");
-#endif
-			return -ENODEV;
-		}
-
-		/* First check that we have at least one AMD64 NB */
-		if (!pci_find_device(PCI_VENDOR_ID_AMD, 0x1103, NULL))
-			return -ENODEV;
-
-		/* Look for any AGP bridge */
-		dev = NULL;
-		err = -ENODEV;
-		while ((dev = pci_find_device(PCI_ANY_ID, PCI_ANY_ID, dev))) {
-			if (!pci_find_capability(dev, PCI_CAP_ID_AGP))
-				continue;
-			/* Only one bridge supported right now */
-			if (agp_amd64_probe(dev, NULL) == 0) {
-				err = 0;
-				break;
-			}
-		}
-	}
-	return err;
+	return pci_register_driver(&agp_amd64_pci_driver);
 }
 
 static void __exit agp_amd64_cleanup(void)

Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[Andi Kleen]

On Thu, Sep 30, 2004 at 11:42:48AM -0700, Greg KH wrote:
> Hi,
> 
> In going through the tree and auditing the usage of pci_module_init(), I
> noticed that the amd64-agp driver was assuming that the return value of
> this function could be greater than 0 (which is what could happen in 2.2
> and 2.4 kernels.)  As this is no longer true, I think the following
> patch is correct.
> 
> I can add this to my bk-pci tree if you wish, otherwise feel free to
> send it upwards.

There needs to be some replacement for it, you cannot just delete 
the code.


The idea is to run it as fallback when no devices are found. 

How about this patch?

-Andi



diff -u linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c-o linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c
--- linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c-o	2004-09-30 10:35:07.000000000 +0200
+++ linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c	2004-09-30 21:19:00.000000000 +0200
@@ -627,7 +627,7 @@
 	int err = 0;
 	if (agp_off)
 		return -EINVAL;
-	if (pci_module_init(&agp_amd64_pci_driver) > 0) {
+	if (pci_module_init(&agp_amd64_pci_driver) < 0) {
 		struct pci_dev *dev;
 		if (!agp_try_unsupported && !agp_try_unsupported_boot) {
 			printk(KERN_INFO PFX "No supported AGP bridge found.\n");

Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[Greg KH]

On Thu, Sep 30, 2004 at 09:20:09PM +0200, Andi Kleen wrote:
> On Thu, Sep 30, 2004 at 11:42:48AM -0700, Greg KH wrote:
> > Hi,
> > 
> > In going through the tree and auditing the usage of pci_module_init(), I
> > noticed that the amd64-agp driver was assuming that the return value of
> > this function could be greater than 0 (which is what could happen in 2.2
> > and 2.4 kernels.)  As this is no longer true, I think the following
> > patch is correct.
> > 
> > I can add this to my bk-pci tree if you wish, otherwise feel free to
> > send it upwards.
> 
> There needs to be some replacement for it, you cannot just delete 
> the code.

But that code has not ever run, since early 2.5 days.  Don't tell me
people are used to it :)

> The idea is to run it as fallback when no devices are found. 
> 
> How about this patch?

That does not work the way you are asking it to work.  pci_module_init()
is just a replacement for pci_register_driver these days.  It will
return either "0" if the driver is successfully registered, or a
negative value if something bad happened.  It will not return the number
of devices that this driver bound to.

So, if no devices are in the system, it will return 0, and again, the
code you are wanting to run, will not.

So, how about using the new pci_dev_present() call instead?  That should
be what you want, right?

thanks,

greg k-h

Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[Andi Kleen]

On Thu, Sep 30, 2004 at 12:59:05PM -0700, Greg KH wrote:
> On Thu, Sep 30, 2004 at 09:20:09PM +0200, Andi Kleen wrote:
> > On Thu, Sep 30, 2004 at 11:42:48AM -0700, Greg KH wrote:
> > > Hi,
> > > 
> > > In going through the tree and auditing the usage of pci_module_init(), I
> > > noticed that the amd64-agp driver was assuming that the return value of
> > > this function could be greater than 0 (which is what could happen in 2.2
> > > and 2.4 kernels.)  As this is no longer true, I think the following
> > > patch is correct.
> > > 
> > > I can add this to my bk-pci tree if you wish, otherwise feel free to
> > > send it upwards.
> > 
> > There needs to be some replacement for it, you cannot just delete 
> > the code.
> 
> But that code has not ever run, since early 2.5 days.  Don't tell me
> people are used to it :)

No, but it's needed for new chipsets that are not in the PCI tables.
People probably didn't complain because we're covering the current
generation of K8 AGP bridges, but there should be new ones soon

2.4 had similar code.

> 
> > The idea is to run it as fallback when no devices are found. 
> > 
> > How about this patch?
> 
> That does not work the way you are asking it to work.  pci_module_init()
> is just a replacement for pci_register_driver these days.  It will
> return either "0" if the driver is successfully registered, or a
> negative value if something bad happened.  It will not return the number
> of devices that this driver bound to.
> 
> So, if no devices are in the system, it will return 0, and again, the
> code you are wanting to run, will not.

Oh, yes I forgot that hotplug makes everything simple complicated.

> 
> So, how about using the new pci_dev_present() call instead?  That should
> be what you want, right?


% grep pci_dev_present include/linux/*
% 

This patch will probably do and doesn't rely on any nonexisting
calls.

-Andi

----------------------------------------------------------------------

Fix fallback code in K8 AGP driver.

Problem pointed out by Greg KH

Signed-off-by: Andi Kleen <ak@suse.de>


diff -u linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c-o linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c
--- linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c-o	2004-09-30 10:35:07.000000000 +0200
+++ linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c	2004-09-30 22:04:56.000000000 +0200
@@ -55,6 +55,8 @@
 static int gart_iterator;
 #define for_each_nb() for(gart_iterator=0;gart_iterator<nr_garts;gart_iterator++)
 
+static int num_bridges;
+
 static void flush_amd64_tlb(struct pci_dev *dev)
 {
 	u32 tmp;
@@ -514,6 +516,7 @@
 	}
 
 	pci_set_drvdata(pdev, bridge);
+	num_bridges++;
 	return agp_add_bridge(bridge);
 }
 
@@ -627,7 +630,8 @@
 	int err = 0;
 	if (agp_off)
 		return -EINVAL;
-	if (pci_module_init(&agp_amd64_pci_driver) > 0) {
+	pci_module_init(&agp_amd64_pci_driver);
+	if (num_bridges == 0) { 
 		struct pci_dev *dev;
 		if (!agp_try_unsupported && !agp_try_unsupported_boot) {
 			printk(KERN_INFO PFX "No supported AGP bridge found.\n");

Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[Greg KH]

On Thu, Sep 30, 2004 at 10:06:30PM +0200, Andi Kleen wrote:
> On Thu, Sep 30, 2004 at 12:59:05PM -0700, Greg KH wrote:
> > On Thu, Sep 30, 2004 at 09:20:09PM +0200, Andi Kleen wrote:
> > > On Thu, Sep 30, 2004 at 11:42:48AM -0700, Greg KH wrote:
> > > > Hi,
> > > > 
> > > > In going through the tree and auditing the usage of pci_module_init(), I
> > > > noticed that the amd64-agp driver was assuming that the return value of
> > > > this function could be greater than 0 (which is what could happen in 2.2
> > > > and 2.4 kernels.)  As this is no longer true, I think the following
> > > > patch is correct.
> > > > 
> > > > I can add this to my bk-pci tree if you wish, otherwise feel free to
> > > > send it upwards.
> > > 
> > > There needs to be some replacement for it, you cannot just delete 
> > > the code.
> > 
> > But that code has not ever run, since early 2.5 days.  Don't tell me
> > people are used to it :)
> 
> No, but it's needed for new chipsets that are not in the PCI tables.
> People probably didn't complain because we're covering the current
> generation of K8 AGP bridges, but there should be new ones soon
> 
> 2.4 had similar code.

Ok, that makes sense.

> > > The idea is to run it as fallback when no devices are found. 
> > > 
> > > How about this patch?
> > 
> > That does not work the way you are asking it to work.  pci_module_init()
> > is just a replacement for pci_register_driver these days.  It will
> > return either "0" if the driver is successfully registered, or a
> > negative value if something bad happened.  It will not return the number
> > of devices that this driver bound to.
> > 
> > So, if no devices are in the system, it will return 0, and again, the
> > code you are wanting to run, will not.
> 
> Oh, yes I forgot that hotplug makes everything simple complicated.

Welcome to my life, hotplug is hard, let's go shopping... :)

> > So, how about using the new pci_dev_present() call instead?  That should
> > be what you want, right?
> 
> 
> % grep pci_dev_present include/linux/*
> % 
> 
> This patch will probably do and doesn't rely on any nonexisting
> calls.

Sorry, that's a function that was added to my bk trees, and is in the
next -mm release.

> ----------------------------------------------------------------------
> 
> Fix fallback code in K8 AGP driver.
> 
> Problem pointed out by Greg KH
> 
> Signed-off-by: Andi Kleen <ak@suse.de>
> 
> 
> diff -u linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c-o linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c
> --- linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c-o	2004-09-30 10:35:07.000000000 +0200
> +++ linux-2.6.9rc3-work/drivers/char/agp/amd64-agp.c	2004-09-30 22:04:56.000000000 +0200
> @@ -55,6 +55,8 @@
>  static int gart_iterator;
>  #define for_each_nb() for(gart_iterator=0;gart_iterator<nr_garts;gart_iterator++)
>  
> +static int num_bridges;
> +
>  static void flush_amd64_tlb(struct pci_dev *dev)
>  {
>  	u32 tmp;
> @@ -514,6 +516,7 @@
>  	}
>  
>  	pci_set_drvdata(pdev, bridge);
> +	num_bridges++;
>  	return agp_add_bridge(bridge);
>  }
>  
> @@ -627,7 +630,8 @@
>  	int err = 0;
>  	if (agp_off)
>  		return -EINVAL;
> -	if (pci_module_init(&agp_amd64_pci_driver) > 0) {
> +	pci_module_init(&agp_amd64_pci_driver);
> +	if (num_bridges == 0) { 

Hm, no.  When I add the "do all device probes in a separate thread"
patch to the tree, this will break, as pci_module_init() will return
right away, and pci device probing will happen in a different thread.
I'm currently still working on this, but don't want to go and add stuff
to the tree today, that I know will not work in the near future.

I still like my patch better, want me to wait until pci_dev_present() is
in the mainline tree before bringing this up again?

thanks,

greg k-h

Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[Greg KH]

On Thu, Sep 30, 2004 at 12:59:05PM -0700, Greg KH wrote:
> > The idea is to run it as fallback when no devices are found. 
> > 
> > How about this patch?
> 
> That does not work the way you are asking it to work.  pci_module_init()
> is just a replacement for pci_register_driver these days.  It will
> return either "0" if the driver is successfully registered, or a
> negative value if something bad happened.  It will not return the number
> of devices that this driver bound to.
> 
> So, if no devices are in the system, it will return 0, and again, the
> code you are wanting to run, will not.
> 
> So, how about using the new pci_dev_present() call instead?  That should
> be what you want, right?

Something like this perhaps?  (warning, not even compile tested...)

greg k-h

===== amd64-agp.c 1.33 vs edited =====
--- 1.33/drivers/char/agp/amd64-agp.c	2004-09-30 13:00:52 -07:00
+++ edited/amd64-agp.c	2004-09-30 13:07:07 -07:00
@@ -627,8 +627,15 @@
 	int err = 0;
 	if (agp_off)
 		return -EINVAL;
-	if (pci_module_init(&agp_amd64_pci_driver) > 0) {
+	if (pci_dev_present(agp_amd_pci_table))
+		return pci_register_driver(&agp_amd64_pci_driver);
+	else {
 		struct pci_dev *dev;
+		static struct pci_device_id amd64NB[] = {
+			{ PCI_DEVICE(PCI_VENDOR_ID_AMD, 0x1103) },
+			{ },
+		};
+
 		if (!agp_try_unsupported && !agp_try_unsupported_boot) {
 			printk(KERN_INFO PFX "No supported AGP bridge found.\n");
 #ifdef MODULE
@@ -640,13 +647,13 @@
 		}
 
 		/* First check that we have at least one AMD64 NB */
-		if (!pci_find_device(PCI_VENDOR_ID_AMD, 0x1103, NULL))
+		if (!pci_dev_present(amd64NB))
 			return -ENODEV;
 
 		/* Look for any AGP bridge */
 		dev = NULL;
 		err = -ENODEV;
-		while ((dev = pci_find_device(PCI_ANY_ID, PCI_ANY_ID, dev))) {
+		while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev))) {
 			if (!pci_find_capability(dev, PCI_CAP_ID_AGP))
 				continue;
 			/* Only one bridge supported right now */

Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c

[Dave Jones]

On Thu, Sep 30, 2004 at 11:42:48AM -0700, Greg KH wrote:
 > Hi,
 
Hi Greg,

 > In going through the tree and auditing the usage of pci_module_init(), I
 > noticed that the amd64-agp driver was assuming that the return value of
 > this function could be greater than 0 (which is what could happen in 2.2
 > and 2.4 kernels.)  As this is no longer true, I think the following
 > patch is correct.
 > 
 > I can add this to my bk-pci tree if you wish, otherwise feel free to
 > send it upwards.

I'm going through all kinds of busyness due to a relocation right now,
so feel free to push this yourself after you and Andi are done beating
out the details.

		Dave