[v8,18/55,media] omap3isp: create links after all subdevs have been bound

Commit Message

Mauro Carvalho Chehab Aug. 30, 2015, 3:06 a.m. UTC

From: Javier Martinez Canillas <[e-mail]>

The omap3isp driver parses the graph endpoints to know how many subdevices
needs to be registered async and register notifiers callbacks for to know
when these are bound and when the async registrations are completed.

Currently the entities pad are linked with the correct ISP input interface
when the subdevs are bound but it happens before entitities are registered
with the media device so that won't work now that the entity links list is
initialized on device registration.

So instead creating the pad links when the subdevice is bound, create them
on the complete callback once all the subdevices have been bound but only
try to create for the ones that have a bus configuration set during bound.

Signed-off-by: Javier Martinez Canillas <[e-mail]>
Signed-off-by: Mauro Carvalho Chehab <[e-mail]>

Comments

Sakari Ailus Sept. 9, 2015, 8:03 a.m. UTC | #1

Hi Javier and Mauro,

On Sun, Aug 30, 2015 at 12:06:29AM -0300, Mauro Carvalho Chehab wrote:
> From: Javier Martinez Canillas <[e-mail]>
> 
> The omap3isp driver parses the graph endpoints to know how many subdevices
> needs to be registered async and register notifiers callbacks for to know
> when these are bound and when the async registrations are completed.
> 
> Currently the entities pad are linked with the correct ISP input interface
> when the subdevs are bound but it happens before entitities are registered
> with the media device so that won't work now that the entity links list is
> initialized on device registration.
> 
> So instead creating the pad links when the subdevice is bound, create them
> on the complete callback once all the subdevices have been bound but only
> try to create for the ones that have a bus configuration set during bound.
> 
> Signed-off-by: Javier Martinez Canillas <[e-mail]>
> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> 
> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> index b8f6f81d2db2..69e7733d36cd 100644
> --- a/drivers/media/platform/omap3isp/isp.c
> +++ b/drivers/media/platform/omap3isp/isp.c
> @@ -2321,26 +2321,33 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
>  				     struct v4l2_subdev *subdev,
>  				     struct v4l2_async_subdev *asd)
>  {
> -	struct isp_device *isp = container_of(async, struct isp_device,
> -					      notifier);
>  	struct isp_async_subdev *isd =
>  		container_of(asd, struct isp_async_subdev, asd);
> -	int ret;
> -
> -	ret = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
> -	if (ret < 0)
> -		return ret;
>  
>  	isd->sd = subdev;
>  	isd->sd->host_priv = &isd->bus;
>  
> -	return ret;
> +	return 0;
>  }
>  
>  static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>  {
>  	struct isp_device *isp = container_of(async, struct isp_device,
>  					      notifier);
> +	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
> +	struct v4l2_subdev *sd;
> +	struct isp_bus_cfg *bus;
> +	int ret;
> +
> +	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> +		/* Only try to link entities whose interface was set on bound */
> +		if (sd->host_priv) {
> +			bus = (struct isp_bus_cfg *)sd->host_priv;
> +			ret = isp_link_entity(isp, &sd->entity, bus->interface);
> +			if (ret < 0)
> +				return ret;
> +		}
> +	}
>  
>  	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
>  }

I think you're working around a problem here, not really fixing it.

This change will create the links only after the media device is registered,
which means the user may obtain a partial enumeration of links if the
enumeration is performed too early.

Before this set, the problem also was that the media device was registered
before the async entities were bound, again making it possible to obtain a
partial enumeration of entities.

What I'd suggest instead is that we split media device initialisation and
registration to the system; that way the media device can be prepared
(entities registered and links created) before it becomes visible to the
user space. I can write a patch for that if you like.

Javier Martinez Canillas Sept. 9, 2015, 8:48 a.m. UTC | #2

Hello Sakari,

