Message-ID: <55F1CC72.6000204@osg.samsung.com>
Date:	Thu, 10 Sep 2015 20:31:14 +0200
From:	Javier Martinez Canillas <javier@....samsung.com>
To:	Sakari Ailus <sakari.ailus@...ux.intel.com>,
	linux-kernel@...r.kernel.org
Cc:	Luis de Bethencourt <luis@...ethencourt.com>,
	linux-sh@...r.kernel.org,
	Sylwester Nawrocki <s.nawrocki@...sung.com>,
	Sören Brinkmann <soren.brinkmann@...inx.com>,
	linux-samsung-soc@...r.kernel.org,
	Hyun Kwon <hyun.kwon@...inx.com>,
	Matthias Schwarzott <zzam@...too.org>,
	Kyungmin Park <kyungmin.park@...sung.com>,
	Tommi Rantala <tt.rantala@...il.com>,
	Laurent Pinchart <laurent.pinchart@...asonboard.com>,
	Hans Verkuil <hans.verkuil@...co.com>,
	linux-media@...r.kernel.org, Kukjin Kim <kgene@...nel.org>,
	Krzysztof Kozlowski <k.kozlowski@...sung.com>,
	Mauro Carvalho Chehab <mchehab@....samsung.com>,
	Michal Simek <michal.simek@...inx.com>,
	Olli Salonen <olli.salonen@....fi>,
	linux-arm-kernel@...ts.infradead.org,
	Stefan Richter <stefanr@...6.in-berlin.de>,
	Antti Palosaari <crope@....fi>,
	Shuah Khan <shuahkh@....samsung.com>,
	Rafael Lourenço de Lima Chehab 
	<chehabrafael@...il.com>
Subject: Re: [PATCH 2/2] [media] media-device: split media initialization and
 registration

Hello Sakari,

On 09/10/2015 07:14 PM, Sakari Ailus wrote:
> Hi Javier,
> 
> Thanks for the set! A few comments below.
>

Thanks to you for your feedback.

> Javier Martinez Canillas wrote:
>> The media device node is registered and so made visible to user-space
>> before entities are registered and links created which means that the
>> media graph obtained by user-space could be only partially enumerated
>> if that happens too early before all the graph has been created.
>>
>> To avoid this race condition, split the media init and registration
>> in separate functions and only register the media device node when
>> all the pending subdevices have been registered, either explicitly
>> by the driver or asynchronously using v4l2_async_register_subdev().
>>
>> Also, add a media_entity_cleanup() function that will destroy the
>> graph_mutex that is initialized in media_entity_init().
>>
>> Suggested-by: Sakari Ailus <sakari.ailus@...ux.intel.com>
>> Signed-off-by: Javier Martinez Canillas <javier@....samsung.com>
>>
>> ---
>>
>>  drivers/media/common/siano/smsdvb-main.c      |  1 +
>>  drivers/media/media-device.c                  | 38 +++++++++++++++++++++++----
>>  drivers/media/platform/exynos4-is/media-dev.c | 12 ++++++---
>>  drivers/media/platform/omap3isp/isp.c         | 11 +++++---
>>  drivers/media/platform/s3c-camif/camif-core.c | 13 ++++++---
>>  drivers/media/platform/vsp1/vsp1_drv.c        | 19 ++++++++++----
>>  drivers/media/platform/xilinx/xilinx-vipp.c   | 11 +++++---
>>  drivers/media/usb/au0828/au0828-core.c        | 26 +++++++++++++-----
>>  drivers/media/usb/cx231xx/cx231xx-cards.c     | 22 +++++++++++-----
>>  drivers/media/usb/dvb-usb-v2/dvb_usb_core.c   | 11 +++++---
>>  drivers/media/usb/dvb-usb/dvb-usb-dvb.c       | 13 ++++++---
>>  drivers/media/usb/siano/smsusb.c              | 14 ++++++++--
>>  drivers/media/usb/uvc/uvc_driver.c            |  9 +++++--
>>  include/media/media-device.h                  |  2 ++
>>  14 files changed, 156 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/media/common/siano/smsdvb-main.c b/drivers/media/common/siano/smsdvb-main.c
>> index ab345490a43a..8a1ea2192439 100644
>> --- a/drivers/media/common/siano/smsdvb-main.c
>> +++ b/drivers/media/common/siano/smsdvb-main.c
>> @@ -617,6 +617,7 @@ static void smsdvb_media_device_unregister(struct smsdvb_client_t *client)
>>  	if (!coredev->media_dev)
>>  		return;
>>  	media_device_unregister(coredev->media_dev);
>> +	media_device_cleanup(coredev->media_dev);
>>  	kfree(coredev->media_dev);
>>  	coredev->media_dev = NULL;
>>  #endif
>> diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
>> index 745defb34b33..a8beb0b445a6 100644
>> --- a/drivers/media/media-device.c
>> +++ b/drivers/media/media-device.c
>> @@ -526,7 +526,7 @@ static void media_device_release(struct media_devnode *mdev)
>>  }
>>  
>>  /**
>> - * media_device_register - register a media device
>> + * media_device_init() - initialize a media device
>>   * @mdev:	The media device
>>   *
>>   * The caller is responsible for initializing the media device before
>> @@ -534,12 +534,11 @@ static void media_device_release(struct media_devnode *mdev)
>>   *
>>   * - dev must point to the parent device
>>   * - model must be filled with the device model name
>> + *
>> + * returns zero on success or a negative error code.
>>   */
>> -int __must_check __media_device_register(struct media_device *mdev,
>> -					 struct module *owner)
>> +int __must_check media_device_init(struct media_device *mdev)
> 
> I think I suggested making media_device_init() return void as the only
> remaining source of errors would be driver bugs.
>

