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

Manivannan Sadhasivam
2024-11-30T08:21:35Z

On Thu, Nov 21, 2024 at 04:23:21PM +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.
>
> 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).
>
> Signed-off-by: Niklas Cassel <cassel@kernel.org>

Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

One suggestion below.

> ---
> 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..caae815ab75a 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,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 ep_can_do_unaligned_access;
> +
> + caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS);
> +
> + ep_can_do_unaligned_access = caps & CAP_UNALIGNED_ACCESS;
> + dev_dbg(dev, "CAP_UNALIGNED_ACCESS: %d\n", ep_can_do_unaligned_access);

IDK if the users really need to know about this flag, nor it will assist in any
debugging. Otherwise, I'd suggest to drop this debug print and just do:

if (pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS))
test->alignment = 0;

- Mani

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