2021-06-23 11:51:10

by Rocco Yue

[permalink] [raw]
Subject: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

This patch add the definition of ARPHRD_PUREIP which can for
example be used by mobile ccmni device as device type.
ARPHRD_PUREIP means that this device doesn't need kernel to
generate ipv6 link-local address in any addr_gen_mode.

Signed-off-by: Rocco Yue <[email protected]>
---
include/uapi/linux/if_arp.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/include/uapi/linux/if_arp.h b/include/uapi/linux/if_arp.h
index c3cc5a9e5eaf..4463c9e9e8b4 100644
--- a/include/uapi/linux/if_arp.h
+++ b/include/uapi/linux/if_arp.h
@@ -61,6 +61,7 @@
#define ARPHRD_DDCMP 517 /* Digital's DDCMP protocol */
#define ARPHRD_RAWHDLC 518 /* Raw HDLC */
#define ARPHRD_RAWIP 519 /* Raw IP */
+#define ARPHRD_PUREIP 520 /* Pure IP */

#define ARPHRD_TUNNEL 768 /* IPIP tunnel */
#define ARPHRD_TUNNEL6 769 /* IP6IP6 tunnel */
--
2.18.0


2021-06-23 11:51:23

by Rocco Yue

[permalink] [raw]
Subject: [PATCH 2/4] net: ipv6: don't generate link local address on PUREIP device

PUREIP device such as ccmni does not need kernel to generate
ipv6 link-local address in any addr_gen_mode, generally, it
shall use the IPv6 Interface Identifier, as provided by the
GGSN, to create its IPv6 link-ocal Unicast Address.

