Re: [PATCH 5/5] media: v4l2-async: Create links during v4l2_async_match_notify()

[Sakari Ailus <sakari.ailus@linux.intel.com>]

On Wed, Dec 15, 2021 at 11:04:44AM +0200, Laurent Pinchart wrote:
> Hi Dan,
> 
> On Tue, Dec 14, 2021 at 11:41:27PM +0000, Daniel Scally wrote:
> > On 14/12/2021 23:01, Laurent Pinchart wrote:
> > > On Tue, Dec 14, 2021 at 10:36:01PM +0000, Daniel Scally wrote:
> > >> On 14/12/2021 22:22, Laurent Pinchart wrote:
> > >>> On Mon, Dec 13, 2021 at 11:28:49PM +0000, Daniel Scally wrote:
> > >>>> Upon an async fwnode match, there's some typical behaviour that the
> > >>>> notifier and matching subdev will want to do. For example, a notifier
> > >>>> representing a sensor matching to an async subdev representing its
> > >>>> VCM will want to create an ancillary link to expose that relationship
> > >>>> to userspace.
> > >>>>
> > >>>> To avoid lots of code in individual drivers, try to build these links
> > >>>> within v4l2 core.
> > >>>>
> > >>>> Signed-off-by: Daniel Scally <djrscally@gmail.com>
> > >>>> ---
> > >>>> Changes since the rfc:
> > >>>>
> > >>>> 	- None
> > >>>>
> > >>>>  drivers/media/v4l2-core/v4l2-async.c | 51 ++++++++++++++++++++++++++++
> > >>>>  1 file changed, 51 insertions(+)
> > >>>>
> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-async.c b/drivers/media/v4l2-core/v4l2-async.c
> > >>>> index 0404267f1ae4..6575b1cbe95f 100644
> > >>>> --- a/drivers/media/v4l2-core/v4l2-async.c
> > >>>> +++ b/drivers/media/v4l2-core/v4l2-async.c
> > >>>> @@ -275,6 +275,45 @@ v4l2_async_nf_try_complete(struct v4l2_async_notifier *notifier)
> > >>>>  static int
> > >>>>  v4l2_async_nf_try_all_subdevs(struct v4l2_async_notifier *notifier);
> > >>>>  
> > >>>> +static int
> > >>>> +__v4l2_async_create_ancillary_link(struct v4l2_async_notifier *notifier,
> > >>>> +				   struct v4l2_subdev *sd)
> > >>>> +{
> > >>>> +	struct media_link *link;
> > >>>> +
> > >>>> +	if (sd->entity.function != MEDIA_ENT_F_LENS &&
> > >>>> +	    sd->entity.function != MEDIA_ENT_F_FLASH)
> > >>>> +		return -EINVAL;
> > >>>> +
> > >>>> +	link = media_create_ancillary_link(&notifier->sd->entity, &sd->entity,
> > >>>
> > >>> Is there a guarantee at this point that notifier->sd->entity has already
> > >>> been registered with the media_device ? That's done by
> > >>> media_device_register_entity() called from
> > >>> v4l2_device_register_subdev().
> > >>
> > >> v4l2_async_match_notify() calls v4l2_device_register_subdev() before the
> > >> point that I've added the call to v4l2_async_try_create_links(), so I
> > >> think that's covered there.
> > >
> > > It calls it on sd, not notifier->sd. It's the latter that concerns me.
> > 
> > Ah, you're right of course...I guess in that case the notifier->sd would
> > get registered during the v4l2_async_match_notify() where the sensor
> > driver's subdev is sd, but I don't think there's any guarantee that that
> > would happen first...I haven't traced it through but my guess is that it
> > would depend on the order in which the ipu3-cio2, sensor and lens
> > controller drivers probed. I'll check to try and be sure how it works
> > tomorrow
> 
> I was looking at media_device_register_entity(), and it sets
> 
> 	INIT_LIST_HEAD(&entity->links);
> 	entity->num_links = 0;
> 	entity->num_backlinks = 0;
> 
> If we create links before that, things may go bad.

Yes.

There's a guarantee that the notifier's complete callback is called once
the notifier's subdevs as well as sub-notifiers are bound and complete. But
there's no guarantee on the initialisation of related entities.

Especially for sensors, the async subdev is registered after the sensor's
own async notifier.

I wonder if the ugly registered callback could be used for this purpose.
Better of course would be to avoid that.

-- 
Sakari Ailus