| Message ID | 1441890195-11650-3-git-send-email-javier@osg.samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Javier, Thanks for the set! A few comments below. 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@linux.intel.com> > Signed-off-by: Javier Martinez Canillas <javier@osg.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. I'd simply replace the WARN_ON() below with BUG(). > { > - int ret; > - > if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0)) > return -EINVAL; > > @@ -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. I don't like the idea but I guess this could be solved later on. > } > > 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. > + 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). > + if (ret < 0) > + dev_err(xdev->dev, "failed to register media device\n"); > + > return ret; > } > > @@ -571,6 +575,7 @@ static void xvip_composite_v4l2_cleanup(struct xvip_composite_device *xdev) > { > v4l2_device_unregister(&xdev->v4l2_dev); > media_device_unregister(&xdev->media_dev); > + media_device_cleanup(&xdev->media_dev); > } > > static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev) > @@ -582,9 +587,9 @@ static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev) > sizeof(xdev->media_dev.model)); > xdev->media_dev.hw_revision = 0; > > - ret = media_device_register(&xdev->media_dev); > + ret = media_device_init(&xdev->media_dev); > if (ret < 0) { > - dev_err(xdev->dev, "media device registration failed (%d)\n", > + dev_err(xdev->dev, "media device init failed (%d)\n", > ret); > return ret; > } > @@ -594,7 +599,7 @@ static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev) > if (ret < 0) { > dev_err(xdev->dev, "V4L2 device registration failed (%d)\n", > ret); > - media_device_unregister(&xdev->media_dev); > + media_device_cleanup(&xdev->media_dev); > return ret; > } > > diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c > index 1c12285428fe..c3d7788c2796 100644 > --- a/drivers/media/usb/au0828/au0828-core.c > +++ b/drivers/media/usb/au0828/au0828-core.c > @@ -136,6 +136,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) > #ifdef CONFIG_MEDIA_CONTROLLER > if (dev->media_dev) { > media_device_unregister(dev->media_dev); > + media_device_cleanup(dev->media_dev); > kfree(dev->media_dev); > dev->media_dev = NULL; > } > @@ -214,8 +215,8 @@ static void au0828_usb_disconnect(struct usb_interface *interface) > au0828_usb_release(dev); > } > > -static void au0828_media_device_register(struct au0828_dev *dev, > - struct usb_device *udev) > +static void au0828_media_device_init(struct au0828_dev *dev, > + struct usb_device *udev) > { > #ifdef CONFIG_MEDIA_CONTROLLER > struct media_device *mdev; > @@ -237,10 +238,10 @@ static void au0828_media_device_register(struct au0828_dev *dev, > mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); > mdev->driver_version = LINUX_VERSION_CODE; > > - ret = media_device_register(mdev); > + ret = media_device_init(mdev); > if (ret) { > pr_err( > - "Couldn't create a media device. Error: %d\n", > + "Couldn't initialize a media device. Error: %d\n", > ret); > kfree(mdev); > return; > @@ -372,8 +373,8 @@ static int au0828_usb_probe(struct usb_interface *interface, > dev->boardnr = id->driver_info; > dev->board = au0828_boards[dev->boardnr]; > > - /* Register the media controller */ > - au0828_media_device_register(dev, usbdev); > + /* Initialize the media controller */ > + au0828_media_device_init(dev, usbdev); > > #ifdef CONFIG_VIDEO_AU0828_V4L2 > dev->v4l2_dev.release = au0828_usb_v4l2_release; > @@ -444,9 +445,20 @@ static int au0828_usb_probe(struct usb_interface *interface, > if (retval) { > pr_err("%s() au0282_dev_register failed to create graph\n", > __func__); > - au0828_usb_disconnect(interface); > + goto err; > } > > + retval = media_device_register(dev->media_dev); > + if (retval < 0) { > + pr_err("%s() au0282_dev_register failed to register media dev\n", > + __func__); > + goto err; > + } > + > + return 0; > + > +err: > + au0828_usb_disconnect(interface); > return retval; > } > > diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c b/drivers/media/usb/cx231xx/cx231xx-cards.c > index 022b30099a89..fee6dfa9e73e 100644 > --- a/drivers/media/usb/cx231xx/cx231xx-cards.c > +++ b/drivers/media/usb/cx231xx/cx231xx-cards.c > @@ -1172,6 +1172,7 @@ static void cx231xx_unregister_media_device(struct cx231xx *dev) > #ifdef CONFIG_MEDIA_CONTROLLER > if (dev->media_dev) { > media_device_unregister(dev->media_dev); > + media_device_cleanup(dev->media_dev); > kfree(dev->media_dev); > dev->media_dev = NULL; > } > @@ -1205,8 +1206,8 @@ void cx231xx_release_resources(struct cx231xx *dev) > clear_bit(dev->devno, &cx231xx_devused); > } > > -static void cx231xx_media_device_register(struct cx231xx *dev, > - struct usb_device *udev) > +static void cx231xx_media_device_init(struct cx231xx *dev, > + struct usb_device *udev) > { > #ifdef CONFIG_MEDIA_CONTROLLER > struct media_device *mdev; > @@ -1224,10 +1225,10 @@ static void cx231xx_media_device_register(struct cx231xx *dev, > mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); > mdev->driver_version = LINUX_VERSION_CODE; > > - ret = media_device_register(mdev); > + ret = media_device_init(mdev); > if (ret) { > dev_err(dev->dev, > - "Couldn't create a media device. Error: %d\n", > + "Couldn't initialize a media device. Error: %d\n", > ret); > kfree(mdev); > return; > @@ -1669,8 +1670,8 @@ static int cx231xx_usb_probe(struct usb_interface *interface, > /* save our data pointer in this interface device */ > usb_set_intfdata(interface, dev); > > - /* Register the media controller */ > - cx231xx_media_device_register(dev, udev); > + /* Initialize the media controller */ > + cx231xx_media_device_init(dev, udev); > > /* Create v4l2 device */ > #ifdef CONFIG_MEDIA_CONTROLLER > @@ -1744,9 +1745,18 @@ static int cx231xx_usb_probe(struct usb_interface *interface, > retval = cx231xx_create_media_graph(dev); > if (retval < 0) { > cx231xx_release_resources(dev); > + return retval; > + } > + > + retval = media_device_register(dev->media_dev); > + if (retval < 0) { > + dev_err(dev->dev, "Couldn't register media device. Error: %d\n", > + retval); > + cx231xx_release_resources(dev); > } > > return retval; > + > err_video_alt: > /* cx231xx_uninit_dev: */ > cx231xx_close_extension(dev); > diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c > index 6d3f61f6dc77..13a6e5a238db 100644 > --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c > +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c > @@ -400,7 +400,7 @@ skip_feed_stop: > return ret; > } > > -static void dvb_usbv2_media_device_register(struct dvb_usb_adapter *adap) > +static void dvb_usbv2_media_device_init(struct dvb_usb_adapter *adap) > { > #ifdef CONFIG_MEDIA_CONTROLLER_DVB > struct media_device *mdev; > @@ -420,7 +420,7 @@ static void dvb_usbv2_media_device_register(struct dvb_usb_adapter *adap) > mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); > mdev->driver_version = LINUX_VERSION_CODE; > > - ret = media_device_register(mdev); > + ret = media_device_init(mdev); > if (ret) { > dev_err(&d->udev->dev, > "Couldn't create a media device. Error: %d\n", > @@ -444,6 +444,7 @@ static void dvb_usbv2_media_device_unregister(struct dvb_usb_adapter *adap) > return; > > media_device_unregister(adap->dvb_adap.mdev); > + media_device_cleanup(adap->dvb_adap.mdev); > kfree(adap->dvb_adap.mdev); > adap->dvb_adap.mdev = NULL; > > @@ -467,7 +468,7 @@ static int dvb_usbv2_adapter_dvb_init(struct dvb_usb_adapter *adap) > > adap->dvb_adap.priv = adap; > > - dvb_usbv2_media_device_register(adap); > + dvb_usbv2_media_device_init(adap); > > if (d->props->read_mac_address) { > ret = d->props->read_mac_address(adap, > @@ -702,6 +703,10 @@ static int dvb_usbv2_adapter_frontend_init(struct dvb_usb_adapter *adap) > if (ret < 0) > goto err_dvb_unregister_frontend; > > + ret = media_device_register(adap->dvb_adap.mdev); > + if (ret) > + goto err_dvb_unregister_frontend; > + > return 0; > > err_dvb_unregister_frontend: > diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c > index b51dbdf03f42..b975fc1ea798 100644 > --- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c > +++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c > @@ -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_register(struct dvb_usb_adapter *adap) > +static void dvb_usb_media_device_init(struct dvb_usb_adapter *adap) > { > #ifdef CONFIG_MEDIA_CONTROLLER_DVB > struct media_device *mdev; > @@ -115,7 +115,7 @@ static void dvb_usb_media_device_register(struct dvb_usb_adapter *adap) > mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); > mdev->driver_version = LINUX_VERSION_CODE; > > - ret = media_device_register(mdev); > + ret = media_device_init(mdev); > if (ret) { > dev_err(&d->udev->dev, > "Couldn't create a media device. Error: %d\n", > @@ -136,6 +136,7 @@ static void dvb_usb_media_device_unregister(struct dvb_usb_adapter *adap) > return; > > media_device_unregister(adap->dvb_adap.mdev); > + media_device_cleanup(adap->dvb_adap.mdev); > kfree(adap->dvb_adap.mdev); > adap->dvb_adap.mdev = NULL; > #endif > @@ -154,7 +155,7 @@ int dvb_usb_adapter_dvb_init(struct dvb_usb_adapter *adap, short *adapter_nums) > } > adap->dvb_adap.priv = adap; > > - dvb_usb_media_device_register(adap); > + dvb_usb_media_device_init(adap); > > if (adap->dev->props.read_mac_address) { > if (adap->dev->props.read_mac_address(adap->dev, adap->dvb_adap.proposed_mac) == 0) > @@ -322,6 +323,12 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap) > return ret; > > ret = dvb_create_media_graph(&adap->dvb_adap); > + if (ret < 0) > + return ret; > + > + ret = media_device_register(adap->dvb_adap.mdev); > + if (ret < 0) > + return ret; > > return ret; > } > diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c > index c945e4c2fbd4..a451793d7c46 100644 > --- a/drivers/media/usb/siano/smsusb.c > +++ b/drivers/media/usb/siano/smsusb.c > @@ -361,17 +361,27 @@ 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; > > + ret = media_device_init(mdev); > + if (ret < 0) { > + pr_err("Couldn't initialize a media device. Error: %d\n", > + ret); > + goto err; > + } > + > ret = media_device_register(mdev); > if (ret) { > pr_err("Couldn't create a media device. Error: %d\n", > ret); > - kfree(mdev); > - return NULL; > + media_device_cleanup(mdev); > + goto err; > } > > pr_info("media controller created\n"); > > return mdev; > +err: > + kfree(mdev); > + return NULL; > #else > return NULL; > #endif > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c > index 4b5b3e8fb7d3..27d111102626 100644 > --- a/drivers/media/usb/uvc/uvc_driver.c > +++ b/drivers/media/usb/uvc/uvc_driver.c > @@ -1655,6 +1655,7 @@ static void uvc_delete(struct uvc_device *dev) > #ifdef CONFIG_MEDIA_CONTROLLER > if (media_devnode_is_registered(&dev->mdev.devnode)) > media_device_unregister(&dev->mdev); > + media_device_cleanup(&dev->mdev); > #endif > > list_for_each_safe(p, n, &dev->chains) { > @@ -1905,7 +1906,7 @@ static int uvc_probe(struct usb_interface *intf, > "linux-uvc-devel mailing list.\n"); > } > > - /* Register the media and V4L2 devices. */ > + /* Initialize the media device and register the V4L2 device. */ > #ifdef CONFIG_MEDIA_CONTROLLER > dev->mdev.dev = &intf->dev; > strlcpy(dev->mdev.model, dev->name, sizeof(dev->mdev.model)); > @@ -1915,7 +1916,7 @@ 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; > - if (media_device_register(&dev->mdev) < 0) > + if (media_device_init(&dev->mdev) < 0) > goto error; > > dev->vdev.mdev = &dev->mdev; > @@ -1935,6 +1936,10 @@ static int uvc_probe(struct usb_interface *intf, > if (uvc_register_chains(dev) < 0) > goto error; > > + /* Register the media device node */ > + if (media_device_register(&dev->mdev) < 0) > + goto error; > + > /* Save our data pointer in the interface data. */ > usb_set_intfdata(intf, dev); > > diff --git a/include/media/media-device.h b/include/media/media-device.h > index 1b12774a9ab4..8d5d9881404d 100644 > --- a/include/media/media-device.h > +++ b/include/media/media-device.h > @@ -106,6 +106,8 @@ struct media_device { > /* media_devnode to media_device */ > #define to_media_device(node) container_of(node, struct media_device, devnode) > > +int __must_check media_device_init(struct media_device *mdev); > +void media_device_cleanup(struct media_device *mdev); > int __must_check __media_device_register(struct media_device *mdev, > struct module *owner); > #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE) >
Javier Martinez Canillas wrote: > Also, add a media_entity_cleanup() function that will destroy the > graph_mutex that is initialized in media_entity_init(). media_device_init() and media_device_cleanup()?
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@linux.intel.com> >> Signed-off-by: Javier Martinez Canillas <javier@osg.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,
Hello Sakari, On 09/10/2015 07:39 PM, Sakari Ailus wrote: > Javier Martinez Canillas wrote: >> Also, add a media_entity_cleanup() function that will destroy the >> graph_mutex that is initialized in media_entity_init(). > > media_device_init() and media_device_cleanup()? > Right, sorry about that. I'll fix it. Thanks! Best regards,
Hi Javier, Javier Martinez Canillas wrote: > 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@linux.intel.com> >>> Signed-off-by: Javier Martinez Canillas <javier@osg.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 :) I thought we agreed, too. But my understanding was that the agreement was different. ;-) > > 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. Do we have such drivers? Would they ever have worked in the first place, as media device registration would have failed? > >> 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. I think it's also fine to use that for basic sanity checks on code paths that will be run early and every time. To support what I'm saying, just do this: $ grep BUG_ON drivers/media/v4l2-core/* Even though most of that is in videobuf, V4L2 core does that, too, and there's a case of especially delicious usage in v4l2_subdev_init(). :-) > > 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. I still maintain it makes no sense to build error handling in drivers if the only source of error would be bad parameters specified by a driver to a function. The driver could also pass a dangling pointer to the function as well, and you couldn't find out. > >>> { >>> - 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. As this patch essentially moves media_device_register() elsewhere and puts media_device_init() in its place, the driver error handling is mostly unaffected in that very location. I'm fine with the approach, although I don't think we'll need much auditing; such drivers would never have functioned in the first place. > >>> @@ -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. Agreed. >>> } >>> >>> 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. I'd be in favour of removing them. If the framework already complains about it, there's no reason to do the same in drivers.
Hello Sakari, On 09/11/2015 07:51 AM, Sakari Ailus wrote: > Hi Javier, > > Javier Martinez Canillas wrote: >> 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@linux.intel.com> >>>> Signed-off-by: Javier Martinez Canillas <javier@osg.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 :) > > I thought we agreed, too. But my understanding was that the agreement was different. ;-) > Fair enough :) >> >> 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. > > Do we have such drivers? Would they ever have worked in the first place, as media device registration would have failed? > Most likely we don't but since I'm changing all the drivers anyways, I'll take a look and change to void and propose a fix if I find something but it seems is just that the function is checking a condition that would not happen with the in-tree media drivers. I'll change to void and remove the return value check in drivers for v2. >> >>> 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. > > I think it's also fine to use that for basic sanity checks on code paths that will be run early and every time. > I still think that causing a panic when it could had be avoided is pretty harsh. And is not only me, even Linus thinks the same: https://lkml.org/lkml/2015/6/24/662 > To support what I'm saying, just do this: > > $ grep BUG_ON drivers/media/v4l2-core/* > > Even though most of that is in videobuf, V4L2 core does that, too, and there's a case of especially delicious usage in v4l2_subdev_init(). :-) > Yes, but I do wonder how many of those are really necessary and if most need to be converted to WARN_ON() and return an error instead... >> >> 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. > > I still maintain it makes no sense to build error handling in drivers if the only source of error would be bad parameters specified by a driver to a function. > > The driver could also pass a dangling pointer to the function as well, and you couldn't find out. You convinced me, then I think we should remove the check altogether (not keeping it and changing it to BUG_ON) do you agree with that? > >> >>>> { >>>> - 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. > > As this patch essentially moves media_device_register() elsewhere and puts media_device_init() in its place, the driver error handling is mostly unaffected in that very location. > > I'm fine with the approach, although I don't think we'll need much auditing; such drivers would never have functioned in the first place. > Yes, but the difference is that by checking and returning an error, such drivers that were not working (probably there even isn't such a driver) would had failed to probe but by removing the check, it could now cause a NULL pointer dereference. That's why we need to audit to be sure that there isn't a buggy one. >> >>>> @@ -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. > > Agreed. > >>>> } >>>> >>>> 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. > > I'd be in favour of removing them. If the framework already complains about it, there's no reason to do the same in drivers. > Ok, I'll remove the error messages from the drivers then on v2. Thanks a lot for your feedback! Best regards,
Em Fri, 11 Sep 2015 09:31:36 +0200 Javier Martinez Canillas <javier@osg.samsung.com> escreveu: > Hello Sakari, > > On 09/11/2015 07:51 AM, Sakari Ailus wrote: > > Hi Javier, > > > > Javier Martinez Canillas wrote: > >> 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@linux.intel.com> > >>>> Signed-off-by: Javier Martinez Canillas <javier@osg.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 :) > > > > I thought we agreed, too. But my understanding was that the agreement was different. ;-) > > > > Fair enough :) > > >> > >> 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. > > > > Do we have such drivers? Would they ever have worked in the first place, as media device registration would have failed? Actually we do. The media controller is an optional feature at the DVB only drivers (dvb-usb, dvb-usb-v2, siano), as it is used only to show the interfaces associated to them, and no functionality will be lost if it fails to register the MC (except for the enumeration). I don't see any reason why making it mandatory at those PC customer DVB drivers. If it fails... well, G_TOPOLOGY won't work, but all the rest will. For hybrid devices (au0828 and cx231xx), they're currently optional. I wrote a patch already on my WIP branch making it mandatory for au0828, provided that the MEDIA_CONTROLLER config option is enabled, and I intend to do the same for cx231xx. The rationale is that we'll use it to lock between analog and digital part, so it should be mandatory there. Regards, Mauro
Hi Mauro, Mauro Carvalho Chehab wrote: > Em Fri, 11 Sep 2015 09:31:36 +0200 > Javier Martinez Canillas <javier@osg.samsung.com> escreveu: > >> Hello Sakari, >> >> On 09/11/2015 07:51 AM, Sakari Ailus wrote: >>> Hi Javier, >>> >>> Javier Martinez Canillas wrote: >>>> 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@linux.intel.com> >>>>>> Signed-off-by: Javier Martinez Canillas <javier@osg.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 :) >>> >>> I thought we agreed, too. But my understanding was that the agreement was different. ;-) >>> >> >> Fair enough :) >> >>>> >>>> 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. >>> >>> Do we have such drivers? Would they ever have worked in the first place, as media device registration would have failed? > > Actually we do. The media controller is an optional feature at the DVB > only drivers (dvb-usb, dvb-usb-v2, siano), as it is used only to show > the interfaces associated to them, and no functionality will be lost > if it fails to register the MC (except for the enumeration). We're talking here about the arguments passed to the media_device_init() function, and the WARN_ON() check in the current media_device_register(). I can see that dvb-usb-dvb.c does not ensure the model is not an empty string. I wonder if it'd be better to drop that check entirely. The model field has no special meaning as far as I can tell. > > I don't see any reason why making it mandatory at those PC customer > DVB drivers. If it fails... well, G_TOPOLOGY won't work, but all > the rest will. I'm not for making it mandatory, but instead relying on drivers to use the API correctly in probe() (or face BUG_ON()).
Em Thu, 10 Sep 2015 20:14:04 +0300 Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > Hi Javier, > > Thanks for the set! A few comments below. > > 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@linux.intel.com> > > Signed-off-by: Javier Martinez Canillas <javier@osg.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. > > I'd simply replace the WARN_ON() below with BUG(). That sounds like bad idea to me, and it is against the current Kernel policy of using BUG() only when there's no other way, e. g. on event so severe that the Kernel has no other thing to do except to stop running. For sure, this is not the case here. Also, all drivers have already a logic that checks if the device init happened. So, they should already be doing the right thing. Regards, Mauro
Hello Mauro, On 12/15/2015 08:13 AM, Mauro Carvalho Chehab wrote: [snip] >>> >>> /** >>> - * 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. >> >> I'd simply replace the WARN_ON() below with BUG(). > > That sounds like bad idea to me, and it is against the current > Kernel policy of using BUG() only when there's no other way, e. g. on > event so severe that the Kernel has no other thing to do except to > stop running. > I agree with you, that was exactly my point and what I told Sakari [0] but he had a strong opinion about it and I didn't mind too much so I decided at the end to just change it to a BUG_ON() so I could get his ack for this set. > For sure, this is not the case here. Also, all drivers have already > a logic that checks if the device init happened. So, they should already > be doing the right thing. > > Regards, > Mauro [0]: https://lkml.org/lkml/2015/9/10/483 Best regards,
Hi Mauro, On Tue, Dec 15, 2015 at 09:13:42AM -0200, Mauro Carvalho Chehab wrote: > Em Thu, 10 Sep 2015 20:14:04 +0300 > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > Hi Javier, > > > > Thanks for the set! A few comments below. > > > > 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@linux.intel.com> > > > Signed-off-by: Javier Martinez Canillas <javier@osg.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. > > > > I'd simply replace the WARN_ON() below with BUG(). > > That sounds like bad idea to me, and it is against the current > Kernel policy of using BUG() only when there's no other way, e. g. on > event so severe that the Kernel has no other thing to do except to > stop running. > > For sure, this is not the case here. Also, all drivers have already > a logic that checks if the device init happened. So, they should already > be doing the right thing. My point is that it's simply counter-productive to require the caller to perform error handling in cases such as the only possible source of the error being a NULL argument passed to the callee. To give you some examples, device_register(), device_add() nor mutex_lock() perform such checks. Some functions in V4L2 do, but I understand that's sometimes for historical reasons where NULL arguments were allowed. Or that there are other possible sources for errors in non-trivial functions and the rest of the checks are done on the side. If you don't like BUG_ON(), just drop it. It's as simple as that. If there are other sources of errors then the matter is naturally entirely different.
Em Mon, 28 Dec 2015 03:14:53 +0200 Sakari Ailus <sakari.ailus@iki.fi> escreveu: > Hi Mauro, > > On Tue, Dec 15, 2015 at 09:13:42AM -0200, Mauro Carvalho Chehab wrote: > > Em Thu, 10 Sep 2015 20:14:04 +0300 > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > > > Hi Javier, > > > > > > Thanks for the set! A few comments below. > > > > > > 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@linux.intel.com> > > > > Signed-off-by: Javier Martinez Canillas <javier@osg.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. > > > > > > I'd simply replace the WARN_ON() below with BUG(). > > > > That sounds like bad idea to me, and it is against the current > > Kernel policy of using BUG() only when there's no other way, e. g. on > > event so severe that the Kernel has no other thing to do except to > > stop running. > > > > For sure, this is not the case here. Also, all drivers have already > > a logic that checks if the device init happened. So, they should already > > be doing the right thing. > > My point is that it's simply counter-productive to require the caller to > perform error handling in cases such as the only possible source of the > error being a NULL argument passed to the callee. > > To give you some examples, device_register(), device_add() nor mutex_lock() > perform such checks. Some functions in V4L2 do, but I understand that's > sometimes for historical reasons where NULL arguments were allowed. Or that > there are other possible sources for errors in non-trivial functions and the > rest of the checks are done on the side. > > If you don't like BUG_ON(), just drop it. It's as simple as that. Ah, I see your point. Yeah, dropping WARN_ON makes sense. I'll address it. > If there are other sources of errors then the matter is naturally entirely > different. > Regards, Mauro
Em Mon, 28 Dec 2015 08:26:33 -0200 Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu: > Em Mon, 28 Dec 2015 03:14:53 +0200 > Sakari Ailus <sakari.ailus@iki.fi> escreveu: > > > Hi Mauro, > > > > On Tue, Dec 15, 2015 at 09:13:42AM -0200, Mauro Carvalho Chehab wrote: > > > Em Thu, 10 Sep 2015 20:14:04 +0300 > > > Sakari Ailus <sakari.ailus@linux.intel.com> escreveu: > > > > > > > Hi Javier, > > > > > > > > Thanks for the set! A few comments below. > > > > > > > > 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@linux.intel.com> > > > > > Signed-off-by: Javier Martinez Canillas <javier@osg.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. > > > > > > > > I'd simply replace the WARN_ON() below with BUG(). > > > > > > That sounds like bad idea to me, and it is against the current > > > Kernel policy of using BUG() only when there's no other way, e. g. on > > > event so severe that the Kernel has no other thing to do except to > > > stop running. > > > > > > For sure, this is not the case here. Also, all drivers have already > > > a logic that checks if the device init happened. So, they should already > > > be doing the right thing. > > > > My point is that it's simply counter-productive to require the caller to > > perform error handling in cases such as the only possible source of the > > error being a NULL argument passed to the callee. > > > > To give you some examples, device_register(), device_add() nor mutex_lock() > > perform such checks. Some functions in V4L2 do, but I understand that's > > sometimes for historical reasons where NULL arguments were allowed. Or that > > there are other possible sources for errors in non-trivial functions and the > > rest of the checks are done on the side. > > > > If you don't like BUG_ON(), just drop it. It's as simple as that. > > Ah, I see your point. Yeah, dropping WARN_ON makes sense. I'll address > it. Ok, I dropped BUG_ON() from Javier's patch. I also fold with the 3 build fixups if !CONFIG_MEDIA_CONTROLLER, as requested by Laurent, plus one fixup from me (that I sent today to the ML), removing two uneeded vars and fixing one call for the DVB frontend that were passing the wrong struct. The final patch is at: https://git.linuxtv.org/mchehab/experimental.git/commit/?h=media-controller-rc6&id=337a643d4ac04c4c6681c9ae2b37d700c9a3fa5f There two new warnings after Javier's patch related to check if the device was properly registered, and if the memory allocation for the media_device has succeded. As such checks were actually unrelated to Javier's changes, I added them on a separate patch: https://git.linuxtv.org/mchehab/experimental.git/commit/?h=media-controller-rc6&id=180a506ef4021a68c061e19c11046978a42934f7 Regards, Mauro
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) { - int ret; - if (WARN_ON(mdev->dev == NULL || mdev->model[0] == 0)) return -EINVAL; @@ -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); +} +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); } 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); + 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); + if (ret < 0) + dev_err(xdev->dev, "failed to register media device\n"); + return ret; } @@ -571,6 +575,7 @@ static void xvip_composite_v4l2_cleanup(struct xvip_composite_device *xdev) { v4l2_device_unregister(&xdev->v4l2_dev); media_device_unregister(&xdev->media_dev); + media_device_cleanup(&xdev->media_dev); } static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev) @@ -582,9 +587,9 @@ static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev) sizeof(xdev->media_dev.model)); xdev->media_dev.hw_revision = 0; - ret = media_device_register(&xdev->media_dev); + ret = media_device_init(&xdev->media_dev); if (ret < 0) { - dev_err(xdev->dev, "media device registration failed (%d)\n", + dev_err(xdev->dev, "media device init failed (%d)\n", ret); return ret; } @@ -594,7 +599,7 @@ static int xvip_composite_v4l2_init(struct xvip_composite_device *xdev) if (ret < 0) { dev_err(xdev->dev, "V4L2 device registration failed (%d)\n", ret); - media_device_unregister(&xdev->media_dev); + media_device_cleanup(&xdev->media_dev); return ret; } diff --git a/drivers/media/usb/au0828/au0828-core.c b/drivers/media/usb/au0828/au0828-core.c index 1c12285428fe..c3d7788c2796 100644 --- a/drivers/media/usb/au0828/au0828-core.c +++ b/drivers/media/usb/au0828/au0828-core.c @@ -136,6 +136,7 @@ static void au0828_unregister_media_device(struct au0828_dev *dev) #ifdef CONFIG_MEDIA_CONTROLLER if (dev->media_dev) { media_device_unregister(dev->media_dev); + media_device_cleanup(dev->media_dev); kfree(dev->media_dev); dev->media_dev = NULL; } @@ -214,8 +215,8 @@ static void au0828_usb_disconnect(struct usb_interface *interface) au0828_usb_release(dev); } -static void au0828_media_device_register(struct au0828_dev *dev, - struct usb_device *udev) +static void au0828_media_device_init(struct au0828_dev *dev, + struct usb_device *udev) { #ifdef CONFIG_MEDIA_CONTROLLER struct media_device *mdev; @@ -237,10 +238,10 @@ static void au0828_media_device_register(struct au0828_dev *dev, mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); mdev->driver_version = LINUX_VERSION_CODE; - ret = media_device_register(mdev); + ret = media_device_init(mdev); if (ret) { pr_err( - "Couldn't create a media device. Error: %d\n", + "Couldn't initialize a media device. Error: %d\n", ret); kfree(mdev); return; @@ -372,8 +373,8 @@ static int au0828_usb_probe(struct usb_interface *interface, dev->boardnr = id->driver_info; dev->board = au0828_boards[dev->boardnr]; - /* Register the media controller */ - au0828_media_device_register(dev, usbdev); + /* Initialize the media controller */ + au0828_media_device_init(dev, usbdev); #ifdef CONFIG_VIDEO_AU0828_V4L2 dev->v4l2_dev.release = au0828_usb_v4l2_release; @@ -444,9 +445,20 @@ static int au0828_usb_probe(struct usb_interface *interface, if (retval) { pr_err("%s() au0282_dev_register failed to create graph\n", __func__); - au0828_usb_disconnect(interface); + goto err; } + retval = media_device_register(dev->media_dev); + if (retval < 0) { + pr_err("%s() au0282_dev_register failed to register media dev\n", + __func__); + goto err; + } + + return 0; + +err: + au0828_usb_disconnect(interface); return retval; } diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c b/drivers/media/usb/cx231xx/cx231xx-cards.c index 022b30099a89..fee6dfa9e73e 100644 --- a/drivers/media/usb/cx231xx/cx231xx-cards.c +++ b/drivers/media/usb/cx231xx/cx231xx-cards.c @@ -1172,6 +1172,7 @@ static void cx231xx_unregister_media_device(struct cx231xx *dev) #ifdef CONFIG_MEDIA_CONTROLLER if (dev->media_dev) { media_device_unregister(dev->media_dev); + media_device_cleanup(dev->media_dev); kfree(dev->media_dev); dev->media_dev = NULL; } @@ -1205,8 +1206,8 @@ void cx231xx_release_resources(struct cx231xx *dev) clear_bit(dev->devno, &cx231xx_devused); } -static void cx231xx_media_device_register(struct cx231xx *dev, - struct usb_device *udev) +static void cx231xx_media_device_init(struct cx231xx *dev, + struct usb_device *udev) { #ifdef CONFIG_MEDIA_CONTROLLER struct media_device *mdev; @@ -1224,10 +1225,10 @@ static void cx231xx_media_device_register(struct cx231xx *dev, mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); mdev->driver_version = LINUX_VERSION_CODE; - ret = media_device_register(mdev); + ret = media_device_init(mdev); if (ret) { dev_err(dev->dev, - "Couldn't create a media device. Error: %d\n", + "Couldn't initialize a media device. Error: %d\n", ret); kfree(mdev); return; @@ -1669,8 +1670,8 @@ static int cx231xx_usb_probe(struct usb_interface *interface, /* save our data pointer in this interface device */ usb_set_intfdata(interface, dev); - /* Register the media controller */ - cx231xx_media_device_register(dev, udev); + /* Initialize the media controller */ + cx231xx_media_device_init(dev, udev); /* Create v4l2 device */ #ifdef CONFIG_MEDIA_CONTROLLER @@ -1744,9 +1745,18 @@ static int cx231xx_usb_probe(struct usb_interface *interface, retval = cx231xx_create_media_graph(dev); if (retval < 0) { cx231xx_release_resources(dev); + return retval; + } + + retval = media_device_register(dev->media_dev); + if (retval < 0) { + dev_err(dev->dev, "Couldn't register media device. Error: %d\n", + retval); + cx231xx_release_resources(dev); } return retval; + err_video_alt: /* cx231xx_uninit_dev: */ cx231xx_close_extension(dev); diff --git a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c index 6d3f61f6dc77..13a6e5a238db 100644 --- a/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c +++ b/drivers/media/usb/dvb-usb-v2/dvb_usb_core.c @@ -400,7 +400,7 @@ skip_feed_stop: return ret; } -static void dvb_usbv2_media_device_register(struct dvb_usb_adapter *adap) +static void dvb_usbv2_media_device_init(struct dvb_usb_adapter *adap) { #ifdef CONFIG_MEDIA_CONTROLLER_DVB struct media_device *mdev; @@ -420,7 +420,7 @@ static void dvb_usbv2_media_device_register(struct dvb_usb_adapter *adap) mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); mdev->driver_version = LINUX_VERSION_CODE; - ret = media_device_register(mdev); + ret = media_device_init(mdev); if (ret) { dev_err(&d->udev->dev, "Couldn't create a media device. Error: %d\n", @@ -444,6 +444,7 @@ static void dvb_usbv2_media_device_unregister(struct dvb_usb_adapter *adap) return; media_device_unregister(adap->dvb_adap.mdev); + media_device_cleanup(adap->dvb_adap.mdev); kfree(adap->dvb_adap.mdev); adap->dvb_adap.mdev = NULL; @@ -467,7 +468,7 @@ static int dvb_usbv2_adapter_dvb_init(struct dvb_usb_adapter *adap) adap->dvb_adap.priv = adap; - dvb_usbv2_media_device_register(adap); + dvb_usbv2_media_device_init(adap); if (d->props->read_mac_address) { ret = d->props->read_mac_address(adap, @@ -702,6 +703,10 @@ static int dvb_usbv2_adapter_frontend_init(struct dvb_usb_adapter *adap) if (ret < 0) goto err_dvb_unregister_frontend; + ret = media_device_register(adap->dvb_adap.mdev); + if (ret) + goto err_dvb_unregister_frontend; + return 0; err_dvb_unregister_frontend: diff --git a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c index b51dbdf03f42..b975fc1ea798 100644 --- a/drivers/media/usb/dvb-usb/dvb-usb-dvb.c +++ b/drivers/media/usb/dvb-usb/dvb-usb-dvb.c @@ -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_register(struct dvb_usb_adapter *adap) +static void dvb_usb_media_device_init(struct dvb_usb_adapter *adap) { #ifdef CONFIG_MEDIA_CONTROLLER_DVB struct media_device *mdev; @@ -115,7 +115,7 @@ static void dvb_usb_media_device_register(struct dvb_usb_adapter *adap) mdev->hw_revision = le16_to_cpu(udev->descriptor.bcdDevice); mdev->driver_version = LINUX_VERSION_CODE; - ret = media_device_register(mdev); + ret = media_device_init(mdev); if (ret) { dev_err(&d->udev->dev, "Couldn't create a media device. Error: %d\n", @@ -136,6 +136,7 @@ static void dvb_usb_media_device_unregister(struct dvb_usb_adapter *adap) return; media_device_unregister(adap->dvb_adap.mdev); + media_device_cleanup(adap->dvb_adap.mdev); kfree(adap->dvb_adap.mdev); adap->dvb_adap.mdev = NULL; #endif @@ -154,7 +155,7 @@ int dvb_usb_adapter_dvb_init(struct dvb_usb_adapter *adap, short *adapter_nums) } adap->dvb_adap.priv = adap; - dvb_usb_media_device_register(adap); + dvb_usb_media_device_init(adap); if (adap->dev->props.read_mac_address) { if (adap->dev->props.read_mac_address(adap->dev, adap->dvb_adap.proposed_mac) == 0) @@ -322,6 +323,12 @@ int dvb_usb_adapter_frontend_init(struct dvb_usb_adapter *adap) return ret; ret = dvb_create_media_graph(&adap->dvb_adap); + if (ret < 0) + return ret; + + ret = media_device_register(adap->dvb_adap.mdev); + if (ret < 0) + return ret; return ret; } diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c index c945e4c2fbd4..a451793d7c46 100644 --- a/drivers/media/usb/siano/smsusb.c +++ b/drivers/media/usb/siano/smsusb.c @@ -361,17 +361,27 @@ 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; + ret = media_device_init(mdev); + if (ret < 0) { + pr_err("Couldn't initialize a media device. Error: %d\n", + ret); + goto err; + } + ret = media_device_register(mdev); if (ret) { pr_err("Couldn't create a media device. Error: %d\n", ret); - kfree(mdev); - return NULL; + media_device_cleanup(mdev); + goto err; } pr_info("media controller created\n"); return mdev; +err: + kfree(mdev); + return NULL; #else return NULL; #endif diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c index 4b5b3e8fb7d3..27d111102626 100644 --- a/drivers/media/usb/uvc/uvc_driver.c +++ b/drivers/media/usb/uvc/uvc_driver.c @@ -1655,6 +1655,7 @@ static void uvc_delete(struct uvc_device *dev) #ifdef CONFIG_MEDIA_CONTROLLER if (media_devnode_is_registered(&dev->mdev.devnode)) media_device_unregister(&dev->mdev); + media_device_cleanup(&dev->mdev); #endif list_for_each_safe(p, n, &dev->chains) { @@ -1905,7 +1906,7 @@ static int uvc_probe(struct usb_interface *intf, "linux-uvc-devel mailing list.\n"); } - /* Register the media and V4L2 devices. */ + /* Initialize the media device and register the V4L2 device. */ #ifdef CONFIG_MEDIA_CONTROLLER dev->mdev.dev = &intf->dev; strlcpy(dev->mdev.model, dev->name, sizeof(dev->mdev.model)); @@ -1915,7 +1916,7 @@ 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; - if (media_device_register(&dev->mdev) < 0) + if (media_device_init(&dev->mdev) < 0) goto error; dev->vdev.mdev = &dev->mdev; @@ -1935,6 +1936,10 @@ static int uvc_probe(struct usb_interface *intf, if (uvc_register_chains(dev) < 0) goto error; + /* Register the media device node */ + if (media_device_register(&dev->mdev) < 0) + goto error; + /* Save our data pointer in the interface data. */ usb_set_intfdata(intf, dev); diff --git a/include/media/media-device.h b/include/media/media-device.h index 1b12774a9ab4..8d5d9881404d 100644 --- a/include/media/media-device.h +++ b/include/media/media-device.h @@ -106,6 +106,8 @@ struct media_device { /* media_devnode to media_device */ #define to_media_device(node) container_of(node, struct media_device, devnode) +int __must_check media_device_init(struct media_device *mdev); +void media_device_cleanup(struct media_device *mdev); int __must_check __media_device_register(struct media_device *mdev, struct module *owner); #define media_device_register(mdev) __media_device_register(mdev, THIS_MODULE)
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@linux.intel.com> Signed-off-by: Javier Martinez Canillas <javier@osg.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(-)