| Message ID | 20171004215051.13385-2-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Thu, Oct 05, 2017 at 12:50:20AM +0300, Sakari Ailus wrote: > Remove V4L2 async re-probing support. The re-probing support has been > there to support cases where the sub-devices require resources provided by > the main driver's hardware to function, such as clocks. > > Reprobing has allowed unbinding and again binding the main driver without > explicilty unbinding the sub-device drivers. This is certainly not a > common need, and the responsibility will be the user's going forward. > > An alternative could have been to introduce notifier specific locks. > Considering the complexity of the re-probing and that it isn't really a > solution to a problem but a workaround, remove re-probing instead. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.co.uk> -- Sebastian > drivers/media/v4l2-core/v4l2-async.c | 54 +----------------------------------- > 1 file changed, 1 insertion(+), 53 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index d741a8e0fdac..60a1a50b9537 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -198,78 +198,26 @@ EXPORT_SYMBOL(v4l2_async_notifier_register); > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > { > struct v4l2_subdev *sd, *tmp; > - 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; > > if (!notifier->v4l2_dev) > return; > > - dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL); > - if (!dev) { > - dev_err(notifier->v4l2_dev->dev, > - "Failed to allocate device cache!\n"); > - } > - > mutex_lock(&list_lock); > > list_del(¬ifier->list); > > list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { > - struct device *d; > - > - d = get_device(sd->dev); > - > 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 > - */ > - if (dev) > - dev[i++] = d; > - else > - put_device(d); > + list_move(&sd->async_list, &subdev_list); > } > > 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]; > - > - if (d && device_attach(d) < 0) { > - const char *name = "(none)"; > - int lock = device_trylock(d); > - > - if (lock && d->driver) > - name = d->driver->name; > - dev_err(d, "Failed to re-probe to %s\n", name); > - if (lock) > - device_unlock(d); > - } > - put_device(d); > - } > - kvfree(dev); > - > notifier->v4l2_dev = NULL; > - > - /* > - * Don't care about the waiting list, it is initialised and populated > - * upon notifier registration. > - */ > } > EXPORT_SYMBOL(v4l2_async_notifier_unregister); > > -- > 2.11.0 >
Em Thu, 5 Oct 2017 00:50:20 +0300 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > Remove V4L2 async re-probing support. The re-probing support has been > there to support cases where the sub-devices require resources provided by > the main driver's hardware to function, such as clocks. > > Reprobing has allowed unbinding and again binding the main driver without > explicilty unbinding the sub-device drivers. This is certainly not a > common need, and the responsibility will be the user's going forward. > > An alternative could have been to introduce notifier specific locks. > Considering the complexity of the re-probing and that it isn't really a > solution to a problem but a workaround, remove re-probing instead. If the re-probing isn't using anywhere, that sounds a nice cleanup. Did you check if this won't break any driver (like soc_camera)? If not, then Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Acked-by: Hans Verkuil <hans.verkuil@cisco.com> > Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-async.c | 54 +----------------------------------- > 1 file changed, 1 insertion(+), 53 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c > index d741a8e0fdac..60a1a50b9537 100644 > --- a/drivers/media/v4l2-core/v4l2-async.c > +++ b/drivers/media/v4l2-core/v4l2-async.c > @@ -198,78 +198,26 @@ EXPORT_SYMBOL(v4l2_async_notifier_register); > void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) > { > struct v4l2_subdev *sd, *tmp; > - 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; > > if (!notifier->v4l2_dev) > return; > > - dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL); > - if (!dev) { > - dev_err(notifier->v4l2_dev->dev, > - "Failed to allocate device cache!\n"); > - } > - > mutex_lock(&list_lock); > > list_del(¬ifier->list); > > list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { > - struct device *d; > - > - d = get_device(sd->dev); > - > 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 > - */ > - if (dev) > - dev[i++] = d; > - else > - put_device(d); > + list_move(&sd->async_list, &subdev_list); > } > > 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]; > - > - if (d && device_attach(d) < 0) { > - const char *name = "(none)"; > - int lock = device_trylock(d); > - > - if (lock && d->driver) > - name = d->driver->name; > - dev_err(d, "Failed to re-probe to %s\n", name); > - if (lock) > - device_unlock(d); > - } > - put_device(d); > - } > - kvfree(dev); > - > notifier->v4l2_dev = NULL; > - > - /* > - * Don't care about the waiting list, it is initialised and populated > - * upon notifier registration. > - */ > } > EXPORT_SYMBOL(v4l2_async_notifier_unregister); > Thanks, Mauro
Hi Mauro, On Mon, Oct 09, 2017 at 08:22:39AM -0300, Mauro Carvalho Chehab wrote: > Em Thu, 5 Oct 2017 00:50:20 +0300 > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > Remove V4L2 async re-probing support. The re-probing support has been > > there to support cases where the sub-devices require resources provided by > > the main driver's hardware to function, such as clocks. > > > > Reprobing has allowed unbinding and again binding the main driver without > > explicilty unbinding the sub-device drivers. This is certainly not a > > common need, and the responsibility will be the user's going forward. > > > > An alternative could have been to introduce notifier specific locks. > > Considering the complexity of the re-probing and that it isn't really a > > solution to a problem but a workaround, remove re-probing instead. > > If the re-probing isn't using anywhere, that sounds a nice cleanup. > Did you check if this won't break any driver (like soc_camera)? That was discussed earlier in the review; Laurent asked the same question. Re-probing never was a proper solution to any problem; it was just a hack to avoid unbinding the sensor if the bridge driver was unbound, no more: it can't be generalised to support more complex use cases. Mind you, this is on devices that aren't actually removable. I've briefly discussed this with Laurent; the proper solution would need to be implemented in the clock framework instead. There, the existing clocks obtained by drivers could be re-activated when the driver for them comes back. My proposal is that if there's real a need to address this, then it could be solved in the clock framework.
On 09/10/17 16:06, Sakari Ailus wrote: > Hi Mauro, > > On Mon, Oct 09, 2017 at 08:22:39AM -0300, Mauro Carvalho Chehab wrote: >> Em Thu, 5 Oct 2017 00:50:20 +0300 >> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: >> >>> Remove V4L2 async re-probing support. The re-probing support has been >>> there to support cases where the sub-devices require resources provided by >>> the main driver's hardware to function, such as clocks. >>> >>> Reprobing has allowed unbinding and again binding the main driver without >>> explicilty unbinding the sub-device drivers. This is certainly not a >>> common need, and the responsibility will be the user's going forward. >>> >>> An alternative could have been to introduce notifier specific locks. >>> Considering the complexity of the re-probing and that it isn't really a >>> solution to a problem but a workaround, remove re-probing instead. >> >> If the re-probing isn't using anywhere, that sounds a nice cleanup. >> Did you check if this won't break any driver (like soc_camera)? > > That was discussed earlier in the review; Laurent asked the same question. > > Re-probing never was a proper solution to any problem; it was just a hack > to avoid unbinding the sensor if the bridge driver was unbound, no more: it > can't be generalised to support more complex use cases. Mind you, this is > on devices that aren't actually removable. > > I've briefly discussed this with Laurent; the proper solution would need to > be implemented in the clock framework instead. There, the existing clocks > obtained by drivers could be re-activated when the driver for them comes > back. > > My proposal is that if there's real a need to address this, then it could > be solved in the clock framework. Can you add this information to the commit log? I think that would be very helpful in the future. Regards, Hans
Hi Hans, On Mon, Oct 09, 2017 at 04:08:47PM +0200, Hans Verkuil wrote: > On 09/10/17 16:06, Sakari Ailus wrote: > > Hi Mauro, > > > > On Mon, Oct 09, 2017 at 08:22:39AM -0300, Mauro Carvalho Chehab wrote: > >> Em Thu, 5 Oct 2017 00:50:20 +0300 > >> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > >> > >>> Remove V4L2 async re-probing support. The re-probing support has been > >>> there to support cases where the sub-devices require resources provided by > >>> the main driver's hardware to function, such as clocks. > >>> > >>> Reprobing has allowed unbinding and again binding the main driver without > >>> explicilty unbinding the sub-device drivers. This is certainly not a > >>> common need, and the responsibility will be the user's going forward. > >>> > >>> An alternative could have been to introduce notifier specific locks. > >>> Considering the complexity of the re-probing and that it isn't really a > >>> solution to a problem but a workaround, remove re-probing instead. > >> > >> If the re-probing isn't using anywhere, that sounds a nice cleanup. > >> Did you check if this won't break any driver (like soc_camera)? > > > > That was discussed earlier in the review; Laurent asked the same question. > > > > Re-probing never was a proper solution to any problem; it was just a hack > > to avoid unbinding the sensor if the bridge driver was unbound, no more: it > > can't be generalised to support more complex use cases. Mind you, this is > > on devices that aren't actually removable. > > > > I've briefly discussed this with Laurent; the proper solution would need to > > be implemented in the clock framework instead. There, the existing clocks > > obtained by drivers could be re-activated when the driver for them comes > > back. > > > > My proposal is that if there's real a need to address this, then it could > > be solved in the clock framework. > > Can you add this information to the commit log? > > I think that would be very helpful in the future. Sure, how about this at the end of the current commit message: If there is a need to support removing the clock provider in the future, this should be implemented in the clock framework instead, not in V4L2.
On 09/10/17 16:18, Sakari Ailus wrote: > Hi Hans, > > On Mon, Oct 09, 2017 at 04:08:47PM +0200, Hans Verkuil wrote: >> On 09/10/17 16:06, Sakari Ailus wrote: >>> Hi Mauro, >>> >>> On Mon, Oct 09, 2017 at 08:22:39AM -0300, Mauro Carvalho Chehab wrote: >>>> Em Thu, 5 Oct 2017 00:50:20 +0300 >>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: >>>> >>>>> Remove V4L2 async re-probing support. The re-probing support has been >>>>> there to support cases where the sub-devices require resources provided by >>>>> the main driver's hardware to function, such as clocks. >>>>> >>>>> Reprobing has allowed unbinding and again binding the main driver without >>>>> explicilty unbinding the sub-device drivers. This is certainly not a >>>>> common need, and the responsibility will be the user's going forward. >>>>> >>>>> An alternative could have been to introduce notifier specific locks. >>>>> Considering the complexity of the re-probing and that it isn't really a >>>>> solution to a problem but a workaround, remove re-probing instead. >>>> >>>> If the re-probing isn't using anywhere, that sounds a nice cleanup. >>>> Did you check if this won't break any driver (like soc_camera)? >>> >>> That was discussed earlier in the review; Laurent asked the same question. >>> >>> Re-probing never was a proper solution to any problem; it was just a hack >>> to avoid unbinding the sensor if the bridge driver was unbound, no more: it >>> can't be generalised to support more complex use cases. Mind you, this is >>> on devices that aren't actually removable. >>> >>> I've briefly discussed this with Laurent; the proper solution would need to >>> be implemented in the clock framework instead. There, the existing clocks >>> obtained by drivers could be re-activated when the driver for them comes >>> back. >>> >>> My proposal is that if there's real a need to address this, then it could >>> be solved in the clock framework. >> >> Can you add this information to the commit log? >> >> I think that would be very helpful in the future. > > Sure, how about this at the end of the current commit message: > > If there is a need to support removing the clock provider in the future, > this should be implemented in the clock framework instead, not in V4L2. Yes, that sounds good. Regards, Hans
Em Mon, 9 Oct 2017 16:20:08 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 09/10/17 16:18, Sakari Ailus wrote: > > Hi Hans, > > > > On Mon, Oct 09, 2017 at 04:08:47PM +0200, Hans Verkuil wrote: > >> On 09/10/17 16:06, Sakari Ailus wrote: > >>> Hi Mauro, > >>> > >>> On Mon, Oct 09, 2017 at 08:22:39AM -0300, Mauro Carvalho Chehab wrote: > >>>> Em Thu, 5 Oct 2017 00:50:20 +0300 > >>>> Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > >>>> > >>>>> Remove V4L2 async re-probing support. The re-probing support has been > >>>>> there to support cases where the sub-devices require resources provided by > >>>>> the main driver's hardware to function, such as clocks. > >>>>> > >>>>> Reprobing has allowed unbinding and again binding the main driver without > >>>>> explicilty unbinding the sub-device drivers. This is certainly not a > >>>>> common need, and the responsibility will be the user's going forward. > >>>>> > >>>>> An alternative could have been to introduce notifier specific locks. > >>>>> Considering the complexity of the re-probing and that it isn't really a > >>>>> solution to a problem but a workaround, remove re-probing instead. > >>>> > >>>> If the re-probing isn't using anywhere, that sounds a nice cleanup. > >>>> Did you check if this won't break any driver (like soc_camera)? > >>> > >>> That was discussed earlier in the review; Laurent asked the same question. > >>> > >>> Re-probing never was a proper solution to any problem; it was just a hack > >>> to avoid unbinding the sensor if the bridge driver was unbound, no more: it > >>> can't be generalised to support more complex use cases. Mind you, this is > >>> on devices that aren't actually removable. > >>> > >>> I've briefly discussed this with Laurent; the proper solution would need to > >>> be implemented in the clock framework instead. There, the existing clocks > >>> obtained by drivers could be re-activated when the driver for them comes > >>> back. > >>> > >>> My proposal is that if there's real a need to address this, then it could > >>> be solved in the clock framework. > >> > >> Can you add this information to the commit log? > >> > >> I think that would be very helpful in the future. > > > > Sure, how about this at the end of the current commit message: > > > > If there is a need to support removing the clock provider in the future, > > this should be implemented in the clock framework instead, not in V4L2. > > Yes, that sounds good. Works for me too. Regards, Mauro
Hi Sakari, On 10/09/2017 04:18 PM, Sakari Ailus wrote: > Sure, how about this at the end of the current commit message: > > If there is a need to support removing the clock provider in the future, > this should be implemented in the clock framework instead, not in V4L2. I find it a little bit misleading, there is already support for removing the clock provider, only any clock references for consumers became then stale. Perhaps: "If there is a need to support the clock provider unregister/register cycle while keeping the clock references in the consumers in the future, this should be implemented in the clock framework instead, not in V4L2." ? That said, I doubt this issue is going to be entirely solved solely in the clock framework, as it is a more general problem of resource dependencies. It could be related to other resources, like regulator or GPIO. It has been discussed for a long time now and it will likely take time until a general solution is available. -- Thanks, Sylwester
Hi Sylwester, On Monday, 9 October 2017 19:44:52 EEST Sylwester Nawrocki wrote: > On 10/09/2017 04:18 PM, Sakari Ailus wrote: > > Sure, how about this at the end of the current commit message: > > > > If there is a need to support removing the clock provider in the future, > > this should be implemented in the clock framework instead, not in V4L2. > > I find it a little bit misleading, there is already support for removing > the clock provider, only any clock references for consumers became then > stale. Perhaps: > > "If there is a need to support the clock provider unregister/register > cycle while keeping the clock references in the consumers in the future, > this should be implemented in the clock framework instead, not in V4L2." > > ? That said, I doubt this issue is going to be entirely solved solely > in the clock framework, as it is a more general problem of resource > dependencies. It could be related to other resources, like regulator > or GPIO. It has been discussed for a long time now and it will likely > take time until a general solution is available. I discussed this issue with Mike Turquette during LPC, and he believed we could fix it in the clock framework by adding support for "un-orphaning" a clock. This remains to be proven by a real implementation, but could work for our use case. I agree with you that similar problems exist for other resources such as regulators and GPIOs, but to my knowledge we have no system that requires V4L2 reprobing due to regulator or GPIO dependencies.
Hi Sylwester, On Mon, Oct 09, 2017 at 06:44:52PM +0200, Sylwester Nawrocki wrote: > Hi Sakari, > > On 10/09/2017 04:18 PM, Sakari Ailus wrote: > > Sure, how about this at the end of the current commit message: > > > > If there is a need to support removing the clock provider in the future, > > this should be implemented in the clock framework instead, not in V4L2. > > I find it a little bit misleading, there is already support for removing > the clock provider, only any clock references for consumers became then > stale. Perhaps: > > "If there is a need to support the clock provider unregister/register > cycle while keeping the clock references in the consumers in the future, > this should be implemented in the clock framework instead, not in V4L2." Yes, I'll use this in v16. > > ? That said, I doubt this issue is going to be entirely solved solely > in the clock framework, as it is a more general problem of resource > dependencies. It could be related to other resources, like regulator > or GPIO. It has been discussed for a long time now and it will likely > take time until a general solution is available. I don't think we can have entirely generic solutions to this as the API through which the resources are accessed is specific to the resources (regulator, GPIO, clock). Or the generic solution would be used by the frameworks behind those APIs. But as Laurent mentioned, we don't have this case with other than clock currently.
diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c index d741a8e0fdac..60a1a50b9537 100644 --- a/drivers/media/v4l2-core/v4l2-async.c +++ b/drivers/media/v4l2-core/v4l2-async.c @@ -198,78 +198,26 @@ EXPORT_SYMBOL(v4l2_async_notifier_register); void v4l2_async_notifier_unregister(struct v4l2_async_notifier *notifier) { struct v4l2_subdev *sd, *tmp; - 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; if (!notifier->v4l2_dev) return; - dev = kvmalloc_array(n_subdev, sizeof(*dev), GFP_KERNEL); - if (!dev) { - dev_err(notifier->v4l2_dev->dev, - "Failed to allocate device cache!\n"); - } - mutex_lock(&list_lock); list_del(¬ifier->list); list_for_each_entry_safe(sd, tmp, ¬ifier->done, async_list) { - struct device *d; - - d = get_device(sd->dev); - 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 - */ - if (dev) - dev[i++] = d; - else - put_device(d); + list_move(&sd->async_list, &subdev_list); } 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]; - - if (d && device_attach(d) < 0) { - const char *name = "(none)"; - int lock = device_trylock(d); - - if (lock && d->driver) - name = d->driver->name; - dev_err(d, "Failed to re-probe to %s\n", name); - if (lock) - device_unlock(d); - } - put_device(d); - } - kvfree(dev); - notifier->v4l2_dev = NULL; - - /* - * Don't care about the waiting list, it is initialised and populated - * upon notifier registration. - */ } EXPORT_SYMBOL(v4l2_async_notifier_unregister);