Received-SPF: pass (google.com: domain of linux-wireless-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210601080538.71036-1-johannes@sipsolutions.net> <20210601100320.7d39e9c33a18.I0474861dad426152ac7e7afddfd7fe3ce70870e4@changeid>
In-Reply-To: <20210601100320.7d39e9c33a18.I0474861dad426152ac7e7afddfd7fe3ce70870e4@changeid>
From:   Loic Poulain <loic.poulain@linaro.org>
Date:   Tue, 1 Jun 2021 11:37:48 +0200
Message-ID: <CAMZdPi-ZaH8WWKfhfKzy0OKpUtNAiCUfekh9R1de5awFP-ed=A@mail.gmail.com>
Subject: Re: [RFC 3/4] wwan: add interface creation support
To:     Johannes Berg <johannes@sipsolutions.net>
Cc:     linux-wireless@vger.kernel.org,
        Network Development <netdev@vger.kernel.org>,
        M Chetan Kumar <m.chetan.kumar@intel.com>,
        Johannes Berg <johannes.berg@intel.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Johannes,

On Tue, 1 Jun 2021 at 10:05, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> From: Johannes Berg <johannes.berg@intel.com>
>
> Add support to create (and destroy) interfaces via a new
> rtnetlink kind "wwan". The responsible driver has to use
> the new wwan_register_ops() to make this possible.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  drivers/net/wwan/wwan_core.c | 219 ++++++++++++++++++++++++++++++++++-
>  include/linux/wwan.h         |  36 ++++++
>  include/uapi/linux/wwan.h    |  17 +++
>  3 files changed, 267 insertions(+), 5 deletions(-)
>  create mode 100644 include/uapi/linux/wwan.h
>
> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> index cff04e532c1e..b1ad78f386bc 100644
> --- a/drivers/net/wwan/wwan_core.c
> +++ b/drivers/net/wwan/wwan_core.c
> @@ -13,6 +13,8 @@
>  #include <linux/slab.h>
>  #include <linux/types.h>
>  #include <linux/wwan.h>
> +#include <net/rtnetlink.h>
> +#include <uapi/linux/wwan.h>
>
>  #define WWAN_MAX_MINORS 256 /* 256 minors allowed with register_chrdev() */
>
> @@ -524,24 +526,231 @@ static const struct file_operations wwan_port_fops = {
>         .llseek = noop_llseek,
>  };
>
> +struct wwan_dev_reg {
> +       struct list_head list;
> +       struct device *dev;
> +       const struct wwan_ops *ops;
> +       void *ctxt;
> +};
> +
> +static DEFINE_MUTEX(wwan_mtx);
> +static LIST_HEAD(wwan_devs);
> +
> +int wwan_register_ops(struct device *parent, const struct wwan_ops *ops,
> +                     void *ctxt)
> +{
> +       struct wwan_dev_reg *reg;
> +       int ret;
> +
> +       if (WARN_ON(!parent || !ops))
> +               return -EINVAL;
> +
> +       mutex_lock(&wwan_mtx);
> +       list_for_each_entry(reg, &wwan_devs, list) {
> +               if (WARN_ON(reg->dev == parent)) {
> +                       ret = -EBUSY;
> +                       goto out;
> +               }
> +       }

Thanks for this, overall it looks good to me, but just checking why
you're not using the wwan_dev internally to create-or-pick wwan_dev
(wwan_dev_create) and register ops to it, instead of having a global
new wwan_devs list.

> +
> +       reg = kzalloc(sizeof(*reg), GFP_KERNEL);
> +       if (!reg) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
> +
> +       reg->dev = parent;
> +       reg->ops = ops;
> +       reg->ctxt = ctxt;
> +       list_add_tail(&reg->list, &wwan_devs);
> +
> +       ret = 0;
> +
> +out:
> +       mutex_unlock(&wwan_mtx);
> +       return ret;
> +}
> +EXPORT_SYMBOL_GPL(wwan_register_ops);

Regards,
Loic