On Mon, 10 Jun 2024 15:59:35 +0200 Niklas Söderlund wrote:
> +static int rtsn_rx(struct net_device *ndev, int budget)
> +{
> + struct rtsn_private *priv = netdev_priv(ndev);
> + unsigned int ndescriptors;
> + unsigned int rx_packets;
> + unsigned int i;
> + bool get_ts;
> +
> + get_ts = priv->ptp_priv->tstamp_rx_ctrl &
> + RCAR_GEN4_RXTSTAMP_TYPE_V2_L2_EVENT;
> +
> + ndescriptors = priv->dirty_rx + priv->num_rx_ring - priv->cur_rx;
> + rx_packets = 0;
> + for (i = 0; i < ndescriptors; i++) {
> + const unsigned int entry = priv->cur_rx % priv->num_rx_ring;
> + struct rtsn_ext_ts_desc *desc = &priv->rx_ring[entry];
> + struct sk_buff *skb;
> + dma_addr_t dma_addr;
> + u16 pkt_len;
> +
> + /* Stop processing descriptors on first empty. */
> + if ((desc->die_dt & DT_MASK) == DT_FEMPTY)
> + break;
> +
> + dma_rmb();
> + pkt_len = le16_to_cpu(desc->info_ds) & RX_DS;
> +
> + skb = priv->rx_skb[entry];
> + priv->rx_skb[entry] = NULL;
> + dma_addr = le32_to_cpu(desc->dptr);
> + dma_unmap_single(ndev->dev.parent, dma_addr, PKT_BUF_SZ,
> + DMA_FROM_DEVICE);
> +
> + /* Get timestamp if enabled. */
> + if (get_ts) {
> + struct skb_shared_hwtstamps *shhwtstamps;
> + struct timespec64 ts;
> +
> + shhwtstamps = skb_hwtstamps(skb);
> + memset(shhwtstamps, 0, sizeof(*shhwtstamps));
> +
> + ts.tv_sec = (u64)le32_to_cpu(desc->ts_sec);
> + ts.tv_nsec = le32_to_cpu(desc->ts_nsec & cpu_to_le32(0x3fffffff));
> +
> + shhwtstamps->hwtstamp = timespec64_to_ktime(ts);
> + }
> +
> + skb_put(skb, pkt_len);
> + skb->protocol = eth_type_trans(skb, ndev);
> + netif_receive_skb(skb);
> +
> + /* Update statistics. */
> + ndev->stats.rx_packets++;
> + ndev->stats.rx_bytes += pkt_len;
> +
> + /* Update counters. */
> + priv->cur_rx++;
> + rx_packets++;
> +
> + /* Stop processing descriptors if budget is consumed. */
> + if (rx_packets >= budget)
> + break;
budget can also be 0 when netpoll calls us to only process tx, don't
process even a single packet in that case. (BTW also double check you
never printk() under the spin lock you take in NAPI)
> + }
> +
> + /* Refill the RX ring buffers */
> + for (; priv->cur_rx - priv->dirty_rx > 0; priv->dirty_rx++) {
> + const unsigned int entry = priv->dirty_rx % priv->num_rx_ring;
> + struct rtsn_ext_ts_desc *desc = &priv->rx_ring[entry];
> + struct sk_buff *skb;
> + dma_addr_t dma_addr;
> +
> + desc->info_ds = cpu_to_le16(PKT_BUF_SZ);
> +
> + if (!priv->rx_skb[entry]) {
> + skb = netdev_alloc_skb(ndev,
> + PKT_BUF_SZ + RTSN_ALIGN - 1);
napi_alloc_skb()
> + if (!skb)
> + break;
> + skb_reserve(skb, NET_IP_ALIGN);
> + dma_addr = dma_map_single(ndev->dev.parent, skb->data,
> + le16_to_cpu(desc->info_ds),
> + DMA_FROM_DEVICE);
> + if (dma_mapping_error(ndev->dev.parent, dma_addr))
> + desc->info_ds = cpu_to_le16(0);
> + desc->dptr = cpu_to_le32(dma_addr);
> + skb_checksum_none_assert(skb);
> + priv->rx_skb[entry] = skb;
> + }
> +
> + dma_wmb();
> + desc->die_dt = DT_FEMPTY | D_DIE;
> + }
> +
> + priv->rx_ring[priv->num_rx_ring].die_dt = DT_LINK;
> +
> + return rx_packets;
> +}
> +static struct net_device_stats *rtsn_get_stats(struct net_device *ndev)
> +{
> + return &ndev->stats;
> +}
Please implement your own 64 bit stats:
struct net_device_stats stats; /* not used by modern drivers */
> +static int rtsn_do_ioctl(struct net_device *ndev, struct ifreq *ifr, int cmd)
> +{
> + if (!netif_running(ndev))
> + return -ENODEV;
> +
> + switch (cmd) {
> + case SIOCGHWTSTAMP:
> + return rtsn_hwstamp_get(ndev, ifr);
> + case SIOCSHWTSTAMP:
> + return rtsn_hwstamp_set(ndev, ifr);
please implement ndo_hwtstamp_{g,s}et instead
> + default:
> + break;
> + }
> +
> + return phy_do_ioctl_running(ndev, ifr, cmd);
> +}
> +static const struct of_device_id rtsn_match_table[] = {
> + {.compatible = "renesas,r8a779g0-ethertsn", },
space missing
> + { /* Sentinel */ }
> +};
> + priv->reset = devm_reset_control_get(&pdev->dev, NULL);
> + if (IS_ERR(priv->reset)) {
> + ret = -PTR_ERR(priv->reset);
PTR_ERR() should give you negative errno
> + priv->ptp_priv->addr = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(priv->ptp_priv->addr)) {
> + ret = -PTR_ERR(priv->ptp_priv->addr);
ditto
> + ret = rtsn_get_phy_params(priv);
> + if (ret)
> + goto error_alloc;
error_free (please name the labels after target)
> + netdev_info(ndev, "MAC address %pM\n", ndev->dev_addr);
That's fairly unusual, why print the MAC address to logs?
--
pw-bot: cr
