| Message ID | 20241121152318.2888179-6-cassel@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI endpoint test: Add support for capabilities | expand |
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> I think 4byte align for readl/writel still required? Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > 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); > + > + if (ep_can_do_unaligned_access) > + 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 >
On Thu, Nov 21, 2024 at 02:43:19PM -0500, Frank Li wrote: > 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> > > I think 4byte align for readl/writel still required? I've been running this series on rk3588 (which has CAP_UNALIGNED_ACCESS set), so alignment will be set to 0 in pci_endpoint_test_{copy,read,write}(). When running: pcitest -r -s 1 pcitest -r -s 2 pcitest -r -s 3 pcitest -r -s 4 many, many times, and dma_alloc_coherent() always returned an address that was 4 byte aligned, regardless of input size. Additionally, when setting alignment to 0, the code in pci_endpoint_test_{copy,read,write}() will behave exactly as it did before the "alignment support" was added in commit: https://github.com/torvalds/linux/commit/13107c60681f19fec25af93de86442ac9373e43f So I think this patch is good as is. Kind regards, Niklas > > Reviewed-by: Frank Li <Frank.Li@nxp.com> > > > --- > > drivers/misc/pci_endpoint_test.c | 21 +++++++++++++++++++++ > > 1 file changed, 21 insertions(+) > >
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
On Sat, Nov 30, 2024 at 01:51:19PM +0530, Manivannan Sadhasivam wrote: > > > > +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 I do think that there is value in having a debug print that prints the capabilities, especially if more caps are added in the future. Let me send a v3 that dumps all caps (as hex) in a single print instead, i.e. simply: caps = pci_endpoint_test_readl(test, PCI_ENDPOINT_TEST_CAPS); dev_dbg(dev, "PCI_ENDPOINT_TEST_CAPS: %#x\n", caps); Kind regards, Niklas
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); + + if (ep_can_do_unaligned_access) + 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);
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> --- drivers/misc/pci_endpoint_test.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+)