From: "Steven J. Hill" <steven.h...@cavium.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?