[PATCH 2/2] [media] media-device: split media initialization and registration

[Sakari Ailus]

Hi Mauro,

Mauro Carvalho Chehab wrote:
> Em Fri, 11 Sep 2015 09:31:36 +0200
> Javier Martinez Canillas <javier at 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 at linux.intel.com>
>>>>>> Signed-off-by: Javier Martinez Canillas <javier at 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()).

-- 
Regards,

Sakari Ailus
sakari.ailus at linux.intel.com