Re: [Qemu-devel] [PATCH v11 02/19] i.MX: Move serial initialization to init/realize of DeviceClass.

[Jean-Christophe DUBOIS]

Le 08/07/2015 22:49, Peter Crosthwaite a écrit :
> On Wed, Jul 8, 2015 at 11:42 AM, Jean-Christophe Dubois
> <address@hidden> wrote:
> > Move constructor to DeviceClass methods
> >   * imx_serial_init
> >   * imx_serial_realize
> >
> > imx32_serial_properties is renamed to imx_serial_properties.
> >
> > Rework of Qdev construction helper function.
> >
> > Signed-off-by: Jean-Christophe Dubois <address@hidden>
> > ---
> >
> > Changes since v1:
> >      * not present on v1
> >
> > Changes since v2:
> >      * not present on v2
> >
> > Changes since v3:
> >      * not present on v3
> >
> > Changes since v4:
> >      * not present on v4
> >
> > Changes since v5:
> >      * not present on v5
> >
> > Changes since v6:
> >      * not present on v6
> >
> > Changes since v7:
> >      * not present on v7
> >
> > Changes since v8:
> >      * Remove Qdev construction helper
> >
> > Changes since v9:
> >      * Qdev construction helper is reintegrated and moved to a header file
> >        as an inline function.
> >
> > Changes since v10:
> >      * Qdev construction helper is put back in the main file.
> >      * Qdev construction helper is reworked
> >      * We don't use qemu_char_get_next_serial() anymore but the chardev
> >        property instead.
> >      * Fix code to work with an unitialized (null) chardev property
> >
> >   hw/char/imx_serial.c | 98 +++++++++++++++++++++++++++++-----------------------
> >   1 file changed, 54 insertions(+), 44 deletions(-)
> >
> > diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> > index 1dcb325..950c740 100644
> > --- a/hw/char/imx_serial.c
> > +++ b/hw/char/imx_serial.c
> > @@ -38,13 +38,13 @@ do { printf("imx_serial: " fmt , ##args); } while (0)
> >   //#define DEBUG_IMPLEMENTATION 1
> >   #ifdef DEBUG_IMPLEMENTATION
> >   #  define IPRINTF(fmt, args...) \
> > -    do  { fprintf(stderr, "imx_serial: " fmt, ##args); } while (0)
> > +    do  { fprintf(stderr, "%s: " fmt, TYPE_IMX_SERIAL, ##args); } while (0)
> >   #else
> >   #  define IPRINTF(fmt, args...) do {} while (0)
> >   #endif
> >
> >   static const VMStateDescription vmstate_imx_serial = {
> > -    .name = "imx-serial",
> > +    .name = TYPE_IMX_SERIAL,
> >       .version_id = 1,
> >       .minimum_version_id = 1,
> >       .fields = (VMStateField[]) {
> > @@ -125,7 +125,9 @@ static uint64_t imx_serial_read(void *opaque, hwaddr offset,
> >               s->usr2 &= ~USR2_RDR;
> >               s->uts1 |= UTS1_RXEMPTY;
> >               imx_update(s);
> > -            qemu_chr_accept_input(s->chr);
> > +            if (s->chr) {
> > +                qemu_chr_accept_input(s->chr);
> > +            }
> >           }
> >           return c;
> >
> > @@ -212,7 +214,9 @@ static void imx_serial_write(void *opaque, hwaddr offset,
> >           }
> >           if (value & UCR2_RXEN) {
> >               if (!(s->ucr2 & UCR2_RXEN)) {
> > -                qemu_chr_accept_input(s->chr);
> > +                if (s->chr) {
> > +                    qemu_chr_accept_input(s->chr);
> > +                }
> Looking around, imx serial is trying to be NULL safe except for these
> two cases. This and the hunk above is a full-on bugfix that should
> probably be split off and go to 2.4.
How do you provide a patch specifically for 2.4 (beside including it in 
my series)?

> >               }
> >           }
> >           s->ucr2 = value & 0xffff;
> > @@ -299,23 +303,16 @@ static void imx_event(void *opaque, int event)
> >       }
> >   }
> >
> > -
> Out of scope style change.
>
> >   static const struct MemoryRegionOps imx_serial_ops = {
> >       .read = imx_serial_read,
> >       .write = imx_serial_write,
> >       .endianness = DEVICE_NATIVE_ENDIAN,
> >   };
> >
> > -static int imx_serial_init(SysBusDevice *dev)
> > +static void imx_serial_realize(DeviceState *dev, Error **errp)
> >   {
> >       IMXSerialState *s = IMX_SERIAL(dev);
> >
> > -
> > -    memory_region_init_io(&s->iomem, OBJECT(s), &imx_serial_ops, s,
> > -                          "imx-serial", 0x1000);
> > -    sysbus_init_mmio(dev, &s->iomem);
> > -    sysbus_init_irq(dev, &s->irq);
> > -
> >       if (s->chr) {
> >           qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive,
> >                                 imx_event, s);
> > @@ -323,45 +320,58 @@ static int imx_serial_init(SysBusDevice *dev)
> >           DPRINTF("No char dev for uart at 0x%lx\n",
> >                   (unsigned long)s->iomem.ram_addr);
> >       }
> > +}
> > +
> > +static void imx_serial_init(Object *obj)
> > +{
> > +    SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > +    IMXSerialState *s = IMX_SERIAL(obj);
> >
> > -    return 0;
> > +    memory_region_init_io(&s->iomem, obj, &imx_serial_ops, s,
> > +                          TYPE_IMX_SERIAL, 0x1000);
> > +    sysbus_init_mmio(sbd, &s->iomem);
> > +    sysbus_init_irq(sbd, &s->irq);
> >   }
> >
> >   void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq)
> >   {
> >       DeviceState *dev;
> > -    SysBusDevice *bus;
> > -    CharDriverState *chr;
> > -    const char chr_name[] = "serial";
> > -    char label[ARRAY_SIZE(chr_name) + 1];
> >
> >       dev = qdev_create(NULL, TYPE_IMX_SERIAL);
> >
> > -    if (uart >= MAX_SERIAL_PORTS) {
> > -        hw_error("Cannot assign uart %d: QEMU supports only %d ports\n",
> > -                 uart, MAX_SERIAL_PORTS);
> > -    }
> > -    chr = serial_hds[uart];
> > -    if (!chr) {
> > -        snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, uart);
> > -        chr = qemu_chr_new(label, "null", NULL);
> > -        if (!(chr)) {
> > -            hw_error("Can't assign serial port to imx-uart%d.\n", uart);
> > +    if (dev) {
> If dev is NULL I think you have big problems. If there is a valid
> reason why this can be NULL in a non-error case then it should just
> short return.

It is obviously an error case (out of resource?) and therefore we should 
not ignore it (as it was previously the case). So I test for it here. Do 
you feel there should be another behavior (hw_error or something)?

> if (!dev) {
>      return;
> }
>
> > +        SysBusDevice *bus;
> > +
> > +        if (uart < MAX_SERIAL_PORTS) {
> > +            CharDriverState *chr;
> > +
> > +            chr = serial_hds[uart];
> > +
> > +            if (!chr) {
> > +                char label[20];
> > +                snprintf(label, sizeof(label), "imx.uart%d", uart);
> > +                chr = qemu_chr_new(label, "null", NULL);
> > +                if (!(chr)) {
> > +                    hw_error("Can't assign serial port to %s.\n", label);
> > +                }
> > +            }
> > +
> > +            qdev_prop_set_chr(dev, "chardev", chr);
> >           }
> > -    }
> >
> > -    qdev_prop_set_chr(dev, "chardev", chr);
> > -    bus = SYS_BUS_DEVICE(dev);
> > -    qdev_init_nofail(dev);
> > -    if (addr != (hwaddr)-1) {
> > -        sysbus_mmio_map(bus, 0, addr);
> > -    }
> > -    sysbus_connect_irq(bus, 0, irq);
> > +        bus = SYS_BUS_DEVICE(dev);
> >
> > -}
> > +        qdev_init_nofail(dev);
> > +
> > +        if (addr != (hwaddr)-1) {
> > +            sysbus_mmio_map(bus, 0, addr);
> > +        }
> >
> > +        sysbus_connect_irq(bus, 0, irq);
> > +    }
> > +}
> So the change to _create looks correct, but it is still out of scope
> of the patch which by commit message is an init/realize conversion. Is
> there a reason the old _create implementation wont work with the new
> init/realize and allow a split?
>
> Aside from the minor comments the code is good, but I would split this
> to 3 patches.
>
> 1: chr NULL guards, and send that as a single for 2.4.
> 2: realize and init conversion
> 3: _create rewrite.

OK, I'll do it even if I am spending quite some time on something that 
is going to be wiped out in a following patch.

> Regards,
> Peter
>
> > -static Property imx32_serial_properties[] = {
> > +static Property imx_serial_properties[] = {
> >       DEFINE_PROP_CHR("chardev", IMXSerialState, chr),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> > @@ -369,21 +379,21 @@ static Property imx32_serial_properties[] = {
> >   static void imx_serial_class_init(ObjectClass *klass, void *data)
> >   {
> >       DeviceClass *dc = DEVICE_CLASS(klass);
> > -    SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
> >
> > -    k->init = imx_serial_init;
> > +    dc->realize = imx_serial_realize;
> >       dc->vmsd = &vmstate_imx_serial;
> >       dc->reset = imx_serial_reset_at_boot;
> >       set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
> >       dc->desc = "i.MX series UART";
> > -    dc->props = imx32_serial_properties;
> > +    dc->props = imx_serial_properties;
> >   }
> >
> >   static const TypeInfo imx_serial_info = {
> > -    .name = TYPE_IMX_SERIAL,
> > -    .parent = TYPE_SYS_BUS_DEVICE,
> > -    .instance_size = sizeof(IMXSerialState),
> > -    .class_init = imx_serial_class_init,
> > +    .name           = TYPE_IMX_SERIAL,
> > +    .parent         = TYPE_SYS_BUS_DEVICE,
> > +    .instance_size  = sizeof(IMXSerialState),
> > +    .instance_init  = imx_serial_init,
> > +    .class_init     = imx_serial_class_init,
> >   };
> >
> >   static void imx_serial_register_types(void)
> > --
> > 2.1.4