drivers/pci/endpoint/functions/pci-epf-test.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
Currently, the test allocates BAR sizes according to fixed table
bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 } . This
does not work with controllers which have fixed size BARs, like
Renesas R-Car V4H PCIe controller, which has BAR4 size limited
to 256 Bytes, which is much less than 131072 currently requested
by this test.
Adjust the test such, that in case a fixed size BAR is detected
on a controller, minimum of requested size and fixed size BAR
size is used during the test instead.
This helps with test failures reported as follows:
"
pci_epf_test pci_epf_test.0: requested BAR size is larger than fixed size
pci_epf_test pci_epf_test.0: Failed to allocate space for BAR4
"
Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
---
Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Damien Le Moal <dlemoal@kernel.org>
Cc: Frank Li <Frank.Li@nxp.com>
Cc: Kishon Vijay Abraham I <kishon@kernel.org>
Cc: Manivannan Sadhasivam <mani@kernel.org>
Cc: Niklas Cassel <cassel@kernel.org>
Cc: Wang Jiang <jiangwang@kylinos.cn>
Cc: linux-kernel@vger.kernel.org
Cc: linux-pci@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org
---
drivers/pci/endpoint/functions/pci-epf-test.c | 11 +++++++++--
1 file changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index e091193bd8a8a..d9c950d4c9a9e 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -1022,7 +1022,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
enum pci_barno test_reg_bar = epf_test->test_reg_bar;
enum pci_barno bar;
const struct pci_epc_features *epc_features = epf_test->epc_features;
- size_t test_reg_size;
+ size_t test_reg_size, test_bar_size;
+ u64 bar_fixed_size;
test_reg_bar_size = ALIGN(sizeof(struct pci_epf_test_reg), 128);
@@ -1050,7 +1051,13 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
if (bar == test_reg_bar)
continue;
- base = pci_epf_alloc_space(epf, bar_size[bar], bar,
+ test_bar_size = bar_size[bar];
+
+ bar_fixed_size = epc_features->bar[bar].fixed_size;
+ if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size)
+ test_bar_size = min(bar_size[bar], bar_fixed_size);
+
+ base = pci_epf_alloc_space(epf, test_bar_size, bar,
epc_features, PRIMARY_INTERFACE);
if (!base)
dev_err(dev, "Failed to allocate space for BAR%d\n",
--
2.50.1
On 9/4/25 11:37 AM, Marek Vasut wrote:
> Currently, the test allocates BAR sizes according to fixed table
> bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 } . This
> does not work with controllers which have fixed size BARs, like
> Renesas R-Car V4H PCIe controller, which has BAR4 size limited
> to 256 Bytes, which is much less than 131072 currently requested
> by this test.
>
> Adjust the test such, that in case a fixed size BAR is detected
> on a controller, minimum of requested size and fixed size BAR
> size is used during the test instead.
>
> This helps with test failures reported as follows:
> "
> pci_epf_test pci_epf_test.0: requested BAR size is larger than fixed size
> pci_epf_test pci_epf_test.0: Failed to allocate space for BAR4
> "
>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> ---
> Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
> Cc: Bjorn Helgaas <bhelgaas@google.com>
> Cc: Damien Le Moal <dlemoal@kernel.org>
> Cc: Frank Li <Frank.Li@nxp.com>
> Cc: Kishon Vijay Abraham I <kishon@kernel.org>
> Cc: Manivannan Sadhasivam <mani@kernel.org>
> Cc: Niklas Cassel <cassel@kernel.org>
> Cc: Wang Jiang <jiangwang@kylinos.cn>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-pci@vger.kernel.org
> Cc: linux-renesas-soc@vger.kernel.org
> ---
> drivers/pci/endpoint/functions/pci-epf-test.c | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> index e091193bd8a8a..d9c950d4c9a9e 100644
> --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> @@ -1022,7 +1022,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> enum pci_barno bar;
> const struct pci_epc_features *epc_features = epf_test->epc_features;
> - size_t test_reg_size;
> + size_t test_reg_size, test_bar_size;
> + u64 bar_fixed_size;
>
> test_reg_bar_size = ALIGN(sizeof(struct pci_epf_test_reg), 128);
>
> @@ -1050,7 +1051,13 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> if (bar == test_reg_bar)
> continue;
>
> - base = pci_epf_alloc_space(epf, bar_size[bar], bar,
> + test_bar_size = bar_size[bar];
> +
> + bar_fixed_size = epc_features->bar[bar].fixed_size;
> + if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size)
> + test_bar_size = min(bar_size[bar], bar_fixed_size);
I think this can be simplified to:
if (epc_features->bar[bar].type == BAR_FIXED)
test_bar_size = epc_features->bar[bar].fixed_size;
else
test_bar_size = bar_size[bar];
because if the bar type is BAR_FIXED, then the size of the bar can only be its
fixed size.
> +
> + base = pci_epf_alloc_space(epf, test_bar_size, bar,
> epc_features, PRIMARY_INTERFACE);
> if (!base)
> dev_err(dev, "Failed to allocate space for BAR%d\n",
--
Damien Le Moal
Western Digital Research
On Thu, Sep 04, 2025 at 11:40:15AM +0900, Damien Le Moal wrote:
> On 9/4/25 11:37 AM, Marek Vasut wrote:
> > Currently, the test allocates BAR sizes according to fixed table
> > bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 } . This
> > does not work with controllers which have fixed size BARs, like
> > Renesas R-Car V4H PCIe controller, which has BAR4 size limited
> > to 256 Bytes, which is much less than 131072 currently requested
> > by this test.
> >
> > Adjust the test such, that in case a fixed size BAR is detected
> > on a controller, minimum of requested size and fixed size BAR
> > size is used during the test instead.
> >
> > This helps with test failures reported as follows:
> > "
> > pci_epf_test pci_epf_test.0: requested BAR size is larger than fixed size
> > pci_epf_test pci_epf_test.0: Failed to allocate space for BAR4
> > "
> >
> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
> > ---
> > Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
> > Cc: Bjorn Helgaas <bhelgaas@google.com>
> > Cc: Damien Le Moal <dlemoal@kernel.org>
> > Cc: Frank Li <Frank.Li@nxp.com>
> > Cc: Kishon Vijay Abraham I <kishon@kernel.org>
> > Cc: Manivannan Sadhasivam <mani@kernel.org>
> > Cc: Niklas Cassel <cassel@kernel.org>
> > Cc: Wang Jiang <jiangwang@kylinos.cn>
> > Cc: linux-kernel@vger.kernel.org
> > Cc: linux-pci@vger.kernel.org
> > Cc: linux-renesas-soc@vger.kernel.org
> > ---
> > drivers/pci/endpoint/functions/pci-epf-test.c | 11 +++++++++--
> > 1 file changed, 9 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
> > index e091193bd8a8a..d9c950d4c9a9e 100644
> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
> > @@ -1022,7 +1022,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> > enum pci_barno test_reg_bar = epf_test->test_reg_bar;
> > enum pci_barno bar;
> > const struct pci_epc_features *epc_features = epf_test->epc_features;
> > - size_t test_reg_size;
> > + size_t test_reg_size, test_bar_size;
> > + u64 bar_fixed_size;
> >
> > test_reg_bar_size = ALIGN(sizeof(struct pci_epf_test_reg), 128);
> >
> > @@ -1050,7 +1051,13 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
> > if (bar == test_reg_bar)
> > continue;
> >
> > - base = pci_epf_alloc_space(epf, bar_size[bar], bar,
> > + test_bar_size = bar_size[bar];
> > +
> > + bar_fixed_size = epc_features->bar[bar].fixed_size;
> > + if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size)
> > + test_bar_size = min(bar_size[bar], bar_fixed_size);
>
> I think this can be simplified to:
>
> if (epc_features->bar[bar].type == BAR_FIXED)
> test_bar_size = epc_features->bar[bar].fixed_size;
> else
> test_bar_size = bar_size[bar];
+1
>
> because if the bar type is BAR_FIXED, then the size of the bar can only be its
> fixed size.
Correct, see:
f015b53d634a ("PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar()")
Actually, Jerome Brunet was also using this weird Renesas R-Car V4H PCIe
controller where BAR4 is a really small fixed-size BAR.
(Even smaller than the iATU minimum alignment requirement for that same
controller.)
See:
793908d60b87 ("PCI: endpoint: Retain fixed-size BAR size as well as aligned size")
But he only appears to have used the vntb epf driver.
Jerome, I suppose that you never ran with the pci-epf-test driver?
pci_epf_alloc_space() works like this:
If the user requests a BAR size that is smaller than the fixed-size BAR,
it will allocate space matching the fixed-size.
As in most cases, having a BAR larger than needed by an EPF driver is
still acceptable.
However, if the user requests a size larger than the fixed-size BAR,
as in your case, we will return an error, as we cannot fulfill the
user's request.
I don't see any alternative other than your/Damien's proposal above.
Unfortunately, all EPF drivers would probably need this same change.
Kind regards,
Niklas
On Thu 04 Sep 2025 at 14:28, Niklas Cassel <cassel@kernel.org> wrote:
> On Thu, Sep 04, 2025 at 11:40:15AM +0900, Damien Le Moal wrote:
>> On 9/4/25 11:37 AM, Marek Vasut wrote:
>> > Currently, the test allocates BAR sizes according to fixed table
>> > bar_size[] = { 512, 512, 1024, 16384, 131072, 1048576 } . This
>> > does not work with controllers which have fixed size BARs, like
>> > Renesas R-Car V4H PCIe controller, which has BAR4 size limited
>> > to 256 Bytes, which is much less than 131072 currently requested
>> > by this test.
>> >
>> > Adjust the test such, that in case a fixed size BAR is detected
>> > on a controller, minimum of requested size and fixed size BAR
>> > size is used during the test instead.
>> >
>> > This helps with test failures reported as follows:
>> > "
>> > pci_epf_test pci_epf_test.0: requested BAR size is larger than fixed size
>> > pci_epf_test pci_epf_test.0: Failed to allocate space for BAR4
>> > "
>> >
>> > Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>
>> > ---
>> > Cc: "Krzysztof Wilczyński" <kwilczynski@kernel.org>
>> > Cc: Bjorn Helgaas <bhelgaas@google.com>
>> > Cc: Damien Le Moal <dlemoal@kernel.org>
>> > Cc: Frank Li <Frank.Li@nxp.com>
>> > Cc: Kishon Vijay Abraham I <kishon@kernel.org>
>> > Cc: Manivannan Sadhasivam <mani@kernel.org>
>> > Cc: Niklas Cassel <cassel@kernel.org>
>> > Cc: Wang Jiang <jiangwang@kylinos.cn>
>> > Cc: linux-kernel@vger.kernel.org
>> > Cc: linux-pci@vger.kernel.org
>> > Cc: linux-renesas-soc@vger.kernel.org
>> > ---
>> > drivers/pci/endpoint/functions/pci-epf-test.c | 11 +++++++++--
>> > 1 file changed, 9 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
>> > index e091193bd8a8a..d9c950d4c9a9e 100644
>> > --- a/drivers/pci/endpoint/functions/pci-epf-test.c
>> > +++ b/drivers/pci/endpoint/functions/pci-epf-test.c
>> > @@ -1022,7 +1022,8 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>> > enum pci_barno test_reg_bar = epf_test->test_reg_bar;
>> > enum pci_barno bar;
>> > const struct pci_epc_features *epc_features = epf_test->epc_features;
>> > - size_t test_reg_size;
>> > + size_t test_reg_size, test_bar_size;
>> > + u64 bar_fixed_size;
>> >
>> > test_reg_bar_size = ALIGN(sizeof(struct pci_epf_test_reg), 128);
>> >
>> > @@ -1050,7 +1051,13 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf)
>> > if (bar == test_reg_bar)
>> > continue;
>> >
>> > - base = pci_epf_alloc_space(epf, bar_size[bar], bar,
>> > + test_bar_size = bar_size[bar];
>> > +
>> > + bar_fixed_size = epc_features->bar[bar].fixed_size;
>> > + if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size)
>> > + test_bar_size = min(bar_size[bar], bar_fixed_size);
>>
>> I think this can be simplified to:
>>
>> if (epc_features->bar[bar].type == BAR_FIXED)
>> test_bar_size = epc_features->bar[bar].fixed_size;
>> else
>> test_bar_size = bar_size[bar];
>
> +1
It's what pci_epf_alloc_space() does too. so it makes sense but it also
means the side must stay aligned.
If a rework is needed, maybe it would be better to get size from
pci_epf_alloc_space() instead of recomputing it ?
>
>>
>> because if the bar type is BAR_FIXED, then the size of the bar can only be its
>> fixed size.
>
> Correct, see:
> f015b53d634a ("PCI: endpoint: Add size check for fixed size BARs in pci_epc_set_bar()")
>
> Actually, Jerome Brunet was also using this weird Renesas R-Car V4H PCIe
> controller where BAR4 is a really small fixed-size BAR.
>
> (Even smaller than the iATU minimum alignment requirement for that same
> controller.)
>
> See:
> 793908d60b87 ("PCI: endpoint: Retain fixed-size BAR size as well as aligned size")
>
> But he only appears to have used the vntb epf driver.
>
> Jerome, I suppose that you never ran with the pci-epf-test driver?
>
Indeed no. I've gone with with ntb-test driver on top or ntb-netdev
>
> pci_epf_alloc_space() works like this:
> If the user requests a BAR size that is smaller than the fixed-size BAR,
> it will allocate space matching the fixed-size.
>
> As in most cases, having a BAR larger than needed by an EPF driver is
> still acceptable.
>
> However, if the user requests a size larger than the fixed-size BAR,
> as in your case, we will return an error, as we cannot fulfill the
> user's request.
>
> I don't see any alternative other than your/Damien's proposal above.
>
> Unfortunately, all EPF drivers would probably need this same change.
>
>
> Kind regards,
> Niklas
--
Jerome
On Fri, Sep 05, 2025 at 09:32:03AM +0200, Jerome Brunet wrote: > On Thu 04 Sep 2025 at 14:28, Niklas Cassel <cassel@kernel.org> wrote: > >> > >> I think this can be simplified to: > >> > >> if (epc_features->bar[bar].type == BAR_FIXED) > >> test_bar_size = epc_features->bar[bar].fixed_size; > >> else > >> test_bar_size = bar_size[bar]; > > > > +1 > > It's what pci_epf_alloc_space() does too. so it makes sense but it also > means the side must stay aligned. Not really, pci_epf_alloc_space() will give you 'fixed_size' if you request size < fixed_size. If you request more, it will give you an error. > > If a rework is needed, maybe it would be better to get size from > pci_epf_alloc_space() instead of recomputing it ? The pci-epf-test driver is just a test driver and we can use whatever BAR size we want for each BAR. However, I don't think that pci_epf_alloc_space() can always give us a BAR size. Sure, for fixed_size BARs, there is only a single size that is possible. But for Programmable and Resizable BARs, there are many possible sizes, so which size should pci_epf_alloc_space() then return? And not all EPF drivers might be happy with an aribitrary BAR size (which is the case for pci-epf-test), some EPF drivers might have strict minimum sizes for a BAR. So, I still think this proposal is the best thing we can do. At least it appears that we only need to patch pci-epf-test. Kind regards, Niklas
On 9/5/25 10:36 AM, Niklas Cassel wrote: > On Fri, Sep 05, 2025 at 09:32:03AM +0200, Jerome Brunet wrote: >> On Thu 04 Sep 2025 at 14:28, Niklas Cassel <cassel@kernel.org> wrote: >>>> >>>> I think this can be simplified to: >>>> >>>> if (epc_features->bar[bar].type == BAR_FIXED) >>>> test_bar_size = epc_features->bar[bar].fixed_size; >>>> else >>>> test_bar_size = bar_size[bar]; >>> >>> +1 >> >> It's what pci_epf_alloc_space() does too. so it makes sense but it also >> means the side must stay aligned. > > Not really, pci_epf_alloc_space() will give you 'fixed_size' > if you request size < fixed_size. > > If you request more, it will give you an error. > >> >> If a rework is needed, maybe it would be better to get size from >> pci_epf_alloc_space() instead of recomputing it ? > > The pci-epf-test driver is just a test driver and we can use whatever > BAR size we want for each BAR. > > However, I don't think that pci_epf_alloc_space() can always give us > a BAR size. Sure, for fixed_size BARs, there is only a single size > that is possible. But for Programmable and Resizable BARs, there are > many possible sizes, so which size should pci_epf_alloc_space() then > return? > > And not all EPF drivers might be happy with an aribitrary BAR size > (which is the case for pci-epf-test), some EPF drivers might have > strict minimum sizes for a BAR. > > So, I still think this proposal is the best thing we can do. > > At least it appears that we only need to patch pci-epf-test. In the meantime, I sent a tested V2, so it is on the list. Thanks !
On 9/4/25 2:28 PM, Niklas Cassel wrote: Hello Niklas, [...] > pci_epf_alloc_space() works like this: > If the user requests a BAR size that is smaller than the fixed-size BAR, > it will allocate space matching the fixed-size. > > As in most cases, having a BAR larger than needed by an EPF driver is > still acceptable. > > However, if the user requests a size larger than the fixed-size BAR, > as in your case, we will return an error, as we cannot fulfill the > user's request. > > I don't see any alternative other than your/Damien's proposal above. > > Unfortunately, all EPF drivers would probably need this same change. It seems that pci-epf-ntb and pci-epf-vntb only use BAR0 (BAR_CONFIG) and BAR0+BAR1 (BAR_CONFIG and BAR_DB) , so those should be OK on this controller. NVMe EPF also seems to use only BAR0 and it specifically handles fixed size BAR. It seems everything that is in the tree so far managed to sidestep hitting fixed-size BAR4 problems on this hardware, except for the test driver.
On Thu 04 Sep 2025 at 23:29, Marek Vasut <marek.vasut@mailbox.org> wrote: > On 9/4/25 2:28 PM, Niklas Cassel wrote: > > Hello Niklas, > > [...] > >> pci_epf_alloc_space() works like this: >> If the user requests a BAR size that is smaller than the fixed-size BAR, >> it will allocate space matching the fixed-size. >> As in most cases, having a BAR larger than needed by an EPF driver is >> still acceptable. >> However, if the user requests a size larger than the fixed-size BAR, >> as in your case, we will return an error, as we cannot fulfill the >> user's request. >> I don't see any alternative other than your/Damien's proposal above. >> Unfortunately, all EPF drivers would probably need this same change. > > It seems that pci-epf-ntb and pci-epf-vntb only use BAR0 (BAR_CONFIG) and > BAR0+BAR1 (BAR_CONFIG and BAR_DB) , so those should be OK on this > controller. NVMe EPF also seems to use only BAR0 and it specifically > handles fixed size BAR. It seems everything that is in the tree so far > managed to sidestep hitting fixed-size BAR4 problems on this hardware, > except for the test driver. As it stands, a vNTB device needs 3 BARs minimum (CFG, DB and MW). The NTB one may get away with with 2 BARs, with DB and MW sharing one. If you referring to Renesas about that BAR4, I did use it for vNTB. It is indeed not upstream ... yet [1] I think it is possible to have vNTB on 2 BARs with some tweaks, putting CFG and DB on the same one. [1]: https://lore.kernel.org/r/20250702-ntb-rcar-support-v3-2-4268d9c85eb7@baylibre.com -- Jerome
On 9/5/25 9:43 AM, Jerome Brunet wrote: Hello Jerome, >>> pci_epf_alloc_space() works like this: >>> If the user requests a BAR size that is smaller than the fixed-size BAR, >>> it will allocate space matching the fixed-size. >>> As in most cases, having a BAR larger than needed by an EPF driver is >>> still acceptable. >>> However, if the user requests a size larger than the fixed-size BAR, >>> as in your case, we will return an error, as we cannot fulfill the >>> user's request. >>> I don't see any alternative other than your/Damien's proposal above. >>> Unfortunately, all EPF drivers would probably need this same change. >> >> It seems that pci-epf-ntb and pci-epf-vntb only use BAR0 (BAR_CONFIG) and >> BAR0+BAR1 (BAR_CONFIG and BAR_DB) , so those should be OK on this >> controller. NVMe EPF also seems to use only BAR0 and it specifically >> handles fixed size BAR. It seems everything that is in the tree so far >> managed to sidestep hitting fixed-size BAR4 problems on this hardware, >> except for the test driver. > > As it stands, a vNTB device needs 3 BARs minimum (CFG, DB and MW). The > NTB one may get away with with 2 BARs, with DB and MW sharing one. I clearly missed the MW, thanks for pointing this out. > If you referring to Renesas about that BAR4, I did use it for vNTB. > It is indeed not upstream ... yet [1] > > I think it is possible to have vNTB on 2 BARs with some tweaks, putting > CFG and DB on the same one. > > [1]: https://lore.kernel.org/r/20250702-ntb-rcar-support-v3-2-4268d9c85eb7@baylibre.com Nitpick, commit message, "Renesas R-Car Gen4" (Gen3 has a different PCIe controller) . Please CC linux-renesas-soc@vger.kernel.org on V3 , thank you ! -- Best regards, Marek Vasut
On 9/4/25 4:40 AM, Damien Le Moal wrote: Hello Damien, >> @@ -1050,7 +1051,13 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf) >> if (bar == test_reg_bar) >> continue; >> >> - base = pci_epf_alloc_space(epf, bar_size[bar], bar, >> + test_bar_size = bar_size[bar]; >> + >> + bar_fixed_size = epc_features->bar[bar].fixed_size; >> + if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) >> + test_bar_size = min(bar_size[bar], bar_fixed_size); > > I think this can be simplified to: > > if (epc_features->bar[bar].type == BAR_FIXED) > test_bar_size = epc_features->bar[bar].fixed_size; > else > test_bar_size = bar_size[bar]; > > because if the bar type is BAR_FIXED, then the size of the bar can only be its > fixed size. That is correct, however, please consider the following case: - The BAR under test is BAR4 , therefore the size requested by this driver is bar_size[4] = 131072 Bytes - The BAR4 on a hypothetical hardware is a fixed size BAR , 262144 Bytes large With your proposed change, the "test_bar_size" would end up being 262144 Bytes , instead of 131072 Bytes without your proposed change , which I think is not the desired behavior. What do you think ?
On 9/4/25 12:32 PM, Marek Vasut wrote: > On 9/4/25 4:40 AM, Damien Le Moal wrote: > > Hello Damien, > >>> @@ -1050,7 +1051,13 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf) >>> if (bar == test_reg_bar) >>> continue; >>> - base = pci_epf_alloc_space(epf, bar_size[bar], bar, >>> + test_bar_size = bar_size[bar]; >>> + >>> + bar_fixed_size = epc_features->bar[bar].fixed_size; >>> + if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) >>> + test_bar_size = min(bar_size[bar], bar_fixed_size); >> >> I think this can be simplified to: >> >> if (epc_features->bar[bar].type == BAR_FIXED) >> test_bar_size = epc_features->bar[bar].fixed_size; >> else >> test_bar_size = bar_size[bar]; >> >> because if the bar type is BAR_FIXED, then the size of the bar can only be its >> fixed size. > That is correct, however, please consider the following case: > > - The BAR under test is BAR4 , therefore the size requested by this driver is > bar_size[4] = 131072 Bytes > - The BAR4 on a hypothetical hardware is a fixed size BAR , 262144 Bytes large > > With your proposed change, the "test_bar_size" would end up being 262144 > Bytes , instead of 131072 Bytes without your proposed change , which I think is > not the desired behavior. > > What do you think ? The bar size for the test is arbitrary. If the bar being tested is not a fixed bar, anything is OK. But in the case of a fixed bar, you can only use the fixed bar size so we should force that. -- Damien Le Moal Western Digital Research
On 9/4/25 5:39 AM, Damien Le Moal wrote: > On 9/4/25 12:32 PM, Marek Vasut wrote: >> On 9/4/25 4:40 AM, Damien Le Moal wrote: >> >> Hello Damien, >> >>>> @@ -1050,7 +1051,13 @@ static int pci_epf_test_alloc_space(struct pci_epf *epf) >>>> if (bar == test_reg_bar) >>>> continue; >>>> - base = pci_epf_alloc_space(epf, bar_size[bar], bar, >>>> + test_bar_size = bar_size[bar]; >>>> + >>>> + bar_fixed_size = epc_features->bar[bar].fixed_size; >>>> + if (epc_features->bar[bar].type == BAR_FIXED && bar_fixed_size) >>>> + test_bar_size = min(bar_size[bar], bar_fixed_size); >>> >>> I think this can be simplified to: >>> >>> if (epc_features->bar[bar].type == BAR_FIXED) >>> test_bar_size = epc_features->bar[bar].fixed_size; >>> else >>> test_bar_size = bar_size[bar]; >>> >>> because if the bar type is BAR_FIXED, then the size of the bar can only be its >>> fixed size. >> That is correct, however, please consider the following case: >> >> - The BAR under test is BAR4 , therefore the size requested by this driver is >> bar_size[4] = 131072 Bytes >> - The BAR4 on a hypothetical hardware is a fixed size BAR , 262144 Bytes large >> >> With your proposed change, the "test_bar_size" would end up being 262144 >> Bytes , instead of 131072 Bytes without your proposed change , which I think is >> not the desired behavior. >> >> What do you think ? > > The bar size for the test is arbitrary. If the bar being tested is not a fixed > bar, anything is OK. But in the case of a fixed bar, you can only use the fixed > bar size so we should force that. OK, understood. I'll run tests on V2 and then submit a V2. Thanks !
© 2016 - 2025 Red Hat, Inc.