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

[Lukas Wunner <lukas@wunner.de>]

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.


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

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.

Thanks,

Lukas