On 09/09/2015 10:03 AM, Sakari Ailus wrote:
> Hi Javier and Mauro,
> 
> On Sun, Aug 30, 2015 at 12:06:29AM -0300, Mauro Carvalho Chehab wrote:
>> From: Javier Martinez Canillas <[e-mail]>
>>
>> The omap3isp driver parses the graph endpoints to know how many subdevices
>> needs to be registered async and register notifiers callbacks for to know
>> when these are bound and when the async registrations are completed.
>>
>> Currently the entities pad are linked with the correct ISP input interface
>> when the subdevs are bound but it happens before entitities are registered
>> with the media device so that won't work now that the entity links list is
>> initialized on device registration.
>>
>> So instead creating the pad links when the subdevice is bound, create them
>> on the complete callback once all the subdevices have been bound but only
>> try to create for the ones that have a bus configuration set during bound.
>>
>> Signed-off-by: Javier Martinez Canillas <[e-mail]>
>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
>>
>> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
>> index b8f6f81d2db2..69e7733d36cd 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -2321,26 +2321,33 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
>>  				     struct v4l2_subdev *subdev,
>>  				     struct v4l2_async_subdev *asd)
>>  {
>> -	struct isp_device *isp = container_of(async, struct isp_device,
>> -					      notifier);
>>  	struct isp_async_subdev *isd =
>>  		container_of(asd, struct isp_async_subdev, asd);
>> -	int ret;
>> -
>> -	ret = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
>> -	if (ret < 0)
>> -		return ret;
>>  
>>  	isd->sd = subdev;
>>  	isd->sd->host_priv = &isd->bus;
>>  
>> -	return ret;
>> +	return 0;
>>  }
>>  
>>  static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>>  {
>>  	struct isp_device *isp = container_of(async, struct isp_device,
>>  					      notifier);
>> +	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
>> +	struct v4l2_subdev *sd;
>> +	struct isp_bus_cfg *bus;
>> +	int ret;
>> +
>> +	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
>> +		/* Only try to link entities whose interface was set on bound */
>> +		if (sd->host_priv) {
>> +			bus = (struct isp_bus_cfg *)sd->host_priv;
>> +			ret = isp_link_entity(isp, &sd->entity, bus->interface);
>> +			if (ret < 0)
>> +				return ret;
>> +		}
>> +	}
>>  
>>  	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
>>  }
> 
> I think you're working around a problem here, not really fixing it.
> 
> This change will create the links only after the media device is registered,
> which means the user may obtain a partial enumeration of links if the
> enumeration is performed too early.
> 
> Before this set, the problem also was that the media device was registered
> before the async entities were bound, again making it possible to obtain a
> partial enumeration of entities.
>

You are absolutely correct but I think these are separate issues. The problem
here is that v4l2_async_test_notify() [0] first invokes the bound notifier
callback and then calls v4l2_device_register_subdev() that register the media
entity with the media device.

Since now is a requirement that the entities must be registered prior creating
pads links (because to init a MEDIA_GRAPH_LINK object a mdev has to be set),
$SUBJECT is needed regardless of the race between subdev registration and the
media dev node being available to user-space before everything is registered.
 
> What I'd suggest instead is that we split media device initialisation and
> registration to the system; that way the media device can be prepared
> (entities registered and links created) before it becomes visible to the
> user space. I can write a patch for that if you like.
>

Agreed, looking at the implementation it seems that __media_device_register()
has to be split (possibly being renamed to __media_device_init) so it only
contains the initialization logic and all the media device node registration
logic moved to another function (that would become media_device_register).

I think the media dev node registration has to be made in the complete callback
to make sure that happens when all the subdevs have been already registered.

Is that what you had in mind? I can also write such a patch if you want.

[0]: http://lxr.free-electrons.com/source/drivers/media/v4l2-core/v4l2-async.c#L96
[1]: http://lxr.free-electrons.com/source/drivers/media/media-device.c#L372

Best regards,

Mauro Carvalho Chehab Sept. 9, 2015, 10:28 a.m. UTC | #3

Hi Sakari,

Em Wed, 09 Sep 2015 11:03:33 +0300
Sakari Ailus <[e-mail]> escreveu:

