Re: [RFC PATCH 3/8] usb/uhci: Move PCI-related code into a separate file

[Cédric Le Goater]

On 11/12/24 15:50, Guenter Roeck wrote:
> Hi Thomas,
>
> On 11/11/24 22:32, Thomas Huth wrote:
> > On 06/09/2024 14.25, Guenter Roeck wrote:
> > > Some machines (like Aspeed ARM) only have a sysbus UHCI controller.
> > > The current UHCI implementation only supports PCI based UHCI controllers.
> > > Move the UHCI-PCI device code into a separate file so that it is possible
> > > to create a sysbus UHCI device without PCI dependency.
> >
> >   Hi Guenter,
> >
> > I think there's a bug in here ...
> >
> > > diff --git a/hw/usb/vt82c686-uhci-pci.c b/hw/usb/vt82c686-uhci-pci.c
> > > index 6162806172..7fdb4697f9 100644
> > > --- a/hw/usb/vt82c686-uhci-pci.c
> > > +++ b/hw/usb/vt82c686-uhci-pci.c
> > > @@ -1,17 +1,17 @@
> > >   #include "qemu/osdep.h"
> > >   #include "hw/irq.h"
> > >   #include "hw/isa/vt82c686.h"
> > > -#include "hcd-uhci.h"
> > > +#include "hcd-uhci-pci.h"
> > >   static void uhci_isa_set_irq(void *opaque, int irq_num, int level)
> > >   {
> > > -    UHCIState *s = opaque;
> > > +    UHCIPCIState *s = opaque;
> >
> > You use UHCIPCIState as parameter for the irq-related functions...
> >
> > >       via_isa_set_irq(&s->dev, 0, level);
> > >   }
> > >   static void usb_uhci_vt82c686b_realize(PCIDevice *dev, Error **errp)
> > >   {
> > > -    UHCIState *s = UHCI(dev);
> > > +    UHCIPCIState *s = UHCI_PCI(dev);
> > >       uint8_t *pci_conf = s->dev.config;
> > >       /* USB misc control 1/2 */
> > > @@ -21,12 +21,12 @@ static void usb_uhci_vt82c686b_realize(PCIDevice *dev, 
> > > Error **errp)
> > >       /* USB legacy support  */
> > >       pci_set_long(pci_conf + 0xc0, 0x00002000);
> > > -    usb_uhci_common_realize(dev, errp);
> > > -    object_unref(s->irq);
> > > -    s->irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
> > > +    usb_uhci_common_realize_pci(dev, errp);
> > > +    object_unref(s->state.irq);
> > > +    s->state.irq = qemu_allocate_irq(uhci_isa_set_irq, &s->state, 0);
> >
> > ... but you set it up with UHCIInfo as parameter here. I think this line should 
> > rather be:
> >
> >      s->state.irq = qemu_allocate_irq(uhci_isa_set_irq, s, 0);
> >
> > Shouldn't it?
>
> Yes, you are correct. Thanks for reporting it. Turns out I had fixed that in the
> meantime, but did not remember to send another version. Is there any interest,
> and should I resend ?

Please do. I was planning on merging the obvious ones in QEMU 10.0
and spend more time reviewing the others.

Thanks,

C.