Re: [PATCH v6 2/7] net: mvneta: driver for Marvell Armada 370/XP network unit

Thomas Petazzoni [off-list ref] :
[...]

+static int mvneta_mac_addr_set(struct mvneta_port *pp, unsigned char *addr,
+			       int queue)
+{
+	unsigned int mac_h;
+	unsigned int mac_l;
+
+	if (queue >= 1) {
+		netdev_err(pp->dev, "RX queue #%d is out of range\n", queue);
+		return -EINVAL;
+	}

(whence q <= 0)

+
+	if (queue != -1) {
+		mac_l = (addr[4] << 8) | (addr[5]);
+		mac_h = (addr[0] << 24) | (addr[1] << 16) |
+			(addr[2] << 8) | (addr[3] << 0);
+
+		mvreg_write(pp, MVNETA_MAC_ADDR_LOW, mac_l);
+		mvreg_write(pp, MVNETA_MAC_ADDR_HIGH, mac_h);
+	}

What is it trying to achieve with the "queue" argument ?

[...]

+/* Refill processing */
+static int mvneta_rx_refill(struct mvneta_port *pp,
+			    struct mvneta_rx_desc *rx_desc)
+
+{
+	dma_addr_t phys_addr;
+	struct sk_buff *skb;
+
+	skb = netdev_alloc_skb(pp->dev, pp->pkt_size);
+	if (!skb)
+		return -ENOMEM;

Could netdev_alloc_skb_ip_align be an option ?

[...]

+static int mvneta_rx(struct mvneta_port *pp, int rx_todo,
+		     struct mvneta_rx_queue *rxq)
+{
+	struct net_device *dev = pp->dev;
+	int rx_done, rx_filled;
+
+	/* Get number of received packets */
+	rx_done = mvneta_rxq_busy_desc_num_get(pp, rxq);
+
+	if (rx_todo > rx_done)
+		rx_todo = rx_done;
+
+	rx_done = 0;
+	rx_filled = 0;
+
+	/* Fairness NAPI loop */
+	while (rx_done < rx_todo) {
+		struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);
+		struct sk_buff *skb;
+		u32 rx_status;
+		int rx_bytes, err;
+
+		prefetch(rx_desc);
+		rx_done++;
+		rx_filled++;
+		rx_status = rx_desc->status;
+		skb = (struct sk_buff *)rx_desc->buf_cookie;
+
+		if (!mvneta_rxq_desc_is_first_last(rx_desc) ||
+		    (rx_status & MVNETA_RXD_ERR_SUMMARY)) {
+			dev->stats.rx_errors++;
+			mvneta_rx_error(pp, rx_desc);
+			mvneta_rx_desc_fill(rx_desc, rx_desc->buf_phys_addr,
+					    (u32)skb);
+			continue;
+		}
+
+		dma_unmap_single(pp->dev->dev.parent, rx_desc->buf_phys_addr,
+				 rx_desc->data_size, DMA_FROM_DEVICE);
+
+		rx_bytes = rx_desc->data_size -
+			(MVNETA_ETH_CRC_SIZE + MVNETA_MH_SIZE);
+		u64_stats_update_begin(&pp->rx_stats.syncp);
+		pp->rx_stats.packets++;
+		pp->rx_stats.bytes += rx_bytes;
+		u64_stats_update_end(&pp->rx_stats.syncp);
+
+		/* Linux processing */
+		skb_reserve(skb, MVNETA_MH_SIZE);
+		skb_put(skb, rx_bytes);

(I suggested to use skb_reserve / skb_put instead of hand-crafted code but
I may have ignored the elephant in the living room)

I understand the skb_put, ok. What is the purpose of the skb_reserve ?

Are MVNETA_ETH_CRC_SIZE (4) and MVNETA_MH_SIZE (2) really different from
ETH_FCS_LEN and NET_IP_ALIGN ?

[...]

+static irqreturn_t mvneta_isr(int irq, void *dev_id)
+{
+	struct mvneta_port *pp = (struct mvneta_port *)dev_id;
+
+	/* Mask all interrupts */
+	mvreg_write(pp, MVNETA_INTR_NEW_MASK, 0);
+
+	/* Verify that the device not already on the polling list */
+	if (napi_schedule_prep(&pp->napi))
+		__napi_schedule(&pp->napi);

Hand-crafted napi_schedule.

[...]

+static int mvneta_open(struct net_device *dev)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	int ret;
+
+	ret = mvneta_mac_addr_set(pp, dev->dev_addr, rxq_def);
+	if (ret < 0) {
+		netdev_err(dev, "mvneta_mac_addr_set failed\n");
+		goto mac_addr_set_failure;
+	}

AFAIU mvneta_mac_addr_set can only fail when (module parameter) rxq_def is
not set correctly. It imho calls for a check in probe/remove and a removal
of the failure case in mvneta_mac_addr_set.

+
+	pp->pkt_size = MVNETA_RX_PKT_SIZE(pp->dev->mtu);
+
+	ret = mvneta_setup_rxqs(pp);
+	if (ret)
+		goto rxqs_setup_failure;
+
+	ret = mvneta_setup_txqs(pp);
+	if (ret)
+		goto txqs_setup_failure;
+
+	/* Connect to port interrupt line */
+	ret = request_irq(pp->dev->irq, mvneta_isr, IRQF_DISABLED,
+			  MVNETA_DRIVER_NAME, pp);

include/linux/interrupt.h
[...]
 * IRQF_DISABLED - keep irqs disabled when calling the action handler.
 *                 DEPRECATED. This flag is a NOOP and scheduled to be removed

[...]

+static int __devinit mvneta_probe(struct platform_device *pdev)
+{
+	const struct mbus_dram_target_info *dram_target_info;
+	struct device_node *dn = pdev->dev.of_node;
+	struct device_node *phy_node;
+	u32 phy_addr, clk_rate_hz;
+	struct mvneta_port *pp;
+	struct net_device *dev;
+	const char *mac_addr;
+	int phy_mode;
+	int err;
+
+	dev = alloc_etherdev_mq(sizeof(struct mvneta_port), 8);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->irq = irq_of_parse_and_map(dn, 0);
+	if (dev->irq == 0) {
+		err = -EINVAL;
+		goto err_irq;
+	}
+
+	phy_node = of_parse_phandle(dn, "phy", 0);
+	if (!phy_node) {
+		dev_err(&pdev->dev, "no associated PHY\n");
+		err = -ENODEV;
+		goto err_node;
+	}
+
+	phy_mode = of_get_phy_mode(dn);
+	if (phy_mode < 0) {
+		dev_err(&pdev->dev, "incorrect phy-mode\n");
+		err = -EINVAL;
+		goto err_node;
+	}
+
+	if (of_property_read_u32(dn, "clock-frequency", &clk_rate_hz) != 0) {
+		dev_err(&pdev->dev, "could not read clock-frequency\n");
+		err = -EINVAL;
+		goto err_node;
+	}
+
+	mac_addr = of_get_mac_address(dn);
+
+	if (!mac_addr || !is_valid_ether_addr(mac_addr))
+		eth_hw_addr_random(dev);
+	else
+		memcpy(dev->dev_addr, mac_addr, ETH_ALEN);
+
+	dev->tx_queue_len = MVNETA_MAX_TXD;
+	dev->watchdog_timeo = 5 * HZ;
+	dev->netdev_ops = &mvneta_netdev_ops;
+
+	SET_ETHTOOL_OPS(dev, &mvneta_eth_tool_ops);
+
+	pp = netdev_priv(dev);
+
+	pp->tx_done_timer.function = mvneta_tx_done_timer_callback;
+	init_timer(&pp->tx_done_timer);
+	clear_bit(MVNETA_F_TX_DONE_TIMER_BIT, &pp->flags);
+
+	pp->weight = MVNETA_RX_POLL_WEIGHT;
+	pp->clk_rate_hz = clk_rate_hz;
+	pp->phy_node = phy_node;
+	pp->phy_interface = phy_mode;
+
+	pp->base = of_iomap(dn, 0);
+	if (pp->base == NULL) {
+		err = -ENOMEM;
+		goto err_node;
+	}
+
+	pp->tx_done_timer.data = (unsigned long)dev;
+
+	pp->tx_ring_size = MVNETA_MAX_TXD;
+	pp->rx_ring_size = MVNETA_MAX_RXD;
+
+	pp->dev = dev;
+	SET_NETDEV_DEV(dev, &pdev->dev);
+
+	if (mvneta_init(pp, phy_addr)) {
+		dev_err(&pdev->dev, "can't init eth hal\n");
+		err = -ENODEV;
+		goto err_base;
+	}

The error code from mvneta_init() should be used.

+	mvneta_port_power_up(pp, phy_mode);
+
+	dram_target_info = mv_mbus_dram_info();
+	if (dram_target_info)
+		mvneta_conf_mbus_windows(pp, dram_target_info);
+
+	netif_napi_add(dev, &pp->napi, mvneta_poll, pp->weight);
+
+	if (register_netdev(dev)) {
+		dev_err(&pdev->dev, "failed to register\n");
+		err = ENOMEM;
+		goto err_base;
+	}

- The error code from register_netdev() should be used.
- Leak: allocations in mvneta_init() are not balanced.
- Nit: the "err_where_did_it_trigger_first" style of label shows its
  downside when compared to the "err_what_should_be_done" when
  it gets used several times.

+
+	dev->features = NETIF_F_SG | NETIF_F_IP_CSUM;
+	dev->hw_features =  NETIF_F_SG | NETIF_F_IP_CSUM;
+	dev->priv_flags |= IFF_UNICAST_FLT;
+
+	dev_info(&pdev->dev, "%s, mac: %pM\n", dev->name,
+		 dev->dev_addr);
+
+	platform_set_drvdata(pdev, pp->dev);
+
+	return 0;
+err_base:
+	iounmap(pp->base);
+err_node:
+	irq_dispose_mapping(dev->irq);
+err_irq:
+	free_netdev(dev);
+	return err;
+}
+
+/* Device removal routine */
+static int __devexit mvneta_remove(struct platform_device *pdev)
+{
+	struct net_device  *dev = platform_get_drvdata(pdev);
+	struct mvneta_port *pp = netdev_priv(dev);
+
+	iounmap(pp->base);
+
+	unregister_netdev(dev);
+	irq_dispose_mapping(dev->irq);
+	free_netdev(dev);
+	mvneta_deinit(pp);

One may expect the same ordering as the mvneta_probe unroll sequence.

-- 
Ueimor