| Message ID | 1617372397-13988-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next,v8,1/2] net: Add a WWAN subsystem | expand |
On Fri, Apr 02, 2021 at 04:06:36PM +0200, Loic Poulain wrote: > This change introduces initial support for a WWAN framework. Given the > complexity and heterogeneity of existing WWAN hardwares and interfaces, > there is no strict definition of what a WWAN device is and how it should > be represented. It's often a collection of multiple devices that perform > the global WWAN feature (netdev, tty, chardev, etc). > > One usual way to expose modem controls and configuration is via high > level protocols such as the well known AT command protocol, MBIM or > QMI. The USB modems started to expose that as character devices, and > user daemons such as ModemManager learnt how to deal with them. This > initial version adds the concept of WWAN port, which can be created > by any driver to expose one of these protocols. The WWAN core takes > care of the generic part, including character device management, and > rely on port operations to received and submit protocol data. > > Since the different components/devices do no necesserarly know about > each others, and can be created/removed in different orders, the > WWAN core ensures that all WAN ports that contribute to the 'whole' > WWAN feature are grouped under the same virtual WWAN device, relying > on the provided parent device (e.g. mhi controller, USB device). It's > a 'trick' I copied from Johannes's earlier WWAN subsystem proposal. > > This initial version is purposely minimalist, it's essentially moving > the generic part of the previously proposed mhi_wwan_ctrl driver inside > a common WWAN framework, but the implementation is open and flexible > enough to allow extension for further drivers. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Always run checkpatch before sending stuff off :( Anyway, one thing did stand out: > --- /dev/null > +++ b/include/linux/wwan.h > @@ -0,0 +1,127 @@ > +/* SPDX-License-Identifier: GPL-2.0-only */ > +/* Copyright (c) 2021, Linaro Ltd <loic.poulain@linaro.org> */ > + > +#ifndef __WWAN_H > +#define __WWAN_H > + > +#include <linux/device.h> > +#include <linux/kernel.h> > +#include <linux/skbuff.h> > + > +/** > + * enum wwan_port_type - WWAN port types > + * @WWAN_PORT_AT: AT commands > + * @WWAN_PORT_MBIM: Mobile Broadband Interface Model control > + * @WWAN_PORT_QMI: Qcom modem/MSM interface for modem control > + * @WWAN_PORT_QCDM: Qcom Modem diagnostic interface > + * @WWAN_PORT_FIREHOSE: XML based command protocol > + * @WWAN_PORT_MAX > + */ > +enum wwan_port_type { > + WWAN_PORT_AT, > + WWAN_PORT_MBIM, > + WWAN_PORT_QMI, > + WWAN_PORT_QCDM, > + WWAN_PORT_FIREHOSE, > + WWAN_PORT_MAX, > +}; > + > +/** > + * struct wwan_port - The structure that defines a WWAN port > + * @type: Port type > + * @start_count: Port start counter > + * @flags: Store port state and capabilities > + * @ops: Pointer to WWAN port operations > + * @ops_lock: Protect port ops > + * @dev: Underlying device > + * @rxq: Buffer inbound queue > + * @waitqueue: The waitqueue for port fops (read/write/poll) > + */ > +struct wwan_port { > + enum wwan_port_type type; > + unsigned int start_count; > + unsigned long flags; > + const struct wwan_port_ops *ops; > + struct mutex ops_lock; > + struct device dev; > + struct sk_buff_head rxq; > + wait_queue_head_t waitqueue; > +}; No need to put the actual definition of struct wwan_port in this .h file, keep it private in your .c file to keep wwan drivers from poking around in it where they shouldn't be :) thanks, greg k-h
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 <loic.poulain@linaro.org> > --- > 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 <loic.poulain@linaro.org> */ > +#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
On Fri, 2 Apr 2021 at 16:05, Greg KH <gregkh@linuxfoundation.org> 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 <loic.poulain@linaro.org> > > --- > > 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 <loic.poulain@linaro.org> */ > > +#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
On Fri, Apr 02, 2021 at 05:41:01PM +0200, Loic Poulain wrote: > On Fri, 2 Apr 2021 at 16:05, Greg KH <gregkh@linuxfoundation.org> 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 <loic.poulain@linaro.org> > > > --- > > > 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 <loic.poulain@linaro.org> */ > > > +#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
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: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
cocci warnings: (new ones prefixed by >>)
>> drivers/net/wwan/wwan_core.c:208:9-16: WARNING: ERR_CAST can be used with wwandev
Please review and possibly fold the followup patch.
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Fri, 2 Apr 2021 at 17:43, Greg KH <gregkh@linuxfoundation.org> wrote: > > On Fri, Apr 02, 2021 at 05:41:01PM +0200, Loic Poulain wrote: > > On Fri, 2 Apr 2021 at 16:05, Greg KH <gregkh@linuxfoundation.org> 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 <loic.poulain@linaro.org> > > > > --- > > > > 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 <loic.poulain@linaro.org> */ > > > > +#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
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index 5895905..74dc8e24 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -502,6 +502,8 @@ source "drivers/net/wan/Kconfig" source "drivers/net/ieee802154/Kconfig" +source "drivers/net/wwan/Kconfig" + config XEN_NETDEV_FRONTEND tristate "Xen network device frontend driver" depends on XEN diff --git a/drivers/net/Makefile b/drivers/net/Makefile index 040e20b..7ffd2d0 100644 --- a/drivers/net/Makefile +++ b/drivers/net/Makefile @@ -68,6 +68,7 @@ obj-$(CONFIG_SUNGEM_PHY) += sungem_phy.o obj-$(CONFIG_WAN) += wan/ obj-$(CONFIG_WLAN) += wireless/ obj-$(CONFIG_IEEE802154) += ieee802154/ +obj-$(CONFIG_WWAN) += wwan/ obj-$(CONFIG_VMXNET3) += vmxnet3/ obj-$(CONFIG_XEN_NETDEV_FRONTEND) += xen-netfront.o diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig new file mode 100644 index 0000000..545fe54 --- /dev/null +++ b/drivers/net/wwan/Kconfig @@ -0,0 +1,22 @@ +# SPDX-License-Identifier: GPL-2.0-only +# +# Wireless WAN device configuration +# + +menuconfig WWAN + bool "Wireless WAN" + help + This section contains Wireless WAN driver configurations. + +if WWAN + +config WWAN_CORE + tristate "WWAN Driver Core" + help + Say Y here if you want to use the WWAN driver core. This driver + provides a common framework for WWAN drivers. + + To compile this driver as a module, choose M here: the module will be + called wwan. + +endif # WWAN diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile new file mode 100644 index 0000000..934590b --- /dev/null +++ b/drivers/net/wwan/Makefile @@ -0,0 +1,7 @@ +# SPDX-License-Identifier: GPL-2.0 +# +# Makefile for the Linux WWAN device drivers. +# + +obj-$(CONFIG_WWAN_CORE) += wwan.o +wwan-objs += wwan_core.o diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c new file mode 100644 index 0000000..4c11666 --- /dev/null +++ b/drivers/net/wwan/wwan_core.c @@ -0,0 +1,524 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* Copyright (c) 2021, Linaro Ltd <loic.poulain@linaro.org> */ + +#include <linux/err.h> +#include <linux/errno.h> +#include <linux/fs.h> +#include <linux/init.h> +#include <linux/idr.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/poll.h> +#include <linux/skbuff.h> +#include <linux/slab.h> +#include <linux/types.h> +#include <linux/wwan.h> + +#define WWAN_MAX_MINORS 256 /* 256 minors allowed with register_chrdev() */ + +static DEFINE_MUTEX(wwan_register_lock); /* WWAN device create|remove lock */ +static DEFINE_IDA(minors); /* minors for WWAN port chardevs */ +static DEFINE_IDA(wwan_dev_ids); /* for unique WWAN device IDs */ +static struct class *wwan_class; +static int wwan_major; + +#define to_wwan_dev(d) container_of(d, struct wwan_device, dev) +#define to_wwan_port(d) container_of(d, struct wwan_port, dev) + +/* WWAN port flags */ +#define WWAN_PORT_TX_OFF BIT(0) + +/** + * struct wwan_device - The structure that defines a WWAN device + * + * @id: WWAN device unique ID. + * @dev: Underlying device. + * @port_id: Current available port ID to pick. + */ +struct wwan_device { + unsigned int id; + struct device dev; + atomic_t port_id; +}; + +static void wwan_dev_destroy(struct device *dev) +{ + struct wwan_device *wwandev = to_wwan_dev(dev); + + ida_free(&wwan_dev_ids, wwandev->id); + kfree(wwandev); +} + +static const struct device_type wwan_dev_type = { + .name = "wwan_dev", + .release = wwan_dev_destroy, +}; + +static int wwan_dev_parent_match(struct device *dev, const void *parent) +{ + return (dev->type == &wwan_dev_type && dev->parent == parent); +} + +static struct wwan_device *wwan_dev_get_by_parent(struct device *parent) +{ + struct device *dev; + + dev = class_find_device(wwan_class, NULL, parent, wwan_dev_parent_match); + if (!dev) + return ERR_PTR(-ENODEV); + + return to_wwan_dev(dev); +} + +/* This function allocates and registers a new WWAN device OR if a WWAN device + * already exist for the given parent, it gets a reference and return it. + * This function is not exported (for now), it is called indirectly via + * wwan_create_port(). + */ +static struct wwan_device *wwan_create_dev(struct device *parent) +{ + struct wwan_device *wwandev; + int err, id; + + /* The 'find-alloc-register' operation must be protected against + * concurrent execution, a WWAN device is possibly shared between + * multiple callers or concurrently unregistered from wwan_remove_dev(). + */ + mutex_lock(&wwan_register_lock); + + /* If wwandev already exists, return it */ + wwandev = wwan_dev_get_by_parent(parent); + if (!IS_ERR(wwandev)) + goto done_unlock; + + id = ida_alloc(&wwan_dev_ids, GFP_KERNEL); + if (id < 0) + goto done_unlock; + + wwandev = kzalloc(sizeof(*wwandev), GFP_KERNEL); + if (!wwandev) { + ida_free(&wwan_dev_ids, id); + goto done_unlock; + } + + wwandev->dev.parent = parent; + wwandev->dev.class = wwan_class; + wwandev->dev.type = &wwan_dev_type; + wwandev->id = id; + dev_set_name(&wwandev->dev, "wwan%d", wwandev->id); + + err = device_register(&wwandev->dev); + if (err) { + put_device(&wwandev->dev); + wwandev = NULL; + } + +done_unlock: + mutex_unlock(&wwan_register_lock); + + return wwandev; +} + +static int is_wwan_child(struct device *dev, void *data) +{ + return dev->class == wwan_class; +} + +static void wwan_remove_dev(struct wwan_device *wwandev) +{ + int ret; + + /* Prevent concurrent picking from wwan_create_dev */ + mutex_lock(&wwan_register_lock); + + /* WWAN device is created and registered (get+add) along with its first + * child port, and subsequent port registrations only grab a reference + * (get). The WWAN device must then be unregistered (del+put) along with + * its latest port, and reference simply dropped (put) otherwise. + */ + ret = device_for_each_child(&wwandev->dev, NULL, is_wwan_child); + if (!ret) + device_unregister(&wwandev->dev); + else + put_device(&wwandev->dev); + + mutex_unlock(&wwan_register_lock); +} + +/* ------- WWAN port management ------- */ + +static void wwan_port_destroy(struct device *dev) +{ + struct wwan_port *port = to_wwan_port(dev); + + ida_free(&minors, MINOR(port->dev.devt)); + skb_queue_purge(&port->rxq); + mutex_destroy(&port->ops_lock); + kfree(port); +} + +static const struct device_type wwan_port_dev_type = { + .name = "wwan_port", + .release = wwan_port_destroy, +}; + +static int wwan_port_minor_match(struct device *dev, const void *minor) +{ + return (dev->type == &wwan_port_dev_type && + MINOR(dev->devt) == *(unsigned int *)minor); +} + +static struct wwan_port *wwan_port_get_by_minor(unsigned int minor) +{ + struct device *dev; + + dev = class_find_device(wwan_class, NULL, &minor, wwan_port_minor_match); + if (!dev) + return ERR_PTR(-ENODEV); + + return to_wwan_port(dev); +} + +/* Keep aligned with wwan_port_type enum */ +static const char * const wwan_port_type_str[] = { + "AT", + "MBIM", + "QMI", + "QCDM", + "FIREHOSE" +}; + +struct wwan_port *wwan_create_port(struct device *parent, + enum wwan_port_type type, + const struct wwan_port_ops *ops, + void *drvdata) +{ + struct wwan_device *wwandev; + struct wwan_port *port; + int minor, err = -ENOMEM; + + if (type >= WWAN_PORT_MAX || !ops) + return ERR_PTR(-EINVAL); + + /* A port is always a child of a WWAN device, retrieve (allocate or + * pick) the WWAN device based on the provided parent device. + */ + wwandev = wwan_create_dev(parent); + if (IS_ERR(wwandev)) + return ERR_PTR(PTR_ERR(wwandev)); + + /* A port is exposed as character device, get a minor */ + minor = ida_alloc_range(&minors, 0, WWAN_MAX_MINORS - 1, GFP_KERNEL); + if (minor < 0) + goto error_wwandev_remove; + + port = kzalloc(sizeof(*port), GFP_KERNEL); + if (!port) { + ida_free(&minors, minor); + goto error_wwandev_remove; + } + + port->type = type; + port->ops = ops; + mutex_init(&port->ops_lock); + skb_queue_head_init(&port->rxq); + init_waitqueue_head(&port->waitqueue); + + port->dev.parent = &wwandev->dev; + port->dev.class = wwan_class; + port->dev.type = &wwan_port_dev_type; + port->dev.devt = MKDEV(wwan_major, minor); + dev_set_drvdata(&port->dev, drvdata); + + /* create unique name based on wwan device id, port index and type */ + dev_set_name(&port->dev, "wwan%up%u%s", wwandev->id, + atomic_inc_return(&wwandev->port_id), + wwan_port_type_str[port->type]); + + err = device_register(&port->dev); + if (err) + goto error_put_device; + + return port; + +error_put_device: + put_device(&port->dev); +error_wwandev_remove: + wwan_remove_dev(wwandev); + + return ERR_PTR(err); +} +EXPORT_SYMBOL_GPL(wwan_create_port); + +void wwan_remove_port(struct wwan_port *port) +{ + struct wwan_device *wwandev = to_wwan_dev(port->dev.parent); + + mutex_lock(&port->ops_lock); + if (port->start_count) + port->ops->stop(port); + port->ops = NULL; /* Prevent any new port operations (e.g. from fops) */ + mutex_unlock(&port->ops_lock); + + wake_up_interruptible(&port->waitqueue); + + skb_queue_purge(&port->rxq); + dev_set_drvdata(&port->dev, NULL); + device_unregister(&port->dev); + + /* Release related wwan device */ + wwan_remove_dev(wwandev); +} +EXPORT_SYMBOL_GPL(wwan_remove_port); + +void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb) +{ + skb_queue_tail(&port->rxq, skb); + wake_up_interruptible(&port->waitqueue); +} +EXPORT_SYMBOL_GPL(wwan_port_rx); + +void wwan_port_txon(struct wwan_port *port) +{ + clear_bit(WWAN_PORT_TX_OFF, &port->flags); + wake_up_interruptible(&port->waitqueue); +} +EXPORT_SYMBOL_GPL(wwan_port_txon); + +void wwan_port_txoff(struct wwan_port *port) +{ + set_bit(WWAN_PORT_TX_OFF, &port->flags); +} +EXPORT_SYMBOL_GPL(wwan_port_txoff); + +static int wwan_port_op_start(struct wwan_port *port) +{ + int ret = 0; + + mutex_lock(&port->ops_lock); + if (!port->ops) { /* Port got unplugged */ + ret = -ENODEV; + goto out_unlock; + } + + /* If port is already started, don't start again */ + if (!port->start_count) + ret = port->ops->start(port); + + if (!ret) + port->start_count++; + +out_unlock: + mutex_unlock(&port->ops_lock); + + return ret; +} + +static void wwan_port_op_stop(struct wwan_port *port) +{ + mutex_lock(&port->ops_lock); + port->start_count--; + if (port->ops && !port->start_count) + port->ops->stop(port); + mutex_unlock(&port->ops_lock); +} + +static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb) +{ + int ret; + + mutex_lock(&port->ops_lock); + if (!port->ops) { /* Port got unplugged */ + ret = -ENODEV; + goto out_unlock; + } + + ret = port->ops->tx(port, skb); + +out_unlock: + mutex_unlock(&port->ops_lock); + + return ret; +} + +static bool is_read_blocked(struct wwan_port *port) +{ + return skb_queue_empty(&port->rxq) && port->ops; +} + +static bool is_write_blocked(struct wwan_port *port) +{ + return test_bit(WWAN_PORT_TX_OFF, &port->flags) && port->ops; +} + +static int wwan_wait_rx(struct wwan_port *port, bool nonblock) +{ + if (!is_read_blocked(port)) + return 0; + + if (nonblock) + return -EAGAIN; + + if (wait_event_interruptible(port->waitqueue, !is_read_blocked(port))) + return -ERESTARTSYS; + + return 0; +} + +static int wwan_wait_tx(struct wwan_port *port, bool nonblock) +{ + if (!is_write_blocked(port)) + return 0; + + if (nonblock) + return -EAGAIN; + + if(wait_event_interruptible(port->waitqueue, !is_write_blocked(port))) + return -ERESTARTSYS; + + return 0; +} + +static int wwan_port_fops_open(struct inode *inode, struct file *file) +{ + struct wwan_port *port; + int err = 0; + + port = wwan_port_get_by_minor(iminor(inode)); + if (IS_ERR(port)) + return PTR_ERR(port); + + file->private_data = port; + stream_open(inode, file); + + err = wwan_port_op_start(port); + if (err) + put_device(&port->dev); + + return err; +} + +static int wwan_port_fops_release(struct inode *inode, struct file *filp) +{ + struct wwan_port *port = filp->private_data; + + wwan_port_op_stop(port); + put_device(&port->dev); + + return 0; +} + +static ssize_t wwan_port_fops_read(struct file *filp, char __user *buf, + size_t count, loff_t *ppos) +{ + struct wwan_port *port = filp->private_data; + struct sk_buff *skb; + size_t copied; + int ret; + + ret = wwan_wait_rx(port, !!(filp->f_flags & O_NONBLOCK)); + if (ret) + return ret; + + skb = skb_dequeue(&port->rxq); + if (!skb) + return -EIO; + + copied = min_t(size_t, count, skb->len); + if (copy_to_user(buf, skb->data, copied)) { + kfree_skb(skb); + return -EFAULT; + } + skb_pull(skb, copied); + + /* skb is not fully consumed, keep it in the queue */ + if (skb->len) + skb_queue_head(&port->rxq, skb); + else + consume_skb(skb); + + return copied; +} + +static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf, + size_t count, loff_t *offp) +{ + struct wwan_port *port = filp->private_data; + struct sk_buff *skb; + int ret; + + ret = wwan_wait_tx(port, !!(filp->f_flags & O_NONBLOCK)); + if (ret) + return ret; + + skb = alloc_skb(count, GFP_KERNEL); + if (!skb) + return -ENOMEM; + + if (copy_from_user(skb_put(skb, count), buf, count)) { + kfree_skb(skb); + return -EFAULT; + } + + ret = wwan_port_op_tx(port, skb); + if (ret) { + kfree_skb(skb); + return ret; + } + + return count; +} + +static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait) +{ + struct wwan_port *port = filp->private_data; + __poll_t mask = 0; + + poll_wait(filp, &port->waitqueue, wait); + + if (!is_write_blocked(port)) + mask |= EPOLLOUT | EPOLLWRNORM; + if (!is_read_blocked(port)) + mask |= EPOLLIN | EPOLLRDNORM; + + return mask; +} + +static const struct file_operations wwan_port_fops = { + .owner = THIS_MODULE, + .open = wwan_port_fops_open, + .release = wwan_port_fops_release, + .read = wwan_port_fops_read, + .write = wwan_port_fops_write, + .poll = wwan_port_fops_poll, + .llseek = noop_llseek, +}; + +static int __init wwan_init(void) +{ + wwan_class = class_create(THIS_MODULE, "wwan"); + if (IS_ERR(wwan_class)) + return PTR_ERR(wwan_class); + + /* chrdev used for wwan ports */ + wwan_major = register_chrdev(0, "wwan_port", &wwan_port_fops); + if (wwan_major < 0) { + class_destroy(wwan_class); + return wwan_major; + } + + return 0; +} + +static void __exit wwan_exit(void) +{ + unregister_chrdev(wwan_major, "wwan_port"); + class_destroy(wwan_class); +} + +module_init(wwan_init); +module_exit(wwan_exit); + +MODULE_AUTHOR("Loic Poulain <loic.poulain@linaro.org>"); +MODULE_DESCRIPTION("WWAN core"); +MODULE_LICENSE("GPL v2"); diff --git a/include/linux/wwan.h b/include/linux/wwan.h new file mode 100644 index 0000000..5b552a9 --- /dev/null +++ b/include/linux/wwan.h @@ -0,0 +1,127 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* Copyright (c) 2021, Linaro Ltd <loic.poulain@linaro.org> */ + +#ifndef __WWAN_H +#define __WWAN_H + +#include <linux/device.h> +#include <linux/kernel.h> +#include <linux/skbuff.h> + +/** + * enum wwan_port_type - WWAN port types + * @WWAN_PORT_AT: AT commands + * @WWAN_PORT_MBIM: Mobile Broadband Interface Model control + * @WWAN_PORT_QMI: Qcom modem/MSM interface for modem control + * @WWAN_PORT_QCDM: Qcom Modem diagnostic interface + * @WWAN_PORT_FIREHOSE: XML based command protocol + * @WWAN_PORT_MAX + */ +enum wwan_port_type { + WWAN_PORT_AT, + WWAN_PORT_MBIM, + WWAN_PORT_QMI, + WWAN_PORT_QCDM, + WWAN_PORT_FIREHOSE, + WWAN_PORT_MAX, +}; + +/** + * struct wwan_port - The structure that defines a WWAN port + * @type: Port type + * @start_count: Port start counter + * @flags: Store port state and capabilities + * @ops: Pointer to WWAN port operations + * @ops_lock: Protect port ops + * @dev: Underlying device + * @rxq: Buffer inbound queue + * @waitqueue: The waitqueue for port fops (read/write/poll) + */ +struct wwan_port { + enum wwan_port_type type; + unsigned int start_count; + unsigned long flags; + const struct wwan_port_ops *ops; + struct mutex ops_lock; + struct device dev; + struct sk_buff_head rxq; + wait_queue_head_t waitqueue; +}; + +#define wwan_port_get_drvdata(p) dev_get_drvdata(&(p)->dev) + +/** struct wwan_port_ops - The WWAN port operations + * @start: The routine for starting the WWAN port device. + * @stop: The routine for stopping the WWAN port device. + * @tx: The routine that sends WWAN port protocol data to the device. + * + * The wwan_port_ops structure contains a list of low-level operations + * that control a WWAN port device. All functions are mandatory. + */ +struct wwan_port_ops { + int (*start)(struct wwan_port *); + void (*stop)(struct wwan_port *); + int (*tx)(struct wwan_port *, struct sk_buff *); +}; + +/** + * wwan_create_port - Add a new WWAN port + * @parent: Device to use as parent and shared by all WWAN ports + * @type: WWAN port type + * @ops: WWAN port operations + * @drvdata: Pointer to caller driver data + * + * Allocate and register a new WWAN port. The port will be automatically exposed + * to user as a character device and attached to the right virtual WWAN device, + * based on the parent pointer. The parent pointer is the device shared by all + * components of a same WWAN modem (e.g. USB dev, PCI dev, MHI controller...). + * + * drvdata will be placed in the WWAN port device driver data and can be + * retrieved with wwan_port_get_drvdata(). + * + * This function must be balanced with a call to wwan_remove_port(). + * + * Returns a valid pointer to wwan_port on success or PTR_ERR on failure + */ +struct wwan_port *wwan_create_port(struct device *parent, + enum wwan_port_type type, + const struct wwan_port_ops *ops, + void *drvdata); + +/** + * wwan_remove_port - Remove a WWAN port + * @port: WWAN port to remove + * + * Remove a previously created port. + */ +void wwan_remove_port(struct wwan_port *port); + +/** + * wwan_port_rx - Receive data from the WWAN port + * @port: WWAN port for which data is received + * @skb: Pointer to the rx buffer + * + * A port driver calls this function upon data reception (MBIM, AT...). + */ +void wwan_port_rx(struct wwan_port *port, struct sk_buff *skb); + +/** + * wwan_port_txoff - Stop TX on WWAN port + * @port: WWAN port for which TX must be stopped + * + * Used for TX flow control, a port driver calls this function to indicate TX + * is temporary unavailable (e.g. due to ring buffer fullness). + */ +void wwan_port_txoff(struct wwan_port *port); + + +/** + * wwan_port_txon - Restart TX on WWAN port + * @port: WWAN port for which TX must be restarted + * + * Used for TX flow control, a port driver calls this function to indicate TX + * is available again. + */ +void wwan_port_txon(struct wwan_port *port); + +#endif /* __WWAN_H */
This change introduces initial support for a WWAN framework. Given the complexity and heterogeneity of existing WWAN hardwares and interfaces, there is no strict definition of what a WWAN device is and how it should be represented. It's often a collection of multiple devices that perform the global WWAN feature (netdev, tty, chardev, etc). One usual way to expose modem controls and configuration is via high level protocols such as the well known AT command protocol, MBIM or QMI. The USB modems started to expose that as character devices, and user daemons such as ModemManager learnt how to deal with them. This initial version adds the concept of WWAN port, which can be created by any driver to expose one of these protocols. The WWAN core takes care of the generic part, including character device management, and rely on port operations to received and submit protocol data. Since the different components/devices do no necesserarly know about each others, and can be created/removed in different orders, the WWAN core ensures that all WAN ports that contribute to the 'whole' WWAN feature are grouped under the same virtual WWAN device, relying on the provided parent device (e.g. mhi controller, USB device). It's a 'trick' I copied from Johannes's earlier WWAN subsystem proposal. This initial version is purposely minimalist, it's essentially moving the generic part of the previously proposed mhi_wwan_ctrl driver inside a common WWAN framework, but the implementation is open and flexible enough to allow extension for further drivers. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> --- v2: not part of the series v3: not part of the series v4: Introduce WWAN framework/subsystem v5: Specify WWAN_CORE module name in Kconfig v6: - Move to unified wwan_core.c file - Make wwan_port a device - Get rid of useless various dev lists (use wwan class) - Get rid of useless wwan dev usage counter and mutex - do not expose wwan_create_device, it's indirectly called via create_port - Increase minor count to the whole available minor range - private_data as wwan_create_port parameter v7: Fix change log (mixed up 1/2 and 2/2) v8: - Move fops implementation in wwan_core (open/read/write/poll/release) - Create wwan_port_ops - Add wwan_port_rx, wwan_port_txoff and wwan_port_txon functions - Fix code comments - skb based TX/RX drivers/net/Kconfig | 2 + drivers/net/Makefile | 1 + drivers/net/wwan/Kconfig | 22 ++ drivers/net/wwan/Makefile | 7 + drivers/net/wwan/wwan_core.c | 524 +++++++++++++++++++++++++++++++++++++++++++ include/linux/wwan.h | 127 +++++++++++ 6 files changed, 683 insertions(+) create mode 100644 drivers/net/wwan/Kconfig create mode 100644 drivers/net/wwan/Makefile create mode 100644 drivers/net/wwan/wwan_core.c create mode 100644 include/linux/wwan.h -- 2.7.4