RE: [PATCH 1/1] hw/net/can: Introduce Xlnx ZynqMP CAN controller for QEMU

[Vikram Garhwal]

Hi Jason,
Thanks for replying. 
Yeah, we can start looking into combining NetClient and CanBusClient in near 
future.

Meanwhile sending a V2 today with incorporating your suggestion.. 
Regards,
Vikram

> -----Original Message-----
> From: Jason Wang <address@hidden>
> Sent: Tuesday, February 25, 2020 7:25 PM
> To: Vikram Garhwal <address@hidden>; address@hidden
> Cc: Francisco Eduardo Iglesias <address@hidden>
> Subject: Re: [PATCH 1/1] hw/net/can: Introduce Xlnx ZynqMP CAN
> controller for QEMU
> 
> 
> On 2020/2/25 下午2:22, Vikram Garhwal wrote:
> > Hi Jason,
> > Apologies for the delayed response. I tried plugging NetClientState in the
> CAN which is required if we use qemu_send_packet but this will change the
> underlying architecture of can-core, can-socketcan a lot. This means
> changes the way CAN bus is created/works and socket CAN works. CAN
> Socket(CAN Raw socket) is much different from Ethernet so plugging/using
> NetClient state is not working here.
> 
> 
> I get you.
> 
> 
> >
> > I apologize for still being a little confused about the filters but when
> looking into the code, I can only find them being used with ethernet frames.
> Since no other can controller uses NetClientState it makes me wonder if this
> model perhaps was thought of being an ethernet NIC?
> 
> 
> Nope NetclientState is not necessarily a NIC, it can be a peer of the NIC (e.g
> network backend like tap, hubport etc).
> 
> 
> > Or has the code in net/can/ which I referenced been obsoleted?
> 
> 
> No :)
> 
> 
> >
> > Sharing this link for SocketCAN(in case you want to have a look):
> https://www.kernel.org/doc/Documentation/networking/can.txt. Section 4
> talks on how CAN Socket is intended to work. Equivalent file is located as
> net/can-socketcan.c.
> 
> 
> Thanks for the pointer.
> 
> I agree that there's no need to change that part. But we may consider to
> unify the CanBusClientState and NetClientState in the future.
> 
> 
> >
> > Regards,
> > Vikram
> >
> >> -----Original Message-----
> >> From: Jason Wang <address@hidden>
> >> Sent: Monday, February 10, 2020 7:09 PM
> >> To: Vikram Garhwal <address@hidden>; address@hidden
> >> Subject: Re: [PATCH 1/1] hw/net/can: Introduce Xlnx ZynqMP CAN
> controller
> >> for QEMU
> >>
> >>
> >> On 2020/2/11 上午5:45, Vikram Garhwal wrote:
> >>>>> +                }
> >>>>> +            } else {
> >>>>> +                /* Normal mode Tx. */
> >>>>> +                generate_frame(&frame, data);
> >>>>> +
> >>>>> +                can_bus_client_send(&s->bus_client, &frame, 1);
> >>>> I had a quick glance at can_bus_client_send():
> >>>>
> >>>> It did:
> >>>>
> >>>>        QTAILQ_FOREACH(peer, &bus->clients, next) {
> >>>>            if (peer->info->can_receive(peer)) {
> >>>>                if (peer == client) {
> >>>>                    /* No loopback support for now */
> >>>>                    continue;
> >>>>                }
> >>>>                if (peer->info->receive(peer, frames, frames_cnt) > 0) {
> >>>>                    ret = 1;
> >>>>                }
> >>>>            }
> >>>>        }
> >>>>
> >>>> which looks not correct. We need to use qemu_send_packet() instead
> of
> >>>> calling peer->info->receive() directly which bypasses filters
> completely.
> >>> [Vikram Garhwal] Can you please elaborate it bit more on why do we
> need
> >> to filter outgoing message? So, I can either add a filter before sending
> the
> >> packets. I am unable to understand the use case for it. For any message
> which
> >> is incoming, we are filtering it for sure before storing in 
> >> update_rx_fifo().
> >>
> >>
> >> I might be not clear, I meant the netfilters supported by qemu which
> allows
> >> you to attach a filter to a specific NetClientState, see
> >> qemu_send_packet_async_with_flags. It doesn't mean the filter
> implemented
> >> in your own NIC model.
> >>
> >> Thanks
> >>
> >>
> >>> Also, I can see existing CAN models like CAN sja1000 and CAN Kavser
> are
> >> using it same can_bus_client_send() function. However, this doesn't
> mean
> >> that it is the correct way to send & receive packets.