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

[Mauro Carvalho Chehab]

Em Mon, 28 Dec 2015 03:14:53 +0200
Sakari Ailus  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  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 
> > > > Signed-off-by: Javier Martinez Canillas 
> > > > 
> > > > ---
> > > > 
> > > >  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
> 

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

[Mauro Carvalho Chehab]

Em Mon, 28 Dec 2015 08:26:33 -0200
Mauro Carvalho Chehab  escreveu:

> Em Mon, 28 Dec 2015 03:14:53 +0200
> Sakari Ailus  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  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 
> > > > > Signed-off-by: Javier Martinez Canillas 
> > > > > 
> > > > > ---
> > > > > 
> > > > >  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, 

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

[Sakari Ailus]

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  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 
> > > Signed-off-by: Javier Martinez Canillas 
> > > 
> > > ---
> > > 
> > >  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 

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

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mauro Carvalho Chehab]

Em Thu, 10 Sep 2015 20:14:04 +0300
Sakari Ailus  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 
> > Signed-off-by: Javier Martinez Canillas 
> > 
> > ---
> > 
> >  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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Javier Martinez Canillas]

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 
 Signed-off-by: Javier Martinez Canillas 

 ---

   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 

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

[Mauro Carvalho Chehab]

Em Fri, 11 Sep 2015 09:31:36 +0200
Javier Martinez Canillas  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 
>  Signed-off-by: Javier Martinez Canillas 
> 
>  ---
> 
>    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 

Re: [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  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 
>> Signed-off-by: Javier Martinez Canillas 
>>
>> ---
>>
>>   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 

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

[Javier Martinez Canillas]

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 
Signed-off-by: Javier Martinez Canillas 

---

 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)
 {
-   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(>lock);
mutex_init(>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(>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 = _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(>v4l2_dev);
 unlock:
mutex_unlock(>media_dev.graph_mutex);
-   return ret;
+   if (ret < 0)
+   return ret;
+
+   return media_device_register(>media_dev);
 }
 
 static int fimc_md_probe(struct platform_device *pdev)
@@ 

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

[Sakari Ailus]

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 
> Signed-off-by: Javier Martinez Canillas 
> 
> ---
> 
>  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(>lock);
>   mutex_init(>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(>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 = _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
> --- 

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

[Sakari Ailus]

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()?

-- 
Sakari Ailus
sakari.ai...@linux.intel.com
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Javier Martinez Canillas]

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 
>> Signed-off-by: Javier Martinez Canillas 
>>
>> ---
>>
>>  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 

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

[Javier Martinez Canillas]

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,
-- 
Javier Martinez Canillas
Open Source Group
Samsung Research America
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Sakari Ailus]

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 
Signed-off-by: Javier Martinez Canillas 

---

  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