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 <javierm@redhat.com>
---

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

On 08/31/2018 11:20 PM, Javier Martinez Canillas wrote:
> 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.
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.
>
> Fixes: 9832e155f1ed ("media: intel-ipu3: cio2: add new MIPI-CSI2 driver")
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>
>  drivers/media/pci/intel/ipu3/ipu3-cio2.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/pci/intel/ipu3/ipu3-cio2.c b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> index 29027159eced..d936f3426c4e 100644
> --- a/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> +++ b/drivers/media/pci/intel/ipu3/ipu3-cio2.c
> @@ -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);
>
  

Hi,

Thanks a lot your feedback.

On 09/03/2018 09:25 AM, Bing Bu Cao wrote:
> 
> 
> On 08/31/2018 11:20 PM, Javier Martinez Canillas wrote:
>> 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.
> 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.

> 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,
  

On 09/03/2018 03:35 PM, Javier Martinez Canillas wrote:
> Hi,
>
> Thanks a lot your feedback.
>
> On 09/03/2018 09:25 AM, Bing Bu Cao wrote:
>>
>> On 08/31/2018 11:20 PM, Javier Martinez Canillas wrote:
>>> 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.
>> 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.
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.
>> 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,
  

On 09/03/2018 10:49 AM, Bing Bu Cao wrote:
> 
> 
> On 09/03/2018 03:35 PM, Javier Martinez Canillas wrote:
>> Hi,
>>
>> Thanks a lot your feedback.
>>
>> On 09/03/2018 09:25 AM, Bing Bu Cao wrote:
>>>
>>> On 08/31/2018 11:20 PM, Javier Martinez Canillas wrote:
>>>> 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.
>>> 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.
> I revisit the commit 9832e155f1ed and understand and agree that.

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

> 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,
  

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


> -----Original Message-----
> From: Javier Martinez Canillas [mailto:javierm@redhat.com]
> Sent: Monday, September 3, 2018 4:52 PM
> To: Bing Bu Cao <bingbu.cao@linux.intel.com>; linux-kernel@vger.kernel.org
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>; Qiu, Tian Shu <tian.shu.qiu@intel.com>; Zheng, Jian Xu
> <jian.xu.zheng@intel.com>; Sakari Ailus <sakari.ailus@linux.intel.com>; Zhi, Yong <yong.zhi@intel.com>; Cao, Bingbu
> <bingbu.cao@intel.com>; linux-media@vger.kernel.org
> Subject: Re: [PATCH] media: intel-ipu3: cio2: register the mdev on v4l2 async notifier complete
> 
> On 09/03/2018 10:49 AM, Bing Bu Cao wrote:
> >
> >
> > On 09/03/2018 03:35 PM, Javier Martinez Canillas wrote:
> >> Hi,
> >>
> >> Thanks a lot your feedback.
> >>
> >> On 09/03/2018 09:25 AM, Bing Bu Cao wrote:
> >>>
> >>> On 08/31/2018 11:20 PM, Javier Martinez Canillas wrote:
> >>>> 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.
> >>> 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.
> > I revisit the commit 9832e155f1ed and understand and agree that.
> 
> Great. Does this mean the patch has your Reviewed-by / Acked-by ?
> 
> > 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 Javier, Tian Shu,

On Tue, Sep 04, 2018 at 05:01:56AM +0000, Qiu, Tian Shu wrote:
> 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.
  

Hi Sakari,  Tian Shu,

On 09/04/2018 08:46 AM, Sakari Ailus wrote:
> Hi Javier, Tian Shu,
> 
> On Tue, Sep 04, 2018 at 05:01:56AM +0000, Qiu, Tian Shu wrote:
>> 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.

This is a valid concern, but unfortunately the media graph is created in a quite
static way currently: the port and enpoints are parsed in the hardware topology,
the driver waits for the subdevices to be registered and bound, and is notified
by the .complete callback when there aren't more pending devices to be registered.

With the current infrastructure, I think we have to wait to register the media
device node until all devices have been bound, even if that would mean a single
camera driver not probing will cause the media graph to not be available.

Probably the correct approach would be to allow the media graph to dynamically
change and notify the user when new nodes are added or deleted (currently we
prevent to remove modules for the devices bound to the media device).

>> 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.
>

Yes, but at least the media device node won't be available.

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

It's true if the drivers are built-in, but they can be built as modules.

The goal with this patch was to prevent having a media graph that's not
useful, for example I've a media graph with no sensors media entities
(because there are no drivers supporting the sensors on my machine).

I prefer not having a media node in that case, that way users can know
that something went wrong / some support is missing.

> 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.
>

I was thinking about using the presence of the media device node as an
indication that the media graph has been fully initialized. And I thought
that was the agreement that lead to the commit 9832e155f1ed ("[media]
media-device: split media initialization and registration").

Best regards,
  

Hi Tian Shu,

On 09/04/2018 07:01 AM, Qiu, Tian Shu wrote:
> 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:

After discussing this with Sakari over IRC, I agree with you that $SUBJECT can
do more harm than good and the patch should just be dropped.

> 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 agree with your comment on (2) since currently the driver isn't able to cope
with the case that you are describing, as you mention the links and the subdev
node registration are done in the .complete callback. So that logic should be
moved to the .bound callback instead, so the media graph is usable even if one
of the drivers for a pending subdevice fails to probe.

> Thanks
> Tianshu Qiu
> 

Best regards,
  

On 09/04/2018 08:46 AM, Sakari Ailus wrote:
> Hi Javier, Tian Shu,
> 
> On Tue, Sep 04, 2018 at 05:01:56AM +0000, Qiu, Tian Shu wrote:
>> 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.
> 

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.

Regards,

	Hans
  

Hi Hans,

Thanks a lot for your feedback.

On 09/27/2018 12:09 PM, Hans Verkuil wrote:
> On 09/04/2018 08:46 AM, Sakari Ailus wrote:
>> Hi Javier, Tian Shu,
>>
>> On Tue, Sep 04, 2018 at 05:01:56AM +0000, Qiu, Tian Shu wrote:
>>> 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.
>>
> 
> 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.
>

I'm OK with $SUBJECT to be merged until we have a mechanism to let user-space
know about the media topology state. Later we can revisit the patches in [0],
once we have that support.

[0]: https://patchwork.kernel.org/cover/10587183/

> 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.
>

Does this mean I have your Acked-by?

Best regards,