On 07/16/2015 11:32 AM, Fam Zheng wrote:
> On Thu, 07/16 10:58, Jason Wang wrote:
>>
>> On 07/15/2015 06:19 PM, Fam Zheng wrote:
>>> eth_can_rx checks s->rxsize and returns false if it is non-zero. Because
>>> of the .can_receive semantics change, this will make the incoming queue
>>> disabled by peer, until it is explicitly flushed. So we should flush it
>>> when s->rxsize is becoming zero.
>>>
>>> Squash eth_can_rx semantics into etx_rx and drop .can_receive()
>>> callback, also add flush when rx buffer becomes available again after a
>>> packet gets queued.
>>>
>>> The other conditions, "!axienet_rx_resetting(s) &&
>>> axienet_rx_enabled(s)" are OK because enet_write already calls
>>> qemu_flush_queued_packets when the register bits are changed.
>>>
>>> Signed-off-by: Fam Zheng <f...@redhat.com>
>>> ---
>>> hw/net/xilinx_axienet.c | 17 +++++++++++++----
>>> 1 file changed, 13 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/net/xilinx_axienet.c b/hw/net/xilinx_axienet.c
>>> index 9205770..d63c423 100644
>>> --- a/hw/net/xilinx_axienet.c
>>> +++ b/hw/net/xilinx_axienet.c
>>> @@ -401,6 +401,9 @@ struct XilinxAXIEnet {
>>>
>>> uint8_t rxapp[CONTROL_PAYLOAD_SIZE];
>>> uint32_t rxappsize;
>>> +
>>> + /* Whether axienet_eth_rx_notify should flush incoming queue. */
>>> + bool need_flush;
>>> };
>>>
>>> static void axienet_rx_reset(XilinxAXIEnet *s)
>>> @@ -658,10 +661,8 @@ static const MemoryRegionOps enet_ops = {
>>> .endianness = DEVICE_LITTLE_ENDIAN,
>>> };
>>>
>>> -static int eth_can_rx(NetClientState *nc)
>>> +static int eth_can_rx(XilinxAXIEnet *s)
>>> {
>>> - XilinxAXIEnet *s = qemu_get_nic_opaque(nc);
>>> -
>>> /* RX enabled? */
>>> return !s->rxsize && !axienet_rx_resetting(s) && axienet_rx_enabled(s);
>>> }
>>> @@ -701,6 +702,10 @@ static void axienet_eth_rx_notify(void *opaque)
>>> s->rxpos += ret;
>>> if (!s->rxsize) {
>>> s->regs[R_IS] |= IS_RX_COMPLETE;
>>> + if (s->need_flush) {
>>> + s->need_flush = false;
>>> + qemu_flush_queued_packets(qemu_get_queue(s->nic));
>>> + }
>>> }
>>> }
>>> enet_update_irq(s);
>>> @@ -721,6 +726,11 @@ static ssize_t eth_rx(NetClientState *nc, const
>>> uint8_t *buf, size_t size)
>>>
>>> DENET(qemu_log("%s: %zd bytes\n", __func__, size));
>>>
>>> + if (!eth_can_rx(s)) {
>>> + s->need_flush = true;
>>> + return 0;
>>> + }
>>> +
>> axienet_eth_rx_notify() was only called by eth_rx(). So when
>> s->need_flush is true, we won't ever reach axienet_eth_rx_notify()?
> We will.
>
> If we are here it measn a previous call to axienet_eth_rx_notify hasn't
> drained
> the buffer:
>
> static void axienet_eth_rx_notify(void *opaque)
> {
> ...
>
> while (s->rxsize && stream_can_push(s->tx_data_dev,
> axienet_eth_rx_notify, s)) {
> size_t ret = stream_push(s->tx_data_dev, (void *)s->rxmem +
> s->rxpos,
> s->rxsize);
> s->rxsize -= ret;
> s->rxpos += ret;
> if (!s->rxsize) {
> s->regs[R_IS] |= IS_RX_COMPLETE;
> }
> }
> ...
>
> }
>
> axienet_eth_rx_notify is passed to stream_can_push so it will be reached again
> once s->tx_data_dev can receive more data:
>
> typedef struct StreamSlaveClass {
> InterfaceClass parent;
> /**
> * can push - determine if a stream slave is capable of accepting at
> least
> * one byte of data. Returns false if cannot accept. If not
> implemented, the
> * slave is assumed to always be capable of receiving.
> * @notify: Optional callback that the slave will call when the slave
> is
> * capable of receiving again. Only called if false is returned.
> * @notify_opaque: opaque data to pass to notify call.
> */
> bool (*can_push)(StreamSlave *obj, StreamCanPushNotifyFn notify,
> void *notify_opaque);
> ...
>
> Am I missing anything?
>
> Fam
Probably not. I misses the possible call in axidma_write(). So the patch
is ok :)
