[PATCH 0/12] pci: update pci bridge resources

[PATCH 0/12] pci: update pci bridge resources

[Yinghai Lu]

this patche set is trying to update pci bridge BAR when that BAR is big enough.

default it is disabled.

could use pci=try=2 to enable it.

Thanks

Yinghai

Re: [PATCH 0/12] pci: update pci bridge resources

[Bjorn Helgaas]

On Fri, 2009-12-18 at 12:54 -0800, Yinghai Lu wrote:
> this patche set is trying to update pci bridge BAR when that BAR is big enough.
> 
> default it is disabled.
> 
> could use pci=try=2 to enable it.

Thanks for posting this as a nice new series.  It was getting hard to
figure out what went where.

I think you mean "when the BAR is *not* big enough."  And strictly
speaking, I think you're concerned with the bridge *window*, which isn't
actually a bridge BAR.

I don't like the new "pci=try=2" parameter.  Linux should be smart
enough to do the right thing without requiring the user to do something
special.  Using a parameter to enable the new code makes it feel like
"here's some new functionality, but I'm not sure it's safe enough for
everybody to use, so try this parameter if you need it."

The result is that the new code will be rarely used and poorly tested,
yet it remains a burden to all future PCI maintainers, who will have to
understand it and try not to break it.

Can you please put the "-v2"-type comments about your development
history in the [0/n] email?  They are useful while reviewing the
different versions, but they don't need to be in the commit logs.

Also, some of the patches in this series have "-v2" in subject, others
have "-v3", others have nothing.  It's easier to follow development of
the series if the version applies to the series as a whole, not to
individual patches.  The version doesn't need to be on the patch subject
line, because that will go into the commit log, where it won't be
relevant.  Tools like stgit put it where it will be automatically
discarded, e.g., "[PATCH -v2 x/n]"

Bjorn

Re: [PATCH 0/12] pci: update pci bridge resources

[Yinghai Lu]

Bjorn Helgaas wrote:
> On Fri, 2009-12-18 at 12:54 -0800, Yinghai Lu wrote:
>> this patche set is trying to update pci bridge BAR when that BAR is big enough.
>>
>> default it is disabled.
>>
>> could use pci=try=2 to enable it.
> 
> Thanks for posting this as a nice new series.  It was getting hard to
> figure out what went where.
> 
> I think you mean "when the BAR is *not* big enough."  And strictly
> speaking, I think you're concerned with the bridge *window*, which isn't
> actually a bridge BAR.

in PCI bridge to bridge spec 1.1, page 38, 3.2.5.1 still call them bridge "Base Address Registers"

so should be fine we call them Bridge BAR, 

and we still have device BAR...

> 
> I don't like the new "pci=try=2" parameter.  Linux should be smart
> enough to do the right thing without requiring the user to do something
> special.  Using a parameter to enable the new code makes it feel like
> "here's some new functionality, but I'm not sure it's safe enough for
> everybody to use, so try this parameter if you need it."

try to get the code merge at first. those code have been there for a while.
it should be good enable it by default

> 
> The result is that the new code will be rarely used and poorly tested,
> yet it remains a burden to all future PCI maintainers, who will have to
> understand it and try not to break it.
> 
> Can you please put the "-v2"-type comments about your development
> history in the [0/n] email?  They are useful while reviewing the
> different versions, but they don't need to be in the commit logs.

actually it should be -v14.

will put back the history later.

> 
> Also, some of the patches in this series have "-v2" in subject, others
> have "-v3", others have nothing.  It's easier to follow development of
> the series if the version applies to the series as a whole, not to
> individual patches.  The version doesn't need to be on the patch subject
> line, because that will go into the commit log, where it won't be
> relevant.  Tools like stgit put it where it will be automatically
> discarded, e.g., "[PATCH -v2 x/n]"

ok. will try that on next time.

YH

Re: [PATCH 0/12] pci: update pci bridge resources

[Bjorn Helgaas]

On Sun, 2009-12-20 at 17:42 -0800, Yinghai Lu wrote:
> Bjorn Helgaas wrote:
> > On Fri, 2009-12-18 at 12:54 -0800, Yinghai Lu wrote:
> >> this patche set is trying to update pci bridge BAR when that BAR is big enough.
> >>
> >> default it is disabled.
> >>
> >> could use pci=try=2 to enable it.
> > 
> > I think you mean "when the BAR is *not* big enough."  And strictly
> > speaking, I think you're concerned with the bridge *window*, which isn't
> > actually a bridge BAR.
> 
> in PCI bridge to bridge spec 1.1, page 38, 3.2.5.1 still call them bridge "Base Address Registers"
> 
> so should be fine we call them Bridge BAR, 
> 
> and we still have device BAR...

PCI bridges have optional BARs, described as you point out in section
3.2.5.1.  These work the same as BARs on any other PCI device, and your
patches aren't concerned with these.

