Renesas Ethernet AVB driver

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+}
+
+static void ravb_set_rate(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+

drivers/net/ethernet/renesas/ravb.c new file +13 -0

+			priv->rx_skb[q][entry] = NULL;
+			dma_sync_single_for_cpu(&ndev->dev, desc->dptr,
+						ALIGN(priv->rx_buffer_size, 16),
+						DMA_FROM_DEVICE);
+			get_ts &= (q == RAVB_NC) ?
+					RAVB_RXTSTAMP_TYPE_V2_L2_EVENT :
+					~RAVB_RXTSTAMP_TYPE_V2_L2_EVENT;
+			if (get_ts) {
+				struct skb_shared_hwtstamps *shhwtstamps;
+
+				shhwtstamps = skb_hwtstamps(skb);
+				memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+				ts.tv_sec = ((u64)desc->ts_sh << 32) |

drivers/net/ethernet/renesas/ravb.c new file +9 -0

+			stats->rx_packets++;
+			stats->rx_bytes += pkt_len;
+		}
+
+		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
+		desc = &priv->rx_ring[q][entry];
+	}
+
+	/* Refill the RX ring buffers. */

drivers/net/ethernet/renesas/ravb.c new file +8 -0

+	/* Wait for stopping the hardware TX process */
+	ravb_wait(ndev, TCCR, TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+		  0);
+
+	ravb_wait(ndev, CSR, CSR_TPO0 | CSR_TPO1 | CSR_TPO2 | CSR_TPO3, 0);
+
+	/* Stop the E-MAC's RX processes. */
+	ravb_write(ndev, ravb_read(ndev, ECMR) & ~ECMR_RE, ECMR);

(quoted diff) +2 -0

+			result = IRQ_HANDLED;
+		}

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+
+	ravb_set_duplex(ndev);
+
+error_exit:
+	mdelay(1);
+
+	/* Enable TX and RX */
+	ravb_rcv_snd_enable(ndev);
+
+	spin_unlock_irqrestore(&priv->lock, flags);
+

drivers/net/ethernet/renesas/ravb.c new file +239 -0

+		spin_lock_irqsave(&priv->lock, flags);
+		error = phy_start_aneg(priv->phydev);
+		spin_unlock_irqrestore(&priv->lock, flags);
+	}
+
+	return error;
+}
+
+static u32 ravb_get_msglevel(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	return priv->msg_enable;
+}
+
+static void ravb_set_msglevel(struct net_device *ndev, u32 value)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	priv->msg_enable = value;
+}
+
+static const char ravb_gstrings_stats[][ETH_GSTRING_LEN] = {
+	"rx_queue_0_current",
+	"tx_queue_0_current",
+	"rx_queue_0_dirty",
+	"tx_queue_0_dirty",
+	"rx_queue_0_packets",
+	"tx_queue_0_packets",
+	"rx_queue_0_bytes",
+	"tx_queue_0_bytes",
+	"rx_queue_0_mcast_packets",
+	"rx_queue_0_errors",
+	"rx_queue_0_crc_errors",
+	"rx_queue_0_frame_errors",
+	"rx_queue_0_length_errors",
+	"rx_queue_0_missed_errors",
+	"rx_queue_0_over_errors",
+
+	"rx_queue_1_current",
+	"tx_queue_1_current",
+	"rx_queue_1_dirty",
+	"tx_queue_1_dirty",
+	"rx_queue_1_packets",
+	"tx_queue_1_packets",
+	"rx_queue_1_bytes",
+	"tx_queue_1_bytes",
+	"rx_queue_1_mcast_packets",
+	"rx_queue_1_errors",
+	"rx_queue_1_crc_errors",
+	"rx_queue_1_frame_errors_",
+	"rx_queue_1_length_errors",
+	"rx_queue_1_missed_errors",
+	"rx_queue_1_over_errors",
+};
+
+#define RAVB_STATS_LEN	ARRAY_SIZE(ravb_gstrings_stats)
+
+static int ravb_get_sset_count(struct net_device *netdev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return RAVB_STATS_LEN;
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void ravb_get_ethtool_stats(struct net_device *ndev,
+				   struct ethtool_stats *stats, u64 *data)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int i = 0;
+	int q;
+
+	/* Device-specific stats */
+	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+		struct net_device_stats *stats = &priv->stats[q];
+
+		data[i++] = priv->cur_rx[q];
+		data[i++] = priv->cur_tx[q];
+		data[i++] = priv->dirty_rx[q];
+		data[i++] = priv->dirty_tx[q];
+		data[i++] = stats->rx_packets;
+		data[i++] = stats->tx_packets;
+		data[i++] = stats->rx_bytes;
+		data[i++] = stats->tx_bytes;
+		data[i++] = stats->multicast;
+		data[i++] = stats->rx_errors;
+		data[i++] = stats->rx_crc_errors;
+		data[i++] = stats->rx_frame_errors;
+		data[i++] = stats->rx_length_errors;
+		data[i++] = stats->rx_missed_errors;
+		data[i++] = stats->rx_over_errors;
+	}
+}
+
+static void ravb_get_strings(struct net_device *ndev, u32 stringset, u8 *data)
+{
+	switch (stringset) {
+	case ETH_SS_STATS:
+		memcpy(data, *ravb_gstrings_stats, sizeof(ravb_gstrings_stats));
+		break;
+	}
+}
+
+static void ravb_get_ringparam(struct net_device *ndev,
+			       struct ethtool_ringparam *ring)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	ring->rx_max_pending = BE_RX_RING_MAX;
+	ring->tx_max_pending = BE_TX_RING_MAX;
+	ring->rx_pending = priv->num_rx_ring[RAVB_BE];
+	ring->tx_pending = priv->num_tx_ring[RAVB_BE];
+}
+
+static int ravb_set_ringparam(struct net_device *ndev,
+			      struct ethtool_ringparam *ring)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int error;
+
+	if (ring->tx_pending > BE_TX_RING_MAX ||
+	    ring->rx_pending > BE_RX_RING_MAX ||
+	    ring->tx_pending < BE_TX_RING_MIN ||
+	    ring->rx_pending < BE_RX_RING_MIN)
+		return -EINVAL;
+	if (ring->rx_mini_pending || ring->rx_jumbo_pending)
+		return -EINVAL;
+
+	if (netif_running(ndev)) {
+		netif_device_detach(ndev);
+		netif_tx_disable(ndev);
+		/* Wait for DMA stopping */
+		ravb_wait_stop_dma(ndev);
+
+		/* Stop AVB-DMAC process */
+		error = ravb_config(ndev);
+		if (error < 0) {
+			netdev_err(ndev,
+				   "cannot set ringparam! Any AVB processes are still running?\n");
+			return error;
+		}
+		synchronize_irq(ndev->irq);
+
+		/* Free all the skbuffs in the RX queue. */
+		ravb_ring_free(ndev, RAVB_BE);
+		ravb_ring_free(ndev, RAVB_NC);
+		/* Free DMA buffer */
+		ravb_free_dma_buffer(priv);
+	}
+
+	/* Set new parameters */
+	priv->num_rx_ring[RAVB_BE] = ring->rx_pending;
+	priv->num_tx_ring[RAVB_BE] = ring->tx_pending;
+	priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
+	priv->num_tx_ring[RAVB_NC] = NC_TX_RING_SIZE;
+
+	if (netif_running(ndev)) {
+		error = ravb_ring_init(ndev, RAVB_BE);
+		if (error < 0) {
+			netdev_err(ndev, "%s: ravb_ring_init(RAVB_BE) failed\n",
+				   __func__);
+			return error;
+		}
+
+		error = ravb_ring_init(ndev, RAVB_NC);
+		if (error < 0) {
+			netdev_err(ndev, "%s: ravb_ring_init(RAVB_NC) failed\n",
+				   __func__);
+			return error;
+		}
+
+		error = ravb_dmac_init(ndev);
+		if (error < 0) {
+			netdev_err(ndev, "%s: ravb_dmac_init() failed\n",
+				   __func__);
+			return error;
+		}
+
+		ravb_emac_init(ndev);
+
+		netif_device_attach(ndev);
+	}
+
+	return 0;
+}
+
+static int ravb_get_ts_info(struct net_device *ndev,
+			    struct ethtool_ts_info *info)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	info->so_timestamping =
+		SOF_TIMESTAMPING_TX_SOFTWARE |
+		SOF_TIMESTAMPING_RX_SOFTWARE |
+		SOF_TIMESTAMPING_SOFTWARE |
+		SOF_TIMESTAMPING_TX_HARDWARE |
+		SOF_TIMESTAMPING_RX_HARDWARE |
+		SOF_TIMESTAMPING_RAW_HARDWARE;
+	info->tx_types = (1 << HWTSTAMP_TX_OFF) | (1 << HWTSTAMP_TX_ON);
+	info->rx_filters =
+		(1 << HWTSTAMP_FILTER_NONE) |
+		(1 << HWTSTAMP_FILTER_PTP_V2_L2_EVENT) |
+		(1 << HWTSTAMP_FILTER_ALL);
+	info->phc_index = ptp_clock_index(priv->ptp.clock);
+
+	return 0;
+}
+
+static const struct ethtool_ops ravb_ethtool_ops = {
+	.get_settings		= ravb_get_settings,
+	.set_settings		= ravb_set_settings,
+	.nway_reset		= ravb_nway_reset,
+	.get_msglevel		= ravb_get_msglevel,
+	.set_msglevel		= ravb_set_msglevel,
+	.get_link		= ethtool_op_get_link,
+	.get_strings		= ravb_get_strings,
+	.get_ethtool_stats	= ravb_get_ethtool_stats,
+	.get_sset_count		= ravb_get_sset_count,
+	.get_ringparam		= ravb_get_ringparam,
+	.set_ringparam		= ravb_set_ringparam,
+	.get_ts_info		= ravb_get_ts_info,
+};
+
+/* Network device open function for Ethernet AVB */
+static int ravb_open(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int error;
+
+	napi_enable(&priv->napi);
+
+	error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
+			    ndev);
+	if (error) {
+		netdev_err(ndev, "cannot request IRQ\n");
+		goto out_napi_off;

drivers/net/ethernet/renesas/ravb.c new file +74 -0

+
+	error = ravb_ring_init(ndev, RAVB_BE);
+	if (error)
+		goto out_free_irq;
+	error = ravb_ring_init(ndev, RAVB_NC);
+	if (error)
+		goto out_free_irq;
+
+	/* Device init */
+	error = ravb_dmac_init(ndev);
+	if (error)
+		goto out_free_irq;
+	ravb_emac_init(ndev);
+
+	netif_start_queue(ndev);
+
+	/* PHY control start */
+	error = ravb_phy_start(ndev);
+	if (error)
+		goto out_free_irq;
+
+	return 0;
+
+out_free_irq:
+	free_irq(ndev->irq, ndev);
+out_napi_off:
+	napi_disable(&priv->napi);
+	return error;
+}
+
+/* Timeout function for Ethernet AVB */
+static void ravb_tx_timeout(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int i, q;
+
+	netif_stop_queue(ndev);
+
+	netif_err(priv, tx_err, ndev,
+		  "transmit timed out, status %8.8x, resetting...\n",
+		  ravb_read(ndev, ISS));
+
+	/* tx_errors count up */
+	ndev->stats.tx_errors++;
+
+	/* Free all the skbuffs */
+	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+		for (i = 0; i < priv->num_rx_ring[q]; i++) {
+			dev_kfree_skb(priv->rx_skb[q][i]);
+			priv->rx_skb[q][i] = NULL;
+		}
+	}
+	for (q = RAVB_BE; q < NUM_TX_QUEUE; q++) {
+		for (i = 0; i < priv->num_tx_ring[q]; i++) {
+			dev_kfree_skb(priv->tx_skb[q][i]);
+			priv->tx_skb[q][i] = NULL;
+			kfree(priv->tx_buffers[q][i]);
+			priv->tx_buffers[q][i] = NULL;
+		}
+	}
+
+	/* Device init */
+	ravb_dmac_init(ndev);
+	ravb_emac_init(ndev);
+	netif_start_queue(ndev);
+}
+
+/* Packet transmit function for Ethernet AVB */
+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_tstamp_skb *ts_skb = NULL;
+	struct ravb_tx_desc *desc;
+	unsigned long flags;

