Messages in this thread | | | Date | Wed, 1 May 2024 19:06:56 +0100 | From | Simon Horman <> | Subject | Re: [net-next PATCH v3 3/9] octeontx2-pf: Create representor netdev |
| |
+ Dan Carpenter
On Sun, Apr 28, 2024 at 04:23:06PM +0530, Geetha sowjanya wrote: > Adds initial devlink support to set/get the switchdev mode. > Representor netdevs are created for each rvu devices when > the switch mode is set to 'switchdev'. These netdevs are > be used to control and configure VFs. > > Signed-off-by: Geetha sowjanya <gakula@marvell.com>
Hi Geetha,
Some minor feedback from my side.
..
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
..
> +int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack *extack) > +{ > + int rep_cnt = priv->rep_cnt; > + struct net_device *ndev; > + struct rep_dev *rep; > + int rep_id, err; > + u16 pcifunc; > + > + priv->reps = devm_kcalloc(priv->dev, rep_cnt, sizeof(struct rep_dev), GFP_KERNEL);
It looks like the size argument should be sizeof(struct rep_dev *) or sizeof(*priv->reps).
Flagged by Coccinelle.
Please consider limiting lines in Networking code to 80 columns wide where it can be achieved without too much bother.
> + if (!priv->reps) > + return -ENOMEM; > + > + for (rep_id = 0; rep_id < rep_cnt; rep_id++) { > + ndev = alloc_etherdev(sizeof(*rep)); > + if (!ndev) { > + NL_SET_ERR_MSG_FMT_MOD(extack, "PFVF representor:%d creation failed\n", > + rep_id); > + err = -ENOMEM; > + goto exit; > + } > + > + rep = netdev_priv(ndev); > + priv->reps[rep_id] = rep; > + rep->mdev = priv; > + rep->netdev = ndev; > + rep->rep_id = rep_id; > + > + ndev->min_mtu = OTX2_MIN_MTU; > + ndev->max_mtu = priv->hw.max_mtu; > + pcifunc = priv->rep_pf_map[rep_id]; > + rep->pcifunc = pcifunc; > + > + snprintf(ndev->name, sizeof(ndev->name), "r%dp%d", rep_id, > + rvu_get_pf(pcifunc)); > + > + eth_hw_addr_random(ndev); > + err = register_netdev(ndev); > + if (err) { > + NL_SET_ERR_MSG_MOD(extack, "PFVF reprentator registration failed\n");
I don't think the string passed to NL_SET_ERR_MSG_MOD needs a trailing '\n'. I'm unsure if this also applies to NL_SET_ERR_MSG_FMT_MOD or not.
Flagged by Coccinelle.
The current ndev appears to be leaked here, as it does not appear to be covered by the unwind loop below.
I think this can be resolved using:
free_netdev(ndev);
> + goto exit; > + } > + } > + err = rvu_rep_napi_init(priv, extack); > + if (err) > + goto exit;
Even with the above fixed, Smatch complains that:
../rep.c:180 rvu_rep_create() warn: 'ndev' from alloc_etherdev_mqs() not released on lines: 180. ../rep.c:180 rvu_rep_create() warn: 'ndev' from register_netdev() not released on lines: 180.
Where line 180 is the very last line of the funciton: return err;
I think this is triggered by the error handling above. However, I also think it is a false positive. I've CCed Dan Carpenter as I'd value a second opinion on this one.
> + > + return 0; > +exit: > + while (--rep_id >= 0) { > + rep = priv->reps[rep_id]; > + unregister_netdev(rep->netdev); > + free_netdev(rep->netdev); > + } > + return err; > +}
..
| |