* [bug report] net/ethtool: add netlink interface for the PLCA RS
@ 2023-01-13 15:55 Dan Carpenter
2023-01-13 16:08 ` Piergiorgio Beruto
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2023-01-13 15:55 UTC (permalink / raw)
To: piergiorgio.beruto; +Cc: netdev
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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [bug report] net/ethtool: add netlink interface for the PLCA RS
2023-01-13 15:55 [bug report] net/ethtool: add netlink interface for the PLCA RS Dan Carpenter
@ 2023-01-13 16:08 ` Piergiorgio Beruto
0 siblings, 0 replies; 2+ messages in thread
From: Piergiorgio Beruto @ 2023-01-13 16:08 UTC (permalink / raw)
To: Dan Carpenter; +Cc: netdev
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-01-13 16:14 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-01-13 15:55 [bug report] net/ethtool: add netlink interface for the PLCA RS Dan Carpenter
2023-01-13 16:08 ` Piergiorgio Beruto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).