[RFC PATCH net-next 0/2] RPMsg based shared memory ethernet driver

[RFC PATCH net-next 0/2] RPMsg based shared memory ethernet driver

[Ravi Gunasekaran]

virtio-net provides a solution for virtual ethernet interface in a
virtualized environment.

There might be a use-case for traffic tunneling between heterogeneous
processors in a non virtualized environment such as TI's AM64x that has
Cortex A53 and Cortex R5 where Linux runs on A53 and a flavour of RTOS
on R5(FreeRTOS) and the ethernet controller is managed by R5 and needs
to pass some low priority data to A53.

One solution for such an use case where the ethernet controller does
not support DMA for Tx/Rx channel, could be a RPMsg based shared memory
ethernet driver. The data plane is over the shared memory while the control
plane is over RPMsg end point channel.

Two separate regions can be carved out in the shared memory, one for the
A53 -> R5 data path, and other for R5 -> A53 data path.

The shared memory layout is as below, with the region between
PKT_1_LEN to PKT_N modelled as circular buffer.

-------------------------
|	   HEAD		|
-------------------------
|	   TAIL		|
-------------------------
|	PKT_1_LEN	|
|	  PKT_1		|
-------------------------
|	PKT_2_LEN	|
|	  PKT_2		|
-------------------------
|	    .		|
|	    .		|
------------------------
|	PKT_N_LEN	|
|	  PKT_N		|
 -----------------------

Polling mechanism can used to check for the offset between head and
tail index to process the packets by both the cores.

I initially intended to post this as a query to know whether a
RPMsg based shared memory ethernet driver would be an upstream
friendly solution or not. But then decided to do some prototyping
and share it as RFC hoping the code might help a bit in understanding
the approach.

There is still quite a handful of work to be done. As of now firmware
just helps in registering the RPMsg driver on Linux by announcing the
RPMsg device ID.

For the purpose of testing, I allocated the memory in Linux and the Tx packet
is injected back into the network stack. These test code snippets are under
TEST_DEBUG macro.

Could you all please share your opinion on this approach?
I wanted to get the community feedback before proceeding into further implementation.

Please note, I have not included the Makefile, Kconfig in this series.

Ravi Gunasekaran (2):
  net: ethernet: ti: Introduce inter-core-virt-eth as RPMsg driver
  net: ethernet: ti: inter-core-virt-eth: Register as network device

 drivers/net/ethernet/ti/inter-core-virt-eth.c | 455 ++++++++++++++++++
 drivers/net/ethernet/ti/inter-core-virt-eth.h | 105 ++++
 2 files changed, 560 insertions(+)
 create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.c
 create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.h

-- 
2.17.1

[RFC PATCH net-next 1/2] net: ethernet: ti: Introduce inter-core-virt-eth as RPMsg driver

[Ravi Gunasekaran]

TI's K3 SoCs comprises heterogeneous processors (Cortex A, Cortex R).
When the ethernet controller is completely managed by a core (Cortex R)
running a flavor of RTOS, in a non virtualized environment, network traffic
tunnelling between heterogeneous processors can be realized by means of
RPMsg based shared memory ethernet driver. With the shared memory used
for the data plane and the RPMsg end point channel used for control plane.

inter-core-virt-eth driver is modelled as a RPMsg based shared
memory ethernet driver for such an use case.

As a first step, register the inter-core-virt-eth as a RPMsg driver.
And introduce basic control messages for querying and responding.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
---
 drivers/net/ethernet/ti/inter-core-virt-eth.c | 139 ++++++++++++++++++
 drivers/net/ethernet/ti/inter-core-virt-eth.h |  89 +++++++++++
 2 files changed, 228 insertions(+)
 create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.c
 create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.h

diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c
new file mode 100644
index 000000000000..d3b689eab1c0
--- /dev/null
+++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c
@@ -0,0 +1,139 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Texas Instruments K3 Inter Core Virtual Ethernet Driver
+ *
+ * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#include "inter-core-virt-eth.h"
+
+static int icve_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
+{
+	struct icve_common *common = dev_get_drvdata(&rpdev->dev);
+	struct message *msg = (struct message *)data;
+	struct icve_port *port = common->port;
+	u32 msg_type = msg->msg_hdr.msg_type;
+	u32 rpmsg_type;
+
+	switch (msg_type) {
+	case ICVE_RESPONSE_MSG:
+		rpmsg_type = msg->resp_msg.type;
+		switch (rpmsg_type) {
+		case ICVE_RESP_SHM_INFO:
+
+			/* Retrieve Tx and Rx shared memory info from msg */
+			port->tx_buffer = msg->resp_msg.shm_info.tx_buffer;
+
+			if (!port->tx_buffer) {
+				dev_err(common->dev, "Tx Buffer invalid\n");
+				return -ENOMEM;
+			}
+
+			port->tx_buffer->base_addr =
+				msg->resp_msg.shm_info.tx_buffer_base_addr;
+
+			if (!port->tx_buffer->base_addr) {
+				dev_err(common->dev, "Tx Buffer address invalid\n");
+				return -ENOMEM;
+			}
+
+			port->rx_buffer = msg->resp_msg.shm_info.rx_buffer;
+
+			if (!port->rx_buffer) {
+				dev_err(common->dev, "Rx Buffer invalid\n");
+				return -ENOMEM;
+			}
+
+			port->rx_buffer->base_addr =
+				msg->resp_msg.shm_info.rx_buffer_base_addr;
+
+			if (!port->rx_buffer->base_addr) {
+				dev_err(common->dev, "Rx Buffer address invalid\n");
+				return -ENOMEM;
+			}
+
+			port->icve_max_buffers =
+				msg->resp_msg.shm_info.max_buffers;
+
+			break;
+		}
+		break;
+	default:
+		dev_err(common->dev, "Invalid msg type\n");
+		break;
+	}
+
+	return 0;
+}
+
+static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg_type)
+{
+	struct message *msg = &common->send_msg;
+	int ret = 0;
+
+	msg->msg_hdr.src_id = common->port->port_id;
+	msg->req_msg.type = rpmsg_type;
+
+	switch (rpmsg_type) {
+	case ICVE_REQ_SHM_INFO:
+		msg->msg_hdr.msg_type = ICVE_REQUEST_MSG;
+		break;
+	default:
+		ret = -EINVAL;
+		dev_err(common->dev, "Invalid RPMSG request\n");
+	};
+
+	return ret;
+}
+
+static int icve_rpmsg_probe(struct rpmsg_device *rpdev)
+{
+	struct device *dev = &rpdev->dev;
+	struct icve_common *common;
+	unsigned long flags;
+
+	common = devm_kzalloc(&rpdev->dev, sizeof(*common), GFP_KERNEL);
+	if (!common)
+		return -ENOMEM;
+
+	dev_set_drvdata(dev, common);
+
+	common->port = devm_kzalloc(dev, sizeof(*common->port), GFP_KERNEL);
+	common->dev = dev;
+	common->rpdev = rpdev;
+
+	spin_lock_init(&common->send_msg_lock);
+	spin_lock_init(&common->recv_msg_lock);
+
+	/* Send request to fetch shared memory details from remote core */
+	spin_lock_irqsave(&common->send_msg_lock, flags);
+	create_request(common, ICVE_REQ_SHM_INFO);
+	rpmsg_send(common->rpdev->ept, (void *)(&common->send_msg), sizeof(common->send_msg));
+	spin_unlock_irqrestore(&common->send_msg_lock, flags);
+
+	return 0;
+}
+
+static void icve_rpmsg_remove(struct rpmsg_device *rpdev)
+{
+	dev_info(&rpdev->dev, "icve rpmsg client driver is removed\n");
+}
+
+static struct rpmsg_device_id icve_rpmsg_id_table[] = {
+	{ .name = "icve-rpsmg-client" },
+	{ },
+};
+MODULE_DEVICE_TABLE(rpmsg, icve_rpmsg_id_table);
+
+static struct rpmsg_driver icve_rpmsg_client = {
+	.drv.name	= KBUILD_MODNAME,
+	.id_table	= icve_rpmsg_id_table,
+	.probe		= icve_rpmsg_probe,
+	.callback	= icve_rpmsg_cb,
+	.remove		= icve_rpmsg_remove,
+};
+module_rpmsg_driver(icve_rpmsg_client);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Siddharth Vadapalli <s-vadapalli@ti.com>");
+MODULE_AUTHOR("Ravi Gunasekaran <r-gunasekaran@ti.com");
+MODULE_DESCRIPTION("TI Inter Core Virtual Ethernet driver");
diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h
new file mode 100644
index 000000000000..063cc371eeb3
--- /dev/null
+++ b/drivers/net/ethernet/ti/inter-core-virt-eth.h
@@ -0,0 +1,89 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Texas Instruments K3 Inter Core Virtual Ethernet Driver
+ *
+ * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
+ */
+
+#ifndef __INTER_CORE_VIRT_ETH_H__
+#define __INTER_CORE_VIRT_ETH_H__
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/rpmsg.h>
+
+enum icve_msg_type {
+	ICVE_REQUEST_MSG = 0,
+	ICVE_RESPONSE_MSG,
+};
+
+enum icve_rpmsg_type {
+	/* Request types */
+	ICVE_REQ_SHM_INFO = 0,
+
+	/* Response types */
+	ICVE_RESP_SHM_INFO,
+};
+
+struct icve_shm_info {
+	void *tx_buffer;
+	void *tx_buffer_base_addr;
+	void *rx_buffer;
+	void *rx_buffer_base_addr;
+	u32 max_buffers;
+} __packed;
+
+struct request_message {
+	u32 type; /* Request Type */
+} __packed;
+
+struct response_message {
+	u32 type;
+	union {
+		struct icve_shm_info shm_info;
+	};
+} __packed;
+
+struct notify_message {
+	u32 type;
+} __packed;
+
+struct message_header {
+	u32 src_id;
+	u32 msg_type; /* Do not use enum type, as enum size is compiler dependent */
+} __packed;
+
+struct message {
+	struct message_header msg_hdr;
+	union {
+		struct request_message req_msg;
+		struct response_message resp_msg;
+		struct notify_message notify_msg;
+	};
+} __packed;
+
+struct shared_mem {
+	u32 head;
+	u32 tail;
+	void *base_addr;
+} __packed;
+
+struct icve_port {
+	struct shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */
+	struct shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */
+	struct icve_common *common;
+	u32 icve_max_buffers;
+	u32 port_id; /* Unique ID for the port : TODO: Define range for use by Linux and non linux */
+
+} __packed;
+
+struct icve_common {
+	struct rpmsg_device *rpdev;
+	spinlock_t send_msg_lock;
+	spinlock_t recv_msg_lock;
+	struct message send_msg;
+	struct message recv_msg;
+	struct icve_port *port;
+	struct device *dev;
+} __packed;
+
+#endif /* __INTER_CORE_VIRT_ETH_H__ */
-- 
2.17.1

