[PATCH 0/4] v4l: async: fixes for v4l2_async_notifier_unregister()

[PATCH 0/4] v4l: async: fixes for v4l2_async_notifier_unregister()

[Niklas Söderlund]

Hi,

This series is based on top of media-tree and some patches where
previously part of the series '[PATCH v5 0/4] v4l2-async: add
subnotifier registration for subdevices'. Hans suggested the cleanups
could be broken out to a separate series, so this is this series :-)

The aim of this series is to cleanup and document some of the odd things
that happens in v4l2_async_notifier_unregister(). The purpose for this
in the short term is to make it easier to implement subnotifiers which
both I and Sakari are trying to address, this feature is blocking other
drivers such as the Renesas R-Car CSI-2 receiver driver. And in the long
run (I hope) to make it easier to get rid of the need to do re-probing
at all in v4l2_async_notifier_unregister() :-)

Niklas Söderlund (4):
  v4l: async: fix unbind error in v4l2_async_notifier_unregister()
  v4l: async: abort if memory allocation fails when unregistering
    notifiers
  v4l: async: do not hold list_lock when re-probing devices
  v4l: async: add comment about re-probing to
    v4l2_async_notifier_unregister()

 drivers/media/v4l2-core/v4l2-async.c | 49 ++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 21 deletions(-)

-- 
2.13.3

[PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Niklas Söderlund]

The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
to notifier->unbind() have no effect and leaves the notifier confused.
Call the unbind() callback prior to cleaning up the subdevice to avoid
this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 851f128eba2219ad..0acf288d7227ba97 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 
 		d = get_device(sd->dev);
 
+		if (notifier->unbind)
+			notifier->unbind(notifier, sd, sd->asd);
+
 		v4l2_async_cleanup(sd);
 
 		/* If we handled USB devices, we'd have to lock the parent too */
 		device_release_driver(d);
 
-		if (notifier->unbind)
-			notifier->unbind(notifier, sd, sd->asd);
-
 		/*
 		 * Store device at the device cache, in order to call
 		 * put_device() on the final step
-- 
2.13.3

[PATCH 2/4] v4l: async: abort if memory allocation fails when unregistering notifiers

[Niklas Söderlund]

Instead of trying to cope with the failed memory allocation and still
leaving the kernel in a semi-broken state (the subdevices will be
released but never re-probed) simply abort. The kernel have already
printed a warning about allocation failure but keep the error printout
to ease pinpointing the problem if it happens.

By doing this we can increase the readability of this complex function
which puts it in a better state to separate the v4l2 housekeeping tasks
from the re-probing of devices. It also serves to prepare for adding
subnotifers.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 13 ++-----------
 1 file changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 0acf288d7227ba97..67852f0f2d3000c9 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -215,6 +215,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	if (!dev) {
 		dev_err(notifier->v4l2_dev->dev,
 			"Failed to allocate device cache!\n");
+		return;
 	}
 
 	mutex_lock(&list_lock);
@@ -234,23 +235,13 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 		/* If we handled USB devices, we'd have to lock the parent too */
 		device_release_driver(d);
 
