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
