[Qemu-devel] [PATCH 0/7] serial: add serial_chr_nonnull()

[Qemu-devel] [PATCH 0/7] serial: add serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Hi,

This series add the serial_chr_nonnull() which connect to the "null" chardev
backend if none is provided.

Inspired by Peter's suggestion:
http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05987.html
which also refers to this issue:
http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg03546.html

Some boards still check serial_hds[x] non null before calling serial_mm_init(),
this check could now be removed on the SoC which always have an UART mapped.

This might be a follow up if this series is accepted.

Regards,

Phil.

Philippe Mathieu-Daudé (7):
  serial: add serial_chr_nonnull() to use the null backend when none provided
  serial: use serial_chr_nonnull() in serial_mm_init()
  hw/arm/fsl_imx*: use serial_chr_nonnull()
  hw/mips/malta: use serial_chr_nonnull()
  hw/char/exynos4210_uart: use serial_chr_nonnull()
  hw/char/omap_uart: serial_mm_init() already check for null chr
  hw/xtensa: serial_mm_init() already check for null chr

 include/hw/char/imx_serial.h |  1 +
 include/hw/char/serial.h     |  1 +
 hw/arm/fsl-imx25.c           |  9 +--------
 hw/arm/fsl-imx31.c           |  9 +--------
 hw/arm/fsl-imx6.c            | 10 +---------
 hw/char/exynos4210_uart.c    | 14 ++------------
 hw/char/omap_uart.c          |  6 ++----
 hw/char/serial.c             | 15 ++++++++++++++-
 hw/mips/mips_malta.c         |  6 +-----
 hw/xtensa/xtfpga.c           |  4 ----
 10 files changed, 24 insertions(+), 51 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Philippe Mathieu-Daudé]

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/serial.h |  1 +
 hw/char/serial.c         | 13 +++++++++++++
 2 files changed, 14 insertions(+)

diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
index c4daf11a14..96bccb39e1 100644
--- a/include/hw/char/serial.h
+++ b/include/hw/char/serial.h
@@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
                             hwaddr base, int it_shift,
                             qemu_irq irq, int baudbase,
                             Chardev *chr, enum device_endian end);
+Chardev *serial_chr_nonnull(Chardev *chr);
 
 /* serial-isa.c */
 #define TYPE_ISA_SERIAL "isa-serial"
diff --git a/hw/char/serial.c b/hw/char/serial.c
index 9aec6c60d8..7a100db107 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
     },
 };
 
