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

[Jason Wang]

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.