Re: [PATCH 02/10] dpaa_eth: add support for DPAA Ethernet
flat view
From: Joe Perches <joe@perches.com>
Date: 2015-07-22 17:37:50
Also in:
linuxppc-dev, lkml
On Wed, 2015-07-22 at 19:16 +0300, Madalin Bucur wrote:
This introduces the Freescale Data Path Acceleration Architecture (DPAA) Ethernet driver (dpaa_eth) that builds upon the DPAA QMan, BMan, PAMU and FMan drivers to deliver Ethernet connectivity on the Freescale DPAA QorIQ platforms.
trivia:
quoted hunk ↗ jump to hunk
+static void __hot _dpa_tx_conf(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) +{
[]
+static struct dpa_bp * __cold +dpa_priv_bp_probe(struct device *dev)
Do the __hot and __cold markings really matter? Some of them may be questionable.
+static int __init dpa_load(void)
+{[]
quoted hunk ↗ jump to hunk
+ err = platform_driver_register(&dpa_driver); + if (unlikely(err < 0)) { + pr_err(KBUILD_MODNAME + ": %s:%hu:%s(): platform_driver_register() = %d\n", + KBUILD_BASENAME ".c", __LINE__, __func__, err); + } + + pr_debug(KBUILD_MODNAME ": %s:%s() ->\n", + KBUILD_BASENAME ".c", __func__);
Perhaps these should use pr_fmt
quoted hunk ↗ jump to hunk
+static void __exit dpa_unload(void) +{ + pr_debug(KBUILD_MODNAME ": -> %s:%s()\n", + KBUILD_BASENAME ".c", __func__);
dynamic debug has __func__ available and perhaps the function tracer might be used instead.
quoted hunk ↗ jump to hunk
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
[]
+#define __hot
curious. Maybe it'd be good to add a real __hot to compiler.h
quoted hunk ↗ jump to hunk
+struct dpa_buffer_layout_s { + u16 priv_data_size; + bool parse_results; + bool time_stamp; + bool hash_results; + u16 data_align; +};
quoted hunk ↗ jump to hunk
+struct dpa_fq { + struct qman_fq fq_base; + struct list_head list; + struct net_device *net_dev;
some inconsistent indentation here and there
quoted hunk ↗ jump to hunk
+struct dpa_bp { + struct bman_pool *pool; + u8 bpid; + struct device *dev; + union { + /* The buffer pools used for the private ports are initialized + * with target_count buffers for each CPU; at runtime the + * number of buffers per CPU is constantly brought back to this + * level + */ + int target_count; + /* The configured value for the number of buffers in the pool, + * used for shared port buffer pools + */ + int config_count; + };
Anonymous unions are relatively rare
+ struct {
+ /**Maybe the /** style should be avoided
quoted hunk ↗ jump to hunk
+ * All egress queues to a given net device belong to one + * (and the same) congestion group. + */ + struct qman_cgr cgr; + } cgr_data;
[]
+int dpa_stop(struct net_device *net_dev)
+{[]
quoted hunk ↗ jump to hunk
+ err = mac_dev->stop(mac_dev); + if (unlikely(err < 0)) + netif_err(priv, ifdown, net_dev, "mac_dev->stop() = %d\n", + err);
Some of the likely/unlikely uses may not be useful/necessary.
quoted hunk ↗ jump to hunk
+ + for_each_port_device(i, mac_dev->port_dev) { + error = fm_port_disable( + fm_port_drv_handle(mac_dev->port_dev[i])); + err = error ? error : err;
if (error) err = error; is more obvious to me.