qemu-devel
[Top][All Lists]
Advanced
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Jean-Christophe DUBOIS
Subject: Re: [Qemu-devel] [PATCH v11 02/19] i.MX: Move serial initialization to init/realize of DeviceClass.
Date: Fri, 10 Jul 2015 21:41:06 +0200
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.7.0 
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
<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?


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
reply via email to
[Prev in Thread] Current Thread [Next in Thread]