Received-SPF: pass (google.com: domain of linux-kernel+bounces-167282-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
From: Geethasowjanya Akula <gakula@marvell.com>
To: Simon Horman <horms@kernel.org>
CC: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "davem@davemloft.net"
	<davem@davemloft.net>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "edumazet@google.com" <edumazet@google.com>,
        Sunil Kovvuri Goutham
	<sgoutham@marvell.com>,
        Subbaraya Sundeep Bhatta <sbhatta@marvell.com>,
        Hariprasad Kelam <hkelam@marvell.com>,
        Dan Carpenter
	<dan.carpenter@linaro.org>
Subject: RE: [EXTERNAL] Re: [net-next PATCH v3 3/9] octeontx2-pf: Create
 representor netdev
Thread-Topic: [EXTERNAL] Re: [net-next PATCH v3 3/9] octeontx2-pf: Create
 representor netdev
Thread-Index: AQHamVpOk4r/tpYYKkK9e3oUMm7yXbGCsf8AgAJhACA=
Date: Fri, 3 May 2024 06:27:48 +0000
Message-ID: 
 <CH0PR18MB43399144F70EE124F891AA34CD1F2@CH0PR18MB4339.namprd18.prod.outlook.com>
References: <20240428105312.9731-1-gakula@marvell.com>
 <20240428105312.9731-4-gakula@marvell.com>
 <20240501180656.GX2575892@kernel.org>
In-Reply-To: <20240501180656.GX2575892@kernel.org>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: 
 =?us-ascii?Q?vCzrdfQD1NbmrHEsln9FEqvlZpXCdwV5wBhNUV3ZHPRne0bXihaczMv9ARX8?=
 =?us-ascii?Q?jXReFz3tUC7S6zZx+gJROfebEjBqzL8gfNkogSudLG2X4pS3z/Mtjxs6R/wW?=
 =?us-ascii?Q?7EQwXt4SL5Q/d4CqH7LuNuIahcuTW8Xh0kMPpCXZEJqgzZDql9cJgGhrfUhb?=
 =?us-ascii?Q?53UZZK5SG/AWJ4uUtF7t9RK4L0wFdB0LyEV7RfR9/nJs+Jkj2LXo9duwRV2E?=
 =?us-ascii?Q?TunQTJAIFGf0O9ukpADWtvfSlQKDj39caZBh+leeAl8V7wGhcWl97uB69D5u?=
 =?us-ascii?Q?w6sgbkWYKLRldfMeRtBSuQe4XPEzaXvuzzDDqpe1fp/R0rDuNpqz6T0ZU4+I?=
 =?us-ascii?Q?SRGwrjn576uePi3EiqoSJfKuTJbg5LsN1mzr8H76g/p/B4O9Z6W6uJQIb3+6?=
 =?us-ascii?Q?Oac6GxLYtYTm9L2trku7sFDC7+6/LM17lxxmjWwuhyj8LUHA7np4BFmt04M4?=
 =?us-ascii?Q?d2Ajwu7jKeqbOvjdG20b45wNhsitaUs+zP4Ddvpp3HLwavJzfVc7iNKPm7y3?=
 =?us-ascii?Q?ZmtJ0lQLx8WQ6mqbpreVbqzhn3J/0m8zLohPauM/gGEdb9jXumCPJeSI8BLg?=
 =?us-ascii?Q?iq283Rdq8C80G7g/XrxGqDK+NVUgB7p1aPJbwfkYPCV5r5hEnsa/meNN5jXL?=
 =?us-ascii?Q?Esz3XL6UQfJAyAaCtWIdCvwIm1XyRFLdEaEIXwItLgXLnP835qj95S6LN5ZI?=
 =?us-ascii?Q?AJ/Zo+PCCmeBCvyzBcFXYkabo2uL/DHHDu8LAeac7iyT7S+MsoE9TUJFKq2N?=
 =?us-ascii?Q?KPaPx2XV1JEoOAL7qsXrxnhZvz0gkS/cZgmWi6ZjwHtZ9z90AA7+CFSNcVaJ?=
 =?us-ascii?Q?c+cfhOmDpMpIz+3m5me7d02geV9y0FKvEyGxfLLDpiuzkzK2MVC+J5iZW+dt?=
 =?us-ascii?Q?marlrOvlB9qUmvArK524DHrAxEBoqcXGQ3WxtSaAYB1DYdy/xj52VVRiPSpS?=
 =?us-ascii?Q?pMzsODuIRO1l+O4I/uv7OCZoGbTX7YF6MpLwlMIYDmWaiXZ+QgPWj8Es08Zp?=
 =?us-ascii?Q?rLzFp+pQg/wLs4xFPI1Ph/CorfUOgc5VSlP+hKBv/Y4/ZHNBA8uYdZJrLfn4?=
 =?us-ascii?Q?eJNKmTpwViBj18DRXA2V/QKZrL03HPlz+ak0onbAnVENH7cEAnXg3jvXIjmb?=
 =?us-ascii?Q?WqPILMRXPiVWWFuz5L4fN3sJk7vG8TWibmgAJr03p33YpRhpZ8gs1ViB2NDS?=
 =?us-ascii?Q?ZSIfyJAg3gH5Hzi0UfFMzacHHVxfeYmRnsHaoQ5Oxwz8IIAEARrAfnfI9gYV?=
 =?us-ascii?Q?mYeeAhduT8ECWl76EUS//nxmsiLOATXrhO10J9FpC6HgwgIZp2QGt1XhAVXS?=
 =?us-ascii?Q?dNdz0+Jwy+ZEBY20SJ5O/+vJVLAFyOT1UkqIlaYl20FmxxchYirqWERP5Uzl?=
 =?us-ascii?Q?w9G7g0drUylkWpj8YB52T9DjAtJ6FzXZ7XMHE5EpWs2X55pu58GeRUlWea5X?=
 =?us-ascii?Q?rtWDjtEYQT7/t9/VdpPv4nphb6RAkZdDKsKBHcAuPJSdgMY0szisBiQ8hAmd?=
 =?us-ascii?Q?Wby5G58hkipgeSqBRkr8lkTYM6da9sjOLhxiFZhPHGj93iT0tjnqgeBnRgN2?=
 =?us-ascii?Q?F4a5AhD+t4qWt6jQH4s=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk
X-Mailing-List: linux-kernel@vger.kernel.org
List-Id: <linux-kernel.vger.kernel.org>
List-Subscribe: <mailto:linux-kernel+subscribe@vger.kernel.org>
List-Unsubscribe: <mailto:linux-kernel+unsubscribe@vger.kernel.org>
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: CH0PR18MB4339.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 294c8844-42c7-448e-5375-08dc6b3a26e2
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 May 2024 06:27:48.4221
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: FnCBj+vSYdpa5eH48Q7Y6bCjtzN7m/Cu4/grRVJqQLw5OU5p3DdMnlJjEaCeAGPC58FTttGKJZbvu79o4BQJqw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM4PR18MB5025
X-Proofpoint-GUID: 5iYlxl8ll2iZFfdQrO_EosAZz70wmjaM
X-Proofpoint-ORIG-GUID: 5iYlxl8ll2iZFfdQrO_EosAZz70wmjaM
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.293,Aquarius:18.0.1011,Hydra:6.0.650,FMLib:17.11.176.26
 definitions=2024-05-03_03,2024-05-03_01,2023-05-22_02


> -----Original Message-----
> From: Simon Horman <horms@kernel.org>
> Sent: Wednesday, May 1, 2024 11:37 PM
> To: Geethasowjanya Akula <gakula@marvell.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; kuba@kernel.org=
;
> davem@davemloft.net; pabeni@redhat.com; edumazet@google.com; Sunil
> Kovvuri Goutham <sgoutham@marvell.com>; Subbaraya Sundeep Bhatta
> <sbhatta@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>; Dan
> Carpenter <dan.carpenter@linaro.org>
> Subject: [EXTERNAL] Re: [net-next PATCH v3 3/9] octeontx2-pf: Create
> representor netdev
>=20
> ----------------------------------------------------------------------
> + Dan Carpenter
>=20
> 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>
>=20
> Hi Geetha,
>=20
> Some minor feedback from my side.
>=20
> ...
>=20
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
>=20
> ...
>=20
> > +int rvu_rep_create(struct otx2_nic *priv, struct netlink_ext_ack
> > +*extack) {
> > +	int rep_cnt =3D priv->rep_cnt;
> > +	struct net_device *ndev;
> > +	struct rep_dev *rep;
> > +	int rep_id, err;
> > +	u16 pcifunc;
> > +
> > +	priv->reps =3D devm_kcalloc(priv->dev, rep_cnt, sizeof(struct
> > +rep_dev), GFP_KERNEL);
>=20
> It looks like the size argument should be sizeof(struct rep_dev *) or
> sizeof(*priv->reps).
>=20
> Flagged by Coccinelle.
>=20
> Please consider limiting lines in Networking code to 80 columns wide wher=
e it
> can be achieved without too much bother.
Ack. Will fix it in next version.
>=20
> > +	if (!priv->reps)
> > +		return -ENOMEM;
> > +
> > +	for (rep_id =3D 0; rep_id < rep_cnt; rep_id++) {
> > +		ndev =3D alloc_etherdev(sizeof(*rep));
> > +		if (!ndev) {
> > +			NL_SET_ERR_MSG_FMT_MOD(extack, "PFVF
> representor:%d creation failed\n",
> > +					       rep_id);
> > +			err =3D -ENOMEM;
> > +			goto exit;
> > +		}
> > +
> > +		rep =3D netdev_priv(ndev);
> > +		priv->reps[rep_id] =3D rep;
> > +		rep->mdev =3D priv;
> > +		rep->netdev =3D ndev;
> > +		rep->rep_id =3D rep_id;
> > +
> > +		ndev->min_mtu =3D OTX2_MIN_MTU;
> > +		ndev->max_mtu =3D priv->hw.max_mtu;
> > +		pcifunc =3D priv->rep_pf_map[rep_id];
> > +		rep->pcifunc =3D pcifunc;
> > +
> > +		snprintf(ndev->name, sizeof(ndev->name), "r%dp%d", rep_id,
> > +			 rvu_get_pf(pcifunc));
> > +
> > +		eth_hw_addr_random(ndev);
> > +		err =3D register_netdev(ndev);
> > +		if (err) {
> > +			NL_SET_ERR_MSG_MOD(extack, "PFVF reprentator
> registration
> > +failed\n");
>=20
> 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.
Ack.
>=20
> Flagged by Coccinelle.
>=20
>=20
> The current ndev appears to be leaked here, as it does not appear to be
> covered by the unwind loop below.
>=20
> I think this can be resolved using:
>=20
> 			free_netdev(ndev);
>=20
> > +			goto exit;
> > +		}
> > +	}
> > +	err =3D rvu_rep_napi_init(priv, extack);
> > +	if (err)
> > +		goto exit;
>=20
> Even with the above fixed, Smatch complains that:
>=20
> .../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.
>=20
> Where line 180 is the very last line of the funciton: return err;
>=20
> 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.
>=20
> > +
> > +	return 0;
> > +exit:
> > +	while (--rep_id >=3D 0) {
> > +		rep =3D priv->reps[rep_id];
> > +		unregister_netdev(rep->netdev);
> > +		free_netdev(rep->netdev);
> > +	}
> > +	return err;
> > +}
>=20
> ...