Date:   Tue, 22 Jun 2021 09:21:08 +0200
From:   Loic Poulain <loic.poulain@...aro.org>
To:     Sergey Ryazanov <ryazanov.s.a@...il.com>
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

On Mon, 21 Jun 2021 at 19:22, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>
> 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.

Exact mhi_net act as the transport layer in that case.

> 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.

Yes, this is what they call the 'real' device (or lower_dev).

> 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?

Once the link is created, packets received by the 'real' ndev are
automatically forwarded to the upper layer (in that case, rmnet). So
the 'transport' netdev doesn't even need to know about the upper layer
details.

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

That should be handled the same way as for mhi_net, indeed.

> 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 :(
>
> --

Regards,
Loic

Powered by blists - more mailing lists