EDD build error

EDD build error

[Randy Dunlap]

Hi,

CONFIG_EDD does not depend on PCI, but edd.c uses pci_* interface
functions, resulting in:

  CC [M]  drivers/firmware/edd.o
drivers/firmware/edd.c: In function 'edd_get_pci_dev':
drivers/firmware/edd.c:672: warning: implicit declaration of function 'pci_get_bus_and_slot'
drivers/firmware/edd.c:673: warning: return makes pointer from integer without a cast
...
ERROR: "pci_get_bus_and_slot" [drivers/firmware/edd.ko] undefined!
make[1]: *** [__modpost] Error 1


Should EDD depend on PCI or should some parts of edd.c be stubbed?


(This is in 2.6.22-rc4-mm2 FWIW.)

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: EDD build error

[Torsten Duwe]

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

On Tuesday 26 June 2007, Randy Dunlap wrote:

> ERROR: "pci_get_bus_and_slot" [drivers/firmware/edd.ko] undefined!
> make[1]: *** [__modpost] Error 1
>
> Should EDD depend on PCI or should some parts of edd.c be stubbed?

I'd suggest to simply stub it, as attached (untested, as my crap-Awarded BIOS
only has 2.1). It may not be nice to pull in the PCI defs and then return NULL
regardless, but should be safe in your case. Does that work?

	Torsten


[-- Attachment #2: EDD-diff --]
[-- Type: text/x-diff, Size: 603 bytes --]

--- drivers/firmware/edd.c.orig	2007-04-26 05:08:32.000000000 +0200
+++ drivers/firmware/edd.c	2007-06-26 23:56:32.000000000 +0200
@@ -669,10 +669,14 @@
 	struct edd_info *info = edd_dev_get_info(edev);
 
 	if (edd_dev_is_type(edev, "PCI")) {
+#ifdef CONFIG_PCI
 		return pci_find_slot(info->params.interface_path.pci.bus,
 				     PCI_DEVFN(info->params.interface_path.pci.slot,
 					       info->params.interface_path.pci.
 					       function));
+#else
+		printk(KERN_WARNING "EDD referenced a PCI device for drive 0x%02x, but PCI is not configured\n", edev->index);
+#endif
 	}
 	return NULL;
 }

Re: EDD build error

[Randy Dunlap]

On Wed, 27 Jun 2007 00:16:44 +0200 Torsten Duwe wrote:

> On Tuesday 26 June 2007, Randy Dunlap wrote:
> 
> > ERROR: "pci_get_bus_and_slot" [drivers/firmware/edd.ko] undefined!
> > make[1]: *** [__modpost] Error 1
> >
> > Should EDD depend on PCI or should some parts of edd.c be stubbed?
> 
> I'd suggest to simply stub it, as attached (untested, as my crap-Awarded BIOS
> only has 2.1). It may not be nice to pull in the PCI defs and then return NULL
> regardless, but should be safe in your case. Does that work?


>From original email:
(This is in 2.6.22-rc4-mm2 FWIW.)

so quilt+patch says:

Applying patch EDD.diff
patching file drivers/firmware/edd.c
Hunk #1 FAILED at 669.
1 out of 1 hunk FAILED -- rejects in file drivers/firmware/edd.c
Patch EDD.diff does not apply (enforce with -f)


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: EDD build error

[Torsten Duwe]

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

On Wednesday 27 June 2007, Randy Dunlap wrote:
>
> From original email:
> (This is in 2.6.22-rc4-mm2 FWIW.)

Sorry. This one should be better.
I've moved the edd_info, to avoid the -Wunused.

	Torsten

