Re: [net-next v4 2/8] dpaa_eth: add support for DPAA Ethernet
flat view
From: Scott Wood <hidden>
Date: 2015-11-11 03:36:08
Also in:
lkml, netdev
On Mon, Nov 02, 2015 at 07:31:34PM +0200, Madalin Bucur wrote:
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/freescale/dpaa/Makefile b/drivers/net/ethernet/freescale/dpaa/Makefile new file mode 100644 index 0000000..3847ec7 --- /dev/null +++ b/drivers/net/ethernet/freescale/dpaa/Makefile@@ -0,0 +1,11 @@ +# +# Makefile for the Freescale DPAA Ethernet controllers +# + +# Include FMan headers +FMAN = $(srctree)/drivers/net/ethernet/freescale/fman +ccflags-y += -I$(FMAN)
...or just put the two parts of the same driver into the same directory.
+#define DPA_DESCRIPTION "FSL DPAA Ethernet driver"
Avoiding duplicating those four words is not worth the obfuscation of moving it somewhere else.
+static u8 debug = -1;
-1 does not fit in a u8. Usually this is declared as int.
+module_param(debug, byte, S_IRUGO); +MODULE_PARM_DESC(debug, "Module/Driver verbosity level");
It would be good to document the range of values here.
quoted hunk ↗ jump to hunk
+ +/* This has to work in tandem with the DPA_CS_THRESHOLD_xxx values. */ +static u16 tx_timeout = 1000; +module_param(tx_timeout, ushort, S_IRUGO); +MODULE_PARM_DESC(tx_timeout, "The Tx timeout in ms");
Could you elaborate on the relationship with DPA_CS_THRESHOLD_xxx? Or even tell me where I can find such a symbol?
+ +/* BM */
BM?
+#define DPAA_ETH_MAX_PAD (L1_CACHE_BYTES * 8)
This isn't used.
quoted hunk ↗ jump to hunk
+static u8 dpa_priv_common_bpid; + +static void _dpa_rx_error(struct net_device *net_dev, + const struct dpa_priv_s *priv, + struct dpa_percpu_priv_s *percpu_priv, + const struct qm_fd *fd, + u32 fqid)
Why the leading underscore? Likewise elsewhere.
quoted hunk ↗ jump to hunk
+{ + /* limit common, possibly innocuous Rx FIFO Overflow errors' + * interference with zero-loss convergence benchmark results. + */
Spamming the console is bad regardless of whether you're running a certain benchmark...
quoted hunk ↗ jump to hunk
+ if (likely(fd->status & FM_FD_ERR_PHYSICAL)) + pr_warn_once("non-zero error counters in fman statistics (sysfs)\n"); + else + if (net_ratelimit()) + netif_err(priv, hw, net_dev, "Err FD status = 0x%08x\n", + fd->status & FM_FD_STAT_RX_ERRORS);
Why are you using likely in error paths? Why do we need to print this error at all? Do other net drivers do that?
quoted hunk ↗ jump to hunk
+ percpu_priv->stats.rx_errors++; + + dpa_fd_release(net_dev, fd);
Can we reserve "fd" for file descriptors and call this something else?
quoted hunk ↗ jump to hunk
+} + +static void _dpa_tx_error(struct net_device *net_dev, + const struct dpa_priv_s *priv, + struct dpa_percpu_priv_s *percpu_priv, + const struct qm_fd *fd, + u32 fqid) +{ + struct sk_buff *skb; + + if (net_ratelimit()) + netif_warn(priv, hw, net_dev, "FD status = 0x%08x\n", + fd->status & FM_FD_STAT_TX_ERRORS); + + percpu_priv->stats.tx_errors++; + + /* If we intended the buffers from this frame to go into the bpools + * when the FMan transmit was done, we need to put it in manually. + */ + if (fd->bpid != 0xff) { + dpa_fd_release(net_dev, fd); + return; + }
Define a symbolic constant for this special 0xff value.
quoted hunk ↗ jump to hunk
+static enum qman_cb_dqrr_result +priv_rx_error_dqrr(struct qman_portal *portal, + struct qman_fq *fq, + const struct qm_dqrr_entry *dq) +{ + struct net_device *net_dev; + struct dpa_priv_s *priv; + struct dpa_percpu_priv_s *percpu_priv; + int *count_ptr; + + net_dev = ((struct dpa_fq *)fq)->net_dev;
Don't open-code the casting of one struct to another like this. Use container_of().
quoted hunk ↗ jump to hunk
+static enum qman_cb_dqrr_result +priv_rx_default_dqrr(struct qman_portal *portal, + struct qman_fq *fq, + const struct qm_dqrr_entry *dq) +{ + struct net_device *net_dev; + struct dpa_priv_s *priv; + struct dpa_percpu_priv_s *percpu_priv; + int *count_ptr; + struct dpa_bp *dpa_bp; + + net_dev = ((struct dpa_fq *)fq)->net_dev; + priv = netdev_priv(net_dev); + dpa_bp = priv->dpa_bp; + + /* IRQ handler, non-migratable; safe to use raw_cpu_ptr here */ + percpu_priv = raw_cpu_ptr(priv->percpu_priv); + count_ptr = raw_cpu_ptr(dpa_bp->percpu_count);
But why do you *need* to use raw?
quoted hunk ↗ jump to hunk
+ + if (unlikely(dpaa_eth_napi_schedule(percpu_priv, portal))) + return qman_cb_dqrr_stop;
If this is always an "IRQ handler" then why is it "unlikely" that this will return non-zero?
quoted hunk ↗ jump to hunk
+static const struct dpa_fq_cbs_t private_fq_cbs = { + .rx_defq = { .cb = { .dqrr = priv_rx_default_dqrr } }, + .tx_defq = { .cb = { .dqrr = priv_tx_conf_default_dqrr } }, + .rx_errq = { .cb = { .dqrr = priv_rx_error_dqrr } }, + .tx_errq = { .cb = { .dqrr = priv_tx_conf_error_dqrr } }, + .egress_ern = { .cb = { .ern = priv_ern } } +}; + +static void dpaa_eth_napi_enable(struct dpa_priv_s *priv) +{ + struct dpa_percpu_priv_s *percpu_priv; + int i, j; + + for_each_possible_cpu(i) { + percpu_priv = per_cpu_ptr(priv->percpu_priv, i); + + for (j = 0; j < qman_portal_max; j++) { + percpu_priv->np[j].down = 0; + napi_enable(&percpu_priv->np[j].napi); + } + } +} + +static void dpaa_eth_napi_disable(struct dpa_priv_s *priv) +{ + struct dpa_percpu_priv_s *percpu_priv; + int i, j; + + for_each_possible_cpu(i) { + percpu_priv = per_cpu_ptr(priv->percpu_priv, i); + + for (j = 0; j < qman_portal_max; j++) { + percpu_priv->np[j].down = 1; + napi_disable(&percpu_priv->np[j].napi); + } + } +}
Why ss there a NAPI instance for every combination of portal and cpu, even though a portal is normally bound to a single cpu?
quoted hunk ↗ jump to hunk
+static int +dpaa_eth_priv_probe(struct platform_device *pdev) +{
Why "priv"? That makes sense when talking about the data structure, not in function names. Also no need for newline after "static int".
quoted hunk ↗ jump to hunk
+ int err = 0, i, channel; + struct device *dev; + struct dpa_bp *dpa_bp; + struct dpa_fq *dpa_fq, *tmp; + size_t count = 1; + struct net_device *net_dev = NULL; + struct dpa_priv_s *priv = NULL; + struct dpa_percpu_priv_s *percpu_priv;
What is this "_s"? If it means "_struct", drop it.
quoted hunk ↗ jump to hunk
+ struct fm_port_fqs port_fqs; + struct dpa_buffer_layout_s *buf_layout = NULL; + struct mac_device *mac_dev; + struct task_struct *kth; + + dev = &pdev->dev; + + /* Get the buffer pool assigned to this interface; + * run only once the default pool probing code + */ + dpa_bp = (dpa_bpid2pool(dpa_priv_common_bpid)) ? : + dpa_priv_bp_probe(dev);
This is an awkward/non-idiomatic way of saying:
dpa_bp = dpa_bpid2pool(dpa_priv_common_bpid);
if (!dpa_bp)
dpa_bp = dpa_priv_bp_probe(dev);
quoted hunk ↗ jump to hunk
+ if (IS_ERR(dpa_bp)) + return PTR_ERR(dpa_bp); + + /* Allocate this early, so we can store relevant information in + * the private area + */ + net_dev = alloc_etherdev_mq(sizeof(*priv), DPAA_ETH_TX_QUEUES); + if (!net_dev) { + dev_err(dev, "alloc_etherdev_mq() failed\n"); + goto alloc_etherdev_mq_failed; + } + +#ifdef CONFIG_FSL_DPAA_ETH_FRIENDLY_IF_NAME + snprintf(net_dev->name, IFNAMSIZ, "fm%d-mac%d", + dpa_mac_fman_index_get(pdev), + dpa_mac_hw_index_get(pdev)); +#endif
Is this sort of in-kernel renaming considered acceptable? Why is it a compile time option?
quoted hunk ↗ jump to hunk
+ + /* Do this here, so we can be verbose early */ + SET_NETDEV_DEV(net_dev, dev); + dev_set_drvdata(dev, net_dev); + + priv = netdev_priv(net_dev); + priv->net_dev = net_dev; + + priv->msg_enable = netif_msg_init(debug, -1);
This is the only driver that passes -1 as the second argument of netif_msg_init(). Why?
quoted hunk ↗ jump to hunk
+ + mac_dev = dpa_mac_dev_get(pdev); + if (IS_ERR(mac_dev) || !mac_dev) { + err = PTR_ERR(mac_dev); + goto mac_probe_failed; + }
When can dpa_mac_dev_get() return NULL rather than an error value?
quoted hunk ↗ jump to hunk
+ + /* We have physical ports, so we need to establish + * the buffer layout. + */ + buf_layout = devm_kzalloc(dev, 2 * sizeof(*buf_layout), + GFP_KERNEL); + if (!buf_layout) + goto alloc_failed;
Why not just make buf_layout a static array in the priv struct?
quoted hunk ↗ jump to hunk
+ + dpa_set_buffers_layout(mac_dev, buf_layout); + + /* For private ports, need to compute the size of the default + * buffer pool, based on FMan port buffer layout;also update + * the maximum buffer size for private ports if necessary + */ + dpa_bp->size = dpa_bp_size(&buf_layout[RX]);
What is a "private port", what is the opposite of a private port, and what does this code do differently for one versus the other?
quoted hunk ↗ jump to hunk
+static int __init dpa_load(void) +{ + int err; + + pr_info(DPA_DESCRIPTION "\n"); +
Don't spam the console just because a driver has been loaded. Wait until a device has actually been found.
quoted hunk ↗ jump to hunk
+ /* initialise dpaa_eth mirror values */ + dpa_rx_extra_headroom = fman_get_rx_extra_headroom(); + dpa_max_frm = fman_get_max_frm(); + + err = platform_driver_register(&dpa_driver); + if (err < 0) + pr_err("Error, platform_driver_register() = %d\n", err);
A user seeing this would wonder what driver's registration failed. It's a good habit to use __func__ on all output, especially if it's using pr_xxx rather than dev_xxx.
quoted hunk ↗ jump to hunk
+MODULE_LICENSE("Dual BSD/GPL"); +MODULE_AUTHOR("Andy Fleming <afleming@freescale.com>"); +MODULE_DESCRIPTION(DPA_DESCRIPTION);
Andy hasn't touched this code in years, and lots of others have worked on it. I'm not sure that citing any individual person here makes sense.
+#define dpa_get_rx_extra_headroom() dpa_rx_extra_headroom +#define dpa_get_max_frm() dpa_max_frm
Why?
+#define DPA_ERR_ON(cond)
So all the DPA_ERR_ON() users are dead code?
+/* This is what FMan is ever allowed to use.
-EPARSE
quoted hunk ↗ jump to hunk
+ * FMan-DMA requires 16-byte alignment for Rx buffers, but SKB_DATA_ALIGN is + * even stronger (SMP_CACHE_BYTES-aligned), so we just get away with that, + * via SKB_WITH_OVERHEAD(). We can't rely on netdev_alloc_frag() giving us + * half-page-aligned buffers (can we?), so we reserve some more space + * for start-of-buffer alignment. + */ +#define dpa_bp_size(buffer_layout) (SKB_WITH_OVERHEAD(DPA_BP_RAW_SIZE) - \ + SMP_CACHE_BYTES)
The buffer_layout parameter is not used.
+/* number of Tx queues to FMan */ +#define DPAA_ETH_TX_QUEUES NR_CPUS
It would be better to use nr_cpu_ids in the places where dynamic allocations are possible (and consider reworking the places that use static arrays to be dynamic).
quoted hunk ↗ jump to hunk
+static inline int dpaa_eth_napi_schedule(struct dpa_percpu_priv_s *percpu_priv, + struct qman_portal *portal) +{ + /* In case of threaded ISR for RT enable kernel, + * in_irq() does not return appropriate value, so use + * in_serving_softirq to distinguish softirq or irq context. + */ + if (unlikely(in_irq() || !in_serving_softirq())) { + /* Disable QMan IRQ and invoke NAPI */ + int ret = qman_p_irqsource_remove(portal, QM_PIRQ_DQRI); + + if (likely(!ret)) { + const struct qman_portal_config *pc = + qman_p_get_portal_config(portal); + struct dpa_napi_portal *np = + &percpu_priv->np[pc->channel]; + + np->p = portal; + napi_schedule(&np->napi); + return 1; + } + } + return 0; +}
If nearly the entire function is "unlikely", why is anything other than the if-statement inlined? s/for RT enable kernel//
quoted hunk ↗ jump to hunk
+/* Use the queue selected by XPS */ +#define dpa_get_queue_mapping(skb) \ + skb_get_queue_mapping(skb)
Why is this wrapper needed?
quoted hunk ↗ jump to hunk
+int dpa_remove(struct platform_device *pdev) +{ + int err; + struct device *dev; + struct net_device *net_dev; + struct dpa_priv_s *priv; + + dev = &pdev->dev; + net_dev = dev_get_drvdata(dev); + + priv = netdev_priv(net_dev); + + dev_set_drvdata(dev, NULL); + unregister_netdev(net_dev); + + err = dpa_fq_free(dev, &priv->dpa_fq_list); + + qman_delete_cgr_safe(&priv->ingress_cgr); + qman_release_cgrid(priv->ingress_cgr.cgrid); + qman_delete_cgr_safe(&priv->cgr_data.cgr); + qman_release_cgrid(priv->cgr_data.cgr.cgrid); + + dpa_private_napi_del(net_dev); + + dpa_bp_free(priv); + + if (priv->buf_layout) + devm_kfree(dev, priv->buf_layout); + + free_netdev(net_dev); + + return err; +}
You don't need to check for NULL before calling kfree(), and you don't need to call devm_kfree() in a .remove() function.
quoted hunk ↗ jump to hunk
+void dpa_set_buffers_layout(struct mac_device *mac_dev, + struct dpa_buffer_layout_s *layout) +{ + /* Rx */ + layout[RX].priv_data_size = (u16)DPA_RX_PRIV_DATA_SIZE;
Unnecessary cast.
quoted hunk ↗ jump to hunk
+ layout[RX].parse_results = true; + layout[RX].hash_results = true; + layout[RX].data_align = DPA_FD_DATA_ALIGNMENT; + + /* Tx */ + layout[TX].priv_data_size = DPA_TX_PRIV_DATA_SIZE;
...and not even consistent.
quoted hunk ↗ jump to hunk
+ layout[TX].parse_results = true; + layout[TX].hash_results = true; + layout[TX].data_align = DPA_FD_DATA_ALIGNMENT; +}
When are parse_results/hash_results ever false? If the answer is in a future patch, why is the mechanism being introduced in this patch, resulting both in this patch being cluttered, and the functionality introduced in the future patch being non-self-contained and thus harder to review?
quoted hunk ↗ jump to hunk
+int dpa_fq_probe_mac(struct device *dev, struct list_head *list, + struct fm_port_fqs *port_fqs, + bool alloc_tx_conf_fqs, + enum port_type ptype) +{ + const struct fqid_cell *fqids; + struct dpa_fq *dpa_fq; + int num_ranges; + int i; + + if (ptype == TX && alloc_tx_conf_fqs) { + if (!dpa_fq_alloc(dev, tx_confirm_fqids, list, + FQ_TYPE_TX_CONF_MQ)) + goto fq_alloc_failed; + } + + fqids = default_fqids[ptype]; + num_ranges = 3; + + for (i = 0; i < num_ranges; i++) { + switch (i) { + case 0: + /* The first queue is the error queue */ + if (fqids[i].count != 1) + goto invalid_error_queue; + + dpa_fq = dpa_fq_alloc(dev, &fqids[i], list, + ptype == RX ? + FQ_TYPE_RX_ERROR : + FQ_TYPE_TX_ERROR); + if (!dpa_fq) + goto fq_alloc_failed; + + if (ptype == RX) + port_fqs->rx_errq = &dpa_fq[0]; + else + port_fqs->tx_errq = &dpa_fq[0]; + break; + case 1: + /* the second queue is the default queue */ + if (fqids[i].count != 1) + goto invalid_default_queue; + + dpa_fq = dpa_fq_alloc(dev, &fqids[i], list, + ptype == RX ? + FQ_TYPE_RX_DEFAULT : + FQ_TYPE_TX_CONFIRM); + if (!dpa_fq) + goto fq_alloc_failed; + + if (ptype == RX) + port_fqs->rx_defq = &dpa_fq[0]; + else + port_fqs->tx_defq = &dpa_fq[0]; + break; + default: + /* all subsequent queues are Tx */ + if (!dpa_fq_alloc(dev, &fqids[i], list, FQ_TYPE_TX)) + goto fq_alloc_failed; + break;
num_ranges can only be three, so there's only one "subsequent queue" and this is overly complicated. Even if eventually num_queues might be greater than 3, there's no reason to shove the first two queues into a loop only to use a switch statement to execute completely separate code for them.
quoted hunk ↗ jump to hunk
+ /* Fill in the relevant L4 parse result fields */ + switch (l4_proto) { + case IPPROTO_UDP: + parse_result->l4r = FM_L4_PARSE_RESULT_UDP; + break; + case IPPROTO_TCP: + parse_result->l4r = FM_L4_PARSE_RESULT_TCP; + break; + default: + /* This can as well be a BUG() */
No, it can't. I'm guessing the assumption is that for other protocols, skb->ip_summed will != CHECKSUM_PARTIAL, but this should at worst be a rate-limited WARN. BUG() is for situations where there's no reasonable path to error out.
quoted hunk ↗ jump to hunk
+/* Convenience macros for storing/retrieving the skb back-pointers. + * + * NB: @off is an offset from a (struct sk_buff **) pointer! + */ +#define DPA_WRITE_SKB_PTR(skb, skbh, addr, off) \ + { \ + skbh = (struct sk_buff **)addr; \ + *(skbh + (off)) = skb; \ + } +#define DPA_READ_SKB_PTR(skb, skbh, addr, off) \ + { \ + skbh = (struct sk_buff **)addr; \ + skb = *(skbh + (off)); \ + } +
Why can these not be functions?
quoted hunk ↗ jump to hunk
+release_bufs: + /* Release the buffers. In case bman is busy, keep trying + * until successful. bman_release() is guaranteed to succeed + * in a reasonable amount of time + */ + while (unlikely(bman_release(dpa_bp->pool, bmb, i, 0))) + cpu_relax();
What is a "reasonable amount of time" and what happens if this guarantee fails? Usually we have timeouts on such waiting-on-hardware loops so that a device failure doesn't turn into a kernel failure.
+ /* The only FD type that we may receive is contig */ + DPA_ERR_ON((fd->format != qm_fd_contig));
Unnecessary parens.
quoted hunk ↗ jump to hunk
+_release_frame: + dpa_fd_release(net_dev, fd); +}
No leading underscores on goto labels.
quoted hunk ↗ jump to hunk
+ +static int skb_to_contig_fd(struct dpa_priv_s *priv, + struct sk_buff *skb, struct qm_fd *fd, + int *count_ptr, int *offset) +{ + struct sk_buff **skbh; + dma_addr_t addr; + struct dpa_bp *dpa_bp = priv->dpa_bp; + struct net_device *net_dev = priv->net_dev; + int err; + enum dma_data_direction dma_dir; + unsigned char *buffer_start; + + { + /* We are guaranteed to have at least tx_headroom bytes + * available, so just use that for offset. + */ + fd->bpid = 0xff; + buffer_start = skb->data - priv->tx_headroom; + fd->offset = priv->tx_headroom; + dma_dir = DMA_TO_DEVICE; + + DPA_WRITE_SKB_PTR(skb, skbh, buffer_start, 0); + }
Why is this block of code inside braces?
quoted hunk ↗ jump to hunk
+ + /* Enable L3/L4 hardware checksum computation. + * + * We must do this before dma_map_single(DMA_TO_DEVICE), because we may + * need to write into the skb. + */ + err = dpa_enable_tx_csum(priv, skb, fd, + ((char *)skbh) + DPA_TX_PRIV_DATA_SIZE); + if (unlikely(err < 0)) { + if (net_ratelimit()) + netif_err(priv, tx_err, net_dev, "HW csum error: %d\n", + err); + return err; + } + + /* Fill in the rest of the FD fields */ + fd->format = qm_fd_contig; + fd->length20 = skb->len; + fd->cmd |= FM_FD_CMD_FCO; + + /* Map the entire buffer size that may be seen by FMan, but no more */ + addr = dma_map_single(dpa_bp->dev, skbh, + skb_tail_pointer(skb) - buffer_start, dma_dir); + if (unlikely(dma_mapping_error(dpa_bp->dev, addr))) { + if (net_ratelimit()) + netif_err(priv, tx_err, net_dev, "dma_map_single() failed\n"); + return -EINVAL; + } + fd->addr_hi = (u8)upper_32_bits(addr); + fd->addr_lo = lower_32_bits(addr);
Why is addr_hi limited to 8 bits? E.g. t4240 has 40-bit physical addresses... -Scott