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

[Jakub Kicinski <kuba@kernel.org>]

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.

> +			   "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 ?

> +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()

> +
> +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()

> +	}
> +	dev_addr_mod(netdev, 0, mac, 6);

Why not dev_addr_set() ?!