Re: [bug report] net/ethtool: add netlink interface for the PLCA RS

[Piergiorgio Beruto <piergiorgio.beruto@gmail.com>]

Hello Dan,
thank you very much for your review.

I have already submitted a patch to fix that. PLease, see
https://patchwork.kernel.org/project/netdevbpf/patch/f2277af8951a51cfee2fb905af8d7a812b7beaf4.1673616357.git.piergiorgio.beruto@gmail.com/

Thanks,
Piergiorgio

On Fri, Jan 13, 2023 at 06:55:33PM +0300, Dan Carpenter wrote:
> Hello Piergiorgio Beruto,
> 
> The patch 8580e16c28f3: "net/ethtool: add netlink interface for the
> PLCA RS" from Jan 9, 2023, leads to the following Smatch static
> checker warning:
> 
> 	net/ethtool/plca.c:155 ethnl_set_plca_cfg()
> 	info: return a literal instead of 'ret'
> 
> net/ethtool/plca.c
>     140 int ethnl_set_plca_cfg(struct sk_buff *skb, struct genl_info *info)
>     141 {
>     142         struct ethnl_req_info req_info = {};
>     143         struct nlattr **tb = info->attrs;
>     144         const struct ethtool_phy_ops *ops;
>     145         struct phy_plca_cfg plca_cfg;
>     146         struct net_device *dev;
>     147         bool mod = false;
>     148         int ret;
>     149 
>     150         ret = ethnl_parse_header_dev_get(&req_info,
>     151                                          tb[ETHTOOL_A_PLCA_HEADER],
>     152                                          genl_info_net(info), info->extack,
>     153                                          true);
>     154         if (!ret)
> --> 155                 return ret;
> 
> This looks like the if statement is reversed.  Otherwise if this is
> a short cut to success, please write it like so:
> 
> 	if (!ret)
> 		return 0;
> 
> 
> There are a bunch of if (!ret) if statements in this function but the
> rest look intentional in context.  It's pretty common to reverse
> the last if statement.  (Not a fan, myself though).
> 
>     156 
>     157         dev = req_info.dev;
>     158 
>     159         rtnl_lock();
>     160 
>     161         // check that the PHY device is available and connected
>     162         if (!dev->phydev) {
>     163                 ret = -EOPNOTSUPP;
>     164                 goto out_rtnl;
>     165         }
>     166 
>     167         ops = ethtool_phy_ops;
>     168         if (!ops || !ops->set_plca_cfg) {
>     169                 ret = -EOPNOTSUPP;
>     170                 goto out_rtnl;
>     171         }
>     172 
>     173         ret = ethnl_ops_begin(dev);
>     174         if (!ret)
>     175                 goto out_rtnl;
>     176 
>     177         memset(&plca_cfg, 0xff, sizeof(plca_cfg));
>     178         plca_update_sint(&plca_cfg.enabled, tb[ETHTOOL_A_PLCA_ENABLED], &mod);
>     179         plca_update_sint(&plca_cfg.node_id, tb[ETHTOOL_A_PLCA_NODE_ID], &mod);
>     180         plca_update_sint(&plca_cfg.node_cnt, tb[ETHTOOL_A_PLCA_NODE_CNT], &mod);
>     181         plca_update_sint(&plca_cfg.to_tmr, tb[ETHTOOL_A_PLCA_TO_TMR], &mod);
>     182         plca_update_sint(&plca_cfg.burst_cnt, tb[ETHTOOL_A_PLCA_BURST_CNT],
>     183                          &mod);
>     184         plca_update_sint(&plca_cfg.burst_tmr, tb[ETHTOOL_A_PLCA_BURST_TMR],
>     185                          &mod);
>     186 
>     187         ret = 0;
>     188         if (!mod)
>     189                 goto out_ops;
>     190 
>     191         ret = ops->set_plca_cfg(dev->phydev, &plca_cfg, info->extack);
>     192         if (!ret)
>     193                 goto out_ops;
>     194 
>     195         ethtool_notify(dev, ETHTOOL_MSG_PLCA_NTF, NULL);
>     196 
>     197 out_ops:
>     198         ethnl_ops_complete(dev);
>     199 out_rtnl:
>     200         rtnl_unlock();
>     201         ethnl_parse_header_dev_put(&req_info);
>     202 
>     203         return ret;
>     204 }
> 
> regards,
> dan carpenter