Re: [PATCH v1 1/2] media: rc: Add user count to rc dev.

[Srinivas KANDAGATLA]

On 19/07/13 12:01, Sean Young wrote:

>> +int rval = 0;
>>  
>> -return rdev->open(rdev);
>> +if (!rdev->users++)
>> +rval = rdev->open(rdev);
>> +
>> +if (rval)
>> +rdev->users--;
>> +
>> +return rval;
> 
> This looks racey. Some locking is needed, I think rc_dev->lock should work
> fine for this. Here and in the lirc code path too.
thanks Sean,
It makes sense, will fix this in v2.


Srini

> 
> Sean
> 
>>  }
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] media: rc: Add user count to rc dev.

[Sean Young]

On Fri, Jul 19, 2013 at 09:39:27AM +0100, Srinivas KANDAGATLA wrote:
> From: Srinivas Kandagatla 
> 
> This patch adds user count to rc_dev structure, the reason to add this
> new member is to allow other code like lirc to open rc device directly.
> In the existing code, rc device is only opened by input subsystem which
> works ok if we have any input drivers to match. But in case like lirc
> where there will be no input driver, rc device will be never opened.
> 
> Having this user count variable will be useful to allow rc device to be
> opened from code other than rc-main.
> 
> Signed-off-by: Srinivas Kandagatla 
> ---
>  drivers/media/rc/rc-main.c |   11 +--
>  include/media/rc-core.h|1 +
>  2 files changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 1cf382a..e800b96 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -702,15 +702,22 @@ EXPORT_SYMBOL_GPL(rc_keydown_notimeout);
>  static int ir_open(struct input_dev *idev)
>  {
>   struct rc_dev *rdev = input_get_drvdata(idev);
> + int rval = 0;
>  
> - return rdev->open(rdev);
> + if (!rdev->users++)
> + rval = rdev->open(rdev);
> +
> + if (rval)
> + rdev->users--;
> +
> + return rval;

This looks racey. Some locking is needed, I think rc_dev->lock should work
fine for this. Here and in the lirc code path too.

Sean

>  }
>  
>  static void ir_close(struct input_dev *idev)
>  {
>   struct rc_dev *rdev = input_get_drvdata(idev);
>  
> -  if (rdev)
> +  if (rdev && !--rdev->users)
>   rdev->close(rdev);
>  }
>  
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index 06a75de..b42016a 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -101,6 +101,7 @@ struct rc_dev {
>   boolidle;
>   u64 allowed_protos;
>   u64 enabled_protocols;
> + u32 users;
>   u32 scanmask;
>   void*priv;
>   spinlock_t  keylock;
> -- 
> 1.7.6.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v1 1/2] media: rc: Add user count to rc dev.

[Srinivas KANDAGATLA]

From: Srinivas Kandagatla 

This patch adds user count to rc_dev structure, the reason to add this
new member is to allow other code like lirc to open rc device directly.
In the existing code, rc device is only opened by input subsystem which
works ok if we have any input drivers to match. But in case like lirc
where there will be no input driver, rc device will be never opened.

Having this user count variable will be useful to allow rc device to be
opened from code other than rc-main.

Signed-off-by: Srinivas Kandagatla 
---
 drivers/media/rc/rc-main.c |   11 +--
 include/media/rc-core.h|1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 1cf382a..e800b96 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -702,15 +702,22 @@ EXPORT_SYMBOL_GPL(rc_keydown_notimeout);
 static int ir_open(struct input_dev *idev)
 {
struct rc_dev *rdev = input_get_drvdata(idev);
+   int rval = 0;
 
-   return rdev->open(rdev);
+   if (!rdev->users++)
+   rval = rdev->open(rdev);
+
+   if (rval)
+   rdev->users--;
+
+   return rval;
 }
 
 static void ir_close(struct input_dev *idev)
 {
struct rc_dev *rdev = input_get_drvdata(idev);
 
-if (rdev)
+if (rdev && !--rdev->users)
rdev->close(rdev);
 }
 
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 06a75de..b42016a 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -101,6 +101,7 @@ struct rc_dev {
boolidle;
u64 allowed_protos;
u64 enabled_protocols;
+   u32 users;
u32 scanmask;
void*priv;
spinlock_t  keylock;
-- 
1.7.6.5

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v1 1/2] media: rc: Add user count to rc dev.

[Srinivas KANDAGATLA]

From: Srinivas Kandagatla srinivas.kandaga...@st.com

