Signed-off-by: BALATON Zoltan balaton@eik.bme.hu --- drivers/usbohci.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/usbohci.c b/drivers/usbohci.c index 774164b..c2c0f78 100644 --- a/drivers/usbohci.c +++ b/drivers/usbohci.c @@ -232,12 +232,12 @@ ohci_init (void *bar)
usb_debug("HCCA addr %p\n", OHCI_INST(controller)->hcca); /* Initialize interrupt table. */ - u32 *const intr_table = OHCI_INST(controller)->hcca->HccaInterruptTable; + ohci_t *const ohci = OHCI_INST(controller); ed_t *const periodic_ed; ofmem_posix_memalign((void **)&periodic_ed, sizeof(ed_t), sizeof(ed_t)); memset((void *)periodic_ed, 0, sizeof(*periodic_ed)); for (i = 0; i < 32; ++i) - intr_table[i] = __cpu_to_le32(virt_to_phys(periodic_ed)); + ohci->hcca->HccaInterruptTable[i] = __cpu_to_le32(virt_to_phys(periodic_ed)); OHCI_INST (controller)->periodic_ed = periodic_ed;
OHCI_INST (controller)->opreg->HcHCCA = __cpu_to_le32(virt_to_phys(OHCI_INST(controller)->hcca)); @@ -607,7 +607,7 @@ struct _intrq_td { u8 *data; struct _intrq_td *next; struct _intr_queue *intrq; -}; +} __attribute__ ((packed));
struct _intr_queue { volatile ed_t ed; @@ -698,14 +698,13 @@ ohci_create_intr_queue(endpoint_t *const ep, const int reqsize, /* Insert ED into periodic table. */ int nothing_placed = 1; ohci_t *const ohci = OHCI_INST(ep->dev->controller); - u32 *const intr_table = ohci->hcca->HccaInterruptTable; const u32 dummy_ptr = __cpu_to_le32(virt_to_phys(ohci->periodic_ed)); for (i = 0; i < 32; i += reqtiming) { /* Advance to the next free position. */ - while ((i < 32) && (intr_table[i] != dummy_ptr)) ++i; + while ((i < 32) && (ohci->hcca->HccaInterruptTable[i] != dummy_ptr)) ++i; if (i < 32) { usb_debug("Placed endpoint %lx to %d\n", virt_to_phys(&intrq->ed), i); - intr_table[i] = __cpu_to_le32(virt_to_phys(&intrq->ed)); + ohci->hcca->HccaInterruptTable[i] = __cpu_to_le32(virt_to_phys(&intrq->ed)); nothing_placed = 0; } } @@ -729,10 +728,9 @@ ohci_destroy_intr_queue(endpoint_t *const ep, void *const q_)
/* Remove interrupt queue from periodic table. */ ohci_t *const ohci = OHCI_INST(ep->dev->controller); - u32 *const intr_table = ohci->hcca->HccaInterruptTable; for (i=0; i < 32; ++i) { - if (intr_table[i] == __cpu_to_le32(virt_to_phys(intrq))) - intr_table[i] = __cpu_to_le32(virt_to_phys(ohci->periodic_ed)); + if (ohci->hcca->HccaInterruptTable[i] == __cpu_to_le32(virt_to_phys(intrq))) + ohci->hcca->HccaInterruptTable[i] = __cpu_to_le32(virt_to_phys(ohci->periodic_ed)); } /* Wait for frame to finish. */ mdelay(1);
On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
No problem, I just pinged so it's not forgotten.
Regards, BALATON Zoltan
On Sat, Jan 8, 2022 at 11:59 AM BALATON Zoltan balaton@eik.bme.hu wrote:
What is the GCC error? Unsafe alignment for packed hcca_t? I.e:
warning: taking address of packed member of ‘struct hcca_t’ may result in an unaligned pointer value [-Waddress-of-packed-member]
Why packing this one?
On Sat, 8 Jan 2022, Philippe Mathieu-Daudé wrote:
Yes, mostly, here are the errors I get with git master:
drivers/usbohci.c: In function ‘ohci_init’: drivers/usbohci_private.h:259:31: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member] 259 | #define OHCI_INST(controller) ((ohci_t*)((controller)->instance)) | ^ drivers/usbohci.c:235:26: note: in expansion of macro ‘OHCI_INST’ 235 | u32 *const intr_table = OHCI_INST(controller)->hcca->HccaInterruptTable; | ^~~~~~~~~ drivers/usbohci.c: In function ‘ohci_create_intr_queue’: drivers/usbohci.c:701:26: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=adddess-of-packed-member] 701 | u32 *const intr_table = ohci->hcca->HccaInterruptTable; | ^~~~ drivers/usbohci.c: In function ‘ohci_destroy_intr_queue’: drivers/usbohci.c:732:26: error: taking address of packed member of ‘struct <anonymous>’ may result in an unaligned pointer value [-Werror=address-of-packed-member] 732 | u32 *const intr_table = ohci->hcca->HccaInterruptTable; | ^~~~ drivers/usbohci.c: In function ‘ohci_process_done_queue’: drivers/usbohci.c:838:4: error: converting a packed ‘td_t’ pointer (alignment 1) to a ‘intrq_td_t’ {aka ‘struct _intrq_td’} pointer (alignment 4) may result in an unaligned pointer value [-Werror=address-of-packed-member] 838 | intrq_td_t *const td = INTRQ_TD_FROM_TD(done_td); | ^~~~~~~~~~ drivers/usbohci.c:605:8: note: defined here 605 | struct _intrq_td { | ^~~~~~~~~ cc1: all warnings being treated as errors
To fix the last error above I think, I forgot about this by now so not sure about the details any more.
Regards, BALATON Zoltan
On 06/11/2021 16:52, BALATON Zoltan wrote:
The changes to intr_table appear to make things less readable to me. Presumably the error is that the existing code is attempting to write through a const pointer? In which case I think simply dropping the const from intr_table both here and below should fix the issue, since it is clearly intended to be used for writing.
This looks like a different fix that should be in a separate patch (wasn't there a similar patch for QEMU recently?).
ATB,
Mark.
On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
Yes but I could not find a simpler way to fix this easily.
Presumably the error is that the existing code is attempting to write through a const pointer? In which case I think simply dropping the const from
No, it's about possible unalligned access, see my answer to Philippe.
intr_table both here and below should fix the issue, since it is clearly intended to be used for writing.
I don't think that would fix the problem.
It's also fixing a build issue with gcc 10 and the problem is similar so I've put it in one patch but feel free to split it up to as many patches as you like or change it or fix the problem in an alternative way. I'm OK with you taking this patch and changing it or doing it differently (you have my S-o-b, just add your maintainer comment and S-o-b after that if you change the patch) but I don't have time to make changes to this now.
Regards, BALATON Zoltan
On Sat, 8 Jan 2022, BALATON Zoltan wrote:
Maybe this wasn't clear enough so I try again to explain. This is only fixing the gcc 10 error, it's not related to any QEMU change. If there was a similar change in QEMU then that's unrelated, I don't remember any change in QEMU that needed this fix apart from the error from gcc which is solely within openbios code so I think it belongs to this patch but as I said I'm OK with you commiting it in a different patch as well.
Regards, BALATON Zoltan
On 08/01/2022 17:28, BALATON Zoltan wrote:
Ah I see, I hadn't spotted the warnings in the reply (I think our replies must have crossed on the mailing list).
So indeed it is related to unaligned accesses. A bit of research reveals that the issue is caused because when a struct is marked as packed, gcc doesn't guarantee that even the first member of the struct is aligned which is something I was not aware of, and was clearly an assumption made by the existing code.
The outcome of this is that the struct access must be done using a -> reference since presumably if the compiler decides to align the struct arbitrarily, it can emit code to correctly handle unaligned reads/writes.
And due to the ast this struct has the same issue which is why it needs to packed too.
No, that all makes sense in the context of the warnings. I'll give it a quick test and if the USB mouse/keyboard seem okay on mac99 then I'll push it to master. If you find a similar issue in future, please do include the generated warnings in the commit message as it makes patches much easier to review.
ATB,
Mark.
On 15/01/2022 11:53, Mark Cave-Ayland wrote:
All seems okay here, so I've pushed to master.
ATB,
Mark.
-
BALATON Zoltan -
Mark Cave-Ayland -
Philippe Mathieu-Daudé