[net-next,v8,2/2] net: Add Qcom WWAN control driver

Commit Message

Loic Poulain April 2, 2021, 2:06 p.m. UTC

The MHI WWWAN control driver allows MHI QCOM-based modems to expose
different modem control protocols/ports via the WWAN framework, so that
userspace modem tools or daemon (e.g. ModemManager) can control WWAN
config and state (APN config, SMS, provider selection...). A QCOM-based
modem can expose one or several of the following protocols:
- AT: Well known AT commands interactive protocol (microcom, minicom...)
- MBIM: Mobile Broadband Interface Model (libmbim, mbimcli)
- QMI: QCOM MSM/Modem Interface (libqmi, qmicli)
- QCDM: QCOM Modem diagnostic interface (libqcdm)
- FIREHOSE: XML-based protocol for Modem firmware management
        (qmi-firmware-update)

Note that this patch is mostly a rework of the earlier MHI UCI
tentative that was a generic interface for accessing MHI bus from
userspace. As suggested, this new version is WWAN specific and is
dedicated to only expose channels used for controlling a modem, and
for which related opensource userpace support exist.

Signed-off-by: Loic Poulain <[e-mail]>
---
 v2: update copyright (2021)
 v3: Move driver to dedicated drivers/net/wwan directory
 v4: Rework to use wwan framework instead of self cdev management
 v5: Fix errors/typos in Kconfig
 v6: - Move to new wwan interface, No need dedicated call to wwan_dev_create
     - Cleanup code (remove legacy from mhi_uci, unused defines/vars...)
     - Remove useless write_lock mutex
     - Add mhi_wwan_wait_writable and mhi_wwan_wait_dlqueue_lock_irq helpers
     - Rework locking
     - Add MHI_WWAN_TX_FULL flag
     - Add support for NONBLOCK read/write
 v7: Fix change log (mixed up 1/2 and 2/2)
 v8: - Implement wwan_port_ops (instead of fops)
     - Remove all mhi wwan data obsolete members (kref, lock, waitqueues)
     - Add tracking of RX buffer budget
     - Use WWAN TX flow control function to stop TX when MHI queue is full

 drivers/net/wwan/Kconfig         |  14 +++
 drivers/net/wwan/Makefile        |   2 +
 drivers/net/wwan/mhi_wwan_ctrl.c | 253 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/net/wwan/mhi_wwan_ctrl.c

Comments

Greg Kroah-Hartman April 2, 2021, 2:05 p.m. UTC | #1

