Message ID | f6812ea2b63a85e7d390e61c758300a13f123772.1436374071.git.jcd@tribudubois.net |
---|---|
State | New |
Headers | show |
On Wed, Jul 8, 2015 at 11:42 AM, Jean-Christophe Dubois <jcd@tribudubois.net> 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 <jcd@tribudubois.net> > --- > > 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. > } > } > 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. 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. 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 > >
Le 08/07/2015 22:49, Peter Crosthwaite a écrit : > On Wed, Jul 8, 2015 at 11:42 AM, Jean-Christophe Dubois > <jcd@tribudubois.net> 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 <jcd@tribudubois.net> >> --- >> >> 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 >> >>
On Wed, Jul 8, 2015 at 10:55 PM, Jean-Christophe DUBOIS <jcd@tribudubois.net> wrote: > Le 08/07/2015 22:49, Peter Crosthwaite a écrit : >> >> On Wed, Jul 8, 2015 at 11:42 AM, Jean-Christophe Dubois >> <jcd@tribudubois.net> 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 <jcd@tribudubois.net> >>> --- >>> >>> 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 >>> >>> > >
sorry accidental send, comments inline below. On Wed, Jul 8, 2015 at 11:36 PM, Peter Crosthwaite <peter.crosthwaite@xilinx.com> wrote: > On Wed, Jul 8, 2015 at 10:55 PM, Jean-Christophe DUBOIS > <jcd@tribudubois.net> wrote: >> Le 08/07/2015 22:49, Peter Crosthwaite a écrit : >>> >>> On Wed, Jul 8, 2015 at 11:42 AM, Jean-Christophe Dubois >>> <jcd@tribudubois.net> 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 <jcd@tribudubois.net> >>>> --- >>>> >>>> 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)? >> Split it off as a single and send it alone, --subject-prefix="PATCH for-2.4" argument to git send-email. >> >>> >>>> } >>>> } >>>> 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. >> Ok but why? Does an unchanged _create work? Note existing bugs are ok, you don't faciliate any of your new functionalities or fix existing issues (e.g. becoming NULL safe or deal with missing error paths) you just need to not cause a regression half way through the series. So if the existing _create just works with your realize change (which AFAICT it does) just leave it unchanged and nuke it later in the series. What am I missing that requires the change to _create at all? Regards, Peter >> >>> >>> 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 >>>> >>>> >> >>
Le 09/07/2015 08:41, Peter Crosthwaite a écrit : > sorry accidental send, comments inline below. > > On Wed, Jul 8, 2015 at 11:36 PM, Peter Crosthwaite > <peter.crosthwaite@xilinx.com> wrote: >> On Wed, Jul 8, 2015 at 10:55 PM, Jean-Christophe DUBOIS >> <jcd@tribudubois.net> wrote: >>> Le 08/07/2015 22:49, Peter Crosthwaite a écrit : >>>> On Wed, Jul 8, 2015 at 11:42 AM, Jean-Christophe Dubois >>>> <jcd@tribudubois.net> 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 <jcd@tribudubois.net> >>>>> --- >>>>> >>>>> 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)? >>> > Split it off as a single and send it alone, --subject-prefix="PATCH > for-2.4" argument to git send-email. > >>>>> } >>>>> } >>>>> 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. >>> > Ok but why? Does an unchanged _create work? Note existing bugs are ok, > you don't faciliate any of your new functionalities or fix existing > issues (e.g. becoming NULL safe or deal with missing error paths) you > just need to not cause a regression half way through the series. So if > the existing _create just works with your realize change (which AFAICT > it does) just leave it unchanged and nuke it later in the series. What > am I missing that requires the change to _create at all? It sure does work in the non error case. So I'll remove my "fix" and let it as is until it is nuked when I use the i.MX31 SOC implementation. > > Regards, > Peter > >>>> 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 >>>>> >>>>> >>>
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); + } } } s->ucr2 = value & 0xffff; @@ -299,23 +303,16 @@ static void imx_event(void *opaque, int event) } } - 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) { + 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); + } +} -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)
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 <jcd@tribudubois.net> --- 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(-)