Re: [Qemu-devel] [PATCH v2 for-2.4] i.MX: Fix UART driver to work with unitialized "chardev" device

[Markus Armbruster]

Jean-Christophe Dubois <address@hidden> writes:

> The "chardev" property initialization might have failed (for example because
> there are not enough chardevs provided by QEMU).
>
> The serial device emulator need to be able to work with an uninitialized
> (NULL) chardev device pointer.
>
> This patch add some missing tests on the chr pointer value before
> using it.
>
> Signed-off-by: Jean-Christophe Dubois <address@hidden>
> Reviewed-by: Peter Crosthwaite <address@hidden>
> ---
>
> Changes since V1:
>  * Grammar sweep on patch description
>  * Added Peter's "Reviewed-by"
>
>  hw/char/imx_serial.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
> index f3fbc77..383e50c 100644
> --- a/hw/char/imx_serial.c
> +++ b/hw/char/imx_serial.c
> @@ -203,7 +203,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;
>  
> @@ -290,7 +292,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;

I'm afraid this approach is inconsistent with existing practice, namely
that if a device requires a backend, and the backend is missing, the
realize() method fails.

Example: serial_realize_core() in hw/char/serial.c, used by
serial_pci_realize() for device "pci-serial", multi_serial_pci_realize()
for devices "pci-serial-2x" and "pci-serial-4x", serial_isa_realizefn()
for device "isa-serial".  The latter has a bug: when the backend is
missing, realize fails, but the actual device is created anyway.

Example: virtio_blk_device_realize() in hw/block/virtio-blk.c.

Note that NICs don't require a backend.  I guess they behave like "no
carrier" then.  net_check_clients() warns "has no peer", however.

Character devices without a backend could be changed not to require a
backend, and behave as if they had a null backend then.  Whether that's
a good user interface needs some thought.  Regardless, it should be
consistent: either all character devices do, or none.

If you can quote another character device that already does it like your
patch, then your patch doesn't create a new inconsistency, it merely
adds to an existing one.  Acceptable.

If you can't, you should either make your device behave like all the
others, or make all the others behave like yours.