* [PATCH 0/2] PCI endpoint test: Add support for capabilities @ 2024-11-20 15:57 Niklas Cassel 2024-11-20 15:57 ` [PATCH 1/2] PCI: endpoint: pci-epf-test: " Niklas Cassel 2024-11-20 15:57 ` [PATCH 2/2] misc: pci_endpoint_test: " Niklas Cassel 0 siblings, 2 replies; 10+ messages in thread From: Niklas Cassel @ 2024-11-20 15:57 UTC (permalink / raw) To: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I Cc: Damien Le Moal, linux-pci, Frank Li, 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] PCI: endpoint: pci-epf-test: Add support for capabilities 2024-11-20 15:57 [PATCH 0/2] PCI endpoint test: Add support for capabilities Niklas Cassel @ 2024-11-20 15:57 ` Niklas Cassel 2024-11-20 15:57 ` [PATCH 2/2] misc: pci_endpoint_test: " Niklas Cassel 1 sibling, 0 replies; 10+ messages in thread From: Niklas Cassel @ 2024-11-20 15:57 UTC (permalink / raw) To: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I Cc: Damien Le Moal, linux-pci, Frank Li, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-20 15:57 [PATCH 0/2] PCI endpoint test: Add support for capabilities Niklas Cassel 2024-11-20 15:57 ` [PATCH 1/2] PCI: endpoint: pci-epf-test: " Niklas Cassel @ 2024-11-20 15:57 ` Niklas Cassel 2024-11-20 16:53 ` Frank Li 2024-11-21 2:54 ` Damien Le Moal 1 sibling, 2 replies; 10+ messages in thread From: Niklas Cassel @ 2024-11-20 15:57 UTC (permalink / raw) To: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I Cc: Damien Le Moal, linux-pci, Frank Li, 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-20 15:57 ` [PATCH 2/2] misc: pci_endpoint_test: " Niklas Cassel @ 2024-11-20 16:53 ` Frank Li 2024-11-20 17:05 ` Niklas Cassel 2024-11-21 2:54 ` Damien Le Moal 1 sibling, 1 reply; 10+ messages in thread From: Frank Li @ 2024-11-20 16:53 UTC (permalink / raw) To: Niklas Cassel Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Damien Le Moal, linux-pci 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-20 16:53 ` Frank Li @ 2024-11-20 17:05 ` Niklas Cassel 2024-11-20 17:12 ` Frank Li 0 siblings, 1 reply; 10+ messages in thread From: Niklas Cassel @ 2024-11-20 17:05 UTC (permalink / raw) To: Frank Li Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Damien Le Moal, linux-pci 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-20 17:05 ` Niklas Cassel @ 2024-11-20 17:12 ` Frank Li 2024-11-20 17:17 ` Niklas Cassel 0 siblings, 1 reply; 10+ messages in thread From: Frank Li @ 2024-11-20 17:12 UTC (permalink / raw) To: Niklas Cassel Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Damien Le Moal, linux-pci 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-20 17:12 ` Frank Li @ 2024-11-20 17:17 ` Niklas Cassel 0 siblings, 0 replies; 10+ messages in thread From: Niklas Cassel @ 2024-11-20 17:17 UTC (permalink / raw) To: Frank Li Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, Damien Le Moal, linux-pci 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-20 15:57 ` [PATCH 2/2] misc: pci_endpoint_test: " Niklas Cassel 2024-11-20 16:53 ` Frank Li @ 2024-11-21 2:54 ` Damien Le Moal 2024-11-21 12:09 ` Niklas Cassel 1 sibling, 1 reply; 10+ messages in thread From: Damien Le Moal @ 2024-11-21 2:54 UTC (permalink / raw) To: Niklas Cassel, Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I Cc: linux-pci, Frank Li 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-21 2:54 ` Damien Le Moal @ 2024-11-21 12:09 ` Niklas Cassel 2024-11-21 12:25 ` Damien Le Moal 0 siblings, 1 reply; 10+ messages in thread From: Niklas Cassel @ 2024-11-21 12:09 UTC (permalink / raw) To: Damien Le Moal Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, linux-pci, Frank Li 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] misc: pci_endpoint_test: Add support for capabilities 2024-11-21 12:09 ` Niklas Cassel @ 2024-11-21 12:25 ` Damien Le Moal 0 siblings, 0 replies; 10+ messages in thread From: Damien Le Moal @ 2024-11-21 12:25 UTC (permalink / raw) To: Niklas Cassel Cc: Manivannan Sadhasivam, Krzysztof Wilczyński, Kishon Vijay Abraham I, linux-pci, Frank Li 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-11-21 12:25 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-11-20 15:57 [PATCH 0/2] PCI endpoint test: Add support for capabilities Niklas Cassel 2024-11-20 15:57 ` [PATCH 1/2] PCI: endpoint: pci-epf-test: " Niklas Cassel 2024-11-20 15:57 ` [PATCH 2/2] misc: pci_endpoint_test: " Niklas Cassel 2024-11-20 16:53 ` Frank Li 2024-11-20 17:05 ` Niklas Cassel 2024-11-20 17:12 ` Frank Li 2024-11-20 17:17 ` Niklas Cassel 2024-11-21 2:54 ` Damien Le Moal 2024-11-21 12:09 ` Niklas Cassel 2024-11-21 12:25 ` Damien Le Moal
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.