From: Madalin-Cristian Bucur <madalin.bucur@freescale.com>
To: Joe Perches <joe@perches.com>
CC: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Scott Wood <scottwood@freescale.com>,
        Liberman Igal <Igal.Liberman@freescale.com>,
        "ppc@mindchasers.com" <ppc@mindchasers.com>,
        "pebolle@tiscali.nl" <pebolle@tiscali.nl>,
        "joakim.tjernlund@transmode.se" <joakim.tjernlund@transmode.se>
Subject: RE: [PATCH 02/10] dpaa_eth: add support for DPAA Ethernet
Thread-Topic: [PATCH 02/10] dpaa_eth: add support for DPAA Ethernet
Thread-Index: AQHQxJn/zNtDIZFnwESLfezW31f8OZ3nwQiAgAMC24A=
Date: Fri, 24 Jul 2015 15:45:56 +0000
Message-ID: <BL2PR03MB545D22D02CEEDA05878E833E6810@BL2PR03MB545.namprd03.prod.outlook.com>
References: <1437581806-17420-1-git-send-email-madalin.bucur@freescale.com>
	 <1437581806-17420-2-git-send-email-madalin.bucur@freescale.com>
 <1437586665.20787.24.camel@perches.com>
In-Reply-To: <1437586665.20787.24.camel@perches.com>
Accept-Language: en-US
Content-Language: en-US
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 8BIT
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 24 Jul 2015 15:45:56.0664
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 710a03f5-10f6-4d38-9ff4-a80b81da590d
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL2PR03MB371
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Length: 4135
Lines: 154

> -----Original Message-----
> From: Joe Perches [mailto:joe@perches.com]
> 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:
> 
> > +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.

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

> > +static int __init dpa_load(void)
> > +{
> []
> > +	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

Agree.

> > +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.
> 
> > 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

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

> > +struct dpa_buffer_layout_s {
> > +	u16	priv_data_size;
> > +	bool		parse_results;
> > +	bool		time_stamp;
> > +	bool		hash_results;
> > +	u16	data_align;
> > +};
> 
> > +struct dpa_fq {
> > +	struct qman_fq		 fq_base;
> > +	struct list_head	 list;
> > +	struct net_device	*net_dev;
> 
> some inconsistent indentation here and there

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

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

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

> > +	struct {
> > +		/**
> 
> Maybe the /** style should be avoided

Will fix.

> > +		 * 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)
> > +{
> []
> > +	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.

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

> > +
> > +	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.

Yes, it's more readable.

Thank you,
Madalin

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/