| Message ID | 20210119150802.19997-9-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 7d9fe90036f75a766dce76df997d4067e22b93c6 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ucc_geth improvements | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: linuxppc-dev@lists.ozlabs.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 72 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit : > These fields are only used within ucc_geth_startup(), so they might as > well be local variables in that function rather than being stashed in > struct ucc_geth_private. > > Aside from making that struct a tiny bit smaller, it also shortens > some lines (getting rid of pointless casts while here), and fixes the > problems with using IS_ERR_VALUE() on a u32 as explained in commit > 800cd6fb76f0 ("soc: fsl: qe: change return type of cpm_muram_alloc() > to s32"). > > Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> > --- > drivers/net/ethernet/freescale/ucc_geth.c | 21 +++++++++------------ > drivers/net/ethernet/freescale/ucc_geth.h | 2 -- > 2 files changed, 9 insertions(+), 14 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c > index 74ee2ed2fbbb..75466489bf9a 100644 > --- a/drivers/net/ethernet/freescale/ucc_geth.c > +++ b/drivers/net/ethernet/freescale/ucc_geth.c > @@ -2351,6 +2351,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) > u8 function_code = 0; > u8 __iomem *endOfRing; > u8 numThreadsRxNumerical, numThreadsTxNumerical; > + s32 rx_glbl_pram_offset, tx_glbl_pram_offset; That's still a quite long name for a local variable. Kernel Codying Style says: LOCAL variable names should be short, and to the point. If you have some random integer loop counter, it should probably be called i. Calling it loop_counter is non-productive, if there is no chance of it being mis-understood. Similarly, tmp can be just about any type of variable that is used to hold a temporary value. If you are afraid to mix up your local variable names, you have another problem, which is called the function-growth-hormone-imbalance syndrome. See chapter 6 (Functions). > > ugeth_vdbg("%s: IN", __func__); > uccf = ugeth->uccf; > @@ -2495,17 +2496,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) > */ > /* Tx global PRAM */ > /* Allocate global tx parameter RAM page */ > - ugeth->tx_glbl_pram_offset = > + tx_glbl_pram_offset = > qe_muram_alloc(sizeof(struct ucc_geth_tx_global_pram), > UCC_GETH_TX_GLOBAL_PRAM_ALIGNMENT); > - if (IS_ERR_VALUE(ugeth->tx_glbl_pram_offset)) { > + if (tx_glbl_pram_offset < 0) { > if (netif_msg_ifup(ugeth)) > pr_err("Can not allocate DPRAM memory for p_tx_glbl_pram\n"); > return -ENOMEM; > } > - ugeth->p_tx_glbl_pram = > - (struct ucc_geth_tx_global_pram __iomem *) qe_muram_addr(ugeth-> > - tx_glbl_pram_offset); > + ugeth->p_tx_glbl_pram = qe_muram_addr(tx_glbl_pram_offset); > /* Fill global PRAM */ > > /* TQPTR */ > @@ -2656,17 +2655,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) > > /* Rx global PRAM */ > /* Allocate global rx parameter RAM page */ > - ugeth->rx_glbl_pram_offset = > + rx_glbl_pram_offset = > qe_muram_alloc(sizeof(struct ucc_geth_rx_global_pram), > UCC_GETH_RX_GLOBAL_PRAM_ALIGNMENT); > - if (IS_ERR_VALUE(ugeth->rx_glbl_pram_offset)) { > + if (rx_glbl_pram_offset < 0) { > if (netif_msg_ifup(ugeth)) > pr_err("Can not allocate DPRAM memory for p_rx_glbl_pram\n"); > return -ENOMEM; > } > - ugeth->p_rx_glbl_pram = > - (struct ucc_geth_rx_global_pram __iomem *) qe_muram_addr(ugeth-> > - rx_glbl_pram_offset); > + ugeth->p_rx_glbl_pram = qe_muram_addr(rx_glbl_pram_offset); > /* Fill global PRAM */ > > /* RQPTR */ > @@ -2928,7 +2925,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) > ((u32) ug_info->numThreadsTx) << ENET_INIT_PARAM_TGF_SHIFT; > > ugeth->p_init_enet_param_shadow->rgftgfrxglobal |= > - ugeth->rx_glbl_pram_offset | ug_info->riscRx; > + rx_glbl_pram_offset | ug_info->riscRx; > if ((ug_info->largestexternallookupkeysize != > QE_FLTR_LARGEST_EXTERNAL_TABLE_LOOKUP_KEY_SIZE_NONE) && > (ug_info->largestexternallookupkeysize != > @@ -2966,7 +2963,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) > } > > ugeth->p_init_enet_param_shadow->txglobal = > - ugeth->tx_glbl_pram_offset | ug_info->riscTx; > + tx_glbl_pram_offset | ug_info->riscTx; > if ((ret_val = > fill_init_enet_entries(ugeth, > &(ugeth->p_init_enet_param_shadow-> > diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h > index c80bed2c995c..be47fa8ced15 100644 > --- a/drivers/net/ethernet/freescale/ucc_geth.h > +++ b/drivers/net/ethernet/freescale/ucc_geth.h > @@ -1166,9 +1166,7 @@ struct ucc_geth_private { > struct ucc_geth_exf_global_pram __iomem *p_exf_glbl_param; > u32 exf_glbl_param_offset; > struct ucc_geth_rx_global_pram __iomem *p_rx_glbl_pram; > - u32 rx_glbl_pram_offset; > struct ucc_geth_tx_global_pram __iomem *p_tx_glbl_pram; > - u32 tx_glbl_pram_offset; > struct ucc_geth_send_queue_mem_region __iomem *p_send_q_mem_reg; > u32 send_q_mem_reg_offset; > struct ucc_geth_thread_data_tx __iomem *p_thread_data_tx; >
On 20/01/2021 07.57, Christophe Leroy wrote: > > > Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit : >> These fields are only used within ucc_geth_startup(), so they might as >> well be local variables in that function rather than being stashed in >> struct ucc_geth_private. >> >> Aside from making that struct a tiny bit smaller, it also shortens >> some lines (getting rid of pointless casts while here), and fixes the >> problems with using IS_ERR_VALUE() on a u32 as explained in commit >> 800cd6fb76f0 ("soc: fsl: qe: change return type of cpm_muram_alloc() >> to s32"). >> >> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> >> --- >> drivers/net/ethernet/freescale/ucc_geth.c | 21 +++++++++------------ >> drivers/net/ethernet/freescale/ucc_geth.h | 2 -- >> 2 files changed, 9 insertions(+), 14 deletions(-) >> >> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c >> b/drivers/net/ethernet/freescale/ucc_geth.c >> index 74ee2ed2fbbb..75466489bf9a 100644 >> --- a/drivers/net/ethernet/freescale/ucc_geth.c >> +++ b/drivers/net/ethernet/freescale/ucc_geth.c >> @@ -2351,6 +2351,7 @@ static int ucc_geth_startup(struct >> ucc_geth_private *ugeth) >> u8 function_code = 0; >> u8 __iomem *endOfRing; >> u8 numThreadsRxNumerical, numThreadsTxNumerical; >> + s32 rx_glbl_pram_offset, tx_glbl_pram_offset; > > That's still a quite long name for a local variable. True, but I wanted to keep this mechanical and easy to verify. If somebody wants to clean up the local variable names (numThreads[RT]xNumerical also stand out), that can be done later. Rasmus
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c index 74ee2ed2fbbb..75466489bf9a 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.c +++ b/drivers/net/ethernet/freescale/ucc_geth.c @@ -2351,6 +2351,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) u8 function_code = 0; u8 __iomem *endOfRing; u8 numThreadsRxNumerical, numThreadsTxNumerical; + s32 rx_glbl_pram_offset, tx_glbl_pram_offset; ugeth_vdbg("%s: IN", __func__); uccf = ugeth->uccf; @@ -2495,17 +2496,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) */ /* Tx global PRAM */ /* Allocate global tx parameter RAM page */ - ugeth->tx_glbl_pram_offset = + tx_glbl_pram_offset = qe_muram_alloc(sizeof(struct ucc_geth_tx_global_pram), UCC_GETH_TX_GLOBAL_PRAM_ALIGNMENT); - if (IS_ERR_VALUE(ugeth->tx_glbl_pram_offset)) { + if (tx_glbl_pram_offset < 0) { if (netif_msg_ifup(ugeth)) pr_err("Can not allocate DPRAM memory for p_tx_glbl_pram\n"); return -ENOMEM; } - ugeth->p_tx_glbl_pram = - (struct ucc_geth_tx_global_pram __iomem *) qe_muram_addr(ugeth-> - tx_glbl_pram_offset); + ugeth->p_tx_glbl_pram = qe_muram_addr(tx_glbl_pram_offset); /* Fill global PRAM */ /* TQPTR */ @@ -2656,17 +2655,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) /* Rx global PRAM */ /* Allocate global rx parameter RAM page */ - ugeth->rx_glbl_pram_offset = + rx_glbl_pram_offset = qe_muram_alloc(sizeof(struct ucc_geth_rx_global_pram), UCC_GETH_RX_GLOBAL_PRAM_ALIGNMENT); - if (IS_ERR_VALUE(ugeth->rx_glbl_pram_offset)) { + if (rx_glbl_pram_offset < 0) { if (netif_msg_ifup(ugeth)) pr_err("Can not allocate DPRAM memory for p_rx_glbl_pram\n"); return -ENOMEM; } - ugeth->p_rx_glbl_pram = - (struct ucc_geth_rx_global_pram __iomem *) qe_muram_addr(ugeth-> - rx_glbl_pram_offset); + ugeth->p_rx_glbl_pram = qe_muram_addr(rx_glbl_pram_offset); /* Fill global PRAM */ /* RQPTR */ @@ -2928,7 +2925,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) ((u32) ug_info->numThreadsTx) << ENET_INIT_PARAM_TGF_SHIFT; ugeth->p_init_enet_param_shadow->rgftgfrxglobal |= - ugeth->rx_glbl_pram_offset | ug_info->riscRx; + rx_glbl_pram_offset | ug_info->riscRx; if ((ug_info->largestexternallookupkeysize != QE_FLTR_LARGEST_EXTERNAL_TABLE_LOOKUP_KEY_SIZE_NONE) && (ug_info->largestexternallookupkeysize != @@ -2966,7 +2963,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth) } ugeth->p_init_enet_param_shadow->txglobal = - ugeth->tx_glbl_pram_offset | ug_info->riscTx; + tx_glbl_pram_offset | ug_info->riscTx; if ((ret_val = fill_init_enet_entries(ugeth, &(ugeth->p_init_enet_param_shadow-> diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h index c80bed2c995c..be47fa8ced15 100644 --- a/drivers/net/ethernet/freescale/ucc_geth.h +++ b/drivers/net/ethernet/freescale/ucc_geth.h @@ -1166,9 +1166,7 @@ struct ucc_geth_private { struct ucc_geth_exf_global_pram __iomem *p_exf_glbl_param; u32 exf_glbl_param_offset; struct ucc_geth_rx_global_pram __iomem *p_rx_glbl_pram; - u32 rx_glbl_pram_offset; struct ucc_geth_tx_global_pram __iomem *p_tx_glbl_pram; - u32 tx_glbl_pram_offset; struct ucc_geth_send_queue_mem_region __iomem *p_send_q_mem_reg; u32 send_q_mem_reg_offset; struct ucc_geth_thread_data_tx __iomem *p_thread_data_tx;
These fields are only used within ucc_geth_startup(), so they might as well be local variables in that function rather than being stashed in struct ucc_geth_private. Aside from making that struct a tiny bit smaller, it also shortens some lines (getting rid of pointless casts while here), and fixes the problems with using IS_ERR_VALUE() on a u32 as explained in commit 800cd6fb76f0 ("soc: fsl: qe: change return type of cpm_muram_alloc() to s32"). Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk> --- drivers/net/ethernet/freescale/ucc_geth.c | 21 +++++++++------------ drivers/net/ethernet/freescale/ucc_geth.h | 2 -- 2 files changed, 9 insertions(+), 14 deletions(-)