Re: [RFC] Integrate RPMSG/SMD into WWAN subsystem

HI Stephan,

On Mon, 7 Jun 2021 at 14:49, Stephan Gerhold [off-list ref] wrote:

Hi,

On Mon, Jun 07, 2021 at 02:16:10PM +0200, Loic Poulain wrote:

quoted

On Mon, 7 Jun 2021 at 13:44, Stephan Gerhold [off-list ref] wrote:

quoted

On Mon, Jun 07, 2021 at 01:23:18PM +0200, Toke Høiland-Jørgensen wrote:

quoted

Stephan Gerhold [off-list ref] writes:

quoted

On Mon, Jun 07, 2021 at 11:27:07AM +0200, Loic Poulain wrote:

quoted

On Sat, 5 Jun 2021 at 11:25, Stephan Gerhold [off-list ref] wrote:

quoted

On Fri, Jun 04, 2021 at 11:11:45PM +0200, Loic Poulain wrote:

quoted

On Wed, 2 Jun 2021 at 20:20, Stephan Gerhold [off-list ref] wrote:

quoted

I've been thinking about creating some sort of "RPMSG" driver for the
new WWAN subsystem; this would be used as a QMI/AT channel to the
integrated modem on some older Qualcomm SoCs such as MSM8916 and MSM8974.

It's easy to confuse all the different approaches that Qualcomm has to
talk to their modems, so I will first try to briefly give an overview
about those that I'm familiar with:

---
There is USB and MHI that are mainly used to talk to "external" modems.

For the integrated modems in many Qualcomm SoCs there is typically
a separate control and data path. They are not really related to each
other (e.g. currently no common parent device in sysfs).

For the data path (network interface) there is "IPA" (drivers/net/ipa)
on newer SoCs or "BAM-DMUX" on some older SoCs (e.g. MSM8916/MSM8974).
I have a driver for BAM-DMUX that I hope to finish up and submit soon.

The connection is set up via QMI. The messages are either sent via
a shared RPMSG channel (net/qrtr sockets in Linux) or via standalone
SMD/RPMSG channels (e.g. "DATA5_CNTL" for QMI and "DATA1" for AT).

This gives a lot of possible combinations like BAM-DMUX+RPMSG
(MSM8916, MSM8974), or IPA+QRTR (SDM845) but also other funny
combinations like IPA+RPMSG (MSM8994) or BAM-DMUX+QRTR (MSM8937).

Simply put, supporting all these in userspace like ModemManager
is a mess (Aleksander can probably confirm).
It would be nice if this could be simplified through the WWAN subsystem.

It's not clear to me if or how well QRTR sockets can be mapped to a char
device for the WWAN subsystem, so for now I'm trying to focus on the
standalone RPMSG approach (for MSM8916, MSM8974, ...).
---

Currently ModemManager uses the RPMSG channels via the rpmsg-chardev
(drivers/rpmsg/rpmsg_char.c). It wasn't my idea to use it like this,
I just took that over from someone else. Realistically speaking, the
current approach isn't too different from the UCI "backdoor interface"
approach that was rejected for MHI...

I kind of expected that I can just trivially copy some code from
rpmsg_char.c into a WWAN driver since they both end up as a simple char
device. But it looks like the abstractions in wwan_core are kind of
getting in the way here... As far as I can tell, they don't really fit
together with the RPMSG interface.

For example there is rpmsg_send(...) (blocking) and rpmsg_trysend(...)
(non-blocking) and even a rpmsg_poll(...) [1] but I don't see a way to
get notified when the TX queue is full or no longer full so I can call
wwan_port_txon/off().

Any suggestions or other thoughts?

It would be indeed nice to get this in the WWAN framework.
I don't know much about rpmsg but I think it is straightforward for
the RX path, the ept_cb can simply forward the buffers to
wwan_port_rx.

Right, that part should be straightforward.

quoted

For tx, simply call rpmsg_trysend() in the wwan tx
callback and don't use the txon/off helpers. In short, keep it simple
and check if you observe any issues.

I'm not sure that's a good idea. This sounds like exactly the kind of
thing that might explode later just because I don't manage to get the
TX queue full in my tests. In that case, writing to the WWAN char dev
would not block, even if O_NONBLOCK is not set.

Right, if you think it could be a problem, you can always implement a
more complex solution like calling rpmsg_send from a
workqueue/kthread, and only re-enable tx once rpmsg_send returns.

I did run into trouble when I tried to stream lots of data into the WWAN
char device (e.g. using dd). However, in practice (with ModemManager)
I did not manage to cause such issues yet. Personally, I think it's
something we should get right, just to avoid trouble later
(like "modem suddenly stops working").

Right now I extended the WWAN port ops a bit so I tells me if the write
should be non-blocking or blocking and so I can call rpmsg_poll(...).

OK, but note that poll seems to be an optional rpmsg ops, rpmsg_poll
can be a no-op and so would not guarantee that EPOLL_OUT will ever be
set.

Wouldn't that be more a limitation of the driver that provides the rpmsg
ops then? If rpmsg_send(...) may block, it should also be possible to
implement rpmsg_poll(...). Right now my implementation basically behaves
mostly like the existing rpmsg-char. That's what has been used on
several devices for more than a year and it works quite well.

Granted, so far testing was mostly done with the "qcom_smd" RPMSG driver
(which is likely going to be the primary use case for my driver for now).
This one also happens to be the only one that implements rpmsg_poll()
at the moment...

I asked some other people to test it for AT messages with "glink" on
newer Qualcomm SoCs and that doesn't work that well yet. However, as far
as I can tell most of the trouble seems to be caused by various strange
bugs in the "glink" driver.
(See drivers/rpmsg for the drivers I'm referring to...)

---

One of the main potential problems I see with some kind of workqueue
approach is that all writes would seemingly succeed for the client.
When the extra thread is done with rpmsg_send(...) we will have already
returned from the char dev write(...) syscall, without a way to report
the error. Perhaps that's unlikely though and we don't need to worry
about that too much?

I'm not really sure which one of the solution is best at the end.
I think it doesn't make too much of a difference at the end, both can
work well and both have some advantages/disadvantages in certain fairly
unlikely situations.

Yes, feel free to submit the series for review.

Regards,
Loic