On 06/14/2013 02:20 AM, Francois Romieu wrote:
> Alexey Brodkin <[email protected]> :
> [...]
>> diff --git a/drivers/net/ethernet/arc/arc_emac.h
>> b/drivers/net/ethernet/arc/arc_emac.h
>> new file mode 100644
>> index 0000000..6691db2
>> --- /dev/null
>> +++ b/drivers/net/ethernet/arc/arc_emac.h
> [...]
>> +struct arc_emac_priv {
>> + struct net_device_stats stats;
>> + unsigned int clock_frequency;
>> + unsigned int max_speed;
>> +
>> + /* Pointers to BD rings - CPU side */
>> + struct arc_emac_bd_t *rxbd;
>
> There does not seem to be much need for rxbd->data.
Could you please clarify this comment? Not clear what do you mean.
>> + struct arc_emac_bd_t *txbd;
>
> [...]
>> +static int arc_emac_get_settings(struct net_device *ndev,
>> + struct ethtool_cmd *cmd)
>> +{
>> + struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> + if (priv->phy_dev)
>> + return phy_ethtool_gset(priv->phy_dev, cmd);
>
> arc_emac_probe() succeeded : priv->phy_dev can't be NULL.
>
> [...]
>> +static int arc_emac_set_settings(struct net_device *ndev,
>> + struct ethtool_cmd *cmd)
>> +{
>> + struct arc_emac_priv *priv = netdev_priv(ndev);
>> +
>> + if (!capable(CAP_NET_ADMIN))
>> + return -EPERM;
>> +
>> + if (priv->phy_dev)
>> + return phy_ethtool_sset(priv->phy_dev, cmd);
>
> priv->phy_dev can't be NULL either.
Nice catch! Fortunately I recognized it as well and already removed
those useless checks.
> [...]
>> +static int arc_emac_poll(struct napi_struct *napi, int budget)
>> +{
>> + struct net_device *ndev = napi->dev;
>> + struct arc_emac_priv *priv = netdev_priv(ndev);
>> + unsigned int i, loop, work_done = 0;
>> + struct sk_buff *skb;
>> +
>> + /* Loop thru the BD chain, but not always from 0.
>> + * Start from right after where we last saw a packet.
>> + */
>> + i = priv->last_rx_bd;
>> +
>> + for (loop = 0; loop < RX_BD_NUM; loop++) {
>> + struct net_device_stats *stats = &priv->stats;
>> + struct buffer_state *rx_buff;
>> + struct arc_emac_bd_t *rxbd;
>> + dma_addr_t addr;
>> + unsigned int info, pktlen;
>> + unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
>> +
>> + i = (i + 1) % RX_BD_NUM;
>> + rx_buff = &priv->rx_buff[i];
>> + rxbd = &priv->rxbd[i];
>> + info = rxbd->info;
>
> You may / should update 'i' so that you can:
>
> struct buffer_state *rx_buff = priv->rx_buff + i;
> struct arc_emac_bd_t *rxbd = priv->rxbd + i;
> unsigned int pktlen, info = rxbd->info;
Done)
>> +
>> + /* Process current BD */
>> +
>> + if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> + /* BD is still owned by EMAC */
>> + continue;
>> + }
>
> Processing starts at priv->last_rx_bd. Could it not be a 'break' ?
It has to be! Fixed.
> [...]
>> + pktlen = info & LEN_MASK;
>> + stats->rx_packets++;
>> + stats->rx_bytes += pktlen;
>> + skb = rx_buff->skb;
>> + skb_put(skb, pktlen);
>> + skb->dev = ndev;
>> + skb->protocol = eth_type_trans(skb, ndev);
>> + netif_receive_skb(skb);
>> +
>> + dma_unmap_single(&ndev->dev,
>> + dma_unmap_addr(&rx_buff, addr),
>> + dma_unmap_len(&rx_buff, len),
>> + DMA_FROM_DEVICE);
>
> dma_unmap_single(&ndev->dev, dma_unmap_addr(&rx_buff, addr),
> dma_unmap_len(&rx_buff, len), DMA_FROM_DEVICE);
>
> Please unmap before netif_receive_skb.
Done. Thanks.
>> +
>> + /* Prepare the BD for next cycle */
>> + rx_buff->skb = netdev_alloc_skb_ip_align(ndev, buflen);
>> + if (unlikely(!rx_buff->skb)) {
>> + if (net_ratelimit())
>> + netdev_err(ndev, "cannot allocate skb\n");
>> + stats->rx_errors++;
>> + continue;
>> + }
>
> The descriptor entry is left unchanged. Afaiu the driver will move to the
> next descriptor and crash on dereferencing NULL (rx_buff->)skb next time
> it wraps.
>
> I suggest avoiding holes: don't netif_receive_skb if you can't alloc a new
> skb.
Frankly I cannot understand how "don't netif_receive_skb" for one of the
received packets helps to prevent crash on the next iteration?
And I don't see a way to return any error state from NAPI poll handler.
Could you please clarify your idea?
> [...]
>> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
>> +{
>> + struct net_device *ndev = dev_instance;
>> + struct arc_emac_priv *priv = netdev_priv(ndev);
>> + struct net_device_stats *stats = &priv->stats;
>> + unsigned int status;
>> +
>> + status = arc_reg_get(priv, R_STATUS);
>> + status &= ~MDIO_MASK;
>> +
>> + /* Reset all flags except "MDIO complete"*/
>> + arc_reg_set(priv, R_STATUS, status);
>> +
>> + if (status & RXINT_MASK) {
>> + if (likely(napi_schedule_prep(&priv->napi))) {
>> + arc_reg_clr(priv, R_ENABLE, RXINT_MASK);
>> + __napi_schedule(&priv->napi);
>> + }
>> + }
>> +
>> + if (status & TXINT_MASK) {
>
> You may consider moving everything into the napi poll handler.
Agree. Would be good - implementing.
> [...]
>> +static int arc_emac_open(struct net_device *ndev)
>> +{
>> + struct arc_emac_priv *priv = netdev_priv(ndev);
>> + struct phy_device *phy_dev = priv->phy_dev;
>> + struct arc_emac_bd_t *bd;
>> + struct sk_buff *skb;
>> + int i;
>> +
>> + phy_dev->autoneg = AUTONEG_ENABLE;
>> + phy_dev->speed = 0;
>> + phy_dev->duplex = 0;
>> + phy_dev->advertising = phy_dev->supported;
>> +
>> + if (priv->max_speed > 100) {
>> + phy_dev->advertising &= PHY_GBIT_FEATURES;
>> + } else if (priv->max_speed <= 100) {
>> + phy_dev->advertising &= PHY_BASIC_FEATURES;
>> + if (priv->max_speed <= 10) {
>> + phy_dev->advertising &= ~SUPPORTED_100baseT_Half;
>> + phy_dev->advertising &= ~SUPPORTED_100baseT_Full;
>> + }
>> + }
>> +
>> + /* Allocate and set buffers for Rx BD's */
>> + bd = priv->rxbd;
>> + for (i = 0; i < RX_BD_NUM; i++) {
>> + skb = netdev_alloc_skb_ip_align(ndev, ndev->mtu +
>> + EMAC_BUFFER_PAD);
>> +
>
> Missing NULL check.
Thanks!
> [...]
>> + /* Set BD ring pointers for device side */
>> + arc_reg_set(priv, R_RX_RING,
>> + (unsigned int)priv->rxbd_dma_hdl);
>
> arc_reg_set(priv, R_RX_RING, (unsigned int)priv->rxbd_dma_hdl);
>
>> +
>> + arc_reg_set(priv, R_TX_RING,
>> + (unsigned int)priv->rxbd_dma_hdl + RX_RING_SZ);
>
> Forgot priv->txbd_dma_hdl ?
Well, it was used implicitly, because priv->rxbd_dma_hdl + RX_RING_SZ =
priv->txbd_dma_hdl. Replaced with explicit priv->txbd_dma_hdl.
>> +
>> + /* Set Poll rate so that it polls every 1 ms */
>> + arc_reg_set(priv, R_POLLRATE,
>> + priv->clock_frequency / 1000000);
>
> arc_reg_set(priv, R_POLLRATE, priv->clock_frequency / 1000000);
>
>> +
>> + /* Enable interrupts */
>> + arc_reg_set(priv, R_ENABLE,
>> + TXINT_MASK | RXINT_MASK | ERR_MASK);
>
> arc_reg_set(priv, R_ENABLE, TXINT_MASK | RXINT_MASK | ERR_MASK);
I see, thanks.
>> +
>> + /* Set CONTROL */
>> + arc_reg_set(priv, R_CTRL,
>> + (RX_BD_NUM << 24) | /* RX BD table length */
>> + (TX_BD_NUM << 16) | /* TX BD table length */
>> + TXRN_MASK | RXRN_MASK);
>> +
>> + /* Enable EMAC */
>> + arc_reg_or(priv, R_CTRL, EN_MASK);
>> +
>> + phy_start_aneg(priv->phy_dev);
>> +
>> + netif_start_queue(ndev);
>> + napi_enable(&priv->napi);
>
> napi must be enabled before the first rx packet / irq kicks in.
Thanks.
> [...]
>> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
>> +{
>> + struct arc_emac_priv *priv = netdev_priv(ndev);
>> + unsigned int info, len, *txbd_curr = &priv->txbd_curr;
>> + dma_addr_t addr;
>> + char *pkt = skb->data;
>> +
>> + len = max_t(unsigned int, ETH_ZLEN, skb->len);
>
> The device automatically pads, right ?
What do you mean here?
>> + info = priv->txbd[*txbd_curr].info;
>> +
>> + /* EMAC still holds this buffer in its possession.
>> + * CPU must not modify this buffer descriptor
>> + */
>> + if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
>> + netif_stop_queue(ndev);
>> + return NETDEV_TX_BUSY;
>
> It is more than unlikely: you may check for it as a bug but you should
> stop_queue when the tx ring is full (before returning below).
So I left this check, but add another one near bottom of Tx function to
prevent mentioned situation.
>> + }
>> +
>> + addr = dma_map_single(&ndev->dev, (void *)pkt, len, DMA_TO_DEVICE);
>> + if (unlikely(dma_mapping_error(&ndev->dev, addr))) {
>> + dev_kfree_skb(skb);
>> + return NETDEV_TX_OK;
>
> Please update tx_dropped stats.
Done.
>> + }
>> + dma_unmap_addr_set(&priv->tx_buff[*txbd_curr], mapping, addr);
>> + dma_unmap_len_set(&priv->tx_buff[*txbd_curr], len, len);
>> +
>> + priv->tx_buff[*txbd_curr].skb = skb;
>> + priv->txbd[*txbd_curr].data = pkt;
>
> Insert memory barrier.
Very good and useful point, done with verbose comment.
>> + priv->txbd[*txbd_curr].info = FOR_EMAC | FRST_MASK | LAST_MASK | len;
>> +
>> + *txbd_curr = (*txbd_curr + 1) % TX_BD_NUM;
>> +
>> + arc_reg_set(priv, R_STATUS, TXPL_MASK);
>> +
>> + skb_tx_timestamp(skb);
>> +
>> + return NETDEV_TX_OK;
>> +}
>
> [...]
>> +static int arc_emac_probe(struct platform_device *pdev)
>> +{
> [...]
>> + err = arc_mdio_probe(pdev->dev.of_node, priv);
>> + if (err) {
>> + dev_err(&pdev->dev, "failed to probe MII bus\n");
>> + goto out;
>> + }
>> +
>> +
>> + priv->phy_dev = of_phy_connect(ndev, priv->phy_node,
>> + arc_emac_adjust_link, 0,
>> + PHY_INTERFACE_MODE_MII);
>> + if (!priv->phy_dev) {
>> + netdev_err(ndev, "of_phy_connect() failed\n");
>> + return -ENODEV;
>
> arc_mdio_remove leak.
Indeed) Fixed.
-Alexey
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
