| Message ID | 20241121152318.2888179-5-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:20PM +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. > > Set CAP_UNALIGNED_ACCESS if the EPC driver can handle any address (because > it implements the .align_addr callback). > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Reviewed-by: Frank Li <Frank.Li@nxp.com> > --- > 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.0 >
On Thu, Nov 21, 2024 at 04:23:20PM +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. > > Set CAP_UNALIGNED_ACCESS if the EPC driver can handle any address (because > it implements the .align_addr callback). > > Signed-off-by: Niklas Cassel <cassel@kernel.org> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> Just a small nit below. > --- > 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; Can we rename the 'magic' register? It is not used since the beginning and I don't know if we will ever have a usecase for it. - Mani > } __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.0 >
Hello Mani, On Sat, Nov 30, 2024 at 01:42:45PM +0530, Manivannan Sadhasivam wrote: > > > > struct pci_epf_test { > > @@ -74,6 +76,7 @@ struct pci_epf_test_reg { > > u32 irq_type; > > u32 irq_number; > > u32 flags; > > + u32 caps; > > Can we rename the 'magic' register? It is not used since the beginning and I > don't know if we will ever have a usecase for it. It is actually used! When doing PCITEST_BAR (pci_endpoint_test_bar()), and barno == test->test_reg_bar, we are only filling the first 4 bytes, rather than filling the whole BAR: https://github.com/torvalds/linux/blob/v6.13-rc1/drivers/misc/pci_endpoint_test.c#L293-L294 These first 4 bytes are stored in the magic register. I do agree that the magic register name is slightly misleading, but that seems completely unrelated to this patch. If you can come up with a better name, send a patch and you shall have my Reviewed-by tag :) Kind regards, Niklas
On Tue, Dec 03, 2024 at 05:43:10AM +0100, Niklas Cassel wrote: > Hello Mani, > > On Sat, Nov 30, 2024 at 01:42:45PM +0530, Manivannan Sadhasivam wrote: > > > > > > struct pci_epf_test { > > > @@ -74,6 +76,7 @@ struct pci_epf_test_reg { > > > u32 irq_type; > > > u32 irq_number; > > > u32 flags; > > > + u32 caps; > > > > Can we rename the 'magic' register? It is not used since the beginning and I > > don't know if we will ever have a usecase for it. > > It is actually used! > > When doing PCITEST_BAR (pci_endpoint_test_bar()), > and barno == test->test_reg_bar, we are only filling the first 4 bytes, > rather than filling the whole BAR: > https://github.com/torvalds/linux/blob/v6.13-rc1/drivers/misc/pci_endpoint_test.c#L293-L294 > > These first 4 bytes are stored in the magic register. > heh, not so evident... thanks anyway. > I do agree that the magic register name is slightly misleading, but that > seems completely unrelated to this patch. > > If you can come up with a better name, send a patch and you shall have > my Reviewed-by tag :) > How about 'scratchpad'? - Mani
On Sun, Dec 08, 2024 at 06:12:08PM +0530, Manivannan Sadhasivam wrote: > On Tue, Dec 03, 2024 at 05:43:10AM +0100, Niklas Cassel wrote: > > Hello Mani, > > > > On Sat, Nov 30, 2024 at 01:42:45PM +0530, Manivannan Sadhasivam wrote: > > > > > > > > struct pci_epf_test { > > > > @@ -74,6 +76,7 @@ struct pci_epf_test_reg { > > > > u32 irq_type; > > > > u32 irq_number; > > > > u32 flags; > > > > + u32 caps; > > > > > > Can we rename the 'magic' register? It is not used since the beginning and I > > > don't know if we will ever have a usecase for it. > > > > It is actually used! > > > > When doing PCITEST_BAR (pci_endpoint_test_bar()), > > and barno == test->test_reg_bar, we are only filling the first 4 bytes, > > rather than filling the whole BAR: > > https://github.com/torvalds/linux/blob/v6.13-rc1/drivers/misc/pci_endpoint_test.c#L293-L294 > > > > These first 4 bytes are stored in the magic register. > > > > heh, not so evident... thanks anyway. > > > I do agree that the magic register name is slightly misleading, but that > > seems completely unrelated to this patch. > > > > If you can come up with a better name, send a patch and you shall have > > my Reviewed-by tag :) > > > > How about 'scratchpad'? Sounds good to me. When sending a patch, don't forget to update: Documentation/PCI/endpoint/pci-test-function.rst: 1) PCI_ENDPOINT_TEST_MAGIC Documentation/PCI/endpoint/pci-test-function.rst:* PCI_ENDPOINT_TEST_MAGIC drivers/misc/pci_endpoint_test.c:#define PCI_ENDPOINT_TEST_MAGIC in addition to renaming the struct member in struct pci_epf_test_reg. Kind regards, Niklas
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;
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). Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/pci/endpoint/functions/pci-epf-test.c | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)