This patch adds user count to rc_dev structure, the reason to add this
new member is to allow other code like lirc to open rc device directly.
In the existing code, rc device is only opened by input subsystem which
works ok if we have any input drivers to match. But in case like lirc
where there will be no input driver, rc device will be never opened.

Having this user count variable will be useful to allow rc device to be
opened from code other than rc-main.

Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
---
 drivers/media/rc/rc-main.c |   11 +--
 include/media/rc-core.h|1 +
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 1cf382a..e800b96 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -702,15 +702,22 @@ EXPORT_SYMBOL_GPL(rc_keydown_notimeout);
 static int ir_open(struct input_dev *idev)
 {
struct rc_dev *rdev = input_get_drvdata(idev);
+   int rval = 0;
 
-   return rdev-open(rdev);
+   if (!rdev-users++)
+   rval = rdev-open(rdev);
+
+   if (rval)
+   rdev-users--;
+
+   return rval;
 }
 
 static void ir_close(struct input_dev *idev)
 {
struct rc_dev *rdev = input_get_drvdata(idev);
 
-if (rdev)
+if (rdev  !--rdev-users)
rdev-close(rdev);
 }
 
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 06a75de..b42016a 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -101,6 +101,7 @@ struct rc_dev {
boolidle;
u64 allowed_protos;
u64 enabled_protocols;
+   u32 users;
u32 scanmask;
void*priv;
spinlock_t  keylock;
-- 
1.7.6.5

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] media: rc: Add user count to rc dev.

[Sean Young]

On Fri, Jul 19, 2013 at 09:39:27AM +0100, Srinivas KANDAGATLA wrote:
 From: Srinivas Kandagatla srinivas.kandaga...@st.com
 
 This patch adds user count to rc_dev structure, the reason to add this
 new member is to allow other code like lirc to open rc device directly.
 In the existing code, rc device is only opened by input subsystem which
 works ok if we have any input drivers to match. But in case like lirc
 where there will be no input driver, rc device will be never opened.
 
 Having this user count variable will be useful to allow rc device to be
 opened from code other than rc-main.
 
 Signed-off-by: Srinivas Kandagatla srinivas.kandaga...@st.com
 ---
  drivers/media/rc/rc-main.c |   11 +--
  include/media/rc-core.h|1 +
  2 files changed, 10 insertions(+), 2 deletions(-)
 
 diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
 index 1cf382a..e800b96 100644
 --- a/drivers/media/rc/rc-main.c
 +++ b/drivers/media/rc/rc-main.c
 @@ -702,15 +702,22 @@ EXPORT_SYMBOL_GPL(rc_keydown_notimeout);
  static int ir_open(struct input_dev *idev)
  {
   struct rc_dev *rdev = input_get_drvdata(idev);
 + int rval = 0;
  
 - return rdev-open(rdev);
 + if (!rdev-users++)
 + rval = rdev-open(rdev);
 +
 + if (rval)
 + rdev-users--;
 +
 + return rval;

This looks racey. Some locking is needed, I think rc_dev-lock should work
fine for this. Here and in the lirc code path too.

Sean

  }
  
  static void ir_close(struct input_dev *idev)
  {
   struct rc_dev *rdev = input_get_drvdata(idev);
  
 -  if (rdev)
 +  if (rdev  !--rdev-users)
   rdev-close(rdev);
  }
  
 diff --git a/include/media/rc-core.h b/include/media/rc-core.h
 index 06a75de..b42016a 100644
 --- a/include/media/rc-core.h
 +++ b/include/media/rc-core.h
 @@ -101,6 +101,7 @@ struct rc_dev {
   boolidle;
   u64 allowed_protos;
   u64 enabled_protocols;
 + u32 users;
   u32 scanmask;
   void*priv;
   spinlock_t  keylock;
 -- 
 1.7.6.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v1 1/2] media: rc: Add user count to rc dev.

[Srinivas KANDAGATLA]

On 19/07/13 12:01, Sean Young wrote:

 +int rval = 0;
  
 -return rdev-open(rdev);
 +if (!rdev-users++)
 +rval = rdev-open(rdev);
 +
 +if (rval)
 +rdev-users--;
 +
 +return rval;
 
 This looks racey. Some locking is needed, I think rc_dev-lock should work
 fine for this. Here and in the lirc code path too.
thanks Sean,
It makes sense, will fix this in v2.


Srini

 
 Sean
 
  }

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/