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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20201205115802.1dc00448@kicinski-fedora-pc1c0hjn.DHCP.thefacebook.com>
Date:   Sat, 5 Dec 2020 11:58:02 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Joakim Zhang <qiangqing.zhang@....com>
Cc:     peppe.cavallaro@...com, alexandre.torgue@...com,
        joabreu@...opsys.com, davem@...emloft.net, 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!