Date:   Tue, 08 May 2018 22:28:24 -0400 (EDT)
From:   David Miller <davem@...emloft.net>
To:     steven.hill@...ium.com
Cc:     netdev@...r.kernel.org, cmunoz@...ium.com, david.daney@...ium.com
Subject: Re: [PATCH net-next v10 3/4] netdev: octeon-ethernet: Add Cavium
 Octeon III support.

From: "Steven J. Hill" <steven.hill@...ium.com>
Date: Mon,  7 May 2018 12:22:10 -0500

> +static atomic_t request_mgmt_once;
> +static atomic_t load_driver_once;
> +static atomic_t pki_id;
 ...
> +		/* One time request driver module */
> +		if (is_mix) {
> +			if (atomic_cmpxchg(&request_mgmt_once, 0, 1) == 0)
> +				request_module_nowait("octeon_mgmt");
> +		}
> +		if (is_pki) {
> +			if (atomic_cmpxchg(&load_driver_once, 0, 1) == 0)
> +				request_module_nowait("octeon3-ethernet");
> +		}

You're going to have to explain this, it makes no sense to me.
 
> +static int bgx_pki_ports_init(void)
> +{
> +	int i;
> +	int j;
> +	int k;

"int i, j, k;" please.

> +static int bgx_port_xgmii_set_link_up(struct bgx_port_priv *priv)
> +{
> +	u64 data;
> +	int timeout;

Please order from longest to shortest line for variable declarations.

> +static int bgx_port_sgmii_set_link_speed(struct bgx_port_priv *priv,
> +					 struct port_status status)
> +{
> +	int timeout;
> +	u64 miscx;
> +	u64 data;
> +	u64 prtx;

Please use "u64 miscx, data, prtx;" and put it on the first line.

> +static struct port_status bgx_port_get_xaui_link(struct bgx_port_priv *priv)
> +{
> +	struct port_status status;
> +	int lanes;
> +	int speed;
> +	u64 data;

"int lanes, speed;"

> +static int bgx_port_gser_27882(struct bgx_port_priv *priv)
> +{
> +	int timeout;
> +	u64 addr;
> +	u64 data;

"u64 addr, data;" and move to first line.

> +static int bgx_port_qlm_rx_equalization(struct bgx_port_priv *priv,
> +					int qlm, int lane)
> +{
> +	int max_lanes = bgx_port_get_max_qlm_lanes(qlm);
> +	int lane_mask;
> +	int timeout;
> +	int rc = 0;
> +	u64 lmode;
> +	u64 addr;
> +	u64 data;
> +	int i;

Please group these local variables.  Have some pity for people who
have not so much vertical space on their screen when they are reading
your code. :)

> +static int bgx_port_init_xaui_link(struct bgx_port_priv *priv)
> +{
> +	int use_training = 0;
> +	int use_ber = 0;
> +	int timeout;
> +	int rc = 0;
> +	u64 data;

Please group the int variables into a smaller number of lines.

> +		/* Wait for mac rx to be ready */
> +		timeout = 10000;
> +		do {
> +			data = oct_csr_read(BGX_SMU_RX_CTL(priv->node, priv->bgx, priv->index));
> +			data &= GENMASK_ULL(1, 0);
> +			if (!data)
> +				break;
> +			timeout--;
> +			udelay(1);
> +		} while (timeout);

This construct is repeated so many times, over and over.  Make a helper
function that performs this operation.

> +static void bgx_port_adjust_link(struct net_device *netdev)
> +{
> +	struct bgx_port_priv *priv = bgx_port_netdev2priv(netdev);
> +	bool link_changed = false;
> +	unsigned int duplex;
> +	unsigned int speed;
> +	unsigned int link;

Please group the unsigned ints.

> +static int bgx_port_probe(struct platform_device *pdev)
> +{
> +	struct bgx_port_priv *priv;
> +	const __be32 *reg;
> +	const u8 *mac;
> +	int numa_node;
> +	u32 index;
> +	u64 addr;
> +	int rc;

Please group variables of the same time into one line.

> +static int __init bgx_port_driver_init(void)
> +{
> +	int r;
> +	int i;
> +	int j;
> +	int k;

"int r, i, j, k;"

> +static inline u64 scratch_read64(u64 offset)

Do not use "inline" for functions in foo.c files, let the compiler
decide.

> +static inline void scratch_write64(u64 offset, u64 value)

Likewise.

> +static inline struct wr_ret octeon3_core_get_work_sync(int grp)
> +{
> +	u64 node = cvmx_get_node_num();
> +	struct wr_ret r;
> +	u64 response;
> +	u64 addr;

"u64 response, addr;"  Don't use inline.

> +static inline void octeon3_core_get_work_async(unsigned int grp)
> +{

Kill the inline.

> +static inline struct wr_ret octeon3_core_get_response_async(void)

Likewise.

> +static int octeon3_eth_tx_complete_hwtstamp(struct octeon3_ethernet *priv,
> +					    struct sk_buff *skb)
> +{
> +	struct skb_shared_hwtstamps shts;
> +	u64 hwts;
> +	u64 ns;

"u64 hwts, ns;"

> +static int octeon3_eth_tx_complete_worker(void *data)
> +{
> +	struct octeon3_ethernet_worker *worker = data;
> +	struct octeon3_ethernet_node *oen;
> +	int tx_complete_stop_thresh;
> +	int backlog_stop_thresh;
> +	int backlog;
> +	u64 aq_cnt;
> +	int order;
> +	int i;

Group the variable declarations, please.

> +static int octeon3_eth_common_ndo_init(struct net_device *netdev,
> +				       int extra_skip)
> +{
> +	struct octeon3_ethernet_node *oen;
> +	int base_rx_grp[MAX_RX_QUEUES];
> +	struct octeon3_ethernet *priv;
> +	int pki_chan;
> +	int aura;
> +	int dq;
> +	int i;
> +	int r;

"int pki_chan, aura, dq, i, r;"

> +static void octeon3_eth_ndo_get_stats64(struct net_device *netdev,
> +					struct rtnl_link_stats64 *s)
> +{
> +	struct octeon3_ethernet *priv = netdev_priv(netdev);
> +	u64 delta_packets;
> +	u64 delta_dropped;
> +	u64 delta_octets;
> +	u64 dropped;
> +	u64 packets;
> +	u64 octets;

Group the u64s please.

> +static int octeon3_eth_common_ndo_open(struct net_device *netdev)
> +{
> +	struct octeon3_ethernet *priv = netdev_priv(netdev);
> +	struct octeon3_rx *rx;
> +	int i;
> +	int r;

"int i, r;"

> +static inline u64 build_pko_send_ext_desc(struct sk_buff *skb)

Kill the inline.

> +static inline u64 build_pko_send_tso(struct sk_buff *skb, uint mtu)

Likewise.

> +static inline u64 build_pko_send_mem_sub(u64 addr)

Likewise.

> +static inline u64 build_pko_send_mem_ts(u64 addr)

Likewise.

> +static inline u64 build_pko_send_free(u64 addr)

Likewise.

> +static inline u64 build_pko_send_work(int grp, u64 addr)

Likewise.

> +static int octeon3_eth_ndo_start_xmit(struct sk_buff *skb,
> +				      struct net_device *netdev)
> +{
> +	struct octeon3_ethernet *priv = netdev_priv(netdev);
> +	struct octeon3_ethernet_node *oen;
> +	u64 scr_off = LMTDMA_SCR_OFFSET;
> +	struct sk_buff *skb_tmp;
> +	u64 pko_send_desc;
> +	u64 *lmtdma_addr;
> +	unsigned int mss;
> +	u64 lmtdma_data;
> +	u64 aq_cnt = 0;
> +	int frag_count;
> +	long backlog;
> +	u64 head_len;
> +	void **work;
> +	int grp;
> +	int i;

Please group these variables, this is crazy...

> +static int octeon3_adjfreq(struct ptp_clock_info *ptp, s32 ppb)
> +{
> +	struct octeon3_ethernet	*priv;
> +	int neg_ppb = 0;
> +	u64 comp;
> +	u64 diff;

"u64 comp, diff;"
> +int octeon_fpa3_init(int node)
> +{
> +	static bool init_done[2];
> +	int aura_cnt;
> +	u64 data;
> +	int i;

"int aura_cnt, i; "

> +int octeon_fpa3_aura_init(int node, int pool, int aura_num,
> +			  int *aura, int num_bufs, unsigned int limit)
> +{
> +	struct global_resource_tag tag;
> +	unsigned int drop;
> +	unsigned int pass;
> +	char buf[16];
> +	int rc = 0;
> +	u64 shift;
> +	u64 data;

"unsigned int drop, pass;"
"u64 shift, data;"

> +void *octeon_fpa3_alloc(int node, int aura)
> +{
> +	void *buf = NULL;
> +	u64 buf_phys;
> +	u64 addr;

"u64 buf_phys, addr;"

> +void octeon_fpa3_free(int node, int aura, const void *buf)
> +{
> +	u64 buf_phys;
> +	u64 addr;

"u64 buf_phys, addr;"

> +int octeon_fpa3_mem_fill(int node, struct kmem_cache *cache,
> +			 int aura, int num_bufs)
> +{
> +	void *mem;
> +	int rc = 0;
> +	int i;

"int i, rc = 0;"

> +static int octeon3_pki_pcam_alloc_entry(int node, int entry, int bank)
> +{
> +	struct global_resource_tag tag;
> +	char buf[16];
> +	int num_clusters;
> +	int rc;
> +	int i;

"int num_clusters, rc, i;"

> +static int octeon3_pki_pcam_write_entry(int node,
> +					struct pcam_term_info *term_info)
> +{
> +	int num_clusters;
> +	u64 action;
> +	int entry;
> +	u64 match;
> +	int bank;
> +	u64 term;
> +	int i;

"u64 action, match, term;"
"int num_clusters, entry, bank, i;"

> +int octeon3_pki_set_ptp_skip(int node, int pknd, int skip)
> +{
> +	int num_clusters;
> +	u64 data;
> +	u64 i;

"u64 data, i;"

That's all I have the stomache for at the moment.

This thing is really large, making it nearly impossible to review
as one huge patch #3. Perhaps you can find a way to split it up
logically somehow?

Powered by blists - more mailing lists