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

[Evgeny Iakovlev]

On 1/23/2023 16:59, Evgeny Iakovlev wrote:
> On 1/23/2023 16:21, Philippe Mathieu-Daudé wrote:
> > On 23/1/23 15:43, Evgeny Iakovlev wrote:
> > > On 1/23/2023 09:14, Philippe Mathieu-Daudé wrote:
> > > > On 20/1/23 16:54, Evgeny Iakovlev wrote:
> > > > > UART should be enabled in general and have RX enabled specifically 
> > > > > to be
> > > > > able to receive data from peripheral device. Same goes for 
> > > > > transmitting
> > > > > data to peripheral device and a TXE flag.
> > > > >
> > > > > Check if UART CR register has EN and RXE or TXE bits enabled before
> > > > > trying to receive or transmit data.
> > > > >
> > > > > Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> > > > > Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> > > > > ---
> > > > >   hw/char/pl011.c | 22 +++++++++++++++++++---
> > > > >   1 file changed, 19 insertions(+), 3 deletions(-)
> > > >
> > > > > +static inline bool pl011_can_transmit(PL011State *s)
> > > > > +{
> > > > > +    return s->cr & PL011_CR_UARTEN && s->cr & PL011_CR_TXE;
> > > > > +}
> > > > > +
> > > > >   static void pl011_write(void *opaque, hwaddr offset,
> > > > >                           uint64_t value, unsigned size)
> > > > >   {
> > > > > @@ -221,7 +231,9 @@ static void pl011_write(void *opaque, hwaddr 
> > > > > offset,
> > > > >         switch (offset >> 2) {
> > > > >       case 0: /* UARTDR */
> > > > > -        /* ??? Check if transmitter is enabled.  */
> > > > > +        if (!pl011_can_transmit(s)) {
> > > > > +            break;
> > > > > +        }
> > > > >           ch = value;
> > > > >           /* XXX this blocks entire thread. Rewrite to use
> > > > >            * qemu_chr_fe_write and background I/O callbacks */
> > > > > @@ -292,7 +304,11 @@ static int pl011_can_receive(void *opaque)
> > > > >       PL011State *s = (PL011State *)opaque;
> > > > >       int r;
> > > > >   -    r = s->read_count < pl011_get_fifo_depth(s);
> > > > > +    if (!(s->cr & PL011_CR_UARTEN) || !(s->cr & PL011_CR_RXE)) {
> > > >
> > > > Maybe add pl011_can_receive() similarly to pl011_can_transmit().
> > > >
> > > > Otherwise:
> > > > Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> > >
> > >
> > > Thanks! There's already a pl011_can_receive though, its the 
> > > pl011_can_transmit that's new :)
> >
> > pl011_can_receive() returns the number of bytes that pl011_receive() 
> > can accept, pl011_can_transmit() returns a boolean.
>
>
> Hm, no, pl011_can_receive never actually returned the number of bytes. 
> It's a boolean value as an int. Which is a bug, because 
> qemu_chr_fe_set_handlers expects it to return the byte count, not a 
> 0/1 value.
>
> I'll fix it then.


Actually, no, i spoke too soon. qemu_chr_fe_set_handlers indeed expects 
a number of bytes from pl011_can_receive, but pl011_can_receive also 
deliberately gives it 1 and not how many bytes are there in queue 
really, because everything else on the receive code path is written with 
an assumption to only accept 1 element at a time (see pl011_put_fifo).

If we want to optimize this part, we would need to change that 
assumption. That should better be done as a separate change though, 
which i can send later.


> > 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().