[PATCH net-next 0/3] net: wwan: Add RPMSG WWAN CTRL driver

[PATCH net-next 0/3] net: wwan: Add RPMSG WWAN CTRL driver

[Stephan Gerhold]

This patch series adds a WWAN "control" driver for the remote processor
messaging (rpmsg) subsystem. This subsystem allows communicating with
an integrated modem DSP on many Qualcomm SoCs, e.g. MSM8916 or MSM8974.

The driver is a fairly simple glue layer between WWAN and RPMSG
and is mostly based on the existing mhi_wwan_ctrl.c and rpmsg_char.c.

For more information, see commit message in PATCH 2/3.

I already posted a RFC for this a while ago:
https://lore.kernel.org/linux-arm-msm/YLfL9Q+4860uqS8f@gerhold.net/
and now I'm looking for some feedback for the actual changes. :)

Especially patch 3/3 is still up for discussion, perhaps there is a cleaner
way to implement the blocking/non-blocking writes for rpmsg_wwan_ctrl?

Stephan Gerhold (3):
  rpmsg: core: Add driver_data for rpmsg_device_id
  net: wwan: Add RPMSG WWAN CTRL driver
  net: wwan: Allow WWAN drivers to provide blocking tx and poll function

 MAINTAINERS                           |   7 ++
 drivers/net/wwan/Kconfig              |  18 +++
 drivers/net/wwan/Makefile             |   1 +
 drivers/net/wwan/iosm/iosm_ipc_port.c |   3 +-
 drivers/net/wwan/mhi_wwan_ctrl.c      |   3 +-
 drivers/net/wwan/rpmsg_wwan_ctrl.c    | 156 ++++++++++++++++++++++++++
 drivers/net/wwan/wwan_core.c          |   9 +-
 drivers/net/wwan/wwan_hwsim.c         |   3 +-
 drivers/rpmsg/rpmsg_core.c            |   4 +-
 include/linux/mod_devicetable.h       |   1 +
 include/linux/wwan.h                  |  13 ++-
 11 files changed, 207 insertions(+), 11 deletions(-)
 create mode 100644 drivers/net/wwan/rpmsg_wwan_ctrl.c

-- 
2.32.0

[PATCH net-next 1/3] rpmsg: core: Add driver_data for rpmsg_device_id

[Stephan Gerhold]

Most device_id structs provide a driver_data field that can be used
by drivers to associate data more easily for a particular device ID.
Add the same for the rpmsg_device_id.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/rpmsg/rpmsg_core.c      | 4 +++-
 include/linux/mod_devicetable.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/rpmsg/rpmsg_core.c b/drivers/rpmsg/rpmsg_core.c
index e5daee4f9373..c1404d3dae2c 100644
--- a/drivers/rpmsg/rpmsg_core.c
+++ b/drivers/rpmsg/rpmsg_core.c
@@ -459,8 +459,10 @@ static int rpmsg_dev_match(struct device *dev, struct device_driver *drv)
 
 	if (ids)
 		for (i = 0; ids[i].name[0]; i++)
-			if (rpmsg_id_match(rpdev, &ids[i]))
+			if (rpmsg_id_match(rpdev, &ids[i])) {
+				rpdev->id.driver_data = ids[i].driver_data;
 				return 1;
+			}
 
 	return of_driver_match_device(dev, drv);
 }
diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
index 7d45b5f989b0..8e291cfdaf06 100644
--- a/include/linux/mod_devicetable.h
+++ b/include/linux/mod_devicetable.h
@@ -447,6 +447,7 @@ struct hv_vmbus_device_id {
 
 struct rpmsg_device_id {
 	char name[RPMSG_NAME_SIZE];
+	kernel_ulong_t driver_data;
 };
 
 /* i2c */
-- 
2.32.0

[PATCH net-next 2/3] net: wwan: Add RPMSG WWAN CTRL driver

[Stephan Gerhold]

The remote processor messaging (rpmsg) subsystem provides an interface
to communicate with other remote processors. On many Qualcomm SoCs this
is used to communicate with an integrated modem DSP that implements most
of the modem functionality and provides high-level protocols like
QMI or AT to allow controlling the modem.

For QMI, most older Qualcomm SoCs (e.g. MSM8916/MSM8974) have
a standalone "DATA5_CNTL" channel that allows exchanging QMI messages.
Note that newer SoCs (e.g. SDM845) only allow exchanging QMI messages
via a shared QRTR channel that is available via a socket API on Linux.

For AT, the "DATA4" channel accepts at least a limited set of AT
commands, on many older and newer Qualcomm SoCs, although QMI is
typically the preferred control protocol.

Note that the data path (network interface) is entirely separate
from the control path and varies between Qualcomm SoCs, e.g. "IPA"
on newer Qualcomm SoCs or "BAM-DMUX" on some older ones.

The RPMSG WWAN CTRL driver exposes the QMI/AT control ports via the
WWAN subsystem, and therefore allows userspace like ModemManager to
set up the modem. Until now, ModemManager had to use the RPMSG-specific
rpmsg-char where the channels must be explicitly exposed as a char
device first and don't show up directly in sysfs.

The driver is a fairly simple glue layer between WWAN and RPMSG
and is mostly based on the existing mhi_wwan_ctrl.c and rpmsg_char.c.

Cc: Loic Poulain <loic.poulain@linaro.org>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
I have mainly tested this driver on Qualcomm MSM8916 with the qcom_smd
RPMSG provider, together with both ModemManager and oFono in userspace.

Note that this driver can also work somewhat with the "glink" RPMSG provider
on newer SoCs (mainly for AT ports), but for some reason dynamically opening
and closing channels like this driver and rpmsg-char do is horribly broken
there. I'm hoping someone with more experience and hardware can fix that later.
---
 MAINTAINERS                        |   7 ++
 drivers/net/wwan/Kconfig           |  18 ++++
 drivers/net/wwan/Makefile          |   1 +
 drivers/net/wwan/rpmsg_wwan_ctrl.c | 143 +++++++++++++++++++++++++++++
 4 files changed, 169 insertions(+)
 create mode 100644 drivers/net/wwan/rpmsg_wwan_ctrl.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 183cc61e2dc0..fbf792962d7b 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15587,6 +15587,13 @@ F:	include/linux/rpmsg/
 F:	include/uapi/linux/rpmsg.h
 F:	samples/rpmsg/
 
+REMOTE PROCESSOR MESSAGING (RPMSG) WWAN CONTROL DRIVER
+M:	Stephan Gerhold <stephan@gerhold.net>
+L:	netdev@vger.kernel.org
+L:	linux-remoteproc@vger.kernel.org
+S:	Maintained
+F:	drivers/net/wwan/rpmsg_wwan_ctrl.c
+
 RENESAS CLOCK DRIVERS
 M:	Geert Uytterhoeven <geert+renesas@glider.be>
 L:	linux-renesas-soc@vger.kernel.org
diff --git a/drivers/net/wwan/Kconfig b/drivers/net/wwan/Kconfig
index 249b3f1ed62b..de9384326bc8 100644
--- a/drivers/net/wwan/Kconfig
+++ b/drivers/net/wwan/Kconfig
@@ -38,6 +38,24 @@ config MHI_WWAN_CTRL
 	  To compile this driver as a module, choose M here: the module will be
 	  called mhi_wwan_ctrl.
 
+config RPMSG_WWAN_CTRL
+	tristate "RPMSG WWAN control driver"
+	depends on RPMSG
+	help
+	  RPMSG WWAN CTRL allows modems available via RPMSG channels to expose
+	  different modem protocols/ports to userspace, including AT and QMI.
+	  These protocols can be accessed directly from userspace
+	  (e.g. AT commands) or via libraries/tools (e.g. libqmi, libqcdm...).
+
+	  This is mainly used for modems integrated into many Qualcomm SoCs,
+	  e.g. for AT and QMI on Qualcomm MSM8916 or MSM8974. Note that many
+	  newer Qualcomm SoCs (e.g. SDM845) still provide an AT port through
+	  this driver but the QMI messages can only be sent through
+	  QRTR network sockets (CONFIG_QRTR).
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called rpmsg_wwan_ctrl.
+
 config IOSM
 	tristate "IOSM Driver for Intel M.2 WWAN Device"
 	depends on INTEL_IOMMU
diff --git a/drivers/net/wwan/Makefile b/drivers/net/wwan/Makefile
index 83dd3482ffc3..d90ac33abaef 100644
--- a/drivers/net/wwan/Makefile
+++ b/drivers/net/wwan/Makefile
@@ -9,4 +9,5 @@ wwan-objs += wwan_core.o
 obj-$(CONFIG_WWAN_HWSIM) += wwan_hwsim.o
 
 obj-$(CONFIG_MHI_WWAN_CTRL) += mhi_wwan_ctrl.o
+obj-$(CONFIG_RPMSG_WWAN_CTRL) += rpmsg_wwan_ctrl.o
 obj-$(CONFIG_IOSM) += iosm/
diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
new file mode 100644
index 000000000000..de226cdb69fd
--- /dev/null
+++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2021, Stephan Gerhold <stephan@gerhold.net> */
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/rpmsg.h>
+#include <linux/wwan.h>
+
+struct rpmsg_wwan_dev {
+	/* Lower level is a rpmsg dev, upper level is a wwan port */
+	struct rpmsg_device *rpdev;
+	struct wwan_port *wwan_port;
+	struct rpmsg_endpoint *ept;
+};
+
+static int rpmsg_wwan_ctrl_callback(struct rpmsg_device *rpdev,
+				    void *buf, int len, void *priv, u32 src)
+{
+	struct rpmsg_wwan_dev *rpwwan = priv;
+	struct sk_buff *skb;
+
+	skb = alloc_skb(len, GFP_ATOMIC);
+	if (!skb)
+		return -ENOMEM;
+
+	skb_put_data(skb, buf, len);
+	wwan_port_rx(rpwwan->wwan_port, skb);
+	return 0;
+}
+
+static int rpmsg_wwan_ctrl_start(struct wwan_port *port)
+{
+	struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
+	struct rpmsg_channel_info chinfo = {
+		.src = rpwwan->rpdev->src,
+		.dst = RPMSG_ADDR_ANY,
+	};
+
+	strncpy(chinfo.name, rpwwan->rpdev->id.name, RPMSG_NAME_SIZE);
+	rpwwan->ept = rpmsg_create_ept(rpwwan->rpdev, rpmsg_wwan_ctrl_callback,
+				       rpwwan, chinfo);
+	if (!rpwwan->ept)
+		return -EREMOTEIO;
+
+	return 0;
+}
+
+static void rpmsg_wwan_ctrl_stop(struct wwan_port *port)
+{
+	struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
+
+	rpmsg_destroy_ept(rpwwan->ept);
+	rpwwan->ept = NULL;
+}
+
+static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
+{
+	struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
+	int ret;
+
+	ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
+	if (ret)
+		return ret;
+
+	consume_skb(skb);
+	return 0;
+}
+
+static const struct wwan_port_ops rpmsg_wwan_pops = {
+	.start = rpmsg_wwan_ctrl_start,
+	.stop = rpmsg_wwan_ctrl_stop,
+	.tx = rpmsg_wwan_ctrl_tx,
+};
+
+static struct device *rpmsg_wwan_find_parent(struct device *dev)
+{
+	/* Select first platform device as parent for the WWAN ports.
+	 * On Qualcomm platforms this is usually the platform device that
+	 * represents the modem remote processor. This might need to be
+	 * adjusted when adding device IDs for other platforms.
+	 */
+	for (dev = dev->parent; dev; dev = dev->parent) {
+		if (dev_is_platform(dev))
+			return dev;
+	}
+	return NULL;
+}
+
+static int rpmsg_wwan_ctrl_probe(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_wwan_dev *rpwwan;
+	struct wwan_port *port;
+	struct device *parent;
+
+	parent = rpmsg_wwan_find_parent(&rpdev->dev);
+	if (!parent)
+		return -ENODEV;
+
+	rpwwan = devm_kzalloc(&rpdev->dev, sizeof(*rpwwan), GFP_KERNEL);
+	if (!rpwwan)
+		return -ENOMEM;
+
+	rpwwan->rpdev = rpdev;
+	dev_set_drvdata(&rpdev->dev, rpwwan);
+
+	/* Register as a wwan port, id.driver_data contains wwan port type */
+	port = wwan_create_port(parent, rpdev->id.driver_data,
+				&rpmsg_wwan_pops, rpwwan);
+	if (IS_ERR(port))
+		return PTR_ERR(port);
+
+	rpwwan->wwan_port = port;
+
+	return 0;
+};
+
+static void rpmsg_wwan_ctrl_remove(struct rpmsg_device *rpdev)
+{
+	struct rpmsg_wwan_dev *rpwwan = dev_get_drvdata(&rpdev->dev);
+
+	wwan_remove_port(rpwwan->wwan_port);
+}
+
+static const struct rpmsg_device_id rpmsg_wwan_ctrl_id_table[] = {
+	/* RPMSG channels for Qualcomm SoCs with integrated modem */
+	{ .name = "DATA5_CNTL", .driver_data = WWAN_PORT_QMI },
+	{ .name = "DATA4", .driver_data = WWAN_PORT_AT },
+	{},
+};
+MODULE_DEVICE_TABLE(rpmsg, rpmsg_wwan_ctrl_id_table);
+
+static struct rpmsg_driver rpmsg_wwan_ctrl_driver = {
+	.drv.name = "rpmsg_wwan_ctrl",
+	.id_table = rpmsg_wwan_ctrl_id_table,
+	.probe = rpmsg_wwan_ctrl_probe,
+	.remove = rpmsg_wwan_ctrl_remove,
+};
+module_rpmsg_driver(rpmsg_wwan_ctrl_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("RPMSG WWAN CTRL Driver");
+MODULE_AUTHOR("Stephan Gerhold <stephan@gerhold.net>");
-- 
2.32.0

[PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function

[Stephan Gerhold]

At the moment, the WWAN core provides wwan_port_txon/off() to implement
blocking writes. The tx() port operation should not block, instead
wwan_port_txon/off() should be called when the TX queue is full or has
free space again.

However, in some cases it is not straightforward to make use of that
functionality. For example, the RPMSG API used by rpmsg_wwan_ctrl.c
does not provide any way to be notified when the TX queue has space
again. Instead, it only provides the following operations:

  - rpmsg_send(): blocking write (wait until there is space)
  - rpmsg_trysend(): non-blocking write (return error if no space)
  - rpmsg_poll(): set poll flags depending on TX queue state

Generally that's totally sufficient for implementing a char device,
but it does not fit well to the currently provided WWAN port ops.

Most of the time, using the non-blocking rpmsg_trysend() in the
WWAN tx() port operation works just fine. However, with high-frequent
writes to the char device it is possible to trigger a situation
where this causes issues. For example, consider the following
(somewhat unrealistic) example:

 # dd if=/dev/zero bs=1000 of=/dev/wwan0p2QMI
 dd: error writing '/dev/wwan0p2QMI': Resource temporarily unavailable
 1+0 records out

This fails immediately after writing the first record. It's likely
only a matter of time until this triggers issues for some real application
(e.g. ModemManager sending a lot of large QMI packets).

The rpmsg_char device does not have this problem, because it uses
rpmsg_trysend() and rpmsg_poll() to support non-blocking operations.
Make it possible to use the same in the RPMSG WWAN driver by extending
the tx() operation with a "nonblock" parameter and adding an optional
poll() callback. This integrates nicely with the RPMSG API and does
not break other WWAN drivers.

With these changes, the dd example above blocks instead of exiting
with an error.

Cc: Loic Poulain <loic.poulain@linaro.org>
Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
Note that rpmsg_poll() is an optional callback currently only implemented
by the qcom_smd RPMSG provider. However, it should be easy to implement
this for other RPMSG providers when needed.

Another potential solution suggested by Loic Poulain in [1] is to always
use the blocking rpmsg_send() from a workqueue/kthread and disable TX
until it is done. I think this could also work (perhaps a bit more
difficult to implement) but the main disadvantage is that I don't see
a way to return any kind of error to the client with this approach.
I assume we return immediately from the write() to the char device
after scheduling the rpmsg_send(), so we already reported success
when rpmsg_send() returns.

At the end all that matters to me is that it works properly, so I'm
open for any other suggestions. :)

[1]: https://lore.kernel.org/linux-arm-msm/CAMZdPi_-Qa=JnThHs_h-144dAfSAjF5s+QdBawdXZ3kk8Mx8ng@mail.gmail.com/
---
 drivers/net/wwan/iosm/iosm_ipc_port.c |  3 ++-
 drivers/net/wwan/mhi_wwan_ctrl.c      |  3 ++-
 drivers/net/wwan/rpmsg_wwan_ctrl.c    | 17 +++++++++++++++--
 drivers/net/wwan/wwan_core.c          |  9 ++++++---
 drivers/net/wwan/wwan_hwsim.c         |  3 ++-
 include/linux/wwan.h                  | 13 +++++++++----
 6 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/drivers/net/wwan/iosm/iosm_ipc_port.c b/drivers/net/wwan/iosm/iosm_ipc_port.c
index beb944847398..2f874e41ceff 100644
--- a/drivers/net/wwan/iosm/iosm_ipc_port.c
+++ b/drivers/net/wwan/iosm/iosm_ipc_port.c
@@ -31,7 +31,8 @@ static void ipc_port_ctrl_stop(struct wwan_port *port)
 }
 
 /* transfer control data to modem */
-static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
+static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
+			    bool nonblock)
 {
 	struct iosm_cdev *ipc_port = wwan_port_get_drvdata(port);
 
diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
index 1bc6b69aa530..9754f014d348 100644
--- a/drivers/net/wwan/mhi_wwan_ctrl.c
+++ b/drivers/net/wwan/mhi_wwan_ctrl.c
@@ -139,7 +139,8 @@ static void mhi_wwan_ctrl_stop(struct wwan_port *port)
 	mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
 }
 
