Re: [PATCH net-next v4 2/2] net: spacemit: Add K1 Ethernet MAC

[Simon Horman <horms@kernel.org>]

On Thu, Jul 03, 2025 at 05:42:03PM +0800, Vivian Wang wrote:
> The Ethernet MACs found on SpacemiT K1 appears to be a custom design
> that only superficially resembles some other embedded MACs. SpacemiT
> refers to them as "EMAC", so let's just call the driver "k1_emac".
> 
> This driver is based on "k1x-emac" in the same directory in the vendor's
> tree [1]. Some debugging tunables have been fixed to vendor-recommended
> defaults, and PTP support is not included yet.
> 
> [1]: https://github.com/spacemit-com/linux-k1x
> 
> Tested-by: Junhui Liu <junhui.liu@pigmoral.tech>
> Signed-off-by: Vivian Wang <wangruikang@iscas.ac.cn>

...

> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c

...

> +/**
> + * struct emac_desc_ring - Software-side information for one descriptor ring
> + * Same struture used for both RX and TX

nit: structure

> + * @desc_addr: Virtual address to the descriptor ring memory
> + * @desc_dma_addr: DMA address of the descriptor ring
> + * @total_size: Size of ring in bytes
> + * @total_cnt: Number of descriptors
> + * @head: Next descriptor to associate a buffer with
> + * @tail: Next descriptor to check status bit
> + * @rx_desc_buf: Array of descriptors for RX
> + * @tx_desc_buf: Array of descriptors for TX, with max of two buffers each
> + */

...

> +static void emac_set_mac_addr(struct emac_priv *priv, const unsigned char *addr)
> +{
> +	emac_wr(priv, MAC_ADDRESS1_HIGH, ((addr[1] << 8) | addr[0]));

nit: no need for inner parentheses here,
     the order of operations is on your side

	emac_wr(priv, MAC_ADDRESS1_HIGH, addr[1] << 8 | addr[0]);

> +	emac_wr(priv, MAC_ADDRESS1_MED, ((addr[3] << 8) | addr[2]));
> +	emac_wr(priv, MAC_ADDRESS1_LOW, ((addr[5] << 8) | addr[4]));
> +}

...

> +static int emac_rx_frame_status(struct emac_priv *priv, struct emac_desc *desc)
> +{
> +	/* Drop if not last descriptor */
> +	if (!(desc->desc0 & RX_DESC_0_LAST_DESCRIPTOR)) {
> +		netdev_dbg(priv->ndev, "RX not last descriptor\n");

Unless I am mistaken these logs can occur on the basis of user
(Network packet) input. If so, I think rate limited debug
messages are more appropriate here and below.

> +		return RX_FRAME_DISCARD;
> +	}
> +
> +	if (desc->desc0 & RX_DESC_0_FRAME_RUNT) {
> +		netdev_dbg(priv->ndev, "RX runt frame\n");
> +		return RX_FRAME_DISCARD;
> +	}
> +
> +	if (desc->desc0 & RX_DESC_0_FRAME_CRC_ERR) {
> +		netdev_dbg(priv->ndev, "RX frame CRC error\n");
> +		return RX_FRAME_DISCARD;
> +	}
> +
> +	if (desc->desc0 & RX_DESC_0_FRAME_MAX_LEN_ERR) {
> +		netdev_dbg(priv->ndev, "RX frame exceeds max length\n");
> +		return RX_FRAME_DISCARD;
> +	}
> +
> +	if (desc->desc0 & RX_DESC_0_FRAME_JABBER_ERR) {
> +		netdev_dbg(priv->ndev, "RX frame jabber error\n");
> +		return RX_FRAME_DISCARD;
> +	}
> +
> +	if (desc->desc0 & RX_DESC_0_FRAME_LENGTH_ERR) {
> +		netdev_dbg(priv->ndev, "RX frame length error\n");
> +		return RX_FRAME_DISCARD;
> +	}
> +
> +	if (rx_frame_len(desc) <= ETH_FCS_LEN ||
> +	    rx_frame_len(desc) > priv->dma_buf_sz) {
> +		netdev_dbg(priv->ndev, "RX frame length unacceptable\n");
> +		return RX_FRAME_DISCARD;
> +	}
> +	return RX_FRAME_OK;
> +}

