Date: Thu, 1 Feb 2024 19:54:24 +0530
From: Ravi Gunasekaran <r-gunasekaran@...com>
To: Simon Horman <horms@...nel.org>
CC: <davem@...emloft.net>, <edumazet@...gle.com>, <kuba@...nel.org>,
        <pabeni@...hat.com>, <andrew@...n.ch>, <rogerq@...nel.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <s-vadapalli@...com>, <srk@...com>
Subject: Re: [RFC PATCH net-next 2/2] net: ethernet: ti: inter-core-virt-eth:
 Register as network device



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@...com>
>> Signed-off-by: Ravi Gunasekaran <r-gunasekaran@...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. 

Powered by blists - more mailing lists