drivers/net/ethernet/renesas/ravb.c new file +7 -0

+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}

drivers/net/ethernet/renesas/ravb.c new file +31 -0

+		return NETDEV_TX_OK;
+	}
+
+	/* TX timestamp required */
+	if (q == RAVB_NC) {
+		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
+		if (!ts_skb)
+			return -ENOMEM;
+		ts_skb->skb = skb;
+		ts_skb->tag = priv->ts_skb_tag++;
+		priv->ts_skb_tag %= 0x400;
+		list_add_tail(&ts_skb->list, &priv->ts_skb_list);
+
+		/* TAG and timestamp required flag */
+		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		skb_tx_timestamp(skb);
+		desc->tsr = 1;
+		desc->tag = ts_skb->tag;
+	}
+
+	/* Descriptor type must be set after all the above writes */
+	dma_wmb();
+	desc->dt = DT_FSINGLE;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	tccr = ravb_read(ndev, TCCR);
+	if (!(tccr & (TCCR_TSRQ0 << q)))
+		ravb_write(ndev, tccr | (TCCR_TSRQ0 << q), TCCR);
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	return NETDEV_TX_OK;

[snip]

drivers/net/ethernet/renesas/ravb.c line 844

Quoted 12 lines

drivers/net/ethernet/renesas/ravb.c new file +12 -0

