| 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
| ||
|
Message-ID: <6c221dcf-4310-4e31-b3e8-a8a3b68c3734@iscas.ac.cn>
Date: Thu, 28 Aug 2025 16:06:58 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Troy Mitchell <troy.mitchell@...ux.spacemit.com>,
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>
Cc: Vivian Wang <uwu@...m.page>, Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
Junhui Liu <junhui.liu@...moral.tech>, Simon Horman <horms@...nel.org>,
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
Subject: Re: [PATCH net-next v7 2/5] net: spacemit: Add K1 Ethernet MAC
Hi Troy,
On 8/28/25 09:33, Troy Mitchell wrote:
> On Tue, Aug 26, 2025 at 02:23:47PM +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.
>>
>> Tested-by: Junhui Liu <junhui.liu@...moral.tech>
>> 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>
>> ---
> Hi Vivian,
> I have tested your patch on MUSE-PI.
> So here:
>
> Tested-by: Troy Mitchell <troy.mitchell@...ux.spacemit.com>
>
> But I still have a minor style comment on the code.
Thanks for your Tested-by and review.
>> drivers/net/ethernet/Kconfig | 1 +
>> drivers/net/ethernet/Makefile | 1 +
>> drivers/net/ethernet/spacemit/Kconfig | 29 +
>> drivers/net/ethernet/spacemit/Makefile | 6 +
>> drivers/net/ethernet/spacemit/k1_emac.c | 2193 +++++++++++++++++++++++++++++++
>> drivers/net/ethernet/spacemit/k1_emac.h | 426 ++++++
>> 6 files changed, 2656 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/spacemit/k1_emac.c b/drivers/net/ethernet/spacemit/k1_emac.c
>> new file mode 100644
>> index 0000000000000000000000000000000000000000..9e558d5893cfbbda0baa7ad21a7209dadda9487e
>> --- /dev/null
>> +++ b/drivers/net/ethernet/spacemit/k1_emac.c
>> @@ -0,0 +1,2193 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * SpacemiT K1 Ethernet driver
>> + *
>> + * Copyright (C) 2023-2025 SpacemiT (Hangzhou) Technology Co. Ltd
>> + * Copyright (C) 2025 Vivian Wang <wangruikang@...as.ac.cn>
>> + */
>> +
> [...]
>> +
>> +static void emac_wr(struct emac_priv *priv, u32 reg, u32 val)
>> +{
>> + writel(val, priv->iobase + reg);
>> +}
> basically short and obvious code like this:
>
> writel(val, priv->iobase + reg)
>
> you replace:
>
> emac_wr(priv, reg, val)
>
> It's not helpful..You could just use writel/readl directly
>> +
>> +static int emac_rd(struct emac_priv *priv, u32 reg)
>> +{
>> + return readl(priv->iobase + reg);
>> +}
> ditto.
I have decided against inlining these wrappers.
Firstly, the wrappers being mostly trivial is not it being "not
helpful". In long blocks of register access code, especially, having
just emac_rd(priv,...) or emac_wr(priv,...) repeated is easier to
recognize and harder to mess up (e.g. precedence of "priv->iomem + ...").
Secondly, they serve as documentation that a normal register access for
this driver is a 32-bit read or write. This is despite the fact that the
registers all "look like" they each only have the low 16 bits.
>> +
>> +static int emac_phy_interface_config(struct emac_priv *priv)
>> +{
>> + u32 val = 0, mask = PHY_INTF_RGMII;
>> +
>> + switch (priv->phy_interface) {
>> + case PHY_INTERFACE_MODE_RMII:
>> + mask |= REF_CLK_SEL;
>> + break;
>> + case PHY_INTERFACE_MODE_RGMII:
>> + case PHY_INTERFACE_MODE_RGMII_ID:
>> + case PHY_INTERFACE_MODE_RGMII_RXID:
>> + case PHY_INTERFACE_MODE_RGMII_TXID:
>> + val |= PHY_INTF_RGMII;
>> +
>> + mask |= RGMII_TX_CLK_SEL;
>> + break;
>> + default:
>> + netdev_err(priv->ndev, "Unsupported PHY interface %d",
>> + priv->phy_interface);
>> + return -EINVAL;
>> + }
>> +
>> + regmap_update_bits(priv->regmap_apmu,
>> + priv->regmap_apmu_offset + APMU_EMAC_CTRL_REG,
>> + mask, val);
>> +
>> + return 0;
>> +}
>> +
> [...]
>> +static bool emac_tx_should_interrupt(struct emac_priv *priv, u32 pkt_num)
>> +{
>> + bool should_interrupt;
>> +
>> + /* Manage TX mitigation */
>> + priv->tx_count_frames += pkt_num;
>> + if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
>> + emac_tx_coal_timer_resched(priv);
>> + should_interrupt = false;
>> + } else {
>> + priv->tx_count_frames = 0;
>> + should_interrupt = true;
>> + }
>> +
>> + return should_interrupt;
> If we really need this variable?
>
> How about this:
> if (likely(priv->tx_coal_frames > priv->tx_count_frames)) {
> emac_tx_coal_timer_resched(priv);
> return false;
> }
>
> priv->tx_count_frames = 0;
> return true;
I will simplify this in the next version.
>> +}
>> +
>> +static void emac_free_tx_buf(struct emac_priv *priv, int i)
>> +{
>> + struct emac_tx_desc_buffer *tx_buf;
>> + struct emac_desc_ring *tx_ring;
>> + struct desc_buf *buf;
>> + int j;
>> +
>> + tx_ring = &priv->tx_ring;
>> + tx_buf = &tx_ring->tx_desc_buf[i];
>> +
>> + for (j = 0; j < 2; j++) {
>> + buf = &tx_buf->buf[j];
>> + if (buf->dma_addr) {
>> + if (buf->map_as_page)
>> + dma_unmap_page(&priv->pdev->dev, buf->dma_addr,
>> + buf->dma_len, DMA_TO_DEVICE);
>> + else
>> + dma_unmap_single(&priv->pdev->dev,
>> + buf->dma_addr, buf->dma_len,
>> + DMA_TO_DEVICE);
>> +
>> + buf->dma_addr = 0;
>> + buf->map_as_page = false;
>> + buf->buff_addr = NULL;
>> + }
> if (!buf->dma_addr)
> continue;
>
> Best regards,
> Troy
You tricked me! There's one more review comment below :P
>> + }
>> +
>> + if (tx_buf->skb) {
>> + dev_kfree_skb_any(tx_buf->skb);
>> + tx_buf->skb = NULL;
>> + }
>> +}
>> +
>> +static void emac_clean_tx_desc_ring(struct emac_priv *priv)
>> +{
>> + struct emac_desc_ring *tx_ring = &priv->tx_ring;
>> + u32 i;
>> +
>> + /* Free all the TX ring skbs */
>> + for (i = 0; i < tx_ring->total_cnt; i++)
>> + emac_free_tx_buf(priv, i);
>> +
>> + tx_ring->head = 0;
>> + tx_ring->tail = 0;
>> +}
>> +
>> +static void emac_clean_rx_desc_ring(struct emac_priv *priv)
>> +{
>> + struct emac_rx_desc_buffer *rx_buf;
>> + struct emac_desc_ring *rx_ring;
>> + u32 i;
>> +
>> + rx_ring = &priv->rx_ring;
>> +
>> + /* Free all the RX ring skbs */
>> + for (i = 0; i < rx_ring->total_cnt; i++) {
>> + rx_buf = &rx_ring->rx_desc_buf[i];
>> + if (rx_buf->skb) {
>> + dma_unmap_single(&priv->pdev->dev, rx_buf->dma_addr,
>> + rx_buf->dma_len, DMA_FROM_DEVICE);
>> +
>> + dev_kfree_skb(rx_buf->skb);
>> + rx_buf->skb = NULL;
>> + }
> if (!rx_buf->skb)
> continue;
I will change both of these to "if (...) continue;" in the next version.
Thanks,
Vivian "dramforever" Wang
Powered by blists - more mailing lists