Received-SPF: pass (google.com: domain of linux-kernel+bounces-198369-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Sun, 2 Jun 2024 18:45:16 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Yojana Mallik <y-mallik@ti.com>
Cc: schnelle@linux.ibm.com, wsa+renesas@sang-engineering.com,
	diogo.ivo@siemens.com, rdunlap@infradead.org, horms@kernel.org,
	vigneshr@ti.com, rogerq@ti.com, danishanwar@ti.com,
	pabeni@redhat.com, kuba@kernel.org, edumazet@google.com,
	davem@davemloft.net, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org, srk@ti.com, rogerq@kernel.org
Subject: Re: [PATCH net-next v2 2/3] net: ethernet: ti: Register the RPMsg
 driver as network device
Message-ID: <4416ada7-399b-4ea0-88b0-32ca432d777b@lunn.ch>
References: <20240531064006.1223417-1-y-mallik@ti.com>
 <20240531064006.1223417-3-y-mallik@ti.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240531064006.1223417-3-y-mallik@ti.com>

> +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?

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,
};

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

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.

> +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?

> +	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