RE: [EXTERNAL] Re: [net-next PATCH v4 03/10] octeontx2-pf: Create representor netdev

[Geethasowjanya Akula]

> -----Original Message-----
> From: Simon Horman <horms@xxxxxxxxxx>
> Sent: Saturday, May 11, 2024 7:41 PM
> To: Geethasowjanya Akula <gakula@xxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kuba@xxxxxxxxxx;
> davem@xxxxxxxxxxxxx; pabeni@xxxxxxxxxx; edumazet@xxxxxxxxxx; Sunil
> Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; Subbaraya Sundeep Bhatta
> <sbhatta@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>
> Subject: [EXTERNAL] Re: [net-next PATCH v4 03/10] octeontx2-pf: Create
> representor netdev
> 
> ----------------------------------------------------------------------
> On Tue, May 07, 2024 at 10:09:14PM +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@xxxxxxxxxxx>
> 
> ...
> 
> > diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > index 33ebbcb223e1..ff4318f414f8 100644
> > --- a/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > +++ b/drivers/net/ethernet/marvell/octeontx2/nic/rep.c
> > @@ -28,6 +28,157 @@ MODULE_DESCRIPTION(DRV_STRING);
> > MODULE_LICENSE("GPL");  MODULE_DEVICE_TABLE(pci, rvu_rep_id_table);
> >
> > +static int rvu_rep_napi_init(struct otx2_nic *priv, struct
> > +netlink_ext_ack *extack) {
> > +	struct otx2_cq_poll *cq_poll = NULL;
> > +	struct otx2_qset *qset = &priv->qset;
> > +	struct otx2_hw *hw = &priv->hw;
> > +	int err = 0, qidx, vec;
> > +	char *irq_name;
> 
> Please consider using reverse xmas tree - longest line to shortest - for local
> variable declarations in new Networking code.
Will fix it in next version.
> 
> This tool can be helpful: https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__github.com_ecree-
> 2Dsolarflare_xmastree&d=DwIBAg&c=nKjWec2b6R0mOyPaz7xtfQ&r=UiEt_nU
> eYFctu7JVLXVlXDhTmq_EAfooaZEYInfGuEQ&m=Mq4aRWpUpar1zTktQDlrt8Jl6
> 8BXQjkee8RpXExMCFcKMalirC_mVOYEGA0fcuSf&s=Kg1fSyZvUeZAO0TSWVB5
> A1GPqngBfG82Tx1Dz46ZH8c&e=
> 
> ...
> 
> > +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);
> > +	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", rep_id);
> 
> gcc-13 seems unhappy with a string spanning multiple lines.
> I suggest living with a line longer than 80 columns in this case.
> Maybe:
> 
> 			NL_SET_ERR_MSG_FMT_MOD(extack,
> 					       "PFVF representor:%d creation
> failed",
> 					       rep_id);
> 
> > +			err = -ENOMEM;
> > +			goto exit;
> > +		}
> 
Thanks for the feedback. Will submit next version with the suggested changes.
> ...