> Hi Javier and Mauro,
> 
> On Sun, Aug 30, 2015 at 12:06:29AM -0300, Mauro Carvalho Chehab wrote:
> > From: Javier Martinez Canillas <[e-mail]>
> > 
> > The omap3isp driver parses the graph endpoints to know how many subdevices
> > needs to be registered async and register notifiers callbacks for to know
> > when these are bound and when the async registrations are completed.
> > 
> > Currently the entities pad are linked with the correct ISP input interface
> > when the subdevs are bound but it happens before entitities are registered
> > with the media device so that won't work now that the entity links list is
> > initialized on device registration.
> > 
> > So instead creating the pad links when the subdevice is bound, create them
> > on the complete callback once all the subdevices have been bound but only
> > try to create for the ones that have a bus configuration set during bound.
> > 
> > Signed-off-by: Javier Martinez Canillas <[e-mail]>
> > Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> > 
> > diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
> > index b8f6f81d2db2..69e7733d36cd 100644
> > --- a/drivers/media/platform/omap3isp/isp.c
> > +++ b/drivers/media/platform/omap3isp/isp.c
> > @@ -2321,26 +2321,33 @@ static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
> >  				     struct v4l2_subdev *subdev,
> >  				     struct v4l2_async_subdev *asd)
> >  {
> > -	struct isp_device *isp = container_of(async, struct isp_device,
> > -					      notifier);
> >  	struct isp_async_subdev *isd =
> >  		container_of(asd, struct isp_async_subdev, asd);
> > -	int ret;
> > -
> > -	ret = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
> > -	if (ret < 0)
> > -		return ret;
> >  
> >  	isd->sd = subdev;
> >  	isd->sd->host_priv = &isd->bus;
> >  
> > -	return ret;
> > +	return 0;
> >  }
> >  
> >  static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
> >  {
> >  	struct isp_device *isp = container_of(async, struct isp_device,
> >  					      notifier);
> > +	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
> > +	struct v4l2_subdev *sd;
> > +	struct isp_bus_cfg *bus;
> > +	int ret;
> > +
> > +	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> > +		/* Only try to link entities whose interface was set on bound */
> > +		if (sd->host_priv) {
> > +			bus = (struct isp_bus_cfg *)sd->host_priv;
> > +			ret = isp_link_entity(isp, &sd->entity, bus->interface);
> > +			if (ret < 0)
> > +				return ret;
> > +		}
> > +	}
> >  
> >  	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> >  }
> 
> I think you're working around a problem here, not really fixing it.
> 
> This change will create the links only after the media device is registered,
> which means the user may obtain a partial enumeration of links if the
> enumeration is performed too early.
> 
> Before this set, the problem also was that the media device was registered
> before the async entities were bound, again making it possible to obtain a
> partial enumeration of entities.

Before of after this series, if userspace tries to read the topology too
early, it will get a partial topology. Before this series, it may lose
entities and links; after this series, it may lose any object.

In any case, userspace won't do the right thing, whatever order we
initialize.

The patches on this series is not meant to solve this issue.

The rationale for this patch is due to a different reason. You should 
notice that, due to the G_TOPOLOGY ioctl requirements, all objects
should have their ID assigned and should be added at the mdev linked
lists, as those are used by G_TOPOLOGY loops that copy the object
data to userspace.

So, before registering/creating any object, the media_device struct
need to exist internally at Kernelspace.

> 
> What I'd suggest instead is that we split media device initialisation and
> registration to the system; that way the media device can be prepared
> (entities registered and links created) before it becomes visible to the
> user space. I can write a patch for that if you like.

Yes, we can split the internal media register stuff from the
creation of the /dev/media0 device node, doing that only after
having everything set in Kernel, as the patch that Javier proposed
on IRC.

Another alternative would be to add a "busy" flag at media_device,
making all ioctl's at /dev/mdeia0 to return -EBUSY during massive
graph changes.

Such flag would be rised at media_device registration and dropped
at the end of the device probe routine.

The advantage of using such flags is that this could be used,
for example, on ARA project, when a module is removed or inserted.

The disadvantage is that current userspace apps may not be prepared
to receive -EBUSY.

I'm OK with both strategies.

Regards,
Mauro
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Laurent Pinchart Dec. 6, 2015, 3:05 a.m. UTC | #4

Hi Javier,