-		/*
-		 * Store device at the device cache, in order to call
-		 * put_device() on the final step
-		 */
-		if (dev)
-			dev[i++] = d;
-		else
-			put_device(d);
+		dev[i++] = d;
 	}
 
 	mutex_unlock(&list_lock);
 
 	/*
 	 * Call device_attach() to reprobe devices
-	 *
-	 * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
-	 * executed.
 	 */
 	while (i--) {
 		struct device *d = dev[i];
-- 
2.13.3

[PATCH 3/4] v4l: async: do not hold list_lock when re-probing devices

[Niklas Söderlund]

There is no good reason to hold the list_lock when re-probing the
devices and it prevents a clean implementation of subdevice notifiers.
Move the actual release of the devices outside of the loop which
requires the lock to be held.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/v4l2-core/v4l2-async.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
index 67852f0f2d3000c9..d91ff0a33fd3eaff 100644
--- a/drivers/media/v4l2-core/v4l2-async.c
+++ b/drivers/media/v4l2-core/v4l2-async.c
@@ -206,7 +206,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	unsigned int notif_n_subdev = notifier->num_subdevs;
 	unsigned int n_subdev = min(notif_n_subdev, V4L2_MAX_SUBDEVS);
 	struct device **dev;
-	int i = 0;
+	int i, count = 0;
 
 	if (!notifier->v4l2_dev)
 		return;
@@ -223,27 +223,26 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
 	list_del(&notifier->list);
 
 	list_for_each_entry_safe(sd, tmp, &notifier->done, async_list) {
-		struct device *d;
-
-		d = get_device(sd->dev);
+		dev[count] = get_device(sd->dev);
+		count++;
 
 		if (notifier->unbind)
 			notifier->unbind(notifier, sd, sd->asd);
 
 		v4l2_async_cleanup(sd);
-
-		/* If we handled USB devices, we'd have to lock the parent too */
-		device_release_driver(d);
-
-		dev[i++] = d;
 	}
 
 	mutex_unlock(&list_lock);
 
+	for (i = 0; i < count; i++) {
+		/* If we handled USB devices, we'd have to lock the parent too */
+		device_release_driver(dev[i]);
+	}
+
 	/*
 	 * Call device_attach() to reprobe devices
 	 */
-	while (i--) {
+	for (i = 0; i < count; i++) {
 		struct device *d = dev[i];
 
 		if (d && device_attach(d) < 0) {
-- 
2.13.3

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

[Niklas Söderlund]

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.
+	 *
+	 * 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]);
-- 
2.13.3

Re: [PATCH 0/4] v4l: async: fixes for v4l2_async_notifier_unregister()

[Hans Verkuil]

On 07/31/2017 12:31 AM, Niklas Söderlund wrote:
> Hi,
> 
> This series is based on top of media-tree and some patches where
> previously part of the series '[PATCH v5 0/4] v4l2-async: add
> subnotifier registration for subdevices'. Hans suggested the cleanups
> could be broken out to a separate series, so this is this series :-)
> 
> The aim of this series is to cleanup and document some of the odd things
> that happens in v4l2_async_notifier_unregister(). The purpose for this
> in the short term is to make it easier to implement subnotifiers which
> both I and Sakari are trying to address, this feature is blocking other
> drivers such as the Renesas R-Car CSI-2 receiver driver. And in the long
> run (I hope) to make it easier to get rid of the need to do re-probing
> at all in v4l2_async_notifier_unregister() :-)
> 
> Niklas Söderlund (4):
>   v4l: async: fix unbind error in v4l2_async_notifier_unregister()
>   v4l: async: abort if memory allocation fails when unregistering
>     notifiers
>   v4l: async: do not hold list_lock when re-probing devices
>   v4l: async: add comment about re-probing to
>     v4l2_async_notifier_unregister()
> 
>  drivers/media/v4l2-core/v4l2-async.c | 49 ++++++++++++++++++++----------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 

Looks good to me, but I'd like Sakari's Ack as well. He's still on vacation
for another week, so it won't be merged before next week at the earliest.

Regards,

	Hans

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

[Sakari Ailus]

Hi Niklas,

Thanks for the patchset.

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.

Or can someone come up with a valid reason why the re-probing code should
stay? :-)

> +	 *
> +	 * 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,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Sakari Ailus]

On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote:
> The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
> to notifier->unbind() have no effect and leaves the notifier confused.
> Call the unbind() callback prior to cleaning up the subdevice to avoid
> this.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

This is a bugfix and worthy without any other patches and so should be
applied separately.

I think it'd be safer to store sd->asd locally and call the notifier unbind
with that. Now you're making changes to the order in which things work, and
that's not necessary to achieve the objective of passing the async subdev
pointer to the notifier.