+Chardev *serial_chr_nonnull(Chardev *chr)
+{
+    static int serial_id;
+    char *label;
+
+    label = g_strdup_printf("discarding-serial%d", serial_id++);
+    chr = qemu_chr_new(label, "null");
+    assert(chr);
+    g_free(label);
+
+    return chr;
+}
+
 SerialState *serial_mm_init(MemoryRegion *address_space,
                             hwaddr base, int it_shift,
                             qemu_irq irq, int baudbase,
-- 
2.14.1

[Qemu-devel] [PATCH 2/7] serial: use serial_chr_nonnull() in serial_mm_init()

[Philippe Mathieu-Daudé]

Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/serial.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/char/serial.c b/hw/char/serial.c
index e4c38dc250..57e89468b4 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -1050,7 +1050,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
     s->it_shift = it_shift;
     s->irq = irq;
     s->baudbase = baudbase;
-    qemu_chr_fe_init(&s->chr, chr, &error_abort);
+    qemu_chr_fe_init(&s->chr, serial_chr_nonnull(chr), &error_abort);
 
     serial_realize_core(s, &error_fatal);
     vmstate_register(NULL, base, &vmstate_serial, s);
-- 
2.14.1

[Qemu-devel] [PATCH 3/7] hw/arm/fsl_imx*: use serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/char/imx_serial.h |  1 +
 hw/arm/fsl-imx25.c           |  9 +--------
 hw/arm/fsl-imx31.c           |  9 +--------
 hw/arm/fsl-imx6.c            | 10 +---------
 4 files changed, 4 insertions(+), 25 deletions(-)

diff --git a/include/hw/char/imx_serial.h b/include/hw/char/imx_serial.h
index baeec3183f..55139dc6ec 100644
--- a/include/hw/char/imx_serial.h
+++ b/include/hw/char/imx_serial.h
@@ -20,6 +20,7 @@
 
 #include "hw/sysbus.h"
 #include "chardev/char-fe.h"
+#include "hw/char/serial.h"
 
 #define TYPE_IMX_SERIAL "imx.serial"
 #define IMX_SERIAL(obj) OBJECT_CHECK(IMXSerialState, (obj), TYPE_IMX_SERIAL)
diff --git a/hw/arm/fsl-imx25.c b/hw/arm/fsl-imx25.c
index 3b97eceb3c..425a9edc36 100644
--- a/hw/arm/fsl-imx25.c
+++ b/hw/arm/fsl-imx25.c
@@ -120,14 +120,7 @@ static void fsl_imx25_realize(DeviceState *dev, Error **errp)
         if (i < MAX_SERIAL_PORTS) {
             Chardev *chr;
 
-            chr = serial_hds[i];
-
-            if (!chr) {
-                char label[20];
-                snprintf(label, sizeof(label), "imx31.uart%d", i);
-                chr = qemu_chr_new(label, "null");
-            }
-
+            chr = serial_chr_nonnull(serial_hds[i]);
             qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
         }
 
diff --git a/hw/arm/fsl-imx31.c b/hw/arm/fsl-imx31.c
index 0f2ebe8161..8d4535a536 100644
--- a/hw/arm/fsl-imx31.c
+++ b/hw/arm/fsl-imx31.c
@@ -109,14 +109,7 @@ static void fsl_imx31_realize(DeviceState *dev, Error **errp)
         if (i < MAX_SERIAL_PORTS) {
             Chardev *chr;
 
-            chr = serial_hds[i];
-
-            if (!chr) {
-                char label[20];
-                snprintf(label, sizeof(label), "imx31.uart%d", i);
-                chr = qemu_chr_new(label, "null");
-            }
-
+            chr = serial_chr_nonnull(serial_hds[i]);
             qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
         }
 
diff --git a/hw/arm/fsl-imx6.c b/hw/arm/fsl-imx6.c
index 26fd214004..7bc1aa1fbe 100644
--- a/hw/arm/fsl-imx6.c
+++ b/hw/arm/fsl-imx6.c
@@ -189,15 +189,7 @@ static void fsl_imx6_realize(DeviceState *dev, Error **errp)
         if (i < MAX_SERIAL_PORTS) {
             Chardev *chr;
 
-            chr = serial_hds[i];
-
-            if (!chr) {
-                char *label = g_strdup_printf("imx6.uart%d", i + 1);
-                chr = qemu_chr_new(label, "null");
-                g_free(label);
-                serial_hds[i] = chr;
-            }
-
+            chr = serial_chr_nonnull(serial_hds[i]);
             qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
         }
 
-- 
2.14.1

[Qemu-devel] [PATCH 4/7] hw/mips/malta: use serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/mips/mips_malta.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c
index af678f5784..8620e9c42c 100644
--- a/hw/mips/mips_malta.c
+++ b/hw/mips/mips_malta.c
@@ -1035,11 +1035,7 @@ void mips_malta_init(MachineState *machine)
 
     /* Make sure the first 3 serial ports are associated with a device. */
     for(i = 0; i < 3; i++) {
-        if (!serial_hds[i]) {
-            char label[32];
-            snprintf(label, sizeof(label), "serial%d", i);
-            serial_hds[i] = qemu_chr_new(label, "null");
-        }
+        serial_hds[i] = serial_chr_nonnull(serial_hds[i]);
     }
 
     /* create CPU */
-- 
2.14.1

[Qemu-devel] [PATCH 5/7] hw/char/exynos4210_uart: use serial_chr_nonnull()

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
This ARRAY_SIZE() first surprised me but was valid :)

 hw/char/exynos4210_uart.c | 14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c
index 3957e78abf..b6cdfc3006 100644
--- a/hw/char/exynos4210_uart.c
+++ b/hw/char/exynos4210_uart.c
@@ -27,6 +27,7 @@
 #include "chardev/char-serial.h"
 
 #include "hw/arm/exynos4210.h"
+#include "hw/char/serial.h"
 
 #undef DEBUG_UART
 #undef DEBUG_UART_EXTEND
