[PATCH] drivers/usb: Fix building with gcc 10.x
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); -- 2.30.2
On Sat, 6 Nov 2021, BALATON Zoltan wrote:
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Ping?
--- 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 08/01/2022 10:59, BALATON Zoltan wrote:
On Sat, 6 Nov 2021, BALATON Zoltan wrote:
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Ping?
Hi Zoltan, I've been busier than normal over the holidays but I will take a quick look now.
--- 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);
ATB, Mark.
On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
On 08/01/2022 10:59, BALATON Zoltan wrote:
On Sat, 6 Nov 2021, BALATON Zoltan wrote:
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Ping?
Hi Zoltan,
I've been busier than normal over the holidays but I will take a quick look now.
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:
On Sat, 6 Nov 2021, BALATON Zoltan wrote:
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Ping?
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]
--- 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));
Why packing this one?
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);
_______________________________________________ OpenBIOS mailing list -- openbios@openbios.org To unsubscribe send an email to openbios-leave@openbios.org
On Sat, 8 Jan 2022, Philippe Mathieu-Daudé wrote:
On Sat, Jan 8, 2022 at 11:59 AM BALATON Zoltan <balaton@eik.bme.hu> wrote:
On Sat, 6 Nov 2021, BALATON Zoltan wrote:
Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Ping?
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]
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
--- 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));
Why packing this one?
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
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);
_______________________________________________ OpenBIOS mailing list -- openbios@openbios.org To unsubscribe send an email to openbios-leave@openbios.org
On 06/11/2021 16:52, BALATON Zoltan wrote:
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;
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.
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));
This looks like a different fix that should be in a separate patch (wasn't there a similar patch for QEMU recently?).
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);
ATB, Mark.
On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
On 06/11/2021 16:52, BALATON Zoltan wrote:
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;
The changes to intr_table appear to make things less readable to me.
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.
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));
This looks like a different fix that should be in a separate patch (wasn't there a similar patch for QEMU recently?).
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
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);
ATB,
Mark.
On Sat, 8 Jan 2022, BALATON Zoltan wrote:
On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
On 06/11/2021 16:52, BALATON Zoltan wrote:
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;
The changes to intr_table appear to make things less readable to me.
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.
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));
This looks like a different fix that should be in a separate patch (wasn't there a similar patch for QEMU recently?).
It's also fixing a build issue with gcc 10 and the problem is similar so I've
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
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
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);
ATB,
Mark.
_______________________________________________ OpenBIOS mailing list -- openbios@openbios.org To unsubscribe send an email to openbios-leave@openbios.org
On 08/01/2022 17:28, BALATON Zoltan wrote:
On Sat, 8 Jan 2022, BALATON Zoltan wrote:
On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
On 06/11/2021 16:52, BALATON Zoltan wrote:
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;
The changes to intr_table appear to make things less readable to me.
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.
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.
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));
This looks like a different fix that should be in a separate patch (wasn't there a similar patch for QEMU recently?).
And due to the ast this struct has the same issue which is why it needs to packed too.
It's also fixing a build issue with gcc 10 and the problem is similar so I've
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.
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:
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.
All seems okay here, so I've pushed to master. ATB, Mark.
participants (3)
-
BALATON Zoltan -
Mark Cave-Ayland -
Philippe Mathieu-Daudé