Re: [Qemu-devel] [PATCH v2 1/4] hw/char: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Mon, Aug 13, 2018 at 10:08 AM Julia Suvorova <address@hidden> wrote:
> On 10.08.2018 09:02, Stefan Hajnoczi wrote:
> > On Wed, Aug 8, 2018 at 10:07 PM, Julia Suvorova <address@hidden> wrote:
> >> +static uint64_t uart_read(void *opaque, hwaddr addr, unsigned int size)
> >> +{
> >> +    NRF51UARTState *s = NRF51_UART(opaque);
> >> +    uint64_t r;
> >> +
> >> +    if (!s->enabled) {
> >> +        return 0;
> >> +    }
> >> +
> >> +    switch (addr) {
> >> +    case A_UART_RXD:
> >> +        r = s->rx_fifo[s->rx_fifo_pos];
> >> +        if (s->rx_started && s->rx_fifo_len) {
> >> +            qemu_chr_fe_accept_input(&s->chr);
> >
> > Should this be called after popping a byte from the rx fifo?  That way
> > .can_receive() will return true again.
>
> Could you explain more, please?

This calls into the chardev's ->chr_accept_input() function.  That
function may do anything it wants.

At this point we haven't popped a byte from our rx fifo yet, so if
->chr_accept_input() calls back into the chardev frontend (us!) it
sees that we cannot receive.  That's strange since we just told the
backend we want to accept input!

I haven't checked if there is any code path where this can happen, but
it's safer to first update internal state before letting the outside
world know that we can accept more input.

> >> +static void nrf51_uart_realize(DeviceState *dev, Error **errp)
> >> +{
> >> +    NRF51UARTState *s = NRF51_UART(dev);
> >> +
> >> +    qemu_chr_fe_set_handlers(&s->chr, uart_can_receive, uart_receive,
> >> +                             uart_event, NULL, s, NULL, true);
> >> +}
> >
> > unrealize() should set the handlers to NULL.  That way the device can
> > be removed without leaving callbacks registered.
>
> I don't know the reason, but almost all char devices do not implement
> this function. Maybe, because when you quit qemu, qemu_chr_cleanup() is 
> called.

It's an assumption that on-board devices cannot be hot unplugged and
that the machine type stays alive until QEMU terminates.

Making this assumption saves 1 call to qemu_chr_fe_set_handlers().
The cost is that we cannot safely stop the system-on-chip because its
devices don't clean up properly.

Since cleanup is so trivial here I think it's worthwhile.

Stefan