Re: [PATCH 0/2] media protect enable and disable source handler paths

[Shuah Khan]

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. 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.

> 
> - 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