Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities

[Niklas Cassel <cassel@kernel.org>]

On Wed, Nov 20, 2024 at 12:12:51PM -0500, Frank Li wrote:
> On Wed, Nov 20, 2024 at 06:05:31PM +0100, Niklas Cassel wrote:
> > On Wed, Nov 20, 2024 at 11:53:29AM -0500, Frank Li wrote:
> > > On Wed, Nov 20, 2024 at 04:57:33PM +0100, Niklas Cassel wrote:
> > > > If running pci_endpoint_test.c (host side) against a version of
> > > > pci-epf-test.c (EP side), we will not see any capabilities being set.
> > > >
> > > > For now, only add the CAP_HAS_ALIGN_ADDR capability.
> > > >
> > > > If the CAP_HAS_ALIGN_ADDR is set, that means that the EP side supports
> > > > reading/writing to an address without any alignment requirements.
> > > >
> > > > Thus, if CAP_HAS_ALIGN_ADDR is set, make sure that we do not add any
> > > > specific padding to the buffers that we allocate (which was only made
> > > > in order to get the buffers to satisfy certain alignment requirements).
> > > >
> > > > Signed-off-by: Niklas Cassel <cassel@kernel.org>
> > > > ---
> > > >  drivers/misc/pci_endpoint_test.c | 21 +++++++++++++++++++++
> > > >  1 file changed, 21 insertions(+)
> > > >
> > > > diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
> > > > index 3aaaf47fa4ee..ab2b322410fb 100644
> > > > --- a/drivers/misc/pci_endpoint_test.c
> > > > +++ b/drivers/misc/pci_endpoint_test.c
> > > > @@ -69,6 +69,9 @@
> > > >  #define PCI_ENDPOINT_TEST_FLAGS			0x2c
> > > >  #define FLAG_USE_DMA				BIT(0)
> > > >
> > > > +#define PCI_ENDPOINT_TEST_CAPS			0x30
> > > > +#define CAP_HAS_ALIGN_ADDR			BIT(0)
> > > > +
> > > >  #define PCI_DEVICE_ID_TI_AM654			0xb00c
> > > >  #define PCI_DEVICE_ID_TI_J7200			0xb00f
> > > >  #define PCI_DEVICE_ID_TI_AM64			0xb010
> > > > @@ -805,6 +808,22 @@ static const struct file_operations pci_endpoint_test_fops = {
> > > >  	.unlocked_ioctl = pci_endpoint_test_ioctl,
> > > >  };
> > > >
> > > > +static void pci_endpoint_test_get_capabilities(struct pci_endpoint_test *test)
> > > > +{
> > > > +	struct pci_dev *pdev = test->pdev;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	u32 caps;
> > > > +	bool has_align_addr;
> > > > +
> > > > +	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> > >
> > > I worry about if there are problem if EP have not such register. for
> > > example, if reg's space size is 64, but host try to read pos 68. I think it
> > > is original design problem, it should have one 'version' or 'size' in
> > > register list.
> >
> > Hello Frank,
> >
> > The test BAR is allocated using pci_epf_alloc_space(), which allocates the
> > backing memory using dma_alloc_coherent(), which will return zeroed memory
> > regardless of __GFP_ZERO was set or not.
> >
> > This means that running a new version of pci-endpoint-test.c (host side)
> > with and old version of pci-epf-test.c (EP side) will not see any
> > capabilities being set (as intended), so this is backwards compatible.
> >
> >
> > And as you probably know, pci-epf-test will always allocate at least 128
> 
> Can you add such information to commit message?

This text is there in patch 1/2, but I can add it to patch 2/2 as well,
and add the text that test BAR reg is always at least 128 bytes.

Will do this in V2, thanks!


Kind regards,
Niklas