Message-ID: <CAFEp6-2n13+Q5sjatgjjgG0vFP28PSiH8PoOJBNB-u9HX04ObQ@mail.gmail.com>
Date: Sat, 19 Apr 2025 13:44:35 +0200
From: Loic Poulain <loic.poulain@....qualcomm.com>
To: Sergey Ryazanov <ryazanov.s.a@...il.com>
Cc: Johannes Berg <johannes@...solutions.net>,
        Andrew Lunn <andrew+netdev@...n.ch>,
        Eric Dumazet <edumazet@...gle.com>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
        netdev@...r.kernel.org
Subject: Re: [RFC PATCH 2/6] net: wwan: core: split port creation and registration

On Sat, Apr 19, 2025 at 1:04 AM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>
> Hi Loic,
>
> please find one extra option below.
>
> On 17.04.2025 23:35, Loic Poulain wrote:
> > Hi Sergey,
> >
> > On Mon, Apr 14, 2025 at 11:28 PM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
> >>
> >> Hi Loic,
> >>
> >> thank you that you found a time to check it. See the explanation below,
> >> might be you can suggest a better solution.
> >>
> >> On 14.04.2025 21:50, Loic Poulain wrote:
> >>> Hi Sergey,
> >>>
> >>> On Wed, Apr 9, 2025 at 1:31 AM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
> >>>>
> >>>> Upcoming GNSS (NMEA) port type support requires exporting it via the
> >>>> GNSS subsystem. On another hand, we still need to do basic WWAN core
> >>>> work: find or allocate the WWAN device, make it the port parent, etc. To
> >>>> reuse as much code as possible, split the port creation function into
> >>>> the registration of a regular WWAN port device, and basic port struct
> >>>> initialization.
> >>>>
> >>>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
> >>>> ---
> >>>>    drivers/net/wwan/wwan_core.c | 86 ++++++++++++++++++++++--------------
> >>>>    1 file changed, 53 insertions(+), 33 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
> >>>> index ade8bbffc93e..045246d7cd50 100644
> >>>> --- a/drivers/net/wwan/wwan_core.c
> >>>> +++ b/drivers/net/wwan/wwan_core.c
> >>>> @@ -357,16 +357,19 @@ static struct attribute *wwan_port_attrs[] = {
> >>>>    };
> >>>>    ATTRIBUTE_GROUPS(wwan_port);
> >>>>
> >>>> -static void wwan_port_destroy(struct device *dev)
> >>>> +static void __wwan_port_destroy(struct wwan_port *port)
> >>>>    {
> >>>> -       struct wwan_port *port = to_wwan_port(dev);
> >>>> -
> >>>> -       ida_free(&minors, MINOR(port->dev.devt));
> >>>>           mutex_destroy(&port->data_lock);
> >>>>           mutex_destroy(&port->ops_lock);
> >>>>           kfree(port);
> >>>>    }
> >>>>
> >>>> +static void wwan_port_destroy(struct device *dev)
> >>>> +{
> >>>> +       ida_free(&minors, MINOR(dev->devt));
> >>>> +       __wwan_port_destroy(to_wwan_port(dev));
> >>>> +}
> >>>> +
> >>>>    static const struct device_type wwan_port_dev_type = {
> >>>>           .name = "wwan_port",
> >>>>           .release = wwan_port_destroy,
> >>>> @@ -440,6 +443,49 @@ static int __wwan_port_dev_assign_name(struct wwan_port *port, const char *fmt)
> >>>>           return dev_set_name(&port->dev, "%s", buf);
> >>>>    }
> >>>>
> >>>> +/* Register a regular WWAN port device (e.g. AT, MBIM, etc.)
> >>>> + *
> >>>> + * NB: in case of error function frees the port memory.
> >>>> + */
> >>>> +static int wwan_port_register_wwan(struct wwan_port *port)
> >>>> +{
> >>>> +       struct wwan_device *wwandev = to_wwan_dev(port->dev.parent);
> >>>> +       char namefmt[0x20];
> >>>> +       int minor, err;
> >>>> +
> >>>> +       /* A port is exposed as character device, get a minor */
> >>>> +       minor = ida_alloc_range(&minors, 0, WWAN_MAX_MINORS - 1, GFP_KERNEL);
> >>>> +       if (minor < 0) {
> >>>> +               __wwan_port_destroy(port);
> >>>
> >>> I see this is documented above, but it's a bit weird that the port is
> >>> freed inside the register function, it should be up to the caller to
> >>> do this. Is there a reason for this?
> >>
> >> I agree that this looks weird and asymmetrical. I left the port
> >> allocation in wwan_create_port() because both WWAN-exported and
> >> GNSS-exported types of port share the same port allocation. And the port
> >> struct is used as a container to keep all the port registration arguments.
> >>
> >> I did the port freeing inside this function because we free the port
> >> differently depending of the device registration state. If we fail to
> >> initialize the port at earlier stage then we use __wwan_port_destroy()
> >> which basically just releases the memory.
> >>
> >> But if device_register() fails then we are required to use put_device()
> >> which does more job.
> >>
> >> I do not think it is acceptable to skip put_device() call and just
> >> release the memory. Also I do not find maintainable to partially open
> >> code put_device() here in the WWAN-exportable handler and release the
> >> memory in caller function wwan_create_port().
> >>
> >> We could somehow try to return this information from
> >> wwan_port_register_wwan() to wwan_create_port(), so the caller could
> >> decide, shall it use __wwan_port_destroy() or put_device() in case of
> >> failure.
> >>
> >> But I can not see a way to clearly indicate, which releasing approach
> >> should be used by the caller. And even in this case it going to look
> >> weird since the called function controls the caller.
> >>
> >> Another solution for the asymmetry problem is to move the allocation
> >> from the caller to the called function. So the memory will be allocated
> >> and released in the same function. But in this case we will need to pass
> >> all the parameters from wwan_create_port() to wwan_port_register_wwan().
> >> Even if we consolidate the port basic allocation/initialization in a
> >> common routine, the final solution going to look a duplication. E.g.
> >>
> >> struct wwan_port *wwan_port_allocate(struct wwan_device *wwandev,
> >>                                        enum wwan_port_type type,
> >>                                        const struct wwan_port_ops *ops,
> >>                                        struct wwan_port_caps *caps,
> >>                                        void *drvdata)
> >> {
> >>       /* Do the mem allocation and init here */
> >>       return port;
> >> }
> >>
> >> struct wwan_port *wwan_port_register_wwan(struct wwan_device *wwandev,
> >>                          enum wwan_port_type type,
> >>                          const struct wwan_port_ops *ops,
> >>                          struct wwan_port_caps *caps,
> >>                          void *drvdata)
> >> {
> >>       port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
> >>       /* Proceed with chardev registration or release on failure */
> >>       /* return port; or return ERR_PTR(-err); */
> >> }
> >>
> >> struct wwan_port *wwan_port_register_gnss(struct wwan_device *wwandev,
> >>                          enum wwan_port_type type,
> >>                          const struct wwan_port_ops *ops,
> >>                          struct wwan_port_caps *caps,
> >>                          void *drvdata)
> >> {
> >>       port = wwan_port_allocate(wwandev, type, ops, caps, drvdata);
> >>       /* Proceed with GNSS registration or release on failure */
> >>       /* return port; or return ERR_PTR(-err); */
> >> }
> >>
> >> struct wwan_port *wwan_create_port(struct device *parent,
> >>                                      enum wwan_port_type type,
> >>                                      const struct wwan_port_ops *ops,
> >>                                      struct wwan_port_caps *caps,
> >>                                      void *drvdata)
> >> {
> >>       ...
> >>       wwandev = wwan_create_dev(parent);
> >>       if (type == WWAN_PORT_NMEA)
> >>           port = wwan_port_register_gnss(wwandev, type, ops,
> >>                                          caps, drvdata);
> >>       else
> >>           port = wwan_port_register_wwan(wwandev, type, ops,
> >>                                          caps, drvdata);
> >>       if (!IS_ERR(port))
> >>           return port;
> >>       wwan_remove_dev(wwandev);
> >>       return ERR_CAST(port);
> >> }
> >>
> >> wwan_create_port() looks better in prices of passing a list of arguments
> >> and allocating the port in multiple places.
> >>
> >> Maybe some other design approach, what was overseen?
> >>
> >>
> >> For me, the ideal solution would be a routine that works like
> >> put_device() except calling the device type release handler. Then we can
> >> use it to cleanup leftovers of the failed device_register() call and
> >> then release the memory in the calling wwan_create_port() function.
> >
> > Ok I see, thanks for the clear explanation, I don't see a perfect
> > solution here without over complication. So the current approach is
> > acceptable, can you add a comment in the caller function as well,so
> > that it's clear why we don't have to release the port on error.
>
> Looks like I've found another one solution to move the port resources
> (memory) release back to the function allocating it. It is also a bit
> hackish and I would like to hear a feedback from you.
>
> The port is released inside wwan_port_register_wwan() just because we
> release it differently depending on the initialization stage, right?
>
> We cannot call put_device() before the device_register() call. And that
> is why I've created that __wwan_port_destroy() routine. What if we make
> port eligible to be released with put_device() ASAP? Internally,
> device_register() just sequentially calls device_initialize() and then
> device_add(). Indeed, device_initialize() makes a device struct eligible
> to be released with put_device().
>
> We can avoid using device_register() and instead of it, do the
> registration step by step. Just after the port memory allocation and
> very basic initialization, we can call device_initialize(). And then
> call device_add() when it is time to register the port with the kernel.
> And if something going to go wrong, we can return just an error from
> wwan_port_register_wwan() and release the port with put_device() in
> wwan_create_port() where it was allocated. Something like this:
>
> static int wwan_port_register_wwan(struct wwan_port *port)
> {
>      ...
>      if (something_wrong)
>          return -E<ERROR_TYPE>;
>      ...
>      return 0;
> }
>
> struct wwan_port *wwan_create_port(struct device *parent,
>                                     enum wwan_port_type type,
>                                     const struct wwan_port_ops *ops,
>                                     struct wwan_port_caps *caps,
>                                     void *drvdata)
> {
>      ...
>      port = kzalloc(sizeof(*port), GFP_KERNEL);
>      /* Do basic port init here */
>      port->dev.type = &wwan_port_dev_type;
>      device_initialize(&port->dev);  /* allows put_device() usage */
>
>      if (port->type == WWAN_PORT_NMEA)
>          err = wwan_port_register_gnss(port);
>      else
>          err = wwan_port_register_wwan(port);
>
>      if (err) {
>          put_device(&port->dev);
>          goto error_wwandev_remove;
>      }
>
>      return port;
>      ...
> }
>
> The only drawback I see here is that we have to use put_device() to
> release the port memory even in case of GNSS port. We don't actually
> register the port as device, but I believe, this can be explained with a
> proper comment.

Yes, that's a good alternative, so you would also have to use
put_device in gnss_unregister, or do something like:

void wwan_remove_port(struct wwan_port *port)
{
[...]
    if (port->type == WWAN_PORT_NMEA)
                wwan_port_unregister_gnss(port);
    /* common unregistering (put_device), not necessarily in a
separate function */
   wwan_port_unregister(port);
[...]
}

And probably have a common wwan_port_destroy function:
static void wwan_port_destroy(struct device *dev)
{
    if (port->type != WWAN_PORT_NMEA)
        ida_free(&minors, MINOR(dev->devt));
    [...]
    kfree(port);
}

Regards,
Loic

Powered by blists - more mailing lists