@@ -589,9 +590,6 @@ DeviceState *exynos4210_uart_create(hwaddr addr,
     DeviceState  *dev;
     SysBusDevice *bus;
 
-    const char chr_name[] = "serial";
-    char label[ARRAY_SIZE(chr_name) + 1];
-
     dev = qdev_create(NULL, TYPE_EXYNOS4210_UART);
 
     if (!chr) {
@@ -600,15 +598,7 @@ DeviceState *exynos4210_uart_create(hwaddr addr,
                          MAX_SERIAL_PORTS);
             exit(1);
         }
-        chr = serial_hds[channel];
-        if (!chr) {
-            snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel);
-            chr = qemu_chr_new(label, "null");
-            if (!(chr)) {
-                error_report("Can't assign serial port to UART%d", channel);
-                exit(1);
-            }
-        }
+        chr = serial_chr_nonnull(serial_hds[channel]);
     }
 
     qdev_prop_set_chr(dev, "chardev", chr);
-- 
2.14.1

[Qemu-devel] [PATCH 6/7] hw/char/omap_uart: serial_mm_init() already check for null chr

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/char/omap_uart.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c
index 6fd1b9cf6b..1f0ac0a053 100644
--- a/hw/char/omap_uart.c
+++ b/hw/char/omap_uart.c
@@ -63,8 +63,7 @@ struct omap_uart_s *omap_uart_init(hwaddr base,
     s->irq = irq;
     s->serial = serial_mm_init(get_system_memory(), base, 2, irq,
                                omap_clk_getrate(fclk)/16,
-                               chr ?: qemu_chr_new(label, "null"),
-                               DEVICE_NATIVE_ENDIAN);
+                               chr, DEVICE_NATIVE_ENDIAN);
     return s;
 }
 
@@ -183,6 +182,5 @@ void omap_uart_attach(struct omap_uart_s *s, Chardev *chr)
     /* TODO: Should reuse or destroy current s->serial */
     s->serial = serial_mm_init(get_system_memory(), s->base, 2, s->irq,
                                omap_clk_getrate(s->fclk) / 16,
-                               chr ?: qemu_chr_new("null", "null"),
-                               DEVICE_NATIVE_ENDIAN);
+                               chr, DEVICE_NATIVE_ENDIAN);
 }
-- 
2.14.1

[Qemu-devel] [PATCH 7/7] hw/xtensa: serial_mm_init() already check for null chr

[Philippe Mathieu-Daudé]

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/xtensa/xtfpga.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/hw/xtensa/xtfpga.c b/hw/xtensa/xtfpga.c
index 635a4d4ec3..223f396902 100644
--- a/hw/xtensa/xtfpga.c
+++ b/hw/xtensa/xtfpga.c
@@ -263,10 +263,6 @@ static void lx_init(const LxBoardDesc *board, MachineState *machine)
                 xtensa_get_extint(env, 1), nd_table);
     }
 
-    if (!serial_hds[0]) {
-        serial_hds[0] = qemu_chr_new("serial0", "null");
-    }
-
     serial_mm_init(system_io, 0x0d050020, 2, xtensa_get_extint(env, 0),
             115200, serial_hds[0], DEVICE_NATIVE_ENDIAN);
 
-- 
2.14.1

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Thomas Huth]

On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/serial.h |  1 +
>  hw/char/serial.c         | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index c4daf11a14..96bccb39e1 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
>                              hwaddr base, int it_shift,
>                              qemu_irq irq, int baudbase,
>                              Chardev *chr, enum device_endian end);
> +Chardev *serial_chr_nonnull(Chardev *chr);

Why do you need the prototype? Please make the function static if
possible (otherwise please add some rationale in the patch description).

>  /* serial-isa.c */
>  #define TYPE_ISA_SERIAL "isa-serial"
> diff --git a/hw/char/serial.c b/hw/char/serial.c
> index 9aec6c60d8..7a100db107 100644
> --- a/hw/char/serial.c
> +++ b/hw/char/serial.c
> @@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
>      },
>  };
>  
> +Chardev *serial_chr_nonnull(Chardev *chr)
> +{
> +    static int serial_id;
> +    char *label;
> +
> +    label = g_strdup_printf("discarding-serial%d", serial_id++);
> +    chr = qemu_chr_new(label, "null");

That looks wrong - you're ignoring the input parameter and always open
the "null" device? Shouldn't there be a "if (chr) return chr;" in front
of this?

> +    assert(chr);
> +    g_free(label);
> +
> +    return chr;
> +}

 Thomas

