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

[Jason Wang <jasowang@redhat.com>]

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 <jasowang@redhat.com>
>> Sent: Monday, February 10, 2020 7:09 PM
>> To: Vikram Garhwal <fnuv@xilinx.com>; qemu-devel@nongnu.org
>> 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.