Signed-off-by: Rocco Yue <[email protected]>
---
net/ipv6/addrconf.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 701eb82acd1c..1bfa7eca1314 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -3339,7 +3339,8 @@ static void addrconf_dev_config(struct net_device *dev)
(dev->type != ARPHRD_IPGRE) &&
(dev->type != ARPHRD_TUNNEL) &&
(dev->type != ARPHRD_NONE) &&
- (dev->type != ARPHRD_RAWIP)) {
+ (dev->type != ARPHRD_RAWIP) &&
+ (dev->type != ARPHRD_PUREIP)) {
/* Alas, we support only Ethernet autoconfiguration. */
idev = __in6_dev_get(dev);
if (!IS_ERR_OR_NULL(idev) && dev->flags & IFF_UP &&
@@ -3352,6 +3353,12 @@ static void addrconf_dev_config(struct net_device *dev)
if (IS_ERR(idev))
return;

+ /* this device type doesn't need to generate
+ * link-local address in any addr_gen_mode
+ */
+ if (dev->type == ARPHRD_PUREIP)
+ return;
+
/* this device type has no EUI support */
if (dev->type == ARPHRD_NONE &&
idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_EUI64)
--
2.18.0

2021-06-23 11:52:49

by Rocco Yue

[permalink] [raw]
Subject: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

ccmni driver is used by all recent chipsets using MediaTek Inc.
modems. It provides a bridge to shield hardware differences and
connect kernel netdevice. Module provides virtual network devices
which can be used to support different mobile networks, such as
a default cellular internet,or IMS network.

Signed-off-by: Rocco Yue <[email protected]>
---
.../device_drivers/cellular/ccmni/ccmni.rst | 151 +++++++++
MAINTAINERS | 9 +
drivers/net/ethernet/mediatek/Kconfig | 5 +
drivers/net/ethernet/mediatek/Makefile | 4 +-
drivers/net/ethernet/mediatek/ccmni/Kconfig | 15 +
drivers/net/ethernet/mediatek/ccmni/Makefile | 7 +
drivers/net/ethernet/mediatek/ccmni/ccmni.c | 291 ++++++++++++++++++
drivers/net/ethernet/mediatek/ccmni/ccmni.h | 53 ++++
8 files changed, 534 insertions(+), 1 deletion(-)
create mode 100644 Documentation/networking/device_drivers/cellular/ccmni/ccmni.rst
create mode 100644 drivers/net/ethernet/mediatek/ccmni/Kconfig
create mode 100644 drivers/net/ethernet/mediatek/ccmni/Makefile
create mode 100644 drivers/net/ethernet/mediatek/ccmni/ccmni.c
create mode 100644 drivers/net/ethernet/mediatek/ccmni/ccmni.h

diff --git a/Documentation/networking/device_drivers/cellular/ccmni/ccmni.rst b/Documentation/networking/device_drivers/cellular/ccmni/ccmni.rst
new file mode 100644
index 000000000000..16d547786cbd
--- /dev/null
+++ b/Documentation/networking/device_drivers/cellular/ccmni/ccmni.rst
@@ -0,0 +1,151 @@
+.. SPDX-License-Identifier: GPL-2.0+
+
+===================================================================
+Linux kernel driver for Cross Core Multi Network Interface (ccmni):
+===================================================================
+
+
+1. Introduction
+===============
+
+ccmni driver is built on the top of AP-CCCI (Application Processor Cross
+Core Communite Interface) to emulate the modem’s data networking service
+as a network interface card. This driver is used by all recent chipsets
+using MediaTek Inc. modems.
+
+This driver can support multiple tx/rx channels, each channel has its
+own IP address, and ipv4 and ipv6 link-local addresses on the interface
+is set through ioctl, which means that ccmni driver is a pure ip device
+and it does not need the kernel to generate these ip addresses
+automatically.
+
+Creating logical network devices (ccmni devices) can be used to handle
+multiple private data networks (PDNs), such as a defaule cellular
+internet, IP Multimedia Subsystem (IMS) network (VoWiFi/VoLTE), IMS
+Emergency, Tethering, Multimedia Messaging Service (MMS), and so on.
+In general, one ccmni device corresponds to one PDN.
+
+ccmni design
+------------
+
+- AT commands and responses between the Application Processor and Modem
+ Processor take place over the ccci tty serial port emulation.
+- IP packets coming into the ccmni driver (from the Modem) needs to be
+ un-framed (via the Packet Framing Protocol) before it can be stuffed
+ into the Socket Buffer.
+- IP Packets going out of the ccmni driver (to the Modem) needs to be
+ framed (via the Packet Framing Protocol) before it can be pushed into
+ the respective ccci tx buffer.
+- The Packet Framing Protocol module contains algorithms to correctly
+ add or remove frames, going out to and coming into, the ccmni driver
+ respectively.
+- Packets are passed to, and received from the Linux networking system,
+ via Socket Buffers.
+- The Android Application Framework contains codes to setup the ccmni’s
+ parameters (such as netmask), and the routing table.
+
+
+2. Architecture
+===============
+
+ +------------------------+ +---------------------+
+user | MTK RilD | | network process |
+space | (config/up/down ccmni) | | (send/recv packets) |
+ +------------------------+ +---------------------+
++--------------------------------------------------------------------+
+ +--------------------------------------------------+
+ | socket |
+ +--------------------------------------------------+
+ +---------------------+
+ | TCP/IP stack |
+ +---------------------+
+ +--------------------------------------------------+
+ | net device layer |
+ +--------------------------------------------------+
+ +--------+ +--------+ +--------+
+kernel | ccmni0 | | ccmni1 | ... | ccmnix |
+space +--------+ +--------+ +--------+
+ +-------------------------------------------------+
+ | ccmni driver |
+ +-------------------------------------------------+
+ +-------------------------------------------------+
+ | AP-CCCI |
+ +-------------------------------------------------+
++---------------------------------------------------------------------+
+ +-------------------------------------------------+
+ | +-------------------+ +-------------------+ |
+ | | DPMAIF hardware | | MD-CCCI | |
+ | +-------------------+ +-------------------+ |
+ | ... ... |
+ | Modem Processor |
+ +-------------------------------------------------+
+
+
+3. Driver information and notes
+===============================
+
+Data Connection Set Up
+----------------------
+
+The data framework will first SetupDataCall with passing ccmni index,
+and then RilD will activate PDN connection and get CID (Connection ID).
+
+Next, RilD will creat ccmni socket to use ioctl to configure ccmni up,
+and then ccmni_open() will be called.
+
+In addition, since ccmni is a pureip device, RilD needs to use ioctl to
+configure the ipv4/ipv6-link-local address for ccmni after it is up.
+
+Alternatively, you can use the ip command as follows::
+ ip link set up dev ccmni<x>
+ ip addr add a.b.c.d dev ccmni<x>
+ ip -6 addr add fe80::1/64 dev ccmni<x>
+
+Data Connection Set Down
+------------------------
+
+The data service implements a method to tear down the data connection,
+after RilD deactivate the PDN connection, RilD will down the specific
+interface of ccmnix through ioctl SIOCSIFFLAGS, and then ccmni_close()
+will be called. After that, if any network process (such as browser) wants
+to write data to ccmni socket, TCP/IP stack will return an error to
+this socket.
+
+Data Transmit
+-------------
+
+In the uplink direction, when there is data to be transmitted to the cellular
+network, ccmni_start_xmit() will be called by the Linux networking system.
+
+main operations in ccmni_start_xmit():
+- the datagram to be transmitted is housed in the Socket Buffer.
+- check if the datagram is within limits (i.e. 1500 bytes) acceptable by the
+ Modem. If the datagram exceeds limit, the datagram will be dropped, and free
+ the Socket Buffer.
+- check if the AP-CCCI TX buffer is busy, or do not have enough space for this
+ datagram. If it is busy, or the free space is too small, ccmni_start_xmit()
+ return NETDEV_TX_BUSY and ask the Linux netdevice to stop the tx queue.
+
+To handle outcoming datagrams to the Modem, ccmni register a callback function
+for AP-CCCI driver. ccmni_hif_hook() means ccci can implement specific egress
+function to send these packets to the specific hardware.
+
+Data Receive
+------------
+
+In the downlink direction, DPMAIF (Data Path MD AP Interface) hardware sends
+packets and messages with channel id matching these packets to AP-CCCI driver.
+
+To handle incoming datagrams from the Modem, ccmni register a callback
+function for the AP-CCCI driver. ccmni_rx_push() responsible for extracting
+the incoming packets from the ccci rx buffer, and updating skb. Once ready,
+ccmni signal to the Linux networking system to take out Socket Buffer
+(via netif_rx() / netif_rx_ni()).
+
+
+Support
+=======
+
+If an issue is identified by published source code and supported adapter on
+the supported kernel, please email the specific information about the issue
+to [email protected] and [email protected]
diff --git a/MAINTAINERS b/MAINTAINERS
index 8c5ee008301a..1e53a754c727 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11612,6 +11612,15 @@ F: Documentation/devicetree/bindings/usb/mediatek,*
F: drivers/usb/host/xhci-mtk*
F: drivers/usb/mtu3/

+MEDIATEK CCMNI DRIVER
+M: Rocco Yue <[email protected]>
+M: Chao Song <[email protected]>
+M: Zhuoliang Zhang <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/networking/device_drivers/cellular/mediatek/ccmni.rst
+F: drivers/net/ethernet/mediatek/ccmni/
+
MEGACHIPS STDPXXXX-GE-B850V3-FW LVDS/DP++ BRIDGES
M: Peter Senna Tschudin <[email protected]>
M: Martin Donnelly <[email protected]>
diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig
index c357c193378e..2f499f3bc720 100644
--- a/drivers/net/ethernet/mediatek/Kconfig
+++ b/drivers/net/ethernet/mediatek/Kconfig
@@ -1,4 +1,7 @@
# SPDX-License-Identifier: GPL-2.0-only
+#
+# MediaTek network device configuration
+#
config NET_VENDOR_MEDIATEK
bool "MediaTek devices"
depends on ARCH_MEDIATEK || SOC_MT7621 || SOC_MT7620
@@ -24,4 +27,6 @@ config NET_MEDIATEK_STAR_EMAC
This driver supports the ethernet MAC IP first used on
MediaTek MT85** SoCs.

+source "drivers/net/ethernet/mediatek/ccmni/Kconfig"
+
endif #NET_VENDOR_MEDIATEK
diff --git a/drivers/net/ethernet/mediatek/Makefile b/drivers/net/ethernet/mediatek/Makefile
index 79d4cdbbcbf5..85bd3ebf2388 100644
--- a/drivers/net/ethernet/mediatek/Makefile
+++ b/drivers/net/ethernet/mediatek/Makefile
@@ -1,8 +1,10 @@
# SPDX-License-Identifier: GPL-2.0-only
#
-# Makefile for the Mediatek SoCs built-in ethernet macs
+# Makefile for the Mediatek network device drivers.
#

obj-$(CONFIG_NET_MEDIATEK_SOC) += mtk_eth.o
mtk_eth-y := mtk_eth_soc.o mtk_sgmii.o mtk_eth_path.o mtk_ppe.o mtk_ppe_debugfs.o mtk_ppe_offload.o
obj-$(CONFIG_NET_MEDIATEK_STAR_EMAC) += mtk_star_emac.o
+
+obj-$(CONFIG_MTK_NET_CCMNI) += ccmni/
diff --git a/drivers/net/ethernet/mediatek/ccmni/Kconfig b/drivers/net/ethernet/mediatek/ccmni/Kconfig
new file mode 100644
index 000000000000..d97c0e48b58e
--- /dev/null
+++ b/drivers/net/ethernet/mediatek/ccmni/Kconfig
@@ -0,0 +1,15 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# MediaTek CCMNI driver
+#
+
+menuconfig MTK_NET_CCMNI
+ tristate "MediaTek CCMNI driver"
+ default n
+ help
+ Cross Core Multi Network Interface:
+ If you select this, you will enable the CCMNI module which is used
+ to shield hardware differences and communicate with kernel netdevice
+ and be used for handling outgoing/incoimg mobile data. Module provides
+ virtual network devices which can be used to support different mobile
+ networks.
diff --git a/drivers/net/ethernet/mediatek/ccmni/Makefile b/drivers/net/ethernet/mediatek/ccmni/Makefile
new file mode 100644
index 000000000000..e79c23d1b79d
--- /dev/null
+++ b/drivers/net/ethernet/mediatek/ccmni/Makefile
@@ -0,0 +1,7 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for the ccmni module
+#
+ccflags-y += -I$(srctree)/drivers/net/ethernet/mediatek/ccmni/
+obj-$(CONFIG_MTK_NET_CCMNI) += ccmni.o
+
diff --git a/drivers/net/ethernet/mediatek/ccmni/ccmni.c b/drivers/net/ethernet/mediatek/ccmni/ccmni.c
new file mode 100644
index 000000000000..700a0d092596
--- /dev/null
+++ b/drivers/net/ethernet/mediatek/ccmni/ccmni.c
@@ -0,0 +1,291 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2021 MediaTek Inc.
+ *
+ * CCMNI Data virtual netwotrk driver
+ */
+
+#include <linux/if_arp.h>
+#include <linux/module.h>
+#include <linux/preempt.h>
+#include <net/sch_generic.h>
+
+#include "ccmni.h"
+
+static struct ccmni_ctl_block *s_ccmni_ctlb;
+static int ccmni_hook_ready;
+
+/* Network Device Operations */
+
+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;
+ }
+
+ 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);
+}
+
+static const struct net_device_ops ccmni_netdev_ops = {
+ .ndo_open = ccmni_open,
+ .ndo_stop = ccmni_close,
+ .ndo_start_xmit = ccmni_start_xmit,
+ .ndo_tx_timeout = ccmni_tx_timeout,
+ .ndo_change_mtu = ccmni_change_mtu,
+};
+
+/* init ccmni network device */
+static inline void ccmni_dev_init(struct net_device *ccmni_dev, unsigned int idx)
+{
+ ccmni_dev->mtu = CCMNI_MTU;
+ ccmni_dev->tx_queue_len = CCMNI_TX_QUEUE;
+ ccmni_dev->watchdog_timeo = CCMNI_NETDEV_WDT_TO;
+ ccmni_dev->flags = IFF_NOARP &
+ (~IFF_BROADCAST & ~IFF_MULTICAST);
+
+ /* not support VLAN */
+ ccmni_dev->features = NETIF_F_VLAN_CHALLENGED;
+ ccmni_dev->features |= NETIF_F_SG;
+ ccmni_dev->hw_features |= NETIF_F_SG;
+
+ /* pure ip mode */
+ ccmni_dev->type = ARPHRD_PUREIP;
+ ccmni_dev->header_ops = NULL;
+ ccmni_dev->hard_header_len = 0;
+ ccmni_dev->addr_len = 0;
+ ccmni_dev->priv_destructor = free_netdev;
+ ccmni_dev->netdev_ops = &ccmni_netdev_ops;
+ random_ether_addr((u8 *)ccmni_dev->dev_addr);
+ sprintf(ccmni_dev->name, "ccmni%d", idx);
+}
+
+/* init ccmni instance */
+static inline void ccmni_inst_init(struct net_device *netdev, unsigned int idx)
+{
+ struct ccmni_inst *ccmni = netdev_priv(netdev);
+
+ ccmni->index = idx;
+ ccmni->dev = netdev;
+ atomic_set(&ccmni->usage, 0);
+
+ s_ccmni_ctlb->ccmni_inst[idx] = ccmni;
+}
+
+/* ccmni driver module startup/shutdown */
+
+static int __init ccmni_init(void)
+{
+ struct net_device *dev = NULL;
+ unsigned int i, j;
+ int ret = 0;
+
+ s_ccmni_ctlb = kzalloc(sizeof(*s_ccmni_ctlb), GFP_KERNEL);
+ if (!s_ccmni_ctlb)
+ return -ENOMEM;
+
+ s_ccmni_ctlb->max_num = MAX_CCMNI_NUM;
+ for (i = 0; i < MAX_CCMNI_NUM; i++) {
+ /* alloc multiple tx queue, 2 txq and 1 rxq */
+ dev = alloc_etherdev_mqs(sizeof(struct ccmni_inst), 2, 1);
+ if (unlikely(!dev)) {
+ ret = -ENOMEM;
+ goto alloc_netdev_fail;
+ }
+ ccmni_dev_init(dev, i);
+ ccmni_inst_init(dev, i);
+ ret = register_netdev(dev);
+ if (ret)
+ goto alloc_netdev_fail;
+ }
+ return ret;
+
+alloc_netdev_fail:
+ if (dev) {
+ free_netdev(dev);
+ s_ccmni_ctlb->ccmni_inst[i] = NULL;
+ }
+ for (j = i - 1; j >= 0; j--) {
+ unregister_netdev(s_ccmni_ctlb->ccmni_inst[j]->dev);
+ s_ccmni_ctlb->ccmni_inst[j] = NULL;
+ }
+ kfree(s_ccmni_ctlb);
+ s_ccmni_ctlb = NULL;
+
+ return ret;
+}
+
+static void __exit ccmni_exit(void)
+{
+ struct ccmni_ctl_block *ctlb = NULL;
+ struct ccmni_inst *ccmni = NULL;
+ int i;
+
+ ctlb = s_ccmni_ctlb;
+ if (!s_ccmni_ctlb)
+ return;
+ for (i = 0; i < s_ccmni_ctlb->max_num; i++) {
+ ccmni = s_ccmni_ctlb->ccmni_inst[i];
+ if (ccmni) {
+ unregister_netdev(ccmni->dev);
+ s_ccmni_ctlb->ccmni_inst[i] = NULL;
+ }
+ }
+ kfree(s_ccmni_ctlb);
+ s_ccmni_ctlb = NULL;
+}
+
+/* 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)
+{
+ struct ccmni_inst *ccmni = NULL;
+ struct net_device *dev = NULL;
+ int pkt_type, skb_len;
+
+ if (unlikely(!ccmni_hook_ready))
+ return -EINVAL;
+
+ /* Some hardware can send us error index. Catch them */
+ if (unlikely(ccmni_idx >= s_ccmni_ctlb->max_num))
+ return -EINVAL;
+
+ ccmni = s_ccmni_ctlb->ccmni_inst[ccmni_idx];
+ dev = ccmni->dev;
+
+ pkt_type = skb->data[0] & 0xF0;
+ skb_reset_transport_header(skb);
+ skb_reset_network_header(skb);
+ skb_set_mac_header(skb, 0);
+ skb_reset_mac_len(skb);
+ skb->dev = dev;
+
+ if (pkt_type == IPV6_VERSION)
+ skb->protocol = htons(ETH_P_IPV6);
+ else if (pkt_type == IPV4_VERSION)
+ skb->protocol = htons(ETH_P_IP);
+
+ skb_len = skb->len;
+
+ if (!in_interrupt())
+ netif_rx_ni(skb);
+ else
+ netif_rx(skb);
+
+ dev->stats.rx_packets++;
+ dev->stats.rx_bytes += skb_len;
+
+ return 0;
+}
+EXPORT_SYMBOL(ccmni_rx_push);
+
+/* exposed API
+ * hardware driver can init the struct ccmni_hif_ops and implement specific
+ * xmnit function to send UL packets to the specific hardware
+ */
+int ccmni_hif_hook(struct ccmni_hif_ops *hif_ops)
+{
+ if (unlikely(!hif_ops)) {
+ pr_err("ccmni: %s fail: argument is NULL\n", __func__);
+ return -EINVAL;
+ }
+ if (unlikely(!s_ccmni_ctlb)) {
+ pr_err("ccmni: %s fail: s_ccmni_ctlb is NULL\n", __func__);
+ return -EINVAL;
+ }
+ if (unlikely(s_ccmni_ctlb->hif_ops)) {
+ pr_err("ccmni: %s fail: hif_ops already hooked\n", __func__);
+ return -EINVAL;
+ }
+
+ s_ccmni_ctlb->hif_ops = hif_ops;
+ if (!hif_ops->xmit_pkt) {
+ pr_err("ccmni: %s fail: key hook func: xmit is NULL\n",
+ __func__);
+ return -EINVAL;
+ }
+
+ s_ccmni_ctlb->xmit_pkt = hif_ops->xmit_pkt;
+ ccmni_hook_ready = 1;
+
+ return 0;
+}
+EXPORT_SYMBOL(ccmni_hif_hook);
+
+module_init(ccmni_init);
+module_exit(ccmni_exit);
+MODULE_AUTHOR("MediaTek, Inc.");
+MODULE_DESCRIPTION("ccmni driver v1.0");
+MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/mediatek/ccmni/ccmni.h b/drivers/net/ethernet/mediatek/ccmni/ccmni.h
new file mode 100644
index 000000000000..e2799aa2c9d4
--- /dev/null
+++ b/drivers/net/ethernet/mediatek/ccmni/ccmni.h
@@ -0,0 +1,53 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2021 MediaTek Inc.
+ *
+ * CCMNI Data virtual netwotrk driver
+ */
+
+#ifndef __CCMNI_NET_H__
+#define __CCMNI_NET_H__
+
+#include <linux/device.h>
+#include <linux/etherdevice.h>
+#include <linux/if_ether.h>
+#include <linux/kernel.h>
+#include <linux/netdevice.h>
+#include <linux/skbuff.h>
+
+#define CCMNI_MTU 1500
+#define CCMNI_TX_QUEUE 1000
+#define CCMNI_NETDEV_WDT_TO (1 * HZ)
+
+#define IPV4_VERSION 0x40
+#define IPV6_VERSION 0x60
+
+#define MAX_CCMNI_NUM 22
+
+/* One instance of this structure is instantiated for each
+ * real_dev associated with ccmni
+ */
+struct ccmni_inst {
+ int index;
+ atomic_t usage;
+ struct net_device *dev;
+ unsigned char name[16];
+};
+
+/* an export struct of ccmni hardware interface operations
+ */
+struct ccmni_hif_ops {
+ int (*xmit_pkt)(int index, void *data, int ref_flag);
+};
+
+struct ccmni_ctl_block {
+ int (*xmit_pkt)(int index, void *data, int ref_flag);
+ struct ccmni_hif_ops *hif_ops;
+ struct ccmni_inst *ccmni_inst[MAX_CCMNI_NUM];
+ int max_num;
+};
+
+int ccmni_hif_hook(struct ccmni_hif_ops *hif_ops);
+int ccmni_rx_push(unsigned int ccmni_idx, struct sk_buff *skb);
+
+#endif /* __CCMNI_NET_H__ */
--
2.18.0

