| 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: <50926ebd-f8c0-485b-87cd-f7552df4bcd4@iscas.ac.cn>
Date: Fri, 22 Aug 2025 21:27:03 +0800
From: Vivian Wang <wangruikang@...as.ac.cn>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, 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>,
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,
Vivian Wang <uwu@...m.page>
Subject: Re: [PATCH net-next v6 2/5] net: spacemit: Add K1 Ethernet MAC
Hi Jakub,
Thank you for the comments.
On 8/22/25 07:14, Jakub Kicinski wrote:
> On Wed, 20 Aug 2025 14:47:51 +0800 Vivian Wang wrote:
>> +static void emac_tx_mem_map(struct emac_priv *priv, struct sk_buff *skb)
>> +{
>> + struct emac_desc_ring *tx_ring = &priv->tx_ring;
>> + struct emac_desc tx_desc, *tx_desc_addr;
>> + struct device *dev = &priv->pdev->dev;
>> + struct emac_tx_desc_buffer *tx_buf;
>> + u32 head, old_head, frag_num, f;
>> + bool buf_idx;
>> +
>> + frag_num = skb_shinfo(skb)->nr_frags;
>> + head = tx_ring->head;
>> + old_head = head;
>> +
>> + for (f = 0; f < frag_num + 1; f++) {
>> + buf_idx = f % 2;
>> +
>> + /*
>> + * If using buffer 1, initialize a new desc. Otherwise, use
>> + * buffer 2 of previous fragment's desc.
>> + */
>> + if (!buf_idx) {
>> + tx_buf = &tx_ring->tx_desc_buf[head];
>> + tx_desc_addr =
>> + &((struct emac_desc *)tx_ring->desc_addr)[head];
>> + memset(&tx_desc, 0, sizeof(tx_desc));
>> +
>> + /*
>> + * Give ownership for all but first desc initially. For
>> + * first desc, give at the end so DMA cannot start
>> + * reading uninitialized descs.
>> + */
>> + if (head != old_head)
>> + tx_desc.desc0 |= TX_DESC_0_OWN;
>> +
>> + if (++head == tx_ring->total_cnt) {
>> + /* Just used last desc in ring */
>> + tx_desc.desc1 |= TX_DESC_1_END_RING;
>> + head = 0;
>> + }
>> + }
>> +
>> + if (emac_tx_map_frag(dev, &tx_desc, tx_buf, skb, f)) {
>> + netdev_err(priv->ndev, "Map TX frag %d failed", f);
>> + goto dma_map_err;
>> + }
>> +
>> + if (f == 0)
>> + tx_desc.desc1 |= TX_DESC_1_FIRST_SEGMENT;
>> +
>> + if (f == frag_num) {
>> + tx_desc.desc1 |= TX_DESC_1_LAST_SEGMENT;
>> + tx_buf->skb = skb;
>> + if (emac_tx_should_interrupt(priv, frag_num + 1))
>> + tx_desc.desc1 |=
>> + TX_DESC_1_INTERRUPT_ON_COMPLETION;
>> + }
>> +
>> + *tx_desc_addr = tx_desc;
>> + }
>> +
>> + /* All descriptors are ready, give ownership for first desc */
>> + tx_desc_addr = &((struct emac_desc *)tx_ring->desc_addr)[old_head];
>> + dma_wmb();
>> + WRITE_ONCE(tx_desc_addr->desc0, tx_desc_addr->desc0 | TX_DESC_0_OWN);
>> +
>> + emac_dma_start_transmit(priv);
>> +
>> + tx_ring->head = head;
>> +
>> + return;
>> +
>> +dma_map_err:
>> + dev_kfree_skb_any(skb);
> You free the skb here..
>
>> + priv->ndev->stats.tx_dropped++;
>> +}
>> +
>> +static netdev_tx_t emac_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> + struct emac_priv *priv = netdev_priv(ndev);
>> + int nfrags = skb_shinfo(skb)->nr_frags;
>> + struct device *dev = &priv->pdev->dev;
>> +
>> + if (unlikely(emac_tx_avail(priv) < nfrags + 1)) {
>> + if (!netif_queue_stopped(ndev)) {
>> + netif_stop_queue(ndev);
>> + dev_err_ratelimited(dev, "TX ring full, stop TX queue\n");
>> + }
>> + return NETDEV_TX_BUSY;
>> + }
>> +
>> + emac_tx_mem_map(priv, skb);
>> +
>> + ndev->stats.tx_packets++;
>> + ndev->stats.tx_bytes += skb->len;
> .. and then you use skb here.
>
>> + /* Make sure there is space in the ring for the next TX. */
>> + if (unlikely(emac_tx_avail(priv) <= MAX_SKB_FRAGS + 2))
>> + netif_stop_queue(ndev);
>> +
>> + return NETDEV_TX_OK;
>> +}
Thanks for the catch. I'll fix the error handling here in the next version.
>> +static void emac_get_ethtool_stats(struct net_device *dev,
>> + struct ethtool_stats *stats, u64 *data)
>> +{
>> + struct emac_priv *priv = netdev_priv(dev);
>> + u64 *rx_stats = (u64 *)&priv->rx_stats;
>> + int i;
>> +
>> + scoped_guard(spinlock_irqsave, &priv->stats_lock) {
> Why is this spin lock taken in irqsave mode?
> Please convert the code not to use scoped_guard()
> There's not a single flow control (return) in any of them.
> It's just hiding the information that you're unnecessarily masking irqs.
> See
> https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs
I'll fix these in the next version.
>> + emac_stats_update(priv);
>> +
>> + for (i = 0; i < ARRAY_SIZE(emac_ethtool_rx_stats); i++)
>> + data[i] = rx_stats[emac_ethtool_rx_stats[i].offset];
>> + }
>> +static void emac_tx_timeout_task(struct work_struct *work)
>> +{
>> + struct net_device *ndev;
>> + struct emac_priv *priv;
>> +
>> + priv = container_of(work, struct emac_priv, tx_timeout_task);
>> + ndev = priv->ndev;
> I don't see this work ever being canceled.
> What prevents ndev from being freed before it gets to run?
Oops. I'll fix the handling of this timer in the next version.
>> +/* Called when net interface is brought up. */
>> +static int emac_open(struct net_device *ndev)
>> +{
>> + struct emac_priv *priv = netdev_priv(ndev);
>> + struct device *dev = &priv->pdev->dev;
>> + int ret;
>> +
>> + ret = emac_alloc_tx_resources(priv);
>> + if (ret) {
>> + dev_err(dev, "Error when setting up the Tx resources\n");
>> + goto emac_alloc_tx_resource_fail;
>> + }
>> +
>> + ret = emac_alloc_rx_resources(priv);
>> + if (ret) {
>> + dev_err(dev, "Error when setting up the Rx resources\n");
>> + goto emac_alloc_rx_resource_fail;
>> + }
>> +
>> + ret = emac_up(priv);
>> + if (ret) {
>> + dev_err(dev, "Error when bringing interface up\n");
>> + goto emac_up_fail;
>> + }
>> + return 0;
>> +
>> +emac_up_fail:
> please name the jump labels after the destination not the source.
> Please fix everywhere in the driver.
> This is covered in the kernel coding style docs.
>
I'll fix in next version.
Thanks,
Vivian "dramforever" Wang
Powered by blists - more mailing lists