Skip to contentSkip to conversation

media-device: handle errors at media_device_init()

PATCH New Mauro Carvalho Chehab 2 participants 2 messages Dec 15, 2015 – Dec 21, 2015

Subsystems (9)
Media (V4L/DVB) Arm Samsung Exynos SuperH Arm/Zynq architecture +5 more subsystems
lore
Share & tools
lore

Mauro Carvalho Chehab

author
#0 lore raw

PATCH v2 1/1 media-device: handle errors at media_device_init()

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 <[email protected]>
---
 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(-)
drivers/media/media-device.c +5 -3
index 76e2b2f3a15f..34621605eed7 100644
@@ -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);
drivers/media/platform/exynos4-is/media-dev.c +6 -1
index 27663dd45294..4fea0037dade 100644
@@ -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;
 }
drivers/media/platform/omap3isp/isp.c +7 -2
index 942b189c0eca..347bfdd8ce65 100644
@@ -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;
drivers/media/platform/s3c-camif/camif-core.c +3 -1
index ea02b7ef2119..6e6ad152adbc 100644
@@ -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;
 }
drivers/media/platform/vsp1/vsp1_drv.c +6 -1
index 42dff9d020af..1e319eaeb8fe 100644
@@ -142,7 +142,12 @@ 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));
-	media_device_init(mdev);
+	ret = media_device_init(mdev);
+	if (ret < 0) {
+		dev_err(vsp1->dev, "media device registration failed (%d)\n",
+			ret);
+		return ret;
+	}
 
 	vdev->mdev = mdev;
 	ret = v4l2_device_register(vsp1->dev, vdev);
drivers/media/platform/xilinx/xilinx-vipp.c +6 -1
index e795a4501e8b..7c5d23010d6f 100644
@@ -585,7 +585,12 @@ static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev)
 		sizeof(xdev->media_dev.model));
 	xdev->media_dev.hw_revision = 0;
 
-	media_device_init(&xdev->media_dev);
+	ret = media_device_init(&xdev->media_dev);
+	if (ret < 0) {
+		dev_err(xdev->dev, "media device registration failed (%d)\n",
+			ret);
+		return ret;
+	}
 
 	xdev->v4l2_dev.mdev = &xdev->media_dev;
 	ret = v4l2_device_register(xdev->dev, &xdev->v4l2_dev);
drivers/media/usb/au0828/au0828-core.c +21 -5
index e7aab47ae4f5..2f91bbc633b4 100644
@@ -217,15 +217,16 @@ static void au0828_usb_disconnect(struct usb_interface *interface)
 	au0828_usb_release(dev);
 }
 
-static void au0828_media_device_init(struct au0828_dev *dev,
-				     struct usb_device *udev)
+static int au0828_media_device_init(struct au0828_dev *dev,
+				    struct usb_device *udev)
 {
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_device *mdev;
+	int ret;
 
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
 	if (!mdev)
-		return;
+		return -ENOMEM;
 
 	mdev->dev = &udev->dev;
 

@@ -239,10 +240,18 @@ static void au0828_media_device_init(struct au0828_dev *dev,
 	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
 	mdev->driver_version = LINUX_VERSION_CODE;
 
-	media_device_init(mdev);
+	ret = media_device_init(mdev);
+	if (ret) {
+		pr_err(
+			"Couldn't create a media device. Error: %d\n",
+			ret);
+		kfree(mdev);
+		return ret;
+	}
 
 	dev->media_dev = mdev;
 #endif
+	return 0;
 }
 
 

@@ -368,7 +377,14 @@ static int au0828_usb_probe(struct usb_interface *interface,
 	dev->board = au0828_boards[dev->boardnr];
 
 	/* Initialize the media controller */
-	au0828_media_device_init(dev, usbdev);
+	retval = au0828_media_device_init(dev, usbdev);
+	if (retval) {
+		pr_err("%s() au0828_media_device_init failed\n",
+		       __func__);
+		mutex_unlock(&dev->lock);
+		kfree(dev);
+		return retval;
+	}
 
 #ifdef CONFIG_VIDEO_AU0828_V4L2
 	dev->v4l2_dev.release = au0828_usb_v4l2_release;
drivers/media/usb/cx231xx/cx231xx-cards.c +18 -5
index 2e997f09a4cd..35692d19b652 100644
@@ -1206,15 +1206,16 @@ void cx231xx_release_resources(struct cx231xx *dev)
 	clear_bit(dev->devno, &cx231xx_devused);
 }
 
-static void cx231xx_media_device_init(struct cx231xx *dev,
-				      struct usb_device *udev)
+static int cx231xx_media_device_init(struct cx231xx *dev,
+				     struct usb_device *udev)
 {
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_device *mdev;
+	int ret;
 
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
 	if (!mdev)
-		return;
+		return -ENOMEM;
 
 	mdev->dev = dev->dev;
 	strlcpy(mdev->model, dev->board.name, sizeof(mdev->model));

@@ -1224,10 +1225,18 @@ static void cx231xx_media_device_init(struct cx231xx *dev,
 	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
 	mdev->driver_version = LINUX_VERSION_CODE;
 
-	media_device_init(mdev);
+	ret = media_device_init(mdev);
+	if (ret) {
+		dev_err(dev->dev,
+			"Couldn't create a media device. Error: %d\n",
+			ret);
+		kfree(mdev);
+		return ret;
+	}
 
 	dev->media_dev = mdev;
 #endif
+	return 0;
 }
 
 static int cx231xx_create_media_graph(struct cx231xx *dev)

@@ -1663,7 +1672,11 @@ static int cx231xx_usb_probe(struct usb_interface *interface,
 	usb_set_intfdata(interface, dev);
 
 	/* Initialize the media controller */
-	cx231xx_media_device_init(dev, udev);
+	retval = cx231xx_media_device_init(dev, udev);
+	if (retval) {
+		dev_err(d, "cx231xx_media_device_init failed\n");
+		goto err_v4l2;
+	}
 
 	/* Create v4l2 device */
 #ifdef CONFIG_MEDIA_CONTROLLER
drivers/media/usb/dvb-usb-v2/dvb_usb_core.c +24 -9
index a0413dcc3361..9992ac6e63cc 100644
@@ -400,7 +400,7 @@ skip_feed_stop:
 	return ret;
 }
 
-static void dvb_usbv2_media_device_init(struct dvb_usb_adapter *adap)
+static int dvb_usbv2_media_device_init(struct dvb_usb_adapter *adap)
 {
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
 	struct media_device *mdev;

@@ -410,7 +410,7 @@ static void dvb_usbv2_media_device_init(struct dvb_usb_adapter *adap)
 
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
 	if (!mdev)
-		return;
+		return -ENOMEM;
 
 	mdev->dev = &udev->dev;
 	strlcpy(mdev->model, d->name, sizeof(mdev->model));

@@ -420,19 +420,28 @@ static void dvb_usbv2_media_device_init(struct dvb_usb_adapter *adap)
 	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
 	mdev->driver_version = LINUX_VERSION_CODE;
 
-	media_device_init(mdev);
+	ret = media_device_init(mdev);
+	if (ret) {
+		dev_err(&d->udev->dev,
+			"Couldn't create a media device. Error: %d\n",
+			ret);
+		kfree(mdev);
+		return ret;
+	}
 
 	dvb_register_media_controller(&adap->dvb_adap, mdev);
 
 	dev_info(&d->udev->dev, "media controller created\n");
-
 #endif
+	return 0;
 }
 
-static void dvb_usbv2_media_device_register(struct dvb_usb_adapter *adap)
+static int dvb_usbv2_media_device_register(struct dvb_usb_adapter *adap)
 {
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-	media_device_register(adap->dvb_adap.mdev);
+	return media_device_register(adap->dvb_adap.mdev);
+#else
+	return 0;
 #endif
 }
 

@@ -468,7 +477,12 @@ static int dvb_usbv2_adapter_dvb_init(struct dvb_usb_adapter *adap)
 
 	adap->dvb_adap.priv = adap;
 
-	dvb_usbv2_media_device_init(adap);
+	ret = dvb_usbv2_media_device_init(adap);
+	if (ret < 0) {
+		dev_dbg(&d->udev->dev, "%s: dvb_usbv2_media_device_init() failed=%d\n",
+				__func__, ret);
+		goto err_dvb_register_mc;
+	}
 
 	if (d->props->read_mac_address) {
 		ret = d->props->read_mac_address(adap,

@@ -519,6 +533,7 @@ err_dvb_dmxdev_init:
 	dvb_dmx_release(&adap->demux);
 err_dvb_dmx_init:
 	dvb_usbv2_media_device_unregister(adap);
+err_dvb_register_mc:
 	dvb_unregister_adapter(&adap->dvb_adap);
 err_dvb_register_adapter:
 	adap->dvb_adap.priv = NULL;

@@ -703,9 +718,9 @@ static int dvb_usbv2_adapter_frontend_init(struct dvb_usb_adapter *adap)
 	if (ret < 0)
 		goto err_dvb_unregister_frontend;
 
-	dvb_usbv2_media_device_register(&adap->dvb_adap);
+	ret = dvb_usbv2_media_device_register(adap);
 
-	return 0;
+	return ret;
 
 err_dvb_unregister_frontend:
 	for (i = count_registered - 1; i >= 0; i--)
drivers/media/usb/dvb-usb/dvb-usb-dvb.c +24 -8
index 4086d9626664..920396f935ab 100644
@@ -95,7 +95,7 @@ static int dvb_usb_stop_feed(struct dvb_demux_feed *dvbdmxfeed)
 	return dvb_usb_ctrl_feed(dvbdmxfeed, 0);
 }
 
-static void dvb_usb_media_device_init(struct dvb_usb_adapter *adap)
+static int dvb_usb_media_device_init(struct dvb_usb_adapter *adap)
 {
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
 	struct media_device *mdev;

@@ -105,7 +105,7 @@ static void dvb_usb_media_device_init(struct dvb_usb_adapter *adap)
 
 	mdev = kzalloc(sizeof(*mdev), GFP_KERNEL);
 	if (!mdev)
-		return;
+		return -EINVAL;
 
 	mdev->dev = &udev->dev;
 	strlcpy(mdev->model, d->desc->name, sizeof(mdev->model));

@@ -115,18 +115,27 @@ static void dvb_usb_media_device_init(struct dvb_usb_adapter *adap)
 	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
 	mdev->driver_version = LINUX_VERSION_CODE;
 
-	media_device_init(mdev);
-
+	ret = media_device_init(mdev);
+	if (ret) {
+		dev_err(&d->udev->dev,
+			"Couldn't create a media device. Error: %d\n",
+			ret);
+		kfree(mdev);
+		return ret;
+	}
 	dvb_register_media_controller(&adap->dvb_adap, mdev);
 
 	dev_info(&d->udev->dev, "media controller created\n");
 #endif
+	return 0;
 }
 
-static void dvb_usb_media_device_register(struct dvb_usb_adapter *adap)
+static int dvb_usb_media_device_register(struct dvb_usb_adapter *adap)
 {
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB
-	media_device_register(adap->dvb_adap.mdev);
+	return media_device_register(adap->dvb_adap.mdev);
+#else
+	return 0;
 #endif
 }
 

@@ -156,7 +165,11 @@ int dvb_usb_adapter_dvb_init(struct dvb_usb_adapter *adap, short *adapter_nums)
 	}
 	adap->dvb_adap.priv = adap;
 
-	dvb_usb_media_device_init(adap);
+	ret = dvb_usb_media_device_init(adap);
+	if (ret < 0) {
+		deb_info("dvb_usb_media_device_init failed: error %d", ret);
+		goto err_mc;
+	}
 
 	if (adap->dev->props.read_mac_address) {
 		if (adap->dev->props.read_mac_address(adap->dev, adap->dvb_adap.proposed_mac) == 0)

@@ -206,6 +219,7 @@ err_dmx_dev:
 	dvb_dmx_release(&adap->demux);
 err_dmx:
 	dvb_usb_media_device_unregister(adap);
+err_mc:
 	dvb_unregister_adapter(&adap->dvb_adap);
 err:
 	return ret;

@@ -324,8 +338,10 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap)
 		return ret;
 
 	ret = dvb_create_media_graph(&adap->dvb_adap);
+	if (ret)
+		return ret;
 
-	dvb_usb_media_device_register(adap);
+	ret = dvb_usb_media_device_register(adap);
 
 	return ret;
 }
drivers/media/usb/siano/smsusb.c +5 -1
index 8abbd3cc8eba..327180efa264 100644
@@ -361,7 +361,11 @@ static void *siano_media_device_register(struct smsusb_device_t *dev,
 	mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
 	mdev->driver_version = LINUX_VERSION_CODE;
 
-	media_device_init(mdev);
+	ret = media_device_init(mdev);
+	if (ret) {
+		kfree(mdev);
+		return NULL;
+	}
 
 	ret = media_device_register(mdev);
 	if (ret) {
drivers/media/usb/uvc/uvc_driver.c +2 -1
index 96fc2c2efa7b..4dfd3eb814e7 100644
@@ -1917,7 +1917,8 @@ static int uvc_probe(struct usb_interface *intf,
 	strcpy(dev->mdev.bus_info, udev->devpath);
 	dev->mdev.hw_revision = le16_to_cpu(udev->descriptor.bcdDevice);
 	dev->mdev.driver_version = LINUX_VERSION_CODE;
-	media_device_init(&dev->mdev);
+	if (media_device_init(&dev->mdev) < 0)
+		goto error;
 
 	dev->vdev.mdev = &dev->mdev;
 #endif
include/media/media-device.h +1 -1
index 572190860cbb..f09be5d47a7d 100644
@@ -351,7 +351,7 @@ struct media_device {
  * within the media device, create pad to pad links and then finally register
  * the media device by calling media_device_register() as a final step.
  */
-void media_device_init(struct media_device *mdev);
+int __must_check media_device_init(struct media_device *mdev);
 
 /**
  * media_device_cleanup() - Cleanups a media device element
Signature · 1 line
2.5.0
#1 lore raw

PATCH v2 1/1 media-device: handle errors at media_device_init()

Hello Mauro,

Quoted 6 lines · 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.

Quoted 7 lines

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 <[email protected]>

---

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 <[email protected]>

On an OMAP3 IGEPv2:

Tested-by Javier Martinez Canillas <[email protected]>

Best regards,

Signature · 3 lines
Javier Martinez Canillas
Open Source Group
Samsung Research America