PS: I think you should also merge the two patches together, they are
small enough.

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Marc-André Lureau]

Hi

On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com> wrote:

> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> > Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > ---
> >  include/hw/char/serial.h |  1 +
> >  hw/char/serial.c         | 13 +++++++++++++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> > index c4daf11a14..96bccb39e1 100644
> > --- a/include/hw/char/serial.h
> > +++ b/include/hw/char/serial.h
> > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion
> *address_space,
> >                              hwaddr base, int it_shift,
> >                              qemu_irq irq, int baudbase,
> >                              Chardev *chr, enum device_endian end);
> > +Chardev *serial_chr_nonnull(Chardev *chr);
>
> Why do you need the prototype? Please make the function static if
> possible (otherwise please add some rationale in the patch description).
>

It's being used from other units in following patches


> >  /* serial-isa.c */
> >  #define TYPE_ISA_SERIAL "isa-serial"
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 9aec6c60d8..7a100db107 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -1025,6 +1025,19 @@ static const MemoryRegionOps serial_mm_ops[3] = {
> >      },
> >  };
> >
> > +Chardev *serial_chr_nonnull(Chardev *chr)
> > +{
> > +    static int serial_id;
> > +    char *label;
> > +
> > +    label = g_strdup_printf("discarding-serial%d", serial_id++);
> > +    chr = qemu_chr_new(label, "null");
>
> That looks wrong - you're ignoring the input parameter and always open
> the "null" device? Shouldn't there be a "if (chr) return chr;" in front
> of this?
>

I think its missing too.

The function name and usage isn't obvious. I would rather see the caller
use "chr ?: serial_chr_null()" rather than "serial_chr_nonnull(chr)".


-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Thomas Huth]

On 31.08.2017 11:36, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com
> <mailto:thuth@redhat.com>> wrote:
> 
>     On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
>     > Suggested-by: Peter Maydell <peter.maydell@linaro.org
>     <mailto:peter.maydell@linaro.org>>
>     > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>     <mailto:f4bug@amsat.org>>
>     > ---
>     >  include/hw/char/serial.h |  1 +
>     >  hw/char/serial.c         | 13 +++++++++++++
>     >  2 files changed, 14 insertions(+)
>     >
>     > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
>     > index c4daf11a14..96bccb39e1 100644
>     > --- a/include/hw/char/serial.h
>     > +++ b/include/hw/char/serial.h
>     > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion
>     *address_space,
>     >                              hwaddr base, int it_shift,
>     >                              qemu_irq irq, int baudbase,
>     >                              Chardev *chr, enum device_endian end);
>     > +Chardev *serial_chr_nonnull(Chardev *chr);
> 
>     Why do you need the prototype? Please make the function static if
>     possible (otherwise please add some rationale in the patch description).
> 
> It's being used from other units in following patches

Ah, well, right. I was only on CC: in the first two patches, so I missed
the other ones at the first glance. So never mind my comment, the
prototype is fine here.

 Thomas

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Peter Maydell]

On 31 August 2017 at 04:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/char/serial.h |  1 +
>  hw/char/serial.c         | 13 +++++++++++++
>  2 files changed, 14 insertions(+)
>
> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
> index c4daf11a14..96bccb39e1 100644
> --- a/include/hw/char/serial.h
> +++ b/include/hw/char/serial.h
> @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
>                              hwaddr base, int it_shift,
>                              qemu_irq irq, int baudbase,
>                              Chardev *chr, enum device_endian end);
> +Chardev *serial_chr_nonnull(Chardev *chr);

For new globally-visible function declarations, can we
have a doc-comment form comment that documents the
function, please?

thanks
-- PMM

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Philippe Mathieu-Daudé]

On 08/31/2017 07:28 AM, Peter Maydell wrote:
> On 31 August 2017 at 04:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Suggested-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/hw/char/serial.h |  1 +
>>   hw/char/serial.c         | 13 +++++++++++++
>>   2 files changed, 14 insertions(+)
>>
>> diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
>> index c4daf11a14..96bccb39e1 100644
>> --- a/include/hw/char/serial.h
>> +++ b/include/hw/char/serial.h
>> @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion *address_space,
>>                               hwaddr base, int it_shift,
>>                               qemu_irq irq, int baudbase,
>>                               Chardev *chr, enum device_endian end);
>> +Chardev *serial_chr_nonnull(Chardev *chr);
> 
> For new globally-visible function declarations, can we
> have a doc-comment form comment that documents the
> function, please?

