All of lore.kernel.org
 help / color / mirror / Atom feed
From: Loic Poulain <loic.poulain@linaro.org>
To: Stephan Gerhold <stephan@gerhold.net>
Cc: "David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	Aleksander Morgado <aleksander@aleksander.es>,
	Sergey Ryazanov <ryazanov.s.a@gmail.com>,
	Johannes Berg <johannes.berg@intel.com>,
	M Chetan Kumar <m.chetan.kumar@intel.com>,
	Ohad Ben-Cohen <ohad@wizery.com>,
	Mathieu Poirier <mathieu.poirier@linaro.org>,
	Network Development <netdev@vger.kernel.org>,
	linux-remoteproc@vger.kernel.org,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	phone-devel@vger.kernel.org,
	open list <linux-kernel@vger.kernel.org>,
	~postmarketos/upstreaming@lists.sr.ht
Subject: Re: [PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function
Date: Tue, 15 Jun 2021 23:24:41 +0200	[thread overview]
Message-ID: <CAMZdPi8JuyqoF3GwJHcdXhdn0e7ks_f2WiUFpmn3E8HH7T_Gng@mail.gmail.com> (raw)
In-Reply-To: <20210615133229.213064-4-stephan@gerhold.net>

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

  reply	other threads:[~2021-06-15 21:15 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-06-15 13:32 [PATCH net-next 0/3] net: wwan: Add RPMSG WWAN CTRL driver Stephan Gerhold
2021-06-15 13:32 ` [PATCH net-next 1/3] rpmsg: core: Add driver_data for rpmsg_device_id Stephan Gerhold
2021-06-15 13:32 ` [PATCH net-next 2/3] net: wwan: Add RPMSG WWAN CTRL driver Stephan Gerhold
2021-06-15 13:32 ` [PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function Stephan Gerhold
2021-06-15 21:24   ` Loic Poulain [this message]
2021-06-15 22:06     ` Stephan Gerhold
2021-06-16  9:28       ` Loic Poulain
2021-06-16 17:13         ` Stephan Gerhold

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAMZdPi8JuyqoF3GwJHcdXhdn0e7ks_f2WiUFpmn3E8HH7T_Gng@mail.gmail.com \
    --to=loic.poulain@linaro.org \
    --cc=aleksander@aleksander.es \
    --cc=bjorn.andersson@linaro.org \
    --cc=davem@davemloft.net \
    --cc=johannes.berg@intel.com \
    --cc=kuba@kernel.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=m.chetan.kumar@intel.com \
    --cc=mathieu.poirier@linaro.org \
    --cc=netdev@vger.kernel.org \
    --cc=ohad@wizery.com \
    --cc=phone-devel@vger.kernel.org \
    --cc=ryazanov.s.a@gmail.com \
    --cc=stephan@gerhold.net \
    --cc=~postmarketos/upstreaming@lists.sr.ht \
    --subject='Re: [PATCH net-next 3/3] net: wwan: Allow WWAN drivers to provide blocking tx and poll function' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.