* [PATCH] pci: 64bit resource fix in setup-res.c @ 2012-06-20 19:56 Nikhil P Rao 2012-06-21 23:47 ` [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() Nikhil P Rao 2012-07-11 23:12 ` [PATCH] pci: 64bit resource fix in setup-res.c Bjorn Helgaas 0 siblings, 2 replies; 8+ messages in thread From: Nikhil P Rao @ 2012-06-20 19:56 UTC (permalink / raw) To: linux-pci; +Cc: linux-kernel, Jesse Barnes, Nikhil P Rao size parameter of _pci_assign_resource() needs to be of type resource_size_t rather than int Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> --- drivers/pci/setup-res.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c index eea85da..9f6bbec 100644 --- a/drivers/pci/setup-res.c +++ b/drivers/pci/setup-res.c @@ -206,7 +206,8 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev, return ret; } -static int _pci_assign_resource(struct pci_dev *dev, int resno, int size, resource_size_t min_align) +static int _pci_assign_resource(struct pci_dev *dev, int resno, + resource_size_t size, resource_size_t min_align) { struct resource *res = dev->resource + resno; struct pci_bus *bus; -- 1.7.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() 2012-06-20 19:56 [PATCH] pci: 64bit resource fix in setup-res.c Nikhil P Rao @ 2012-06-21 23:47 ` Nikhil P Rao 2012-06-23 18:15 ` Bjorn Helgaas 2012-07-11 23:12 ` [PATCH] pci: 64bit resource fix in setup-res.c Bjorn Helgaas 1 sibling, 1 reply; 8+ messages in thread From: Nikhil P Rao @ 2012-06-21 23:47 UTC (permalink / raw) To: linux-pci; +Cc: linux-kernel, Jesse Barnes, nikhil.rao I ran into the "disabling BAR .." error message when trying to use a 8Gb PCIe card on a system with a BIOS that didnt have support for BAR size > 2Gb. This patch fixed the problem, but I also see the code reading the IORESOURCE_MEM_64 flag so I am not sure what the right solution is Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> --- drivers/pci/setup-bus.c | 4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 8fa2d4b..3b3601f 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -780,7 +780,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, { struct pci_dev *dev; resource_size_t min_align, align, size, size0, size1; - resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */ + resource_size_t aligns[14]; /* Alignments from 1Mb to 8Gb */ int order, max_order; struct resource *b_res = find_free_bus_resource(bus, type); unsigned int mem64_mask = 0; @@ -819,7 +819,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, /* For bridges size != alignment */ align = pci_resource_alignment(dev, r); order = __ffs(align) - 20; - if (order > 11) { + if (order >= ARRAY_SIZE(aligns)) { dev_warn(&dev->dev, "disabling BAR %d: %pR " "(bad alignment %#llx)\n", i, r, (unsigned long long) align); -- 1.7.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() 2012-06-21 23:47 ` [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() Nikhil P Rao @ 2012-06-23 18:15 ` Bjorn Helgaas 2012-06-25 20:54 ` Nikhil P Rao 0 siblings, 1 reply; 8+ messages in thread From: Bjorn Helgaas @ 2012-06-23 18:15 UTC (permalink / raw) To: Nikhil P Rao; +Cc: linux-pci, linux-kernel, Jesse Barnes On Thu, Jun 21, 2012 at 5:47 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: > I ran into the "disabling BAR .." error message when > trying to use a 8Gb PCIe card on a system with a BIOS > that didnt have support for BAR size > 2Gb. So the BIOS left the 8Gb BAR unassigned, and you got the "disabling BAR ... (bad alignment)" message when Linux tried to enable it? How do we know 8Gb is the correct new limit? Are we going to be fixing this again when we see a 16Gb or a 32Gb BAR? Do we need a better algorithm that doesn't have a limit like this? This patch fixed > the problem, but I also see the code reading the IORESOURCE_MEM_64 > flag so I am not sure what the right solution is > > Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> > --- > drivers/pci/setup-bus.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 8fa2d4b..3b3601f 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -780,7 +780,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > { > struct pci_dev *dev; > resource_size_t min_align, align, size, size0, size1; > - resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */ > + resource_size_t aligns[14]; /* Alignments from 1Mb to 8Gb */ > int order, max_order; > struct resource *b_res = find_free_bus_resource(bus, type); > unsigned int mem64_mask = 0; > @@ -819,7 +819,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > /* For bridges size != alignment */ > align = pci_resource_alignment(dev, r); > order = __ffs(align) - 20; > - if (order > 11) { > + if (order >= ARRAY_SIZE(aligns)) { > dev_warn(&dev->dev, "disabling BAR %d: %pR " > "(bad alignment %#llx)\n", i, r, > (unsigned long long) align); > -- > 1.7.1 > > > > -- > 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() 2012-06-23 18:15 ` Bjorn Helgaas @ 2012-06-25 20:54 ` Nikhil P Rao 2012-07-11 22:53 ` Bjorn Helgaas 0 siblings, 1 reply; 8+ messages in thread From: Nikhil P Rao @ 2012-06-25 20:54 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Jesse Barnes On Sat, 2012-06-23 at 12:15 -0600, Bjorn Helgaas wrote: > On Thu, Jun 21, 2012 at 5:47 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: > > I ran into the "disabling BAR .." error message when > > trying to use a 8Gb PCIe card on a system with a BIOS > > that didnt have support for BAR size > 2Gb. > > So the BIOS left the 8Gb BAR unassigned, and you got the "disabling > BAR ... (bad alignment)" message when Linux tried to enable it? Yes. > How do we know 8Gb is the correct new limit? Are we going to be > fixing this again when we see a 16Gb or a 32Gb BAR? Do we need a > better algorithm that doesn't have a limit like this? > The original error message seems applicable to 32bit archs. and not to 64 bit archs. How about the patch below - is aligns[44] (256bytes more) acceptable ? From: Nikhil P Rao <nikhil.rao@intel.com> Date: Mon, 25 Jun 2012 13:33:55 -0700 Subject: [PATCH] pci: fix resource size check Support a PCI BAR alignment of > 2Gb, the original check was only applicable to 32 bit kernels, Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> --- drivers/pci/setup-bus.c | 5 +++-- 1 files changed, 3 insertions(+), 2 deletions(-) diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c index 8fa2d4b..9f8d9ea 100644 --- a/drivers/pci/setup-bus.c +++ b/drivers/pci/setup-bus.c @@ -780,7 +780,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, { struct pci_dev *dev; resource_size_t min_align, align, size, size0, size1; - resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */ + resource_size_t aligns[44]; /* Alignments from 1Mb to 2^63 */ int order, max_order; struct resource *b_res = find_free_bus_resource(bus, type); unsigned int mem64_mask = 0; @@ -819,7 +819,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, /* For bridges size != alignment */ align = pci_resource_alignment(dev, r); order = __ffs(align) - 20; - if (order > 11) { + if ((sizeof(size_t) == 4 && order > 11) || + (sizeof(size_t) == 8 && order > 43)) { dev_warn(&dev->dev, "disabling BAR %d: %pR " "(bad alignment %#llx)\n", i, r, (unsigned long long) align); -- 1.7.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() 2012-06-25 20:54 ` Nikhil P Rao @ 2012-07-11 22:53 ` Bjorn Helgaas 2012-07-12 0:13 ` Yinghai Lu 0 siblings, 1 reply; 8+ messages in thread From: Bjorn Helgaas @ 2012-07-11 22:53 UTC (permalink / raw) To: Nikhil P Rao; +Cc: linux-pci, linux-kernel, Jesse Barnes, Yinghai Lu On Mon, Jun 25, 2012 at 2:54 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: > On Sat, 2012-06-23 at 12:15 -0600, Bjorn Helgaas wrote: >> On Thu, Jun 21, 2012 at 5:47 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: >> > I ran into the "disabling BAR .." error message when >> > trying to use a 8Gb PCIe card on a system with a BIOS >> > that didnt have support for BAR size > 2Gb. >> >> So the BIOS left the 8Gb BAR unassigned, and you got the "disabling >> BAR ... (bad alignment)" message when Linux tried to enable it? > > Yes. > >> How do we know 8Gb is the correct new limit? Are we going to be >> fixing this again when we see a 16Gb or a 32Gb BAR? Do we need a >> better algorithm that doesn't have a limit like this? >> > > The original error message seems applicable to 32bit archs. and not to > 64 bit archs. How about the patch below - is aligns[44] (256bytes more) > acceptable ? > > > From: Nikhil P Rao <nikhil.rao@intel.com> > Date: Mon, 25 Jun 2012 13:33:55 -0700 > Subject: [PATCH] pci: fix resource size check > > Support a PCI BAR alignment of > 2Gb, the original check was > only applicable to 32 bit kernels, > > Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> > --- > drivers/pci/setup-bus.c | 5 +++-- > 1 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c > index 8fa2d4b..9f8d9ea 100644 > --- a/drivers/pci/setup-bus.c > +++ b/drivers/pci/setup-bus.c > @@ -780,7 +780,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > { > struct pci_dev *dev; > resource_size_t min_align, align, size, size0, size1; > - resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */ > + resource_size_t aligns[44]; /* Alignments from 1Mb to 2^63 */ > int order, max_order; > struct resource *b_res = find_free_bus_resource(bus, type); > unsigned int mem64_mask = 0; > @@ -819,7 +819,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, > /* For bridges size != alignment */ > align = pci_resource_alignment(dev, r); > order = __ffs(align) - 20; > - if (order > 11) { > + if ((sizeof(size_t) == 4 && order > 11) || > + (sizeof(size_t) == 8 && order > 43)) { > dev_warn(&dev->dev, "disabling BAR %d: %pR " > "(bad alignment %#llx)\n", i, r, > (unsigned long long) align); Yinghai, what's your opinion on this? The aligns[] array on the stack is currently 96 bytes and would grow to 352 with this patch, which does seem like quite a bit. I do think the 2GB limit here is out-of-date. Bjorn ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() 2012-07-11 22:53 ` Bjorn Helgaas @ 2012-07-12 0:13 ` Yinghai Lu 2012-07-12 20:34 ` Bjorn Helgaas 0 siblings, 1 reply; 8+ messages in thread From: Yinghai Lu @ 2012-07-12 0:13 UTC (permalink / raw) To: Bjorn Helgaas; +Cc: Nikhil P Rao, linux-pci, linux-kernel, Jesse Barnes On Wed, Jul 11, 2012 at 3:53 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Mon, Jun 25, 2012 at 2:54 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: >> On Sat, 2012-06-23 at 12:15 -0600, Bjorn Helgaas wrote: >>> On Thu, Jun 21, 2012 at 5:47 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: >>> > I ran into the "disabling BAR .." error message when >>> > trying to use a 8Gb PCIe card on a system with a BIOS >>> > that didnt have support for BAR size > 2Gb. >>> >>> So the BIOS left the 8Gb BAR unassigned, and you got the "disabling >>> BAR ... (bad alignment)" message when Linux tried to enable it? >> >> Yes. >> >>> How do we know 8Gb is the correct new limit? Are we going to be >>> fixing this again when we see a 16Gb or a 32Gb BAR? Do we need a >>> better algorithm that doesn't have a limit like this? >>> >> >> The original error message seems applicable to 32bit archs. and not to >> 64 bit archs. How about the patch below - is aligns[44] (256bytes more) >> acceptable ? >> >> >> From: Nikhil P Rao <nikhil.rao@intel.com> >> Date: Mon, 25 Jun 2012 13:33:55 -0700 >> Subject: [PATCH] pci: fix resource size check >> >> Support a PCI BAR alignment of > 2Gb, the original check was >> only applicable to 32 bit kernels, >> >> Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> >> --- >> drivers/pci/setup-bus.c | 5 +++-- >> 1 files changed, 3 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c >> index 8fa2d4b..9f8d9ea 100644 >> --- a/drivers/pci/setup-bus.c >> +++ b/drivers/pci/setup-bus.c >> @@ -780,7 +780,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, >> { >> struct pci_dev *dev; >> resource_size_t min_align, align, size, size0, size1; >> - resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */ >> + resource_size_t aligns[44]; /* Alignments from 1Mb to 2^63 */ >> int order, max_order; >> struct resource *b_res = find_free_bus_resource(bus, type); >> unsigned int mem64_mask = 0; >> @@ -819,7 +819,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, >> /* For bridges size != alignment */ >> align = pci_resource_alignment(dev, r); >> order = __ffs(align) - 20; >> - if (order > 11) { >> + if ((sizeof(size_t) == 4 && order > 11) || >> + (sizeof(size_t) == 8 && order > 43)) { >> dev_warn(&dev->dev, "disabling BAR %d: %pR " >> "(bad alignment %#llx)\n", i, r, >> (unsigned long long) align); > > Yinghai, what's your opinion on this? The aligns[] array on the stack > is currently 96 bytes and would grow to 352 with this patch, which > does seem like quite a bit. > > I do think the 2GB limit here is out-of-date. yeah, should be ok. only thing that sanity checking is not complete enough, for example, for bar that only support 32bit, we will have get range for under 4g. that will still need 2g limitation to fend off some bad devices. Thanks Yinghai ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() 2012-07-12 0:13 ` Yinghai Lu @ 2012-07-12 20:34 ` Bjorn Helgaas 0 siblings, 0 replies; 8+ messages in thread From: Bjorn Helgaas @ 2012-07-12 20:34 UTC (permalink / raw) To: Yinghai Lu; +Cc: Nikhil P Rao, linux-pci, linux-kernel, Jesse Barnes On Wed, Jul 11, 2012 at 6:13 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Wed, Jul 11, 2012 at 3:53 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Mon, Jun 25, 2012 at 2:54 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: >>> On Sat, 2012-06-23 at 12:15 -0600, Bjorn Helgaas wrote: >>>> On Thu, Jun 21, 2012 at 5:47 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: >>>> > I ran into the "disabling BAR .." error message when >>>> > trying to use a 8Gb PCIe card on a system with a BIOS >>>> > that didnt have support for BAR size > 2Gb. >>>> >>>> So the BIOS left the 8Gb BAR unassigned, and you got the "disabling >>>> BAR ... (bad alignment)" message when Linux tried to enable it? >>> >>> Yes. >>> >>>> How do we know 8Gb is the correct new limit? Are we going to be >>>> fixing this again when we see a 16Gb or a 32Gb BAR? Do we need a >>>> better algorithm that doesn't have a limit like this? >>>> >>> >>> The original error message seems applicable to 32bit archs. and not to >>> 64 bit archs. How about the patch below - is aligns[44] (256bytes more) >>> acceptable ? >>> >>> >>> From: Nikhil P Rao <nikhil.rao@intel.com> >>> Date: Mon, 25 Jun 2012 13:33:55 -0700 >>> Subject: [PATCH] pci: fix resource size check >>> >>> Support a PCI BAR alignment of > 2Gb, the original check was >>> only applicable to 32 bit kernels, >>> >>> Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> >>> --- >>> drivers/pci/setup-bus.c | 5 +++-- >>> 1 files changed, 3 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c >>> index 8fa2d4b..9f8d9ea 100644 >>> --- a/drivers/pci/setup-bus.c >>> +++ b/drivers/pci/setup-bus.c >>> @@ -780,7 +780,7 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, >>> { >>> struct pci_dev *dev; >>> resource_size_t min_align, align, size, size0, size1; >>> - resource_size_t aligns[12]; /* Alignments from 1Mb to 2Gb */ >>> + resource_size_t aligns[44]; /* Alignments from 1Mb to 2^63 */ >>> int order, max_order; >>> struct resource *b_res = find_free_bus_resource(bus, type); >>> unsigned int mem64_mask = 0; >>> @@ -819,7 +819,8 @@ static int pbus_size_mem(struct pci_bus *bus, unsigned long mask, >>> /* For bridges size != alignment */ >>> align = pci_resource_alignment(dev, r); >>> order = __ffs(align) - 20; >>> - if (order > 11) { >>> + if ((sizeof(size_t) == 4 && order > 11) || >>> + (sizeof(size_t) == 8 && order > 43)) { >>> dev_warn(&dev->dev, "disabling BAR %d: %pR " >>> "(bad alignment %#llx)\n", i, r, >>> (unsigned long long) align); >> >> Yinghai, what's your opinion on this? The aligns[] array on the stack >> is currently 96 bytes and would grow to 352 with this patch, which >> does seem like quite a bit. >> >> I do think the 2GB limit here is out-of-date. > > yeah, should be ok. > > only thing that sanity checking is not complete enough, for example, > for bar that only support 32bit, we will have get range for under 4g. > that will still need 2g limitation to fend off some bad > devices. Can you propose a better patch with more complete sanity checking? I'm not sure I completely understand the point you're making. We call pbus_size_mem() twice: once to collect the size & alignment info for the upstream bridge prefetchable aperture, and again to do it for the non-prefetchable aperture. But I guess we also have to pay attention to whether the aperture must be 32-bit addressable. Is that your point? Do we pay attention to that today? Does Nikhil's patch make anything *worse* in that regard? If his patch is an improvement that merely leaves an existing issue unfixed, I think it's OK to add it. Bjorn ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] pci: 64bit resource fix in setup-res.c 2012-06-20 19:56 [PATCH] pci: 64bit resource fix in setup-res.c Nikhil P Rao 2012-06-21 23:47 ` [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() Nikhil P Rao @ 2012-07-11 23:12 ` Bjorn Helgaas 1 sibling, 0 replies; 8+ messages in thread From: Bjorn Helgaas @ 2012-07-11 23:12 UTC (permalink / raw) To: Nikhil P Rao; +Cc: linux-pci, linux-kernel, Jesse Barnes, Yinghai Lu On Wed, Jun 20, 2012 at 1:56 PM, Nikhil P Rao <nikhil.rao@intel.com> wrote: > size parameter of _pci_assign_resource() needs to be > of type resource_size_t rather than int > > Signed-off-by: Nikhil P Rao <nikhil.rao@intel.com> > --- > drivers/pci/setup-res.c | 3 ++- > 1 files changed, 2 insertions(+), 1 deletions(-) > > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c > index eea85da..9f6bbec 100644 > --- a/drivers/pci/setup-res.c > +++ b/drivers/pci/setup-res.c > @@ -206,7 +206,8 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev, > return ret; > } > > -static int _pci_assign_resource(struct pci_dev *dev, int resno, int size, resource_size_t min_align) > +static int _pci_assign_resource(struct pci_dev *dev, int resno, > + resource_size_t size, resource_size_t min_align) > { > struct resource *res = dev->resource + resno; > struct pci_bus *bus; I applied this, thanks! Sorry it took so long. Bjorn ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2012-07-12 20:34 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-06-20 19:56 [PATCH] pci: 64bit resource fix in setup-res.c Nikhil P Rao 2012-06-21 23:47 ` [PATCH] pci: support alignments upto 8Gb in pbus_size_mem() Nikhil P Rao 2012-06-23 18:15 ` Bjorn Helgaas 2012-06-25 20:54 ` Nikhil P Rao 2012-07-11 22:53 ` Bjorn Helgaas 2012-07-12 0:13 ` Yinghai Lu 2012-07-12 20:34 ` Bjorn Helgaas 2012-07-11 23:12 ` [PATCH] pci: 64bit resource fix in setup-res.c Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).