Re: [PATCH v2 1/2] net: Add MHI Endpoint network driver

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Jun 07, 2023 at 10:52:54AM -0600, Jeffrey Hugo wrote:
> 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.
> 

TBH, I tried to stick to mhi_net, so just carried this macro. And I agree, it
could be made more understandeable.

> 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.
> 

At some point I had a patch for moving this macro to mhi.h but it fell into
cracks... Will bring it back.

- Mani

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

-- 
மணிவண்ணன் சதாசிவம்