Re: [RFC PATCH net-next 1/2] net: ethernet: ti: Introduce inter-core-virt-eth as RPMsg driver

[Simon Horman]

On Tue, Jan 30, 2024 at 04:39:43PM +0530, Ravi Gunasekaran wrote:
> TI's K3 SoCs comprises heterogeneous processors (Cortex A, Cortex R).
> When the ethernet controller is completely managed by a core (Cortex R)
> running a flavor of RTOS, in a non virtualized environment, network traffic
> tunnelling between heterogeneous processors can be realized by means of
> RPMsg based shared memory ethernet driver. With the shared memory used
> for the data plane and the RPMsg end point channel used for control plane.
> 
> inter-core-virt-eth driver is modelled as a RPMsg based shared
> memory ethernet driver for such an use case.
> 
> As a first step, register the inter-core-virt-eth as a RPMsg driver.
> And introduce basic control messages for querying and responding.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> ---
>  drivers/net/ethernet/ti/inter-core-virt-eth.c | 139 ++++++++++++++++++
>  drivers/net/ethernet/ti/inter-core-virt-eth.h |  89 +++++++++++
>  2 files changed, 228 insertions(+)
>  create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.c
>  create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.h
> 
> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c
> new file mode 100644
> index 000000000000..d3b689eab1c0
> --- /dev/null
> +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c
> @@ -0,0 +1,139 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

Hi Ravi and Siddharth,

The correct style for SPDX headers in .c files is a '//' comment:

// SPDX-License-Identifier: GPL-2.0

> +/* Texas Instruments K3 Inter Core Virtual Ethernet Driver
> + *
> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
> + */

...

> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h

...

> +struct icve_common {
> +	struct rpmsg_device *rpdev;
> +	spinlock_t send_msg_lock;
> +	spinlock_t recv_msg_lock;

Spinlocks ought to come with an comment regarding what they lock.

> +	struct message send_msg;
> +	struct message recv_msg;
> +	struct icve_port *port;
> +	struct device *dev;
> +} __packed;
> +
> +#endif /* __INTER_CORE_VIRT_ETH_H__ */
> -- 
> 2.17.1
> 
> 

Re: [RFC PATCH net-next 1/2] net: ethernet: ti: Introduce inter-core-virt-eth as RPMsg driver

[Ravi Gunasekaran]

Simon,

On 2/1/2024 7:00 PM, Simon Horman wrote:
> On Tue, Jan 30, 2024 at 04:39:43PM +0530, Ravi Gunasekaran wrote:
>> TI's K3 SoCs comprises heterogeneous processors (Cortex A, Cortex R).
>> When the ethernet controller is completely managed by a core (Cortex R)
>> running a flavor of RTOS, in a non virtualized environment, network traffic
>> tunnelling between heterogeneous processors can be realized by means of
>> RPMsg based shared memory ethernet driver. With the shared memory used
>> for the data plane and the RPMsg end point channel used for control plane.
>>
>> inter-core-virt-eth driver is modelled as a RPMsg based shared
>> memory ethernet driver for such an use case.
>>
>> As a first step, register the inter-core-virt-eth as a RPMsg driver.
>> And introduce basic control messages for querying and responding.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>> ---
>>  drivers/net/ethernet/ti/inter-core-virt-eth.c | 139 ++++++++++++++++++
>>  drivers/net/ethernet/ti/inter-core-virt-eth.h |  89 +++++++++++
>>  2 files changed, 228 insertions(+)
>>  create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.c
>>  create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.h
>>
>> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c
>> new file mode 100644
>> index 000000000000..d3b689eab1c0
>> --- /dev/null
>> +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c
>> @@ -0,0 +1,139 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> Hi Ravi and Siddharth,
>
> The correct style for SPDX headers in .c files is a '//' comment:
>
> // SPDX-License-Identifier: GPL-2.0

I will fix this.

>> +/* Texas Instruments K3 Inter Core Virtual Ethernet Driver
>> + *
>> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
>> + */
> ...
>
>> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h
> ...
>
>> +struct icve_common {
>> +	struct rpmsg_device *rpdev;
>> +	spinlock_t send_msg_lock;
>> +	spinlock_t recv_msg_lock;
> Spinlocks ought to come with an comment regarding what they lock.

I will add the comments as reported by checkpatch.

>
>> +	struct message send_msg;
>> +	struct message recv_msg;
>> +	struct icve_port *port;
>> +	struct device *dev;
>> +} __packed;
>> +
>> +#endif /* __INTER_CORE_VIRT_ETH_H__ */
>> -- 
>> 2.17.1
>>
>>