2021-06-23 11:53:41

by Rocco Yue

[permalink] [raw]
Subject: [PATCH 3/4] net: dev_is_mac_header_xmit() return false for ARPHRD_PUREIP

the incoming/outgoing packets on that pure ip interface do
not have an ethernet header. So adding ARPHRD_PUREIP to the
dev_is_mac_header_xmit() make the function return false, and
__bpf_redirect() checks this boolean to determine whether to
prefix an ethernet header.

Signed-off-by: Rocco Yue <[email protected]>
---
include/linux/if_arp.h | 1 +
1 file changed, 1 insertion(+)

diff --git a/include/linux/if_arp.h b/include/linux/if_arp.h
index bf5c5f32c65e..0dc7af11c463 100644
--- a/include/linux/if_arp.h
+++ b/include/linux/if_arp.h
@@ -51,6 +51,7 @@ static inline bool dev_is_mac_header_xmit(const struct net_device *dev)
case ARPHRD_VOID:
case ARPHRD_NONE:
case ARPHRD_RAWIP:
+ case ARPHRD_PUREIP:
return false;
default:
return true;
--
2.18.0

2021-06-23 17:23:52

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Wed, Jun 23, 2021 at 07:34:49PM +0800, Rocco Yue wrote:
> This patch add the definition of ARPHRD_PUREIP which can for
> example be used by mobile ccmni device as device type.
> ARPHRD_PUREIP means that this device doesn't need kernel to
> generate ipv6 link-local address in any addr_gen_mode.
>
> Signed-off-by: Rocco Yue <[email protected]>
> ---
> include/uapi/linux/if_arp.h | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/include/uapi/linux/if_arp.h b/include/uapi/linux/if_arp.h
> index c3cc5a9e5eaf..4463c9e9e8b4 100644
> --- a/include/uapi/linux/if_arp.h
> +++ b/include/uapi/linux/if_arp.h
> @@ -61,6 +61,7 @@
> #define ARPHRD_DDCMP 517 /* Digital's DDCMP protocol */
> #define ARPHRD_RAWHDLC 518 /* Raw HDLC */
> #define ARPHRD_RAWIP 519 /* Raw IP */
> +#define ARPHRD_PUREIP 520 /* Pure IP */

