| Message ID | 20191023173154.30051-14-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > Memory mapped serial device is in fact a sysbus device. The following > patches will make use of sysbus facilities for resource and > registration. > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > --- > hw/char/omap_uart.c | 2 +- > hw/char/serial.c | 47 ++++++++++++++++++++++++++++------------ > hw/mips/boston.c | 2 +- > hw/mips/mips_malta.c | 2 +- > include/hw/char/serial.h | 20 ++++++++++++----- > 5 files changed, 51 insertions(+), 22 deletions(-) > -SerialState *serial_mm_init(MemoryRegion *address_space, > +SerialMM *serial_mm_init(MemoryRegion *address_space, > hwaddr base, int it_shift, > qemu_irq irq, int baudbase, > Chardev *chr, enum device_endian end) > { > - DeviceState *dev = DEVICE(object_new(TYPE_SERIAL)); > - SerialState *s = SERIAL(dev); > + SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM)); > + SerialState *s = &self->serial; > > - s->it_shift = it_shift; > + self->it_shift = it_shift; > s->irq = irq; > - qdev_prop_set_uint32(dev, "baudbase", baudbase); > - qdev_prop_set_chr(dev, "chardev", chr); > - qdev_prop_set_int32(dev, "instance-id", base); > - qdev_init_nofail(dev); > + qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase); > + qdev_prop_set_chr(DEVICE(s), "chardev", chr); > + qdev_prop_set_int32(DEVICE(s), "instance-id", base); > + > + qdev_init_nofail(DEVICE(s)); > + qdev_init_nofail(DEVICE(self)); Something odd is going on here. This is a convenience wrapper around creating the SERIAL_MM device, so it's correct that it has to init DEVICE(self). But it should not be doing anything with the internals of 'self'. It's the instance_init/realize of the SERIAL_MM object that should instance_init/realize the 'self->serial' object. You have the code below to do the 'instance_init' in the serial_mm_instance_init function, but are missing the equivalent realize code. > - memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s, > + memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self, > "serial", 8 << it_shift); > memory_region_add_subregion(address_space, base, &s->io); > - return s; > + > + return self; > +} > + > +static void serial_mm_instance_init(Object *o) > +{ > + SerialMM *self = SERIAL_MM(o); 'self' is not idiomatic for the name of the variable containing the pointer to the object in QOM code ("git grep '\Wself\W' hw" shows no uses of it at all, which is quite unusual for us -- usually the codebase has at least a few uses of any non-standard way of writing something ;-)) Usually we use something approximating to the abbreviation of the type name, so here 'smm' would do. > + > + object_initialize_child(o, "serial", &self->serial, sizeof(self->serial), > + TYPE_SERIAL, &error_abort, NULL); > } thanks -- PMM
Hi On Mon, Nov 18, 2019 at 6:43 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 23 Oct 2019 at 18:33, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > Memory mapped serial device is in fact a sysbus device. The following > > patches will make use of sysbus facilities for resource and > > registration. > > > > Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> > > --- > > hw/char/omap_uart.c | 2 +- > > hw/char/serial.c | 47 ++++++++++++++++++++++++++++------------ > > hw/mips/boston.c | 2 +- > > hw/mips/mips_malta.c | 2 +- > > include/hw/char/serial.h | 20 ++++++++++++----- > > 5 files changed, 51 insertions(+), 22 deletions(-) > > > > -SerialState *serial_mm_init(MemoryRegion *address_space, > > +SerialMM *serial_mm_init(MemoryRegion *address_space, > > hwaddr base, int it_shift, > > qemu_irq irq, int baudbase, > > Chardev *chr, enum device_endian end) > > { > > - DeviceState *dev = DEVICE(object_new(TYPE_SERIAL)); > > - SerialState *s = SERIAL(dev); > > + SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM)); > > + SerialState *s = &self->serial; > > > > - s->it_shift = it_shift; > > + self->it_shift = it_shift; > > s->irq = irq; > > - qdev_prop_set_uint32(dev, "baudbase", baudbase); > > - qdev_prop_set_chr(dev, "chardev", chr); > > - qdev_prop_set_int32(dev, "instance-id", base); > > - qdev_init_nofail(dev); > > + qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase); > > + qdev_prop_set_chr(DEVICE(s), "chardev", chr); > > + qdev_prop_set_int32(DEVICE(s), "instance-id", base); > > + > > + qdev_init_nofail(DEVICE(s)); > > + qdev_init_nofail(DEVICE(self)); > > Something odd is going on here. This is a convenience > wrapper around creating the SERIAL_MM device, so it's > correct that it has to init DEVICE(self). But it should > not be doing anything with the internals of 'self'. > It's the instance_init/realize of the SERIAL_MM object that should > instance_init/realize the 'self->serial' object. You have the > code below to do the 'instance_init' in the serial_mm_instance_init > function, but are missing the equivalent realize code. This should be addressed later with "serial-mm: use sysbus facilities". I'll add a comment. > > > - memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s, > > + memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self, > > "serial", 8 << it_shift); > > memory_region_add_subregion(address_space, base, &s->io); > > - return s; > > + > > + return self; > > +} > > + > > +static void serial_mm_instance_init(Object *o) > > +{ > > + SerialMM *self = SERIAL_MM(o); > > 'self' is not idiomatic for the name of the variable containing > the pointer to the object in QOM code ("git grep '\Wself\W' hw" > shows no uses of it at all, which is quite unusual for us -- > usually the codebase has at least a few uses of any non-standard > way of writing something ;-)) > > Usually we use something approximating to the abbreviation > of the type name, so here 'smm' would do. I would prefer something more straightforward than having to come up with various name mangling. Imho, we loose some readability, consistency & semantic by not naming the object argument of the method "self" > > > + > > + object_initialize_child(o, "serial", &self->serial, sizeof(self->serial), > > + TYPE_SERIAL, &error_abort, NULL); > > } > > thanks > -- PMM >
On Wed, 20 Nov 2019 at 07:34, Marc-André Lureau <marcandre.lureau@redhat.com> wrote: > > > +static void serial_mm_instance_init(Object *o) > > > +{ > > > + SerialMM *self = SERIAL_MM(o); > > > > 'self' is not idiomatic for the name of the variable containing > > the pointer to the object in QOM code ("git grep '\Wself\W' hw" > > shows no uses of it at all, which is quite unusual for us -- > > usually the codebase has at least a few uses of any non-standard > > way of writing something ;-)) > > > > Usually we use something approximating to the abbreviation > > of the type name, so here 'smm' would do. > > I would prefer something more straightforward than having to come up > with various name mangling. > > Imho, we loose some readability, consistency & semantic by not naming > the object argument of the method "self" There are two problems here: (1) "self" gives no hint at all about whether it's the Object*, the DeviceState*, or the SerialMM*. All of those are the object the method is operating on, but the type differences matter. (2) *Absolutely nobody anywhere else in any other device model is using the 'self' argument name*. It's not a convention if only this one file is using it, and adopting it here gives us absolutely no consistency -- exactly the opposite. Item (1) is part of why we do things the way we do; item (2) is why we should not make the 16550 UART some weird exception from the way we do things. thanks -- PMM
Hi On Wed, Nov 20, 2019 at 2:36 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 20 Nov 2019 at 07:34, Marc-André Lureau > <marcandre.lureau@redhat.com> wrote: > > > > > +static void serial_mm_instance_init(Object *o) > > > > +{ > > > > + SerialMM *self = SERIAL_MM(o); > > > > > > 'self' is not idiomatic for the name of the variable containing > > > the pointer to the object in QOM code ("git grep '\Wself\W' hw" > > > shows no uses of it at all, which is quite unusual for us -- > > > usually the codebase has at least a few uses of any non-standard > > > way of writing something ;-)) > > > > > > Usually we use something approximating to the abbreviation > > > of the type name, so here 'smm' would do. > > > > I would prefer something more straightforward than having to come up > > with various name mangling. > > > > Imho, we loose some readability, consistency & semantic by not naming > > the object argument of the method "self" > > There are two problems here: > (1) "self" gives no hint at all about whether it's the Object*, > the DeviceState*, or the SerialMM*. All of those are the > object the method is operating on, but the type differences matter. "self" should have the type of the object that is being implemented. serial_mm_instance_init -> SerialMM *self > > (2) *Absolutely nobody anywhere else in any other device model > is using the 'self' argument name*. It's not a convention if > only this one file is using it, and adopting it here gives us > absolutely no consistency -- exactly the opposite. Since there is no clear convention, then adding "self" isn't breaking any convention. > > Item (1) is part of why we do things the way we do; item (2) > is why we should not make the 16550 UART some weird > exception from the way we do things. It's about trying to make things better, and not about staying in the current undefined/free zone.
On Wed, 20 Nov 2019 at 10:40, Marc-André Lureau <marcandre.lureau@gmail.com> wrote: > On Wed, Nov 20, 2019 at 2:36 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > There are two problems here: > > (1) "self" gives no hint at all about whether it's the Object*, > > the DeviceState*, or the SerialMM*. All of those are the > > object the method is operating on, but the type differences matter. > > "self" should have the type of the object that is being implemented. > > serial_mm_instance_init -> SerialMM *self In a typical device implementation, some functions take the object as a "SerialMM" or other specific pointer. Some take an Object*. Some take a DeviceState*. Some take void*. Which of those is "the type of the object that is being implemented" ? > > (2) *Absolutely nobody anywhere else in any other device model > > is using the 'self' argument name*. It's not a convention if > > only this one file is using it, and adopting it here gives us > > absolutely no consistency -- exactly the opposite. > > Since there is no clear convention, then adding "self" isn't breaking > any convention. There is a clear convention, which I explained to you already (variable is named with some abbreviation/initialism of the type name, or sometimes just 's' for "state"). You are trying to write code which ignores that convention and does something else *which no other code is doing*. Please stop doing that -- in particular, don't do it in one device in the middle of a refactoring series. If you want to argue that we should change our convention for how we write QOM code, feel free -- but that should be a separate discussion and probably ought to come with a plan for how to do a general update of the codebase to avoid a weird mix of styles. thanks -- PMM
Hi On Wed, Nov 20, 2019 at 2:58 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Wed, 20 Nov 2019 at 10:40, Marc-André Lureau > <marcandre.lureau@gmail.com> wrote: > > On Wed, Nov 20, 2019 at 2:36 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > > There are two problems here: > > > (1) "self" gives no hint at all about whether it's the Object*, > > > the DeviceState*, or the SerialMM*. All of those are the > > > object the method is operating on, but the type differences matter. > > > > "self" should have the type of the object that is being implemented. > > > > serial_mm_instance_init -> SerialMM *self > > In a typical device implementation, some functions take the > object as a "SerialMM" or other specific pointer. Some take > an Object*. Some take a DeviceState*. Some take void*. > Which of those is "the type of the object that is being implemented" ? In OOP, you implement methods for a specific type. "self" is of the type that is being implemented. serial_mm_foo(SerialMM *self); serial_mm_instance_init(Object *o) { SerialMM *self = SERIAL_MM(o); .. } If you have a generic function, not tied to a specific class, this rule doesn't apply. do_this(some arg, SerialMM *smm) > > > > (2) *Absolutely nobody anywhere else in any other device model > > > is using the 'self' argument name*. It's not a convention if > > > only this one file is using it, and adopting it here gives us > > > absolutely no consistency -- exactly the opposite. > > > > Since there is no clear convention, then adding "self" isn't breaking > > any convention. > > There is a clear convention, which I explained to you already > (variable is named with some abbreviation/initialism of > the type name, or sometimes just 's' for "state"). You are trying > to write code which ignores that convention and does something > else *which no other code is doing*. Please stop doing that -- in > particular, don't do it in one device in the middle of a refactoring > series. > > If you want to argue that we should change our convention for > how we write QOM code, feel free -- but that should be a separate > discussion and probably ought to come with a plan for how > to do a general update of the codebase to avoid a weird > mix of styles. > Fair enough We can only change things incrementally, and while doing refactoring is the right moment to write better and more readable code.
diff --git a/hw/char/omap_uart.c b/hw/char/omap_uart.c index 13e4f43c4c..e8da933378 100644 --- a/hw/char/omap_uart.c +++ b/hw/char/omap_uart.c @@ -27,7 +27,7 @@ struct omap_uart_s { MemoryRegion iomem; hwaddr base; - SerialState *serial; /* TODO */ + SerialMM *serial; /* TODO */ struct omap_target_agent_s *ta; omap_clk fclk; qemu_irq irq; diff --git a/hw/char/serial.c b/hw/char/serial.c index 667570e310..0290b3c633 100644 --- a/hw/char/serial.c +++ b/hw/char/serial.c @@ -1032,16 +1032,16 @@ static const TypeInfo serial_info = { static uint64_t serial_mm_read(void *opaque, hwaddr addr, unsigned size) { - SerialState *s = opaque; - return serial_ioport_read(s, addr >> s->it_shift, 1); + SerialMM *s = SERIAL_MM(opaque); + return serial_ioport_read(&s->serial, addr >> s->it_shift, 1); } static void serial_mm_write(void *opaque, hwaddr addr, uint64_t value, unsigned size) { - SerialState *s = opaque; + SerialMM *s = SERIAL_MM(opaque); value &= 255; - serial_ioport_write(s, addr >> s->it_shift, value, 1); + serial_ioport_write(&s->serial, addr >> s->it_shift, value, 1); } static const MemoryRegionOps serial_mm_ops[3] = { @@ -1068,30 +1068,49 @@ static const MemoryRegionOps serial_mm_ops[3] = { }, }; -SerialState *serial_mm_init(MemoryRegion *address_space, +SerialMM *serial_mm_init(MemoryRegion *address_space, hwaddr base, int it_shift, qemu_irq irq, int baudbase, Chardev *chr, enum device_endian end) { - DeviceState *dev = DEVICE(object_new(TYPE_SERIAL)); - SerialState *s = SERIAL(dev); + SerialMM *self = SERIAL_MM(qdev_create(NULL, TYPE_SERIAL_MM)); + SerialState *s = &self->serial; - s->it_shift = it_shift; + self->it_shift = it_shift; s->irq = irq; - qdev_prop_set_uint32(dev, "baudbase", baudbase); - qdev_prop_set_chr(dev, "chardev", chr); - qdev_prop_set_int32(dev, "instance-id", base); - qdev_init_nofail(dev); + qdev_prop_set_uint32(DEVICE(s), "baudbase", baudbase); + qdev_prop_set_chr(DEVICE(s), "chardev", chr); + qdev_prop_set_int32(DEVICE(s), "instance-id", base); + + qdev_init_nofail(DEVICE(s)); + qdev_init_nofail(DEVICE(self)); - memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], s, + memory_region_init_io(&s->io, NULL, &serial_mm_ops[end], self, "serial", 8 << it_shift); memory_region_add_subregion(address_space, base, &s->io); - return s; + + return self; +} + +static void serial_mm_instance_init(Object *o) +{ + SerialMM *self = SERIAL_MM(o); + + object_initialize_child(o, "serial", &self->serial, sizeof(self->serial), + TYPE_SERIAL, &error_abort, NULL); } +static const TypeInfo serial_mm_info = { + .name = TYPE_SERIAL_MM, + .parent = TYPE_SYS_BUS_DEVICE, + .instance_init = serial_mm_instance_init, + .instance_size = sizeof(SerialMM), +}; + static void serial_register_types(void) { type_register_static(&serial_info); + type_register_static(&serial_mm_info); } type_init(serial_register_types) diff --git a/hw/mips/boston.c b/hw/mips/boston.c index ca7d813a52..23fdd5ec6a 100644 --- a/hw/mips/boston.c +++ b/hw/mips/boston.c @@ -50,7 +50,7 @@ typedef struct { MachineState *mach; MIPSCPSState cps; - SerialState *uart; + SerialMM *uart; CharBackend lcd_display; char lcd_content[8]; diff --git a/hw/mips/mips_malta.c b/hw/mips/mips_malta.c index 4d9c64b36a..efc3307973 100644 --- a/hw/mips/mips_malta.c +++ b/hw/mips/mips_malta.c @@ -86,7 +86,7 @@ typedef struct { uint32_t i2csel; CharBackend display; char display_text[9]; - SerialState *uart; + SerialMM *uart; bool display_inited; } MaltaFPGAState; diff --git a/include/hw/char/serial.h b/include/hw/char/serial.h index 7cc6304f1d..f146d426c2 100644 --- a/include/hw/char/serial.h +++ b/include/hw/char/serial.h @@ -57,7 +57,6 @@ typedef struct SerialState { qemu_irq irq; CharBackend chr; int last_break_enable; - int it_shift; uint32_t baudbase; uint32_t tsr_retry; guint watch_tag; @@ -81,6 +80,14 @@ typedef struct SerialState { int instance_id; } SerialState; +typedef struct SerialMM { + SysBusDevice parent; + + SerialState serial; + + int it_shift; +} SerialMM; + extern const VMStateDescription vmstate_serial; extern const MemoryRegionOps serial_io_ops; @@ -89,12 +96,15 @@ void serial_set_frequency(SerialState *s, uint32_t frequency); #define TYPE_SERIAL "serial" #define SERIAL(s) OBJECT_CHECK(SerialState, (s), TYPE_SERIAL) +#define TYPE_SERIAL_MM "serial-mm" +#define SERIAL_MM(s) OBJECT_CHECK(SerialMM, (s), TYPE_SERIAL_MM) + SerialState *serial_init(int base, qemu_irq irq, int baudbase, Chardev *chr, MemoryRegion *system_io); -SerialState *serial_mm_init(MemoryRegion *address_space, - hwaddr base, int it_shift, - qemu_irq irq, int baudbase, - Chardev *chr, enum device_endian end); +SerialMM *serial_mm_init(MemoryRegion *address_space, + hwaddr base, int it_shift, + qemu_irq irq, int baudbase, + Chardev *chr, enum device_endian end); /* serial-isa.c */
Memory mapped serial device is in fact a sysbus device. The following patches will make use of sysbus facilities for resource and registration. Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/char/omap_uart.c | 2 +- hw/char/serial.c | 47 ++++++++++++++++++++++++++++------------ hw/mips/boston.c | 2 +- hw/mips/mips_malta.c | 2 +- include/hw/char/serial.h | 20 ++++++++++++----- 5 files changed, 51 insertions(+), 22 deletions(-)