Ravi

Re: [RFC PATCH net-next 1/2] net: ethernet: ti: Introduce inter-core-virt-eth as RPMsg driver

[Ravi Gunasekaran]

Simon,

On 2/1/2024 7:00 PM, Simon Horman wrote:
> On Tue, Jan 30, 2024 at 04:39:43PM +0530, Ravi Gunasekaran wrote:
>> TI's K3 SoCs comprises heterogeneous processors (Cortex A, Cortex R).
>> When the ethernet controller is completely managed by a core (Cortex R)
>> running a flavor of RTOS, in a non virtualized environment, network traffic
>> tunnelling between heterogeneous processors can be realized by means of
>> RPMsg based shared memory ethernet driver. With the shared memory used
>> for the data plane and the RPMsg end point channel used for control plane.
>>
>> inter-core-virt-eth driver is modelled as a RPMsg based shared
>> memory ethernet driver for such an use case.
>>
>> As a first step, register the inter-core-virt-eth as a RPMsg driver.
>> And introduce basic control messages for querying and responding.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
>> ---
>>  drivers/net/ethernet/ti/inter-core-virt-eth.c | 139 ++++++++++++++++++
>>  drivers/net/ethernet/ti/inter-core-virt-eth.h |  89 +++++++++++
>>  2 files changed, 228 insertions(+)
>>  create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.c
>>  create mode 100644 drivers/net/ethernet/ti/inter-core-virt-eth.h
>>
>> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c
>> new file mode 100644
>> index 000000000000..d3b689eab1c0
>> --- /dev/null
>> +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c
>> @@ -0,0 +1,139 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> Hi Ravi and Siddharth,
>
> The correct style for SPDX headers in .c files is a '//' comment:
>
> // SPDX-License-Identifier: GPL-2.0

I will fix this.

>> +/* Texas Instruments K3 Inter Core Virtual Ethernet Driver
>> + *
>> + * Copyright (C) 2024 Texas Instruments Incorporated - https://www.ti.com/
>> + */
> ...
>
>> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h
> ...
>
>> +struct icve_common {
>> +	struct rpmsg_device *rpdev;
>> +	spinlock_t send_msg_lock;
>> +	spinlock_t recv_msg_lock;
> Spinlocks ought to come with an comment regarding what they lock.

I will add the comments as reported by checkpatch.

>
>> +	struct message send_msg;
>> +	struct message recv_msg;
>> +	struct icve_port *port;
>> +	struct device *dev;
>> +} __packed;
>> +
>> +#endif /* __INTER_CORE_VIRT_ETH_H__ */
>> -- 
>> 2.17.1
>>
>>

Ravi

[RFC PATCH net-next 2/2] net: ethernet: ti: inter-core-virt-eth: Register as network device

[Ravi Gunasekaran]

Register the RPMsg driver as network device and add support for
basic ethernet functionality by using the shared memory for data
plane.

The shared memory layout is as below, with the region between
PKT_1_LEN to PKT_N modelled as circular buffer.

-------------------------
|          HEAD         |
-------------------------
|          TAIL         |
-------------------------
|       PKT_1_LEN       |
|         PKT_1         |
-------------------------
|       PKT_2_LEN       |
|         PKT_2         |
-------------------------
|           .           |
|           .           |
-------------------------
|       PKT_N_LEN       |
|         PKT_N         |
-------------------------

The offset between the HEAD and TAIL is polled to process the Rx packets.

Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
---
 drivers/net/ethernet/ti/inter-core-virt-eth.c | 316 ++++++++++++++++++
 drivers/net/ethernet/ti/inter-core-virt-eth.h |  16 +
 2 files changed, 332 insertions(+)

diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c
index d3b689eab1c0..735482001f4d 100644
--- a/drivers/net/ethernet/ti/inter-core-virt-eth.c
+++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c
@@ -6,6 +6,50 @@
 
 #include "inter-core-virt-eth.h"
 
+#define ICVE_MIN_PACKET_SIZE	ETH_ZLEN
+#define ICVE_MAX_PACKET_SIZE	(ETH_FRAME_LEN + ETH_FCS_LEN)
+#define ICVE_MAX_TX_QUEUES	1
+#define ICVE_MAX_RX_QUEUES	1
+
+#define TEST_DEBUG		1
+
+#ifdef TEST_DEBUG
+#define ICVE_MAX_BUFFERS	100 //TODO : Set to power of 2 to leverage shift operations
+#endif
+
+#define PKT_LEN_SIZE_TYPE	sizeof(u32)
+
+/* 4 bytes to hold packet length and ICVE_MAX_PACKET_SIZE to hold packet */
+#define ICVE_BUFFER_SIZE	(ICVE_MAX_PACKET_SIZE + PKT_LEN_SIZE_TYPE)
+
+#define RX_POLL_TIMEOUT		250
+
+#define icve_ndev_to_priv(ndev) \
+	((struct icve_ndev_priv *)netdev_priv(ndev))
+#define icve_ndev_to_port(ndev) (icve_ndev_to_priv(ndev)->port)
+#define icve_ndev_to_common(ndev) (icve_ndev_to_port(ndev)->common)
+
+static void icve_rx_timer(struct timer_list *timer)
+{
+	struct icve_port *port = from_timer(port, timer, rx_timer);
+	struct napi_struct *napi;
+	int num_pkts = 0;
+	u32 head, tail;
+
+	head = port->rx_buffer->head;
+	tail = port->rx_buffer->tail;
+
+	num_pkts = tail - head;
+	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);
+
+	napi = &port->rx_napi;
+	if (num_pkts && likely(napi_schedule_prep(napi))) {
+		__napi_schedule(napi);
+	} else {
+		mod_timer(&port->rx_timer, RX_POLL_TIMEOUT);
+	}
+}
+
 static int icve_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len, void *priv, u32 src)
 {
 	struct icve_common *common = dev_get_drvdata(&rpdev->dev);
@@ -57,6 +101,18 @@ static int icve_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len, void *
 			break;
 		}
 		break;
+	case ICVE_NOTIFY_MSG:
+		rpmsg_type = msg->notify_msg.type;
+		switch (rpmsg_type) {
+		case ICVE_NOTIFY_REMOTE_READY:
+			/* Turn on carrier once remote core signals ready */
+			netif_carrier_on(port->ndev);
+			break;
+		case ICVE_NOTIFY_PORT_UP:
+		case ICVE_NOTIFY_PORT_DOWN:
+			break;
+		}
+		break;
 	default:
 		dev_err(common->dev, "Invalid msg type\n");
 		break;
@@ -77,6 +133,10 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg
 	case ICVE_REQ_SHM_INFO:
 		msg->msg_hdr.msg_type = ICVE_REQUEST_MSG;
 		break;
+	case ICVE_NOTIFY_PORT_UP:
+	case ICVE_NOTIFY_PORT_DOWN:
+		msg->msg_hdr.msg_type = ICVE_NOTIFY_MSG;
+		break;
 	default:
 		ret = -EINVAL;
 		dev_err(common->dev, "Invalid RPMSG request\n");
@@ -85,11 +145,262 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg
 	return ret;
 }
 
