Em Tue, 29 Nov 2016 10:07:21 -0700
Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
> On 11/29/2016 02:15 AM, Mauro Carvalho Chehab wrote:
> > Em Mon, 28 Nov 2016 19:15:12 -0700
> > Shuah Khan <shuahkh@xxxxxxxxxxxxxxx> escreveu:
> >
> >> These two patches fix enable and disable source handler paths. These
> >> aren't dependent patches, grouped because they fix similar problems.
> >
> > Those two patches should be fold, as applying just the first patch
> > would cause au0828 to try to double lock.
> >
>
> No it doesn't. The first patch holds the lock to just clear and set
> enable disable source handlers and doesn't change any other paths.
> The second patch removes lock hold from enable and disable source
> handlers and the callers to hold the lock.
>
> However, I can easily fold them together and not a problem.
>
> >>
> >> This work is triggered by a review comment from Mauro Chehab on a
> >> snd_usb_audio patch about protecting the enable and disabel handler
> >> path in it.
> >>
> >> Ran tests to make sure enable and disable handler paths work. When
> >> digital stream is active, analog app finds the tuner busy and vice
> >> versa. Also ran the Sakari's unbind while video stream is active test.
> >
> > Sorry, but your patches descriptions don't make things clear:
>
> Right. I should have explained it better.
>
> >
> > - It doesn't present any OOPS or logs that would help to
> > understand what you're trying to fix;
> >
> > - From what I understood, you're moving the lock out of
> > enable/disable handlers, and letting their callers to do
> > the locks themselves. Why? Are there any condition where it
> > won't need to be locked?
>
> So here is the scenario these patches fix. Say user app starts
> and during start of video streaming v4l2 checks to see if enable
> source handler is defined. This check is done without holding the
> graph_mutex.
Why? You need to hold the graph_mutex before navigating at the
media graph, or to convert MC to use a lockless protection (like
RCU or restartable sequence).
> If unbind happens to be in progress, au0828 could
> clear enable and disable source handlers. So these could race.
> I am not how large this window is, but could happen.
>
> If graph_mutex protects the check for enable source handler not
> being null, then it has to be released before calling enable source
> handler as shown below:
>
> if (mdev) {
> mutex_lock(&mdev->graph_mutex);
> if (mdev->disable_source) {
> mutex_unlock(&mdev->graph_mutex);
> mdev->disable_source(&vdev->entity);
> } else
> mutex_unlock(&mdev->graph_mutex);
> }
>
> The above will leave another window for handlers to be cleared.
> That is why it would make sense for the caller to hold the lock
> and the call enable and disable source handlers.
I see. Please add such explanation at the patch description,
showing how it should happen, instead.
>
> >
> > - It is not touching documentation. If now the callbacks should
> > not implement locks, this should be explicitly described.
>
> Yes documentation needs to be updated and I can do that in v2 if
> we are okay with this approach.
>
> >
> > Btw, I think it is a bad idea to let the callers to handle
> > the locks. The best would be, instead, to change the code in
> > some other way to avoid it, if possible. If not possible at all,
> > clearly describe why it is not possible and insert some comments
> > inside the code, to avoid some cleanup patch to mess up with this.
> >
>
> Hope the above explanation helps answer the question. We do need a
> way to protect enable and disable handler access and the call itself.
> I am using the same graph_mutex for both, hence I decided to have the
> caller hold the lock. Any other ideas welcome.
>
> thanks,
> -- Shuah
>
Thanks,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
