lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <0605f176-5cdb-4f5b-9a6b-afa139c96732@iscas.ac.cn>
Date: Fri, 5 Sep 2025 23:45:29 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Simon Horman <horms@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, Jakub Kicinski <kuba@...nel.org>,
 Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Yixun Lan <dlan@...too.org>,
 "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Paolo Abeni <pabeni@...hat.com>, Philipp Zabel <p.zabel@...gutronix.de>,
 Paul Walmsley <paul.walmsley@...ive.com>, Palmer Dabbelt
 <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>,
 Alexandre Ghiti <alex@...ti.fr>, Vadim Fedorenko
 <vadim.fedorenko@...ux.dev>, Junhui Liu <junhui.liu@...moral.tech>,
 Maxime Chevallier <maxime.chevallier@...tlin.com>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-riscv@...ts.infradead.org,
 spacemit@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Troy Mitchell <troy.mitchell@...ux.spacemit.com>, Vivian Wang <uwu@...m.page>
Subject: Re: [PATCH net-next v9 2/5] net: spacemit: Add K1 Ethernet MAC

Hi Simon,

Thanks for the review.

(I have a question about the use of ndev->stats - see below.)

On 9/5/25 23:35, Simon Horman wrote:
> On Fri, Sep 05, 2025 at 07:09:31PM +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".
>>
>> Supports RGMII and RMII interfaces. Includes support for MAC hardware
>> statistics counters. PTP support is not implemented.
>>
>> Signed-off-by: Vivian Wang <wangruikang@...as.ac.cn>
>> Reviewed-by: Maxime Chevallier <maxime.chevallier@...tlin.com>
>> Reviewed-by: Vadim Fedorenko <vadim.fedorenko@...ux.dev>
>> Reviewed-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
>> Tested-by: Junhui Liu <junhui.liu@...moral.tech>
>> Tested-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
> ...
>
>> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
> ...
>
>> +static void emac_init_hw(struct emac_priv *priv)
>> +{
>> +	/* Destination address for 802.3x Ethernet flow control */
>> +	u8 fc_dest_addr[ETH_ALEN] = { 0x01, 0x80, 0xc2, 0x00, 0x00, 0x01 };
>> +
>> +	u32 rxirq = 0, dma = 0;
>> +
>> +	regmap_set_bits(priv->regmap_apmu,
>> +			priv->regmap_apmu_offset + APMU_EMAC_CTRL_REG,
>> +			AXI_SINGLE_ID);
>> +
>> +	/* Disable transmit and receive units */
>> +	emac_wr(priv, MAC_RECEIVE_CONTROL, 0x0);
>> +	emac_wr(priv, MAC_TRANSMIT_CONTROL, 0x0);
>> +
>> +	/* Enable MAC address 1 filtering */
>> +	emac_wr(priv, MAC_ADDRESS_CONTROL, MREGBIT_MAC_ADDRESS1_ENABLE);
>> +
>> +	/* Zero initialize the multicast hash table */
>> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE1, 0x0);
>> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE2, 0x0);
>> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE3, 0x0);
>> +	emac_wr(priv, MAC_MULTICAST_HASH_TABLE4, 0x0);
>> +
>> +	/* Configure thresholds */
>> +	emac_wr(priv, MAC_TRANSMIT_FIFO_ALMOST_FULL, DEFAULT_TX_ALMOST_FULL);
>> +	emac_wr(priv, MAC_TRANSMIT_PACKET_START_THRESHOLD,
>> +		DEFAULT_TX_THRESHOLD);
>> +	emac_wr(priv, MAC_RECEIVE_PACKET_START_THRESHOLD, DEFAULT_RX_THRESHOLD);
>> +
>> +	/* Configure flow control (enabled in emac_adjust_link() later) */
>> +	emac_set_mac_addr_reg(priv, fc_dest_addr, MAC_FC_SOURCE_ADDRESS_HIGH);
>> +	emac_wr(priv, MAC_FC_PAUSE_HIGH_THRESHOLD, DEFAULT_FC_FIFO_HIGH);
>> +	emac_wr(priv, MAC_FC_HIGH_PAUSE_TIME, DEFAULT_FC_PAUSE_TIME);
>> +	emac_wr(priv, MAC_FC_PAUSE_LOW_THRESHOLD, 0);
>> +
>> +	/* RX IRQ mitigation */
>> +	rxirq = EMAC_RX_FRAMES & MREGBIT_RECEIVE_IRQ_FRAME_COUNTER_MASK;
>> +	rxirq |= (EMAC_RX_COAL_TIMEOUT
>> +		  << MREGBIT_RECEIVE_IRQ_TIMEOUT_COUNTER_SHIFT) &
>> +		 MREGBIT_RECEIVE_IRQ_TIMEOUT_COUNTER_MASK;
> Probably this driver can benefit from using FIELD_PREP and FIELD_GET
> in a number of places. In this case I think it would mean that
> MREGBIT_RECEIVE_IRQ_TIMEOUT_COUNTER_SHIFT can be removed entirely.

That looks useful. There's a few more uses of *_SHIFT in this driver,
and I think I can get them all to use FIELD_PREP. I'll change those in
the next version.

>> +
>> +	rxirq |= MREGBIT_RECEIVE_IRQ_MITIGATION_ENABLE;
>> +	emac_wr(priv, DMA_RECEIVE_IRQ_MITIGATION_CTRL, rxirq);
> ...
>
>> +/* Returns number of packets received */
>> +static int emac_rx_clean_desc(struct emac_priv *priv, int budget)
>> +{
>> +	struct net_device *ndev = priv->ndev;
>> +	struct emac_rx_desc_buffer *rx_buf;
>> +	struct emac_desc_ring *rx_ring;
>> +	struct sk_buff *skb = NULL;
>> +	struct emac_desc *rx_desc;
>> +	u32 got = 0, skb_len, i;
>> +	int status;
>> +
>> +	rx_ring = &priv->rx_ring;
>> +
>> +	i = rx_ring->tail;
>> +
>> +	while (budget--) {
>> +		rx_desc = &((struct emac_desc *)rx_ring->desc_addr)[i];
>> +
>> +		/* Stop checking if rx_desc still owned by DMA */
>> +		if (READ_ONCE(rx_desc->desc0) & RX_DESC_0_OWN)
>> +			break;
>> +
>> +		dma_rmb();
>> +
>> +		rx_buf = &rx_ring->rx_desc_buf[i];
>> +
>> +		if (!rx_buf->skb)
>> +			break;
>> +
>> +		got++;
>> +
>> +		dma_unmap_single(&priv->pdev->dev, rx_buf->dma_addr,
>> +				 rx_buf->dma_len, DMA_FROM_DEVICE);
>> +
>> +		status = emac_rx_frame_status(priv, rx_desc);
>> +		if (unlikely(status == RX_FRAME_DISCARD)) {
>> +			ndev->stats.rx_dropped++;
> As per the comment in struct net-device,
> ndev->stats should not be used in modern drivers.
>
> Probably you want to implement NETDEV_PCPU_STAT_TSTATS.
>
> Sorry for not mentioning this in an earlier review of
> stats in this driver.
>
On a closer look, these counters in ndev->stats seems to be redundant
with the hardware-tracked statistics, so maybe I should just not bother
with updating ndev->stats. Does that make sense?

Thanks,
Vivian "dramforever" Wang


Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