With that changed,

Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 851f128eba2219ad..0acf288d7227ba97 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  
>  		d = get_device(sd->dev);
>  
> +		if (notifier->unbind)
> +			notifier->unbind(notifier, sd, sd->asd);
> +
>  		v4l2_async_cleanup(sd);
>  
>  		/* If we handled USB devices, we'd have to lock the parent too */
>  		device_release_driver(d);
>  
> -		if (notifier->unbind)
> -			notifier->unbind(notifier, sd, sd->asd);
> -
>  		/*
>  		 * Store device at the device cache, in order to call
>  		 * put_device() on the final step
> -- 
> 2.13.3
> 

-- 
Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Monday 31 Jul 2017 00:31:55 Niklas Söderlund wrote:
> The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
> to notifier->unbind() have no effect and leaves the notifier confused.
> Call the unbind() callback prior to cleaning up the subdevice to avoid
> this.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/v4l2-core/v4l2-async.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c
> b/drivers/media/v4l2-core/v4l2-async.c index
> 851f128eba2219ad..0acf288d7227ba97 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct
> v4l2_async_notifier *notifier)
> 
>  		d = get_device(sd->dev);
> 
> +		if (notifier->unbind)
> +			notifier->unbind(notifier, sd, sd->asd);
> +
>  		v4l2_async_cleanup(sd);
> 
>  		/* If we handled USB devices, we'd have to lock the parent too 
*/
>  		device_release_driver(d);
> 
> -		if (notifier->unbind)
> -			notifier->unbind(notifier, sd, sd->asd);
> -
>  		/*
>  		 * Store device at the device cache, in order to call
>  		 * put_device() on the final step

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Laurent Pinchart]

Hi Sakari,

On Tuesday 15 Aug 2017 19:16:14 Sakari Ailus wrote:
> On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote:
> > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
> > to notifier->unbind() have no effect and leaves the notifier confused.
> > Call the unbind() callback prior to cleaning up the subdevice to avoid
> > this.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> This is a bugfix and worthy without any other patches and so should be
> applied separately.
> 
> I think it'd be safer to store sd->asd locally and call the notifier unbind
> with that. Now you're making changes to the order in which things work, and
> that's not necessary to achieve the objective of passing the async subdev
> pointer to the notifier.

But on the other hand I think the unbind notification should be called before 
the subdevice gets unbound, the same way the bound notification is called 
after it gets bound. One of the purposes of the unbind notification is to 
allow drivers to prepare for subdev about to be unbound, and they can't 
prepare if the unbind happened already.

> With that changed,
> 
> Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> 
> > ---
> > 
> >  drivers/media/v4l2-core/v4l2-async.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > b/drivers/media/v4l2-core/v4l2-async.c index
> > 851f128eba2219ad..0acf288d7227ba97 100644
> > --- a/drivers/media/v4l2-core/v4l2-async.c
> > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct
> > v4l2_async_notifier *notifier)> 
> >  		d = get_device(sd->dev);
> > 
> > +		if (notifier->unbind)
> > +			notifier->unbind(notifier, sd, sd->asd);
> > +
> > 
> >  		v4l2_async_cleanup(sd);
> >  		
> >  		/* If we handled USB devices, we'd have to lock the parent too 
*/
> >  		device_release_driver(d);
> > 
> > -		if (notifier->unbind)
> > -			notifier->unbind(notifier, sd, sd->asd);
> > -
> > 
> >  		/*
> >  		
> >  		 * Store device at the device cache, in order to call
> >  		 * put_device() on the final step

-- 
Regards,

Laurent Pinchart

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

[Laurent Pinchart]

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

> Or can someone come up with a valid reason why the re-probing code should
> stay? :-)
> 
> > +	 *
> > +	 * 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

Re: [PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Niklas Söderlund]

Hi,

On 2017-08-18 14:15:26 +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday 15 Aug 2017 19:16:14 Sakari Ailus wrote:
> > On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote:
> > > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
> > > to notifier->unbind() have no effect and leaves the notifier confused.
> > > Call the unbind() callback prior to cleaning up the subdevice to avoid
> > > this.
> > > 
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > This is a bugfix and worthy without any other patches and so should be
> > applied separately.
> > 
> > I think it'd be safer to store sd->asd locally and call the notifier unbind
> > with that. Now you're making changes to the order in which things work, and
> > that's not necessary to achieve the objective of passing the async subdev
> > pointer to the notifier.
> 
> But on the other hand I think the unbind notification should be called before 
> the subdevice gets unbound, the same way the bound notification is called 
> after it gets bound. One of the purposes of the unbind notification is to 
> allow drivers to prepare for subdev about to be unbound, and they can't 
> prepare if the unbind happened already.

I'm not opposed to move in the direction suggested by Sakari but I agree 
with Laurent here. It makes more sens that the unbind callback is called 
before the actual unbind happens. At the same time I agree that it dose 
change the behavior, but I think it's for the better.

> 
> > With that changed,
> > 
> > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > 
> > > ---
> > > 
> > >  drivers/media/v4l2-core/v4l2-async.c | 6 +++---
> > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > > b/drivers/media/v4l2-core/v4l2-async.c index
> > > 851f128eba2219ad..0acf288d7227ba97 100644
> > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct
> > > v4l2_async_notifier *notifier)> 
> > >  		d = get_device(sd->dev);
> > > 
> > > +		if (notifier->unbind)
> > > +			notifier->unbind(notifier, sd, sd->asd);
> > > +
> > > 
> > >  		v4l2_async_cleanup(sd);
> > >  		
> > >  		/* If we handled USB devices, we'd have to lock the parent too 
> */
> > >  		device_release_driver(d);
> > > 
> > > -		if (notifier->unbind)
> > > -			notifier->unbind(notifier, sd, sd->asd);
> > > -
> > > 
> > >  		/*
> > >  		
> > >  		 * Store device at the device cache, in order to call
> > >  		 * put_device() on the final step
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Regards,
Niklas Söderlund

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

[Niklas Söderlund]

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?

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

Re: [PATCH 1/4] v4l: async: fix unbind error in v4l2_async_notifier_unregister()

[Sakari Ailus]

On Fri, Aug 18, 2017 at 03:42:07PM +0200, Niklas Söderlund wrote:
> Hi,
> 
> On 2017-08-18 14:15:26 +0300, Laurent Pinchart wrote:
> > Hi Sakari,
> > 
> > On Tuesday 15 Aug 2017 19:16:14 Sakari Ailus wrote:
> > > On Mon, Jul 31, 2017 at 12:31:55AM +0200, Niklas Söderlund wrote:
> > > > The call to v4l2_async_cleanup() will set sd->asd to NULL so passing it
> > > > to notifier->unbind() have no effect and leaves the notifier confused.
> > > > Call the unbind() callback prior to cleaning up the subdevice to avoid
> > > > this.
> > > > 
> > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > 
> > > This is a bugfix and worthy without any other patches and so should be
> > > applied separately.
> > > 
> > > I think it'd be safer to store sd->asd locally and call the notifier unbind
> > > with that. Now you're making changes to the order in which things work, and
> > > that's not necessary to achieve the objective of passing the async subdev
> > > pointer to the notifier.
> > 
> > But on the other hand I think the unbind notification should be called before 
> > the subdevice gets unbound, the same way the bound notification is called 
> > after it gets bound. One of the purposes of the unbind notification is to 
> > allow drivers to prepare for subdev about to be unbound, and they can't 
> > prepare if the unbind happened already.
> 
> I'm not opposed to move in the direction suggested by Sakari but I agree 
> with Laurent here. It makes more sens that the unbind callback is called 
> before the actual unbind happens. At the same time I agree that it dose 
> change the behavior, but I think it's for the better.

I agree with Laurent's reasoning. Feel free to add my ack.

> 
> > 
> > > With that changed,
> > > 
> > > Acked-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > 
> > > > ---
> > > > 
> > > >  drivers/media/v4l2-core/v4l2-async.c | 6 +++---
> > > >  1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/v4l2-core/v4l2-async.c
> > > > b/drivers/media/v4l2-core/v4l2-async.c index
> > > > 851f128eba2219ad..0acf288d7227ba97 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-async.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-async.c
> > > > @@ -226,14 +226,14 @@ void v4l2_async_notifier_unregister(struct
> > > > v4l2_async_notifier *notifier)> 
> > > >  		d = get_device(sd->dev);
> > > > 
> > > > +		if (notifier->unbind)
> > > > +			notifier->unbind(notifier, sd, sd->asd);
> > > > +
> > > > 
> > > >  		v4l2_async_cleanup(sd);
> > > >  		
> > > >  		/* If we handled USB devices, we'd have to lock the parent too 
> > */
> > > >  		device_release_driver(d);
> > > > 
> > > > -		if (notifier->unbind)
> > > > -			notifier->unbind(notifier, sd, sd->asd);
> > > > -
> > > > 
> > > >  		/*
> > > >  		
> > > >  		 * Store device at the device cache, in order to call
> > > >  		 * put_device() on the final step
> > 
> > -- 
> > Regards,
> > 
> > Laurent Pinchart
> > 
> 
> -- 
> Regards,
> Niklas Söderlund

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

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

[Niklas Söderlund]

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.

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

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

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

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

-- 
Regards,
Niklas Söderlund

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

[Hans Verkuil]

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
> 

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

[Sakari Ailus]

Hi Hans,

On Thu, Aug 24, 2017 at 09:59:41AM +0200, Hans Verkuil wrote:
> 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

Re-probing never helped anything with omap3isp driver as the clock is
removed *after* unregistering async notifier. This means that the
re-probing sub-device driver will get the same clock which is about to be
removed and continues with that happily, only to find the clock gone in a
brief moment.

This could be fixed in the omap3isp driver but it is telling that _no-one
ever complained_.

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

One thing we could do is to remove the memory allocation there. After that
it couldn't fail anymore, leaving the device in an unknown state.

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

Agreed.

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

Re: [PATCH 2/4] v4l: async: abort if memory allocation fails when unregistering notifiers

[Sakari Ailus]

Hi Niklas,

On Mon, Jul 31, 2017 at 12:31:56AM +0200, Niklas Söderlund wrote:
> Instead of trying to cope with the failed memory allocation and still
> leaving the kernel in a semi-broken state (the subdevices will be
> released but never re-probed) simply abort. The kernel have already
> printed a warning about allocation failure but keep the error printout
> to ease pinpointing the problem if it happens.
> 
> By doing this we can increase the readability of this complex function
> which puts it in a better state to separate the v4l2 housekeeping tasks
> from the re-probing of devices. It also serves to prepare for adding
> subnotifers.

By the time the notifier has been removed from the notifier list, we're the
sole user of the notifier done list. There is thus no need to acquire the
list lock to serialise access to that list.

Is there something that prevents re-probing the devices directly off the
notifier's done list without gathering the device pointers first? As the
notifier has been removed from the notifier list already, there will be no
matches found.

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/v4l2-core/v4l2-async.c | 13 ++-----------
>  1 file changed, 2 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> index 0acf288d7227ba97..67852f0f2d3000c9 100644
> --- a/drivers/media/v4l2-core/v4l2-async.c
> +++ b/drivers/media/v4l2-core/v4l2-async.c
> @@ -215,6 +215,7 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  	if (!dev) {
>  		dev_err(notifier->v4l2_dev->dev,
>  			"Failed to allocate device cache!\n");
> +		return;
>  	}
>  
>  	mutex_lock(&list_lock);
> @@ -234,23 +235,13 @@ void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier)
>  		/* If we handled USB devices, we'd have to lock the parent too */
>  		device_release_driver(d);
>  
> -		/*
> -		 * Store device at the device cache, in order to call
> -		 * put_device() on the final step
> -		 */
> -		if (dev)
> -			dev[i++] = d;
> -		else
> -			put_device(d);
> +		dev[i++] = d;
>  	}
>  
>  	mutex_unlock(&list_lock);
>  
>  	/*
>  	 * Call device_attach() to reprobe devices
> -	 *
> -	 * NOTE: If dev allocation fails, i is 0, and the whole loop won't be
> -	 * executed.
>  	 */
>  	while (i--) {
>  		struct device *d = dev[i];
> -- 
> 2.13.3
> 

-- 
Regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi

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

[Hans Verkuil]

On 24/08/17 18:17, Sakari Ailus wrote:
> Hi Hans,
> 
> On Thu, Aug 24, 2017 at 09:59:41AM +0200, Hans Verkuil wrote:
>> 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
> 
> Re-probing never helped anything with omap3isp driver as the clock is
> removed *after* unregistering async notifier. This means that the
> re-probing sub-device driver will get the same clock which is about to be
> removed and continues with that happily, only to find the clock gone in a
> brief moment.
> 
> This could be fixed in the omap3isp driver but it is telling that _no-one
> ever complained_.
> 
>>>>
>>>>
>>>> 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.
> 
> One thing we could do is to remove the memory allocation there. After that
> it couldn't fail anymore, leaving the device in an unknown state.
> 
>>
>>> - 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.
> 
> Agreed.
> 

I want to add another option:

Keep it, finish the async work, then as a final patch remove the reprobe code.

If we discover that this actually breaks something then we can just revert that
final patch without having to rework the whole series.

Regards,

	Hans