| Message ID | 20180420145249.32435-10-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | Drop compile time limit on number of serial ports | expand |
On 04/20/2018 11:52 AM, Peter Maydell wrote: > The handling of NULL chardevs in exynos4210_uart_create() is now > all unnecessary: we don't need to create 'null' chardevs, and we > don't need to enforce a bounds check on serial_hd(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- > hw/char/exynos4210_uart.c | 20 -------------------- > 1 file changed, 20 deletions(-) > > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c > index c2bba03362..a5a285655f 100644 > --- a/hw/char/exynos4210_uart.c > +++ b/hw/char/exynos4210_uart.c > @@ -589,28 +589,8 @@ DeviceState *exynos4210_uart_create(hwaddr addr, > DeviceState *dev; > SysBusDevice *bus; > > - const char chr_name[] = "serial"; > - char label[ARRAY_SIZE(chr_name) + 1]; > - > dev = qdev_create(NULL, TYPE_EXYNOS4210_UART); > > - if (!chr) { > - if (channel >= MAX_SERIAL_PORTS) { > - error_report("Only %d serial ports are supported by QEMU", > - MAX_SERIAL_PORTS); > - exit(1); > - } > - chr = serial_hd(channel); > - if (!chr) { > - snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel); > - chr = qemu_chr_new(label, "null"); > - if (!(chr)) { > - error_report("Can't assign serial port to UART%d", channel); > - exit(1); > - } > - } > - } > - > qdev_prop_set_chr(dev, "chardev", chr); > qdev_prop_set_uint32(dev, "channel", channel); > qdev_prop_set_uint32(dev, "rx-size", fifo_size); >
On 20.04.2018 16:52, Peter Maydell wrote: > The handling of NULL chardevs in exynos4210_uart_create() is now > all unnecessary: we don't need to create 'null' chardevs, and we > don't need to enforce a bounds check on serial_hd(). > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > hw/char/exynos4210_uart.c | 20 -------------------- > 1 file changed, 20 deletions(-) > > diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c > index c2bba03362..a5a285655f 100644 > --- a/hw/char/exynos4210_uart.c > +++ b/hw/char/exynos4210_uart.c > @@ -589,28 +589,8 @@ DeviceState *exynos4210_uart_create(hwaddr addr, > DeviceState *dev; > SysBusDevice *bus; > > - const char chr_name[] = "serial"; > - char label[ARRAY_SIZE(chr_name) + 1]; > - > dev = qdev_create(NULL, TYPE_EXYNOS4210_UART); > > - if (!chr) { > - if (channel >= MAX_SERIAL_PORTS) { > - error_report("Only %d serial ports are supported by QEMU", > - MAX_SERIAL_PORTS); > - exit(1); > - } If I get the EXYNOS 4210 data sheet right, this chip has only 4 channels indeed: http://www.samsung.com/global/business/semiconductor/file/product/Exynos_4_Dual_45nm_User_Manaul_Public_REV1.00-0.pdf So I think you should at least keep an "assert(channel < 4)" in here? Apart from that, the patch looks sane to me, so when you add that assert(): Reviewed-by: Thomas Huth <thuth@redhat.com>
Hi Thomas, On 04/25/2018 12:05 PM, Thomas Huth wrote: > On 20.04.2018 16:52, Peter Maydell wrote: >> The handling of NULL chardevs in exynos4210_uart_create() is now >> all unnecessary: we don't need to create 'null' chardevs, and we >> don't need to enforce a bounds check on serial_hd(). >> >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> hw/char/exynos4210_uart.c | 20 -------------------- >> 1 file changed, 20 deletions(-) >> >> diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c >> index c2bba03362..a5a285655f 100644 >> --- a/hw/char/exynos4210_uart.c >> +++ b/hw/char/exynos4210_uart.c >> @@ -589,28 +589,8 @@ DeviceState *exynos4210_uart_create(hwaddr addr, >> DeviceState *dev; >> SysBusDevice *bus; >> >> - const char chr_name[] = "serial"; >> - char label[ARRAY_SIZE(chr_name) + 1]; >> - >> dev = qdev_create(NULL, TYPE_EXYNOS4210_UART); >> >> - if (!chr) { >> - if (channel >= MAX_SERIAL_PORTS) { >> - error_report("Only %d serial ports are supported by QEMU", >> - MAX_SERIAL_PORTS); >> - exit(1); >> - } > > If I get the EXYNOS 4210 data sheet right, this chip has only 4 channels > indeed: > > http://www.samsung.com/global/business/semiconductor/file/product/Exynos_4_Dual_45nm_User_Manaul_Public_REV1.00-0.pdf > > So I think you should at least keep an "assert(channel < 4)" in here? This file models a single UART, I don't think a such assert belongs here. Although it is named EXYNOS4210, I'm pretty sure it should works to model UARTs from other SoCs from the Exynos4xxx series. There are no restriction on newer SoCs to have > 4 UARTs. For this particular Exynos4210 SoC, the 4 channels are correctly limited in exynos4210_init(). Regards, Phil. > > Apart from that, the patch looks sane to me, so when you add that assert(): > > Reviewed-by: Thomas Huth <thuth@redhat.com>
diff --git a/hw/char/exynos4210_uart.c b/hw/char/exynos4210_uart.c index c2bba03362..a5a285655f 100644 --- a/hw/char/exynos4210_uart.c +++ b/hw/char/exynos4210_uart.c @@ -589,28 +589,8 @@ DeviceState *exynos4210_uart_create(hwaddr addr, DeviceState *dev; SysBusDevice *bus; - const char chr_name[] = "serial"; - char label[ARRAY_SIZE(chr_name) + 1]; - dev = qdev_create(NULL, TYPE_EXYNOS4210_UART); - if (!chr) { - if (channel >= MAX_SERIAL_PORTS) { - error_report("Only %d serial ports are supported by QEMU", - MAX_SERIAL_PORTS); - exit(1); - } - chr = serial_hd(channel); - if (!chr) { - snprintf(label, ARRAY_SIZE(label), "%s%d", chr_name, channel); - chr = qemu_chr_new(label, "null"); - if (!(chr)) { - error_report("Can't assign serial port to UART%d", channel); - exit(1); - } - } - } - qdev_prop_set_chr(dev, "chardev", chr); qdev_prop_set_uint32(dev, "channel", channel); qdev_prop_set_uint32(dev, "rx-size", fifo_size);
The handling of NULL chardevs in exynos4210_uart_create() is now all unnecessary: we don't need to create 'null' chardevs, and we don't need to enforce a bounds check on serial_hd(). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- hw/char/exynos4210_uart.c | 20 -------------------- 1 file changed, 20 deletions(-)