Thank you for the patch.

On Wednesday 09 September 2015 10:48:29 Javier Martinez Canillas wrote:
> On 09/09/2015 10:03 AM, Sakari Ailus wrote:
> > On Sun, Aug 30, 2015 at 12:06:29AM -0300, Mauro Carvalho Chehab wrote:
> >> From: Javier Martinez Canillas <[e-mail]>
> >> 
> >> The omap3isp driver parses the graph endpoints to know how many
> >> subdevices needs to be registered async and register notifiers callbacks
> >> for to know when these are bound and when the async registrations are
> >> completed.
> >> 
> >> Currently the entities pad are linked with the correct ISP input
> >> interface when the subdevs are bound but it happens before entitities are
> >> registered with the media device so that won't work now that the entity
> >> links list is initialized on device registration.
> >> 
> >> So instead creating the pad links when the subdevice is bound, create
> >> them on the complete callback once all the subdevices have been bound but
> >> only try to create for the ones that have a bus configuration set during
> >> bound.
> >> 
> >> Signed-off-by: Javier Martinez Canillas <[e-mail]>
> >> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
> >> 
> >> diff --git a/drivers/media/platform/omap3isp/isp.c
> >> b/drivers/media/platform/omap3isp/isp.c index b8f6f81d2db2..69e7733d36cd
> >> 100644
> >> --- a/drivers/media/platform/omap3isp/isp.c
> >> +++ b/drivers/media/platform/omap3isp/isp.c
> >> @@ -2321,26 +2321,33 @@ static int isp_subdev_notifier_bound(struct
> >> v4l2_async_notifier *async,
> >>  				     struct v4l2_subdev *subdev,
> >>  				     struct v4l2_async_subdev *asd)
> >>  {
> >> -	struct isp_device *isp = container_of(async, struct isp_device,
> >> -					      notifier);
> >>  	struct isp_async_subdev *isd =
> >>  		container_of(asd, struct isp_async_subdev, asd);
> >> -	int ret;
> >> -
> >> -	ret = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
> >> -	if (ret < 0)
> >> -		return ret;
> >> 
> >>  	isd->sd = subdev;
> >>  	isd->sd->host_priv = &isd->bus;
> >> 
> >> -	return ret;
> >> +	return 0;
> >>  }
> >>  
> >>  static int isp_subdev_notifier_complete(struct v4l2_async_notifier
> >>  *async)
> >>  {
> >>  	struct isp_device *isp = container_of(async, struct isp_device,
> >>  					      notifier);
> >> +	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
> >> +	struct v4l2_subdev *sd;
> >> +	struct isp_bus_cfg *bus;
> >> +	int ret;
> >> +
> >> +	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
> >> +		/* Only try to link entities whose interface was set on bound */
> >> +		if (sd->host_priv) {
> >> +			bus = (struct isp_bus_cfg *)sd->host_priv;
> >> +			ret = isp_link_entity(isp, &sd->entity, bus->interface);
> >> +			if (ret < 0)
> >> +				return ret;
> >> +		}
> >> +	}
> >>  	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
> >>  }
> > 
> > I think you're working around a problem here, not really fixing it.
> > 
> > This change will create the links only after the media device is
> > registered, which means the user may obtain a partial enumeration of
> > links if the enumeration is performed too early.
> > 
> > Before this set, the problem also was that the media device was registered
> > before the async entities were bound, again making it possible to obtain a
> > partial enumeration of entities.
> 
> You are absolutely correct but I think these are separate issues. The
> problem here is that v4l2_async_test_notify() [0] first invokes the bound
> notifier callback and then calls v4l2_device_register_subdev() that
> register the media entity with the media device.
> 
> Since now is a requirement that the entities must be registered prior
> creating pads links (because to init a MEDIA_GRAPH_LINK object a mdev has
> to be set), $SUBJECT is needed regardless of the race between subdev
> registration and the media dev node being available to user-space before
> everything is registered.
> > What I'd suggest instead is that we split media device initialisation and
> > registration to the system; that way the media device can be prepared
> > (entities registered and links created) before it becomes visible to the
> > user space. I can write a patch for that if you like.
> 
> Agreed, looking at the implementation it seems that
> __media_device_register() has to be split (possibly being renamed to
> __media_device_init) so it only contains the initialization logic and all
> the media device node registration logic moved to another function (that
> would become media_device_register).
> 
> I think the media dev node registration has to be made in the complete
> callback to make sure that happens when all the subdevs have been already
> registered.
> 
> Is that what you had in mind? I can also write such a patch if you want.

