| 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
| ||
|
Message-ID: <1767e50f-efb7-5e89-22f6-0917821b660d@nvidia.com>
Date: Sat, 2 May 2020 08:38:37 -0700
From: Sowjanya Komatineni <skomatineni@...dia.com>
To: Dmitry Osipenko <digetx@...il.com>, <thierry.reding@...il.com>,
<jonathanh@...dia.com>, <frankc@...dia.com>, <hverkuil@...all.nl>,
<sakari.ailus@....fi>, <helen.koike@...labora.com>
CC: <sboyd@...nel.org>, <linux-media@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-clk@...r.kernel.org>,
<linux-tegra@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [RFC PATCH v11 6/9] media: tegra: Add Tegra210 Video input driver
On 5/2/20 8:16 AM, Dmitry Osipenko wrote:
> 02.05.2020 06:55, Sowjanya Komatineni пишет:
>> On 5/1/20 8:39 PM, Sowjanya Komatineni wrote:
>>>
>>> On 5/1/20 2:05 PM, Sowjanya Komatineni wrote:
>>>>
>>>> On 5/1/20 1:58 PM, Sowjanya Komatineni wrote:
>>>>>
>>>>> On 5/1/20 1:44 PM, Sowjanya Komatineni wrote:
>>>>>>
>>>>>> On 5/1/20 11:03 AM, Sowjanya Komatineni wrote:
>>>>>>>
>>>>>>> On 4/30/20 4:33 PM, Sowjanya Komatineni wrote:
>>>>>>>>
>>>>>>>> On 4/30/20 4:14 PM, Sowjanya Komatineni wrote:
>>>>>>>>>>>>>>> And in this case synchronization between start/finish
>>>>>>>>>>>>>>> threads should be
>>>>>>>>>>>>>>> needed in regards to freezing.
>>>>>>>>>>>>>> Was thinking to have counter to track outstanding frame
>>>>>>>>>>>>>> w.r.t single shot issue b/w start and finish and allow to
>>>>>>>>>>>>>> freeze only when no outstanding frames in process.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> This will make sure freeze will not happen when any buffers
>>>>>>>>>>>>>> are in progress
>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Note that this could be a wrong assumption, I'm not
>>>>>>>>>>>>>>> closely familiar
>>>>>>>>>>>>>>> with how freezer works.
>>>>>>>>>>>>> kthread_start can unconditionally allow try_to_freeze before
>>>>>>>>>>>>> start of frame capture
>>>>>>>>>>>>>
>>>>>>>>>>>>> We can compute captures inflight w.r.t single shot issued
>>>>>>>>>>>>> during capture start and finished frames by kthread_finish
>>>>>>>>>>>>> and allow kthread_finish to freeze only when captures
>>>>>>>>>>>>> inflight is 0.
>>>>>>>>>>>>>
>>>>>>>>>>>>> This allows freeze to happen b/w frames but not in middle of
>>>>>>>>>>>>> frame
>>>>>>>>>> will have caps inflight check in v12 to allow freeze finish
>>>>>>>>>> thread only when no captures are in progress
>>>>>>>>>
>>>>>>>>> try_to_freeze() returns thread frozen state and looks like we
>>>>>>>>> can use this in kthread finish to allow finish thread to freeze
>>>>>>>>> only when kthread_start is already frozen and no buffers in
>>>>>>>>> progress/initiated for capture.
>>>>>>>>>
>>>>>>>> chan->capture_frozen holds frozen state returned from
>>>>>>>> try_to_freeze() in start kthread
>>>>>>>>
>>>>>>>> chan->capture_reqs increments after every single shot issued.
>>>>>>>>
>>>>>>>>
>>>>>>>> static int chan_capture_kthread_finish(void *data)
>>>>>>>>
>>>>>>>> {
>>>>>>>> struct tegra_vi_channel *chan = data;
>>>>>>>> struct tegra_channel_buffer *buf;
>>>>>>>> int caps_inflight;
>>>>>>>>
>>>>>>>> set_freezable();
>>>>>>>>
>>>>>>>> while (1) {
>>>>>>>> wait_event_interruptible(chan->done_wait,
>>>>>>>> !list_empty(&chan->done) ||
>>>>>>>> kthread_should_stop());
>>>>>>>>
>>>>>>>> /* dequeue buffers and finish capture */
>>>>>>>> buf = dequeue_buf_done(chan);
>>>>>>>> while (buf) {
>>>>>>>> tegra_channel_capture_done(chan, buf);
>>>>>>>> buf = dequeue_buf_done(chan);
>>>>>>>> }
>>>>>>>>
>>>>>>>> if (kthread_should_stop())
>>>>>>>> break;
>>>>>>>>
>>>>>>>> caps_inflight = chan->capture_reqs - chan->sequence;
>>>>>>>> if (chan->capture_frozen && !caps_inflight)
>>>>>>>> try_to_freeze();
>>>>>>>> }
>>>>>>>>
>>>>>>>> return 0;
>>>>>>>> }
>>>>>>>
>>>>>>> Freezing happens prior to suspend() during suspend entry and when
>>>>>>> we implement suspend/resume during suspend we stop streaming where
>>>>>>> we stop threads anyway.
>>>>>>>
>>>>>>> So, was thinking why we need these threads freezable here?
>>>>>>>
>>>>>>>
>>>>>> Hi Dmitry,
>>>>>>
>>>>>> Did some testing and below are latest observation and fix I tested.
>>>>>>
>>>>>> wait_event_interruptible() uses schedule() which blocks the freezer.
>>>>>> When I do suspend while keeping streaming active in background, I
>>>>>> see freezing of these threads fail and call trace shows __schedule
>>>>>> -> __switch_to from these kthreads.
>>>>>>
>>>>>> wait_event_freezable() uses freezable_schedule() which should not
>>>>>> block the freezer but we can't use this here as we need conditional
>>>>>> try_to_freeze().
>>>>>>
>>>>>>
>>>>>> So, doing below sequence works where we set PF_FREEZER_SKIP flag
>>>>>> thru freezer_not_count() before wait_event which calls schedule()
>>>>>> and remove PF_FREEZER_SKIP after schedule allows try_to_freeze to
>>>>>> work and also conditional try_to_freeze below prevents freezing
>>>>>> thread in middle of capture.
>>>>>>
>>>>>> while (1) {
>>>>>> freezer_not_count()
>>>>>> wait_event_interruptible()
>>>>>> freezer_count()
>>>>>> ...
>>>>>> ...
>>>>>> if (chan->capture_frozen && !caps_inflight)
>>>>>> try_to_freeze()
>>>>>> }
>>>>>>
>>>>>> Please comment if you agree with above sequence. Will include this
>>>>>> in v12.
>>>>>>
>>>> sorry, freezer_count() does try_to_freeze after clearing skip flag.
>>>> So, dont think we can use this as we need conditional try_to_freeze.
>>>> Please ignore above sequence.
>>>>> Or probably we can take closer look on this later when we add
>>>>> suspend/resume support as it need more testing as well.
>>>>>
>>>>> As this is initial series which has TPG only I think we shouldn't
>>>>> get blocked on this now. Series-2 and 3 will be for sensor support
>>>>> and on next series when we add suspend/resume will look into this.
>>>>>
>>>>>
>>> When freeze activity starts and in case if finish thread freezes prior
>>> to start thread issuing capture, its the VI hardware writes data to
>>> the allocated buffer address.
>>>
>>> finish thread just checks for the event from the hardware and we don't
>>> handle/process directly on memory in this driver.
>>>
>>> So even we freeze done thread when single shot is issued frame buffer
>>> gets updated.
>>>
>>> In case if capture thread is frozen there will not buffers queued to
>>> process by finish thread. So, this will not be an issue.
>>>
>>> So, probably we don't need to do conditional try_to_freeze and what we
>>> have should work good in this corner case.
>>>
>> I still need to change wait_event_interruptible() to
>> wait_event_freezable() but no need to synchronize finish thread freeze
>> with start thread as even on issuing capture start its vi hardware that
>> does frame buffer update and finish thread just checks for mw_ack event
>> and returns buffer to application.
> The problem we are primarily trying to avoid is to have suspending being
> done in the middle of IO.
>
> IIUC, even if system will be suspended in the middle of VI IO, it won't
> be fatal. In worst case the buffer capture should fail on resume from
> suspend. Could you please try to simulate this potential issue and see
> what result will be on suspending in the middle of VI IO?
>
> We don't want to suspend system / stop streaming in the middle of IO, so
> this problem of a proper threads tear-down still exists. It should
> become easier to resolve the problem in a case of a proper suspending
> callback because the "start" thread could be turned down at any time, so
> it should be easier to maintain a proper tear-down order when threads
> are fully controlled by the driver, i.e. the "start" thread goes down
> first and the "finish" is second, blocking until the capture is completed.
>
> I think yours suggestion about dropping the freezing from the threads
> for now and returning back to it later on (once a proper suspend/resume
> support will be added) sounds reasonable.
>
> But if you'd want to keep the freezing, then the easy solution could be
> like that:
>
> 1. "start" thread could freeze at any time
> 2. "finish" thread could freeze only when the "start" thread is frozen
> and capture isn't in-progress. Use frozen(kthread_start_capture) to
> check the freezing state.
>
> https://elixir.bootlin.com/linux/v5.7-rc3/source/include/linux/freezer.h#L25
That's exactly what I tried, below is the snippet.
But as mentioned I am seeing freezing fail when I
wait_event_interruptible() in either of the threads.
60.368709] Call trace:
[ 60.371216] __switch_to+0xec/0x140
[ 60.374768] __schedule+0x32c/0x668
[ 60.378315] schedule+0x78/0x118
[ 60.381606] chan_capture_kthread_finish+0x244/0x2a0 [tegra_video]
[ 60.387865] kthread+0x124/0x150
[ 60.391150] ret_from_fork+0x10/0x1c
wait_event_interruptible() API uses schedule() which blocks freezer
while wait_event_freezable APIs uses freezable_schedule() which allows
to skip freezer during schedule and then clears skip and calls
try_to_freeze()
But we can't use wait_event_freezable() here as we need conditional freeze.
while (1) {
caps_inflight = chan->capture_reqs - chan->sequence;
if (frozen(chan->kthread_start_capture) && !caps_inflight)
wait_event_freezable(chan->done_wait,
!list_empty(&chan->done) ||
kthread_should_stop());
else
wait_event_interruptible(chan->done_wait,
!list_empty(&chan->done) ||
kthread_should_stop());
/* dequeue buffers and finish capture */
...
...
if (kthread_should_stop())
break;
}
Powered by blists - more mailing lists