lkml.org 
[lkml]   [2022]   [Nov]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering
    From
    Hi Oleh,

    On 2022/11/11 18:39, Oleh Kravchenko wrote:
    > Hello Wang,
    >
    >> 11 лист. 2022 р. о 11:21 wangyufen <wangyufen@huawei.com> написав(ла):
    >>
    >>
    >> 在 2022/11/9 18:43, Oleh Kravchenko 写道:
    >>>
    >>>
    >>>> 9 лист. 2022 р. о 12:25 wangyufen <wangyufen@huawei.com> написав(ла):
    >>>>
    >>>>
    >>>> 在 2022/11/9 17:39, Oleh Kravchenko 写道:
    >>>>
    >>>>>> -static void el15203000_remove(struct spi_device *spi)
    >>>>>>
    >>>>> Is remove() callback from struct spi_driver deprecated?
    >>>>>
    >>>> It is not that remove() callback is deprecated,
    >>>> it's that after wrapping mutex_destroy() call with devm_add_action_or_reset(),
    >>>> remove() callback is unnecessary here.
    >>>>
    >>> When remove() is called, the memory allocated by devm_*() is valid.
    >>> So what you try to fix here?
    >>
    >> Fix the &priv->lock used after destroy, for details, please see patch #0
    >> LKML: Wang Yufen: [PATCH 00/13] leds: Fix devm vs. non-devm ordering
    >
    > It doesn’t make any sense for me.
    > You saying that remove() called before devm_* allocation
    > if it true then set_brightness_delayed() will crash the system in anyway.
    >
    > LED device has a parent SPI device; LED device can’t exist without SPI device.
    >
    > So deallocation order should be next:
    > 1. LED device devm_*()
    > 2. SPI device remove()

    The allocation order is as follows:

    el15203000_probe()
    mutex_init(&priv->lock);
    el15203000_probe_dt(priv)
    device_for_each_child_node(priv->dev, child) {
    ...
    led->ldev.brightness_set_blocking = el15203000_set_blocking;
    ...
    devm_led_classdev_register_ext(priv->dev, &led->ldev, &init_data);
    dr = devres_alloc(devm_led_classdev_release, sizeof(*dr), GFP_KERNEL);
    <-- dr->node.release = devm_led_classdev_release()
    ...
    devres_add(parent, dr);
    <-- add dr->node to &priv->dev->devres_head

    And the full deallocation order should be this:

    1. SPI device .remove callback
    2. LED device devm_*()
    3. SPI device deallocation

    spi_unregister_device()
    device_del()
    bus_remove_device()
    device_release_driver_internal()
    __device_release_driver()
    ...
    device_remove()
    spi_remove() <-- call el15203000_remove() here, mutex_destroy(&priv->lock), lock destroy
    ...
    device_unbind_cleanup()
    devres_release_all()
    release_nodes()
    <-- traverse spi->dev->devres_head list and call dr->node.release in sequence.
    devm_led_classdev_release()
    led_classdev_unregister()
    <-- flush set_brightness_work here, before the work flush, set_brightness_work may be sched.
    <-- that is el15203000_set_blocking()..-> mutex_lock(&led->priv->lock) is called,
    <-- this leads to the priv->lock use after destroy.
    put_device(&spi->dev) <-- spi device is deallocation in here


    Regards,
    Wei Yongjun

    \
     
     \ /
      Last update: 2022-11-15 03:08    [W:4.715 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site