-static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
+static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
+			    bool nonblock)
 {
 	struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
 	int ret;
diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
index de226cdb69fd..63f431eada39 100644
--- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
+++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
@@ -54,12 +54,16 @@ static void rpmsg_wwan_ctrl_stop(struct wwan_port *port)
 	rpwwan->ept = NULL;
 }
 
-static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
+static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
+			      bool nonblock)
 {
 	struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
 	int ret;
 
-	ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
+	if (nonblock)
+		ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
+	else
+		ret = rpmsg_send(rpwwan->ept, skb->data, skb->len);
 	if (ret)
 		return ret;
 
@@ -67,10 +71,19 @@ static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
 	return 0;
 }
 
+static __poll_t rpmsg_wwan_ctrl_poll(struct wwan_port *port, struct file *filp,
+				     poll_table *wait)
+{
+	struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
+
+	return rpmsg_poll(rpwwan->ept, filp, wait);
+}
+
 static const struct wwan_port_ops rpmsg_wwan_pops = {
 	.start = rpmsg_wwan_ctrl_start,
 	.stop = rpmsg_wwan_ctrl_stop,
 	.tx = rpmsg_wwan_ctrl_tx,
+	.poll = rpmsg_wwan_ctrl_poll,
 };
 
 static struct device *rpmsg_wwan_find_parent(struct device *dev)
diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
index 7e728042fc41..c7fd0b897f87 100644
--- a/drivers/net/wwan/wwan_core.c
+++ b/drivers/net/wwan/wwan_core.c
@@ -500,7 +500,8 @@ static void wwan_port_op_stop(struct wwan_port *port)
 	mutex_unlock(&port->ops_lock);
 }
 
-static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
+static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
+			   bool nonblock)
 {
 	int ret;
 
@@ -510,7 +511,7 @@ static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
 		goto out_unlock;
 	}
 
-	ret = port->ops->tx(port, skb);
+	ret = port->ops->tx(port, skb, nonblock);
 
 out_unlock:
 	mutex_unlock(&port->ops_lock);
@@ -637,7 +638,7 @@ static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf,
 		return -EFAULT;
 	}
 
-	ret = wwan_port_op_tx(port, skb);
+	ret = wwan_port_op_tx(port, skb, !!(filp->f_flags & O_NONBLOCK));
 	if (ret) {
 		kfree_skb(skb);
 		return ret;
@@ -659,6 +660,8 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
 		mask |= EPOLLIN | EPOLLRDNORM;
 	if (!port->ops)
 		mask |= EPOLLHUP | EPOLLERR;
+	else if (port->ops->poll)
+		mask |= port->ops->poll(port, filp, wait);
 
 	return mask;
 }
diff --git a/drivers/net/wwan/wwan_hwsim.c b/drivers/net/wwan/wwan_hwsim.c
index 472cae544a2b..e5ecbc70658d 100644
--- a/drivers/net/wwan/wwan_hwsim.c
+++ b/drivers/net/wwan/wwan_hwsim.c
@@ -83,7 +83,8 @@ static void wwan_hwsim_port_stop(struct wwan_port *wport)
  *
  * Be aware that this processor is not fully V.250 compliant.
  */
