When trying to allocate space for an endpoint function on a BAR with a
fixed size, the size saved in the 'struct pci_epf_bar' should be the fixed
size. This is expected by pci_epc_set_bar().
However, if the fixed_size is smaller that the alignment, the size saved
in the 'struct pci_epf_bar' matches the alignment and it is a problem for
pci_epc_set_bar().
To solve this, continue to allocate space that match the iATU alignment
requirement but save the size that matches what is present in the BAR.
Fixes: 2a9a801620ef ("PCI: endpoint: Add support to specify alignment for buffers allocated to BARs")
Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
drivers/pci/endpoint/pci-epf-core.c | 25 +++++++++++++++++--------
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
index b7deb0ee1760b23a24f49abf3baf53ea2f273476..fb902b751e1c965c902c5199d57969ae0a757c2e 100644
--- a/drivers/pci/endpoint/pci-epf-core.c
+++ b/drivers/pci/endpoint/pci-epf-core.c
@@ -225,6 +225,7 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
struct device *dev;
struct pci_epf_bar *epf_bar;
struct pci_epc *epc;
+ size_t size;
if (!addr)
return;
@@ -237,9 +238,12 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
epf_bar = epf->sec_epc_bar;
}
+ size = epf_bar[bar].size;
+ if (epc_features->align)
+ size = ALIGN(size, epc_features->align);
+
dev = epc->dev.parent;
- dma_free_coherent(dev, epf_bar[bar].size, addr,
- epf_bar[bar].phys_addr);
+ dma_free_coherent(dev, size, addr, epf_bar[bar].phys_addr);
epf_bar[bar].phys_addr = 0;
epf_bar[bar].addr = NULL;
@@ -266,7 +270,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
enum pci_epc_interface_type type)
{
u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
- size_t align = epc_features->align;
+ size_t aligned_size, align = epc_features->align;
struct pci_epf_bar *epf_bar;
dma_addr_t phys_addr;
struct pci_epc *epc;
@@ -287,12 +291,17 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
return NULL;
}
size = bar_fixed_size;
+ } else {
+ /* BAR size must be power of two */
+ size = roundup_pow_of_two(size);
}
- if (align)
- size = ALIGN(size, align);
- else
- size = roundup_pow_of_two(size);
+ /*
+ * Allocate enough memory to accommodate the iATU alignment requirement.
+ * In most cases, this will be the same as .size but it might be different
+ * if, for example, the fixed size of a BAR is smaller than align
+ */
+ aligned_size = align ? ALIGN(size, align) : size;
if (type == PRIMARY_INTERFACE) {
epc = epf->epc;
@@ -303,7 +312,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
}
dev = epc->dev.parent;
- space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
+ space = dma_alloc_coherent(dev, aligned_size, &phys_addr, GFP_KERNEL);
if (!space) {
dev_err(dev, "failed to allocate mem space\n");
return NULL;
--
2.47.2Hello Jerome,
On Mon, Apr 07, 2025 at 04:39:08PM +0200, Jerome Brunet wrote:
> When trying to allocate space for an endpoint function on a BAR with a
> fixed size, the size saved in the 'struct pci_epf_bar' should be the fixed
> size. This is expected by pci_epc_set_bar().
>
> However, if the fixed_size is smaller that the alignment, the size saved
> in the 'struct pci_epf_bar' matches the alignment and it is a problem for
> pci_epc_set_bar().
>
> To solve this, continue to allocate space that match the iATU alignment
> requirement but save the size that matches what is present in the BAR.
>
> Fixes: 2a9a801620ef ("PCI: endpoint: Add support to specify alignment for buffers allocated to BARs")
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
> drivers/pci/endpoint/pci-epf-core.c | 25 +++++++++++++++++--------
> 1 file changed, 17 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> index b7deb0ee1760b23a24f49abf3baf53ea2f273476..fb902b751e1c965c902c5199d57969ae0a757c2e 100644
> --- a/drivers/pci/endpoint/pci-epf-core.c
> +++ b/drivers/pci/endpoint/pci-epf-core.c
> @@ -225,6 +225,7 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> struct device *dev;
> struct pci_epf_bar *epf_bar;
> struct pci_epc *epc;
> + size_t size;
>
> if (!addr)
> return;
> @@ -237,9 +238,12 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> epf_bar = epf->sec_epc_bar;
> }
>
> + size = epf_bar[bar].size;
> + if (epc_features->align)
> + size = ALIGN(size, epc_features->align);
Personally, I think that you should just save the aligned_size / mem_size /
backing_mem_size as a new struct member, as that avoids the risk that someone
later modifies pci_epf_alloc_space() but forgets to update
pci_epf_free_space() accordingly.
But I will leave the decision to the PCI endpoint maintainers.
Kind regards,
Niklas
> +
> dev = epc->dev.parent;
> - dma_free_coherent(dev, epf_bar[bar].size, addr,
> - epf_bar[bar].phys_addr);
> + dma_free_coherent(dev, size, addr, epf_bar[bar].phys_addr);
>
> epf_bar[bar].phys_addr = 0;
> epf_bar[bar].addr = NULL;
> @@ -266,7 +270,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> enum pci_epc_interface_type type)
> {
> u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
> - size_t align = epc_features->align;
> + size_t aligned_size, align = epc_features->align;
> struct pci_epf_bar *epf_bar;
> dma_addr_t phys_addr;
> struct pci_epc *epc;
> @@ -287,12 +291,17 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> return NULL;
> }
> size = bar_fixed_size;
> + } else {
> + /* BAR size must be power of two */
> + size = roundup_pow_of_two(size);
> }
>
> - if (align)
> - size = ALIGN(size, align);
> - else
> - size = roundup_pow_of_two(size);
> + /*
> + * Allocate enough memory to accommodate the iATU alignment requirement.
> + * In most cases, this will be the same as .size but it might be different
> + * if, for example, the fixed size of a BAR is smaller than align
> + */
> + aligned_size = align ? ALIGN(size, align) : size;
>
> if (type == PRIMARY_INTERFACE) {
> epc = epf->epc;
> @@ -303,7 +312,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
> }
>
> dev = epc->dev.parent;
> - space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
> + space = dma_alloc_coherent(dev, aligned_size, &phys_addr, GFP_KERNEL);
> if (!space) {
> dev_err(dev, "failed to allocate mem space\n");
> return NULL;
>
> --
> 2.47.2
>
On Mon 07 Apr 2025 at 17:35, Niklas Cassel <cassel@kernel.org> wrote:
> Hello Jerome,
>
> On Mon, Apr 07, 2025 at 04:39:08PM +0200, Jerome Brunet wrote:
>> When trying to allocate space for an endpoint function on a BAR with a
>> fixed size, the size saved in the 'struct pci_epf_bar' should be the fixed
>> size. This is expected by pci_epc_set_bar().
>>
>> However, if the fixed_size is smaller that the alignment, the size saved
>> in the 'struct pci_epf_bar' matches the alignment and it is a problem for
>> pci_epc_set_bar().
>>
>> To solve this, continue to allocate space that match the iATU alignment
>> requirement but save the size that matches what is present in the BAR.
>>
>> Fixes: 2a9a801620ef ("PCI: endpoint: Add support to specify alignment for buffers allocated to BARs")
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>> drivers/pci/endpoint/pci-epf-core.c | 25 +++++++++++++++++--------
>> 1 file changed, 17 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
>> index b7deb0ee1760b23a24f49abf3baf53ea2f273476..fb902b751e1c965c902c5199d57969ae0a757c2e 100644
>> --- a/drivers/pci/endpoint/pci-epf-core.c
>> +++ b/drivers/pci/endpoint/pci-epf-core.c
>> @@ -225,6 +225,7 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
>> struct device *dev;
>> struct pci_epf_bar *epf_bar;
>> struct pci_epc *epc;
>> + size_t size;
>>
>> if (!addr)
>> return;
>> @@ -237,9 +238,12 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
>> epf_bar = epf->sec_epc_bar;
>> }
>>
>> + size = epf_bar[bar].size;
>> + if (epc_features->align)
>> + size = ALIGN(size, epc_features->align);
>
> Personally, I think that you should just save the aligned_size / mem_size /
> backing_mem_size as a new struct member, as that avoids the risk that someone
> later modifies pci_epf_alloc_space() but forgets to update
> pci_epf_free_space() accordingly.
I tried but it looked a bit silly to store that when it was only a
matter of calling ALIGN() with parameters we already had, and it is
supposed to be only used in those two functions.
>
> But I will leave the decision to the PCI endpoint maintainers.
>
Ultimately, I do not have a strong opinion on this. Either way is fine
by me.
>
> Kind regards,
> Niklas
>
>
>> +
>> dev = epc->dev.parent;
>> - dma_free_coherent(dev, epf_bar[bar].size, addr,
>> - epf_bar[bar].phys_addr);
>> + dma_free_coherent(dev, size, addr, epf_bar[bar].phys_addr);
>>
>> epf_bar[bar].phys_addr = 0;
>> epf_bar[bar].addr = NULL;
>> @@ -266,7 +270,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>> enum pci_epc_interface_type type)
>> {
>> u64 bar_fixed_size = epc_features->bar[bar].fixed_size;
>> - size_t align = epc_features->align;
>> + size_t aligned_size, align = epc_features->align;
>> struct pci_epf_bar *epf_bar;
>> dma_addr_t phys_addr;
>> struct pci_epc *epc;
>> @@ -287,12 +291,17 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>> return NULL;
>> }
>> size = bar_fixed_size;
>> + } else {
>> + /* BAR size must be power of two */
>> + size = roundup_pow_of_two(size);
>> }
>>
>> - if (align)
>> - size = ALIGN(size, align);
>> - else
>> - size = roundup_pow_of_two(size);
>> + /*
>> + * Allocate enough memory to accommodate the iATU alignment requirement.
>> + * In most cases, this will be the same as .size but it might be different
>> + * if, for example, the fixed size of a BAR is smaller than align
>> + */
>> + aligned_size = align ? ALIGN(size, align) : size;
>>
>> if (type == PRIMARY_INTERFACE) {
>> epc = epf->epc;
>> @@ -303,7 +312,7 @@ void *pci_epf_alloc_space(struct pci_epf *epf, size_t size, enum pci_barno bar,
>> }
>>
>> dev = epc->dev.parent;
>> - space = dma_alloc_coherent(dev, size, &phys_addr, GFP_KERNEL);
>> + space = dma_alloc_coherent(dev, aligned_size, &phys_addr, GFP_KERNEL);
>> if (!space) {
>> dev_err(dev, "failed to allocate mem space\n");
>> return NULL;
>>
>> --
>> 2.47.2
>>
--
Jerome
On Mon, Apr 07, 2025 at 05:43:00PM +0200, Jerome Brunet wrote:
> On Mon 07 Apr 2025 at 17:35, Niklas Cassel <cassel@kernel.org> wrote:
>
> > Hello Jerome,
> >
> > On Mon, Apr 07, 2025 at 04:39:08PM +0200, Jerome Brunet wrote:
> >> When trying to allocate space for an endpoint function on a BAR with a
> >> fixed size, the size saved in the 'struct pci_epf_bar' should be the fixed
> >> size. This is expected by pci_epc_set_bar().
> >>
> >> However, if the fixed_size is smaller that the alignment, the size saved
> >> in the 'struct pci_epf_bar' matches the alignment and it is a problem for
> >> pci_epc_set_bar().
> >>
> >> To solve this, continue to allocate space that match the iATU alignment
> >> requirement but save the size that matches what is present in the BAR.
> >>
> >> Fixes: 2a9a801620ef ("PCI: endpoint: Add support to specify alignment for buffers allocated to BARs")
> >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> >> ---
> >> drivers/pci/endpoint/pci-epf-core.c | 25 +++++++++++++++++--------
> >> 1 file changed, 17 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> >> index b7deb0ee1760b23a24f49abf3baf53ea2f273476..fb902b751e1c965c902c5199d57969ae0a757c2e 100644
> >> --- a/drivers/pci/endpoint/pci-epf-core.c
> >> +++ b/drivers/pci/endpoint/pci-epf-core.c
> >> @@ -225,6 +225,7 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> >> struct device *dev;
> >> struct pci_epf_bar *epf_bar;
> >> struct pci_epc *epc;
> >> + size_t size;
> >>
> >> if (!addr)
> >> return;
> >> @@ -237,9 +238,12 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> >> epf_bar = epf->sec_epc_bar;
> >> }
> >>
> >> + size = epf_bar[bar].size;
> >> + if (epc_features->align)
> >> + size = ALIGN(size, epc_features->align);
> >
> > Personally, I think that you should just save the aligned_size / mem_size /
> > backing_mem_size as a new struct member, as that avoids the risk that someone
> > later modifies pci_epf_alloc_space() but forgets to update
> > pci_epf_free_space() accordingly.
>
> I tried but it looked a bit silly to store that when it was only a
> matter of calling ALIGN() with parameters we already had, and it is
> supposed to be only used in those two functions.
Another advantage is that you could kill patch 1/3 in this series, as
there would be no need to supply epc_features to pci_epf_free_space().
Kind regards,
Niklas
On Tue, Apr 08, 2025 at 11:36:32AM +0200, Niklas Cassel wrote:
> On Mon, Apr 07, 2025 at 05:43:00PM +0200, Jerome Brunet wrote:
> > On Mon 07 Apr 2025 at 17:35, Niklas Cassel <cassel@kernel.org> wrote:
> >
> > > Hello Jerome,
> > >
> > > On Mon, Apr 07, 2025 at 04:39:08PM +0200, Jerome Brunet wrote:
> > >> When trying to allocate space for an endpoint function on a BAR with a
> > >> fixed size, the size saved in the 'struct pci_epf_bar' should be the fixed
> > >> size. This is expected by pci_epc_set_bar().
> > >>
> > >> However, if the fixed_size is smaller that the alignment, the size saved
> > >> in the 'struct pci_epf_bar' matches the alignment and it is a problem for
> > >> pci_epc_set_bar().
> > >>
> > >> To solve this, continue to allocate space that match the iATU alignment
> > >> requirement but save the size that matches what is present in the BAR.
> > >>
> > >> Fixes: 2a9a801620ef ("PCI: endpoint: Add support to specify alignment for buffers allocated to BARs")
> > >> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> > >> ---
> > >> drivers/pci/endpoint/pci-epf-core.c | 25 +++++++++++++++++--------
> > >> 1 file changed, 17 insertions(+), 8 deletions(-)
> > >>
> > >> diff --git a/drivers/pci/endpoint/pci-epf-core.c b/drivers/pci/endpoint/pci-epf-core.c
> > >> index b7deb0ee1760b23a24f49abf3baf53ea2f273476..fb902b751e1c965c902c5199d57969ae0a757c2e 100644
> > >> --- a/drivers/pci/endpoint/pci-epf-core.c
> > >> +++ b/drivers/pci/endpoint/pci-epf-core.c
> > >> @@ -225,6 +225,7 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> > >> struct device *dev;
> > >> struct pci_epf_bar *epf_bar;
> > >> struct pci_epc *epc;
> > >> + size_t size;
> > >>
> > >> if (!addr)
> > >> return;
> > >> @@ -237,9 +238,12 @@ void pci_epf_free_space(struct pci_epf *epf, void *addr, enum pci_barno bar,
> > >> epf_bar = epf->sec_epc_bar;
> > >> }
> > >>
> > >> + size = epf_bar[bar].size;
> > >> + if (epc_features->align)
> > >> + size = ALIGN(size, epc_features->align);
> > >
> > > Personally, I think that you should just save the aligned_size / mem_size /
> > > backing_mem_size as a new struct member, as that avoids the risk that someone
> > > later modifies pci_epf_alloc_space() but forgets to update
> > > pci_epf_free_space() accordingly.
+1. It is always a better approach to store the aligned size during allocation
and use it while freeing. It indeed avoids the alloc/free to go unsymmetry in
the future.
> >
> > I tried but it looked a bit silly to store that when it was only a
> > matter of calling ALIGN() with parameters we already had, and it is
> > supposed to be only used in those two functions.
>
> Another advantage is that you could kill patch 1/3 in this series, as
> there would be no need to supply epc_features to pci_epf_free_space().
>
Yes!
- Mani
--
மணிவண்ணன் சதாசிவம்
© 2016 - 2026 Red Hat, Inc.