Yes you did and I think I explained why I preferred to leave it as
is and I thought we agreed :)

Currently media_device_register() is failing gracefully if a buggy
driver is not setting mdev->dev. So changing media_device_init() to
return void instead, would be a semantic change and if drivers are
not checking that anymore, can lead to NULL pointer dereference bugs.

> I'd simply replace the WARN_ON() below with BUG().
>

Sorry but I disagree, I think that BUG() should only be used for
exceptional cases in which execution can't really continue without
causing data corruption or something like that, so bringing down
the machine is the safest and least bad option.

But that's not the case here, if there is a buggy driver then the
worst thing that would happen is that a driver probe function is
going to fail. It is true that drivers may not be cheking this but
that's why is annotated with __must_check.

>>  {
>> -	int ret;
>> -
>>  	if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0))
>>  		return -EINVAL;
>>

We can later audit all drivers and change this function to return
void instead and get rid of this check but I would prefer to do it
as a followup patch.

>> @@ -550,6 +549,35 @@ int __must_check __media_device_register(struct media_device *mdev,
>>  	spin_lock_init(&mdev->lock);
>>  	mutex_init(&mdev->graph_mutex);
>>  
>> +	dev_dbg(mdev->dev, "Media device initialized\n");
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_init);
>> +
>> +/**
>> + * media_device_cleanup() - Cleanup a media device
>> + * @mdev:	The media device
>> + *
>> + */
>> +void media_device_cleanup(struct media_device *mdev)
>> +{
>> +	mutex_destroy(&mdev->graph_mutex);
> 
> Very nice!
>
>> +}
>> +EXPORT_SYMBOL_GPL(media_device_cleanup);
>> +
>> +/**
>> + * __media_device_register() - register a media device
>> + * @mdev:	The media device
>> + * @owner:	The module owner
>> + *
>> + * returns zero on success or a negative error code.
>> + */
>> +int __must_check __media_device_register(struct media_device *mdev,
>> +					 struct module *owner)
>> +{
>> +	int ret;
>> +
>>  	/* Register the device node. */
>>  	mdev->devnode.fops = &media_device_fops;
>>  	mdev->devnode.parent = mdev->dev;
>> diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
>> index 4a25df9dd869..158738bd23fc 100644
>> --- a/drivers/media/platform/exynos4-is/media-dev.c
>> +++ b/drivers/media/platform/exynos4-is/media-dev.c
>> @@ -1313,7 +1313,10 @@ static int subdev_notifier_complete(struct v4l2_async_notifier *notifier)
>>  	ret = v4l2_device_register_subdev_nodes(&fmd->v4l2_dev);
>>  unlock:
>>  	mutex_unlock(&fmd->media_dev.graph_mutex);
>> -	return ret;
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return media_device_register(&fmd->media_dev);
> 
> Uh-oh.
> 
> I guess it's fair to simply fail here. probe() is already over so I
> wonder if there's anything we could really do except await someone
> unloading and loading the module again? Or, replugging the hardware.
>

Yes, in fact I noticed the same. The problem is that drivers that
register a notifier with v4l2_async_notifier_register(), don't know if
a complete callback (or bound callback FWIW) failed. Only the subdev
driver knows that something failed when it tried to register a subdev
asynchronously with v4l2_async_register_subdev().

> I don't like the idea but I guess this could be solved later on.
>

Yes me neither but since v4l2_device_register_subdev_nodes() can also
fail and the return value was returned to v4l2_async_test_notify(), I
decided to do the same when adding the call to media_device_register().

But I agree that this should be fixed somehow, either making all drivers
do cleanup if either the bound or complete notifier callbacks fail or
the v4l2-async core should call a cleanup function that is registered by
the drivers.

But as you said I also think this could be solved later on since it's a
separate issue IMHO.

>>  }
>>  
>>  static int fimc_md_probe(struct platform_device *pdev)
>> @@ -1350,9 +1353,9 @@ static int fimc_md_probe(struct platform_device *pdev)
>>  		return ret;
>>  	}
>>  
>> -	ret = media_device_register(&fmd->media_dev);
>> +	ret = media_device_init(&fmd->media_dev);
>>  	if (ret < 0) {
>> -		v4l2_err(v4l2_dev, "Failed to register media device: %d\n", ret);
>> +		v4l2_err(v4l2_dev, "Failed to init media device: %d\n", ret);
>>  		goto err_v4l2_dev;
>>  	}
>>  
>> @@ -1424,7 +1427,7 @@ err_clk:
>>  err_m_ent:
>>  	fimc_md_unregister_entities(fmd);
>>  err_md:
>> -	media_device_unregister(&fmd->media_dev);
>> +	media_device_cleanup(&fmd->media_dev);
>>  err_v4l2_dev:
>>  	v4l2_device_unregister(&fmd->v4l2_dev);
>>  	return ret;
>> @@ -1445,6 +1448,7 @@ static int fimc_md_remove(struct platform_device *pdev)
>>  	fimc_md_unregister_entities(fmd);
>>  	fimc_md_pipelines_free(fmd);
>>  	media_device_unregister(&fmd->media_dev);
>> +	media_device_cleanup(&fmd->media_dev);
>>  	fimc_md_put_clocks(fmd);
>>  
>>  	return 0;
>> diff --git a/drivers/media/platform/omap3isp/isp.c b/drivers/media/platform/omap3isp/isp.c
>> index cb8ac90086c1..b7eda3043c31 100644
>> --- a/drivers/media/platform/omap3isp/isp.c
>> +++ b/drivers/media/platform/omap3isp/isp.c
>> @@ -1793,6 +1793,7 @@ static void isp_unregister_entities(struct isp_device *isp)
>>  
>>  	v4l2_device_unregister(&isp->v4l2_dev);
>>  	media_device_unregister(&isp->media_dev);
>> +	media_device_cleanup(&isp->media_dev);
>>  }
>>  
>>  static int isp_link_entity(
>> @@ -1875,9 +1876,9 @@ static int isp_register_entities(struct isp_device *isp)
>>  		sizeof(isp->media_dev.model));
>>  	isp->media_dev.hw_revision = isp->revision;
>>  	isp->media_dev.link_notify = isp_pipeline_link_notify;
>> -	ret = media_device_register(&isp->media_dev);
>> +	ret = media_device_init(&isp->media_dev);
>>  	if (ret < 0) {
>> -		dev_err(isp->dev, "%s: Media device registration failed (%d)\n",
>> +		dev_err(isp->dev, "%s: Media device init failed (%d)\n",
>>  			__func__, ret);
>>  		return ret;
>>  	}
>> @@ -2347,7 +2348,11 @@ static int isp_subdev_notifier_complete(struct v4l2_async_notifier *async)
>>  		}
>>  	}
>>  
>> -	return v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
>> +	ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return media_device_register(&isp->media_dev);
>>  }
>>  
>>  /*
>> diff --git a/drivers/media/platform/s3c-camif/camif-core.c b/drivers/media/platform/s3c-camif/camif-core.c
>> index 3e33c60be004..1428db2f804d 100644
>> --- a/drivers/media/platform/s3c-camif/camif-core.c
>> +++ b/drivers/media/platform/s3c-camif/camif-core.c
>> @@ -305,7 +305,7 @@ static void camif_unregister_media_entities(struct camif_dev *camif)
>>  /*
>>   * Media device
>>   */
>> -static int camif_media_dev_register(struct camif_dev *camif)
>> +static int camif_media_dev_init(struct camif_dev *camif)
>>  {
>>  	struct media_device *md = &camif->media_dev;
>>  	struct v4l2_device *v4l2_dev = &camif->v4l2_dev;
>> @@ -328,7 +328,7 @@ static int camif_media_dev_register(struct camif_dev *camif)
>>  	if (ret < 0)
>>  		return ret;
>>  
>> -	ret = media_device_register(md);
>> +	ret = media_device_init(md);
>>  	if (ret < 0)
>>  		v4l2_device_unregister(v4l2_dev);
>>  
>> @@ -483,7 +483,7 @@ static int s3c_camif_probe(struct platform_device *pdev)
>>  		goto err_alloc;
>>  	}
>>  
>> -	ret = camif_media_dev_register(camif);
>> +	ret = camif_media_dev_init(camif);
>>  	if (ret < 0)
>>  		goto err_mdev;
>>  
>> @@ -510,6 +510,11 @@ static int s3c_camif_probe(struct platform_device *pdev)
>>  		goto err_unlock;
>>  
>>  	mutex_unlock(&camif->media_dev.graph_mutex);
>> +
>> +	ret = media_device_register(&camif->media_dev);
>> +	if (ret < 0)
>> +		goto err_sens;
>> +
>>  	pm_runtime_put(dev);
>>  	return 0;
>>  
>> @@ -518,6 +523,7 @@ err_unlock:
>>  err_sens:
>>  	v4l2_device_unregister(&camif->v4l2_dev);
>>  	media_device_unregister(&camif->media_dev);
>> +	media_device_cleanup(&camif->media_dev);
>>  	camif_unregister_media_entities(camif);
>>  err_mdev:
>>  	vb2_dma_contig_cleanup_ctx(camif->alloc_ctx);
>> @@ -539,6 +545,7 @@ static int s3c_camif_remove(struct platform_device *pdev)
>>  	struct s3c_camif_plat_data *pdata = &camif->pdata;
>>  
>>  	media_device_unregister(&camif->media_dev);
>> +	media_device_cleanup(&camif->media_dev);
>>  	camif_unregister_media_entities(camif);
>>  	v4l2_device_unregister(&camif->v4l2_dev);
>>  
>> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c
>> index 8f995d267646..bcbc24e55bf5 100644
>> --- a/drivers/media/platform/vsp1/vsp1_drv.c
>> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
>> @@ -127,6 +127,7 @@ static void vsp1_destroy_entities(struct vsp1_device *vsp1)
>>  
>>  	v4l2_device_unregister(&vsp1->v4l2_dev);
>>  	media_device_unregister(&vsp1->media_dev);
>> +	media_device_cleanup(&vsp1->media_dev);
>>  }
>>  
>>  static int vsp1_create_entities(struct vsp1_device *vsp1)
>> @@ -141,9 +142,9 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
>>  	strlcpy(mdev->model, "VSP1", sizeof(mdev->model));
>>  	snprintf(mdev->bus_info, sizeof(mdev->bus_info), "platform:%s",
>>  		 dev_name(mdev->dev));
>> -	ret = media_device_register(mdev);
>> +	ret = media_device_init(mdev);
>>  	if (ret < 0) {
>> -		dev_err(vsp1->dev, "media device registration failed (%d)\n",
>> +		dev_err(vsp1->dev, "media device init failed (%d)\n",
>>  			ret);
>>  		return ret;
>>  	}
>> @@ -288,11 +289,19 @@ static int vsp1_create_entities(struct vsp1_device *vsp1)
>>  	}
>>  
>>  	ret = v4l2_device_register_subdev_nodes(&vsp1->v4l2_dev);
>> -
>> -done:
>>  	if (ret < 0)
>> -		vsp1_destroy_entities(vsp1);
>> +		goto done;
>>  
>> +	ret = media_device_register(mdev);
> 
> Are there cases where media_device_register() won't complain aloud
> already? I wouldn't print an error message in a driver anymore.
>