+static int icve_rx_packets(struct napi_struct *napi, int budget)
+{
+	struct icve_port *port = container_of(napi, struct icve_port, rx_napi);
+	u32 count, process_pkts;
+	struct sk_buff *skb;
+	u32 head, tail;
+	u32 pkt_len;
+	int num_pkts;
+
+	head = port->rx_buffer->head;
+	tail = port->rx_buffer->tail;
+
+	num_pkts = tail - head;
+	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);
+	process_pkts = min(num_pkts, budget);
+	count = 0;
+	while (count < process_pkts) {
+		memcpy((void *)&pkt_len,
+		       (void *)port->rx_buffer->base_addr + ((head + count) * ICVE_BUFFER_SIZE),
+		       PKT_LEN_SIZE_TYPE);
+
+		/* Start building the skb */
+		skb = napi_alloc_skb(napi, pkt_len);
+		skb->dev = port->ndev;
+		skb_put(skb, pkt_len);
+
+		memcpy((void *)skb->data,
+		       (void *)(port->rx_buffer->base_addr + PKT_LEN_SIZE_TYPE) + ((head + count) * ICVE_BUFFER_SIZE),
+		       pkt_len);
+
+		skb->protocol = eth_type_trans(skb, port->ndev);
+
+		/* Push skb into network stack */
+		napi_gro_receive(napi, skb);
+
+		count++;
+	}
+
+	if (num_pkts) {
+		port->rx_buffer->head = (port->rx_buffer->head + count) % port->icve_max_buffers;
+
+		if (num_pkts < budget && napi_complete_done(napi, count))
+			mod_timer(&port->rx_timer, RX_POLL_TIMEOUT);
+	}
+	return count;
+}
+
+#ifdef TEST_DEBUG
+static int test_tx_rx_path(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct icve_port *port = icve_ndev_to_port(ndev);
+	u32 *data;
+	u32 len;
+
+	len = skb_headlen(skb);
+
+	/* Copy length */
+	memcpy((void *)port->rx_buffer->base_addr + (port->rx_buffer->tail * ICVE_BUFFER_SIZE),
+	       (void *)&len, PKT_LEN_SIZE_TYPE);
+
+	/* Copy data to shared mem */
+	memcpy((void *)(port->rx_buffer->base_addr + PKT_LEN_SIZE_TYPE) + (port->rx_buffer->tail * ICVE_BUFFER_SIZE),
+	       (void *)skb->data, len);
+
+	data = (u32 *)(port->rx_buffer->base_addr + (port->rx_buffer->tail * ICVE_BUFFER_SIZE));
+
+	port->rx_buffer->tail = (port->rx_buffer->tail + 1) % ICVE_MAX_BUFFERS;
+
+	return 0;
+}
+#endif
+
+static int icve_ndo_open(struct net_device *ndev)
+{
+	struct icve_common *common = icve_ndev_to_common(ndev);
+	struct icve_port *port = icve_ndev_to_port(ndev);
+	unsigned long flags;
+
+	/* Send a msg to remote core signalling that we are ready */
+	spin_lock_irqsave(&common->send_msg_lock, flags);
+#ifndef TEST_DEBUG
+	create_request(common, ICVE_NOTIFY_PORT_UP);
+	rpmsg_send(common->rpdev->ept, (void *)(&common->send_msg), sizeof(common->send_msg));
+#endif
+	spin_unlock_irqrestore(&common->send_msg_lock, flags);
+
+	if (!(port->tx_buffer && port->rx_buffer)) {
+		netdev_err(ndev, "Shared memory not setup\n");
+		return -EPERM;
+	}
+
+	netif_napi_add(ndev, &port->rx_napi, icve_rx_packets);
+	napi_enable(&port->rx_napi);
+
+	timer_setup(&port->rx_timer, icve_rx_timer, 0);
+	mod_timer(&port->rx_timer, RX_POLL_TIMEOUT);
+
+	return 0;
+}
+
+static int icve_ndo_stop(struct net_device *ndev)
+{
+	struct icve_common *common = icve_ndev_to_common(ndev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&common->send_msg_lock, flags);
+#ifndef TEST_DEBUG
+	create_request(common, ICVE_NOTIFY_PORT_DOWN);
+	rpmsg_send(common->rpdev->ept, (void *)(&common->send_msg), sizeof(common->send_msg));
+#endif
+	spin_unlock_irqrestore(&common->send_msg_lock, flags);
+	return 0;
+}
+
+static netdev_tx_t icve_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct icve_port *port = icve_ndev_to_port(ndev);
+	struct ethhdr *ether;
+	u32 head, tail;
+	u32 num_pkts;
+	u32 len;
+
+	ether = eth_hdr(skb);
+	len = skb_headlen(skb);
+
+	head = port->tx_buffer->head;
+	tail = port->tx_buffer->tail;
+
+	/* If the buffer queue is full, then drop packet */
+	num_pkts = tail - head;
+	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);
+	if ((num_pkts + 1) == port->icve_max_buffers) {
+		netdev_warn(ndev, "Tx buffer full\n");
+		goto ring_full;
+	}
+
+	/* Copy length */
+	memcpy((void *)port->tx_buffer->base_addr + (port->tx_buffer->tail * ICVE_BUFFER_SIZE),
+	       (void *)&len, PKT_LEN_SIZE_TYPE);
+
+	/* Copy data to shared mem */
+	memcpy((void *)(port->tx_buffer->base_addr + PKT_LEN_SIZE_TYPE) +
+	       (port->tx_buffer->tail * ICVE_BUFFER_SIZE),
+	       (void *)skb->data, len);
+
+#ifdef TEST_DEBUG
+	/* For quick Rx path testing, inject Tx pkt back into network */
+	test_tx_rx_path(skb, ndev);
+#endif
+	port->tx_buffer->tail = (port->tx_buffer->tail + 1) % port->icve_max_buffers;
+
+	dev_consume_skb_any(skb);
+
+	return NETDEV_TX_OK;
+
+ring_full:
+	return NETDEV_TX_BUSY;
+}
+
+static int icve_set_mac_address(struct net_device *ndev, void *addr)
+{
+	eth_mac_addr(ndev, addr);
+
+	/* TODO : Inform remote core about MAC address change */
+	return 0;
+}
+
+static const struct net_device_ops icve_netdev_ops = {
+	.ndo_open		= icve_ndo_open,
+	.ndo_stop		= icve_ndo_stop,
+	.ndo_start_xmit		= icve_start_xmit,
+	.ndo_set_mac_address	= icve_set_mac_address,
+};
+
+static int icve_init_ndev(struct icve_common *common)
+{
+	struct device *dev = &common->rpdev->dev;
+	struct icve_ndev_priv *ndev_priv;
+	struct icve_port *port;
+	static u32 port_id;
+	int err;
+
+	port = common->port;
+	port->common = common;
+	port->port_id = port_id++;
+
+	port->ndev = devm_alloc_etherdev_mqs(common->dev, sizeof(*ndev_priv),
+					     ICVE_MAX_TX_QUEUES,
+					     ICVE_MAX_RX_QUEUES);
+
+	if (!port->ndev) {
+		dev_err(dev, "error allocating net_device\n");
+		return -ENOMEM;
+	}
+
+	ndev_priv = netdev_priv(port->ndev);
+	ndev_priv->port = port;
+	SET_NETDEV_DEV(port->ndev, dev);
+
+	port->ndev->min_mtu = ICVE_MIN_PACKET_SIZE;
+	port->ndev->max_mtu = ICVE_MAX_PACKET_SIZE;
+	port->ndev->netdev_ops = &icve_netdev_ops;
+
+#ifdef TEST_DEBUG
+	/* Allocate memory to test without actual RPMsg handshaking */
+	port->tx_buffer = devm_kzalloc(dev, sizeof(port->tx_buffer),
+				       GFP_KERNEL);
+	if (!port->tx_buffer) {
+		dev_err(dev, "Memory not available\n");
+		return -ENOMEM;
+	}
+
+	port->tx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS,
+						  GFP_KERNEL);
+	if (!port->tx_buffer->base_addr) {
+		dev_err(dev, "Memory not available\n");
+		return -ENOMEM;
+	}
+
+	port->rx_buffer = devm_kzalloc(dev, sizeof(port->rx_buffer),
+				       GFP_KERNEL);
+	if (!port->rx_buffer) {
+		dev_err(dev, "Memory not available\n");
+		return -ENOMEM;
+	};
+
+	port->rx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS,
+						  GFP_KERNEL);
+	if (!port->rx_buffer->base_addr) {
+		dev_err(dev, "Memory not available\n");
+		return -ENOMEM;
+	}
+
+	port->icve_max_buffers = ICVE_MAX_BUFFERS;
+#else
+	/* Shared memory details will be sent by the remote core.
+	 * So turn off the carrier, until both the virtual port and
+	 * remote core is ready
+	 */
+	netif_carrier_off(port->ndev);
+
+#endif
+	err = register_netdev(port->ndev);
+
+	if (err)
+		dev_err(dev, "error registering icve net device %d\n", err);
+
+	return 0;
+}
+
 static int icve_rpmsg_probe(struct rpmsg_device *rpdev)
 {
 	struct device *dev = &rpdev->dev;
 	struct icve_common *common;
 	unsigned long flags;
+	int ret;
 
 	common = devm_kzalloc(&rpdev->dev, sizeof(*common), GFP_KERNEL);
 	if (!common)
@@ -104,6 +415,11 @@ static int icve_rpmsg_probe(struct rpmsg_device *rpdev)
 	spin_lock_init(&common->send_msg_lock);
 	spin_lock_init(&common->recv_msg_lock);
 
+	/* Register the network device */
+	ret = icve_init_ndev(common);
+	if (ret)
+		return ret;
+
 	/* Send request to fetch shared memory details from remote core */
 	spin_lock_irqsave(&common->send_msg_lock, flags);
 	create_request(common, ICVE_REQ_SHM_INFO);
diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h
index 063cc371eeb3..c3386a5ff714 100644
--- a/drivers/net/ethernet/ti/inter-core-virt-eth.h
+++ b/drivers/net/ethernet/ti/inter-core-virt-eth.h
@@ -7,13 +7,16 @@
 #ifndef __INTER_CORE_VIRT_ETH_H__
 #define __INTER_CORE_VIRT_ETH_H__
 
+#include <linux/etherdevice.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/netdevice.h>
 #include <linux/rpmsg.h>
 
 enum icve_msg_type {
 	ICVE_REQUEST_MSG = 0,
 	ICVE_RESPONSE_MSG,
+	ICVE_NOTIFY_MSG,
 };
 
 enum icve_rpmsg_type {
@@ -22,6 +25,11 @@ enum icve_rpmsg_type {
 
 	/* Response types */
 	ICVE_RESP_SHM_INFO,
+
+	/* Notification types */
+	ICVE_NOTIFY_PORT_UP,
+	ICVE_NOTIFY_PORT_DOWN,
+	ICVE_NOTIFY_REMOTE_READY,
 };
 
 struct icve_shm_info {
@@ -70,7 +78,11 @@ struct shared_mem {
 struct icve_port {
 	struct shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */
 	struct shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */
+	struct timer_list rx_timer;
 	struct icve_common *common;
+	struct napi_struct rx_napi;
+	u8 local_mac_addr[ETH_ALEN];
+	struct net_device *ndev;
 	u32 icve_max_buffers;
 	u32 port_id; /* Unique ID for the port : TODO: Define range for use by Linux and non linux */
 
@@ -86,4 +98,8 @@ struct icve_common {
 	struct device *dev;
 } __packed;
 
+struct icve_ndev_priv {
+	struct icve_port *port;
+};
+
 #endif /* __INTER_CORE_VIRT_ETH_H__ */
-- 
2.17.1

Re: [RFC PATCH net-next 2/2] net: ethernet: ti: inter-core-virt-eth: Register as network device

[Simon Horman]

On Tue, Jan 30, 2024 at 04:39:44PM +0530, Ravi Gunasekaran wrote:
> Register the RPMsg driver as network device and add support for
> basic ethernet functionality by using the shared memory for data
> plane.
> 
> The shared memory layout is as below, with the region between
> PKT_1_LEN to PKT_N modelled as circular buffer.
> 
> -------------------------
> |          HEAD         |
> -------------------------
> |          TAIL         |
> -------------------------
> |       PKT_1_LEN       |
> |         PKT_1         |
> -------------------------
> |       PKT_2_LEN       |
> |         PKT_2         |
> -------------------------
> |           .           |
> |           .           |
> -------------------------
> |       PKT_N_LEN       |
> |         PKT_N         |
> -------------------------
> 
> The offset between the HEAD and TAIL is polled to process the Rx packets.
> 
> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>

Hi Ravi,

some feedback, mainly regarding issues flagged by linters and static analysis.

> ---
>  drivers/net/ethernet/ti/inter-core-virt-eth.c | 316 ++++++++++++++++++
>  drivers/net/ethernet/ti/inter-core-virt-eth.h |  16 +
>  2 files changed, 332 insertions(+)
> 
> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c
> index d3b689eab1c0..735482001f4d 100644
> --- a/drivers/net/ethernet/ti/inter-core-virt-eth.c
> +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c
> @@ -6,6 +6,50 @@
>  
>  #include "inter-core-virt-eth.h"
>  
> +#define ICVE_MIN_PACKET_SIZE	ETH_ZLEN
> +#define ICVE_MAX_PACKET_SIZE	(ETH_FRAME_LEN + ETH_FCS_LEN)
> +#define ICVE_MAX_TX_QUEUES	1
> +#define ICVE_MAX_RX_QUEUES	1
> +
> +#define TEST_DEBUG		1

Please remove TEST_DEBUG and associated code.
This does not seem appropriate for upstream.

> +
> +#ifdef TEST_DEBUG
> +#define ICVE_MAX_BUFFERS	100 //TODO : Set to power of 2 to leverage shift operations
> +#endif
> +
> +#define PKT_LEN_SIZE_TYPE	sizeof(u32)
> +
> +/* 4 bytes to hold packet length and ICVE_MAX_PACKET_SIZE to hold packet */
> +#define ICVE_BUFFER_SIZE	(ICVE_MAX_PACKET_SIZE + PKT_LEN_SIZE_TYPE)
> +
> +#define RX_POLL_TIMEOUT		250
> +
> +#define icve_ndev_to_priv(ndev) \
> +	((struct icve_ndev_priv *)netdev_priv(ndev))
> +#define icve_ndev_to_port(ndev) (icve_ndev_to_priv(ndev)->port)
> +#define icve_ndev_to_common(ndev) (icve_ndev_to_port(ndev)->common)
> +
> +static void icve_rx_timer(struct timer_list *timer)
> +{
> +	struct icve_port *port = from_timer(port, timer, rx_timer);
> +	struct napi_struct *napi;
> +	int num_pkts = 0;
> +	u32 head, tail;
> +
> +	head = port->rx_buffer->head;
> +	tail = port->rx_buffer->tail;
> +
> +	num_pkts = tail - head;
> +	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);

Networking (still) prefers code to be < 80 columns wide.
In this case I'd probably go for (completely untested!):

	num_pkts = tail - head;
	if (num_pkts <= 0)
		num_pkts = num_pkts + port->icve_max_buffers;

Please consider running checkpatch as it is run by the CI.

[1] https://github.com/linux-netdev/nipa/blob/main/tests/patch/checkpatch/checkpatch.sh`

> +
> +	napi = &port->rx_napi;
> +	if (num_pkts && likely(napi_schedule_prep(napi))) {
> +		__napi_schedule(napi);
> +	} else {
> +		mod_timer(&port->rx_timer, RX_POLL_TIMEOUT);

Smatch warns that mod_timer takes an absolute time rather than an offset.
So, perhaps (completely untested!):

		mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT);

> +	}

If any arm of an if statement has {} then all should,
but {} is not needed unless there is more than one statement
convered by conditions.

So (completely untested!):

	if (num_pkts && likely(napi_schedule_prep(napi)))
		__napi_schedule(napi);
	else
		mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT);

...

> @@ -85,11 +145,262 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg
>  	return ret;
>  }
>  
> +static int icve_rx_packets(struct napi_struct *napi, int budget)
> +{
> +	struct icve_port *port = container_of(napi, struct icve_port, rx_napi);
> +	u32 count, process_pkts;
> +	struct sk_buff *skb;
> +	u32 head, tail;
> +	u32 pkt_len;
> +	int num_pkts;

nit: Please use reverse xmas tree - longest line to shortest - for local
     variables in Networking code.

This tool can help achieve that.
* https://github.com/ecree-solarflare/xmastree

> +
> +	head = port->rx_buffer->head;
> +	tail = port->rx_buffer->tail;
> +
> +	num_pkts = tail - head;
> +	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);
> +	process_pkts = min(num_pkts, budget);
> +	count = 0;
> +	while (count < process_pkts) {
> +		memcpy((void *)&pkt_len,
> +		       (void *)port->rx_buffer->base_addr + ((head + count) * ICVE_BUFFER_SIZE),
> +		       PKT_LEN_SIZE_TYPE);

I don't think there is any need to cast to (void *) like this.

...

> +static netdev_tx_t icve_start_xmit(struct sk_buff *skb, struct net_device *ndev)
> +{
> +	struct icve_port *port = icve_ndev_to_port(ndev);
> +	struct ethhdr *ether;
> +	u32 head, tail;
> +	u32 num_pkts;
> +	u32 len;
> +
> +	ether = eth_hdr(skb);

ether is assigned but otherwise unused in this function.

Flaged by W=1 builds using both gcc-13 and clang-17.

> +	len = skb_headlen(skb);
> +
> +	head = port->tx_buffer->head;
> +	tail = port->tx_buffer->tail;
> +
> +	/* If the buffer queue is full, then drop packet */
> +	num_pkts = tail - head;
> +	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);

num_pkts is unsigned, so the condition above is always true.

Flagged by Coccinelle and Smatch.

> +	if ((num_pkts + 1) == port->icve_max_buffers) {
> +		netdev_warn(ndev, "Tx buffer full\n");
> +		goto ring_full;
> +	}
> +
> +	/* Copy length */
> +	memcpy((void *)port->tx_buffer->base_addr + (port->tx_buffer->tail * ICVE_BUFFER_SIZE),
> +	       (void *)&len, PKT_LEN_SIZE_TYPE);
> +
> +	/* Copy data to shared mem */
> +	memcpy((void *)(port->tx_buffer->base_addr + PKT_LEN_SIZE_TYPE) +
> +	       (port->tx_buffer->tail * ICVE_BUFFER_SIZE),
> +	       (void *)skb->data, len);
> +
> +#ifdef TEST_DEBUG
> +	/* For quick Rx path testing, inject Tx pkt back into network */
> +	test_tx_rx_path(skb, ndev);
> +#endif
> +	port->tx_buffer->tail = (port->tx_buffer->tail + 1) % port->icve_max_buffers;
> +
> +	dev_consume_skb_any(skb);
> +
> +	return NETDEV_TX_OK;
> +
> +ring_full:
> +	return NETDEV_TX_BUSY;
> +}
> +
> +static int icve_set_mac_address(struct net_device *ndev, void *addr)
> +{
> +	eth_mac_addr(ndev, addr);
> +
> +	/* TODO : Inform remote core about MAC address change */
> +	return 0;
> +}
> +
> +static const struct net_device_ops icve_netdev_ops = {
> +	.ndo_open		= icve_ndo_open,
> +	.ndo_stop		= icve_ndo_stop,
> +	.ndo_start_xmit		= icve_start_xmit,
> +	.ndo_set_mac_address	= icve_set_mac_address,
> +};
> +
> +static int icve_init_ndev(struct icve_common *common)
> +{
> +	struct device *dev = &common->rpdev->dev;
> +	struct icve_ndev_priv *ndev_priv;
> +	struct icve_port *port;
> +	static u32 port_id;
> +	int err;
> +
> +	port = common->port;
> +	port->common = common;
> +	port->port_id = port_id++;
> +
> +	port->ndev = devm_alloc_etherdev_mqs(common->dev, sizeof(*ndev_priv),
> +					     ICVE_MAX_TX_QUEUES,
> +					     ICVE_MAX_RX_QUEUES);
> +
> +	if (!port->ndev) {
> +		dev_err(dev, "error allocating net_device\n");
> +		return -ENOMEM;
> +	}
> +
> +	ndev_priv = netdev_priv(port->ndev);
> +	ndev_priv->port = port;
> +	SET_NETDEV_DEV(port->ndev, dev);
> +
> +	port->ndev->min_mtu = ICVE_MIN_PACKET_SIZE;
> +	port->ndev->max_mtu = ICVE_MAX_PACKET_SIZE;
> +	port->ndev->netdev_ops = &icve_netdev_ops;
> +
> +#ifdef TEST_DEBUG
> +	/* Allocate memory to test without actual RPMsg handshaking */
> +	port->tx_buffer = devm_kzalloc(dev, sizeof(port->tx_buffer),
> +				       GFP_KERNEL);

(I think this code should be removed, but in any case I'll flag
 the problems that I am aware of.)

This allocates the size of the port->tx_buffer pointer, rather than the
size of port->tx_buffer itself (4 or 8 bytes instead of 12 or 16 bytes).
So perhaps it should be (completely untested!):

	port->tx_buffer = devm_kzalloc(dev, sizeof(*port->tx_buffer),
				       GFP_KERNEL);

Flagged by Sparse.


> +	if (!port->tx_buffer) {
> +		dev_err(dev, "Memory not available\n");

Out of memory messages will be logged by the code.
So there is no need for the dev_err() call above.

> +		return -ENOMEM;
> +	}
> +
> +	port->tx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS,
> +						  GFP_KERNEL);
> +	if (!port->tx_buffer->base_addr) {
> +		dev_err(dev, "Memory not available\n");
> +		return -ENOMEM;
> +	}
> +
> +	port->rx_buffer = devm_kzalloc(dev, sizeof(port->rx_buffer),
> +				       GFP_KERNEL);
> +	if (!port->rx_buffer) {
> +		dev_err(dev, "Memory not available\n");
> +		return -ENOMEM;
> +	};
> +
> +	port->rx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS,
> +						  GFP_KERNEL);
> +	if (!port->rx_buffer->base_addr) {
> +		dev_err(dev, "Memory not available\n");
> +		return -ENOMEM;
> +	}
> +
> +	port->icve_max_buffers = ICVE_MAX_BUFFERS;
> +#else
> +	/* Shared memory details will be sent by the remote core.
> +	 * So turn off the carrier, until both the virtual port and
> +	 * remote core is ready
> +	 */
> +	netif_carrier_off(port->ndev);
> +
> +#endif
> +	err = register_netdev(port->ndev);
> +
> +	if (err)
> +		dev_err(dev, "error registering icve net device %d\n", err);
> +
> +	return 0;
> +}
> +

