Re: [Qemu-devel] [RFC 2/3] hw/char/nrf51_uart: Implement nRF51 SoC UART

[Stefan Hajnoczi]

On Fri, Jun 1, 2018 at 4:58 PM, Peter Maydell <address@hidden> wrote:
> On 1 June 2018 at 16:21, Julia Suvorova <address@hidden> wrote:
>> On 31.05.2018 12:42, Stefan Hajnoczi wrote:
>>>> +static int uart_can_receive(void *opaque)
>>>> +{
>>>> +    Nrf51UART *s = NRF51_UART(opaque);
>>>> +
>>>> +    return (s->rx_fifo_len < sizeof(s->rx_fifo));
>>>
>>> This is very subtle: this function returns the number of bytes that can
>>> be read.  This expression looks like a boolean return value but actually
>>> relies on the implicit int cast (false -> 0 bytes, true -> 1 byte).
>>>
>>> Please write it so it doesn't look like a boolean return value:
>>>
>>>    if (s->rx_fifo_len >= sizeof(s->rx_fifo)) {
>>>        return 0;
>>>    }
>>>
>>>    return 1 /* byte */;
>>>
>>> Alternatively, you could handle more than 1 byte:
>>>
>>>    return sizeof(s->rx_fifo) - s->rx_fifo_len;
>>>
>>> but now uart_receive() needs to handle up to sizeof(s->rx_fifo) bytes of
>>> data in a single call.
>>
>> I will rewrite uart_receive function to accept many bytes at once.
>
> Stefan, I was wondering about this when I read this patch.
> What is the API for the can_receive and receive functions?
> There's no documentation of the semantics either with the
> IOReadHandler or IOCanReadHandler typedefs in main-loop.h,
> or in the doc comment for qemu_chr_fe_set_handlers.
>
> I'm guessing that the answer is "your can_read handler should
> return the number of bytes you can accept, and a subsequent call
> to the read handler then must accept that many bytes, it can't
> change its mind and only accept a smaller number" (with some sort
> of guarantee by the caller that it won't let guest execution or
> other simulation-changes things between the call to can_receive
> and receive) ?
>
> (Similarly we don't document the semantics for the NetClientInfo
> can_receive and receive functions.)

I'll send documentation patches.

Stefan