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