Good question, there are already error messages printed if the media dev
node fails to be registered or sysfs attribute file fails to be created.

What I did is to follow the convention used on each driver and added an
error message for both media_device_init() and media_device_register()
if the driver already had one for the old media_device_register() and
didn't if the driver was not doing it.

>> +	if (ret < 0) {
>> +		dev_err(vsp1->dev, "media device init failed (%d)\n", ret);
>> +		goto done;
>> +	}
>> +
>> +	return 0;
>> +
>> +done:
>> +	vsp1_destroy_entities(vsp1);
>>  	return ret;
>>  }
>>  
>> diff --git a/drivers/media/platform/xilinx/xilinx-vipp.c b/drivers/media/platform/xilinx/xilinx-vipp.c
>> index 79d4be7ce9a5..5bb18aee0707 100644
>> --- a/drivers/media/platform/xilinx/xilinx-vipp.c
>> +++ b/drivers/media/platform/xilinx/xilinx-vipp.c
>> @@ -311,6 +311,10 @@ static int xvip_graph_notify_complete(struct v4l2_async_notifier *notifier)
>>  	if (ret < 0)
>>  		dev_err(xdev->dev, "failed to register subdev nodes\n");
>>  
>> +	ret = media_device_register(&xdev->media_dev);
> 
> Same here (and elsewhere).
>

I don't have a strong opinion, if you think that is better to remove
the error messages, I can do it when re-spining the patches.

[snip]

Best regards,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists