Re: [PATCH v3 2/3] PCI: endpoint: improve fixed_size bar handling when allocating space

[Niklas Cassel <cassel@kernel.org>]

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