Re: [Qemu-devel] [PATCH v2 1/2] hw/net: add support for Allwinner EMAC Fast Ethernet controller

[Peter Crosthwaite]

On Tue, Jan 14, 2014 at 3:19 PM, Stefan Hajnoczi <address@hidden> wrote:
> On Mon, Jan 13, 2014 at 11:16:37PM +1000, Peter Crosthwaite wrote:
>> On Mon, Jan 13, 2014 at 11:15 PM, Peter Crosthwaite
>> <address@hidden> wrote:
>> > On Sat, Jan 11, 2014 at 8:13 PM, Beniamino Galvani <address@hidden> wrote:
>> >> +    } else {
...
>> >> +static ssize_t aw_emac_receive(NetClientState *nc, const uint8_t *buf,
>> >> +                               size_t size)
>> >> +{
>> >> +    AwEmacState *s = qemu_get_nic_opaque(nc);
>> >> +    AwEmacFifo *fifo;
>> >> +    uint32_t crc, *word;
>> >> +    uint8_t *dest;
>> >> +
>> >> +    if (s->num_rx >= NUM_RX_FIFOS) {
>> >
>> > Seems inconsistent that you check for fifo vacancy but you dont check
>> > for s->ctl & EMAC_CTL_RX_EN (as above in can_recieve). Then again,
>> > both conditions should be guarded by can_recieve, so I wonder whether
>> > you can just drop this.
>> >
>> > Stefan, if you are reading, can the recieve function rely on
>> > can_recieve success and drop such checks?
>
> It is safer to perform full checks in .receive() since
> qemu_net_queue_flush() calls .receive() repeatedly without
> .can_receive().  This means especially if the state can change between
> calls, you need to do a full check.
>

Ok, then original implementation is all good. No change required for
this issue. Sry for the noise.

Regards,
Peter