| Message ID | 20230801123422.374541-1-ruanjinjie@huawei.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net/mlx4: remove many unnecessary NULL values | expand |
On Tue, Aug 01, 2023 at 08:34:22PM +0800, Ruan Jinjie wrote: > Ther are many pointers assigned first, which need not to be initialized, so > remove the NULL assignment. How about something like: Don't initialise local variables to NULL which are always set to other values elsewhere in the same function. > Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> ... > diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c ... > @@ -2294,8 +2294,8 @@ static int mlx4_init_fw(struct mlx4_dev *dev) > static int mlx4_init_hca(struct mlx4_dev *dev) > { > struct mlx4_priv *priv = mlx4_priv(dev); > - struct mlx4_init_hca_param *init_hca = NULL; > - struct mlx4_dev_cap *dev_cap = NULL; > + struct mlx4_init_hca_param *init_hca; > + struct mlx4_dev_cap *dev_cap; > struct mlx4_adapter adapter; > struct mlx4_profile profile; > u64 icm_size; This last hunk doesn't seem correct, as it doesn't seem these aren't always initialised elsewhere in the function before being passed to kfree(). > -- > 2.34.1 > >
On 8/1/2023 5:34 AM, Ruan Jinjie wrote: > Ther are many pointers assigned first, which need not to be initialized, so > remove the NULL assignment. > > Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> Thanks for your patch, make sure you're always explaining "why" you're making a change in your commit message. but see below please. > --- > drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 10 +++++----- > drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++-- > drivers/net/ethernet/mellanox/mlx4/main.c | 12 ++++++------ > 3 files changed, 13 insertions(+), 13 deletions(-) > > diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > index 7d45f1d55f79..164a13272faa 100644 > --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c > @@ -1467,8 +1467,8 @@ static int add_ip_rule(struct mlx4_en_priv *priv, > struct list_head *list_h) > { > int err; > - struct mlx4_spec_list *spec_l2 = NULL; > - struct mlx4_spec_list *spec_l3 = NULL; > + struct mlx4_spec_list *spec_l2; > + struct mlx4_spec_list *spec_l3; What sequence of commands did you use to identify this set of things to change? That would be useful data for the commit message. gcc with -Wunused-something? cppcheck? I've sent these types of patches before, but they've been rejected as churn if they don't fix a clear W=1 or C=2 warning. Did you run the above and see these issues? > struct ethtool_usrip4_spec *l3_mask = &cmd->fs.m_u.usr_ip4_spec; > > spec_l3 = kzalloc(sizeof(*spec_l3), GFP_KERNEL); > @@ -1505,9 +1505,9 @@ static int add_tcp_udp_rule(struct mlx4_en_priv *priv, > struct list_head *list_h, int proto) > { > int err; > - struct mlx4_spec_list *spec_l2 = NULL; > - struct mlx4_spec_list *spec_l3 = NULL; > - struct mlx4_spec_list *spec_l4 = NULL; > + struct mlx4_spec_list *spec_l2; > + struct mlx4_spec_list *spec_l3; > + struct mlx4_spec_list *spec_l4; > struct ethtool_tcpip4_spec *l4_mask = &cmd->fs.m_u.tcp_ip4_spec; > > spec_l2 = kzalloc(sizeof(*spec_l2), GFP_KERNEL); I suggest if you want to have these kind of changes committed you spend more time to make a detailed commit message and explain what's going on for the change as otherwise it's not going to be accepted.
On 2023/8/1 23:40, Simon Horman wrote: > On Tue, Aug 01, 2023 at 08:34:22PM +0800, Ruan Jinjie wrote: >> Ther are many pointers assigned first, which need not to be initialized, so >> remove the NULL assignment. > > How about something like: > > Don't initialise local variables to NULL which are always > set to other values elsewhere in the same function. I think these NULL assignments can also be removed. > >> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> > > ... > >> diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c > > ... > >> @@ -2294,8 +2294,8 @@ static int mlx4_init_fw(struct mlx4_dev *dev) >> static int mlx4_init_hca(struct mlx4_dev *dev) >> { >> struct mlx4_priv *priv = mlx4_priv(dev); >> - struct mlx4_init_hca_param *init_hca = NULL; >> - struct mlx4_dev_cap *dev_cap = NULL; >> + struct mlx4_init_hca_param *init_hca; >> + struct mlx4_dev_cap *dev_cap; >> struct mlx4_adapter adapter; >> struct mlx4_profile profile; >> u64 icm_size; > > This last hunk doesn't seem correct, as it doesn't > seem these aren't always initialised elsewhere in the function > before being passed to kfree(). Yes, this kind of situation the NULL assignments should not be removed,I'll reserch it more. > >> -- >> 2.34.1 >> >>
On 2023/8/2 1:55, Jesse Brandeburg wrote: > On 8/1/2023 5:34 AM, Ruan Jinjie wrote: >> Ther are many pointers assigned first, which need not to be initialized, so >> remove the NULL assignment. >> >> Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> > > Thanks for your patch, make sure you're always explaining "why" you're > making a change in your commit message. Thank you for your sincere advice! I'll notice the issue in future patches. > > but see below please. > >> --- >> drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 10 +++++----- >> drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++-- >> drivers/net/ethernet/mellanox/mlx4/main.c | 12 ++++++------ >> 3 files changed, 13 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> index 7d45f1d55f79..164a13272faa 100644 >> --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c >> @@ -1467,8 +1467,8 @@ static int add_ip_rule(struct mlx4_en_priv *priv, >> struct list_head *list_h) >> { >> int err; >> - struct mlx4_spec_list *spec_l2 = NULL; >> - struct mlx4_spec_list *spec_l3 = NULL; >> + struct mlx4_spec_list *spec_l2; >> + struct mlx4_spec_list *spec_l3; > > What sequence of commands did you use to identify this set of things to > change? That would be useful data for the commit message. > > gcc with -Wunused-something? > cppcheck? Just code walk-through and some keyword match such as malloc and NULL assignment. > > I've sent these types of patches before, but they've been rejected as > churn if they don't fix a clear W=1 or C=2 warning. > > Did you run the above and see these issues? > > >> struct ethtool_usrip4_spec *l3_mask = &cmd->fs.m_u.usr_ip4_spec; >> >> spec_l3 = kzalloc(sizeof(*spec_l3), GFP_KERNEL); >> @@ -1505,9 +1505,9 @@ static int add_tcp_udp_rule(struct mlx4_en_priv *priv, >> struct list_head *list_h, int proto) >> { >> int err; >> - struct mlx4_spec_list *spec_l2 = NULL; >> - struct mlx4_spec_list *spec_l3 = NULL; >> - struct mlx4_spec_list *spec_l4 = NULL; >> + struct mlx4_spec_list *spec_l2; >> + struct mlx4_spec_list *spec_l3; >> + struct mlx4_spec_list *spec_l4; >> struct ethtool_tcpip4_spec *l4_mask = &cmd->fs.m_u.tcp_ip4_spec; >> >> spec_l2 = kzalloc(sizeof(*spec_l2), GFP_KERNEL); > > I suggest if you want to have these kind of changes committed you spend > more time to make a detailed commit message and explain what's going on > for the change as otherwise it's not going to be accepted. Thank you very much! I'll give more explain in future patches. > > >
diff --git a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c index 7d45f1d55f79..164a13272faa 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_ethtool.c @@ -1467,8 +1467,8 @@ static int add_ip_rule(struct mlx4_en_priv *priv, struct list_head *list_h) { int err; - struct mlx4_spec_list *spec_l2 = NULL; - struct mlx4_spec_list *spec_l3 = NULL; + struct mlx4_spec_list *spec_l2; + struct mlx4_spec_list *spec_l3; struct ethtool_usrip4_spec *l3_mask = &cmd->fs.m_u.usr_ip4_spec; spec_l3 = kzalloc(sizeof(*spec_l3), GFP_KERNEL); @@ -1505,9 +1505,9 @@ static int add_tcp_udp_rule(struct mlx4_en_priv *priv, struct list_head *list_h, int proto) { int err; - struct mlx4_spec_list *spec_l2 = NULL; - struct mlx4_spec_list *spec_l3 = NULL; - struct mlx4_spec_list *spec_l4 = NULL; + struct mlx4_spec_list *spec_l2; + struct mlx4_spec_list *spec_l3; + struct mlx4_spec_list *spec_l4; struct ethtool_tcpip4_spec *l4_mask = &cmd->fs.m_u.tcp_ip4_spec; spec_l2 = kzalloc(sizeof(*spec_l2), GFP_KERNEL); diff --git a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c index e11bc0ac880e..403604ceebc8 100644 --- a/drivers/net/ethernet/mellanox/mlx4/en_netdev.c +++ b/drivers/net/ethernet/mellanox/mlx4/en_netdev.c @@ -291,7 +291,7 @@ mlx4_en_filter_alloc(struct mlx4_en_priv *priv, int rxq_index, __be32 src_ip, __be32 dst_ip, u8 ip_proto, __be16 src_port, __be16 dst_port, u32 flow_id) { - struct mlx4_en_filter *filter = NULL; + struct mlx4_en_filter *filter; filter = kzalloc(sizeof(struct mlx4_en_filter), GFP_ATOMIC); if (!filter) @@ -2935,7 +2935,7 @@ static void mlx4_en_bond_work(struct work_struct *work) static int mlx4_en_queue_bond_work(struct mlx4_en_priv *priv, int is_bonded, u8 v2p_p1, u8 v2p_p2) { - struct mlx4_en_bond *bond = NULL; + struct mlx4_en_bond *bond; bond = kzalloc(sizeof(*bond), GFP_ATOMIC); if (!bond) diff --git a/drivers/net/ethernet/mellanox/mlx4/main.c b/drivers/net/ethernet/mellanox/mlx4/main.c index 61286b0d9b0c..9320b57cf788 100644 --- a/drivers/net/ethernet/mellanox/mlx4/main.c +++ b/drivers/net/ethernet/mellanox/mlx4/main.c @@ -864,7 +864,7 @@ static void mlx4_slave_destroy_special_qp_cap(struct mlx4_dev *dev) static int mlx4_slave_special_qp_cap(struct mlx4_dev *dev) { - struct mlx4_func_cap *func_cap = NULL; + struct mlx4_func_cap *func_cap; struct mlx4_caps *caps = &dev->caps; int i, err = 0; @@ -908,9 +908,9 @@ static int mlx4_slave_cap(struct mlx4_dev *dev) { int err; u32 page_size; - struct mlx4_dev_cap *dev_cap = NULL; - struct mlx4_func_cap *func_cap = NULL; - struct mlx4_init_hca_param *hca_param = NULL; + struct mlx4_dev_cap *dev_cap; + struct mlx4_func_cap *func_cap; + struct mlx4_init_hca_param *hca_param; hca_param = kzalloc(sizeof(*hca_param), GFP_KERNEL); func_cap = kzalloc(sizeof(*func_cap), GFP_KERNEL); @@ -2294,8 +2294,8 @@ static int mlx4_init_fw(struct mlx4_dev *dev) static int mlx4_init_hca(struct mlx4_dev *dev) { struct mlx4_priv *priv = mlx4_priv(dev); - struct mlx4_init_hca_param *init_hca = NULL; - struct mlx4_dev_cap *dev_cap = NULL; + struct mlx4_init_hca_param *init_hca; + struct mlx4_dev_cap *dev_cap; struct mlx4_adapter adapter; struct mlx4_profile profile; u64 icm_size;
Ther are many pointers assigned first, which need not to be initialized, so remove the NULL assignment. Signed-off-by: Ruan Jinjie <ruanjinjie@huawei.com> --- drivers/net/ethernet/mellanox/mlx4/en_ethtool.c | 10 +++++----- drivers/net/ethernet/mellanox/mlx4/en_netdev.c | 4 ++-- drivers/net/ethernet/mellanox/mlx4/main.c | 12 ++++++------ 3 files changed, 13 insertions(+), 13 deletions(-)