pci_read_base patch series

pci_read_base patch series

[Matthew Wilcox]

Hi Jesse,

Could I get you to review this patch series?  I think factoring the code
out of pci_read_bases is a worthwhile thing to do all by itself (and
honestly I've been wanting to do it for a while).  When Eric came up
with a reason to spend a little bit of time on it, I was overjoyed.
He'll be posting his patches that use the new interface later today.

[PATCH 1/3] Rewrite PCI BAR reading code

[Matthew Wilcox]

Factor out the code to read one BAR from the loop in pci_read_bases into
a new function, __pci_read_base.  The new code is slightly more
readable, better commented and removes the ifdef.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/pci/probe.c |  242 ++++++++++++++++++++++++++-------------------------
 1 files changed, 123 insertions(+), 119 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index b1724cf..3b690c3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -163,12 +163,9 @@ static inline unsigned int pci_calc_resource_flags(unsigned int flags)
 	return IORESOURCE_MEM;
 }
 
-/*
- * Find the extent of a PCI decode..
- */
-static u32 pci_size(u32 base, u32 maxbase, u32 mask)
+static u64 pci_size(u64 base, u64 maxbase, u64 mask)
 {
-	u32 size = mask & maxbase;	/* Find the significant bits */
+	u64 size = mask & maxbase;	/* Find the significant bits */
 	if (!size)
 		return 0;
 
@@ -184,135 +181,142 @@ static u32 pci_size(u32 base, u32 maxbase, u32 mask)
 	return size;
 }
 
-static u64 pci_size64(u64 base, u64 maxbase, u64 mask)
-{
-	u64 size = mask & maxbase;	/* Find the significant bits */
-	if (!size)
-		return 0;
+enum pci_bar_type {
+	pci_bar_unknown,	/* Standard PCI BAR probe */
+	pci_bar_io,		/* An io port BAR */
+	pci_bar_mem32,		/* A 32-bit memory BAR */
+	pci_bar_mem64,		/* A 64-bit memory BAR */
+};
 
-	/* Get the lowest of them to find the decode size, and
-	   from that the extent.  */
-	size = (size & ~(size-1)) - 1;
+static inline enum pci_bar_type decode_bar(struct resource *res, u32 bar)
+{
+	if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
+		res->flags = bar & ~PCI_BASE_ADDRESS_IO_MASK;
+		return pci_bar_io;
+	}
 
-	/* base == maxbase can be valid only if the BAR has
-	   already been programmed with all 1s.  */
-	if (base == maxbase && ((base | size) & mask) != mask)
-		return 0;
+	res->flags = bar & ~PCI_BASE_ADDRESS_MEM_MASK;
 
-	return size;
+	if (res->flags == PCI_BASE_ADDRESS_MEM_TYPE_64)
+		return pci_bar_mem64;
+	return pci_bar_mem32;
 }
 
-static inline int is_64bit_memory(u32 mask)
+/*
+ * If the type is not unknown, we assume that the lowest bit is 'enable'.
+ * Returns 1 if the BAR was 64-bit and 0 if it was 32-bit.
+ */
+static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
+			struct resource *res, unsigned int pos)
 {
-	if ((mask & (PCI_BASE_ADDRESS_SPACE|PCI_BASE_ADDRESS_MEM_TYPE_MASK)) ==
-	    (PCI_BASE_ADDRESS_SPACE_MEMORY|PCI_BASE_ADDRESS_MEM_TYPE_64))
-		return 1;
-	return 0;
-}
+	u32 l, sz, mask;
 
