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

[Siddharth Vadapalli]

On Fri, May 31, 2024 at 12:10:05PM +0530, Yojana Mallik 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.

The author of this patch from the RFC series is:
Ravi Gunasekaran <r-gunasekaran@xxxxxx>
The authorship should be preserved across versions unless the
implementation has changed drastically requiring major rework
(which doesn't seem to be the case for this patch based on the
Changelog).

> 
> Signed-off-by: Yojana Mallik <y-mallik@xxxxxx>
> ---
>  drivers/net/ethernet/ti/icve_rpmsg_common.h   |  86 ++++
>  drivers/net/ethernet/ti/inter_core_virt_eth.c | 453 +++++++++++++++++-
>  drivers/net/ethernet/ti/inter_core_virt_eth.h |  35 +-
>  3 files changed, 570 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/icve_rpmsg_common.h b/drivers/net/ethernet/ti/icve_rpmsg_common.h
> index 7cd157479d4d..2e3833de14bd 100644
> --- a/drivers/net/ethernet/ti/icve_rpmsg_common.h
> +++ b/drivers/net/ethernet/ti/icve_rpmsg_common.h
> @@ -15,14 +15,58 @@ enum icve_msg_type {
>  	ICVE_NOTIFY_MSG,
>  };

[...]

>  
> +
>  #endif /* __ICVE_RPMSG_COMMON_H__ */
> diff --git a/drivers/net/ethernet/ti/inter_core_virt_eth.c b/drivers/net/ethernet/ti/inter_core_virt_eth.c
> index bea822d2373a..d96547d317fe 100644
> --- a/drivers/net/ethernet/ti/inter_core_virt_eth.c
> +++ b/drivers/net/ethernet/ti/inter_core_virt_eth.c
> @@ -6,11 +6,145 @@
>  
>  #include "inter_core_virt_eth.h"
>  
> +#define ICVE_MIN_PACKET_SIZE ETH_ZLEN
> +#define ICVE_MAX_PACKET_SIZE 1540 //(ETH_FRAME_LEN + ETH_FCS_LEN)

Is the commented portion above required?

> +#define ICVE_MAX_TX_QUEUES 1
> +#define ICVE_MAX_RX_QUEUES 1
> +
> +#define PKT_LEN_SIZE_TYPE sizeof(u32)
> +#define MAGIC_NUM_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 + MAGIC_NUM_SIZE_TYPE)
> +
> +#define RX_POLL_TIMEOUT 1000 /* 1000usec */

The macro name could be updated to contain "USEC" to make it clear that
the units are in microseconds. Same comment applies to other macros
below where they can be named to contain the units.

> +#define RX_POLL_JIFFIES (jiffies + usecs_to_jiffies(RX_POLL_TIMEOUT))
> +
> +#define STATE_MACHINE_TIME msecs_to_jiffies(100)
> +#define ICVE_REQ_TIMEOUT msecs_to_jiffies(100)
> +
> +#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)

[...]

>  	u32 msg_type = msg->msg_hdr.msg_type;
>  	u32 rpmsg_type;
>  
> @@ -24,11 +158,79 @@ static int icve_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len,
>  		rpmsg_type = msg->resp_msg.type;
>  		dev_dbg(common->dev, "Msg type = %d; RPMsg type = %d\n",
>  			msg_type, rpmsg_type);
> +		switch (rpmsg_type) {
> +		case ICVE_RESP_SHM_INFO:
> +			/* Retrieve Tx and Rx shared memory info from msg */
> +			port->tx_buffer->head =

[...]

> +					sizeof(*port->rx_buffer->tail));
> +
> +			port->icve_rx_max_buffers =
> +				msg->resp_msg.shm_info.shm_info_rx.num_pkt_bufs;
> +
> +			mutex_lock(&common->state_lock);
> +			common->state = ICVE_STATE_READY;
> +			mutex_unlock(&common->state_lock);
> +
> +			mod_delayed_work(system_wq,
> +					 &common->state_work,
> +					 STATE_MACHINE_TIME);
> +
> +			break;
> +		case ICVE_RESP_SET_MAC_ADDR:
> +			break;
> +		}
> +
>  		break;
> +
>  	case ICVE_NOTIFY_MSG:
>  		rpmsg_type = msg->notify_msg.type;
> -		dev_dbg(common->dev, "Msg type = %d; RPMsg type = %d\n",
> -			msg_type, rpmsg_type);

Why does the debug message above have to be deleted? If it was not
required, it could have been omitted in the previous patch itself,
rather than adding it in the previous patch and removing it here.

> +		switch (rpmsg_type) {
> +		case ICVE_NOTIFY_REMOTE_READY:
> +			mutex_lock(&common->state_lock);
> +			common->state = ICVE_STATE_RUNNING;
> +			mutex_unlock(&common->state_lock);
> +
> +			mod_delayed_work(system_wq,
> +					 &common->state_work,
> +					 STATE_MACHINE_TIME);
> +			break;
> +		case ICVE_NOTIFY_PORT_UP:
> +		case ICVE_NOTIFY_PORT_DOWN:
> +			break;
> +		}
>  		break;
>  	default:

[...]

>  }
>  
> diff --git a/drivers/net/ethernet/ti/inter_core_virt_eth.h b/drivers/net/ethernet/ti/inter_core_virt_eth.h
> index 91a3aba96996..4fc420cb9eab 100644
> --- a/drivers/net/ethernet/ti/inter_core_virt_eth.h
> +++ b/drivers/net/ethernet/ti/inter_core_virt_eth.h
> @@ -14,14 +14,45 @@
>  #include <linux/rpmsg.h>
>  #include "icve_rpmsg_common.h"
>  
> +enum icve_state {
> +	ICVE_STATE_PROBE,
> +	ICVE_STATE_OPEN,
> +	ICVE_STATE_CLOSE,
> +	ICVE_STATE_READY,
> +	ICVE_STATE_RUNNING,
> +
> +};
> +
>  struct icve_port {
> +	struct icve_shared_mem *tx_buffer; /* Write buffer for data to be consumed remote side */
> +	struct icve_shared_mem *rx_buffer; /* Read buffer for data to be consumed by this driver */
> +	struct timer_list rx_timer;
>  	struct icve_common *common;
> -} __packed;

Is the "__packed" attribute no longer required, or was it overlooked?

> +	struct napi_struct rx_napi;
> +	u8 local_mac_addr[ETH_ALEN];
> +	struct net_device *ndev;
> +	u32 icve_tx_max_buffers;
> +	u32 icve_rx_max_buffers;
> +	u32 port_id;
> +};
>  
>  struct icve_common {
>  	struct rpmsg_device *rpdev;
> +	spinlock_t send_msg_lock; /* Acquire this lock while sending RPMsg */
> +	spinlock_t recv_msg_lock; /* Acquire this lock while processing received RPMsg */
> +	struct message send_msg;
> +	struct message recv_msg;
>  	struct icve_port *port;
>  	struct device *dev;
> -} __packed;

Same comment here as well.

[...]

There seem to be a lot of changes compared to the RFC patch which
haven't been mentioned in the Changelog. Please mention all the changes
when posting new versions.

Regards,
Siddharth.