[-- Attachment #2: EDD-diff --]
[-- Type: text/x-diff, Size: 835 bytes --]

--- linux-2.6.22-rc4-mm2/drivers/firmware/edd.c.orig	2007-06-27 00:36:45.000000000 +0200
+++ linux-2.6.22-rc4-mm2/drivers/firmware/edd.c	2007-06-27 00:38:52.000000000 +0200
@@ -666,13 +666,17 @@ edd_dev_is_type(struct edd_device *edev,
 static struct pci_dev *
 edd_get_pci_dev(struct edd_device *edev)
 {
-	struct edd_info *info = edd_dev_get_info(edev);
 
 	if (edd_dev_is_type(edev, "PCI")) {
+#ifdef CONFIG_PCI
+		struct edd_info *info = edd_dev_get_info(edev);
 		return pci_get_bus_and_slot(info->params.interface_path.pci.bus,
 				     PCI_DEVFN(info->params.interface_path.pci.slot,
 					       info->params.interface_path.pci.
 					       function));
+#else
+		printk(KERN_WARNING "EDD referenced a PCI device for drive 0x%02x, but PCI is not configured\n", edev->index);
+#endif
 	}
 	return NULL;
 }

Re: EDD build error

[Randy Dunlap]

On Wed, 27 Jun 2007 00:44:02 +0200 Torsten Duwe wrote:

> On Wednesday 27 June 2007, Randy Dunlap wrote:
> >
> > From original email:
> > (This is in 2.6.22-rc4-mm2 FWIW.)
> 
> Sorry. This one should be better.
> I've moved the edd_info, to avoid the -Wunused.

Yes, that one works fine.  Thanks.

---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: pci.h stubs (was: EDD build error)

[Matthew Wilcox]

On Tue, Jun 26, 2007 at 09:45:05PM -0500, Matt Domsch wrote:
> On Tue, Jun 26, 2007 at 01:26:22PM -0700, Randy Dunlap wrote:
> > ERROR: "pci_get_bus_and_slot" [drivers/firmware/edd.ko] undefined!
> 
> It should be stubbed, in include/linux/pci.h where all the other PCI
> functions are stubbed for the !CONFIG_PCI case.

pci_get_bus_and_slot() shouldn't be used because it fails to work on
systems with multiple domains.  pci_get_slot() avoids this problem (and
is much faster too).

Re: pci.h stubs (was: EDD build error)

[Alan Cox]

On Wed, 27 Jun 2007 05:30:48 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Tue, Jun 26, 2007 at 09:45:05PM -0500, Matt Domsch wrote:
> > On Tue, Jun 26, 2007 at 01:26:22PM -0700, Randy Dunlap wrote:
> > > ERROR: "pci_get_bus_and_slot" [drivers/firmware/edd.ko] undefined!
> > 
> > It should be stubbed, in include/linux/pci.h where all the other PCI
> > functions are stubbed for the !CONFIG_PCI case.
> 
> pci_get_bus_and_slot() shouldn't be used because it fails to work on
> systems with multiple domains.  pci_get_slot() avoids this problem (and
> is much faster too).

But requires you have a bus pointer to hand which is often not the
case. Do we need pci_get_domain_bus_and_slot() ?

Alan

Re: pci.h stubs (was: EDD build error)

[Matthew Wilcox]

On Wed, Jun 27, 2007 at 02:55:00PM +0100, Alan Cox wrote:
> On Wed, 27 Jun 2007 05:30:48 -0600
> Matthew Wilcox <matthew@wil.cx> wrote:
> > pci_get_bus_and_slot() shouldn't be used because it fails to work on
> > systems with multiple domains.  pci_get_slot() avoids this problem (and
> > is much faster too).
> 
> But requires you have a bus pointer to hand which is often not the
> case. Do we need pci_get_domain_bus_and_slot() ?

We have pci_find_bus(domain, busnr) which will get you the bus pointer.

Re: pci.h stubs (was: EDD build error)

[Matthew Wilcox]

On Wed, Jun 27, 2007 at 03:32:58PM +0100, Alan Cox wrote:
> On Wed, 27 Jun 2007 08:03:05 -0600
> Matthew Wilcox <matthew@wil.cx> wrote:
> 
> > On Wed, Jun 27, 2007 at 02:55:00PM +0100, Alan Cox wrote:
> > > On Wed, 27 Jun 2007 05:30:48 -0600
> > > Matthew Wilcox <matthew@wil.cx> wrote:
> > > > pci_get_bus_and_slot() shouldn't be used because it fails to work on
> > > > systems with multiple domains.  pci_get_slot() avoids this problem (and
> > > > is much faster too).
> > > 
> > > But requires you have a bus pointer to hand which is often not the
> > > case. Do we need pci_get_domain_bus_and_slot() ?
> > 
> > We have pci_find_bus(domain, busnr) which will get you the bus pointer.
> 
> So the answer is "yes" we should then, and move from pci_get_bus_and_slot
> to pci_get_bus_domain_and_slot() [or just add an argument]. The point of
> the functions is to be easy to use, so it should be handled internally.

My point was that code which doesn't currently keep the bus pointer to
hand should keep the bus pointer to hand instead of the bus number.
That's easier to use, particularly if you want to actually do any pci
config accesses.

Re: pci.h stubs (was: EDD build error)

[Alan Cox]

On Wed, 27 Jun 2007 08:03:05 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Jun 27, 2007 at 02:55:00PM +0100, Alan Cox wrote:
> > On Wed, 27 Jun 2007 05:30:48 -0600
> > Matthew Wilcox <matthew@wil.cx> wrote:
> > > pci_get_bus_and_slot() shouldn't be used because it fails to work on
> > > systems with multiple domains.  pci_get_slot() avoids this problem (and
> > > is much faster too).
> > 
> > But requires you have a bus pointer to hand which is often not the
> > case. Do we need pci_get_domain_bus_and_slot() ?
> 
> We have pci_find_bus(domain, busnr) which will get you the bus pointer.

So the answer is "yes" we should then, and move from pci_get_bus_and_slot
to pci_get_bus_domain_and_slot() [or just add an argument]. The point of
the functions is to be easy to use, so it should be handled internally.

Alan

Re: pci.h stubs (was: EDD build error)

[Alan Cox]

> > So the answer is "yes" we should then, and move from pci_get_bus_and_slot
> > to pci_get_bus_domain_and_slot() [or just add an argument]. The point of
> > the functions is to be easy to use, so it should be handled internally.
> 
> My point was that code which doesn't currently keep the bus pointer to
> hand should keep the bus pointer to hand instead of the bus number.
> That's easier to use, particularly if you want to actually do any pci
> config accesses.

If you've got the bus pointer yes. If you don't then teaching
pci_get_bus_and_slot() to handle it makes sense.

Another problem here - which bus has the legacy decode mappings ? That
will matter for ATA which also uses pci_get_bus_and_slot...

Re: pci.h stubs

[H. Peter Anvin]

Alan Cox wrote:
> On Wed, 27 Jun 2007 05:30:48 -0600
> Matthew Wilcox <matthew@wil.cx> wrote:
> 
>> On Tue, Jun 26, 2007 at 09:45:05PM -0500, Matt Domsch wrote:
>>> On Tue, Jun 26, 2007 at 01:26:22PM -0700, Randy Dunlap wrote:
>>>> ERROR: "pci_get_bus_and_slot" [drivers/firmware/edd.ko] undefined!
>>> It should be stubbed, in include/linux/pci.h where all the other PCI
>>> functions are stubbed for the !CONFIG_PCI case.
>> pci_get_bus_and_slot() shouldn't be used because it fails to work on
>> systems with multiple domains.  pci_get_slot() avoids this problem (and
>> is much faster too).
> 
> But requires you have a bus pointer to hand which is often not the
> case. Do we need pci_get_domain_bus_and_slot() ?
> 

Note that EDD has no way of referencing anything but the zero domain 
(which is presumably the one which is addressed by I/O ports CF8/CFC on 
the BSP.)  So in this particular case I would say pci_get_bus_and_slot() 
is fine.

	-hpa

Re: pci.h stubs

[Matthew Wilcox]

On Wed, Jun 27, 2007 at 12:41:41PM -0400, H. Peter Anvin wrote:
> Note that EDD has no way of referencing anything but the zero domain 
> (which is presumably the one which is addressed by I/O ports CF8/CFC on 
> the BSP.)  So in this particular case I would say pci_get_bus_and_slot() 
> is fine.

Except that you're not guaranteed to get the bus that's in domain zero.

Re: pci.h stubs

[Alan Cox]

On Wed, 27 Jun 2007 10:54:30 -0600
Matthew Wilcox <matthew@wil.cx> wrote:

> On Wed, Jun 27, 2007 at 12:41:41PM -0400, H. Peter Anvin wrote:
> > Note that EDD has no way of referencing anything but the zero domain 
> > (which is presumably the one which is addressed by I/O ports CF8/CFC on 
> > the BSP.)  So in this particular case I would say pci_get_bus_and_slot() 
> > is fine.
> 
> Except that you're not guaranteed to get the bus that's in domain zero.

Which is trivial to fix

Re: pci.h stubs

[H. Peter Anvin]

Alan Cox wrote:
> On Wed, 27 Jun 2007 10:54:30 -0600
> Matthew Wilcox <matthew@wil.cx> wrote:
> 
>> On Wed, Jun 27, 2007 at 12:41:41PM -0400, H. Peter Anvin wrote:
>>> Note that EDD has no way of referencing anything but the zero domain 
>>> (which is presumably the one which is addressed by I/O ports CF8/CFC on 
>>> the BSP.)  So in this particular case I would say pci_get_bus_and_slot() 
>>> is fine.
>> Except that you're not guaranteed to get the bus that's in domain zero.
> 
> Which is trivial to fix

Yes, and that's probably the only sane definition of that API.

	-hpa

[PATCH] PCI: limit pci_get_bus_and_slot to domain 0

[Randy Dunlap]

On Wed, 27 Jun 2007 14:34:03 -0400 H. Peter Anvin wrote:

> Alan Cox wrote:
> > On Wed, 27 Jun 2007 10:54:30 -0600
> > Matthew Wilcox <matthew@wil.cx> wrote:
> > 
> >> On Wed, Jun 27, 2007 at 12:41:41PM -0400, H. Peter Anvin wrote:
> >>> Note that EDD has no way of referencing anything but the zero domain 
> >>> (which is presumably the one which is addressed by I/O ports CF8/CFC on 
> >>> the BSP.)  So in this particular case I would say pci_get_bus_and_slot() 
> >>> is fine.
> >> Except that you're not guaranteed to get the bus that's in domain zero.
> > 
> > Which is trivial to fix
> 
> Yes, and that's probably the only sane definition of that API.

Is this what you mean?
---

From: Randy Dunlap <randy.dunlap@oracle.com>

Limit pci_get_bus_and_slot() to domain (segment) 0 since domain is not
specified in the function call and defaulting to domain 0 is the only
reasonable thing to do (rather than returning a device from some other
unknown domain).

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 drivers/pci/search.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

--- linux-2622-rc6mm1.orig/drivers/pci/search.c
+++ linux-2622-rc6mm1/drivers/pci/search.c
@@ -173,12 +173,14 @@ struct pci_dev * pci_get_slot(struct pci
 }
 
 /**
- * pci_get_bus_and_slot - locate PCI device from a given PCI slot
+ * pci_get_bus_and_slot - locate PCI device from a given PCI bus & slot
  * @bus: number of PCI bus on which desired PCI device resides
  * @devfn: encodes number of PCI slot in which the desired PCI
  * device resides and the logical device number within that slot
  * in case of multi-function devices.
  *
+ * Note: the bus/slot search is limited to PCI domain (segment) 0.
+ *
  * Given a PCI bus and slot/function number, the desired PCI device
  * is located in system global list of PCI devices.  If the device
  * is found, a pointer to its data structure is returned.  If no
@@ -191,7 +193,8 @@ struct pci_dev * pci_get_bus_and_slot(un
 	struct pci_dev *dev = NULL;
 
 	while ((dev = pci_get_device(PCI_ANY_ID, PCI_ANY_ID, dev)) != NULL) {
-		if (dev->bus->number == bus && dev->devfn == devfn)
+		if (pci_domain_nr(dev->bus) == 0 &&
+		   (dev->bus->number == bus && dev->devfn == devfn))
 			return dev;
 	}
 	return NULL;

[PATCH] pci.h stubs (for EDD build error)

[Randy Dunlap]

On Tue, 26 Jun 2007 21:45:05 -0500 Matt Domsch wrote:

> On Tue, Jun 26, 2007 at 01:26:22PM -0700, Randy Dunlap wrote:
> > Hi,
> > 
> > CONFIG_EDD does not depend on PCI, but edd.c uses pci_* interface
> > functions, resulting in:
> > 
> >   CC [M]  drivers/firmware/edd.o
> > drivers/firmware/edd.c: In function 'edd_get_pci_dev':
> > drivers/firmware/edd.c:672: warning: implicit declaration of function 'pci_get_bus_and_slot'
> > drivers/firmware/edd.c:673: warning: return makes pointer from integer without a cast
> > ...
> > ERROR: "pci_get_bus_and_slot" [drivers/firmware/edd.ko] undefined!
> > make[1]: *** [__modpost] Error 1
> > 
> > 
> > Should EDD depend on PCI or should some parts of edd.c be stubbed?
> 
> Good catch, thanks.
> 
> It should be stubbed, in include/linux/pci.h where all the other PCI
> functions are stubbed for the !CONFIG_PCI case.
> 
> There are several functions in include/linux/pci.h that have been
> added in the CONFIG_PCI case, but not stubbed in !CONFIG_PCI.
> pci_get_bus_and_slot() is just one of those.  That list appears to
> include:
> 
> struct pci_bus *pci_find_next_bus(const struct pci_bus *from);
> struct pci_dev *pci_get_slot (struct pci_bus *bus, unsigned int devfn);
> struct pci_dev *pci_get_bus_and_slot (unsigned int bus, unsigned int devfn);

Did those -- patch is below.

Get clean build of drivers/firmware/edd.c with this patch
even when CONFIG_PCI=n.


> Perhaps arguably, functions that take a struct pci_dev * as an
> argument need not be stubbed, as they shouldn't be called with a null
> pointer argument in the first place.  These include:
> 
> int pci_find_ht_capability (struct pci_dev *dev, int ht_cap);
> int pci_find_next_ht_capability (struct pci_dev *dev, int pos, int ht_cap);
> static inline int pci_is_managed(struct pci_dev *pdev)
> 
> 
> I'm out of the office this week and next, but will add to my (growing)
> list of cleanups to do when I'm back and more online, unless you or
> someone wants to get to it first. :-)



From: Randy Dunlap <randy.dunlap@oracle.com>

Provide stubs for more PCI bus/slot functions when CONFIG_PCI=n.

Signed-off-by: Randy Dunlap <randy.dunlap@oracle.com>
---
 include/linux/pci.h |   11 +++++++++++
 1 file changed, 11 insertions(+)

--- linux-2622-rc6.orig/include/linux/pci.h
+++ linux-2622-rc6/include/linux/pci.h
@@ -748,6 +748,17 @@ static inline void pci_release_regions(s
 static inline void pci_block_user_cfg_access(struct pci_dev *dev) { }
 static inline void pci_unblock_user_cfg_access(struct pci_dev *dev) { }
 
+static inline struct pci_bus *pci_find_next_bus(const struct pci_bus *from)
+{ return NULL; }
+
+static inline struct pci_dev *pci_get_slot(struct pci_bus *bus,
+						unsigned int devfn)
+{ return NULL; }
+
+static inline struct pci_dev *pci_get_bus_and_slot(unsigned int bus,
+						unsigned int devfn)
+{ return NULL; }
+
 #endif /* CONFIG_PCI */
 
 /* Include architecture-dependent settings and functions */

Re: [PATCH] PCI: limit pci_get_bus_and_slot to domain 0

[H. Peter Anvin]

Randy Dunlap wrote:
> 
> Is this what you mean?

Yup.

Acked-by: H. Peter Anvin <hpa@zytor.com>