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

[Stephan Gerhold <stephan@gerhold.net>]

Hi Aleksander!

On Fri, Jun 18, 2021 at 10:21:18AM +0200, Aleksander Morgado wrote:
> > +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 },
> > +       {},
> > +};
> 
> If I understand this properly, now these rpmsg backed control ports
> would be automatically exposed without the need of a userspace CLI
> tool to do that (rpmsgexport).
> 

Yep, that's the main advantage compared to the current approach.

> And if I recall correctly, DATA5_CNTL and DATA4 were the only channels
> actively exported with udev actions using rpmsgexport in postmarketos,
> but that didn't mean someone could add additional rules to export
> other channels (i.e. as per the ModemManager port type hint rules,
> DATA[0-9]*_CNTL as QMI and DATA[0-9]* as AT, except for DATA40_CNTL
> and DATA_40 which are the USB tethering related ones).
> 

Yep.

> So, does this mean we're limiting the amount of channels exported to
> only one QMI control port and one AT control port?

Yep, but I think:
  - It's easy to extend this with additional ports later
    if someone has a real use case for that.
  - It's still possible to export via rpmsgexport.

> Not saying that's wrong, but maybe it makes sense to add a comment
> somewhere specifying that explicitly.

Given that these channels were only found through reverse engineering,
saying that DATA*_CNTL/DATA* are fully equivalent QMI/AT ports is just
a theory, I have no proof for this. Generally these channels had some
fixed use case on the original Android system, for example DATA1 (AT)
seems to have been often used for Bluetooth Dial-Up Networking (DUN)
while DATA4 was often more general purpose.

Perhaps DATA* are all fully equivalent, independent AT channels at the
end, or perhaps DATA1/DATA4 behave slightly differently because there
were some special requirements for Bluetooth DUN. I have no way to tell.
And it can vary from device to device since we're stuck with
device-specific (and usually signed) firmware.

Another example: I have seen DATA11 on some devices, but it does not
seem to work as AT port for some reason, there is no reply at all
from the modem on that channel. Perhaps it needs to be activated
somehow, perhaps it's not an AT channel at all, I have no way to tell.

My point is: Here I'm only enabling what is proven to work on all
devices (used in postmarketOS for more than a year). I have insufficient
data to vouch for the reliability of any other channel. I cannot say if
the channels are really independent, or influence each other somehow.

As far as I understand, we currently do not have any use case for having
multiple QMI/AT ports exposed for ModemManager, right? And if someone
does have a use case, perhaps exposing them through the WWAN subsystem
is not even what they want, perhaps they want to forward them through
USB or something.

> Also, would it make sense to have some way to trigger the export of
> additional channels somehow via userspace? e.g. something like
> rpmsgexport but using the wwan subsystem. I'm not sure if that's a
> true need anywhere or just over-engineering the solution, truth be
> told.

So personally I think we should keep this simple and limited to existing
use cases. If someone shows up with different requirements we can
investigate this further.

If I send a v3 I will check if I can clarify this in the commit
message somewhat. I actually had something related in there but removed
it shortly before submitting the patch because I thought it's mostly
just speculation and the message was already quite long. Oh well :)

Stephan