Message-ID: <ZpEz-o1Dkg1gF_ud@lore-desk>
Date: Fri, 12 Jul 2024 15:47:38 +0200
From: Lorenzo Bianconi <lorenzo@...nel.org>
To: Jakub Kicinski <kuba@...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?

The Airoha eth SoC architecture is similar to mtk_eth_soc one (e.g MT7988a).
The FrameEngine (FE) module has multiple GDM ports that are connected to
different blocks. Current airoha_eth driver supports just GDM1 that is connected
to a MT7530 DSA switch (I have not posted a tiny patch for mt7530 driver yet).
In the future we will support even GDM{2,3,4} that will connect to differ
phy modues (e.g. 2.5Gbps phy).

> 
> > +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)

ack, I will fix it in v8

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

ack, I will fix it in v8

> 
> > +	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?

reviewing this chunk, I guess we can get rid of it since we already block the
txq at the end of airoha_dev_xmit() if we do not have enough space for the next
packet:

	if (q->ndesc - q->queued < q->free_thr)
		netif_tx_stop_queue(txq);

> 
> > +	}
> > +
> > +	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..

ack, I guess we can use .get_eth_mac_stats() and .get_rmon_stats() callbacks
and get rid of .get_ethtool_stats() one since it will duplicate stats
otherwise.

> 
> > +	"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 😮️

ack, we have the same architecture here, rx queues/page_pools are shared
between net_devices. So far I used page_pool stats just for debugging so
I will remove them for the moment till we have a good/defined solution
for this kind of architecture.

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

ack, I will fix it in v8.

Regards,
Lorenzo

Download attachment "signature.asc" of type "application/pgp-signature" (229 bytes)

Powered by blists - more mailing lists