Re: [PATCH v1 07/15] net/xsc: add ethernet netdev registration

[Andrew Lunn]

> @@ -1,7 +1,6 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # Copyright (C) 2021-2026, Shanghai Yunsilicon Technology Co., Ltd.
>  # All rights reserved.
> -#
>  # Yunsilicon driver configuration
>  #

Please don't include hunks like this. Was this added be a previous
patch in this patchset? Then fix it back then. If this is already in
mainline, add a patch making these 'whitespace' changed.

> +static int xsc_eth_open(struct net_device *netdev)
> +{
> +	struct xsc_adapter *adapter = netdev_priv(netdev);
> +	int ret;
> +
> +	if (adapter->opened) {
> +		netdev_err(adapter->netdev, "ndo_open on an already opened device\n");
> +		return 0;
> +	}
> +

How does this happen?

And returning 0 seems odd. Something bad has happened, so at least
EBUSY.

> +
> +	xsc_set_default_xps_cpumasks(adapter);
> +
> +	ret = xsc_set_port_admin_status(adapter, XSC_PORT_UP);
> +	if (ret)
> +		netdev_err(netdev, "failed to set port admin status up, err=%d\n",
> +			   ret);
> +
> +	return 0;

return err?


> +}
> +
> +static int xsc_eth_close(struct net_device *netdev)
> +{
> +	struct xsc_adapter *adapter = netdev_priv(netdev);
> +	int ret;
> +
> +	if (!adapter->opened)
> +		return 0;

How does this happen?

Is there an error code ENOTBUSY? 

> +static void xsc_eth_build_nic_netdev(struct xsc_adapter *adapter)
> +{
> +	struct net_device *netdev = adapter->netdev;
> +
> +	/* Set up network device as normal. */
> +	netdev->netdev_ops = &xsc_netdev_ops;
> +
> +	netdev->min_mtu = SW_DEFAULT_MTU;
> +	netdev->max_mtu = SW_DEFAULT_MTU;
> +	/*mtu - macheaderlen - ipheaderlen should be aligned in 8B*/
> +	netdev->mtu = SW_DEFAULT_MTU;

What has MTU got to do with alignment?

> +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;
> +
> +	out = kzalloc_obj(*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_warn(xsc_dev_to_netdev(xdev),
> +			    "get mac failed! err=%d, out.status=%u\n",
> +			    err, out->hdr.status);
> +		err = -ENOEXEC;
> +		goto exit;
> +	}
> +
> +	memcpy(mac, out->mac, ETH_ALEN);

There are helpers for copying MAC addresses around.

> +static int xsc_eth_probe(struct auxiliary_device *adev,
> +			 const struct auxiliary_device_id *adev_id)
> +{

...

> +	err = register_netdev(netdev);
> +	if (err) {
> +		netdev_err(netdev, "register_netdev failed, err=%d\n", err);
> +		goto err_free_stats;
> +	}

The stack can up the interface and start sending packets before this
function call returns. NFS root often does that.

> +
> +	xdev->netdev = netdev;


Does anything bad happens in open and packet Rx and Tx if xdev->netdev
== NULL because this assignment has not happened yet.

> +
> +	xsc_irq_event_register(xdev, xsc_eth_link_event_handler);

And does anything bad happen if the event handler is not registered?

> +static void xsc_eth_remove(struct auxiliary_device *adev)
> +{
> +	struct xsc_adev *xsc_adev = container_of(adev, struct xsc_adev, adev);
> +	struct xsc_core_device *xdev = xsc_adev->xdev;
> +	struct xsc_adapter *adapter;
> +
> +	if (!xdev)
> +		return;

How does that happen?

> +
> +	adapter = xdev->eth_priv;
> +	if (!adapter)
> +		return;

How does that happen?

> +
> +	/* Stop link events reaching this adapter and wait out one already in
> +	 * flight, which would otherwise touch the memory freed below.
> +	 */
> +	xsc_irq_event_unregister(xdev);
> +
> +	/* Runs ndo_stop(), so the queues and the HCA are shut down first. */
> +	unregister_netdev(adapter->netdev);
> +
> +	/* Must precede free_netdev(): the adapter lives in the netdev priv. */
> +	xsc_eth_nic_cleanup(adapter);
> +	kvfree(adapter->stats);
> +	free_netdev(adapter->netdev);
> +
> +	xdev->netdev = NULL;
> +	xdev->eth_priv = NULL;

> +static inline void xsc_tx_dma_unmap(struct device *dev, struct xsc_sq_dma *dma)
> +{
> +	switch (dma->type) {
> +	case XSC_DMA_MAP_SINGLE:
> +		dma_unmap_single(dev, dma->addr, dma->size, DMA_TO_DEVICE);
> +		break;
> +	case XSC_DMA_MAP_PAGE:
> +		dma_unmap_page(dev, dma->addr, dma->size, DMA_TO_DEVICE);
> +		break;
> +	default:
> +		break;
> +	}

This should probably not be hiding away somewhere in a header file.

> +static inline struct xsc_cqe *xsc_cqwq_get_cqe(struct xsc_cqwq *wq)
> +{
> +	u32 ci = xsc_cqwq_get_ci(wq);
> +	u8 cqe_ownership_bit;
> +	struct xsc_cqe *cqe;
> +	u8 sw_ownership_val;
> +
> +	cqe = xsc_cqwq_get_cqe_buff(wq, ci);
> +
> +	cqe_ownership_bit = !!(le32_to_cpu(cqe->data1) & XSC_CQE_DATA1_OWNER);
> +	sw_ownership_val = xsc_cqwq_get_wrap_cnt(wq) & 1;
> +
> +	if (cqe_ownership_bit != sw_ownership_val)
> +		return NULL;
> +
> +	/* ensure cqe content is read after cqe ownership bit */
> +	dma_rmb();
> +
> +	return cqe;

This is also a bit big for an include file inline. Plus that dma_rmb()
should not be hiding, it should be in plain sight.

	Andrew