In looking at the patches, what differs "PUREIP" from "RAWIP"? It seems
to be the same to me. If they are different, where is that documented?

thanks,

greg k-h

2021-06-23 17:26:11

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

On Wed, Jun 23, 2021 at 07:34:52PM +0800, Rocco Yue wrote:
> +EXPORT_SYMBOL(ccmni_rx_push);

Why are you exporting symbols that are not used by anyone in this patch
series? That doesn't feel right. Who needs this?

> +EXPORT_SYMBOL(ccmni_hif_hook);

Same with this, who calls this?

> +++ b/drivers/net/ethernet/mediatek/ccmni/ccmni.h

Why do you have a .h file for a single .c file? that shouldn't be
needed.

thanks,

greg k-h

2021-06-23 17:35:12

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

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

2021-06-24 03:50:56

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Wed, 2021-06-23 at 19:19 +0200, Greg KH wrote:
On Wed, Jun 23, 2021 at 07:34:49PM +0800, Rocco Yue wrote:
>> This patch add the definition of ARPHRD_PUREIP which can for
>> example be used by mobile ccmni device as device type.
>> ARPHRD_PUREIP means that this device doesn't need kernel to
>> generate ipv6 link-local address in any addr_gen_mode.
>>
>> Signed-off-by: Rocco Yue <[email protected]>
>> ---
>> include/uapi/linux/if_arp.h | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/include/uapi/linux/if_arp.h b/include/uapi/linux/if_arp.h
>> index c3cc5a9e5eaf..4463c9e9e8b4 100644
>> --- a/include/uapi/linux/if_arp.h
>> +++ b/include/uapi/linux/if_arp.h
>> @@ -61,6 +61,7 @@
>> #define ARPHRD_DDCMP 517 /* Digital's DDCMP protocol */
>> #define ARPHRD_RAWHDLC 518 /* Raw HDLC */
>> #define ARPHRD_RAWIP 519 /* Raw IP */
>> +#define ARPHRD_PUREIP 520 /* Pure IP */
>
> In looking at the patches, what differs "PUREIP" from "RAWIP"? It seems

Thanks for your review.

The difference between RAWIP and PUREIP is that they generate IPv6
link-local address and IPv6 global address in different ways.

RAWIP:
~~~~~~
In the ipv6_generate_eui64() function, using RAWIP will always return 0,
which will cause the kernel to automatically generate an IPv6 link-local
address in EUI64 format and an IPv6 global address in EUI64 format.

PUREIP:
~~~~~~~
After this patch set, when using PUREIP, kernel doesn't generate IPv6
link-local address regardless of which IN6_ADDR_GEN_MODE is used.

@@ static void addrconf_dev_config(struct net_device *dev)
+ if (dev->type == ARPHRD_PUREIP)
+ return;

And after recving RA message, kernel iterates over the link-local address
that exists for the interface and uses the low 64bits of the link-local
address to generate the IPv6 global address.
The general process is as follows:
ndisc_router_discovery() -> addrconf_prefix_rcv() -> ipv6_generate_eui64() -> ipv6_inherit_eui64()

> to be the same to me. If they are different, where is that documented?
>
> thanks,
>
> greg k-h

I tried to find corresponding documents about other device types, but I
am sorry I didn't find it. If it is needed, I am willing to provide.

Thanks,
Rocco

2021-06-24 05:21:03

by David Ahern

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On 6/23/21 9:33 PM, Rocco Yue wrote:
>
> The difference between RAWIP and PUREIP is that they generate IPv6
> link-local address and IPv6 global address in different ways.
>
> RAWIP:
> ~~~~~~
> In the ipv6_generate_eui64() function, using RAWIP will always return 0,
> which will cause the kernel to automatically generate an IPv6 link-local
> address in EUI64 format and an IPv6 global address in EUI64 format.
>
> PUREIP:
> ~~~~~~~
> After this patch set, when using PUREIP, kernel doesn't generate IPv6
> link-local address regardless of which IN6_ADDR_GEN_MODE is used.
>
> @@ static void addrconf_dev_config(struct net_device *dev)
> + if (dev->type == ARPHRD_PUREIP)
> + return;
>
> And after recving RA message, kernel iterates over the link-local address
> that exists for the interface and uses the low 64bits of the link-local
> address to generate the IPv6 global address.
> The general process is as follows:
> ndisc_router_discovery() -> addrconf_prefix_rcv() -> ipv6_generate_eui64() -> ipv6_inherit_eui64()
>

please add that to the commit message.

2021-06-24 05:31:51

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, Jun 24, 2021 at 11:33:53AM +0800, Rocco Yue wrote:
> On Wed, 2021-06-23 at 19:19 +0200, Greg KH wrote:
> On Wed, Jun 23, 2021 at 07:34:49PM +0800, Rocco Yue wrote:
> >> This patch add the definition of ARPHRD_PUREIP which can for
> >> example be used by mobile ccmni device as device type.
> >> ARPHRD_PUREIP means that this device doesn't need kernel to
> >> generate ipv6 link-local address in any addr_gen_mode.
> >>
> >> Signed-off-by: Rocco Yue <[email protected]>
> >> ---
> >> include/uapi/linux/if_arp.h | 1 +
> >> 1 file changed, 1 insertion(+)
> >>
> >> diff --git a/include/uapi/linux/if_arp.h b/include/uapi/linux/if_arp.h
> >> index c3cc5a9e5eaf..4463c9e9e8b4 100644
> >> --- a/include/uapi/linux/if_arp.h
> >> +++ b/include/uapi/linux/if_arp.h
> >> @@ -61,6 +61,7 @@
> >> #define ARPHRD_DDCMP 517 /* Digital's DDCMP protocol */
> >> #define ARPHRD_RAWHDLC 518 /* Raw HDLC */
> >> #define ARPHRD_RAWIP 519 /* Raw IP */
> >> +#define ARPHRD_PUREIP 520 /* Pure IP */
> >
> > In looking at the patches, what differs "PUREIP" from "RAWIP"? It seems
>
> Thanks for your review.
>
> The difference between RAWIP and PUREIP is that they generate IPv6
> link-local address and IPv6 global address in different ways.
>
> RAWIP:
> ~~~~~~
> In the ipv6_generate_eui64() function, using RAWIP will always return 0,
> which will cause the kernel to automatically generate an IPv6 link-local
> address in EUI64 format and an IPv6 global address in EUI64 format.
>
> PUREIP:
> ~~~~~~~
> After this patch set, when using PUREIP, kernel doesn't generate IPv6
> link-local address regardless of which IN6_ADDR_GEN_MODE is used.
>
> @@ static void addrconf_dev_config(struct net_device *dev)
> + if (dev->type == ARPHRD_PUREIP)
> + return;
>
> And after recving RA message, kernel iterates over the link-local address
> that exists for the interface and uses the low 64bits of the link-local
> address to generate the IPv6 global address.
> The general process is as follows:
> ndisc_router_discovery() -> addrconf_prefix_rcv() -> ipv6_generate_eui64() -> ipv6_inherit_eui64()

