netdev - Re: [PATCH net-next 10/10] wwan: core: add WWAN common private data for netdev

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 21 Jun 2021 09:37:09 +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

Hi Sergey,

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.

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

Regards,
Loic