Message-ID: <CAHNKnsQ8ciMMD-8oGaRgJ-WjQYvc7hNU2Zt2AyMF72qzJ9Qf3A@mail.gmail.com>
Date:   Mon, 21 Jun 2021 12:54:56 +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,

On Mon, Jun 21, 2021 at 9:44 AM Loic Poulain <loic.poulain@...aro.org> wrote:
> On Sun, 20 Jun 2021 at 15:51, Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>> 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.
>
> Right, there is an ongoing discussion about supporting iosm:
> https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/issues/385#note_958408
>
> So once we have it working for iosm, it should work for any WWAN
> device using WWAN framework.

Thank you for the clarification, I will keep in mind.

-- 
Sergey

Powered by blists - more mailing lists