Thanks for the explaination, why is this hardware somehow "special" in
this way that this has never been needed before?

thanks,

greg k-h

2021-06-24 05:51:15

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Wed, 2021-06-23 at 23:15 -0600, David Ahern wrote:
On 6/23/21 9:33 PM, Rocco Yue wrote:
>>
>> The difference between RAWIP and PUREIP is that they generate IPv6
>> link-local address and IPv6 global address in different ways.
>>
>> RAWIP:
>> ~~~~~~
>> In the ipv6_generate_eui64() function, using RAWIP will always return 0,
>> which will cause the kernel to automatically generate an IPv6 link-local
>> address in EUI64 format and an IPv6 global address in EUI64 format.
>>
>> PUREIP:
>> ~~~~~~~
>> After this patch set, when using PUREIP, kernel doesn't generate IPv6
>> link-local address regardless of which IN6_ADDR_GEN_MODE is used.
>>
>> @@ static void addrconf_dev_config(struct net_device *dev)
>> + if (dev->type == ARPHRD_PUREIP)
>> + return;
>>
>> And after recving RA message, kernel iterates over the link-local address
>> that exists for the interface and uses the low 64bits of the link-local
>> address to generate the IPv6 global address.
>> The general process is as follows:
>> ndisc_router_discovery() -> addrconf_prefix_rcv() ->
>> ipv6_generate_eui64() -> ipv6_inherit_eui64()
>>
>
> please add that to the commit message.

Thanks for your reminding, will do.

Thanks,
Rocco

2021-06-24 06:30:16

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, 2021-06-24 at 07:29 +0200, Greg KH wrote:
>
> Thanks for the explaination, why is this hardware somehow "special" in
> this way that this has never been needed before?
>
> thanks,
>
> greg k-h
>

Before kernel-4.18, RAWIP was the same as PUREIP, neither of them
automatically generates an IPv6 link-local address, and the way to
generate an IPv6 global address is the same.

After kernel-4.18 (include 4.18 version), the behavior of RAWIP had
changed due to the following patch:
@@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
+ case ARPHRD_RAWIP:
+ return addrconf_ifid_rawip(eui, dev);
}
return -1;
}

the reason why the kernel doesn't need to generate the link-local
address automatically is as follows:

In the 3GPP 29.061, here is some description as follows:
"in order to avoid any conflict between the link-local address of
MS and that of the GGSN, the Interface-Identifier used by the MS to
build its link-local address shall be assigned by the GGSN. The GGSN
ensures the uniqueness of this Interface-Identifier. Then MT shall
then enforce the use of this Interface-Identifier by the TE"

In other words, in the cellular network, GGSN determines whether to
reply to the Router Solicitation message of UE by identifying the
low 64bits of UE interface's ipv6 link-local address.

When using a new kernel and RAWIP, kernel will generate an EUI64
format ipv6 link-local address, and if the device uses this address
to send RS, GGSN will not reply RA message.

Therefore, in that background, we came up with PUREIP to make kernel
doesn't generate a ipv6 link-local address in any address generate
mode.

Thanks,
Rocco

2021-06-24 09:06:07

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, Jun 24, 2021 at 02:13:10PM +0800, Rocco Yue wrote:
> On Thu, 2021-06-24 at 07:29 +0200, Greg KH wrote:
> >
> > Thanks for the explaination, why is this hardware somehow "special" in
> > this way that this has never been needed before?
> >
> > thanks,
> >
> > greg k-h
> >
>
> Before kernel-4.18, RAWIP was the same as PUREIP, neither of them
> automatically generates an IPv6 link-local address, and the way to
> generate an IPv6 global address is the same.
>
> After kernel-4.18 (include 4.18 version), the behavior of RAWIP had
> changed due to the following patch:
> @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
> + case ARPHRD_RAWIP:
> + return addrconf_ifid_rawip(eui, dev);
> }
> return -1;
> }
>
> the reason why the kernel doesn't need to generate the link-local
> address automatically is as follows:
>
> In the 3GPP 29.061, here is some description as follows:
> "in order to avoid any conflict between the link-local address of
> MS and that of the GGSN, the Interface-Identifier used by the MS to
> build its link-local address shall be assigned by the GGSN. The GGSN
> ensures the uniqueness of this Interface-Identifier. Then MT shall
> then enforce the use of this Interface-Identifier by the TE"
>
> In other words, in the cellular network, GGSN determines whether to
> reply to the Router Solicitation message of UE by identifying the
> low 64bits of UE interface's ipv6 link-local address.
>
> When using a new kernel and RAWIP, kernel will generate an EUI64
> format ipv6 link-local address, and if the device uses this address
> to send RS, GGSN will not reply RA message.
>
> Therefore, in that background, we came up with PUREIP to make kernel
> doesn't generate a ipv6 link-local address in any address generate
> mode.

Thanks for the better description. That should go into the changelog
text somewhere so that others know what is going on here with this new
option.

And are these user-visable flags documented in a man page or something
else somewhere? If not, how does userspace know about them?

thanks,

greg k-h

2021-06-24 12:10:26

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Wed, 2021-06-23 at 19:31 +0200, Greg KH wrote:
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.
>

Thanks for your review.
Looking back at this code block, it does have some ploblems,
ccmni->usage hasn't been used to protect some resources or do
some specific things in the current code, I will delete them.

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

The jedgment of ccmni->usage here is to ensure that the ccmnix interface
is already up when do wake up tx queue behavior.

Then I re-read the kernel code, I think my previous ider should be
wrong. the reason is that before calling ccmni_tx_timeout(), it
will check whether the dev exist or not, for example, it will be
checked in dev_watchdog().

I can delete this code.

