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

[Joe Perches <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.

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

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

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

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

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