[lvc-project] [PATCH v2] media: usb: hackrf: fix device unregister order in hackrf_probe()

[Жандарович Никита Игоревич]

Фёдор, добрый день.

>-----Original Message-----
>From: Fedor Pchelkin [mailto:pchelkin at ispras.ru]
>Sent: Wednesday, February 19, 2025 2:00 PM
>To: Жандарович Никита Игоревич <n.zhandarovich at fintech.ru>
>Cc: lvc-project at linuxtesting.org
>Subject: Re: [lvc-project] [PATCH v2] media: usb: hackrf: fix device
>unregister order in hackrf_probe()
>
>On Tue, 18. Feb 08:56, Жандарович Никита Игоревич wrote:
>> >-----Original Message-----
>> >From: Fedor Pchelkin [mailto:pchelkin at ispras.ru] On Mon, 17. Feb
>> >16:29, Жандарович Никита Игоревич wrote:
>> >> Добрый вечер, Фёдор.
>> >>
>> >> 100% уверенности в корректности решения у меня нет. Но
>после
>> >Ну, в описании не сказано, почему порядок разрегистрации на
>пути
>> >ошибки probe-функции сделан неправильно и как это связано с
>репортом
>> >KASAN. Я бы не сказал, что это очевидно читателю патча.
>> >Скорее всего, в её отсутствие в описании патча ему придётся
>> >дополнительно лезть в исходные тексты ядра и самостоятельно
>> >выискивать нужную информацию.
>> >
>>
>> Согласен, на момент оформления патча этот момент мне казался
>более
>> очевидным, чем следовало. Постараюсь быть чуть более
>подробным в
>> описании проблемы и ее решения.
>
>Изменение порядка вызова фукнций - это ключевой момент патча.
>Отсутствие 100%-ной уверенности в корректности решения и в то
>же время очевидность с Ваших слов ключевого момента патча как-
>то не соотносятся друг с другом.
>
>На текущий момент порядок разрегистрации и освобождения
>объектов выполнен в обратном порядке процессу их
>инициализации. То есть на первый взгляд порядок выглядит
>логично. Поэтому непонятно, каким образом проблема,
>выявленная санитайзером, в целом связана с тем, что делает патч.
>Может есть какая-то более глубинная информация?
>

Ваши замечания корректны.

Я не смог вспомнить свою прежнюю рационализацию. 
Подозреваю, что в ходе первого анализа я сделал неверный вывод о связи и порядке вызовов освобождения ctrl_handler и v4l2_dev. И проблема лежит в другом месте, но пока идентифицировать ее однозначно я не могу.

Косвенные аргументы в пользу патча также не работают: какие-то драйверы делают
	v4l2_device_unregister
	v4l2_ctrl_handler_free
какие-то наоборот. Отмечу, что порядок необязательно должен быть обратным, хотя это и разумный подход.

Отсутствие падений с репродюсером также не имеет веса, т.к. процесс воспроизведения не точный, и иногда требуются ~10 минут, чтобы поймать проблему на непатченной версии.

Немало важно и то, что мой патч неверно переместил метку, поэтому даже если проблема была выявлена верно, патч точно остался неправильным.

Буду дальше исследовать и напишу мейнтейнерам сделать NACK.

Спасибо за комментарии.

>v4l2_ctrl_handler_init(&dev->rx_ctrl_handler, 5) ...
>if (dev->rx_ctrl_handler.error) {
>	ret = dev->rx_ctrl_handler.error;
>	dev_err(dev->dev, "Could not initialize controls\n");
>	goto err_v4l2_ctrl_handler_free_rx;
>}
>...
>v4l2_ctrl_handler_init(&dev->tx_ctrl_handler, 4); if (dev-
>>tx_ctrl_handler.error) {
>	ret = dev->tx_ctrl_handler.error;
>	dev_err(dev->dev, "Could not initialize controls\n");
>	goto err_v4l2_ctrl_handler_free_tx;
>}
>...
>ret = v4l2_device_register(&intf->dev, &dev->v4l2_dev); if (ret) {
>	dev_err(dev->dev, "Failed to register v4l2-device (%d)\n", ret);
>	goto err_v4l2_ctrl_handler_free_tx;
>}
>...
>ret = video_register_device(&dev->rx_vdev, VFL_TYPE_SDR, -1); if (ret)
>{
>	dev_err(dev->dev,
>		"Failed to register as video device (%d)\n", ret);
>	goto err_v4l2_device_unregister;
>}
>...
>err_v4l2_device_unregister:
>	v4l2_device_unregister(&dev->v4l2_dev);
>err_v4l2_ctrl_handler_free_tx:
>	v4l2_ctrl_handler_free(&dev->tx_ctrl_handler);
>err_v4l2_ctrl_handler_free_rx:
>	v4l2_ctrl_handler_free(&dev->rx_ctrl_handler);