I was afraid someone asked that, but since this file has no other 
comment I tried :p

Good to know for future contributions, someone fluent with English 
should add this to CODING_STYLE.

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Philippe Mathieu-Daudé]

On 08/31/2017 02:19 AM, Thomas Huth wrote:
> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
[...]>> +Chardev *serial_chr_nonnull(Chardev *chr)
>> +{
>> +    static int serial_id;
>> +    char *label;
>> +
>> +    label = g_strdup_printf("discarding-serial%d", serial_id++);
>> +    chr = qemu_chr_new(label, "null");
> 
> That looks wrong - you're ignoring the input parameter and always open
> the "null" device? Shouldn't there be a "if (chr) return chr;" in front
> of this?

You right. I had this correct in my first patch when this code was 
embedded, I then failed at extracting as another function :/

> 
>> +    assert(chr);
>> +    g_free(label);
>> +
>> +    return chr;
>> +}
> 
>   Thomas
> 
> PS: I think you should also merge the two patches together, they are
> small enough.

Ok.

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Thomas Huth]

On 31.08.2017 17:20, Philippe Mathieu-Daudé wrote:
> On 08/31/2017 02:19 AM, Thomas Huth wrote:
>> On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
> [...]>> +Chardev *serial_chr_nonnull(Chardev *chr)
>>> +{
>>> +    static int serial_id;
>>> +    char *label;
>>> +
>>> +    label = g_strdup_printf("discarding-serial%d", serial_id++);
>>> +    chr = qemu_chr_new(label, "null");
>>
>> That looks wrong - you're ignoring the input parameter and always open
>> the "null" device? Shouldn't there be a "if (chr) return chr;" in front
>> of this?
> 
> You right. I had this correct in my first patch when this code was
> embedded, I then failed at extracting as another function :/
> 
>>
>>> +    assert(chr);
>>> +    g_free(label);
>>> +
>>> +    return chr;
>>> +}
>>
>>   Thomas
>>
>> PS: I think you should also merge the two patches together, they are
>> small enough.
> 
> Ok.

Well, I wrote that comment about merging the two patches together when I
was thinking that your series consists of only two patches (since I've
only been CC:-ed on the first two patches). So please simply ignore that
suggestion :-)

 Thomas

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Philippe Mathieu-Daudé]

On 08/31/2017 06:43 AM, Thomas Huth wrote:
> On 31.08.2017 11:36, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com
>> <mailto:thuth@redhat.com>> wrote:
>>
>>      On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
>>      > Suggested-by: Peter Maydell <peter.maydell@linaro.org
>>      <mailto:peter.maydell@linaro.org>>
>>      > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>>      <mailto:f4bug@amsat.org>>
>>      > ---
>>      >  include/hw/char/serial.h |  1 +
>>      >  hw/char/serial.c         | 13 +++++++++++++
>>      >  2 files changed, 14 insertions(+)
>>      >
>>      > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
>>      > index c4daf11a14..96bccb39e1 100644
>>      > --- a/include/hw/char/serial.h
>>      > +++ b/include/hw/char/serial.h
>>      > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion
>>      *address_space,
>>      >                              hwaddr base, int it_shift,
>>      >                              qemu_irq irq, int baudbase,
>>      >                              Chardev *chr, enum device_endian end);
>>      > +Chardev *serial_chr_nonnull(Chardev *chr);
>>
>>      Why do you need the prototype? Please make the function static if
>>      possible (otherwise please add some rationale in the patch description).
>>
>> It's being used from other units in following patches
> 
> Ah, well, right. I was only on CC: in the first two patches, so I missed
> the other ones at the first glance. So never mind my comment, the
> prototype is fine here.

Is it better/easier to use the same list for the cover and all the patches?
I try to shorten the it to help overloaded reviewer to focus on the 
patches I think they can help. But this case show it's not that useful.

Re: [Qemu-devel] [PATCH 1/7] serial: add serial_chr_nonnull() to use the null backend when none provided