I think I've already commented on it in my review of another patch (but can't 
find it right now), I agree with you. We need to properly think about 
initialization (and, for that matter, cleanup as well) order, both for the 
media device and the entities. And, as a corollary, for subdevs too. The 
current media entity and subdevs initialization and registration code grew in 
an organic way without much design behind it, let's not repeat the same 
mistake.

> [0]:
> http://lxr.free-electrons.com/source/drivers/media/v4l2-core/v4l2-async.c#L
> 96 [1]:
> http://lxr.free-electrons.com/source/drivers/media/media-device.c#L372

Javier Martinez Canillas Dec. 7, 2015, 3:17 p.m. UTC | #5

Hello Laurent,

On 12/06/2015 12:05 AM, Laurent Pinchart wrote:
> Hi Javier,
> 
> Thank you for the patch.
> 
> On Wednesday 09 September 2015 10:48:29 Javier Martinez Canillas wrote:
>> On 09/09/2015 10:03 AM, Sakari Ailus wrote:
>>> On Sun, Aug 30, 2015 at 12:06:29AM -0300, Mauro Carvalho Chehab wrote:
>>>> From: Javier Martinez Canillas <[e-mail]>
>>>>
>>>> The omap3isp driver parses the graph endpoints to know how many
>>>> subdevices needs to be registered async and register notifiers callbacks
>>>> for to know when these are bound and when the async registrations are
>>>> completed.
>>>>
>>>> Currently the entities pad are linked with the correct ISP input
>>>> interface when the subdevs are bound but it happens before entitities are
>>>> registered with the media device so that won't work now that the entity
>>>> links list is initialized on device registration.
>>>>
>>>> So instead creating the pad links when the subdevice is bound, create
>>>> them on the complete callback once all the subdevices have been bound but
>>>> only try to create for the ones that have a bus configuration set during
>>>> bound.
>>>>
>>>> Signed-off-by: Javier Martinez Canillas <[e-mail]>
>>>> Signed-off-by: Mauro Carvalho Chehab <[e-mail]>
>>>>
>>>> diff --git a/drivers/media/platform/omap3isp/isp.c
>>>> b/drivers/media/platform/omap3isp/isp.c index b8f6f81d2db2..69e7733d36cd
>>>> 100644
>>>> --- a/drivers/media/platform/omap3isp/isp.c
>>>> +++ b/drivers/media/platform/omap3isp/isp.c
>>>> @@ -2321,26 +2321,33 @@ static int isp_subdev_notifier_bound(struct
>>>> v4l2_async_notifier *async,
>>>>  				     struct v4l2_subdev *subdev,
>>>>  				     struct v4l2_async_subdev *asd)
>>>>  {
>>>> -	struct isp_device *isp = container_of(async, struct isp_device,
>>>> -					      notifier);
>>>>  	struct isp_async_subdev *isd =
>>>>  		container_of(asd, struct isp_async_subdev, asd);
>>>> -	int ret;
>>>> -
>>>> -	ret = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
>>>> -	if (ret < 0)
>>>> -		return ret;
>>>>
>>>>  	isd->sd = subdev;
>>>>  	isd->sd->host_priv = &isd->bus;
>>>>
>>>> -	return ret;
>>>> +	return 0;
>>>>  }
>>>>  
>>>>  static int isp_subdev_notifier_complete(struct v4l2_async_notifier
>>>>  *async)
>>>>  {
>>>>  	struct isp_device *isp = container_of(async, struct isp_device,
>>>>  					      notifier);
>>>> +	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
>>>> +	struct v4l2_subdev *sd;
>>>> +	struct isp_bus_cfg *bus;
>>>> +	int ret;
>>>> +
>>>> +	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
>>>> +		/* Only try to link entities whose interface was set on bound */
>>>> +		if (sd->host_priv) {
>>>> +			bus = (struct isp_bus_cfg *)sd->host_priv;
>>>> +			ret = isp_link_entity(isp, &sd->entity, bus->interface);
>>>> +			if (ret < 0)
>>>> +				return ret;
>>>> +		}
>>>> +	}
>>>>  	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
>>>>  }
>>>
>>> I think you're working around a problem here, not really fixing it.
>>>
>>> This change will create the links only after the media device is
>>> registered, which means the user may obtain a partial enumeration of
>>> links if the enumeration is performed too early.
>>>
>>> Before this set, the problem also was that the media device was registered
>>> before the async entities were bound, again making it possible to obtain a
>>> partial enumeration of entities.
>>
>> You are absolutely correct but I think these are separate issues. The
>> problem here is that v4l2_async_test_notify() [0] first invokes the bound
>> notifier callback and then calls v4l2_device_register_subdev() that
>> register the media entity with the media device.
>>
>> Since now is a requirement that the entities must be registered prior
>> creating pads links (because to init a MEDIA_GRAPH_LINK object a mdev has
>> to be set), $SUBJECT is needed regardless of the race between subdev
>> registration and the media dev node being available to user-space before
>> everything is registered.
>>> What I'd suggest instead is that we split media device initialisation and
>>> registration to the system; that way the media device can be prepared
>>> (entities registered and links created) before it becomes visible to the
>>> user space. I can write a patch for that if you like.
>>
>> Agreed, looking at the implementation it seems that
>> __media_device_register() has to be split (possibly being renamed to
>> __media_device_init) so it only contains the initialization logic and all
>> the media device node registration logic moved to another function (that
>> would become media_device_register).
>>
>> I think the media dev node registration has to be made in the complete
>> callback to make sure that happens when all the subdevs have been already
>> registered.
>>
>> Is that what you had in mind? I can also write such a patch if you want.
> 
> I think I've already commented on it in my review of another patch (but can't 
> find it right now), I agree with you. We need to properly think about 

