netdev - RE: [PATCH] net: stmmac: implement .set_intf

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <DB8PR04MB679504FD15F09CCBE3BD061FE6CE0@DB8PR04MB6795.eurprd04.prod.outlook.com>
Date:   Mon, 7 Dec 2020 08:15:29 +0000
From:   Joakim Zhang <qiangqing.zhang@....com>
To:     Jakub Kicinski <kuba@...nel.org>
CC:     "peppe.cavallaro@...com" <peppe.cavallaro@...com>,
        "alexandre.torgue@...com" <alexandre.torgue@...com>,
        "joabreu@...opsys.com" <joabreu@...opsys.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        dl-linux-imx <linux-imx@....com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH] net: stmmac: implement .set_intf_mode() callback for
 imx8dxl


> -----Original Message-----
> From: Jakub Kicinski <kuba@...nel.org>
> Sent: 2020年12月6日 3:58
> To: Joakim Zhang <qiangqing.zhang@....com>
> Cc: peppe.cavallaro@...com; alexandre.torgue@...com;
> joabreu@...opsys.com; davem@...emloft.net; dl-linux-imx
> <linux-imx@....com>; netdev@...r.kernel.org
> Subject: Re: [PATCH] net: stmmac: implement .set_intf_mode() callback for
> imx8dxl
> 
> On Thu,  3 Dec 2020 12:10:38 +0800 Joakim Zhang wrote:
> > From: Fugang Duan <fugang.duan@....com>
> >
> > Implement .set_intf_mode() callback for imx8dxl.
> >
> > Signed-off-by: Fugang Duan <fugang.duan@....com>
> > Signed-off-by: Joakim Zhang <qiangqing.zhang@....com>
> 
> Couple minor issues.
> 
> > @@ -86,7 +88,37 @@ imx8dxl_set_intf_mode(struct
> plat_stmmacenet_data
> > *plat_dat)  {
> >  	int ret = 0;
> >
> > -	/* TBD: depends on imx8dxl scu interfaces to be upstreamed */
> > +	struct imx_sc_ipc *ipc_handle;
> > +	int val;
> 
> Looks like you're gonna have a empty line in the middle of variable
> declarations?
> 
> Please remove it and order the variable lines longest to shortest.
> 
> > +
> > +	ret = imx_scu_get_handle(&ipc_handle);
> > +	if (ret)
> > +		return ret;
> > +
> > +	switch (plat_dat->interface) {
> > +	case PHY_INTERFACE_MODE_MII:
> > +		val = GPR_ENET_QOS_INTF_SEL_MII;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RMII:
> > +		val = GPR_ENET_QOS_INTF_SEL_RMII;
> > +		break;
> > +	case PHY_INTERFACE_MODE_RGMII:
> > +	case PHY_INTERFACE_MODE_RGMII_ID:
> > +	case PHY_INTERFACE_MODE_RGMII_RXID:
> > +	case PHY_INTERFACE_MODE_RGMII_TXID:
> > +		val = GPR_ENET_QOS_INTF_SEL_RGMII;
> > +		break;
> > +	default:
> > +		pr_debug("imx dwmac doesn't support %d interface\n",
> > +			 plat_dat->interface);
> > +		return -EINVAL;
> > +	}
> > +
> > +	ret = imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> > +				      IMX_SC_C_INTF_SEL, val >> 16);
> > +	ret |= imx_sc_misc_set_control(ipc_handle, IMX_SC_R_ENET_1,
> > +				       IMX_SC_C_CLK_GEN_EN, 0x1);
> >  	return ret;
> 
> These calls may return different errors AFAICT.
> 
> You can't just errno values to gether the result will be meaningless.
> 
> please use the normal flow, and return the result of the second call
> directly:
> 
> 	ret = func1();
> 	if (ret)
> 		return ret;
> 
> 	return func2();
> 
> Please also CC the maintainers of the Ethernet PHY subsystem on v2, to make
> sure there is nothing wrong with the patch from their PoV.


Thanks Jakub for your kindly review, I will improve patch following your comments.

Best Regards,
Joakim Zhang
> Thanks!