| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <26c7c3cc-2dec-c86e-2e9d-63dc1a7ddba1@csgroup.eu>
Date: Wed, 20 Jan 2021 07:57:03 +0100
From: Christophe Leroy <christophe.leroy@...roup.eu>
To: Rasmus Villemoes <rasmus.villemoes@...vas.dk>,
netdev@...r.kernel.org
Cc: Li Yang <leoyang.li@....com>,
"David S . Miller" <davem@...emloft.net>,
Zhao Qiang <qiang.zhao@....com>, Andrew Lunn <andrew@...n.ch>,
Jakub Kicinski <kuba@...nel.org>,
Joakim Tjernlund <Joakim.Tjernlund@...inera.com>
Subject: Re: [PATCH net-next v2 08/17] ethernet: ucc_geth: remove
{rx,tx}_glbl_pram_offset from struct ucc_geth_private
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@...vas.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;
>
Powered by blists - more mailing lists