lkml.org 
[lkml]   [2024]   [May]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [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;
    > +}

    ..

    \
     
     \ /
      Last update: 2024-05-27 18:12    [W:2.883 / U:0.040 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site