[PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Jiri Slaby]

Generated in 2.6.13-mm2 kernel version.

Signed-off-by: Jiri Slaby <xslaby@fi.muni.cz>

Repost, posted on:
02 Sep 2005

 ide.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/include/asm-i386/ide.h b/include/asm-i386/ide.h
--- a/include/asm-i386/ide.h
+++ b/include/asm-i386/ide.h
@@ -41,7 +41,12 @@ static __inline__ int ide_default_irq(un
 
 static __inline__ unsigned long ide_default_io_base(int index)
 {
-	if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID, NULL) == NULL) {
+	struct pci_dev *pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, NULL);
+	unsigned int a = !pdev;
+
+	pci_dev_put(pdev);
+
+	if (a) {
 		switch(index) {
 			case 2: return 0x1e8;
 			case 3: return 0x168;

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Jeff Garzik]

Jiri Slaby wrote:
> diff --git a/include/asm-i386/ide.h b/include/asm-i386/ide.h
> --- a/include/asm-i386/ide.h
> +++ b/include/asm-i386/ide.h
> @@ -41,7 +41,12 @@ static __inline__ int ide_default_irq(un
>  
>  static __inline__ unsigned long ide_default_io_base(int index)
>  {
> -	if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID, NULL) == NULL) {
> +	struct pci_dev *pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, NULL);
> +	unsigned int a = !pdev;
> +
> +	pci_dev_put(pdev);


Looks like we need to resurrect pci_present() from the ancient past.

	Jeff

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Greg KH]

On Sat, Sep 10, 2005 at 04:55:10PM -0400, Jeff Garzik wrote:
> Jiri Slaby wrote:
> >diff --git a/include/asm-i386/ide.h b/include/asm-i386/ide.h
> >--- a/include/asm-i386/ide.h
> >+++ b/include/asm-i386/ide.h
> >@@ -41,7 +41,12 @@ static __inline__ int ide_default_irq(un
> > 
> > static __inline__ unsigned long ide_default_io_base(int index)
> > {
> >-	if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID, NULL) == NULL) {
> >+	struct pci_dev *pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, NULL);
> >+	unsigned int a = !pdev;
> >+
> >+	pci_dev_put(pdev);
> 
> 
> Looks like we need to resurrect pci_present() from the ancient past.

Heh, ick, no :)

Jiri, any other way to do this instead?

thanks,

greg k-h

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Jiri Slaby]

Greg KH napsal(a):
> On Sat, Sep 10, 2005 at 04:55:10PM -0400, Jeff Garzik wrote:
> 
>>Jiri Slaby wrote:
>>
>>>diff --git a/include/asm-i386/ide.h b/include/asm-i386/ide.h
>>>--- a/include/asm-i386/ide.h
>>>+++ b/include/asm-i386/ide.h
>>>@@ -41,7 +41,12 @@ static __inline__ int ide_default_irq(un
>>>
>>>static __inline__ unsigned long ide_default_io_base(int index)
>>>{
>>>-	if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID, NULL) == NULL) {
>>>+	struct pci_dev *pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, NULL);
>>>+	unsigned int a = !pdev;
>>>+
>>>+	pci_dev_put(pdev);
>>
>>
>>Looks like we need to resurrect pci_present() from the ancient past.
> 
> 
> Heh, ick, no :)
> 
> Jiri, any other way to do this instead?
I have no idea, how to do it more elegant. So hint me a little bit...

thanks,
-- 
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Jeff Garzik]

Greg KH wrote:
> On Sat, Sep 10, 2005 at 04:55:10PM -0400, Jeff Garzik wrote:
> 
>>Jiri Slaby wrote:
>>
>>>diff --git a/include/asm-i386/ide.h b/include/asm-i386/ide.h
>>>--- a/include/asm-i386/ide.h
>>>+++ b/include/asm-i386/ide.h
>>>@@ -41,7 +41,12 @@ static __inline__ int ide_default_irq(un
>>>
>>>static __inline__ unsigned long ide_default_io_base(int index)
>>>{
>>>-	if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID, NULL) == NULL) {
>>>+	struct pci_dev *pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, NULL);
>>>+	unsigned int a = !pdev;
>>>+
>>>+	pci_dev_put(pdev);
>>
>>
>>Looks like we need to resurrect pci_present() from the ancient past.
> 
> 
> Heh, ick, no :)
> 
> Jiri, any other way to do this instead?

