Message-ID: <20240711181003.4089a633@kernel.org>
Date: Thu, 11 Jul 2024 18:10:03 -0700
From: Jakub Kicinski <kuba@...nel.org>
To: Lorenzo Bianconi <lorenzo@...nel.org>
Cc: netdev@...r.kernel.org, nbd@....name, lorenzo.bianconi83@...il.com,
 davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
 conor@...nel.org, linux-arm-kernel@...ts.infradead.org, robh+dt@...nel.org,
 krzysztof.kozlowski+dt@...aro.org, conor+dt@...nel.org,
 devicetree@...r.kernel.org, catalin.marinas@....com, will@...nel.org,
 upstream@...oha.com, angelogioacchino.delregno@...labora.com,
 benjamin.larsson@...exis.eu, rkannoth@...vell.com, sgoutham@...vell.com,
 andrew@...n.ch, arnd@...db.de, horms@...nel.org
Subject: Re: [PATCH v7 net-next 2/2] net: airoha: Introduce ethernet support
 for EN7581 SoC

On Wed, 10 Jul 2024 10:47:41 +0200 Lorenzo Bianconi wrote:
> Add airoha_eth driver in order to introduce ethernet support for
> Airoha EN7581 SoC available on EN7581 development board (en7581-evb).
> en7581-evb networking architecture is composed by airoha_eth as mac
> controller (cpu port) and a mt7530 dsa based switch.
> EN7581 mac controller is mainly composed by Frame Engine (FE) and
> QoS-DMA (QDMA) modules. FE is used for traffic offloading (just basic
> functionalities are supported now) while QDMA is used for DMA operation
> and QOS functionalities between mac layer and the dsa switch (hw QoS is
> not available yet and it will be added in the future).
> Currently only hw lan features are available, hw wan will be added with
> subsequent patches.

It seems a bit unusual for DSA to have multiple ports, isn't it?
Can you explain how this driver differs from normal DSA a little 
more in the commit message?

> +static void airoha_dev_get_stats64(struct net_device *dev,
> +				   struct rtnl_link_stats64 *storage)
> +{
> +	struct airoha_gdm_port *port = netdev_priv(dev);
> +
> +	mutex_lock(&port->stats.mutex);

can't take sleeping locks here :( Gotta do periodic updates from 
a workqueue or spinlock. there are callers under RCU (annoyingly)

> +	airoha_update_hw_stats(port);
> +	storage->rx_packets = port->stats.rx_ok_pkts;
> +	storage->tx_packets = port->stats.tx_ok_pkts;
> +	storage->rx_bytes = port->stats.rx_ok_bytes;
> +	storage->tx_bytes = port->stats.tx_ok_bytes;
> +	storage->multicast = port->stats.rx_multicast;
> +	storage->rx_errors = port->stats.rx_errors;
> +	storage->rx_dropped = port->stats.rx_drops;
> +	storage->tx_dropped = port->stats.tx_drops;
> +	storage->rx_crc_errors = port->stats.rx_crc_error;
> +	storage->rx_over_errors = port->stats.rx_over_errors;
> +
> +	mutex_unlock(&port->stats.mutex);
> +}
> +
> +static netdev_tx_t airoha_dev_xmit(struct sk_buff *skb,
> +				   struct net_device *dev)
> +{
> +	struct skb_shared_info *sinfo = skb_shinfo(skb);
> +	struct airoha_gdm_port *port = netdev_priv(dev);
> +	int i, qid = skb_get_queue_mapping(skb);
> +	struct airoha_eth *eth = port->eth;
> +	u32 nr_frags, msg0 = 0, msg1;
> +	u32 len = skb_headlen(skb);
> +	struct netdev_queue *txq;
> +	struct airoha_queue *q;
> +	void *data = skb->data;
> +	u16 index;
> +	u8 fport;
> +
> +	if (skb->ip_summed == CHECKSUM_PARTIAL)
> +		msg0 |= FIELD_PREP(QDMA_ETH_TXMSG_TCO_MASK, 1) |
> +			FIELD_PREP(QDMA_ETH_TXMSG_UCO_MASK, 1) |
> +			FIELD_PREP(QDMA_ETH_TXMSG_ICO_MASK, 1);
> +
> +	/* TSO: fill MSS info in tcp checksum field */
> +	if (skb_is_gso(skb)) {
> +		if (skb_cow_head(skb, 0))
> +			goto error;
> +
> +		if (sinfo->gso_type & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6)) {
> +			__be16 csum = cpu_to_be16(sinfo->gso_size);
> +
> +			tcp_hdr(skb)->check = (__force __sum16)csum;
> +			msg0 |= FIELD_PREP(QDMA_ETH_TXMSG_TSO_MASK, 1);
> +		}
> +	}
> +
> +	fport = port->id == 4 ? FE_PSE_PORT_GDM4 : port->id;
> +	msg1 = FIELD_PREP(QDMA_ETH_TXMSG_FPORT_MASK, fport) |
> +	       FIELD_PREP(QDMA_ETH_TXMSG_METER_MASK, 0x7f);
> +
> +	if (WARN_ON_ONCE(qid >= ARRAY_SIZE(eth->q_tx)))
> +		qid = 0;

