| Message ID | 20241120155730.2833836-6-cassel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Krzysztof Wilczyński |
| Headers | show |
| Series | PCI endpoint test: Add support for capabilities | expand |
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 >
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
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
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
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);
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
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);
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(+)