Look at what the IDE code is trying to do.  All it cares about is 
whether -any PCI device at all- is present, a boolean value.

	Jeff

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Matthew Wilcox]

On Sat, Sep 10, 2005 at 05:39:52PM -0400, Jeff Garzik wrote:
> Look at what the IDE code is trying to do.  All it cares about is 
> whether -any PCI device at all- is present, a boolean value.

Why not change it to query whether any IDE device is present, perhaps
using pci_get_class()?

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Jeff Garzik]

Matthew Wilcox wrote:
> On Sat, Sep 10, 2005 at 05:39:52PM -0400, Jeff Garzik wrote:
> 
>>Look at what the IDE code is trying to do.  All it cares about is 
>>whether -any PCI device at all- is present, a boolean value.
> 
> 
> Why not change it to query whether any IDE device is present, perhaps
> using pci_get_class()?

Because that's not what the code is attempting to discover.

	Jeff

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Grant Grundler]

On Sat, Sep 10, 2005 at 07:35:10PM -0400, Jeff Garzik wrote:
> >Why not change it to query whether any IDE device is present, perhaps
> >using pci_get_class()?
> 
> Because that's not what the code is attempting to discover.

If ide_scan_pcibus() finds any pci device, it calls ide_scan_pcidev().
ide_scan_pcidev() only seems to handle PCI devices.
Are you saying there are PCI IDE devices out there that
don't advertise PCI_CLASS_STORAGE_IDE?

thanks,
grant

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Jeff Garzik]

Grant Grundler wrote:
> On Sat, Sep 10, 2005 at 07:35:10PM -0400, Jeff Garzik wrote:
> 
>>>Why not change it to query whether any IDE device is present, perhaps
>>>using pci_get_class()?
>>
>>Because that's not what the code is attempting to discover.
> 
> 
> If ide_scan_pcibus() finds any pci device, it calls ide_scan_pcidev().
> ide_scan_pcidev() only seems to handle PCI devices.
> Are you saying there are PCI IDE devices out there that
> don't advertise PCI_CLASS_STORAGE_IDE?

The code is not searching for PCI devices.  The code is searching for... 
precisely what it is searching for:  presence of PCI in the system.

	Jeff

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Alan Cox]

On Sad, 2005-09-10 at 18:34 -0600, Grant Grundler wrote:
> If ide_scan_pcibus() finds any pci device, it calls ide_scan_pcidev().
> ide_scan_pcidev() only seems to handle PCI devices.
> Are you saying there are PCI IDE devices out there that
> don't advertise PCI_CLASS_STORAGE_IDE?

Lots of them. We also want to know if PCI is present so we can know
whether to do the IDE tertiary scan which isn't safe on a PCI bus box.

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Grant Grundler]

On Sun, Sep 11, 2005 at 02:06:56AM +0100, Alan Cox wrote:
> On Sad, 2005-09-10 at 18:34 -0600, Grant Grundler wrote:
> > If ide_scan_pcibus() finds any pci device, it calls ide_scan_pcidev().
> > ide_scan_pcidev() only seems to handle PCI devices.
> > Are you saying there are PCI IDE devices out there that
> > don't advertise PCI_CLASS_STORAGE_IDE?
> 
> Lots of them. We also want to know if PCI is present so we can know
> whether to do the IDE tertiary scan which isn't safe on a PCI bus box.

ah ok. I'm not seeing where that happens.

Anyway, pci_present() could be as simple as "(pci_root_buses!=NULL)".

ide_system_bus_speed() in drivers/ide/ide.c might want this too.


BTW, I'm not convinced the current code does *exactly* what you want.
Hypothetically, we can have a PCI bus but no PCI devices.
Maybe no such system exists but I wouldn't bet on it.


thanks,
grant

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Matthew Wilcox]