-static int wwan_hwsim_port_tx(struct wwan_port *wport, struct sk_buff *in)
+static int wwan_hwsim_port_tx(struct wwan_port *wport, struct sk_buff *in,
+			      bool nonblock)
 {
 	struct wwan_hwsim_port *port = wwan_port_get_drvdata(wport);
 	struct sk_buff *out;
diff --git a/include/linux/wwan.h b/include/linux/wwan.h
index 430a3a0817de..d7c8c6ac7594 100644
--- a/include/linux/wwan.h
+++ b/include/linux/wwan.h
@@ -6,6 +6,7 @@
 
 #include <linux/device.h>
 #include <linux/kernel.h>
+#include <linux/poll.h>
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
 
@@ -38,17 +39,21 @@ enum wwan_port_type {
 struct wwan_port;
 
 /** 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.
+ * @start: The routine for starting the WWAN port device. Required.
+ * @stop: The routine for stopping the WWAN port device. Required.
  * @tx: The routine that sends WWAN port protocol data to the device.
+ *      May only block if nonblock is false. Required.
+ * @poll: A routine to set additional poll flags. Optional.
  *
  * The wwan_port_ops structure contains a list of low-level operations
- * that control a WWAN port device. All functions are mandatory.
+ * that control a WWAN port device.
  */
 struct wwan_port_ops {
 	int (*start)(struct wwan_port *port);
 	void (*stop)(struct wwan_port *port);
-	int (*tx)(struct wwan_port *port, struct sk_buff *skb);
+	int (*tx)(struct wwan_port *port, struct sk_buff *skb, bool nonblock);
+	__poll_t (*poll)(struct wwan_port *port, struct file *filp,
+			 poll_table *wait);
 };
 
 /**
-- 
2.32.0

Re: [PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function

[Loic Poulain]

Hi Stephan,

On Tue, 15 Jun 2021 at 15:34, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> At the moment, the WWAN core provides wwan_port_txon/off() to implement
> blocking writes. The tx() port operation should not block, instead
> wwan_port_txon/off() should be called when the TX queue is full or has
> free space again.
>
> However, in some cases it is not straightforward to make use of that
> functionality. For example, the RPMSG API used by rpmsg_wwan_ctrl.c
> does not provide any way to be notified when the TX queue has space
> again. Instead, it only provides the following operations:
>
>   - rpmsg_send(): blocking write (wait until there is space)
>   - rpmsg_trysend(): non-blocking write (return error if no space)
>   - rpmsg_poll(): set poll flags depending on TX queue state
>
> Generally that's totally sufficient for implementing a char device,
> but it does not fit well to the currently provided WWAN port ops.
>
> Most of the time, using the non-blocking rpmsg_trysend() in the
> WWAN tx() port operation works just fine. However, with high-frequent
> writes to the char device it is possible to trigger a situation
> where this causes issues. For example, consider the following
> (somewhat unrealistic) example:
>
>  # dd if=/dev/zero bs=1000 of=/dev/wwan0p2QMI
>  dd: error writing '/dev/wwan0p2QMI': Resource temporarily unavailable
>  1+0 records out
>
> This fails immediately after writing the first record. It's likely
> only a matter of time until this triggers issues for some real application
> (e.g. ModemManager sending a lot of large QMI packets).
>
> The rpmsg_char device does not have this problem, because it uses
> rpmsg_trysend() and rpmsg_poll() to support non-blocking operations.
> Make it possible to use the same in the RPMSG WWAN driver by extending
> the tx() operation with a "nonblock" parameter and adding an optional
> poll() callback. This integrates nicely with the RPMSG API and does
> not break other WWAN drivers.
>
> With these changes, the dd example above blocks instead of exiting
> with an error.
>
> Cc: Loic Poulain <loic.poulain@linaro.org>
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
> Note that rpmsg_poll() is an optional callback currently only implemented
> by the qcom_smd RPMSG provider. However, it should be easy to implement
> this for other RPMSG providers when needed.
>
> Another potential solution suggested by Loic Poulain in [1] is to always
> use the blocking rpmsg_send() from a workqueue/kthread and disable TX
> until it is done. I think this could also work (perhaps a bit more
> difficult to implement) but the main disadvantage is that I don't see
> a way to return any kind of error to the client with this approach.
> I assume we return immediately from the write() to the char device
> after scheduling the rpmsg_send(), so we already reported success
> when rpmsg_send() returns.
>
> At the end all that matters to me is that it works properly, so I'm
> open for any other suggestions. :)
>
> [1]: https://lore.kernel.org/linux-arm-msm/CAMZdPi_-Qa=JnThHs_h-144dAfSAjF5s+QdBawdXZ3kk8Mx8ng@mail.gmail.com/
> ---
>  drivers/net/wwan/iosm/iosm_ipc_port.c |  3 ++-
>  drivers/net/wwan/mhi_wwan_ctrl.c      |  3 ++-
>  drivers/net/wwan/rpmsg_wwan_ctrl.c    | 17 +++++++++++++++--
>  drivers/net/wwan/wwan_core.c          |  9 ++++++---
>  drivers/net/wwan/wwan_hwsim.c         |  3 ++-
>  include/linux/wwan.h                  | 13 +++++++++----
>  6 files changed, 36 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/wwan/iosm/iosm_ipc_port.c b/drivers/net/wwan/iosm/iosm_ipc_port.c
> index beb944847398..2f874e41ceff 100644
> --- a/drivers/net/wwan/iosm/iosm_ipc_port.c
> +++ b/drivers/net/wwan/iosm/iosm_ipc_port.c
> @@ -31,7 +31,8 @@ static void ipc_port_ctrl_stop(struct wwan_port *port)
>  }
>
>  /* transfer control data to modem */
> -static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> +static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> +                           bool nonblock)
>  {
>         struct iosm_cdev *ipc_port = wwan_port_get_drvdata(port);
>
> diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> index 1bc6b69aa530..9754f014d348 100644
> --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> @@ -139,7 +139,8 @@ static void mhi_wwan_ctrl_stop(struct wwan_port *port)
>         mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
>  }
>
> -static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> +static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> +                           bool nonblock)
>  {
>         struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
>         int ret;
> diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> index de226cdb69fd..63f431eada39 100644
> --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
> +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> @@ -54,12 +54,16 @@ static void rpmsg_wwan_ctrl_stop(struct wwan_port *port)
>         rpwwan->ept = NULL;
>  }
>
> -static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> +static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> +                             bool nonblock)
>  {
>         struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
>         int ret;
>
> -       ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> +       if (nonblock)
> +               ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> +       else
> +               ret = rpmsg_send(rpwwan->ept, skb->data, skb->len);
>         if (ret)
>                 return ret;
>
> @@ -67,10 +71,19 @@ static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
>         return 0;
>  }
>
> +static __poll_t rpmsg_wwan_ctrl_poll(struct wwan_port *port, struct file *filp,
> +                                    poll_table *wait)
> +{
> +       struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
> +
> +       return rpmsg_poll(rpwwan->ept, filp, wait);
> +}
> +
>  static const struct wwan_port_ops rpmsg_wwan_pops = {
>         .start = rpmsg_wwan_ctrl_start,
>         .stop = rpmsg_wwan_ctrl_stop,
>         .tx = rpmsg_wwan_ctrl_tx,
> +       .poll = rpmsg_wwan_ctrl_poll,
>  };
>
>  static struct device *rpmsg_wwan_find_parent(struct device *dev)
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index 7e728042fc41..c7fd0b897f87 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -500,7 +500,8 @@ static void wwan_port_op_stop(struct wwan_port *port)
>         mutex_unlock(&port->ops_lock);
>  }
>
> -static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
> +static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
> +                          bool nonblock)
>  {
>         int ret;
>
> @@ -510,7 +511,7 @@ static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
>                 goto out_unlock;
>         }
>
> -       ret = port->ops->tx(port, skb);
> +       ret = port->ops->tx(port, skb, nonblock);
>
>  out_unlock:
>         mutex_unlock(&port->ops_lock);
> @@ -637,7 +638,7 @@ static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf,
>                 return -EFAULT;
>         }
>
> -       ret = wwan_port_op_tx(port, skb);
> +       ret = wwan_port_op_tx(port, skb, !!(filp->f_flags & O_NONBLOCK));
>         if (ret) {
>                 kfree_skb(skb);
>                 return ret;
> @@ -659,6 +660,8 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
>                 mask |= EPOLLIN | EPOLLRDNORM;
>         if (!port->ops)
>                 mask |= EPOLLHUP | EPOLLERR;
> +       else if (port->ops->poll)
> +               mask |= port->ops->poll(port, filp, wait);

I'm not sure it useful here because EPOLLOUT flag is already set above, right?

>
>         return mask;
>  }

