Hello. On 04/14/2015 01:38 AM, Florian Fainelli wrote:
[snip]
+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.
Thanks for the idea.
[snip]
+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);
OK, missed this.
+} + +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 */ + 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).
Yes, only 100/1000, at least so says the manual.
[snip]
+ 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.
+ } 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?
We don't set the descriptor type to FEMPTY for such cases, so the AVB-DMAC shouldn't handle such descriptors.
[...]
+ 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?
The comments in ravb_interrupt() say that the network control queue should be handled ASAP, due to timestamping.
+ 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. */ + 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?
We don't expect the allocation to succeed after it failed, so the *break* is appropriate, I think.
[snip]
+/* 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?
Perhaps, though the manual doesn't say so...
+ /* 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]
+ /* 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...
+ result = IRQ_HANDLED; + }
[snip]
+ if (ecmd->duplex == DUPLEX_FULL) + priv->duplex = 1; + else + priv->duplex = 0;
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?
[...]
+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. [...]
+/* 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));
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...
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.
[...]
+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) {
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()...
+ 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);
~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?
+ 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... :-)
+ 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? [...] WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
