Re: [PATCH v6 2/2] net: tulip: add .can_receive routine

[Jason Wang]

On 2020/3/24 上午10:04, Li Qiang wrote:
> P J P <address@hidden <mailto:address@hidden>> 于2020年3月23日周一 
> 下午8:25写道：
>
>     From: Prasad J Pandit <address@hidden
>     <mailto:address@hidden>>
>
>     Define .can_receive routine to do sanity checks before receiving
>     packet data. And call qemu_flush_queued_packets to flush queued
>     packets once they are read in tulip_receive().
>
>     Suggested-by: Jason Wang <address@hidden
>     <mailto:address@hidden>>
>     Signed-off-by: Prasad J Pandit <address@hidden
>     <mailto:address@hidden>>
>     ---
>      hw/net/tulip.c | 17 ++++++++++++++++-
>      1 file changed, 16 insertions(+), 1 deletion(-)
>
>     Update v6: merge earlier patch 2 & 3 into one
>       ->
>     https://lists.gnu.org/archive/html/qemu-devel/2020-03/msg06868.html
>
>     diff --git a/hw/net/tulip.c b/hw/net/tulip.c
>     index fbe40095da..8d8c9519e7 100644
>     --- a/hw/net/tulip.c
>     +++ b/hw/net/tulip.c
>     @@ -229,6 +229,18 @@ static bool tulip_filter_address(TULIPState
>     *s, const uint8_t *addr)
>          return ret;
>      }
>
>     +static int
>     +tulip_can_receive(NetClientState *nc)
>     +{
>     +    TULIPState *s = qemu_get_nic_opaque(nc);
>     +
>     +    if (s->rx_frame_len || tulip_rx_stopped(s)) {
>     +        return false;
>     +    }
>     +
>     +    return true;
>     +}
>     +
>      static ssize_t tulip_receive(TULIPState *s, const uint8_t *buf,
>     size_t size)
>      {
>          struct tulip_descriptor desc;
>     @@ -236,7 +248,7 @@ static ssize_t tulip_receive(TULIPState *s,
>     const uint8_t *buf, size_t size)
>          trace_tulip_receive(buf, size);
>
>          if (size < 14 || size > sizeof(s->rx_frame) - 4
>     -        || s->rx_frame_len || tulip_rx_stopped(s)) {
>     +        || !tulip_can_receive(s->nic->ncs)) {
>              return 0;
>          }
>
>     @@ -275,6 +287,8 @@ static ssize_t tulip_receive(TULIPState *s,
>     const uint8_t *buf, size_t size)
>              tulip_desc_write(s, s->current_rx_desc, &desc);
>              tulip_next_rx_descriptor(s, &desc);
>          } while (s->rx_frame_len);
>     +
>     +    qemu_flush_queued_packets(qemu_get_queue(s->nic));
>
>
>
> Hi Prasad ans Jason,
> I want to know why here we need call 'qemu_flush_queued_packets'.
> AFAICS, the other networking card emulation need no this.


Right, I thought we need this because rx_frame_len is checked in 
tulip_can_receive().

But not I think it's unnecessary. According to kernel driver code, the 
rx is not necessarily stopped when rx buffer is overrun, e.g, 
tulip_rx_refill() only toggle the doorbell when chip is LC82C168. But 
own emulation did DC21143.

This means using can_receive() is wrong.

So I think we can drop this patch.

Thanks for the checking and sorry for the wrong suggestion.


> Thanks,
> Li Qiang
>
>          return size;
>      }
>
>     @@ -288,6 +302,7 @@ static NetClientInfo net_tulip_info = {
>          .type = NET_CLIENT_DRIVER_NIC,
>          .size = sizeof(NICState),
>          .receive = tulip_receive_nc,
>     +    .can_receive = tulip_can_receive,
>      };
>
>      static const char *tulip_reg_name(const hwaddr addr)
>     -- 
>     2.25.1