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: <CAHNKnsRDuf=zuqeAMJgZ5kW6Kd1GsOd6-v5AX4ScCt7_muJp6g@mail.gmail.com>
Date:   Sun, 20 Jun 2021 16:51:30 +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>,
        Aleksander Morgado <aleksander@...ksander.es>
Subject: Re: [PATCH net-next 09/10] net: mhi_net: create default link via WWAN core

Hi Loic,

CC Aleksander, as the talk drifts towards ModemManager.

On Tue, Jun 15, 2021 at 10:08 AM Loic Poulain <loic.poulain@...aro.org> wrote:
> 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.

Looks like I missed the complexity of WWAN devices handling. Thank you
for pointing that out. Now I will drop this patch from the series.

Just curious, am I right to say that any network interface created
with wwan-core is not usable with ModemManager at the moment? AFAIU,
ModemManager is unable to bundle a control port and a netdev into a
common "modem" object, even if they both have the same parent Linux
device, just because that device is not a physical USB device.

--
Sergey