[Markus Armbruster]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 08/31/2017 06:43 AM, Thomas Huth wrote:
>> On 31.08.2017 11:36, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Thu, Aug 31, 2017 at 7:20 AM Thomas Huth <thuth@redhat.com
>>> <mailto:thuth@redhat.com>> wrote:
>>>
>>>      On 31.08.2017 05:53, Philippe Mathieu-Daudé wrote:
>>>      > Suggested-by: Peter Maydell <peter.maydell@linaro.org
>>>      <mailto:peter.maydell@linaro.org>>
>>>      > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org
>>>      <mailto:f4bug@amsat.org>>
>>>      > ---
>>>      >  include/hw/char/serial.h |  1 +
>>>      >  hw/char/serial.c         | 13 +++++++++++++
>>>      >  2 files changed, 14 insertions(+)
>>>      >
>>>      > diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h
>>>      > index c4daf11a14..96bccb39e1 100644
>>>      > --- a/include/hw/char/serial.h
>>>      > +++ b/include/hw/char/serial.h
>>>      > @@ -93,6 +93,7 @@ SerialState *serial_mm_init(MemoryRegion
>>>      *address_space,
>>>      >                              hwaddr base, int it_shift,
>>>      >                              qemu_irq irq, int baudbase,
>>>      >                              Chardev *chr, enum device_endian end);
>>>      > +Chardev *serial_chr_nonnull(Chardev *chr);
>>>
>>>      Why do you need the prototype? Please make the function static if
>>>      possible (otherwise please add some rationale in the patch description).
>>>
>>> It's being used from other units in following patches
>>
>> Ah, well, right. I was only on CC: in the first two patches, so I missed
>> the other ones at the first glance. So never mind my comment, the
>> prototype is fine here.
>
> Is it better/easier to use the same list for the cover and all the patches?
> I try to shorten the it to help overloaded reviewer to focus on the
> patches I think they can help. But this case show it's not that
> useful.

Clear an unequivocal answer: it depends :)

Re: [Qemu-devel] [PATCH 0/7] serial: add serial_chr_nonnull()

[Peter Maydell]

On 31 August 2017 at 04:52, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Hi,
>
> This series add the serial_chr_nonnull() which connect to the "null" chardev
> backend if none is provided.
>
> Inspired by Peter's suggestion:
> http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg05987.html
> which also refers to this issue:
> http://lists.nongnu.org/archive/html/qemu-devel/2017-08/msg03546.html
>
> Some boards still check serial_hds[x] non null before calling serial_mm_init(),
> this check could now be removed on the SoC which always have an UART mapped.

I think this is definitely an area we need to sort out,
so thanks for sending this patchset. I have two major comments
on it:

(1) I don't think we should be using serial_chr_nonnull() in the
board and SoC code (your patches 3 and 4 here). I think the design
principle we should be aiming for is:
 * devices which accept a Chardev should cope with being passed
   a NULL pointer, and so their users never need to care

serial_mm_init() is basically a device (though it has never been
QOMified), so it is correct that it should change to handle NULL.

Patch 4 (malta) : we should just delete the entire for() loop,
ideally, but this runs into awkwardness because serial_hds_isa_init()
then won't create uart 0 and 1. Maybe leave it alone for the moment.

In patch 5, exynos4210_uart_create() is code that uses a device,
not code that implements it, so it should be able to simplify to
just doing chr = serial_hds[channel]; .
(there is scope for further cleanup here because there are only
4 callsites for this utility function, which really ought to
just always take a Chardev* and not support the 'or pass an
integer channel number' stuff. I'd leave that for a different
patchset though.)

Patches 6 and 7 look OK.


On to point (2): is creating a dummy null chardev the right
way to arrange for devices to handle a NULL chardev pointer?
Most of the qemu_chr_fe_*() functions have checks for
"if we are passed a CharBackend with a NULL Chardev pointer
do nothing", which suggests that the design intent here is
that char devices should be able to just work without having
to special case NULL. If that can generally be done without
char device authors having to think too hard then I feel
like we should continue with that approach. It's only
if it looks like that's too error-prone that the "create
a dummy null chardev" approach makes sense, I think (and in
that case maybe we can drop all those NULL pointer checks
in the fe functions?)

What is going wrong in the serial_mm_init() case that's
causing it to fail when the chardev pointer is NULL ?

thanks
-- PMM