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

[Peter Crosthwaite]

sorry accidental send, comments inline below.

On Wed, Jul 8, 2015 at 11:36 PM, Peter Crosthwaite
<address@hidden> wrote:
> On Wed, Jul 8, 2015 at 10:55 PM, Jean-Christophe DUBOIS
> <address@hidden> wrote:
>> 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)?
>>

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