RE: [PATCH 02/10] dpaa_eth: add support for DPAA Ethernet

-----Original Message-----
From: Joe Perches [mailto:joe@perches.com]
On Wed, 2015-07-22 at 19:16 +0300, Madalin Bucur wrote:

quoted

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.

=20
trivia:
=20

quoted

+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)
+{

[]

quoted

+static struct dpa_bp * __cold
+dpa_priv_bp_probe(struct device *dev)

=20
Do the __hot and __cold markings really matter?
Some of them may be questionable.

Some may be, yes. I need to go through all of them.

quoted

+static int __init dpa_load(void)
+{

[]

quoted

+	err =3D platform_driver_register(&dpa_driver);
+	if (unlikely(err < 0)) {
+		pr_err(KBUILD_MODNAME
+			": %s:%hu:%s(): platform_driver_register() =3D %d\n",
+			KBUILD_BASENAME ".c", __LINE__, __func__, err);
+	}
+
+	pr_debug(KBUILD_MODNAME ": %s:%s() ->\n",
+		 KBUILD_BASENAME ".c", __func__);

=20
Perhaps these should use pr_fmt

Agree.

quoted

+static void __exit dpa_unload(void)
+{
+	pr_debug(KBUILD_MODNAME ": -> %s:%s()\n",
+		 KBUILD_BASENAME ".c", __func__);

=20
dynamic debug has __func__ available and perhaps
the function tracer might be used instead.
=20

quoted

diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h

b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.h
[]

quoted

+#define __hot

=20
curious.
=20
Maybe it'd be good to add a real __hot to compiler.h

They're mostly there to make readers aware the code is critical, any
changes could mess performance.

quoted

+struct dpa_buffer_layout_s {
+	u16	priv_data_size;
+	bool		parse_results;
+	bool		time_stamp;
+	bool		hash_results;
+	u16	data_align;
+};

=20

quoted

+struct dpa_fq {
+	struct qman_fq		 fq_base;
+	struct list_head	 list;
+	struct net_device	*net_dev;

=20
some inconsistent indentation here and there

Yes, I've tried to align the style but given the many editors along the tim=
e the code existed=20
there still are areas out of sync.

quoted

+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

quoted

+		 * level
+		 */
+		int target_count;
+		/* The configured value for the number of buffers in the

pool,

quoted

+		 * used for shared port buffer pools
+		 */
+		int config_count;
+	};

=20
Anonymous unions are relatively rare

We liked the direct access to members...
In this particular case the use is a bit excessive, we can do without it.

quoted

+	struct {
+		/**

=20
Maybe the /** style should be avoided

Will fix.

quoted

+		 * All egress queues to a given net device belong to one
+		 * (and the same) congestion group.
+		 */
+		struct qman_cgr cgr;
+	} cgr_data;

=20
[]
=20

quoted

+int dpa_stop(struct net_device *net_dev)
+{

[]

quoted

+	err =3D mac_dev->stop(mac_dev);
+	if (unlikely(err < 0))
+		netif_err(priv, ifdown, net_dev, "mac_dev->stop() =3D %d\n",
+			  err);

=20
Some of the likely/unlikely uses may not
be useful/necessary.

In this particular case it's gratuitous, I'll go through all of them.

quoted

+
+	for_each_port_device(i, mac_dev->port_dev) {
+		error =3D fm_port_disable(
+				fm_port_drv_handle(mac_dev-
port_dev[i]));
+		err =3D error ? error : err;

=20
		if (error)
			err =3D error;
=20
is more obvious to me.

Yes, it's more readable.

Thank you,
Madalin