| Message ID | 20230607152427.108607-1-manivannan.sadhasivam@linaro.org |
|---|---|
| Headers | show |
| Series | Add MHI Endpoint network driver | expand |
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 > + > +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.
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. > >
On Wed, Jun 07, 2023 at 09:49:22AM -0700, Jakub Kicinski wrote: > On Wed, 7 Jun 2023 20:54:25 +0530 Manivannan Sadhasivam wrote: > > This series adds 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 (like Modems). This driver allows the MHI endpoint > > devices to establish IP communication with the host machines (x86, ARM64) over > > MHI bus. > > > > On the host side, the existing mhi_net driver provides the network connectivity > > to the host. > > Why are you posting the next version before the discussion on the > previous one concluded? :| > Previous discussion doesn't sound any controversial to me, so I thought of respinning. Maybe I should've waited... > In any case, I'm opposed to reuse of the networking stack to talk > to firmware. It's a local device. The networking subsystem doesn't > have to cater to fake networks. Please carry: > > Nacked-by: Jakub Kicinski <kuba@kernel.org> > > if there are future submissions. Why shouldn't it be? With this kind of setup one could share the data connectivity available in the device with the host over IP tunneling. If the IP source in the device (like modem DSP) has no way to be shared with the host, then those IP packets could be tunneled through this interface for providing connectivity to the host. I believe this is a common usecase among the PCIe based wireless endpoint devices. - Mani
On Wed, 7 Jun 2023 22:41:53 +0530 Manivannan Sadhasivam wrote: > > In any case, I'm opposed to reuse of the networking stack to talk > > to firmware. It's a local device. The networking subsystem doesn't > > have to cater to fake networks. Please carry: > > > > Nacked-by: Jakub Kicinski <kuba@kernel.org> > > > > if there are future submissions. > > Why shouldn't it be? With this kind of setup one could share the data connectivity > available in the device with the host over IP tunneling. If the IP source in the > device (like modem DSP) has no way to be shared with the host, then those IP > packets could be tunneled through this interface for providing connectivity to > the host. > > I believe this is a common usecase among the PCIe based wireless endpoint > devices. We can handwave our way into many scenarios and terrible architectures. I don't see any compelling reason to merge this.
On Wed, Jun 07, 2023 at 10:43:50AM -0700, Jakub Kicinski wrote: > On Wed, 7 Jun 2023 22:41:53 +0530 Manivannan Sadhasivam wrote: > > > In any case, I'm opposed to reuse of the networking stack to talk > > > to firmware. It's a local device. The networking subsystem doesn't > > > have to cater to fake networks. Please carry: > > > > > > Nacked-by: Jakub Kicinski <kuba@kernel.org> > > > > > > if there are future submissions. > > > > Why shouldn't it be? With this kind of setup one could share the data connectivity > > available in the device with the host over IP tunneling. If the IP source in the > > device (like modem DSP) has no way to be shared with the host, then those IP > > packets could be tunneled through this interface for providing connectivity to > > the host. > > > > I believe this is a common usecase among the PCIe based wireless endpoint > > devices. > > We can handwave our way into many scenarios and terrible architectures. > I don't see any compelling reason to merge this. These kind of usecases exist in the products out there in the market. Regarding your comment on "opposed to reuse of the network stack to talk to firmware", it not the just the firmware, it is the device in general that is talking to the host over this interface. And I don't see how different it is from the host perspective. And these kind of scenarios exist with all types of interfaces like usb-gadget, virtio etc... So not sure why the rule is different for networking subsystem. - Mani
On Wed, Jun 07, 2023 at 08:13:32PM +0200, Andrew Lunn wrote: > On Wed, Jun 07, 2023 at 09:49:22AM -0700, Jakub Kicinski wrote: > > On Wed, 7 Jun 2023 20:54:25 +0530 Manivannan Sadhasivam wrote: > > > This series adds 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 (like Modems). This driver allows the MHI endpoint > > > devices to establish IP communication with the host machines (x86, ARM64) over > > > MHI bus. > > > > > > On the host side, the existing mhi_net driver provides the network connectivity > > > to the host. > > > > Why are you posting the next version before the discussion on the > > previous one concluded? :| > > > > In any case, I'm opposed to reuse of the networking stack to talk > > to firmware. It's a local device. The networking subsystem doesn't > > have to cater to fake networks. Please carry: > > > > Nacked-by: Jakub Kicinski <kuba@kernel.org> > > Remote Processor Messaging (rpmsg) Framework does seem to be what is > supposed to be used for these sorts of situations. Not that i know > much about it. > Rpmsg is another messaging protocol used for talking to the remote processor. MHI is somewhat similar in terms of usecase but it is a proprietary protocol used by Qcom for their devices. - Mani > Andrew
Hi Jakub, On Thu, Jun 08, 2023 at 06:07:20PM +0530, Manivannan Sadhasivam wrote: > On Wed, Jun 07, 2023 at 10:43:50AM -0700, Jakub Kicinski wrote: > > On Wed, 7 Jun 2023 22:41:53 +0530 Manivannan Sadhasivam wrote: > > > > In any case, I'm opposed to reuse of the networking stack to talk > > > > to firmware. It's a local device. The networking subsystem doesn't > > > > have to cater to fake networks. Please carry: > > > > > > > > Nacked-by: Jakub Kicinski <kuba@kernel.org> > > > > > > > > if there are future submissions. > > > > > > Why shouldn't it be? With this kind of setup one could share the data connectivity > > > available in the device with the host over IP tunneling. If the IP source in the > > > device (like modem DSP) has no way to be shared with the host, then those IP > > > packets could be tunneled through this interface for providing connectivity to > > > the host. > > > > > > I believe this is a common usecase among the PCIe based wireless endpoint > > > devices. > > > > We can handwave our way into many scenarios and terrible architectures. > > I don't see any compelling reason to merge this. > > These kind of usecases exist in the products out there in the market. Regarding > your comment on "opposed to reuse of the network stack to talk to firmware", it > not the just the firmware, it is the device in general that is talking to the > host over this interface. And I don't see how different it is from the host > perspective. > > And these kind of scenarios exist with all types of interfaces like usb-gadget, > virtio etc... So not sure why the rule is different for networking subsystem. > Sorry to revive this old thread, this discussion seems to have fell through the cracks... As I explained above, other interfaces also expose this kind of functionality between host and the device. One of the credible usecase with this driver is sharing the network connectivity available in either host or the device with the other end. To make it clear, we have 2 kind of channels exposed by MHI for networking. 1. IP_SW0 2. IP_HW0 IP_SW0 is useful in scenarios I explained above and IP_HW0 is purely used to provide data connectivity to the host machines with the help of modem IP in the device. And the host side stack is already well supported in mainline. With the proposed driver, Linux can run on the device itself and it will give Qcom a chance to get rid of their proprietary firmware used on the PCIe endpoint devices like modems, etc... - Mani > - Mani > > -- > மணிவண்ணன் சதாசிவம்
On Fri, 17 Nov 2023 12:36:02 +0530 Manivannan Sadhasivam wrote: > Sorry to revive this old thread, this discussion seems to have fell through the > cracks... It did not fall thru the cracks, you got a nack. Here it is in a more official form: Nacked-by: Jakub Kicinski <kuba@kernel.org> Please make sure you keep this tag and CC me if you ever post any form of these patches again.
On Fri, Nov 17, 2023 at 04:26:38PM -0800, Jakub Kicinski wrote: > On Fri, 17 Nov 2023 12:36:02 +0530 Manivannan Sadhasivam wrote: > > Sorry to revive this old thread, this discussion seems to have fell through the > > cracks... > > It did not fall thru the cracks, you got a nack. Here it is in a more > official form: > > Nacked-by: Jakub Kicinski <kuba@kernel.org> > > Please make sure you keep this tag and CC me if you ever post any form > of these patches again. Thanks for the NACK. Could you please respond to my reply justifying this driver (the part you just snipped)? I'm posting it below: > As I explained above, other interfaces also expose this kind of functionality > between host and the device. One of the credible usecase with this driver is > sharing the network connectivity available in either host or the device with the > other end. > > To make it clear, we have 2 kind of channels exposed by MHI for networking. > > 1. IP_SW0 > 2. IP_HW0 > > IP_SW0 is useful in scenarios I explained above and IP_HW0 is purely used to > provide data connectivity to the host machines with the help of modem IP in the > device. And the host side stack is already well supported in mainline. With the > proposed driver, Linux can run on the device itself and it will give Qcom a > chance to get rid of their proprietary firmware used on the PCIe endpoint > devices like modems, etc... I think you made up your mind that this driver is exposing the network interface to the firmware on the device. I ought to clearify that the device running this driver doesn't necessarily be a modem but a PCIe endpoint instance that uses the netdev exposed by this driver to share data connectivity with another device. This concept is not new and being supported by other protocols such as Virtio etc... - Mani
On Mon, 27 Nov 2023 at 18:46, Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 27 Nov 2023 11:34:39 +0530 Manivannan Sadhasivam wrote: > > I think you made up your mind that this driver is exposing the network interface > > to the firmware on the device. I ought to clearify that the device running this > > driver doesn't necessarily be a modem but a PCIe endpoint instance that uses the > > netdev exposed by this driver to share data connectivity with another device. > > Doesn't matter how many legit use cases you can come up with. > Using netdev as a device comm channel is something I am > fundamentally opposed to. > > > This concept is not new and being supported by other protocols such as Virtio > > etc... > > Yes. Use virtio, please. We can try using virtio if we control both sides of the link. However there are usecases of the upstream Linux running on the modem (PCIe EP) side and other systems (Win, Android) running on the RC side. In such cases we have to provide the interface that is expected by the host driver, which unfortunately is MHI. Not to mention that one of the PCIe EP regions contains registers which are targeting the MHI protocol. I am not sure how hardware will react if we bypass this completely and implement VirtIIO or NTB instead. Also, please excuse me if this was already answered, just for my understanding: - If we limit functionality to just networking channels which are used to pass IP data between host and EP, will that be accepted? - If we were to implement the PCIe networking card running Linux (e.g. using Freescale PowerQUICC or Cavium Octeon chips), would you also be opposed to implementing the EP side of the link as the netdev?
On Tue, 28 Nov 2023 22:35:50 +0200 Dmitry Baryshkov wrote: > Also, please excuse me if this was already answered, just for my understanding: > - If we limit functionality to just networking channels which are used > to pass IP data between host and EP, will that be accepted? That's too hard to enforce. We have 200+ drivers, we can't carefully review every single line of code to make sure you stick to the "just networking" promise you make us. Plus the next guy will come and tell us "but you let the company X do it". > - If we were to implement the PCIe networking card running Linux (e.g. > using Freescale PowerQUICC or Cavium Octeon chips), would you also be > opposed to implementing the EP side of the link as the netdev? Yes. It's very tempting to reuse existing code, written for traffic to build a control channel. This becomes painful because: - the lifetime rules for interfaces to configure vs to pass traffic are different, which inevitably leads to bugs in common code, - the use cases are different, which leads to hacks / abuse, and then it's a lot harder for us to refactor and optimize core code / data structures, - IDK how "channel to talk to FW" fits with the normal IP stack... The "FW channel netdevs" exist for decades now, and are very popular with middle box SDKs, I know. Your choices are: - keep the code out of tree, - use a generic interface with a strong standard definition, like virtio, and expect that no customizations will be allowed.
On Tue, 28 Nov 2023 at 22:58, Jakub Kicinski <kuba@kernel.org> wrote: > > On Tue, 28 Nov 2023 22:35:50 +0200 Dmitry Baryshkov wrote: > > Also, please excuse me if this was already answered, just for my understanding: > > - If we limit functionality to just networking channels which are used > > to pass IP data between host and EP, will that be accepted? > > That's too hard to enforce. We have 200+ drivers, we can't carefully > review every single line of code to make sure you stick to the "just > networking" promise you make us. Plus the next guy will come and tell > us "but you let the company X do it". > > > - If we were to implement the PCIe networking card running Linux (e.g. > > using Freescale PowerQUICC or Cavium Octeon chips), would you also be > > opposed to implementing the EP side of the link as the netdev? > > Yes. > > It's very tempting to reuse existing code, written for traffic to build > a control channel. This becomes painful because: > - the lifetime rules for interfaces to configure vs to pass traffic > are different, which inevitably leads to bugs in common code, > - the use cases are different, which leads to hacks / abuse, > and then it's a lot harder for us to refactor and optimize core > code / data structures, > - IDK how "channel to talk to FW" fits with the normal IP stack... Ok, here you are talking about the control path. I can then assume that you consider it to be fine to use netdev for the EP data path, if the control path is kept separate and those two can not be mixed. Does that sound correct? > > The "FW channel netdevs" exist for decades now, and are very popular > with middle box SDKs, I know. Your choices are: > - keep the code out of tree, > - use a generic interface with a strong standard definition, like > virtio, and expect that no customizations will be allowed.
On Mon, 4 Dec 2023 at 18:12, Jakub Kicinski <kuba@kernel.org> wrote: > > On Mon, 4 Dec 2023 14:12:12 +0200 Dmitry Baryshkov wrote: > > Ok, here you are talking about the control path. I can then assume > > that you consider it to be fine to use netdev for the EP data path, if > > the control path is kept separate and those two can not be mixed. Does > > that sound correct? > > If datapath == traffic which is intended to leave the card via > the external port, then yes. Then I think I understand what causes the confusion. The MHI netdev is used to deliver network traffic to the modem CPU, but it is not the controlpath. For the control path we have non-IP MHI channels (QMI, IPCR, etc). This can be the traffic targeting e.g. SSH or HTTP server running on the EP side of the link. I probably fail to see the difference between this scenario and the proper virtio netdev which also allows us to send the same IPv4/v6 traffic to the CPU on the EP side.