Regards,
Loic

Re: [PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function

[Stephan Gerhold]

On Tue, Jun 15, 2021 at 11:24:41PM +0200, Loic Poulain wrote:
> On Tue, 15 Jun 2021 at 15:34, Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > At the moment, the WWAN core provides wwan_port_txon/off() to implement
> > blocking writes. The tx() port operation should not block, instead
> > wwan_port_txon/off() should be called when the TX queue is full or has
> > free space again.
> >
> > However, in some cases it is not straightforward to make use of that
> > functionality. For example, the RPMSG API used by rpmsg_wwan_ctrl.c
> > does not provide any way to be notified when the TX queue has space
> > again. Instead, it only provides the following operations:
> >
> >   - rpmsg_send(): blocking write (wait until there is space)
> >   - rpmsg_trysend(): non-blocking write (return error if no space)
> >   - rpmsg_poll(): set poll flags depending on TX queue state
> >
> > Generally that's totally sufficient for implementing a char device,
> > but it does not fit well to the currently provided WWAN port ops.
> >
> > Most of the time, using the non-blocking rpmsg_trysend() in the
> > WWAN tx() port operation works just fine. However, with high-frequent
> > writes to the char device it is possible to trigger a situation
> > where this causes issues. For example, consider the following
> > (somewhat unrealistic) example:
> >
> >  # dd if=/dev/zero bs=1000 of=/dev/wwan0p2QMI
> >  dd: error writing '/dev/wwan0p2QMI': Resource temporarily unavailable
> >  1+0 records out
> >
> > This fails immediately after writing the first record. It's likely
> > only a matter of time until this triggers issues for some real application
> > (e.g. ModemManager sending a lot of large QMI packets).
> >
> > The rpmsg_char device does not have this problem, because it uses
> > rpmsg_trysend() and rpmsg_poll() to support non-blocking operations.
> > Make it possible to use the same in the RPMSG WWAN driver by extending
> > the tx() operation with a "nonblock" parameter and adding an optional
> > poll() callback. This integrates nicely with the RPMSG API and does
> > not break other WWAN drivers.
> >
> > With these changes, the dd example above blocks instead of exiting
> > with an error.
> >
> > Cc: Loic Poulain <loic.poulain@linaro.org>
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> > Note that rpmsg_poll() is an optional callback currently only implemented
> > by the qcom_smd RPMSG provider. However, it should be easy to implement
> > this for other RPMSG providers when needed.
> >
> > Another potential solution suggested by Loic Poulain in [1] is to always
> > use the blocking rpmsg_send() from a workqueue/kthread and disable TX
> > until it is done. I think this could also work (perhaps a bit more
> > difficult to implement) but the main disadvantage is that I don't see
> > a way to return any kind of error to the client with this approach.
> > I assume we return immediately from the write() to the char device
> > after scheduling the rpmsg_send(), so we already reported success
> > when rpmsg_send() returns.
> >
> > At the end all that matters to me is that it works properly, so I'm
> > open for any other suggestions. :)
> >
> > [1]: https://lore.kernel.org/linux-arm-msm/CAMZdPi_-Qa=JnThHs_h-144dAfSAjF5s+QdBawdXZ3kk8Mx8ng@mail.gmail.com/
> > ---
> >  drivers/net/wwan/iosm/iosm_ipc_port.c |  3 ++-
> >  drivers/net/wwan/mhi_wwan_ctrl.c      |  3 ++-
> >  drivers/net/wwan/rpmsg_wwan_ctrl.c    | 17 +++++++++++++++--
> >  drivers/net/wwan/wwan_core.c          |  9 ++++++---
> >  drivers/net/wwan/wwan_hwsim.c         |  3 ++-
> >  include/linux/wwan.h                  | 13 +++++++++----
> >  6 files changed, 36 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/wwan/iosm/iosm_ipc_port.c b/drivers/net/wwan/iosm/iosm_ipc_port.c
> > index beb944847398..2f874e41ceff 100644
> > --- a/drivers/net/wwan/iosm/iosm_ipc_port.c
> > +++ b/drivers/net/wwan/iosm/iosm_ipc_port.c
> > @@ -31,7 +31,8 @@ static void ipc_port_ctrl_stop(struct wwan_port *port)
> >  }
> >
> >  /* transfer control data to modem */
> > -static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > +static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > +                           bool nonblock)
> >  {
> >         struct iosm_cdev *ipc_port = wwan_port_get_drvdata(port);
> >
> > diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> > index 1bc6b69aa530..9754f014d348 100644
> > --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> > +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> > @@ -139,7 +139,8 @@ static void mhi_wwan_ctrl_stop(struct wwan_port *port)
> >         mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
> >  }
> >
> > -static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > +static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > +                           bool nonblock)
> >  {
> >         struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> >         int ret;
> > diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > index de226cdb69fd..63f431eada39 100644
> > --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > @@ -54,12 +54,16 @@ static void rpmsg_wwan_ctrl_stop(struct wwan_port *port)
> >         rpwwan->ept = NULL;
> >  }
> >
> > -static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > +static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > +                             bool nonblock)
> >  {
> >         struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
> >         int ret;
> >
> > -       ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> > +       if (nonblock)
> > +               ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> > +       else
> > +               ret = rpmsg_send(rpwwan->ept, skb->data, skb->len);
> >         if (ret)
> >                 return ret;
> >
> > @@ -67,10 +71,19 @@ static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> >         return 0;
> >  }
> >
> > +static __poll_t rpmsg_wwan_ctrl_poll(struct wwan_port *port, struct file *filp,
> > +                                    poll_table *wait)
> > +{
> > +       struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
> > +
> > +       return rpmsg_poll(rpwwan->ept, filp, wait);
> > +}
> > +
> >  static const struct wwan_port_ops rpmsg_wwan_pops = {
> >         .start = rpmsg_wwan_ctrl_start,
> >         .stop = rpmsg_wwan_ctrl_stop,
> >         .tx = rpmsg_wwan_ctrl_tx,
> > +       .poll = rpmsg_wwan_ctrl_poll,
> >  };
> >
> >  static struct device *rpmsg_wwan_find_parent(struct device *dev)
> > diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> > index 7e728042fc41..c7fd0b897f87 100644
> > --- a/drivers/net/wwan/wwan_core.c
> > +++ b/drivers/net/wwan/wwan_core.c
> > @@ -500,7 +500,8 @@ static void wwan_port_op_stop(struct wwan_port *port)
> >         mutex_unlock(&port->ops_lock);
> >  }
> >
> > -static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
> > +static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
> > +                          bool nonblock)
> >  {
> >         int ret;
> >
> > @@ -510,7 +511,7 @@ static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
> >                 goto out_unlock;
> >         }
> >
> > -       ret = port->ops->tx(port, skb);
> > +       ret = port->ops->tx(port, skb, nonblock);
> >
> >  out_unlock:
> >         mutex_unlock(&port->ops_lock);
> > @@ -637,7 +638,7 @@ static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf,
> >                 return -EFAULT;
> >         }
> >
> > -       ret = wwan_port_op_tx(port, skb);
> > +       ret = wwan_port_op_tx(port, skb, !!(filp->f_flags & O_NONBLOCK));
> >         if (ret) {
> >                 kfree_skb(skb);
> >                 return ret;
> > @@ -659,6 +660,8 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
> >                 mask |= EPOLLIN | EPOLLRDNORM;
> >         if (!port->ops)
> >                 mask |= EPOLLHUP | EPOLLERR;
> > +       else if (port->ops->poll)
> > +               mask |= port->ops->poll(port, filp, wait);
> 
> I'm not sure it useful here because EPOLLOUT flag is already set above, right?
> 

Oops, you're right - sorry! I thought the flags are inverted (only set
if (is_write_blocked())), then it would have worked fine. :)

