[PATCH 0/3] PCI endpoint 64-bit BAR fixes

[PATCH 0/3] PCI endpoint 64-bit BAR fixes

[Niklas Cassel]

PCI endpoint fixes to improve the way 64-bit BARs are handled.


There are still future improvements that could be made:

pci-epf-test.c always allocates space for
6 BARs, even when using 64-bit BARs (which
really only requires us to allocate 3 BARs).

pcitest.sh will print "NOT OKAY" for BAR1,
BAR3, and BAR5 when using 64-bit BARs.
This could probably be improved to say
something like "N/A (64-bit BAR)".

Niklas Cassel (3):
  PCI: endpoint: Handle 64-bit BARs properly
  misc: pci_endpoint_test: Handle 64-bit BARs properly
  PCI: designware-ep: Return an error when requesting a too large BAR
    size

 drivers/misc/pci_endpoint_test.c              | 2 ++
 drivers/pci/dwc/pcie-designware-ep.c          | 5 +++++
 drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++
 3 files changed, 9 insertions(+)

-- 
2.14.2

[PATCH 1/3] PCI: endpoint: Handle 64-bit BARs properly

[Niklas Cassel]

A 64-bit BAR uses the succeeding BAR for the upper bits, therefore
we cannot call pci_epc_set_bar() on a BAR that follows a 64-bit BAR.

If pci_epc_set_bar() is called with flag PCI_BASE_ADDRESS_MEM_TYPE_64,
it has to be up to the controller driver to write both BAR[x] and BAR[x+1]
(and BAR_mask[x] and BAR_mask[x+1]).

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index 800da09d9005..eef85820f59e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -382,6 +382,8 @@ static int pci_epf_test_set_bar(struct pci_epf *epf)
 			if (bar == test_reg_bar)
 				return ret;
 		}
+		if (flags & PCI_BASE_ADDRESS_MEM_TYPE_64)
+			bar++;
 	}
 
 	return 0;
-- 
2.14.2

[PATCH 2/3] misc: pci_endpoint_test: Handle 64-bit BARs properly

