Thomas Petazzoni [off-list ref] :
[...]
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 [off-list ref] :
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ if (queue < 0 || queue >= txq_number)
+ return NULL;
+ return &pp->txqs[queue];
return (queue < 0 || queue >= txq_number) ? NULL : &pp->txqs[queue];
[...]
quoted hunk ↗ jump to hunk
+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))) {
quoted hunk ↗ jump to hunk
+ 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
[...]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ if (queue < 0 || queue >= rxq_number)
+ return NULL;
+ return &pp->rxqs[queue];
See mvneta_tx_done_policy above.
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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,
[...]
quoted hunk ↗ jump to hunk
+ 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;
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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;
quoted hunk ↗ jump to hunk
+ 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;
+ }
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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);
quoted hunk ↗ jump to hunk
+ 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.
(...]
quoted hunk ↗ jump to hunk
+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);
quoted hunk ↗ jump to hunk
+ 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);
[...]
quoted hunk ↗ jump to hunk
+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);
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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",
quoted hunk ↗ jump to hunk
+ __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)
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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)
quoted hunk ↗ jump to hunk
+
+ 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.
quoted hunk ↗ jump to hunk
+
+ 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.
quoted hunk ↗ jump to hunk
+ 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.
quoted hunk ↗ jump to hunk
+}
+
+/* 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.
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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.
[...]
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+};
+
+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.
[...]
quoted hunk ↗ jump to hunk
+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.
(...]
quoted hunk ↗ jump to hunk
+static void __devinit mvneta_conf_mbus_windows(struct mvneta_port *pp,
+ const struct mbus_dram_target_info *dram)
+{
[...]
quoted hunk ↗ jump to hunk
+ 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);
[...]
quoted hunk ↗ jump to hunk
+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