...

> +static int emac_resume(struct device *dev)
> +{
> +	struct emac_priv *priv = dev_get_drvdata(dev);
> +	struct net_device *ndev = priv->ndev;
> +	int ret;
> +
> +	ret = clk_prepare_enable(priv->bus_clk);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable bus clock: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if (!netif_running(ndev))
> +		return 0;
> +
> +	ret = emac_open(ndev);
> +	if (ret)

Smatch flags that priv->bus_clk resources are leaked here, and I agree.

> +		return ret;
> +
> +	netif_device_attach(ndev);
> +	return 0;
> +}

I would suggest addressing that like this.
(Compile tested only!)

diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
index 6158e776bc67..ebd02ec2bb01 100644
--- a/drivers/net/ethernet/spacemit/k1_emac.c
+++ b/drivers/net/ethernet/spacemit/k1_emac.c
@@ -1843,10 +1843,14 @@ static int emac_resume(struct device *dev)
 
 	ret = emac_open(ndev);
 	if (ret)
-		return ret;
+		goto err_clk_disable_unprepare;
 
 	netif_device_attach(ndev);
 	return 0;
+
+err_clk_disable_unprepare:
+	clk_disable_unprepare(priv->bus_clk);
+	return ret;
 }
 
 static int emac_suspend(struct device *dev)

...

> diff --git a/drivers/net/ethernet/spacemit/k1_emac.h b/drivers/net/ethernet/spacemit/k1_emac.h

...

> +struct emac_hw_stats {
> +	u32 tx_ok_pkts;
> +	u32 tx_total_pkts;
> +	u32 tx_ok_bytes;
> +	u32 tx_err_pkts;
> +	u32 tx_singleclsn_pkts;
> +	u32 tx_multiclsn_pkts;
> +	u32 tx_lateclsn_pkts;
> +	u32 tx_excessclsn_pkts;
> +	u32 tx_unicast_pkts;
> +	u32 tx_multicast_pkts;
> +	u32 tx_broadcast_pkts;
> +	u32 tx_pause_pkts;
> +	u32 rx_ok_pkts;
> +	u32 rx_total_pkts;
> +	u32 rx_crc_err_pkts;
> +	u32 rx_align_err_pkts;
> +	u32 rx_err_total_pkts;
> +	u32 rx_ok_bytes;
> +	u32 rx_total_bytes;
> +	u32 rx_unicast_pkts;
> +	u32 rx_multicast_pkts;
> +	u32 rx_broadcast_pkts;
> +	u32 rx_pause_pkts;
> +	u32 rx_len_err_pkts;
> +	u32 rx_len_undersize_pkts;
> +	u32 rx_len_oversize_pkts;
> +	u32 rx_len_fragment_pkts;
> +	u32 rx_len_jabber_pkts;
> +	u32 rx_64_pkts;
> +	u32 rx_65_127_pkts;
> +	u32 rx_128_255_pkts;
> +	u32 rx_256_511_pkts;
> +	u32 rx_512_1023_pkts;
> +	u32 rx_1024_1518_pkts;
> +	u32 rx_1519_plus_pkts;
> +	u32 rx_drp_fifo_full_pkts;
> +	u32 rx_truncate_fifo_full_pkts;
> +};

Many of the stats above appear to cover stats covered by struct
rtnl_link_stats64, ethtool_pause_stats, struct ethtool_rmon_stats, and
possibly others standardised in ethtool.h. Please only report standard
counters using standard mechanisms. And only use get_ethtool_stats to
report non-standard counters.

Link: https://www.kernel.org/doc/html/v6.16-rc4/networking/statistics.html#notes-for-driver-authors

...

-- 
pw-bot: changes-requested