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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On Thursday, February 1, 2018 1:58 PM, Andy Shevchenko wrote:
>
> On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <[email protected]>
> 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
On Thu, Feb 1, 2018 at 6:11 PM, Niklas Cassel <[email protected]> wrote:
include/linux/sizes.h:
+SZ_4G 0x100000000ULL
> + if (size > 0x100000000ULL) {
#include <linux/sizes.h>
if (size > SZ_4G) {
?
--
With Best Regards,
Andy Shevchenko
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 <[email protected]>
> > 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
On Mon, Feb 5, 2018 at 6:25 PM, Niklas Cassel <[email protected]> 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 <[email protected]>
>> > 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
On Tue, Feb 06, 2018 at 09:38:09PM +0200, Andy Shevchenko wrote:
> On Mon, Feb 5, 2018 at 6:25 PM, Niklas Cassel <[email protected]> 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 <[email protected]>
> >> > 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