Re: [RFC PATCH v7 6/9] media: tegra: Add Tegra210 Video input driver

From: Sowjanya Komatineni <skomatineni@nvidia.com>
To: Dmitry Osipenko <digetx@gmail.com>, <thierry.reding@gmail.com>,
	<jonathanh@nvidia.com>, <frankc@nvidia.com>, <hverkuil@xs4all.nl>,
	<sakari.ailus@iki.fi>, <helen.koike@collabora.com>
Cc: <sboyd@kernel.org>, <linux-media@vger.kernel.org>,
	<devicetree@vger.kernel.org>, <linux-clk@vger.kernel.org>,
	<linux-tegra@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [RFC PATCH v7 6/9] media: tegra: Add Tegra210 Video input driver
Date: Wed, 15 Apr 2020 11:53:50 -0700	[thread overview]
Message-ID: <786949a9-8507-7723-f29b-b91a216bfd28@nvidia.com> (raw)
In-Reply-To: <d6a9e07c-474a-a076-8313-32f5f4ca8d64@nvidia.com>


On 4/15/20 11:39 AM, Sowjanya Komatineni wrote:
>
> On 4/15/20 10:48 AM, Sowjanya Komatineni wrote:
>>
>> On 4/15/20 10:47 AM, Sowjanya Komatineni wrote:
>>>
>>> On 4/15/20 10:21 AM, Sowjanya Komatineni wrote:
>>>>
>>>> On 4/15/20 9:54 AM, Sowjanya Komatineni wrote:
>>>>>
>>>>> On 4/15/20 7:22 AM, Dmitry Osipenko wrote:
>>>>>> External email: Use caution opening links or attachments
>>>>>>
>>>>>>
>>>>>> 15.04.2020 05:57, Sowjanya Komatineni пишет:
>>>>>>> +static int tegra_csi_remove(struct platform_device *pdev)
>>>>>>> +{
>>>>>>> +     struct tegra_csi *csi = platform_get_drvdata(pdev);
>>>>>>> +     int err;
>>>>>>> +
>>>>>>> +     err = host1x_client_unregister(&csi->client);
>>>>>>> +     if (err < 0) {
>>>>>>> +             dev_err(csi->dev,
>>>>>>> +                     "failed to unregister host1x client: 
>>>>>>> %d\n", err);
>>>>>>> +             return err;
>>>>>>> +     }
>>>>>>> +
>>>>>>> +     pm_runtime_disable(csi->dev);
>>>>>>> +     kfree(csi);
>>>>>> IIRC, the driver removal is invoked on the unbinding. Hence, I'm not
>>>>>> sure how moving away from the resource-managed API helps here. 
>>>>>> Could you
>>>>>> please explain in a more details?
>>>>>>
>>>>>> Have you tried to test this driver under KASAN? I suspect that 
>>>>>> you just
>>>>>> masked the problem, instead of fixing it.
Tested with kmemleak scan and did not see any memory leaks
>>>>> Using devm_kzalloc for vi/csi structures based on prior feedback 
>>>>> request to switch to use kzalloc all over this driver.
>>>>>
>>>>> Hi Hans,
>>>>>
>>>>> video devices lifetime is till video device nodes are released. 
>>>>> So, v4l2 device release callback does the release of tegra channel 
>>>>> allocation which hold video device.
>>>>>
>>>>> Below are the 3 possible cases of unbind/unload,
>>>>>
>>>>> 1. during tegra-video module unload, if v4l2 device refcnt is not 
>>>>> 0 which is the case when any of video device node handle is kept 
>>>>> opened then unloading module will not happen and module refcnt is 
>>>>> also non-zero and unloading tegra-video module reports module in use.
> v4l2 device is associated with host1x device where during 
> v4l2_device_register get_device causes refcnt of tegra video host1x 
> device to increase and prevents allowing module unload/load till v4l2 
> device release happens.
>
>
>>>>> 2. during tegra-video driver unbind, tegra-video driver removal 
>>>>> will do vi/csi clients exit ops which unregisters video device 
>>>>> allocated memory during release callback of v4l2 device. vi/csi 
>>>>> structure allocation remains same as vi/csi driver removal will 
>>>>> not happen in this case.
>>>>>
>>>>>
>>>>> 3. during direct host1x client drivers vi/csi unbind, both 
>>>>> host1x_clients vi/csi gets unregistered, deletes host1x logical 
>>>>> device which executes tegra-video driver removal() -> vi/csi 
>>>>> exit() before vi/csi memory gets freed in vi/csi driver remove().
>>>>>
>>>>> So, any active streaming will stop and video devices are 
>>>>> unregistered during direct client driver unbind prior to freeing 
>>>>> vi/csi memory.
>>>>>
>>>>> Also vi/csi driver remove does explicit free vi/csi as its 
>>>>> allocated with kzalloc. So not sure how using kzalloc is different 
>>>>> to devm_kzalloc for vi/csi structure in terms of when vi/csi 
>>>>> memory gets freed?
>>>>>
>>>>> Except for channel allocation which holds video device and as 
>>>>> video device life time is beyond tegra-video module unbind->vi 
>>>>> exit(), looks like we can use devm_kzalloc for vi/csi.
>>>>>
>>>>>
>>>>> Can you please comment if you still think we need to use kzalloc 
>>>>> rather than devm_kzalloc for vi/csi structure allocation?
>>>>>
>>>>> Thanks
>>>>>
>>>>> Sowjanya
>>>>>
>>>> One more case is when video device node is kept opened with 
>>>> v4l2-ctl sleep (rather than streaming), where it will keep device 
>>>> node open for specified time and if direct vi client driver unbind 
>>>> happens then vi driver remove() will free vi memory before v4l2 
>>>> device release happens.
>>>>
>>>> But I don't see any crash or errors with this case.
>
> In the above case, channels allocated memory release may not happen in 
> this case as list head pointer will be gone when vi memory is freed 
> during direct client unbind and by the time v4l2 device release 
> callback gets executed vi channels list head is gone.
>
> Also, freeing vi structure memory can't be done in v4l2 device release 
> callback either.
>
>>>>
>>>> Also if we allow direct client driver unbind, then vi structure 
>>>> memory lifetime should also be till v4l2 device release happens.
>>>>
>>>> But we can free vi in v4l2 device release callback as in case when 
>>>> device node is not kept opened, video device release happens 
>>>> immediate and we cant free vi that early.
>>>
>>> typo fix:
>>>
>>> But we can't free vi structure memory allocation in v4l2 device 
>>> release callback as in case when device node is not kept opened, 
>>> device release happens immediate and we can't free vi structure 
>>> memory that early.
>>>
>
>>>> Hans/Thierry, Can you please comment on this case?
>>>>
>>>> Thanks
>>>>
>>>> Sowjanya
>>>>
>> Also, Can you please help explain on cases where we do/need direct 
>> host1x clients vi/csi drivers unbind?