Re: [Qemu-devel] [PATCH v2 for-2.3 2/5] hw: Mark devices picking up char backends actively FIXME

["Andreas Färber" <andreas.faerber@web.de>]

Am 25.03.2015 um 15:16 schrieb Markus Armbruster:
> Character devices defined with -serial and -parallel are for board
> initialization to wire up.  Board code examines serial_hds[] and
> parallel_hds[] to find them, and creates devices with their qdev
> chardev properties set accordingly.
> 
> Except a few devices go on a fishing expedition for a suitable backend
> instead of exposing a chardev property for board code to set: they use
> serial_hds[] (often via qemu_char_get_next_serial()) or parallel_hds[]
> in their realize() or init() method to connect to a backend.
> 
> Picking up backends that way works when the devices are created by
> board code.  But it's inappropriate for -device or device_add.  Not
> only is it inconsistent with how the other characrer device models
> work (they connect to a backend explicitly identified by a "chardev"
> property), it breaks when the backend has been picked up by the board
> or a previous -device / device_add already.
> 
> Example:
> 
>     $ qemu-system-ppc64 -M bamboo -S -device i82378 -device pc87312 -device pc87312
>     qemu-system-ppc64: -device pc87312: Property 'isa-parallel.chardev' can't take value 'parallel0', it's in use
> 
> Mark them with suitable FIXME comments.
> 
> Cc: Li Guang <lig.fnst@cn.fujitsu.com>
> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Cc: Antony Pavlov <antonynpavlov@gmail.com>
> Cc: "Edgar E. Iglesias" <edgar.iglesias@gmail.com>
> Cc: Michael Walle <michael@walle.cc>
> Cc: Peter Crosthwaite <peter.crosthwaite@xilinx.com>
> Cc: "Andreas Färber" <andreas.faerber@web.de>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
>  hw/arm/allwinner-a10.c    | 1 +
>  hw/char/cadence_uart.c    | 1 +
>  hw/char/digic-uart.c      | 1 +
>  hw/char/etraxfs_ser.c     | 1 +
>  hw/char/lm32_juart.c      | 1 +
>  hw/char/lm32_uart.c       | 1 +
>  hw/char/milkymist-uart.c  | 1 +
>  hw/char/pl011.c           | 1 +
>  hw/char/stm32f2xx_usart.c | 1 +
>  hw/char/xilinx_uartlite.c | 1 +
>  hw/isa/pc87312.c          | 2 ++
>  11 files changed, 12 insertions(+)
[...]
> diff --git a/hw/isa/pc87312.c b/hw/isa/pc87312.c
> index 2849e8d..3b1fcec 100644
> --- a/hw/isa/pc87312.c
> +++ b/hw/isa/pc87312.c
> @@ -278,6 +278,7 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
>      pc87312_hard_reset(s);
>  
>      if (is_parallel_enabled(s)) {
> +        /* FIXME use a qdev chardev prop instead of parallel_hds[] */
>          chr = parallel_hds[0];
>          if (chr == NULL) {
>              chr = qemu_chr_new("par0", "null", NULL);
> @@ -296,6 +297,7 @@ static void pc87312_realize(DeviceState *dev, Error **errp)
>  
>      for (i = 0; i < 2; i++) {
>          if (is_uart_enabled(s, i)) {
> +            /* FIXME use a qdev chardev prop instead of serial_hds[] */
>              chr = serial_hds[i];
>              if (chr == NULL) {
>                  snprintf(name, sizeof(name), "ser%d", i);

Same question here: The isa-parallel and isa-serial sub-devices a few
lines further below do have a chardev property, just not the container
device.

Andreas