Re: [PATCH 2/2] PCI: endpoint: pci-epf-vntb: simplify ctrl/spad space allocation

[Jerome Brunet <jbrunet@baylibre.com>]

On Mon 31 Mar 2025 at 10:48, Frank Li <Frank.li@nxp.com> wrote:

> On Fri, Mar 28, 2025 at 03:53:43PM +0100, Jerome Brunet wrote:
>> When allocating the shared ctrl/spad space, epf_ntb_config_spad_bar_alloc()
>> should not try to handle the size quirks for the underlying BAR, whether it
>> is fixed size or alignment. This is already handled by
>> pci_epf_alloc_space().
>>
>> Also, when handling the alignment, this allocate more space than necessary.
>> For example, with a spad size of 1024B and a ctrl size of 308B, the space
>> necessary is 1332B. If the alignment is 1MB,
>> epf_ntb_config_spad_bar_alloc() tries to allocate 2MB where 1MB would have
>> been more than enough.
>>
>> Just drop all the handling of the BAR size quirks and let
>> pci_epf_alloc_space() handle that.
>>
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  drivers/pci/endpoint/functions/pci-epf-vntb.c | 24 ++----------------------
>>  1 file changed, 2 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/pci/endpoint/functions/pci-epf-vntb.c b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> index 874cb097b093ae645bbc4bf3c9d28ca812d7689d..c20a60fcb99e6e16716dd78ab59ebf7cf074b2a6 100644
>> --- a/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> +++ b/drivers/pci/endpoint/functions/pci-epf-vntb.c
>> @@ -408,11 +408,9 @@ static void epf_ntb_config_spad_bar_free(struct epf_ntb *ntb)
>>   */
>>  static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>>  {
>> -	size_t align;
>>  	enum pci_barno barno;
>>  	struct epf_ntb_ctrl *ctrl;
>>  	u32 spad_size, ctrl_size;
>> -	u64 size;
>>  	struct pci_epf *epf = ntb->epf;
>>  	struct device *dev = &epf->dev;
>>  	u32 spad_count;
>> @@ -422,31 +420,13 @@ static int epf_ntb_config_spad_bar_alloc(struct epf_ntb *ntb)
>>  								epf->func_no,
>>  								epf->vfunc_no);
>>  	barno = ntb->epf_ntb_bar[BAR_CONFIG];
>> -	size = epc_features->bar[barno].fixed_size;
>> -	align = epc_features->align;
>> -
>> -	if ((!IS_ALIGNED(size, align)))
>> -		return -EINVAL;
>> -
>>  	spad_count = ntb->spad_count;
>>
>>  	ctrl_size = sizeof(struct epf_ntb_ctrl);
>
> I think keep ctrl_size at least align to 4 bytes.

Sure, makes sense

> keep align 2^n is more safe to keep spad area start at align
> possition.

That's something else. Both region are registers (or the emulation of
it) so a 32bits aligned is enough, AFAICT.

What purpose would 2^n aligned serve ? If it is safer, what's is the risk
exactly ?

>
> 	ctrl_size = roundup_pow_of_two(sizeof(struct epf_ntb_ctrl));
>
> Frank
>
>>  	spad_size = 2 * spad_count * sizeof(u32);
>>
>> -	if (!align) {
>> -		ctrl_size = roundup_pow_of_two(ctrl_size);
>> -		spad_size = roundup_pow_of_two(spad_size);
>> -	} else {
>> -		ctrl_size = ALIGN(ctrl_size, align);
>> -		spad_size = ALIGN(spad_size, align);
>> -	}
>> -
>> -	if (!size)
>> -		size = ctrl_size + spad_size;
>> -	else if (size < ctrl_size + spad_size)
>> -		return -EINVAL;
>> -
>> -	base = pci_epf_alloc_space(epf, size, barno, epc_features, 0);
>> +	base = pci_epf_alloc_space(epf, ctrl_size + spad_size,
>> +				   barno, epc_features, 0);
>>  	if (!base) {
>>  		dev_err(dev, "Config/Status/SPAD alloc region fail\n");
>>  		return -ENOMEM;
>>
>> --
>> 2.47.2
>>

-- 
Jerome