Re: [PATCH v3 5/5] hw/char/pl011: check if UART is enabled before RX or TX operation

[Philippe Mathieu-Daudé]

On 23/1/23 17:23, Peter Maydell wrote:
> On Mon, 23 Jan 2023 at 15:21, Philippe Mathieu-Daudé <philmd@linaro.org> wrote:
> > pl011_can_receive() returns the number of bytes that pl011_receive() can
> > accept, pl011_can_transmit() returns a boolean.
> >
> > I was thinking of:
> >
> > -- >8 --
> > diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> > index dd20b76609..ea5769a31c 100644
> > --- a/hw/char/pl011.c
> > +++ b/hw/char/pl011.c
> > @@ -221,6 +221,11 @@ static inline bool pl011_can_transmit(PL011State *s)
> >        return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
> >    }
> >
> > +static inline bool pl011_can_receive(PL011State *s)
> > +{
> > +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_RXE;
> > +}
> > +
> >    static void pl011_write(void *opaque, hwaddr offset,
> >                            uint64_t value, unsigned size)
> >    {
> > @@ -299,12 +304,12 @@ static void pl011_write(void *opaque, hwaddr offset,
> >        }
> >    }
> >
> > -static int pl011_can_receive(void *opaque)
> > +static int pl011_receivable_bytes(void *opaque)
> >    {
> >        PL011State *s = (PL011State *)opaque;
> >        int r;
> >
> > -    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
> > +    if (!pl011_can_receive(s)) {
> >            r = 0;
> >        } else {
> >            r = s->read_count < pl011_get_fifo_depth(s);
> > @@ -459,7 +464,7 @@ static void pl011_realize(DeviceState *dev, Error
> > **errp)
> >    {
> >        PL011State *s = PL011(dev);
> >
> > -    qemu_chr_fe_set_handlers(&s->chr, pl011_can_receive, pl011_receive,
> > +    qemu_chr_fe_set_handlers(&s->chr, pl011_receivable_bytes,
> > pl011_receive,
> >                                 pl011_event, NULL, s, NULL, true);
> >    }
> >
> > ---
> >
> > with maybe a better name for pl011_receivable_bytes().
>
> Our standard-ish name for the function you pass to
> qemu_chr_fe_set_handlers() is either foo_can_receive
> or foo_can_read, though. That is followed through in
> the name of the function argument (fd_can_read),
> its type (IOCanReadHandler), and the field it gets stored
> in (CharBackend::chr_can_read). It's not a great convention
> but at least it is a convention...

I agree this deserves a better name; maybe this is not a
convention but I'd expect functions starting with is_* / can_*
to return a boolean value, not a number:

/**
 * IOCanReadHandler: Return the number of bytes that #IOReadHandler can 
accept
 *
 * This function reports how many bytes #IOReadHandler is prepared to 
accept.
 * #IOReadHandler may be invoked with up to this number of bytes.  If this
 * function returns 0 then #IOReadHandler is not invoked.
 *
 * This function is typically called from an event loop.  If the number of
 * bytes changes outside the event loop (e.g. because a vcpu thread 
drained the
 * buffer), then it is necessary to kick the event loop so that this 
function
 * is called again.  aio_notify() or qemu_notify_event() can be used to 
kick
 * the event loop.
 */
typedef int IOCanReadHandler(void *opaque);

Also, maybe using unsigned or size_t type for the return value would
better fit. Anyhow, not really a priority :)