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

[PATCH v3 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 no longer need any special padding to the
buffers that is allocated on the host side. (This was only done in order
to satisfy the 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 only provide buffers that
are nicely aligned.

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

For EPC drivers that have not implemented the .align_addr callback, this
series will not introduce any functional changes.


Kind regards,
Niklas


Changes since v2:
-Picked up tags
-Changed debug print to dump the CAPS register instead of having a print
 per capability.


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              | 19 +++++++++++++++++++
 drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++++
 2 files changed, 38 insertions(+)

-- 
2.47.1

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

[Niklas Cassel]

The test BAR is on the EP side 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 an old version of pci-epf-test.c (EP side) will not see any
capabilities being set (as intended), so this is backwards compatible.

Additionally, the EP side always allocates at least 128 bytes for the test
BAR (excluding the MSI-X table), this means that adding another register at
offset 0x30 is still within the 128 available bytes.

For now, we only add the CAP_UNALIGNED_ACCESS capability.

Set CAP_UNALIGNED_ACCESS if the EPC driver can handle any address (because
it implements the .align_addr callback).

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
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..7351289ecddd 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_UNALIGNED_ACCESS		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_UNALIGNED_ACCESS;
+
+	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.1

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

[Niklas Cassel]

The test BAR is on the EP side 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 an old version of pci-epf-test.c (EP side) will not see any
capabilities being set (as intended), so this is backwards compatible.

Additionally, the EP side always allocates at least 128 bytes for the test
BAR (excluding the MSI-X table), this means that adding another register at
offset 0x30 is still within the 128 available bytes.

For now, we only add the CAP_UNALIGNED_ACCESS capability.

If CAP_UNALIGNED_ACCESS is set, that means that the EP side supports
reading/writing to an address without any alignment requirements.

Thus, if CAP_UNALIGNED_ACCESS is set, make sure that the host side does
not add any extra padding to the buffers that we allocate (which was only
done in order to get the buffers to satisfy certain alignment requirements
by the endpoint controller).

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Niklas Cassel <cassel@kernel.org>
---
 drivers/misc/pci_endpoint_test.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/misc/pci_endpoint_test.c b/drivers/misc/pci_endpoint_test.c
index 3aaaf47fa4ee..8a31bd4c237d 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_UNALIGNED_ACCESS			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,20 @@ 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;
+
+	caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
+	dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", caps);
+
+	/* CAP_UNALIGNED_ACCESS is set if the EP can do unaligned access */
+	if (caps & CAP_UNALIGNED_ACCESS)
+		test->alignment = 0;
+}
+
 static int pci_endpoint_test_probe(struct pci_dev *pdev,
 				   const struct pci_device_id *ent)
 {
@@ -906,6 +923,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.1

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

[Niklas Cassel]

On Tue, Dec 03, 2024 at 07:38:52AM +0100, Niklas Cassel wrote:
> 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 no longer need any special padding to the
> buffers that is allocated on the host side. (This was only done in order
> to satisfy the 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 only provide buffers that
> are nicely aligned.
> 
> However, since not all EPC drivers have implemented the .align_addr
> callback, add support for capabilities in pci-epf-test, and if the
> EPC driver implements the .align_addr callback, set a new
> CAP_UNALIGNED_ACCESS capability. If CAP_UNALIGNED_ACCESS is set, do not
> allocate overly sized buffers on the host side.
> 
> For EPC drivers that have not implemented the .align_addr callback, this
> series will not introduce any functional changes.
> 
> 
> Kind regards,
> Niklas
> 
> 
> Changes since v2:
> -Picked up tags
> -Changed debug print to dump the CAPS register instead of having a print
>  per capability.
> 
> 
> 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              | 19 +++++++++++++++++++
>  drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++++
>  2 files changed, 38 insertions(+)
> 
> -- 
> 2.47.1
> 

Since this series has two R-b tags on both patches in the series,
any chance of getting this series picked up?


Kind regards,
Niklas

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

[Niklas Cassel]

On Thu, Dec 12, 2024 at 09:52:17AM +0100, Niklas Cassel wrote:
> 
> Since this series has two R-b tags on both patches in the series,
> any chance of getting this series picked up?

Another week has past, hence another ping :)


Kind regards,
Niklas

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

[Krzysztof Wilczyński]

Hello,

> 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 no longer need any special padding to the
> buffers that is allocated on the host side. (This was only done in order
> to satisfy the 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 only provide buffers that
> are nicely aligned.
> 
> However, since not all EPC drivers have implemented the .align_addr
> callback, add support for capabilities in pci-epf-test, and if the
> EPC driver implements the .align_addr callback, set a new
> CAP_UNALIGNED_ACCESS capability. If CAP_UNALIGNED_ACCESS is set, do not
> allocate overly sized buffers on the host side.
> 
> For EPC drivers that have not implemented the .align_addr callback, this
> series will not introduce any functional changes.

Applied to endpoint, thank you!

[01/02] PCI: endpoint: pci-epf-test: Add support for capabilities
        https://git.kernel.org/pci/pci/c/15c4281440a5
[02/02] misc: pci_endpoint_test: Add support for capabilities
        https://git.kernel.org/pci/pci/c/a00ac0bc28ed

	Krzysztof

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

[Bjorn Helgaas]

On Tue, Dec 03, 2024 at 07:38:53AM +0100, Niklas Cassel wrote:
> The test BAR is on the EP side 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.

> +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_UNALIGNED_ACCESS;
> +
> +	reg->caps = cpu_to_le32(caps);

"make C=2 drivers/pci/" complains about this:

  drivers/pci/endpoint/functions/pci-epf-test.c:756:19: warning: incorrect type in assignment (different base types)
  drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    expected unsigned int [usertype] caps
  drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    got restricted __le32 [usertype]

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

[Niklas Cassel]

Hello Bjorn,

On Sat, Jan 18, 2025 at 02:34:21PM -0600, Bjorn Helgaas wrote:
> On Tue, Dec 03, 2024 at 07:38:53AM +0100, Niklas Cassel wrote:
> > The test BAR is on the EP side 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.
> 
> > +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_UNALIGNED_ACCESS;
> > +
> > +	reg->caps = cpu_to_le32(caps);
> 
> "make C=2 drivers/pci/" complains about this:
> 
>   drivers/pci/endpoint/functions/pci-epf-test.c:756:19: warning: incorrect type in assignment (different base types)
>   drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    expected unsigned int [usertype] caps
>   drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    got restricted __le32 [usertype]

Yes, pci-epf-test is broken when it comes to endianness, as reported here:
https://lore.kernel.org/linux-pci/ZxYHoi4mv-4eg0TK@ryzen.lan/


Nice to see that sparse is complaining about it! :)

Mani said that he was going to work on it, but I guess that it fell through
the cracks.

I sent patch for it here:
https://lore.kernel.org/linux-pci/20250120115009.2748899-2-cassel@kernel.org/T/#u

FWIW, I tested it on rk3588 which is little-endian, and that still worked.

However, if you feel that it is a bit too late to queue it now, you could
also take only the following change from the patch above:

diff --git a/drivers/pci/endpoint/functions/pci-epf-test.c b/drivers/pci/endpoint/functions/pci-epf-test.c
index ffb534a8e50a..cb7e57377214 100644
--- a/drivers/pci/endpoint/functions/pci-epf-test.c
+++ b/drivers/pci/endpoint/functions/pci-epf-test.c
@@ -76,7 +76,7 @@ struct pci_epf_test_reg {
        u32     irq_type;
        u32     irq_number;
        u32     flags;
-       u32     caps;
+       __le32  caps;
 } __packed;
 
 static struct pci_epf_header test_header = {


Kind regards,
Niklas

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

[Manivannan Sadhasivam]

On Mon, Jan 20, 2025 at 01:00:15PM +0100, Niklas Cassel wrote:
> Hello Bjorn,
> 
> On Sat, Jan 18, 2025 at 02:34:21PM -0600, Bjorn Helgaas wrote:
> > On Tue, Dec 03, 2024 at 07:38:53AM +0100, Niklas Cassel wrote:
> > > The test BAR is on the EP side 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.
> > 
> > > +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_UNALIGNED_ACCESS;
> > > +
> > > +	reg->caps = cpu_to_le32(caps);
> > 
> > "make C=2 drivers/pci/" complains about this:
> > 
> >   drivers/pci/endpoint/functions/pci-epf-test.c:756:19: warning: incorrect type in assignment (different base types)
> >   drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    expected unsigned int [usertype] caps
> >   drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    got restricted __le32 [usertype]
> 
> Yes, pci-epf-test is broken when it comes to endianness, as reported here:
> https://lore.kernel.org/linux-pci/ZxYHoi4mv-4eg0TK@ryzen.lan/
> 
> 
> Nice to see that sparse is complaining about it! :)
> 
> Mani said that he was going to work on it, but I guess that it fell through
> the cracks.
> 

It doesn't but I put it in the back burner due to other high priority issues to
fix.

> I sent patch for it here:
> https://lore.kernel.org/linux-pci/20250120115009.2748899-2-cassel@kernel.org/T/#u
> 

Thanks a lot!

- Mani

-- 
மணிவண்ணன் சதாசிவம்

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

[Niklas Cassel]

On Mon, Jan 20, 2025 at 09:14:15PM +0530, Manivannan Sadhasivam wrote:
> On Mon, Jan 20, 2025 at 01:00:15PM +0100, Niklas Cassel wrote:
> > Hello Bjorn,
> > 
> > On Sat, Jan 18, 2025 at 02:34:21PM -0600, Bjorn Helgaas wrote:
> > > On Tue, Dec 03, 2024 at 07:38:53AM +0100, Niklas Cassel wrote:
> > > > The test BAR is on the EP side 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.
> > > 
> > > > +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_UNALIGNED_ACCESS;
> > > > +
> > > > +	reg->caps = cpu_to_le32(caps);
> > > 
> > > "make C=2 drivers/pci/" complains about this:
> > > 
> > >   drivers/pci/endpoint/functions/pci-epf-test.c:756:19: warning: incorrect type in assignment (different base types)
> > >   drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    expected unsigned int [usertype] caps
> > >   drivers/pci/endpoint/functions/pci-epf-test.c:756:19:    got restricted __le32 [usertype]
> > 
> > Yes, pci-epf-test is broken when it comes to endianness, as reported here:
> > https://lore.kernel.org/linux-pci/ZxYHoi4mv-4eg0TK@ryzen.lan/
> > 
> > 
> > Nice to see that sparse is complaining about it! :)
> > 
> > Mani said that he was going to work on it, but I guess that it fell through
> > the cracks.
> > 
> 
> It doesn't but I put it in the back burner due to other high priority issues to
> fix.

I know that feeling :)

Yeah, I know you have been very busy lately :)

Have a nice evening!


Kind regards,
Niklas