Hm, how? Stack should protect against that.

> +	q = &eth->q_tx[qid];
> +	if (WARN_ON_ONCE(!q->ndesc))
> +		goto error;
> +
> +	spin_lock_bh(&q->lock);
> +
> +	nr_frags = 1 + sinfo->nr_frags;
> +	if (q->queued + nr_frags > q->ndesc) {
> +		/* not enough space in the queue */
> +		spin_unlock_bh(&q->lock);
> +		return NETDEV_TX_BUSY;

no need to stop the queue?

> +	}
> +
> +	index = q->head;
> +	for (i = 0; i < nr_frags; i++) {
> +		struct airoha_qdma_desc *desc = &q->desc[index];
> +		struct airoha_queue_entry *e = &q->entry[index];
> +		skb_frag_t *frag = &sinfo->frags[i];
> +		dma_addr_t addr;
> +		u32 val;
> +
> +		addr = dma_map_single(dev->dev.parent, data, len,
> +				      DMA_TO_DEVICE);
> +		if (unlikely(dma_mapping_error(dev->dev.parent, addr)))
> +			goto error_unmap;
> +
> +		index = (index + 1) % q->ndesc;
> +
> +		val = FIELD_PREP(QDMA_DESC_LEN_MASK, len);
> +		if (i < nr_frags - 1)
> +			val |= FIELD_PREP(QDMA_DESC_MORE_MASK, 1);
> +		WRITE_ONCE(desc->ctrl, cpu_to_le32(val));
> +		WRITE_ONCE(desc->addr, cpu_to_le32(addr));
> +		val = FIELD_PREP(QDMA_DESC_NEXT_ID_MASK, index);
> +		WRITE_ONCE(desc->data, cpu_to_le32(val));
> +		WRITE_ONCE(desc->msg0, cpu_to_le32(msg0));
> +		WRITE_ONCE(desc->msg1, cpu_to_le32(msg1));
> +		WRITE_ONCE(desc->msg2, cpu_to_le32(0xffff));
> +
> +		e->skb = i ? NULL : skb;
> +		e->dma_addr = addr;
> +		e->dma_len = len;
> +
> +		airoha_qdma_rmw(eth, REG_TX_CPU_IDX(qid), TX_RING_CPU_IDX_MASK,
> +				FIELD_PREP(TX_RING_CPU_IDX_MASK, index));
> +
> +		data = skb_frag_address(frag);
> +		len = skb_frag_size(frag);
> +	}
> +
> +	q->head = index;
> +	q->queued += i;
> +
> +	txq = netdev_get_tx_queue(dev, qid);
> +	netdev_tx_sent_queue(txq, skb->len);
> +	skb_tx_timestamp(skb);
> +
> +	if (q->ndesc - q->queued < q->free_thr)
> +		netif_tx_stop_queue(txq);
> +
> +	spin_unlock_bh(&q->lock);
> +
> +	return NETDEV_TX_OK;
> +
> +error_unmap:
> +	for (i--; i >= 0; i++)
> +		dma_unmap_single(dev->dev.parent, q->entry[i].dma_addr,
> +				 q->entry[i].dma_len, DMA_TO_DEVICE);
> +
> +	spin_unlock_bh(&q->lock);
> +error:
> +	dev_kfree_skb_any(skb);
> +	dev->stats.tx_dropped++;
> +
> +	return NETDEV_TX_OK;
> +}
> +
> +static const char * const airoha_ethtool_stats_name[] = {
> +	"tx_eth_bc_cnt",
> +	"tx_eth_mc_cnt",

struct ethtool_eth_mac_stats

> +	"tx_eth_lt64_cnt",
> +	"tx_eth_eq64_cnt",
> +	"tx_eth_65_127_cnt",
> +	"tx_eth_128_255_cnt",
> +	"tx_eth_256_511_cnt",
> +	"tx_eth_512_1023_cnt",
> +	"tx_eth_1024_1518_cnt",
> +	"tx_eth_gt1518_cnt",

struct ethtool_rmon_stats 

> +	"rx_eth_bc_cnt",
> +	"rx_eth_frag_cnt",
> +	"rx_eth_jabber_cnt",

those are also covered.. somewhere..

> +	"rx_eth_fc_drops",
> +	"rx_eth_rc_drops",
> +	"rx_eth_lt64_cnt",
> +	"rx_eth_eq64_cnt",
> +	"rx_eth_65_127_cnt",
> +	"rx_eth_128_255_cnt",
> +	"rx_eth_256_511_cnt",
> +	"rx_eth_512_1023_cnt",
> +	"rx_eth_1024_1518_cnt",
> +	"rx_eth_gt1518_cnt",
> +};
> +
> +static void airoha_ethtool_get_drvinfo(struct net_device *dev,
> +				       struct ethtool_drvinfo *info)
> +{
> +	struct airoha_gdm_port *port = netdev_priv(dev);
> +	struct airoha_eth *eth = port->eth;
> +
> +	strscpy(info->driver, eth->dev->driver->name, sizeof(info->driver));
> +	strscpy(info->bus_info, dev_name(eth->dev), sizeof(info->bus_info));
> +	info->n_stats = ARRAY_SIZE(airoha_ethtool_stats_name) +
> +			page_pool_ethtool_stats_get_count();
> +}
> +
> +static void airoha_ethtool_get_strings(struct net_device *dev, u32 sset,
> +				       u8 *data)
> +{
> +	int i;
> +
> +	if (sset != ETH_SS_STATS)
> +		return;
> +
> +	for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++)
> +		ethtool_puts(&data, airoha_ethtool_stats_name[i]);
> +
> +	page_pool_ethtool_stats_get_strings(data);
> +}
> +
> +static int airoha_ethtool_get_sset_count(struct net_device *dev, int sset)
> +{
> +	if (sset != ETH_SS_STATS)
> +		return -EOPNOTSUPP;
> +
> +	return ARRAY_SIZE(airoha_ethtool_stats_name) +
> +	       page_pool_ethtool_stats_get_count();
> +}
> +
> +static void airoha_ethtool_get_stats(struct net_device *dev,
> +				     struct ethtool_stats *stats, u64 *data)
> +{
> +	int off = offsetof(struct airoha_hw_stats, tx_broadcast) / sizeof(u64);
> +	struct airoha_gdm_port *port = netdev_priv(dev);
> +	u64 *hw_stats = (u64 *)&port->stats + off;
> +	struct page_pool_stats pp_stats = {};
> +	struct airoha_eth *eth = port->eth;
> +	int i;
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(airoha_ethtool_stats_name) + off !=
> +		     sizeof(struct airoha_hw_stats) / sizeof(u64));
> +
> +	mutex_lock(&port->stats.mutex);
> +
> +	airoha_update_hw_stats(port);
> +	for (i = 0; i < ARRAY_SIZE(airoha_ethtool_stats_name); i++)
> +		*data++ = hw_stats[i];
> +
> +	for (i = 0; i < ARRAY_SIZE(eth->q_rx); i++) {
> +		if (!eth->q_rx[i].ndesc)
> +			continue;
> +
> +		page_pool_get_stats(eth->q_rx[i].page_pool, &pp_stats);
> +	}
> +	page_pool_ethtool_stats_get(data, &pp_stats);

We can't use the netlink stats because of the shared DMA / shared
device? mlxsw had a similar problem recently:

https://lore.kernel.org/all/20240625120807.1165581-1-amcohen@nvidia.com/

Can we list the stats without a netdev or add a new netlink attr
to describe such devices? BTW setting pp->netdev to an unregistered
device is probably a bad idea, we should add a WARN_ON() for that
if we don't have one 😮️

> +	for_each_child_of_node(pdev->dev.of_node, np) {
> +		if (!of_device_is_compatible(np, "airoha,eth-mac"))
> +			continue;
> +
> +		if (!of_device_is_available(np))
> +			continue;
> +
> +		err = airoha_alloc_gdm_port(eth, np);
> +		if (err) {
> +			of_node_put(np);
> +			goto error;
> +		}
> +	}
> +
> +	airoha_qdma_start_napi(eth);

Are you sure starting the NAPI after registration is safe?
Nothing will go wrong if interface gets opened before
airoha_qdma_start_napi() gets to run?

Also you don't seem to call napi_disable(), NAPI can reschedule itself,
and netif_napi_del() doesn't wait for quiescence.

Powered by blists - more mailing lists