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.
> 
> 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"

[...]

>  
> +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;
> +	case ICVE_REQ_SET_MAC_ADDR:
> +		msg->msg_hdr.msg_type = ICVE_REQUEST_MSG;
> +		ether_addr_copy(msg->req_msg.mac_addr.addr,
> +				common->port->ndev->dev_addr);
> +		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");
> +	};
> +	return ret;
> +}
> +
> +static int icve_create_send_request(struct icve_common *common,
> +				    enum icve_rpmsg_type rpmsg_type,
> +				    bool wait)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	if (wait)
> +		reinit_completion(&common->sync_msg);
> +
> +	spin_lock_irqsave(&common->send_msg_lock, flags);
> +	create_request(common, rpmsg_type);

Why isn't the return value of create_request() being checked?
If it is guaranteed to always return 0 based on the design, convert it
to a void function.

> +	rpmsg_send(common->rpdev->ept, (void *)(&common->send_msg),
> +		   sizeof(common->send_msg));
> +	spin_unlock_irqrestore(&common->send_msg_lock, flags);
> +
> +	if (wait) {
> +		ret = wait_for_completion_timeout(&common->sync_msg,
> +						  ICVE_REQ_TIMEOUT);
> +
> +		if (!ret) {
> +			dev_err(common->dev, "Failed to receive response within %ld jiffies\n",
> +				ICVE_REQ_TIMEOUT);
> +			ret = -ETIMEDOUT;
> +			return ret;
> +		}
> +	}
> +	return ret;
> +}
> +
> +static void icve_state_machine(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct icve_common *common;
> +	struct icve_port *port;
> +
> +	common = container_of(dwork, struct icve_common, state_work);
> +	port = common->port;
> +
> +	mutex_lock(&common->state_lock);
> +
> +	switch (common->state) {
> +	case ICVE_STATE_PROBE:
> +		break;
> +	case ICVE_STATE_OPEN:
> +		icve_create_send_request(common, ICVE_REQ_SHM_INFO, false);

The return value of icve_create_send_request() is not being checked. Is
it guaranteed to succeed? Where is the error handling path if
icve_create_send_request() fails?

> +		break;
> +	case ICVE_STATE_CLOSE:
> +		break;
> +	case ICVE_STATE_READY:
> +		icve_create_send_request(common, ICVE_REQ_SET_MAC_ADDR, false);

Same here and at all other places where icve_create_send_request() is
being invoked. The icve_create_send_request() seems to be newly added in
this version of the series and wasn't there in the RFC patch. This should
be mentioned in the Changelog.

[...]

Regards,
Siddharth.