>> +/* 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.
>

without MTK ap ccci driver (modem driver), ccmni_rx_push() and
ccmni_hif_hook() are not be used.

Both of them are exported as symbols because MTK ap ccci driver
will be complied to the ccci.ko file.

In current codes, I implementated the basic functionality of ccmni,
such as open, close, xmit packet, rcv packet. And my original
intention was that I can gradually complete some of the more
functions of ccmni on this basis, such as sw-gro, napi, or meet the
requirement of high throughput performance.

In addition, the code of MTK's modem driver is a bit complicated,
because this part has more than 30,000 lines of code and contains
more than 10 modules. We are completeing the upload of this huge
code step by step. Our original intention was to upload the ccmni
driver that directly interacts with the kernel first, and then
complete the code from ccmni to the bottom layer one by one from
top to bottom. We expect the completion period to be about 1 year.

> +++ b/drivers/net/ethernet/mediatek/ccmni/ccmni.h
>
> Why do you have a .h file for a single .c file? that shouldn't be
> needed.

I add a .h file to facilitate subsequent code expansion. If it's
not appropriate to do this here, I can add the content of .h into
.c file.

Thanks,
Rocco

2021-06-24 12:25:47

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, Jun 24, 2021 at 07:53:49PM +0800, Rocco Yue wrote:
> >> +/* 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.
> >
>
> without MTK ap ccci driver (modem driver), ccmni_rx_push() and
> ccmni_hif_hook() are not be used.
>
> Both of them are exported as symbols because MTK ap ccci driver
> will be complied to the ccci.ko file.

But I do not see any code in this series that use these symbols. We can
not have exports that no one uses. Please add the driver to this patch
series when you resend it.

> In addition, the code of MTK's modem driver is a bit complicated,
> because this part has more than 30,000 lines of code and contains
> more than 10 modules. We are completeing the upload of this huge
> code step by step. Our original intention was to upload the ccmni
> driver that directly interacts with the kernel first, and then
> complete the code from ccmni to the bottom layer one by one from
> top to bottom. We expect the completion period to be about 1 year.

Again, we can not add code to the kernel that is not used, sorry. That
would not make any sense, would you want to maintain such a thing?

And 30k of code seems a bit excesive for a modem driver. Vendors find
that when they submit code for inclusion in the kernel tree, in the end,
they end up 1/3 the original size, so 10k is reasonable.

I can also take any drivers today into the drivers/staging/ tree, and
you can do the cleanups there as well as getting help from others.

1 year seems like a long time to do "cleanup", good luck!

> > +++ b/drivers/net/ethernet/mediatek/ccmni/ccmni.h
> >
> > Why do you have a .h file for a single .c file? that shouldn't be
> > needed.
>
> I add a .h file to facilitate subsequent code expansion. If it's
> not appropriate to do this here, I can add the content of .h into
> .c file.

If nothing other than a single .c file needs it, put it into that .c
file please.

thanks,

greg k-h

2021-06-24 12:40:56

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, 2021-06-24 at 11:04 +0200, Greg KH wrote:
On Thu, Jun 24, 2021 at 02:13:10PM +0800, Rocco Yue wrote:
>> On Thu, 2021-06-24 at 07:29 +0200, Greg KH wrote:
>>>
>>> Thanks for the explaination, why is this hardware somehow "special" in
>>> this way that this has never been needed before?
>>>
>>> thanks,
>>>
>>> greg k-h
>>>
>>
>> Before kernel-4.18, RAWIP was the same as PUREIP, neither of them
>> automatically generates an IPv6 link-local address, and the way to
>> generate an IPv6 global address is the same.
>>
>> After kernel-4.18 (include 4.18 version), the behavior of RAWIP had
>> changed due to the following patch:
>> @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
>> + case ARPHRD_RAWIP:
>> + return addrconf_ifid_rawip(eui, dev);
>> }
>> return -1;
>> }
>>
>> the reason why the kernel doesn't need to generate the link-local
>> address automatically is as follows:
>>
>> In the 3GPP 29.061, here is some description as follows:
>> "in order to avoid any conflict between the link-local address of
>> MS and that of the GGSN, the Interface-Identifier used by the MS to
>> build its link-local address shall be assigned by the GGSN. The GGSN
>> ensures the uniqueness of this Interface-Identifier. Then MT shall
>> then enforce the use of this Interface-Identifier by the TE"
>>
>> In other words, in the cellular network, GGSN determines whether to
>> reply to the Router Solicitation message of UE by identifying the
>> low 64bits of UE interface's ipv6 link-local address.
>>
>> When using a new kernel and RAWIP, kernel will generate an EUI64
>> format ipv6 link-local address, and if the device uses this address
>> to send RS, GGSN will not reply RA message.
>>
>> Therefore, in that background, we came up with PUREIP to make kernel
>> doesn't generate a ipv6 link-local address in any address generate
>> mode.
>
> Thanks for the better description. That should go into the changelog
> text somewhere so that others know what is going on here with this new
> option.
>

Does changelog mean adding these details to the commit message ?
I am willing do it.

> And are these user-visable flags documented in a man page or something
> else somewhere? If not, how does userspace know about them?
>

There are mappings of these device types value in the libc:
"/bionic/libc/kernel/uapi/linux/if_arp.h".
userspace can get it from here.

But I also failed to find a man page or a description of these
device types.

Thanks,
Rocco

2021-06-24 13:08:01

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, Jun 24, 2021 at 08:24:35PM +0800, Rocco Yue wrote:
> On Thu, 2021-06-24 at 11:04 +0200, Greg KH wrote:
> On Thu, Jun 24, 2021 at 02:13:10PM +0800, Rocco Yue wrote:
> >> On Thu, 2021-06-24 at 07:29 +0200, Greg KH wrote:
> >>>
> >>> Thanks for the explaination, why is this hardware somehow "special" in
> >>> this way that this has never been needed before?
> >>>
> >>> thanks,
> >>>
> >>> greg k-h
> >>>
> >>
> >> Before kernel-4.18, RAWIP was the same as PUREIP, neither of them
> >> automatically generates an IPv6 link-local address, and the way to
> >> generate an IPv6 global address is the same.
> >>
> >> After kernel-4.18 (include 4.18 version), the behavior of RAWIP had
> >> changed due to the following patch:
> >> @@ static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
> >> + case ARPHRD_RAWIP:
> >> + return addrconf_ifid_rawip(eui, dev);
> >> }
> >> return -1;
> >> }
> >>
> >> the reason why the kernel doesn't need to generate the link-local
> >> address automatically is as follows:
> >>
> >> In the 3GPP 29.061, here is some description as follows:
> >> "in order to avoid any conflict between the link-local address of
> >> MS and that of the GGSN, the Interface-Identifier used by the MS to
> >> build its link-local address shall be assigned by the GGSN. The GGSN
> >> ensures the uniqueness of this Interface-Identifier. Then MT shall
> >> then enforce the use of this Interface-Identifier by the TE"
> >>
> >> In other words, in the cellular network, GGSN determines whether to
> >> reply to the Router Solicitation message of UE by identifying the
> >> low 64bits of UE interface's ipv6 link-local address.
> >>
> >> When using a new kernel and RAWIP, kernel will generate an EUI64
> >> format ipv6 link-local address, and if the device uses this address
> >> to send RS, GGSN will not reply RA message.
> >>
> >> Therefore, in that background, we came up with PUREIP to make kernel
> >> doesn't generate a ipv6 link-local address in any address generate
> >> mode.
> >
> > Thanks for the better description. That should go into the changelog
> > text somewhere so that others know what is going on here with this new
> > option.
> >
>
> Does changelog mean adding these details to the commit message ?

Yes please.

> > And are these user-visable flags documented in a man page or something
> > else somewhere? If not, how does userspace know about them?
> >
>
> There are mappings of these device types value in the libc:
> "/bionic/libc/kernel/uapi/linux/if_arp.h".
> userspace can get it from here.

Yes, they will show up in a libc definition, but where is it documented
in text form what the flag does?

thanks,

greg k-h

2021-06-24 16:14:13

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

On Thu, 2021-06-24 at 14:23 +0200, Greg KH wrote:
On Thu, Jun 24, 2021 at 07:53:49PM +0800, Rocco Yue wrote:
>>
>> without MTK ap ccci driver (modem driver), ccmni_rx_push() and
>> ccmni_hif_hook() are not be used.
>>
>> Both of them are exported as symbols because MTK ap ccci driver
>> will be compiled to the ccci.ko file.
>
> But I do not see any code in this series that use these symbols. We can

will delete these symbols.

> not have exports that no one uses. Please add the driver to this patch
> series when you resend it.
>

I've just took a look at what the Linux staging tree is. It looks like
a good choice for the current ccmni driver.

honstly, If I simply upload the relevant driver code B that calls
A (e.g. ccmni_rx_push), there is still a lack of code to call B.
This seems to be a continuty problem, unless all drivers codes are
uploaded (e.g. power on modem, get hardware status, complete tx/rx flow).

>> In addition, the code of MTK's modem driver is a bit complicated,
>> because this part has more than 30,000 lines of code and contains
>> more than 10 modules. We are completeing the upload of this huge
>> code step by step. Our original intention was to upload the ccmni
>> driver that directly interacts with the kernel first, and then
>> complete the code from ccmni to the bottom layer one by one from
>> top to bottom. We expect the completion period to be about 1 year.
>
> Again, we can not add code to the kernel that is not used, sorry. That
> would not make any sense, would you want to maintain such a thing?
>
> And 30k of code seems a bit excesive for a modem driver. Vendors find
> that when they submit code for inclusion in the kernel tree, in the end,
> they end up 1/3 the original size, so 10k is reasonable.
>
> I can also take any drivers today into the drivers/staging/ tree, and
> you can do the cleanups there as well as getting help from others.
>
> 1 year seems like a long time to do "cleanup", good luck!
>

Thanks~

Can I resend patch set as follows:
(1) supplement the details of pureip for patch 1/4;
(2) the document of ccmni.rst still live in the Documentation/...
(3) modify ccmni and move it into the drivers/staging/...

>>> +++ b/drivers/net/ethernet/mediatek/ccmni/ccmni.h
>>>
>>> Why do you have a .h file for a single .c file? that shouldn't be
>>> needed.
>>
>> I add a .h file to facilitate subsequent code expansion. If it's
>> not appropriate to do this here, I can add the content of .h into
>> .c file.
>
> If nothing other than a single .c file needs it, put it into that .c
> file please.

will do.

Thanks,
Rocco

2021-06-24 16:17:17

by Dan Williams

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, 2021-06-24 at 14:13 +0800, Rocco Yue wrote:
> On Thu, 2021-06-24 at 07:29 +0200, Greg KH wrote:
> >
> > Thanks for the explaination, why is this hardware somehow "special"
> > in
> > this way that this has never been needed before?
> >
> > thanks,
> >
> > greg k-h
> >
>
> Before kernel-4.18, RAWIP was the same as PUREIP, neither of them
> automatically generates an IPv6 link-local address, and the way to
> generate an IPv6 global address is the same.

This distinction seems confusing from a kernel standpoint if it only
changes how v6 IIDs are determined. Do we really need something that's
also reflected to userspace (in struct ifinfomsg -> ifi_type) if the
kernel is handling the behavior that's different? Why should userspace
care?

I'm also curious why this isn't an issue for the ipa/rmnet (Qualcomm)
modem drivers. There's probably a good reason, but would be good to
know what that is from Alex Elder or Loic or Bjorn...

Dan

>
> After kernel-4.18 (include 4.18 version), the behavior of RAWIP had
> changed due to the following patch:
> @@  static int ipv6_generate_eui64(u8 *eui, struct net_device *dev)
> +       case ARPHRD_RAWIP:
> +               return addrconf_ifid_rawip(eui, dev);
>         }
>         return -1;
> }
>
> the reason why the kernel doesn't need to generate the link-local
> address automatically is as follows:
>
> In the 3GPP 29.061, here is some description as follows:
> "in order to avoid any conflict between the link-local address of
> MS and that of the GGSN, the Interface-Identifier used by the MS to
> build its link-local address shall be assigned by the GGSN. The GGSN
> ensures the uniqueness of this Interface-Identifier. Then MT shall
> then enforce the use of this Interface-Identifier by the TE"
>
> In other words, in the cellular network, GGSN determines whether to
> reply to the Router Solicitation message of UE by identifying the
> low 64bits of UE interface's ipv6 link-local address.
>
> When using a new kernel and RAWIP, kernel will generate an EUI64
> format ipv6 link-local address, and if the device uses this address
> to send RS, GGSN will not reply RA message.
>
> Therefore, in that background, we came up with PUREIP to make kernel
> doesn't generate a ipv6 link-local address in any address generate
> mode.
>
> Thanks,
> Rocco
>


2021-06-24 16:52:39

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

On Thu, Jun 24, 2021 at 11:55:02PM +0800, Rocco Yue wrote:
> On Thu, 2021-06-24 at 14:23 +0200, Greg KH wrote:
> On Thu, Jun 24, 2021 at 07:53:49PM +0800, Rocco Yue wrote:
> >>
> >> without MTK ap ccci driver (modem driver), ccmni_rx_push() and
> >> ccmni_hif_hook() are not be used.
> >>
> >> Both of them are exported as symbols because MTK ap ccci driver
> >> will be compiled to the ccci.ko file.
> >
> > But I do not see any code in this series that use these symbols. We can
>
> will delete these symbols.
>
> > not have exports that no one uses. Please add the driver to this patch
> > series when you resend it.
> >
>
> I've just took a look at what the Linux staging tree is. It looks like
> a good choice for the current ccmni driver.
>
> honstly, If I simply upload the relevant driver code B that calls
> A (e.g. ccmni_rx_push), there is still a lack of code to call B.
> This seems to be a continuty problem, unless all drivers codes are
> uploaded (e.g. power on modem, get hardware status, complete tx/rx flow).

Great, send it all! Why is it different modules, it's only for one
chunk of hardware, no need to split it up into tiny pieces. That way
only causes it to be more code overall.

> >> In addition, the code of MTK's modem driver is a bit complicated,
> >> because this part has more than 30,000 lines of code and contains
> >> more than 10 modules. We are completeing the upload of this huge
> >> code step by step. Our original intention was to upload the ccmni
> >> driver that directly interacts with the kernel first, and then
> >> complete the code from ccmni to the bottom layer one by one from
> >> top to bottom. We expect the completion period to be about 1 year.
> >
> > Again, we can not add code to the kernel that is not used, sorry. That
> > would not make any sense, would you want to maintain such a thing?
> >
> > And 30k of code seems a bit excesive for a modem driver. Vendors find
> > that when they submit code for inclusion in the kernel tree, in the end,
> > they end up 1/3 the original size, so 10k is reasonable.
> >
> > I can also take any drivers today into the drivers/staging/ tree, and
> > you can do the cleanups there as well as getting help from others.
> >
> > 1 year seems like a long time to do "cleanup", good luck!
> >
>
> Thanks~
>
> Can I resend patch set as follows:
> (1) supplement the details of pureip for patch 1/4;
> (2) the document of ccmni.rst still live in the Documentation/...
> (3) modify ccmni and move it into the drivers/staging/...

for drivers/staging/ the code needs to be "self contained" in that it
does not require adding anything outside of the directory for it.

If you still require this core networking change, that needs to be
accepted first by the networking developers and maintainers.

thanks,

greg k-h

2021-06-25 06:18:07

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

> Does changelog mean adding these details to the commit message ?
>
> Yes please.
>

will do.

> > > And are these user-visable flags documented in a man page or something
> > > else somewhere? If not, how does userspace know about them?
> > >
> >
> > There are mappings of these device types value in the libc:
> > "/bionic/libc/kernel/uapi/linux/if_arp.h".
> > userspace can get it from here.
>
> Yes, they will show up in a libc definition, but where is it documented
> in text form what the flag does?

Judging from the changes of ARPHRD_xxx submitted before, I am sorry
I could not find the corresponding doucuments to describe their
respective behaviors in details. Perhaps the best way to understand
their behaviors is to read the code directly.

Thanks,
Rocco

2021-06-25 06:21:52

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 1/4] net: if_arp: add ARPHRD_PUREIP type

On Thu, 2021-06-24 at 11:14 -0500, Dan Williams wrote:
On Thu, 2021-06-24 at 14:13 +0800, Rocco Yue wrote:
>> On Thu, 2021-06-24 at 07:29 +0200, Greg KH wrote:
>>
>> Before kernel-4.18, RAWIP was the same as PUREIP, neither of them
>> automatically generates an IPv6 link-local address, and the way to
>> generate an IPv6 global address is the same.
>
> This distinction seems confusing from a kernel standpoint if it only
> changes how v6 IIDs are determined. Do we really need something that's

Hi Dan,

Thanks for your comment,

In the cellular network, v6 IID is important, If the device use the
link-local address formed by the incorrect IID to send RS message to
the network, based on 3GPP, GGSN will not reply solicited RA message.
It will lead to the device can get ipv6 address prefix and ipv6 route.

Maybe the table below is a little bit clearer

three device type: ARPHRD_RAWIP , ARPHRD_PUREIP, ARPHRD_NONE
three mode: IN6_ADDR_GEN_MODE_EUI64 , IN6_ADDR_GEN_MODE_NONE, IN6_ADDR_GEN_MODE_STABLE_PRIVACY

ipv6 link-local address generate behavior in the kernel:
+---------+-------------------+---------------------+----------------+
| | MODE_EUI64 | MODE_STABLE_PRIVACY | MODE_NONE |
+---------+-------------------+---------------------+----------------+
| RAWIP | fe80::(eui64-id) | fe80::(privacy-id) | no address gen |
+---------+-------------------+---------------------+----------------+
| PUREIP | no address gen | no address gen | no address gen |
+---------+-------------------+---------------------+----------------+
| NONE | fe80::(random-id) | fe80::(privacy-id) | no address gen |
+---------+-------------------+---------------------+----------------+

ipv6 global address generate behavior in the kernel:
+---------+-------------------+---------------------+-------------------+
| | MODE_EUI64 | MODE_STABLE_PRIVACY | MODE_NONE |
+---------+-------------------+---------------------+-------------------+
| RAWIP | prefix+(eui64-id) | prefix+(privacy-id) | prefix+(eui64-id) |
+---------+-------------------+---------------------+-------------------+
| PUREIP | prefix+(GGSN-id) | prefix+(privacy-id) | prefix+(GGSN-id) |
+---------+-------------------+---------------------+-------------------+
| NONE | prefix+(random-id)| prefix+(privacy-id) | prefix+(random-id)|
+---------+-------------------+---------------------+-------------------+

> also reflected to userspace (in struct ifinfomsg -> ifi_type) if the
> kernel is handling the behavior that's different? Why should userspace
> care?
>

In my opinion, userspace program cares about it because the kernel behaves
differently for different device types.
userspace can get the device type of the interface through ioctl, such as
the following code weblink:
https://cs.android.com/android/platform/superproject/+/master:system/netd/server/OffloadUtils.cpp;drc=master;l=41?q=ARPHRD_RAWIP&ss=android%2Fplatform%2Fsuperproject&start=11

> I'm also curious why this isn't an issue for the ipa/rmnet (Qualcomm)
> modem drivers. There's probably a good reason, but would be good to
> know what that is from Alex Elder or Loic or Bjorn...
>
> Dan

MediaTek and Qualcomm has different hardware or modem design.
For the MediaTek platform, device send the RS message that generated by
the kernel to the GGSN.

Thanks,
Rocco

2021-06-28 07:35:49

by Rocco Yue

[permalink] [raw]
Subject: Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

On Thu, 2021-06-24 at 18:51 +0200, Greg KH wrote:
On Thu, Jun 24, 2021 at 11:55:02PM +0800, Rocco Yue wrote:
>> On Thu, 2021-06-24 at 14:23 +0200, Greg KH wrote:
>> On Thu, Jun 24, 2021 at 07:53:49PM +0800, Rocco Yue wrote:
>>>
>>> not have exports that no one uses. Please add the driver to this patch
>>> series when you resend it.
>>>
>>
>> I've just took a look at what the Linux staging tree is. It looks like
>> a good choice for the current ccmni driver.
>>
>> honstly, If I simply upload the relevant driver code B that calls
>> A (e.g. ccmni_rx_push), there is still a lack of code to call B.
>> This seems to be a continuty problem, unless all drivers codes are
>> uploaded (e.g. power on modem, get hardware status, complete tx/rx flow).
>
> Great, send it all! Why is it different modules, it's only for one
> chunk of hardware, no need to split it up into tiny pieces. That way
> only causes it to be more code overall.
>
>>
>> Thanks~
>>
>> Can I resend patch set as follows:
>> (1) supplement the details of pureip for patch 1/4;
>> (2) the document of ccmni.rst still live in the Documentation/...
>> (3) modify ccmni and move it into the drivers/staging/...
>
> for drivers/staging/ the code needs to be "self contained" in that it
> does not require adding anything outside of the directory for it.
>
> If you still require this core networking change, that needs to be
> accepted first by the networking developers and maintainers.
>
> thanks,
>
> greg k-h
>

Hi Greg,

I am grateful for your help.

Both ccmni change and networking changes are needed, because as far
as I know, usually a device type should have at least one device to
use it, and pureip is what the ccmni driver needs, so I uploaded the
networking change and ccmni driver together;

Since MTK’s modem driver has a large amount of code and strong code
coupling, it takes some time to clean up them. At this stage, it may
be difficult to upstream all the codes together.

During this period, even if ccmni is incomplete, can I put the ccmni
driver initial code in the driver/staging first ? After that, we will
gradually implement more functions of ccmni in the staging tree, and
we can also gradually sort out and clean up modem driver in the staging.

In addition, due to the requirements of GKI 2.0, if ccmni device
uses RAWIP or NONE, it will hit ipv6 issue; and if ccmni uses
a device type other than PUREIP/RAWIP/NONE, there will be tethering
ebpf offload or clat ebpf offload can not work problems.

I hope PUREIP and ccmni can be accepted by the Linux community.

Thanks,
Rocco

2021-06-28 09:34:31

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH 4/4] drivers: net: mediatek: initial implementation of ccmni

On Mon, Jun 28, 2021 at 03:18:30PM +0800, Rocco Yue wrote:
> On Thu, 2021-06-24 at 18:51 +0200, Greg KH wrote:
> On Thu, Jun 24, 2021 at 11:55:02PM +0800, Rocco Yue wrote:
> >> On Thu, 2021-06-24 at 14:23 +0200, Greg KH wrote:
> >> On Thu, Jun 24, 2021 at 07:53:49PM +0800, Rocco Yue wrote:
> >>>
> >>> not have exports that no one uses. Please add the driver to this patch
> >>> series when you resend it.
> >>>
> >>
> >> I've just took a look at what the Linux staging tree is. It looks like
> >> a good choice for the current ccmni driver.
> >>
> >> honstly, If I simply upload the relevant driver code B that calls
> >> A (e.g. ccmni_rx_push), there is still a lack of code to call B.
> >> This seems to be a continuty problem, unless all drivers codes are
> >> uploaded (e.g. power on modem, get hardware status, complete tx/rx flow).
> >
> > Great, send it all! Why is it different modules, it's only for one
> > chunk of hardware, no need to split it up into tiny pieces. That way
> > only causes it to be more code overall.
> >
> >>
> >> Thanks~
> >>
> >> Can I resend patch set as follows:
> >> (1) supplement the details of pureip for patch 1/4;
> >> (2) the document of ccmni.rst still live in the Documentation/...
> >> (3) modify ccmni and move it into the drivers/staging/...
> >
> > for drivers/staging/ the code needs to be "self contained" in that it
> > does not require adding anything outside of the directory for it.
> >
> > If you still require this core networking change, that needs to be
> > accepted first by the networking developers and maintainers.
> >
> > thanks,
> >
> > greg k-h
> >
>
> Hi Greg,
>
> I am grateful for your help.
>
> Both ccmni change and networking changes are needed, because as far
> as I know, usually a device type should have at least one device to
> use it, and pureip is what the ccmni driver needs, so I uploaded the
> networking change and ccmni driver together;
>
> Since MTK’s modem driver has a large amount of code and strong code
> coupling, it takes some time to clean up them. At this stage, it may
> be difficult to upstream all the codes together.

Why? Just dump the whole thing in a drivers/staging/mtk/ directory
structure and all should be fine.

> During this period, even if ccmni is incomplete, can I put the ccmni
> driver initial code in the driver/staging first ? After that, we will
> gradually implement more functions of ccmni in the staging tree, and
> we can also gradually sort out and clean up modem driver in the staging.

I do not know, let's see the code first. But we can not add frameworks
with no in-kernel users, as that does not make any sense at all.

> In addition, due to the requirements of GKI 2.0,

That is a Google requirement, not a kernel.org requirement. Please work
with Google if you have questions/issues about that, there is NOTHING we
can do about that here in the community for obvious reasons.

> if ccmni device
> uses RAWIP or NONE, it will hit ipv6 issue; and if ccmni uses
> a device type other than PUREIP/RAWIP/NONE, there will be tethering
> ebpf offload or clat ebpf offload can not work problems.
>
> I hope PUREIP and ccmni can be accepted by the Linux community.

As I stated before you need to have an in-kernel user for us to be able
to accept frameworks and functions into the tree. Otherwise Linux would
quickly become unmanagable and unmaintainable. Would you want to try to
maintain code with no in-tree users? What would you do if you were in
our position?

But for networking flags like this that go into userspace, I do not know
what the maintainers of the networking stack require, so that really is
up to them, not me.

thanks,

greg k-h