On Sun, Sep 11, 2005 at 02:06:56AM +0100, Alan Cox wrote:
> On Sad, 2005-09-10 at 18:34 -0600, Grant Grundler wrote:
> > If ide_scan_pcibus() finds any pci device, it calls ide_scan_pcidev().
> > ide_scan_pcidev() only seems to handle PCI devices.
> > Are you saying there are PCI IDE devices out there that
> > don't advertise PCI_CLASS_STORAGE_IDE?
> 
> Lots of them. We also want to know if PCI is present so we can know
> whether to do the IDE tertiary scan which isn't safe on a PCI bus box.

surely this is worthy of a comment in the code.  there's at least 3
people on the cc who're confused bby what it's for.

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Alan Cox]

On Sad, 2005-09-10 at 19:30 -0600, Matthew Wilcox wrote:
> > Lots of them. We also want to know if PCI is present so we can know
> > whether to do the IDE tertiary scan which isn't safe on a PCI bus box.
> 
> surely this is worthy of a comment in the code.  there's at least 3
> people on the cc who're confused bby what it's for.

Thats because someone removed the obvious pci_present() function some
time ago.

Alan

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Matthew Wilcox]

On Mon, Sep 12, 2005 at 11:17:21AM +0100, Alan Cox wrote:
> On Sad, 2005-09-10 at 19:30 -0600, Matthew Wilcox wrote:
> > > Lots of them. We also want to know if PCI is present so we can know
> > > whether to do the IDE tertiary scan which isn't safe on a PCI bus box.
> > 
> > surely this is worthy of a comment in the code.  there's at least 3
> > people on the cc who're confused bby what it's for.
> 
> Thats because someone removed the obvious pci_present() function some
> time ago.

Huh?  Even if we had pci_present(), it still wouldn't be obvious that a
tertiary scan is unsafe on a PCI-based box.

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Alan Cox]

On Llu, 2005-09-12 at 05:21 -0600, Matthew Wilcox wrote:
> > > surely this is worthy of a comment in the code.  there's at least 3
> > > people on the cc who're confused bby what it's for.
> > 
> > Thats because someone removed the obvious pci_present() function some
> > time ago.
> 
> Huh?  Even if we had pci_present(), it still wouldn't be obvious that a
> tertiary scan is unsafe on a PCI-based box.

Ah sorry - I misunderstood what you meant was not obvious. I sent in the
original patch for this so I'll send Bartlomiej a comment update to go
with it.

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Alan Cox]

On Sad, 2005-09-10 at 14:19 -0700, Greg KH wrote:
> > Looks like we need to resurrect pci_present() from the ancient past.
> 
> Heh, ick, no :)
> 
> Jiri, any other way to do this instead?

IDE really does want to know if you have a PCI bus of any kind attached.
Perhaps pci_present should really come back - its better than hiding the
gory details in drivers

Re: [PATCH] include: pci_find_device remove (include/asm-i386/ide.h)

[Jiri Slaby]

On 9/10/05, Jeff Garzik <jgarzik@pobox.com> wrote:
> Jiri Slaby wrote:
> > diff --git a/include/asm-i386/ide.h b/include/asm-i386/ide.h
> > --- a/include/asm-i386/ide.h
> > +++ b/include/asm-i386/ide.h
> > @@ -41,7 +41,12 @@ static __inline__ int ide_default_irq(un
> >
> >  static __inline__ unsigned long ide_default_io_base(int index)
> >  {
> > -     if (pci_find_device(PCI_ANY_ID, PCI_ANY_ID, NULL) == NULL) {
> > +     struct pci_dev *pdev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, NULL);
> > +     unsigned int a = !pdev;
> > +
> > +     pci_dev_put(pdev);
>
>
> Looks like we need to resurrect pci_present() from the ancient past.
So, what was the result of this debate? I can't see any solution in
that thread, not even in 2.6.14-rc2-mm2.

thanks,
--
Jiri Slaby         www.fi.muni.cz/~xslaby
~\-/~      jirislaby@gmail.com      ~\-/~
B67499670407CE62ACC8 22A032CC55C339D47A7E