Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c
From: Greg KH (greg_at_kroah.com)
Date: 09/30/04
- Previous message: Martin Hermanowski: "Re: Hard lockup on IBM ThinkPad T42"
- In reply to: Andi Kleen: "Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Date: Thu, 30 Sep 2004 14:05:16 -0700 To: Andi Kleen <ak@suse.de>
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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Previous message: Martin Hermanowski: "Re: Hard lockup on IBM ThinkPad T42"
- In reply to: Andi Kleen: "Re: [PATCH] fix improper use of pci_module_init() in drivers/char/agp/amd64-agp.c"
- Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]
Relevant Pages
|
|
