[PATCH] drivers/usb: Fix building with gcc 10.x

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan 
---
 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);

[BALATON Zoltan]

On Sat, 6 Nov 2021, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan

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);

[Mark Cave-Ayland]

On 08/01/2022 10:59, BALATON Zoltan wrote:
> On Sat, 6 Nov 2021, BALATON Zoltan wrote:
> > Signed-off-by: BALATON Zoltan
>
> 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.

[BALATON Zoltan]

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

[Philippe Mathieu-Daudé]

On Sat, Jan 8, 2022 at 11:59 AM BALATON Zoltan  wrote:
> On Sat, 6 Nov 2021, BALATON Zoltan wrote:
> > Signed-off-by: BALATON Zoltan
>
> 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 -- [e-mail]
> To unsubscribe send an email to [e-mail]

[BALATON Zoltan]

On Sat, 8 Jan 2022, Philippe Mathieu-Daudé wrote:
> On Sat, Jan 8, 2022 at 11:59 AM BALATON Zoltan  wrote:
> > On Sat, 6 Nov 2021, BALATON Zoltan wrote:
> > > Signed-off-by: BALATON Zoltan
> >
> > 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 -- [e-mail]
> > To unsubscribe send an email to [e-mail]

[Mark Cave-Ayland]

On 06/11/2021 16:52, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan
>
> 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.

[BALATON Zoltan]

On Sat, 8 Jan 2022, Mark Cave-Ayland wrote:
> On 06/11/2021 16:52, BALATON Zoltan wrote:
> > Signed-off-by: BALATON Zoltan
> >
> > 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.

[BALATON Zoltan]

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
> > >
> > > 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 -- [e-mail]
> To unsubscribe send an email to [e-mail]

[Mark Cave-Ayland]

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

[Mark Cave-Ayland]

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.