Your patch series is primarily concerned with the bridge Base & Limit
registers described in sections 3.2.5.6 (I/O), 3.2.5.8 (memory), and
3.2.5.9 (prefetchable memory).  These define the address ranges
forwarded from one bridge interface to the other.  The spec doesn't have
a nice term for these ranges, but they're often referred to as "windows"
or "apertures" because they are the areas where you can "see through"
the bridge to devices on the other side.

Bjorn

Re: [PATCH 0/12] pci: update pci bridge resources

[Yinghai Lu]

Bjorn Helgaas wrote:
> On Sun, 2009-12-20 at 17:42 -0800, Yinghai Lu wrote:
>> Bjorn Helgaas wrote:
>>> On Fri, 2009-12-18 at 12:54 -0800, Yinghai Lu wrote:
>>>> this patche set is trying to update pci bridge BAR when that BAR is big enough.
>>>>
>>>> default it is disabled.
>>>>
>>>> could use pci=try=2 to enable it.
>>> I think you mean "when the BAR is *not* big enough."  And strictly
>>> speaking, I think you're concerned with the bridge *window*, which isn't
>>> actually a bridge BAR.
>> in PCI bridge to bridge spec 1.1, page 38, 3.2.5.1 still call them bridge "Base Address Registers"
>>
>> so should be fine we call them Bridge BAR, 
>>
>> and we still have device BAR...
> 
> PCI bridges have optional BARs, described as you point out in section
> 3.2.5.1.  These work the same as BARs on any other PCI device, and your
> patches aren't concerned with these.
> 
> Your patch series is primarily concerned with the bridge Base & Limit
> registers described in sections 3.2.5.6 (I/O), 3.2.5.8 (memory), and
> 3.2.5.9 (prefetchable memory).  These define the address ranges
> forwarded from one bridge interface to the other.  The spec doesn't have
> a nice term for these ranges, but they're often referred to as "windows"
> or "apertures" because they are the areas where you can "see through"
> the bridge to devices on the other side.

I have following patch. how to format the "bar reading" ?

[PATCH] pci: reject mmio range start from 0 on pci_bridge read

that is wrong.

exposed by that patch that doesn't shrink pci bridge res.

Signed-off-by: Yinghai Lu <yinghai@kernel.org>

---
 drivers/pci/probe.c |   18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

Index: linux-2.6/drivers/pci/probe.c
===================================================================
--- linux-2.6.orig/drivers/pci/probe.c
+++ linux-2.6/drivers/pci/probe.c
@@ -316,13 +316,17 @@ void __devinit pci_read_bridge_bases(str
 		limit |= (io_limit_hi << 16);
 	}
 
-	if (base <= limit) {
+	if (base <= limit && base) {
 		res->flags = (io_base_lo & PCI_IO_RANGE_TYPE_MASK) | IORESOURCE_IO;
 		if (!res->start)
 			res->start = base;
 		if (!res->end)
 			res->end = limit + 0xfff;
 		dev_printk(KERN_DEBUG, &dev->dev, "  bridge window %pR\n", res);
+	} else {
+		dev_printk(KERN_DEBUG, &dev->dev,
+			 "  bridge window [io  %04lx - %04lx] bar reading\n",
+				 base, limit);
 	}
 
 	res = child->resource[1];
@@ -330,11 +334,15 @@ void __devinit pci_read_bridge_bases(str
 	pci_read_config_word(dev, PCI_MEMORY_LIMIT, &mem_limit_lo);
 	base = (mem_base_lo & PCI_MEMORY_RANGE_MASK) << 16;
 	limit = (mem_limit_lo & PCI_MEMORY_RANGE_MASK) << 16;
-	if (base <= limit) {
+	if (base <= limit && base) {
 		res->flags = (mem_base_lo & PCI_MEMORY_RANGE_TYPE_MASK) | IORESOURCE_MEM;
 		res->start = base;
 		res->end = limit + 0xfffff;
 		dev_printk(KERN_DEBUG, &dev->dev, "  bridge window %pR\n", res);
+	} else {
+		dev_printk(KERN_DEBUG, &dev->dev,
+			 "  bridge window [mem 0x%08lx - 0x%08lx] bar reading\n",
+					 base, limit + 0xfffff);
 	}
 
 	res = child->resource[2];
@@ -366,7 +374,7 @@ void __devinit pci_read_bridge_bases(str
 #endif
 		}
 	}
-	if (base <= limit) {
+	if (base <= limit && base) {
 		res->flags = (mem_base_lo & PCI_PREF_RANGE_TYPE_MASK) |
 					 IORESOURCE_MEM | IORESOURCE_PREFETCH;
 		if (res->flags & PCI_PREF_RANGE_TYPE_64)
@@ -374,6 +382,10 @@ void __devinit pci_read_bridge_bases(str
 		res->start = base;
 		res->end = limit + 0xfffff;
 		dev_printk(KERN_DEBUG, &dev->dev, "  bridge window %pR\n", res);
+	} else {
+		dev_printk(KERN_DEBUG, &dev->dev,
+			 "  bridge window [mem 0x%016lx - %016lx pref] bar reading\n",
+					 base, limit + 0xfffff);
 	}
 }