diff mbox series

[2/2] misc: pci_endpoint_test: Add support for capabilities

Message ID 20241120155730.2833836-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

Commit Message

Niklas Cassel Nov. 20, 2024, 3:57 p.m. UTC
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(+)

Comments

Frank Li Nov. 20, 2024, 4:53 p.m. UTC | #1
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
>
Niklas Cassel Nov. 20, 2024, 5:05 p.m. UTC | #2
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
Frank Li Nov. 20, 2024, 5:12 p.m. UTC | #3
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
Niklas Cassel Nov. 20, 2024, 5:17 p.m. UTC | #4
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
Damien Le Moal Nov. 21, 2024, 2:54 a.m. UTC | #5
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);
Niklas Cassel Nov. 21, 2024, 12:09 p.m. UTC | #6
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
Damien Le Moal Nov. 21, 2024, 12:25 p.m. UTC | #7
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
diff mbox series

Patch

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);