I think this should be easy to fix though, I can just make the

	if (!is_write_blocked(port))
		mask |= EPOLLOUT | EPOLLWRNORM;

if statement conditional to (port->ops->poll == NULL). It only makes
sense to supply the poll() op if the built-in write-blocking cannot be
used easily (like in my case).

I can add that in v2, will wait a bit more in case someone has some
other comments or a better suggestion.

Thanks for the review!
Stephan

Re: [PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function

[Loic Poulain]

On Wed, 16 Jun 2021 at 00:06, Stephan Gerhold <stephan@gerhold.net> wrote:
>
> On Tue, Jun 15, 2021 at 11:24:41PM +0200, Loic Poulain wrote:
> > On Tue, 15 Jun 2021 at 15:34, Stephan Gerhold <stephan@gerhold.net> wrote:
> > >
> > > At the moment, the WWAN core provides wwan_port_txon/off() to implement
> > > blocking writes. The tx() port operation should not block, instead
> > > wwan_port_txon/off() should be called when the TX queue is full or has
> > > free space again.
> > >
> > > However, in some cases it is not straightforward to make use of that
> > > functionality. For example, the RPMSG API used by rpmsg_wwan_ctrl.c
> > > does not provide any way to be notified when the TX queue has space
> > > again. Instead, it only provides the following operations:
> > >
> > >   - rpmsg_send(): blocking write (wait until there is space)
> > >   - rpmsg_trysend(): non-blocking write (return error if no space)
> > >   - rpmsg_poll(): set poll flags depending on TX queue state
> > >
> > > Generally that's totally sufficient for implementing a char device,
> > > but it does not fit well to the currently provided WWAN port ops.
> > >
> > > Most of the time, using the non-blocking rpmsg_trysend() in the
> > > WWAN tx() port operation works just fine. However, with high-frequent
> > > writes to the char device it is possible to trigger a situation
> > > where this causes issues. For example, consider the following
> > > (somewhat unrealistic) example:
> > >
> > >  # dd if=/dev/zero bs=1000 of=/dev/wwan0p2QMI
> > >  dd: error writing '/dev/wwan0p2QMI': Resource temporarily unavailable
> > >  1+0 records out
> > >
> > > This fails immediately after writing the first record. It's likely
> > > only a matter of time until this triggers issues for some real application
> > > (e.g. ModemManager sending a lot of large QMI packets).
> > >
> > > The rpmsg_char device does not have this problem, because it uses
> > > rpmsg_trysend() and rpmsg_poll() to support non-blocking operations.
> > > Make it possible to use the same in the RPMSG WWAN driver by extending
> > > the tx() operation with a "nonblock" parameter and adding an optional
> > > poll() callback. This integrates nicely with the RPMSG API and does
> > > not break other WWAN drivers.
> > >
> > > With these changes, the dd example above blocks instead of exiting
> > > with an error.
> > >
> > > Cc: Loic Poulain <loic.poulain@linaro.org>
> > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > ---
> > > Note that rpmsg_poll() is an optional callback currently only implemented
> > > by the qcom_smd RPMSG provider. However, it should be easy to implement
> > > this for other RPMSG providers when needed.
> > >
> > > Another potential solution suggested by Loic Poulain in [1] is to always
> > > use the blocking rpmsg_send() from a workqueue/kthread and disable TX
> > > until it is done. I think this could also work (perhaps a bit more
> > > difficult to implement) but the main disadvantage is that I don't see
> > > a way to return any kind of error to the client with this approach.
> > > I assume we return immediately from the write() to the char device
> > > after scheduling the rpmsg_send(), so we already reported success
> > > when rpmsg_send() returns.
> > >
> > > At the end all that matters to me is that it works properly, so I'm
> > > open for any other suggestions. :)
> > >
> > > [1]: https://lore.kernel.org/linux-arm-msm/CAMZdPi_-Qa=JnThHs_h-144dAfSAjF5s+QdBawdXZ3kk8Mx8ng@mail.gmail.com/
> > > ---
> > >  drivers/net/wwan/iosm/iosm_ipc_port.c |  3 ++-
> > >  drivers/net/wwan/mhi_wwan_ctrl.c      |  3 ++-
> > >  drivers/net/wwan/rpmsg_wwan_ctrl.c    | 17 +++++++++++++++--
> > >  drivers/net/wwan/wwan_core.c          |  9 ++++++---
> > >  drivers/net/wwan/wwan_hwsim.c         |  3 ++-
> > >  include/linux/wwan.h                  | 13 +++++++++----
> > >  6 files changed, 36 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/drivers/net/wwan/iosm/iosm_ipc_port.c b/drivers/net/wwan/iosm/iosm_ipc_port.c
> > > index beb944847398..2f874e41ceff 100644
> > > --- a/drivers/net/wwan/iosm/iosm_ipc_port.c
> > > +++ b/drivers/net/wwan/iosm/iosm_ipc_port.c
> > > @@ -31,7 +31,8 @@ static void ipc_port_ctrl_stop(struct wwan_port *port)
> > >  }
> > >
> > >  /* transfer control data to modem */
> > > -static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > > +static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > > +                           bool nonblock)
> > >  {
> > >         struct iosm_cdev *ipc_port = wwan_port_get_drvdata(port);
> > >
> > > diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > index 1bc6b69aa530..9754f014d348 100644
> > > --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> > > +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > @@ -139,7 +139,8 @@ static void mhi_wwan_ctrl_stop(struct wwan_port *port)
> > >         mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
> > >  }
> > >
> > > -static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > > +static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > > +                           bool nonblock)
> > >  {
> > >         struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> > >         int ret;
> > > diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > > index de226cdb69fd..63f431eada39 100644
> > > --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > > +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > > @@ -54,12 +54,16 @@ static void rpmsg_wwan_ctrl_stop(struct wwan_port *port)
> > >         rpwwan->ept = NULL;
> > >  }
> > >
> > > -static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > > +static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > > +                             bool nonblock)
> > >  {
> > >         struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
> > >         int ret;
> > >
> > > -       ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> > > +       if (nonblock)
> > > +               ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> > > +       else
> > > +               ret = rpmsg_send(rpwwan->ept, skb->data, skb->len);
> > >         if (ret)
> > >                 return ret;
> > >
> > > @@ -67,10 +71,19 @@ static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > >         return 0;
> > >  }
> > >
> > > +static __poll_t rpmsg_wwan_ctrl_poll(struct wwan_port *port, struct file *filp,
> > > +                                    poll_table *wait)
> > > +{
> > > +       struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
> > > +
> > > +       return rpmsg_poll(rpwwan->ept, filp, wait);
> > > +}
> > > +
> > >  static const struct wwan_port_ops rpmsg_wwan_pops = {
> > >         .start = rpmsg_wwan_ctrl_start,
> > >         .stop = rpmsg_wwan_ctrl_stop,
> > >         .tx = rpmsg_wwan_ctrl_tx,
> > > +       .poll = rpmsg_wwan_ctrl_poll,
> > >  };
> > >
> > >  static struct device *rpmsg_wwan_find_parent(struct device *dev)
> > > diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> > > index 7e728042fc41..c7fd0b897f87 100644
> > > --- a/drivers/net/wwan/wwan_core.c
> > > +++ b/drivers/net/wwan/wwan_core.c
> > > @@ -500,7 +500,8 @@ static void wwan_port_op_stop(struct wwan_port *port)
> > >         mutex_unlock(&port->ops_lock);
> > >  }
> > >
> > > -static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
> > > +static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
> > > +                          bool nonblock)
> > >  {
> > >         int ret;
> > >
> > > @@ -510,7 +511,7 @@ static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
> > >                 goto out_unlock;
> > >         }
> > >
> > > -       ret = port->ops->tx(port, skb);
> > > +       ret = port->ops->tx(port, skb, nonblock);
> > >
> > >  out_unlock:
> > >         mutex_unlock(&port->ops_lock);
> > > @@ -637,7 +638,7 @@ static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf,
> > >                 return -EFAULT;
> > >         }
> > >
> > > -       ret = wwan_port_op_tx(port, skb);
> > > +       ret = wwan_port_op_tx(port, skb, !!(filp->f_flags & O_NONBLOCK));
> > >         if (ret) {
> > >                 kfree_skb(skb);
> > >                 return ret;
> > > @@ -659,6 +660,8 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
> > >                 mask |= EPOLLIN | EPOLLRDNORM;
> > >         if (!port->ops)
> > >                 mask |= EPOLLHUP | EPOLLERR;
> > > +       else if (port->ops->poll)
> > > +               mask |= port->ops->poll(port, filp, wait);
> >
> > I'm not sure it useful here because EPOLLOUT flag is already set above, right?
> >
>
> Oops, you're right - sorry! I thought the flags are inverted (only set
> if (is_write_blocked())), then it would have worked fine. :)
>
> I think this should be easy to fix though, I can just make the
>
>         if (!is_write_blocked(port))
>                 mask |= EPOLLOUT | EPOLLWRNORM;
>
> if statement conditional to (port->ops->poll == NULL). It only makes
> sense to supply the poll() op if the built-in write-blocking cannot be
> used easily (like in my case).

