On Wed, Jun 23, 2021 at 07:34:52PM +0800, Rocco Yue wrote:

+static int ccmni_open(struct net_device *ccmni_dev)
+{
+	struct ccmni_inst *ccmni = netdev_priv(ccmni_dev);
+
+	netif_tx_start_all_queues(ccmni_dev);
+	netif_carrier_on(ccmni_dev);
+
+	if (atomic_inc_return(&ccmni->usage) > 1) {
+		atomic_dec(&ccmni->usage);
+		netdev_err(ccmni_dev, "dev already open\n");
+		return -EINVAL;

You only check this _AFTER_ starting up?  If so, why even check a count
at all?  Why does it matter as it's not keeping anything from working
here.

+	}
+
+	return 0;
+}
+
+static int ccmni_close(struct net_device *ccmni_dev)
+{
+	struct ccmni_inst *ccmni = netdev_priv(ccmni_dev);
+
+	atomic_dec(&ccmni->usage);
+	netif_tx_disable(ccmni_dev);
+
+	return 0;
+}
+
+static netdev_tx_t
+ccmni_start_xmit(struct sk_buff *skb, struct net_device *ccmni_dev)
+{
+	struct ccmni_inst *ccmni = NULL;
+
+	if (unlikely(!ccmni_hook_ready))
+		goto tx_ok;
+
+	if (!skb || !ccmni_dev)
+		goto tx_ok;
+
+	ccmni = netdev_priv(ccmni_dev);
+
+	/* some process can modify ccmni_dev->mtu */
+	if (skb->len > ccmni_dev->mtu) {
+		netdev_err(ccmni_dev, "xmit fail: len(0x%x) > MTU(0x%x, 0x%x)",
+			   skb->len, CCMNI_MTU, ccmni_dev->mtu);
+		goto tx_ok;
+	}
+
+	/* hardware driver send packet will return a negative value
+	 * ask the Linux netdevice to stop the tx queue
+	 */
+	if ((s_ccmni_ctlb->xmit_pkt(ccmni->index, skb, 0)) < 0)
+		return NETDEV_TX_BUSY;
+
+	return NETDEV_TX_OK;
+tx_ok:
+	dev_kfree_skb(skb);
+	ccmni_dev->stats.tx_dropped++;
+	return NETDEV_TX_OK;
+}
+
+static int ccmni_change_mtu(struct net_device *ccmni_dev, int new_mtu)
+{
+	if (new_mtu < 0 || new_mtu > CCMNI_MTU)
+		return -EINVAL;
+
+	if (unlikely(!ccmni_dev))
+		return -EINVAL;
+
+	ccmni_dev->mtu = new_mtu;
+	return 0;
+}
+
+static void ccmni_tx_timeout(struct net_device *ccmni_dev, unsigned int txqueue)
+{
+	struct ccmni_inst *ccmni = netdev_priv(ccmni_dev);
+
+	ccmni_dev->stats.tx_errors++;
+	if (atomic_read(&ccmni->usage) > 0)
+		netif_tx_wake_all_queues(ccmni_dev);

Why does it matter what the reference count is?  What happens if it
drops _RIGHT_ after testing for it?

Anytime you do an atomic_read() call, it's almost always a sign that the
logic is not correct.

Again, why have this reference count at all?  What is it protecting?

+/* exposed API
+ * receive incoming datagrams from the Modem and push them to the
+ * kernel networking system
+ */
+int ccmni_rx_push(unsigned int ccmni_idx, struct sk_buff *skb)

Ah, so this driver doesn't really do anything on its own, as there is no
modem driver for it.

So without a modem driver, it will never be used?  Please submit the
modem driver at the same time, otherwise it's impossible to review this
correctly.

thanks,

greg k-h