Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni
flat view
From: Greg KH <hidden>
Date: 2021-06-23 17:31:09
Also in:
bpf, linux-arm-kernel, linux-mediatek, lkml, netdev
On Wed, Jun 23, 2021 at 07:34:52PM +0800, Rocco Yue wrote:
quoted hunk ↗ jump to hunk
+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.
quoted hunk ↗ jump to hunk
+ } + + 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?
quoted hunk ↗ jump to hunk
+/* 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