On Fri, Apr 02, 2021 at 04:06:37PM +0200, Loic Poulain wrote:
> The MHI WWWAN control driver allows MHI QCOM-based modems to expose
> different modem control protocols/ports via the WWAN framework, so that
> userspace modem tools or daemon (e.g. ModemManager) can control WWAN
> config and state (APN config, SMS, provider selection...). A QCOM-based
> modem can expose one or several of the following protocols:
> - AT: Well known AT commands interactive protocol (microcom, minicom...)
> - MBIM: Mobile Broadband Interface Model (libmbim, mbimcli)
> - QMI: QCOM MSM/Modem Interface (libqmi, qmicli)
> - QCDM: QCOM Modem diagnostic interface (libqcdm)
> - FIREHOSE: XML-based protocol for Modem firmware management
>         (qmi-firmware-update)
> 
> Note that this patch is mostly a rework of the earlier MHI UCI
> tentative that was a generic interface for accessing MHI bus from
> userspace. As suggested, this new version is WWAN specific and is
> dedicated to only expose channels used for controlling a modem, and
> for which related opensource userpace support exist.
> 
> Signed-off-by: Loic Poulain <[e-mail]>
> ---
>  v2: update copyright (2021)
>  v3: Move driver to dedicated drivers/net/wwan directory
>  v4: Rework to use wwan framework instead of self cdev management
>  v5: Fix errors/typos in Kconfig
>  v6: - Move to new wwan interface, No need dedicated call to wwan_dev_create
>      - Cleanup code (remove legacy from mhi_uci, unused defines/vars...)
>      - Remove useless write_lock mutex
>      - Add mhi_wwan_wait_writable and mhi_wwan_wait_dlqueue_lock_irq helpers
>      - Rework locking
>      - Add MHI_WWAN_TX_FULL flag
>      - Add support for NONBLOCK read/write
>  v7: Fix change log (mixed up 1/2 and 2/2)
>  v8: - Implement wwan_port_ops (instead of fops)
>      - Remove all mhi wwan data obsolete members (kref, lock, waitqueues)
>      - Add tracking of RX buffer budget
>      - Use WWAN TX flow control function to stop TX when MHI queue is full
> 
>  drivers/net/wwan/Kconfig         |  14 +++
>  drivers/net/wwan/Makefile        |   2 +
>  drivers/net/wwan/mhi_wwan_ctrl.c | 253 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 269 insertions(+)
>  create mode 100644 drivers/net/wwan/mhi_wwan_ctrl.c
> 
> diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> index 545fe54..ce0bbfb 100644
> --- a/drivers/net/wwan/Kconfig
> +++ b/drivers/net/wwan/Kconfig
> @@ -19,4 +19,18 @@ config WWAN_CORE
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called wwan.
>  
> +config MHI_WWAN_CTRL
> +	tristate "MHI WWAN control driver for QCOM-based PCIe modems"
> +	select WWAN_CORE
> +	depends on MHI_BUS
> +	help
> +	  MHI WWAN CTRL allows QCOM-based PCIe modems to expose different modem
> +	  control protocols/ports to userspace, including AT, MBIM, QMI, DIAG
> +	  and FIREHOSE. These protocols can be accessed directly from userspace
> +	  (e.g. AT commands) or via libraries/tools (e.g. libmbim, libqmi,
> +	  libqcdm...).
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called mhi_wwan_ctrl
> +
>  endif # WWAN
> diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile
> index 934590b..556cd90 100644
> --- a/drivers/net/wwan/Makefile
> +++ b/drivers/net/wwan/Makefile
> @@ -5,3 +5,5 @@
>  
>  obj-$(CONFIG_WWAN_CORE) += wwan.o
>  wwan-objs += wwan_core.o
> +
> +obj-$(CONFIG_MHI_WWAN_CTRL) += mhi_wwan_ctrl.o
> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> new file mode 100644
> index 0000000..f2fab23
> --- /dev/null
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -0,0 +1,253 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2021, Linaro Ltd <[e-mail]> */
> +#include <linux/kernel.h>
> +#include <linux/mhi.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/wwan.h>
> +
> +/* MHI wwan flags */
> +#define MHI_WWAN_DL_CAP		BIT(0)
> +#define MHI_WWAN_UL_CAP		BIT(1)
> +#define MHI_WWAN_STARTED	BIT(2)
> +
> +#define MHI_WWAN_MAX_MTU	0x8000
> +
> +struct mhi_wwan_dev {
> +	/* Lower level is a mhi dev, upper level is a wwan port */
> +	struct mhi_device *mhi_dev;
> +	struct wwan_port *wwan_port;
> +
> +	/* State and capabilities */
> +	unsigned long flags;
> +	size_t mtu;
> +
> +	/* Protect against concurrent TX and TX-completion (bh) */
> +	spinlock_t tx_lock;
> +
> +	struct work_struct rx_refill;
> +	atomic_t rx_budget;

Why is this atomic if you have a real lock already?


> +};
> +
> +static bool mhi_wwan_ctrl_refill_needed(struct mhi_wwan_dev *mhiwwan)
> +{
> +	if (!test_bit(MHI_WWAN_STARTED, &mhiwwan->flags))
> +		return false;
> +
> +	if (!test_bit(MHI_WWAN_DL_CAP, &mhiwwan->flags))
> +		return false;

What prevents these bits from being changed right after reading them?

> +
> +	if (!atomic_read(&mhiwwan->rx_budget))
> +		return false;

Why is this atomic?  What happens if it changes right after returning?

This feels really odd.

> +
> +	return true;
> +}
> +
> +void __mhi_skb_destructor(struct sk_buff *skb)
> +{
> +	struct mhi_wwan_dev *mhiwwan = skb_shinfo(skb)->destructor_arg;
> +
> +	/* RX buffer has been consumed, increase the allowed budget */
> +	atomic_inc(&mhiwwan->rx_budget);

So this is a reference count?  What is this thing?

> +
> +	if (mhi_wwan_ctrl_refill_needed(mhiwwan))
> +		schedule_work(&mhiwwan->rx_refill);

What if refill is needed right after this check?  Did you just miss the
call?


> +static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
> +	{ .chan = "DUN", .driver_data = WWAN_PORT_AT },
> +	{ .chan = "MBIM", .driver_data = WWAN_PORT_MBIM },
> +	{ .chan = "QMI", .driver_data = WWAN_PORT_QMI },
> +	{ .chan = "DIAG", .driver_data = WWAN_PORT_QCDM },
> +	{ .chan = "FIREHOSE", .driver_data = WWAN_PORT_FIREHOSE },

Wait, I thought these were all going to be separate somehow.  Now they
are all muxed back together?

totally confused,

greg k-h

Loic Poulain April 2, 2021, 3:41 p.m. UTC | #2

On Fri, 2 Apr 2021 at 16:05, Greg KH <[e-mail]> wrote:
>
> On Fri, Apr 02, 2021 at 04:06:37PM +0200, Loic Poulain wrote:
> > The MHI WWWAN control driver allows MHI QCOM-based modems to expose
> > different modem control protocols/ports via the WWAN framework, so that
> > userspace modem tools or daemon (e.g. ModemManager) can control WWAN
> > config and state (APN config, SMS, provider selection...). A QCOM-based
> > modem can expose one or several of the following protocols:
> > - AT: Well known AT commands interactive protocol (microcom, minicom...)
> > - MBIM: Mobile Broadband Interface Model (libmbim, mbimcli)
> > - QMI: QCOM MSM/Modem Interface (libqmi, qmicli)
> > - QCDM: QCOM Modem diagnostic interface (libqcdm)
> > - FIREHOSE: XML-based protocol for Modem firmware management
> >         (qmi-firmware-update)
> >
> > Note that this patch is mostly a rework of the earlier MHI UCI
> > tentative that was a generic interface for accessing MHI bus from
> > userspace. As suggested, this new version is WWAN specific and is
> > dedicated to only expose channels used for controlling a modem, and
> > for which related opensource userpace support exist.
> >
> > Signed-off-by: Loic Poulain <[e-mail]>
> > ---
> >  v2: update copyright (2021)
> >  v3: Move driver to dedicated drivers/net/wwan directory
> >  v4: Rework to use wwan framework instead of self cdev management
> >  v5: Fix errors/typos in Kconfig
> >  v6: - Move to new wwan interface, No need dedicated call to wwan_dev_create
> >      - Cleanup code (remove legacy from mhi_uci, unused defines/vars...)
> >      - Remove useless write_lock mutex
> >      - Add mhi_wwan_wait_writable and mhi_wwan_wait_dlqueue_lock_irq helpers
> >      - Rework locking
> >      - Add MHI_WWAN_TX_FULL flag
> >      - Add support for NONBLOCK read/write
> >  v7: Fix change log (mixed up 1/2 and 2/2)
> >  v8: - Implement wwan_port_ops (instead of fops)
> >      - Remove all mhi wwan data obsolete members (kref, lock, waitqueues)
> >      - Add tracking of RX buffer budget
> >      - Use WWAN TX flow control function to stop TX when MHI queue is full
> >
> >  drivers/net/wwan/Kconfig         |  14 +++
> >  drivers/net/wwan/Makefile        |   2 +
> >  drivers/net/wwan/mhi_wwan_ctrl.c | 253 +++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 269 insertions(+)
> >  create mode 100644 drivers/net/wwan/mhi_wwan_ctrl.c
> >
> > diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> > index 545fe54..ce0bbfb 100644
> > --- a/drivers/net/wwan/Kconfig
> > +++ b/drivers/net/wwan/Kconfig
> > @@ -19,4 +19,18 @@ config WWAN_CORE
> >         To compile this driver as a module, choose M here: the module will be
> >         called wwan.
> >
> > +config MHI_WWAN_CTRL
> > +     tristate "MHI WWAN control driver for QCOM-based PCIe modems"
> > +     select WWAN_CORE
> > +     depends on MHI_BUS
> > +     help
> > +       MHI WWAN CTRL allows QCOM-based PCIe modems to expose different modem
> > +       control protocols/ports to userspace, including AT, MBIM, QMI, DIAG
> > +       and FIREHOSE. These protocols can be accessed directly from userspace
> > +       (e.g. AT commands) or via libraries/tools (e.g. libmbim, libqmi,
> > +       libqcdm...).
> > +
> > +       To compile this driver as a module, choose M here: the module will be
> > +       called mhi_wwan_ctrl
> > +
> >  endif # WWAN
> > diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile
> > index 934590b..556cd90 100644
> > --- a/drivers/net/wwan/Makefile
> > +++ b/drivers/net/wwan/Makefile
> > @@ -5,3 +5,5 @@
> >
> >  obj-$(CONFIG_WWAN_CORE) += wwan.o
> >  wwan-objs += wwan_core.o
> > +
> > +obj-$(CONFIG_MHI_WWAN_CTRL) += mhi_wwan_ctrl.o
> > diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> > new file mode 100644
> > index 0000000..f2fab23
> > --- /dev/null
> > +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> > @@ -0,0 +1,253 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2021, Linaro Ltd <[e-mail]> */
> > +#include <linux/kernel.h>
> > +#include <linux/mhi.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/module.h>
> > +#include <linux/wwan.h>
> > +
> > +/* MHI wwan flags */
> > +#define MHI_WWAN_DL_CAP              BIT(0)
> > +#define MHI_WWAN_UL_CAP              BIT(1)
> > +#define MHI_WWAN_STARTED     BIT(2)
> > +
> > +#define MHI_WWAN_MAX_MTU     0x8000
> > +
> > +struct mhi_wwan_dev {
> > +     /* Lower level is a mhi dev, upper level is a wwan port */
> > +     struct mhi_device *mhi_dev;
> > +     struct wwan_port *wwan_port;
> > +
> > +     /* State and capabilities */
> > +     unsigned long flags;
> > +     size_t mtu;
> > +
> > +     /* Protect against concurrent TX and TX-completion (bh) */
> > +     spinlock_t tx_lock;
> > +
> > +     struct work_struct rx_refill;
> > +     atomic_t rx_budget;
>
> Why is this atomic if you have a real lock already?

Access to rx_budget value is not under any locking protection and can
be modified (dec/inc) from different and possibly concurrent places.

>
>
> > +};
> > +
> > +static bool mhi_wwan_ctrl_refill_needed(struct mhi_wwan_dev *mhiwwan)
> > +{
> > +     if (!test_bit(MHI_WWAN_STARTED, &mhiwwan->flags))
> > +             return false;
> > +
> > +     if (!test_bit(MHI_WWAN_DL_CAP, &mhiwwan->flags))
> > +             return false;
>
> What prevents these bits from being changed right after reading them?

Nothing, I've think (maybe wrongly) it's not a problem in the current code.

>
> > +
> > +     if (!atomic_read(&mhiwwan->rx_budget))
> > +             return false;
>
> Why is this atomic?  What happens if it changes right after returning?


If rx_budget was null and becomes non-null, it has been incremented by
__mhi_skb_destructor() which will anyway call
mhi_wwan_ctrl_refill_needed() again, so that's not a problem. On the
other hand, if rx_budget was non-null and becomes null, the
refill_work that will be unnecessarily scheduled will check the value
again and will just return without doing anything.

>
> This feels really odd.
>
> > +
> > +     return true;
> > +}
> > +
> > +void __mhi_skb_destructor(struct sk_buff *skb)
> > +{
> > +     struct mhi_wwan_dev *mhiwwan = skb_shinfo(skb)->destructor_arg;
> > +
> > +     /* RX buffer has been consumed, increase the allowed budget */
> > +     atomic_inc(&mhiwwan->rx_budget);
>
> So this is a reference count?  What is this thing?

This represents the remaining number of buffers that can be allocated
for RX. It is decremented When a buffer is allocated/queued and
incremented when a buffer is consumed (e.g. on WWAN port reading).

>
> > +
> > +     if (mhi_wwan_ctrl_refill_needed(mhiwwan))
> > +             schedule_work(&mhiwwan->rx_refill);
>
> What if refill is needed right after this check?  Did you just miss the
> call?

In running condition, refill is allowed when rx_budget is non-zero,
and __mhi_skb_destructor() is the only path that increments the budget
(and so allow refill) and schedules the refill,  so for this scenario
to happen it would mean that a parallel  __mhi_skb_destructor() is
executed (and incremented rx_budget), so this second parallel call
will schedule the refill.

I realize it's probably odd, but I don't see any scenario in which we
can end badly (e.g. missing refill scheduling, queueing too many
buffers), but I admit it would be certainly simpler and less
error-prone with regular locking.

>
>
> > +static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
> > +     { .chan = "DUN", .driver_data = WWAN_PORT_AT },
> > +     { .chan = "MBIM", .driver_data = WWAN_PORT_MBIM },
> > +     { .chan = "QMI", .driver_data = WWAN_PORT_QMI },
> > +     { .chan = "DIAG", .driver_data = WWAN_PORT_QCDM },
> > +     { .chan = "FIREHOSE", .driver_data = WWAN_PORT_FIREHOSE },
>
> Wait, I thought these were all going to be separate somehow.  Now they
> are all muxed back together?

A WWAN 'port driver' abstracts the method for accessing WWAN control
protocols, so that userspace can e.g. talk MBIM to the port without
knowledge of the underlying bus. Here this is just about abstracting
the MHI/PCI transport, a  MHI modem can support one or several of
these protocols. So this MHI driver binds all MHI control devices, and
each one is registered as a WWAN port. Other 'port drivers' can be
created for different busses or vendors.

Thanks,
Loic

Greg Kroah-Hartman April 2, 2021, 3:43 p.m. UTC | #3

On Fri, Apr 02, 2021 at 05:41:01PM +0200, Loic Poulain wrote:
> On Fri, 2 Apr 2021 at 16:05, Greg KH <[e-mail]> wrote:
> >
> > On Fri, Apr 02, 2021 at 04:06:37PM +0200, Loic Poulain wrote:
> > > The MHI WWWAN control driver allows MHI QCOM-based modems to expose
> > > different modem control protocols/ports via the WWAN framework, so that
> > > userspace modem tools or daemon (e.g. ModemManager) can control WWAN
> > > config and state (APN config, SMS, provider selection...). A QCOM-based
> > > modem can expose one or several of the following protocols:
> > > - AT: Well known AT commands interactive protocol (microcom, minicom...)
> > > - MBIM: Mobile Broadband Interface Model (libmbim, mbimcli)
> > > - QMI: QCOM MSM/Modem Interface (libqmi, qmicli)
> > > - QCDM: QCOM Modem diagnostic interface (libqcdm)
> > > - FIREHOSE: XML-based protocol for Modem firmware management
> > >         (qmi-firmware-update)
> > >
> > > Note that this patch is mostly a rework of the earlier MHI UCI
> > > tentative that was a generic interface for accessing MHI bus from
> > > userspace. As suggested, this new version is WWAN specific and is
> > > dedicated to only expose channels used for controlling a modem, and
> > > for which related opensource userpace support exist.
> > >
> > > Signed-off-by: Loic Poulain <[e-mail]>
> > > ---
> > >  v2: update copyright (2021)
> > >  v3: Move driver to dedicated drivers/net/wwan directory
> > >  v4: Rework to use wwan framework instead of self cdev management
> > >  v5: Fix errors/typos in Kconfig
> > >  v6: - Move to new wwan interface, No need dedicated call to wwan_dev_create
> > >      - Cleanup code (remove legacy from mhi_uci, unused defines/vars...)
> > >      - Remove useless write_lock mutex
> > >      - Add mhi_wwan_wait_writable and mhi_wwan_wait_dlqueue_lock_irq helpers
> > >      - Rework locking
> > >      - Add MHI_WWAN_TX_FULL flag
> > >      - Add support for NONBLOCK read/write
> > >  v7: Fix change log (mixed up 1/2 and 2/2)
> > >  v8: - Implement wwan_port_ops (instead of fops)
> > >      - Remove all mhi wwan data obsolete members (kref, lock, waitqueues)
> > >      - Add tracking of RX buffer budget
> > >      - Use WWAN TX flow control function to stop TX when MHI queue is full
> > >
> > >  drivers/net/wwan/Kconfig         |  14 +++
> > >  drivers/net/wwan/Makefile        |   2 +
> > >  drivers/net/wwan/mhi_wwan_ctrl.c | 253 +++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 269 insertions(+)
> > >  create mode 100644 drivers/net/wwan/mhi_wwan_ctrl.c
> > >
> > > diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> > > index 545fe54..ce0bbfb 100644
> > > --- a/drivers/net/wwan/Kconfig
> > > +++ b/drivers/net/wwan/Kconfig
> > > @@ -19,4 +19,18 @@ config WWAN_CORE
> > >         To compile this driver as a module, choose M here: the module will be
> > >         called wwan.
> > >
> > > +config MHI_WWAN_CTRL
> > > +     tristate "MHI WWAN control driver for QCOM-based PCIe modems"
> > > +     select WWAN_CORE
> > > +     depends on MHI_BUS
> > > +     help
> > > +       MHI WWAN CTRL allows QCOM-based PCIe modems to expose different modem
> > > +       control protocols/ports to userspace, including AT, MBIM, QMI, DIAG
> > > +       and FIREHOSE. These protocols can be accessed directly from userspace
> > > +       (e.g. AT commands) or via libraries/tools (e.g. libmbim, libqmi,
> > > +       libqcdm...).
> > > +
> > > +       To compile this driver as a module, choose M here: the module will be
> > > +       called mhi_wwan_ctrl
> > > +
> > >  endif # WWAN
> > > diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile
> > > index 934590b..556cd90 100644
> > > --- a/drivers/net/wwan/Makefile
> > > +++ b/drivers/net/wwan/Makefile
> > > @@ -5,3 +5,5 @@
> > >
> > >  obj-$(CONFIG_WWAN_CORE) += wwan.o
> > >  wwan-objs += wwan_core.o
> > > +
> > > +obj-$(CONFIG_MHI_WWAN_CTRL) += mhi_wwan_ctrl.o
> > > diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > new file mode 100644
> > > index 0000000..f2fab23
> > > --- /dev/null
> > > +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > @@ -0,0 +1,253 @@
> > > +// SPDX-License-Identifier: GPL-2.0-only
> > > +/* Copyright (c) 2021, Linaro Ltd <[e-mail]> */
> > > +#include <linux/kernel.h>
> > > +#include <linux/mhi.h>
> > > +#include <linux/mod_devicetable.h>
> > > +#include <linux/module.h>
> > > +#include <linux/wwan.h>
> > > +
> > > +/* MHI wwan flags */
> > > +#define MHI_WWAN_DL_CAP              BIT(0)
> > > +#define MHI_WWAN_UL_CAP              BIT(1)
> > > +#define MHI_WWAN_STARTED     BIT(2)
> > > +
> > > +#define MHI_WWAN_MAX_MTU     0x8000
> > > +
> > > +struct mhi_wwan_dev {
> > > +     /* Lower level is a mhi dev, upper level is a wwan port */
> > > +     struct mhi_device *mhi_dev;
> > > +     struct wwan_port *wwan_port;
> > > +
> > > +     /* State and capabilities */
> > > +     unsigned long flags;
> > > +     size_t mtu;
> > > +
> > > +     /* Protect against concurrent TX and TX-completion (bh) */
> > > +     spinlock_t tx_lock;
> > > +
> > > +     struct work_struct rx_refill;
> > > +     atomic_t rx_budget;
> >
> > Why is this atomic if you have a real lock already?
> 
> Access to rx_budget value is not under any locking protection and can
> be modified (dec/inc) from different and possibly concurrent places.

Then use the lock you have instead of creating a "fake lock" with the
atomic value.  If that's really even working (you can't check it and do
something based on it as it could change right afterward).

> > > +};
> > > +
> > > +static bool mhi_wwan_ctrl_refill_needed(struct mhi_wwan_dev *mhiwwan)
> > > +{
> > > +     if (!test_bit(MHI_WWAN_STARTED, &mhiwwan->flags))
> > > +             return false;
> > > +
> > > +     if (!test_bit(MHI_WWAN_DL_CAP, &mhiwwan->flags))
> > > +             return false;
> >
> > What prevents these bits from being changed right after reading them?
> 
> Nothing, I've think (maybe wrongly) it's not a problem in the current code.

Why not?  Why isn't the lock being used here?

Where is the lock used?

> > > +
> > > +     if (!atomic_read(&mhiwwan->rx_budget))
> > > +             return false;
> >
> > Why is this atomic?  What happens if it changes right after returning?
> 
> 
> If rx_budget was null and becomes non-null, it has been incremented by
> __mhi_skb_destructor() which will anyway call
> mhi_wwan_ctrl_refill_needed() again, so that's not a problem. On the
> other hand, if rx_budget was non-null and becomes null, the
> refill_work that will be unnecessarily scheduled will check the value
> again and will just return without doing anything.

You should document the heck out of this as it's not obvious :(

> >
> > This feels really odd.
> >
> > > +
> > > +     return true;
> > > +}
> > > +
> > > +void __mhi_skb_destructor(struct sk_buff *skb)
> > > +{
> > > +     struct mhi_wwan_dev *mhiwwan = skb_shinfo(skb)->destructor_arg;
> > > +
> > > +     /* RX buffer has been consumed, increase the allowed budget */
> > > +     atomic_inc(&mhiwwan->rx_budget);
> >
> > So this is a reference count?  What is this thing?
> 
> This represents the remaining number of buffers that can be allocated
> for RX. It is decremented When a buffer is allocated/queued and
> incremented when a buffer is consumed (e.g. on WWAN port reading).

Why have any budget at all?

That being said, budgets are fine, but properly lock things please.

> > > +
> > > +     if (mhi_wwan_ctrl_refill_needed(mhiwwan))
> > > +             schedule_work(&mhiwwan->rx_refill);
> >
> > What if refill is needed right after this check?  Did you just miss the
> > call?
> 
> In running condition, refill is allowed when rx_budget is non-zero,
> and __mhi_skb_destructor() is the only path that increments the budget
> (and so allow refill) and schedules the refill,  so for this scenario
> to happen it would mean that a parallel  __mhi_skb_destructor() is
> executed (and incremented rx_budget), so this second parallel call
> will schedule the refill.
> 
> I realize it's probably odd, but I don't see any scenario in which we
> can end badly (e.g. missing refill scheduling, queueing too many
> buffers), but I admit it would be certainly simpler and less
> error-prone with regular locking.

Document this all please.

> > > +static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
> > > +     { .chan = "DUN", .driver_data = WWAN_PORT_AT },
> > > +     { .chan = "MBIM", .driver_data = WWAN_PORT_MBIM },
> > > +     { .chan = "QMI", .driver_data = WWAN_PORT_QMI },
> > > +     { .chan = "DIAG", .driver_data = WWAN_PORT_QCDM },
> > > +     { .chan = "FIREHOSE", .driver_data = WWAN_PORT_FIREHOSE },
> >
> > Wait, I thought these were all going to be separate somehow.  Now they
> > are all muxed back together?
> 
> A WWAN 'port driver' abstracts the method for accessing WWAN control
> protocols, so that userspace can e.g. talk MBIM to the port without
> knowledge of the underlying bus. Here this is just about abstracting
> the MHI/PCI transport, a  MHI modem can support one or several of
> these protocols. So this MHI driver binds all MHI control devices, and
> each one is registered as a WWAN port. Other 'port drivers' can be
> created for different busses or vendors.

ok, feels odd, I'll review it again for your next submission as this
seems like just a "raw pipe" to the device and is not actually
standardizing anything, but I could be totally wrong.

thanks,

greg k-h

kernel test robot April 2, 2021, 4:29 p.m. UTC | #4

Hi Loic,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Loic-Poulain/net-Add-a-WWAN-subsystem/20210402-220002
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git bd78980be1a68d14524c51c4b4170782fada622b
config: sh-allmodconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/16d753f4f524ce1aa95f45057244b1d28b50adc9
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Loic-Poulain/net-Add-a-WWAN-subsystem/20210402-220002
        git checkout 16d753f4f524ce1aa95f45057244b1d28b50adc9
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=sh 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[e-mail]>

All warnings (new ones prefixed by >>):

>> drivers/net/wwan/mhi_wwan_ctrl.c:46:6: warning: no previous prototype for '__mhi_skb_destructor' [-Wmissing-prototypes]
      46 | void __mhi_skb_destructor(struct sk_buff *skb)
         |      ^~~~~~~~~~~~~~~~~~~~

Kconfig warnings: (for reference only)
   WARNING: unmet direct dependencies detected for SND_ATMEL_SOC_PDC
   Depends on SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && HAS_DMA
   Selected by
   - SND_ATMEL_SOC_SSC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC
   - SND_ATMEL_SOC_SSC_PDC && SOUND && !UML && SND && SND_SOC && SND_ATMEL_SOC && ATMEL_SSC


vim +/__mhi_skb_destructor +46 drivers/net/wwan/mhi_wwan_ctrl.c

    45	
  > 46	void __mhi_skb_destructor(struct sk_buff *skb)
    47	{
    48		struct mhi_wwan_dev *mhiwwan = skb_shinfo(skb)->destructor_arg;
    49	
    50		/* RX buffer has been consumed, increase the allowed budget */
    51		atomic_inc(&mhiwwan->rx_budget);
    52	
    53		if (mhi_wwan_ctrl_refill_needed(mhiwwan))
    54			schedule_work(&mhiwwan->rx_refill);
    55	}
    56	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[e-mail]

Loic Poulain April 2, 2021, 6:18 p.m. UTC | #5

On Fri, 2 Apr 2021 at 17:43, Greg KH <[e-mail]> wrote:
>
> On Fri, Apr 02, 2021 at 05:41:01PM +0200, Loic Poulain wrote:
> > On Fri, 2 Apr 2021 at 16:05, Greg KH <[e-mail]> wrote:
> > >
> > > On Fri, Apr 02, 2021 at 04:06:37PM +0200, Loic Poulain wrote:
> > > > The MHI WWWAN control driver allows MHI QCOM-based modems to expose
> > > > different modem control protocols/ports via the WWAN framework, so that
> > > > userspace modem tools or daemon (e.g. ModemManager) can control WWAN
> > > > config and state (APN config, SMS, provider selection...). A QCOM-based
> > > > modem can expose one or several of the following protocols:
> > > > - AT: Well known AT commands interactive protocol (microcom, minicom...)
> > > > - MBIM: Mobile Broadband Interface Model (libmbim, mbimcli)
> > > > - QMI: QCOM MSM/Modem Interface (libqmi, qmicli)
> > > > - QCDM: QCOM Modem diagnostic interface (libqcdm)
> > > > - FIREHOSE: XML-based protocol for Modem firmware management
> > > >         (qmi-firmware-update)
> > > >
> > > > Note that this patch is mostly a rework of the earlier MHI UCI
> > > > tentative that was a generic interface for accessing MHI bus from
> > > > userspace. As suggested, this new version is WWAN specific and is
> > > > dedicated to only expose channels used for controlling a modem, and
> > > > for which related opensource userpace support exist.
> > > >
> > > > Signed-off-by: Loic Poulain <[e-mail]>
> > > > ---
> > > >  v2: update copyright (2021)
> > > >  v3: Move driver to dedicated drivers/net/wwan directory
> > > >  v4: Rework to use wwan framework instead of self cdev management
> > > >  v5: Fix errors/typos in Kconfig
> > > >  v6: - Move to new wwan interface, No need dedicated call to wwan_dev_create
> > > >      - Cleanup code (remove legacy from mhi_uci, unused defines/vars...)
> > > >      - Remove useless write_lock mutex
> > > >      - Add mhi_wwan_wait_writable and mhi_wwan_wait_dlqueue_lock_irq helpers
> > > >      - Rework locking
> > > >      - Add MHI_WWAN_TX_FULL flag
> > > >      - Add support for NONBLOCK read/write
> > > >  v7: Fix change log (mixed up 1/2 and 2/2)
> > > >  v8: - Implement wwan_port_ops (instead of fops)
> > > >      - Remove all mhi wwan data obsolete members (kref, lock, waitqueues)
> > > >      - Add tracking of RX buffer budget
> > > >      - Use WWAN TX flow control function to stop TX when MHI queue is full
> > > >
> > > >  drivers/net/wwan/Kconfig         |  14 +++
> > > >  drivers/net/wwan/Makefile        |   2 +
> > > >  drivers/net/wwan/mhi_wwan_ctrl.c | 253 +++++++++++++++++++++++++++++++++++++++
> > > >  3 files changed, 269 insertions(+)
> > > >  create mode 100644 drivers/net/wwan/mhi_wwan_ctrl.c
> > > >
> > > > diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
> > > > index 545fe54..ce0bbfb 100644
> > > > --- a/drivers/net/wwan/Kconfig
> > > > +++ b/drivers/net/wwan/Kconfig
> > > > @@ -19,4 +19,18 @@ config WWAN_CORE
> > > >         To compile this driver as a module, choose M here: the module will be
> > > >         called wwan.
> > > >
> > > > +config MHI_WWAN_CTRL
> > > > +     tristate "MHI WWAN control driver for QCOM-based PCIe modems"
> > > > +     select WWAN_CORE
> > > > +     depends on MHI_BUS
> > > > +     help
> > > > +       MHI WWAN CTRL allows QCOM-based PCIe modems to expose different modem
> > > > +       control protocols/ports to userspace, including AT, MBIM, QMI, DIAG
> > > > +       and FIREHOSE. These protocols can be accessed directly from userspace
> > > > +       (e.g. AT commands) or via libraries/tools (e.g. libmbim, libqmi,
> > > > +       libqcdm...).
> > > > +
> > > > +       To compile this driver as a module, choose M here: the module will be
> > > > +       called mhi_wwan_ctrl
> > > > +
> > > >  endif # WWAN
> > > > diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile
> > > > index 934590b..556cd90 100644
> > > > --- a/drivers/net/wwan/Makefile
> > > > +++ b/drivers/net/wwan/Makefile
> > > > @@ -5,3 +5,5 @@
> > > >
> > > >  obj-$(CONFIG_WWAN_CORE) += wwan.o
> > > >  wwan-objs += wwan_core.o
> > > > +
> > > > +obj-$(CONFIG_MHI_WWAN_CTRL) += mhi_wwan_ctrl.o
> > > > diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > > new file mode 100644
> > > > index 0000000..f2fab23
> > > > --- /dev/null
> > > > +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > > @@ -0,0 +1,253 @@
> > > > +// SPDX-License-Identifier: GPL-2.0-only
> > > > +/* Copyright (c) 2021, Linaro Ltd <[e-mail]> */
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/mhi.h>
> > > > +#include <linux/mod_devicetable.h>
> > > > +#include <linux/module.h>
> > > > +#include <linux/wwan.h>
> > > > +
> > > > +/* MHI wwan flags */
> > > > +#define MHI_WWAN_DL_CAP              BIT(0)
> > > > +#define MHI_WWAN_UL_CAP              BIT(1)
> > > > +#define MHI_WWAN_STARTED     BIT(2)
> > > > +
> > > > +#define MHI_WWAN_MAX_MTU     0x8000
> > > > +
> > > > +struct mhi_wwan_dev {
> > > > +     /* Lower level is a mhi dev, upper level is a wwan port */
> > > > +     struct mhi_device *mhi_dev;
> > > > +     struct wwan_port *wwan_port;
> > > > +
> > > > +     /* State and capabilities */
> > > > +     unsigned long flags;
> > > > +     size_t mtu;
> > > > +
> > > > +     /* Protect against concurrent TX and TX-completion (bh) */
> > > > +     spinlock_t tx_lock;
> > > > +
> > > > +     struct work_struct rx_refill;
> > > > +     atomic_t rx_budget;
> > >
> > > Why is this atomic if you have a real lock already?
> >
> > Access to rx_budget value is not under any locking protection and can
> > be modified (dec/inc) from different and possibly concurrent places.
>
> Then use the lock you have instead of creating a "fake lock" with the
> atomic value.  If that's really even working (you can't check it and do
> something based on it as it could change right afterward).
>
> > > > +};
> > > > +
> > > > +static bool mhi_wwan_ctrl_refill_needed(struct mhi_wwan_dev *mhiwwan)
> > > > +{
> > > > +     if (!test_bit(MHI_WWAN_STARTED, &mhiwwan->flags))
> > > > +             return false;
> > > > +
> > > > +     if (!test_bit(MHI_WWAN_DL_CAP, &mhiwwan->flags))
> > > > +             return false;
> > >
> > > What prevents these bits from being changed right after reading them?
> >
> > Nothing, I've think (maybe wrongly) it's not a problem in the current code.
>
> Why not?  Why isn't the lock being used here?
>
> Where is the lock used?
>
> > > > +
> > > > +     if (!atomic_read(&mhiwwan->rx_budget))
> > > > +             return false;
> > >
> > > Why is this atomic?  What happens if it changes right after returning?
> >
> >
> > If rx_budget was null and becomes non-null, it has been incremented by
> > __mhi_skb_destructor() which will anyway call
> > mhi_wwan_ctrl_refill_needed() again, so that's not a problem. On the
> > other hand, if rx_budget was non-null and becomes null, the
> > refill_work that will be unnecessarily scheduled will check the value
> > again and will just return without doing anything.
>
> You should document the heck out of this as it's not obvious :(
>
> > >
> > > This feels really odd.
> > >
> > > > +
> > > > +     return true;
> > > > +}
> > > > +
> > > > +void __mhi_skb_destructor(struct sk_buff *skb)
> > > > +{
> > > > +     struct mhi_wwan_dev *mhiwwan = skb_shinfo(skb)->destructor_arg;
> > > > +
> > > > +     /* RX buffer has been consumed, increase the allowed budget */
> > > > +     atomic_inc(&mhiwwan->rx_budget);
> > >
> > > So this is a reference count?  What is this thing?
> >
> > This represents the remaining number of buffers that can be allocated
> > for RX. It is decremented When a buffer is allocated/queued and
> > incremented when a buffer is consumed (e.g. on WWAN port reading).
>
> Why have any budget at all?
>
> That being said, budgets are fine, but properly lock things please.
>
> > > > +
> > > > +     if (mhi_wwan_ctrl_refill_needed(mhiwwan))
> > > > +             schedule_work(&mhiwwan->rx_refill);
> > >
> > > What if refill is needed right after this check?  Did you just miss the
> > > call?
> >
> > In running condition, refill is allowed when rx_budget is non-zero,
> > and __mhi_skb_destructor() is the only path that increments the budget
> > (and so allow refill) and schedules the refill,  so for this scenario
> > to happen it would mean that a parallel  __mhi_skb_destructor() is
> > executed (and incremented rx_budget), so this second parallel call
> > will schedule the refill.
> >
> > I realize it's probably odd, but I don't see any scenario in which we
> > can end badly (e.g. missing refill scheduling, queueing too many
> > buffers), but I admit it would be certainly simpler and less
> > error-prone with regular locking.
>
> Document this all please.
>
> > > > +static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
> > > > +     { .chan = "DUN", .driver_data = WWAN_PORT_AT },
> > > > +     { .chan = "MBIM", .driver_data = WWAN_PORT_MBIM },
> > > > +     { .chan = "QMI", .driver_data = WWAN_PORT_QMI },
> > > > +     { .chan = "DIAG", .driver_data = WWAN_PORT_QCDM },
> > > > +     { .chan = "FIREHOSE", .driver_data = WWAN_PORT_FIREHOSE },
> > >
> > > Wait, I thought these were all going to be separate somehow.  Now they
> > > are all muxed back together?
> >
> > A WWAN 'port driver' abstracts the method for accessing WWAN control
> > protocols, so that userspace can e.g. talk MBIM to the port without
> > knowledge of the underlying bus. Here this is just about abstracting
> > the MHI/PCI transport, a  MHI modem can support one or several of
> > these protocols. So this MHI driver binds all MHI control devices, and
> > each one is registered as a WWAN port. Other 'port drivers' can be
> > created for different busses or vendors.
>
> ok, feels odd, I'll review it again for your next submission as this
> seems like just a "raw pipe" to the device and is not actually
> standardizing anything, but I could be totally wrong.

It's kind of a raw pipe since modems expose various high-level
protocols that are directly accessed by user space tools (AT/MBIM...).
There is really nothing to do from the kernel side here except using
this class for standardizing the way these protocols are accessed
(chardev read/write), and grouping them under the same 'virtual WWAN'
node.

Because of their history, modems are quite different from other pieces
of hardware such as WiFi, Bluetooth, etc... that are usually
represented through a unique device (e.g. wireless_dev, hci...) and a
well-defined API.
Modems were only voice at the beginning and only one
simple serial/uart was necessary for control purpose (via AT
commands), then data support was added (GPRS), using the existing
or secondary serial to send network data (by attaching e.g. ppp line
discipline),
Next step was the USB WWAN devices (either as regular USB stick, or
via PCMCIA with integrated USB), that initially re-used the existing
architecture by simply exposing the serial(s)/tty(s) over USB.
Later, some moved to more modern and optimized data and control
interfaces/protocols (such as QMAP, MBIM, QMI....).
At the end, it it's not surprising that you end today with several
ttyUSB*/ttyACM*, character devices, and netdev when you
plug a USB modem, all of them contributing to the whole WWAN feature,
and it's up to the user (well... ModemManager) to deal with that.
In the MHI/PCI case, the USB concept of interfaces/endpoints has simply
been translated to 'MHI channels', and we also end with a bunch of MHI
devices for network data (netdev) and controls (chardev)... How to
expose these controls channels is the question, we can either go with a
new bus specific MHI character driver (what we've tried with mhi_uci)
or initiate a bus agnostic WWAN chardev driver via this new WWAN
framework, which I agree is not much more than a factorized
passthrough layer for now.

Regards,
Loic

Patch

diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 545fe54..ce0bbfb 100644
--- a/drivers/net/wwan/Kconfig
+++ b/drivers/net/wwan/Kconfig
@@ -19,4 +19,18 @@  config WWAN_CORE
 	  To compile this driver as a module, choose M here: the module will be
 	  called wwan.
 
+config MHI_WWAN_CTRL
+	tristate "MHI WWAN control driver for QCOM-based PCIe modems"
+	select WWAN_CORE
+	depends on MHI_BUS
+	help
+	  MHI WWAN CTRL allows QCOM-based PCIe modems to expose different modem
+	  control protocols/ports to userspace, including AT, MBIM, QMI, DIAG
+	  and FIREHOSE. These protocols can be accessed directly from userspace
+	  (e.g. AT commands) or via libraries/tools (e.g. libmbim, libqmi,
+	  libqcdm...).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called mhi_wwan_ctrl
+
 endif # WWAN
diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile
index 934590b..556cd90 100644
--- a/drivers/net/wwan/Makefile
+++ b/drivers/net/wwan/Makefile
@@ -5,3 +5,5 @@ 
 
 obj-$(CONFIG_WWAN_CORE) += wwan.o
 wwan-objs += wwan_core.o
+
+obj-$(CONFIG_MHI_WWAN_CTRL) += mhi_wwan_ctrl.o
diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
new file mode 100644
index 0000000..f2fab23
--- /dev/null
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -0,0 +1,253 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, Linaro Ltd <[e-mail]> */
+#include <linux/kernel.h>
+#include <linux/mhi.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/wwan.h>
+
+/* MHI wwan flags */
+#define MHI_WWAN_DL_CAP		BIT(0)
+#define MHI_WWAN_UL_CAP		BIT(1)
+#define MHI_WWAN_STARTED	BIT(2)
+
+#define MHI_WWAN_MAX_MTU	0x8000
+
+struct mhi_wwan_dev {
+	/* Lower level is a mhi dev, upper level is a wwan port */
+	struct mhi_device *mhi_dev;
+	struct wwan_port *wwan_port;
+
+	/* State and capabilities */
+	unsigned long flags;
+	size_t mtu;
+
+	/* Protect against concurrent TX and TX-completion (bh) */
+	spinlock_t tx_lock;
+
+	struct work_struct rx_refill;
+	atomic_t rx_budget;
+};
+
+static bool mhi_wwan_ctrl_refill_needed(struct mhi_wwan_dev *mhiwwan)
+{
+	if (!test_bit(MHI_WWAN_STARTED, &mhiwwan->flags))
+		return false;
+
+	if (!test_bit(MHI_WWAN_DL_CAP, &mhiwwan->flags))
+		return false;
+
+	if (!atomic_read(&mhiwwan->rx_budget))
+		return false;
+
+	return true;
+}
+
+void __mhi_skb_destructor(struct sk_buff *skb)
+{
+	struct mhi_wwan_dev *mhiwwan = skb_shinfo(skb)->destructor_arg;
+
+	/* RX buffer has been consumed, increase the allowed budget */
+	atomic_inc(&mhiwwan->rx_budget);
+
+	if (mhi_wwan_ctrl_refill_needed(mhiwwan))
+		schedule_work(&mhiwwan->rx_refill);
+}
+
+static void mhi_wwan_ctrl_refill_work(struct work_struct *work)
+{
+	struct mhi_wwan_dev *mhiwwan = container_of(work, struct mhi_wwan_dev, rx_refill);
+	struct mhi_device *mhi_dev = mhiwwan->mhi_dev;
+
+	if (!mhi_wwan_ctrl_refill_needed(mhiwwan))
+		return;
+
+	while (atomic_read(&mhiwwan->rx_budget)) {
+		struct sk_buff *skb;
+
+		skb = alloc_skb(mhiwwan->mtu, GFP_KERNEL);
+		if (!skb)
+			break;
+
+		/* To prevent unlimited buffer allocation if nothing consumes
+		 * the RX buffers (passed to WWAN core), track their lifespan
+		 * to not allocate more than allowed budget.
+		 */
+		skb->destructor = __mhi_skb_destructor;
+		skb_shinfo(skb)->destructor_arg = mhiwwan;
+
+		if (mhi_queue_skb(mhi_dev, DMA_FROM_DEVICE, skb, mhiwwan->mtu, MHI_EOT)) {
+			dev_err(&mhi_dev->dev, "Failed to queue buffer\n");
+			kfree_skb(skb);
+			break;
+		}
+
+		atomic_dec(&mhiwwan->rx_budget);
+	}
+}
+
+static int mhi_wwan_ctrl_start(struct wwan_port *port)
+{
+	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
+	int ret, rx_budget;
+
+	/* Start mhi device's channel(s) */
+	ret = mhi_prepare_for_transfer(mhiwwan->mhi_dev);
+	if (ret)
+		return ret;
+
+	set_bit(MHI_WWAN_STARTED, &mhiwwan->flags);
+
+	/* Don't allocate more buffers than MHI channel queue size */
+	rx_budget = mhi_get_free_desc_count(mhiwwan->mhi_dev, DMA_FROM_DEVICE);
+	atomic_set(&mhiwwan->rx_budget, rx_budget);
+
+	/* Add buffers to the MHI inbound queue */
+	mhi_wwan_ctrl_refill_work(&mhiwwan->rx_refill);
+
+	return 0;
+}
+
+static void mhi_wwan_ctrl_stop(struct wwan_port *port)
+{
+	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
+
+	clear_bit(MHI_WWAN_STARTED, &mhiwwan->flags);
+	mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
+}
+
+static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
+{
+	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
+	int ret;
+
+	if (skb->len > mhiwwan->mtu)
+		return -EMSGSIZE;
+
+	if (!test_bit(MHI_WWAN_UL_CAP, &mhiwwan->flags))
+		return -ENOTSUPP;
+
+	spin_lock_bh(&mhiwwan->tx_lock);
+	ret = mhi_queue_skb(mhiwwan->mhi_dev, DMA_TO_DEVICE, skb, skb->len, MHI_EOT);
+	if (mhi_queue_is_full(mhiwwan->mhi_dev, DMA_TO_DEVICE))
+		wwan_port_txoff(port);
+	spin_unlock_bh(&mhiwwan->tx_lock);
+
+	return ret;
+}
+
+static const struct wwan_port_ops wwan_pops = {
+	.start = mhi_wwan_ctrl_start,
+	.stop = mhi_wwan_ctrl_stop,
+	.tx = mhi_wwan_ctrl_tx,
+};
+
+static void mhi_ul_xfer_cb(struct mhi_device *mhi_dev,
+			   struct mhi_result *mhi_result)
+{
+	struct mhi_wwan_dev *mhiwwan = dev_get_drvdata(&mhi_dev->dev);
+	struct wwan_port *port = mhiwwan->wwan_port;
+	struct sk_buff *skb = mhi_result->buf_addr;
+
+	dev_dbg(&mhi_dev->dev, "%s: status: %d xfer_len: %zu\n", __func__,
+		mhi_result->transaction_status, mhi_result->bytes_xferd);
+
+	/* MHI core has done with the buffer, release it */
+	consume_skb(skb);
+
+	spin_lock_bh(&mhiwwan->tx_lock);
+	if (!mhi_queue_is_full(mhiwwan->mhi_dev, DMA_TO_DEVICE))
+		wwan_port_txon(port);
+	spin_unlock_bh(&mhiwwan->tx_lock);
+}
+
+static void mhi_dl_xfer_cb(struct mhi_device *mhi_dev,
+			   struct mhi_result *mhi_result)
+{
+	struct mhi_wwan_dev *mhiwwan = dev_get_drvdata(&mhi_dev->dev);
+	struct wwan_port *port = mhiwwan->wwan_port;
+	struct sk_buff *skb = mhi_result->buf_addr;
+
+	dev_dbg(&mhi_dev->dev, "%s: status: %d receive_len: %zu\n", __func__,
+		mhi_result->transaction_status, mhi_result->bytes_xferd);
+
+	if (mhi_result->transaction_status &&
+	    mhi_result->transaction_status != -EOVERFLOW) {
+		kfree_skb(skb);
+		return;
+	}
+
+	/* MHI core does not update skb->len, do it before forward */
+	skb_put(skb, mhi_result->bytes_xferd);
+	wwan_port_rx(port, skb);
+}
+
+static int mhi_wwan_ctrl_probe(struct mhi_device *mhi_dev,
+			       const struct mhi_device_id *id)
+{
+	struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
+	struct mhi_wwan_dev *mhiwwan;
+
+	mhiwwan = kzalloc(sizeof(*mhiwwan), GFP_KERNEL);
+	if (!mhiwwan)
+		return -ENOMEM;
+
+	mhiwwan->mhi_dev = mhi_dev;
+	mhiwwan->mtu = MHI_WWAN_MAX_MTU;
+	INIT_WORK(&mhiwwan->rx_refill, mhi_wwan_ctrl_refill_work);
+	spin_lock_init(&mhiwwan->tx_lock);
+
+	if (mhi_dev->dl_chan)
+		set_bit(MHI_WWAN_DL_CAP, &mhiwwan->flags);
+	if (mhi_dev->ul_chan)
+		set_bit(MHI_WWAN_UL_CAP, &mhiwwan->flags);
+
+	dev_set_drvdata(&mhi_dev->dev, mhiwwan);
+
+	/* Register as a wwan port, id->driver_data contains wwan port type */
+	mhiwwan->wwan_port = wwan_create_port(&cntrl->mhi_dev->dev,
+					      id->driver_data,
+					      &wwan_pops, mhiwwan);
+	if (IS_ERR(mhiwwan->wwan_port)) {
+		kfree(mhiwwan);
+		return PTR_ERR(mhiwwan->wwan_port);
+	}
+
+	return 0;
+};
+
+static void mhi_wwan_ctrl_remove(struct mhi_device *mhi_dev)
+{
+	struct mhi_wwan_dev *mhiwwan = dev_get_drvdata(&mhi_dev->dev);
+
+	wwan_remove_port(mhiwwan->wwan_port);
+	cancel_work_sync(&mhiwwan->rx_refill);
+	kfree(mhiwwan);
+}
+
+static const struct mhi_device_id mhi_wwan_ctrl_match_table[] = {
+	{ .chan = "DUN", .driver_data = WWAN_PORT_AT },
+	{ .chan = "MBIM", .driver_data = WWAN_PORT_MBIM },
+	{ .chan = "QMI", .driver_data = WWAN_PORT_QMI },
+	{ .chan = "DIAG", .driver_data = WWAN_PORT_QCDM },
+	{ .chan = "FIREHOSE", .driver_data = WWAN_PORT_FIREHOSE },
+	{},
+};
+MODULE_DEVICE_TABLE(mhi, mhi_wwan_ctrl_match_table);
+
+static struct mhi_driver mhi_wwan_ctrl_driver = {
+	.id_table = mhi_wwan_ctrl_match_table,
+	.remove = mhi_wwan_ctrl_remove,
+	.probe = mhi_wwan_ctrl_probe,
+	.ul_xfer_cb = mhi_ul_xfer_cb,
+	.dl_xfer_cb = mhi_dl_xfer_cb,
+	.driver = {
+		.name = "mhi_wwan_ctrl",
+	},
+};
+
+module_mhi_driver(mhi_wwan_ctrl_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("MHI WWAN CTRL Driver");
+MODULE_AUTHOR("Loic Poulain <[e-mail]>");