Messages in this thread | | | Date | Tue, 22 Nov 2022 09:10:12 +0800 | Subject | Re: [PATCH 02/13] leds: el15203000: Fix devm vs. non-devm ordering | From | Wang Yufen <> |
| |
在 2022/11/15 10:06, Wei Yongjun 写道: > 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 > > Hi Oleh,
Judging from the deallocation order above, there is a issue that the &priv->lock used after destroy, right?
And thanks Wei for the detailed explanation.
Thanks, Wang
> Regards, > Wei Yongjun >
| |