Le 09/07/2015 08:41, Peter Crosthwaite a écrit :
sorry accidental send, comments inline below.
On Wed, Jul 8, 2015 at 11:36 PM, Peter Crosthwaite
<peter.crosthwa...@xilinx.com> wrote:
On Wed, Jul 8, 2015 at 10:55 PM, Jean-Christophe DUBOIS
<j...@tribudubois.net> wrote:
Le 08/07/2015 22:49, Peter Crosthwaite a écrit :
On Wed, Jul 8, 2015 at 11:42 AM, Jean-Christophe Dubois
<j...@tribudubois.net> wrote:
Move constructor to DeviceClass methods
* imx_serial_init
* imx_serial_realize
imx32_serial_properties is renamed to imx_serial_properties.
Rework of Qdev construction helper function.
Signed-off-by: Jean-Christophe Dubois <j...@tribudubois.net>
---
Changes since v1:
* not present on v1
Changes since v2:
* not present on v2
Changes since v3:
* not present on v3
Changes since v4:
* not present on v4
Changes since v5:
* not present on v5
Changes since v6:
* not present on v6
Changes since v7:
* not present on v7
Changes since v8:
* Remove Qdev construction helper
Changes since v9:
* Qdev construction helper is reintegrated and moved to a header
file
as an inline function.
Changes since v10:
* Qdev construction helper is put back in the main file.
* Qdev construction helper is reworked
* We don't use qemu_char_get_next_serial() anymore but the chardev
property instead.
* Fix code to work with an unitialized (null) chardev property
hw/char/imx_serial.c | 98
+++++++++++++++++++++++++++++-----------------------
1 file changed, 54 insertions(+), 44 deletions(-)
diff --git a/hw/char/imx_serial.c b/hw/char/imx_serial.c
index 1dcb325..950c740 100644
--- a/hw/char/imx_serial.c
+++ b/hw/char/imx_serial.c
@@ -38,13 +38,13 @@ do { printf("imx_serial: " fmt , ##args); } while (0)
//#define DEBUG_IMPLEMENTATION 1
#ifdef DEBUG_IMPLEMENTATION
# define IPRINTF(fmt, args...) \
- do { fprintf(stderr, "imx_serial: " fmt, ##args); } while (0)
+ do { fprintf(stderr, "%s: " fmt, TYPE_IMX_SERIAL, ##args); } while
(0)
#else
# define IPRINTF(fmt, args...) do {} while (0)
#endif
static const VMStateDescription vmstate_imx_serial = {
- .name = "imx-serial",
+ .name = TYPE_IMX_SERIAL,
.version_id = 1,
.minimum_version_id = 1,
.fields = (VMStateField[]) {
@@ -125,7 +125,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;
@@ -212,7 +214,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);
+ }
Looking around, imx serial is trying to be NULL safe except for these
two cases. This and the hunk above is a full-on bugfix that should
probably be split off and go to 2.4.
How do you provide a patch specifically for 2.4 (beside including it in my
series)?
Split it off as a single and send it alone, --subject-prefix="PATCH
for-2.4" argument to git send-email.
}
}
s->ucr2 = value & 0xffff;
@@ -299,23 +303,16 @@ static void imx_event(void *opaque, int event)
}
}
-
Out of scope style change.
static const struct MemoryRegionOps imx_serial_ops = {
.read = imx_serial_read,
.write = imx_serial_write,
.endianness = DEVICE_NATIVE_ENDIAN,
};
-static int imx_serial_init(SysBusDevice *dev)
+static void imx_serial_realize(DeviceState *dev, Error **errp)
{
IMXSerialState *s = IMX_SERIAL(dev);
-
- memory_region_init_io(&s->iomem, OBJECT(s), &imx_serial_ops, s,
- "imx-serial", 0x1000);
- sysbus_init_mmio(dev, &s->iomem);
- sysbus_init_irq(dev, &s->irq);
-
if (s->chr) {
qemu_chr_add_handlers(s->chr, imx_can_receive, imx_receive,
imx_event, s);
@@ -323,45 +320,58 @@ static int imx_serial_init(SysBusDevice *dev)
DPRINTF("No char dev for uart at 0x%lx\n",
(unsigned long)s->iomem.ram_addr);
}
+}
+
+static void imx_serial_init(Object *obj)
+{
+ SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
+ IMXSerialState *s = IMX_SERIAL(obj);
- return 0;
+ memory_region_init_io(&s->iomem, obj, &imx_serial_ops, s,
+ TYPE_IMX_SERIAL, 0x1000);
+ sysbus_init_mmio(sbd, &s->iomem);
+ sysbus_init_irq(sbd, &s->irq);
}
void imx_serial_create(int uart, const hwaddr addr, qemu_irq irq)
{
DeviceState *dev;
- SysBusDevice *bus;
- CharDriverState *chr;
- const char chr_name[] = "serial";
- char label[ARRAY_SIZE(chr_name) + 1];
dev = qdev_create(NULL, TYPE_IMX_SERIAL);
- if (uart >= MAX_SERIAL_PORTS) {
- hw_error("Cannot assign uart %d: QEMU supports only %d ports\n",
- uart, MAX_SERIAL_PORTS);
- }
- chr = serial_hds[uart];
- if (!chr) {
- snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, uart);
- chr = qemu_chr_new(label, "null", NULL);
- if (!(chr)) {
- hw_error("Can't assign serial port to imx-uart%d.\n", uart);
+ if (dev) {
If dev is NULL I think you have big problems. If there is a valid
reason why this can be NULL in a non-error case then it should just
short return.
It is obviously an error case (out of resource?) and therefore we should not
ignore it (as it was previously the case). So I test for it here. Do you
feel there should be another behavior (hw_error or something)?
if (!dev) {
return;
}
+ SysBusDevice *bus;
+
+ if (uart < MAX_SERIAL_PORTS) {
+ CharDriverState *chr;
+
+ chr = serial_hds[uart];
+
+ if (!chr) {
+ char label[20];
+ snprintf(label, sizeof(label), "imx.uart%d", uart);
+ chr = qemu_chr_new(label, "null", NULL);
+ if (!(chr)) {
+ hw_error("Can't assign serial port to %s.\n",
label);
+ }
+ }
+
+ qdev_prop_set_chr(dev, "chardev", chr);
}
- }
- qdev_prop_set_chr(dev, "chardev", chr);
- bus = SYS_BUS_DEVICE(dev);
- qdev_init_nofail(dev);
- if (addr != (hwaddr)-1) {
- sysbus_mmio_map(bus, 0, addr);
- }
- sysbus_connect_irq(bus, 0, irq);
+ bus = SYS_BUS_DEVICE(dev);
-}
+ qdev_init_nofail(dev);
+
+ if (addr != (hwaddr)-1) {
+ sysbus_mmio_map(bus, 0, addr);
+ }
+ sysbus_connect_irq(bus, 0, irq);
+ }
+}
So the change to _create looks correct, but it is still out of scope
of the patch which by commit message is an init/realize conversion. Is
there a reason the old _create implementation wont work with the new
init/realize and allow a split?
Aside from the minor comments the code is good, but I would split this
to 3 patches.
1: chr NULL guards, and send that as a single for 2.4.
2: realize and init conversion
3: _create rewrite.
OK, I'll do it even if I am spending quite some time on something that is
going to be wiped out in a following patch.
Ok but why? Does an unchanged _create work? Note existing bugs are ok,
you don't faciliate any of your new functionalities or fix existing
issues (e.g. becoming NULL safe or deal with missing error paths) you
just need to not cause a regression half way through the series. So if
the existing _create just works with your realize change (which AFAICT
it does) just leave it unchanged and nuke it later in the series. What
am I missing that requires the change to _create at all?
It sure does work in the non error case.
So I'll remove my "fix" and let it as is until it is nuked when I use
the i.MX31 SOC implementation.
Regards,
Peter
Regards,
Peter
-static Property imx32_serial_properties[] = {
+static Property imx_serial_properties[] = {
DEFINE_PROP_CHR("chardev", IMXSerialState, chr),
DEFINE_PROP_END_OF_LIST(),
};
@@ -369,21 +379,21 @@ static Property imx32_serial_properties[] = {
static void imx_serial_class_init(ObjectClass *klass, void *data)
{
DeviceClass *dc = DEVICE_CLASS(klass);
- SysBusDeviceClass *k = SYS_BUS_DEVICE_CLASS(klass);
- k->init = imx_serial_init;
+ dc->realize = imx_serial_realize;
dc->vmsd = &vmstate_imx_serial;
dc->reset = imx_serial_reset_at_boot;
set_bit(DEVICE_CATEGORY_INPUT, dc->categories);
dc->desc = "i.MX series UART";
- dc->props = imx32_serial_properties;
+ dc->props = imx_serial_properties;
}
static const TypeInfo imx_serial_info = {
- .name = TYPE_IMX_SERIAL,
- .parent = TYPE_SYS_BUS_DEVICE,
- .instance_size = sizeof(IMXSerialState),
- .class_init = imx_serial_class_init,
+ .name = TYPE_IMX_SERIAL,
+ .parent = TYPE_SYS_BUS_DEVICE,
+ .instance_size = sizeof(IMXSerialState),
+ .instance_init = imx_serial_init,
+ .class_init = imx_serial_class_init,
};
static void imx_serial_register_types(void)
--
2.1.4
