Re: [PATCH] VIDEO: ivtvfb, remove unneeded NULL test

2010-07-05 Thread Jiri Slaby
On 07/04/2010 03:22 PM, Andy Walls wrote:
 On Sun, 2010-07-04 at 09:24 +0200, Jiri Slaby wrote:
 On 07/04/2010 06:11 AM, Andy Walls wrote:
 --- a/drivers/media/video/ivtv/ivtvfb.c
 +++ b/drivers/media/video/ivtv/ivtvfb.c
 @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv)
  static int __init ivtvfb_callback_init(struct device *dev, void *p)
  {
 struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
 -   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
 +   struct ivtv *itv;
  
 -   if (itv  (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT)) {
 +   if (v4l2_dev == NULL)
 +   return 0;
 +
 +   itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
 +
 +   if (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT) {
 if (ivtvfb_init_card(itv) == 0) {
 IVTVFB_INFO(Framebuffer registered on %s\n,
 itv-v4l2_dev.name);
 @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct 
 device *de
  static int ivtvfb_callback_cleanup(struct device *dev, void *p)
  {
 struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
 -   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
 -   struct osd_info *oi = itv-osd_info;
 +   struct ivtv *itv;
 +   struct osd_info *oi;
 +
 +   if (v4l2_dev == NULL)
 +   return 0;

 From my POV I NACK this. Given that it never triggered and drvdata are
 set in v4l2_device_register called from ivtv_probe I can't see how
 v4l2_dev be NULL. Could you elaborate?
 
 I hemmed and hawed over that too.  I didn't do a very thorough analysis
 of the restrictions on unloading modules, but here was my line of
 reasoning:
...
 2. Note that the ivtvfb driver is not automatically loaded nor unloaded
 by the kernel ivtv driver.  Something from userspace will reqeust the
 load and unload of the module.
 
 There are windows of time where a struct device * will exist for a card
 in the ivtv driver, but a struct v4l2_device * may not: the end of
 ivtv_remove() and the beginning of ivtv_probe().

If there is no locking or refcounting, this won't change with the added
check. The window is still there, but it begins after the check with
your patch. Hence will still cause oopses.

 I was contemplating a case where user space requested unloading both the
 ivtvfb and the ivtv driver.  Given all the I2C devices these cards can
 have, I thought v4l2_device_unregister() at the end of ivtv_remove()
 could present a window of time large enough to worry about a race.
 v4l2_device_unregister() sets the struct device  drvdata pointer to
 NULL, and then begins unregistering the i2c clients.  I haven't profiled
 the process to know how long it typically takes, though.
 
 I also don't know if kernel mechanisms will absolutely prevent
 initiating the unload of ivtv.ko before the unload of ivtvfb.ko is
 completely finished.  Will they?

Given ivtvfb uses functions from ivtv, no, it can't unload ivtv earlier
than ivtvfb.

regards,
-- 
js
suse labs
--
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] VIDEO: ivtvfb, remove unneeded NULL test

2010-07-05 Thread Andy Walls
On Mon, 2010-07-05 at 09:10 +0200, Jiri Slaby wrote:
 On 07/04/2010 03:22 PM, Andy Walls wrote:
  On Sun, 2010-07-04 at 09:24 +0200, Jiri Slaby wrote:
  On 07/04/2010 06:11 AM, Andy Walls wrote:

  There are windows of time where a struct device * will exist for a card
  in the ivtv driver, but a struct v4l2_device * may not: the end of
  ivtv_remove() and the beginning of ivtv_probe().
 
 If there is no locking or refcounting, this won't change with the added
 check. The window is still there, but it begins after the check with
 your patch. Hence will still cause oopses.

Jiri,

Of course, you're absolutley right.

Please resubmit a version of your original patch fixing both instances
of the check.  I'll add my ack and SOB.

If any users ever report an Oops, then I'll bother to add interlocking.

Regards,
Andy

--
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] VIDEO: ivtvfb, remove unneeded NULL test

2010-07-04 Thread Jiri Slaby
On 07/04/2010 06:11 AM, Andy Walls wrote:
 You missed an identical instance of the useless test 10 lines prior in
 ivtvfb_callback_init(). :)

Ah, thank you for pointing out. Find my comment below.

 --- a/drivers/media/video/ivtv/ivtvfb.c
 +++ b/drivers/media/video/ivtv/ivtvfb.c
 @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv)
  static int __init ivtvfb_callback_init(struct device *dev, void *p)
  {
 struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
 -   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
 +   struct ivtv *itv;
  
 -   if (itv  (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT)) {
 +   if (v4l2_dev == NULL)
 +   return 0;
 +
 +   itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
 +
 +   if (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT) {
 if (ivtvfb_init_card(itv) == 0) {
 IVTVFB_INFO(Framebuffer registered on %s\n,
 itv-v4l2_dev.name);
 @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct device 
 *de
  static int ivtvfb_callback_cleanup(struct device *dev, void *p)
  {
 struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
 -   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
 -   struct osd_info *oi = itv-osd_info;
 +   struct ivtv *itv;
 +   struct osd_info *oi;
 +
 +   if (v4l2_dev == NULL)
 +   return 0;

From my POV I NACK this. Given that it never triggered and drvdata are
set in v4l2_device_register called from ivtv_probe I can't see how
v4l2_dev be NULL. Could you elaborate?

-- 
js
suse labs
--
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] VIDEO: ivtvfb, remove unneeded NULL test

2010-07-04 Thread Andy Walls
On Sun, 2010-07-04 at 09:24 +0200, Jiri Slaby wrote:
 On 07/04/2010 06:11 AM, Andy Walls wrote:
  You missed an identical instance of the useless test 10 lines prior in
  ivtvfb_callback_init(). :)
 
 Ah, thank you for pointing out. Find my comment below.
 
  --- a/drivers/media/video/ivtv/ivtvfb.c
  +++ b/drivers/media/video/ivtv/ivtvfb.c
  @@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv)
   static int __init ivtvfb_callback_init(struct device *dev, void *p)
   {
  struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
  -   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
  +   struct ivtv *itv;
   
  -   if (itv  (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT)) {
  +   if (v4l2_dev == NULL)
  +   return 0;
  +
  +   itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
  +
  +   if (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT) {
  if (ivtvfb_init_card(itv) == 0) {
  IVTVFB_INFO(Framebuffer registered on %s\n,
  itv-v4l2_dev.name);
  @@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct 
  device *de
   static int ivtvfb_callback_cleanup(struct device *dev, void *p)
   {
  struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
  -   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
  -   struct osd_info *oi = itv-osd_info;
  +   struct ivtv *itv;
  +   struct osd_info *oi;
  +
  +   if (v4l2_dev == NULL)
  +   return 0;
 
 From my POV I NACK this. Given that it never triggered and drvdata are
 set in v4l2_device_register called from ivtv_probe I can't see how
 v4l2_dev be NULL. Could you elaborate?

I hemmed and hawed over that too.  I didn't do a very thorough analysis
of the restrictions on unloading modules, but here was my line of
reasoning:

1. I assumed the check was there because presumably someone has run into
an Ooops there before, prior to the conversion to the v4l2_device
infrastrucutre.

I have reasearched that with git log this morning, and found that
assumption was false.  The itv NULL check is a hold-over from when the
ivtvfb and ivtv were apparently more tightly coupled.  The itv pointer
was originally read from an array in ivtv that could have entries set to
NULL.  That array is long gone.



2. Note that the ivtvfb driver is not automatically loaded nor unloaded
by the kernel ivtv driver.  Something from userspace will reqeust the
load and unload of the module.

There are windows of time where a struct device * will exist for a card
in the ivtv driver, but a struct v4l2_device * may not: the end of
ivtv_remove() and the beginning of ivtv_probe().

I was contemplating a case where user space requested unloading both the
ivtvfb and the ivtv driver.  Given all the I2C devices these cards can
have, I thought v4l2_device_unregister() at the end of ivtv_remove()
could present a window of time large enough to worry about a race.
v4l2_device_unregister() sets the struct device  drvdata pointer to
NULL, and then begins unregistering the i2c clients.  I haven't profiled
the process to know how long it typically takes, though.

I also don't know if kernel mechanisms will absolutely prevent
initiating the unload of ivtv.ko before the unload of ivtvfb.ko is
completely finished.  Will they?

$ modinfo ivtvfb | grep -E '(depend)|(alias)'
depends:ivtv

$ modinfo ivtv | grep -E '(depend)|(alias)'
alias:  pci:vd0016sv*sd*bc*sc*i*
alias:  pci:vd0803sv*sd*bc*sc*i*
depends:cx2341x,videodev,tveeprom,v4l2-common,i2c-core,i2c-algo-bit

Regards,
Andy

--
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] VIDEO: ivtvfb, remove unneeded NULL test

2010-07-03 Thread Andy Walls
On Tue, 2010-06-22 at 13:41 +0200, Jiri Slaby wrote:
 Stanse found that in ivtvfb_callback_cleanup there is an unneeded test
 for itv being NULL. But itv is initialized as container_of with
 non-zero offset, so it is never NULL (even if v4l2_dev is). This was
 found because itv is dereferenced earlier than the test.
 
 Signed-off-by: Jiri Slaby jsl...@suse.cz
 Cc: Andy Walls awa...@md.metrocast.net
 Cc: Mauro Carvalho Chehab mche...@infradead.org
 Cc: Tejun Heo t...@kernel.org
 Cc: Ian Armstrong i...@iarmst.demon.co.uk
 Cc: ivtv-de...@ivtvdriver.org
 Cc: linux-media@vger.kernel.org
 ---
  drivers/media/video/ivtv/ivtvfb.c |2 +-
  1 files changed, 1 insertions(+), 1 deletions(-)
 
 diff --git a/drivers/media/video/ivtv/ivtvfb.c 
 b/drivers/media/video/ivtv/ivtvfb.c
 index 9ff3425..5dc460e 100644
 --- a/drivers/media/video/ivtv/ivtvfb.c
 +++ b/drivers/media/video/ivtv/ivtvfb.c
 @@ -1219,7 +1219,7 @@ static int ivtvfb_callback_cleanup(struct device *dev, 
 void *p)
   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
   struct osd_info *oi = itv-osd_info;
  
 - if (itv  (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT)) {
 + if (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT) {
   if (unregister_framebuffer(itv-osd_info-ivtvfb_info)) {
   IVTVFB_WARN(Framebuffer %d is in use, cannot unload\n,
  itv-instance);

Jiri,

You missed an identical instance of the useless test 10 lines prior in
ivtvfb_callback_init(). :)

How about the patch below, instead?

Regards,
Andy

[PATCH] VIDEO: ivtvfb, fix NULL check

Jiri Slaby reported that stanse found ivtvfb_callback_cleanup has an
unneeded test for itv being NULL. itv was initialized as container_of
with non-zero offset, so it was never NULL (even if v4l2_dev was).

This fix now checks for v4l2_dev being NULL, and not itv.

Thanks to Jiri Slaby for reporting this problem and providing an initial
patch.

Reported-by: Jiri Slaby jsl...@suse.cz
Signed-off-by: Andy Walls awa...@md.metrocast.net
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Tejun Heo t...@kernel.org
Cc: Ian Armstrong i...@iarmst.demon.co.uk
Cc: ivtv-de...@ivtvdriver.org
Cc: linux-media@vger.kernel.org


 drivers/media/video/ivtv/ivtvfb.c |   21 -
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/ivtv/ivtvfb.c b/drivers/media/video/ivtv/ivtvfb
index 9ff3425..2b3259c 100644
--- a/drivers/media/video/ivtv/ivtvfb.c
+++ b/drivers/media/video/ivtv/ivtvfb.c
@@ -1201,9 +1201,14 @@ static int ivtvfb_init_card(struct ivtv *itv)
 static int __init ivtvfb_callback_init(struct device *dev, void *p)
 {
struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
-   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
+   struct ivtv *itv;
 
-   if (itv  (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT)) {
+   if (v4l2_dev == NULL)
+   return 0;
+
+   itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
+
+   if (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT) {
if (ivtvfb_init_card(itv) == 0) {
IVTVFB_INFO(Framebuffer registered on %s\n,
itv-v4l2_dev.name);
@@ -1216,10 +1221,16 @@ static int __init ivtvfb_callback_init(struct device *de
 static int ivtvfb_callback_cleanup(struct device *dev, void *p)
 {
struct v4l2_device *v4l2_dev = dev_get_drvdata(dev);
-   struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
-   struct osd_info *oi = itv-osd_info;
+   struct ivtv *itv;
+   struct osd_info *oi;
+
+   if (v4l2_dev == NULL)
+   return 0;
+
+   itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
+   oi = itv-osd_info;
 
-   if (itv  (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT)) {
+   if (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT) {
if (unregister_framebuffer(itv-osd_info-ivtvfb_info)) {
IVTVFB_WARN(Framebuffer %d is in use, cannot unload\n,
   itv-instance);


--
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


[PATCH] VIDEO: ivtvfb, remove unneeded NULL test

2010-06-22 Thread Jiri Slaby
Stanse found that in ivtvfb_callback_cleanup there is an unneeded test
for itv being NULL. But itv is initialized as container_of with
non-zero offset, so it is never NULL (even if v4l2_dev is). This was
found because itv is dereferenced earlier than the test.

Signed-off-by: Jiri Slaby jsl...@suse.cz
Cc: Andy Walls awa...@md.metrocast.net
Cc: Mauro Carvalho Chehab mche...@infradead.org
Cc: Tejun Heo t...@kernel.org
Cc: Ian Armstrong i...@iarmst.demon.co.uk
Cc: ivtv-de...@ivtvdriver.org
Cc: linux-media@vger.kernel.org
---
 drivers/media/video/ivtv/ivtvfb.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/ivtv/ivtvfb.c 
b/drivers/media/video/ivtv/ivtvfb.c
index 9ff3425..5dc460e 100644
--- a/drivers/media/video/ivtv/ivtvfb.c
+++ b/drivers/media/video/ivtv/ivtvfb.c
@@ -1219,7 +1219,7 @@ static int ivtvfb_callback_cleanup(struct device *dev, 
void *p)
struct ivtv *itv = container_of(v4l2_dev, struct ivtv, v4l2_dev);
struct osd_info *oi = itv-osd_info;
 
-   if (itv  (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT)) {
+   if (itv-v4l2_cap  V4L2_CAP_VIDEO_OUTPUT) {
if (unregister_framebuffer(itv-osd_info-ivtvfb_info)) {
IVTVFB_WARN(Framebuffer %d is in use, cannot unload\n,
   itv-instance);
-- 
1.7.1


--
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