[PATCH 0/2] PCI endpoint test: Add support for capabilities

[PATCH 0/2] PCI endpoint test: Add support for capabilities

[Niklas Cassel]

Hello all,

The pci-epf-test driver recently moved to the pci_epc_mem_map() API.
This API call handle unaligned addresses seamlessly, if the EPC driver
being used has implemented the .align_addr callback.

This means that pci-epf-test does no longer need any special padding
to the buffers that is allocated on the host side. (This was only done
in order to satisfy EPC's alignment requirements.)

In fact, to test that the pci_epc_mem_map() API is working as intended,
it is important that the host side does not allocate only give us buffers
that are nicely aligned.

However, since not all EPC drivers have implemented .align_addr,
add support for capabilities in pci-epf-test, and if .align_addr is
implemented, set a new align_addr capability. If this new align_addr
is set, do not allocate overly sized buffers on the host side.

EPC drivers that have not implemented .align_addr will behave just as
they did before.


Kind regards,
Niklas


Niklas Cassel (2):
  PCI: endpoint: pci-epf-test: Add support for capabilities
  misc: pci_endpoint_test: Add support for capabilities

 drivers/misc/pci_endpoint_test.c              | 21 +++++++++++++++++++
 drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++
 2 files changed, 40 insertions(+)

-- 
2.47.0

[PATCH 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities

[Niklas Cassel]

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.

For now, only add the CAP_HAS_ALIGN_ADDR capability.

Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index ef6677f34116..e3a74a6fcb24 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -44,6 +44,8 @@
 
 #define TIMER_RESOLUTION		1
 
+#define CAP_HAS_ALIGN_ADDR		BIT(0)
+
 static struct workqueue_struct *kpcitest_workqueue;
 
 struct pci_epf_test {
@@ -74,6 +76,7 @@ struct pci_epf_test_reg {
 	u32	irq_type;
 	u32	irq_number;
 	u32	flags;
+	u32	caps;
 } __packed;
 
 static struct pci_epf_header test_header = {
@@ -739,6 +742,20 @@ static void pci_epf_test_clear_bar(struct pci_epf *epf)
 	}
 }
 
+static void pci_epf_test_set_capabilities(struct pci_epf *epf)
+{
+	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
+	enum pci_barno test_reg_bar = epf_test->test_reg_bar;
+	struct pci_epf_test_reg *reg = epf_test->reg[test_reg_bar];
+	struct pci_epc *epc = epf->epc;
+	u32 caps = 0;
+
+	if (epc->ops->align_addr)
+		caps |= CAP_HAS_ALIGN_ADDR;
+
+	reg->caps = cpu_to_le32(caps);
+}
+
 static int pci_epf_test_epc_init(struct pci_epf *epf)
 {
 	struct pci_epf_test *epf_test = epf_get_drvdata(epf);
@@ -763,6 +780,8 @@ static int pci_epf_test_epc_init(struct pci_epf *epf)
 		}
 	}
 
+	pci_epf_test_set_capabilities(epf);
+
 	ret = pci_epf_test_set_bar(epf);
 	if (ret)
 		return ret;
-- 
2.47.0

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

[Niklas Cassel]

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);
+
+	has_align_addr = caps & CAP_HAS_ALIGN_ADDR;
+	dev_dbg(dev, "CAP_HAS_ALIGN_ADDR: %d\n", has_align_addr);
+
+	if (has_align_addr)
+		test->alignment = 0;
+}
+
 static int pci_endpoint_test_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *ent)
 {
@@ -906,6 +925,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
 		goto err_kfree_test_name;
 	}
 
+	pci_endpoint_test_get_capabilities(test);
+
 	misc_device = &test->miscdev;
 	misc_device->minor = MISC_DYNAMIC_MINOR;
 	misc_device->name = kstrdup(name, GFP_KERNEL);
-- 
2.47.0

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

[Frank Li]

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.

Frank

> +
> +	has_align_addr = caps & CAP_HAS_ALIGN_ADDR;
> +	dev_dbg(dev, "CAP_HAS_ALIGN_ADDR: %d\n", has_align_addr);
> +
> +	if (has_align_addr)
> +		test->alignment = 0;
> +}
> +
>  static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  				   const struct pci_device_id *ent)
>  {
> @@ -906,6 +925,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  		goto err_kfree_test_name;
>  	}
>
> +	pci_endpoint_test_get_capabilities(test);
> +
>  	misc_device = &test->miscdev;
>  	misc_device->minor = MISC_DYNAMIC_MINOR;
>  	misc_device->name = kstrdup(name, GFP_KERNEL);
> --
> 2.47.0
>

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

[Niklas Cassel]

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
bytes for the test BAR:
https://github.com/torvalds/linux/blob/v6.12/drivers/pci/endpoint/functions/pci-epf-test.c#L833

This patch uses:
#define PCI_ENDPOINT_TEST_CAPS                     0x30

