Re: [PATCH net-next v2 2/3] net: ethernet: ti: Register the RPMsg driver as network device

[Yojana Mallik]

Hi Andrew,

On 6/2/24 22:15, Andrew Lunn wrote:
>> +enum icve_rpmsg_type {
>> +	/* Request types */
>> +	ICVE_REQ_SHM_INFO = 0,
>> +	ICVE_REQ_SET_MAC_ADDR,
>> +
>> +	/* Response types */
>> +	ICVE_RESP_SHM_INFO,
>> +	ICVE_RESP_SET_MAC_ADDR,
>> +
>> +	/* Notification types */
>> +	ICVE_NOTIFY_PORT_UP,
>> +	ICVE_NOTIFY_PORT_DOWN,
>> +	ICVE_NOTIFY_PORT_READY,
>> +	ICVE_NOTIFY_REMOTE_READY,
>> +};
> 
> +struct message_header {
> +       u32 src_id;
> +       u32 msg_type; /* Do not use enum type, as enum size is compiler dependent */
> +} __packed;
> 
> 
> Given how you have defined icve_rpmsg_type, what is the point of
> message_header.msg_type?
> 


+	rpmsg_send(common->rpdev->ept, (void *)(&common->send_msg),
+		   sizeof(common->send_msg));

rpmsg_send in icve_create_send_request is sending message_header.msg_type to R5
core using (void *)(&common->send_msg).
In icve_rpmsg_cb function switch case statements for option msg_type are used
and cases are from enum icve_rpmsg_type.

> It seems like this would make more sense:
> 
> enum icve_rpmsg_request_type {
> 	ICVE_REQ_SHM_INFO = 0,
> 	ICVE_REQ_SET_MAC_ADDR,
> }
> 
> enum icve_rpmsg_response_type {
> 	ICVE_RESP_SHM_INFO,
> 	ICVE_RESP_SET_MAC_ADDR,
> }
> enum icve_rpmsg_notify_type {
> 	ICVE_NOTIFY_PORT_UP,
> 	ICVE_NOTIFY_PORT_DOWN,
> 	ICVE_NOTIFY_PORT_READY,
> 	ICVE_NOTIFY_REMOTE_READY,
> };
> 

Since msg_hdr.msg_type which takes value from enum icve_msg_type is being used
in rpmsg_send and icve_rpmsg_cb, I would prefer to continue with the 2 separate
enums, that is enum icve_msg_type and enum icve_rpmsg_type. However I am always
open to make improvements and would be happy to discuss this further if you
have additional insights.

> Also, why SET_MAC_ADDR? It would be good to document where the MAC
> address are coming from. And what address this is setting.
> 

The interface which is controlled by Linux and interacting with the R5 core is
assigned mac address 00:00:00:00:00:00 by default. To ensure reliable
communication and compliance with network standards a different MAC address is
set for this interface using icve_set_mac_address.

> In fact, please put all the protocol documentation into a .rst
> file. That will help us discuss the protocol independent of the
> implementation. The protocol is an ABI, so needs to be reviewed well.
> 

I will do it.

>> +struct icve_shm_info {
>> +	/* Total shared memory size */
>> +	u32 total_shm_size;
>> +	/* Total number of buffers */
>> +	u32 num_pkt_bufs;
>> +	/* Per buff slot size i.e MTU Size + 4 bytes for magic number + 4 bytes
>> +	 * for Pkt len
>> +	 */
> 
> What is your definition of MTU?
> 
> enp2s0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc fq_codel state DOWN mode DEFAULT group default qlen 1000
> 
> Typically, MTU does not include the Ethernet header or checksum. Is
> that what you mean here?
> 

MTU is only the Payload. I have realized the macro names are misleading and I
will change them as
#define ICVE_PACKET_BUFFER_SIZE   1540
#define MAX_MTU   ICVE_PACKET_BUFFER_SIZE - (ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN)
Hence value of max mtu is 1518 bytes.

>> +	u32 buff_slot_size;
>> +	/* Base Address for Tx or Rx shared memory */
>> +	u32 base_addr;
>> +} __packed;
> 
> What do you mean by address here? Virtual address, physical address,
> DMA address? And whos address is this, you have two CPUs here, with no
> guaranteed the shared memory is mapped to the same address in both
> address spaces.
> 
> 	Andrew

The address referred above is physical address. It is the address of Tx and Rx
buffer under the control of Linux operating over A53 core. The check if the
shared memory is mapped to the same address in both address spaces is checked
by the R5 core.

Thanks for the feedback.
Regards
Yojana Mallik