...

> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h

...

> @@ -70,7 +78,11 @@ struct shared_mem {
>  struct icve_port {
>  	struct shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */
>  	struct shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */
> +	struct timer_list rx_timer;
>  	struct icve_common *common;
> +	struct napi_struct rx_napi;
> +	u8 local_mac_addr[ETH_ALEN];

local_mac_addr does not appear to be used in this patch.
If so, please drop it and add it when it is needed.

> +	struct net_device *ndev;
>  	u32 icve_max_buffers;
>  	u32 port_id; /* Unique ID for the port : TODO: Define range for use by Linux and non linux */
>  

...

Re: [RFC PATCH net-next 2/2] net: ethernet: ti: inter-core-virt-eth: Register as network device

[Ravi Gunasekaran]

On 2/1/2024 6:49 PM, Simon Horman wrote:
> On Tue, Jan 30, 2024 at 04:39:44PM +0530, Ravi Gunasekaran wrote:
>> Register the RPMsg driver as network device and add support for
>> basic ethernet functionality by using the shared memory for data
>> plane.
>>
>> The shared memory layout is as below, with the region between
>> PKT_1_LEN to PKT_N modelled as circular buffer.
>>
>> -------------------------
>> |          HEAD         |
>> -------------------------
>> |          TAIL         |
>> -------------------------
>> |       PKT_1_LEN       |
>> |         PKT_1         |
>> -------------------------
>> |       PKT_2_LEN       |
>> |         PKT_2         |
>> -------------------------
>> |           .           |
>> |           .           |
>> -------------------------
>> |       PKT_N_LEN       |
>> |         PKT_N         |
>> -------------------------
>>
>> The offset between the HEAD and TAIL is polled to process the Rx packets.
>>
>> Signed-off-by: Siddharth Vadapalli <s-vadapalli@ti.com>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@ti.com>
> Hi Ravi,
>
> some feedback, mainly regarding issues flagged by linters and static analysis.
>
>> ---
>>  drivers/net/ethernet/ti/inter-core-virt-eth.c | 316 ++++++++++++++++++
>>  drivers/net/ethernet/ti/inter-core-virt-eth.h |  16 +
>>  2 files changed, 332 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.c b/drivers/net/ethernet/ti/inter-core-virt-eth.c
>> index d3b689eab1c0..735482001f4d 100644
>> --- a/drivers/net/ethernet/ti/inter-core-virt-eth.c
>> +++ b/drivers/net/ethernet/ti/inter-core-virt-eth.c
>> @@ -6,6 +6,50 @@
>>  
>>  #include "inter-core-virt-eth.h"
>>  
>> +#define ICVE_MIN_PACKET_SIZE	ETH_ZLEN
>> +#define ICVE_MAX_PACKET_SIZE	(ETH_FRAME_LEN + ETH_FCS_LEN)
>> +#define ICVE_MAX_TX_QUEUES	1
>> +#define ICVE_MAX_RX_QUEUES	1
>> +
>> +#define TEST_DEBUG		1
> Please remove TEST_DEBUG and associated code.
> This does not seem appropriate for upstream.

Sure,  I will do so.

>
>> +
>> +#ifdef TEST_DEBUG
>> +#define ICVE_MAX_BUFFERS	100 //TODO : Set to power of 2 to leverage shift operations
>> +#endif
>> +
>> +#define PKT_LEN_SIZE_TYPE	sizeof(u32)
>> +
>> +/* 4 bytes to hold packet length and ICVE_MAX_PACKET_SIZE to hold packet */
>> +#define ICVE_BUFFER_SIZE	(ICVE_MAX_PACKET_SIZE + PKT_LEN_SIZE_TYPE)
>> +
>> +#define RX_POLL_TIMEOUT		250
>> +
>> +#define icve_ndev_to_priv(ndev) \
>> +	((struct icve_ndev_priv *)netdev_priv(ndev))
>> +#define icve_ndev_to_port(ndev) (icve_ndev_to_priv(ndev)->port)
>> +#define icve_ndev_to_common(ndev) (icve_ndev_to_port(ndev)->common)
>> +
>> +static void icve_rx_timer(struct timer_list *timer)
>> +{
>> +	struct icve_port *port = from_timer(port, timer, rx_timer);
>> +	struct napi_struct *napi;
>> +	int num_pkts = 0;
>> +	u32 head, tail;
>> +
>> +	head = port->rx_buffer->head;
>> +	tail = port->rx_buffer->tail;
>> +
>> +	num_pkts = tail - head;
>> +	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);
> Networking (still) prefers code to be < 80 columns wide.
> In this case I'd probably go for (completely untested!):

I will take care of this from now on.

> 	num_pkts = tail - head;
> 	if (num_pkts <= 0)
> 		num_pkts = num_pkts + port->icve_max_buffers;
>
> Please consider running checkpatch as it is run by the CI.
>
> [1] https://github.com/linux-netdev/nipa/blob/main/tests/patch/checkpatch/checkpatch.sh`
>
>> +
>> +	napi = &port->rx_napi;
>> +	if (num_pkts && likely(napi_schedule_prep(napi))) {
>> +		__napi_schedule(napi);
>> +	} else {
>> +		mod_timer(&port->rx_timer, RX_POLL_TIMEOUT);
> Smatch warns that mod_timer takes an absolute time rather than an offset.
> So, perhaps (completely untested!):
>
> 		mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT);
>
>> +	}
> If any arm of an if statement has {} then all should,
> but {} is not needed unless there is more than one statement
> convered by conditions.
>
> So (completely untested!):
>
> 	if (num_pkts && likely(napi_schedule_prep(napi)))
> 		__napi_schedule(napi);
> 	else
> 		mod_timer(&port->rx_timer, jiffies + RX_POLL_TIMEOUT);
>
> ...

Noted. I had added debug prints between the {}, forgot take care the single
statement rule
while cleaning up. Not an excuse though.

>> @@ -85,11 +145,262 @@ static int create_request(struct icve_common *common, enum icve_rpmsg_type rpmsg
>>  	return ret;
>>  }
>>  
>> +static int icve_rx_packets(struct napi_struct *napi, int budget)
>> +{
>> +	struct icve_port *port = container_of(napi, struct icve_port, rx_napi);
>> +	u32 count, process_pkts;
>> +	struct sk_buff *skb;
>> +	u32 head, tail;
>> +	u32 pkt_len;
>> +	int num_pkts;
> nit: Please use reverse xmas tree - longest line to shortest - for local
>      variables in Networking code.
>
> This tool can help achieve that.
> * https://github.com/ecree-solarflare/xmastree

Thanks for sharing the link. I will use it from now on.

>> +
>> +	head = port->rx_buffer->head;
>> +	tail = port->rx_buffer->tail;
>> +
>> +	num_pkts = tail - head;
>> +	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);
>> +	process_pkts = min(num_pkts, budget);
>> +	count = 0;
>> +	while (count < process_pkts) {
>> +		memcpy((void *)&pkt_len,
>> +		       (void *)port->rx_buffer->base_addr + ((head + count) * ICVE_BUFFER_SIZE),
>> +		       PKT_LEN_SIZE_TYPE);
> I don't think there is any need to cast to (void *) like this.
>
> ...

Ok.

>
>> +static netdev_tx_t icve_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> +	struct icve_port *port = icve_ndev_to_port(ndev);
>> +	struct ethhdr *ether;
>> +	u32 head, tail;
>> +	u32 num_pkts;
>> +	u32 len;
>> +
>> +	ether = eth_hdr(skb);
> ether is assigned but otherwise unused in this function.
>
> Flaged by W=1 builds using both gcc-13 and clang-17.
>
>> +	len = skb_headlen(skb);
>> +
>> +	head = port->tx_buffer->head;
>> +	tail = port->tx_buffer->tail;
>> +
>> +	/* If the buffer queue is full, then drop packet */
>> +	num_pkts = tail - head;
>> +	num_pkts = num_pkts >= 0 ? num_pkts : (num_pkts + port->icve_max_buffers);
> num_pkts is unsigned, so the condition above is always true.
>
> Flagged by Coccinelle and Smatch.
>
>> +	if ((num_pkts + 1) == port->icve_max_buffers) {
>> +		netdev_warn(ndev, "Tx buffer full\n");
>> +		goto ring_full;
>> +	}
>> +
>> +	/* Copy length */
>> +	memcpy((void *)port->tx_buffer->base_addr + (port->tx_buffer->tail * ICVE_BUFFER_SIZE),
>> +	       (void *)&len, PKT_LEN_SIZE_TYPE);
>> +
>> +	/* Copy data to shared mem */
>> +	memcpy((void *)(port->tx_buffer->base_addr + PKT_LEN_SIZE_TYPE) +
>> +	       (port->tx_buffer->tail * ICVE_BUFFER_SIZE),
>> +	       (void *)skb->data, len);
>> +
>> +#ifdef TEST_DEBUG
>> +	/* For quick Rx path testing, inject Tx pkt back into network */
>> +	test_tx_rx_path(skb, ndev);
>> +#endif
>> +	port->tx_buffer->tail = (port->tx_buffer->tail + 1) % port->icve_max_buffers;
>> +
>> +	dev_consume_skb_any(skb);
>> +
>> +	return NETDEV_TX_OK;
>> +
>> +ring_full:
>> +	return NETDEV_TX_BUSY;
>> +}
>> +
>> +static int icve_set_mac_address(struct net_device *ndev, void *addr)
>> +{
>> +	eth_mac_addr(ndev, addr);
>> +
>> +	/* TODO : Inform remote core about MAC address change */
>> +	return 0;
>> +}
>> +
>> +static const struct net_device_ops icve_netdev_ops = {
>> +	.ndo_open		= icve_ndo_open,
>> +	.ndo_stop		= icve_ndo_stop,
>> +	.ndo_start_xmit		= icve_start_xmit,
>> +	.ndo_set_mac_address	= icve_set_mac_address,
>> +};
>> +
>> +static int icve_init_ndev(struct icve_common *common)
>> +{
>> +	struct device *dev = &common->rpdev->dev;
>> +	struct icve_ndev_priv *ndev_priv;
>> +	struct icve_port *port;
>> +	static u32 port_id;
>> +	int err;
>> +
>> +	port = common->port;
>> +	port->common = common;
>> +	port->port_id = port_id++;
>> +
>> +	port->ndev = devm_alloc_etherdev_mqs(common->dev, sizeof(*ndev_priv),
>> +					     ICVE_MAX_TX_QUEUES,
>> +					     ICVE_MAX_RX_QUEUES);
>> +
>> +	if (!port->ndev) {
>> +		dev_err(dev, "error allocating net_device\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	ndev_priv = netdev_priv(port->ndev);
>> +	ndev_priv->port = port;
>> +	SET_NETDEV_DEV(port->ndev, dev);
>> +
>> +	port->ndev->min_mtu = ICVE_MIN_PACKET_SIZE;
>> +	port->ndev->max_mtu = ICVE_MAX_PACKET_SIZE;
>> +	port->ndev->netdev_ops = &icve_netdev_ops;
>> +
>> +#ifdef TEST_DEBUG
>> +	/* Allocate memory to test without actual RPMsg handshaking */
>> +	port->tx_buffer = devm_kzalloc(dev, sizeof(port->tx_buffer),
>> +				       GFP_KERNEL);
> (I think this code should be removed, but in any case I'll flag
>  the problems that I am aware of.)
>
> This allocates the size of the port->tx_buffer pointer, rather than the
> size of port->tx_buffer itself (4 or 8 bytes instead of 12 or 16 bytes).
> So perhaps it should be (completely untested!):
>
> 	port->tx_buffer = devm_kzalloc(dev, sizeof(*port->tx_buffer),
> 				       GFP_KERNEL);
>
> Flagged by Sparse.

My bad, a mistake on my part.

>
>> +	if (!port->tx_buffer) {
>> +		dev_err(dev, "Memory not available\n");
> Out of memory messages will be logged by the code.
> So there is no need for the dev_err() call above.
>
>> +		return -ENOMEM;
>> +	}
>> +
>> +	port->tx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS,
>> +						  GFP_KERNEL);
>> +	if (!port->tx_buffer->base_addr) {
>> +		dev_err(dev, "Memory not available\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	port->rx_buffer = devm_kzalloc(dev, sizeof(port->rx_buffer),
>> +				       GFP_KERNEL);
>> +	if (!port->rx_buffer) {
>> +		dev_err(dev, "Memory not available\n");
>> +		return -ENOMEM;
>> +	};
>> +
>> +	port->rx_buffer->base_addr = devm_kzalloc(dev, ICVE_BUFFER_SIZE * ICVE_MAX_BUFFERS,
>> +						  GFP_KERNEL);
>> +	if (!port->rx_buffer->base_addr) {
>> +		dev_err(dev, "Memory not available\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	port->icve_max_buffers = ICVE_MAX_BUFFERS;
>> +#else
>> +	/* Shared memory details will be sent by the remote core.
>> +	 * So turn off the carrier, until both the virtual port and
>> +	 * remote core is ready
>> +	 */
>> +	netif_carrier_off(port->ndev);
>> +
>> +#endif
>> +	err = register_netdev(port->ndev);
>> +
>> +	if (err)
>> +		dev_err(dev, "error registering icve net device %d\n", err);
>> +
>> +	return 0;
>> +}
>> +
> ...
>
>> diff --git a/drivers/net/ethernet/ti/inter-core-virt-eth.h b/drivers/net/ethernet/ti/inter-core-virt-eth.h
> ...
>
>> @@ -70,7 +78,11 @@ struct shared_mem {
>>  struct icve_port {
>>  	struct shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */
>>  	struct shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */
>> +	struct timer_list rx_timer;
>>  	struct icve_common *common;
>> +	struct napi_struct rx_napi;
>> +	u8 local_mac_addr[ETH_ALEN];
> local_mac_addr does not appear to be used in this patch.
> If so, please drop it and add it when it is needed.

Noted.

>
>> +	struct net_device *ndev;
>>  	u32 icve_max_buffers;
>>  	u32 port_id; /* Unique ID for the port : TODO: Define range for use by Linux and non linux */
>>  
> ...
Thanks for taking time to review the patches.

The primary intention of this series was to know if the RPMsg based approach
would be upstream friendly or not. But I would not like to use that as an excuse
for not fixing checks/warnings/errors reported by checkpatch completely.
Even though if its RFC, I will treat it as an actual upstream patch  and address the
checkpatch/smatch/sparse findings or atleast mention in the cover letter that the
findings have not been fully addressed.

I apologize for the inconvenience caused and be careful in future. 

Re: [RFC PATCH net-next 2/2] net: ethernet: ti: inter-core-virt-eth: Register as network device

[Simon Horman]

On Thu, Feb 01, 2024 at 07:54:24PM +0530, Ravi Gunasekaran wrote:

...

> Thanks for taking time to review the patches.
> 
> The primary intention of this series was to know if the RPMsg based approach
> would be upstream friendly or not. But I would not like to use that as an excuse
> for not fixing checks/warnings/errors reported by checkpatch completely.
> Even though if its RFC, I will treat it as an actual upstream patch  and address the
> checkpatch/smatch/sparse findings or atleast mention in the cover letter that the
> findings have not been fully addressed.

Understood. TBH, I am unsure of the value of this kind of review for an RFC
- I understand it is important to get the bigger picture questions out of
the way at this point.

...