Re: [PATCH 2/2] [media] media-device: split media initialization and registration
From: Sakari Ailus
Date: Fri Sep 11 2015 - 05:28:53 EST
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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/