Re: [RFC 00/19] Async sub-notifiers and how to use them

On 08/04/17 20:25, Sakari Ailus wrote:

Niklas Söderlund wrote:

quoted

Hi Sakari,

On 2017-07-20 19:14:01 +0300, Sakari Ailus wrote:

quoted

Hi Hans,

Thanks for the review.

On Wed, Jul 19, 2017 at 01:42:55PM +0200, Hans Verkuil wrote:

quoted

On 18/07/17 21:03, Sakari Ailus wrote:

quoted

Hi folks,

This RFC patchset achieves a number of things which I've put to the same
patchset for they need to be show together to demonstrate the use cases.

I don't really intend this to compete with Niklas's patchset but much of
the problem area addressed by the two is the same.

Comments would be welcome.

- Add AS3645A LED flash class driver.

- Add async notifiers (by Niklas).

- V4L2 sub-device node registration is moved to take place at the same time
  with the registration of the sub-device itself. With this change,
  sub-device node registration behaviour is aligned with video node
  registration.

- The former is made possible by moving the bound() callback after
  sub-device registration.

- As all the device node registration and link creation is done as the
  respective devices are probed, there is no longer dependency to the
  notifier complete callback which as itself is seen problematic. The
  complete callback still exists but there's no need to use it, pending
  changes in individual drivers.

  See:
  <URL:http://www.spinics.net/lists/linux-media/msg118323.html>

  As a result, if a part of the media device fails to initialise because it
  is e.g. physically broken, it will be possible to use what works.

I've got major problems with this from a userspace point of view. In the vast
majority of cases you just want to bail out if one or more subdevs fail.

I admit it's easier for the user space if the device becomes available only
when all its component drivers have registered.

Also remember that video nodes are registered in the file system right on
device probe time. It's only sub-device and media device node registration
that has taken place in the notifier's complete handler.

Is this always the case? In the R-Car VIN driver I register the video 
devices using video_register_device() in the complete handler. Am I 
doing things wrong in that driver? I had a patch where I moved the 
video_register_device() call to probe time but it got shoot down in 
review and was dropped.

I don't think the current implementation is wrong, it's just different
from other drivers; there's really no requirement regarding this AFAIU.
It's one of the things where no attention has been paid I presume.

It actually is a requirement: when a device node appears applications can
reasonably expect to have a fully functioning device. True for any device
node. You don't want to have to wait until some unspecified time before
the full functionality is there.

I try to pay attention to this when reviewing code, since not following this
rule basically introduces a race condition which is hard to test.

However doing anything that can fail earlier on would be nicer since
there's no reasonable way to signal an error from complete callback either.

Right.

Adding support for cases where devices may not be present is very desirable,
but this should go through an RFC process first to hammer out all the details.

Today we do not support this and we have to review code with that in mind.

So the first async subnotifiers implementation should NOT support this (although
it can of course be designed with this in mind). Once it is in we can start
on an RFC on how to support partial pipelines. I have a lot of questions about
that that need to be answered first.

One thing at a time. Trying to do everything at once never works.

Regards,

	Hans