diff mbox series

hw/char/serial: Convert to three-phase reset

Message ID 20250110175707.82097-1-philmd@linaro.org
State New
Headers show
Series hw/char/serial: Convert to three-phase reset | expand

Commit Message

Philippe Mathieu-Daudé Jan. 10, 2025, 5:57 p.m. UTC 
Convert the TYPE_SERIAL (16550A UART) to three-phase reset.

Local states are reset in the ResetHold handler.
Move the IRQ lowering to ResetExit, since it an external
object is accessed.
Note, this fixes a bug where serial_realize() was calling
serial_reset() -> qemu_irq_lower() while the IRQ was not
yet created.

Signed-off-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
That said, externally creating IRQ like that is odd, see:

  serial_pci_realize()
  {
    SerialState *s = &pci->state;
    qdev_realize(DEVICE(s), NULL, ...);
    s->irq = pci_allocate_irq(&pci->dev);

But too much cleanup for now, one step at a time.
---
 hw/char/serial.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Comments

Paolo Bonzini Jan. 10, 2025, 6:12 p.m. UTC | #1 
On 1/10/25 18:57, Philippe Mathieu-Daudé wrote:
> Convert the TYPE_SERIAL (16550A UART) to three-phase reset.
> 
> Local states are reset in the ResetHold handler.
> Move the IRQ lowering to ResetExit, since it an external
> object is accessed.

Accessing external objects is fine for hold; only "enter" cannot do so.

> ---
> That said, externally creating IRQ like that is odd, see:
> 
>    serial_pci_realize()
>    {
>      SerialState *s = &pci->state;
>      qdev_realize(DEVICE(s), NULL, ...);
>      s->irq = pci_allocate_irq(&pci->dev);
> 
> But too much cleanup for now, one step at a time.
> ---

serial_realize cannot fail.  Just move qdev_realize after the assignment 
and pass &error_abort?  Same for serial_mm_realize and 
multi_serial_pci_realize; serial_isa_realizefn instead is doing the 
right thing.

Paolo
Philippe Mathieu-Daudé Jan. 10, 2025, 6:27 p.m. UTC | #2 
On 10/1/25 19:12, Paolo Bonzini wrote:
> On 1/10/25 18:57, Philippe Mathieu-Daudé wrote:
>> Convert the TYPE_SERIAL (16550A UART) to three-phase reset.
>>
>> Local states are reset in the ResetHold handler.
>> Move the IRQ lowering to ResetExit, since it an external
>> object is accessed.
> 
> Accessing external objects is fine for hold; only "enter" cannot do so.
> 
>> ---
>> That said, externally creating IRQ like that is odd, see:
>>
>>    serial_pci_realize()
>>    {
>>      SerialState *s = &pci->state;
>>      qdev_realize(DEVICE(s), NULL, ...);
>>      s->irq = pci_allocate_irq(&pci->dev);
>>
>> But too much cleanup for now, one step at a time.
>> ---
> 
> serial_realize cannot fail.  Just move qdev_realize after the assignment 
> and pass &error_abort?  Same for serial_mm_realize and 
> multi_serial_pci_realize; serial_isa_realizefn instead is doing the 
> right thing.

OK, v2 coming (without &error_abort, can be done later).
diff mbox series

Patch

diff --git a/hw/char/serial.c b/hw/char/serial.c
index 70044e14a0f..0dab5fba176 100644
--- a/hw/char/serial.c
+++ b/hw/char/serial.c
@@ -851,9 +851,9 @@  const VMStateDescription vmstate_serial = {
     }
 };
 
-static void serial_reset(void *opaque)
+static void serial_reset_hold(Object *obj, ResetType type)
 {
-    SerialState *s = opaque;
+    SerialState *s = (SerialState *)obj;
 
     if (s->watch_tag > 0) {
         g_source_remove(s->watch_tag);
@@ -885,12 +885,18 @@  static void serial_reset(void *opaque)
 
     s->thr_ipending = 0;
     s->last_break_enable = 0;
-    qemu_irq_lower(s->irq);
 
     serial_update_msl(s);
     s->msr &= ~UART_MSR_ANY_DELTA;
 }
 
+static void serial_reset_exit(Object *obj, ResetType type)
+{
+    SerialState *s = (SerialState *)obj;
+
+    qemu_irq_lower(s->irq);
+}
+
 static int serial_be_change(void *opaque)
 {
     SerialState *s = opaque;
@@ -926,13 +932,11 @@  static void serial_realize(DeviceState *dev, Error **errp)
     s->modem_status_poll = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) serial_update_msl, s);
 
     s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, (QEMUTimerCB *) fifo_timeout_int, s);
-    qemu_register_reset(serial_reset, s);
 
     qemu_chr_fe_set_handlers(&s->chr, serial_can_receive1, serial_receive1,
                              serial_event, serial_be_change, s, NULL, true);
     fifo8_create(&s->recv_fifo, UART_FIFO_LENGTH);
     fifo8_create(&s->xmit_fifo, UART_FIFO_LENGTH);
-    serial_reset(s);
 }
 
 static void serial_unrealize(DeviceState *dev)
@@ -947,8 +951,6 @@  static void serial_unrealize(DeviceState *dev)
 
     fifo8_destroy(&s->recv_fifo);
     fifo8_destroy(&s->xmit_fifo);
-
-    qemu_unregister_reset(serial_reset, s);
 }
 
 const MemoryRegionOps serial_io_ops = {
@@ -973,12 +975,15 @@  static const Property serial_properties[] = {
 static void serial_class_init(ObjectClass *klass, void* data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    ResettableClass *rc = RESETTABLE_CLASS(klass);
 
     /* internal device for serialio/serialmm, not user-creatable */
     dc->user_creatable = false;
     dc->realize = serial_realize;
     dc->unrealize = serial_unrealize;
     device_class_set_props(dc, serial_properties);
+    rc->phases.hold = serial_reset_hold;
+    rc->phases.exit = serial_reset_exit;
 }
 
 static const TypeInfo serial_info = {