Message-ID: <20121103115321.GA4539@electric-eye.fr.zoreil.com>
Date:	Sat, 3 Nov 2012 12:53:21 +0100
From:	Francois Romieu <romieu@...zoreil.com>
To:	Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>
Cc:	Eric Dumazet <eric.dumazet@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Lennert Buytenhek <kernel@...tstofly.org>,
	netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Jason Cooper <jason@...edaemon.net>,
	Andrew Lunn <andrew@...n.ch>,
	Gregory Clement <gregory.clement@...e-electrons.com>,
	Lior Amsalem <alior@...vell.com>,
	Maen Suleiman <maen@...vell.com>
Subject: Re: [PATCH v4] Network driver for the Armada 370 and Armada XP ARM Marvell SoCs

Thomas Petazzoni <thomas.petazzoni@...e-electrons.com> :
[...]
> Any comments on this patch set? What should I do to make it progress
> towards integration?

Nits review above. I'll search more substantial things this evening.
It looks quite good.

Thomas Petazzoni <thomas.petazzoni@...e-electrons.com> :
[...]
> +mvneta_rxq_next_desc_get(struct mvneta_rx_queue *rxq)
> +{
> +	int rx_desc = rxq->next_desc_to_proc;
> +	rxq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(rxq, rx_desc);

Insert an empty line after local variables declaration.

[...]
> +static struct mvneta_tx_desc *
> +mvneta_txq_next_desc_get(struct mvneta_tx_queue *txq)
> +{
> +	int tx_desc = txq->next_desc_to_proc;
> +	txq->next_desc_to_proc = MVNETA_QUEUE_NEXT_DESC(txq, tx_desc);

Insert an empty line after local variables declaration.

[...]
> +static struct mvneta_tx_queue *mvneta_tx_done_policy(struct mvneta_port *pp,
> +						     u32 cause)
> +{
> +	int queue;
> +	queue = fls(cause) - 1;

Insert an empty line after local variables declaration.

You may set this variable when you declare it.

> +	if (queue < 0 || queue >= txq_number)
> +		return NULL;
> +	return &pp->txqs[queue];

	return (queue < 0 || queue >= txq_number) ? NULL : &pp->txqs[queue];

[...]
> +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 1;
> +
> +	phys_addr = dma_map_single(pp->dev->dev.parent, skb->head,
> +				   MVNETA_RX_BUF_SIZE(pp->pkt_size),
> +				   DMA_FROM_DEVICE);
> +	if (unlikely(dma_mapping_error(pp->dev->dev.parent,
> +				       phys_addr))) {

	if (unlikely(dma_mapping_error(pp->dev->dev.parent, phys_addr))) {

> +		dev_kfree_skb(skb);
> +		return 1;
> +	}
> +
> +	mvneta_rx_desc_fill(rx_desc, phys_addr, (u32)skb);
> +
> +	return 0;
> +}

This is a bool returning function in disguise.

Were it supposed to be an error returning function, it should return <= 0

[...]
> +static struct mvneta_rx_queue *mvneta_rx_policy(struct mvneta_port *pp,
> +						u32 cause)
> +{
> +	int queue = fls(cause >> 8) - 1;

Insert an empty line after local variables declaration.

> +	if (queue < 0 || queue >= rxq_number)
> +		return NULL;
> +	return &pp->rxqs[queue];

See mvneta_tx_done_policy above.

[...]
> +static void mvneta_rxq_drop_pkts(struct mvneta_port *pp,
> +				 struct mvneta_rx_queue *rxq)
> +{
> +	int rx_done, i;
> +
> +	rx_done = mvneta_rxq_busy_desc_num_get(pp, rxq);
> +	for (i = 0; i < rxq->size; i++) {
> +		struct mvneta_rx_desc *rx_desc = rxq->descs + i;
> +		struct sk_buff *skb = (struct sk_buff *)rx_desc->buf_cookie;
> +		dev_kfree_skb_any(skb);

Insert an empty line after local variables declaration.

[...]
> +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 (((rx_status & MVNETA_RXD_FIRST_LAST_DESC)
> +		     != MVNETA_RXD_FIRST_LAST_DESC)
> +		    || (rx_status & MVNETA_RXD_ERR_SUMMARY)) {

Operators appear at the end.

A dedicated (inline) function will help.

[...]
> +static int mvneta_tx_frag_process(struct mvneta_port *pp, struct sk_buff *skb,
[...]
> +		if (i == (skb_shinfo(skb)->nr_frags - 1)) {
> +			/* Last descriptor */
> +			tx_desc->command = (MVNETA_TXD_L_DESC |
> +					    MVNETA_TXD_Z_PAD);

			tx_desc->command = MVNETA_TXD_L_DESC | MVNETA_TXD_Z_PAD;

[...]
> +error:
> +	/* Release all descriptors that were used to map fragments of
> +	 * this packet, as well as the corresponding DMA mappings */
> +	for (j = i - 1; j >= 0; j--) {

j isn't needed. Recycle i.

[...]
> +static int mvneta_tx(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +	struct mvneta_tx_queue *txq = &pp->txqs[txq_def];
> +	struct netdev_queue *nq;
> +	struct mvneta_tx_desc *tx_desc;
> +	int frags = 0;
> +	u32 tx_cmd;

Long lines first when possible.

[...]
> +		/* Continue with other skb fragments */
> +		if (mvneta_tx_frag_process(pp, skb, txq)) {

			got out_unmap;

> +			dma_unmap_single(dev->dev.parent,
> +					 tx_desc->buf_phys_addr,
> +					 tx_desc->data_size,
> +					 DMA_TO_DEVICE);
> +			mvneta_txq_desc_put(txq);
> +			frags = 0;
> +			goto out;
> +		}

[...]
> +static void mvneta_txq_done_force(struct mvneta_port *pp,
> +				  struct mvneta_tx_queue *txq)
> +
> +{
> +	int tx_done = txq->count;
> +	mvneta_txq_bufs_free(pp, txq, tx_done);

Insert an empty line after local variables declaration.

[...]
> +static int mvneta_rxq_fill(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
> +			   int num)
> +{
> +	int i;
> +	struct net_device *dev = pp->dev;

Long lines first when possible.

[...]
> +static int mvneta_rxq_init(struct mvneta_port *pp,
> +			   struct mvneta_rx_queue *rxq)
> +
> +{
> +	rxq->size = pp->rx_ring_size;
> +
> +	/* Allocate memory for RX descriptors */
> +	rxq->descs = dma_alloc_coherent(pp->dev->dev.parent,
> +					rxq->size * MVNETA_DESC_ALIGNED_SIZE,
> +					&rxq->descs_phys,
> +					GFP_KERNEL);

					&rxq->descs_phys, GFP_KERNEL);

> +	if (rxq->descs == NULL) {
> +		netdev_err(pp->dev,
> +			   "rxQ=%d: Can't allocate %d bytes for %d RX descr\n",
> +			   rxq->id, rxq->size * MVNETA_DESC_ALIGNED_SIZE,
> +			   rxq->size);
> +		return -ENOMEM;
> +	}
> +
> +	BUG_ON(rxq->descs !=
> +	       PTR_ALIGN(rxq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));

There is no reason to crash.

(...]
> +static int mvneta_txq_init(struct mvneta_port *pp,
> +			   struct mvneta_tx_queue *txq)
> +{
> +	txq->size = pp->tx_ring_size;
> +
> +	/* Allocate memory for TX descriptors */
> +	txq->descs = dma_alloc_coherent(pp->dev->dev.parent,
> +					txq->size * MVNETA_DESC_ALIGNED_SIZE,
> +					&txq->descs_phys,
> +					DMA_BIDIRECTIONAL);

					&txq->descs_phys, DMA_BIDIRECTIONAL);

> +	if (txq->descs == NULL) {
> +		netdev_err(pp->dev,
> +			   "txQ=%d: Can't allocate %d bytes for %d TX descr\n",
> +			   txq->id, txq->size * MVNETA_DESC_ALIGNED_SIZE,
> +			   txq->size);
> +		return -ENOMEM;
> +	}
> +
> +	/* Make sure descriptor address is cache line size aligned  */
> +	BUG_ON(txq->descs !=
> +	       PTR_ALIGN(txq->descs, MVNETA_CPU_D_CACHE_LINE_SIZE));

There is no reason to crash.

[...]
> +	txq->tx_skb = kmalloc(txq->size * sizeof(*txq->tx_skb),
> +			      GFP_KERNEL);

	txq->tx_skb = kmalloc(txq->size * sizeof(*txq->tx_skb), GFP_KERNEL);

[...]
> +static void mvneta_txq_deinit(struct mvneta_port *pp,
> +			      struct mvneta_tx_queue *txq)
> +{
> +	kfree(txq->tx_skb);
> +
> +	if (txq->descs)
> +		dma_free_coherent(pp->dev->dev.parent,
> +				  txq->size * MVNETA_DESC_ALIGNED_SIZE,
> +				  txq->descs,
> +				  txq->descs_phys);

				  txq->descs, txq->descs_phys);

[...]
> +static void mvneta_cleanup_txqs(struct mvneta_port *pp)
> +{
> +	int queue;
> +	for (queue = 0; queue < txq_number; queue++)
> +		mvneta_txq_deinit(pp, &pp->txqs[queue]);

Insert an empty line after local variables declaration.

[...]
> +static void mvneta_cleanup_rxqs(struct mvneta_port *pp)
> +{
> +	int queue;
> +	for (queue = 0; queue < rxq_number; queue++)
> +		mvneta_rxq_deinit(pp, &pp->rxqs[queue]);

Insert an empty line after local variables declaration.

[...]
> +static int mvneta_setup_rxqs(struct mvneta_port *pp)
> +{
> +	int queue;
> +
> +	for (queue = 0; queue < rxq_number; queue++) {
> +		int err = mvneta_rxq_init(pp, &pp->rxqs[queue]);
> +		if (err) {
> +			netdev_err(pp->dev,
> +				   "%s: can't create RxQ rxq=%d\n",

			netdev_err(pp->dev, "%s: can't create RxQ rxq=%d\n",

> +				   __func__, queue);
> +			mvneta_cleanup_rxqs(pp);
> +			return -ENODEV;

mvneta_rxq_init should return a proper error code and it should be
propagated (option: break instead of return and mvneta_setup_rxqs scoped
err variable)

[...]
> +static int mvneta_setup_txqs(struct mvneta_port *pp)
> +{
> +	int queue;
> +
> +	for (queue = 0; queue < txq_number; queue++) {
> +		int err = mvneta_txq_init(pp, &pp->txqs[queue]);
> +		if (err) {
> +			netdev_err(pp->dev,
> +				   "%s: can't create TxQ txq=%d\n",
> +				   __func__, queue);
> +			mvneta_cleanup_txqs(pp);
> +			return err;
> +		}
> +	}

See mvneta_setup_rxqs above.

[...]
> +static void mvneta_start_dev(struct mvneta_port *pp)
> +{
> +	mvneta_max_rx_size_set(pp, pp->pkt_size);
> +	mvneta_txq_max_tx_size_set(pp, pp->pkt_size);
> +
> +	/* start the Rx/Tx activity */
> +	mvneta_port_enable(pp);
> +
> +	/* Enable polling on the port */
> +	napi_enable(&pp->napi);
> +
> +	/* Unmask interrupts */
> +	mvneta_interrupts_unmask(pp);
> +	smp_call_function_many(cpu_online_mask,
> +			       mvneta_interrupts_unmask,
> +			       pp, 1);

	smp_call_function_many(cpu_online_mask, mvneta_interrupts_unmask,

(as done in mvneta_stop_dev)

> +
> +	phy_start(pp->phy_dev);
> +	netif_tx_start_all_queues(pp->dev);
> +}
> +
> +static void mvneta_stop_dev(struct mvneta_port *pp)
> +{
> +	phy_stop(pp->phy_dev);
> +
> +	napi_disable(&pp->napi);
> +
> +	/* Stop upper layer */
> +	netif_carrier_off(pp->dev);

Useless comment.

> +
> +	mvneta_port_down(pp);
> +	netif_tx_stop_all_queues(pp->dev);
> +
> +	/* Stop the port activity */
> +	mvneta_port_disable(pp);
> +
> +	/* Clear all ethernet port interrupts */
> +	mvreg_write(pp, MVNETA_INTR_MISC_CAUSE, 0);
> +	mvreg_write(pp, MVNETA_INTR_OLD_CAUSE, 0);
> +
> +	/* Mask all interrupts */
> +	mvneta_interrupts_mask(pp);

Useless comment.

> +	smp_call_function_many(cpu_online_mask, mvneta_interrupts_mask,
> +			       pp, 1);
> +
> +	/* Reset TX port here. */
> +	mvneta_tx_reset(pp);
> +	mvneta_rx_reset(pp);

Useless comment.

> +}
> +
> +/* tx timeout callback - display a message and stop/start the network device */
> +static void mvneta_tx_timeout(struct net_device *dev)
> +{
> +	struct mvneta_port *pp = netdev_priv(dev);
> +	netdev_info(dev, "tx timeout\n");

Insert an empty line after local variables declaration.

[...]
> +static int mvneta_check_mtu_valid(struct net_device *dev, int mtu)
> +{
> +	if (mtu < 68) {
> +		netdev_err(dev, "cannot change mtu to less than 68\n");
> +		return -EINVAL;
> +	}
> +
> +	if (mtu > 9676 /* 9700 - 20 and rounding to 8 */) {

Please use a different comment style, say on a proper line, or a define with
a comment above it.

[...]
> +static int mvneta_ethtool_set_coalesce(struct net_device *dev,
> +				       struct ethtool_coalesce *c)
> +{
> +	int queue;
> +	struct mvneta_port *pp = netdev_priv(dev);

Long lines first when possible.

[...]
> +static const struct net_device_ops mvneta_netdev_ops = {
> +	.ndo_open = mvneta_open,
> +	.ndo_stop = mvneta_stop,
> +	.ndo_start_xmit = mvneta_tx,
> +	.ndo_set_rx_mode = mvneta_set_rx_mode,
> +	.ndo_set_mac_address = mvneta_set_mac_addr,
> +	.ndo_change_mtu = mvneta_change_mtu,
> +	.ndo_tx_timeout = mvneta_tx_timeout,
> +	.ndo_get_stats64 = mvneta_get_stats64,

Please use tabs before '=' and line things up.

> +};
> +
> +const struct ethtool_ops mvneta_eth_tool_ops = {
> +	.get_link = ethtool_op_get_link,
> +	.get_settings = mvneta_ethtool_get_settings,
> +	.set_settings = mvneta_ethtool_set_settings,
> +	.set_coalesce = mvneta_ethtool_set_coalesce,
> +	.get_coalesce = mvneta_ethtool_get_coalesce,
> +	.get_drvinfo  = mvneta_ethtool_get_drvinfo,
> +	.get_ringparam	= mvneta_ethtool_get_ringparam,
> +	.set_ringparam	= mvneta_ethtool_set_ringparam,

Please use tabs before '=' and line things up.

[...]
> +static int __devinit mvneta_init(struct mvneta_port *pp, int phy_addr)
> +{
> +	int queue, i, ret = 0;
> +
> +	/* Disable port */
> +	mvneta_port_disable(pp);
> +
> +	/* Set port default values */
> +	mvneta_defaults_set(pp);
> +
> +	pp->txqs = kzalloc(txq_number * sizeof(struct mvneta_tx_queue),
> +			   GFP_KERNEL);
> +	if (!pp->txqs) {
> +		netdev_err(pp->dev, "out of memory in allocating tx queue\n");

I doubt a message is really needed when a GFP_KERNEL oom triggers.

(...]
> +static void __devinit mvneta_conf_mbus_windows(struct mvneta_port *pp,
> +				const struct mbus_dram_target_info *dram)
> +{
[...]
> +	for (i = 0; i < dram->num_cs; i++) {
> +		const struct mbus_dram_window *cs = dram->cs + i;
> +		mvreg_write(pp, MVNETA_WIN_BASE(i),
> +			    (cs->base & 0xffff0000) |
> +			    (cs->mbus_attr << 8) |
> +			    dram->mbus_dram_target_id);

		mvreg_write(pp, MVNETA_WIN_BASE(i), (cs->base & 0xffff0000) |
			    (cs->mbus_attr << 8) | dram->mbus_dram_target_id);

[...]
> +static int __devinit mvneta_probe(struct platform_device *pdev)
> +{
> +	int err = -EINVAL;
> +	struct mvneta_port *pp;
> +	struct net_device *dev;
> +	u32 phy_addr, clk_rate_hz;
> +	int phy_mode;
> +	const char *mac_addr;
> +	const struct mbus_dram_target_info *dram_target_info;
> +	struct device_node *dn = pdev->dev.of_node;

Long lines first when possible.

-- 
Ueimor
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists