Re: [PATCH v2] [media] media-device: handle errors at media_device_init()

[Javier Martinez Canillas]

Hello Mauro,

On 12/15/2015 09:22 AM, Mauro Carvalho Chehab wrote:
> Changeset 43ac4401dca9 ("[media] media-device: split media
> initialization and registration") broke media device register
> into two separate functions, but introduced a BUG_ON() and
> made media_device_init() void. It also introduced several
> warnings.
> 
> Instead of adding BUG_ON(), let's revert to WARN_ON() and fix
> the init code in a way that, if something goes wrong during
> device init, driver probe will fail without causing the Kernel
> to BUG.
> 
> Signed-off-by: Mauro Carvalho Chehab 
> ---

I agree with your patch, in fact the first version of the media
split patch did exactly this and later media_device_init() was
converted to void and the BUG_ON() introduced to address some
feedback I got during the patches review.

Reviewed-by: Javier Martinez Canillas 

On an OMAP3 IGEPv2:

Tested-by: Javier Martinez Canillas 

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

[PATCH v2] [media] media-device: handle errors at media_device_init()

[Mauro Carvalho Chehab]

Changeset 43ac4401dca9 ("[media] media-device: split media
initialization and registration") broke media device register
into two separate functions, but introduced a BUG_ON() and
made media_device_init() void. It also introduced several
warnings.

Instead of adding BUG_ON(), let's revert to WARN_ON() and fix
the init code in a way that, if something goes wrong during
device init, driver probe will fail without causing the Kernel
to BUG.

Signed-off-by: Mauro Carvalho Chehab 
---
 drivers/media/media-device.c  |  8 ---
 drivers/media/platform/exynos4-is/media-dev.c |  7 +-
 drivers/media/platform/omap3isp/isp.c |  9 ++--
 drivers/media/platform/s3c-camif/camif-core.c |  4 +++-
 drivers/media/platform/vsp1/vsp1_drv.c|  7 +-
 drivers/media/platform/xilinx/xilinx-vipp.c   |  7 +-
 drivers/media/usb/au0828/au0828-core.c| 26 +
 drivers/media/usb/cx231xx/cx231xx-cards.c | 23 +++
 drivers/media/usb/dvb-usb-v2/dvb_usb_core.c   | 33 +++
 drivers/media/usb/dvb-usb/dvb-usb-dvb.c   | 32 +++---
 drivers/media/usb/siano/smsusb.c  |  6 -
 drivers/media/usb/uvc/uvc_driver.c|  3 ++-
 include/media/media-device.h  |  2 +-
 13 files changed, 128 insertions(+), 39 deletions(-)

diff --git a/drivers/media/media-device.c b/drivers/media/media-device.c
index 76e2b2f3a15f..34621605eed7 100644
--- a/drivers/media/media-device.c
+++ b/drivers/media/media-device.c
@@ -625,10 +625,10 @@ EXPORT_SYMBOL_GPL(media_device_unregister_entity);
  * - dev must point to the parent device
  * - model must be filled with the device model name
  */
-void media_device_init(struct media_device *mdev)
+int __must_check media_device_init(struct media_device *mdev)
 {
-
-   BUG_ON(mdev->dev == NULL);
+   if (WARN_ON(mdev->dev == NULL))
+   return -EINVAL;
 
INIT_LIST_HEAD(&mdev->entities);
INIT_LIST_HEAD(&mdev->interfaces);
@@ -638,6 +638,8 @@ void media_device_init(struct media_device *mdev)
mutex_init(&mdev->graph_mutex);
 
dev_dbg(mdev->dev, "Media device initialized\n");
+
+   return 0;
 }
 EXPORT_SYMBOL_GPL(media_device_init);
 
diff --git a/drivers/media/platform/exynos4-is/media-dev.c 
b/drivers/media/platform/exynos4-is/media-dev.c
index 27663dd45294..4fea0037dade 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -1353,7 +1353,11 @@ static int fimc_md_probe(struct platform_device *pdev)
return ret;
}
 
-   media_device_init(&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);
+   goto err_v4l2_dev;
+   }
 
ret = fimc_md_get_clocks(fmd);
if (ret)
@@ -1424,6 +1428,7 @@ err_m_ent:
fimc_md_unregister_entities(fmd);
 err_md:
media_device_cleanup(&fmd->media_dev);
+err_v4l2_dev:
v4l2_device_unregister(&fmd->v4l2_dev);
return ret;
 }
diff --git a/drivers/media/platform/omap3isp/isp.c 
b/drivers/media/platform/omap3isp/isp.c
index 942b189c0eca..347bfdd8ce65 100644
--- a/drivers/media/platform/omap3isp/isp.c
+++ b/drivers/media/platform/omap3isp/isp.c
@@ -1876,7 +1876,12 @@ 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;
-   media_device_init(&isp->media_dev);
+   ret = media_device_init(&isp->media_dev);
+   if (ret < 0) {
+   dev_err(isp->dev, "%s: Media device registration failed (%d)\n",
+   __func__, ret);
+   return ret;
+   }
 
isp->v4l2_dev.mdev = &isp->media_dev;
ret = v4l2_device_register(isp->dev, &isp->v4l2_dev);
@@ -2361,7 +2366,7 @@ static int isp_subdev_notifier_complete(struct 
v4l2_async_notifier *async)
}
}
 
-   ret = v4l2_device_register_subdev_nodes(&isp->v4l2_dev);
+   ret = v4l2_device_register_subdev_nodes(v4l2_dev);
if (ret < 0)
return ret;
 
diff --git a/drivers/media/platform/s3c-camif/camif-core.c 
b/drivers/media/platform/s3c-camif/camif-core.c
index ea02b7ef2119..6e6ad152adbc 100644
--- a/drivers/media/platform/s3c-camif/camif-core.c
+++ b/drivers/media/platform/s3c-camif/camif-core.c
@@ -328,7 +328,9 @@ static int camif_media_dev_init(struct camif_dev *camif)
if (ret < 0)
return ret;
 
-   media_device_init(md);
+   ret = media_device_init(md);
+   if (ret < 0)
+   v4l2_device_unregister(v4l2_dev);
 
return ret;
 }
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c 
b/drivers/media/platform/vsp1/vsp1_drv.c
index 42dff9d020af..1e319eaeb8fe 100644
--- a/dr