quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+
+ 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?
quoted hunk ↗ jump to hunk
+}
+
+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?
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+static int xsc_eth_probe(struct auxiliary_device *adev,
+ const struct auxiliary_device_id *adev_id)
+{
...
quoted hunk ↗ jump to hunk
+ 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?
quoted hunk ↗ jump to hunk
+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?
quoted hunk ↗ jump to hunk
+
+ adapter = xdev->eth_priv;
+ if (!adapter)
+ return;
How does that happen?
quoted hunk ↗ jump to hunk
+
+ /* 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;
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+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