Re: [PATCH v3 01/16] cxl/pci: Fix CDAT retrieval on big endian

[Alexey Kardashevskiy <aik@amd.com>]

On 28/2/23 19:24, Lukas Wunner wrote:
> On Tue, Feb 28, 2023 at 01:53:46PM +1100, Alexey Kardashevskiy wrote:
>> On 11/2/23 07:25, Lukas Wunner wrote:
>>> @@ -493,8 +493,8 @@ static void cxl_doe_task_complete(struct pci_doe_task *task)
>>>    }
>>>    struct cdat_doe_task {
>>> -	u32 request_pl;
>>> -	u32 response_pl[32];
>>> +	__le32 request_pl;
>>> +	__le32 response_pl[32];
>>
>> This is ok as it is a binary format of DOE message (is it?)...
> 
> Yes, the DOE request and response is an opaque byte stream.
> 
> The above-quoted type changes are necessary to avoid sparse warnings
> (as noted in the commit message).
> 
> 
>>> --- a/drivers/pci/doe.c
>>> +++ b/drivers/pci/doe.c
>>> @@ -143,7 +143,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
>>>    					  length));
>>>    	for (i = 0; i < task->request_pl_sz / sizeof(u32); i++)
>>>    		pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
>>> -				       task->request_pl[i]);
>>> +				       le32_to_cpu(task->request_pl[i]));
>>
>> Does it really work on BE? My little brain explodes on all these convertions
>>
>> char buf[] = { 1, 2, 3, 4 }
>> u32 *request_pl = buf;
>>
>> request_pl[0] will be 0x01020304.
>> le32_to_cpu(request_pl[0]) will be 0x04030201
>> And then pci_write_config_dword() will do another swap.
>>
>> Did I miss something? (/me is gone bringing up a BE system).
> 
> Correct.

Uff, ok, your code works correct, sorry for the noise.


> 
>>> @@ -45,9 +49,9 @@ struct pci_doe_mb;
>>>     */
>>>    struct pci_doe_task {
>>>    	struct pci_doe_protocol prot;
>>> -	u32 *request_pl;
>>> +	__le32 *request_pl;
>>>    	size_t request_pl_sz;
>>> -	u32 *response_pl;
>>> +	__le32 *response_pl;
>>
>>
>> This does not look right. Either:
>> - pci_doe() should also take __le32* or
>> - pci_doe() should do cpu_to_le32() for the request, and
>> pci_doe_task_complete() - for the response.
> 
> Again, the type changes are necessary to avoid sparse warnings.

I'd shut it up like this:

===
          for (i = 0; i < task->request_pl_sz / sizeof(__le32); i++)
                 pci_write_config_dword(pdev, offset + PCI_DOE_WRITE,
-                                      le32_to_cpu(task->request_pl[i]));
+                                      le32_to_cpu(*(__le32 
*)&task->request_pl[i]));
===

+ may be a comment that LE is a natural order. Dunno.

Changing types to __le32 without explicit conversions or comments just 
confuses. Thanks,


> pci_doe() takes a void * because patch [15/16] eliminates the need
> for the request and response size to be a multiple of dwords.
> Passing __le32 * to pci_doe() would then no longer be correct.


-- 
Alexey