+struct ravb_private {
+	struct net_device *ndev;
+	struct platform_device *pdev;
+	void __iomem *addr;
+	struct mdiobb_ctrl mdiobb;
+	u32 num_rx_ring[NUM_RX_QUEUE];
+	u32 num_tx_ring[NUM_TX_QUEUE];
+	u32 desc_bat_size;
+	dma_addr_t desc_bat_dma;
+	struct ravb_desc *desc_bat;
+	dma_addr_t rx_desc_dma[NUM_RX_QUEUE];
+	dma_addr_t tx_desc_dma[NUM_TX_QUEUE];

As a future optimization, you could try to group the variables by
direction: RX and TX such that you have better cache locality.

[snip]

drivers/net/ethernet/renesas/ravb.c line 934

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c new file +8 -0

+static void ravb_set_duplex(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	if (priv->duplex)	/* Full */
+		ravb_write(ndev, ravb_read(ndev, ECMR) | ECMR_DM, ECMR);
+	else			/* Half */
+		ravb_write(ndev, ravb_read(ndev, ECMR) & ~ECMR_DM, ECMR);

	reg = ravb_read(ndev, ECMR);
	if (priv->duplex)
		reg |= ECMR_DM;
	else
		reg &= ~ECMR_DM;
	ravb_writel(ndev, reg, ECMR);

Quoted 11 lines

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+}
+
+static void ravb_set_rate(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+
+	switch (priv->speed) {
+	case 100:		/* 100BASE */
+		ravb_write(ndev, GECMR_SPEED_100, GECMR);
+		break;
+	case 1000:		/* 1000BASE */

drivers/net/ethernet/renesas/ravb.c line 950

Quoted 5 lines

drivers/net/ethernet/renesas/ravb.c new file +5 -0

+		ravb_write(ndev, GECMR_SPEED_1000, GECMR);
+		break;
+	default:
+		break;
+	}

That still won't quite work with 10Mbits/sec will it? Or is this controller 100/1000 only (which would be extremely surprising).

[snip]

drivers/net/ethernet/renesas/ravb.c line 1409

Quoted 11 lines

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+		if (desc_status & (MSC_CRC | MSC_RFE | MSC_RTSF | MSC_RTLF |
+				   MSC_CEEF)) {
+			stats->rx_errors++;
+			if (desc_status & MSC_CRC)
+				stats->rx_crc_errors++;
+			if (desc_status & MSC_RFE)
+				stats->rx_frame_errors++;
+			if (desc_status & (MSC_RTLF | MSC_RTSF))
+				stats->rx_length_errors++;
+			if (desc_status & MSC_CEEF)
+				stats->rx_missed_errors++;

The flow after the else condition, while refiling might deserve some explanation.

drivers/net/ethernet/renesas/ravb.c line 1413

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+		} else {
+			u32 get_ts = priv->tstamp_rx_ctrl & RAVB_RXTSTAMP_TYPE;
+
+			skb = priv->rx_skb[q][entry];

Based on the refill logic below, it seems to me like you could leave holes in your ring where rx_skb[q][entry] is NULL, should not that be checked here?

drivers/net/ethernet/renesas/ravb.c line 1436

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+			skb_put(skb, pkt_len);
+			skb->protocol = eth_type_trans(skb, ndev);
+			if (q == RAVB_NC)
+				netif_rx(skb);
+			else
+				netif_receive_skb(skb);

Can't you always invoke netif_receive_skb() here? Why is there a special queue?

Quoted 13 lines

drivers/net/ethernet/renesas/ravb.c new file +13 -0

+			stats->rx_packets++;
+			stats->rx_bytes += pkt_len;
+		}
+
+		entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
+		desc = &priv->rx_ring[q][entry];
+	}
+
+	/* Refill the RX ring buffers. */
+	for (; priv->cur_rx[q] - priv->dirty_rx[q] > 0; priv->dirty_rx[q]++) {
+		entry = priv->dirty_rx[q] % priv->num_rx_ring[q];
+		desc = &priv->rx_ring[q][entry];
+		/* The size of the buffer should be on 16-byte boundary. */

drivers/net/ethernet/renesas/ravb.c line 1455

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+		desc->ds = ALIGN(priv->rx_buffer_size, 16);
+
+		if (!priv->rx_skb[q][entry]) {
+			skb = netdev_alloc_skb(ndev, skb_size);
+			if (!skb)
+				break;	/* Better luck next round. */

Should this really be a break or a continue?

[snip]

drivers/net/ethernet/renesas/ravb.c line 1494

Quoted 3 lines

drivers/net/ethernet/renesas/ravb.c new file +3 -0

+/* function for waiting dma process finished */
+static void ravb_wait_stop_dma(struct net_device *ndev)
+{

Should not you stop the MAC TX here as well for consistency?

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c new file +8 -0

+	/* Wait for stopping the hardware TX process */
+	ravb_wait(ndev, TCCR, TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 | TCCR_TSRQ3,
+		  0);
+
+	ravb_wait(ndev, CSR, CSR_TPO0 | CSR_TPO1 | CSR_TPO2 | CSR_TPO3, 0);
+
+	/* Stop the E-MAC's RX processes. */
+	ravb_write(ndev, ravb_read(ndev, ECMR) & ~ECMR_RE, ECMR);

[snip]

drivers/net/ethernet/renesas/ravb.c line 1625

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+		/* Transmited network control queue */
+		if (tis & TIS_FTF1) {
+			ravb_tx_free(ndev, RAVB_NC);
+			netif_wake_queue(ndev);

This would be better moved to the NAPI handler.

Quoted 2 lines

(quoted diff) +2 -0

+			result = IRQ_HANDLED;
+		}

[snip]

drivers/net/ethernet/renesas/ravb.c line 1867

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+	if (ecmd->duplex == DUPLEX_FULL)
+		priv->duplex = 1;
+	else
+		priv->duplex = 0;

Why not use what priv->phydev->duplex has cached for you?

drivers/net/ethernet/renesas/ravb.c line 1888

Quoted 7 lines

drivers/net/ethernet/renesas/ravb.c new file +7 -0

+static int ravb_nway_reset(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int error = -ENODEV;
+	unsigned long flags;
+
+	if (priv->phydev) {

Is checking against priv->phydev really necessary, it does not look like the driver will work or accept an invalid PHY device at all anyway?

Quoted 17 lines

drivers/net/ethernet/renesas/ravb.c new file +17 -0

+/* Network device open function for Ethernet AVB */
+static int ravb_open(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int error;
+
+	napi_enable(&priv->napi);
+
+	error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
+			    ndev);
+	if (error) {
+		netdev_err(ndev, "cannot request IRQ\n");
+		goto out_napi_off;
+	}
+
+	/* Descriptor set */
+	/* +26 gets the maximum ethernet encapsulation, +7 & ~7 because the

drivers/net/ethernet/renesas/ravb.c line 2137

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+	 * card needs room to do 8 byte alignment, +2 so we can reserve
+	 * the first 2 bytes, and +16 gets room for the status word from the
+	 * card.
+	 */
+	priv->rx_buffer_size = (ndev->mtu <= 1492 ? PKT_BUF_SZ :
+				(((ndev->mtu + 26 + 7) & ~7) + 2 + 16));

Is not that something that should be moved to a local ndo_change_mtu()

function? What happens if I change the MTU of an interface running, does not that completely break this RX buffer estimation?

Quoted 10 lines

drivers/net/ethernet/renesas/ravb.c new file +10 -0

+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_tstamp_skb *ts_skb = NULL;
+	struct ravb_tx_desc *desc;
+	unsigned long flags;
+	void *buffer;
+	u32 entry;
+	u32 tccr;
+	int q;

drivers/net/ethernet/renesas/ravb.c line 2221

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+
+	/* If skb needs TX timestamp, it is handled in network control queue */
+	q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC : RAVB_BE;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q] - 4) {

What's so special about 4 here, you don't seem to be using 4 descriptors

Quoted 11 lines

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+	priv->cur_tx[q]++;
+	spin_unlock_irqrestore(&priv->lock, flags);
+

drivers/net/ethernet/renesas/ravb.c line 2238

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+	if (skb_put_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
+
+	priv->tx_skb[q][entry] = skb;
+	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+	memcpy(buffer, skb->data, skb->len);

~1500 bytes memcpy(), not good...

Review

Quoted 1 line

(quoted diff) +1 -0

+	desc = &priv->tx_ring[q][entry];

Since we have released the spinlock few lines above, is there something protecting ravb_tx_free() from concurrently running with this xmit() call and trashing this entry?

drivers/net/ethernet/renesas/ravb.c line 2245

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+	desc->ds = skb->len;
+	desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+				    DMA_TO_DEVICE);
+	if (dma_mapping_error(&ndev->dev, desc->dptr)) {
+		dev_kfree_skb_any(skb);
+		priv->tx_skb[q][entry] = NULL;

Don't you need to make sure this NULL is properly seen by ravb_tx_free()?

Quoted 11 lines

Quoted 11 lines

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+	priv->cur_tx[q]++;
+	spin_unlock_irqrestore(&priv->lock, flags);
+

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2238

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+	if (skb_put_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
+
+	priv->tx_skb[q][entry] = skb;
+	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+	memcpy(buffer, skb->data, skb->len);

~1500 bytes memcpy(), not good...

I'm looking in the manual and not finding the hard requirement to have the
buffer address aligned to 128 bytes (RAVB_ALIGN), sigh... Kimura-san?

Hello Sergei.

Quoted 20 lines · Sergei Shtylyov

Quoted 11 lines

Quoted 11 lines

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+	priv->cur_tx[q]++;
+	spin_unlock_irqrestore(&priv->lock, flags);
+

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2238

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+	if (skb_put_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
+
+	priv->tx_skb[q][entry] = skb;
+	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+	memcpy(buffer, skb->data, skb->len);

~1500 bytes memcpy(), not good...

Quoted 2 lines

I'm looking in the manual and not finding the hard requirement to have the
buffer address aligned to 128 bytes (RAVB_ALIGN), sigh... Kimura-san?

There are the hardware requirement that the frame data must be aligned with a 32-bit boundary in the URAM, see section 45A.3.3.1 Data Representation in the manual.
I think that the original skb->data is almost aligned with 2 bytes boundary by NET_IP_ALING, so we copied original skb->data to the local aligned buffer.

In addition, see section 45A.3.3.12 Tips for Optimizing Performance in Handling Descriptors, it mentioned that frame data is accessed in blocks up to 128 bytes and the number of 128 byte borders (addresses H'xxx00 and H'xxx80) and frame data inside should be minimized.
So we set RAVB_ALIGN to 128 bytes.

Quoted 29 lines

Quoted 20 lines

Quoted 11 lines

Quoted 11 lines

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+	priv->cur_tx[q]++;
+	spin_unlock_irqrestore(&priv->lock, flags);
+

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2238

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+	if (skb_put_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
+
+	priv->tx_skb[q][entry] = skb;
+	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+	memcpy(buffer, skb->data, skb->len);

~1500 bytes memcpy(), not good...

Quoted 2 lines

I'm looking in the manual and not finding the hard requirement to have the
buffer address aligned to 128 bytes (RAVB_ALIGN), sigh... Kimura-san?

There are the hardware requirement that the frame data must be aligned with a 32-bit boundary in the URAM, see section 45A.3.3.1 Data Representation in the manual.
I think that the original skb->data is almost aligned with 2 bytes boundary by NET_IP_ALING, so we copied original skb->data to the local aligned buffer.

Quoted 5 lines

In addition, see section 45A.3.3.12 Tips for Optimizing Performance in Handling Descriptors, it mentioned that frame data is accessed in blocks up to 128 bytes and the number of 128 byte borders (addresses H'xxx00 and H'xxx80) and frame data inside should be minimized.
So we set RAVB_ALIGN to 128 bytes.

There is no way that copying is going to be faster than finding an adequate way to transmit directly out of the SKB memory.

In this day and age there is simply no excuse for something like this, you will have to find a way.

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

Quoted 3 lines

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

NET_IP_ALIGN is for receive, not transmit.

Hello.

On 04/22/2015 11:42 PM, David Miller wrote:

Quoted 5 lines

Quoted 3 lines

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

NET_IP_ALIGN is for receive, not transmit.

Hm, then 'skb->data' should be aligned to 4 byte boundary on TX, right?

Quoted 3 lines

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

NET_IP_ALIGN is for receive, not transmit.

Quoted 6 lines

Quoted 4 lines

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

NET_IP_ALIGN is for receive, not transmit.

But when I print 'skb->data' from the ndo_start_xmit() method (in the 'sh_eth' driver), all addresses end with 2, so it looks like NET_IP_ALIGN gets added somewhere...

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

Quoted 5 lines

Quoted 1 line

NET_IP_ALIGN is for receive, not transmit.

But when I print 'skb->data' from the ndo_start_xmit() method (in the 'sh_eth' driver), all addresses end with 2, so it looks like NET_IP_ALIGN gets added somewhere...

It's the IPV4 header which is 4 byte aligned, then the ethernet header is pushed which is 14 bytes.

   Sigh... I'm seeing no way out of that then, only copying. :-(

Quoted 1 line

    Sigh... I'm seeing no way out of that then, only copying. :-(

What exactly is the device's restriction?

Any reasonable modern chip allows one of two things.

Either it allows arbitrary alignment of the start of the TX frame when DMA'ing.

_or_

It allows a variable number of pad bytes to be inserted by the driver before giving it to the card, which do not go onto the wire, in order to meet the device's DMA restrictions.

For example, if the packet is only 2 byte aligned, you set the "ignore offset" to 2 and push two zero bytes in front of the ethernet frame before giving it to the card.

If a chip made in this day and era cannot do one of those two things, this is beyond disappointing and is a massive engineering failure. Whoever designed this chip made no investigation into how their hardware is going to be actually used.

Sent: 22 April 2015 22:39

Quoted 3 lines · David Miller

Quoted 3 lines

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

Quoted 1 line

NET_IP_ALIGN is for receive, not transmit.

    But when I print 'skb->data' from the ndo_start_xmit() method (in the

'sh_eth' driver), all addresses end with 2, so it looks like NET_IP_ALIGN gets added somewhere...

Quoted 7 lines

Sent: 22 April 2015 22:39

Quoted 3 lines · David Miller

Quoted 3 lines

Hmm, I've been digging in the net core, and was unable to see where TX skb's get their NET_IP_ALIGN bytes reserved. Have I missed something? Probably need to print out skb's fields...

Quoted 5 lines

Quoted 1 line

NET_IP_ALIGN is for receive, not transmit.

     But when I print 'skb->data' from the ndo_start_xmit() method (in the

'sh_eth' driver), all addresses end with 2, so it looks like NET_IP_ALIGN gets added somewhere...

For a locally generated message:
The TCP userdata is likely to be 4 byte aligned.
The TCP and IP headers are multiples of 4 bytes.
The MAC header is 14 bytes.
So you end up with a buffer that starts on a 4n+2 boundary or an initial short fragment that is 4n+2 bytes long.

If a message is being forwarded the alignment probably depends on where it came from.

If you have ethernet hardware that requires tx or rx buffers to be on

4n boundaries you should send it back as 'not fit for purpose'.

David

Sent: 24 April 2015 19:27

Quoted 2 lines

If you have ethernet hardware that requires tx or rx buffers to be on 4n boundaries you should send it back as 'not fit for purpose'.

The RX buffers can be adjusted with skb_resrerve(), it's only the TX
buffers that need to be copied...

I'm afraid we can't. :-)
However, my colleague has suggested a scheme minimizing the copying:
only up to 3 first bytes need to be copied to the driver's internal buffers, the rest can be sent from an skb itself. That would require substantial changes to the driver though...

Quoted 13 lines

Quoted 10 lines

(quoted diff) +2 -0

+    /* Wait for stopping the hardware TX process */
+    ravb_wait(ndev, TCCR, TCCR_TSRQ0 | TCCR_TSRQ1 | TCCR_TSRQ2 |

TCCR_TSRQ3,

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+          0);
+
+    ravb_wait(ndev, CSR, CSR_TPO0 | CSR_TPO1 | CSR_TPO2 | CSR_TPO3, 0);
+
+    /* Stop the E-MAC's RX processes. */
+    ravb_write(ndev, ravb_read(ndev, ECMR) & ~ECMR_RE, ECMR);

[snip]

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c line 1625

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+        /* Transmited network control queue */
+        if (tis & TIS_FTF1) {
+            ravb_tx_free(ndev, RAVB_NC);
+            netif_wake_queue(ndev);

This would be better moved to the NAPI handler.

Maybe, not sure...

Quoted 9 lines

Quoted 2 lines

(quoted diff) +2 -0

+            result = IRQ_HANDLED;
+        }

[snip]

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+    if (ecmd->duplex == DUPLEX_FULL)
+        priv->duplex = 1;
+    else
+        priv->duplex = 0;

Quoted 1 line

Why not use what priv->phydev->duplex has cached for you?

Because we compare 'priv->duplex' with 'priv->phydev->duplex' in
ravb_adjust_link(). Or what did you mean?

[...]

Quoted 10 lines

drivers/net/ethernet/renesas/ravb.c line 1888

Quoted 7 lines

drivers/net/ethernet/renesas/ravb.c new file +7 -0

+static int ravb_nway_reset(struct net_device *ndev)
+{
+    struct ravb_private *priv = netdev_priv(ndev);
+    int error = -ENODEV;
+    unsigned long flags;
+
+    if (priv->phydev) {

Is checking against priv->phydev really necessary, it does not look like the driver will work or accept an invalid PHY device at all anyway?

You still can run 'ethtool' on a closed network device.

[...]

Quoted 19 lines

Quoted 19 lines

drivers/net/ethernet/renesas/ravb.c new file +9 -0

+/* Network device open function for Ethernet AVB */
+static int ravb_open(struct net_device *ndev)
+{
+    struct ravb_private *priv = netdev_priv(ndev);
+    int error;
+
+    napi_enable(&priv->napi);
+
+    error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,

ndev->name,

drivers/net/ethernet/renesas/ravb.c new file +9 -0

+                ndev);
+    if (error) {
+        netdev_err(ndev, "cannot request IRQ\n");
+        goto out_napi_off;
+    }
+
+    /* Descriptor set */
+    /* +26 gets the maximum ethernet encapsulation, +7 & ~7 because the
+     * card needs room to do 8 byte alignment, +2 so we can reserve

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2137

Quoted 6 lines

+ * the first 2 bytes, and +16 gets room for the status word from
the

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+     * card.
+     */
+    priv->rx_buffer_size = (ndev->mtu <= 1492 ? PKT_BUF_SZ :
+                (((ndev->mtu + 26 + 7) & ~7) + 2 + 16));

Is not that something that should be moved to a local ndo_change_mtu()

That was copied from sh_eth.c verbatim, I even doubt that the formula
is correct for EtherAVB...

Quoted 2 lines

function? What happens if I change the MTU of an interface running, does not that completely break this RX buffer estimation?

Well, not completely, I think. eth_change_mtu() doesn't allow MTU >
1500 bytes, so it looks like we just need to change 1492 to 1500 here.

[...]

Quoted 14 lines

Quoted 14 lines

+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+{
+    struct ravb_private *priv = netdev_priv(ndev);
+    struct ravb_tstamp_skb *ts_skb = NULL;
+    struct ravb_tx_desc *desc;
+    unsigned long flags;
+    void *buffer;
+    u32 entry;
+    u32 tccr;
+    int q;
+
+    /* If skb needs TX timestamp, it is handled in network control

queue */

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c

Quoted 6 lines

+ q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC :
RAVB_BE;

drivers/net/ethernet/renesas/ravb.c new file +3 -1

+
+    spin_lock_irqsave(&priv->lock, flags);
+    if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q]
- 4) {

What's so special about 4 here, you don't seem to be using 4 descriptors

Not sure, this was clearly copied from sh_eth.c. Perhaps it's just a
threshold for calling ravb_tx_free()...

Quoted 12 lines

Quoted 12 lines

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+        if (!ravb_tx_free(ndev, q)) {
+            netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+            netif_stop_queue(ndev);
+            spin_unlock_irqrestore(&priv->lock, flags);
+            return NETDEV_TX_BUSY;
+        }
+    }
+    entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+    priv->cur_tx[q]++;
+    spin_unlock_irqrestore(&priv->lock, flags);
+

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2238

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+    if (skb_put_padto(skb, ETH_ZLEN))
+        return NETDEV_TX_OK;
+
+    priv->tx_skb[q][entry] = skb;
+    buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+    memcpy(buffer, skb->data, skb->len);

~1500 bytes memcpy(), not good...

I'm looking in the manual and not finding the hard requirement to
have the buffer address aligned to 128 bytes (RAVB_ALIGN), sigh... Kimura-san?

Quoted 5 lines

Quoted 1 line

+ desc = &priv->tx_ring[q][entry];

Since we have released the spinlock few lines above, is there something protecting ravb_tx_free() from concurrently running with this xmit() call and trashing this entry?

   Probably nothing... :-)

Quoted 6 lines

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+    desc->ds = skb->len;
+    desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+                    DMA_TO_DEVICE);
+    if (dma_mapping_error(&ndev->dev, desc->dptr)) {
+        dev_kfree_skb_any(skb);
+        priv->tx_skb[q][entry] = NULL;

Quoted 1 line

Don't you need to make sure this NULL is properly seen by ravb_tx_free()?

You mean doing this before releasing the spinlock? Or what?

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+    if (ecmd->duplex == DUPLEX_FULL)
+        priv->duplex = 1;
+    else
+        priv->duplex = 0;

Quoted 4 lines

Quoted 1 line

Why not use what priv->phydev->duplex has cached for you?

    Because we compare 'priv->duplex' with 'priv->phydev->duplex' in

ravb_adjust_link(). Or what did you mean?

Oh I see how you are using this now, but it does not look like it is necessary, since you use phy_ethtool_sset() using phydev->duplex

directly ought to be enough anywhere in your driver?

Unless there is a
mode where you are running PHY-less, and not using a fixed PHY to emulate a PHY...

Quoted 12 lines

[...]

Quoted 10 lines

drivers/net/ethernet/renesas/ravb.c line 1888

Quoted 7 lines

drivers/net/ethernet/renesas/ravb.c new file +7 -0

+static int ravb_nway_reset(struct net_device *ndev)
+{
+    struct ravb_private *priv = netdev_priv(ndev);
+    int error = -ENODEV;
+    unsigned long flags;
+
+    if (priv->phydev) {

Is checking against priv->phydev really necessary, it does not look like the driver will work or accept an invalid PHY device at all anyway?

Quoted 1 line

You still can run 'ethtool' on a closed network device.

Sure, but that does not mean that priv->phydev becomes NULL, even if you

have called phy_disconnect() in your ndo_close() function, you should still have a correct priv->phydev reference to the PHY device, no?

Quoted 22 lines

Quoted 22 lines

Quoted 16 lines

+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+{
+    struct ravb_private *priv = netdev_priv(ndev);
+    struct ravb_tstamp_skb *ts_skb = NULL;
+    struct ravb_tx_desc *desc;
+    unsigned long flags;
+    void *buffer;
+    u32 entry;
+    u32 tccr;
+    int q;
+
+    /* If skb needs TX timestamp, it is handled in network control

queue */
+ q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC :
RAVB_BE;

drivers/net/ethernet/renesas/ravb.c

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +3 -1

+
+    spin_lock_irqsave(&priv->lock, flags);
+    if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q]
- 4) {

What's so special about 4 here, you don't seem to be using 4 descriptors

Quoted 2 lines

Not sure, this was clearly copied from sh_eth.c. Perhaps it's just a
threshold for calling ravb_tx_free()...

Then 1 inclusive or 0 exclusive is probably what you should be comparing to, otherwise you may just stop the tx queue earlier than needed.

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+    desc->ds = skb->len;
+    desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+                    DMA_TO_DEVICE);
+    if (dma_mapping_error(&ndev->dev, desc->dptr)) {
+        dev_kfree_skb_any(skb);
+        priv->tx_skb[q][entry] = NULL;

Quoted 3 lines

Quoted 1 line

Don't you need to make sure this NULL is properly seen by ravb_tx_free()?

You mean doing this before releasing the spinlock? Or what?

Yes, the locking your transmit function seems to open windows during which it is possible for the interrupt handler running on another CPU to mess up with the data you are using here.

--
Florian

On 04/23/2015 02:22 AM, Florian Fainelli wrote:

[...]

Quoted 5 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+    if (ecmd->duplex == DUPLEX_FULL)
+        priv->duplex = 1;
+    else
+        priv->duplex = 0;

Quoted 7 lines

Quoted 4 lines

Quoted 1 line

Why not use what priv->phydev->duplex has cached for you?

    Because we compare 'priv->duplex' with 'priv->phydev->duplex' in

ravb_adjust_link(). Or what did you mean?

Oh I see how you are using this now, but it does not look like it is necessary, since you use phy_ethtool_sset() using phydev->duplex

It only writes to it, doesn't use it AFAICS...

Quoted 1 line

directly ought to be enough anywhere in your driver?

    'priv->phydev' is NULL when the device is closed, so I just can't call 

phy_ethtool_sset().

Quoted 3 lines

Unless there is a
mode where you are running PHY-less, and not using a fixed PHY to emulate a PHY...

No such mode.

Quoted 12 lines

Quoted 12 lines

[...]

Quoted 10 lines

drivers/net/ethernet/renesas/ravb.c line 1888

Quoted 7 lines

drivers/net/ethernet/renesas/ravb.c new file +7 -0

+static int ravb_nway_reset(struct net_device *ndev)
+{
+    struct ravb_private *priv = netdev_priv(ndev);
+    int error = -ENODEV;
+    unsigned long flags;
+
+    if (priv->phydev) {

Is checking against priv->phydev really necessary, it does not look like the driver will work or accept an invalid PHY device at all anyway?

This check was copied from sh_eth that was fixed by Ben ot to crash due to
'ethtool' being called on closed device, see:

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/net/ethernet/renesas/sh_eth.c?id=4f9dce230b32eec45cec8c28cae61efdfa2f7d57

That commit refers to a dangling pointer, not sure what does this mean...
The PHy device doesn't seem to be freed by phy_disconnect(). Ben?

Quoted 22 lines

Quoted 22 lines

Quoted 22 lines

Quoted 16 lines

+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+{
+    struct ravb_private *priv = netdev_priv(ndev);
+    struct ravb_tstamp_skb *ts_skb = NULL;
+    struct ravb_tx_desc *desc;
+    unsigned long flags;
+    void *buffer;
+    u32 entry;
+    u32 tccr;
+    int q;
+
+    /* If skb needs TX timestamp, it is handled in network control

queue */
+ q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC :
RAVB_BE;

drivers/net/ethernet/renesas/ravb.c

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +3 -1

+
+    spin_lock_irqsave(&priv->lock, flags);
+    if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q]
- 4) {

What's so special about 4 here, you don't seem to be using 4 descriptors

Quoted 5 lines

Quoted 2 lines

Not sure, this was clearly copied from sh_eth.c. Perhaps it's just a
threshold for calling ravb_tx_free()...

Then 1 inclusive or 0 exclusive is probably what you should be comparing to, otherwise you may just stop the tx queue earlier than needed.

Will look into this...

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2245

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+    desc->ds = skb->len;
+    desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+                    DMA_TO_DEVICE);
+    if (dma_mapping_error(&ndev->dev, desc->dptr)) {
+        dev_kfree_skb_any(skb);
+        priv->tx_skb[q][entry] = NULL;

Don't you need to make sure this NULL is properly seen by ravb_tx_free()?

Quoted 5 lines

Quoted 1 line

You mean doing this before releasing the spinlock? Or what?

Yes, the locking your transmit function seems to open windows during which it is possible for the interrupt handler running on another CPU to mess up with the data you are using here.

Will look into that too...

Quoted 2 lines

--
Florian

Quoted 14 lines

Quoted 14 lines

Quoted 10 lines

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2245

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+    desc->ds = skb->len;
+    desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+                    DMA_TO_DEVICE);
+    if (dma_mapping_error(&ndev->dev, desc->dptr)) {
+        dev_kfree_skb_any(skb);
+        priv->tx_skb[q][entry] = NULL;

Don't you need to make sure this NULL is properly seen by ravb_tx_free()?

You mean doing this before releasing the spinlock? Or what?

Yes, the locking your transmit function seems to open windows during which it is possible for the interrupt handler running on another CPU to mess up with the data you are using here.

Quoted 1 line

Will look into that too...

I have looked into the code and I must admit I don't understand how the
data can be messed up with. ravb_tx_free() only advances 'priv->dirty_tx' and doesn't go beyond (or change) 'priv->cur_tx' which is used here...

Quoted 2 lines

Quoted 2 lines

--
Florian

drivers/net/ethernet/renesas/ravb.c new file +61 -0

+struct ravb_desc {
+#ifdef __LITTLE_ENDIAN
+	u32 ds: 12;	/* Descriptor size */
+	u32 cc: 12;	/* Content control */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 dt: 4;	/* Descriptor type */
+#else
+	u32 dt: 4;	/* Descriptor type */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 cc: 12;	/* Content control */
+	u32 ds: 12;	/* Descriptor size */
+#endif
+	u32 dptr;	/* Descriptor pointer */
+};
+
+struct ravb_rx_desc {
+#ifdef __LITTLE_ENDIAN
+	u32 ds: 12;	/* Descriptor size */
+	u32 ei: 1;	/* Error indication */
+	u32 ps: 2;	/* Padding selection */
+	u32 tr: 1;	/* Truncation indication */
+	u32 msc: 8;	/* MAC status code */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 dt: 4;	/* Descriptor type */
+#else
+	u32 dt: 4;	/* Descriptor type */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 msc: 8;	/* MAC status code */
+	u32 ps: 2;	/* Padding selection */
+	u32 ei: 1;	/* Error indication */
+	u32 tr: 1;	/* Truncation indication */
+	u32 ds: 12;	/* Descriptor size */
+#endif
+	u32 dptr;	/* Descpriptor pointer */
+};
+
+struct ravb_ex_rx_desc {
+#ifdef __LITTLE_ENDIAN
+	u32 ds: 12;	/* Descriptor size */
+	u32 ei: 1;	/* Error indication */
+	u32 ps: 2;	/* Padding selection */
+	u32 tr: 1;	/* Truncation indication */
+	u32 msc: 8;	/* MAC status code */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 dt: 4;	/* Descriptor type */
+#else
+	u32 dt: 4;	/* Descriptor type */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 msc: 8;	/* MAC status code */
+	u32 ps: 2;	/* Padding selection */
+	u32 ei: 1;	/* Error indication */
+	u32 tr: 1;	/* Truncation indication */
+	u32 ds: 12;	/* Descriptor size */
+#endif
+	u32 dptr;	/* Descpriptor pointer */

drivers/net/ethernet/renesas/ravb.c new file +12 -0

+
+struct ravb_tx_desc {
+#ifdef __LITTLE_ENDIAN
+	u32 ds: 12;	/* Descriptor size */
+	u32 tag: 10;	/* Frame tag */
+	u32 tsr: 1;	/* Timestamp storage request */
+	u32 msc: 1;	/* MAC status storage request */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 dt: 4;	/* Descriptor type */
+#else
+	u32 dt: 4;	/* Descriptor type */

drivers/net/ethernet/renesas/ravb.c new file +39 -0

+
+/* Network device open function for Ethernet AVB */
+static int ravb_open(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int error;
+
+	napi_enable(&priv->napi);
+
+	error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
+			    ndev);
+	if (error) {
+		netdev_err(ndev, "cannot request IRQ\n");
+		goto out_napi_off;
+	}
+
+	/* Descriptor set */
+	/* +26 gets the maximum ethernet encapsulation, +7 & ~7 because the
+	 * card needs room to do 8 byte alignment, +2 so we can reserve
+	 * the first 2 bytes, and +16 gets room for the status word from the
+	 * card.
+	 */
+	priv->rx_buffer_size = (ndev->mtu <= 1492 ? PKT_BUF_SZ :
+				(((ndev->mtu + 26 + 7) & ~7) + 2 + 16));
+
+	error = ravb_ring_init(ndev, RAVB_BE);
+	if (error)
+		goto out_free_irq;
+	error = ravb_ring_init(ndev, RAVB_NC);
+	if (error)
+		goto out_free_irq;
+
+	/* Device init */
+	error = ravb_dmac_init(ndev);
+	if (error)
+		goto out_free_irq;
+	ravb_emac_init(ndev);
+
+	netif_start_queue(ndev);

drivers/net/ethernet/renesas/ravb.c new file +30 -0

+out_napi_off:
+	napi_disable(&priv->napi);
+	return error;
+}
 
 
 
 
+/* Timeout function for Ethernet AVB */
+static void ravb_tx_timeout(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int i, q;
+
+	netif_stop_queue(ndev);
+
+	netif_err(priv, tx_err, ndev,
+		  "transmit timed out, status %8.8x, resetting...\n",
+		  ravb_read(ndev, ISS));
+
+	/* tx_errors count up */
+	ndev->stats.tx_errors++;
+
+	/* Free all the skbuffs */
+	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+		for (i = 0; i < priv->num_rx_ring[q]; i++) {
+			dev_kfree_skb(priv->rx_skb[q][i]);
+			priv->rx_skb[q][i] = NULL;
+		}
+	}
+	for (q = RAVB_BE; q < NUM_TX_QUEUE; q++) {
+		for (i = 0; i < priv->num_tx_ring[q]; i++) {
+			dev_kfree_skb(priv->tx_skb[q][i]);
+			priv->tx_skb[q][i] = NULL;

drivers/net/ethernet/renesas/ravb.c new file +39 -0

+/* Packet transmit function for Ethernet AVB */
+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_tstamp_skb *ts_skb = NULL;
+	struct ravb_tx_desc *desc;
+	unsigned long flags;
+	void *buffer;
+	u32 entry;
+	u32 tccr;
+	int q;
+
+	/* If skb needs TX timestamp, it is handled in network control queue */
+	q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC : RAVB_BE;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q] - 4) {
+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+	priv->cur_tx[q]++;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (skb_put_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
+
+	priv->tx_skb[q][entry] = skb;
+	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+	memcpy(buffer, skb->data, skb->len);
+	desc = &priv->tx_ring[q][entry];
+	desc->ds = skb->len;
+	desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+				    DMA_TO_DEVICE);
+	if (dma_mapping_error(&ndev->dev, desc->dptr)) {

drivers/net/ethernet/renesas/ravb.c new file +11 -0

+		ts_skb->skb = skb;
+		ts_skb->tag = priv->ts_skb_tag++;
+		priv->ts_skb_tag %= 0x400;
+		list_add_tail(&ts_skb->list, &priv->ts_skb_list);
+
+		/* TAG and timestamp required flag */
+		skb_shinfo(skb)->tx_flags |= SKBTX_IN_PROGRESS;
+		skb_tx_timestamp(skb);
+		desc->tsr = 1;
+		desc->tag = ts_skb->tag;
+	}

drivers/net/ethernet/renesas/ravb.c new file +74 -0

+	return NETDEV_TX_OK;
+}
+
 
 
 
 
+
+static int ravb_probe(struct platform_device *pdev)
+{
+	struct device_node *np = pdev->dev.of_node;
+	struct ravb_private *priv;
+	struct net_device *ndev;
+	int error, irq, q;
+	struct resource *res;
+
+	if (!np) {
+		dev_err(&pdev->dev,
+			"this driver is required to be instantiated from device tree\n");
+		return -EINVAL;
+	}
+
+	/* Get base address */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "invalid resource\n");
+		return -EINVAL;
+	}
+
+	ndev = alloc_etherdev(sizeof(struct ravb_private));
+	if (!ndev)
+		return -ENOMEM;
+
+	pm_runtime_enable(&pdev->dev);
+	pm_runtime_get_sync(&pdev->dev);
+
+	/* The Ether-specific entries in the device structure. */
+	ndev->base_addr = res->start;
+	ndev->dma = -1;
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		error = -ENODEV;
+		goto out_release;
+	}
+	ndev->irq = irq;
+
+	SET_NETDEV_DEV(ndev, &pdev->dev);
+
+	priv = netdev_priv(ndev);
+	priv->ndev = ndev;
+	priv->pdev = pdev;
+	priv->num_tx_ring[RAVB_BE] = BE_TX_RING_SIZE;
+	priv->num_rx_ring[RAVB_BE] = BE_RX_RING_SIZE;
+	priv->num_tx_ring[RAVB_NC] = NC_TX_RING_SIZE;
+	priv->num_rx_ring[RAVB_NC] = NC_RX_RING_SIZE;
+	priv->addr = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(priv->addr)) {
+		error = PTR_ERR(priv->addr);
+		goto out_release;
+	}
+
+	spin_lock_init(&priv->lock);
+
+	priv->phy_interface = of_get_phy_mode(np);
+
+	priv->no_avb_link = of_property_read_bool(np, "renesas,no-ether-link");
+	priv->avb_link_active_low =
+		of_property_read_bool(np, "renesas,ether-link-active-low");
+
+	ndev->netdev_ops = &ravb_netdev_ops;
+
+	priv->rx_over_errors = 0;
+	priv->rx_fifo_errors = 0;
+	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+		struct net_device_stats *stats = &priv->stats[q];
+
+		stats->rx_packets = 0;
+		stats->tx_packets = 0;

Quoted 5 lines

drivers/net/ethernet/renesas/ravb.c new file +5 -0

+#ifdef __LITTLE_ENDIAN
+	u32 res: 16;	/* Reserved bits */
+	u32 ts_sh: 16;	/* Timestamp high */
+#else
+	u32 ts_sh: 16;	/* Timestamp high */

drivers/net/ethernet/renesas/ravb.c line 765

Quoted 3 lines

drivers/net/ethernet/renesas/ravb.c new file +3 -0

+	u32 res: 16;	/* Reserved bits */
+#endif
+};

I recall a thread in which the use of bitfields for structs that are shared with the hardware was considered a bad idea (because the compiler is free to reorder the fields). Shift operations are probably a better choice here.

Quoted 68 lines

drivers/net/ethernet/renesas/ravb.c new file +68 -0

+struct ravb_desc {
+#ifdef __LITTLE_ENDIAN
+	u32 ds: 12;	/* Descriptor size */
+	u32 cc: 12;	/* Content control */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 dt: 4;	/* Descriptor type */
+#else
+	u32 dt: 4;	/* Descriptor type */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 cc: 12;	/* Content control */
+	u32 ds: 12;	/* Descriptor size */
+#endif
+	u32 dptr;	/* Descriptor pointer */
+};
+
+struct ravb_rx_desc {
+#ifdef __LITTLE_ENDIAN
+	u32 ds: 12;	/* Descriptor size */
+	u32 ei: 1;	/* Error indication */
+	u32 ps: 2;	/* Padding selection */
+	u32 tr: 1;	/* Truncation indication */
+	u32 msc: 8;	/* MAC status code */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 dt: 4;	/* Descriptor type */
+#else
+	u32 dt: 4;	/* Descriptor type */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 msc: 8;	/* MAC status code */
+	u32 ps: 2;	/* Padding selection */
+	u32 ei: 1;	/* Error indication */
+	u32 tr: 1;	/* Truncation indication */
+	u32 ds: 12;	/* Descriptor size */
+#endif
+	u32 dptr;	/* Descpriptor pointer */
+};
+
+struct ravb_ex_rx_desc {
+#ifdef __LITTLE_ENDIAN
+	u32 ds: 12;	/* Descriptor size */
+	u32 ei: 1;	/* Error indication */
+	u32 ps: 2;	/* Padding selection */
+	u32 tr: 1;	/* Truncation indication */
+	u32 msc: 8;	/* MAC status code */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 dt: 4;	/* Descriptor type */
+#else
+	u32 dt: 4;	/* Descriptor type */
+	u32 die: 4;	/* Descriptor interrupt enable */
+			/* 0: disable, other: enable */
+	u32 msc: 8;	/* MAC status code */
+	u32 ps: 2;	/* Padding selection */
+	u32 ei: 1;	/* Error indication */
+	u32 tr: 1;	/* Truncation indication */
+	u32 ds: 12;	/* Descriptor size */
+#endif
+	u32 dptr;	/* Descpriptor pointer */
+	u32 ts_n;	/* Timestampe nsec */
+	u32 ts_sl;	/* Timestamp low */
+#ifdef __LITTLE_ENDIAN
+	u32 res: 16;	/* Reserved bits */
+	u32 ts_sh: 16;	/* Timestamp high */
+#else
+	u32 ts_sh: 16;	/* Timestamp high */

drivers/net/ethernet/renesas/ravb.c line 765

Quoted 3 lines

drivers/net/ethernet/renesas/ravb.c new file +3 -0

+	u32 res: 16;	/* Reserved bits */
+#endif
+};

I recall a thread in which the use of bitfields for structs that are shared with the hardware was considered a bad idea (because the compiler is free to reorder the fields). Shift operations are probably a better choice here.

Quoted 43 lines

drivers/net/ethernet/renesas/ravb.c new file +43 -0

+/* Network device open function for Ethernet AVB */
+static int ravb_open(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int error;
+
+	napi_enable(&priv->napi);
+
+	error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
+			    ndev);
+	if (error) {
+		netdev_err(ndev, "cannot request IRQ\n");
+		goto out_napi_off;
+	}
+
+	/* Descriptor set */
+	/* +26 gets the maximum ethernet encapsulation, +7 & ~7 because the
+	 * card needs room to do 8 byte alignment, +2 so we can reserve
+	 * the first 2 bytes, and +16 gets room for the status word from the
+	 * card.
+	 */
+	priv->rx_buffer_size = (ndev->mtu <= 1492 ? PKT_BUF_SZ :
+				(((ndev->mtu + 26 + 7) & ~7) + 2 + 16));
+
+	error = ravb_ring_init(ndev, RAVB_BE);
+	if (error)
+		goto out_free_irq;
+	error = ravb_ring_init(ndev, RAVB_NC);
+	if (error)
+		goto out_free_irq;
+
+	/* Device init */
+	error = ravb_dmac_init(ndev);
+	if (error)
+		goto out_free_irq;
+	ravb_emac_init(ndev);
+
+	netif_start_queue(ndev);
+
+	/* PHY control start */
+	error = ravb_phy_start(ndev);
+	if (error)
+		goto out_free_irq;

drivers/net/ethernet/renesas/ravb.c line 2162

Quoted 5 lines

drivers/net/ethernet/renesas/ravb.c new file +5 -0

+
+	return 0;
+
+out_free_irq:
+	free_irq(ndev->irq, ndev);

freeing all the memory allocated in the former avb_ring_init calls is missing.

Quoted 20 lines

drivers/net/ethernet/renesas/ravb.c new file +20 -0

+/* Timeout function for Ethernet AVB */
+static void ravb_tx_timeout(struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	int i, q;
+
+	netif_stop_queue(ndev);
+
+	netif_err(priv, tx_err, ndev,
+		  "transmit timed out, status %8.8x, resetting...\n",
+		  ravb_read(ndev, ISS));
+
+	/* tx_errors count up */
+	ndev->stats.tx_errors++;
+
+	/* Free all the skbuffs */
+	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+		for (i = 0; i < priv->num_rx_ring[q]; i++) {
+			dev_kfree_skb(priv->rx_skb[q][i]);
+			priv->rx_skb[q][i] = NULL;

drivers/net/ethernet/renesas/ravb.c line 2195

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c new file +8 -0

+		}
+	}
+	for (q = RAVB_BE; q < NUM_TX_QUEUE; q++) {
+		for (i = 0; i < priv->num_tx_ring[q]; i++) {
+			dev_kfree_skb(priv->tx_skb[q][i]);
+			priv->tx_skb[q][i] = NULL;
+			kfree(priv->tx_buffers[q][i]);
+			priv->tx_buffers[q][i] = NULL;

drivers/net/ethernet/renesas/ravb.c line 2203

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c new file +8 -0

+		}
+	}
+
+	/* Device init */
+	ravb_dmac_init(ndev);
+	ravb_emac_init(ndev);
+	netif_start_queue(ndev);
+}

Does this really work?

At least the hardware should be shut down before
the queues are freed, shouldn't it?

Quoted 43 lines

drivers/net/ethernet/renesas/ravb.c new file +43 -0

+/* Packet transmit function for Ethernet AVB */
+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_tstamp_skb *ts_skb = NULL;
+	struct ravb_tx_desc *desc;
+	unsigned long flags;
+	void *buffer;
+	u32 entry;
+	u32 tccr;
+	int q;
+
+	/* If skb needs TX timestamp, it is handled in network control queue */
+	q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC : RAVB_BE;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q] - 4) {
+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+	priv->cur_tx[q]++;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (skb_put_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
+
+	priv->tx_skb[q][entry] = skb;
+	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+	memcpy(buffer, skb->data, skb->len);
+	desc = &priv->tx_ring[q][entry];
+	desc->ds = skb->len;
+	desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+				    DMA_TO_DEVICE);
+	if (dma_mapping_error(&ndev->dev, desc->dptr)) {
+		dev_kfree_skb_any(skb);
+		priv->tx_skb[q][entry] = NULL;
+		return NETDEV_TX_OK;
+	}

drivers/net/ethernet/renesas/ravb.c line 2253

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+
+	/* TX timestamp required */
+	if (q == RAVB_NC) {
+		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
+		if (!ts_skb)
+			return -ENOMEM;

Dma mapping has to be undone.

Quoted 4 lines

drivers/net/ethernet/renesas/ravb.c new file +4 -0

+	/* Descriptor type must be set after all the above writes */
+	dma_wmb();
+	desc->dt = DT_FSINGLE;
+

drivers/net/ethernet/renesas/ravb.c line 2274

Quoted 5 lines

drivers/net/ethernet/renesas/ravb.c new file +5 -0

+	spin_lock_irqsave(&priv->lock, flags);
+	tccr = ravb_read(ndev, TCCR);
+	if (!(tccr & (TCCR_TSRQ0 << q)))
+		ravb_write(ndev, tccr | (TCCR_TSRQ0 << q), TCCR);
+	spin_unlock_irqrestore(&priv->lock, flags);

According to memory-barriers.txt this needs a mmiowb prior to unlock (there are still a lot more of those candidates in this driver).

Quoted 2 lines

drivers/net/ethernet/renesas/ravb.c new file +2 -0

+static int ravb_probe(struct platform_device *pdev)
+{

Quoted 14 lines

drivers/net/ethernet/renesas/ravb.c new file +14 -0

+	ndev->netdev_ops = &ravb_netdev_ops;
+
+	priv->rx_over_errors = 0;
+	priv->rx_fifo_errors = 0;
+	for (q = RAVB_BE; q < NUM_RX_QUEUE; q++) {
+		struct net_device_stats *stats = &priv->stats[q];
+
+		stats->rx_packets = 0;
+		stats->tx_packets = 0;
+		stats->rx_bytes = 0;
+		stats->tx_bytes = 0;
+		stats->multicast = 0;
+		stats->rx_errors = 0;
+		stats->rx_crc_errors = 0;

drivers/net/ethernet/renesas/ravb.c line 2920

Quoted 5 lines

drivers/net/ethernet/renesas/ravb.c new file +5 -0

+		stats->rx_frame_errors = 0;
+		stats->rx_length_errors = 0;
+		stats->rx_missed_errors = 0;
+		stats->rx_over_errors = 0;
+	}

The memory returned by alloc_etherdev is already zeroed so this is not necessary.

Also maybe it would be better to split the driver into more source files.

The result would be much easier to understand and to review. For example all ptp related code could be put into its own file.

Regards,
Lino

Quoted 5 lines

I recall a thread in which the use of bitfields for structs that are shared with the hardware was considered a bad idea (because the compiler is free to reorder the fields). Shift operations are probably a better choice here.

Well, it looks as the compiler is not free to reorder bit fields, and the
order is determined by the ABI. Will look into getting rid of them anyway...

Quoted 43 lines

Quoted 43 lines

drivers/net/ethernet/renesas/ravb.c new file +43 -0

+/* Packet transmit function for Ethernet AVB */
+static int ravb_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+	struct ravb_private *priv = netdev_priv(ndev);
+	struct ravb_tstamp_skb *ts_skb = NULL;
+	struct ravb_tx_desc *desc;
+	unsigned long flags;
+	void *buffer;
+	u32 entry;
+	u32 tccr;
+	int q;
+
+	/* If skb needs TX timestamp, it is handled in network control queue */
+	q = (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) ? RAVB_NC : RAVB_BE;
+
+	spin_lock_irqsave(&priv->lock, flags);
+	if (priv->cur_tx[q] - priv->dirty_tx[q] >= priv->num_tx_ring[q] - 4) {
+		if (!ravb_tx_free(ndev, q)) {
+			netif_warn(priv, tx_queued, ndev, "TX FD exhausted.\n");
+			netif_stop_queue(ndev);
+			spin_unlock_irqrestore(&priv->lock, flags);
+			return NETDEV_TX_BUSY;
+		}
+	}
+	entry = priv->cur_tx[q] % priv->num_tx_ring[q];
+	priv->cur_tx[q]++;
+	spin_unlock_irqrestore(&priv->lock, flags);
+
+	if (skb_put_padto(skb, ETH_ZLEN))
+		return NETDEV_TX_OK;
+
+	priv->tx_skb[q][entry] = skb;
+	buffer = PTR_ALIGN(priv->tx_buffers[q][entry], RAVB_ALIGN);
+	memcpy(buffer, skb->data, skb->len);
+	desc = &priv->tx_ring[q][entry];
+	desc->ds = skb->len;
+	desc->dptr = dma_map_single(&ndev->dev, buffer, skb->len,
+				    DMA_TO_DEVICE);
+	if (dma_mapping_error(&ndev->dev, desc->dptr)) {
+		dev_kfree_skb_any(skb);
+		priv->tx_skb[q][entry] = NULL;
+		return NETDEV_TX_OK;
+	}

Quoted 8 lines

drivers/net/ethernet/renesas/ravb.c line 2253

Quoted 6 lines

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+
+	/* TX timestamp required */
+	if (q == RAVB_NC) {
+		ts_skb = kmalloc(sizeof(*ts_skb), GFP_ATOMIC);
+		if (!ts_skb)
+			return -ENOMEM;

Dma mapping has to be undone.

OK, fixed. Not sure what we should return in this case: error code or
NETDEV_TX_OK...

Quoted 1 line

example all ptp related code could be put into its own file.

OK, will try to split the driver back... Perhaps I should also split the
patch accordingly?

drivers/net/ethernet/renesas/ravb.c new file +17 -0

+	/* Stop the E-MAC's RX processes. */
+	ravb_write(ndev, ravb_read(ndev, ECMR) & ~ECMR_RE, ECMR);
+
+	/* Wait for stopping the RX DMA process */
+	ravb_wait(ndev, CSR, CSR_RPO, 0);
+}
+
+/* Caller must hold the lock */
+static void ravb_ptp_update_compare(struct ravb_private *priv, u32 ns)
+{
+	struct net_device *ndev = priv->ndev;
+	/* When the comparison value (GPTC.PTCV) is in range of
+	 * [x-1 to x+1] (x is the configured increment value in
+	 * GTI.TIV), it may happen that a comparison match is
+	 * not detected when the timer wraps around.
+	 */
+	u32 gti_ns_plus_1 = (priv->ptp.current_addend >> 20) + 1;

drivers/net/ethernet/renesas/ravb.c new file +6 -0

+	}
+}
 
+/* Caller must hold lock */
+static void ravb_ptp_time_write(struct ravb_private *priv,
+				const struct timespec64 *ts)
+{

drivers/net/ethernet/renesas/ravb.c new file +20 -0

+}
+
+/* Caller must hold lock */
+static u64 ravb_ptp_cnt_read(struct ravb_private *priv)
+{
+	struct timespec64 ts;
+	ktime_t kt;
+
+	ravb_ptp_time_read(priv, &ts);
+	kt = timespec64_to_ktime(ts);
+
+	return ktime_to_ns(kt);
+}
+
+/* Caller must hold lock */
+static void ravb_ptp_cnt_write(struct ravb_private *priv, u64 ns)
+{
+	struct timespec64 ts = ns_to_timespec64(ns);
+
+	ravb_ptp_time_write(priv, &ts);

drivers/net/ethernet/renesas/ravb.c new file +5 -0

+	val = ravb_read(ndev, GCCR) & ~GCCR_TCSS;
+	ravb_write(ndev, val | (sel << 8), GCCR);
+}
+
+/* Caller must hold lock */

drivers/net/ethernet/renesas/ravb.c new file +15 -0

+}
+
+/* PTP clock operations */
+static int ravb_ptp_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
+{
+	struct ravb_private *priv = container_of(ptp, struct ravb_private,
+						 ptp.info);
+	unsigned long flags;
+	u32 diff, addend;
+	int neg_adj = 0;
+	u64 adj;
+
+	if (ppb < 0) {
+		neg_adj = 1;
+		ppb = -ppb;