Message-ID: <F9203EE2-8486-4CAD-8DD0-36971F3E9DA0@gmail.com>
Date: Thu, 15 Jan 2026 08:52:08 +0200
From: Sergey Ryazanov <ryazanov.s.a@...il.com>
To: Loic Poulain <loic.poulain@....qualcomm.com>, Slark Xiao <slark_xiao@....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,
 Daniele Palmas <dnlplm@...il.com>
Subject: Re: [RFC PATCH v5 2/7] net: wwan: core: explicit WWAN device reference counting

On January 14, 2026 11:31:45 PM, Loic Poulain <loic.poulain@....qualcomm.com> wrote:
>On Fri, Jan 9, 2026 at 2:09 AM Sergey Ryazanov <ryazanov.s.a@...il.com> wrote:
>>
>> We need information about existing WWAN device children since we remove
>> the device after removing the last child. Previously, we tracked users
>> implicitly by checking whether ops was registered and existence of a
>> child device of the wwan_class class. Upcoming GNSS (NMEA) port type
>> support breaks this approach by introducing a child device of the
>> gnss_class class.
>>
>> And a modem driver can easily trigger a kernel Oops by removing regular
>> (e.g., MBIM, AT) ports first and then removing a GNSS port. The WWAN
>> device will be unregistered on removal of a last regular WWAN port. And
>> subsequent GNSS port removal will cause NULL pointer dereference in
>> simple_recursive_removal().
>>
>> In order to support ports of classes other than wwan_class, switch to
>> explicit references counting. Introduce a dedicated counter to the WWAN
>> device struct, increment it on every wwan_create_dev() call, decrement
>> on wwan_remove_dev(), and actually unregister the WWAN device when there
>> are no more references.
>>
>> Run tested with wwan_hwsim with NMEA support patches applied and
>> different port removing sequences.
>>
>> Reported-by: Daniele Palmas <dnlplm@...il.com>
>> Closes: https://lore.kernel.org/netdev/CAGRyCJE28yf-rrfkFbzu44ygLEvoUM7fecK1vnrghjG_e9UaRA@mail.gmail.com/
>> Suggested-by: Loic Poulain <loic.poulain@....qualcomm.com>
>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@...il.com>
>> ---
>> Changes:
>> * RFCv2->RFCv5: new patch to address modem disconnection / system
>>   shutdown issues
>> ---
>>  drivers/net/wwan/wwan_core.c | 37 ++++++++++++++++++------------------
>>  1 file changed, 18 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/net/wwan/wwan_core.c b/drivers/net/wwan/wwan_core.c
>> index ade8bbffc93e..33f7a140fba9 100644
>> --- a/drivers/net/wwan/wwan_core.c
>> +++ b/drivers/net/wwan/wwan_core.c
>> @@ -42,6 +42,9 @@ static struct dentry *wwan_debugfs_dir;
>>   * struct wwan_device - The structure that defines a WWAN device
>>   *
>>   * @id: WWAN device unique ID.
>> + * @refcount: Reference count of this WWAN device. When this refcount reaches
>> + * zero, the device is deleted. NB: access is protected by global
>> + * wwan_register_lock mutex.
>>   * @dev: Underlying device.
>>   * @ops: wwan device ops
>>   * @ops_ctxt: context to pass to ops
>> @@ -49,6 +52,7 @@ static struct dentry *wwan_debugfs_dir;
>>   */
>>  struct wwan_device {
>>         unsigned int id;
>> +       int refcount;
>>         struct device dev;
>>         const struct wwan_ops *ops;
>>         void *ops_ctxt;
>> @@ -222,8 +226,10 @@ static struct wwan_device *wwan_create_dev(struct device *parent)
>>
>>         /* If wwandev already exists, return it */
>>         wwandev = wwan_dev_get_by_parent(parent);
>> -       if (!IS_ERR(wwandev))
>> +       if (!IS_ERR(wwandev)) {
>> +               wwandev->refcount++;
>>                 goto done_unlock;
>> +       }
>>
>>         id = ida_alloc(&wwan_dev_ids, GFP_KERNEL);
>>         if (id < 0) {
>> @@ -242,6 +248,7 @@ static struct wwan_device *wwan_create_dev(struct device *parent)
>>         wwandev->dev.class = &wwan_class;
>>         wwandev->dev.type = &wwan_dev_type;
>>         wwandev->id = id;
>> +       wwandev->refcount = 1;
>>         dev_set_name(&wwandev->dev, "wwan%d", wwandev->id);
>>
>>         err = device_register(&wwandev->dev);
>> @@ -263,30 +270,21 @@ static struct wwan_device *wwan_create_dev(struct device *parent)
>>         return wwandev;
>>  }
>>
>> -static int is_wwan_child(struct device *dev, void *data)
>> -{
>> -       return dev->class == &wwan_class;
>> -}
>> -
>>  static void wwan_remove_dev(struct wwan_device *wwandev)
>>  {
>> -       int ret;
>> -
>>         /* Prevent concurrent picking from wwan_create_dev */
>>         mutex_lock(&wwan_register_lock);
>
>FYI, you can use guarded mutex:
>guard(mutex)(&wwan_register_lock);
>This ensures the lock is 'automatically' released when leaving the
>scope/function, and would save the below goto/out_unlock.

Sounds curious. Will keep in my mind for a future development. I would rather keep this patch as simple and clear as possible.

>> -       /* WWAN device is created and registered (get+add) along with its first
>> -        * child port, and subsequent port registrations only grab a reference
>> -        * (get). The WWAN device must then be unregistered (del+put) along with
>> -        * its last port, and reference simply dropped (put) otherwise. In the
>> -        * same fashion, we must not unregister it when the ops are still there.
>> -        */
>> -       if (wwandev->ops)
>> -               ret = 1;
>> -       else
>> -               ret = device_for_each_child(&wwandev->dev, NULL, is_wwan_child);
>> +       if (--wwandev->refcount <= 0) {
>> +               struct device *child = device_find_any_child(&wwandev->dev);
>> +
>> +               if (WARN_ON(wwandev->ops))      /* Paranoid */
>
>You may keep a reference to child (if existing)
>
>> +                       goto out_unlock;
>> +               if (WARN_ON(child)) {           /* Paranoid */
>> +                       put_device(child);
>> +                       goto out_unlock;
>> +               }
>
>Maybe you can simplify that with:
>```
>struct device *child = device_find_any_child(&wwandev->dev);
>put_device(child) /* NULL param is ok */
>if (WARN_ON(child || wwandev->ops))
>    return; /* or goto */
>```

Good point.

Slark, could you adjust this in your future submission or do you want me to send another RFC series?

>>
>> -       if (!ret) {
>>  #ifdef CONFIG_WWAN_DEBUGFS
>>                 debugfs_remove_recursive(wwandev->debugfs_dir);
>>  #endif
>> @@ -295,6 +293,7 @@ static void wwan_remove_dev(struct wwan_device *wwandev)
>>                 put_device(&wwandev->dev);
>>         }
>>
>> +out_unlock:
>>         mutex_unlock(&wwan_register_lock);
>>  }

--
Sergey

Hi Loic,

Powered by blists - more mailing lists