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

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