Re: [PATCH] misc: pci_endpoint_test: Handle BAR sizes larger than INT_MAX

[Niklas Cassel <cassel@kernel.org>]

On Thu, Jan 23, 2025 at 02:09:24PM -0500, Frank Li wrote:
> On Thu, Jan 23, 2025 at 07:35:06PM +0100, Niklas Cassel wrote:
> > >
> > > Actually, you change code logic although functionality is the same. I feel
> > > like you should mention at commit message or use origial code by just
> > > change variable type.
> > >
> > > #ifdef CONFIG_PHYS_ADDR_T_64BIT
> > > typedef u64 phys_addr_t;
> > > #else
> > > typedef u32 phys_addr_t;
> > > #endif
> >
> > Hello Frank,
> >
> > I personally think that is a horrible idea :)
> >
> > We do not want to introduce ifdefs in the middle of the code, unless
> > in exceptional circumstances, like architecture specific optimized code.
> 
> You miss understand what my means. I copy it from type.h to indicate
> resource_size_t is not 64bit at all platforms.

I know that resource_size_t is typedefed to phys_addr_t, which can be 32-bit
or 64-bit. (I compile tested this patch on 32-bit both with and without PAE.)

resource_size_t is the type returned by pci_resource_len().
That is why the patch in subject changes the type to use resource_size_t.
IMO, it does not make sense to use any other type (e.g. u64), since the
value returned by pci_resource_len() will still be limited to what can be
represented by resource_size_t.

A BARs larger than 4GB, on systems with 32-bit resource_size_t, will get
disabled by PCI core:
https://github.com/torvalds/linux/blob/v6.13/drivers/pci/probe.c#L265-L270

So all good.



As for your question why I don't keep the division, please read the comment
section in this patch (where the changelog usually is), or read the thread:
https://lore.kernel.org/linux-pci/20250109094556.1724663-1-18255117159@163.com/T/#t

I guess I could have added:
"
In order to handle 64-bit resource_type_t on 32-bit platforms, we would
have needed to use a function like div_u64() or similar. Instead, change
the code to use addition instead of division. This avoids the need for
div_u64() or similar, while also simplifying the code.
"

Let me send a V2 with that senctence added to address your review comment.


Kind regards,
Niklas