linux-kernel - Re: [RFC PATCH v2 11/18] media: tegra-video: Add support for external sensor capture

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 7 Jul 2020 21:35:12 +0200
From:   Hans Verkuil <hverkuil@...all.nl>
To:     Sowjanya Komatineni <skomatineni@...dia.com>,
        thierry.reding@...il.com, jonathanh@...dia.com, frankc@...dia.com,
        sakari.ailus@....fi, robh+dt@...nel.org, helen.koike@...labora.com
Cc:     digetx@...il.com, sboyd@...nel.org, gregkh@...uxfoundation.org,
        linux-media@...r.kernel.org, devicetree@...r.kernel.org,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-i2c@...r.kernel.org
Subject: Re: [RFC PATCH v2 11/18] media: tegra-video: Add support for external
 sensor capture

On 07/07/2020 21:25, Sowjanya Komatineni wrote:
> 
> On 7/7/20 12:01 PM, Sowjanya Komatineni wrote:
>>
>>
>> On 7/6/20 2:10 AM, Hans Verkuil wrote:
>>>> +static void tegra_vi_graph_cleanup(struct tegra_vi *vi)
>>>> +{
>>>> +	struct tegra_vi_channel *chan;
>>>> +
>>>> +	list_for_each_entry(chan, &vi->vi_chans, list) {
>>>> +		video_unregister_device(&chan->video);
>>>> +		mutex_lock(&chan->video_lock);
>>>> +		vb2_queue_release(&chan->queue);
>>> No need for this since this is done in vb2_fop_release().
>>>
>>> In fact, vb2_queue_release should never be called by drivers. Just using
>>> vb2_fop_release or __vb2_fop_release is sufficient.
>>>
>>> The confusion is due to the fact that the name suggests that vb2_queue_release
>>> has to be balanced with vb2_queue_init, but that's not the case. Perhaps
>>> vb2_queue_stop or something like that might be a better name. I'll have to
>>> think about this since I see that a lot of drivers do this wrong.
>>>
>>>> +		mutex_unlock(&chan->video_lock);
>>>> +		v4l2_async_notifier_unregister(&chan->notifier);
>>>> +		v4l2_async_notifier_cleanup(&chan->notifier);
>>>> +	}
>>>> +}
>>>> +
>>
>> vb2_queue_release() here is called to stop streaming a head before media links are removed in case of when driver unbind happens while
>> userspace application holds video device with active streaming in progress.
>>
>> Without vb2_queue_release() here streaming will be active during the driver unbind and by the time vb2_queue_release() happens from
>> vb2_fop_release(), async notifiers gets unregistered and media links will be removed which causes channel stop stream to crash as we can't
>> retrieve sensor subdev  thru media entity pads to execute s_stream on subdev.
>>
> I think we don't need async notifier unbind. Currently media links are removed during unbind so during notifier unregister all subdevs gets
> unbind and links removed.
> 
> media_device_unregister during video device release callback takes care of media entity unregister and removing links.
> 
> So, will try by removing notifier unbind along with removing vb2_queue_release during cleanup.
> 

I actually wonder if vb2_queue_release shouldn't be called from video_unregister_device.

I'll look into this tomorrow.

Regards,

	Hans