| Message ID | 20180208123346.20784-3-niklas.cassel@axis.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | PCI endpoint 64-bit BAR fixes | expand |
On Thu, Feb 08, 2018 at 01:33:45PM +0100, Niklas Cassel wrote: > A 64-bit BAR uses the succeeding BAR for the upper bits, > so we cannot simply call pci_ioremap_bar() on every single BAR. > > Ignore BARs that does not have a valid resource length. > > pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit] > pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit] > pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit] > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0] > pci-endpoint-test 0000:01:00.0: failed to read BAR1 > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0] > pci-endpoint-test 0000:01:00.0: failed to read BAR3 > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0] > pci-endpoint-test 0000:01:00.0: failed to read BAR5 > > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > --- > Lorenzo/Bjorn: pci_resource_len() seems to fix my problem, > but is it the correct function to use here? > If BAR[x] is a 64-bit BAR, I'm assuming that pci_resource_len() on BAR[x+1] > will always return 0 (since BAR[x+1] cannot have any prefetchable/type bits > when BAR[x] is 64-bit). > > drivers/misc/pci_endpoint_test.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > index 320276f42653..3af31bfdcfdd 100644 > --- a/drivers/misc/pci_endpoint_test.c > +++ b/drivers/misc/pci_endpoint_test.c > @@ -534,6 +534,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > } > > for (bar = BAR_0; bar <= BAR_5; bar++) { > + if (pci_resource_len(pdev, bar) == 0) > + continue; Should not it be handled by checking the resource flags as you loop through the bar counter and incrementing the bar counter (+1) if IORESOURCE_MEM_64 is detected ? I would like to get Bjorn's point of view on this since it is definitely more comprehensive than mine. Thanks, Lorenzo > base = pci_ioremap_bar(pdev, bar); > if (!base) { > dev_err(dev, "failed to read BAR%d\n", bar); > -- > 2.14.2 >
On Mon, Feb 26, 2018 at 05:26:18PM +0000, Lorenzo Pieralisi wrote: > On Thu, Feb 08, 2018 at 01:33:45PM +0100, Niklas Cassel wrote: > > A 64-bit BAR uses the succeeding BAR for the upper bits, > > so we cannot simply call pci_ioremap_bar() on every single BAR. > > > > Ignore BARs that does not have a valid resource length. > > > > pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit] > > pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit] > > pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit] > > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0] > > pci-endpoint-test 0000:01:00.0: failed to read BAR1 > > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0] > > pci-endpoint-test 0000:01:00.0: failed to read BAR3 > > pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0] > > pci-endpoint-test 0000:01:00.0: failed to read BAR5 > > > > Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> > > --- > > Lorenzo/Bjorn: pci_resource_len() seems to fix my problem, > > but is it the correct function to use here? > > If BAR[x] is a 64-bit BAR, I'm assuming that pci_resource_len() on BAR[x+1] > > will always return 0 (since BAR[x+1] cannot have any prefetchable/type bits > > when BAR[x] is 64-bit). > > > > drivers/misc/pci_endpoint_test.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c > > index 320276f42653..3af31bfdcfdd 100644 > > --- a/drivers/misc/pci_endpoint_test.c > > +++ b/drivers/misc/pci_endpoint_test.c > > @@ -534,6 +534,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, > > } > > > > for (bar = BAR_0; bar <= BAR_5; bar++) { > > + if (pci_resource_len(pdev, bar) == 0) > > + continue; > > Should not it be handled by checking the resource flags as you loop > through the bar counter and incrementing the bar counter (+1) if > IORESOURCE_MEM_64 is detected ? I agree, pci_resource_len() is the wrong thing here. The length (actually the entire resource[x]) *should* be zero if the slot corresponds to the upper bits of a 64-bit BAR, but I think it would be more natural to do this: if (pci_resource_flags(pdev, bar) & IORESOURCE_MEM) base = pci_ioremap_bar(pdev, bar); You *could* check for IORESOURCE_MEM_64 and increment "bar" if you find it, but I don't think that's really idiomatic, and it builds in a little bit of unnecessary knowledge about how the PCI core maps BAR registers to the resource[] array. > > base = pci_ioremap_bar(pdev, bar); > > if (!base) { > > dev_err(dev, "failed to read BAR%d\n", bar); > > -- > > 2.14.2 > >
diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c index 320276f42653..3af31bfdcfdd 100644 --- a/drivers/misc/pci_endpoint_test.c +++ b/drivers/misc/pci_endpoint_test.c @@ -534,6 +534,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev, } for (bar = BAR_0; bar <= BAR_5; bar++) { + if (pci_resource_len(pdev, bar) == 0) + continue; base = pci_ioremap_bar(pdev, bar); if (!base) { dev_err(dev, "failed to read BAR%d\n", bar);
A 64-bit BAR uses the succeeding BAR for the upper bits, so we cannot simply call pci_ioremap_bar() on every single BAR. Ignore BARs that does not have a valid resource length. pci 0000:01:00.0: BAR 4: assigned [mem 0xc0300000-0xc031ffff 64bit] pci 0000:01:00.0: BAR 2: assigned [mem 0xc0320000-0xc03203ff 64bit] pci 0000:01:00.0: BAR 0: assigned [mem 0xc0320400-0xc03204ff 64bit] pci-endpoint-test 0000:01:00.0: can't ioremap BAR 1: [??? 0x00000000 flags 0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR1 pci-endpoint-test 0000:01:00.0: can't ioremap BAR 3: [??? 0x00000000 flags 0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR3 pci-endpoint-test 0000:01:00.0: can't ioremap BAR 5: [??? 0x00000000 flags 0x0] pci-endpoint-test 0000:01:00.0: failed to read BAR5 Signed-off-by: Niklas Cassel <niklas.cassel@axis.com> --- Lorenzo/Bjorn: pci_resource_len() seems to fix my problem, but is it the correct function to use here? If BAR[x] is a 64-bit BAR, I'm assuming that pci_resource_len() on BAR[x+1] will always return 0 (since BAR[x+1] cannot have any prefetchable/type bits when BAR[x] is 64-bit). drivers/misc/pci_endpoint_test.c | 2 ++ 1 file changed, 2 insertions(+)