[Niklas Cassel]

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;
 		base = pci_ioremap_bar(pdev, bar);
 		if (!base) {
 			dev_err(dev, "failed to read BAR%d\n", bar);
-- 
2.14.2

[PATCH 3/3] PCI: designware-ep: Return an error when requesting a too large BAR size

[Niklas Cassel]

pci_epc_set_bar() can be called with flag PCI_BASE_ADDRESS_MEM_TYPE_64,
and can thus request a BAR size larger than 4 GB.

However, the pcie-designware-ep.c driver currently doesn't handle
BAR sizes larger than 4 GB. (Since we are only writing the BAR_mask[x]
register and not the BAR_mask[x+1] register.)

For now, return an error when requesting a BAR size larger than 4 GB.

Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
 drivers/pci/dwc/pcie-designware-ep.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/pci/dwc/pcie-designware-ep.c b/drivers/pci/dwc/pcie-designware-ep.c
index 3a6feeff5f5b..4a0085ead1e3 100644
--- a/drivers/pci/dwc/pcie-designware-ep.c
+++ b/drivers/pci/dwc/pcie-designware-ep.c
@@ -126,6 +126,11 @@ static int dw_pcie_ep_set_bar(struct pci_epc *epc, u8 func_no,
 	enum dw_pcie_as_type as_type;
 	u32 reg = PCI_BASE_ADDRESS_0 + (4 * bar);
 
+	if (size > 0x100000000ULL) {
+		dev_err(pci->dev, "can't handle BAR larger than 4GB\n");
+		return -EINVAL;
+	}
+
 	if (!(flags & PCI_BASE_ADDRESS_SPACE))
 		as_type = DW_PCIE_AS_MEM;
 	else
-- 
2.14.2

Re: [PATCH 3/3] PCI: designware-ep: Return an error when requesting a too large BAR size

[Andy Shevchenko]

On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com> wrote:

include/linux/sizes.h:

+SZ_4G  0x100000000ULL

> +       if (size > 0x100000000ULL) {

#include <linux/sizes.h>

if (size > SZ_4G) {

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] PCI: designware-ep: Return an error when requesting a too large BAR size

[Jingoo Han]

On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote:
> 
> On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com>
> wrote:
> 
> include/linux/sizes.h:
> 
> +SZ_4G  0x100000000ULL
> 
> > +       if (size > 0x100000000ULL) {
> 
> #include <linux/sizes.h>
> 
> if (size > SZ_4G) {

I like this one for the readability.
Thank you.

Best regards,
Jingoo Han

> 
> ?
> 
> --
> With Best Regards,
> Andy Shevchenko

Re: [PATCH 3/3] PCI: designware-ep: Return an error when requesting a too large BAR size

[Niklas Cassel]

On Thu, Feb 01, 2018 at 02:00:40PM -0500, Jingoo Han wrote:
> On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote:
> > 
> > On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com>
> > wrote:
> > 
> > include/linux/sizes.h:
> > 
> > +SZ_4G  0x100000000ULL
> > 
> > > +       if (size > 0x100000000ULL) {
> > 
> > #include <linux/sizes.h>
> > 
> > if (size > SZ_4G) {
> 
> I like this one for the readability.
> Thank you.
> 

I liked it too, however both variants

if (size > 0x100000000ULL) {

if (size > SZ_4G) {

result in:

drivers/pci/dwc/pcie-designware-ep.c:131:11: warning:
  comparison is always false due to limited range of data type [-Wtype-limits]

when compiling with W=1 on a platform with 32-bit size_t.


The annoying thing here is that a BAR can be 64-bit,
yet the parameter size is defined as a size_t,
so the error will only show on 32-bit and not on 64-bit.

What do you think about:

if (upper_32_bits(size)) {
	dev_err(pci->dev, "can't handle BAR larger than 4GB\n");
	return -EINVAL;
}

That should compile without warnings for both
32-bit size_t and 64-bit size_t.


Regards,
Niklas

Re: [PATCH 3/3] PCI: designware-ep: Return an error when requesting a too large BAR size

[Andy Shevchenko]

On Mon, Feb 5, 2018 at 6:25 PM, Niklas Cassel <niklas.cassel@axis.com> wrote:
> On Thu, Feb 01, 2018 at 02:00:40PM -0500, Jingoo Han wrote:
>> On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote:
>> >
>> > On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com>
>> > wrote:
>> >
>> > include/linux/sizes.h:
>> >
>> > +SZ_4G  0x100000000ULL
>> >
>> > > +       if (size > 0x100000000ULL) {
>> >
>> > #include <linux/sizes.h>
>> >
>> > if (size > SZ_4G) {
>>
>> I like this one for the readability.
>> Thank you.
>>
>
> I liked it too, however both variants
>
> if (size > 0x100000000ULL) {
>
> if (size > SZ_4G) {
>
> result in:
>
> drivers/pci/dwc/pcie-designware-ep.c:131:11: warning:
>   comparison is always false due to limited range of data type [-Wtype-limits]
>
> when compiling with W=1 on a platform with 32-bit size_t.
>
>
> The annoying thing here is that a BAR can be 64-bit,
> yet the parameter size is defined as a size_t,
> so the error will only show on 32-bit and not on 64-bit.

Oh, indeed. And it looks moving to u64 or alike is not a solution
(because if would not describe real hardware in that case).

> What do you think about:

> if (upper_32_bits(size)) {
>         dev_err(pci->dev, "can't handle BAR larger than 4GB\n");
>         return -EINVAL;
> }
>
> That should compile without warnings for both
> 32-bit size_t and 64-bit size_t.

Can you derive some helper based on the code in __pci_read_base() code?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 3/3] PCI: designware-ep: Return an error when requesting a too large BAR size

[Niklas Cassel]

On Tue, Feb 06, 2018 at 09:38:09PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 5, 2018 at 6:25 PM, Niklas Cassel <niklas.cassel@axis.com> wrote:
> > On Thu, Feb 01, 2018 at 02:00:40PM -0500, Jingoo Han wrote:
> >> On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote:
> >> >
> >> > On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <niklas.cassel@axis.com>
> >> > wrote:
> >> >
> >> > include/linux/sizes.h:
> >> >
> >> > +SZ_4G  0x100000000ULL
> >> >
> >> > > +       if (size > 0x100000000ULL) {
> >> >
> >> > #include <linux/sizes.h>
> >> >
> >> > if (size > SZ_4G) {
> >>
> >> I like this one for the readability.
> >> Thank you.
> >>
> >
> > I liked it too, however both variants
> >
> > if (size > 0x100000000ULL) {
> >
> > if (size > SZ_4G) {
> >
> > result in:
> >
> > drivers/pci/dwc/pcie-designware-ep.c:131:11: warning:
> >   comparison is always false due to limited range of data type [-Wtype-limits]
> >
> > when compiling with W=1 on a platform with 32-bit size_t.
> >
> >
> > The annoying thing here is that a BAR can be 64-bit,
> > yet the parameter size is defined as a size_t,
> > so the error will only show on 32-bit and not on 64-bit.
> 
> Oh, indeed. And it looks moving to u64 or alike is not a solution
> (because if would not describe real hardware in that case).
> 
> > What do you think about:
> 
> > if (upper_32_bits(size)) {
> >         dev_err(pci->dev, "can't handle BAR larger than 4GB\n");
> >         return -EINVAL;
> > }
> >
> > That should compile without warnings for both
> > 32-bit size_t and 64-bit size_t.
> 
> Can you derive some helper based on the code in __pci_read_base() code?

Hello Andy,

I guess that would be possible, however I think that
simply checking upper_32_bits(size) is simpler.

If someone is ever to fix dw_pcie_ep_set_bar()
so that it works with 64-bit BARs, the function will
probably need to check upper_32_bits(size) anyway.


Regards,
Niklas