Received-SPF: pass (google.com: domain of linux-kernel+bounces-198154-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 2 Jun 2024 13:05:11 +0530
From: Siddharth Vadapalli <s-vadapalli@ti.com>
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>,
        <s-vadapalli@ti.com>
Subject: Re: [PATCH net-next v2 2/3] net: ethernet: ti: Register the RPMsg
 driver as network device
Message-ID: <70166bc4-2c55-4a46-b442-4a3c49d6d64d@ti.com>
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>

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@ti.com>
> ---
>  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.