Hi Mauro, Mauro Carvalho Chehab wrote: > Em Fri, 11 Sep 2015 09:31:36 +0200 > Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> >>>>>> Signed-off-by: Javier Martinez Canillas <javier@xxxxxxxxxxxxxxx> >>>>>> >>>>>> --- >>>>>> >>>>>> 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()). -- Regards, Sakari Ailus sakari.ailus@xxxxxxxxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