Yes, so maybe in that case poll ops should be renamed to something like tx_poll?

Regards,
Loic

Re: [PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function

[Stephan Gerhold]

On Wed, Jun 16, 2021 at 11:28:46AM +0200, Loic Poulain wrote:
> On Wed, 16 Jun 2021 at 00:06, Stephan Gerhold <stephan@gerhold.net> wrote:
> >
> > On Tue, Jun 15, 2021 at 11:24:41PM +0200, Loic Poulain wrote:
> > > On Tue, 15 Jun 2021 at 15:34, Stephan Gerhold <stephan@gerhold.net> wrote:
> > > >
> > > > At the moment, the WWAN core provides wwan_port_txon/off() to implement
> > > > blocking writes. The tx() port operation should not block, instead
> > > > wwan_port_txon/off() should be called when the TX queue is full or has
> > > > free space again.
> > > >
> > > > However, in some cases it is not straightforward to make use of that
> > > > functionality. For example, the RPMSG API used by rpmsg_wwan_ctrl.c
> > > > does not provide any way to be notified when the TX queue has space
> > > > again. Instead, it only provides the following operations:
> > > >
> > > >   - rpmsg_send(): blocking write (wait until there is space)
> > > >   - rpmsg_trysend(): non-blocking write (return error if no space)
> > > >   - rpmsg_poll(): set poll flags depending on TX queue state
> > > >
> > > > Generally that's totally sufficient for implementing a char device,
> > > > but it does not fit well to the currently provided WWAN port ops.
> > > >
> > > > Most of the time, using the non-blocking rpmsg_trysend() in the
> > > > WWAN tx() port operation works just fine. However, with high-frequent
> > > > writes to the char device it is possible to trigger a situation
> > > > where this causes issues. For example, consider the following
> > > > (somewhat unrealistic) example:
> > > >
> > > >  # dd if=/dev/zero bs=1000 of=/dev/wwan0p2QMI
> > > >  dd: error writing '/dev/wwan0p2QMI': Resource temporarily unavailable
> > > >  1+0 records out
> > > >
> > > > This fails immediately after writing the first record. It's likely
> > > > only a matter of time until this triggers issues for some real application
> > > > (e.g. ModemManager sending a lot of large QMI packets).
> > > >
> > > > The rpmsg_char device does not have this problem, because it uses
> > > > rpmsg_trysend() and rpmsg_poll() to support non-blocking operations.
> > > > Make it possible to use the same in the RPMSG WWAN driver by extending
> > > > the tx() operation with a "nonblock" parameter and adding an optional
> > > > poll() callback. This integrates nicely with the RPMSG API and does
> > > > not break other WWAN drivers.
> > > >
> > > > With these changes, the dd example above blocks instead of exiting
> > > > with an error.
> > > >
> > > > Cc: Loic Poulain <loic.poulain@linaro.org>
> > > > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > > > ---
> > > > Note that rpmsg_poll() is an optional callback currently only implemented
> > > > by the qcom_smd RPMSG provider. However, it should be easy to implement
> > > > this for other RPMSG providers when needed.
> > > >
> > > > Another potential solution suggested by Loic Poulain in [1] is to always
> > > > use the blocking rpmsg_send() from a workqueue/kthread and disable TX
> > > > until it is done. I think this could also work (perhaps a bit more
> > > > difficult to implement) but the main disadvantage is that I don't see
> > > > a way to return any kind of error to the client with this approach.
> > > > I assume we return immediately from the write() to the char device
> > > > after scheduling the rpmsg_send(), so we already reported success
> > > > when rpmsg_send() returns.
> > > >
> > > > At the end all that matters to me is that it works properly, so I'm
> > > > open for any other suggestions. :)
> > > >
> > > > [1]: https://lore.kernel.org/linux-arm-msm/CAMZdPi_-Qa=JnThHs_h-144dAfSAjF5s+QdBawdXZ3kk8Mx8ng@mail.gmail.com/
> > > > ---
> > > >  drivers/net/wwan/iosm/iosm_ipc_port.c |  3 ++-
> > > >  drivers/net/wwan/mhi_wwan_ctrl.c      |  3 ++-
> > > >  drivers/net/wwan/rpmsg_wwan_ctrl.c    | 17 +++++++++++++++--
> > > >  drivers/net/wwan/wwan_core.c          |  9 ++++++---
> > > >  drivers/net/wwan/wwan_hwsim.c         |  3 ++-
> > > >  include/linux/wwan.h                  | 13 +++++++++----
> > > >  6 files changed, 36 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wwan/iosm/iosm_ipc_port.c b/drivers/net/wwan/iosm/iosm_ipc_port.c
> > > > index beb944847398..2f874e41ceff 100644
> > > > --- a/drivers/net/wwan/iosm/iosm_ipc_port.c
> > > > +++ b/drivers/net/wwan/iosm/iosm_ipc_port.c
> > > > @@ -31,7 +31,8 @@ static void ipc_port_ctrl_stop(struct wwan_port *port)
> > > >  }
> > > >
> > > >  /* transfer control data to modem */
> > > > -static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > > > +static int ipc_port_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > > > +                           bool nonblock)
> > > >  {
> > > >         struct iosm_cdev *ipc_port = wwan_port_get_drvdata(port);
> > > >
> > > > diff --git a/drivers/net/wwan/mhi_wwan_ctrl.c b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > > index 1bc6b69aa530..9754f014d348 100644
> > > > --- a/drivers/net/wwan/mhi_wwan_ctrl.c
> > > > +++ b/drivers/net/wwan/mhi_wwan_ctrl.c
> > > > @@ -139,7 +139,8 @@ static void mhi_wwan_ctrl_stop(struct wwan_port *port)
> > > >         mhi_unprepare_from_transfer(mhiwwan->mhi_dev);
> > > >  }
> > > >
> > > > -static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > > > +static int mhi_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > > > +                           bool nonblock)
> > > >  {
> > > >         struct mhi_wwan_dev *mhiwwan = wwan_port_get_drvdata(port);
> > > >         int ret;
> > > > diff --git a/drivers/net/wwan/rpmsg_wwan_ctrl.c b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > > > index de226cdb69fd..63f431eada39 100644
> > > > --- a/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > > > +++ b/drivers/net/wwan/rpmsg_wwan_ctrl.c
> > > > @@ -54,12 +54,16 @@ static void rpmsg_wwan_ctrl_stop(struct wwan_port *port)
> > > >         rpwwan->ept = NULL;
> > > >  }
> > > >
> > > > -static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > > > +static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb,
> > > > +                             bool nonblock)
> > > >  {
> > > >         struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
> > > >         int ret;
> > > >
> > > > -       ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> > > > +       if (nonblock)
> > > > +               ret = rpmsg_trysend(rpwwan->ept, skb->data, skb->len);
> > > > +       else
> > > > +               ret = rpmsg_send(rpwwan->ept, skb->data, skb->len);
> > > >         if (ret)
> > > >                 return ret;
> > > >
> > > > @@ -67,10 +71,19 @@ static int rpmsg_wwan_ctrl_tx(struct wwan_port *port, struct sk_buff *skb)
> > > >         return 0;
> > > >  }
> > > >
> > > > +static __poll_t rpmsg_wwan_ctrl_poll(struct wwan_port *port, struct file *filp,
> > > > +                                    poll_table *wait)
> > > > +{
> > > > +       struct rpmsg_wwan_dev *rpwwan = wwan_port_get_drvdata(port);
> > > > +
> > > > +       return rpmsg_poll(rpwwan->ept, filp, wait);
> > > > +}
> > > > +
> > > >  static const struct wwan_port_ops rpmsg_wwan_pops = {
> > > >         .start = rpmsg_wwan_ctrl_start,
> > > >         .stop = rpmsg_wwan_ctrl_stop,
> > > >         .tx = rpmsg_wwan_ctrl_tx,
> > > > +       .poll = rpmsg_wwan_ctrl_poll,
> > > >  };
> > > >
> > > >  static struct device *rpmsg_wwan_find_parent(struct device *dev)
> > > > diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> > > > index 7e728042fc41..c7fd0b897f87 100644
> > > > --- a/drivers/net/wwan/wwan_core.c
> > > > +++ b/drivers/net/wwan/wwan_core.c
> > > > @@ -500,7 +500,8 @@ static void wwan_port_op_stop(struct wwan_port *port)
> > > >         mutex_unlock(&port->ops_lock);
> > > >  }
> > > >
> > > > -static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
> > > > +static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb,
> > > > +                          bool nonblock)
> > > >  {
> > > >         int ret;
> > > >
> > > > @@ -510,7 +511,7 @@ static int wwan_port_op_tx(struct wwan_port *port, struct sk_buff *skb)
> > > >                 goto out_unlock;
> > > >         }
> > > >
> > > > -       ret = port->ops->tx(port, skb);
> > > > +       ret = port->ops->tx(port, skb, nonblock);
> > > >
> > > >  out_unlock:
> > > >         mutex_unlock(&port->ops_lock);
> > > > @@ -637,7 +638,7 @@ static ssize_t wwan_port_fops_write(struct file *filp, const char __user *buf,
> > > >                 return -EFAULT;
> > > >         }
> > > >
> > > > -       ret = wwan_port_op_tx(port, skb);
> > > > +       ret = wwan_port_op_tx(port, skb, !!(filp->f_flags & O_NONBLOCK));
> > > >         if (ret) {
> > > >                 kfree_skb(skb);
> > > >                 return ret;
> > > > @@ -659,6 +660,8 @@ static __poll_t wwan_port_fops_poll(struct file *filp, poll_table *wait)
> > > >                 mask |= EPOLLIN | EPOLLRDNORM;
> > > >         if (!port->ops)
> > > >                 mask |= EPOLLHUP | EPOLLERR;
> > > > +       else if (port->ops->poll)
> > > > +               mask |= port->ops->poll(port, filp, wait);
> > >
> > > I'm not sure it useful here because EPOLLOUT flag is already set above, right?
> > >
> >
> > Oops, you're right - sorry! I thought the flags are inverted (only set
> > if (is_write_blocked())), then it would have worked fine. :)
> >
> > I think this should be easy to fix though, I can just make the
> >
> >         if (!is_write_blocked(port))
> >                 mask |= EPOLLOUT | EPOLLWRNORM;
> >
> > if statement conditional to (port->ops->poll == NULL). It only makes
> > sense to supply the poll() op if the built-in write-blocking cannot be
> > used easily (like in my case).
> 
> Yes, so maybe in that case poll ops should be renamed to something like tx_poll?
> 

Sounds good! I will rename it to tx_poll() in v2. :)

Thanks!
Stephan