Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b5cfa726-b61e-90eb-7d4b-d81844189cf6@quicinc.com>
Date:   Wed, 7 Jun 2023 10:52:54 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.6.0
Subject: Re: [PATCH v2 1/2] net: Add MHI Endpoint network driver
Content-Language: en-US
From:   Jeffrey Hugo <quic_jhugo@quicinc.com>
To:     Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>,
        <pabeni@redhat.com>
CC:     <mhi@lists.linux.dev>, <linux-arm-msm@vger.kernel.org>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <loic.poulain@linaro.org>
References: <20230607152427.108607-1-manivannan.sadhasivam@linaro.org>
 <20230607152427.108607-2-manivannan.sadhasivam@linaro.org>
 <26a85bae-1a33-dd1f-5e73-0ab6da100abf@quicinc.com>
In-Reply-To: <26a85bae-1a33-dd1f-5e73-0ab6da100abf@quicinc.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 6/7/2023 10:27 AM, Jeffrey Hugo wrote:
> On 6/7/2023 9:24 AM, Manivannan Sadhasivam wrote:
>> Add a network driver for the Modem Host Interface (MHI) endpoint devices
>> that provides network interfaces to the PCIe based Qualcomm endpoint
>> devices supporting MHI bus. This driver allows the MHI endpoint 
>> devices to
>> establish IP communication with the host machines (x86, ARM64) over MHI
>> bus.
>>
>> The driver currently supports only IP_SW0 MHI channel that can be used
>> to route IP traffic from the endpoint CPU to host machine.
>>
>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> ---
>>   drivers/net/Kconfig      |   9 ++
>>   drivers/net/Makefile     |   1 +
>>   drivers/net/mhi_ep_net.c | 331 +++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 341 insertions(+)
>>   create mode 100644 drivers/net/mhi_ep_net.c
>>
>> diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
>> index 368c6f5b327e..36b628e2e49f 100644
>> --- a/drivers/net/Kconfig
>> +++ b/drivers/net/Kconfig
>> @@ -452,6 +452,15 @@ config MHI_NET
>>         QCOM based WWAN modems for IP or QMAP/rmnet protocol (like 
>> SDX55).
>>         Say Y or M.
>> +config MHI_EP_NET
>> +    tristate "MHI Endpoint network driver"
>> +    depends on MHI_BUS_EP
>> +    help
>> +      This is the network driver for MHI bus implementation in endpoint
>> +      devices. It is used provide the network interface for QCOM 
>> endpoint
>> +      devices such as SDX55 modems.
>> +      Say Y or M.
> 
> What will the module be called if "m" is selected?
> 
>> +
>>   endif # NET_CORE
>>   config SUNGEM_PHY
>> diff --git a/drivers/net/Makefile b/drivers/net/Makefile
>> index e26f98f897c5..b8e706a4150e 100644
>> --- a/drivers/net/Makefile
>> +++ b/drivers/net/Makefile
>> @@ -40,6 +40,7 @@ obj-$(CONFIG_NLMON) += nlmon.o
>>   obj-$(CONFIG_NET_VRF) += vrf.o
>>   obj-$(CONFIG_VSOCKMON) += vsockmon.o
>>   obj-$(CONFIG_MHI_NET) += mhi_net.o
>> +obj-$(CONFIG_MHI_EP_NET) += mhi_ep_net.o
>>   #
>>   # Networking Drivers
>> diff --git a/drivers/net/mhi_ep_net.c b/drivers/net/mhi_ep_net.c
>> new file mode 100644
>> index 000000000000..0d7939caefc7
>> --- /dev/null
>> +++ b/drivers/net/mhi_ep_net.c
>> @@ -0,0 +1,331 @@
>> +// SPDX-License-Identifier: GPL-2.0-or-later
>> +/*
>> + * MHI Endpoint Network driver
>> + *
>> + * Based on drivers/net/mhi_net.c
>> + *
>> + * Copyright (c) 2023, Linaro Ltd.
>> + * Author: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>> + */
>> +
>> +#include <linux/if_arp.h>
>> +#include <linux/mhi_ep.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/module.h>
>> +#include <linux/netdevice.h>
>> +#include <linux/skbuff.h>
>> +#include <linux/u64_stats_sync.h>
>> +
>> +#define MHI_NET_MIN_MTU        ETH_MIN_MTU
>> +#define MHI_NET_MAX_MTU        0xffff

ETH_MAX_MTU ?

Personal preference thing.  If you think 0xffff is really the superior 
option, so be it.  Personally, it takes me a second to figure out that 
is 64k - 1 and then relate it to the MHI packet size limit.  Also seems 
really odd with this line of code right next to, and related to, 
ETH_MIN_MTU.  Feels like a non-magic number here will make things more 
maintainable.

Alternatively move MHI_MAX_MTU out of host/internal.h into something 
that is convenient for this driver to include and use?  It is a 
fundamental constant for the MHI protocol, we just haven't yet had a 
need for it to be used outside of the MHI bus implementation code.

>> +
>> +struct mhi_ep_net_stats {
>> +    u64_stats_t rx_packets;
>> +    u64_stats_t rx_bytes;
>> +    u64_stats_t rx_errors;
>> +    u64_stats_t tx_packets;
>> +    u64_stats_t tx_bytes;
>> +    u64_stats_t tx_errors;
>> +    u64_stats_t tx_dropped;
>> +    struct u64_stats_sync tx_syncp;
>> +    struct u64_stats_sync rx_syncp;
>> +};
>> +
>> +struct mhi_ep_net_dev {
>> +    struct mhi_ep_device *mdev;
>> +    struct net_device *ndev;
>> +    struct mhi_ep_net_stats stats;
>> +    struct workqueue_struct *xmit_wq;
>> +    struct work_struct xmit_work;
>> +    struct sk_buff_head tx_buffers;
>> +    spinlock_t tx_lock; /* Lock for protecting tx_buffers */
>> +    u32 mru;
>> +};
>> +
>> +static void mhi_ep_net_dev_process_queue_packets(struct work_struct 
>> *work)
>> +{
>> +    struct mhi_ep_net_dev *mhi_ep_netdev = container_of(work,
>> +            struct mhi_ep_net_dev, xmit_work);
> 
> Looks like this can fit all on one line to me.
> 
>