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@xxxxxxxxxx>
>>> ---
>>> 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