I'm glad that you agree, could you please review the v4 of patch series
"Fix race between graph enumeration and entities registration" [0] that
I posted a while ago?

> initialization (and, for that matter, cleanup as well) order, both for the 
> media device and the entities. And, as a corollary, for subdevs too. The
> current media entity and subdevs initialization and registration code grew in 
> an organic way without much design behind it, let's not repeat the same 
> mistake.
> 

Agreed, and this should be properly documented so driver authors can follow.

[0]: https://lkml.org/lkml/2015/9/15/371

Best regards,

Patch

diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
index b8f6f81d2db2..69e7733d36cd 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -2321,26 +2321,33 @@  static int isp_subdev_notifier_bound(struct v4l2_async_notifier *async,
 				     struct v4l2_subdev *subdev,
 				     struct v4l2_async_subdev *asd)
 {
-	struct isp_device *isp = container_of(async, struct isp_device,
-					      notifier);
 	struct isp_async_subdev *isd =
 		container_of(asd, struct isp_async_subdev, asd);
-	int ret;
-
-	ret = isp_link_entity(isp, &subdev->entity, isd->bus.interface);
-	if (ret < 0)
-		return ret;
 
 	isd->sd = subdev;
 	isd->sd->host_priv = &isd->bus;
 
-	return ret;
+	return 0;
 }
 
 static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
 {
 	struct isp_device *isp = container_of(async, struct isp_device,
 					      notifier);
+	struct v4l2_device *v4l2_dev = &isp->v4l2_dev;
+	struct v4l2_subdev *sd;
+	struct isp_bus_cfg *bus;
+	int ret;
+
+	list_for_each_entry(sd, &v4l2_dev->subdevs, list) {
+		/* Only try to link entities whose interface was set on bound */
+		if (sd->host_priv) {
+			bus = (struct isp_bus_cfg *)sd->host_priv;
+			ret = isp_link_entity(isp, &sd->entity, bus->interface);
+			if (ret < 0)
+				return ret;
+		}
+	}
 
 	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
 }