netdev - Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

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]

Message-ID: <CAMZdPi-C+Yhf27+-7Ct-1pkp0htrr6Qbt=Om2KQk+5aVVFPRMQ@mail.gmail.com>
Date:   Tue, 15 Jun 2021 09:17:32 +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>
Subject: Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

Hi Sergey,

On Tue, 15 Jun 2021 at 02:30, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>
> Utilize the just introduced WWAN core feature to create a default netdev
> for the default data channel. Since the netdev is now created via the
> WWAN core, rely on it ability to destroy all child netdevs on ops
> unregistering.
>
> While at it, remove the RTNL lock acquiring hacks that were earlier used
> to call addlink/dellink without holding the RTNL lock. Also make the
> WWAN netdev ops structure static to make sparse happy.
>
> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> ---
>  drivers/net/mhi/net.c | 54 +++++--------------------------------------
>  1 file changed, 6 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/net/mhi/net.c b/drivers/net/mhi/net.c
> index b003003cbd42..06253acecaa2 100644
> --- a/drivers/net/mhi/net.c
> +++ b/drivers/net/mhi/net.c
> @@ -342,10 +342,7 @@ static int mhi_net_newlink(void *ctxt, struct net_device *ndev, u32 if_id,
>         /* Number of transfer descriptors determines size of the queue */
>         mhi_netdev->rx_queue_sz = mhi_get_free_desc_count(mhi_dev, DMA_FROM_DEVICE);
>
> -       if (extack)
> -               err = register_netdevice(ndev);
> -       else
> -               err = register_netdev(ndev);
> +       err = register_netdevice(ndev);
>         if (err)
>                 goto out_err;
>
> @@ -370,10 +367,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>         struct mhi_net_dev *mhi_netdev = netdev_priv(ndev);
>         struct mhi_device *mhi_dev = ctxt;
>
> -       if (head)
> -               unregister_netdevice_queue(ndev, head);
> -       else
> -               unregister_netdev(ndev);
> +       unregister_netdevice_queue(ndev, head);
>
>         mhi_unprepare_from_transfer(mhi_dev);
>
> @@ -382,7 +376,7 @@ static void mhi_net_dellink(void *ctxt, struct net_device *ndev,
>         dev_set_drvdata(&mhi_dev->dev, NULL);
>  }
>
> -const struct wwan_ops mhi_wwan_ops = {
> +static const struct wwan_ops mhi_wwan_ops = {
>         .priv_size = sizeof(struct mhi_net_dev),
>         .setup = mhi_net_setup,
>         .newlink = mhi_net_newlink,
> @@ -392,55 +386,19 @@ const struct wwan_ops mhi_wwan_ops = {
>  static int mhi_net_probe(struct mhi_device *mhi_dev,
>                          const struct mhi_device_id *id)
>  {
> -       const struct mhi_device_info *info = (struct mhi_device_info *)id->driver_data;
>         struct mhi_controller *cntrl = mhi_dev->mhi_cntrl;
> -       struct net_device *ndev;
> -       int err;
> -
> -       err = wwan_register_ops(&cntrl->mhi_dev->dev, &mhi_wwan_ops, mhi_dev,
> -                               WWAN_NO_DEFAULT_LINK);
> -       if (err)
> -               return err;
> -
> -       if (!create_default_iface)
> -               return 0;
> -
> -       /* Create a default interface which is used as either RMNET real-dev,
> -        * MBIM link 0 or ip link 0)
> -        */
> -       ndev = alloc_netdev(sizeof(struct mhi_net_dev), info->netname,
> -                           NET_NAME_PREDICTABLE, mhi_net_setup);

I like the idea of the default link, but here we need to create the
netdev manually for several reasons:
- In case of QMAP/rmnet, this link is the lower netdev (transport
layer) and is not associated with any link id.
- In case of MBIM, it changes the netdev parent device from the MHI
dev to the WWAN dev, which (currently) breaks how ModemManager groups
ports/netdevs (based on bus).

For the last one, I don't think device hierarchy is considered as
UAPI, so we probably just need to add this new wwan link support to
user tools like MM. For the first one, I plan to split the mhi_net
driver into two different ones (mhi_net_qmap, mhi_net_mbim), and in
the case of qmap(rmnet) forward newlink/dellink call to rmnet
rtnetlink ops.

Regards,
Loic