Re: [PATCH 01/14] sm501: replace PROP_PTR with PROP_LINK

[Peter Maydell]

On Fri, 18 Oct 2019 at 16:42, Marc-André Lureau
<address@hidden> wrote:
>
> Signed-off-by: Marc-André Lureau <address@hidden>
> ---
>  hw/display/sm501.c | 5 +++--
>  hw/sh4/r2d.c       | 3 ++-
>  2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/hw/display/sm501.c b/hw/display/sm501.c
> index 1f33c87e65..a87d18efab 100644
> --- a/hw/display/sm501.c
> +++ b/hw/display/sm501.c
> @@ -1930,7 +1930,7 @@ typedef struct {
>      SM501State state;
>      uint32_t vram_size;
>      uint32_t base;
> -    void *chr_state;
> +    Chardev *chr_state;
>  } SM501SysBusState;
>
>  static void sm501_realize_sysbus(DeviceState *dev, Error **errp)
> @@ -1968,7 +1968,8 @@ static void sm501_realize_sysbus(DeviceState *dev, 
> Error **errp)
>  static Property sm501_sysbus_properties[] = {
>      DEFINE_PROP_UINT32("vram-size", SM501SysBusState, vram_size, 0),
>      DEFINE_PROP_UINT32("base", SM501SysBusState, base, 0),
> -    DEFINE_PROP_PTR("chr-state", SM501SysBusState, chr_state),
> +    DEFINE_PROP_LINK("chr-state", SM501SysBusState, chr_state,
> +                     TYPE_CHARDEV, Chardev *),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>
> diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
> index ee0840f380..5780ee85d9 100644
> --- a/hw/sh4/r2d.c
> +++ b/hw/sh4/r2d.c
> @@ -272,7 +272,8 @@ static void r2d_init(MachineState *machine)
>      busdev = SYS_BUS_DEVICE(dev);
>      qdev_prop_set_uint32(dev, "vram-size", SM501_VRAM_SIZE);
>      qdev_prop_set_uint32(dev, "base", 0x10000000);
> -    qdev_prop_set_ptr(dev, "chr-state", serial_hd(2));
> +    object_property_set_link(OBJECT(dev), OBJECT(serial_hd(2)),
> +                             "chr-state", &error_abort);
>      qdev_init_nofail(dev);
>      sysbus_mmio_map(busdev, 0, 0x10000000);
>      sysbus_mmio_map(busdev, 1, 0x13e00000);

We have a typed way of passing a Chardev to devices:
use qdev_prop_set_chr(). Unfortunately it's not trivially
easy to drop in here, because it sets a property defined
with DEFINE_PROP_CHR to set a field of type CharBackend
(note, not Chardev, and not a pointer) in the device struct.
But serial_mm_init() wants a Chardev*, because it is a
non-QOM interface to the serial device and is manually
doing the qemu_chr_fe_init() that connects the Chardev
to its own CharBackend. The QOM CHR property setter wants
to do that qemu_chr_fe_init() itself.

So I think the right fix here is to properly QOMify the
code which is not QOMified, ie hw/char/serial.c, in a
way that means that the various "memory mapped 16650-ish
devices" we have can use it and can define a
TYPE_CHARDEV property.

In general I think our uses of PROP_PTR are code smells
that indicate places where we have not properly converted
code over to the general approach that the QOM/qdev
design desires; but we should be getting rid of PROP_PTR
by actually doing all those (difficult) conversions.
Merely removing PROP_PTR itself by rephrasing the dubious
inter-device connections in a way that makes them harder
to grep for doesn't seem to me to be necessarily worth
doing. Is the existence of PROP_PTR getting in your way
for a change you want to make ?

thanks
-- PMM