netdev - Re: [PATCH net-next v9 09/14] xsc: Init net device

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: <941d54c4-15d5-43b7-a7af-4eb913448492@yunsilicon.com>
Date: Mon, 7 Apr 2025 17:16:53 +0800
From: "Xin Tian" <tianx@...silicon.com>
To: "Jakub Kicinski" <kuba@...nel.org>
Cc: <netdev@...r.kernel.org>, <leon@...nel.org>, <andrew+netdev@...n.ch>, 
	<pabeni@...hat.com>, <edumazet@...gle.com>, <davem@...emloft.net>, 
	<jeff.johnson@....qualcomm.com>, <przemyslaw.kitszel@...el.com>, 
	<weihg@...silicon.com>, <wanry@...silicon.com>, <jacky@...silicon.com>, 
	<horms@...nel.org>, <parthiban.veerasooran@...rochip.com>, 
	<masahiroy@...nel.org>, <kalesh-anakkur.purayil@...adcom.com>, 
	<geert+renesas@...der.be>, <geert@...ux-m68k.org>
Subject: Re: [PATCH net-next v9 09/14] xsc: Init net device

On 2025/3/25 20:19, Jakub Kicinski wrote:
> On Tue, 18 Mar 2025 23:15:11 +0800 Xin Tian wrote:
>> +static int xsc_eth_set_hw_mtu(struct xsc_core_device *xdev,
>> +			      u16 mtu, u16 rx_buf_sz)
>> +{
>> +	struct xsc_set_mtu_mbox_out out;
>> +	struct xsc_set_mtu_mbox_in in;
>> +	int ret;
>> +
>> +	memset(&in, 0, sizeof(struct xsc_set_mtu_mbox_in));
>> +	memset(&out, 0, sizeof(struct xsc_set_mtu_mbox_out));
>> +
>> +	in.hdr.opcode = cpu_to_be16(XSC_CMD_OP_SET_MTU);
>> +	in.mtu = cpu_to_be16(mtu);
>> +	in.rx_buf_sz_min = cpu_to_be16(rx_buf_sz);
>> +	in.mac_port = xdev->mac_port;
>> +
>> +	ret = xsc_cmd_exec(xdev, &in, sizeof(struct xsc_set_mtu_mbox_in), &out,
>> +			   sizeof(struct xsc_set_mtu_mbox_out));
>> +	if (ret || out.hdr.status) {
>> +		netdev_err(((struct xsc_adapter *)xdev->eth_priv)->netdev,
> Please use temporary variable or define a local print macro.
> The cast is too ugly.
OK，will use temporary variable here
>
>> +			   "failed to set hw_mtu=%u rx_buf_sz=%u, err=%d, status=%d\n",
>> +			   mtu, rx_buf_sz, ret, out.hdr.status);
>> +		ret = -ENOEXEC;
> Why are you overwriting the ret code from xsc_cmd_exec() ?
> And why with such an unusual errno ?


Thanks for pointing this out — it's a mistake.


In our old implementation, there were two return values:

the return value of xsc_cmd_exec indicated whether the message was 
properly transmitted through CMDQ,

while out.status reflected the result of the command execution in the 
firmware.

So we chose -ENOEXEC to cover both types of errors.


But now the return value of xsc_cmd_exec already reflects the result of 
out.status,

and -ENOEXEC seems not an appropriate return value here.


I'll fix this and return the result of xsc_cmd_exec directly.


>
>> +static int xsc_eth_get_mac(struct xsc_core_device *xdev, char *mac)
>> +{
>> +	struct xsc_query_eth_mac_mbox_out *out;
>> +	struct xsc_query_eth_mac_mbox_in in;
>> +	int err = 0;
>> +
>> +	out = kzalloc(sizeof(*out), GFP_KERNEL);
>> +	if (!out)
>> +		return -ENOMEM;
>> +
>> +	memset(&in, 0, sizeof(in));
>> +	in.hdr.opcode = cpu_to_be16(XSC_CMD_OP_QUERY_ETH_MAC);
>> +
>> +	err = xsc_cmd_exec(xdev, &in, sizeof(in), out, sizeof(*out));
>> +	if (err || out->hdr.status) {
>> +		netdev_err(((struct xsc_adapter *)xdev->eth_priv)->netdev,
>> +			   "get mac failed! err=%d, out.status=%u\n",
>> +			   err, out->hdr.status);
>> +		err = -ENOEXEC;
>> +		goto err_free;
>> +	}
>> +
>> +	memcpy(mac, out->mac, 6);
> 6 -> ETH_ALEN or ether_addr_copy()
Ack
>
>> +
>> +err_free:
>> +	kfree(out);
>> +	return err;
>> +}
>> +
>> +static void xsc_eth_l2_addr_init(struct xsc_adapter *adapter)
>> +{
>> +	struct net_device *netdev = adapter->netdev;
>> +	char mac[6] = {0};
>> +	int ret = 0;
>> +
>> +	ret = xsc_eth_get_mac(adapter->xdev, mac);
>> +	if (ret) {
>> +		netdev_err(netdev, "get mac failed %d, generate random mac...",
>> +			   ret);
>> +		eth_random_addr(mac);
> eth_hw_addr_random()
OK, no need for set now. Will change.
>
>> +	}
>> +	dev_addr_mod(netdev, 0, mac, 6);
> Why not dev_addr_set() ?!

OK，dev_addr_set() is more suitable

Thanks