0x30 == 48, so we are currently only using 49 bytes.

So we should be good.


Kind regards,
Niklas

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

[Frank Li]

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?

Frank

> bytes for the test BAR:
> https://github.com/torvalds/linux/blob/v6.12/drivers/pci/endpoint/functions/pci-epf-test.c#L833
>
> This patch uses:
> #define PCI_ENDPOINT_TEST_CAPS                     0x30
>
> 0x30 == 48, so we are currently only using 49 bytes.
>
> So we should be good.
>
>
> Kind regards,
> Niklas

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

[Niklas Cassel]

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

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

[Damien Le Moal]

On 11/21/24 00:57, 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);
> +
> +	has_align_addr = caps & CAP_HAS_ALIGN_ADDR;
> +	dev_dbg(dev, "CAP_HAS_ALIGN_ADDR: %d\n", has_align_addr);
> +
> +	if (has_align_addr)

Shouldn't this be "if (!has_align_addr)" ?

> +		test->alignment = 0;
> +}
> +
>  static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  				   const struct pci_device_id *ent)
>  {
> @@ -906,6 +925,8 @@ static int pci_endpoint_test_probe(struct pci_dev *pdev,
>  		goto err_kfree_test_name;
>  	}
>  
> +	pci_endpoint_test_get_capabilities(test);
> +
>  	misc_device = &test->miscdev;
>  	misc_device->minor = MISC_DYNAMIC_MINOR;
>  	misc_device->name = kstrdup(name, GFP_KERNEL);


-- 
Damien Le Moal
Western Digital Research

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

[Niklas Cassel]

Hello Damien,

On Thu, Nov 21, 2024 at 11:54:48AM +0900, Damien Le Moal wrote:
> On 11/21/24 00:57, 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);
> > +
> > +	has_align_addr = caps & CAP_HAS_ALIGN_ADDR;
> > +	dev_dbg(dev, "CAP_HAS_ALIGN_ADDR: %d\n", has_align_addr);
> > +
> > +	if (has_align_addr)
> 
> Shouldn't this be "if (!has_align_addr)" ?

Nope. Check patch 1/2 in this series.

+	if (epc->ops->align_addr)
+		caps |= CAP_HAS_ALIGN_ADDR;

i.e. if the EP implements the addr_align callback, then we know for sure
that the EP read/write anywhere.


However, if even you as the author of the .addr_align callback get confused
by this, then perhaps we should rename things.

How about:

ep_has_align_addr_cb = caps & CAP_HAS_ALIGN_ADDR_CB;
if (ep_has_align_addr_cb)
	test->alignment = 0;

or

ep_can_do_unaligned_access = caps & CAP_HAS_UNALIGNED_ACCESS;
if (ep_can_do_unaligned_access)
	test->alignment = 0;


Do you have any better suggestion?


Kind regards,
Niklas

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

[Damien Le Moal]

On 11/21/24 21:09, Niklas Cassel wrote:
> Hello Damien,
> 
> On Thu, Nov 21, 2024 at 11:54:48AM +0900, Damien Le Moal wrote:
>> On 11/21/24 00:57, 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);
>>> +
>>> +	has_align_addr = caps & CAP_HAS_ALIGN_ADDR;
>>> +	dev_dbg(dev, "CAP_HAS_ALIGN_ADDR: %d\n", has_align_addr);
>>> +
>>> +	if (has_align_addr)
>>
>> Shouldn't this be "if (!has_align_addr)" ?
> 
> Nope. Check patch 1/2 in this series.
> 
> +	if (epc->ops->align_addr)
> +		caps |= CAP_HAS_ALIGN_ADDR;
> 
> i.e. if the EP implements the addr_align callback, then we know for sure
> that the EP read/write anywhere.
> 
> 
> However, if even you as the author of the .addr_align callback get confused
> by this, then perhaps we should rename things.
> 
> How about:
> 
> ep_has_align_addr_cb = caps & CAP_HAS_ALIGN_ADDR_CB;
> if (ep_has_align_addr_cb)
> 	test->alignment = 0;

I see my confusion: "has_align_addr" means that the EP handles any address with
the .align_addr method. OK. So what about reversing this to make it clear:

	needs_aligned_address = !(caps & CAP_HAS_ALIGN_ADDR);

	if (!needs_aligned_address)
		test->alignment = 0;

> ep_can_do_unaligned_access = caps & CAP_HAS_UNALIGNED_ACCESS;
> if (ep_can_do_unaligned_access)
> 	test->alignment = 0;

Yes, this one :) I find it less confusing.
Maybe CAP_HAS_UNALIGNED_ACCESS can simply be named CAP_UNALIGNED_ACCESS (i.e.
unaligned accesses is OK and is a capability).

> 
> 
> Do you have any better suggestion?
> 
> 
> Kind regards,
> Niklas


-- 
Damien Le Moal
Western Digital Research