Message-ID: <CAHNKnsS44SrnWXXkMNX=HvgeMRnZMckE-CWVTK_Z+Nyd3RRcPg@mail.gmail.com>
Date:   Mon, 21 Jun 2021 20:22:42 +0300
From:   Sergey Ryazanov <ryazanov.s.a@...il.com>
To:     Loic Poulain <loic.poulain@...aro.org>
Cc:     Johannes Berg <johannes@...solutions.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Network Development <netdev@...r.kernel.org>,
        M Chetan Kumar <m.chetan.kumar@...el.com>,
        Intel Corporation <linuxwwan@...el.com>
Subject: Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data
 for netdev

Hi Loic,

On Mon, Jun 21, 2021 at 10:28 AM Loic Poulain <loic.poulain@...aro.org> wrote:
> On Sun, 20 Jun 2021 at 16:39, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>> On Tue, Jun 15, 2021 at 10:24 AM Loic Poulain <loic.poulain@...aro.org> wrote:
>>> On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>>>> The WWAN core not only multiplex the netdev configuration data, but
>>>> process it too, and needs some space to store its private data
>>>> associated with the netdev. Add a structure to keep common WWAN core
>>>> data. The structure will be stored inside the netdev private data before
>>>> WWAN driver private data and have a field to make it easier to access
>>>> the driver data. Also add a helper function that simplifies drivers
>>>> access to their data.
>>>
>>> Would it be possible to store wwan_netdev_priv at the end of priv data instead?
>>> That would allow drivers to use the standard netdev_priv without any change.
>>> And would also simplify forwarding to rmnet (in mhi_net) since rmnet
>>> uses netdev_priv.
>>
>> I do not think that mimicking something by one subsystem for another
>> is generally a good idea. This could look good in a short term, but
>> finally it will become a headache due to involvement of too many
>> entities.
>>
>> IMHO, a suitable approach to share the rmnet library and data
>> structures among drivers is to make the rmnet interface more generic.
>>
>> E.g. consider such netdev/rtnl specific function:
>>
>> static int rmnet_foo_action(struct net_device *dev, ...)
>> {
>>     struct rmnet_priv *rmdev = netdev_priv(dev);
>>     <do a foo action here>
>> }
>>
>> It could be split into a wrapper and an actual handler:
>>
>> int __rmnet_foo_action(struct rmnet_priv *rmdev, ...)
>> {
>>     <do a foo action here>
>> }
>> EXPORT_GPL(__rmnet_foo_action)
>>
>> static int rmnet_foo_action(struct net_device *dev, ...)
>> {
>>     struct rmnet_priv *rmdev = netdev_priv(dev);
>>     return __rmnet_foo_action(rmdev, ...)
>> }
>>
>> So a call from mhi_net to rmnet could looks like this:
>>
>> static int mhi_net_foo_action(struct net_device *dev, ...)
>> {
>>     struct rmnet_priv *rmdev = wwan_netdev_drvpriv(dev);
>>     return __rmnet_foo_action(rmdev, ...)
>> }
>>
>> In such a way, only the rmnet users know something special, while
>> other wwan core users and the core itself behave without any
>> surprises. E.g. any regular wwan core minidriver can access the
>> link_id field of the wwan common data by calling netdev_priv() without
>> further calculating the common data offset.
>
> Yes, that would work, but it's an important refactoring since rmnet is
> all built around the idea that netdev_priv is rmnet_priv, including rx
> path (netdev_priv(skb->dev)).
> My initial tests were based on this 'simple' change:
> https://git.linaro.org/people/loic.poulain/linux.git/commit/?h=wwan_rmnet&id=6308d49790f10615bd33a38d56bc7f101646558f
>
> Moreover, a driver like mhi_net also supports non WWAN local link
> (called mhi_swip), which is a network link between the host and the
> modem cpu (for modem hosted services...). This link is not managed by
> the WWAN layer and is directly created by mhi_net. I could create a
> different driver or set of handlers for this netdev, but it's
> additional complexity.

Correct me if I am wrong. I just checked the rmnet code and realized
that rmnet should work on top of mhi_net and not vice versa. mhi_net
should provide some kind of transportation for QMAP packets from a HW
device to rmnet. Then rmnet will perform demultiplexing, deaggregation
and decapsulation of QMAP packets to pure IP packets.

rmnet itself receives these QMAP packets via a network device. So any
driver that would like to provide the QMAP transport for rmnet should
create a network device for this task.

The main issue with the integration of mhi_net with the wwan is that
the mhi_net driver should pass its traffic through the rmnet demuxer.
While the network device that will be created by the rmnet demuxer
will not be a child of a MHI device or a wwan device. So to properly
integrate the mhi_net driver with the wwan core netdev capabilities,
you should begin to use rmnet not as an independent demux created on
top of a transport network interface, but as a library. Am I correctly
understanding?

Does the same issue appear when we begin a more tight integration of
the qmi_wwan USB driver with the wwan core?

I would like to say that one way or another, rmnet will be converted
to a quite abstract library that should avoid direct access to the
network device private data.

>>>> At the moment we use the common WWAN private data to store the WWAN data
>>>> link (channel) id at the time the link is created, and report it back to
>>>> user using the .fill_info() RTNL callback. This should help the user to
>>>> be aware which network interface is binded to which WWAN device data
>>>> channel.
>
> I wonder if it would not be simpler to store the link ID into
> netdev->dev_port, it's after all a kind of virtual/logical port.
> That would only postpone the introduction of a wwan_netdev_priv struct though.

I like this idea. This is likely to solve the link id storage problem.
But only if we plan to never extend the wwan core private data.
Otherwise, as you mention, this only postpones the wwan data structure
introduction to a moment when we will need to rework a lot of drivers.

Looks like we have no absolutely good solution. Only a set of
proposals, each which has its own shortcomings :(

-- 
Sergey

Powered by blists - more mailing lists