Re: [PATCH 4/4] v4l: async: add comment about re-probing to v4l2_async_notifier_unregister()

[Hans Verkuil <hverkuil@xs4all.nl>]

On 08/23/17 21:03, Niklas Söderlund wrote:
> Hi,
> 
> On 2017-08-18 15:42:37 +0200, Niklas Söderlund wrote:
>> Hi Sakari and Laurent,
>>
>> Thanks for your feedback.
>>
>> On 2017-08-18 14:20:08 +0300, Laurent Pinchart wrote:
>>> Hello,
>>>
>>> On Tuesday 15 Aug 2017 19:09:33 Sakari Ailus wrote:
>>>> On Mon, Jul 31, 2017 at 12:31:58AM +0200, Niklas Söderlund wrote:
>>>>> The re-probing of subdevices when unregistering a notifier is tricky to
>>>>> understand, and implemented somewhat as a hack. Add a comment trying to
>>>>> explain why the re-probing is needed in the first place and why existing
>>>>> helper functions can't be used in this situation.
>>>>>
>>>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>>>> ---
>>>>>
>>>>>  drivers/media/v4l2-core/v4l2-async.c | 17 +++++++++++++++++
>>>>>  1 file changed, 17 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c
>>>>> b/drivers/media/v4l2-core/v4l2-async.c index
>>>>> d91ff0a33fd3eaff..a3c5a1f6d4d2ab03 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-async.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
>>>>> @@ -234,6 +234,23 @@ void v4l2_async_notifier_unregister(struct
>>>>> v4l2_async_notifier *notifier)> 
>>>>>  	mutex_unlock(&list_lock);
>>>>>
>>>>> +	/*
>>>>> +	 * Try to re-probe the subdevices which where part of the notifier.
>>>>> +	 * This is done so subdevices which where part of the notifier will
>>>>> +	 * be re-probed to a pristine state and put back on the global
>>>>> +	 * list of subdevices so they can once more be found and associated
>>>>> +	 * with a new notifier.
>>>>
>>>> Instead of tweaking the code trying to handle unhandleable error conditions
>>>> in notifier unregistration and adding lengthy stories on why this is done
>>>> the way it is, could we simply get rid of the driver re-probing?
>>>>
>>>> I can't see why drivers shouldn't simply cope with the current interfaces
>>>> without re-probing to which I've never seen any reasoned cause. When a
>>>> sub-device driver is unbound, simply return the sub-device node to the list
>>>> of async sub-devices.
>>>
>>> I agree, this is a hack that we should get rid of. Reprobing has been there 
>>> from the very beginning, it's now 4 years and a half old, let's allow it to 
>>> retire :-)
>>
>> I would also be happy to see this code go away :-)
>>
>>>
>>>> Or can someone come up with a valid reason why the re-probing code should
>>>> stay? :-)
>>
>> Hans kindly dug out the original reason talking about why this code was 
>> added in the first place at
>>
>>     http://lkml.iu.edu/hypermail/linux/kernel/1210.2/00713.html
>>
>> I would also like record here what Laurent stated about this after 
>> reading the above on #v4l 
>>
>> 13:53  pinchartl : what could happen is this
>> 13:53  pinchartl : the master could export resources used by the subdev
>> 13:53  pinchartl : the omap3 isp driver, for instance, is a clock source
>> 13:54  pinchartl : and the clock can be used by sensors
>> 13:54  pinchartl : so if you remove the omap3 isp, the clock won't be 
>>    there anymore
>> 13:54  pinchartl : and that's bad for the subdev
>>
>>
>> I don't claim I fully understand all the consequences of removing this 
>> reprobing now. But maybe it's safer to lave the current behavior in for 
>> now until the full problem is understood and move forward whit these 
>> patches since at least they document the behavior and removes another 
>> funky bit when trying to handle the situation where the memory 
>> allocation fails? What do you guys think?
> 
> Any thoughts about how I can move forward with this? The reason I'm 
> asking is that this is a dependency for the sub-notifier patches which 
> in turn is dependency for the R-Car CSI-2 driver :-) If someone wants to 
> think more about this that is fine I just don't want it to be forgotten.  
> As I see it these are the options open to me, but as always I'm always 
> open to other solutions which I'm to narrow minded to see :-)
> 
> - If after the latest discussions it feels the safest option is to keep 
>   the re-probe logic but separating the v4l2 housekeeping from re-probe 
>   logic move forward with this series as-is.

I prefer this. We can always remove the reprobe code later once we have
a better understanding. I see no downside to this cleanup series and it
doesn't block any future development.

> - Post 1/4 separately and repost patch 2/4 -- 4/4 in a v2 to allow for 
>   more input on what is the right thing to do here.

I'm OK with this as well, we missed the 4.14 merge window anyway.

> - Post 1/4 separately, drop patch 2/4 -- 4/4 and create a new patch 
>   which removes all re-probe related code and post that as a new patch.  
>   I would feel a but uneasy about this without a consensus from all you 
>   guys since I don't understand all the ramifications in doing so.

I'm uneasy about this as well.

> - Post 1/4 separately, drop patch 2/4 -- 4/4 and try to rework the 
>   sub-notifier code to work the intertwined v4l2 and re-probe portions 
>   of the code.

Sorry, I don't understand this one.

Regards,

	Hans

> 
>>
>>>>
>>>>> +	 *
>>>>> +	 * One might be tempted to use device_reprobe() to handle the re-
>>>>> +	 * probing. Unfortunately this is not possible since some video
>>>>> +	 * device drivers call v4l2_async_notifier_unregister() from
>>>>> +	 * there remove function leading to a dead lock situation on
>>>>> +	 * device_lock(dev->parent). This lock is held when video device
>>>>> +	 * drivers remove function is called and device_reprobe() also
>>>>> +	 * tries to take the same lock, so using it here could lead to a
>>>>> +	 * dead lock situation.
>>>>> +	 */
>>>>> +
>>>>>  	for (i = 0; i < count; i++) {
>>>>>  	
>>>>>  		/* If we handled USB devices, we'd have to lock the parent too 
>>> */
>>>>>  		device_release_driver(dev[i]);
>>>
>>> -- 
>>> Regards,
>>>
>>> Laurent Pinchart
>>>
>>
>> -- 
>> Regards,
>> Niklas Söderlund
>