Re: [PATCH 0/2] Use Media Dev Allocator to fix vimc dev lifetime bugs

[Shuah Khan <skhan@linuxfoundation.org>]

Hi Laurent,

On 6/16/19 12:45 PM, Laurent Pinchart wrote:
> Hi Shuah,
> 
> On Fri, Jun 14, 2019 at 05:26:46PM -0600, Shuah Khan wrote:
>> On 6/13/19 7:24 AM, Helen Koike wrote:
>>> On 6/13/19 2:44 AM, Hans Verkuil wrote:
>>>> On 5/24/19 5:31 AM, Shuah Khan wrote:
>>>>> media_device is embedded in struct vimc_device and when vimc is removed
>>>>> vimc_device and the embedded media_device goes with it, while the active
>>>>> stream and vimc_capture continue to access it.
>>>>>
>>>>> Fix the media_device lifetime problem by changing vimc to create shared
>>>>> media_device using Media Device Allocator API and vimc_capture getting
>>>>> a reference to vimc module. With this change, vimc module can be removed
>>>>> only when the references are gone. vimc can be removed after vimc_capture
>>>>> is removed.
>>>>>
>>>>> Media Device Allocator API supports just USB devices. Enhance it
>>>>> adding a genetic device allocate interface to support other media
>>>>> drivers.
>>>>>
>>>>> The new interface takes pointer to struct device instead and creates
>>>>> media device. This interface allows a group of drivers that have a
>>>>> common root device to share media device resource and ensure media
>>>>> device doesn't get deleted as long as one of the drivers holds its
>>>>> reference.
>>>>>
>>>>> The new interface has been tested with vimc component driver to fix
>>>>> panics when vimc module is removed while streaming is in progress.
>>>>
>>>> Helen, can you review this series? I'm not sure this is the right approach
>>>> for a driver like vimc, and even if it is, then it is odd that vimc-capture
>>>> is the only vimc module that's handled here.
>>>
>>> Hi Hans,
>>>
>>> Yes, I can take a look. Sorry, I've been a bit busy these days but I'll
>>> try to take a look at this patch series (and the others) asap.
>>>
>>> Helen
>>>
>>>> My gut feeling is that this should be handled inside vimc directly and not
>>>> using the media-dev-allocator.
>>
>> Hi Hans and Helen,
>>
>> I explored fixing the problem within vimc before I went down the path to
>> use Media Device Allocator API. I do think that it is cleaner to go this
>> way and easier to maintain.
>>
>> vimc is a group pf component drivers that rely on the media device vimc
>> in vimc and falls into the use-case Media Device Allocator API is added
>> to address. The release and life-time management happens without vimc
>> component drivers being changed other than using the API to get and put
>> media device reference.
> 
> Our replies crossed each other, please see my reply to Hans. I would
> just like to comment here that if having multiple kernel modules causes
> issue, they can all be merged together. There's no need for vimc to be
> handled through multiple modules (I actually think it's quite
> counterproductive, it only makes it more complex, for no added value).
> 

There are several problems in this group of drivers as far as lifetime
management is concerned. I explained some of it in the patch 2/2

If vimc module is removed while streaming is active, vimc_exit runs
into NULL pointer dereference error when streaming thread tries to
access and lock graph_mutex in the struct media_device.

The primary reason for this is that:

media_device is embedded in struct vimc_device and when vimc is removed
vimc_device and the embedded media_device goes with it, while the active
stream and vimc_capture continue to access it.

If we chose to keep these drivers as component drivers, media device
needs to stick around until all components stop using it. This is tricky
because there is no tie between these set of drivers. vimc module can
be deleted while others are still active. As vimc gets removed, other
component drivers start wanting to access the media device tree.

This is classic media device lifetime problem which could be solved
easily with the way I solved it with this series. I saw this as a
variation on the same use-case we had with sound and media drivers
sharing the media device.

I have a TODO request from you asking to extend Media Device Allocator
API to generic case and not restrict it to USB devices. My thinking is
that this gives a perfect test case to extend the API to be generic
and use to solve this problem.

Collapsing the drivers into one might be lot more difficult and complex
than solving this problem with Media Device Allocator API. This approach
has an added benefit of extending the API to be generic and not just for
USB.

I looked at this as a good way to add generic API and have a great test
case for it. This patch series fixes the problem for the current vimc
architecture.

thanks,
-- Shuah