media: intel-ipu3: cio2: register the mdev on v4l2 async notifier complete

Commit 9832e155f1ed ("[media] media-device: split media initialization and registration") split the media_device_register() function in two, to avoid a race condition that can happen when the media device node is accessed by userpace before the pending subdevices have been asynchronously registered.

But the ipu3-cio2 driver calls the media_device_register() function right after calling media_device_init() which defeats the purpose of having two separate functions.

In that case, userspace could have a partial view of the media device if it opened the media device node before all the pending devices have been bound. So instead, only register the media device once all pending v4l2 subdevices have been registered.

Fixes 9832e155f1ed ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")

Signed-off-by Javier Martinez Canillas <[email protected]>

---

 drivers/media/pci/intel/ipu3/ipu3-cio2.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

drivers/media/pci/intel/ipu3/ipu3-cio2.c +10 -9

index 29027159eced..d936f3426c4e 100644
@@ -1468,7 +1468,14 @@ static int cio2_notifier_complete(struct v4l2_async_notifier *notifier)
 		}
 	}
 
-	return v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
+	ret = v4l2_device_register_subdev_nodes(&cio2->v4l2_dev);
+	if (ret) {
+		dev_err(&cio2->pci_dev->dev,
+			"failed to register V4L2 subdev nodes (%d)\n", ret);
+		return ret;
+	}
+
+	return media_device_register(&cio2->media_dev);
 }
 
 static const struct v4l2_async_notifier_operations cio2_async_ops = {

@@ -1792,16 +1799,12 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	cio2->media_dev.hw_revision = 0;
 
 	media_device_init(&cio2->media_dev);
-	r = media_device_register(&cio2->media_dev);
-	if (r < 0)
-		goto fail_mutex_destroy;
-
 	cio2->v4l2_dev.mdev = &cio2->media_dev;
 	r = v4l2_device_register(&pci_dev->dev, &cio2->v4l2_dev);
 	if (r) {
 		dev_err(&pci_dev->dev,
 			"failed to register V4L2 device (%d)\n", r);
-		goto fail_media_device_unregister;
+		goto fail_media_device_cleanup;
 	}
 
 	r = cio2_queues_init(cio2);

@@ -1831,10 +1834,8 @@ static int cio2_pci_probe(struct pci_dev *pci_dev,
 	cio2_queues_exit(cio2);
 fail_v4l2_device_unregister:
 	v4l2_device_unregister(&cio2->v4l2_dev);
-fail_media_device_unregister:
-	media_device_unregister(&cio2->media_dev);
+fail_media_device_cleanup:
 	media_device_cleanup(&cio2->media_dev);
-fail_mutex_destroy:
 	mutex_destroy(&cio2->lock);
 	cio2_fbpt_exit_dummy(cio2);
 

Quoted 9 lines · Javier Martinez Canillas

Commit 9832e155f1ed ("[media] media-device: split media initialization and registration") split the media_device_register() function in two, to avoid a race condition that can happen when the media device node is accessed by userpace before the pending subdevices have been asynchronously registered.

But the ipu3-cio2 driver calls the media_device_register() function right after calling media_device_init() which defeats the purpose of having two separate functions.

Quoted 4 lines

In that case, userspace could have a partial view of the media device if it opened the media device node before all the pending devices have been bound. So instead, only register the media device once all pending v4l2 subdevices have been registered.

Javier, Thanks for your patch.
IMHO, there are no big differences for registering the cio2 before and after all the subdevices are ready. User may see a partial view of media graph but it presents what it really is then.
It indicate that device is not available currently not it is not there.

Could you help tell more details about your problem? The full context is helpful for me to reproduce your problem.

Hi,

Thanks a lot your feedback.

Quoted 11 lines · Bing Bu Cao

Quoted 9 lines · Javier Martinez Canillas

Commit 9832e155f1ed ("[media] media-device: split media initialization and registration") split the media_device_register() function in two, to avoid a race condition that can happen when the media device node is accessed by userpace before the pending subdevices have been asynchronously registered.

But the ipu3-cio2 driver calls the media_device_register() function right after calling media_device_init() which defeats the purpose of having two separate functions.

Quoted 8 lines

Quoted 4 lines

In that case, userspace could have a partial view of the media device if it opened the media device node before all the pending devices have been bound. So instead, only register the media device once all pending v4l2 subdevices have been registered.

Javier, Thanks for your patch.
IMHO, there are no big differences for registering the cio2 before and after all the subdevices are ready. User may see a partial view of media graph but it presents what it really is then.
It indicate that device is not available currently not it is not there.

I disagree that there are no differences. The media graph shouldn't be exposed until its complete. That's the reason why we have a v4l2 async notifier .bound and .complete callbacks (otherwise the .bound would be enough).

It's also the reason why media register was split in _init and _register, as I mentioned in the commit message.

Quoted 1 line

Could you help tell more details about your problem? The full context is helpful for me to reproduce your problem.

If an application opens the media device node, how it would know that has an incomplete media graph? how it would know once the subdevice has been .bound and that has to query the media graph again?

AFAIK there's no way to notify that information to user-space currenctly but I may be wrong.

Best regards,

Quoted 24 lines · Javier Martinez Canillas

Hi,

Thanks a lot your feedback.

Quoted 19 lines · Bing Bu Cao

Quoted 5 lines · Javier Martinez Canillas

Commit 9832e155f1ed ("[media] media-device: split media initialization and registration") split the media_device_register() function in two, to avoid a race condition that can happen when the media device node is accessed by userpace before the pending subdevices have been asynchronously registered.

Quoted 8 lines

But the ipu3-cio2 driver calls the media_device_register() function right after calling media_device_init() which defeats the purpose of having two separate functions.

In that case, userspace could have a partial view of the media device if it opened the media device node before all the pending devices have been bound. So instead, only register the media device once all pending v4l2 subdevices have been registered.

Javier, Thanks for your patch.
IMHO, there are no big differences for registering the cio2 before and after all the subdevices are ready. User may see a partial view of media graph but it presents what it really is then.
It indicate that device is not available currently not it is not there.

Quoted 6 lines

I disagree that there are no differences. The media graph shouldn't be exposed until its complete. That's the reason why we have a v4l2 async notifier .bound and .complete callbacks (otherwise the .bound would be enough).

It's also the reason why media register was split in _init and _register, as I mentioned in the commit message.

I revisit the commit 9832e155f1ed and understand and agree that.

Seems like there are some other drivers (such as rcar-vin and omap4iss) still have similar issues.

-----Original Message-----
From: Javier Martinez Canillas [mailto:[email protected]]
Sent: Monday, September 3, 2018 4:52 PM
To: Bing Bu Cao <[email protected]>; [email protected]
Cc: Mauro Carvalho Chehab <[email protected]>; Qiu, Tian Shu <[email protected]>; Zheng, Jian Xu <[email protected]>; Sakari Ailus <[email protected]>; Zhi, Yong <[email protected]>; Cao, Bingbu <[email protected]>; [email protected]
Subject: Re: [PATCH] media: intel-ipu3: cio2: register the mdev on v4l2 async notifier complete

Quoted 27 lines · Bing Bu Cao

Quoted 24 lines · Javier Martinez Canillas

Hi,

Thanks a lot your feedback.

Quoted 19 lines · Bing Bu Cao

Quoted 5 lines · Javier Martinez Canillas

Commit 9832e155f1ed ("[media] media-device: split media initialization and registration") split the media_device_register() function in two, to avoid a race condition that can happen when the media device node is accessed by userpace before the pending subdevices have been asynchronously registered.

Quoted 8 lines

But the ipu3-cio2 driver calls the media_device_register() function right after calling media_device_init() which defeats the purpose of having two separate functions.

In that case, userspace could have a partial view of the media device if it opened the media device node before all the pending devices have been bound. So instead, only register the media device once all pending v4l2 subdevices have been registered.

Javier, Thanks for your patch.
IMHO, there are no big differences for registering the cio2 before and after all the subdevices are ready. User may see a partial view of media graph but it presents what it really is then.
It indicate that device is not available currently not it is not there.

Quoted 7 lines

Quoted 6 lines

I disagree that there are no differences. The media graph shouldn't be exposed until its complete. That's the reason why we have a v4l2 async notifier .bound and .complete callbacks (otherwise the .bound would be enough).

It's also the reason why media register was split in _init and _register, as I mentioned in the commit message.

I revisit the commit 9832e155f1ed and understand and agree that.

Great. Does this mean the patch has your Reviewed-by / Acked-by ?

Quoted 1 line

Seems like there are some other drivers (such as rcar-vin and omap4iss) still have similar issues.

Yes it seems so. I don't have access to hardware with these IP blocks though, I'm hesitant to attempt to fix them.

Best regards,
--
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat

Hi,

Raise my point.
The case here is that we have multiple sensors connected to CIO2. The sensors work independently. So failure on one sensor should not block the function of the other. That is, we should not rely on that all sensors are ready before allowing user to operate on the ready cameras.
Sometimes due to hardware issues or incompleteness, we did met the case that one sensor is not probing properly. And in this case, the current implementation blocks us using the working one. What I can think now to solve this are:
1. Register multiple media devices. One for each sensor path. This will increase media device count.
2. Use .bound callback to create the link and register the subdev node for each sensor. Leave .complete empty.
Not sure if this breaks the rule of media framework. And also have not found an API to register one single subdev node.

Hi Javier, Tian Shu,

Quoted 6 lines · Tian Shu

Hi,

Raise my point.
The case here is that we have multiple sensors connected to CIO2. The sensors work independently. So failure on one sensor should not block the function of the other. That is, we should not rely on that all sensors are ready before allowing user to operate on the ready cameras.
Sometimes due to hardware issues or incompleteness, we did met the case that one sensor is not probing properly. And in this case, the current implementation blocks us using the working one.

Quoted 4 lines

What I can think now to solve this are:
1. Register multiple media devices. One for each sensor path. This will increase media device count.
2. Use .bound callback to create the link and register the subdev node for each sensor. Leave .complete empty. Not sure if this breaks the rule of media framework. And also have not found an API to register one single subdev node.

I'd prefer to keep the driver as-is.

Even if the media device is only created once all the sub-devices are around, the devices are still created one by one so there's no way to prevent the user space seeing a partially registered media device complex.

In general that doesn't happen as the sensors are typically registered early during system boot.

Javier is right in asking a way for the user to know whether everything is fully initialised. That should be added but I don't think it is in any way specific to the cio2 driver.

Hi Javier, Tian Shu,

Quoted 10 lines · Tian Shu

Hi,

Raise my point.
The case here is that we have multiple sensors connected to CIO2. The sensors work independently. So failure on one sensor should not block the function of the other. That is, we should not rely on that all sensors are ready before allowing user to operate on the ready cameras.
Sometimes due to hardware issues or incompleteness, we did met the case that one sensor is not probing properly. And in this case, the current implementation blocks us using the working one. What I can think now to solve this are:
1. Register multiple media devices. One for each sensor path. This will increase media device count.
2. Use .bound callback to create the link and register the subdev node for each sensor. Leave .complete empty.
Not sure if this breaks the rule of media framework. And also have not found an API to register one single subdev node.

I'd prefer to keep the driver as-is.

Even if the media device is only created once all the sub-devices are around, the devices are still created one by one so there's no way to prevent the user space seeing a partially registered media device complex.

In general that doesn't happen as the sensors are typically registered early during system boot.

Javier is right in asking a way for the user to know whether everything is fully initialised. That should be added but I don't think it is in any way specific to the cio2 driver.

Quoted 20 lines · Sakari Ailus

Hi Javier, Tian Shu,

Quoted 10 lines · Tian Shu

Hi,

Raise my point.
The case here is that we have multiple sensors connected to CIO2. The sensors work independently. So failure on one sensor should not block the function of the other. That is, we should not rely on that all sensors are ready before allowing user to operate on the ready cameras.
Sometimes due to hardware issues or incompleteness, we did met the case that one sensor is not probing properly. And in this case, the current implementation blocks us using the working one. What I can think now to solve this are:
1. Register multiple media devices. One for each sensor path. This will increase media device count.
2. Use .bound callback to create the link and register the subdev node for each sensor. Leave .complete empty.
Not sure if this breaks the rule of media framework. And also have not found an API to register one single subdev node.

I'd prefer to keep the driver as-is.

Even if the media device is only created once all the sub-devices are around, the devices are still created one by one so there's no way to prevent the user space seeing a partially registered media device complex.

Quoted 7 lines

In general that doesn't happen as the sensors are typically registered early during system boot.

Javier is right in asking a way for the user to know whether everything is fully initialised. That should be added but I don't think it is in any way specific to the cio2 driver.

Today we have no userspace mechanism to deal with partially initialized topologies. Instead if parts fails to come up we shouldn't register any media device and instead (once we discover that something is broken) tear everything down.

In fact, video/subdev/media devices shouldn't be registered until everything is complete.

I know we want to allow for partial bring up as well, and I fully agree with that, but in that case someone needs to write an RFC with a proposal how userspace should handle this.

We've discussed this in the past, but I have not seen such an RFC.

So until we add support for partial bringup I think this patch does the right thing since otherwise this is out-of-spec.

Hi,

Raise my point.
The case here is that we have multiple sensors connected to CIO2. The sensors work independently. So failure on one sensor should not block the function of the other. That is, we should not rely on that all sensors are ready before allowing user to operate on the ready cameras.
Sometimes due to hardware issues or incompleteness, we did met the case that one sensor is not probing properly. And in this case, the current implementation blocks us using the working one. What I can think now to solve this are:

1. Register multiple media devices. One for each sensor path. This will increase media device count.
2. Use .bound callback to create the link and register the subdev node for each sensor. Leave .complete empty. Not sure if this breaks the rule of media framework. And also have not found an API to register one single subdev node.

Thanks
Tianshu Qiu