Message-ID: <9b36d9b6-c2da-43ef-a958-167c663792e4@gmail.com>
Date: Sat, 19 Apr 2025 02:04:59 +0300
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Loic Poulain <loic.poulain@....qualcomm.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

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.

What do you think, is it worth to try to rework the code in this way?

--
Sergey

Powered by blists - more mailing lists