-static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
-{
-	unsigned int pos, reg, next;
-	u32 l, sz;
-	struct resource *res;
+	mask = type ? ~PCI_ROM_ADDRESS_ENABLE : ~0;
 
-	for(pos=0; pos<howmany; pos = next) {
-		u64 l64;
-		u64 sz64;
-		u32 raw_sz;
+	res->name = pci_name(dev);
 
-		next = pos+1;
-		res = &dev->resource[pos];
-		res->name = pci_name(dev);
-		reg = PCI_BASE_ADDRESS_0 + (pos << 2);
-		pci_read_config_dword(dev, reg, &l);
-		pci_write_config_dword(dev, reg, ~0);
-		pci_read_config_dword(dev, reg, &sz);
-		pci_write_config_dword(dev, reg, l);
-		if (!sz || sz == 0xffffffff)
-			continue;
-		if (l == 0xffffffff)
-			l = 0;
-		raw_sz = sz;
-		if ((l & PCI_BASE_ADDRESS_SPACE) ==
-				PCI_BASE_ADDRESS_SPACE_MEMORY) {
-			sz = pci_size(l, sz, (u32)PCI_BASE_ADDRESS_MEM_MASK);
-			/*
-			 * For 64bit prefetchable memory sz could be 0, if the
-			 * real size is bigger than 4G, so we need to check
-			 * szhi for that.
-			 */
-			if (!is_64bit_memory(l) && !sz)
-				continue;
-			res->start = l & PCI_BASE_ADDRESS_MEM_MASK;
-			res->flags |= l & ~PCI_BASE_ADDRESS_MEM_MASK;
+	pci_read_config_dword(dev, pos, &l);
+	pci_write_config_dword(dev, pos, mask);
+	pci_read_config_dword(dev, pos, &sz);
+	pci_write_config_dword(dev, pos, l);
+
+	/*
+	 * All bits set in sz means the device isn't working properly.
+	 * If the BAR isn't implemented, all bits must be 0.  If it's a
+	 * memory BAR or a ROM, bit 0 must be clear; if it's an io BAR, bit
+	 * 1 must be clear.
+	 */
+	if (!sz || sz == 0xffffffff)
+		goto fail;
+
+	/*
+	 * I don't know how l can have all bits set.  Copied from old code.
+	 * Maybe it fixes a bug on some ancient platform.
+	 */
+	if (l == 0xffffffff)
+		l = 0;
+
+	if (type == pci_bar_unknown) {
+		type = decode_bar(res, l);
+		res->flags |= pci_calc_resource_flags(l) | IORESOURCE_SIZEALIGN;
+		if (type == pci_bar_io) {
+			l &= PCI_BASE_ADDRESS_IO_MASK;
+			mask = PCI_BASE_ADDRESS_IO_MASK & 0xffff;
 		} else {
-			sz = pci_size(l, sz, PCI_BASE_ADDRESS_IO_MASK & 0xffff);
-			if (!sz)
-				continue;
-			res->start = l & PCI_BASE_ADDRESS_IO_MASK;
-			res->flags |= l & ~PCI_BASE_ADDRESS_IO_MASK;
+			l &= PCI_BASE_ADDRESS_MEM_MASK;
+			mask = (u32)PCI_BASE_ADDRESS_MEM_MASK;
 		}
-		res->end = res->start + (unsigned long) sz;
-		res->flags |= pci_calc_resource_flags(l) | IORESOURCE_SIZEALIGN;
-		if (is_64bit_memory(l)) {
-			u32 szhi, lhi;
-
-			pci_read_config_dword(dev, reg+4, &lhi);
-			pci_write_config_dword(dev, reg+4, ~0);
-			pci_read_config_dword(dev, reg+4, &szhi);
-			pci_write_config_dword(dev, reg+4, lhi);
-			sz64 = ((u64)szhi << 32) | raw_sz;
-			l64 = ((u64)lhi << 32) | l;
-			sz64 = pci_size64(l64, sz64, PCI_BASE_ADDRESS_MEM_MASK);
-			next++;
-#if BITS_PER_LONG == 64
-			if (!sz64) {
-				res->start = 0;
-				res->end = 0;
-				res->flags = 0;
-				continue;
-			}
-			res->start = l64 & PCI_BASE_ADDRESS_MEM_MASK;
-			res->end = res->start + sz64;
-#else
-			if (sz64 > 0x100000000ULL) {
-				dev_err(&dev->dev, "BAR %d: can't handle 64-bit"
-					" BAR\n", pos);
-				res->start = 0;
-				res->flags = 0;
-			} else if (lhi) {
-				/* 64-bit wide address, treat as disabled */
-				pci_write_config_dword(dev, reg,
-					l & ~(u32)PCI_BASE_ADDRESS_MEM_MASK);
-				pci_write_config_dword(dev, reg+4, 0);
-				res->start = 0;
-				res->end = sz;
-			}
-#endif
+	} else {
+		res->flags |= (l & IORESOURCE_ROM_ENABLE);
+		l &= PCI_ROM_ADDRESS_MASK;
+		mask = (u32)PCI_ROM_ADDRESS_MASK;
+	}
+
+	if (type == pci_bar_mem64) {
+		u64 l64 = l;
+		u64 sz64 = sz;
+		u64 mask64 = mask | (u64)~0 << 32;
+
+		pci_read_config_dword(dev, pos + 4, &l);
+		pci_write_config_dword(dev, pos + 4, ~0);
+		pci_read_config_dword(dev, pos + 4, &sz);
+		pci_write_config_dword(dev, pos + 4, l);
+
+		l64 |= ((u64)l << 32);
+		sz64 |= ((u64)sz << 32);
+
+		sz64 = pci_size(l64, sz64, mask64);
+
+		if (!sz64)
+			goto fail;
+
+		if ((BITS_PER_LONG < 64) && (sz64 > 0x100000000ULL)) {
+			dev_err(&dev->dev, "can't handle 64-bit BAR\n");
+			goto fail;
+		} else if ((BITS_PER_LONG < 64) && l) {
+			/* Address above 32-bit boundary; disable the BAR */
+			pci_write_config_dword(dev, pos, 0);
+			pci_write_config_dword(dev, pos + 4, 0);
+			res->start = 0;
+			res->end = sz64;
+		} else {
+			res->start = l64;
+			res->end = l64 + sz64;
 		}
+	} else {
+		sz = pci_size(l, sz, mask);
+
+		if (!sz)
+			goto fail;
+
+		res->start = l;
+		res->end = l + sz;
 	}
+
+ out:
+	return (type == pci_bar_mem64) ? 1 : 0;
+ fail:
+	res->flags = 0;
+	goto out;
+}
+
+static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
+{
+	unsigned int pos, reg;
+
+	for (pos = 0; pos < howmany; pos++) {
+		struct resource *res = &dev->resource[pos];
+		reg = PCI_BASE_ADDRESS_0 + (pos << 2);
+		pos += __pci_read_base(dev, pci_bar_unknown, res, reg);
+	}
+
 	if (rom) {
+		struct resource *res = &dev->resource[PCI_ROM_RESOURCE];
 		dev->rom_base_reg = rom;
-		res = &dev->resource[PCI_ROM_RESOURCE];
-		res->name = pci_name(dev);
-		pci_read_config_dword(dev, rom, &l);
-		pci_write_config_dword(dev, rom, ~PCI_ROM_ADDRESS_ENABLE);
-		pci_read_config_dword(dev, rom, &sz);
-		pci_write_config_dword(dev, rom, l);
-		if (l == 0xffffffff)
-			l = 0;
-		if (sz && sz != 0xffffffff) {
-			sz = pci_size(l, sz, (u32)PCI_ROM_ADDRESS_MASK);
-			if (sz) {
-				res->flags = (l & IORESOURCE_ROM_ENABLE) |
-				  IORESOURCE_MEM | IORESOURCE_PREFETCH |
-				  IORESOURCE_READONLY | IORESOURCE_CACHEABLE |
-				  IORESOURCE_SIZEALIGN;
-				res->start = l & PCI_ROM_ADDRESS_MASK;
-				res->end = res->start + (unsigned long) sz;
-			}
-		}
+		res->flags = IORESOURCE_MEM | IORESOURCE_PREFETCH |
+				IORESOURCE_READONLY | IORESOURCE_CACHEABLE |
+				IORESOURCE_SIZEALIGN;
+		__pci_read_base(dev, pci_bar_mem32, res, rom);
 	}
 }
 
-- 
1.5.5.4

[PATCH 2/3] PCI: Handle 64-bit resources better on 32-bit machines

[Matthew Wilcox]

If the kernel is configured to support 64-bit resources on a 32-bit
machine, we can support 64-bit BARs properly.  Just change the condition
to check sizeof(resource_size_t) instead of BITS_PER_LONG.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/pci/probe.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 3b690c3..2036300 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -270,10 +270,10 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 		if (!sz64)
 			goto fail;
 
-		if ((BITS_PER_LONG < 64) && (sz64 > 0x100000000ULL)) {
+		if ((sizeof(resource_size_t) < 8) && (sz64 > 0x100000000ULL)) {
 			dev_err(&dev->dev, "can't handle 64-bit BAR\n");
 			goto fail;
-		} else if ((BITS_PER_LONG < 64) && l) {
+		} else if ((sizeof(resource_size_t) < 8) && l) {
 			/* Address above 32-bit boundary; disable the BAR */
 			pci_write_config_dword(dev, pos, 0);
 			pci_write_config_dword(dev, pos + 4, 0);
-- 
1.5.5.4

[PATCH 3/3] PCI: Add pci_read_base() API

[Matthew Wilcox]

Some devices have a BAR at a non-standard address.  The pci_read_base()
API allows us to probe these BARs and fill in a resource for it as if
they were standard BARs.

Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
---
 drivers/pci/probe.c |   47 ++++++++++++++++++++++++++++++++++++++++-------
 include/linux/pci.h |   10 ++++++++++
 2 files changed, 50 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 2036300..a977d07 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -181,13 +181,6 @@ static u64 pci_size(u64 base, u64 maxbase, u64 mask)
 	return size;
 }
 
-enum pci_bar_type {
-	pci_bar_unknown,	/* Standard PCI BAR probe */
-	pci_bar_io,		/* An io port BAR */
-	pci_bar_mem32,		/* A 32-bit memory BAR */
-	pci_bar_mem64,		/* A 64-bit memory BAR */
-};
-
 static inline enum pci_bar_type decode_bar(struct resource *res, u32 bar)
 {
 	if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
@@ -300,6 +293,46 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
 	goto out;
 }
 
+/**
+ * pci_read_base - Read a BAR from a specified location
+ * @dev: The PCI device to read
+ * @type: The type of BAR to read
+ * @res: A struct resource to be filled in
+ * @reg: The address in PCI config space to read the BAR from.
+ *
+ * Some devices have BARs in unusual places.  This function lets a driver ask
+ * the PCI subsystem to read it and place it in the resource tree.  If it is
+ * like a ROM BAR with an enable in bit 0, the caller should specify a @type
+ * of io, mem32 or mem64.  If it's like a normal BAR with memory type in the
+ * low bits, specify unknown, even if the caller knows what kind of BAR it is.
+ *
+ * Returns -ENXIO if the BAR was not successfully read.  If the BAR is read,
+ * but no suitable parent resource can be found for the BAR, this function
+ * returns -ENODEV.  If the resource cannot be inserted into the resource tree,
+ * it will return -EBUSY.  Note that the resource is still 'live' for these
+ * last two cases; the caller should set res->flags to 0 if this is not wanted.
+ */
+int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
+					struct resource *res, unsigned int reg)
+{
+	struct pci_bus_region region;
+	struct resource *parent;
+
+	__pci_read_base(dev, type, res, reg);
+	if (!res->flags)
+		return -ENXIO;
+
+	region.start = res->start;
+	region.end = res->end;
+	pcibios_bus_to_resource(dev, res, &region);
+
+	parent = pci_find_parent_resource(dev, res);
+	if (!parent)
+		return -ENODEV;
+	return request_resource(parent, res);
+}
+EXPORT_SYMBOL_GPL(pci_read_base);
+
 static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
 {
 	unsigned int pos, reg;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index a6a088e..f6ad5e8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -310,6 +310,16 @@ struct pci_bus {
 #define pci_bus_b(n)	list_entry(n, struct pci_bus, node)
 #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
 
+enum pci_bar_type {
+	pci_bar_unknown,	/* Standard PCI BAR probe */
+	pci_bar_io,		/* An io port BAR */
+	pci_bar_mem32,		/* A 32-bit memory BAR */
+	pci_bar_mem64,		/* A 64-bit memory BAR */
+};
+
+int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
+					struct resource *res, unsigned int reg);
+
 /*
  * Error values that may be returned by PCI functions.
  */
-- 
1.5.5.4

Re: [PATCH 2/3] PCI: Handle 64-bit resources better on 32-bit machines

[Jesse Barnes]

On Monday, July 28, 2008 10:39 am Matthew Wilcox wrote:
> If the kernel is configured to support 64-bit resources on a 32-bit
> machine, we can support 64-bit BARs properly.  Just change the condition
> to check sizeof(resource_size_t) instead of BITS_PER_LONG.
>
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/pci/probe.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 3b690c3..2036300 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -270,10 +270,10 @@ static int __pci_read_base(struct pci_dev *dev, enum
> pci_bar_type type, if (!sz64)
>  			goto fail;
>
> -		if ((BITS_PER_LONG < 64) && (sz64 > 0x100000000ULL)) {
> +		if ((sizeof(resource_size_t) < 8) && (sz64 > 0x100000000ULL)) {
>  			dev_err(&dev->dev, "can't handle 64-bit BAR\n");
>  			goto fail;
> -		} else if ((BITS_PER_LONG < 64) && l) {
> +		} else if ((sizeof(resource_size_t) < 8) && l) {
>  			/* Address above 32-bit boundary; disable the BAR */
>  			pci_write_config_dword(dev, pos, 0);
>  			pci_write_config_dword(dev, pos + 4, 0);

Both this and 1/3 look good, I've applied them to my for-linus tree.  I'll 
push them out after a little testing.

3/3 also looks nice; is there any in-tree code that could be converted over to 
using it?  If not, we can just push it as part of Eric's stuff.

Thanks,
Jesse

Re: [PATCH 2/3] PCI: Handle 64-bit resources better on 32-bit machines

[Matthew Wilcox]

On Mon, Jul 28, 2008 at 02:30:20PM -0700, Jesse Barnes wrote:
> Both this and 1/3 look good, I've applied them to my for-linus tree.  I'll 
> push them out after a little testing.

Thanks!

> 3/3 also looks nice; is there any in-tree code that could be converted over to 
> using it?  If not, we can just push it as part of Eric's stuff.

I think it goes best with Eric's patches, but I wanted to get it review
here first.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

RE: [PATCH 3/3] PCI: Add pci_read_base() API

[Zhao, Yu]

Greetings,

It would be nice if people can have an enhanced pci_update_resource to update non-standard BARs probed by pci_read_base.

Thanks,
Yu

>-----Original Message-----
>From: linux-pci-owner@vger.kernel.org
>[mailto:linux-pci-owner@vger.kernel.org] On Behalf Of Matthew Wilcox
>Sent: Tuesday, July 29, 2008 1:39 AM
>To: jbarnes@virtuousgeek.org
>Cc: linux-kernel@vger.kernel.org; eric@anholt.net; Matthew Wilcox; Matthew
>Wilcox
>Subject: [PATCH 3/3] PCI: Add pci_read_base() API
>
>Some devices have a BAR at a non-standard address.  The pci_read_base()
>API allows us to probe these BARs and fill in a resource for it as if
>they were standard BARs.
>
>Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
>---
> drivers/pci/probe.c |   47 ++++++++++++++++++++++++++++++++++++++++-------
> include/linux/pci.h |   10 ++++++++++
> 2 files changed, 50 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>index 2036300..a977d07 100644
>--- a/drivers/pci/probe.c
>+++ b/drivers/pci/probe.c
>@@ -181,13 +181,6 @@ static u64 pci_size(u64 base, u64 maxbase, u64 mask)
> 	return size;
> }
>
>-enum pci_bar_type {
>-	pci_bar_unknown,	/* Standard PCI BAR probe */
>-	pci_bar_io,		/* An io port BAR */
>-	pci_bar_mem32,		/* A 32-bit memory BAR */
>-	pci_bar_mem64,		/* A 64-bit memory BAR */
>-};
>-
> static inline enum pci_bar_type decode_bar(struct resource *res, u32 bar)
> {
> 	if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
>@@ -300,6 +293,46 @@ static int __pci_read_base(struct pci_dev *dev, enum
>pci_bar_type type,
> 	goto out;
> }
>
>+/**
>+ * pci_read_base - Read a BAR from a specified location
>+ * @dev: The PCI device to read
>+ * @type: The type of BAR to read
>+ * @res: A struct resource to be filled in
>+ * @reg: The address in PCI config space to read the BAR from.
>+ *
>+ * Some devices have BARs in unusual places.  This function lets a driver ask
>+ * the PCI subsystem to read it and place it in the resource tree.  If it is
>+ * like a ROM BAR with an enable in bit 0, the caller should specify a @type
>+ * of io, mem32 or mem64.  If it's like a normal BAR with memory type in the
>+ * low bits, specify unknown, even if the caller knows what kind of BAR it is.
>+ *
>+ * Returns -ENXIO if the BAR was not successfully read.  If the BAR is read,
>+ * but no suitable parent resource can be found for the BAR, this function
>+ * returns -ENODEV.  If the resource cannot be inserted into the resource tree,
>+ * it will return -EBUSY.  Note that the resource is still 'live' for these
>+ * last two cases; the caller should set res->flags to 0 if this is not wanted.
>+ */
>+int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>+					struct resource *res, unsigned int reg)
>+{
>+	struct pci_bus_region region;
>+	struct resource *parent;
>+
>+	__pci_read_base(dev, type, res, reg);
>+	if (!res->flags)
>+		return -ENXIO;
>+
>+	region.start = res->start;
>+	region.end = res->end;
>+	pcibios_bus_to_resource(dev, res, &region);
>+
>+	parent = pci_find_parent_resource(dev, res);
>+	if (!parent)
>+		return -ENODEV;
>+	return request_resource(parent, res);
>+}
>+EXPORT_SYMBOL_GPL(pci_read_base);
>+
> static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
> {
> 	unsigned int pos, reg;
>diff --git a/include/linux/pci.h b/include/linux/pci.h
>index a6a088e..f6ad5e8 100644
>--- a/include/linux/pci.h
>+++ b/include/linux/pci.h
>@@ -310,6 +310,16 @@ struct pci_bus {
> #define pci_bus_b(n)	list_entry(n, struct pci_bus, node)
> #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
>
>+enum pci_bar_type {
>+	pci_bar_unknown,	/* Standard PCI BAR probe */
>+	pci_bar_io,		/* An io port BAR */
>+	pci_bar_mem32,		/* A 32-bit memory BAR */
>+	pci_bar_mem64,		/* A 64-bit memory BAR */
>+};
>+
>+int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>+					struct resource *res, unsigned int reg);
>+
> /*
>  * Error values that may be returned by PCI functions.
>  */
>--
>1.5.5.4
>--
>To unsubscribe from this list: send the line "unsubscribe linux-pci" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] PCI: Add pci_read_base() API

[Matthew Wilcox]

On Tue, Jul 29, 2008 at 09:22:03AM +0800, Zhao, Yu wrote:
> Greetings,
> 
> It would be nice if people can have an enhanced pci_update_resource to update non-standard BARs probed by pci_read_base.

Yes, that would be a natural addition to the API.  However, we don't
tend to add APIs until we have a use case for them.  Do you have
anything in the pipeline that could use this feature?  I'm quite happy
to write the code for you if you do.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

RE: [PATCH 3/3] PCI: Add pci_read_base() API

[Zhao, Yu]

On Tuesday, July 29, 2008 9:50 AM, Matthew Wilcox <mailto:matthew@wil.cx> wrote:

> On Tue, Jul 29, 2008 at 09:22:03AM +0800, Zhao, Yu wrote:
>> Greetings,
>> 
>> It would be nice if people can have an enhanced
> pci_update_resource to update non-standard BARs probed by
> pci_read_base. 
> 
> Yes, that would be a natural addition to the API.  However, we don't
> tend to add APIs until we have a use case for them.  Do you have
> anything in the pipeline that could use this feature?  I'm quite happy
> to write the code for you if you do.

There is a new PCIe extended capability SR-IOV, which defines 6 additional BARs encapsulated inside the capability. I'm writing code to support it, and will be using your APIs to probe and update these non-standard BARs.

The SR-IOV specification can be found at:
http://www.pcisig.com/members/downloads/specifications/iov/sr-iov1.0_11Sep07.pdf

Thanks.

> 
> --
> Intel are signing my paycheques ... these opinions are still mine
> "Bill, look, we understand that you're interested in selling us this
> operating system, but compare it to ours.  We can't possibly take such
> a retrograde step."

Re: [PATCH 3/3] PCI: Add pci_read_base() API

[Grant Grundler]

On Mon, Jul 28, 2008 at 01:39:01PM -0400, Matthew Wilcox wrote:
> Some devices have a BAR at a non-standard address.  The pci_read_base()
> API allows us to probe these BARs and fill in a resource for it as if
> they were standard BARs.

Willy,
Can you add a comment to the code listing the offending device?
That way we know how to test next time this code changes.

thanks,
grant

> 
> Signed-off-by: Matthew Wilcox <willy@linux.intel.com>
> ---
>  drivers/pci/probe.c |   47 ++++++++++++++++++++++++++++++++++++++++-------
>  include/linux/pci.h |   10 ++++++++++
>  2 files changed, 50 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 2036300..a977d07 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -181,13 +181,6 @@ static u64 pci_size(u64 base, u64 maxbase, u64 mask)
>  	return size;
>  }
>  
> -enum pci_bar_type {
> -	pci_bar_unknown,	/* Standard PCI BAR probe */
> -	pci_bar_io,		/* An io port BAR */
> -	pci_bar_mem32,		/* A 32-bit memory BAR */
> -	pci_bar_mem64,		/* A 64-bit memory BAR */
> -};
> -
>  static inline enum pci_bar_type decode_bar(struct resource *res, u32 bar)
>  {
>  	if ((bar & PCI_BASE_ADDRESS_SPACE) == PCI_BASE_ADDRESS_SPACE_IO) {
> @@ -300,6 +293,46 @@ static int __pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
>  	goto out;
>  }
>  
> +/**
> + * pci_read_base - Read a BAR from a specified location
> + * @dev: The PCI device to read
> + * @type: The type of BAR to read
> + * @res: A struct resource to be filled in
> + * @reg: The address in PCI config space to read the BAR from.
> + *
> + * Some devices have BARs in unusual places.  This function lets a driver ask
> + * the PCI subsystem to read it and place it in the resource tree.  If it is
> + * like a ROM BAR with an enable in bit 0, the caller should specify a @type
> + * of io, mem32 or mem64.  If it's like a normal BAR with memory type in the
> + * low bits, specify unknown, even if the caller knows what kind of BAR it is.
> + *
> + * Returns -ENXIO if the BAR was not successfully read.  If the BAR is read,
> + * but no suitable parent resource can be found for the BAR, this function
> + * returns -ENODEV.  If the resource cannot be inserted into the resource tree,
> + * it will return -EBUSY.  Note that the resource is still 'live' for these
> + * last two cases; the caller should set res->flags to 0 if this is not wanted.
> + */
> +int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> +					struct resource *res, unsigned int reg)
> +{
> +	struct pci_bus_region region;
> +	struct resource *parent;
> +
> +	__pci_read_base(dev, type, res, reg);
> +	if (!res->flags)
> +		return -ENXIO;
> +
> +	region.start = res->start;
> +	region.end = res->end;
> +	pcibios_bus_to_resource(dev, res, &region);
> +
> +	parent = pci_find_parent_resource(dev, res);
> +	if (!parent)
> +		return -ENODEV;
> +	return request_resource(parent, res);
> +}
> +EXPORT_SYMBOL_GPL(pci_read_base);
> +
>  static void pci_read_bases(struct pci_dev *dev, unsigned int howmany, int rom)
>  {
>  	unsigned int pos, reg;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index a6a088e..f6ad5e8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -310,6 +310,16 @@ struct pci_bus {
>  #define pci_bus_b(n)	list_entry(n, struct pci_bus, node)
>  #define to_pci_bus(n)	container_of(n, struct pci_bus, dev)
>  
> +enum pci_bar_type {
> +	pci_bar_unknown,	/* Standard PCI BAR probe */
> +	pci_bar_io,		/* An io port BAR */
> +	pci_bar_mem32,		/* A 32-bit memory BAR */
> +	pci_bar_mem64,		/* A 64-bit memory BAR */
> +};
> +
> +int pci_read_base(struct pci_dev *dev, enum pci_bar_type type,
> +					struct resource *res, unsigned int reg);
> +
>  /*
>   * Error values that may be returned by PCI functions.
>   */
> -- 
> 1.5.5.4
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] PCI: Add pci_read_base() API

[Matthew Wilcox]

On Mon, Aug 04, 2008 at 12:38:33AM -0600, Grant Grundler wrote:
> On Mon, Jul 28, 2008 at 01:39:01PM -0400, Matthew Wilcox wrote:
> > Some devices have a BAR at a non-standard address.  The pci_read_base()
> > API allows us to probe these BARs and fill in a resource for it as if
> > they were standard BARs.
> 
> Willy,
> Can you add a comment to the code listing the offending device?
> That way we know how to test next time this code changes.

It's not an offending device -- devices are allowed to put whatever they
want anywhere outside the first 0x40 bytes of config space.

Eric Anholt's patches add a user for this interface.  If you want to see an
example, download the Intel 3 Series Express Chipset Family Datashee
(the filename is 316966.pdf) and see section 5.1.13 about the MCHBAR.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."

Re: [PATCH 3/3] PCI: Add pci_read_base() API

[Grant Grundler]

On Wed, Aug 06, 2008 at 08:38:45AM -0600, Matthew Wilcox wrote:
> On Mon, Aug 04, 2008 at 12:38:33AM -0600, Grant Grundler wrote:
> > On Mon, Jul 28, 2008 at 01:39:01PM -0400, Matthew Wilcox wrote:
> > > Some devices have a BAR at a non-standard address.  The pci_read_base()
> > > API allows us to probe these BARs and fill in a resource for it as if
> > > they were standard BARs.
> > 
> > Willy,
> > Can you add a comment to the code listing the offending device?
> > That way we know how to test next time this code changes.
> 
> It's not an offending device -- devices are allowed to put whatever they
> want anywhere outside the first 0x40 bytes of config space.

Sorry - bad phrasing on my part.

> Eric Anholt's patches add a user for this interface.

ok - people will find the "user" in the email archives now.

> If you want to see an
> example, download the Intel 3 Series Express Chipset Family Datashee
> (the filename is 316966.pdf) and see section 5.1.13 about the MCHBAR.

This would be a suitable comment in the code.
My point still stands: The next person modifying the code will
need to know how/what to test and where to look it up.

thanks,
grant

Re: [PATCH 3/3] PCI: Add pci_read_base() API

[Matthew Wilcox]

On Sat, Aug 09, 2008 at 06:47:50PM -0600, Grant Grundler wrote:
> This would be a suitable comment in the code.
> My point still stands: The next person modifying the code will
> need to know how/what to test and where to look it up.

It turns out Eric doesn't need this code after all.  So we have no
users, and this patch (3/3) isn't going into the tree.  1/3 and 2/3
are already in, and make the code easier to follow while adding no
additional functionality.

-- 
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."