Re:Re: [RFC PATCH v5 2/7] net: wwan: core: explicit WWAN device reference counting

["Slark Xiao" <slark_xiao@163.com>]

At 2026-01-15 14:52:08, "Sergey Ryazanov" <ryazanov.s.a@gmail.com> wrote:
>On January 14, 2026 11:31:45 PM, Loic Poulain <loic.poulain@oss.qualcomm.com> wrote:
>>On Fri, Jan 9, 2026 at 2:09 AM Sergey Ryazanov <ryazanov.s.a@gmail.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@gmail.com>
>>> Closes: https://lore.kernel.org/netdev/CAGRyCJE28yf-rrfkFbzu44ygLEvoUM7fecK1vnrghjG_e9UaRA@mail.gmail.com/
>>> Suggested-by: Loic Poulain <loic.poulain@oss.qualcomm.com>
>>> Signed-off-by: Sergey Ryazanov <ryazanov.s.a@gmail.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?
>
I will update it in my next submission.

>>>
>>> -       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,